[ovs-dev,ovn] ovn-northd: Fix IP multicast flooding to mrouter.
diff mbox series

Message ID 1571222465-29290-1-git-send-email-dceara@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev,ovn] ovn-northd: Fix IP multicast flooding to mrouter.
Related show

Commit Message

Dumitru Ceara Oct. 16, 2019, 10:41 a.m. UTC
OVN logical flow "drop" actions can't be combined with other actions.
Commit 79308138891a created such a scenario if a logical switch has
mcast_snoop=true, mcast_flood_unregistered=false and is connected to a
logical router with mcast_relay=enabled.

To fix the issue we now explicitly add a drop flow for unregistered IP
multicast traffic in a logical switch if mcast_snoop=true,
mcast_flood_unregistered=false and the switch doesn't have any ports
with mcast_flood=true and isn't connected to a router with
mcast_relay=true.

Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 northd/ovn-northd.c |  8 +++++++-
 tests/ovn.at        | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 4 deletions(-)

Comments

Numan Siddique Oct. 16, 2019, 12:43 p.m. UTC | #1
On Wed, Oct 16, 2019 at 4:11 PM Dumitru Ceara <dceara@redhat.com> wrote:

> OVN logical flow "drop" actions can't be combined with other actions.
> Commit 79308138891a created such a scenario if a logical switch has
> mcast_snoop=true, mcast_flood_unregistered=false and is connected to a
> logical router with mcast_relay=enabled.
>
> To fix the issue we now explicitly add a drop flow for unregistered IP
> multicast traffic in a logical switch if mcast_snoop=true,
> mcast_flood_unregistered=false and the switch doesn't have any ports
> with mcast_flood=true and isn't connected to a router with
> mcast_relay=true.
>
> Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood
> configuration")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>

Hi Dumitru,

Can you please update the ovn-northd documentation too about the flow
changes ?

Thanks
Numan


> ---
>  northd/ovn-northd.c |  8 +++++++-
>  tests/ovn.at        | 50
> +++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index e41c9d7..d0844dd 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5661,7 +5661,13 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
>
>                  if (mcast_sw_info->flood_static) {
>                      ds_put_cstr(&actions, "outport =\""MC_STATIC"\";
> output;");
> -                } else {
> +                }
> +
> +                /* Explicitly drop the traffic if relay or static flooding
> +                 * is not configured.
> +                 */
> +                if (!mcast_sw_info->flood_relay &&
> +                        !mcast_sw_info->flood_static) {
>                      ds_put_cstr(&actions, "drop;");
>                  }
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index df00517..d141367 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -16306,7 +16306,7 @@ sleep 1
>  OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected])
>  OVN_CHECK_PACKETS([hv2/vif3-tx.pcap], [expected])
>
> -# Dissable IGMP querier on sw2.
> +# Disable IGMP querier on sw2.
>  ovn-nbctl set Logical_Switch sw2 \
>      other_config:mcast_querier="false"
>
> @@ -16357,6 +16357,50 @@ send_igmp_v3_report hv2-vif3 hv2 \
>      000000000001 $(ip_to_hex 10 0 0 1) f9f8 \
>      $(ip_to_hex 239 0 1 68) 04 e9b9 \
>      /dev/null
> +
> +# Check that the IGMP Group is learned by all switches.
> +OVS_WAIT_UNTIL([
> +    total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" | wc -l`
> +    test "${total_entries}" = "2"
> +])
> +
> +# Send traffic from sw3 and make sure it is relayed by rtr.
> +# to ports that joined.
> +truncate -s 0 expected_routed_sw1
> +truncate -s 0 expected_routed_sw2
> +truncate -s 0 expected_empty
> +
> +as hv1 reset_pcap_file hv1-vif1 hv1/vif1
> +as hv1 reset_pcap_file hv1-vif2 hv1/vif2
> +as hv1 reset_pcap_file hv1-vif3 hv1/vif3
> +as hv1 reset_pcap_file hv1-vif4 hv1/vif4
> +as hv2 reset_pcap_file hv2-vif1 hv2/vif1
> +as hv2 reset_pcap_file hv2-vif2 hv2/vif2
> +as hv2 reset_pcap_file hv2-vif3 hv2/vif3
> +as hv2 reset_pcap_file hv2-vif4 hv2/vif4
> +
> +send_ip_multicast_pkt hv2-vif4 hv2 \
> +    000000000001 01005e000144 \
> +    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
> +    e518e518000a3b3a0000
> +store_ip_multicast_pkt \
> +    000000000100 01005e000144 \
> +    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
> +    e518e518000a3b3a0000 expected_routed_sw1
> +store_ip_multicast_pkt \
> +    000000000200 01005e000144 \
> +    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
> +    e518e518000a3b3a0000 expected_routed_sw2
> +
> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected_routed_sw1])
> +OVN_CHECK_PACKETS([hv2/vif3-tx.pcap], [expected_routed_sw2])
> +OVN_CHECK_PACKETS([hv1/vif4-tx.pcap], [expected_empty])
> +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [expected_empty])
> +OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected_empty])
> +OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected_empty])
> +OVN_CHECK_PACKETS([hv2/vif2-tx.pcap], [expected_empty])
> +OVN_CHECK_PACKETS([hv2/vif4-tx.pcap], [expected_empty])
> +
>  # Inject IGMP Join for 239.0.1.68 on sw3-p1.
>  send_igmp_v3_report hv1-vif4 hv1 \
>      000000000001 $(ip_to_hex 10 0 0 1) f9f8 \
> @@ -16369,8 +16413,8 @@ OVS_WAIT_UNTIL([
>      test "${total_entries}" = "3"
>  ])
>
> -# Send traffic from sw3 and make sure it is relayed by rtr.
> -# and ports that joined.
> +# Send traffic from sw3 and make sure it is relayed by rtr
> +# to ports that joined.
>  truncate -s 0 expected_routed_sw1
>  truncate -s 0 expected_routed_sw2
>  truncate -s 0 expected_switched
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Dumitru Ceara Oct. 16, 2019, 1:08 p.m. UTC | #2
On Wed, Oct 16, 2019 at 2:43 PM Numan Siddique <numans@ovn.org> wrote:
>
>
>
> On Wed, Oct 16, 2019 at 4:11 PM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> OVN logical flow "drop" actions can't be combined with other actions.
>> Commit 79308138891a created such a scenario if a logical switch has
>> mcast_snoop=true, mcast_flood_unregistered=false and is connected to a
>> logical router with mcast_relay=enabled.
>>
>> To fix the issue we now explicitly add a drop flow for unregistered IP
>> multicast traffic in a logical switch if mcast_snoop=true,
>> mcast_flood_unregistered=false and the switch doesn't have any ports
>> with mcast_flood=true and isn't connected to a router with
>> mcast_relay=true.
>>
>> Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration")
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>
>
> Hi Dumitru,
>
> Can you please update the ovn-northd documentation too about the flow changes ?
>
> Thanks
> Numan

Hi Numan,

Thanks for the review.
I sent a v2: https://patchwork.ozlabs.org/patch/1177904/

Thanks,
Dumitru

>
>>
>> ---
>>  northd/ovn-northd.c |  8 +++++++-
>>  tests/ovn.at        | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index e41c9d7..d0844dd 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -5661,7 +5661,13 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>>
>>                  if (mcast_sw_info->flood_static) {
>>                      ds_put_cstr(&actions, "outport =\""MC_STATIC"\"; output;");
>> -                } else {
>> +                }
>> +
>> +                /* Explicitly drop the traffic if relay or static flooding
>> +                 * is not configured.
>> +                 */
>> +                if (!mcast_sw_info->flood_relay &&
>> +                        !mcast_sw_info->flood_static) {
>>                      ds_put_cstr(&actions, "drop;");
>>                  }
>>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index df00517..d141367 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -16306,7 +16306,7 @@ sleep 1
>>  OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected])
>>  OVN_CHECK_PACKETS([hv2/vif3-tx.pcap], [expected])
>>
>> -# Dissable IGMP querier on sw2.
>> +# Disable IGMP querier on sw2.
>>  ovn-nbctl set Logical_Switch sw2 \
>>      other_config:mcast_querier="false"
>>
>> @@ -16357,6 +16357,50 @@ send_igmp_v3_report hv2-vif3 hv2 \
>>      000000000001 $(ip_to_hex 10 0 0 1) f9f8 \
>>      $(ip_to_hex 239 0 1 68) 04 e9b9 \
>>      /dev/null
>> +
>> +# Check that the IGMP Group is learned by all switches.
>> +OVS_WAIT_UNTIL([
>> +    total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" | wc -l`
>> +    test "${total_entries}" = "2"
>> +])
>> +
>> +# Send traffic from sw3 and make sure it is relayed by rtr.
>> +# to ports that joined.
>> +truncate -s 0 expected_routed_sw1
>> +truncate -s 0 expected_routed_sw2
>> +truncate -s 0 expected_empty
>> +
>> +as hv1 reset_pcap_file hv1-vif1 hv1/vif1
>> +as hv1 reset_pcap_file hv1-vif2 hv1/vif2
>> +as hv1 reset_pcap_file hv1-vif3 hv1/vif3
>> +as hv1 reset_pcap_file hv1-vif4 hv1/vif4
>> +as hv2 reset_pcap_file hv2-vif1 hv2/vif1
>> +as hv2 reset_pcap_file hv2-vif2 hv2/vif2
>> +as hv2 reset_pcap_file hv2-vif3 hv2/vif3
>> +as hv2 reset_pcap_file hv2-vif4 hv2/vif4
>> +
>> +send_ip_multicast_pkt hv2-vif4 hv2 \
>> +    000000000001 01005e000144 \
>> +    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
>> +    e518e518000a3b3a0000
>> +store_ip_multicast_pkt \
>> +    000000000100 01005e000144 \
>> +    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>> +    e518e518000a3b3a0000 expected_routed_sw1
>> +store_ip_multicast_pkt \
>> +    000000000200 01005e000144 \
>> +    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>> +    e518e518000a3b3a0000 expected_routed_sw2
>> +
>> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected_routed_sw1])
>> +OVN_CHECK_PACKETS([hv2/vif3-tx.pcap], [expected_routed_sw2])
>> +OVN_CHECK_PACKETS([hv1/vif4-tx.pcap], [expected_empty])
>> +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [expected_empty])
>> +OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected_empty])
>> +OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected_empty])
>> +OVN_CHECK_PACKETS([hv2/vif2-tx.pcap], [expected_empty])
>> +OVN_CHECK_PACKETS([hv2/vif4-tx.pcap], [expected_empty])
>> +
>>  # Inject IGMP Join for 239.0.1.68 on sw3-p1.
>>  send_igmp_v3_report hv1-vif4 hv1 \
>>      000000000001 $(ip_to_hex 10 0 0 1) f9f8 \
>> @@ -16369,8 +16413,8 @@ OVS_WAIT_UNTIL([
>>      test "${total_entries}" = "3"
>>  ])
>>
>> -# Send traffic from sw3 and make sure it is relayed by rtr.
>> -# and ports that joined.
>> +# Send traffic from sw3 and make sure it is relayed by rtr
>> +# to ports that joined.
>>  truncate -s 0 expected_routed_sw1
>>  truncate -s 0 expected_routed_sw2
>>  truncate -s 0 expected_switched
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index e41c9d7..d0844dd 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5661,7 +5661,13 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
 
                 if (mcast_sw_info->flood_static) {
                     ds_put_cstr(&actions, "outport =\""MC_STATIC"\"; output;");
-                } else {
+                }
+
+                /* Explicitly drop the traffic if relay or static flooding
+                 * is not configured.
+                 */
+                if (!mcast_sw_info->flood_relay &&
+                        !mcast_sw_info->flood_static) {
                     ds_put_cstr(&actions, "drop;");
                 }
 
diff --git a/tests/ovn.at b/tests/ovn.at
index df00517..d141367 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -16306,7 +16306,7 @@  sleep 1
 OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected])
 OVN_CHECK_PACKETS([hv2/vif3-tx.pcap], [expected])
 
-# Dissable IGMP querier on sw2.
+# Disable IGMP querier on sw2.
 ovn-nbctl set Logical_Switch sw2 \
     other_config:mcast_querier="false"
 
@@ -16357,6 +16357,50 @@  send_igmp_v3_report hv2-vif3 hv2 \
     000000000001 $(ip_to_hex 10 0 0 1) f9f8 \
     $(ip_to_hex 239 0 1 68) 04 e9b9 \
     /dev/null
+
+# Check that the IGMP Group is learned by all switches.
+OVS_WAIT_UNTIL([
+    total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" | wc -l`
+    test "${total_entries}" = "2"
+])
+
+# Send traffic from sw3 and make sure it is relayed by rtr.
+# to ports that joined.
+truncate -s 0 expected_routed_sw1
+truncate -s 0 expected_routed_sw2
+truncate -s 0 expected_empty
+
+as hv1 reset_pcap_file hv1-vif1 hv1/vif1
+as hv1 reset_pcap_file hv1-vif2 hv1/vif2
+as hv1 reset_pcap_file hv1-vif3 hv1/vif3
+as hv1 reset_pcap_file hv1-vif4 hv1/vif4
+as hv2 reset_pcap_file hv2-vif1 hv2/vif1
+as hv2 reset_pcap_file hv2-vif2 hv2/vif2
+as hv2 reset_pcap_file hv2-vif3 hv2/vif3
+as hv2 reset_pcap_file hv2-vif4 hv2/vif4
+
+send_ip_multicast_pkt hv2-vif4 hv2 \
+    000000000001 01005e000144 \
+    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
+    e518e518000a3b3a0000
+store_ip_multicast_pkt \
+    000000000100 01005e000144 \
+    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
+    e518e518000a3b3a0000 expected_routed_sw1
+store_ip_multicast_pkt \
+    000000000200 01005e000144 \
+    $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
+    e518e518000a3b3a0000 expected_routed_sw2
+
+OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected_routed_sw1])
+OVN_CHECK_PACKETS([hv2/vif3-tx.pcap], [expected_routed_sw2])
+OVN_CHECK_PACKETS([hv1/vif4-tx.pcap], [expected_empty])
+OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [expected_empty])
+OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected_empty])
+OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected_empty])
+OVN_CHECK_PACKETS([hv2/vif2-tx.pcap], [expected_empty])
+OVN_CHECK_PACKETS([hv2/vif4-tx.pcap], [expected_empty])
+
 # Inject IGMP Join for 239.0.1.68 on sw3-p1.
 send_igmp_v3_report hv1-vif4 hv1 \
     000000000001 $(ip_to_hex 10 0 0 1) f9f8 \
@@ -16369,8 +16413,8 @@  OVS_WAIT_UNTIL([
     test "${total_entries}" = "3"
 ])
 
-# Send traffic from sw3 and make sure it is relayed by rtr.
-# and ports that joined.
+# Send traffic from sw3 and make sure it is relayed by rtr
+# to ports that joined.
 truncate -s 0 expected_routed_sw1
 truncate -s 0 expected_routed_sw2
 truncate -s 0 expected_switched