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

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            | 236 ++++++++++++++++++++++++++++++++++++
 lib/lb.h            |  77 ++++++++++++
 lib/ovn-util.c      |  28 +++++
 lib/ovn-util.h      |   2 +
 northd/ovn-northd.c | 286 +++++---------------------------------------
 6 files changed, 378 insertions(+), 255 deletions(-)
 create mode 100644 lib/lb.c
 create mode 100644 lib/lb.h

On 10/27/20 6:16 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>
> ---
>  lib/automake.mk     |   4 +-
>  lib/lb.c            | 236 ++++++++++++++++++++++++++++++++++++
>  lib/lb.h            |  77 ++++++++++++
>  lib/ovn-util.c      |  28 +++++
>  lib/ovn-util.h      |   2 +
>  northd/ovn-northd.c | 286 +++++---------------------------------------
>  6 files changed, 378 insertions(+), 255 deletions(-)
>  create mode 100644 lib/lb.c
>  create mode 100644 lib/lb.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index f3e9c8818b..430cd11fc6 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -23,7 +23,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..db2d3d552f
> --- /dev/null
> +++ b/lib/lb.c
> @@ -0,0 +1,236 @@
> +/* 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_lb *
> +ovn_lb_create(const struct smap *vips)
> +{
> +    struct ovn_lb *lb = xzalloc(sizeof *lb);
> +
> +    lb->n_vips = smap_count(vips);
> +    lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> +    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_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 (struct lb_vip_backend));

This should be 'sizeof *lb->vips[n_vips].backends'.

> +

Nit: It's probably fine to remove an empty line here, the one above is already
indented enough to the right.

> +        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_key(token, &backend_ip,
> +                                              &backend_port,
> +                                              &addr_family)) {
> +                continue;
> +            }
> +
> +            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;
> +            i++;
> +        }
> +
> +        free(tokstr);
> +        n_vips++;
> +    }
> +
> +    return lb;
> +}
> +
> +struct ovn_lb *
> +ovn_nb_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_lb *lb = ovn_lb_create(&nbrec_lb->vips);
> +    hmap_insert(lbs, &lb->hmap_node, uuid_hash(&nbrec_lb->header_.uuid));
> +    lb->nlb = nbrec_lb;
> +    lb->nb_lb = true;
> +
> +    for (size_t i = 0; i < lb->n_vips; i++) {
> +        struct 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 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_lb *
> +ovn_sb_lb_create(const struct sbrec_load_balancer *sbrec_lb)
> +{
> +    struct ovn_lb *lb = ovn_lb_create(&sbrec_lb->vips);
> +    lb->slb = sbrec_lb;
> +    lb->nb_lb = false;
> +
> +    return lb;
> +}
> +
> +struct ovn_lb *
> +ovn_lb_find(struct hmap *lbs, 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;
> +}
> +
> +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);
> +        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);

This is not exactly correct if the record was created with ovn_sb_lb_create().
It happens to work but it's actually freeing padding that happens to have been
initialized to 0.

> +}
> +
> +void
> +ovn_lbs_destroy(struct hmap *lbs)
> +{
> +    struct ovn_lb *lb;
> +    HMAP_FOR_EACH_POP (lb, hmap_node, lbs) {
> +        ovn_lb_destroy(lb);
> +        free(lb);
> +    }
> +    hmap_destroy(lbs);
> +}
> diff --git a/lib/lb.h b/lib/lb.h
> new file mode 100644
> index 0000000000..ffb3ba1fd1
> --- /dev/null
> +++ b/lib/lb.h
> @@ -0,0 +1,77 @@
> +/* 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 "openvswitch/hmap.h"
> +
> +struct nbrec_load_balancer;
> +struct sbrec_load_balancer;
> +struct ovn_port;
> +struct uuid;
> +
> +struct ovn_lb {
> +    struct hmap_node hmap_node;
> +
> +    bool nb_lb; /* NB load balancer or SB load balancer. */
> +    union {
> +        struct {
> +            const struct nbrec_load_balancer *nlb; /* May be NULL. */
> +            char *selection_fields;
> +        };
> +        const struct sbrec_load_balancer *slb; /* May be NULL. */
> +    };
> +
> +    struct lb_vip *vips;
> +    size_t n_vips;
> +};

I think it's better to have two different structures for NB/SB load balancers.
We never store them together anyway.  What about something like:

struct ovn_lb {
    struct hmap_node hmap_node;
    struct lb_vip *vips;
    size_t n_vips;
};

struct ovn_nb_lb {
   struct ovn_lb common;
   const struct nbrec_load_balancer *nlb;
   char *selection_fields;
   /* Some more fields that are NB specific. */
};

struct ovn_sb_lb {
   struct ovn_lb common;
   const struct sbrec_load_balancer *slb;
   /* Some more fields that are SB specific. */
};

struct ovn_nb_lb *
ovn_nb_lb_from_lb(struct ovn_lb *lb)
{
    return CONTAINER_OF(lb, struct ovn_nb_lb *, common);
}

struct ovn_sb_lb *
ovn_sb_lb_from_lb(struct ovn_lb *lb)
{
    return CONTAINER_OF(lb, struct ovn_sb_lb *, common);
}

> +
> +struct lb_vip {
> +    char *vip;
> +    uint16_t vip_port;
> +    int addr_family;
> +    char *backend_ips;
> +    char *vip_port_str;
> +
> +    /* Valid only for NB load balancer. */

This is not really true.  In patch 3/7 we use some of the struct lb_vip_backend
fields from SB load balancers 'backends'.

> +    struct nbrec_load_balancer_health_check *lb_health_check;
> +    struct lb_vip_backend *backends;
> +    size_t n_backends;

If we'd have separate NB/SB structures this would go in the NB-specific part.

> +};
> +
> +struct lb_vip_backend {
> +    char *ip;
> +    uint16_t port;
> +    int addr_family;
> +
> +    /* Valid only for NB load balancer. */
> +    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;

If we'd have separate NB/SB structures this would go in the NB-specific part.

Thanks,
Dumitru

> +};
> +
> +struct ovn_lb *ovn_nb_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_lb *ovn_sb_lb_create(const struct sbrec_load_balancer *sbrec_lb);
> +struct ovn_lb * ovn_lb_find(struct hmap *lbs, struct uuid *uuid);
> +void ovn_lb_destroy(struct ovn_lb *lb);
> +void ovn_lbs_destroy(struct hmap *lbs);
> +
> +#endif /* OVN_LIB_LB_H 1 */
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 1daf665037..02770bd55a 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -24,6 +24,7 @@
>  #include "ovn-sb-idl.h"
>  #include "unixctl.h"
>  #include <ctype.h>
> +#include "socket-util.h"
>  
>  VLOG_DEFINE_THIS_MODULE(ovn_util);
>  
> @@ -667,3 +668,30 @@ ovn_smap_get_uint(const struct smap *smap, const char *key, unsigned int def)
>  
>      return u_value;
>  }
> +
> +/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
> + * 'ip_address'.  The caller must free() the memory allocated for
> + * 'ip_address'.
> + * Returns true if parsing of 'key' was successful, false otherwise.
> + */
> +bool
> +ip_address_and_port_from_key(const char *key, char **ip_address,
> +                             uint16_t *port, int *addr_family)
> +{
> +    struct sockaddr_storage ss;
> +    if (!inet_parse_active(key, 0, &ss, false)) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL(&rl, "bad ip address or port for key %s", key);
> +        *ip_address = NULL;
> +        *port = 0;
> +        *addr_family = 0;
> +        return false;
> +    }
> +
> +    struct ds s = DS_EMPTY_INITIALIZER;
> +    ss_format_address_nobracks(&ss, &s);
> +    *ip_address = ds_steal_cstr(&s);
> +    *port = ss_get_port(&ss);
> +    *addr_family = ss.ss_family;
> +    return true;
> +}
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 1d22a691f5..7fe53fd736 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -157,6 +157,8 @@ char *normalize_v46_prefix(const struct v46_ip *prefix, unsigned int plen);
>  unsigned int ovn_smap_get_uint(const struct smap *smap, const char *key,
>                                 unsigned int def);
>  
> +bool ip_address_and_port_from_key(const char *key, char **ip_address,
> +                                  uint16_t *port, int *addr_family);
>  /* Returns a lowercase copy of orig.
>   * Caller must free the returned string.
>   */
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index d11888d203..1da31caf3d 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"
> @@ -2525,10 +2526,6 @@ join_logical_ports(struct northd_context *ctx,
>      }
>  }
>  
> -static bool
> -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> -                                uint16_t *port, int *addr_family);
> -
>  static void
>  get_router_load_balancer_ips(const struct ovn_datapath *od,
>                               struct sset *all_ips_v4, struct sset *all_ips_v6)
> @@ -2548,8 +2545,8 @@ get_router_load_balancer_ips(const struct ovn_datapath *od,
>              uint16_t port;
>              int addr_family;
>  
> -            if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port,
> -                                                 &addr_family)) {
> +            if (!ip_address_and_port_from_key(node->key, &ip_address, &port,
> +                                              &addr_family)) {
>                  continue;
>              }
>  
> @@ -3309,53 +3306,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. */
> -    char *selection_fields;
> -    struct lb_vip *vips;
> -    size_t n_vips;
> -};
> -
> -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, 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;
> -}
> -
> -
>  struct service_monitor_info {
>      struct hmap_node hmap_node;
>      const struct sbrec_service_monitor *sbrec_mon;
> @@ -3395,126 +3345,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_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 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++;
> -        }
> -
> -        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;
> -            }
> +        for (size_t j = 0; j < lb_vip->n_backends; j++) {
> +            struct lb_vip_backend *backend = &lb_vip->backends[j];
>  
> -            /* 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) ||
> @@ -3524,72 +3384,24 @@ 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);
>  }
>  
>  static void build_lb_vip_ct_lb_actions(struct lb_vip *lb_vip,
>                                         struct ds *action,
>                                         char *selection_fields)
>  {
> -    if (lb_vip->health_check) {
> +    if (lb_vip->lb_health_check) {
>          ds_put_cstr(action, "ct_lb(backends=");
>  
>          size_t n_active_backends = 0;
> @@ -3644,7 +3456,12 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *ports,
>  
>      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_nb_lb_create(nbrec_lb, ports, lbs, (void *)ovn_port_find);
> +    }
> +
> +    struct ovn_lb *lb;
> +    HMAP_FOR_EACH (lb, hmap_node, lbs) {
> +        ovn_lb_svc_create(ctx, lb, &monitor_map);
>      }
>  
>      struct service_monitor_info *mon_info;
> @@ -3658,16 +3475,6 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *ports,
>      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);
> -    }
> -}
> -
>  /* Updates the southbound Port_Binding table so that it contains the logical
>   * switch ports specified by the northbound database.
>   *
> @@ -5030,34 +4837,6 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>      }
>  }
>  
> -/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
> - * 'ip_address'.  The caller must free() the memory allocated for
> - * 'ip_address'.
> - * Returns true if parsing of 'key' was successful, false otherwise.
> - */
> -static bool
> -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> -                                uint16_t *port, int *addr_family)
> -{
> -    struct sockaddr_storage ss;
> -    if (!inet_parse_active(key, 0, &ss, false)) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key %s",
> -                     key);
> -        *ip_address = NULL;
> -        *port = 0;
> -        *addr_family = 0;
> -        return false;
> -    }
> -
> -    struct ds s = DS_EMPTY_INITIALIZER;
> -    ss_format_address_nobracks(&ss, &s);
> -    *ip_address = ds_steal_cstr(&s);
> -    *port = ss_get_port(&ss);
> -    *addr_family = ss.ss_family;
> -    return true;
> -}
> -
>  /*
>   * Returns true if logical switch is configured with DNS records, false
>   * otherwise.
> @@ -7004,7 +6783,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>      struct ovn_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;
>              }
>  
> @@ -12358,8 +12137,7 @@ ovnnb_db_run(struct northd_context *ctx,
>      sync_meters(ctx);
>      sync_dns_entries(ctx, datapaths);
>      sync_lb_entries(ctx, datapaths);
> -    destroy_ovn_lbs(&lbs);
> -    hmap_destroy(&lbs);
> +    ovn_lbs_destroy(&lbs);
>  
>      struct ovn_igmp_group *igmp_group, *next_igmp_group;
>  
>

On Fri, Oct 30, 2020 at 1:52 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 10/27/20 6:16 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>
> > ---
> >  lib/automake.mk     |   4 +-
> >  lib/lb.c            | 236 ++++++++++++++++++++++++++++++++++++
> >  lib/lb.h            |  77 ++++++++++++
> >  lib/ovn-util.c      |  28 +++++
> >  lib/ovn-util.h      |   2 +
> >  northd/ovn-northd.c | 286 +++++---------------------------------------
> >  6 files changed, 378 insertions(+), 255 deletions(-)
> >  create mode 100644 lib/lb.c
> >  create mode 100644 lib/lb.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index f3e9c8818b..430cd11fc6 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -23,7 +23,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..db2d3d552f
> > --- /dev/null
> > +++ b/lib/lb.c
> > @@ -0,0 +1,236 @@
> > +/* 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_lb *
> > +ovn_lb_create(const struct smap *vips)
> > +{
> > +    struct ovn_lb *lb = xzalloc(sizeof *lb);
> > +
> > +    lb->n_vips = smap_count(vips);
> > +    lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> > +    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_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 (struct lb_vip_backend));
>
> This should be 'sizeof *lb->vips[n_vips].backends'.

This part of the code is moved from ovn-northd.c. We could use both -
an expression
or a structure in sizeof. The coding guidelines give preference to the former.

>
> > +
>
> Nit: It's probably fine to remove an empty line here, the one above is already
> indented enough to the right.
>
> > +        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_key(token, &backend_ip,
> > +                                              &backend_port,
> > +                                              &addr_family)) {
> > +                continue;
> > +            }
> > +
> > +            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;
> > +            i++;
> > +        }
> > +
> > +        free(tokstr);
> > +        n_vips++;
> > +    }
> > +
> > +    return lb;
> > +}
> > +
> > +struct ovn_lb *
> > +ovn_nb_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_lb *lb = ovn_lb_create(&nbrec_lb->vips);
> > +    hmap_insert(lbs, &lb->hmap_node, uuid_hash(&nbrec_lb->header_.uuid));
> > +    lb->nlb = nbrec_lb;
> > +    lb->nb_lb = true;
> > +
> > +    for (size_t i = 0; i < lb->n_vips; i++) {
> > +        struct 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 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_lb *
> > +ovn_sb_lb_create(const struct sbrec_load_balancer *sbrec_lb)
> > +{
> > +    struct ovn_lb *lb = ovn_lb_create(&sbrec_lb->vips);
> > +    lb->slb = sbrec_lb;
> > +    lb->nb_lb = false;
> > +
> > +    return lb;
> > +}
> > +
> > +struct ovn_lb *
> > +ovn_lb_find(struct hmap *lbs, 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;
> > +}
> > +
> > +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);
> > +        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);
>
> This is not exactly correct if the record was created with ovn_sb_lb_create().
> It happens to work but it's actually freeing padding that happens to have been
> initialized to 0.

selection_fields is of type char *, which is set to 0 during
allocation. And calling free with a NULL
pointer is fine right ?


>
> > +}
> > +
> > +void
> > +ovn_lbs_destroy(struct hmap *lbs)
> > +{
> > +    struct ovn_lb *lb;
> > +    HMAP_FOR_EACH_POP (lb, hmap_node, lbs) {
> > +        ovn_lb_destroy(lb);
> > +        free(lb);
> > +    }
> > +    hmap_destroy(lbs);
> > +}
> > diff --git a/lib/lb.h b/lib/lb.h
> > new file mode 100644
> > index 0000000000..ffb3ba1fd1
> > --- /dev/null
> > +++ b/lib/lb.h
> > @@ -0,0 +1,77 @@
> > +/* 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 "openvswitch/hmap.h"
> > +
> > +struct nbrec_load_balancer;
> > +struct sbrec_load_balancer;
> > +struct ovn_port;
> > +struct uuid;
> > +
> > +struct ovn_lb {
> > +    struct hmap_node hmap_node;
> > +
> > +    bool nb_lb; /* NB load balancer or SB load balancer. */
> > +    union {
> > +        struct {
> > +            const struct nbrec_load_balancer *nlb; /* May be NULL. */
> > +            char *selection_fields;
> > +        };
> > +        const struct sbrec_load_balancer *slb; /* May be NULL. */
> > +    };
> > +
> > +    struct lb_vip *vips;
> > +    size_t n_vips;
> > +};
>
> I think it's better to have two different structures for NB/SB load balancers.
> We never store them together anyway.  What about something like:

I thought about this, but I think it gets  complicated. Like every
backend in NB DB can have
a health check related params. Now how do we represent that in struct
ovn_nb_lb ?

If we want to have separate structures, we should have separate lb_vip
and lb_backend structures
for NB and SB and a separate parser function too.

I'm in favor of having proposed structures in this patch.

>
> struct ovn_lb {
>     struct hmap_node hmap_node;
>     struct lb_vip *vips;
>     size_t n_vips;
> };
>
> struct ovn_nb_lb {
>    struct ovn_lb common;
>    const struct nbrec_load_balancer *nlb;
>    char *selection_fields;
>    /* Some more fields that are NB specific. */
> };
>
> struct ovn_sb_lb {
>    struct ovn_lb common;
>    const struct sbrec_load_balancer *slb;
>    /* Some more fields that are SB specific. */
> };
>
> struct ovn_nb_lb *
> ovn_nb_lb_from_lb(struct ovn_lb *lb)
> {
>     return CONTAINER_OF(lb, struct ovn_nb_lb *, common);
> }
>
> struct ovn_sb_lb *
> ovn_sb_lb_from_lb(struct ovn_lb *lb)
> {
>     return CONTAINER_OF(lb, struct ovn_sb_lb *, common);
> }
>
> > +
> > +struct lb_vip {
> > +    char *vip;
> > +    uint16_t vip_port;
> > +    int addr_family;
> > +    char *backend_ips;
> > +    char *vip_port_str;
> > +
> > +    /* Valid only for NB load balancer. */
>
> This is not really true.  In patch 3/7 we use some of the struct lb_vip_backend
> fields from SB load balancers 'backends'.
>
> > +    struct nbrec_load_balancer_health_check *lb_health_check;
> > +    struct lb_vip_backend *backends;
> > +    size_t n_backends;
>
> If we'd have separate NB/SB structures this would go in the NB-specific part.

Do you have suggestions on how to define the NB/SB structure for this ?


>
> > +};
> > +
> > +struct lb_vip_backend {
> > +    char *ip;
> > +    uint16_t port;
> > +    int addr_family;
> > +
> > +    /* Valid only for NB load balancer. */
> > +    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;
>
> If we'd have separate NB/SB structures this would go in the NB-specific part.

Do you have suggestions on how to define the NB/SB structure for this ?

Thanks
Numan

>
> Thanks,
> Dumitru
>
> > +};
> > +
> > +struct ovn_lb *ovn_nb_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_lb *ovn_sb_lb_create(const struct sbrec_load_balancer *sbrec_lb);
> > +struct ovn_lb * ovn_lb_find(struct hmap *lbs, struct uuid *uuid);
> > +void ovn_lb_destroy(struct ovn_lb *lb);
> > +void ovn_lbs_destroy(struct hmap *lbs);
> > +
> > +#endif /* OVN_LIB_LB_H 1 */
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index 1daf665037..02770bd55a 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -24,6 +24,7 @@
> >  #include "ovn-sb-idl.h"
> >  #include "unixctl.h"
> >  #include <ctype.h>
> > +#include "socket-util.h"
> >
> >  VLOG_DEFINE_THIS_MODULE(ovn_util);
> >
> > @@ -667,3 +668,30 @@ ovn_smap_get_uint(const struct smap *smap, const char *key, unsigned int def)
> >
> >      return u_value;
> >  }
> > +
> > +/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
> > + * 'ip_address'.  The caller must free() the memory allocated for
> > + * 'ip_address'.
> > + * Returns true if parsing of 'key' was successful, false otherwise.
> > + */
> > +bool
> > +ip_address_and_port_from_key(const char *key, char **ip_address,
> > +                             uint16_t *port, int *addr_family)
> > +{
> > +    struct sockaddr_storage ss;
> > +    if (!inet_parse_active(key, 0, &ss, false)) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        VLOG_WARN_RL(&rl, "bad ip address or port for key %s", key);
> > +        *ip_address = NULL;
> > +        *port = 0;
> > +        *addr_family = 0;
> > +        return false;
> > +    }
> > +
> > +    struct ds s = DS_EMPTY_INITIALIZER;
> > +    ss_format_address_nobracks(&ss, &s);
> > +    *ip_address = ds_steal_cstr(&s);
> > +    *port = ss_get_port(&ss);
> > +    *addr_family = ss.ss_family;
> > +    return true;
> > +}
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index 1d22a691f5..7fe53fd736 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -157,6 +157,8 @@ char *normalize_v46_prefix(const struct v46_ip *prefix, unsigned int plen);
> >  unsigned int ovn_smap_get_uint(const struct smap *smap, const char *key,
> >                                 unsigned int def);
> >
> > +bool ip_address_and_port_from_key(const char *key, char **ip_address,
> > +                                  uint16_t *port, int *addr_family);
> >  /* Returns a lowercase copy of orig.
> >   * Caller must free the returned string.
> >   */
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index d11888d203..1da31caf3d 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"
> > @@ -2525,10 +2526,6 @@ join_logical_ports(struct northd_context *ctx,
> >      }
> >  }
> >
> > -static bool
> > -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> > -                                uint16_t *port, int *addr_family);
> > -
> >  static void
> >  get_router_load_balancer_ips(const struct ovn_datapath *od,
> >                               struct sset *all_ips_v4, struct sset *all_ips_v6)
> > @@ -2548,8 +2545,8 @@ get_router_load_balancer_ips(const struct ovn_datapath *od,
> >              uint16_t port;
> >              int addr_family;
> >
> > -            if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port,
> > -                                                 &addr_family)) {
> > +            if (!ip_address_and_port_from_key(node->key, &ip_address, &port,
> > +                                              &addr_family)) {
> >                  continue;
> >              }
> >
> > @@ -3309,53 +3306,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. */
> > -    char *selection_fields;
> > -    struct lb_vip *vips;
> > -    size_t n_vips;
> > -};
> > -
> > -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, 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;
> > -}
> > -
> > -
> >  struct service_monitor_info {
> >      struct hmap_node hmap_node;
> >      const struct sbrec_service_monitor *sbrec_mon;
> > @@ -3395,126 +3345,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_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 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++;
> > -        }
> > -
> > -        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;
> > -            }
> > +        for (size_t j = 0; j < lb_vip->n_backends; j++) {
> > +            struct lb_vip_backend *backend = &lb_vip->backends[j];
> >
> > -            /* 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) ||
> > @@ -3524,72 +3384,24 @@ 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);
> >  }
> >
> >  static void build_lb_vip_ct_lb_actions(struct lb_vip *lb_vip,
> >                                         struct ds *action,
> >                                         char *selection_fields)
> >  {
> > -    if (lb_vip->health_check) {
> > +    if (lb_vip->lb_health_check) {
> >          ds_put_cstr(action, "ct_lb(backends=");
> >
> >          size_t n_active_backends = 0;
> > @@ -3644,7 +3456,12 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *ports,
> >
> >      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_nb_lb_create(nbrec_lb, ports, lbs, (void *)ovn_port_find);
> > +    }
> > +
> > +    struct ovn_lb *lb;
> > +    HMAP_FOR_EACH (lb, hmap_node, lbs) {
> > +        ovn_lb_svc_create(ctx, lb, &monitor_map);
> >      }
> >
> >      struct service_monitor_info *mon_info;
> > @@ -3658,16 +3475,6 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *ports,
> >      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);
> > -    }
> > -}
> > -
> >  /* Updates the southbound Port_Binding table so that it contains the logical
> >   * switch ports specified by the northbound database.
> >   *
> > @@ -5030,34 +4837,6 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
> >      }
> >  }
> >
> > -/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
> > - * 'ip_address'.  The caller must free() the memory allocated for
> > - * 'ip_address'.
> > - * Returns true if parsing of 'key' was successful, false otherwise.
> > - */
> > -static bool
> > -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> > -                                uint16_t *port, int *addr_family)
> > -{
> > -    struct sockaddr_storage ss;
> > -    if (!inet_parse_active(key, 0, &ss, false)) {
> > -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > -        VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key %s",
> > -                     key);
> > -        *ip_address = NULL;
> > -        *port = 0;
> > -        *addr_family = 0;
> > -        return false;
> > -    }
> > -
> > -    struct ds s = DS_EMPTY_INITIALIZER;
> > -    ss_format_address_nobracks(&ss, &s);
> > -    *ip_address = ds_steal_cstr(&s);
> > -    *port = ss_get_port(&ss);
> > -    *addr_family = ss.ss_family;
> > -    return true;
> > -}
> > -
> >  /*
> >   * Returns true if logical switch is configured with DNS records, false
> >   * otherwise.
> > @@ -7004,7 +6783,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
> >      struct ovn_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;
> >              }
> >
> > @@ -12358,8 +12137,7 @@ ovnnb_db_run(struct northd_context *ctx,
> >      sync_meters(ctx);
> >      sync_dns_entries(ctx, datapaths);
> >      sync_lb_entries(ctx, datapaths);
> > -    destroy_ovn_lbs(&lbs);
> > -    hmap_destroy(&lbs);
> > +    ovn_lbs_destroy(&lbs);
> >
> >      struct ovn_igmp_group *igmp_group, *next_igmp_group;
> >
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Tue, Nov 3, 2020 at 3:14 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Fri, Oct 30, 2020 at 1:52 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > On 10/27/20 6:16 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>
> > > ---
> > >  lib/automake.mk     |   4 +-
> > >  lib/lb.c            | 236 ++++++++++++++++++++++++++++++++++++
> > >  lib/lb.h            |  77 ++++++++++++
> > >  lib/ovn-util.c      |  28 +++++
> > >  lib/ovn-util.h      |   2 +
> > >  northd/ovn-northd.c | 286 +++++---------------------------------------
> > >  6 files changed, 378 insertions(+), 255 deletions(-)
> > >  create mode 100644 lib/lb.c
> > >  create mode 100644 lib/lb.h
> > >
> > > diff --git a/lib/automake.mk b/lib/automake.mk
> > > index f3e9c8818b..430cd11fc6 100644
> > > --- a/lib/automake.mk
> > > +++ b/lib/automake.mk
> > > @@ -23,7 +23,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..db2d3d552f
> > > --- /dev/null
> > > +++ b/lib/lb.c
> > > @@ -0,0 +1,236 @@
> > > +/* 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_lb *
> > > +ovn_lb_create(const struct smap *vips)
> > > +{
> > > +    struct ovn_lb *lb = xzalloc(sizeof *lb);
> > > +
> > > +    lb->n_vips = smap_count(vips);
> > > +    lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> > > +    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_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 (struct lb_vip_backend));
> >
> > This should be 'sizeof *lb->vips[n_vips].backends'.
>
> This part of the code is moved from ovn-northd.c. We could use both -
> an expression
> or a structure in sizeof. The coding guidelines give preference to the former.
>
> >
> > > +
> >
> > Nit: It's probably fine to remove an empty line here, the one above is already
> > indented enough to the right.
> >
> > > +        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_key(token, &backend_ip,
> > > +                                              &backend_port,
> > > +                                              &addr_family)) {
> > > +                continue;
> > > +            }
> > > +
> > > +            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;
> > > +            i++;
> > > +        }
> > > +
> > > +        free(tokstr);
> > > +        n_vips++;
> > > +    }
> > > +
> > > +    return lb;
> > > +}
> > > +
> > > +struct ovn_lb *
> > > +ovn_nb_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_lb *lb = ovn_lb_create(&nbrec_lb->vips);
> > > +    hmap_insert(lbs, &lb->hmap_node, uuid_hash(&nbrec_lb->header_.uuid));
> > > +    lb->nlb = nbrec_lb;
> > > +    lb->nb_lb = true;
> > > +
> > > +    for (size_t i = 0; i < lb->n_vips; i++) {
> > > +        struct 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 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_lb *
> > > +ovn_sb_lb_create(const struct sbrec_load_balancer *sbrec_lb)
> > > +{
> > > +    struct ovn_lb *lb = ovn_lb_create(&sbrec_lb->vips);
> > > +    lb->slb = sbrec_lb;
> > > +    lb->nb_lb = false;
> > > +
> > > +    return lb;
> > > +}
> > > +
> > > +struct ovn_lb *
> > > +ovn_lb_find(struct hmap *lbs, 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;
> > > +}
> > > +
> > > +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);
> > > +        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);
> >
> > This is not exactly correct if the record was created with ovn_sb_lb_create().
> > It happens to work but it's actually freeing padding that happens to have been
> > initialized to 0.
>
> selection_fields is of type char *, which is set to 0 during
> allocation. And calling free with a NULL
> pointer is fine right ?

You're right. Please ignore my comment.

Thanks
Numan

>
>
> >
> > > +}
> > > +
> > > +void
> > > +ovn_lbs_destroy(struct hmap *lbs)
> > > +{
> > > +    struct ovn_lb *lb;
> > > +    HMAP_FOR_EACH_POP (lb, hmap_node, lbs) {
> > > +        ovn_lb_destroy(lb);
> > > +        free(lb);
> > > +    }
> > > +    hmap_destroy(lbs);
> > > +}
> > > diff --git a/lib/lb.h b/lib/lb.h
> > > new file mode 100644
> > > index 0000000000..ffb3ba1fd1
> > > --- /dev/null
> > > +++ b/lib/lb.h
> > > @@ -0,0 +1,77 @@
> > > +/* 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 "openvswitch/hmap.h"
> > > +
> > > +struct nbrec_load_balancer;
> > > +struct sbrec_load_balancer;
> > > +struct ovn_port;
> > > +struct uuid;
> > > +
> > > +struct ovn_lb {
> > > +    struct hmap_node hmap_node;
> > > +
> > > +    bool nb_lb; /* NB load balancer or SB load balancer. */
> > > +    union {
> > > +        struct {
> > > +            const struct nbrec_load_balancer *nlb; /* May be NULL. */
> > > +            char *selection_fields;
> > > +        };
> > > +        const struct sbrec_load_balancer *slb; /* May be NULL. */
> > > +    };
> > > +
> > > +    struct lb_vip *vips;
> > > +    size_t n_vips;
> > > +};
> >
> > I think it's better to have two different structures for NB/SB load balancers.
> > We never store them together anyway.  What about something like:
>
> I thought about this, but I think it gets  complicated. Like every
> backend in NB DB can have
> a health check related params. Now how do we represent that in struct
> ovn_nb_lb ?
>
> If we want to have separate structures, we should have separate lb_vip
> and lb_backend structures
> for NB and SB and a separate parser function too.
>
> I'm in favor of having proposed structures in this patch.
>
> >
> > struct ovn_lb {
> >     struct hmap_node hmap_node;
> >     struct lb_vip *vips;
> >     size_t n_vips;
> > };
> >
> > struct ovn_nb_lb {
> >    struct ovn_lb common;
> >    const struct nbrec_load_balancer *nlb;
> >    char *selection_fields;
> >    /* Some more fields that are NB specific. */
> > };
> >
> > struct ovn_sb_lb {
> >    struct ovn_lb common;
> >    const struct sbrec_load_balancer *slb;
> >    /* Some more fields that are SB specific. */
> > };
> >
> > struct ovn_nb_lb *
> > ovn_nb_lb_from_lb(struct ovn_lb *lb)
> > {
> >     return CONTAINER_OF(lb, struct ovn_nb_lb *, common);
> > }
> >
> > struct ovn_sb_lb *
> > ovn_sb_lb_from_lb(struct ovn_lb *lb)
> > {
> >     return CONTAINER_OF(lb, struct ovn_sb_lb *, common);
> > }
> >
> > > +
> > > +struct lb_vip {
> > > +    char *vip;
> > > +    uint16_t vip_port;
> > > +    int addr_family;
> > > +    char *backend_ips;
> > > +    char *vip_port_str;
> > > +
> > > +    /* Valid only for NB load balancer. */
> >
> > This is not really true.  In patch 3/7 we use some of the struct lb_vip_backend
> > fields from SB load balancers 'backends'.
> >
> > > +    struct nbrec_load_balancer_health_check *lb_health_check;
> > > +    struct lb_vip_backend *backends;
> > > +    size_t n_backends;
> >
> > If we'd have separate NB/SB structures this would go in the NB-specific part.
>
> Do you have suggestions on how to define the NB/SB structure for this ?
>
>
> >
> > > +};
> > > +
> > > +struct lb_vip_backend {
> > > +    char *ip;
> > > +    uint16_t port;
> > > +    int addr_family;
> > > +
> > > +    /* Valid only for NB load balancer. */
> > > +    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;
> >
> > If we'd have separate NB/SB structures this would go in the NB-specific part.
>
> Do you have suggestions on how to define the NB/SB structure for this ?
>
> Thanks
> Numan
>
> >
> > Thanks,
> > Dumitru
> >
> > > +};
> > > +
> > > +struct ovn_lb *ovn_nb_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_lb *ovn_sb_lb_create(const struct sbrec_load_balancer *sbrec_lb);
> > > +struct ovn_lb * ovn_lb_find(struct hmap *lbs, struct uuid *uuid);
> > > +void ovn_lb_destroy(struct ovn_lb *lb);
> > > +void ovn_lbs_destroy(struct hmap *lbs);
> > > +
> > > +#endif /* OVN_LIB_LB_H 1 */
> > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > > index 1daf665037..02770bd55a 100644
> > > --- a/lib/ovn-util.c
> > > +++ b/lib/ovn-util.c
> > > @@ -24,6 +24,7 @@
> > >  #include "ovn-sb-idl.h"
> > >  #include "unixctl.h"
> > >  #include <ctype.h>
> > > +#include "socket-util.h"
> > >
> > >  VLOG_DEFINE_THIS_MODULE(ovn_util);
> > >
> > > @@ -667,3 +668,30 @@ ovn_smap_get_uint(const struct smap *smap, const char *key, unsigned int def)
> > >
> > >      return u_value;
> > >  }
> > > +
> > > +/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
> > > + * 'ip_address'.  The caller must free() the memory allocated for
> > > + * 'ip_address'.
> > > + * Returns true if parsing of 'key' was successful, false otherwise.
> > > + */
> > > +bool
> > > +ip_address_and_port_from_key(const char *key, char **ip_address,
> > > +                             uint16_t *port, int *addr_family)
> > > +{
> > > +    struct sockaddr_storage ss;
> > > +    if (!inet_parse_active(key, 0, &ss, false)) {
> > > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > +        VLOG_WARN_RL(&rl, "bad ip address or port for key %s", key);
> > > +        *ip_address = NULL;
> > > +        *port = 0;
> > > +        *addr_family = 0;
> > > +        return false;
> > > +    }
> > > +
> > > +    struct ds s = DS_EMPTY_INITIALIZER;
> > > +    ss_format_address_nobracks(&ss, &s);
> > > +    *ip_address = ds_steal_cstr(&s);
> > > +    *port = ss_get_port(&ss);
> > > +    *addr_family = ss.ss_family;
> > > +    return true;
> > > +}
> > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > > index 1d22a691f5..7fe53fd736 100644
> > > --- a/lib/ovn-util.h
> > > +++ b/lib/ovn-util.h
> > > @@ -157,6 +157,8 @@ char *normalize_v46_prefix(const struct v46_ip *prefix, unsigned int plen);
> > >  unsigned int ovn_smap_get_uint(const struct smap *smap, const char *key,
> > >                                 unsigned int def);
> > >
> > > +bool ip_address_and_port_from_key(const char *key, char **ip_address,
> > > +                                  uint16_t *port, int *addr_family);
> > >  /* Returns a lowercase copy of orig.
> > >   * Caller must free the returned string.
> > >   */
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index d11888d203..1da31caf3d 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"
> > > @@ -2525,10 +2526,6 @@ join_logical_ports(struct northd_context *ctx,
> > >      }
> > >  }
> > >
> > > -static bool
> > > -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> > > -                                uint16_t *port, int *addr_family);
> > > -
> > >  static void
> > >  get_router_load_balancer_ips(const struct ovn_datapath *od,
> > >                               struct sset *all_ips_v4, struct sset *all_ips_v6)
> > > @@ -2548,8 +2545,8 @@ get_router_load_balancer_ips(const struct ovn_datapath *od,
> > >              uint16_t port;
> > >              int addr_family;
> > >
> > > -            if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port,
> > > -                                                 &addr_family)) {
> > > +            if (!ip_address_and_port_from_key(node->key, &ip_address, &port,
> > > +                                              &addr_family)) {
> > >                  continue;
> > >              }
> > >
> > > @@ -3309,53 +3306,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. */
> > > -    char *selection_fields;
> > > -    struct lb_vip *vips;
> > > -    size_t n_vips;
> > > -};
> > > -
> > > -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, 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;
> > > -}
> > > -
> > > -
> > >  struct service_monitor_info {
> > >      struct hmap_node hmap_node;
> > >      const struct sbrec_service_monitor *sbrec_mon;
> > > @@ -3395,126 +3345,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_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 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++;
> > > -        }
> > > -
> > > -        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;
> > > -            }
> > > +        for (size_t j = 0; j < lb_vip->n_backends; j++) {
> > > +            struct lb_vip_backend *backend = &lb_vip->backends[j];
> > >
> > > -            /* 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) ||
> > > @@ -3524,72 +3384,24 @@ 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);
> > >  }
> > >
> > >  static void build_lb_vip_ct_lb_actions(struct lb_vip *lb_vip,
> > >                                         struct ds *action,
> > >                                         char *selection_fields)
> > >  {
> > > -    if (lb_vip->health_check) {
> > > +    if (lb_vip->lb_health_check) {
> > >          ds_put_cstr(action, "ct_lb(backends=");
> > >
> > >          size_t n_active_backends = 0;
> > > @@ -3644,7 +3456,12 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *ports,
> > >
> > >      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_nb_lb_create(nbrec_lb, ports, lbs, (void *)ovn_port_find);
> > > +    }
> > > +
> > > +    struct ovn_lb *lb;
> > > +    HMAP_FOR_EACH (lb, hmap_node, lbs) {
> > > +        ovn_lb_svc_create(ctx, lb, &monitor_map);
> > >      }
> > >
> > >      struct service_monitor_info *mon_info;
> > > @@ -3658,16 +3475,6 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *ports,
> > >      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);
> > > -    }
> > > -}
> > > -
> > >  /* Updates the southbound Port_Binding table so that it contains the logical
> > >   * switch ports specified by the northbound database.
> > >   *
> > > @@ -5030,34 +4837,6 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
> > >      }
> > >  }
> > >
> > > -/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
> > > - * 'ip_address'.  The caller must free() the memory allocated for
> > > - * 'ip_address'.
> > > - * Returns true if parsing of 'key' was successful, false otherwise.
> > > - */
> > > -static bool
> > > -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> > > -                                uint16_t *port, int *addr_family)
> > > -{
> > > -    struct sockaddr_storage ss;
> > > -    if (!inet_parse_active(key, 0, &ss, false)) {
> > > -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > -        VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key %s",
> > > -                     key);
> > > -        *ip_address = NULL;
> > > -        *port = 0;
> > > -        *addr_family = 0;
> > > -        return false;
> > > -    }
> > > -
> > > -    struct ds s = DS_EMPTY_INITIALIZER;
> > > -    ss_format_address_nobracks(&ss, &s);
> > > -    *ip_address = ds_steal_cstr(&s);
> > > -    *port = ss_get_port(&ss);
> > > -    *addr_family = ss.ss_family;
> > > -    return true;
> > > -}
> > > -
> > >  /*
> > >   * Returns true if logical switch is configured with DNS records, false
> > >   * otherwise.
> > > @@ -7004,7 +6783,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
> > >      struct ovn_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;
> > >              }
> > >
> > > @@ -12358,8 +12137,7 @@ ovnnb_db_run(struct northd_context *ctx,
> > >      sync_meters(ctx);
> > >      sync_dns_entries(ctx, datapaths);
> > >      sync_lb_entries(ctx, datapaths);
> > > -    destroy_ovn_lbs(&lbs);
> > > -    hmap_destroy(&lbs);
> > > +    ovn_lbs_destroy(&lbs);
> > >
> > >      struct ovn_igmp_group *igmp_group, *next_igmp_group;
> > >
> > >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >