[ovs-dev] [PATCH ovn v2] Stop setting Load_Balancer references in SB Datapath_Binding records.
Numan Siddique
numans at ovn.org
Mon Jul 26 20:38:03 UTC 2021
On Mon, Jul 26, 2021 at 10:01 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> Commit 6aab704fb67d ("controller: Avoid unnecessary load balancer flow
> processing.") handled the case when load balancers are updated but
> addition/removal of load balancers will trigger almost all logical
> flows to be reprocessed by ovn-controller.
>
> The references are just used for optimizing lookups in ovn-controller
> but we can avoid them all together.
>
> Reported-at: https://bugzilla.redhat.com/1978605
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
> On a large scale deployment (simulating ovn-kubernetes) with 120 nodes,
> and 16K load balancer VIPs associated to each node's logical switch,
> when adding a new load balancer (just one VIP) inc_proc_eng debug logs
> show:
>
> - without fix:
> 2021-07-23T08:49:34.811Z|43363|inc_proc_eng|DBG|node: logical_flow_output, handler for input SB_logical_flow took 16824ms
> 2021-07-23T08:49:34.813Z|43364|inc_proc_eng|DBG|node: logical_flow_output, handler for input SB_logical_dp_group took 0ms
> 2021-07-23T08:49:44.018Z|43365|inc_proc_eng|DBG|node: logical_flow_output, handler for input SB_load_balancer took 9205ms
>
> - with fix:
> 2021-07-23T16:24:12.284Z|17720|inc_proc_eng|DBG|node: logical_flow_output, handler for input SB_logical_flow took 163ms
> 2021-07-23T16:24:12.284Z|17721|inc_proc_eng|DBG|node: logical_flow_output, handler for input SB_logical_dp_group took 0ms
> 2021-07-23T16:24:12.285Z|17722|inc_proc_eng|DBG|node: logical_flow_output, handler for input SB_load_balancer took 2ms
>
> v2:
> - Addressed Mark's and Numan's comments.
> - Note: Did not add Mark's ack because there are quite a few changes
> between v1 and v2.
> - Add reported-at tag to ease up downstream tracking.
> - Set SB Datapath_Binding.load_balancers only if it's not empty.
Thanks for the v2.
Acked-by: Numan Siddique <numans at ovn.org>
There is one small minor nit below. I don't think there is a need to submit v3.
Whoever decides to commit it can modify it.
Thanks
Numan
> ---
> controller/lflow.c | 9 ++-
> controller/lflow.h | 3 +
> controller/ovn-controller.c | 114 +++++++++++++++++++++++++++++++++++-
> lib/ovn-util.c | 2 +-
> northd/ovn-northd.c | 17 ++----
> northd/ovn_northd.dl | 14 +++--
> ovn-sb.xml | 2 +-
> tests/ovn-northd.at | 12 ++--
> 8 files changed, 140 insertions(+), 33 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 67bc20203..1eed1cccc 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1713,6 +1713,8 @@ lflow_destroy(void)
>
> bool
> lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
> + const struct sbrec_load_balancer **dp_lbs,
> + size_t n_dp_lbs,
> struct lflow_ctx_in *l_ctx_in,
> struct lflow_ctx_out *l_ctx_out)
> {
> @@ -1796,11 +1798,8 @@ lflow_processing_end:
>
> /* Add load balancer hairpin flows if the datapath has any load balancers
> * associated. */
> - for (size_t i = 0; i < dp->n_load_balancers; i++) {
> - const struct sbrec_load_balancer *lb =
> - sbrec_load_balancer_table_get_for_uuid(l_ctx_in->lb_table,
> - &dp->load_balancers[i]);
> - consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> + for (size_t i = 0; i < n_dp_lbs; i++) {
> + consider_lb_hairpin_flows(dp_lbs[i], l_ctx_in->local_datapaths,
> l_ctx_out->flow_table);
> }
>
> diff --git a/controller/lflow.h b/controller/lflow.h
> index c17ff6dd4..c5af197d1 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -47,6 +47,7 @@ struct hmap_node;
> struct sbrec_chassis;
> struct sbrec_dhcp_options_table;
> struct sbrec_dhcpv6_options_table;
> +struct sbrec_load_balancer;
> struct sbrec_logical_flow_table;
> struct sbrec_mac_binding_table;
> struct sbrec_datapath_binding;
> @@ -176,6 +177,8 @@ bool lflow_handle_changed_fdbs(struct lflow_ctx_in *, struct lflow_ctx_out *);
> void lflow_destroy(void);
>
> bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
> + const struct sbrec_load_balancer **dp_lbs,
> + size_t n_dp_lbs,
> struct lflow_ctx_in *,
> struct lflow_ctx_out *);
> bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 3a9bdbc97..088c3f575 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1961,6 +1961,105 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data)
> engine_set_node_state(node, EN_UNCHANGED);
> }
>
> +/* Stores the load balancers that are applied to the datapath 'dp'. */
> +struct load_balancers_by_dp {
> + struct hmap_node node;
> + const struct sbrec_datapath_binding *dp;
> + const struct sbrec_load_balancer **dp_lbs;
> + size_t n_allocated_dp_lbs;
> + size_t n_dp_lbs;
> +};
> +
> +static struct load_balancers_by_dp *
> +load_balancers_by_dp_create(struct hmap *lbs,
> + const struct sbrec_datapath_binding *dp)
> +{
> + struct load_balancers_by_dp *lbs_by_dp = xzalloc(sizeof *lbs_by_dp);
> +
> + lbs_by_dp->dp = dp;
> + hmap_insert(lbs, &lbs_by_dp->node, hash_uint64(dp->tunnel_key));
> + return lbs_by_dp;
> +}
> +
> +static void
> +load_balancers_by_dp_destroy(struct load_balancers_by_dp *lbs_by_dp)
> +{
> + if (!lbs_by_dp) {
> + return;
> + }
> +
> + free(lbs_by_dp->dp_lbs);
> + free(lbs_by_dp);
> +}
> +
> +static struct load_balancers_by_dp *
> +load_balancers_by_dp_find(struct hmap *lbs,
> + const struct sbrec_datapath_binding *dp)
> +{
> + uint32_t hash = hash_uint64(dp->tunnel_key);
> + struct load_balancers_by_dp *lbs_by_dp;
> +
> + HMAP_FOR_EACH_WITH_HASH (lbs_by_dp, node, hash, lbs) {
> + if (lbs_by_dp->dp == dp) {
> + return lbs_by_dp;
> + }
> + }
> + return NULL;
> +}
> +
> +/* Builds and returns a hmap of 'load_balancers_by_dp', one record for each
> + * local datapath.
> + */
> +static struct hmap *
> +load_balancers_by_dp_init(const struct hmap *local_datapaths,
> + const struct sbrec_load_balancer_table *lb_table)
> +{
> + struct hmap *lbs = xmalloc(sizeof *lbs);
> + hmap_init(lbs);
> +
> + const struct sbrec_load_balancer *lb;
> + SBREC_LOAD_BALANCER_TABLE_FOR_EACH (lb, lb_table) {
> + for (size_t i = 0; i < lb->n_datapaths; i++) {
> + struct local_datapath *ldp =
> + get_local_datapath(local_datapaths,
> + lb->datapaths[i]->tunnel_key);
> + if (!ldp) {
> + continue;
> + }
> +
> + struct load_balancers_by_dp *lbs_by_dp =
> + load_balancers_by_dp_find(lbs, ldp->datapath);
> + if (!lbs_by_dp) {
> + lbs_by_dp = load_balancers_by_dp_create(lbs, ldp->datapath);
> + }
> +
> + if (lbs_by_dp->n_dp_lbs == lbs_by_dp->n_allocated_dp_lbs) {
> + lbs_by_dp->dp_lbs = x2nrealloc(lbs_by_dp->dp_lbs,
> + &lbs_by_dp->n_allocated_dp_lbs,
> + sizeof *lbs_by_dp->dp_lbs);
> + }
> + lbs_by_dp->dp_lbs[lbs_by_dp->n_dp_lbs++] = lb;
> + }
> + }
> + return lbs;
> +}
> +
> +static void
> +load_balancers_by_dp_cleanup(struct hmap *lbs)
> +{
> + struct load_balancers_by_dp *lbs_by_dp;
The above declaration can be moved after the below if(!lbs).
Thanks
Numan
> +
> + if (!lbs) {
> + return;
> + }
> +
> + HMAP_FOR_EACH_POP (lbs_by_dp, node, lbs) {
> + load_balancers_by_dp_destroy(lbs_by_dp);
> + }
> + hmap_destroy(lbs);
> + free(lbs);
> +}
> +
> struct lflow_output_persistent_data {
> uint32_t conj_id_ofs;
> struct lflow_cache *lflow_cache;
> @@ -2429,13 +2528,23 @@ lflow_output_runtime_data_handler(struct engine_node *node,
> struct lflow_ctx_in l_ctx_in;
> struct lflow_ctx_out l_ctx_out;
> struct ed_type_lflow_output *fo = data;
> + struct hmap *lbs = NULL;
> init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
>
> struct tracked_binding_datapath *tdp;
> HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> if (tdp->is_new) {
> - if (!lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
> - &l_ctx_out)) {
> + if (!lbs) {
> + lbs = load_balancers_by_dp_init(&rt_data->local_datapaths,
> + l_ctx_in.lb_table);
> + }
> +
> + struct load_balancers_by_dp *lbs_by_dp =
> + load_balancers_by_dp_find(lbs, tdp->dp);
> + if (!lflow_add_flows_for_datapath(
> + tdp->dp, lbs_by_dp ? lbs_by_dp->dp_lbs : NULL,
> + lbs_by_dp ? lbs_by_dp->n_dp_lbs : 0,
> + &l_ctx_in, &l_ctx_out)) {
> return false;
> }
> } else {
> @@ -2450,6 +2559,7 @@ lflow_output_runtime_data_handler(struct engine_node *node,
> }
> }
>
> + load_balancers_by_dp_cleanup(lbs);
> engine_set_node_state(node, EN_UPDATED);
> return true;
> }
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 494d6d42d..3805923c8 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -760,7 +760,7 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address,
>
> /* Increment this for any logical flow changes, if an existing OVN action is
> * modified or a stage is added to a logical pipeline. */
> -#define OVN_INTERNAL_MINOR_VER 1
> +#define OVN_INTERNAL_MINOR_VER 2
>
> /* Returns the OVN version. The caller must free the returned value. */
> char *
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index ebe12cace..ab9000899 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3632,24 +3632,17 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
> free(lb_dps);
> }
>
> - /* Set the list of associated load balanacers to a logical switch
> - * datapath binding in the SB DB. */
> + /* Datapath_Binding.load_balancers is not used anymore, it's still in the
> + * schema for compatibility reasons. Reset it to empty, just in case.
> + */
> HMAP_FOR_EACH (od, key_node, datapaths) {
> if (!od->nbs) {
> continue;
> }
>
> - struct uuid *lb_uuids =
> - xmalloc(od->nbs->n_load_balancer * sizeof *lb_uuids);
> - for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
> - const struct uuid *lb_uuid =
> - &od->nbs->load_balancer[i]->header_.uuid;
> - lb = ovn_northd_lb_find(lbs, lb_uuid);
> - lb_uuids[i] = lb->slb->header_.uuid;
> + if (od->sb->n_load_balancers) {
> + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
> }
> - sbrec_datapath_binding_set_load_balancers(od->sb, lb_uuids,
> - od->nbs->n_load_balancer);
> - free(lb_uuids);
> }
> }
>
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 7bfaae992..6ddf19677 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -54,14 +54,12 @@ sb::Out_Meter(._uuid = hash128(name),
> * except tunnel id, which is allocated separately (see TunKeyAllocation). */
> relation OutProxy_Datapath_Binding (
> _uuid: uuid,
> - load_balancers: Set<uuid>,
> external_ids: Map<string,string>
> )
>
> /* Datapath_Binding table */
> -OutProxy_Datapath_Binding(uuid, load_balancers, external_ids) :-
> +OutProxy_Datapath_Binding(uuid, external_ids) :-
> nb::Logical_Switch(._uuid = uuid, .name = name, .external_ids = ids,
> - .load_balancer = load_balancers,
> .other_config = other_config),
> var uuid_str = uuid2str(uuid),
> var external_ids = {
> @@ -77,7 +75,7 @@ OutProxy_Datapath_Binding(uuid, load_balancers, external_ids) :-
> eids
> }.
>
> -OutProxy_Datapath_Binding(uuid, set_empty(), external_ids) :-
> +OutProxy_Datapath_Binding(uuid, external_ids) :-
> lr in nb::Logical_Router(._uuid = uuid, .name = name, .external_ids = ids,
> .options = options),
> lr.is_enabled(),
> @@ -100,8 +98,12 @@ OutProxy_Datapath_Binding(uuid, set_empty(), external_ids) :-
> }.
>
> sb::Out_Datapath_Binding(uuid, tunkey, load_balancers, external_ids) :-
> - OutProxy_Datapath_Binding(uuid, load_balancers, external_ids),
> - TunKeyAllocation(uuid, tunkey).
> + OutProxy_Datapath_Binding(uuid, external_ids),
> + TunKeyAllocation(uuid, tunkey),
> + /* Datapath_Binding.load_balancers is not used anymore, it's still in the
> + * schema for compatibility reasons. Reset it to empty, just in case.
> + */
> + var load_balancers = set_empty().
>
>
> /* Proxy table for Out_Datapath_Binding: contains all Datapath_Binding fields,
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index f12efd0fa..e6ce243cf 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2655,7 +2655,7 @@ tcp.flags = RST;
>
> <column name="load_balancers">
> <p>
> - Load balancers associated with the datapath.
> + Not used anymore; kept for backwards compatibility of the schema.
> </p>
> </column>
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 94405349e..653e445e5 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2366,7 +2366,7 @@ echo
> echo "__file__:__line__: Check that SB lb0 has sw0 in datapaths column."
>
> check_column "$sw0_sb_uuid" sb:load_balancer datapaths name=lb0
> -check_column "$lb0_uuid" sb:datapath_binding load_balancers external_ids:name=sw0
> +check_column "" sb:datapath_binding load_balancers external_ids:name=sw0
>
> check ovn-nbctl --wait=sb set load_balancer . vips:"10.0.0.20\:90"="20.0.0.4:8080,30.0.0.4:8080"
>
> @@ -2390,7 +2390,7 @@ sw1_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=sw1)
> echo
> echo "__file__:__line__: Check that SB lb0 has sw0 and sw1 in datapaths column."
> check_column "$sw0_sb_uuid $sw1_sb_uuid" sb:load_balancer datapaths name=lb0
> -check_column "$lb0_uuid" sb:datapath_binding load_balancers external_ids:name=sw1
> +check_column "" sb:datapath_binding load_balancers external_ids:name=sw1
>
> check ovn-nbctl --wait=sb lb-add lb1 10.0.0.30:80 20.0.0.50:8080 udp
> check_row_count sb:load_balancer 1
> @@ -2416,8 +2416,8 @@ echo "__file__:__line__: Check that SB lb1 has sw1 in datapaths column."
> check_column "$sw1_sb_uuid" sb:load_balancer datapaths name=lb1
>
> echo
> -echo "__file__:__line__: check that datapath sw1 has lb0 and lb1 set in the load_balancers column."
> -check_column "$lb0_uuid $lb1_uuid" sb:datapath_binding load_balancers external_ids:name=sw1
> +echo "__file__:__line__: check that datapath sw1 has no entry in the load_balancers column."
> +check_column "" sb:datapath_binding load_balancers external_ids:name=sw1
>
>
> echo
> @@ -2426,10 +2426,10 @@ check ovn-nbctl --wait=sb set Load_Balancer lb1 options:hairpin_snat_ip="42.42.4
> check_column "$lb1_uuid" sb:load_balancer _uuid name=lb1 options='{hairpin_orig_tuple="true", hairpin_snat_ip="42.42.42.42 4242::4242"}'
>
> echo
> -echo "__file__:__line__: Delete load balancer lb1 an check that datapath sw1's load_balancers are updated accordingly."
> +echo "__file__:__line__: Delete load balancer lb1 and check that datapath sw1's load_balancers is still empty."
>
> ovn-nbctl --wait=sb lb-del lb1
> -check_column "$lb0_uuid" sb:datapath_binding load_balancers external_ids:name=sw1
> +check_column "" sb:datapath_binding load_balancers external_ids:name=sw1
> AT_CLEANUP
> ])
>
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list