diff mbox series

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

Message ID 20201112115441.1361909-1-numans@ovn.org
State Superseded
Headers show
Series Optimize load balancer hairpin logical flows. | expand

Commit Message

Numan Siddique Nov. 12, 2020, 11:54 a.m. UTC
From: Numan Siddique <numans@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@ovn.org>
---
 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

Comments

Dumitru Ceara Nov. 17, 2020, 12:16 p.m. UTC | #1
On 11/12/20 12:54 PM, numans@ovn.org wrote:
> From: Numan Siddique <numans@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@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
Numan Siddique Nov. 17, 2020, 2:33 p.m. UTC | #2
On Tue, Nov 17, 2020 at 5:46 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 11/12/20 12:54 PM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@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@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. I think your changes looks good to me. I will
incorporate your changes
and submit v5.

Request to please take a look.


Numan

>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

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) {
+                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];
+                    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) {
+                continue;
+            }
+
+            if (addr_family == AF_INET) {
+                ovs_be32 ip4;
+                ip_parse(backend_ip, &ip4);
+                in6_addr_set_mapped_ipv4(
+                    &lb->vips[n_vips].backends[n_backends].ip, ip4);
+            } else {
+                ipv6_parse(backend_ip,
+                           &lb->vips[n_vips].backends[n_backends].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;
+}
+
+void
+ovn_controller_lb_destroy(struct ovn_controller_lb *lb)
+{
+    for (size_t i = 0; i < lb->n_vips; i++) {
+        free(lb->vips[i].backends);
+    }
+    free(lb->vips);
+    free(lb);
+}
diff --git a/lib/lb.h b/lib/lb.h
new file mode 100644
index 0000000000..bf7cbc581e
--- /dev/null
+++ b/lib/lb.h
@@ -0,0 +1,105 @@ 
+/* 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.
+ */
+
+
+#ifndef OVN_LIB_LB_H
+#define OVN_LIB_LB_H 1
+
+#include <sys/types.h>
+#include <netinet/in.h>
+#include "openvswitch/hmap.h"
+
+struct nbrec_load_balancer;
+struct sbrec_load_balancer;
+struct sbrec_datapath_binding;
+struct ovn_port;
+struct uuid;
+
+struct ovn_northd_lb {
+    struct hmap_node hmap_node;
+
+    const struct nbrec_load_balancer *nlb; /* May be NULL. */
+    const struct sbrec_load_balancer *slb; /* May be NULL. */
+    char *selection_fields;
+    struct ovn_northd_lb_vip *vips;
+    size_t n_vips;
+
+    size_t n_dps;
+    size_t n_allocated_dps;
+    const struct sbrec_datapath_binding **dps;
+};
+
+struct ovn_northd_lb_vip {
+    char *vip;
+    uint16_t vip_port;
+    int addr_family;
+    char *vip_port_str;
+
+    /* Backend information. */
+    char *backend_ips;
+    struct ovn_northd_lb_vip_backend *backends;
+    size_t n_backends;
+
+    struct nbrec_load_balancer_health_check *lb_health_check;
+};
+
+struct ovn_northd_lb_vip_backend {
+    char *ip;
+    uint16_t port;
+    int addr_family;
+
+    struct ovn_port *op; /* Logical port to which the ip belong to. */
+    bool health_check;
+    char *svc_mon_src_ip; /* Source IP to use for monitoring. */
+    const struct sbrec_service_monitor *sbrec_monitor;
+};
+
+struct ovn_northd_lb *ovn_northd_lb_create(
+    const struct nbrec_load_balancer *,
+    struct hmap *ports, struct hmap *lbs,
+    void * (*ovn_port_find)(const struct hmap *ports, const char *name));
+struct ovn_northd_lb * ovn_northd_lb_find(struct hmap *, const struct uuid *);
+void ovn_northd_lb_destroy(struct ovn_northd_lb *);
+void ovn_northd_lbs_destroy(struct hmap *lbs);
+void ovn_northd_lb_add_datapath(struct ovn_northd_lb *,
+                                const struct sbrec_datapath_binding *);
+
+struct ovn_controller_lb {
+    const struct sbrec_load_balancer *slb; /* May be NULL. */
+
+    struct ovn_controller_lb_vip *vips;
+    size_t n_vips;
+};
+
+struct ovn_controller_lb_vip {
+    struct in6_addr vip;
+    uint16_t vip_port;
+    int addr_family;
+
+    struct ovn_controller_lb_vip_backend *backends;
+    size_t n_backends;
+};
+
+struct ovn_controller_lb_vip_backend {
+    struct in6_addr ip;
+    uint16_t port;
+    int addr_family;
+};
+
+struct ovn_controller_lb *ovn_controller_lb_create(
+    const struct sbrec_load_balancer *);
+void ovn_controller_lb_destroy(struct ovn_controller_lb *);
+
+#endif /* OVN_LIB_LB_H 1 */
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 7f470afe89..836a209100 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -35,6 +35,7 @@ 
 #include "lib/ovn-nb-idl.h"
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
+#include "lib/lb.h"
 #include "ovn/actions.h"
 #include "ovn/logical-fields.h"
 #include "packets.h"
@@ -3320,66 +3321,6 @@  cleanup_sb_ha_chassis_groups(struct northd_context *ctx,
     }
 }
 
-struct ovn_lb {
-    struct hmap_node hmap_node;
-
-    const struct nbrec_load_balancer *nlb; /* May be NULL. */
-    const struct sbrec_load_balancer *slb; /* May be NULL. */
-    char *selection_fields;
-    struct lb_vip *vips;
-    size_t n_vips;
-
-    size_t n_dps;
-    size_t n_allocated_dps;
-    const struct sbrec_datapath_binding **dps;
-};
-
-struct lb_vip {
-    char *vip;
-    uint16_t vip_port;
-    int addr_family;
-    char *backend_ips;
-
-    bool health_check;
-    struct lb_vip_backend *backends;
-    size_t n_backends;
-};
-
-struct lb_vip_backend {
-    char *ip;
-    uint16_t port;
-    int addr_family;
-
-    struct ovn_port *op; /* Logical port to which the ip belong to. */
-    bool health_check;
-    char *svc_mon_src_ip; /* Source IP to use for monitoring. */
-    const struct sbrec_service_monitor *sbrec_monitor;
-};
-
-
-static inline struct ovn_lb *
-ovn_lb_find(struct hmap *lbs, const struct uuid *uuid)
-{
-    struct ovn_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;
-}
-
-static void
-ovn_lb_add_datapath(struct ovn_lb *lb, struct ovn_datapath *od)
-{
-    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++] = od->sb;
-}
-
 struct service_monitor_info {
     struct hmap_node hmap_node;
     const struct sbrec_service_monitor *sbrec_mon;
@@ -3419,126 +3360,36 @@  create_or_get_service_mon(struct northd_context *ctx,
     return mon_info;
 }
 
-static struct ovn_lb *
-ovn_lb_create(struct northd_context *ctx, struct hmap *lbs,
-              const struct nbrec_load_balancer *nbrec_lb,
-              struct hmap *ports, struct hmap *monitor_map)
+static void
+ovn_lb_svc_create(struct northd_context *ctx, struct ovn_northd_lb *lb,
+                  struct hmap *monitor_map)
 {
-    struct ovn_lb *lb = xzalloc(sizeof *lb);
-
-    size_t hash = uuid_hash(&nbrec_lb->header_.uuid);
-    lb->nlb = nbrec_lb;
-    hmap_insert(lbs, &lb->hmap_node, hash);
-
-    lb->n_vips = smap_count(&nbrec_lb->vips);
-    lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
-    struct smap_node *node;
-    size_t n_vips = 0;
-
-    SMAP_FOR_EACH (node, &nbrec_lb->vips) {
-        char *vip;
-        uint16_t port;
-        int addr_family;
+    for (size_t i = 0; i < lb->n_vips; i++) {
+        struct ovn_northd_lb_vip *lb_vip = &lb->vips[i];
 
-        if (!ip_address_and_port_from_lb_key(node->key, &vip, &port,
-                                             &addr_family)) {
+        if (!lb_vip->lb_health_check) {
             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].backend_ips = xstrdup(node->value);
-
-        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 i = 0; i < nbrec_lb->n_health_check; i++) {
-                if (!strcmp(nbrec_lb->health_check[i]->vip, node->key)) {
-                    lb_health_check = nbrec_lb->health_check[i];
-                    break;
-                }
-            }
-        }
-
-        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++;
-        }
+        for (size_t j = 0; j < lb_vip->n_backends; j++) {
+            struct ovn_northd_lb_vip_backend *backend = &lb_vip->backends[j];
 
-        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 (struct lb_vip_backend));
-        lb->vips[n_vips].health_check = lb_health_check ? true: false;
-
-        size_t i = 0;
-        for (token = strtok_r(tokstr, ",", &save_ptr);
-            token != NULL;
-            token = strtok_r(NULL, ",", &save_ptr)) {
-            char *backend_ip;
-            uint16_t backend_port;
-
-            if (!ip_address_and_port_from_lb_key(token, &backend_ip,
-                                                 &backend_port,
-                                                 &addr_family)) {
-                continue;
-            }
-
-            /* Get the logical port to which this ip belongs to. */
-            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);
-            }
-
-            lb->vips[n_vips].backends[i].ip = backend_ip;
-            lb->vips[n_vips].backends[i].port = backend_port;
-            lb->vips[n_vips].backends[i].addr_family = addr_family;
-            lb->vips[n_vips].backends[i].op = op;
-            lb->vips[n_vips].backends[i].svc_mon_src_ip = svc_mon_src_ip;
-
-            if (lb_health_check && op && svc_mon_src_ip) {
-                const char *protocol = nbrec_lb->protocol;
+            if (backend->op && backend->svc_mon_src_ip) {
+                backend->health_check = true;
+                const char *protocol = lb->nlb->protocol;
                 if (!protocol || !protocol[0]) {
                     protocol = "tcp";
                 }
-                lb->vips[n_vips].backends[i].health_check = true;
+                backend->health_check = true;
                 struct service_monitor_info *mon_info =
-                    create_or_get_service_mon(ctx, monitor_map, backend_ip,
-                                              op->nbsp->name, backend_port,
+                    create_or_get_service_mon(ctx, monitor_map, backend->ip,
+                                              backend->op->nbsp->name,
+                                              backend->port,
                                               protocol);
 
                 ovs_assert(mon_info);
                 sbrec_service_monitor_set_options(
-                    mon_info->sbrec_mon, &lb_health_check->options);
+                    mon_info->sbrec_mon, &lb_vip->lb_health_check->options);
                 struct eth_addr ea;
                 if (!mon_info->sbrec_mon->src_mac ||
                     !eth_addr_from_string(mon_info->sbrec_mon->src_mac, &ea) ||
@@ -3548,80 +3399,31 @@  ovn_lb_create(struct northd_context *ctx, struct hmap *lbs,
                 }
 
                 if (!mon_info->sbrec_mon->src_ip ||
-                    strcmp(mon_info->sbrec_mon->src_ip, svc_mon_src_ip)) {
+                    strcmp(mon_info->sbrec_mon->src_ip,
+                           backend->svc_mon_src_ip)) {
                     sbrec_service_monitor_set_src_ip(mon_info->sbrec_mon,
-                                                     svc_mon_src_ip);
+                                                     backend->svc_mon_src_ip);
                 }
 
-                lb->vips[n_vips].backends[i].sbrec_monitor =
-                    mon_info->sbrec_mon;
+                lb_vip->backends[j].sbrec_monitor = mon_info->sbrec_mon;
                 mon_info->required = true;
-            } else {
-                lb->vips[n_vips].backends[i].health_check = false;
-            }
-
-            i++;
-        }
-
-        free(tokstr);
-        n_vips++;
-    }
-
-    char *proto = NULL;
-    if (nbrec_lb->protocol && nbrec_lb->protocol[0]) {
-        proto = nbrec_lb->protocol;
-    }
-
-    if (lb->nlb->n_selection_fields) {
-        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;
-}
-
-static void
-ovn_lb_destroy(struct ovn_lb *lb)
-{
-    for (size_t i = 0; i < lb->n_vips; i++) {
-        free(lb->vips[i].vip);
-        free(lb->vips[i].backend_ips);
-
-        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);
 }
 
-static void build_lb_vip_ct_lb_actions(struct lb_vip *lb_vip,
+static void build_lb_vip_ct_lb_actions(struct ovn_northd_lb_vip *lb_vip,
                                        struct ds *action,
                                        char *selection_fields)
 {
     bool skip_hash_fields = false;
 
-    if (lb_vip->health_check) {
+    if (lb_vip->lb_health_check) {
         ds_put_cstr(action, "ct_lb(backends=");
 
         size_t n_active_backends = 0;
         for (size_t k = 0; k < lb_vip->n_backends; k++) {
-            struct lb_vip_backend *backend = &lb_vip->backends[k];
+            struct ovn_northd_lb_vip_backend *backend = &lb_vip->backends[k];
             if (backend->health_check && backend->sbrec_monitor &&
                 backend->sbrec_monitor->status &&
                 strcmp(backend->sbrec_monitor->status, "online")) {
@@ -3672,7 +3474,12 @@  build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
 
     const struct nbrec_load_balancer *nbrec_lb;
     NBREC_LOAD_BALANCER_FOR_EACH (nbrec_lb, ctx->ovnnb_idl) {
-        ovn_lb_create(ctx, lbs, nbrec_lb, ports, &monitor_map);
+        ovn_northd_lb_create(nbrec_lb, ports, lbs, (void *)ovn_port_find);
+    }
+
+    struct ovn_northd_lb *lb;
+    HMAP_FOR_EACH (lb, hmap_node, lbs) {
+        ovn_lb_svc_create(ctx, lb, &monitor_map);
     }
 
     struct ovn_datapath *od;
@@ -3684,14 +3491,12 @@  build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
         for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
             const struct uuid *lb_uuid =
                 &od->nbs->load_balancer[i]->header_.uuid;
-            struct ovn_lb *lb = ovn_lb_find(lbs, lb_uuid);
+            lb = ovn_northd_lb_find(lbs, lb_uuid);
 
-            ovn_lb_add_datapath(lb, od);
+            ovn_northd_lb_add_datapath(lb, od->sb);
         }
     }
 
-    struct ovn_lb *lb;
-
     /* Delete any stale SB load balancer rows. */
     const struct sbrec_load_balancer *sbrec_lb, *next;
     SBREC_LOAD_BALANCER_FOR_EACH_SAFE (sbrec_lb, next, ctx->ovnsb_idl) {
@@ -3702,7 +3507,7 @@  build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
             continue;
         }
 
-        lb = ovn_lb_find(lbs, &lb_uuid);
+        lb = ovn_northd_lb_find(lbs, &lb_uuid);
         if (lb && lb->n_dps) {
             lb->slb = sbrec_lb;
         } else {
@@ -3747,7 +3552,7 @@  build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
         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_lb_find(lbs, lb_uuid);
+            lb = ovn_northd_lb_find(lbs, lb_uuid);
             sbrec_lbs[i] = lb->slb;
         }
 
@@ -3768,16 +3573,6 @@  build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
     hmap_destroy(&monitor_map);
 }
 
-static void
-destroy_ovn_lbs(struct hmap *lbs)
-{
-    struct ovn_lb *lb;
-    HMAP_FOR_EACH_POP (lb, hmap_node, lbs) {
-        ovn_lb_destroy(lb);
-        free(lb);
-    }
-}
-
 static bool
 ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key)
 {
@@ -5196,7 +4991,7 @@  ls_has_dns_records(const struct nbrec_logical_switch *nbs)
 
 static void
 build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows,
-                          struct lb_vip *lb_vip,
+                          struct ovn_northd_lb_vip *lb_vip,
                           struct nbrec_load_balancer *lb,
                           int pl, struct shash *meter_groups)
 {
@@ -5288,12 +5083,12 @@  build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
     bool vip_configured = false;
     for (int i = 0; i < od->nbs->n_load_balancer; i++) {
         struct nbrec_load_balancer *nb_lb = od->nbs->load_balancer[i];
-        struct ovn_lb *lb =
-            ovn_lb_find(lbs, &nb_lb->header_.uuid);
+        struct ovn_northd_lb *lb =
+            ovn_northd_lb_find(lbs, &nb_lb->header_.uuid);
         ovs_assert(lb);
 
         for (size_t j = 0; j < lb->n_vips; j++) {
-            struct lb_vip *lb_vip = &lb->vips[j];
+            struct ovn_northd_lb_vip *lb_vip = &lb->vips[j];
             build_empty_lb_event_flow(od, lflows, lb_vip, nb_lb,
                                       S_SWITCH_IN_PRE_LB, meter_groups);
 
@@ -6024,7 +5819,8 @@  build_lb(struct ovn_datapath *od, struct hmap *lflows)
 
 static void
 build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows,
-                       struct ovn_lb *lb, struct lb_vip *lb_vip,
+                       struct ovn_northd_lb *lb,
+                       struct ovn_northd_lb_vip *lb_vip,
                        const char *ip_match, const char *proto)
 {
     if (lb_vip->n_backends == 0) {
@@ -6046,7 +5842,7 @@  build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows,
      */
     ds_put_char(&match_reply, '(');
     for (size_t i = 0; i < lb_vip->n_backends; i++) {
-        struct lb_vip_backend *backend = &lb_vip->backends[i];
+        struct ovn_northd_lb_vip_backend *backend = &lb_vip->backends[i];
 
         /* Packets that after load balancing have equal source and
          * destination IPs should be hairpinned.
@@ -6101,10 +5897,11 @@  build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows,
 }
 
 static void
-build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb)
+build_lb_rules(struct ovn_datapath *od, struct hmap *lflows,
+               struct ovn_northd_lb *lb)
 {
     for (size_t i = 0; i < lb->n_vips; i++) {
-        struct lb_vip *lb_vip = &lb->vips[i];
+        struct ovn_northd_lb_vip *lb_vip = &lb->vips[i];
 
         const char *ip_match = NULL;
         if (lb_vip->addr_family == AF_INET) {
@@ -6190,8 +5987,8 @@  build_stateful(struct ovn_datapath *od, struct hmap *lflows, struct hmap *lbs)
      * connection, so it is okay if we do not hit the above match on
      * REGBIT_CONNTRACK_COMMIT. */
     for (int i = 0; i < od->nbs->n_load_balancer; i++) {
-        struct ovn_lb *lb =
-            ovn_lb_find(lbs, &od->nbs->load_balancer[i]->header_.uuid);
+        struct ovn_northd_lb *lb =
+            ovn_northd_lb_find(lbs, &od->nbs->load_balancer[i]->header_.uuid);
 
         ovs_assert(lb);
         build_lb_rules(od, lflows, lb);
@@ -7080,10 +6877,10 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
 
     /* Ingress table 13: ARP/ND responder for service monitor source ip.
      * (priority 110)*/
-    struct ovn_lb *lb;
+    struct ovn_northd_lb *lb;
     HMAP_FOR_EACH (lb, hmap_node, lbs) {
         for (size_t i = 0; i < lb->n_vips; i++) {
-            if (!lb->vips[i].health_check) {
+            if (!lb->vips[i].lb_health_check) {
                 continue;
             }
 
@@ -8334,7 +8131,7 @@  get_force_snat_ip(struct ovn_datapath *od, const char *key_type,
 static void
 add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
                    struct ds *match, struct ds *actions, int priority,
-                   bool lb_force_snat_ip, struct lb_vip *lb_vip,
+                   bool lb_force_snat_ip, struct ovn_northd_lb_vip *lb_vip,
                    const char *proto, struct nbrec_load_balancer *lb,
                    struct shash *meter_groups, struct sset *nat_entries)
 {
@@ -8414,7 +8211,7 @@  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
     ds_put_format(&undnat_match, "%s && (", ip_match);
 
     for (size_t i = 0; i < lb_vip->n_backends; i++) {
-        struct lb_vip_backend *backend = &lb_vip->backends[i];
+        struct ovn_northd_lb_vip_backend *backend = &lb_vip->backends[i];
         ds_put_format(&undnat_match, "(%s.src == %s", ip_match, backend->ip);
 
         if (backend->port) {
@@ -9789,12 +9586,12 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
 
         for (int i = 0; i < od->nbr->n_load_balancer; i++) {
             struct nbrec_load_balancer *nb_lb = od->nbr->load_balancer[i];
-            struct ovn_lb *lb =
-                ovn_lb_find(lbs, &nb_lb->header_.uuid);
+            struct ovn_northd_lb *lb =
+                ovn_northd_lb_find(lbs, &nb_lb->header_.uuid);
             ovs_assert(lb);
 
             for (size_t j = 0; j < lb->n_vips; j++) {
-                struct lb_vip *lb_vip = &lb->vips[j];
+                struct ovn_northd_lb_vip *lb_vip = &lb->vips[j];
                 ds_clear(&actions);
                 build_lb_vip_ct_lb_actions(lb_vip, &actions,
                                            lb->selection_fields);
@@ -12266,8 +12063,7 @@  ovnnb_db_run(struct northd_context *ctx,
     sync_port_groups(ctx, &port_groups);
     sync_meters(ctx);
     sync_dns_entries(ctx, datapaths);
-    destroy_ovn_lbs(&lbs);
-    hmap_destroy(&lbs);
+    ovn_northd_lbs_destroy(&lbs);
 
     struct ovn_igmp_group *igmp_group, *next_igmp_group;