Jacob Tanenbaum reported to me an issue he was having with a patch he was writing based off this series. It turns out there is a bug in this particular patch that needs to be addressed. Since this is a change for a single patch in an unreviewed series, I figured I'd post the follow-up diff here instead of posting an entire new version of the series:

Please apply the following diff on top of this patch when reviewing/merging:

-- >8 --

 This document will expire in 6 days.
diff --git a/northd/datapath-sync.h b/northd/datapath-sync.h
index 879904903..9d9deb0e2 100644
--- a/northd/datapath-sync.h
+++ b/northd/datapath-sync.h
@@ -107,9 +107,6 @@ struct ovn_synced_datapath {
     struct hmap_node hmap_node;
     const struct ovsdb_idl_row *nb_row;
     const struct sbrec_datapath_binding *sb_dp;
-    /* SB datapath_binding must be refreshed with stable
-     * pointer to DB row. */
-    bool update_sb_dp;
 };

 struct ovn_synced_datapaths {
diff --git a/northd/en-datapath-sync.c b/northd/en-datapath-sync.c
index fc4f0a37b..e7fb9b791 100644
--- a/northd/en-datapath-sync.c
+++ b/northd/en-datapath-sync.c
@@ -116,15 +116,13 @@ struct candidate_sdp {

 static struct ovn_synced_datapath *
 synced_datapath_alloc(const struct ovn_unsynced_datapath *udp,
-                      const struct sbrec_datapath_binding *sb_dp,
-                      bool update_sb_dp)
+                      const struct sbrec_datapath_binding *sb_dp)
 {
     struct ovn_synced_datapath *sdp;
     sdp = xmalloc(sizeof *sdp);
     *sdp = (struct ovn_synced_datapath) {
         .sb_dp = sb_dp,
         .nb_row = udp->nb_row,
-        .update_sb_dp = update_sb_dp,
     };
     return sdp;
 }
@@ -197,7 +195,7 @@ create_synced_datapath_candidates_from_sb(
         }

         struct candidate_sdp candidate = {
-            .sdp = synced_datapath_alloc(udp, sb_dp, false),
+            .sdp = synced_datapath_alloc(udp, sb_dp),
             .sb_dp = sb_dp,
             .requested_tunnel_key = udp->requested_tunnel_key,
             .existing_tunnel_key = sb_dp->tunnel_key,
@@ -225,7 +223,7 @@ create_synced_datapath_candidates_from_nb(
             struct sbrec_datapath_binding *sb_dp;
             sb_dp = sbrec_datapath_binding_insert(ovnsb_idl_txn);
             struct candidate_sdp candidate = {
-                .sdp = synced_datapath_alloc(udp, NULL, true),
+                .sdp = synced_datapath_alloc(udp, NULL),
                 .sb_dp = sb_dp,
                 .requested_tunnel_key = udp->requested_tunnel_key,
                 .existing_tunnel_key = sb_dp->tunnel_key,
@@ -364,7 +362,7 @@ datapath_sync_unsynced_datapath_handler(
     HMAPX_FOR_EACH (n, &map->deleted) {
         udp = n->data;
         sdp = find_synced_datapath_from_udp(synced_datapaths, udp);
-        if (!sdp || sdp->update_sb_dp) {
+        if (!sdp) {
             return EN_UNHANDLED;
         }
         hmap_remove(&synced_datapaths->synced_dps, &sdp->hmap_node);
@@ -402,7 +400,7 @@ datapath_sync_unsynced_datapath_handler(
         struct sbrec_datapath_binding *sb_dp =
             sbrec_datapath_binding_insert(ovnsb_idl_txn);
         sbrec_datapath_binding_set_tunnel_key(sb_dp, tunnel_key);
-        sdp = synced_datapath_alloc(udp, NULL, true);
+        sdp = synced_datapath_alloc(udp, NULL);
         synced_datapath_set_sb_fields(sb_dp, udp);
         hmap_insert(&synced_datapaths->pending_dps, &sdp->hmap_node,
                     uuid_hash(sb_dp->nb_uuid));

On 7/18/25 12:14 PM, Mark Michelson via dev wrote:
From: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>

Introduce Incremetal processing logic to datapath-sync node for
en_datapath_logical_switch and en_datapath_logical_router inputs.

When processing incrementally, the behavior of en-datapath-sync is
different than when run non-incrementally. When new northbound logical
datapath types are added, en-datapath-sync will not propagate the new
logical datapath downstream to other nodes. Instead, en-datapath-sync
waits for the southbound database to report the newly inserted
Datapath_Binding, and then en-datapath-sync reports the new datapath.
See the large comment in northd/datapath-sync.h for more details.

One test change that may seem unrelated is the propage sb_cfg test in
ovn-northd.at. That test checked for a specific sequence of events to
happen based on northbound database manipulation. The test had used the
insertion of a logical switch to test this. Now, inserting a logical
switch causes more than one engine run to occur because of the way
en-datapath-sync works. The easiest way to fix the test was to insert a
different type of row into the database, so this commit changes to
using a port group now.

Reported-at: https://issues.redhat.com/browse/FDP-1519
Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
---
  northd/datapath-sync.h    |  37 +++++
  northd/en-datapath-sync.c | 324 ++++++++++++++++++++++++++++++++++++--
  northd/en-datapath-sync.h |   7 +
  northd/en-northd.c        |   2 -
  northd/inc-proc-northd.c  |  12 +-
  northd/northd.h           |   1 -
  tests/ovn-northd.at       |  44 +++++-
  7 files changed, 402 insertions(+), 25 deletions(-)

diff --git a/northd/datapath-sync.h b/northd/datapath-sync.h
index 58f5f1382..879904903 100644
--- a/northd/datapath-sync.h
+++ b/northd/datapath-sync.h
@@ -36,6 +36,39 @@
   * version (e.g. ovn_synced_logical_router). Later nodes can then consume
   * these type-specific synced datapath types in order to perform
   * further processing.
+ *
+ * Incremental processing throws an interesting wrinkle to the mix.
+ * When northbound logical datapaths are new, the datapath_sync node
+ * inserts a new Datapath_Binding into the southbound database. The
+ * new synced datapath is added to a "pending" hmap of synced datapaths.
+ * Once the southbound database sends its update showing the inserted
+ * Datapath_Binding, then the synced datapath is moved from the "pending"
+ * hmap to the "synced_dps" hmap. The engine nodes that process synced
+ * datapaths only pay attention to the synced datapaths in the "synced_dps"
+ * hmap. This means that consumers of synced logical switches or synced
+ * logical routers will be informed of "new" synced datapaths after the
+ * northbound IDL has reported the logical router or logical switch as
+ * new. This means that the nbrec_logical_switch_is_new() and
+ * nbrec_logical_router_is_new() functions cannot be used to detect if
+ * a logical switch or logical router is new. Instead, consumers of
+ * synced datapath types can tell if the datapath is new by its
+ * presence in the "new" hmapx of the synced logical datapath map.
+ *
+ * The reason this is done is to ensure that valid southbound
+ * datapath binding pointers are provided to all engine nodes. If we
+ * provided the southbound datapath binding at the time of insertion,
+ * then the pointer would go out of scope when the IDL is run next. By
+ * waiting for an updated from the southbound database, we get a more
+ * permanent pointer to the southbound datapath binding that is safe
+ * to cache.
+ *
+ * For updated or deleted northbound logical datapaths, there is no
+ * such delay. The update or deletion is passed to downstream engine
+ * nodes in the same IDL run when they are detected as being updated
+ * or deleted in the northbound database. Therefore, the
+ * nbrec_logical_switch_is_deleted() and nbrec_logical_router_is_deleted()
+ * functions are still valid to call from consumers of synced datapath
+ * engine nodes.
   */
enum ovn_datapath_type {
@@ -74,10 +107,14 @@ struct ovn_synced_datapath {
      struct hmap_node hmap_node;
      const struct ovsdb_idl_row *nb_row;
      const struct sbrec_datapath_binding *sb_dp;
+    /* SB datapath_binding must be refreshed with stable
+     * pointer to DB row. */
+    bool update_sb_dp;
  };
struct ovn_synced_datapaths {
      struct hmap synced_dps;
+    struct hmap pending_dps;
      struct hmap dp_tnlids;
struct hmapx new;
diff --git a/northd/en-datapath-sync.c b/northd/en-datapath-sync.c
index db84448e0..fc4f0a37b 100644
--- a/northd/en-datapath-sync.c
+++ b/northd/en-datapath-sync.c
@@ -35,7 +35,11 @@ en_datapath_sync_init(struct engine_node *node OVS_UNUSED,
          = xmalloc(sizeof *synced_datapaths);
      *synced_datapaths = (struct ovn_synced_datapaths) {
          .synced_dps = HMAP_INITIALIZER(&synced_datapaths->synced_dps),
+        .pending_dps = HMAP_INITIALIZER(&synced_datapaths->pending_dps),
          .dp_tnlids = HMAP_INITIALIZER(&synced_datapaths->dp_tnlids),
+        .new = HMAPX_INITIALIZER(&synced_datapaths->new),
+        .deleted = HMAPX_INITIALIZER(&synced_datapaths->deleted),
+        .updated = HMAPX_INITIALIZER(&synced_datapaths->updated),
      };
return synced_datapaths;
@@ -70,8 +74,41 @@ find_unsynced_datapath(const struct 
ovn_unsynced_datapath_map **maps,
      return NULL;
  }
+static struct ovn_synced_datapath *
+find_synced_datapath_from_udp(
+        const struct ovn_synced_datapaths *synced_datapaths,
+        const struct ovn_unsynced_datapath *udp)
+{
+    struct ovn_synced_datapath *sdp;
+    uint32_t hash = uuid_hash(&udp->nb_row->uuid);
+    HMAP_FOR_EACH_WITH_HASH (sdp, hmap_node, hash,
+                             &synced_datapaths->synced_dps) {
+        if (uuid_equals(&sdp->nb_row->uuid, &udp->nb_row->uuid)) {
+            return sdp;
+        }
+    }
+
+    return NULL;
+}
+
+static struct ovn_synced_datapath *
+find_synced_datapath_from_sb(const struct hmap *datapaths,
+                             const struct sbrec_datapath_binding *sb_dp)
+{
+    struct ovn_synced_datapath *sdp;
+    uint32_t hash = uuid_hash(sb_dp->nb_uuid);
+    HMAP_FOR_EACH_WITH_HASH (sdp, hmap_node, hash, datapaths) {
+        if (uuid_equals(&sdp->nb_row->uuid, sb_dp->nb_uuid)) {
+            return sdp;
+        }
+    }
+
+    return NULL;
+}
+
  struct candidate_sdp {
      struct ovn_synced_datapath *sdp;
+    const struct sbrec_datapath_binding *sb_dp;
      uint32_t requested_tunnel_key;
      uint32_t existing_tunnel_key;
      bool tunnel_key_assigned;
@@ -79,21 +116,42 @@ struct candidate_sdp {
static struct ovn_synced_datapath *
  synced_datapath_alloc(const struct ovn_unsynced_datapath *udp,
-                      const struct sbrec_datapath_binding *sb_dp)
+                      const struct sbrec_datapath_binding *sb_dp,
+                      bool update_sb_dp)
  {
      struct ovn_synced_datapath *sdp;
      sdp = xmalloc(sizeof *sdp);
      *sdp = (struct ovn_synced_datapath) {
          .sb_dp = sb_dp,
          .nb_row = udp->nb_row,
+        .update_sb_dp = update_sb_dp,
      };
+    return sdp;
+}
+
+static void
+synced_datapath_set_sb_fields(const struct sbrec_datapath_binding *sb_dp,
+                              const struct ovn_unsynced_datapath *udp)
+{
      sbrec_datapath_binding_set_external_ids(sb_dp, &udp->external_ids);
sbrec_datapath_binding_set_nb_uuid(sb_dp, &udp->nb_row->uuid, 1); sbrec_datapath_binding_set_type(sb_dp,
                                      ovn_datapath_type_to_string(udp->type));
-    return sdp;
+}
+
+static void
+clear_tracked_data(struct ovn_synced_datapaths *synced_datapaths)
+{
+    hmapx_clear(&synced_datapaths->new);
+    hmapx_clear(&synced_datapaths->updated);
+
+    struct hmapx_node *node;
+    HMAPX_FOR_EACH_SAFE (node, &synced_datapaths->deleted) {
+        free(node->data);
+        hmapx_delete(&synced_datapaths->deleted, node);
+    }
  }
static void
@@ -103,7 +161,11 @@ reset_synced_datapaths(struct ovn_synced_datapaths 
*synced_datapaths)
      HMAP_FOR_EACH_POP (sdp, hmap_node, &synced_datapaths->synced_dps) {
          free(sdp);
      }
+    HMAP_FOR_EACH_POP (sdp, hmap_node, &synced_datapaths->pending_dps) {
+        free(sdp);
+    }
      ovn_destroy_tnlids(&synced_datapaths->dp_tnlids);
+    clear_tracked_data(synced_datapaths);
      hmap_init(&synced_datapaths->dp_tnlids);
  }
@@ -135,10 +197,12 @@ create_synced_datapath_candidates_from_sb(
          }
struct candidate_sdp candidate = {
-            .sdp = synced_datapath_alloc(udp, sb_dp),
+            .sdp = synced_datapath_alloc(udp, sb_dp, false),
+            .sb_dp = sb_dp,
              .requested_tunnel_key = udp->requested_tunnel_key,
              .existing_tunnel_key = sb_dp->tunnel_key,
          };
+        synced_datapath_set_sb_fields(sb_dp, udp);
          vector_push(candidate_sdps, &candidate);
          uuidset_insert(visited, &udp->nb_row->uuid);
      }
@@ -161,10 +225,12 @@ create_synced_datapath_candidates_from_nb(
              struct sbrec_datapath_binding *sb_dp;
              sb_dp = sbrec_datapath_binding_insert(ovnsb_idl_txn);
              struct candidate_sdp candidate = {
-                .sdp = synced_datapath_alloc(udp, sb_dp),
+                .sdp = synced_datapath_alloc(udp, NULL, true),
+                .sb_dp = sb_dp,
                  .requested_tunnel_key = udp->requested_tunnel_key,
                  .existing_tunnel_key = sb_dp->tunnel_key,
              };
+            synced_datapath_set_sb_fields(sb_dp, udp);
              vector_push(candidate_sdps, &candidate);
          }
      }
@@ -188,10 +254,17 @@ assign_requested_tunnel_keys(struct vector 
*candidate_sdps,
                           candidate->requested_tunnel_key);
              continue;
          }
-        sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp,
+        sbrec_datapath_binding_set_tunnel_key(candidate->sb_dp,
                                                
candidate->requested_tunnel_key);
-        hmap_insert(&synced_datapaths->synced_dps, &candidate->sdp->hmap_node,
-                    uuid_hash(candidate->sdp->sb_dp->nb_uuid));
+        if (candidate->sdp->sb_dp) {
+            hmap_insert(&synced_datapaths->synced_dps,
+                        &candidate->sdp->hmap_node,
+                        uuid_hash(candidate->sb_dp->nb_uuid));
+        } else {
+            hmap_insert(&synced_datapaths->pending_dps,
+                        &candidate->sdp->hmap_node,
+                        uuid_hash(candidate->sb_dp->nb_uuid));
+        }
          candidate->tunnel_key_assigned = true;
      }
  }
@@ -211,9 +284,15 @@ assign_existing_tunnel_keys(struct vector *candidate_sdps,
           */
          if (ovn_add_tnlid(&synced_datapaths->dp_tnlids,
                            candidate->existing_tunnel_key)) {
-            hmap_insert(&synced_datapaths->synced_dps,
-                        &candidate->sdp->hmap_node,
-                        uuid_hash(candidate->sdp->sb_dp->nb_uuid));
+            if (candidate->sdp->sb_dp) {
+                hmap_insert(&synced_datapaths->synced_dps,
+                            &candidate->sdp->hmap_node,
+                            uuid_hash(candidate->sb_dp->nb_uuid));
+            } else {
+                hmap_insert(&synced_datapaths->pending_dps,
+                            &candidate->sdp->hmap_node,
+                            uuid_hash(candidate->sb_dp->nb_uuid));
+            }
              candidate->tunnel_key_assigned = true;
          }
      }
@@ -237,10 +316,17 @@ allocate_tunnel_keys(struct vector *candidate_sdps,
          if (!tunnel_key) {
              continue;
          }
-        sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp,
+        sbrec_datapath_binding_set_tunnel_key(candidate->sb_dp,
                                                tunnel_key);
-        hmap_insert(&synced_datapaths->synced_dps, &candidate->sdp->hmap_node,
-                    uuid_hash(candidate->sdp->sb_dp->nb_uuid));
+        if (candidate->sdp->sb_dp) {
+            hmap_insert(&synced_datapaths->synced_dps,
+                        &candidate->sdp->hmap_node,
+                        uuid_hash(candidate->sb_dp->nb_uuid));
+        } else {
+            hmap_insert(&synced_datapaths->pending_dps,
+                        &candidate->sdp->hmap_node,
+                        uuid_hash(candidate->sb_dp->nb_uuid));
+        }
          candidate->tunnel_key_assigned = true;
      }
  }
@@ -253,11 +339,203 @@ delete_unassigned_candidates(struct vector 
*candidate_sdps)
          if (candidate->tunnel_key_assigned) {
              continue;
          }
-        sbrec_datapath_binding_delete(candidate->sdp->sb_dp);
+        sbrec_datapath_binding_delete(candidate->sb_dp);
          free(candidate->sdp);
      }
  }
+static enum engine_input_handler_result
+datapath_sync_unsynced_datapath_handler(
+        const struct ovn_unsynced_datapath_map *map,
+        const struct ed_type_global_config *global_config,
+        struct ovsdb_idl_txn *ovnsb_idl_txn, void *data)
+{
+    enum engine_input_handler_result ret = EN_HANDLED_UNCHANGED;
+    struct ovn_synced_datapaths *synced_datapaths = data;
+    struct ovn_unsynced_datapath *udp;
+    struct ovn_synced_datapath *sdp;
+
+    if (!(hmapx_count(&map->new) + hmapx_count(&map->deleted) +
+          hmapx_count(&map->updated))) {
+        return EN_UNHANDLED;
+    }
+
+    struct hmapx_node *n;
+    HMAPX_FOR_EACH (n, &map->deleted) {
+        udp = n->data;
+        sdp = find_synced_datapath_from_udp(synced_datapaths, udp);
+        if (!sdp || sdp->update_sb_dp) {
+            return EN_UNHANDLED;
+        }
+        hmap_remove(&synced_datapaths->synced_dps, &sdp->hmap_node);
+        hmapx_add(&synced_datapaths->deleted, sdp);
+        ovn_free_tnlid(&synced_datapaths->dp_tnlids,
+                       sdp->sb_dp->tunnel_key);
+        sbrec_datapath_binding_delete(sdp->sb_dp);
+        ret = EN_HANDLED_UPDATED;
+    }
+
+    HMAPX_FOR_EACH (n, &map->new) {
+        udp = n->data;
+        uint32_t tunnel_key;
+
+        if (find_synced_datapath_from_udp(synced_datapaths, udp)) {
+            return EN_UNHANDLED;
+        }
+
+        if (udp->requested_tunnel_key) {
+            tunnel_key = udp->requested_tunnel_key;
+            if (!ovn_add_tnlid(&synced_datapaths->dp_tnlids, tunnel_key)) {
+                return EN_UNHANDLED;
+            }
+        } else {
+            uint32_t hint = 0;
+            tunnel_key = ovn_allocate_tnlid(&synced_datapaths->dp_tnlids,
+                                            "datapath", OVN_MIN_DP_KEY_LOCAL,
+                                            global_config->max_dp_tunnel_id,
+                                            &hint);
+            if (!tunnel_key) {
+                return EN_UNHANDLED;
+            }
+        }
+
+        struct sbrec_datapath_binding *sb_dp =
+            sbrec_datapath_binding_insert(ovnsb_idl_txn);
+        sbrec_datapath_binding_set_tunnel_key(sb_dp, tunnel_key);
+        sdp = synced_datapath_alloc(udp, NULL, true);
+        synced_datapath_set_sb_fields(sb_dp, udp);
+        hmap_insert(&synced_datapaths->pending_dps, &sdp->hmap_node,
+                    uuid_hash(sb_dp->nb_uuid));
+        /* Don't mark the node as UPDATED since we are only adding this
+         * to the pending datapaths. When the southbound datapath binding
+         * handler runs, this will get moved to synced_dps and added to
+         * synced_datapaths->new.
+         */
+    }
+
+    HMAPX_FOR_EACH (n, &map->updated) {
+        udp = n->data;
+        sdp = find_synced_datapath_from_udp(synced_datapaths, udp);
+        if (!sdp || !sdp->sb_dp) {
+            return EN_UNHANDLED;
+        }
+        if (udp->requested_tunnel_key &&
+            udp->requested_tunnel_key != sdp->sb_dp->tunnel_key) {
+            ovn_free_tnlid(&synced_datapaths->dp_tnlids,
+                           sdp->sb_dp->tunnel_key);
+            if (!ovn_add_tnlid(&synced_datapaths->dp_tnlids,
+                               udp->requested_tunnel_key)) {
+                return EN_UNHANDLED;
+            }
+            sbrec_datapath_binding_set_tunnel_key(sdp->sb_dp,
+                                                  udp->requested_tunnel_key);
+        }
+        if (!smap_equal(&udp->external_ids, &sdp->sb_dp->external_ids)) {
+            sbrec_datapath_binding_set_external_ids(sdp->sb_dp,
+                                                    &udp->external_ids);
+        }
+        hmapx_add(&synced_datapaths->updated, sdp);
+        ret = EN_HANDLED_UPDATED;
+    }
+
+    return ret;
+}
+
+enum engine_input_handler_result
+datapath_sync_logical_switch_handler(struct engine_node *node, void *data)
+{
+    const struct ovn_unsynced_datapath_map *map =
+        engine_get_input_data("datapath_logical_switch", node);
+    const struct engine_context *eng_ctx = engine_get_context();
+    const struct ed_type_global_config *global_config =
+        engine_get_input_data("global_config", node);
+
+    return datapath_sync_unsynced_datapath_handler(map, global_config,
+                                                   eng_ctx->ovnsb_idl_txn,
+                                                   data);
+}
+
+enum engine_input_handler_result
+datapath_sync_logical_router_handler(struct engine_node *node, void *data)
+{
+    const struct ovn_unsynced_datapath_map *map =
+        engine_get_input_data("datapath_logical_router", node);
+    const struct engine_context *eng_ctx = engine_get_context();
+    const struct ed_type_global_config *global_config =
+        engine_get_input_data("global_config", node);
+
+    return datapath_sync_unsynced_datapath_handler(map, global_config,
+                                                   eng_ctx->ovnsb_idl_txn,
+                                                   data);
+}
+
+enum engine_input_handler_result
+datapath_sync_sb_datapath_binding(struct engine_node *node, void *data)
+{
+    const struct sbrec_datapath_binding_table *sb_dp_table =
+        EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
+    enum engine_input_handler_result ret = EN_HANDLED_UNCHANGED;
+    struct ovn_synced_datapaths *synced_datapaths = data;
+
+    const struct sbrec_datapath_binding *sb_dp;
+    SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sb_dp, sb_dp_table) {
+        struct ovn_synced_datapath *sdp =
+            find_synced_datapath_from_sb(&synced_datapaths->synced_dps, sb_dp);
+        if (sbrec_datapath_binding_is_deleted(sb_dp)) {
+            if (sdp) {
+                /* The SB datapath binding was deleted, but we still have a
+                 * record of it locally. This implies the SB datapath binding
+                 * was deleted by something other than ovn-northd. We need
+                 * to recompute in this case.
+                 */
+                return EN_UNHANDLED;
+            }
+            continue;
+        }
+
+        if (sbrec_datapath_binding_is_new(sb_dp)) {
+            if (sdp) {
+                /* There is a new datapath binding, but we already have
+                 * a non-pending synced datapath. This indicates that
+                 * something other than ovn-northd added this datapath
+                 * binding to the database, and we need to recompute.
+                 */
+                return EN_UNHANDLED;
+            } else {
+                sdp = find_synced_datapath_from_sb(
+                    &synced_datapaths->pending_dps, sb_dp);
+                if (!sdp) {
+                    return EN_UNHANDLED;
+                }
+                sdp->sb_dp = sb_dp;
+                hmap_remove(&synced_datapaths->pending_dps, &sdp->hmap_node);
+                hmap_insert(&synced_datapaths->synced_dps, &sdp->hmap_node,
+                            sdp->hmap_node.hash);
+                hmapx_add(&synced_datapaths->new, sdp);
+                ret = EN_HANDLED_UPDATED;
+            }
+            continue;
+        }
+
+        /* Datapath binding is updated. This happens if the northbound
+         * logical datapath is updated and we make changes to the existing
+         * southbound datapath binding. In this case, the handler for the
+         * unsynced datapath will add sdp to synced_datapaths->updated, so
+         * we don't need to do anything here.
+         */
+    }
+
+    return ret;
+}
+
+void
+en_datapath_sync_clear_tracked_data(void *data)
+{
+    struct ovn_synced_datapaths *synced_datapaths = data;
+
+    clear_tracked_data(synced_datapaths);
+}
+
  enum engine_node_state
  en_datapath_sync_run(struct engine_node *node , void *data)
  {
@@ -304,7 +582,11 @@ en_datapath_sync_run(struct engine_node *node , void *data)
      delete_unassigned_candidates(&candidate_sdps);
      vector_destroy(&candidate_sdps);
- return EN_UPDATED;
+    if (hmap_count(&synced_datapaths->synced_dps)) {
+        return EN_UPDATED;
+    } else {
+        return EN_UPDATED;
+    }
  }
void en_datapath_sync_cleanup(void *data)
@@ -312,9 +594,21 @@ void en_datapath_sync_cleanup(void *data)
      struct ovn_synced_datapaths *synced_datapaths = data;
      struct ovn_synced_datapath *sdp;
+ hmapx_destroy(&synced_datapaths->new);
+    hmapx_destroy(&synced_datapaths->updated);
+    struct hmapx_node *node;
+    HMAPX_FOR_EACH_SAFE (node, &synced_datapaths->deleted) {
+        free(node->data);
+    }
+    hmapx_destroy(&synced_datapaths->deleted);
+
      HMAP_FOR_EACH_POP (sdp, hmap_node, &synced_datapaths->synced_dps) {
          free(sdp);
      }
+    HMAP_FOR_EACH_POP (sdp, hmap_node, &synced_datapaths->pending_dps) {
+        free(sdp);
+    }
      hmap_destroy(&synced_datapaths->synced_dps);
+    hmap_destroy(&synced_datapaths->pending_dps);
      ovn_destroy_tnlids(&synced_datapaths->dp_tnlids);
  }
diff --git a/northd/en-datapath-sync.h b/northd/en-datapath-sync.h
index 3b3262304..bed2d5a7b 100644
--- a/northd/en-datapath-sync.h
+++ b/northd/en-datapath-sync.h
@@ -21,7 +21,14 @@
void *en_datapath_sync_init(struct engine_node *,
                              struct engine_arg *);
+enum engine_input_handler_result
+datapath_sync_logical_switch_handler(struct engine_node *, void *data);
+enum engine_input_handler_result
+datapath_sync_logical_router_handler(struct engine_node *, void *data);
+enum engine_input_handler_result
+datapath_sync_sb_datapath_binding(struct engine_node *, void *data);
  enum engine_node_state en_datapath_sync_run(struct engine_node *, void *data);
  void en_datapath_sync_cleanup(void *data);
+void en_datapath_sync_clear_tracked_data(void *data);
#endif /* EN_DATAPATH_SYNC_H */
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 76e35c97c..60f3f2ef9 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -81,8 +81,6 @@ northd_get_input_data(struct engine_node *node,
      input_data->nbrec_port_group_table =
          EN_OVSDB_GET(engine_get_input("NB_port_group", node));
- input_data->sbrec_datapath_binding_table =
-        EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
      input_data->sbrec_port_binding_table =
          EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
      input_data->sbrec_mac_binding_table =
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 0dfbf00b5..166c0d814 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -184,7 +184,7 @@ static ENGINE_NODE(dynamic_routes);
  static ENGINE_NODE(group_ecmp_route, CLEAR_TRACKED_DATA);
  static ENGINE_NODE(datapath_logical_router, CLEAR_TRACKED_DATA);
  static ENGINE_NODE(datapath_logical_switch, CLEAR_TRACKED_DATA);
-static ENGINE_NODE(datapath_sync);
+static ENGINE_NODE(datapath_sync, CLEAR_TRACKED_DATA);
  static ENGINE_NODE(datapath_synced_logical_router);
  static ENGINE_NODE(datapath_synced_logical_switch);
@@ -225,10 +225,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
      engine_add_input(&en_datapath_logical_router, &en_nb_logical_router,
                       en_datapath_logical_router_logical_router_handler);
- engine_add_input(&en_datapath_sync, &en_datapath_logical_switch, NULL);
-    engine_add_input(&en_datapath_sync, &en_datapath_logical_router, NULL);
-    engine_add_input(&en_datapath_sync, &en_sb_datapath_binding, NULL);
      engine_add_input(&en_datapath_sync, &en_global_config, NULL);
+    engine_add_input(&en_datapath_sync, &en_sb_datapath_binding,
+                     datapath_sync_sb_datapath_binding);
+    engine_add_input(&en_datapath_sync, &en_datapath_logical_switch,
+                     datapath_sync_logical_switch_handler);
+    engine_add_input(&en_datapath_sync, &en_datapath_logical_router,
+                     datapath_sync_logical_router_handler);
engine_add_input(&en_datapath_synced_logical_router, &en_datapath_sync,
                       NULL);
@@ -243,7 +246,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
      engine_add_input(&en_northd, &en_sb_chassis, NULL);
      engine_add_input(&en_northd, &en_sb_mirror, NULL);
      engine_add_input(&en_northd, &en_sb_meter, NULL);
-    engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
      engine_add_input(&en_northd, &en_sb_dns, NULL);
      engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
      engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
diff --git a/northd/northd.h b/northd/northd.h
index 05591c07e..4c142cde0 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -42,7 +42,6 @@ struct northd_input {
      const struct nbrec_port_group_table *nbrec_port_group_table;
/* Southbound table references */
-    const struct sbrec_datapath_binding_table *sbrec_datapath_binding_table;
      const struct sbrec_port_binding_table *sbrec_port_binding_table;
      const struct sbrec_mac_binding_table *sbrec_mac_binding_table;
      const struct sbrec_ha_chassis_group_table *sbrec_ha_chassis_group_table;
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 76b59408c..d3fd924fb 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -17446,9 +17446,9 @@ check ovn-nbctl --wait=sb sync
  > northd/ovn-northd.log
  check as northd ovn-appctl -t ovn-northd vlog/set jsonrpc:dbg
  check as northd ovn-appctl -t ovn-northd vlog/set inc_proc_eng:dbg
-# Any change that invoke engine processing, for example add logical switch.
+# Any change that invoke engine processing, for example add address set.
  # nb_cfg bump must be present in order to get feedback as sb_cfg.
-check ovn-nbctl --wait=sb ls-add sw1
+check ovn-nbctl --wait=sb pg-add pg1
  #
  # Get following messages from log file:
  # 'unix... transact ... Southbound' --> Indicates the pack of updates has been
@@ -17806,3 +17806,43 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
  AT_CLEANUP
  ])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Datapath Sync incremental processing])
+ovn_start
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb ls-add sw0
+check_engine_stats datapath_sync norecompute compute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb set logical_switch sw0 
other_config:fdb_age_threshold=5
+check_engine_stats datapath_sync norecompute compute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb set logical_switch sw0 
other_config:requested-tnl-key=123
+check_engine_stats datapath_sync norecompute compute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb ls-del sw0
+check_engine_stats datapath_sync norecompute compute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb lr-add lr0
+check_engine_stats datapath_sync norecompute compute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl set Logical_Router lr0 options:ct-zone-limit=10
+check_engine_stats datapath_sync norecompute compute
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb lr-del lr0
+check_engine_stats datapath_sync norecompute compute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AT_CLEANUP
+])

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

Reply via email to