On Sun, Oct 11, 2026 at 9:48 PM Jacob Tanenbaum <[email protected]> wrote: > > > > On Tue, Oct 6, 2026 at 1:50 PM Mark Michelson via dev > <[email protected]> wrote: >> >> Prior to this commit, logical router policies were inspected in >> en_northd's logical router change handler. northd would recompute if the >> "policies" column on a logical router changed, if any of the policies >> attached to a logical router changed, or if a deleted logical router had >> any policies on it. >> >> The catch is that en_northd does not actually do anything with logical >> router policies. In other words, changes to logical router policies do >> not affect the data that en_northd exports to other incremental >> processing nodes. Therefore, it doesn't make sense for en_northd to >> recompute if router policy changes are detected. >> >> In this commit, the exact same logic that was in en_northd has been >> transplanted into en_route_policies. This way, en_route_policies >> recomputes its router policies if changes to logical router policies is >> detected. >> >> Now if router policies change, en_northd will not recompute, and it will >> report no change. Since en_northd is an input to many nodes, this means >> that those nodes will no longer run as a result of logical router policy >> changes. en_lr_nat, en_lr_stateful, en_ls_stateful, >> en_mac_binding_aging, en_fdb_aging, en_routes, en_dynamic_routes, >> en_advertised_mac_binding_sync, en_learned_route_sync, >> en_multicast_igmp, en_sync_to_sb_addr_set, en_port_group, >> en_sync_to_sb_pb, and en_sync_from_sb will no longer run at all when >> router policies change. >> >> The existing "Logical router incremental processing for NAT" test >> exercised incremental logic when logical router policies were >> manipulated. That logic has been moved to a new test specifically for >> logical router policy incremental processing. This gives us a place for >> ensuring all of northd's dependent nodes behave as expected as a result >> of this change, and it gives us a dedicated space for further router >> policy incremental processing testing. >> >> Signed-off-by: Mark Michelson <[email protected]> >> --- >> northd/en-northd.c | 55 ++++++++++-- >> northd/en-northd.h | 3 + >> northd/inc-proc-northd.c | 2 + >> northd/northd.c | 14 ++-- >> tests/ovn-inc-proc-graph-dump.at | 1 + >> tests/ovn-northd.at | 138 +++++++++++++++++++++++-------- >> 6 files changed, 162 insertions(+), 51 deletions(-) >> >> diff --git a/northd/en-northd.c b/northd/en-northd.c >> index 178c53313..5286dbbb2 100644 >> --- a/northd/en-northd.c >> +++ b/northd/en-northd.c >> @@ -31,6 +31,7 @@ >> #include "northd.h" >> #include "lib/util.h" >> #include "openvswitch/vlog.h" >> +#include "en-datapath-logical-router.h" >> >> VLOG_DEFINE_THIS_MODULE(en_northd); >> COVERAGE_DEFINE(northd_run); >> @@ -277,7 +278,6 @@ northd_nb_port_group_handler(struct engine_node *node, >> void *data) >> return EN_HANDLED_UNCHANGED; >> } >> >> - >> enum engine_input_handler_result >> route_policies_northd_change_handler(struct engine_node *node, >> void *data OVS_UNUSED) >> @@ -294,15 +294,54 @@ route_policies_northd_change_handler(struct >> engine_node *node, >> * is created or deleted. >> * Northd engine node presently falls back to full recompute when >> * this happens and so does this node. >> - * Note: When we add I-P to the created/deleted logical routers or >> - * logical router ports, we need to revisit this handler. >> * >> - * This node also accesses the route policies of the logical >> router. >> - * When these route policies get updated, en_northd engine >> recomputes >> - * and so does this node. >> - * Note: When we add I-P to handle route policies changes, we need >> - * to revisit this handler. >> */ >> + >> + return EN_HANDLED_UNCHANGED; >> +} >> + >> +enum engine_input_handler_result >> +route_policies_datapath_synced_logical_router_handler(struct engine_node >> *node, >> + void *data OVS_UNUSED) >> +{ >> + const struct ovn_synced_logical_router_map *synced_lrs = >> + engine_get_input_data("datapath_synced_logical_router", node); >> + >> + if (hmapx_is_empty(&synced_lrs->new) && >> + hmapx_is_empty(&synced_lrs->updated) && >> + hmapx_is_empty(&synced_lrs->deleted)) { >> + return EN_UNHANDLED; >> + } >> + >> + struct hmapx_node *lr_node; >> + HMAPX_FOR_EACH (lr_node, &synced_lrs->deleted) { >> + const struct ovn_synced_logical_router *lr = lr_node->data; >> + if (lr->nb->n_policies > 0) { >> + return EN_UNHANDLED; >> + } >> + } >> + >> + HMAPX_FOR_EACH (lr_node, &synced_lrs->new) { >> + const struct ovn_synced_logical_router *lr = lr_node->data; >> + if (lr->nb->n_policies > 0) { >> + return EN_UNHANDLED; >> + } >> + } >> + >> + HMAPX_FOR_EACH (lr_node, &synced_lrs->updated) { >> + const struct ovn_synced_logical_router *lr = lr_node->data; >> + if (nbrec_logical_router_is_updated( >> + lr->nb, NBREC_LOGICAL_ROUTER_COL_POLICIES)) { >> + return EN_UNHANDLED; >> + } >> + for (size_t i = 0; i < lr->nb->n_policies; i++) { >> + if >> (nbrec_logical_router_policy_row_get_seqno(lr->nb->policies[i], >> + OVSDB_IDL_CHANGE_MODIFY) > 0) { >> + return EN_UNHANDLED; >> + } >> + } >> + } >> + >> return EN_HANDLED_UNCHANGED; >> } >> >> diff --git a/northd/en-northd.h b/northd/en-northd.h >> index c62631008..706b49e45 100644 >> --- a/northd/en-northd.h >> +++ b/northd/en-northd.h >> @@ -35,6 +35,9 @@ 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, >> void *data); >> void *en_route_policies_init(struct engine_node *node OVS_UNUSED, >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index 53604a20a..e28cd9d97 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -333,6 +333,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_bfd, &en_sb_bfd, NULL); >> >> engine_add_input(&en_route_policies, &en_bfd, NULL); >> + 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); >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 4e07942dc..5aaf320d1 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -5522,6 +5522,10 @@ bool northd_handle_ipam_changes(struct northd_data >> *nd) >> * Presently supports i-p for the below changes: >> * - load balancers and load balancer groups. >> * - NAT changes >> + * >> + * We ignore changes to the following because they >> + * are handled by other incremental nodes: >> + * - logical router policies >> */ >> static bool >> lr_changes_can_be_handled(const struct nbrec_logical_router *lr) >> @@ -5538,7 +5542,8 @@ lr_changes_can_be_handled(const struct >> nbrec_logical_router *lr) >> if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER >> || col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP >> || col == NBREC_LOGICAL_ROUTER_COL_NAT >> - || col == NBREC_LOGICAL_ROUTER_COL_STATIC_ROUTES) { >> + || col == NBREC_LOGICAL_ROUTER_COL_STATIC_ROUTES >> + || col == NBREC_LOGICAL_ROUTER_COL_POLICIES) { >> continue; >> } >> return false; >> @@ -5557,12 +5562,6 @@ lr_changes_can_be_handled(const struct >> nbrec_logical_router *lr) >> OVSDB_IDL_CHANGE_MODIFY) > 0) { >> return false; >> } >> - for (size_t i = 0; i < lr->n_policies; i++) { >> - if (nbrec_logical_router_policy_row_get_seqno(lr->policies[i], >> - OVSDB_IDL_CHANGE_MODIFY) > 0) { >> - return false; >> - } >> - } >> >> return true; >> } >> @@ -5717,7 +5716,6 @@ northd_handle_lr_changes(const struct northd_input *ni, >> if (deleted_lr->copp || >> !hmap_is_empty(&od->ports) || >> deleted_lr->n_ports > 0 || >> - deleted_lr->n_policies > 0 || >> deleted_lr->n_static_routes > 0) { >> goto fail; >> } >> diff --git a/tests/ovn-inc-proc-graph-dump.at >> b/tests/ovn-inc-proc-graph-dump.at >> index 8348c3467..e763fa189 100644 >> --- a/tests/ovn-inc-proc-graph-dump.at >> +++ b/tests/ovn-inc-proc-graph-dump.at >> @@ -156,6 +156,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"]]; >> bfd -> route_policies [[label=""]]; >> + datapath_synced_logical_router -> route_policies >> [[label="route_policies_datapath_synced_logical_router_handler"]]; >> northd -> route_policies >> [[label="route_policies_northd_change_handler"]]; >> bfd_sync [[style=filled, shape=box, fillcolor=white, >> label="bfd_sync"]]; >> bfd -> bfd_sync [[label=""]]; >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index c570922ef..0a488ed64 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -4693,7 +4693,7 @@ wait_column down bfd status logical_port=r0-sw8 >> bfd_route_policy_uuid=$(fetch_column nb:bfd _uuid logical_port=r0-sw8) >> AT_CHECK([ovn-nbctl list logical_router_policy | grep -q >> $bfd_route_policy_uuid]) >> >> -check_engine_stats northd recompute incremental >> +check_engine_stats northd norecompute incremental >> check_engine_stats bfd recompute nocompute >> check_engine_stats routes recompute nocompute >> check_engine_stats lflow recompute nocompute >> @@ -4708,7 +4708,7 @@ wait_column down bfd status dst_ip=192.168.9.2 >> wait_column down bfd status dst_ip=192.168.9.3 >> wait_column down bfd status dst_ip=192.168.9.4 >> >> -check_engine_stats northd recompute nocompute >> +check_engine_stats northd norecompute compute >> check_engine_stats bfd recompute nocompute >> check_engine_stats route_policies recompute nocompute >> check_engine_stats lflow recompute nocompute >> @@ -14161,39 +14161,6 @@ check_engine_stats sync_to_sb_pb norecompute compute >> check_engine_stats sync_to_sb_lb norecompute compute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> -# Create router Policy >> -check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> -check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" >> reroute 172.168.0.101,172.168.0.102 >> -check_engine_stats northd recompute nocompute >> -check_engine_stats lr_nat recompute nocompute >> -check_engine_stats lr_stateful recompute nocompute >> -check_engine_stats sync_to_sb_pb recompute nocompute >> -check_engine_stats sync_to_sb_lb recompute nocompute >> -check_engine_stats lflow recompute nocompute >> -CHECK_NO_CHANGE_AFTER_RECOMPUTE >> - >> -# Change router Policy to use explicit output port. >> -lrp_lr0_sw0=$(fetch_column nb:logical_router_port _uuid name=lr0-sw0) >> -check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> -check ovn-nbctl --wait=sb set logical_router_policy . >> output_port=$lrp_lr0_sw0 >> -check_engine_stats northd recompute nocompute >> -check_engine_stats lr_nat recompute nocompute >> -check_engine_stats lr_stateful recompute nocompute >> -check_engine_stats sync_to_sb_pb recompute nocompute >> -check_engine_stats sync_to_sb_lb recompute nocompute >> -check_engine_stats lflow recompute nocompute >> -CHECK_NO_CHANGE_AFTER_RECOMPUTE >> - >> -check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> -check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" >> -check_engine_stats northd recompute nocompute >> -check_engine_stats lr_nat recompute nocompute >> -check_engine_stats lr_stateful recompute nocompute >> -check_engine_stats sync_to_sb_pb recompute nocompute >> -check_engine_stats sync_to_sb_lb recompute nocompute >> -check_engine_stats lflow recompute nocompute >> -CHECK_NO_CHANGE_AFTER_RECOMPUTE >> - >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> ]) >> @@ -24657,3 +24624,104 @@ AT_CHECK([grep -qE "Bad out port lrp2 for policy >> ip4.src == 10.0.0.100" northd/o >> OVN_CLEANUP_NORTHD >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([Router policy incremental processing]) > > > I know that you do more with this testing later in the patchset but could you > add a sub-case that modifies a policy column other then output port? > and assert route_policies recompute plus CHECK_NO_CHANGE_AFTER_RECOMPUTE? > That would put the in-place modify to route re-tracking > path explicitly under test.
Hi Jacob, I'm not sure I understand the request here. The goal of the test cases in this patch was to ensure that router policy changes no longer trigger recomputes in northd (or its dependent nodes). The test takes the approach of testing add, modify, and delete operations on a logical router policy to ensure that northd doesn't recompute. I'm not sure what testing a modify on a different router policy column would accomplish. I don't know what you mean by "in-place modify to route re-tracking path" either. Can you elaborate? Thanks! > >> >> +ovn_start >> + >> +check ovn-nbctl lr-add lr0 >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-p1 00:00:00:00:ff:01 10.0.0.1/24 >> + >> +# Create router Policy >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" >> reroute 172.168.0.101,172.168.0.102 >> +# northd should be able to handle a policy change without needing to >> +# recompute. >> +check_engine_stats northd norecompute compute >> +# northd should report no change on a logical router policy change, >> +# so dependent nodes should not run. Note: It would be nice to be >> +# able to programmatically iterate over all nodes for which northd >> +# is an input. This way, the test could stay up-to-date in case new >> +# consumers of northd are added. >> +check_engine_stats lr_nat norecompute nocompute >> +check_engine_stats lr_stateful norecompute nocompute >> +check_engine_stats ls_stateful norecompute nocompute >> +check_engine_stats mac_binding_aging norecompute nocompute >> +check_engine_stats fdb_aging norecompute nocompute >> +check_engine_stats routes norecompute nocompute >> +check_engine_stats dynamic_routes norecompute nocompute >> +check_engine_stats advertised_mac_binding_sync norecompute nocompute >> +check_engine_stats learned_route_sync norecompute nocompute >> +check_engine_stats multicast_igmp norecompute nocompute >> +check_engine_stats sync_to_sb_addr_set norecompute nocompute >> +check_engine_stats port_group norecompute nocompute >> +check_engine_stats sync_to_sb_pb norecompute nocompute >> +check_engine_stats sync_from_sb norecompute nocompute >> +# Even though northd reports no change, sync_to_sb_lb will still >> +# compute because it computes on all northbound logical datapath >> +# changes. >> +check_engine_stats sync_to_sb_lb norecompute compute >> +# route_policies recomputes on a logical router policy change, so >> +# its dependent nodes should also recompute. >> +check_engine_stats route_policies recompute nocompute >> +check_engine_stats bfd_sync recompute nocompute >> +check_engine_stats lflow recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Change router Policy to use explicit output port. >> +lr0_p1=$(fetch_column nb:logical_router_port _uuid name=lr0-p1) >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb set logical_router_policy . output_port=$lr0_p1 >> + >> +# All engine nodes should have the same result as when we added the >> +# policy. >> +check_engine_stats northd norecompute compute >> +check_engine_stats lr_nat norecompute nocompute >> +check_engine_stats lr_stateful norecompute nocompute >> +check_engine_stats ls_stateful norecompute nocompute >> +check_engine_stats mac_binding_aging norecompute nocompute >> +check_engine_stats fdb_aging norecompute nocompute >> +check_engine_stats routes norecompute nocompute >> +check_engine_stats dynamic_routes norecompute nocompute >> +check_engine_stats advertised_mac_binding_sync norecompute nocompute >> +check_engine_stats learned_route_sync norecompute nocompute >> +check_engine_stats multicast_igmp norecompute nocompute >> +check_engine_stats sync_to_sb_addr_set norecompute nocompute >> +check_engine_stats port_group norecompute nocompute >> +check_engine_stats sync_to_sb_pb norecompute nocompute >> +check_engine_stats sync_from_sb norecompute nocompute >> +check_engine_stats sync_to_sb_lb norecompute compute >> +check_engine_stats route_policies recompute nocompute >> +check_engine_stats bfd_sync recompute nocompute >> +check_engine_stats lflow recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" >> + >> +# All engine nodes should have the same result as when we added the >> +# policy. >> +check_engine_stats northd norecompute compute >> +check_engine_stats lr_nat norecompute nocompute >> +check_engine_stats lr_stateful norecompute nocompute >> +check_engine_stats ls_stateful norecompute nocompute >> +check_engine_stats mac_binding_aging norecompute nocompute >> +check_engine_stats fdb_aging norecompute nocompute >> +check_engine_stats routes norecompute nocompute >> +check_engine_stats dynamic_routes norecompute nocompute >> +check_engine_stats advertised_mac_binding_sync norecompute nocompute >> +check_engine_stats learned_route_sync norecompute nocompute >> +check_engine_stats multicast_igmp norecompute nocompute >> +check_engine_stats sync_to_sb_addr_set norecompute nocompute >> +check_engine_stats port_group norecompute nocompute >> +check_engine_stats sync_to_sb_pb norecompute nocompute >> +check_engine_stats sync_from_sb norecompute nocompute >> +check_engine_stats sync_to_sb_lb norecompute compute >> +check_engine_stats route_policies recompute nocompute >> +check_engine_stats bfd_sync recompute nocompute >> +check_engine_stats lflow recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +OVN_CLEANUP_NORTHD >> +AT_CLEANUP >> +]) >> -- >> 2.55.0 >> >> _______________________________________________ >> 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
