diff mbox series

[ovs-dev,v9,2/3] controller: Fix ct zone assignment I+P.

Message ID c5374ef0a15178f96f2b9323959669811810db81.1746623091.git.felix.huettner@stackit.cloud
State Changes Requested
Headers show
Series controller: Add garp_rarp engine. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Felix Huettner May 7, 2025, 2:21 p.m. UTC 
Previously local ports that had an external_id:iface-id set would get a
ct-zone assigned on recompute, independent if there is a matching
southbound port_binding for them or if this port_binding can actually
reside on the local chassis.
However during incremental processing this was not the case and only
bound local ports would have their ct zone added or removed (as only
they are included in tracked_dp_bindings).

To fix this we add a status enum to local_lports that defines if there
is a matching port_binding or not.
The ct_zones engine node no only considers local_lports that are backed
by some port_binding entry.

This means we can also get rid of workarounds in testcases that where
introduced in 9f052bdb because of this problem.

Acked-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
---
v8->v9: addressed review comments
v6->v7:
  * introduced
  * addressed review comments

 controller/binding.c        | 33 ++++++++++++++++++++++++---------
 controller/binding.h        |  8 +++++++-
 controller/ct-zone.c        | 19 ++++++++++++++-----
 controller/ct-zone.h        |  2 +-
 controller/ovn-controller.c | 21 ++++++++++++---------
 tests/ovn.at                | 21 ---------------------
 6 files changed, 58 insertions(+), 46 deletions(-)
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index f5091d2cf..8e371605f 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -654,11 +654,19 @@  update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
  * Also track if the set has changed.
  */
 static void
-update_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx)
+update_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx,
+                    enum binding_local_lport_status status)
 {
-    if (sset_add(b_ctx->local_lports, iface_id) != NULL) {
-        b_ctx->local_lports_changed = true;
+    struct simap_node *node = simap_find(b_ctx->local_lports, iface_id);
+    if (node && node->data == status) {
+        return;
     }
+    if (node) {
+        node->data = status;
+    } else {
+        simap_put(b_ctx->local_lports, iface_id, status);
+    }
+    b_ctx->local_lports_changed = true;
 }
 
 /* Remove an interface ID from the set of local lports. Also track if the
@@ -667,7 +675,7 @@  update_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx)
 static void
 remove_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx)
 {
-    if (sset_find_and_delete(b_ctx->local_lports, iface_id)) {
+    if (simap_find_and_delete(b_ctx->local_lports, iface_id)) {
         b_ctx->local_lports_changed = true;
     }
 }
@@ -1604,7 +1612,8 @@  consider_vif_lport_(const struct sbrec_port_binding *pb,
                                b_ctx_out->local_datapaths,
                                b_ctx_out->tracked_dp_bindings);
             update_related_lport(pb, b_ctx_out);
-            update_local_lports(pb->logical_port, b_ctx_out);
+            update_local_lports(pb->logical_port, b_ctx_out,
+                                LPORT_STATUS_BOUND);
             if (binding_lport_update_port_sec(b_lport, pb) &&
                     b_ctx_out->tracked_dp_bindings) {
                 tracked_datapath_lport_add(pb, TRACKED_RESOURCE_UPDATED,
@@ -1907,6 +1916,10 @@  consider_localport(const struct sbrec_port_binding *pb,
         remove_related_lport(pb, b_ctx_out);
     }
 
+    /* Add all localnet ports to local_ifaces so that we allocate ct zones
+     * for them. */
+    update_local_lports(pb->logical_port, b_ctx_out, LPORT_STATUS_BOUND);
+
     update_related_lport(pb, b_ctx_out);
     return true;
 }
@@ -1925,7 +1938,7 @@  consider_nonvif_lport_(const struct sbrec_port_binding *pb,
                            pb->datapath->tunnel_key);
 
     if (our_chassis) {
-        update_local_lports(pb->logical_port, b_ctx_out);
+        update_local_lports(pb->logical_port, b_ctx_out, LPORT_STATUS_BOUND);
         if (!ld) {
             add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
                                b_ctx_in->sbrec_port_binding_by_datapath,
@@ -2034,7 +2047,7 @@  consider_localnet_lport(const struct sbrec_port_binding *pb,
 
     /* Add all localnet ports to local_ifaces so that we allocate ct zones
      * for them. */
-    update_local_lports(pb->logical_port, b_ctx_out);
+    update_local_lports(pb->logical_port, b_ctx_out, LPORT_STATUS_BOUND);
 
     add_or_del_qos_port(pb->logical_port, true);
     update_related_lport(pb, b_ctx_out);
@@ -2135,7 +2148,8 @@  build_local_bindings(struct binding_ctx_in *b_ctx_in,
                         iface_rec->name);
                 }
 
-                update_local_lports(iface_id, b_ctx_out);
+                update_local_lports(iface_id, b_ctx_out,
+                                    LPORT_STATUS_NOT_BOUND);
                 smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
                              iface_id);
             } else if (smap_get_bool(&iface_rec->external_ids,
@@ -2402,7 +2416,6 @@  consider_iface_claim(const struct ovsrec_interface *iface_rec,
                      struct binding_ctx_in *b_ctx_in,
                      struct binding_ctx_out *b_ctx_out)
 {
-    update_local_lports(iface_id, b_ctx_out);
     smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
 
     struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
@@ -2430,6 +2443,8 @@  consider_iface_claim(const struct ovsrec_interface *iface_rec,
         pb = b_lport->pb;
     }
 
+    update_local_lports(iface_id, b_ctx_out, pb ? LPORT_STATUS_BOUND :
+                        LPORT_STATUS_NOT_BOUND);
     if (!pb) {
         /* There is no port_binding row for this local binding. */
         return true;
diff --git a/controller/binding.h b/controller/binding.h
index 00dd9dbf5..8a5c6996c 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -71,6 +71,12 @@  struct related_lports {
                               */
 };
 
+enum binding_local_lport_status {
+    LPORT_STATUS_NOT_BOUND, /* Set if just iface-id is set but the port is
+                               not in sb. */
+    LPORT_STATUS_BOUND, /* Set if the port is in sb and locally relevant. */
+};
+
 void related_lports_init(struct related_lports *);
 void related_lports_destroy(struct related_lports *);
 
@@ -81,7 +87,7 @@  struct binding_ctx_out {
     struct local_binding_data *lbinding_data;
 
     /* sset of (potential) local lports. */
-    struct sset *local_lports;
+    struct simap *local_lports;
     /* Track if local_lports have been updated. */
     bool local_lports_changed;
 
diff --git a/controller/ct-zone.c b/controller/ct-zone.c
index 469a8fc54..a4f94375f 100644
--- a/controller/ct-zone.c
+++ b/controller/ct-zone.c
@@ -167,7 +167,7 @@  out:
 }
 
 void
-ct_zones_update(const struct sset *local_lports,
+ct_zones_update(const struct simap *local_lports,
                 const struct ovsrec_open_vswitch_table *ovs_table,
                 const struct hmap *local_datapaths, struct ct_zone_ctx *ctx)
 {
@@ -178,9 +178,11 @@  ct_zones_update(const struct sset *local_lports,
     unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES);
     struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones);
 
-    const char *local_lport;
-    SSET_FOR_EACH (local_lport, local_lports) {
-        sset_add(&all_users, local_lport);
+    const struct simap_node *sinode;
+    SIMAP_FOR_EACH (sinode, local_lports) {
+        if (sinode->data == LPORT_STATUS_BOUND) {
+            sset_add(&all_users, sinode->name);
+        }
     }
 
     /* Local patched datapath (gateway routers) need zones assigned. */
@@ -217,7 +219,14 @@  ct_zones_update(const struct sset *local_lports,
     SHASH_FOR_EACH_SAFE (node, &ctx->current) {
         struct ct_zone *ct_zone = node->data;
         if (!sset_contains(&all_users, node->name)) {
-            ct_zone_remove(ctx, node->name);
+            /* If we started recently and do not monitor all db entries then we
+             * might not have yet learned a port binding entry to the port.
+             * In this case we skip removing the zone until we either know the
+             * port binding or we are sure the zone is no longer needed. */
+            if (!(daemon_started_recently() &&
+                    simap_contains(local_lports, node->name))) {
+                ct_zone_remove(ctx, node->name);
+            }
         } else if (!simap_find(&req_snat_zones, node->name)) {
             if (ct_zone->zone < min_ct_zone || ct_zone->zone > max_ct_zone) {
                 ct_zone_remove(ctx, node->name);
diff --git a/controller/ct-zone.h b/controller/ct-zone.h
index 6df03975c..cc313d423 100644
--- a/controller/ct-zone.h
+++ b/controller/ct-zone.h
@@ -70,7 +70,7 @@  void ct_zones_restore(struct ct_zone_ctx *ctx,
                       const struct ovsrec_open_vswitch_table *ovs_table,
                       const struct sbrec_datapath_binding_table *dp_table,
                       const struct ovsrec_bridge *br_int);
-void ct_zones_update(const struct sset *local_lports,
+void ct_zones_update(const struct simap *local_lports,
                      const struct ovsrec_open_vswitch_table *ovs_table,
                      const struct hmap *local_datapaths,
                      struct ct_zone_ctx *ctx);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 946e6146d..34a021416 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -218,7 +218,7 @@  static char *get_file_system_id(void)
 static unsigned int
 update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
                    const struct sbrec_chassis *chassis,
-                   const struct sset *local_ifaces,
+                   const struct simap *local_ifaces,
                    const struct shash *local_bindings,
                    struct hmap *local_datapaths,
                    bool monitor_all)
@@ -358,7 +358,9 @@  update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
         const char *name;
 
         ovs_assert(local_bindings);
-        SSET_FOR_EACH (name, local_ifaces) {
+        const struct simap_node *n;
+        SIMAP_FOR_EACH (n, local_ifaces) {
+            name = n->name;
             /* Skip the VIFs we bound already, we should have a local datapath
              * for those. */
             const struct sbrec_port_binding *local_pb
@@ -1293,8 +1295,9 @@  struct ed_type_runtime_data {
      * hypervisor.  These logical ports include the VIFs (and their child
      * logical ports, if any) that belong to VMs running on the hypervisor,
      * l2gateway ports for which options:l2gateway-chassis designates the
-     * local hypervisor, and localnet ports. */
-    struct sset local_lports;
+     * local hypervisor, and localnet ports.
+     * The value is mapped to enum binding_local_lport_status. */
+    struct simap local_lports;
 
     /* Port bindings that are relevant to the local chassis (VIFs bound
      * localy, patch ports).
@@ -1398,7 +1401,7 @@  en_runtime_data_init(struct engine_node *node OVS_UNUSED,
     struct ed_type_runtime_data *data = xzalloc(sizeof *data);
 
     hmap_init(&data->local_datapaths);
-    sset_init(&data->local_lports);
+    simap_init(&data->local_lports);
     related_lports_init(&data->related_lports);
     sset_init(&data->active_tunnels);
     hmap_init(&data->qos_map);
@@ -1418,7 +1421,7 @@  en_runtime_data_cleanup(void *data)
 {
     struct ed_type_runtime_data *rt_data = data;
 
-    sset_destroy(&rt_data->local_lports);
+    simap_destroy(&rt_data->local_lports);
     related_lports_destroy(&rt_data->related_lports);
     sset_destroy(&rt_data->active_tunnels);
     destroy_qos_map(&rt_data->qos_map);
@@ -1532,7 +1535,7 @@  en_runtime_data_run(struct engine_node *node, void *data)
     struct hmap *local_datapaths = &rt_data->local_datapaths;
     struct shash *local_active_ipv6_pd = &rt_data->local_active_ports_ipv6_pd;
     struct shash *local_active_ras = &rt_data->local_active_ports_ras;
-    struct sset *local_lports = &rt_data->local_lports;
+    struct simap *local_lports = &rt_data->local_lports;
     struct sset *active_tunnels = &rt_data->active_tunnels;
 
     static bool first_run = true;
@@ -1544,13 +1547,13 @@  en_runtime_data_run(struct engine_node *node, void *data)
         shash_clear(local_active_ipv6_pd);
         shash_clear(local_active_ras);
         local_binding_data_destroy(&rt_data->lbinding_data);
-        sset_destroy(local_lports);
+        simap_destroy(local_lports);
         related_lports_destroy(&rt_data->related_lports);
         sset_destroy(active_tunnels);
         destroy_qos_map(&rt_data->qos_map);
         smap_destroy(&rt_data->local_iface_ids);
         hmap_init(local_datapaths);
-        sset_init(local_lports);
+        simap_init(local_lports);
         related_lports_init(&rt_data->related_lports);
         sset_init(active_tunnels);
         hmap_init(&rt_data->qos_map);
diff --git a/tests/ovn.at b/tests/ovn.at
index 9af74774f..5a4546150 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -15816,16 +15816,6 @@  echo $request >> hv3/third.expected
 
 check_packets
 
-# ct_zone allocation differs between I+P and recompute:
-# - In I+P an ovs interface with external-ids:iface-id set (but no
-#   logical_port in sb) results in no ct_zone allocated.
-# - In recompute, in the same case, a ct_zone is allocated - this is
-#   necessary to avoid flushing ct_zone if/when controller is restarted.
-# As migrrator has iface-id set on hv1 but is bound to hv2, we would hit
-# some ct-zone related differences in flows before and after recompute.
-# Avoid/hide this by removing iface-id on hv1.
-
-as hv1 check ovs-vsctl remove Interface migrator external-ids iface-id
 OVN_CLEANUP([hv1],[hv2],[hv3])
 
 AT_CLEANUP
@@ -16216,11 +16206,6 @@  echo $request >> hv2/n1.expected
 
 check_packets
 
-# ct_zone allocation differs between I+P and recompute.
-# As migrrator has iface-id set on hv1 but is bound to hv2, we would hit
-# some ct-zone related differences in flows before and after recompute.
-# Avoid/hide this by removing iface-id on hv1.
-as hv1 check ovs-vsctl remove Interface migrator external-ids iface-id
 OVN_CLEANUP([hv1],[hv2],[hv3])
 
 AT_CLEANUP
@@ -16777,12 +16762,6 @@  m4_define([ACTIVATION_STRATEGY_TEST],
     check ovn-nbctl lsp-set-options migrator requested-chassis=hv2
     OVS_WAIT_UNTIL([test x = x$(ovn-sbctl get Port_Binding $pb_uuid options:additional-chassis-activated)])
 
-    # ct_zone allocation differs between I+P and recompute.
-    # As migrrator has iface-id set on hv1 but is bound to hv2, we would hit
-    # some ct-zone related differences in flows before and after recompute.
-    # Avoid/hide this by removing iface-id on hv1.
-    as hv1 check ovs-vsctl remove Interface migrator external-ids iface-id
-
     OVN_CLEANUP([hv1],[hv2],[hv3])
 
     AT_CLEANUP