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]) +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
