On Thu, Oct 15, 2026 at 11:21 AM Mark Michelson <[email protected]> wrote: > > 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!
I think I may know what you're talking about now. Since the test sets up an ECMP reroute policy, setting the output_port is actually invalid for the policy. So even though we are getting the result we want, it's hitting a case in route policy recompute that rejects the policy. I'll update the test to modify a different column. > > > > >> > >> +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
