On 11/12/20 12:54 PM, [email protected] wrote: > From: Numan Siddique <[email protected]> > > Parsing of the load balancer VIPs is moved to a separate file - lib/lb.c. > ovn-northd makes use of these functions. Upcoming patch will make use of these > util functions for parsing SB Load_Balancers. > > Signed-off-by: Numan Siddique <[email protected]> > ---
Hi Numan, > lib/automake.mk | 4 +- > lib/lb.c | 348 ++++++++++++++++++++++++++++++++++++++++++++ > lib/lb.h | 105 +++++++++++++ > northd/ovn-northd.c | 312 +++++++-------------------------------- > 4 files changed, 510 insertions(+), 259 deletions(-) > create mode 100644 lib/lb.c > create mode 100644 lib/lb.h > > diff --git a/lib/automake.mk b/lib/automake.mk > index d38d5c50c7..250c7aefae 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -24,7 +24,9 @@ lib_libovn_la_SOURCES = \ > lib/ovn-util.h \ > lib/logical-fields.c \ > lib/inc-proc-eng.c \ > - lib/inc-proc-eng.h > + lib/inc-proc-eng.h \ > + lib/lb.c \ > + lib/lb.h > nodist_lib_libovn_la_SOURCES = \ > lib/ovn-dirs.c \ > lib/ovn-nb-idl.c \ > diff --git a/lib/lb.c b/lib/lb.c > new file mode 100644 > index 0000000000..d94fe9383c > --- /dev/null > +++ b/lib/lb.c > @@ -0,0 +1,348 @@ > +/* Copyright (c) 2020, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > + > +#include "lb.h" > +#include "lib/ovn-nb-idl.h" > +#include "lib/ovn-sb-idl.h" > +#include "lib/ovn-util.h" > + > +/* OpenvSwitch lib includes. */ > +#include "openvswitch/vlog.h" > +#include "lib/smap.h" > + > +VLOG_DEFINE_THIS_MODULE(lb); > + > +static struct ovn_northd_lb * > +ovn_northd_lb_create_(const struct smap *vips) > +{ > + struct ovn_northd_lb *lb = xzalloc(sizeof *lb); > + > + lb->n_vips = smap_count(vips); > + lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips); > + struct smap_node *node; > + size_t n_vips = 0; > + > + SMAP_FOR_EACH (node, vips) { > + char *vip; > + uint16_t port; > + int addr_family; > + > + if (!ip_address_and_port_from_lb_key(node->key, &vip, &port, > + &addr_family)) { > + continue; > + } > + > + lb->vips[n_vips].vip = vip; > + lb->vips[n_vips].vip_port = port; > + lb->vips[n_vips].addr_family = addr_family; > + lb->vips[n_vips].vip_port_str = xstrdup(node->key); > + lb->vips[n_vips].backend_ips = xstrdup(node->value); > + > + char *tokstr = xstrdup(node->value); > + char *save_ptr = NULL; > + char *token; > + size_t n_backends = 0; > + /* Format for a backend ips : IP1:port1,IP2:port2,...". */ > + for (token = strtok_r(tokstr, ",", &save_ptr); > + token != NULL; > + token = strtok_r(NULL, ",", &save_ptr)) { > + n_backends++; > + } > + > + free(tokstr); > + tokstr = xstrdup(node->value); > + save_ptr = NULL; > + > + lb->vips[n_vips].n_backends = n_backends; > + lb->vips[n_vips].backends = xcalloc(n_backends, > + sizeof > *lb->vips[n_vips].backends); > + n_backends = 0; > + for (token = strtok_r(tokstr, ",", &save_ptr); > + token != NULL; > + token = strtok_r(NULL, ",", &save_ptr)) { > + char *backend_ip; > + uint16_t backend_port; > + int backend_addr_family; > + if (!ip_address_and_port_from_lb_key(token, &backend_ip, > + &backend_port, > + &backend_addr_family)) { > + continue; > + } > + > + if (addr_family != backend_addr_family) { We leak 'backend_ip' here. > + continue; > + } > + > + lb->vips[n_vips].backends[n_backends].ip = backend_ip; > + lb->vips[n_vips].backends[n_backends].port = backend_port; > + lb->vips[n_vips].backends[n_backends].addr_family = addr_family; > + n_backends++; > + } > + > + lb->vips[n_vips].n_backends = n_backends; > + free(tokstr); > + n_vips++; > + } > + > + /* Its possible that ip_address_and_port_from_lb_key() may fail. > + * Update the lb->n_vips to the correct value. */ > + lb->n_vips = n_vips; > + return lb; > +} > + > +struct ovn_northd_lb * > +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb, > + struct hmap *ports, struct hmap *lbs, > + void * (*ovn_port_find)(const struct hmap *ports, > + const char *name)) > +{ > + struct ovn_northd_lb *lb = ovn_northd_lb_create_(&nbrec_lb->vips); > + hmap_insert(lbs, &lb->hmap_node, uuid_hash(&nbrec_lb->header_.uuid)); > + lb->nlb = nbrec_lb; > + > + for (size_t i = 0; i < lb->n_vips; i++) { > + struct ovn_northd_lb_vip *lb_vip = &lb->vips[i]; > + > + struct nbrec_load_balancer_health_check *lb_health_check = NULL; > + if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) { > + if (nbrec_lb->n_health_check > 0) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 1); > + VLOG_WARN_RL(&rl, > + "SCTP load balancers do not currently support " > + "health checks. Not creating health checks for " > + "load balancer " UUID_FMT, > + UUID_ARGS(&nbrec_lb->header_.uuid)); > + } > + } else { > + for (size_t j = 0; j < nbrec_lb->n_health_check; j++) { > + if (!strcmp(nbrec_lb->health_check[j]->vip, > + lb_vip->vip_port_str)) { > + lb_health_check = nbrec_lb->health_check[i]; This is probably a pre-existing bug but I think it should be: lb_health_check = nbrec_lb->health_check[j]; > + break; > + } > + } > + } > + > + lb_vip->lb_health_check = lb_health_check; > + > + for (size_t j = 0; j < lb_vip->n_backends; j++) { > + struct ovn_northd_lb_vip_backend *backend = &lb_vip->backends[j]; > + > + struct ovn_port *op = NULL; > + char *svc_mon_src_ip = NULL; > + const char *s = smap_get(&nbrec_lb->ip_port_mappings, > + backend->ip); > + if (s) { > + char *port_name = xstrdup(s); > + char *p = strstr(port_name, ":"); > + if (p) { > + *p = 0; > + p++; > + op = ovn_port_find(ports, port_name); > + svc_mon_src_ip = xstrdup(p); > + } > + free(port_name); > + } > + > + backend->op = op; > + backend->svc_mon_src_ip = svc_mon_src_ip; > + } > + } > + > + if (nbrec_lb->n_selection_fields) { > + char *proto = NULL; > + if (nbrec_lb->protocol && nbrec_lb->protocol[0]) { > + proto = nbrec_lb->protocol; > + } > + > + struct ds sel_fields = DS_EMPTY_INITIALIZER; > + for (size_t i = 0; i < lb->nlb->n_selection_fields; i++) { > + char *field = lb->nlb->selection_fields[i]; > + if (!strcmp(field, "tp_src") && proto) { > + ds_put_format(&sel_fields, "%s_src,", proto); > + } else if (!strcmp(field, "tp_dst") && proto) { > + ds_put_format(&sel_fields, "%s_dst,", proto); > + } else { > + ds_put_format(&sel_fields, "%s,", field); > + } > + } > + ds_chomp(&sel_fields, ','); > + lb->selection_fields = ds_steal_cstr(&sel_fields); > + } > + > + return lb; > +} > + > +struct ovn_northd_lb * > +ovn_northd_lb_find(struct hmap *lbs, const struct uuid *uuid) > +{ > + struct ovn_northd_lb *lb; > + size_t hash = uuid_hash(uuid); > + HMAP_FOR_EACH_WITH_HASH (lb, hmap_node, hash, lbs) { > + if (uuid_equals(&lb->nlb->header_.uuid, uuid)) { > + return lb; > + } > + } > + > + return NULL; > +} > + > +void > +ovn_northd_lb_add_datapath(struct ovn_northd_lb *lb, > + const struct sbrec_datapath_binding *sb) > +{ > + if (lb->n_allocated_dps == lb->n_dps) { > + lb->dps = x2nrealloc(lb->dps, &lb->n_allocated_dps, sizeof *lb->dps); > + } > + lb->dps[lb->n_dps++] = sb; > +} > + > +void > +ovn_northd_lb_destroy(struct ovn_northd_lb *lb) > +{ > + for (size_t i = 0; i < lb->n_vips; i++) { > + free(lb->vips[i].vip); > + free(lb->vips[i].backend_ips); > + free(lb->vips[i].vip_port_str); > + > + for (size_t j = 0; j < lb->vips[i].n_backends; j++) { > + free(lb->vips[i].backends[j].ip); > + free(lb->vips[i].backends[j].svc_mon_src_ip); > + } > + > + free(lb->vips[i].backends); > + } > + free(lb->vips); > + free(lb->selection_fields); > + free(lb->dps); > + free(lb); > +} > + > +void > +ovn_northd_lbs_destroy(struct hmap *lbs) > +{ > + struct ovn_northd_lb *lb; > + HMAP_FOR_EACH_POP (lb, hmap_node, lbs) { > + ovn_northd_lb_destroy(lb); > + } > + hmap_destroy(lbs); > +} > + > +struct ovn_controller_lb * > +ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb) > +{ > + struct ovn_controller_lb *lb = xzalloc(sizeof *lb); > + > + lb->slb = sbrec_lb; > + lb->n_vips = smap_count(&sbrec_lb->vips); > + lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips); > + > + struct smap_node *node; > + size_t n_vips = 0; > + > + SMAP_FOR_EACH (node, &sbrec_lb->vips) { > + char *vip; > + uint16_t port; > + int addr_family; > + > + if (!ip_address_and_port_from_lb_key(node->key, &vip, &port, > + &addr_family)) { > + continue; > + } > + > + if (addr_family == AF_INET) { > + ovs_be32 vip4; > + ip_parse(vip, &vip4); > + in6_addr_set_mapped_ipv4(&lb->vips[n_vips].vip, vip4); > + } else { > + ipv6_parse(vip, &lb->vips[n_vips].vip); > + } > + > + lb->vips[n_vips].vip_port = port; > + lb->vips[n_vips].addr_family = addr_family; > + > + char *tokstr = xstrdup(node->value); > + char *save_ptr = NULL; > + char *token; > + size_t n_backends = 0; > + /* Format for a backend ips : IP1:port1,IP2:port2,...". */ > + for (token = strtok_r(tokstr, ",", &save_ptr); > + token != NULL; > + token = strtok_r(NULL, ",", &save_ptr)) { > + n_backends++; > + } > + > + free(tokstr); > + tokstr = xstrdup(node->value); > + save_ptr = NULL; > + > + lb->vips[n_vips].n_backends = n_backends; > + lb->vips[n_vips].backends = xcalloc(n_backends, > + sizeof > *lb->vips[n_vips].backends); > + n_backends = 0; > + for (token = strtok_r(tokstr, ",", &save_ptr); > + token != NULL; > + token = strtok_r(NULL, ",", &save_ptr)) { > + char *backend_ip; > + uint16_t backend_port; > + int backend_addr_family; > + > + if (!ip_address_and_port_from_lb_key(token, &backend_ip, > + &backend_port, > + &backend_addr_family)) { > + continue; > + } > + > + if (addr_family != backend_addr_family) { We leak 'backend_ip' here. It didn't feel quite right in the end to have the code duplication. Also, the fact that we ended up copying the memory leak is also a sign that we should try to avoid duplicating the code. What do you think of the following incremental on top of your patch? https://github.com/dceara/ovn/commit/36d6f67cd98ebb5e1acde533050e6f60d5fbeac8 I ended up storing the string version of the VIP/backend-ips in both ovn-northd and ovn-controller load balancers. In ovn-controller we only use the string to store the parsed VIP/backend IP, so there's a tiny bit of waste there but it allows sharing more data structures and code between ovn-northd and ovn-controller. Full changes to make the rest of the series work with the suggested refactoring: https://github.com/dceara/ovn/commits/review-pws213906-load-balancer-hairpin-v4-modified I had to add a patch to use the new data structures in ovn-controller: https://github.com/dceara/ovn/commit/720baea481ce3345efd2a9259707e62be7168bfa And the following had minor conflicts when cherry-picking: https://github.com/dceara/ovn/commit/1a07c22a77278152086efe4733687962a738e270 Sorry if I went a bit overboard with the refactoring :) Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
