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. +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
