Hi Paulo, The en_dp_enum engine node is a bit of a problem. It appears to do two things: 1. It places all SB Datapath_Bindings into a single shash, with the key varying depending on whether the SB Datapath binding is a router or switch. 2. It gathers all of the allocated tunnel keys on the SB Datapath_Bindings into an hmap.
Operation (1) is not especially useful since the consumers of en_dp_enum are en_ts and en_tr. en_ts only cares about the switches, and en_tr only cares about the routers. Combining the two types into one shash doesn't really make sense if the consumers are always going to filter by type. In addition, you currently have to clone the shash at the beginning of en_ts and en_tr's operations so that they only modify their local copy of the shash instead of modifying the contents of en_dp_enum. Even with this safeguard in place, en_dp_enum can end up with dangling pointers if en_ts or en_tr deletes an SB datapath binding. These dangling pointers shouldn't result in any crashes or anything, but it's still not great that they exist. One option would be to move the shash creation into en_ts and en_tr directly. At the beginning of their run, en_ts can create the shash of switches, and en_tr can create the shash of routers. Then each has the freedom to manipulate their shash as they wish since it is their own local data. Operation (2) basically has the same issue as operation (1) with regards to mutability of the input data. The problem here is that en_ts and en_tr both make modifications to the same set of data. Tunnel keys have to be unique amongst all datapath types, so they can't just make their own local copies and operate on those independently. Without completely overhauling how datapaths are synced in ovn-ic, I think the best way to address this is to have en_tr and en_ts operate as the currently do, except that they do not do any sort of tunnel key allocation or manipulation. After those engine nodes have run, a new en_tunnel_key node can take care of syncing tunnel keys for all datapath types. With the above two options addressed, it means that en_dp_enum no longer needs to exist. I made notes below about the en_dp_enum code as it exists, but if we remove en_dp_enum from the code, then you can ignore the comments below. On Tue, Jul 21, 2026 at 1:44 PM Paulo Guilherme Silva <[email protected]> wrote: > > Add the incremental change handler for the en_dp_enum node so an IC-SB > Datapath_Binding change updates the datapath maps in place instead of > forcing a full recompute, and stop recomputing en_dp_enum on transit > switch/router (IC-NB) changes, which do not affect the datapath maps. > > Track the datapath bindings from the locked IC-SB IDL (bridging the > unlocked tracked changes by _uuid) and tolerate duplicate IC-SB datapath > bindings, so a stale/duplicate binding never leaves a dangling map entry > that the GC would double-delete. > > Assisted-by: Claude Opus 4.8, Claude Code > Co-authored-by: Tiago Matos <[email protected]> > Signed-off-by: Tiago Matos <[email protected]> > Signed-off-by: Paulo Guilherme Silva <[email protected]> > --- > ic/en-dp-enum.c | 189 +++++++++++++++++++++++++++++++++++++++++++++++ > ic/en-dp-enum.h | 15 ++++ > ic/en-tr.c | 8 +- > ic/en-ts.c | 8 +- > ic/inc-proc-ic.c | 39 +++++++--- > ic/ovn-ic.c | 21 +----- > ic/ovn-ic.h | 9 +-- > 7 files changed, 251 insertions(+), 38 deletions(-) > > diff --git a/ic/en-dp-enum.c b/ic/en-dp-enum.c > index fd447ab52..1ec9303e1 100644 > --- a/ic/en-dp-enum.c > +++ b/ic/en-dp-enum.c > @@ -16,14 +16,38 @@ > > #include "en-dp-enum.h" > #include "lib/inc-proc-eng.h" > +#include "lib/ovn-ic-nb-idl.h" > +#include "lib/ovn-ic-sb-idl.h" > #include "lib/ovn-util.h" > #include "openvswitch/hmap.h" > #include "openvswitch/shash.h" > #include "openvswitch/vlog.h" > #include "ovn-ic.h" > +#include "ovsdb-idl.h" > +#include "uuid.h" > > VLOG_DEFINE_THIS_MODULE(en_ic_dp_enum); > > +static void > +enumerate_datapaths(struct ic_context *ctx, struct hmap *dp_tnlids, > + struct shash *isb_ts_dps, struct shash *isb_tr_dps) > +{ > + const struct icsbrec_datapath_binding *isb_dp; > + ICSBREC_DATAPATH_BINDING_FOR_EACH (isb_dp, ctx->ovnisb_idl) { > + ovn_add_tnlid(dp_tnlids, isb_dp->tunnel_key); > + > + enum ic_datapath_type dp_type = ic_dp_get_type(isb_dp); > + if (dp_type == IC_ROUTER) { > + char *uuid_str = uuid_to_string(isb_dp->nb_ic_uuid); > + shash_add(isb_tr_dps, uuid_str, isb_dp); > + free(uuid_str); > + } else { > + shash_add(isb_ts_dps, isb_dp->transit_switch, isb_dp); > + } > + } > +} > + > + > enum engine_node_state > en_dp_enum_run(struct engine_node *node OVS_UNUSED, void *data_) > { > @@ -44,6 +68,171 @@ en_dp_enum_run(struct engine_node *node OVS_UNUSED, void > *data_) > return EN_UPDATED; > } > > +/* Adds 'isb_dp_tracked' to the datapath maps and the tunnel-key allocator. > + * > + * 'isb_dp_tracked' is a tracked change from the *unlocked* IC-SB IDL: that > IDL > + * is the passive observer that sees every change, including ovn-ic's own > + * locked-transaction writes (which the locked IDL does not re-track), so it > is > + * the right source for change detection. But the maps must hold rows from > the > + * *locked* IDL, because enumerate_datapaths() (the recompute path) builds > them > + * from ctx->ovnisb_idl and ts_sync_scope() deletes datapath bindings via the > + * locked transaction; a mix of locked and unlocked row objects breaks > + * dp_enum_remove()'s identity check and left a dangling map entry that > + * ts_sync_scope()'s GC double-deleted. Bridge the two IDLs by the stable > + * _uuid: store the locked-IDL row for the same DB row. > + * > + * The maps are keyed by transit-switch name / IC-NB uuid. Duplicate IC-SB > + * Datapath_Bindings for the same key can appear transiently (e.g. leader > + * races during instability), so use shash_replace() rather than shash_add(): > + * this keeps at most one node per key, so the map never holds more than one > + * pointer for a key. */ > +static bool > +dp_enum_add(struct ed_type_dp_enum *data, struct ic_context *ctx, > + const struct icsbrec_datapath_binding *isb_dp_tracked) > +{ > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + > + const struct icsbrec_datapath_binding *isb_dp = > + icsbrec_datapath_binding_get_for_uuid(ctx->ovnisb_idl, > + &isb_dp_tracked->header_.uuid); > + if (!isb_dp) { > + /* The locked IDL has not caught up with this row yet (e.g. its > + * connection is mid-reconnect, or IC-SB is transiently paused). We > + * cannot store a consistent locked-IDL row, so signal the caller to > + * fall back to a full recompute rather than leave the map missing > this > + * entry (which would desync en_ts/en_tr and duplicate NB ports). */ > + return false; > + } > + > + ovn_add_tnlid(&data->dp_tnlids, isb_dp->tunnel_key); > + > + if (ic_dp_get_type(isb_dp) == IC_ROUTER) { > + char *uuid_str = uuid_to_string(isb_dp->nb_ic_uuid); > + if (shash_replace(&data->isb_tr_dps, uuid_str, isb_dp)) { I don't think shash_replace() is correct here. In enumerate_datapaths(), we use shash_add() when adding to the shash. The idea is that the database schema should prevent any duplicate entries from being added. When processing incrementally, you remove deleted datapaths first, and then add new ones after. Therefore, the database schema should still prevent any duplicate keys from appearing. I think using shash_add() here should work just fine. > + VLOG_WARN_RL(&rl, "duplicate IC-SB transit-router datapath " > + "binding for %s", uuid_str); > + } > + free(uuid_str); > + } else { > + if (shash_replace(&data->isb_ts_dps, isb_dp->transit_switch, > isb_dp)) { > + VLOG_WARN_RL(&rl, "duplicate IC-SB transit-switch datapath " > + "binding for %s", isb_dp->transit_switch); > + } > + } > + return true; > +} > + > +/* Removes the deleted 'isb_dp_tracked' (a tracked change from the unlocked > + * IC-SB IDL) from the datapath maps and the tunnel-key allocator. > + * > + * The maps store locked-IDL rows (see dp_enum_add()), so match by the stable > + * _uuid rather than the pointer: remove the map node only when the stored > + * (locked) row is the SAME DB row as the deleted (tracked) one. With > + * duplicate bindings the node may already have been replaced by > dp_enum_add() > + * with a different (live) row, so removing by name alone could drop that > live > + * entry; the uuid guard keeps the map consistent and never dangling. */ > +static void > +dp_enum_remove(struct ed_type_dp_enum *data, > + const struct icsbrec_datapath_binding *isb_dp_tracked) > +{ > + ovn_free_tnlid(&data->dp_tnlids, isb_dp_tracked->tunnel_key); > + > + struct shash *map; > + char *uuid_str = NULL; > + const char *key; > + if (ic_dp_get_type(isb_dp_tracked) == IC_ROUTER) { > + uuid_str = uuid_to_string(isb_dp_tracked->nb_ic_uuid); > + map = &data->isb_tr_dps; > + key = uuid_str; > + } else { > + map = &data->isb_ts_dps; > + key = isb_dp_tracked->transit_switch; > + } > + > + struct shash_node *node = shash_find(map, key); > + if (node) { > + const struct icsbrec_datapath_binding *stored = node->data; > + if (uuid_equals(&stored->header_.uuid, > + &isb_dp_tracked->header_.uuid)) { I don't think the uuid_equals() check is necessary here. If finding an item by key is not enough to guarantee finding the correct item, then it means the key should be changed for the shash. In this case, I think the strings used as keys should be fine. You can replace all of this code with a call to shash_find_and_delete() instead. If you actually have found instances where the UUID check was necessary, then change to an hmap based on the SB IC Datapath_Binding's UUID. > + shash_delete(map, node); > + } > + } > + free(uuid_str); > +} > + > +/* Incrementally maintains the datapath maps from tracked IC-SB > + * Datapath_Binding changes. Inserts and deletes are handled in place. A > + * modification that touches the tunnel key or either map key cannot be fixed > + * up incrementally (the tracked row only exposes the new values, so the old > + * key needed to remove the stale entry is unavailable); in that case we fall > + * back to a full recompute. */ > +enum engine_input_handler_result > +en_dp_enum_icsb_datapath_binding_handler(struct engine_node *node, void > *data_) > +{ > + struct ed_type_dp_enum *data = data_; > + struct ic_context *ctx = engine_get_context()->client_ctx; > + const struct icsbrec_datapath_binding_table *tbl = > + EN_OVSDB_GET(engine_get_input("ICSB_datapath_binding", node)); > + const struct icsbrec_datapath_binding *isb_dp; > + bool updated = false; nit: Instead of this updated boolean, you could have a local variable of type enum engine_input_handler_result. Initialize it to EN_HANDLED_UNCHANGED. Then anywhere where you have been setting updated = true, instead set the new variable to EN_HANDLED_UPDATED. Then you can just return the value of this variable at the end of the function. This makes a more clearer distinction of what causes the return value to be what it is. > + > + ICSBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (isb_dp, tbl) { > + if (icsbrec_datapath_binding_is_deleted(isb_dp)) { > + dp_enum_remove(data, isb_dp); > + updated = true; > + } else if (icsbrec_datapath_binding_is_new(isb_dp)) { > + if (!dp_enum_add(data, ctx, isb_dp)) { > + return EN_UNHANDLED; > + } > + updated = true; > + } else if (ovsdb_idl_track_is_updated(&isb_dp->header_, > + &icsbrec_datapath_binding_col_tunnel_key) || > + ovsdb_idl_track_is_updated(&isb_dp->header_, > + &icsbrec_datapath_binding_col_transit_switch) || > + ovsdb_idl_track_is_updated(&isb_dp->header_, > + &icsbrec_datapath_binding_col_nb_ic_uuid)) { > + return EN_UNHANDLED; > + } > + } > + > + return updated ? EN_HANDLED_UPDATED : EN_HANDLED_UNCHANGED; > +} > + > +/* Shared change handler for the IC-NB Global input of nodes that only care > + * about its 'options' (e.g. vxlan_mode), not the nb_ic_cfg/sb_ic_cfg > sequence > + * numbers that the sequence-number protocol bumps on every change. Returns > + * EN_UNHANDLED (forcing a recompute) only when the options change or the row > + * is created/deleted; sequence-number-only updates are a no-op. */ > +enum engine_input_handler_result > +en_ic_nb_global_handler(struct engine_node *node, void *data OVS_UNUSED) Please change the name of this function to either en_dp_enum_nb_global_handler() or dp_enum_nb_global_handler(). Prefixing the function with "en_ic" makes it seem like it's a handler run by the en_ic node, not en_dp_enum. > +{ > + const struct icnbrec_ic_nb_global_table *tbl = > + EN_OVSDB_GET(engine_get_input("ICNB_ic_nb_global", node)); > + const struct icnbrec_ic_nb_global *icnb; > + > + ICNBREC_IC_NB_GLOBAL_TABLE_FOR_EACH_TRACKED (icnb, tbl) { > + if (icnbrec_ic_nb_global_is_new(icnb) || > + icnbrec_ic_nb_global_is_deleted(icnb) || > + ovsdb_idl_track_is_updated(&icnb->header_, > + &icnbrec_ic_nb_global_col_options)) { > + return EN_UNHANDLED; > + } > + } > + > + return EN_HANDLED_UNCHANGED; > +} > + > +void > +dp_enum_shash_clone(struct shash *dst, const struct shash *src) > +{ > + const struct shash_node *node; > + > + shash_init(dst); > + SHASH_FOR_EACH (node, src) { > + shash_add(dst, node->name, node->data); > + } > +} > + > void * > en_dp_enum_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > diff --git a/ic/en-dp-enum.h b/ic/en-dp-enum.h > index b843400e5..645ca011c 100644 > --- a/ic/en-dp-enum.h > +++ b/ic/en-dp-enum.h > @@ -29,4 +29,19 @@ enum engine_node_state en_dp_enum_run(struct engine_node > *node, void *data); > void *en_dp_enum_init(struct engine_node *node, struct engine_arg *arg); > void en_dp_enum_cleanup(void *data); > > +enum engine_input_handler_result > +en_dp_enum_icsb_datapath_binding_handler(struct engine_node *node, void > *data); > + > +/* Shared handler for the IC-NB Global input: ignores nb_ic_cfg/sb_ic_cfg > + * sequence-number-only changes, recomputes on options (vxlan_mode) changes. > */ > +enum engine_input_handler_result > +en_ic_nb_global_handler(struct engine_node *node, void *data); > + > +/* Shallow-copies the datapath shash 'src' into 'dst' (initialized here). > The > + * values (IDL row pointers) are shared, not duplicated. en_ts and en_tr > + * destructively consume their datapath shash (removing matched entries and > + * deleting the leftovers' datapaths), so they must operate on a copy to keep > + * en_dp_enum's authoritative maps intact across iterations. */ > +void dp_enum_shash_clone(struct shash *dst, const struct shash *src); > + > #endif /* EN_IC_DP_ENUM_H */ > diff --git a/ic/en-tr.c b/ic/en-tr.c > index 8b46f89e6..524fe6ad6 100644 > --- a/ic/en-tr.c > +++ b/ic/en-tr.c > @@ -35,7 +35,13 @@ en_tr_run(struct engine_node *node, void *data OVS_UNUSED) > return EN_UNCHANGED; > } > > - tr_run(ctx, &dp->dp_tnlids, &dp->isb_tr_dps); > + /* tr_run() destructively consumes the transit-router datapath shash, so > + * pass it a copy to keep en_dp_enum's authoritative map intact. The > + * shared tunnel-key allocator (dp_tnlids) is passed directly. */ > + struct shash isb_tr_dps; > + dp_enum_shash_clone(&isb_tr_dps, &dp->isb_tr_dps); > + tr_run(ctx, &dp->dp_tnlids, &isb_tr_dps); > + shash_destroy(&isb_tr_dps); > > return EN_UPDATED; > } > diff --git a/ic/en-ts.c b/ic/en-ts.c > index 84e37a30a..8745e6aa4 100644 > --- a/ic/en-ts.c > +++ b/ic/en-ts.c > @@ -35,7 +35,13 @@ en_ts_run(struct engine_node *node, void *data OVS_UNUSED) > return EN_UNCHANGED; > } > > - ts_run(ctx, &dp->dp_tnlids, &dp->isb_ts_dps); > + /* ts_run() destructively consumes the transit-switch datapath shash, so > + * pass it a copy to keep en_dp_enum's authoritative map intact. The > + * shared tunnel-key allocator (dp_tnlids) is passed directly. */ > + struct shash isb_ts_dps; > + dp_enum_shash_clone(&isb_ts_dps, &dp->isb_ts_dps); > + ts_run(ctx, &dp->dp_tnlids, &isb_ts_dps); > + shash_destroy(&isb_ts_dps); > > return EN_UPDATED; > } > diff --git a/ic/inc-proc-ic.c b/ic/inc-proc-ic.c > index 9bde8f1f8..3528542a6 100644 > --- a/ic/inc-proc-ic.c > +++ b/ic/inc-proc-ic.c > @@ -204,17 +204,34 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, > /* en_dp_enum: enumerate IC-SB datapath bindings (tunnel-key allocator > and > * transit switch/router datapath maps shared by en_ts and en_tr). > * > - * en_ts and en_tr allocate datapath tunnel keys from the shared > - * 'dp_tnlids' set owned by this node, mutating it during their run. To > - * keep that allocator correct, en_dp_enum must rebuild it from scratch > in > - * the same iteration as any allocation. It therefore depends not only > on > - * the IC-SB datapath bindings themselves, but also on every input that > can > - * cause en_ts/en_tr to allocate a key: a new transit switch or router, > or > - * a change of vxlan_mode (which forces a tunnel-key refresh). */ > - engine_add_input(&en_dp_enum, &en_icsb_datapath_binding, NULL); > - engine_add_input(&en_dp_enum, &en_icnb_transit_switch, NULL); > - engine_add_input(&en_dp_enum, &en_icnb_transit_router, NULL); > - engine_add_input(&en_dp_enum, &en_icnb_ic_nb_global, NULL); > + * The node's state (dp_tnlids, isb_ts_dps, isb_tr_dps) is derived purely > + * from the IC-SB Datapath_Binding rows and is maintained incrementally > by > + * the en_icsb_datapath_binding handler (insert/delete in place; a tunnel > + * key/transit-switch/nb_ic_uuid modify falls back to a recompute). > + * > + * The IC-NB transit switch/router inputs are ordering dependencies only: > + * en_ts and en_tr (downstream of en_dp_enum) allocate datapath tunnel > keys > + * from the shared 'dp_tnlids' set during their own scoped handlers. > That > + * allocation needs no en_dp_enum rebuild - ovn_allocate_tnlid() reserves > + * the key in the live 'dp_tnlids' immediately (so concurrent allocations > + * in the same iteration cannot collide), ovn_add_tnlid() is idempotent > + * (so the committed binding re-arriving as an insert is a no-op), and > the > + * binding's insert/delete is reconciled by the en_icsb_datapath_binding > + * handler on a later iteration. A transit switch/router add or delete > + * therefore does not require re-enumerating every datapath binding, so > + * these edges use a no-op handler instead of forcing a full recompute > + * - which matters at scale (tens of thousands of transit switches). The > + * only IC-NB Global change that affects the allocator is vxlan_mode (a > + * global tunnel-key refresh), which still forces a recompute via > + * en_ic_nb_global_handler. */ > + engine_add_input(&en_dp_enum, &en_icsb_datapath_binding, > + en_dp_enum_icsb_datapath_binding_handler); > + engine_add_input(&en_dp_enum, &en_icnb_transit_switch, > + engine_noop_handler); > + engine_add_input(&en_dp_enum, &en_icnb_transit_router, > + engine_noop_handler); > + engine_add_input(&en_dp_enum, &en_icnb_ic_nb_global, > + en_ic_nb_global_handler); > > /* en_gateway: sync gateways/chassis between SB and IC-SB. */ > engine_add_input(&en_gateway, &en_az, NULL); > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index 3970974bc..428fe7cab 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -109,7 +109,7 @@ allocate_dp_key(struct hmap *dp_tnlids, bool vxlan_mode, > const char *name) > &hint); > } > > -static enum ic_datapath_type > +enum ic_datapath_type > ic_dp_get_type(const struct icsbrec_datapath_binding *isb_dp) > { > if (isb_dp->type && !strcmp(isb_dp->type, "transit-router")) { > @@ -129,25 +129,6 @@ ic_pb_get_type(const struct icsbrec_port_binding *isb_pb) > return IC_SWITCH_PORT; > } > > -void > -enumerate_datapaths(struct ic_context *ctx, struct hmap *dp_tnlids, > - struct shash *isb_ts_dps, struct shash *isb_tr_dps) > -{ > - const struct icsbrec_datapath_binding *isb_dp; > - ICSBREC_DATAPATH_BINDING_FOR_EACH (isb_dp, ctx->ovnisb_idl) { > - ovn_add_tnlid(dp_tnlids, isb_dp->tunnel_key); > - > - enum ic_datapath_type dp_type = ic_dp_get_type(isb_dp); > - if (dp_type == IC_ROUTER) { > - char *uuid_str = uuid_to_string(isb_dp->nb_ic_uuid); > - shash_add(isb_tr_dps, uuid_str, isb_dp); > - free(uuid_str); > - } else { > - shash_add(isb_ts_dps, isb_dp->transit_switch, isb_dp); > - } > - } > -} > - > /* > * Check if the AZ is the leader by checking the lock. > */ > diff --git a/ic/ovn-ic.h b/ic/ovn-ic.h > index b06725373..202f160c0 100644 > --- a/ic/ovn-ic.h > +++ b/ic/ovn-ic.h > @@ -63,12 +63,11 @@ enum ic_port_binding_type { IC_SWITCH_PORT, > IC_ROUTER_PORT, IC_PORT_MAX }; > > struct hmap; > struct shash; > +struct icsbrec_datapath_binding; > + > +enum ic_datapath_type ic_dp_get_type( > + const struct icsbrec_datapath_binding *isb_dp); > > -/* Per-subsystem entry points, invoked by the incremental-processing engine > - * nodes (see ic/en-*.c). Each performs a full recompute of its subsystem > and > - * may be invoked independently when its engine inputs change. */ > -void enumerate_datapaths(struct ic_context *ctx, struct hmap *dp_tnlids, > - struct shash *isb_ts_dps, struct shash *isb_tr_dps); > void gateway_run(struct ic_context *ctx); > void address_set_run(struct ic_context *ctx); > void ts_run(struct ic_context *ctx, struct hmap *dp_tnlids, > -- > 2.34.1 > > > -- > > > > > _'Esta mensagem é direcionada apenas para os endereços constantes no > cabeçalho inicial. Se você não está listado nos endereços constantes no > cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa > mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão > imediatamente anuladas e proibidas'._ > > > * **'Apesar do Magazine Luiza tomar > todas as precauções razoáveis para assegurar que nenhum vírus esteja > presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por > quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
