[ovs-dev] [PATCH ovn v4 2/7] northd: Refactor load balancer vip parsing.

Dumitru Ceara dceara at redhat.com
Tue Nov 17 12:16:14 UTC 2020


On 11/12/20 12:54 PM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> 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 <numans at ovn.org>
> ---

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



More information about the dev mailing list