On Tue, Oct 6, 2026 at 2:56 PM Mark Michelson via dev < [email protected]> wrote:
> Prior to this commit, any time that en-northd needed to recompute, it > would also trigger a recompute of en-route-policies. > > The vast majority of reasons that en-northd could recompute is > irrelevant to logical router policy processing. The only change that > actually makes a difference is if the logical router ports change, and > there are "reroute" policies installed on the logical router. > > In this commit, we now check for logical router port changes in our > handler for synced logical routers. If we detect port changes, then we > will rebuild the policies for the logical router incrementally. > > This allows us to remove the handler for en-northd entirely. Now, if > en-northd recomputes, en-route-policies does not care and likely can > incrementally handle whatever caused en-northd to recompute. > > Reported-at: https://redhat.atlassian.net/browse/FDP-3984 > Signed-off-by: Mark Michelson <[email protected]> > --- > northd/en-route-policies.c | 57 ++++++++++++++++++++------------ > northd/en-route-policies.h | 3 -- > northd/inc-proc-northd.c | 7 ++-- > tests/ovn-inc-proc-graph-dump.at | 2 +- > tests/ovn-northd.at | 22 ++++++++++-- > 5 files changed, 60 insertions(+), 31 deletions(-) > > diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c > index 4227a78dd..e8d95d5e2 100644 > --- a/northd/en-route-policies.c > +++ b/northd/en-route-policies.c > @@ -551,27 +551,6 @@ en_route_policies_cleanup(void *data) > route_policies_destroy(data); > } > > -enum engine_input_handler_result > -route_policies_northd_change_handler(struct engine_node *node, > - void *data OVS_UNUSED) > -{ > - struct northd_data *northd_data = engine_get_input_data("northd", > node); > - if (!northd_has_tracked_data(&northd_data->trk_data)) { > - return EN_UNHANDLED; > - } > - > - /* This node uses the below data from the en_northd engine node. > - * See (lr_stateful_get_input_data()) > - * 1. northd_data->lr_datapaths > - * This data gets updated when a logical router or logical > router port > - * is created or deleted. > - * Northd engine node presently falls back to full recompute when > - * this happens and so does this node. > - */ > - > - return EN_HANDLED_UNCHANGED; > -} > - > static bool > logical_router_policies_updated(const struct nbrec_logical_router *lr) > { > @@ -590,6 +569,39 @@ logical_router_policies_updated(const struct > nbrec_logical_router *lr) > return false; > } > > +static bool > +logical_router_ports_updated(const struct nbrec_logical_router *lr) > +{ > + /* We only care if ports change if the lr has policies with a > + * "reroute" action. Otherwise, we don't care about the ports. > + */ > + bool has_reroute = false; > + for (size_t i = 0; i < lr->n_policies; i++) { > + const struct nbrec_logical_router_policy *rule = lr->policies[i]; > + if (!strcmp(rule->action, "reroute")) { > + has_reroute = true; > + break; > + } > + } > + if (!has_reroute) { > + return false; > + } > + > + if (nbrec_logical_router_is_updated(lr, > + NBREC_LOGICAL_ROUTER_COL_PORTS)) { > + return true; > + } > + for (size_t i = 0; i < lr->n_ports; i++) { > + const struct nbrec_logical_router_port *lrp = lr->ports[i]; > + if (nbrec_logical_router_port_row_get_seqno( > + lrp, OVSDB_IDL_CHANGE_MODIFY) > 0) { > + return true; > + } > + } > + > + return false; > +} > + > enum engine_input_handler_result > route_policies_datapath_synced_logical_router_handler(struct engine_node > *node, > void *data) > @@ -668,7 +680,8 @@ > route_policies_datapath_synced_logical_router_handler(struct engine_node > *node, > HMAPX_FOR_EACH (lr_node, &synced_lrs->updated) { > const struct ovn_synced_logical_router *lr = lr_node->data; > > - if (!logical_router_policies_updated(lr->nb)) { > + if (!logical_router_policies_updated(lr->nb) && > + !logical_router_ports_updated(lr->nb)) { > continue; > } > struct ovn_datapath *od = > diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h > index e176cc3a0..2180bbc5a 100644 > --- a/northd/en-route-policies.h > +++ b/northd/en-route-policies.h > @@ -100,9 +100,6 @@ struct route_policies_data { > > void en_route_policies_cleanup(void *data); > enum engine_input_handler_result > -route_policies_northd_change_handler(struct engine_node *node, > - void *data OVS_UNUSED); > -enum engine_input_handler_result > route_policies_datapath_synced_logical_router_handler(struct engine_node > *node, > void *data > OVS_UNUSED); > enum engine_node_state en_route_policies_run(struct engine_node *node, > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 603590697..49348e42f 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -331,8 +331,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(&en_route_policies, > &en_datapath_synced_logical_router, > > route_policies_datapath_synced_logical_router_handler); > - engine_add_input(&en_route_policies, &en_northd, > - route_policies_northd_change_handler); > + /* en_route_policies uses data from en_northd, but it makes the > + * determination about whether to recompute based on synced logical > + * routers. > + */ > + engine_add_input(&en_route_policies, &en_northd, engine_noop_handler); > > engine_add_input(&en_routes, &en_northd, > routes_northd_change_handler); > diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ > ovn-inc-proc-graph-dump.at > index 8cd199b11..fe59e4e89 100644 > --- a/tests/ovn-inc-proc-graph-dump.at > +++ b/tests/ovn-inc-proc-graph-dump.at > @@ -152,7 +152,7 @@ digraph "Incremental-Processing-Engine" { > NB_logical_router_static_route -> routes > [[label="routes_static_route_change_handler"]]; > route_policies [[style=filled, shape=box, fillcolor=white, > label="route_policies"]]; > datapath_synced_logical_router -> route_policies > [[label="route_policies_datapath_synced_logical_router_handler"]]; > - northd -> route_policies > [[label="route_policies_northd_change_handler"]]; > + northd -> route_policies [[label="engine_noop_handler"]]; > bfd_sync [[style=filled, shape=box, fillcolor=white, > label="bfd_sync"]]; > NB_bfd -> bfd_sync [[label=""]]; > SB_bfd -> bfd_sync [[label=""]]; > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index ffc387aa5..42ee713db 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -16295,7 +16295,7 @@ check_engine_stats lflow recompute nocompute > # For the below engine nodes, en_northd is input. So check > # their engine status. > check_engine_stats lr_stateful recompute nocompute > -check_engine_stats route_policies recompute nocompute > +check_engine_stats route_policies norecompute compute > check_engine_stats routes recompute nocompute > check_engine_stats bfd_sync recompute nocompute > check_engine_stats sync_to_sb_lb recompute nocompute > @@ -24773,15 +24773,31 @@ check_engine_stats bfd_sync norecompute compute > check_engine_stats lflow norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > +# Add a port to lr1. This should trigger a recompute in northd, > +# but route_policies should be able to handle the change incrementally. > This is true but you also mentioned that reroute policies are important, and lr1 only has an allow policy. Could you add a test for the incremental processing with a reroute policy? get some test exposure into logical_router_ports_updated(). > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl lrp-add lr1 new_port 00:00:00:00:00:05 20.0.0.1/24 > +check ovn-nbctl --wait=sb sync > + > +check_engine_stats northd recompute compute > +check_engine_stats route_policies norecompute compute > +# bfd_sync has to recompute because northd recomputed. > +# Since bfd_sync recomputes, so does lflow. > +check_engine_stats bfd_sync recompute nocompute > +check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > # Delete the logical router that has policies on it. > # This should also not require a recompute in route_policies > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-del lr1 > > -check_engine_stats northd norecompute compute > +# northd will recompute when this router is deleted since it > +# had ports. > +check_engine_stats northd recompute compute > check_engine_stats route_policies norecompute compute > check_engine_stats bfd_sync recompute nocompute > -check_engine_stats lflow norecompute compute > +check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > OVN_CLEANUP_NORTHD > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Jacob Tanenbaum _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
