On Mon, Jul 26, 2021 at 10:01 AM Dumitru Ceara <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
