On Tue, Oct 7, 2025 at 10:12 PM Jacob Tanenbaum via dev < [email protected]> wrote:
> Adding sync_to_sb_pb_lr_stateful_handler() allows port_bindings to be > computed in the incremental processor instead of always fully > recalculating when the lr_stateful engine is updated. > > The process is taking the updated lr_stateful_record, getting the > logical router it references and syncing the routers ports and peers to > those router ports. > > Signed-off-by: Jacob Tanenbaum <[email protected]> > Hi Jacob, thank you for the patch, it is missing the Fixes tag and I have a couple of comments down below. > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 198881e20..f0110d155 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -422,6 +422,28 @@ sync_to_sb_pb_northd_handler(struct engine_node > *node, void *data OVS_UNUSED) > return EN_HANDLED_UPDATED; > } > > +enum engine_input_handler_result > +sync_to_sb_pb_lr_stateful_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + struct ed_type_lr_stateful *lr_stateful_data = > + engine_get_input_data("lr_stateful", node); > + struct northd_data *northd_data = engine_get_input_data("northd", > node); > nit: Please add an empty line. > + const struct ovn_datapaths lr_datapaths = northd_data->lr_datapaths; > This should be a pointer instead of copying the whole struct. > + struct hmapx_node *hmapx_node; > nit: Please move this right above the loop. > + const struct lr_stateful_record *lr_stateful_rec; > nit: This can be inside the loop. > + HMAPX_FOR_EACH (hmapx_node, &lr_stateful_data->trk_data.crupdated) { > + lr_stateful_rec = hmapx_node->data; > + const struct ovn_datapath *od = > + ovn_datapaths_find_by_index(&lr_datapaths, > + lr_stateful_rec->lr_index); > + sync_pbs_for_lr_stateful_changes(od, > + &lr_stateful_data->table); > + } > + > + return EN_HANDLED_UPDATED; > +} > + > /* static functions. */ > static void > sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h > index 21061a5b4..67768fe0c 100644 > --- a/northd/en-sync-sb.h > +++ b/northd/en-sync-sb.h > @@ -31,5 +31,7 @@ enum engine_node_state en_sync_to_sb_pb_run(struct > engine_node *, void *data); > void en_sync_to_sb_pb_cleanup(void *data); > enum engine_input_handler_result > sync_to_sb_pb_northd_handler(struct engine_node *, void *data OVS_UNUSED); > +enum engine_input_handler_result > +sync_to_sb_pb_lr_stateful_handler(struct engine_node *, void *data > OVS_UNUSED); > > #endif /* end of EN_SYNC_SB_H */ > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index ff9515be7..fdea550d7 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -431,7 +431,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(&en_sync_to_sb_pb, &en_northd, > sync_to_sb_pb_northd_handler); > - engine_add_input(&en_sync_to_sb_pb, &en_lr_stateful, NULL); > + engine_add_input(&en_sync_to_sb_pb, &en_lr_stateful, > + sync_to_sb_pb_lr_stateful_handler); > > /* en_sync_to_sb engine node syncs the SB database tables from > * the NB database tables. > diff --git a/northd/northd.c b/northd/northd.c > index e6062979e..e687338a7 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3941,6 +3941,22 @@ sync_pbs_for_northd_changed_ovn_ports( > } > } > > +void > +sync_pbs_for_lr_stateful_changes(const struct ovn_datapath *od, > + const struct lr_stateful_table > *lr_stateful) > +{ > + struct ovn_port *op; > + HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { > nit: No need for _SAFE, nothing is deleted from the hmap. > + sync_pb_for_lrp(op, lr_stateful); > + if (op->peer && op->peer->nbsp != NULL) { > + sync_pb_for_lsp(op->peer, lr_stateful); > + } > + if (op->cr_port) { > + sync_pb_for_lrp(op->cr_port, lr_stateful); > + } > nit: I would put spaces before each if. > + } > +} > + > static bool > ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > { > diff --git a/northd/northd.h b/northd/northd.h > index a9c971aed..14639936d 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -992,6 +992,10 @@ void sync_pbs_for_northd_changed_ovn_ports( > struct tracked_ovn_ports *, > const struct lr_stateful_table *); > > +void > +sync_pbs_for_lr_stateful_changes(const struct ovn_datapath *od, > + const struct lr_stateful_table > *lr_stateful); > + > static inline bool > northd_has_tracked_data(struct northd_tracked_data *trk_nd_changes) { > return trk_nd_changes->type != NORTHD_TRACKED_NONE; > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index b1aee0008..07fb57bd6 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12609,7 +12609,7 @@ check ovn-nbctl --wait=sb lr-add lr0 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -12619,7 +12619,7 @@ check ovn-nbctl --wait=sb lr-del lr0 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -12687,7 +12687,7 @@ check ovn-nbctl --wait=sb lr-nat-add lr0 > dnat_and_snat 172.168.0.110 10.0.0.4 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12697,7 +12697,7 @@ check ovn-nbctl --wait=sb set NAT . options:foo=bar > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12708,7 +12708,7 @@ check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12719,7 +12719,7 @@ check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12730,7 +12730,7 @@ check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12741,7 +12741,7 @@ check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12753,7 +12753,7 @@ check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -12770,7 +12770,7 @@ check ovn-nbctl --wait=sb lr-nat-add lr0 > dnat_and_snat 172.168.0.140 10.0.0.20 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -12782,7 +12782,7 @@ check ovn-nbctl --wait=sb lr-nat-add lr0 > dnat_and_snat 172.168.0.150 10.0.0.41 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -12794,7 +12794,7 @@ check ovn-nbctl --wait=sb lr-nat-del lr0 > dnat_and_snat 172.168.0.150 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -12806,7 +12806,7 @@ check ovn-nbctl --wait=sb lr-nat-del lr0 > dnat_and_snat 172.168.0.140 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -12818,7 +12818,7 @@ check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > check_engine_stats lr_stateful norecompute compute > check_engine_stats lflow norecompute compute > -check_engine_stats sync_to_sb_pb recompute nocompute > +check_engine_stats sync_to_sb_pb norecompute compute > check_engine_stats sync_to_sb_lb norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > -- > 2.51.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
