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