Hi Alexandra, I think this patch should be removed from the series. Saving engine input data directly on the engine data defeats the purpose of having the engine inputs in the first place. If engine node A has inputs X and Y, then A should always call engine_node_get_input_data() to get X and Y's data. Similarly, if engine node B needs data from nodes A and X, B needs to call engine_node_get_input_data() on both A and X rather than just calling it on A in order to get X's data. Generally speaking, the reason for this is to ensure that you're always getting up-to-date data from the input nodes. Caching data on intermediate nodes can lead to having out-of-date data on the intermediate nodes.
On Thu, Dec 18, 2025 at 7:32 AM Alexandra Rukomoinikova <[email protected]> wrote: > > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > --- > northd/en-sync-from-sb.c | 28 +++++++++++++++++++--------- > northd/en-sync-from-sb.h | 6 ++++++ > northd/northd.c | 29 +++++++++++++++++------------ > northd/northd.h | 7 +++---- > 4 files changed, 45 insertions(+), 25 deletions(-) > > diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c > index dde0e9f55..6d4ff3e39 100644 > --- a/northd/en-sync-from-sb.c > +++ b/northd/en-sync-from-sb.c > @@ -40,23 +40,33 @@ void * > en_sync_from_sb_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > { > - return NULL; > + struct en_sync_from_sb_data *data = xmalloc(sizeof *data); > + return data; > +} > + > +static void > +en_sync_from_sb_get_input_data(struct engine_node *node, > + struct en_sync_from_sb_data *data) > +{ > + data->sb_pb_table = > + EN_OVSDB_GET(engine_get_input("SB_port_binding", node)); > + data->sb_ha_ch_grp_table = > + EN_OVSDB_GET(engine_get_input("SB_ha_chassis_group", node)); > } > > enum engine_node_state > -en_sync_from_sb_run(struct engine_node *node, void *data OVS_UNUSED) > +en_sync_from_sb_run(struct engine_node *node, void *data_) > { > + struct en_sync_from_sb_data *data = > + (struct en_sync_from_sb_data *) data_; > const struct engine_context *eng_ctx = engine_get_context(); > - struct northd_data *nd = engine_get_input_data("northd", node); > + struct northd_data *northd_data = engine_get_input_data("northd", node); > + > + en_sync_from_sb_get_input_data(node, data); > > - const struct sbrec_port_binding_table *sb_pb_table = > - EN_OVSDB_GET(engine_get_input("SB_port_binding", node)); > - const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table = > - EN_OVSDB_GET(engine_get_input("SB_ha_chassis_group", node)); > stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); > ovnsb_db_run(eng_ctx->ovnsb_idl_txn, > - sb_pb_table, sb_ha_ch_grp_table, > - &nd->ls_ports, &nd->lr_ports); > + data, northd_data); > stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); > > return EN_UNCHANGED; > diff --git a/northd/en-sync-from-sb.h b/northd/en-sync-from-sb.h > index 0ad07853a..bea248c45 100644 > --- a/northd/en-sync-from-sb.h > +++ b/northd/en-sync-from-sb.h > @@ -3,6 +3,12 @@ > > #include "lib/inc-proc-eng.h" > > +struct en_sync_from_sb_data { > + /* Southbound table references */ > + const struct sbrec_port_binding_table *sb_pb_table; > + const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table; > +}; > + > void *en_sync_from_sb_init(struct engine_node *, struct engine_arg *); > enum engine_node_state en_sync_from_sb_run(struct engine_node *, void *data); > void en_sync_from_sb_cleanup(void *data); > diff --git a/northd/northd.c b/northd/northd.c > index 79bff21b4..f6411dd3b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -55,6 +55,7 @@ > #include "en-sampling-app.h" > #include "en-datapath-logical-switch.h" > #include "en-datapath-logical-router.h" > +#include "en-sync-from-sb.h" > #include "lib/ovn-parallel-hmap.h" > #include "ovn/actions.h" > #include "ovn/features.h" > @@ -2429,7 +2430,7 @@ op_get_name(const struct ovn_port *op) > } > > static void > -ovn_update_ipv6_prefix(struct hmap *lr_ports) > +ovn_update_ipv6_prefix(const struct hmap *lr_ports) > { > const struct ovn_port *op; > HMAP_FOR_EACH (op, key_node, lr_ports) { > @@ -20912,10 +20913,11 @@ handle_cr_port_binding_changes(const struct > sbrec_port_binding *sb, > * this column is not empty, it means we need to set the corresponding > logical > * port as 'up' in the northbound DB. */ > static void > -handle_port_binding_changes(const struct sbrec_port_binding_table > *sb_pb_table, > +handle_port_binding_changes( > + const struct sbrec_port_binding_table *sb_pb_table, > const struct sbrec_ha_chassis_group_table > *sb_ha_ch_grp_table, > - struct hmap *ls_ports, > - struct hmap *lr_ports, > + const struct hmap *ls_ports, > + const struct hmap *lr_ports, > struct shash *ha_ref_chassis_map) > { > struct hmapx lr_groups = HMAPX_INITIALIZER(&lr_groups); > @@ -20991,23 +20993,26 @@ handle_port_binding_changes(const struct > sbrec_port_binding_table *sb_pb_table, > /* Handle a fairly small set of changes in the southbound database. */ > void > ovnsb_db_run(struct ovsdb_idl_txn *ovnsb_txn, > - const struct sbrec_port_binding_table *sb_pb_table, > - const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table, > - struct hmap *ls_ports, > - struct hmap *lr_ports) > + const struct en_sync_from_sb_data *sync_from_sb_data, > + const struct northd_data *northd_data) > { > if (!ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) { > return; > } > > struct shash ha_ref_chassis_map = SHASH_INITIALIZER(&ha_ref_chassis_map); > - handle_port_binding_changes(sb_pb_table, sb_ha_ch_grp_table, > - ls_ports, lr_ports, &ha_ref_chassis_map); > - update_sb_ha_group_ref_chassis(sb_ha_ch_grp_table, &ha_ref_chassis_map); > + handle_port_binding_changes(sync_from_sb_data->sb_pb_table, > + sync_from_sb_data->sb_ha_ch_grp_table, > + &northd_data->ls_ports, > + &northd_data->lr_ports, > + &ha_ref_chassis_map); > + > + update_sb_ha_group_ref_chassis(sync_from_sb_data->sb_ha_ch_grp_table, > + &ha_ref_chassis_map); > > shash_destroy(&ha_ref_chassis_map); > > - ovn_update_ipv6_prefix(lr_ports); > + ovn_update_ipv6_prefix(&northd_data->lr_ports); > } > > const struct ovn_datapath * > diff --git a/northd/northd.h b/northd/northd.h > index bba3ee530..bb2196515 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -22,6 +22,7 @@ > #include "lib/sset.h" > #include "lib/hmapx.h" > #include "northd/en-port-group.h" > +#include "northd/en-sync-from-sb.h" > #include "northd/ipam.h" > #include "northd/lb.h" > #include "openvswitch/hmap.h" > @@ -896,10 +897,8 @@ void ovnnb_db_run(struct northd_input *input_data, > struct northd_data *data, > struct ovsdb_idl_txn *ovnsb_txn); > void ovnsb_db_run(struct ovsdb_idl_txn *ovnsb_txn, > - const struct sbrec_port_binding_table *, > - const struct sbrec_ha_chassis_group_table *, > - struct hmap *ls_ports, > - struct hmap *lr_ports); > + const struct en_sync_from_sb_data *sync_from_sb_data, > + const struct northd_data *northd_data); > bool northd_handle_ls_changes(struct ovsdb_idl_txn *, > const struct northd_input *, > struct northd_data *); > -- > 2.48.1 > > _______________________________________________ > 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
