On Mon, Nov 27, 2023 at 6:38 PM <num...@ovn.org> wrote:
>
> From: Numan Siddique <num...@ovn.org>
>
> Since northd tracked data has the changed lb data, northd
> engine handler for lflow engine now handles the lb changes
> incrementally. All the lflows generated for each lb is
> stored in the ovn_lb_datapaths->lflow_ref and this is used
> similar to how we handle ovn_port changes.
>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> northd/en-lflow.c | 11 +++---
> northd/lflow-mgr.c | 41 ++++++++++++++++++-
> northd/lflow-mgr.h | 3 ++
> northd/northd.c | 95 +++++++++++++++++++++++++++++++++++++++++----
> northd/northd.h | 28 +++++++++++++
> tests/ovn-northd.at | 30 ++++++++++----
> 6 files changed, 186 insertions(+), 22 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 65d2f45ebc..13284b5556 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -125,11 +125,6 @@ lflow_northd_handler(struct engine_node *node,
> return false;
> }
>
> - /* Fall back to recompute if load balancers have changed. */
> - if (northd_has_lbs_in_tracked_data(&northd_data->trk_data)) {
> - return false;
> - }
> -
> const struct engine_context *eng_ctx = engine_get_context();
> struct lflow_data *lflow_data = data;
>
> @@ -142,6 +137,12 @@ lflow_northd_handler(struct engine_node *node,
> return false;
> }
>
> + if (!lflow_handle_northd_lb_changes(eng_ctx->ovnsb_idl_txn,
> + &northd_data->trk_data.trk_lbs,
> + &lflow_input, lflow_data->lflow_table)) {
> + return false;
> + }
> +
> engine_set_node_state(node, EN_UPDATED);
> return true;
> }
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index 08962e9172..d779e7e087 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -105,6 +105,10 @@ static void ovn_dp_group_add_with_reference(struct
ovn_lflow *,
> size_t bitmap_len);
>
> static void unlink_lflows_from_datapath(struct lflow_ref *);
> +static void unlink_lflows_from_all_datapaths(struct lflow_ref *,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths);
> +
> static void lflow_ref_sync_lflows_to_sb__(struct lflow_ref *,
> struct lflow_table *,
> struct ovsdb_idl_txn *ovnsb_txn,
> @@ -394,6 +398,15 @@ lflow_ref_clear_lflows(struct lflow_ref *lflow_ref)
> unlink_lflows_from_datapath(lflow_ref);
> }
>
> +void
> +lflow_ref_clear_lflows_for_all_dps(struct lflow_ref *lflow_ref,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths)
> +{
> + unlink_lflows_from_all_datapaths(lflow_ref, n_ls_datapaths,
> + n_lr_datapaths);
> +}
> +
> void
> lflow_ref_clear_and_sync_lflows(struct lflow_ref *lflow_ref,
> struct lflow_table *lflow_table,
> @@ -462,7 +475,9 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
> /* lflow_ref_node for this lflow doesn't exist yet. Add
it. */
> struct lflow_ref_node *ref_node = xzalloc(sizeof *ref_node);
> ref_node->lflow = lflow;
> - ref_node->dp_index = od->index;
> + if (od) {
> + ref_node->dp_index = od->index;
> + }
> ovs_list_insert(&lflow_ref->lflows_ref_list,
> &ref_node->ref_list_node);
>
> @@ -1047,6 +1062,30 @@ unlink_lflows_from_datapath(struct lflow_ref
*lflow_ref)
> }
> }
>
> +static void
> +unlink_lflows_from_all_datapaths(struct lflow_ref *lflow_ref,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths)
> +{
> + struct lflow_ref_node *ref_node;
> + struct ovn_lflow *lflow;
> + LIST_FOR_EACH (ref_node, ref_list_node, &lflow_ref->lflows_ref_list)
{
> + size_t n_datapaths;
> + size_t index;
> +
> + lflow = ref_node->lflow;
> + if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> + n_datapaths = n_ls_datapaths;
> + } else {
> + n_datapaths = n_lr_datapaths;
> + }
> +
> + BITMAP_FOR_EACH_1 (index, n_datapaths, lflow->dpg_bitmap) {
> + bitmap_set0(lflow->dpg_bitmap, index);
> + }
It would be way more efficient to replace the loop by ULLONG_SET0.
Thanks,
Han
> + }
> +}
> +
> static void
> lflow_ref_sync_lflows_to_sb__(struct lflow_ref *lflow_ref,
> struct lflow_table *lflow_table,
> diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
> index 02b74aa131..c65cd70e71 100644
> --- a/northd/lflow-mgr.h
> +++ b/northd/lflow-mgr.h
> @@ -54,6 +54,9 @@ struct lflow_ref *lflow_ref_alloc(const char *res_name);
> void lflow_ref_destroy(struct lflow_ref *);
> void lflow_ref_reset(struct lflow_ref *lflow_ref);
> void lflow_ref_clear_lflows(struct lflow_ref *);
> +void lflow_ref_clear_lflows_for_all_dps(struct lflow_ref *,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths);
> void lflow_ref_clear_and_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 104068e293..f56916b3da 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3523,6 +3523,7 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb
*lb, size_t n_ls_datapaths,
> lb_dps->lb = lb;
> lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> + lb_dps->lflow_ref = lflow_ref_alloc(lb->nlb->name);
>
> return lb_dps;
> }
> @@ -3532,6 +3533,7 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths
*lb_dps)
> {
> bitmap_free(lb_dps->nb_lr_map);
> bitmap_free(lb_dps->nb_ls_map);
> + lflow_ref_destroy(lb_dps->lflow_ref);
> free(lb_dps);
> }
>
> @@ -16063,11 +16065,11 @@ build_lflows_thread(void *arg)
> lsi->lflows,
> &lsi->match,
> &lsi->actions,
> - NULL);
> +
lb_dps->lflow_ref);
> build_lrouter_defrag_flows_for_lb(lb_dps,
lsi->lflows,
> lsi->lr_datapaths,
> &lsi->match,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_flows_for_lb(lb_dps, lsi->lflows,
> lsi->meter_groups,
> lsi->lr_datapaths,
> @@ -16075,14 +16077,14 @@ build_lflows_thread(void *arg)
> lsi->features,
> lsi->svc_monitor_map,
> &lsi->match,
&lsi->actions,
> - NULL);
> + lb_dps->lflow_ref);
> build_lswitch_flows_for_lb(lb_dps, lsi->lflows,
> lsi->meter_groups,
> lsi->ls_datapaths,
> lsi->features,
> lsi->svc_monitor_map,
> &lsi->match,
&lsi->actions,
> - NULL);
> + lb_dps->lflow_ref);
> }
> }
> for (bnum = control->id;
> @@ -16298,20 +16300,20 @@ build_lswitch_and_lrouter_flows(const struct
ovn_datapaths *ls_datapaths,
> build_lswitch_arp_nd_service_monitor(lb_dps->lb,
lsi.ls_ports,
> lsi.lflows,
&lsi.actions,
> &lsi.match,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_defrag_flows_for_lb(lb_dps, lsi.lflows,
> lsi.lr_datapaths,
&lsi.match,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_flows_for_lb(lb_dps, lsi.lflows,
lsi.meter_groups,
> lsi.lr_datapaths,
lsi.lr_sful_table,
> lsi.features, lsi.svc_monitor_map,
> &lsi.match, &lsi.actions,
> - NULL);
> + lb_dps->lflow_ref);
> build_lswitch_flows_for_lb(lb_dps, lsi.lflows,
lsi.meter_groups,
> lsi.ls_datapaths, lsi.features,
> lsi.svc_monitor_map,
> &lsi.match, &lsi.actions,
> - NULL);
> + lb_dps->lflow_ref);
> }
> stopwatch_stop(LFLOWS_LBS_STOPWATCH_NAME, time_msec());
>
> @@ -16483,11 +16485,17 @@ void build_lflows(struct ovsdb_idl_txn
*ovnsb_txn,
> void
> reset_lflow_refs_for_northd_resources(struct lflow_input *lflow_input)
> {
> + struct ovn_lb_datapaths *lb_dps;
> struct ovn_port *op;
> +
> HMAP_FOR_EACH (op, key_node, lflow_input->ls_ports) {
> lflow_ref_reset(op->lflow_ref);
> lflow_ref_reset(op->stateful_lflow_ref);
> }
> +
> + HMAP_FOR_EACH (lb_dps, hmap_node, lflow_input->lb_datapaths_map) {
> + lflow_ref_reset(lb_dps->lflow_ref);
> + }
> }
>
> bool
> @@ -16626,6 +16634,77 @@ lflow_handle_northd_port_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> return true;
> }
>
> +bool
> +lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn,
> + struct tracked_lbs *trk_lbs,
> + struct lflow_input *lflow_input,
> + struct lflow_table *lflows)
> +{
> + struct ovn_lb_datapaths *lb_dps;
> + struct hmapx_node *hmapx_node;
> + HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) {
> + lb_dps = hmapx_node->data;
> +
> + lflow_ref_clear_lflows_for_all_dps(lb_dps->lflow_ref,
> +
ods_size(lflow_input->ls_datapaths),
> +
ods_size(lflow_input->lr_datapaths));
> + lflow_ref_sync_lflows_to_sb(lb_dps->lflow_ref, lflows, ovnsb_txn,
> + lflow_input->ls_datapaths,
> + lflow_input->lr_datapaths,
> + lflow_input->ovn_internal_version_changed,
> + lflow_input->sbrec_logical_flow_table,
> + lflow_input->sbrec_logical_dp_group_table);
> + }
> +
> + HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) {
> + lb_dps = hmapx_node->data;
> +
> + /* unlink old lflows. */
> + lflow_ref_clear_lflows_for_all_dps(lb_dps->lflow_ref,
> +
ods_size(lflow_input->ls_datapaths),
> +
ods_size(lflow_input->lr_datapaths));
> +
> + /* Generate new lflows. */
> + struct ds match = DS_EMPTY_INITIALIZER;
> + struct ds actions = DS_EMPTY_INITIALIZER;
> +
> + build_lswitch_arp_nd_service_monitor(lb_dps->lb,
lflow_input->ls_ports,
> + lflows, &actions,
> + &match, lb_dps->lflow_ref);
> + build_lrouter_defrag_flows_for_lb(lb_dps, lflows,
> + lflow_input->lr_datapaths,
&match,
> + lb_dps->lflow_ref);
> + build_lrouter_flows_for_lb(lb_dps, lflows,
> + lflow_input->meter_groups,
> + lflow_input->lr_datapaths,
> + lflow_input->lr_sful_table,
> + lflow_input->features,
> + lflow_input->svc_monitor_map,
> + &match, &actions,
> + lb_dps->lflow_ref);
> + build_lswitch_flows_for_lb(lb_dps, lflows,
> + lflow_input->meter_groups,
> + lflow_input->ls_datapaths,
> + lflow_input->features,
> + lflow_input->svc_monitor_map,
> + &match, &actions,
> + lb_dps->lflow_ref);
> +
> + ds_destroy(&match);
> + ds_destroy(&actions);
> +
> + /* Sync the new flows to SB. */
> + lflow_ref_sync_lflows_to_sb(lb_dps->lflow_ref, lflows, ovnsb_txn,
> + lflow_input->ls_datapaths,
> + lflow_input->lr_datapaths,
> + lflow_input->ovn_internal_version_changed,
> + lflow_input->sbrec_logical_flow_table,
> + lflow_input->sbrec_logical_dp_group_table);
> + }
> +
> + return true;
> +}
> +
> static bool
> mirror_needs_update(const struct nbrec_mirror *nb_mirror,
> const struct sbrec_mirror *sb_mirror)
> diff --git a/northd/northd.h b/northd/northd.h
> index e2df53e6f8..863bcce001 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -101,6 +101,30 @@ struct ovn_lb_datapaths {
>
> size_t n_nb_lr;
> unsigned long *nb_lr_map;
> +
> + /* Reference of lflows generated for this load balancer.
> + *
> + * This data is initialized and destroyed by the en_northd node, but
> + * populated and used only by the en_lflow node. Ideally this data
should
> + * be maintained as part of en_lflow's data (struct lflow_data): a
hash
> + * index from ovn_port key to lflows. However, it would be less
efficient
> + * and more complex:
> + *
> + * 1. It would require an extra search (using the index) to find the
> + * lflows.
> + *
> + * 2. Building the index needs to be thread-safe, using either a
global
> + * lock which is obviously less efficient, or hash-based lock array
which
> + * is more complex.
> + *
> + * Maintaining the lflow_ref here is more straightforward. The
drawback is
> + * that we need to keep in mind that this data belongs to en_lflow
node,
> + * so never access it from any other nodes.
> + *
> + * 'lflow_ref' is used to reference logical flows generated for this
> + * load balancer.
> + */
> + struct lflow_ref *lflow_ref;
> };
>
> struct ovn_lb_group_datapaths {
> @@ -700,6 +724,10 @@ bool lflow_handle_northd_port_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> 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_input *,
> + struct lflow_table *lflows);
> 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-northd.at b/tests/ovn-northd.at
> index 03d62695db..50d88fecd5 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10507,7 +10507,7 @@ check ovn-nbctl --wait=sb lb-add lb1 10.0.0.10:80
10.0.0.3:80
>
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
>
> CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
> @@ -10517,21 +10517,26 @@ check ovn-nbctl --wait=sb set load_balancer .
ip_port_mappings:10.0.0.3=sw0-p1:1
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>
> check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80 --
lb-add lb3 30.0.0.10:80 30.0.0.20:80
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
>
> CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
> @@ -10541,7 +10546,7 @@ check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del
lb3
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
>
> CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
> @@ -10753,8 +10758,9 @@ check ovn-nbctl --wait=sb add load_balancer_group
. load_Balancer $lb1_uuid
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb clear load_balancer_group . load_Balancer
> @@ -10769,7 +10775,7 @@ check as northd ovn-appctl -t NORTHD_TYPE
inc-engine/clear-stats
> check ovn-nbctl --wait=sb add load_balancer_group . load_Balancer
$lb1_uuid
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> @@ -10778,6 +10784,7 @@ check ovn-nbctl --wait=sb add logical_switch sw0
load_balancer_group $lbg1_uuid
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> +check_engine_stats ls_stateful norecompute compute
> check_engine_stats lflow recompute nocompute
> check_engine_stats sync_to_sb_lb recompute compute
>
> @@ -10786,6 +10793,7 @@ check as northd ovn-appctl -t NORTHD_TYPE
inc-engine/clear-stats
> check ovn-nbctl --wait=sb set load_balancer . options:bar=foo
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> +check_engine_stats ls_stateful norecompute compute
> check_engine_stats lflow recompute nocompute
> check_engine_stats sync_to_sb_lb recompute compute
>
> @@ -10795,6 +10803,7 @@ check ovn-nbctl --wait=sb set load_balancer lb1
vips:'"10.0.0.10:80"'='"10.0.0.1
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> +check_engine_stats ls_stateful norecompute compute
> check_engine_stats lflow recompute nocompute
> check_engine_stats sync_to_sb_lb recompute compute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> @@ -10892,6 +10901,7 @@ check_engine_stats northd recompute nocompute
> check_engine_stats lr_stateful recompute nocompute
> check_engine_stats lflow recompute nocompute
> check_engine_stats sync_to_sb_lb recompute compute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> # Add back lb group to logical switch and then delete it.
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> @@ -10901,6 +10911,7 @@ check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> check_engine_stats lflow recompute nocompute
> check_engine_stats sync_to_sb_lb recompute compute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb clear logical_switch sw0 load_balancer_group
-- \
> @@ -10934,14 +10945,17 @@ check_engine_stats northd norecompute compute
> check_engine_stats lr_stateful norecompute compute
> check_engine_stats lflow norecompute nocompute
> check_engine_stats sync_to_sb_lb norecompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb set load_balancer_group .
load_balancer="$lb2_uuid,$lb3_uuid,$lb4_uuid"
> check_engine_stats lb_data norecompute compute
> check_engine_stats northd norecompute compute
> +check_engine_stats ls_stateful norecompute compute
> check_engine_stats lr_stateful norecompute compute
> -check_engine_stats lflow recompute nocompute
> +check_engine_stats lflow norecompute compute
> check_engine_stats sync_to_sb_lb recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb set logical_switch sw0
load_balancer_group=$lbg1_uuid
> --
> 2.41.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
