| Message ID | 20210723162757.32274-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Mark Michelson |
| Headers | show |
| Series | [ovs-dev] Stop setting Load_Balancer references in SB Datapath_Binding records. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
Hi Dumitru, From a functionality standpoint, Acked-by: Mark Michelson <mmichels@redhat.com> I have a question and suggestion below. On 7/23/21 12:27 PM, Dumitru Ceara 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. > > Signed-off-by: Dumitru Ceara <dceara@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 > --- > controller/lflow.c | 10 +++++----- > controller/ovn-controller.c | 38 +++++++++++++++++++++++++++++++++++++ > controller/ovn-controller.h | 5 +++++ > 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, 68 insertions(+), 32 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 871d3c54d..b2976992d 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -1764,11 +1764,11 @@ 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, > + struct local_datapath *ldp = get_local_datapath(l_ctx_in->local_datapaths, > + dp->tunnel_key); > + for (size_t i = 0; i < ldp->n_load_balancers; i++) { > + consider_lb_hairpin_flows(ldp->load_balancers[i], > + l_ctx_in->local_datapaths, > l_ctx_out->flow_table); > } > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 3a9bdbc97..2cf467dff 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2405,6 +2405,42 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) > return _lflow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP); > } > > +static void > +init_lbs_per_datapath(struct ed_type_runtime_data *rt_data, > + const struct sbrec_load_balancer_table *lb_table) > +{ > + 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(&rt_data->local_datapaths, > + lb->datapaths[i]->tunnel_key); > + if (!ldp) { > + continue; > + } > + if (ldp->n_load_balancers == ldp->n_allocated_load_balancers) { > + ldp->load_balancers = > + x2nrealloc(ldp->load_balancers, > + &ldp->n_allocated_load_balancers, > + sizeof *ldp->load_balancers); > + } Since "ldp->load_balancers" is never modified after this point, you could avoid some reallocations by pre-allocating ldp->load_balancers to the number of load balancers in the SB load balancer table. The tradeoff would be using more memory (in most cases). Would it be worth making that change? > + ldp->load_balancers[ldp->n_load_balancers++] = lb; > + } > + } > +} > + > +static void > +cleanup_lbs_per_datapath(struct ed_type_runtime_data *rt_data) > +{ > + struct local_datapath *dp; > + HMAP_FOR_EACH (dp, hmap_node, &rt_data->local_datapaths) { > + free(dp->load_balancers); > + dp->load_balancers = NULL; > + dp->n_allocated_load_balancers = 0; > + dp->n_load_balancers = 0; > + } > +} > + > static bool > lflow_output_runtime_data_handler(struct engine_node *node, > void *data OVS_UNUSED) > @@ -2430,6 +2466,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, > struct lflow_ctx_out l_ctx_out; > struct ed_type_lflow_output *fo = data; > init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > + init_lbs_per_datapath(rt_data, l_ctx_in.lb_table); > > struct tracked_binding_datapath *tdp; > HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { > @@ -2450,6 +2487,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, > } > } > > + cleanup_lbs_per_datapath(rt_data); > engine_set_node_state(node, EN_UPDATED); > return true; > } > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > index b864ed0fa..4e907e930 100644 > --- a/controller/ovn-controller.h > +++ b/controller/ovn-controller.h > @@ -69,6 +69,11 @@ struct local_datapath { > size_t n_allocated_peer_ports; > > struct shash external_ports; > + > + const struct sbrec_load_balancer **load_balancers; > + > + size_t n_load_balancers; > + size_t n_allocated_load_balancers; > }; > > struct local_datapath *get_local_datapath(const struct hmap *, > 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 1058c1c26..a0b946e71 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -3628,24 +3628,15 @@ 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; > - } > - sbrec_datapath_binding_set_load_balancers(od->sb, lb_uuids, > - od->nbs->n_load_balancer); > - free(lb_uuids); > + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); > } > } > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index ab33a139e..64239d809 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -53,14 +53,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 = { > @@ -76,7 +74,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(), > @@ -99,8 +97,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 a39778ee0..e57d4f7ec 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -2649,7 +2649,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 3fb4c8bc9..689f76737 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 an check that datapath sw1's load_balancers is still empty." If you're changing this line: s/an/and/ > > 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 > ]) > >
On Fri, Jul 23, 2021 at 2:42 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Dumitru, > > From a functionality standpoint, > > Acked-by: Mark Michelson <mmichels@redhat.com> > > I have a question and suggestion below. > Hi Dumitru, Thanks for addressing this issue. I have a few comments. Please see below. > On 7/23/21 12:27 PM, Dumitru Ceara 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. > > > > Signed-off-by: Dumitru Ceara <dceara@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 > > --- > > controller/lflow.c | 10 +++++----- > > controller/ovn-controller.c | 38 +++++++++++++++++++++++++++++++++++++ > > controller/ovn-controller.h | 5 +++++ > > 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, 68 insertions(+), 32 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 871d3c54d..b2976992d 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -1764,11 +1764,11 @@ 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, > > + struct local_datapath *ldp = get_local_datapath(l_ctx_in->local_datapaths, > > + dp->tunnel_key); > > + for (size_t i = 0; i < ldp->n_load_balancers; i++) { > > + consider_lb_hairpin_flows(ldp->load_balancers[i], > > + l_ctx_in->local_datapaths, > > l_ctx_out->flow_table); > > } > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 3a9bdbc97..2cf467dff 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -2405,6 +2405,42 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) > > return _lflow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP); > > } > > > > +static void > > +init_lbs_per_datapath(struct ed_type_runtime_data *rt_data, > > + const struct sbrec_load_balancer_table *lb_table) > > +{ > > + 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(&rt_data->local_datapaths, > > + lb->datapaths[i]->tunnel_key); > > + if (!ldp) { > > + continue; > > + } > > + if (ldp->n_load_balancers == ldp->n_allocated_load_balancers) { > > + ldp->load_balancers = > > + x2nrealloc(ldp->load_balancers, > > + &ldp->n_allocated_load_balancers, > > + sizeof *ldp->load_balancers); > > + } > > Since "ldp->load_balancers" is never modified after this point, you > could avoid some reallocations by pre-allocating ldp->load_balancers to > the number of load balancers in the SB load balancer table. The tradeoff > would be using more memory (in most cases). Would it be worth making > that change? > > > + ldp->load_balancers[ldp->n_load_balancers++] = lb; > > + } > > + } > > +} > > + > > +static void > > +cleanup_lbs_per_datapath(struct ed_type_runtime_data *rt_data) > > +{ > > + struct local_datapath *dp; > > + HMAP_FOR_EACH (dp, hmap_node, &rt_data->local_datapaths) { > > + free(dp->load_balancers); > > + dp->load_balancers = NULL; > > + dp->n_allocated_load_balancers = 0; > > + dp->n_load_balancers = 0; > > + } > > +} > > + > > static bool > > lflow_output_runtime_data_handler(struct engine_node *node, > > void *data OVS_UNUSED) > > @@ -2430,6 +2466,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, > > struct lflow_ctx_out l_ctx_out; > > struct ed_type_lflow_output *fo = data; > > init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > > + init_lbs_per_datapath(rt_data, l_ctx_in.lb_table); > > For every runtime data change we will build the 'lbs_per_datapath'. I think this is not necessary. We use the ldp->load_balancers only in the function lflow_add_flows_for_datapath() when a new datapath is added to the "local_datapaths". I'd suggest building the list of load balancers for the newly added local datapath ? or perhaps run the HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {} to build the list only if there is a new datapath added and run the loop again ? A couple of more suggestions: 1. Since we are building and destroying the load balancer list, I'd suggest to store it in a separate hmap rather than using the "struct local_datapath". 2. Probably we can pass the "load balancers" and "n_load_balancers" directly to the function lflow_add_flows_for_datapath() Let me know what you think. Thanks Numan > > struct tracked_binding_datapath *tdp; > > HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { > > @@ -2450,6 +2487,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, > > } > > } > > > > + cleanup_lbs_per_datapath(rt_data); > > engine_set_node_state(node, EN_UPDATED); > > return true; > > } > > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > > index b864ed0fa..4e907e930 100644 > > --- a/controller/ovn-controller.h > > +++ b/controller/ovn-controller.h > > @@ -69,6 +69,11 @@ struct local_datapath { > > size_t n_allocated_peer_ports; > > > > struct shash external_ports; > > + > > + const struct sbrec_load_balancer **load_balancers; > > + > > + size_t n_load_balancers; > > + size_t n_allocated_load_balancers; > > }; > > > > struct local_datapath *get_local_datapath(const struct hmap *, > > 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 1058c1c26..a0b946e71 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -3628,24 +3628,15 @@ 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; > > - } > > - sbrec_datapath_binding_set_load_balancers(od->sb, lb_uuids, > > - od->nbs->n_load_balancer); > > - free(lb_uuids); > > + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); > > } > > } > > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > index ab33a139e..64239d809 100644 > > --- a/northd/ovn_northd.dl > > +++ b/northd/ovn_northd.dl > > @@ -53,14 +53,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 = { > > @@ -76,7 +74,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(), > > @@ -99,8 +97,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 a39778ee0..e57d4f7ec 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -2649,7 +2649,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 3fb4c8bc9..689f76737 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 an check that datapath sw1's load_balancers is still empty." > > If you're changing this line: > > s/an/and/ > > > > > > 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 > > ]) > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 7/24/21 12:37 AM, Numan Siddique wrote: > On Fri, Jul 23, 2021 at 2:42 PM Mark Michelson <mmichels@redhat.com> wrote: >> >> Hi Dumitru, >> >> From a functionality standpoint, >> >> Acked-by: Mark Michelson <mmichels@redhat.com> >> >> I have a question and suggestion below. >> > > Hi Dumitru, Hi, Mark, Numan, Thanks for the reviews! > > Thanks for addressing this issue. > > I have a few comments. Please see below. > > >> On 7/23/21 12:27 PM, Dumitru Ceara 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. >>> >>> Signed-off-by: Dumitru Ceara <dceara@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 >>> --- >>> controller/lflow.c | 10 +++++----- >>> controller/ovn-controller.c | 38 +++++++++++++++++++++++++++++++++++++ >>> controller/ovn-controller.h | 5 +++++ >>> 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, 68 insertions(+), 32 deletions(-) >>> >>> diff --git a/controller/lflow.c b/controller/lflow.c >>> index 871d3c54d..b2976992d 100644 >>> --- a/controller/lflow.c >>> +++ b/controller/lflow.c >>> @@ -1764,11 +1764,11 @@ 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, >>> + struct local_datapath *ldp = get_local_datapath(l_ctx_in->local_datapaths, >>> + dp->tunnel_key); >>> + for (size_t i = 0; i < ldp->n_load_balancers; i++) { >>> + consider_lb_hairpin_flows(ldp->load_balancers[i], >>> + l_ctx_in->local_datapaths, >>> l_ctx_out->flow_table); >>> } >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 3a9bdbc97..2cf467dff 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -2405,6 +2405,42 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) >>> return _lflow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP); >>> } >>> >>> +static void >>> +init_lbs_per_datapath(struct ed_type_runtime_data *rt_data, >>> + const struct sbrec_load_balancer_table *lb_table) >>> +{ >>> + 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(&rt_data->local_datapaths, >>> + lb->datapaths[i]->tunnel_key); >>> + if (!ldp) { >>> + continue; >>> + } >>> + if (ldp->n_load_balancers == ldp->n_allocated_load_balancers) { >>> + ldp->load_balancers = >>> + x2nrealloc(ldp->load_balancers, >>> + &ldp->n_allocated_load_balancers, >>> + sizeof *ldp->load_balancers); >>> + } >> >> Since "ldp->load_balancers" is never modified after this point, you >> could avoid some reallocations by pre-allocating ldp->load_balancers to >> the number of load balancers in the SB load balancer table. The tradeoff >> would be using more memory (in most cases). Would it be worth making >> that change? >> It feels like too much of a waste to me, to be honest. In particular, in the ovn-kubernetes use case, most load balancers are applied to most node logical switches. However, in the general case that's not true so most of the time this array would be empty. On the other hand, x2nrealloc effectively has O(n) complexity when n (the number of load balancers per datapath) is large enough, so I don't really see it as an issue. >>> + ldp->load_balancers[ldp->n_load_balancers++] = lb; >>> + } >>> + } >>> +} >>> + >>> +static void >>> +cleanup_lbs_per_datapath(struct ed_type_runtime_data *rt_data) >>> +{ >>> + struct local_datapath *dp; >>> + HMAP_FOR_EACH (dp, hmap_node, &rt_data->local_datapaths) { >>> + free(dp->load_balancers); >>> + dp->load_balancers = NULL; >>> + dp->n_allocated_load_balancers = 0; >>> + dp->n_load_balancers = 0; >>> + } >>> +} >>> + >>> static bool >>> lflow_output_runtime_data_handler(struct engine_node *node, >>> void *data OVS_UNUSED) >>> @@ -2430,6 +2466,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, >>> struct lflow_ctx_out l_ctx_out; >>> struct ed_type_lflow_output *fo = data; >>> init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); >>> + init_lbs_per_datapath(rt_data, l_ctx_in.lb_table); >>> > > For every runtime data change we will build the 'lbs_per_datapath'. I > think this is not necessary. > > We use the ldp->load_balancers only in the function > lflow_add_flows_for_datapath() when a new datapath > is added to the "local_datapaths". > > I'd suggest building the list of load balancers for the newly added > local datapath ? or perhaps run the > HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {} to build the list > only if there is a new datapath added > and run the loop again ? > > A couple of more suggestions: > 1. Since we are building and destroying the load balancer list, I'd > suggest to store it in a separate hmap > rather than using the "struct local_datapath". > > 2. Probably we can pass the "load balancers" and "n_load_balancers" > directly to the function lflow_add_flows_for_datapath() > > Let me know what you think. You're right, I'll integrate something like this in v2, thanks for the suggestions! > > Thanks > Numan > > > >>> struct tracked_binding_datapath *tdp; >>> HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { >>> @@ -2450,6 +2487,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, >>> } >>> } >>> >>> + cleanup_lbs_per_datapath(rt_data); >>> engine_set_node_state(node, EN_UPDATED); >>> return true; >>> } >>> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h >>> index b864ed0fa..4e907e930 100644 >>> --- a/controller/ovn-controller.h >>> +++ b/controller/ovn-controller.h >>> @@ -69,6 +69,11 @@ struct local_datapath { >>> size_t n_allocated_peer_ports; >>> >>> struct shash external_ports; >>> + >>> + const struct sbrec_load_balancer **load_balancers; >>> + >>> + size_t n_load_balancers; >>> + size_t n_allocated_load_balancers; >>> }; >>> >>> struct local_datapath *get_local_datapath(const struct hmap *, >>> 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 1058c1c26..a0b946e71 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -3628,24 +3628,15 @@ 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; >>> - } >>> - sbrec_datapath_binding_set_load_balancers(od->sb, lb_uuids, >>> - od->nbs->n_load_balancer); >>> - free(lb_uuids); >>> + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); >>> } >>> } >>> >>> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl >>> index ab33a139e..64239d809 100644 >>> --- a/northd/ovn_northd.dl >>> +++ b/northd/ovn_northd.dl >>> @@ -53,14 +53,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 = { >>> @@ -76,7 +74,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(), >>> @@ -99,8 +97,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 a39778ee0..e57d4f7ec 100644 >>> --- a/ovn-sb.xml >>> +++ b/ovn-sb.xml >>> @@ -2649,7 +2649,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 3fb4c8bc9..689f76737 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 an check that datapath sw1's load_balancers is still empty." >> >> If you're changing this line: >> >> s/an/and/ Ack, will do, thanks! Regards, Dumitru
diff --git a/controller/lflow.c b/controller/lflow.c index 871d3c54d..b2976992d 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -1764,11 +1764,11 @@ 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, + struct local_datapath *ldp = get_local_datapath(l_ctx_in->local_datapaths, + dp->tunnel_key); + for (size_t i = 0; i < ldp->n_load_balancers; i++) { + consider_lb_hairpin_flows(ldp->load_balancers[i], + l_ctx_in->local_datapaths, l_ctx_out->flow_table); } diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 3a9bdbc97..2cf467dff 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2405,6 +2405,42 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) return _lflow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP); } +static void +init_lbs_per_datapath(struct ed_type_runtime_data *rt_data, + const struct sbrec_load_balancer_table *lb_table) +{ + 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(&rt_data->local_datapaths, + lb->datapaths[i]->tunnel_key); + if (!ldp) { + continue; + } + if (ldp->n_load_balancers == ldp->n_allocated_load_balancers) { + ldp->load_balancers = + x2nrealloc(ldp->load_balancers, + &ldp->n_allocated_load_balancers, + sizeof *ldp->load_balancers); + } + ldp->load_balancers[ldp->n_load_balancers++] = lb; + } + } +} + +static void +cleanup_lbs_per_datapath(struct ed_type_runtime_data *rt_data) +{ + struct local_datapath *dp; + HMAP_FOR_EACH (dp, hmap_node, &rt_data->local_datapaths) { + free(dp->load_balancers); + dp->load_balancers = NULL; + dp->n_allocated_load_balancers = 0; + dp->n_load_balancers = 0; + } +} + static bool lflow_output_runtime_data_handler(struct engine_node *node, void *data OVS_UNUSED) @@ -2430,6 +2466,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, struct lflow_ctx_out l_ctx_out; struct ed_type_lflow_output *fo = data; init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); + init_lbs_per_datapath(rt_data, l_ctx_in.lb_table); struct tracked_binding_datapath *tdp; HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { @@ -2450,6 +2487,7 @@ lflow_output_runtime_data_handler(struct engine_node *node, } } + cleanup_lbs_per_datapath(rt_data); engine_set_node_state(node, EN_UPDATED); return true; } diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h index b864ed0fa..4e907e930 100644 --- a/controller/ovn-controller.h +++ b/controller/ovn-controller.h @@ -69,6 +69,11 @@ struct local_datapath { size_t n_allocated_peer_ports; struct shash external_ports; + + const struct sbrec_load_balancer **load_balancers; + + size_t n_load_balancers; + size_t n_allocated_load_balancers; }; struct local_datapath *get_local_datapath(const struct hmap *, 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 1058c1c26..a0b946e71 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -3628,24 +3628,15 @@ 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; - } - sbrec_datapath_binding_set_load_balancers(od->sb, lb_uuids, - od->nbs->n_load_balancer); - free(lb_uuids); + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); } } diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index ab33a139e..64239d809 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -53,14 +53,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 = { @@ -76,7 +74,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(), @@ -99,8 +97,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 a39778ee0..e57d4f7ec 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -2649,7 +2649,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 3fb4c8bc9..689f76737 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 an 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 ])
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. Signed-off-by: Dumitru Ceara <dceara@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 --- controller/lflow.c | 10 +++++----- controller/ovn-controller.c | 38 +++++++++++++++++++++++++++++++++++++ controller/ovn-controller.h | 5 +++++ 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, 68 insertions(+), 32 deletions(-)