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

2023-12-08 Thread Daniel Ding 
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 
---
 northd/northd.c | 31 +++
 tests/ovn-northd.at | 12 
 2 files changed, 43 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index e9cb906e2..2e6764d7b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9004,6 +9004,37 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 }
 }
 
+struct shash_node *snat_snode;
+SHASH_FOR_EACH (snat_snode, >od->snat_ips) {
+struct ovn_snat_ip *snat_ip = snat_snode->data;
+
+if (ovs_list_is_empty(_ip->snat_entries)) {
+continue;
+}
+
+struct ovn_nat *nat_entry =
+CONTAINER_OF(ovs_list_front(_ip->snat_entries),
+ struct ovn_nat, ext_addr_list_node);
+const struct nbrec_nat *nat = nat_entry->nb;
+
+/* Check if the ovn port has a network configured on which we could
+ * expect ARP requests/NS for the DNAT external_ip.
+ */
+if (nat_entry_is_v6(nat_entry)) {
+if (!sset_contains(>od->lb_ips->ips_v6, nat->external_ip)) {
+build_lswitch_rport_arp_req_flow(
+nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows,
+stage_hint);
+}
+} else {
+if (!sset_contains(>od->lb_ips->ips_v4, nat->external_ip)) {
+build_lswitch_rport_arp_req_flow(
+nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows,
+stage_hint);
+}
+}
+}
+
 for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
 build_lswitch_rport_arp_req_flow(
 op->lrp_networks.ipv4_addrs[i].addr_s, AF_INET, sw_op, sw_od, 80,
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],
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
   table=??(ls_in_l2_lkup  ), priority=75   , match=(eth.src == 
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport 
= "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport = "ls1-ro1"; 
output; }; outport = "_MC_flood_l2"; output;)
+  table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 10.0.0.200), action=(clone {outport = "ls1-ro1"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 192.168.1.1), action=(clone {outport = "ls1-ro1"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport = 
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
 ])
@@ -5098,6 +5099,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 
's/table=../table=??/' | sort],
   table=??(ls_in_l2_lkup  ), priority=75   , match=(eth.src == 
{00:00:00:00:02:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport 
= "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 192.168.2.1), action=(clone {outport = "ls2-ro2"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 20.0.0.100), action=(clone {outport = "ls2-ro2"; 
output; }; outport = "_MC_flood_l2"; output;)
+  table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 20.0.0.200), action=(clone {outport = "ls2-ro2"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
nd_ns && nd.target == fe80::200:ff:fe00:201), action=(clone {outport = 
"ls2-ro2"; output; }; outport = "_MC_flood_l2"; output;)
 ])
 
@@ -5118,8 +5120,10 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
's/table=../table=??/' | sort],
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
   table=??(ls_in_l2_lkup  ), priority=75   , match=(eth.src == 
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport 
= "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80

Re: [ovs-dev] [PATCH ovn v3 08/16] northd: Refactor lflow management into a separate module.

2023-12-08 Thread Dumitru Ceara 
On 11/28/23 03:36, num...@ovn.org wrote:
> From: Numan Siddique 
> 
> ovn_lflow_add() and other related functions/macros are now moved
> into a separate module - lflow-mgr.c.  This module maintains a
> table 'struct lflow_table' for the logical flows.  lflow table
> maintains a hmap to store the logical flows.
> 
> It also maintains the logical switch and router dp groups.
> 
> Previous commits which added lflow incremental processing for
> the VIF logical ports, stored the references to
> the logical ports' lflows using 'struct lflow_ref_list'.  This
> struct is renamed to 'struct lflow_ref' and is part of lflow-mgr.c.
> It is  modified a bit to store the resource to lflow references.
> 
> Example usage of 'struct lflow_ref'.
> 
> 'struct ovn_port' maintains 2 instances of lflow_ref.  i,e
> 
> struct ovn_port {
>...
>...
>struct lflow_ref *lflow_ref;
>struct lflow_ref *stateful_lflow_ref;
> };
> 
> All the logical flows generated by
> build_lswitch_and_lrouter_iterate_by_lsp() uses the ovn_port->lflow_ref.
> 
> All the logical flows generated by build_lsp_lflows_for_lbnats()
> uses the ovn_port->stateful_lflow_ref.
> 
> When handling the ovn_port changes incrementally, the lflows referenced
> in 'struct ovn_port' are cleared and regenerated and synced to the
> SB logical flows.
> 
> eg.
> 
> lflow_ref_clear_lflows(op->lflow_ref);
> build_lswitch_and_lrouter_iterate_by_lsp(op, ...);
> lflow_ref_sync_lflows_to_sb(op->lflow_ref, ...);
> 
> This patch does few more changes:
>   -  Logical flows are now hashed without the logical
>  datapaths.  If a logical flow is referenced by just one
>  datapath, we don't rehash it.
> 
>   -  The synthetic 'hash' column of sbrec_logical_flow now
>  doesn't use the logical datapath.  This means that
>  when ovn-northd is updated/upgraded and has this commit,
>  all the logical flows with 'logical_datapath' column
>  set will get deleted and re-added causing some disruptions.
> 
>   -  With the commit [1] which added I-P support for logical
>  port changes, multiple logical flows with same match 'M'
>  and actions 'A' are generated and stored without the
>  dp groups, which was not the case prior to
>  that patch.
>  One example to generate these lflows is:
>  ovn-nbctl lsp-set-addresses sw0p1 "MAC1 IP1"
>  ovn-nbctl lsp-set-addresses sw1p1 "MAC1 IP1"
>ovn-nbctl lsp-set-addresses sw2p1 "MAC1 IP1"
> 
>  Now with this patch we go back to the earlier way.  i.e
>  one logical flow with logical_dp_groups set.
> 
>   -  With this patch any updates to a logical port which
>  doesn't result in new logical flows will not result in
>  deletion and addition of same logical flows.
>  Eg.
>  ovn-nbctl set logical_switch_port sw0p1 external_ids:foo=bar
>  will be a no-op to the SB logical flow table.
> 
> [1] - 8bbd678("northd: Incremental processing of VIF additions in 'lflow' 
> node.")
> 
> Signed-off-by: Numan Siddique 
> ---

Recheck-request: github-robot-_Build_and_Test

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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 v4 1/2] mcast-snooping: Store IGMP/MLD protocol version.

2023-12-08 Thread Simon Horman 
On Mon, Nov 20, 2023 at 08:52:00PM +0200, Mohammad Heib wrote:
> On Mon, Nov 20, 2023 at 6:09 PM Simon Horman  wrote:
> 
> > On Mon, Nov 20, 2023 at 04:22:44PM +0200, Mohammad Heib wrote:
> > > Store the igmp/mld protocol version into the
> > > mcast_group internally.
> > >
> > > This can be used by ovs consumers to update
> > > about the igmp/mld version of each group.
> >
> > Thanks Mohammad,
> >
> > I see in patch 2/2 that the user can now gain access to the igmp/mld
> > version of each group. But I am wondering if we could add some text
> > to the commit message to explain, perhaps via an example, why
> > a user might want such information.
> >
> 
> Hi Simon,
> 
> Thank you for the review.
> actually, i don't really have a good reason why the user will need the
> group protocol in OVS stand-alone case,
> but I'm trying to expand that here and save the protocol because i need it
> in the OVN/OVS case where we store
> each Mcast group information inside ovn-sb DB as raw in *the MCAST_GROUP*
> table, and i have to expose a protocol version
> for each Group in this table, cause OVN relies on the ovs mcast
> implementation to maintain this table, i thought this would be the cleaner
> way to accomplish that.
> 
> so now i don't really know which example can be good to add :(
> do you think adding a small example of extracting the protocol will be good
> enough?

Thanks Mohammad,

and sorry for the slow response.

I think a good way forwards would be to include an explanation
along the lines of usage in the OVN/OVS use-case.

Perhaps also add that exposing this to user tooling is expected
to aid debugging and test coverage (if that is true).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v3] controller: fixed potential segfault when changing tunnel_key and deleting ls

2023-12-08 Thread Xavier Simonart 
When a tunnel_key for a datapath was changed, the local_datapaths hmap was not 
properly updated
as the old/initial entry was not removed.
- If the datapath was not deleted at the same time, a new entry (for the same 
dp) was created
  in the local_datapaths as the previous entry was not found (wrong hash).
- If the datapath was deleted at the same time, the former entry also remained 
(as, again, the hash
  was wrong). So, we kept a deleted (dp) entry in the hmap, and might crash 
when we used it later.

When tunnel_key is updated for an existing datapath, we now clean the 
local_datapaths,
removing bad entries (i.e entries for which the hash is not the tunnel_key).

This issue was identified by flaky failures of test "ovn-ic -- route deletion 
upon TS deletion".

Backtrace:
0  0x00504a9a in hmap_first_with_hash (hmap=0x20f4d90, 
hmap@entry=0x5d5634, hmap=0x20f4d90, hmap@entry=0x5d5634, hash=1956414673) at 
./include/openvswitch/hmap.h:360
1  smap_find__ (smap=smap@entry=0x20f4d90, key=key@entry=0x5d5634 
"snat-ct-zone", key_len=key_len@entry=12, hash=1956414673) at lib/smap.c:421
2  0x00505073 in smap_get_node (key=0x5d5634 "snat-ct-zone", 
smap=0x20f4d90) at lib/smap.c:217
3  smap_get_def (def=0x0, key=0x5d5634 "snat-ct-zone", smap=0x20f4d90) at 
lib/smap.c:208
4  smap_get (smap=0x20f4d90, key=key@entry=0x5d5634 "snat-ct-zone") at 
lib/smap.c:200
5  0x00419898 in get_common_nat_zone (ldp=0x20a8c40, ldp=0x20a8c40) at 
controller/lflow.c:831
6  add_matches_to_flow_table (lflow=lflow@entry=0x21ddf90, 
ldp=ldp@entry=0x20a8c40, matches=matches@entry=0x211bb50, 
ptable=ptable@entry=10 '\n', output_ptable=output_ptable@entry=37 '%', 
ovnacts=ovnacts@entry=0x7ffd19b99de0,
ingress=true, l_ctx_in=0x7ffd19b9a300, l_ctx_out=0x7ffd19b9a2c0) at 
controller/lflow.c:892
7  0x0041a29b in consider_logical_flow__ (lflow=lflow@entry=0x21ddf90, 
dp=0x20eb720, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1207
8  0x0041a587 in consider_logical_flow (lflow=lflow@entry=0x21ddf90, 
is_recompute=is_recompute@entry=false, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1276
9  0x0041e30f in lflow_handle_changed_flows 
(l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:271
10 0x00439fc7 in lflow_output_sb_logical_flow_handler 
(node=0x7ffd19ba5b70, data=) at controller/ovn-controller.c:4045
11 0x004682fe in engine_compute (recompute_allowed=, 
node=) at lib/inc-proc-eng.c:441
12 engine_run_node (recompute_allowed=true, node=0x7ffd19ba5b70) at 
lib/inc-proc-eng.c:503
13 engine_run (recompute_allowed=recompute_allowed@entry=true) at 
lib/inc-proc-eng.c:528
14 0x0040ade2 in main (argc=, argv=) at 
controller/ovn-controller.c:5709

Signed-off-by: Xavier Simonart 

---
v2: Fixed potential memory leak.
v3: - Updated based on Dumitru's feedback: style & fixed another potential 
memory leak.
- Modify test case to detect potential memory leak.
- Rebased on latest main.
---
 controller/local_data.c | 14 
 controller/local_data.h |  4 
 controller/ovn-controller.c | 23 
 tests/ovn.at| 43 +
 4 files changed, 84 insertions(+)

diff --git a/controller/local_data.c b/controller/local_data.c
index 3a7d3afeb..8f0890a14 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -56,6 +56,20 @@ static bool datapath_is_transit_switch(const struct 
sbrec_datapath_binding *);
 
 static uint64_t local_datapath_usage;
 
+/* To be used when hmap_node.hash might be wrong e.g. tunnel_key got updated */
+struct local_datapath *
+get_local_datapath_no_hash(const struct hmap *local_datapaths,
+uint32_t tunnel_key)
+{
+struct local_datapath *ld;
+HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
+if (ld->datapath->tunnel_key == tunnel_key) {
+return ld;
+}
+}
+return NULL;
+}
+
 struct local_datapath *
 get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
 {
diff --git a/controller/local_data.h b/controller/local_data.h
index f6d8f725f..1586a76da 100644
--- a/controller/local_data.h
+++ b/controller/local_data.h
@@ -69,6 +69,10 @@ struct local_datapath *local_datapath_alloc(
 const struct sbrec_datapath_binding *);
 struct local_datapath *get_local_datapath(const struct hmap *,
   uint32_t tunnel_key);
+struct local_datapath
+*get_local_datapath_no_hash(const struct hmap *local_datapaths,
+uint32_t tunnel_key);
+
 bool
 need_add_peer_to_local(
 struct ovsdb_idl_index *sbrec_port_binding_by_name,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 760b7788b..e4fb3a5e4 100644
--- a/controller/ovn-controller.c

Re: [ovs-dev] [PATCH ovn v2 07/18] northd: Generate logical router's LB and NAT flows using lr_lbnat_data.

2023-12-08 Thread Dumitru Ceara 
On 12/6/23 04:38, Numan Siddique wrote:
> On Thu, Nov 23, 2023 at 4:15 PM Dumitru Ceara  wrote:
>>
>> On 10/26/23 20:15, num...@ovn.org wrote:
>>> From: Numan Siddique 
>>>
>>> Previous commits added new engine nodes to store logical router's lb
>>> and NAT data.  Make use of the data stored by these engine nodes
>>> to generate logical flows related to router's LBs and NATs.
>>>
>>> Signed-off-by: Numan Siddique 
>>> ---
>>>  northd/en-lflow.c  |   3 -
>>>  northd/en-lr-lb-nat-data.h |   4 +
>>>  northd/inc-proc-northd.c   |   1 -
>>>  northd/northd.c| 752 -
>>>  northd/northd.h|   1 -
>>>  5 files changed, 496 insertions(+), 265 deletions(-)
>>>
>>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
>>> index 9cb0ead3f0..229f4be1d0 100644
>>> --- a/northd/en-lflow.c
>>> +++ b/northd/en-lflow.c
>>> @@ -42,8 +42,6 @@ lflow_get_input_data(struct engine_node *node,
>>>  engine_get_input_data("port_group", node);
>>>  struct sync_meters_data *sync_meters_data =
>>>  engine_get_input_data("sync_meters", node);
>>> -struct ed_type_lr_nat_data *lr_nat_data =
>>> -engine_get_input_data("lr_nat", node);
>>>  struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
>>>  engine_get_input_data("lr_lb_nat_data", node);
>>>
>>> @@ -68,7 +66,6 @@ lflow_get_input_data(struct engine_node *node,
>>>  lflow_input->ls_ports = _data->ls_ports;
>>>  lflow_input->lr_ports = _data->lr_ports;
>>>  lflow_input->ls_port_groups = _data->ls_port_groups;
>>> -lflow_input->lr_nats = _nat_data->lr_nats;
>>>  lflow_input->lr_lbnats = _lb_nat_data->lr_lbnats;
>>>  lflow_input->meter_groups = _meters_data->meter_groups;
>>>  lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
>>> diff --git a/northd/en-lr-lb-nat-data.h b/northd/en-lr-lb-nat-data.h
>>> index 9029aee339..ffe41cad73 100644
>>> --- a/northd/en-lr-lb-nat-data.h
>>> +++ b/northd/en-lr-lb-nat-data.h
>>> @@ -56,6 +56,10 @@ struct lr_lb_nat_data_table {
>>>  #define LR_LB_NAT_DATA_TABLE_FOR_EACH(LR_LB_NAT_REC, TABLE) \
>>>  HMAP_FOR_EACH (LR_LB_NAT_REC, key_node, &(TABLE)->entries)
>>>
>>> +#define LR_LB_NAT_DATA_TABLE_FOR_EACH_IN_P(LR_LB_NAT_REC, JOBID, TABLE) \
>>> +HMAP_FOR_EACH_IN_PARALLEL (LR_LB_NAT_REC, key_node, JOBID, \
>>> +   &(TABLE)->entries)
>>> +
>>>  struct lr_lb_nat_data_tracked_data {
>>>  /* Created or updated logical router with LB data. */
>>>  struct hmapx crupdated; /* Stores 'struct lr_lb_nat_data_record'. */
>>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
>>> index 369a151fa3..84627070a8 100644
>>> --- a/northd/inc-proc-northd.c
>>> +++ b/northd/inc-proc-northd.c
>>> @@ -228,7 +228,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>>>  engine_add_input(_lflow, _sb_igmp_group, NULL);
>>>  engine_add_input(_lflow, _northd, lflow_northd_handler);
>>>  engine_add_input(_lflow, _port_group, lflow_port_group_handler);
>>> -engine_add_input(_lflow, _lr_nat, NULL);
>>
>> Was this supposed to go in the previous patch, the one that adds
>> en_lr_lb_nat_data?
> 
> Yes.  It should have been.  Addressed in v3.
> 
> Thanks
> 
> 
> 
>>
>>>  engine_add_input(_lflow, _lr_lb_nat_data, NULL);
>>>
>>>  engine_add_input(_sync_to_sb_addr_set, _nb_address_set,
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index 24df14c0de..1877cbc7df 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -8854,18 +8854,14 @@ build_lrouter_groups(struct hmap *lr_ports, struct 
>>> ovs_list *lr_list)
>>>   */
>>>  static void
>>>  build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op,
>>> -   uint32_t priority,
>>> -   struct ovn_datapath *od,
>>> -   const struct lr_nat_table 
>>> *lr_nats,
>>> -   struct hmap *lflows)
>>> +uint32_t priority,
>>> +const struct ovn_datapath *od,
>>> +const struct lr_nat_record 
>>> *lrnat_rec,
>>> +struct hmap *lflows)
>>
>>
>> Nit: indentation.
> 
> I had to do this way to keep under 80.
> 
> 
>>
>>>  {
>>>  struct ds eth_src = DS_EMPTY_INITIALIZER;
>>>  struct ds match = DS_EMPTY_INITIALIZER;
>>>
>>> -const struct lr_nat_record *lrnat_rec = lr_nat_table_find_by_index(
>>> -lr_nats, op->od->index);
>>> -ovs_assert(lrnat_rec);
>>> -
>>>  /* Self originated ARP requests/RARP/ND need to be flooded to the L2 
>>> domain
>>>   * (except on router ports).  Determine that packets are self 
>>> originated
>>>   * by also matching on source MAC. Matching on ingress port is not
>>> @@ -8952,7 +8948,8 @@ lrouter_port_ipv6_reachable(const struct ovn_port *op,
>>>

Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-12-08 Thread Dumitru Ceara 
On 12/6/23 04:03, Numan Siddique wrote:
> On Thu, Nov 23, 2023 at 9:04 AM Dumitru Ceara  wrote:
>>
>> On 11/17/23 23:05, Numan Siddique wrote:
>>> On Wed, Nov 15, 2023 at 1:27 AM Han Zhou  wrote:

 On Thu, Oct 26, 2023 at 11:15 AM  wrote:
>
> From: Numan Siddique 
>
> This new engine now maintains the NAT related data for each
> logical router which was earlier maintained by the northd
> engine node in the 'struct ovn_datapath'.  Main inputs to
> this engine node are:
>- northd
>- NB logical router
>
> A record for each logical router is maintained in the 'lr_nats'
> hmap table and this record
>   - stores the ovn_nat's

 It seems the sentence is incomplete.

>
> Handlers are also added to handle the changes to both these
> inputs.  This engine node becomes an input to 'lflow' node.
> This essentially decouples the lr NAT data from the northd
> engine node.
>
> Signed-off-by: Numan Siddique 
> ---
>  lib/ovn-util.c   |   6 +-
>  lib/ovn-util.h   |   2 +-
>  lib/stopwatch-names.h|   1 +
>  northd/automake.mk   |   2 +
>  northd/en-lflow.c|   5 +
>  northd/en-lr-nat.c   | 498 +
>  northd/en-lr-nat.h   | 134 ++
>  northd/en-sync-sb.c  |  11 +-
>  northd/inc-proc-northd.c |   9 +
>  northd/northd.c  | 514 ++-
>  northd/northd.h  |  32 ++-
>  tests/ovn-northd.at  |  18 ++
>  12 files changed, 877 insertions(+), 355 deletions(-)
>  create mode 100644 northd/en-lr-nat.c
>  create mode 100644 northd/en-lr-nat.h
>>
>> I would call these en-nat.{hc}, I think.  There's no NAT for switches.
> 
> 
> In v3 I haven't renamed it.  I can rename it in the next version once
> most of the review comments
> are addressed  (it's a bit of a pain to rename and fix all the compiler 
> errors).
> 
> But in this case I still prefer lr-nat (and the structures -
> lr_nat_record, ... ) because,
>  -  This engine node maintains the data for the NATs associated
> with a logical router.  A router
>  can have 0 or more NATs
>  - We have a NB NAT table which is for each NAT entry, whereas
> this engine node is for NATs associated with a router
> and it could confuse and not convey the relationship between
> router and NATs.
>  -  This patch creates a 'lr_nat_record' entry for every logical
> router even if it doesn't have any NATs.
> 
> 

OK, this seems reasonable.

>>
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 33105202f2..05e635a6b4 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
 sbrec_port_binding *binding,
>  }
>
>  bool
> -lport_addresses_is_empty(struct lport_addresses *laddrs)
> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
>  {
>  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
>  }
> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
 *laddrs)
>  {
>  free(laddrs->ipv4_addrs);
>  free(laddrs->ipv6_addrs);
> +laddrs->ipv4_addrs = NULL;
> +laddrs->ipv6_addrs = NULL;
> +laddrs->n_ipv4_addrs = 0;
> +laddrs->n_ipv6_addrs = 0;
>  }
>
>  /* Returns a string of the IP address of 'laddrs' that overlaps with
 'ip_s'.
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index bff50dbde9..5805415885 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
 sbrec_port_binding *binding,
>  bool extract_lrp_networks__(char *mac, char **networks, size_t
 n_networks,
>  struct lport_addresses *laddrs);
>
> -bool lport_addresses_is_empty(struct lport_addresses *);
> +bool lport_addresses_is_empty(const struct lport_addresses *);
>  void destroy_lport_addresses(struct lport_addresses *);
>  const char *find_lport_address(const struct lport_addresses *laddrs,
> const char *ip_s);
> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> index 3452cc71cf..0a16da211e 100644
> --- a/lib/stopwatch-names.h
> +++ b/lib/stopwatch-names.h
> @@ -32,5 +32,6 @@
>  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
>  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
>  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>
>  #endif
> diff --git a/northd/automake.mk b/northd/automake.mk
> index cf622fc3c9..ae367a2a8b 100644
> --- a/northd/automake.mk
> +++ b/northd/automake.mk
> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
>

Re: [ovs-dev] [PATCH ovn v2] controller: fixed potential segfault when changing tunnel_key and deleting ls

2023-12-08 Thread Xavier Simonart 
Hi Dumitru

Thanks for the review and sorry for the delay.

On Fri, Nov 10, 2023 at 4:30 PM Dumitru Ceara  wrote:

> On 11/3/23 10:38, Ales Musil wrote:
> > On Mon, Oct 30, 2023 at 9:46 AM Xavier Simonart 
> wrote:
> >
> >> When a tunnel_key for a datapath was changed, the local_datapaths hmap
> was
> >> not properly updated
> >> as the old/initial entry was not removed.
> >> - If the datapath was not deleted at the same time, a new entry (for the
> >> same dp) was created
> >>   in the local_datapaths as the previous entry was not found (wrong
> hash).
> >> - If the datapath was deleted at the same time, the former entry also
> >> remained (as, again, the hash
> >>   was wrong). So, we kept a deleted (dp) entry in the hmap, and might
> >> crash when we used it later.
> >>
> >> When tunnel_key is updated for an existing datapath, we now clean the
> >> local_datapaths,
> >> removing bad entries (i.e entries for which the hash is not the
> >> tunnel_key).
> >>
> >> This issue was identified by flaky failures of test "ovn-ic -- route
> >> deletion upon TS deletion".
> >>
> >> Backtrace:
> >> 0  0x00504a9a in hmap_first_with_hash (hmap=0x20f4d90,
> hmap@entry=0x5d5634,
> >> hmap=0x20f4d90, hmap@entry=0x5d5634, hash=1956414673) at
> >> ./include/openvswitch/hmap.h:360
> >> 1  smap_find__ (smap=smap@entry=0x20f4d90, key=key@entry=0x5d5634
> >> "snat-ct-zone", key_len=key_len@entry=12, hash=1956414673) at
> >> lib/smap.c:421
> >> 2  0x00505073 in smap_get_node (key=0x5d5634 "snat-ct-zone",
> >> smap=0x20f4d90) at lib/smap.c:217
> >> 3  smap_get_def (def=0x0, key=0x5d5634 "snat-ct-zone", smap=0x20f4d90)
> at
> >> lib/smap.c:208
> >> 4  smap_get (smap=0x20f4d90, key=key@entry=0x5d5634 "snat-ct-zone") at
> >> lib/smap.c:200
> >> 5  0x00419898 in get_common_nat_zone (ldp=0x20a8c40,
> >> ldp=0x20a8c40) at controller/lflow.c:831
> >> 6  add_matches_to_flow_table (lflow=lflow@entry=0x21ddf90,
> ldp=ldp@entry=0x20a8c40,
> >> matches=matches@entry=0x211bb50, ptable=ptable@entry=10 '\n',
> >> output_ptable=output_ptable@entry=37 '%', ovnacts=ovnacts@entry
> >> =0x7ffd19b99de0,
> >> ingress=true, l_ctx_in=0x7ffd19b9a300, l_ctx_out=0x7ffd19b9a2c0) at
> >> controller/lflow.c:892
> >> 7  0x0041a29b in consider_logical_flow__ (lflow=lflow@entry
> =0x21ddf90,
> >> dp=0x20eb720, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300,
> >> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1207
> >> 8  0x0041a587 in consider_logical_flow (lflow=lflow@entry
> =0x21ddf90,
> >> is_recompute=is_recompute@entry=false, l_ctx_in=l_ctx_in@entry
> =0x7ffd19b9a300,
> >> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1276
> >> 9  0x0041e30f in lflow_handle_changed_flows
> >> (l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, l_ctx_out=l_ctx_out@entry
> =0x7ffd19b9a2c0)
> >> at controller/lflow.c:271
> >> 10 0x00439fc7 in lflow_output_sb_logical_flow_handler
> >> (node=0x7ffd19ba5b70, data=) at
> >> controller/ovn-controller.c:4045
> >> 11 0x004682fe in engine_compute (recompute_allowed= >> out>, node=) at lib/inc-proc-eng.c:441
> >> 12 engine_run_node (recompute_allowed=true, node=0x7ffd19ba5b70) at
> >> lib/inc-proc-eng.c:503
> >> 13 engine_run (recompute_allowed=recompute_allowed@entry=true) at
> >> lib/inc-proc-eng.c:528
> >> 14 0x0040ade2 in main (argc=, argv= out>)
> >> at controller/ovn-controller.c:5709
> >>
> >> Signed-off-by: Xavier Simonart 
> >>
> >> ---
> >> v2: Fixed potential memory leak.
> >> ---
>
> Thanks, Xavier and Ales!  A few comments inline.
>
> >>  controller/local_data.c | 14 +
> >>  controller/local_data.h |  4 
> >>  controller/ovn-controller.c | 22 +++
> >>  tests/ovn.at| 42 +
> >>  4 files changed, 82 insertions(+)
> >>
> >> diff --git a/controller/local_data.c b/controller/local_data.c
> >> index 3a7d3afeb..8f0890a14 100644
> >> --- a/controller/local_data.c
> >> +++ b/controller/local_data.c
> >> @@ -56,6 +56,20 @@ static bool datapath_is_transit_switch(const struct
> >> sbrec_datapath_binding *);
> >>
> >>  static uint64_t local_datapath_usage;
> >>
> >> +/* To be used when hmap_node.hash might be wrong e.g. tunnel_key got
> >> updated */
> >> +struct local_datapath *
> >> +get_local_datapath_no_hash(const struct hmap *local_datapaths,
> >> +uint32_t tunnel_key)
> >> +{
> >> +struct local_datapath *ld;
> >> +HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> >> +if (ld->datapath->tunnel_key == tunnel_key) {
> >> +return ld;
> >> +}
> >> +}
> >> +return NULL;
> >> +}
> >> +
> >>  struct local_datapath *
> >>  get_local_datapath(const struct hmap *local_datapaths, uint32_t
> >> tunnel_key)
> >>  {
> >> diff --git a/controller/local_data.h b/controller/local_data.h
> >> index f6d8f725f..a1bf0790b 100644
> >> --- a/controller/local_data.h
> >> +++

Re: [ovs-dev] [PATCH ovn] ovn-macros: Make sure stopped daemons continue before the test ends.

2023-12-08 Thread Dumitru Ceara 
On 12/7/23 14:40, Xavier Simonart wrote:
> Hi Dumitru
> 
> Thanks for the patch.
> 
> On Thu, Dec 7, 2023 at 2:12 PM Dumitru Ceara  wrote:
> 
>> In case there's a test failure while daemons are stopped ensure that we
>> send a SIGCONT on exit so that they properly clean up.
>>
>> 30952c248d4f ("binding: fixed ovn-installed not properly removed
>> (recomputes)")
>>
> Missing "Fixes: "
> 
>> Fixes: 0794a6edf40b ("qos: fix potential double deletion of ovs idl row")
>> Fixes: feb918434172 ("northd: Skip transient IDL records.")
>> Suggested-by: Ilya Maximets 
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  tests/ovn-macros.at | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
>> index 94bdaff2c4..dd5df40b35 100644
>> --- a/tests/ovn-macros.at
>> +++ b/tests/ovn-macros.at
>> @@ -894,6 +894,7 @@ start_scapy_server() {
>>  sleep_northd() {
>>echo Northd going to sleep
>>AT_CHECK([kill -STOP $(cat northd/ovn-northd.pid)])
>> +  on_exit "kill -CONT $(cat northd/ovn-northd.pid)"
>>  }
>>
>>  wake_up_northd() {
>> @@ -904,6 +905,7 @@ wake_up_northd() {
>>  sleep_sb() {
>>echo SB going to sleep
>>AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])
>> +  on_exit "kill -CONT $(cat ovn-sb/ovsdb-server.pid)"
>>  }
>>  wake_up_sb() {
>>echo SB waking up
>> @@ -927,6 +929,7 @@ sleep_ovs() {
>>hv=$1
>>echo ovs $hv going to sleep
>>AT_CHECK([kill -STOP $(cat $hv/ovs-vswitchd.pid)])
>> +  on_exit "kill -CONT $(cat $hv/ovs-vswitchd.pid)"
>>  }
>>
>>  wake_up_ovs() {
>> @@ -938,6 +941,7 @@ wake_up_ovs() {
>>  sleep_ovsdb() {
>>echo OVSDB $1 going to sleep
>>AT_CHECK([kill -STOP $(cat $1/ovsdb-server.pid)])
>> +  on_exit "kill -CONT $(cat $1/ovsdb-server.pid)"
>>  }
>>  wake_up_ovsdb() {
>>echo OVSDB $1 waking up
>> --
>> 2.39.3
>>
>> Except for the missing "Fixes" in the commit message, it looks good to me.
> Acked-by: Xavier Simonart 
> 

Thanks for the review, Xavier!  I added the missing "Fixes" and pushed
this to main and stable branches down to 22.09.

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] northd: fix missing port up when deleting and adding back an lsp

2023-12-08 Thread Xavier Simonart 
Hi Numan

Thanks for the review and the comments

On Thu, Dec 7, 2023 at 12:48 AM Numan Siddique  wrote:

> On Mon, Nov 20, 2023 at 7:09 AM Ales Musil  wrote:
> >
> > On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart 
> wrote:
> >
> > > When a logical switch port was deleted and added back quickly, it could
> > > happen that the lsp was never reported up
> > >
> > > Signed-off-by: Xavier Simonart 
> > >
> >
> > Hi Xavier,
> >
> > I have one small nit below which could be addressed during merge if
> others
> > agree.
> >
> > ---
> > >  northd/northd.c | 17 +++--
> > >  tests/ovn.at| 45 +
> > >  2 files changed, 56 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index f8b046d83..0bea3bf2c 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -5192,11 +5192,16 @@ ls_port_has_changed(const struct
> > > nbrec_logical_switch_port *old,
> > >  }
> > >
> > >  static struct ovn_port *
> > > -ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name)
> > > +ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name,
> > > +  const struct uuid uuid)
> > >  {
> > >  struct ovn_port *op;
> > >  HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(name, 0),
> > > >ports) {
> > >  if (!strcmp(op->key, name)) {
> > > +if (op->nbsp && memcmp(>nbsp->header_.uuid, ,
> > > +sizeof(uuid))) {
> > >
> >
> > nit: There is uuid_equals() which we should use.
> >
>
> Hi Xavier,
>
> Thanks for the patch.
>
> If a logical switch has lots of logical ports,  we would end up
> calling "memcmp" for all of them which could be costly.
>
But using uuid_equals()  should be fine.
>
> I think we can simplify the code a bit since op->nbsp and new_nbsp
> pointers would be different if a logical port was deleted
> and re-added.  I think we can just compare op->nbsp == new_nbsp here.
>
> Does the below diff look good to you ?
>
>
> -
> diff --git a/northd/northd.c b/northd/northd.c
> index c043393860..3aa48ab074 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5180,28 +5180,20 @@ lsp_can_be_inc_processed(const struct
> nbrec_logical_switch_port *nbsp)
>  }
>
>  static bool
> -ls_port_has_changed(const struct nbrec_logical_switch_port *old,
> -const struct nbrec_logical_switch_port *new)
> +ls_port_has_changed(const struct nbrec_logical_switch_port *new)
>  {
> -if (old != new) {
> -return true;
> -}
>  /* XXX: Need a better OVSDB IDL interface for this check. */
>  return (nbrec_logical_switch_port_row_get_seqno(new,
>  OVSDB_IDL_CHANGE_MODIFY) > 0);
>  }
>
>  static struct ovn_port *
> -ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name,
> -  const struct uuid uuid)
> +ovn_port_find_in_datapath(struct ovn_datapath *od,
> +  const struct nbrec_logical_switch_port *nbsp)
>  {
>  struct ovn_port *op;
> -HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(name, 0),
> >ports) {
> -if (!strcmp(op->key, name)) {
> -if (op->nbsp && memcmp(>nbsp->header_.uuid, ,
> -sizeof(uuid))) {
> -continue;
> -}
> +HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(nbsp->name, 0),
> + >ports) {
> +if (!strcmp(op->key, nbsp->name) && op->nbsp == nbsp) {
>
Do we still need to compare the name/key ?  Is "if (op->nbsp == nbsp) {"
not enough ?

>  return op;
>  }
>  }
> @@ -5386,8 +5378,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  /* Compare the individual ports in the old and new Logical Switches */
>  for (size_t j = 0; j < changed_ls->n_ports; ++j) {
>  struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j];
> -op = ovn_port_find_in_datapath(od, new_nbsp->name,
> -   new_nbsp->header_.uuid);
> +op = ovn_port_find_in_datapath(od, new_nbsp);
> +
>  if (!op) {
>  if (!lsp_can_be_inc_processed(new_nbsp)) {
>  goto fail;
> @@ -5403,7 +5395,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  }
>  ovs_list_push_back(_change->added_ports,
>  >list);
> -} else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
> +} else if (ls_port_has_changed(new_nbsp)) {
>  /* Existing port updated */
>  bool temp = false;
>  if (lsp_is_type_changed(op->sb, new_nbsp, ) ||
>
> -
>
>
> >
> > > +continue;
> > > +}
> > >  return op;
> > >