Re: [ovs-dev] [PATCH] [RFC] [ovn] northd: Fix IGMP external subscriber subscribing to internal feed
-Original Message-
From: Dumitru Ceara
Sent: Tuesday, May 24, 2022 10:42 AM
To: Alin Serdean
Cc: Alin-Gabriel Serdean ; d...@openvswitch.org; Diko
Parvanov
Subject: Re: [PATCH] [RFC] [ovn] northd: Fix IGMP external subscriber
subscribing to internal feed
On 5/24/22 01:13, Alin Serdean wrote:
>
>
>> On 24 May 2022, at 00:15, Dumitru Ceara wrote:
>>
>> On 5/3/22 17:15, Alin-Gabriel Serdean wrote:
>>> Add localnet ports to their corresponding southbound multicast group.
>>>
>>> Update unit tests.
>>>
>>> Reported-at: https://github.com/ovn-org/ovn/issues/125
>>> Reported-by: Diko Parvanov
>>> Suggested-by: Dumitru Ceara
>>> Signed-off-by: Alin-Gabriel Serdean
>>> ---
>>
>> Hi Alin,
>>
>> Thanks for the patch!
>
> Hi Dumitru,
>
> Thanks for the review!
>>
>>> northd/northd.c | 17 ++---
>>> tests/ovn.at| 15 ++-
>>> 2 files changed, 12 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c index
>>> a5297..4441ef631 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -4591,13 +4591,6 @@ ovn_igmp_group_aggregate_ports(struct ovn_igmp_group
>>> *igmp_group,
>>> ovn_igmp_group_destroy_entry(entry);
>>> free(entry);
>>> }
>>> -
>>> -if (igmp_group->datapath->n_localnet_ports) {
>>> -ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>>> -&igmp_group->mcgroup,
>>> -igmp_group->datapath->localnet_ports,
>>> -igmp_group->datapath->n_localnet_ports);
>>> -}
>>> }
>>>
>>> static void
>>> @@ -15066,6 +15059,16 @@ build_mcast_groups(struct lflow_input
>>> *input_data,
>>>
>>> /* Add the extracted ports to the IGMP group. */
>>> ovn_igmp_group_add_entry(igmp_group, igmp_ports,
>>> n_igmp_ports);
>>> +if (!ovn_igmp_group_allocate_id(igmp_group)) {
>>> +ovn_igmp_group_destroy(igmp_groups, igmp_group);
>>> +continue;
>>> +}
>>> +if (igmp_group->datapath->n_localnet_ports) {
>>> +ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>>> +&igmp_group->mcgroup,
>>> +igmp_group->datapath->localnet_ports,
>>> +
>>> igmp_group->datapath->n_localnet_ports);
>>> +}
>>
>> I'm not sure I understand how this changes things. After looking at
>> the main branch code more closely, we already include all localnet
>> ports in the multicast_group generated for an IGMP group. We just do
>> it at the end of build_mcast_groups(), in
>> ovn_igmp_group_aggregate_ports() which is called once for every IGMP Group
>> learnt in a logical datapath.
>
> Indeed I got confused between branches and I didn’t saw the
> ovn_igmp_group_aggregate_ports.
>
>>> }
>>>
>>> /* Build IGMP groups for multicast routers with relay enabled.
>>> The router diff --git a/tests/ovn.at b/tests/ovn.at index
>>> 34b6abfc0..5440639a8 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -19570,7 +19570,8 @@ ovn-nbctl lsp-add sw3 sw3-p1 ovn-nbctl
>>> lsp-add sw3 sw3-p2
>>> ovn-nbctl lsp-add sw3 sw3-ln\
>>> -- lsp-set-type sw3-ln localnet \
>>> --- lsp-set-options sw3-ln network_name=phys
>>> +-- lsp-set-options sw3-ln network_name=phys \
>>> +-- lsp-set-options sw3-ln mcast_flood=true
>>
>> This threw me off track for a bit.. "lsp-set-options "
>> will actually overwrite all options of with . Which
>> most likely is not what we want here because we would be removing the
>> network_name, essentially disconnecting the localnet port.
>>
>> If I understand the intention correctly, you should probably use:
>>
>> -- lsp-set-options sw3-ln network_name=phys mcast_flood=true
>
> You are correct. This actually made me think I was changing things.
>
>>
>>>
>>> ovn-nbctl lr-add rtr
>>> ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24 @@
>>> -19985,18 +19986,6 @@ store_ip_multicast_pkt \
>>> $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
>>> e518e518000a3b3a expected_switched
>>>
>>> -# TODO: IGMP Relay duplicates IP multicast packets in some
>>> conditions, for -# more details see TODO.rst, section "IP Multicast
>>> Relay". Once that issue is -# fixed the duplicated packets should not
>>> appear anymore.
>>> -store_ip_multicast_pkt \
>>> -0100 01005e000144 \
>>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>>> -e518e518000a3b3a expected_routed_sw1
>>> -store_ip_multicast_pkt \
>>> -0200 01005e000144 \
>>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>>> -e518e518000a3b3a expected_routed_sw2
>>> -
>>
>> After changing the test to properly set mcast_flood=true on the
>> localnet port, it starts failing due to the duplicated routed packets
>> (th
Re: [ovs-dev] [PATCH] [RFC] [ovn] northd: Fix IGMP external subscriber subscribing to internal feed
On 5/24/22 01:13, Alin Serdean wrote:
>
>
>> On 24 May 2022, at 00:15, Dumitru Ceara wrote:
>>
>> On 5/3/22 17:15, Alin-Gabriel Serdean wrote:
>>> Add localnet ports to their corresponding southbound multicast group.
>>>
>>> Update unit tests.
>>>
>>> Reported-at: https://github.com/ovn-org/ovn/issues/125
>>> Reported-by: Diko Parvanov
>>> Suggested-by: Dumitru Ceara
>>> Signed-off-by: Alin-Gabriel Serdean
>>> ---
>>
>> Hi Alin,
>>
>> Thanks for the patch!
>
> Hi Dumitru,
>
> Thanks for the review!
>>
>>> northd/northd.c | 17 ++---
>>> tests/ovn.at| 15 ++-
>>> 2 files changed, 12 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index a5297..4441ef631 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -4591,13 +4591,6 @@ ovn_igmp_group_aggregate_ports(struct ovn_igmp_group
>>> *igmp_group,
>>> ovn_igmp_group_destroy_entry(entry);
>>> free(entry);
>>> }
>>> -
>>> -if (igmp_group->datapath->n_localnet_ports) {
>>> -ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>>> -&igmp_group->mcgroup,
>>> -igmp_group->datapath->localnet_ports,
>>> -igmp_group->datapath->n_localnet_ports);
>>> -}
>>> }
>>>
>>> static void
>>> @@ -15066,6 +15059,16 @@ build_mcast_groups(struct lflow_input *input_data,
>>>
>>> /* Add the extracted ports to the IGMP group. */
>>> ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports);
>>> +if (!ovn_igmp_group_allocate_id(igmp_group)) {
>>> +ovn_igmp_group_destroy(igmp_groups, igmp_group);
>>> +continue;
>>> +}
>>> +if (igmp_group->datapath->n_localnet_ports) {
>>> +ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>>> +&igmp_group->mcgroup,
>>> +igmp_group->datapath->localnet_ports,
>>> +
>>> igmp_group->datapath->n_localnet_ports);
>>> +}
>>
>> I'm not sure I understand how this changes things. After looking at the
>> main branch code more closely, we already include all localnet ports in
>> the multicast_group generated for an IGMP group. We just do it at the
>> end of build_mcast_groups(), in ovn_igmp_group_aggregate_ports() which
>> is called once for every IGMP Group learnt in a logical datapath.
>
> Indeed I got confused between branches and I didn’t saw the
> ovn_igmp_group_aggregate_ports.
>
>>> }
>>>
>>> /* Build IGMP groups for multicast routers with relay enabled. The
>>> router
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index 34b6abfc0..5440639a8 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -19570,7 +19570,8 @@ ovn-nbctl lsp-add sw3 sw3-p1
>>> ovn-nbctl lsp-add sw3 sw3-p2
>>> ovn-nbctl lsp-add sw3 sw3-ln\
>>> -- lsp-set-type sw3-ln localnet \
>>> --- lsp-set-options sw3-ln network_name=phys
>>> +-- lsp-set-options sw3-ln network_name=phys \
>>> +-- lsp-set-options sw3-ln mcast_flood=true
>>
>> This threw me off track for a bit.. "lsp-set-options "
>> will actually overwrite all options of with . Which
>> most likely is not what we want here because we would be removing the
>> network_name, essentially disconnecting the localnet port.
>>
>> If I understand the intention correctly, you should probably use:
>>
>> -- lsp-set-options sw3-ln network_name=phys mcast_flood=true
>
> You are correct. This actually made me think I was changing things.
>
>>
>>>
>>> ovn-nbctl lr-add rtr
>>> ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24
>>> @@ -19985,18 +19986,6 @@ store_ip_multicast_pkt \
>>> $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
>>> e518e518000a3b3a expected_switched
>>>
>>> -# TODO: IGMP Relay duplicates IP multicast packets in some conditions, for
>>> -# more details see TODO.rst, section "IP Multicast Relay". Once that issue
>>> is
>>> -# fixed the duplicated packets should not appear anymore.
>>> -store_ip_multicast_pkt \
>>> -0100 01005e000144 \
>>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>>> -e518e518000a3b3a expected_routed_sw1
>>> -store_ip_multicast_pkt \
>>> -0200 01005e000144 \
>>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>>> -e518e518000a3b3a expected_routed_sw2
>>> -
>>
>> After changing the test to properly set mcast_flood=true on the localnet
>> port, it starts failing due to the duplicated routed packets (the TODO
>> above). There was no logical change in northd.c as far as I can tell,
>> so that makes sense.
>>
>>> OVS_WAIT_UNTIL(
>>> [check_packets 'hv1/vif1-tx.pcap expected_routed_sw1' \
>>> 'hv2/vif3-tx.pcap expected_routed_sw2' \
>>
>> Then I tried to replicate the issue Diko
Re: [ovs-dev] [PATCH] [RFC] [ovn] northd: Fix IGMP external subscriber subscribing to internal feed
> On 24 May 2022, at 00:15, Dumitru Ceara wrote:
>
> On 5/3/22 17:15, Alin-Gabriel Serdean wrote:
>> Add localnet ports to their corresponding southbound multicast group.
>>
>> Update unit tests.
>>
>> Reported-at: https://github.com/ovn-org/ovn/issues/125
>> Reported-by: Diko Parvanov
>> Suggested-by: Dumitru Ceara
>> Signed-off-by: Alin-Gabriel Serdean
>> ---
>
> Hi Alin,
>
> Thanks for the patch!
Hi Dumitru,
Thanks for the review!
>
>> northd/northd.c | 17 ++---
>> tests/ovn.at| 15 ++-
>> 2 files changed, 12 insertions(+), 20 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index a5297..4441ef631 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -4591,13 +4591,6 @@ ovn_igmp_group_aggregate_ports(struct ovn_igmp_group
>> *igmp_group,
>> ovn_igmp_group_destroy_entry(entry);
>> free(entry);
>> }
>> -
>> -if (igmp_group->datapath->n_localnet_ports) {
>> -ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>> -&igmp_group->mcgroup,
>> -igmp_group->datapath->localnet_ports,
>> -igmp_group->datapath->n_localnet_ports);
>> -}
>> }
>>
>> static void
>> @@ -15066,6 +15059,16 @@ build_mcast_groups(struct lflow_input *input_data,
>>
>> /* Add the extracted ports to the IGMP group. */
>> ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports);
>> +if (!ovn_igmp_group_allocate_id(igmp_group)) {
>> +ovn_igmp_group_destroy(igmp_groups, igmp_group);
>> +continue;
>> +}
>> +if (igmp_group->datapath->n_localnet_ports) {
>> +ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>> +&igmp_group->mcgroup,
>> +igmp_group->datapath->localnet_ports,
>> +igmp_group->datapath->n_localnet_ports);
>> +}
>
> I'm not sure I understand how this changes things. After looking at the
> main branch code more closely, we already include all localnet ports in
> the multicast_group generated for an IGMP group. We just do it at the
> end of build_mcast_groups(), in ovn_igmp_group_aggregate_ports() which
> is called once for every IGMP Group learnt in a logical datapath.
Indeed I got confused between branches and I didn’t saw the
ovn_igmp_group_aggregate_ports.
>> }
>>
>> /* Build IGMP groups for multicast routers with relay enabled. The router
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 34b6abfc0..5440639a8 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -19570,7 +19570,8 @@ ovn-nbctl lsp-add sw3 sw3-p1
>> ovn-nbctl lsp-add sw3 sw3-p2
>> ovn-nbctl lsp-add sw3 sw3-ln\
>> -- lsp-set-type sw3-ln localnet \
>> --- lsp-set-options sw3-ln network_name=phys
>> +-- lsp-set-options sw3-ln network_name=phys \
>> +-- lsp-set-options sw3-ln mcast_flood=true
>
> This threw me off track for a bit.. "lsp-set-options "
> will actually overwrite all options of with . Which
> most likely is not what we want here because we would be removing the
> network_name, essentially disconnecting the localnet port.
>
> If I understand the intention correctly, you should probably use:
>
> -- lsp-set-options sw3-ln network_name=phys mcast_flood=true
You are correct. This actually made me think I was changing things.
>
>>
>> ovn-nbctl lr-add rtr
>> ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24
>> @@ -19985,18 +19986,6 @@ store_ip_multicast_pkt \
>> $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
>> e518e518000a3b3a expected_switched
>>
>> -# TODO: IGMP Relay duplicates IP multicast packets in some conditions, for
>> -# more details see TODO.rst, section "IP Multicast Relay". Once that issue
>> is
>> -# fixed the duplicated packets should not appear anymore.
>> -store_ip_multicast_pkt \
>> -0100 01005e000144 \
>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>> -e518e518000a3b3a expected_routed_sw1
>> -store_ip_multicast_pkt \
>> -0200 01005e000144 \
>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>> -e518e518000a3b3a expected_routed_sw2
>> -
>
> After changing the test to properly set mcast_flood=true on the localnet
> port, it starts failing due to the duplicated routed packets (the TODO
> above). There was no logical change in northd.c as far as I can tell,
> so that makes sense.
>
>> OVS_WAIT_UNTIL(
>> [check_packets 'hv1/vif1-tx.pcap expected_routed_sw1' \
>> 'hv2/vif3-tx.pcap expected_routed_sw2' \
>
> Then I tried to replicate the issue Diko reported but I couldn't. The
> databases attached to the GitHub issue are truncated, I get:
>
> ovsdb-tool: syntax error: ovnnb_db.db: 731669 bytes starting at offset
Re: [ovs-dev] [PATCH] [RFC] [ovn] northd: Fix IGMP external subscriber subscribing to internal feed
On 5/3/22 17:15, Alin-Gabriel Serdean wrote:
> Add localnet ports to their corresponding southbound multicast group.
>
> Update unit tests.
>
> Reported-at: https://github.com/ovn-org/ovn/issues/125
> Reported-by: Diko Parvanov
> Suggested-by: Dumitru Ceara
> Signed-off-by: Alin-Gabriel Serdean
> ---
Hi Alin,
Thanks for the patch!
> northd/northd.c | 17 ++---
> tests/ovn.at| 15 ++-
> 2 files changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a5297..4441ef631 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4591,13 +4591,6 @@ ovn_igmp_group_aggregate_ports(struct ovn_igmp_group
> *igmp_group,
> ovn_igmp_group_destroy_entry(entry);
> free(entry);
> }
> -
> -if (igmp_group->datapath->n_localnet_ports) {
> -ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
> -&igmp_group->mcgroup,
> -igmp_group->datapath->localnet_ports,
> -igmp_group->datapath->n_localnet_ports);
> -}
> }
>
> static void
> @@ -15066,6 +15059,16 @@ build_mcast_groups(struct lflow_input *input_data,
>
> /* Add the extracted ports to the IGMP group. */
> ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports);
> +if (!ovn_igmp_group_allocate_id(igmp_group)) {
> +ovn_igmp_group_destroy(igmp_groups, igmp_group);
> +continue;
> +}
> +if (igmp_group->datapath->n_localnet_ports) {
> +ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
> +&igmp_group->mcgroup,
> +igmp_group->datapath->localnet_ports,
> +igmp_group->datapath->n_localnet_ports);
> +}
I'm not sure I understand how this changes things. After looking at the
main branch code more closely, we already include all localnet ports in
the multicast_group generated for an IGMP group. We just do it at the
end of build_mcast_groups(), in ovn_igmp_group_aggregate_ports() which
is called once for every IGMP Group learnt in a logical datapath.
> }
>
> /* Build IGMP groups for multicast routers with relay enabled. The router
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 34b6abfc0..5440639a8 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -19570,7 +19570,8 @@ ovn-nbctl lsp-add sw3 sw3-p1
> ovn-nbctl lsp-add sw3 sw3-p2
> ovn-nbctl lsp-add sw3 sw3-ln\
> -- lsp-set-type sw3-ln localnet \
> --- lsp-set-options sw3-ln network_name=phys
> +-- lsp-set-options sw3-ln network_name=phys \
> +-- lsp-set-options sw3-ln mcast_flood=true
This threw me off track for a bit.. "lsp-set-options "
will actually overwrite all options of with . Which
most likely is not what we want here because we would be removing the
network_name, essentially disconnecting the localnet port.
If I understand the intention correctly, you should probably use:
-- lsp-set-options sw3-ln network_name=phys mcast_flood=true
>
> ovn-nbctl lr-add rtr
> ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24
> @@ -19985,18 +19986,6 @@ store_ip_multicast_pkt \
> $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
> e518e518000a3b3a expected_switched
>
> -# TODO: IGMP Relay duplicates IP multicast packets in some conditions, for
> -# more details see TODO.rst, section "IP Multicast Relay". Once that issue is
> -# fixed the duplicated packets should not appear anymore.
> -store_ip_multicast_pkt \
> -0100 01005e000144 \
> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
> -e518e518000a3b3a expected_routed_sw1
> -store_ip_multicast_pkt \
> -0200 01005e000144 \
> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
> -e518e518000a3b3a expected_routed_sw2
> -
After changing the test to properly set mcast_flood=true on the localnet
port, it starts failing due to the duplicated routed packets (the TODO
above). There was no logical change in northd.c as far as I can tell,
so that makes sense.
> OVS_WAIT_UNTIL(
>[check_packets 'hv1/vif1-tx.pcap expected_routed_sw1' \
> 'hv2/vif3-tx.pcap expected_routed_sw2' \
Then I tried to replicate the issue Diko reported but I couldn't. The
databases attached to the GitHub issue are truncated, I get:
ovsdb-tool: syntax error: ovnnb_db.db: 731669 bytes starting at offset
62 have SHA-1 hash 161c2831d154dce19aeb316b5d95de5db66988ac but should
have hash 0d9bc5cb94dcce9f58bb99c5588b249b551c27cd
And I tried setting up a unit test to do exactly what is reported in the
GitHub issue but regardless of mcast_flood value I always get multicast
traffic generated from an "internal" source properly forwarded to the
"external" subscribers that I created and at
