Split ovn-northd's en_lflow engine node into a compute-only en_lflow node and a new en_dp_group_resolved node that handles datapath-group resolution and all SB database writes. This separation makes en_lflow independent of the SB database, preparing for future incremental processing improvements.
en_lflow's handlers now track dirty lflow_refs in a hmapx instead of calling lflow_ref_sync_lflows directly. en_dp_group_resolved drains the dirty set on its lflow handler, falling back to a full lflow_table_sync_to_sb when en_lflow did a full recompute or when the IGMP/MLD handler set the needs_full_sync flag. A new lflow_ref_unlink_and_prune() function replaces lflow_ref_resync_flows() for the IGMP and IC-learned service monitor handlers. These handlers use a single shared lflow_ref whose flows are also built by the per-datapath pipeline (with lflow_ref = NULL). The per-datapath build's dp bits are not tracked by any lflow_ref, so lflow_ref_unlink_lflows alone cannot clear them via dp_refcnt. lflow_ref_unlink_and_prune destroys all lrns and orphaned lflows in-memory without SB writes, equivalent to lflow_ref_resync_flows' cleanup. The lflow_table_add_lflow__ upgrade-to-bitmap logic (for single lflow_refs that contribute multiple datapaths to the same lflow) is extended with per-datapath dp_refcnt accounting during mid-cycle bitmap upgrades, preventing premature dp bit release when multiple lflow_refs share a flow across overlapping datapaths. Reported-at: https://redhat.atlassian.net/browse/FDP-2747 Assisted-by: Claude Opus 4.8, Claude Code Signed-off-by: Jacob Tanenbaum <[email protected]> --- northd/automake.mk | 2 + northd/en-dp-group-resolved.c | 129 ++++++++++++++++ northd/en-dp-group-resolved.h | 30 ++++ northd/en-lflow.c | 140 +++++------------ northd/en-lflow.h | 3 + northd/en-northd-output.c | 4 +- northd/en-northd-output.h | 3 +- northd/inc-proc-northd.c | 29 +++- northd/lflow-mgr.c | 168 +++++++++++++++++---- northd/lflow-mgr.h | 8 +- northd/northd.c | 251 +++++++------------------------ northd/northd.h | 39 +++-- tests/ovn-inc-proc-graph-dump.at | 12 +- tests/ovn-northd.at | 81 +++++++--- 14 files changed, 506 insertions(+), 393 deletions(-) create mode 100644 northd/en-dp-group-resolved.c create mode 100644 northd/en-dp-group-resolved.h diff --git a/northd/automake.mk b/northd/automake.mk index 45ca0337f..d1439294d 100644 --- a/northd/automake.mk +++ b/northd/automake.mk @@ -16,6 +16,8 @@ northd_ovn_northd_SOURCES = \ northd/en-datapath-logical-router.h \ northd/en-datapath-sync.c \ northd/en-datapath-sync.h \ + northd/en-dp-group-resolved.c \ + northd/en-dp-group-resolved.h \ northd/en-ecmp-nexthop.c \ northd/en-ecmp-nexthop.h \ northd/en-global-config.c \ diff --git a/northd/en-dp-group-resolved.c b/northd/en-dp-group-resolved.c new file mode 100644 index 000000000..d5f9e7738 --- /dev/null +++ b/northd/en-dp-group-resolved.c @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2026, Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> + +#include "en-dp-group-resolved.h" +#include "en-datapath-sync.h" +#include "en-global-config.h" +#include "en-lflow.h" +#include "lflow-mgr.h" + +#include "lib/inc-proc-eng.h" +#include "northd.h" +#include "lib/stopwatch-names.h" +#include "stopwatch.h" +#include "timeval.h" +#include "openvswitch/vlog.h" + +VLOG_DEFINE_THIS_MODULE(en_dp_group_resolved); + +void * +en_dp_group_resolved_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + return NULL; +} + +static void +dp_group_resolved_sync_to_sb(struct engine_node *node, + struct lflow_data *lflow_data) +{ + const struct engine_context *eng_ctx = engine_get_context(); + struct all_synced_datapaths *all_dps = + engine_get_input_data("datapath_sync", node); + + const struct sbrec_logical_flow_table *sb_flow_table = + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); + const struct sbrec_logical_dp_group_table *sb_dpgrp_table = + EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); + + struct ed_type_global_config *global_config = + engine_get_input_data("global_config", node); + + stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); + lflow_table_sync_to_sb(lflow_data->lflow_table, + eng_ctx->ovnsb_idl_txn, + all_dps->synced_dps, + global_config->ovn_internal_version_changed, + sb_flow_table, sb_dpgrp_table); + stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); +} + +enum engine_node_state +en_dp_group_resolved_run(struct engine_node *node, + void *data OVS_UNUSED) +{ + struct lflow_data *lflow_data = engine_get_input_data("lflow", node); + + /* The full sync below covers every lflow in the table, so any + * per-ref dirty tracking left over from en_lflow's handlers is + * redundant. Clear it so the next incremental cycle starts + * clean. */ + hmapx_clear(&lflow_data->dirty_lflow_refs); + lflow_data->needs_full_sync = false; + + dp_group_resolved_sync_to_sb(node, lflow_data); + return EN_UPDATED; +} + +enum engine_input_handler_result +dp_group_resolved_lflow_handler(struct engine_node *node, + void *data OVS_UNUSED) +{ + struct lflow_data *lflow_data = engine_get_input_data("lflow", node); + + if (hmapx_is_empty(&lflow_data->dirty_lflow_refs) + || lflow_data->needs_full_sync) { + hmapx_clear(&lflow_data->dirty_lflow_refs); + lflow_data->needs_full_sync = false; + dp_group_resolved_sync_to_sb(node, lflow_data); + return EN_HANDLED_UPDATED; + } + + const struct engine_context *eng_ctx = engine_get_context(); + struct all_synced_datapaths *all_dps = + engine_get_input_data("datapath_sync", node); + + const struct sbrec_logical_flow_table *sb_flow_table = + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); + const struct sbrec_logical_dp_group_table *sb_dpgrp_table = + EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); + + struct ed_type_global_config *global_config = + engine_get_input_data("global_config", node); + + struct hmapx_node *hmapx_node; + HMAPX_FOR_EACH (hmapx_node, &lflow_data->dirty_lflow_refs) { + struct lflow_ref *ref = hmapx_node->data; + if (!lflow_ref_sync_lflows( + ref, lflow_data->lflow_table, + eng_ctx->ovnsb_idl_txn, + all_dps->synced_dps, + global_config->ovn_internal_version_changed, + sb_flow_table, sb_dpgrp_table)) { + return EN_UNHANDLED; + } + } + hmapx_clear(&lflow_data->dirty_lflow_refs); + + return EN_HANDLED_UPDATED; +} + +void +en_dp_group_resolved_cleanup(void *data OVS_UNUSED) +{ +} diff --git a/northd/en-dp-group-resolved.h b/northd/en-dp-group-resolved.h new file mode 100644 index 000000000..9f25072c1 --- /dev/null +++ b/northd/en-dp-group-resolved.h @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2026, Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef EN_DP_GROUP_RESOLVED_H +#define EN_DP_GROUP_RESOLVED_H 1 + +#include "lib/inc-proc-eng.h" + +void *en_dp_group_resolved_init(struct engine_node *node, + struct engine_arg *arg); +enum engine_node_state en_dp_group_resolved_run(struct engine_node *node, + void *data); +void en_dp_group_resolved_cleanup(void *data); +enum engine_input_handler_result +dp_group_resolved_lflow_handler(struct engine_node *node, void *data); + +#endif /* EN_DP_GROUP_RESOLVED_H */ diff --git a/northd/en-lflow.c b/northd/en-lflow.c index 8cb987777..38b1639d6 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -68,10 +68,6 @@ lflow_get_input_data(struct engine_node *node, struct all_synced_datapaths *all_dps = engine_get_input_data("datapath_sync", node); - lflow_input->sbrec_logical_flow_table = - EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); - lflow_input->sbrec_logical_dp_group_table = - EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); lflow_input->sbrec_acl_id_table = EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); @@ -119,8 +115,6 @@ lflow_get_input_data(struct engine_node *node, enum engine_node_state en_lflow_run(struct engine_node *node, void *data) { - const struct engine_context *eng_ctx = engine_get_context(); - struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); @@ -129,9 +123,10 @@ en_lflow_run(struct engine_node *node, void *data) search_mode == LFLOW_TABLE_SEARCH_FIELDS); lflow_reset_northd_refs(&lflow_input); lflow_ref_clear(lflow_input.igmp_lflow_ref); + hmapx_clear(&lflow_data->dirty_lflow_refs); + lflow_data->needs_full_sync = false; - build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input, - lflow_data->lflow_table); + build_lflows(&lflow_input, lflow_data->lflow_table); return EN_UPDATED; } @@ -149,31 +144,25 @@ lflow_northd_handler(struct engine_node *node, return EN_UNHANDLED; } - const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); - if (!lflow_handle_northd_lr_changes(eng_ctx->ovnsb_idl_txn, - &northd_data->trk_data.trk_routers, - &lflow_input, - lflow_data->lflow_table)) { - return EN_UNHANDLED; - } + lflow_handle_northd_lr_changes(&northd_data->trk_data.trk_routers, + &lflow_input, + lflow_data->lflow_table, + &lflow_data->dirty_lflow_refs); - if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn, - &northd_data->trk_data.trk_lsps, - &lflow_input, - lflow_data->lflow_table)) { - return EN_UNHANDLED; - } + lflow_handle_northd_port_changes(&northd_data->trk_data.trk_lsps, + &lflow_input, + lflow_data->lflow_table, + &lflow_data->dirty_lflow_refs); - if (!lflow_handle_northd_lb_changes( - eng_ctx->ovnsb_idl_txn, &northd_data->trk_data.trk_lbs, - &lflow_input, lflow_data->lflow_table)) { - return EN_UNHANDLED; - } + lflow_handle_northd_lb_changes(&northd_data->trk_data.trk_lbs, + &lflow_input, + lflow_data->lflow_table, + &lflow_data->dirty_lflow_refs); return EN_HANDLED_UPDATED; } @@ -189,17 +178,14 @@ lflow_lr_stateful_handler(struct engine_node *node, void *data) return EN_UNHANDLED; } - const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); - if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn, - &lr_sful_data->trk_data, - &lflow_input, - lflow_data->lflow_table)) { - return EN_UNHANDLED; - } + lflow_handle_lr_stateful_changes(&lr_sful_data->trk_data, + &lflow_input, + lflow_data->lflow_table, + &lflow_data->dirty_lflow_refs); return EN_HANDLED_UPDATED; } @@ -214,17 +200,14 @@ lflow_ls_stateful_handler(struct engine_node *node, void *data) return EN_UNHANDLED; } - const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); - if (!lflow_handle_ls_stateful_changes(eng_ctx->ovnsb_idl_txn, - &ls_sful_data->trk_data, - &lflow_input, - lflow_data->lflow_table)) { - return EN_UNHANDLED; - } + lflow_handle_ls_stateful_changes(&ls_sful_data->trk_data, + &lflow_input, + lflow_data->lflow_table, + &lflow_data->dirty_lflow_refs); return EN_HANDLED_UPDATED; } @@ -235,35 +218,20 @@ lflow_multicast_igmp_handler(struct engine_node *node, void *data) struct multicast_igmp_data *mcast_igmp_data = engine_get_input_data("multicast_igmp", node); - const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); - if (!lflow_ref_resync_flows(mcast_igmp_data->lflow_ref, - lflow_data->lflow_table, - eng_ctx->ovnsb_idl_txn, - lflow_input.dps, - lflow_input.ovn_internal_version_changed, - lflow_input.sbrec_logical_flow_table, - lflow_input.sbrec_logical_dp_group_table)) { - return EN_UNHANDLED; - } + lflow_ref_unlink_and_prune(mcast_igmp_data->lflow_ref, + lflow_data->lflow_table); build_igmp_lflows(&mcast_igmp_data->igmp_groups, &lflow_input.ls_datapaths->datapaths, lflow_data->lflow_table, mcast_igmp_data->lflow_ref); - if (!lflow_ref_sync_lflows(mcast_igmp_data->lflow_ref, - lflow_data->lflow_table, - eng_ctx->ovnsb_idl_txn, - lflow_input.dps, - lflow_input.ovn_internal_version_changed, - lflow_input.sbrec_logical_flow_table, - lflow_input.sbrec_logical_dp_group_table)) { - return EN_UNHANDLED; - } + lflow_data->needs_full_sync = true; + hmapx_add(&lflow_data->dirty_lflow_refs, mcast_igmp_data->lflow_ref); return EN_HANDLED_UPDATED; } @@ -280,7 +248,6 @@ lflow_group_ecmp_route_change_handler(struct engine_node *node, return EN_UNHANDLED; } - const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; @@ -289,25 +256,13 @@ lflow_group_ecmp_route_change_handler(struct engine_node *node, struct group_ecmp_datapath *route_node; struct hmapx_node *hmapx_node; - /* We need to handle deletions before additions as they could potentially - * overlap. */ HMAPX_FOR_EACH (hmapx_node, &group_ecmp_route_data->trk_data.deleted_datapath_routes) { route_node = hmapx_node->data; lflow_ref_unlink_lflows(route_node->lflow_ref); - - bool handled = lflow_ref_sync_lflows( - route_node->lflow_ref, lflow_data->lflow_table, - eng_ctx->ovnsb_idl_txn, lflow_input.dps, - lflow_input.ovn_internal_version_changed, - lflow_input.sbrec_logical_flow_table, - lflow_input.sbrec_logical_dp_group_table); - if (!handled) { - return EN_UNHANDLED; - } + hmapx_add(&lflow_data->dirty_lflow_refs, route_node->lflow_ref); } - /* Now we handle created or updated route nodes. */ struct hmapx *crupdated_datapath_routes = &group_ecmp_route_data->trk_data.crupdated_datapath_routes; HMAPX_FOR_EACH (hmapx_node, crupdated_datapath_routes) { @@ -316,16 +271,7 @@ lflow_group_ecmp_route_change_handler(struct engine_node *node, build_route_data_flows_for_lrouter( route_node->od, lflow_data->lflow_table, route_node, lflow_input.bfd_ports); - - bool handled = lflow_ref_sync_lflows( - route_node->lflow_ref, lflow_data->lflow_table, - eng_ctx->ovnsb_idl_txn, lflow_input.dps, - lflow_input.ovn_internal_version_changed, - lflow_input.sbrec_logical_flow_table, - lflow_input.sbrec_logical_dp_group_table); - if (!handled) { - return EN_UNHANDLED; - } + hmapx_add(&lflow_data->dirty_lflow_refs, route_node->lflow_ref); } return EN_HANDLED_UPDATED; @@ -338,7 +284,6 @@ lflow_ic_learned_svc_mons_handler(struct engine_node *node, struct ic_learned_svc_monitors_data *ic_learned_svc_monitors_data = engine_get_input_data("ic_learned_svcs", node); - const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); @@ -349,16 +294,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node *node, &ic_learned_svc_monitors_data->ic_learned_svc_monitors_map, ic_learned_svc_monitors_data->lflow_ref); - if (!lflow_ref_resync_flows( - ic_learned_svc_monitors_data->lflow_ref, - lflow_data->lflow_table, - eng_ctx->ovnsb_idl_txn, - lflow_input.dps, - lflow_input.ovn_internal_version_changed, - lflow_input.sbrec_logical_flow_table, - lflow_input.sbrec_logical_dp_group_table)) { - return EN_UNHANDLED; - } + lflow_ref_unlink_and_prune(ic_learned_svc_monitors_data->lflow_ref, + lflow_data->lflow_table); build_lswitch_arp_nd_ic_learned_svc_mon( &svc_mons_data, @@ -366,16 +303,9 @@ lflow_ic_learned_svc_mons_handler(struct engine_node *node, lflow_input.svc_monitor_mac, lflow_data->lflow_table); - if (!lflow_ref_sync_lflows( - ic_learned_svc_monitors_data->lflow_ref, - lflow_data->lflow_table, - eng_ctx->ovnsb_idl_txn, - lflow_input.dps, - lflow_input.ovn_internal_version_changed, - lflow_input.sbrec_logical_flow_table, - lflow_input.sbrec_logical_dp_group_table)) { - return EN_UNHANDLED; - } + lflow_data->needs_full_sync = true; + hmapx_add(&lflow_data->dirty_lflow_refs, + ic_learned_svc_monitors_data->lflow_ref); return EN_HANDLED_UPDATED; } @@ -386,6 +316,7 @@ void *en_lflow_init(struct engine_node *node OVS_UNUSED, struct lflow_data *data = xmalloc(sizeof *data); data->lflow_table = lflow_table_alloc(); lflow_table_init(data->lflow_table); + hmapx_init(&data->dirty_lflow_refs); return data; } @@ -393,4 +324,5 @@ void en_lflow_cleanup(void *data_) { struct lflow_data *data = data_; lflow_table_destroy(data->lflow_table); + hmapx_destroy(&data->dirty_lflow_refs); } diff --git a/northd/en-lflow.h b/northd/en-lflow.h index 99bcfda15..5dab6cc19 100644 --- a/northd/en-lflow.h +++ b/northd/en-lflow.h @@ -7,12 +7,15 @@ #include <stdlib.h> #include <stdio.h> +#include "lib/hmapx.h" #include "lib/inc-proc-eng.h" struct lflow_table; struct lflow_data { struct lflow_table *lflow_table; + struct hmapx dirty_lflow_refs; /* lflow_refs changed by handlers. */ + bool needs_full_sync; /* Full lflow_table_sync_to_sb needed. */ }; enum engine_node_state en_lflow_run(struct engine_node *node, void *data); diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c index b492a771c..ed8dec0ac 100644 --- a/northd/en-northd-output.c +++ b/northd/en-northd-output.c @@ -51,8 +51,8 @@ northd_output_sync_to_sb_handler(struct engine_node *node OVS_UNUSED, } enum engine_input_handler_result -northd_output_lflow_handler(struct engine_node *node OVS_UNUSED, - void *data OVS_UNUSED) +northd_output_dp_group_resolved_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) { return EN_HANDLED_UPDATED; } diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h index b7053e60c..bb5e35900 100644 --- a/northd/en-northd-output.h +++ b/northd/en-northd-output.h @@ -13,7 +13,8 @@ enum engine_input_handler_result northd_output_sync_to_sb_handler(struct engine_node *node, void *data OVS_UNUSED); enum engine_input_handler_result -northd_output_lflow_handler(struct engine_node *node, void *data OVS_UNUSED); +northd_output_dp_group_resolved_handler(struct engine_node *node, + void *data OVS_UNUSED); enum engine_input_handler_result northd_output_mac_binding_aging_handler(struct engine_node *node, void *data OVS_UNUSED); diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index d2f28666c..4c0c8acaa 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -37,6 +37,7 @@ #include "en-multicast.h" #include "en-northd.h" #include "en-lflow.h" +#include "en-dp-group-resolved.h" #include "en-northd-output.h" #include "en-meters.h" #include "en-sampling-app.h" @@ -160,7 +161,8 @@ enum sb_engine_node { static ENGINE_NODE(northd, CLEAR_TRACKED_DATA, SB_WRITE); static ENGINE_NODE(sync_from_sb, SB_WRITE); static ENGINE_NODE(sampling_app); -static ENGINE_NODE(lflow, SB_WRITE); +static ENGINE_NODE(lflow); +static ENGINE_NODE(dp_group_resolved, SB_WRITE); static ENGINE_NODE(mac_binding_aging, SB_WRITE); static ENGINE_NODE(mac_binding_aging_waker); static ENGINE_NODE(northd_output); @@ -399,9 +401,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_multicast_igmp, &en_sb_igmp_group, NULL); engine_add_input(&en_lflow, &en_sync_meters, NULL); - engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); - engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); engine_add_input(&en_lflow, &en_bfd_sync, NULL); engine_add_input(&en_lflow, &en_route_policies, NULL); /* Route changes are propagated to en_lflow through the en_group_ecmp_route @@ -436,6 +436,25 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_lflow, &en_ic_learned_svc_monitors, lflow_ic_learned_svc_mons_handler); + engine_add_input(&en_dp_group_resolved, &en_lflow, + dp_group_resolved_lflow_handler); + engine_add_input(&en_dp_group_resolved, &en_sb_logical_flow, NULL); + engine_add_input(&en_dp_group_resolved, &en_sb_logical_dp_group, NULL); + /* dp_group_resolved reads synced datapath arrays (dps[DP_MAX]) when + * calling lflow_table_sync_to_sb / lflow_ref_sync_lflows. A noop + * handler is sufficient because en_lflow already recomputes when + * datapath_sync changes; dp_group_resolved just needs access to + * the data. */ + engine_add_input(&en_dp_group_resolved, &en_datapath_sync, + engine_noop_handler); + /* dp_group_resolved reads ovn_internal_version_changed from + * global_config when syncing flows to SB. Changes to + * global_config propagate through en_lflow (which has its own + * global_config handler), so a noop handler here avoids a + * redundant recompute. */ + engine_add_input(&en_dp_group_resolved, &en_global_config, + engine_noop_handler); + engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL); engine_add_input(&en_sync_to_sb_addr_set, &en_lr_stateful, NULL); engine_add_input(&en_sync_to_sb_addr_set, &en_sb_address_set, NULL); @@ -490,8 +509,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_northd_output, &en_sync_from_sb, NULL); engine_add_input(&en_northd_output, &en_sync_to_sb, northd_output_sync_to_sb_handler); - engine_add_input(&en_northd_output, &en_lflow, - northd_output_lflow_handler); + engine_add_input(&en_northd_output, &en_dp_group_resolved, + northd_output_dp_group_resolved_handler); engine_add_input(&en_northd_output, &en_mac_binding_aging, northd_output_mac_binding_aging_handler); engine_add_input(&en_northd_output, &en_fdb_aging, diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c index ce9c4f854..7a61212bc 100644 --- a/northd/lflow-mgr.c +++ b/northd/lflow-mgr.c @@ -261,6 +261,28 @@ lflow_table_set_size(struct lflow_table *lflow_table, size_t size) lflow_table->entries.n = size; } +/* An lflow that applies to no datapaths (its dp group bitmap is empty) must + * not exist in the SB DB. Such lflows can be left in the table by en_lflow's + * incremental handlers, which unlink lflows (clearing their dp bits) and defer + * the teardown to the dp-group-resolved sync stage. Release the lflow's dp + * group and destroy it; its SB row (if any) is removed by the caller. Returns + * true if the lflow was destroyed. */ +static bool +lflow_prune_if_no_datapaths(struct lflow_table *lflow_table, + struct ovn_lflow *lflow) +{ + if (dynamic_bitmap_count1(&lflow->dpg_bitmap)) { + return false; + } + + enum ovn_datapath_type dp_type = ovn_stage_to_datapath_type(lflow->stage); + ovs_assert(dp_type < DP_MAX); + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], lflow->dpg); + lflow->dpg = NULL; + ovn_lflow_destroy(lflow_table, lflow); + return true; +} + void lflow_table_sync_to_sb(struct lflow_table *lflow_table, struct ovsdb_idl_txn *ovnsb_txn, @@ -287,6 +309,12 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table, ovn_lflow_destroy(lflow_table, lflow); continue; } + /* An lflow with no datapaths must be removed from the SB DB. It is + * skipped here (not added to 'sb_uuid_set'), so its SB row, if any, is + * deleted by the reconciliation loop below. */ + if (lflow_prune_if_no_datapaths(lflow_table, lflow)) { + continue; + } sbflow = NULL; if (!uuid_is_zero(&lflow->sb_uuid)) { sbflow = sbrec_logical_flow_table_get_for_uuid(sb_flow_table, @@ -372,7 +400,7 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table, lflows, &stage, sbflow->priority, sbflow->match, sbflow->actions, sbflow->controller_meter, acl_ct_translation, sbflow->hash); - if (lflow) { + if (lflow && !lflow_prune_if_no_datapaths(lflow_table, lflow)) { const struct ovn_synced_datapaths *datapaths; struct hmap *dp_groups; dp_groups = &lflow_table->dp_groups[dp_type]; @@ -393,6 +421,9 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table, if (search_mode != LFLOW_TABLE_SEARCH_FIELDS) { break; } + if (lflow_prune_if_no_datapaths(lflow_table, lflow)) { + continue; + } const struct ovn_synced_datapaths *datapaths; struct hmap *dp_groups; enum ovn_datapath_type dp_type = @@ -681,20 +712,32 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref) } } -bool -lflow_ref_resync_flows(struct lflow_ref *lflow_ref, - struct lflow_table *lflow_table, - struct ovsdb_idl_txn *ovnsb_txn, - const struct ovn_synced_datapaths dps[DP_MAX], - bool ovn_internal_version_changed, - const struct sbrec_logical_flow_table *sbflow_table, - const struct sbrec_logical_dp_group_table *dpgrp_table) +/* Unlinks and destroys all lrns in 'lflow_ref', then destroys any lflow + * whose referenced_by list is empty (no other lflow_ref references it). + * Unlike lflow_ref_unlink_lflows (which only clears dp bits and sets + * linked=false), this function removes the lrns and orphaned lflows + * from the in-memory table entirely, without writing to SB. */ +void +lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref, + struct lflow_table *lflow_table) { lflow_ref_unlink_lflows(lflow_ref); - return lflow_ref_sync_lflows__(lflow_ref, lflow_table, ovnsb_txn, - dps, - ovn_internal_version_changed, sbflow_table, - dpgrp_table); + + struct lflow_ref_node *lrn; + HMAP_FOR_EACH_SAFE (lrn, ref_node, &lflow_ref->lflow_ref_nodes) { + struct ovn_lflow *lflow = lrn->lflow; + lflow_ref_node_destroy(lrn); + + if (ovs_list_is_empty(&lflow->referenced_by)) { + enum ovn_datapath_type dp_type = + ovn_stage_to_datapath_type(lflow->stage); + ovs_assert(dp_type < DP_MAX); + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], + lflow->dpg); + lflow->dpg = NULL; + ovn_lflow_destroy(lflow_table, lflow); + } + } } bool @@ -781,10 +824,78 @@ lflow_table_add_lflow__(struct lflow_table *lflow_table, } ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node); hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, hash); + } else if (sdp) { + /* Single-datapath (re-)add of an existing node. */ + if (!lrn->dpgrp_lflow) { + if (!lrn->linked) { + /* First add of this (re-)link cycle establishes the base + * datapath this reference tracks for the lflow. */ + lrn->dp_index = sdp->index; + } else if (lrn->dp_index != sdp->index) { + /* The same 'lflow_ref' references this lflow L(M, A) for a + * second datapath in this cycle (e.g. the single lflow_ref + * shared by all IGMP flows). A single-datapath + * lflow_ref_node tracks only one datapath index, so + * lflow_ref_unlink_lflows() would leave the other + * datapaths set in the lflow's dp group bitmap. Upgrade + * the node to track a datapath bitmap instead, so that + * unlinking clears every datapath this reference + * contributed. */ + size_t len = sparse_array_len(&sdp->dps->dps_array); + lrn->dpgrp_bitmap = bitmap_allocate(len); + lrn->dpgrp_bitmap_len = len; + bitmap_set1(lrn->dpgrp_bitmap, lrn->dp_index); + bitmap_set1(lrn->dpgrp_bitmap, sdp->index); + lrn->dpgrp_lflow = true; + + /* This reference already accounted for 'lrn->dp_index' + * (the first datapath it added this cycle) in the block + * below. This add contributes a second datapath, which + * the block below will not see because it only runs on + * the first link of the cycle. Account for it here so + * that a datapath shared with another reference is not + * released prematurely. */ + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, + sdp->index)) { + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); + } + } + } else { + /* A node previously upgraded to track a datapath bitmap is + * re-linked one datapath at a time. */ + if (!lrn->linked) { + /* First add of this (re-)link cycle: rebuild the tracked + * bitmap from scratch so it reflects the current datapath + * set and the current datapath array length (which may + * have grown or shrunk since the node was last linked). */ + size_t len = sparse_array_len(&sdp->dps->dps_array); + bitmap_free(lrn->dpgrp_bitmap); + lrn->dpgrp_bitmap = bitmap_allocate(len); + lrn->dpgrp_bitmap_len = len; + } else if (!bitmap_is_set(lrn->dpgrp_bitmap, sdp->index)) { + /* A second or later datapath this reference contributes in + * the same cycle. The first datapath was accounted for by + * the block below (which only runs on the first link of + * the cycle); account for this one too so that a datapath + * shared with another reference is not released + * prematurely. */ + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, + sdp->index)) { + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); + } + } + bitmap_set1(lrn->dpgrp_bitmap, sdp->index); + } } if (!lrn->linked) { - if (lrn->dpgrp_lflow) { + /* Allocate a reference counter only if the datapath(s) added by + * this reference are already used by the lflow. */ + if (sdp) { + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, sdp->index)) { + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); + } + } else { ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len); size_t index; BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) { @@ -793,11 +904,6 @@ lflow_table_add_lflow__(struct lflow_table *lflow_table, dp_refcnt_use(&lflow->dp_refcnts_map, index); } } - } else { - /* Allocate a reference counter only if already used. */ - if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, lrn->dp_index)) { - dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index); - } } } lrn->linked = true; @@ -1231,17 +1337,12 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, &lflow->dpg->dpg_uuid); if (!lflow->dpg->dp_group) { - /* Ideally this should not happen. But it can still happen - * due to 2 reasons: - * 1. There is a bug in the dp_group management. We should - * perhaps assert here. - * 2. A User or CMS may delete the logical_dp_groups in SB DB - * or clear the SB:Logical_flow.logical_dp_groups column - * (intentionally or accidentally) - * - * Because of (2) it is better to return false instead of - * assert,so that we recover from th inconsistent SB DB. - */ + /* The SB Logical_DP_Group row referenced by this in-memory dp + * group no longer exists. This can happen if a user or CMS + * deletes the SB:Logical_DP_Group rows or clears the + * SB:Logical_Flow.logical_dp_group column (intentionally or + * accidentally). Release the stale dp group and fall through + * to create a fresh one. */ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "SB Logical flow ["UUID_FMT"]'s " "logical_dp_group column is not set " @@ -1249,10 +1350,11 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, "referencing the dp group ["UUID_FMT"]", UUID_ARGS(&sbflow->header_.uuid), UUID_ARGS(&lflow->dpg->dpg_uuid)); - lflow->sync_state = LFLOW_STALE; - return false; + ovn_dp_group_release(dp_groups, lflow->dpg); + lflow->dpg = NULL; } - } else { + } + if (!lflow->dpg) { lflow->dpg = ovn_dp_group_create( ovnsb_txn, dp_groups, sbrec_dp_group, &lflow->dpg_bitmap, diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h index 84d0b3e67..e924ffc22 100644 --- a/northd/lflow-mgr.h +++ b/northd/lflow-mgr.h @@ -58,13 +58,7 @@ struct lflow_ref *lflow_ref_create(void); void lflow_ref_destroy(struct lflow_ref *); void lflow_ref_clear(struct lflow_ref *lflow_ref); void lflow_ref_unlink_lflows(struct lflow_ref *); -bool lflow_ref_resync_flows(struct lflow_ref *, - struct lflow_table *lflow_table, - struct ovsdb_idl_txn *ovnsb_txn, - const struct ovn_synced_datapaths dps[DP_MAX], - bool ovn_internal_version_changed, - const struct sbrec_logical_flow_table *, - const struct sbrec_logical_dp_group_table *); +void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table *); bool lflow_ref_sync_lflows(struct lflow_ref *, struct lflow_table *lflow_table, struct ovsdb_idl_txn *ovnsb_txn, diff --git a/northd/northd.c b/northd/northd.c index 47f76b108..fbc0068b7 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -20965,10 +20965,9 @@ void run_update_worker_pool(int n_threads) } } -/* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database, - * constructing their contents based on the OVN_NB database. */ -void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, - struct lflow_input *input_data, +/* Builds the in-memory logical flow table from the OVN_NB database. + * The flows are synced to the SB database by en_dp_group_resolved. */ +void build_lflows(struct lflow_input *input_data, struct lflow_table *lflows) { struct svc_monitors_map_data svc_mons_data = @@ -21012,14 +21011,6 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, /* Parallel build may result in a suboptimal hash. Resize the * lflow map to a correct size before doing lookups */ lflow_table_expand(lflows); - - stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); - lflow_table_sync_to_sb(lflows, ovnsb_txn, input_data->dps, - input_data->ovn_internal_version_changed, - input_data->sbrec_logical_flow_table, - input_data->sbrec_logical_dp_group_table); - - stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); } void @@ -21064,24 +21055,17 @@ lflow_reset_northd_refs(struct lflow_input *lflow_input) } } -bool -lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_dps *tracked_lrs, - struct lflow_input *lflow_input, - struct lflow_table *lflows) +void +lflow_handle_northd_lr_changes(struct tracked_dps *tracked_lrs, + struct lflow_input *lflow_input, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs) { - bool handled = true; struct hmapx_node *hmapx_node; HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->deleted) { struct ovn_datapath *od = hmapx_node->data; - handled = lflow_ref_resync_flows( - od->datapath_lflows, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - return handled; - } + lflow_ref_unlink_lflows(od->datapath_lflows); + hmapx_add(dirty_lflow_refs, od->datapath_lflows); } struct lswitch_flow_build_info lsi = { @@ -21099,34 +21083,18 @@ lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsb_txn, lflow_ref_unlink_lflows(od->datapath_lflows); build_lswitch_and_lrouter_iterate_by_lr(od, &lsi); - } - - /* We need to make sure that all datapath groups are allocated before - * trying to sync logical flows. Otherwise, we would need to recompute - * those datapath groups within those flows over and over again. */ - HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) { - struct ovn_datapath *od = hmapx_node->data; - - handled = lflow_ref_sync_lflows( - od->datapath_lflows, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - break; - } + hmapx_add(dirty_lflow_refs, od->datapath_lflows); } ds_destroy(&lsi.actions); ds_destroy(&lsi.match); - return handled; } -bool -lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_ovn_ports *trk_lsps, +void +lflow_handle_northd_port_changes(struct tracked_ovn_ports *trk_lsps, struct lflow_input *lflow_input, - struct lflow_table *lflows) + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs) { struct hmapx_node *hmapx_node; struct ovn_port *op; @@ -21135,14 +21103,8 @@ lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, op = hmapx_node->data; /* Make sure 'op' is an lsp and not lrp. */ ovs_assert(op->nbsp); - bool handled = lflow_ref_resync_flows( - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - return false; - } + lflow_ref_unlink_lflows(op->lflow_ref); + hmapx_add(dirty_lflow_refs, op->lflow_ref); /* No need to update SB multicast groups, thanks to weak * references. */ } @@ -21163,33 +21125,17 @@ lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, &match, &actions, lflow_input->svc_monitor_mac, lflows); - /* Sync the new flows to SB. */ - bool handled = lflow_ref_sync_lflows( - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (handled) { - /* Now regenerate the stateful lflows for 'op' */ - /* Clear old lflows. */ - lflow_ref_unlink_lflows(op->stateful_lflow_ref); - build_lbnat_lflows_iterate_by_lsp(op, - lflow_input->lr_stateful_table, - &match, &actions, lflows); - handled = lflow_ref_sync_lflows( - op->stateful_lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - } + hmapx_add(dirty_lflow_refs, op->lflow_ref); + + /* Now regenerate the stateful lflows for 'op' */ + lflow_ref_unlink_lflows(op->stateful_lflow_ref); + build_lbnat_lflows_iterate_by_lsp(op, + lflow_input->lr_stateful_table, + &match, &actions, lflows); + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); ds_destroy(&match); ds_destroy(&actions); - - if (!handled) { - return false; - } } HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) { @@ -21205,42 +21151,24 @@ lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, &match, &actions, lflow_input->svc_monitor_mac, lflows); + hmapx_add(dirty_lflow_refs, op->lflow_ref); - /* Sync the newly added flows to SB. */ - bool handled = lflow_ref_sync_lflows( - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (handled) { - /* Now generate the stateful lflows for 'op' */ - build_lbnat_lflows_iterate_by_lsp(op, - lflow_input->lr_stateful_table, - &match, &actions, lflows); - handled = lflow_ref_sync_lflows( - op->stateful_lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - } + /* Now generate the stateful lflows for 'op' */ + build_lbnat_lflows_iterate_by_lsp(op, + lflow_input->lr_stateful_table, + &match, &actions, lflows); + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); ds_destroy(&match); ds_destroy(&actions); - - if (!handled) { - return false; - } } - - return true; } -bool -lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_lbs *trk_lbs, +void +lflow_handle_northd_lb_changes(struct tracked_lbs *trk_lbs, struct lflow_input *lflow_input, - struct lflow_table *lflows) + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs) { struct ovn_lb_datapaths *lb_dps; struct hmapx_node *hmapx_node; @@ -21252,12 +21180,8 @@ lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { lb_dps = hmapx_node->data; - - lflow_ref_resync_flows( - lb_dps->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); + lflow_ref_unlink_lflows(lb_dps->lflow_ref); + hmapx_add(dirty_lflow_refs, lb_dps->lflow_ref); } HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) { @@ -21291,31 +21215,20 @@ lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, ds_destroy(&match); ds_destroy(&actions); - /* Sync the new flows to SB. */ - bool handled = lflow_ref_sync_lflows( - lb_dps->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - return false; - } + hmapx_add(dirty_lflow_refs, lb_dps->lflow_ref); } - - return true; } -bool -lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct lr_stateful_tracked_data *trk_data, - struct lflow_input *lflow_input, - struct lflow_table *lflows) +void +lflow_handle_lr_stateful_changes(struct lr_stateful_tracked_data *trk_data, + struct lflow_input *lflow_input, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs) { struct lr_stateful_record *lr_stateful_rec; struct ds actions = DS_EMPTY_INITIALIZER; struct ds match = DS_EMPTY_INITIALIZER; struct hmapx_node *hmapx_node; - bool handled = true; HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { lr_stateful_rec = hmapx_node->data; @@ -21328,17 +21241,7 @@ lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, &match, &actions, lflow_input->meter_groups, lflow_input->features); - - /* Sync the new flows to SB. */ - handled = lflow_ref_sync_lflows( - lr_stateful_rec->lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - goto exit; - } + hmapx_add(dirty_lflow_refs, lr_stateful_rec->lflow_ref); const struct ovn_datapath *od = ovn_datapaths_find_by_index(lflow_input->lr_datapaths, @@ -21353,16 +21256,7 @@ lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, lflow_input->bfd_ports, &match, &actions, lflows); - - handled = lflow_ref_sync_lflows( - op->stateful_lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - goto exit; - } + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); if (op->peer && op->peer->nbsp) { lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref); @@ -21370,32 +21264,20 @@ lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, build_lbnat_lflows_iterate_by_lsp( op->peer, lflow_input->lr_stateful_table, &match, &actions, lflows); - - handled = lflow_ref_sync_lflows( - op->peer->stateful_lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - goto exit; - } + hmapx_add(dirty_lflow_refs, op->peer->stateful_lflow_ref); } } } -exit: ds_destroy(&match); ds_destroy(&actions); - - return handled; } -bool -lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct ls_stateful_tracked_data *trk_data, - struct lflow_input *lflow_input, - struct lflow_table *lflows) +void +lflow_handle_ls_stateful_changes(struct ls_stateful_tracked_data *trk_data, + struct lflow_input *lflow_input, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs) { struct hmapx_node *hmapx_node; @@ -21420,39 +21302,14 @@ lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, build_network_function(od, lflows, lflow_input->ls_port_groups, ls_stateful_rec->lflow_ref); - } - - /* We need to make sure that all datapath groups are allocated before - * trying to sync logical flows. Otherwise, we would need to recompute - * those datapath groups within those flows over and over again. */ - HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { - struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; - /* Sync the new flows to SB. */ - bool handled = lflow_ref_sync_lflows( - ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table); - if (!handled) { - return false; - } + hmapx_add(dirty_lflow_refs, ls_stateful_rec->lflow_ref); } HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; - - if (!lflow_ref_resync_flows( - ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, - lflow_input->dps, - lflow_input->ovn_internal_version_changed, - lflow_input->sbrec_logical_flow_table, - lflow_input->sbrec_logical_dp_group_table)) { - return false; - } + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); + hmapx_add(dirty_lflow_refs, ls_stateful_rec->lflow_ref); } - - return true; } static bool diff --git a/northd/northd.h b/northd/northd.h index 2e3a9e00d..df2189c97 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -260,9 +260,7 @@ struct lflow_ref; struct lr_nat_table; struct lflow_input { - /* Southbound table references */ - const struct sbrec_logical_flow_table *sbrec_logical_flow_table; - const struct sbrec_logical_dp_group_table *sbrec_logical_dp_group_table; + /* Southbound table references (used during lflow computation). */ const struct sbrec_acl_id_table *sbrec_acl_id_table; /* Indexes */ @@ -979,8 +977,7 @@ struct lr_stateful_tracked_data; struct ls_stateful_tracked_data; struct group_ecmp_datapath; -void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, - struct lflow_input *input_data, +void build_lflows(struct lflow_input *input_data, struct lflow_table *); void lflow_reset_northd_refs(struct lflow_input *); void build_route_data_flows_for_lrouter( @@ -988,26 +985,26 @@ void build_route_data_flows_for_lrouter( const struct group_ecmp_datapath *route_node, const struct sset *bfd_ports); -bool lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsh_txn, - struct tracked_dps *, - struct lflow_input *, - struct lflow_table *lflows); -bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_ovn_ports *, +void lflow_handle_northd_lr_changes(struct tracked_dps *, + struct lflow_input *, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs); +void lflow_handle_northd_port_changes(struct tracked_ovn_ports *, struct lflow_input *, - struct lflow_table *lflows); -bool lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_lbs *, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs); +void lflow_handle_northd_lb_changes(struct tracked_lbs *, struct lflow_input *, - struct lflow_table *lflows); -bool lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *, - struct lr_stateful_tracked_data *, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs); +void lflow_handle_lr_stateful_changes(struct lr_stateful_tracked_data *, struct lflow_input *, - struct lflow_table *lflows); -bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, - struct ls_stateful_tracked_data *, + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs); +void lflow_handle_ls_stateful_changes(struct ls_stateful_tracked_data *, struct lflow_input *, - struct lflow_table *lflows); + struct lflow_table *lflows, + struct hmapx *dirty_lflow_refs); bool northd_handle_sb_port_binding_changes( const struct sbrec_port_binding_table *, struct hmap *ls_ports, struct hmap *lr_ports); diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ovn-inc-proc-graph-dump.at index 44bf5689e..af951da8d 100644 --- a/tests/ovn-inc-proc-graph-dump.at +++ b/tests/ovn-inc-proc-graph-dump.at @@ -143,7 +143,6 @@ digraph "Incremental-Processing-Engine" { sync_meters -> sync_to_sb [[label=""]]; sync_to_sb_lb -> sync_to_sb [[label=""]]; sync_to_sb_pb -> sync_to_sb [[label=""]]; - SB_logical_flow [[style=filled, shape=box, fillcolor=white, label="SB_logical_flow"]]; SB_multicast_group [[style=filled, shape=box, fillcolor=white, label="SB_multicast_group"]]; NB_bfd [[style=filled, shape=box, fillcolor=white, label="NB_bfd"]]; SB_bfd [[style=filled, shape=box, fillcolor=white, label="SB_bfd"]]; @@ -186,9 +185,7 @@ digraph "Incremental-Processing-Engine" { SB_igmp_group -> multicast_igmp [[label=""]]; lflow [[style=filled, shape=box, fillcolor=white, label="lflow"]]; sync_meters -> lflow [[label=""]]; - SB_logical_flow -> lflow [[label=""]]; SB_multicast_group -> lflow [[label=""]]; - SB_logical_dp_group -> lflow [[label=""]]; bfd_sync -> lflow [[label=""]]; route_policies -> lflow [[label=""]]; routes -> lflow [[label="engine_noop_handler"]]; @@ -203,6 +200,13 @@ digraph "Incremental-Processing-Engine" { multicast_igmp -> lflow [[label="lflow_multicast_igmp_handler"]]; SB_acl_id -> lflow [[label=""]]; ic_learned_svc_monitors -> lflow [[label="lflow_ic_learned_svc_mons_handler"]]; + SB_logical_flow [[style=filled, shape=box, fillcolor=white, label="SB_logical_flow"]]; + dp_group_resolved [[style=filled, shape=box, fillcolor=white, label="dp_group_resolved"]]; + lflow -> dp_group_resolved [[label="dp_group_resolved_lflow_handler"]]; + SB_logical_flow -> dp_group_resolved [[label=""]]; + SB_logical_dp_group -> dp_group_resolved [[label=""]]; + datapath_sync -> dp_group_resolved [[label="engine_noop_handler"]]; + global_config -> dp_group_resolved [[label="engine_noop_handler"]]; mac_binding_aging_waker [[style=filled, shape=box, fillcolor=white, label="mac_binding_aging_waker"]]; mac_binding_aging [[style=filled, shape=box, fillcolor=white, label="mac_binding_aging"]]; SB_mac_binding -> mac_binding_aging [[label=""]]; @@ -237,7 +241,7 @@ digraph "Incremental-Processing-Engine" { acl_id -> northd_output [[label="northd_output_acl_id_handler"]]; sync_from_sb -> northd_output [[label=""]]; sync_to_sb -> northd_output [[label="northd_output_sync_to_sb_handler"]]; - lflow -> northd_output [[label="northd_output_lflow_handler"]]; + dp_group_resolved -> northd_output [[label="northd_output_dp_group_resolved_handler"]]; mac_binding_aging -> northd_output [[label="northd_output_mac_binding_aging_handler"]]; fdb_aging -> northd_output [[label="northd_output_fdb_aging_handler"]]; ecmp_nexthop -> northd_output [[label="northd_output_ecmp_nexthop_handler"]]; diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d19978a62..2a070901b 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -13266,17 +13266,7 @@ check ovn-nbctl --wait=sb set load_balancer lb1 options:bar=foo check_engine_stats lflow norecompute compute CHECK_NO_CHANGE_AFTER_RECOMPUTE -# Clear the SB:Logical_Flow.logical_dp_groups column of all the -# logical flows and then modify the NB:Load_balancer. ovn-northd -# should resync the logical flows. -for l in $(ovn-sbctl --bare --columns _uuid list logical_flow) -do - check ovn-sbctl clear logical_flow $l logical_dp_group -done - -check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats check ovn-nbctl --wait=sb set load_balancer lb1 options:foo=bar -check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE lb_lflow_uuid=$(fetch_column Logical_flow _uuid match='"ct.new && ip4.dst == 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80"') @@ -13445,16 +13435,7 @@ AT_CHECK([echo $dpgrp_dps | grep $sw3_uuid], [0], [ignore]) AT_CHECK([echo $dpgrp_dps | grep $sw4_uuid], [0], [ignore]) AT_CHECK([echo $dpgrp_dps | grep $sw5_uuid], [0], [ignore]) -# Clear the SB:Logical_Flow.logical_dp_groups column of all the -# logical flows and then modify the NB:Load_balancer. ovn-northd -# should resync the logical flows. -for l in $(ovn-sbctl --bare --columns _uuid list logical_flow) -do - check ovn-sbctl clear logical_flow $l logical_dp_group -done - check ovn-nbctl --wait=sb set load_balancer lb2 vips='{"10.0.0.10:80"="10.0.0.3:80,10.0.0.4:80"}' -check_engine_stats lflow recompute compute CHECK_NO_CHANGE_AFTER_RECOMPUTE lb_lflow_uuid=$(fetch_column Logical_flow _uuid match='"ct.new && ip4.dst == 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80"') @@ -23883,3 +23864,65 @@ AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/enable-stopwatch nonexis OVN_CLEANUP_NORTHD AT_CLEANUP ]) + +AT_SETUP([Datapath group SB writes only when bitmap changes]) +ovn_start + +# Create three switches, each with an ACL so they share the +# "has ACLs" lflows in the same datapath group {sw0, sw1, sw2}. +check ovn-nbctl ls-add sw0 +check ovn-nbctl ls-add sw1 +check ovn-nbctl ls-add sw2 +check ovn-nbctl lsp-add sw0 sw0-p0 +check ovn-nbctl lsp-add sw1 sw1-p0 +check ovn-nbctl lsp-add sw2 sw2-p0 +check ovn-nbctl acl-add sw0 to-lport 1000 ip4 allow +check ovn-nbctl acl-add sw1 to-lport 1000 ip6 allow +check ovn-nbctl --wait=sb acl-add sw2 to-lport 1000 tcp allow + +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Record dp_group state (UUIDs + content). +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ + | sort > dpg_before + +# ---------------------------------------------------------- +# Case 1: Change that triggers incremental lflow compute but +# does NOT alter the dp-group bitmap. +# +# Setting a port address rebuilds the port's lflows but the +# set of datapaths that share the "has ACLs" lflows is still +# {sw0, sw1, sw2}. The dp_group rows should not change. +# ---------------------------------------------------------- +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb lsp-set-addresses sw0-p0 "00:00:00:00:00:01" +check_engine_stats lflow norecompute compute + +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ + | sort > dpg_after +AT_CHECK([diff dpg_before dpg_after]) + +# ---------------------------------------------------------- +# Case 2: Change that removes a datapath from a shared flow, +# altering the dp-group bitmap. +# +# Move sw0's ACL to tier 1. The egress "acl action" flows +# now differ for sw0, so the {sw0, sw1, sw2} group loses sw0 +# and a {sw1, sw2} group appears (or the existing one is +# updated). The dp_group rows must change. +# ---------------------------------------------------------- +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +acl0=$(fetch_column nb:ACL _uuid match=ip4) +check ovn-nbctl --wait=sb set ACL $acl0 tier=1 +check_engine_stats lflow norecompute compute + +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ + | sort > dpg_changed +AT_CHECK([diff dpg_before dpg_changed], [1], [ignore]) + +check ovn-nbctl --wait=sb ls-del sw0 + +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
