Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-08 Thread Dumitru Ceara 
On 12/8/23 14:51, Daniel Ding wrote:
> On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara  wrote:
> 
>> On 12/6/23 02:56, Daniel Ding wrote:
>>> Hi Dumitru!
>>>
>>> On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:
>>>
 On 12/5/23 13:58, Daniel Ding wrote:
>
>
> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > wrote:
>
> Hi Daniel,
>
> Thanks for this new revision but why is it v3?  I don't think I saw
 v2
> posted anywhere but maybe I missed it.
>
>
> Sorry, that is my mistake.
>

 No problem.

>
> On 12/5/23 08:33, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp
 address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp,
 so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> whenever possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> 
> >
> > Signed-off-by: Daniel Ding  >
> > Acked-by: Dumitru Ceara >>> dce...@redhat.com>>
>
> Please don't add an "Acked-by: ... " if the person never explicitly
> replied with "Acked-by: ... " on the previous version of the patch
>> or
> if you didn't have explicit agreement from that person to do so.
>
> Quoting from my previous reply to your v1, I said:
>
> "I think it makes sense to do what you're suggesting."
>
>

>> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
 <

>> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>
>
> That doesn't mean I acked the patch.
>
>
> Got it. Thx!
>

 No worries.

>
> > ---
> >  northd/northd.c | 18 +-
> >  tests/ovn-northd.at  | 12 
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> >  }
> >  }
> >
> > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >  const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> >  }
> >
> >  if (!strcmp(nat->type, "snat")) {
> > -continue;
> > +if (nat_entry_is_v6(nat_entry)) {
> > +if (sset_contains(_ips_v6,
 nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(_ips_v4,
 nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v4, nat->external_ip);
> > +}
>
> Essentially this just makes sure we don't skip NAT entries and that
 we
> consider unique external_ips.  I'm fine with relaxing the second
 part of
> the condition in which case, as mentioned on v1, I think we can
>> just
> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
>
> With the following incremental change applied (it removes the
>> block)
> your test still passes:
>
> diff --git a/northd/northd.c b/northd/northd.c
> index df7d2d60a5..20efd3b74c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
>  }
>  }
>
> -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>  const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-08 Thread Daniel Ding 
On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara  wrote:

> On 12/6/23 02:56, Daniel Ding wrote:
> > Hi Dumitru!
> >
> > On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:
> >
> >> On 12/5/23 13:58, Daniel Ding wrote:
> >>>
> >>>
> >>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  >>> > wrote:
> >>>
> >>> Hi Daniel,
> >>>
> >>> Thanks for this new revision but why is it v3?  I don't think I saw
> >> v2
> >>> posted anywhere but maybe I missed it.
> >>>
> >>>
> >>> Sorry, that is my mistake.
> >>>
> >>
> >> No problem.
> >>
> >>>
> >>> On 12/5/23 08:33, Daniel Ding wrote:
> >>> > If the router has a snat rule and it's external ip isn't lrp
> >> address,
> >>> > when the arp request from other router for this external ip, will
> >>> > be drop, because of this external ip use same mac address as lrp,
> >> so
> >>> > can not forward to MC_FLOOD.
> >>> >
> >>> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> >>> whenever possible.")
> >>> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> >>> 
> >>> >
> >>> > Signed-off-by: Daniel Ding  >>> >
> >>> > Acked-by: Dumitru Ceara  >> dce...@redhat.com>>
> >>>
> >>> Please don't add an "Acked-by: ... " if the person never explicitly
> >>> replied with "Acked-by: ... " on the previous version of the patch
> or
> >>> if you didn't have explicit agreement from that person to do so.
> >>>
> >>> Quoting from my previous reply to your v1, I said:
> >>>
> >>> "I think it makes sense to do what you're suggesting."
> >>>
> >>>
> >>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >> <
> >>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >>>
> >>>
> >>> That doesn't mean I acked the patch.
> >>>
> >>>
> >>> Got it. Thx!
> >>>
> >>
> >> No worries.
> >>
> >>>
> >>> > ---
> >>> >  northd/northd.c | 18 +-
> >>> >  tests/ovn-northd.at  | 12 
> >>> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >>> >
> >>> > diff --git a/northd/northd.c b/northd/northd.c
> >>> > index e9cb906e2..99fb30f46 100644
> >>> > --- a/northd/northd.c
> >>> > +++ b/northd/northd.c
> >>> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> >>> ovn_port *op,
> >>> >  }
> >>> >  }
> >>> >
> >>> > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> >>> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> >>> > +
> >>> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >>> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >>> >  const struct nbrec_nat *nat = nat_entry->nb;
> >>> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> >>> ovn_port *op,
> >>> >  }
> >>> >
> >>> >  if (!strcmp(nat->type, "snat")) {
> >>> > -continue;
> >>> > +if (nat_entry_is_v6(nat_entry)) {
> >>> > +if (sset_contains(_ips_v6,
> >> nat->external_ip)) {
> >>> > +continue;
> >>> > +}
> >>> > +sset_add(_ips_v6, nat->external_ip);
> >>> > +} else {
> >>> > +if (sset_contains(_ips_v4,
> >> nat->external_ip)) {
> >>> > +continue;
> >>> > +}
> >>> > +sset_add(_ips_v4, nat->external_ip);
> >>> > +}
> >>>
> >>> Essentially this just makes sure we don't skip NAT entries and that
> >> we
> >>> consider unique external_ips.  I'm fine with relaxing the second
> >> part of
> >>> the condition in which case, as mentioned on v1, I think we can
> just
> >>> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
> >>>
> >>> With the following incremental change applied (it removes the
> block)
> >>> your test still passes:
> >>>
> >>> diff --git a/northd/northd.c b/northd/northd.c
> >>> index df7d2d60a5..20efd3b74c 100644
> >>> --- a/northd/northd.c
> >>> +++ b/northd/northd.c
> >>> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
> >>> ovn_port *op,
> >>>  }
> >>>  }
> >>>
> >>> -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> >>> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> >>> -
> >>>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >>>  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >>>  const struct nbrec_nat *nat = nat_entry->nb;
> >>> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
> >>> ovn_port *op,
> >>>

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-07 Thread Dumitru Ceara 
On 12/6/23 02:56, Daniel Ding wrote:
> Hi Dumitru!
> 
> On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:
> 
>> On 12/5/23 13:58, Daniel Ding wrote:
>>>
>>>
>>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara >> > wrote:
>>>
>>> Hi Daniel,
>>>
>>> Thanks for this new revision but why is it v3?  I don't think I saw
>> v2
>>> posted anywhere but maybe I missed it.
>>>
>>>
>>> Sorry, that is my mistake.
>>>
>>
>> No problem.
>>
>>>
>>> On 12/5/23 08:33, Daniel Ding wrote:
>>> > If the router has a snat rule and it's external ip isn't lrp
>> address,
>>> > when the arp request from other router for this external ip, will
>>> > be drop, because of this external ip use same mac address as lrp,
>> so
>>> > can not forward to MC_FLOOD.
>>> >
>>> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
>>> whenever possible.")
>>> > Reported-at: https://github.com/ovn-org/ovn/issues/209
>>> 
>>> >
>>> > Signed-off-by: Daniel Ding >> >
>>> > Acked-by: Dumitru Ceara > dce...@redhat.com>>
>>>
>>> Please don't add an "Acked-by: ... " if the person never explicitly
>>> replied with "Acked-by: ... " on the previous version of the patch or
>>> if you didn't have explicit agreement from that person to do so.
>>>
>>> Quoting from my previous reply to your v1, I said:
>>>
>>> "I think it makes sense to do what you're suggesting."
>>>
>>>
>> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>> <
>> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>>>
>>>
>>> That doesn't mean I acked the patch.
>>>
>>>
>>> Got it. Thx!
>>>
>>
>> No worries.
>>
>>>
>>> > ---
>>> >  northd/northd.c | 18 +-
>>> >  tests/ovn-northd.at  | 12 
>>> >  2 files changed, 29 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/northd/northd.c b/northd/northd.c
>>> > index e9cb906e2..99fb30f46 100644
>>> > --- a/northd/northd.c
>>> > +++ b/northd/northd.c
>>> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
>>> ovn_port *op,
>>> >  }
>>> >  }
>>> >
>>> > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
>>> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
>>> > +
>>> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>>> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
>>> >  const struct nbrec_nat *nat = nat_entry->nb;
>>> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
>>> ovn_port *op,
>>> >  }
>>> >
>>> >  if (!strcmp(nat->type, "snat")) {
>>> > -continue;
>>> > +if (nat_entry_is_v6(nat_entry)) {
>>> > +if (sset_contains(_ips_v6,
>> nat->external_ip)) {
>>> > +continue;
>>> > +}
>>> > +sset_add(_ips_v6, nat->external_ip);
>>> > +} else {
>>> > +if (sset_contains(_ips_v4,
>> nat->external_ip)) {
>>> > +continue;
>>> > +}
>>> > +sset_add(_ips_v4, nat->external_ip);
>>> > +}
>>>
>>> Essentially this just makes sure we don't skip NAT entries and that
>> we
>>> consider unique external_ips.  I'm fine with relaxing the second
>> part of
>>> the condition in which case, as mentioned on v1, I think we can just
>>> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
>>>
>>> With the following incremental change applied (it removes the block)
>>> your test still passes:
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index df7d2d60a5..20efd3b74c 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
>>> ovn_port *op,
>>>  }
>>>  }
>>>
>>> -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
>>> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
>>> -
>>>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>>>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>>>  const struct nbrec_nat *nat = nat_entry->nb;
>>> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
>>> ovn_port *op,
>>>  continue;
>>>  }
>>>
>>> -if (!strcmp(nat->type, "snat")) {
>>> -if (nat_entry_is_v6(nat_entry)) {
>>> -if (sset_contains(_ips_v6, nat->external_ip)) {
>>> -continue;
>>> -}
>>> -

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Daniel Ding 
Hi Dumitru!

On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:

> On 12/5/23 13:58, Daniel Ding wrote:
> >
> >
> > On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > > wrote:
> >
> > Hi Daniel,
> >
> > Thanks for this new revision but why is it v3?  I don't think I saw
> v2
> > posted anywhere but maybe I missed it.
> >
> >
> > Sorry, that is my mistake.
> >
>
> No problem.
>
> >
> > On 12/5/23 08:33, Daniel Ding wrote:
> > > If the router has a snat rule and it's external ip isn't lrp
> address,
> > > when the arp request from other router for this external ip, will
> > > be drop, because of this external ip use same mac address as lrp,
> so
> > > can not forward to MC_FLOOD.
> > >
> > > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> > whenever possible.")
> > > Reported-at: https://github.com/ovn-org/ovn/issues/209
> > 
> > >
> > > Signed-off-by: Daniel Ding  > >
> > > Acked-by: Dumitru Ceara  dce...@redhat.com>>
> >
> > Please don't add an "Acked-by: ... " if the person never explicitly
> > replied with "Acked-by: ... " on the previous version of the patch or
> > if you didn't have explicit agreement from that person to do so.
> >
> > Quoting from my previous reply to your v1, I said:
> >
> > "I think it makes sense to do what you're suggesting."
> >
> >
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> <
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >
> >
> > That doesn't mean I acked the patch.
> >
> >
> > Got it. Thx!
> >
>
> No worries.
>
> >
> > > ---
> > >  northd/northd.c | 18 +-
> > >  tests/ovn-northd.at  | 12 
> > >  2 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index e9cb906e2..99fb30f46 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> > >  }
> > >  }
> > >
> > > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > > +
> > >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> > >  const struct nbrec_nat *nat = nat_entry->nb;
> > > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> > >  }
> > >
> > >  if (!strcmp(nat->type, "snat")) {
> > > -continue;
> > > +if (nat_entry_is_v6(nat_entry)) {
> > > +if (sset_contains(_ips_v6,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(_ips_v6, nat->external_ip);
> > > +} else {
> > > +if (sset_contains(_ips_v4,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(_ips_v4, nat->external_ip);
> > > +}
> >
> > Essentially this just makes sure we don't skip NAT entries and that
> we
> > consider unique external_ips.  I'm fine with relaxing the second
> part of
> > the condition in which case, as mentioned on v1, I think we can just
> > remove the whole "if (!strcmp(nat->type, "snat")) {" block.
> >
> > With the following incremental change applied (it removes the block)
> > your test still passes:
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index df7d2d60a5..20efd3b74c 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> >  }
> >  }
> >
> > -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > -
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >  const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> >  continue;
> >  }
> >
> > -if (!strcmp(nat->type, "snat")) {
> > -if (nat_entry_is_v6(nat_entry)) {
> > -if (sset_contains(_ips_v6, nat->external_ip)) {
> > -continue;
> > -}
> > -sset_add(_ips_v6, nat->external_ip);
> > -} else {
> >

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Dumitru Ceara 
On 12/5/23 13:58, Daniel Ding wrote:
> 
> 
> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > wrote:
> 
> Hi Daniel,
> 
> Thanks for this new revision but why is it v3?  I don't think I saw v2
> posted anywhere but maybe I missed it.
> 
>  
> Sorry, that is my mistake.
>  

No problem.

> 
> On 12/5/23 08:33, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp, so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> whenever possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> 
> >
> > Signed-off-by: Daniel Ding  >
> > Acked-by: Dumitru Ceara mailto:dce...@redhat.com>>
> 
> Please don't add an "Acked-by: ... " if the person never explicitly
> replied with "Acked-by: ... " on the previous version of the patch or
> if you didn't have explicit agreement from that person to do so.
> 
> Quoting from my previous reply to your v1, I said:
> 
> "I think it makes sense to do what you're suggesting."
> 
> 
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>  
> 
> 
> That doesn't mean I acked the patch.
> 
> 
> Got it. Thx!
>  

No worries.

> 
> > ---
> >  northd/northd.c     | 18 +-
> >  tests/ovn-northd.at  | 12 
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> >          }
> >      }
> > 
> > +    struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > +    struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >      for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >          struct ovn_nat *nat_entry = >od->nat_entries[i];
> >          const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> >          }
> > 
> >          if (!strcmp(nat->type, "snat")) {
> > -            continue;
> > +            if (nat_entry_is_v6(nat_entry)) {
> > +                if (sset_contains(_ips_v6, nat->external_ip)) {
> > +                    continue;
> > +                }
> > +                sset_add(_ips_v6, nat->external_ip);
> > +            } else {
> > +                if (sset_contains(_ips_v4, nat->external_ip)) {
> > +                    continue;
> > +                }
> > +                sset_add(_ips_v4, nat->external_ip);
> > +            }
> 
> Essentially this just makes sure we don't skip NAT entries and that we
> consider unique external_ips.  I'm fine with relaxing the second part of
> the condition in which case, as mentioned on v1, I think we can just
> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
> 
> With the following incremental change applied (it removes the block)
> your test still passes:
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index df7d2d60a5..20efd3b74c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
>          }
>      }
> 
> -    struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> -    struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>      for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>          struct ovn_nat *nat_entry = >od->nat_entries[i];
>          const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
>              continue;
>          }
> 
> -        if (!strcmp(nat->type, "snat")) {
> -            if (nat_entry_is_v6(nat_entry)) {
> -                if (sset_contains(_ips_v6, nat->external_ip)) {
> -                    continue;
> -                }
> -                sset_add(_ips_v6, nat->external_ip);
> -            } else {
> -                if (sset_contains(_ips_v4, nat->external_ip)) {
> -                    continue;
> -                }
> -                sset_add(_ips_v4, nat->external_ip);
> -            }
> -        }
> -
>          /* Check if the ovn port has a network

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Daniel Ding 
On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  wrote:

> Hi Daniel,
>
> Thanks for this new revision but why is it v3?  I don't think I saw v2
> posted anywhere but maybe I missed it.
>
>
Sorry, that is my mistake.


> On 12/5/23 08:33, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp, so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
> possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> >
> > Signed-off-by: Daniel Ding 
> > Acked-by: Dumitru Ceara 
>
> Please don't add an "Acked-by: ... " if the person never explicitly
> replied with "Acked-by: ... " on the previous version of the patch or
> if you didn't have explicit agreement from that person to do so.
>
> Quoting from my previous reply to your v1, I said:
>
> "I think it makes sense to do what you're suggesting."
>
>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>
> That doesn't mean I acked the patch.
>
>
Got it. Thx!


> > ---
> >  northd/northd.c | 18 +-
> >  tests/ovn-northd.at | 12 
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> >  }
> >  }
> >
> > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >  const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> >  }
> >
> >  if (!strcmp(nat->type, "snat")) {
> > -continue;
> > +if (nat_entry_is_v6(nat_entry)) {
> > +if (sset_contains(_ips_v6, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(_ips_v4, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v4, nat->external_ip);
> > +}
>
> Essentially this just makes sure we don't skip NAT entries and that we
> consider unique external_ips.  I'm fine with relaxing the second part of
> the condition in which case, as mentioned on v1, I think we can just
> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
>
> With the following incremental change applied (it removes the block)
> your test still passes:
>
> diff --git a/northd/northd.c b/northd/northd.c
> index df7d2d60a5..20efd3b74c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
>  }
>  }
>
> -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>  const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
>  continue;
>  }
>
> -if (!strcmp(nat->type, "snat")) {
> -if (nat_entry_is_v6(nat_entry)) {
> -if (sset_contains(_ips_v6, nat->external_ip)) {
> -continue;
> -}
> -sset_add(_ips_v6, nat->external_ip);
> -} else {
> -if (sset_contains(_ips_v4, nat->external_ip)) {
> -continue;
> -}
> -sset_add(_ips_v4, nat->external_ip);
> -}
> -}
> -
>  /* Check if the ovn port has a network configured on which we
> could
>   * expect ARP requests/NS for the DNAT external_ip.
>   */
> @@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
>  if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
>  build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
>  }
> -
> -sset_destroy(_ips_v4);
> -sset_destroy(_ips_v6);
>  }
>
>
If the nat_entries has multiple snats with the same external_ip, I think it
should check exernal_ip whether in a string hash. In addition, the
snat_and_dnat entry is working normally, so exclude it.

 static void
> ---
>
> Best regards,
> Dumitru
>
> >  }
> >
> >  /* Check if the ovn port has a network configured on which we
> could
> > @@ -9025,6 +9038,9 @@

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Dumitru Ceara 
Hi Daniel,

Thanks for this new revision but why is it v3?  I don't think I saw v2
posted anywhere but maybe I missed it.

On 12/5/23 08:33, Daniel Ding wrote:
> If the router has a snat rule and it's external ip isn't lrp address,
> when the arp request from other router for this external ip, will
> be drop, because of this external ip use same mac address as lrp, so
> can not forward to MC_FLOOD.
> 
> Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever 
> possible.")
> Reported-at: https://github.com/ovn-org/ovn/issues/209
> 
> Signed-off-by: Daniel Ding 
> Acked-by: Dumitru Ceara 

Please don't add an "Acked-by: ... " if the person never explicitly
replied with "Acked-by: ... " on the previous version of the patch or
if you didn't have explicit agreement from that person to do so.

Quoting from my previous reply to your v1, I said:

"I think it makes sense to do what you're suggesting."

https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934

That doesn't mean I acked the patch.

> ---
>  northd/northd.c | 18 +-
>  tests/ovn-northd.at | 12 
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index e9cb906e2..99fb30f46 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>  }
>  }
>  
> +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> +
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>  const struct nbrec_nat *nat = nat_entry->nb;
> @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>  }
>  
>  if (!strcmp(nat->type, "snat")) {
> -continue;
> +if (nat_entry_is_v6(nat_entry)) {
> +if (sset_contains(_ips_v6, nat->external_ip)) {
> +continue;
> +}
> +sset_add(_ips_v6, nat->external_ip);
> +} else {
> +if (sset_contains(_ips_v4, nat->external_ip)) {
> +continue;
> +}
> +sset_add(_ips_v4, nat->external_ip);
> +}

Essentially this just makes sure we don't skip NAT entries and that we
consider unique external_ips.  I'm fine with relaxing the second part of
the condition in which case, as mentioned on v1, I think we can just
remove the whole "if (!strcmp(nat->type, "snat")) {" block.

With the following incremental change applied (it removes the block)
your test still passes:

diff --git a/northd/northd.c b/northd/northd.c
index df7d2d60a5..20efd3b74c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 }
 }
 
-struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
-struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
-
 for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
 struct ovn_nat *nat_entry = >od->nat_entries[i];
 const struct nbrec_nat *nat = nat_entry->nb;
@@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 continue;
 }
 
-if (!strcmp(nat->type, "snat")) {
-if (nat_entry_is_v6(nat_entry)) {
-if (sset_contains(_ips_v6, nat->external_ip)) {
-continue;
-}
-sset_add(_ips_v6, nat->external_ip);
-} else {
-if (sset_contains(_ips_v4, nat->external_ip)) {
-continue;
-}
-sset_add(_ips_v4, nat->external_ip);
-}
-}
-
 /* Check if the ovn port has a network configured on which we could
  * expect ARP requests/NS for the DNAT external_ip.
  */
@@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
 build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
 }
-
-sset_destroy(_ips_v4);
-sset_destroy(_ips_v6);
 }
 
 static void
---

Best regards,
Dumitru

>  }
>  
>  /* Check if the ovn port has a network configured on which we could
> @@ -9025,6 +9038,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>  if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
>  build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
>  }
> +
> +sset_destroy(_ips_v4);
> +sset_destroy(_ips_v6);
>  }
>  
>  static void
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5c9da811f..953e0d829 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
> 's/table=../table=??/' | sort],
>