en-route-policies already had a handler for logical router changes. However, the choices were either to a) indicate no change occurred, or b) recompute.
With this change, we can now incrementally handle logical router policy changes. Adding a new policy, deleting a policy, or updating a policy are handled incrementally. Signed-off-by: Mark Michelson <[email protected]> --- northd/en-route-policies.c | 304 ++++++++++++++++++++++++++++++++----- northd/en-route-policies.h | 28 ++++ northd/inc-proc-northd.c | 2 +- tests/ovn-northd.at | 91 +++++++++-- 4 files changed, 374 insertions(+), 51 deletions(-) diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c index 69187ca26..4227a78dd 100644 --- a/northd/en-route-policies.c +++ b/northd/en-route-policies.c @@ -329,11 +329,66 @@ build_route_policies(struct ovn_datapath *od, } } +static void +route_policies_tracking_data_init(struct route_policies_tracking_data *trk) +{ + *trk = (struct route_policies_tracking_data) { + .has_tracked = false, + .has_tracked_policies = false, + .new_policies = HMAPX_INITIALIZER(&trk->new_policies), + .deleted_policies = HMAPX_INITIALIZER(&trk->deleted_policies), + .has_tracked_bfd = false, + .new_bfd = HMAPX_INITIALIZER(&trk->new_bfd), + .deleted_bfd = HMAPX_INITIALIZER(&trk->deleted_bfd), + }; +} + +static void +route_policies_tracking_data_add_new_dp_policies( + struct route_policies_tracking_data *trk, + struct datapath_route_policies *dp_rp) +{ + hmapx_add(&trk->new_policies, dp_rp); + trk->has_tracked_policies = true; + trk->has_tracked = true; +} + +static void +route_policies_tracking_data_add_deleted_dp_policies( + struct route_policies_tracking_data *trk, + struct datapath_route_policies *dp_rp) +{ + hmapx_add(&trk->deleted_policies, dp_rp); + trk->has_tracked_policies = true; + trk->has_tracked = true; +} + +static void +route_policies_tracking_data_add_new_bfd( + struct route_policies_tracking_data *trk, + struct datapath_bfd_active_connections *dp_bfd) +{ + hmapx_add(&trk->new_bfd, dp_bfd); + trk->has_tracked_bfd = true; + trk->has_tracked = true; +} + +static void +route_policies_tracking_data_add_deleted_bfd( + struct route_policies_tracking_data *trk, + struct datapath_bfd_active_connections *dp_bfd) +{ + hmapx_add(&trk->deleted_bfd, dp_bfd); + trk->has_tracked_bfd = true; + trk->has_tracked = true; +} + static void route_policies_init(struct route_policies_data *data) { sparse_array_init(&data->dp_route_policies, 0); sparse_array_init(&data->dp_bfd_active_connections, 0); + route_policies_tracking_data_init(&data->trk); } static struct datapath_route_policies * @@ -387,6 +442,26 @@ dp_bfd_active_connections_destroy( free(dp_bfd); } +static void +route_policies_tracking_data_destroy(struct route_policies_tracking_data *trk) +{ + hmapx_destroy(&trk->new_policies); + hmapx_destroy(&trk->new_bfd); + + struct hmapx_node *node; + HMAPX_FOR_EACH_SAFE (node, &trk->deleted_policies) { + struct datapath_route_policies *dp_rp = node->data; + datapath_route_policies_destroy(dp_rp); + hmapx_delete(&trk->deleted_policies, node); + } + + HMAPX_FOR_EACH_SAFE (node, &trk->deleted_bfd) { + struct datapath_bfd_active_connections *dp_bfd = node->data; + dp_bfd_active_connections_destroy(dp_bfd); + hmapx_delete(&trk->deleted_bfd, node); + } +} + static void route_policies_destroy(struct route_policies_data *data) { @@ -401,6 +476,43 @@ route_policies_destroy(struct route_policies_data *data) dp_bfd_active_connections_destroy(dp_bfd); } sparse_array_destroy(&data->dp_bfd_active_connections); + + route_policies_tracking_data_destroy(&data->trk); +} + +static void +build_datapath_route_policies( + struct ovn_datapath *od, + struct route_policies_data *rp_data, + struct datapath_route_policies **p_dp_rp, + struct datapath_bfd_active_connections **p_dp_bfd) +{ + *p_dp_rp = NULL; + *p_dp_bfd = NULL; + struct datapath_route_policies *dp_rp = datapath_route_policies_alloc(od); + struct datapath_bfd_active_connections *dp_bfd = + dp_bfd_active_connections_alloc(); + + build_route_policies(od, &dp_rp->route_policies, + &dp_bfd->active_connections, + &dp_rp->chain_ids, + &dp_rp->ecmp_group_ids); + + if (hmap_is_empty(&dp_rp->route_policies)) { + datapath_route_policies_destroy(dp_rp); + } else { + sparse_array_add_at(&rp_data->dp_route_policies, dp_rp, + od->sdp->index); + *p_dp_rp = dp_rp; + } + + if (uuidset_is_empty(&dp_bfd->active_connections)) { + dp_bfd_active_connections_destroy(dp_bfd); + } else { + sparse_array_add_at(&rp_data->dp_bfd_active_connections, dp_bfd, + od->sdp->index); + *p_dp_bfd = dp_bfd; + } } enum engine_node_state @@ -414,29 +526,10 @@ en_route_policies_run(struct engine_node *node, void *data) struct ovn_datapath *od; HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) { - struct datapath_route_policies *dp_rp = - datapath_route_policies_alloc(od); - struct datapath_bfd_active_connections *dp_bfd = - dp_bfd_active_connections_alloc(); - build_route_policies(od, &dp_rp->route_policies, - &dp_bfd->active_connections, - &dp_rp->chain_ids, - &dp_rp->ecmp_group_ids); - - if (hmap_is_empty(&dp_rp->route_policies)) { - datapath_route_policies_destroy(dp_rp); - } else { - sparse_array_add_at(&route_policies_data->dp_route_policies, dp_rp, - od->sdp->index); - } - - if (uuidset_is_empty(&dp_bfd->active_connections)) { - dp_bfd_active_connections_destroy(dp_bfd); - } else { - sparse_array_add_at( - &route_policies_data->dp_bfd_active_connections, dp_bfd, - od->sdp->index); - } + struct datapath_route_policies *dp_rp; + struct datapath_bfd_active_connections *dp_bfd; + build_datapath_route_policies(od, route_policies_data, &dp_rp, + &dp_bfd); } return EN_UPDATED; @@ -479,12 +572,33 @@ route_policies_northd_change_handler(struct engine_node *node, return EN_HANDLED_UNCHANGED; } +static bool +logical_router_policies_updated(const struct nbrec_logical_router *lr) +{ + if (nbrec_logical_router_is_updated(lr, + NBREC_LOGICAL_ROUTER_COL_POLICIES)) { + return true; + } + for (size_t i = 0; i < lr->n_policies; i++) { + const struct nbrec_logical_router_policy *rule = lr->policies[i]; + if (nbrec_logical_router_policy_row_get_seqno( + rule, 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 OVS_UNUSED) + void *data) { const struct ovn_synced_logical_router_map *synced_lrs = engine_get_input_data("datapath_synced_logical_router", node); + const struct northd_data *northd_data = + engine_get_input_data("northd", node); + struct route_policies_data *rp_data = data; if (hmapx_is_empty(&synced_lrs->new) && hmapx_is_empty(&synced_lrs->updated) && @@ -492,34 +606,156 @@ route_policies_datapath_synced_logical_router_handler(struct engine_node *node, return EN_UNHANDLED; } + enum engine_input_handler_result result = EN_HANDLED_UNCHANGED; 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; + struct datapath_route_policies *dp_rp = + sparse_array_remove(&rp_data->dp_route_policies, lr->sdp->index); + if (dp_rp) { + route_policies_tracking_data_add_deleted_dp_policies(&rp_data->trk, + dp_rp); + result = EN_HANDLED_UPDATED; + } + struct datapath_bfd_active_connections *dp_bfd = + sparse_array_remove(&rp_data->dp_bfd_active_connections, + lr->sdp->index); + if (dp_bfd) { + route_policies_tracking_data_add_deleted_bfd(&rp_data->trk, + dp_bfd); + result = EN_HANDLED_UPDATED; } } HMAPX_FOR_EACH (lr_node, &synced_lrs->new) { const struct ovn_synced_logical_router *lr = lr_node->data; - if (lr->nb->n_policies > 0) { + if (lr->nb->n_policies == 0) { + continue; + } + struct datapath_route_policies *dp_rp = + sparse_array_get(&rp_data->dp_route_policies, lr->sdp->index); + struct datapath_bfd_active_connections *dp_bfd = + sparse_array_get(&rp_data->dp_bfd_active_connections, + lr->sdp->index); + if (dp_rp || dp_bfd) { + /* This should never happen since the router is new, but just + * in case, let's fall back to a recompute. + */ return EN_UNHANDLED; } + struct ovn_datapath *od = + ovn_datapaths_find_by_index(&northd_data->lr_datapaths, + lr->sdp->index); + if (!od) { + return EN_UNHANDLED; + } + struct datapath_route_policies *rebuilt_dp_rp; + struct datapath_bfd_active_connections *rebuilt_dp_bfd; + build_datapath_route_policies(od, rp_data, &rebuilt_dp_rp, + &rebuilt_dp_bfd); + if (rebuilt_dp_rp) { + route_policies_tracking_data_add_new_dp_policies(&rp_data->trk, + rebuilt_dp_rp); + result = EN_HANDLED_UPDATED; + } + if (rebuilt_dp_bfd) { + route_policies_tracking_data_add_new_bfd(&rp_data->trk, + rebuilt_dp_bfd); + result = EN_HANDLED_UPDATED; + } } 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)) { + + if (!logical_router_policies_updated(lr->nb)) { + continue; + } + struct ovn_datapath *od = + ovn_datapaths_find_by_index(&northd_data->lr_datapaths, + lr->sdp->index); + if (!od) { 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; + struct datapath_route_policies *dp_rp = + sparse_array_remove(&rp_data->dp_route_policies, lr->sdp->index); + struct datapath_bfd_active_connections *dp_bfd = + sparse_array_remove(&rp_data->dp_bfd_active_connections, + lr->sdp->index); + if (dp_bfd) { + /* Mark the old BFD active connections as deleted. We're going + * to build new ones later. + */ + route_policies_tracking_data_add_deleted_bfd(&rp_data->trk, + dp_bfd); + result = EN_HANDLED_UPDATED; + } + if (!dp_rp) { + /* This updated router had no policies, but now does. Therefore + * the policies should be treated as new. + */ + struct datapath_route_policies *rebuilt_dp_rp; + struct datapath_bfd_active_connections *rebuilt_dp_bfd; + build_datapath_route_policies(od, rp_data, &rebuilt_dp_rp, + &rebuilt_dp_bfd); + if (rebuilt_dp_rp) { + route_policies_tracking_data_add_new_dp_policies( + &rp_data->trk, rebuilt_dp_rp); + result = EN_HANDLED_UPDATED; + } + if (rebuilt_dp_bfd) { + route_policies_tracking_data_add_new_bfd(&rp_data->trk, + rebuilt_dp_bfd); + result = EN_HANDLED_UPDATED; } + continue; + } + + /* This updated router had policies and they are updated. We'll + * mark the existing policies as deleted and then rebuild the + * policies for this datapath and mark them as new. + */ + route_policies_tracking_data_add_deleted_dp_policies(&rp_data->trk, + dp_rp); + result = EN_HANDLED_UPDATED; + struct datapath_route_policies *rebuilt_dp_rp; + struct datapath_bfd_active_connections *rebuilt_dp_bfd; + build_datapath_route_policies(od, rp_data, &rebuilt_dp_rp, + &rebuilt_dp_bfd); + if (rebuilt_dp_rp) { + route_policies_tracking_data_add_new_dp_policies(&rp_data->trk, + rebuilt_dp_rp); + } + if (rebuilt_dp_bfd) { + route_policies_tracking_data_add_new_bfd(&rp_data->trk, + rebuilt_dp_bfd); } } - return EN_HANDLED_UNCHANGED; + return result; +} + +void +en_route_policies_clear_tracked_data(void *data) +{ + struct route_policies_data *rp_data = data; + + rp_data->trk.has_tracked = false; + rp_data->trk.has_tracked_bfd = false; + rp_data->trk.has_tracked_policies = false; + hmapx_clear(&rp_data->trk.new_policies); + hmapx_clear(&rp_data->trk.new_bfd); + + struct hmapx_node *node; + HMAPX_FOR_EACH_SAFE (node, &rp_data->trk.deleted_policies) { + struct datapath_route_policies *dp_rp = node->data; + datapath_route_policies_destroy(dp_rp); + hmapx_delete(&rp_data->trk.deleted_policies, node); + } + + HMAPX_FOR_EACH_SAFE (node, &rp_data->trk.deleted_bfd) { + struct datapath_bfd_active_connections *dp_bfd = node->data; + dp_bfd_active_connections_destroy(dp_bfd); + hmapx_delete(&rp_data->trk.deleted_bfd, node); + } } diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h index 3dd42c41d..e176cc3a0 100644 --- a/northd/en-route-policies.h +++ b/northd/en-route-policies.h @@ -26,6 +26,7 @@ #include "uuidset.h" #include "sparse-array.h" #include "simap.h" +#include "hmapx.h" /* Each instance of this represents a nexthop for a router * policy with "reroute" action. The fields are used for @@ -64,11 +65,37 @@ struct datapath_bfd_active_connections { struct uuidset active_connections; }; +struct route_policies_tracking_data { + /* Indicates that some data was incrementally + * processed by the en-route-policies node. + */ + bool has_tracked; + + /* Indicates that routing policies were incrementally + * processed by the en-route-policies node. + */ + bool has_tracked_policies; + /* These contain struct datapath_route_policies. */ + struct hmapx new_policies; + struct hmapx deleted_policies; + + /* Indicates that BFD connections were incrementally + * processed by the en-route-policies node. + */ + bool has_tracked_bfd; + /* These contain struct datapath_bfd_active_connections. */ + struct hmapx new_bfd; + struct hmapx deleted_bfd; +}; + /* Global route policy data exported by en-route-policies. */ struct route_policies_data { /* Each entry is a struct datapath_route_policies pointer */ struct sparse_array dp_route_policies; struct sparse_array dp_bfd_active_connections; + + /* Tracking data. */ + struct route_policies_tracking_data trk; }; void en_route_policies_cleanup(void *data); @@ -82,5 +109,6 @@ 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, struct engine_arg *arg OVS_UNUSED); +void en_route_policies_clear_tracked_data(void *data); #endif /* EN_ROUTE_POLICIES_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 7e92f7fec..0fde77032 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -180,7 +180,7 @@ static ENGINE_NODE(lb_data, CLEAR_TRACKED_DATA); static ENGINE_NODE(lr_nat, CLEAR_TRACKED_DATA); static ENGINE_NODE(lr_stateful, CLEAR_TRACKED_DATA); static ENGINE_NODE(ls_stateful, CLEAR_TRACKED_DATA); -static ENGINE_NODE(route_policies); +static ENGINE_NODE(route_policies, CLEAR_TRACKED_DATA); static ENGINE_NODE(routes, CLEAR_TRACKED_DATA); static ENGINE_NODE(bfd_sync, SB_WRITE); static ENGINE_NODE(ecmp_nexthop, SB_WRITE); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 81e571d36..e11ebc6d5 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -4689,10 +4689,7 @@ 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 norecompute incremental -# route_policies was able to compute when the static route was -# added earlier, but has to recompute as a result of the policy -# being added. -check_engine_stats route_policies recompute compute +check_engine_stats route_policies norecompute compute check_engine_stats lflow recompute nocompute check_engine_stats northd_output norecompute compute CHECK_NO_CHANGE_AFTER_RECOMPUTE @@ -4706,11 +4703,8 @@ 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 norecompute compute -# In this case, though, the only operation that happened since -# clearing incremental stats is that a policy was added. In this -# case, route_policies has recomputed but has not computed. -check_engine_stats route_policies recompute nocompute -check_engine_stats lflow recompute nocompute +check_engine_stats route_policies norecompute compute +check_engine_stats lflow recompute compute check_engine_stats northd_output norecompute compute CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats @@ -24633,7 +24627,7 @@ 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 +check ovn-nbctl --wait=sb lr-policy-add lr0 100 "ip4.src == 100.0.0.0/24" reroute 10.0.0.50 # northd should be able to handle a policy change without needing to # recompute. check_engine_stats northd norecompute compute @@ -24660,9 +24654,7 @@ check_engine_stats sync_from_sb norecompute nocompute # 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 route_policies norecompute compute check_engine_stats bfd_sync recompute nocompute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE @@ -24690,13 +24682,13 @@ 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 route_policies norecompute compute 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" +check ovn-nbctl --wait=sb lr-policy-del lr0 # All engine nodes should have the same result as when we added the # policy. @@ -24716,7 +24708,74 @@ 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 route_policies norecompute compute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# From this point on, operations we perform may result in +# ovn-northd exporting tracked data, which makes some of its dependent +# nodes compute/recompute. Since this test is mainly focused +# on en-route-policies, we'll stop checking the giant list of +# nodes from this point on. + +# Add a logical router with a policy on it in the same +# transaction. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lr-add lr1 \ + -- lr-policy-add lr1 100 "1" allow +check ovn-nbctl --wait=sb sync + +check_engine_stats northd norecompute compute +check_engine_stats route_policies norecompute compute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Invalidate the policy by changing it to a jump policy but not +# setting the jump_chain. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +policy_uuid=$(fetch_column nb:Logical_Router_Policy _uuid action=allow) +check ovn-nbctl set Logical_Router_Policy $policy_uuid action=jump + +check_engine_stats northd norecompute compute +check_engine_stats route_policies norecompute compute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Un-invalidate the policy by changing it back to an allow policy. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl set Logical_Router_Policy $policy_uuid action=allow + +check_engine_stats northd norecompute compute +check_engine_stats route_policies norecompute compute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Add a new logical router with an invalid policy in the same transaction +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lr-add lr2 \ + -- lr-policy-add lr2 100 "1" jump fakechain +check ovn-nbctl --wait=sb sync + +check_engine_stats northd norecompute compute +check_engine_stats route_policies norecompute compute +# Since the added policy was invalid, route_policies will +# compute but report no change. bfd_sync and lflow will react to +# the northd change and will compute as a result. +check_engine_stats bfd_sync norecompute compute +check_engine_stats lflow norecompute compute +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 +check_engine_stats route_policies norecompute compute check_engine_stats bfd_sync recompute nocompute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
