[ovs-dev,6/8] lb: Support using templates.

Allow the CMS to configure template LBs.  The following configurations are
supported:
- VIPs of the form: ^vip_variable[:^port_variable|:port]
- Backends of the form:
  ^backendip_variable1[:^port_variable1|:port],^backendip_variable2[:^port_variable2|:port]
  OR
  ^backends_variable1,^backends_variable2

The CMS needs to provide a bit more information than with non-template load
balancers and must explicitly specify the address family to be used.

There is currently no support for template load balancers with
options:add_route=true set.  That is because ovn-northd does not
instantiate template variables.  While this is a limitation in a way, its
impact is not huge.  The load balancer 'add_route' option was added as a
way to make the CMS life easier and to avoid having to explicitly add a
route for the VIP.  The CMS can still achieve the same logical topology by
explicitly adding the VIP route.

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/lflow.c    |   18 +-
 lib/lb.c              |  463 ++++++++++++++++++++++++++++++++++++++++++-------
 lib/lb.h              |   39 +++-
 lib/ovn-util.c        |    3 
 northd/northd.c       |   85 +++++----
 ovn-nb.xml            |   12 +
 tests/ovn-nbctl.at    |   23 +-
 tests/ovn-northd.at   |    7 +
 utilities/ovn-nbctl.c |  122 +++++++------
 9 files changed, 576 insertions(+), 196 deletions(-)

On 9/30/22 16:02, Dumitru Ceara wrote:
> Allow the CMS to configure template LBs.  The following configurations are
> supported:
> - VIPs of the form: ^vip_variable[:^port_variable|:port]
> - Backends of the form:
>   ^backendip_variable1[:^port_variable1|:port],^backendip_variable2[:^port_variable2|:port]
>   OR
>   ^backends_variable1,^backends_variable2
> 
> The CMS needs to provide a bit more information than with non-template load
> balancers and must explicitly specify the address family to be used.
> 
> There is currently no support for template load balancers with
> options:add_route=true set.  That is because ovn-northd does not
> instantiate template variables.  While this is a limitation in a way, its
> impact is not huge.  The load balancer 'add_route' option was added as a
> way to make the CMS life easier and to avoid having to explicitly add a
> route for the VIP.  The CMS can still achieve the same logical topology by
> explicitly adding the VIP route.
> 
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---

This needs the following incremental fix without which
ls_in_pre_stateful logical flows are incorrect.  I'll
add this in v2 as well.

diff --git a/northd/northd.c b/northd/northd.c
index 21ce8c9185..7313972a2e 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6941,8 +6941,8 @@ build_lb_rules_pre_stateful(struct hmap *lflows, struct ovn_northd_lb *lb,
         ds_put_format(action, "%s;", ct_lb_mark ? "ct_lb_mark" : "ct_lb");
 
         ds_put_format(match, "%s.dst == %s", ip_match, lb_vip->vip_str);
-        if (lb_vip->vip_port) {
-            ds_put_format(match, " && %s.dst == %d", proto, lb_vip->vip_port);
+        if (lb_vip->port_str) {
+            ds_put_format(match, " && %s.dst == %s", proto, lb_vip->port_str);
         }
 
         struct ovn_lflow *lflow_ref = NULL;


>  controller/lflow.c    |   18 +-
>  lib/lb.c              |  463 ++++++++++++++++++++++++++++++++++++++++++-------
>  lib/lb.h              |   39 +++-
>  lib/ovn-util.c        |    3 
>  northd/northd.c       |   85 +++++----
>  ovn-nb.xml            |   12 +
>  tests/ovn-nbctl.at    |   23 +-
>  tests/ovn-northd.at   |    7 +
>  utilities/ovn-nbctl.c |  122 +++++++------
>  9 files changed, 576 insertions(+), 196 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index ad58fcd3c..ffc8fdb6e 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1993,7 +1993,9 @@ add_lb_ct_snat_hairpin_flows(struct ovn_controller_lb *lb,
>  
>  static void
>  consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
> -                          const struct hmap *local_datapaths, bool use_ct_mark,
> +                          const struct hmap *local_datapaths,
> +                          const struct smap *template_vars,
> +                          bool use_ct_mark,
>                            struct ovn_desired_flow_table *flow_table,
>                            struct simap *ids)
>  {
> @@ -2029,7 +2031,8 @@ consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
>          return;
>      }
>  
> -    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb);
> +    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb,
> +                                                            template_vars);
>      uint8_t lb_proto = IPPROTO_TCP;
>      if (lb->slb->protocol && lb->slb->protocol[0]) {
>          if (!strcmp(lb->slb->protocol, "udp")) {
> @@ -2059,7 +2062,9 @@ consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
>   * backends to handle the load balanced hairpin traffic. */
>  static void
>  add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table,
> -                     const struct hmap *local_datapaths, bool use_ct_mark,
> +                     const struct hmap *local_datapaths,
> +                     const struct smap *template_vars,
> +                     bool use_ct_mark,
>                       struct ovn_desired_flow_table *flow_table,
>                       struct simap *ids,
>                       struct id_pool *pool)
> @@ -2082,8 +2087,8 @@ add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table,
>              ovs_assert(id_pool_alloc_id(pool, &id));
>              simap_put(ids, lb->name, id);
>          }
> -        consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark, flow_table,
> -                                  ids);
> +        consider_lb_hairpin_flows(lb, local_datapaths, template_vars,
> +                                  use_ct_mark, flow_table, ids);
>      }
>  }
>  
> @@ -2220,6 +2225,7 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out)
>                         l_ctx_in->local_datapaths,
>                         l_ctx_out->flow_table);
>      add_lb_hairpin_flows(l_ctx_in->lb_table, l_ctx_in->local_datapaths,
> +                         l_ctx_in->template_vars,
>                           l_ctx_in->lb_hairpin_use_ct_mark,
>                           l_ctx_out->flow_table,
>                           l_ctx_out->hairpin_lb_ids,
> @@ -2351,6 +2357,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
>       * associated. */
>      for (size_t i = 0; i < n_dp_lbs; i++) {
>          consider_lb_hairpin_flows(dp_lbs[i], l_ctx_in->local_datapaths,
> +                                  l_ctx_in->template_vars,
>                                    l_ctx_in->lb_hairpin_use_ct_mark,
>                                    l_ctx_out->flow_table,
>                                    l_ctx_out->hairpin_lb_ids);
> @@ -2493,6 +2500,7 @@ lflow_handle_changed_lbs(struct lflow_ctx_in *l_ctx_in,
>          VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT,
>                   UUID_ARGS(&lb->header_.uuid));
>          consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> +                                  l_ctx_in->template_vars,
>                                    l_ctx_in->lb_hairpin_use_ct_mark,
>                                    l_ctx_out->flow_table,
>                                    l_ctx_out->hairpin_lb_ids);
> diff --git a/lib/lb.c b/lib/lb.c
> index 77674ea28..15cbf29ee 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -19,6 +19,7 @@
>  #include "lib/ovn-nb-idl.h"
>  #include "lib/ovn-sb-idl.h"
>  #include "lib/ovn-util.h"
> +#include "ovn/lex.h"
>  
>  /* OpenvSwitch lib includes. */
>  #include "openvswitch/vlog.h"
> @@ -26,6 +27,16 @@
>  
>  VLOG_DEFINE_THIS_MODULE(lb);
>  
> +static const char *lb_neighbor_responder_mode_names[] = {
> +    [LB_NEIGH_RESPOND_REACHABLE] = "reachable",
> +    [LB_NEIGH_RESPOND_ALL] = "all",
> +    [LB_NEIGH_RESPOND_NONE] = "none",
> +};
> +
> +static struct nbrec_load_balancer_health_check *
> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
> +                        const char *vip_port_str, bool template);
> +
>  struct ovn_lb_ip_set *
>  ovn_lb_ip_set_create(void)
>  {
> @@ -71,108 +82,312 @@ ovn_lb_ip_set_clone(struct ovn_lb_ip_set *lb_ip_set)
>      return clone;
>  }
>  
> -static
> -bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> -                     const char *lb_value)
> +/* Format for backend ips: "IP1:port1,IP2:port2,...". */
> +static char *
> +ovn_lb_backends_init_explicit(struct ovn_lb_vip *lb_vip, const char *value,
> +                              size_t *n_backends)
>  {
> -    int addr_family;
> -
> -    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
> -                                         &lb_vip->vip_port, &addr_family)) {
> -        return false;
> -    }
> -
> -    if (addr_family == AF_INET) {
> -        ovs_be32 vip4;
> -        ip_parse(lb_vip->vip_str, &vip4);
> -        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
> -    } else {
> -        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
> -    }
> -
> -    /* Format for backend ips: "IP1:port1,IP2:port2,...". */
> -    size_t n_backends = 0;
> +    struct ds errors = DS_EMPTY_INITIALIZER;
>      size_t n_allocated_backends = 0;
> -    char *tokstr = xstrdup(lb_value);
> +    char *tokstr = xstrdup(value);
>      char *save_ptr = NULL;
> +    *n_backends = 0;
> +
>      for (char *token = strtok_r(tokstr, ",", &save_ptr);
>          token != NULL;
>          token = strtok_r(NULL, ",", &save_ptr)) {
>  
> -        if (n_backends == n_allocated_backends) {
> +        if (*n_backends == n_allocated_backends) {
>              lb_vip->backends = x2nrealloc(lb_vip->backends,
>                                            &n_allocated_backends,
>                                            sizeof *lb_vip->backends);
>          }
>  
> -        struct ovn_lb_backend *backend = &lb_vip->backends[n_backends];
> +        struct ovn_lb_backend *backend = &lb_vip->backends[(*n_backends)];
>          int backend_addr_family;
>          if (!ip_address_and_port_from_lb_key(token, &backend->ip_str,
>                                               &backend->port,
>                                               &backend_addr_family)) {
> +            if (lb_vip->port_str) {
> +                ds_put_format(&errors, "%s: should be an IP address and a "
> +                                       "port number with : as a separator, ",
> +                              token);
> +            } else {
> +                ds_put_format(&errors, "%s: should be an IP address, ", token);
> +            }
>              continue;
>          }
>  
> -        if (addr_family != backend_addr_family) {
> +        if (lb_vip->address_family != backend_addr_family) {
>              free(backend->ip_str);
> +            ds_put_format(&errors, "%s: IP address family is different from "
> +                                   "VIP %s, ",
> +                          token, lb_vip->vip_str);
>              continue;
>          }
>  
> -        if (addr_family == AF_INET) {
> +        if (lb_vip->port_str) {
> +            if (!backend->port) {
> +                free(backend->ip_str);
> +                ds_put_format(&errors, "%s: should be an IP address and "
> +                                       "a port number with : as a separator, ",
> +                              token);
> +                continue;
> +            }
> +        } else {
> +            if (backend->port) {
> +                free(backend->ip_str);
> +                ds_put_format(&errors, "%s: should be an IP address, ", token);
> +                continue;
> +            }
> +        }
> +
> +        backend->port_str =
> +            backend->port ? xasprintf("%"PRIu16, backend->port) : NULL;
> +
> +        if (lb_vip->address_family == AF_INET) {
>              ovs_be32 ip4;
>              ip_parse(backend->ip_str, &ip4);
>              in6_addr_set_mapped_ipv4(&backend->ip, ip4);
>          } else {
>              ipv6_parse(backend->ip_str, &backend->ip);
>          }
> -        n_backends++;
> +        (*n_backends)++;
>      }
>      free(tokstr);
> -    lb_vip->n_backends = n_backends;
> -    return true;
> +
> +    if (ds_last(&errors) != EOF) {
> +        ds_chomp(&errors, ' ');
> +        ds_chomp(&errors, ',');
> +        ds_put_char(&errors, '.');
> +        return ds_steal_cstr(&errors);
> +    }
> +    return NULL;
>  }
>  
>  static
> -void ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
> +char *ovn_lb_vip_init_explicit(struct ovn_lb_vip *lb_vip, const char *lb_key,
> +                               const char *lb_value)
> +{
> +    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
> +                                         &lb_vip->vip_port,
> +                                         &lb_vip->address_family)) {
> +        return xasprintf("%s: should be an IP address (or an IP address "
> +                         "and a port number with : as a separator).", lb_key);
> +    }
> +
> +    lb_vip->port_str = lb_vip->vip_port
> +                       ? xasprintf("%"PRIu16, lb_vip->vip_port)
> +                       : NULL;
> +
> +    if (lb_vip->address_family == AF_INET) {
> +        ovs_be32 vip4;
> +        ip_parse(lb_vip->vip_str, &vip4);
> +        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
> +    } else {
> +        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
> +    }
> +    return ovn_lb_backends_init_explicit(lb_vip, lb_value,
> +                                         &lb_vip->n_backends);
> +}
> +
> +/* Parses backends of a templated LB VIP.
> + * For now only the following template forms are supported:
> + * A.
> + *   ^backendip_variable1[:^port_variable1|:port],
> + *   ^backendip_variable2[:^port_variable2|:port]
> + *
> + * B.
> + *   ^backends_variable1,^backends_variable2 is also a thing
> + *      where 'backends_variable1' may expand to IP1_1:PORT1_1 on chassis-1
> + *                                               IP1_2:PORT1_2 on chassis-2
> + *        and 'backends_variable2' may expand to IP2_1:PORT2_1 on chassis-1
> + *                                               IP2_2:PORT2_2 on chassis-2
> + */
> +static char *
> +ovn_lb_backends_init_template(struct ovn_lb_vip *lb_vip, const char *value_,
> +                              size_t *n_backends)
> +{
> +    struct ds errors = DS_EMPTY_INITIALIZER;
> +    char *value = xstrdup(value_);
> +    char *save_ptr = NULL;
> +    size_t n_allocated_backends = 0;
> +    *n_backends = 0;
> +
> +    for (char *backend = strtok_r(value, ",", &save_ptr); backend;
> +         backend = strtok_r(NULL, ",", &save_ptr)) {
> +
> +        char *atom = xstrdup(backend);
> +        char *save_ptr2 = NULL;
> +        bool success = false;
> +        char *backend_ip = NULL;
> +        char *backend_port = NULL;
> +
> +        for (char *subatom = strtok_r(atom, ":", &save_ptr2); subatom;
> +             subatom = strtok_r(NULL, ":", &save_ptr2)) {
> +            if (backend_ip && backend_port) {
> +                success = false;
> +                break;
> +            }
> +            success = true;
> +            if (!backend_ip) {
> +                backend_ip = xstrdup(subatom);
> +            } else {
> +                backend_port = xstrdup(subatom);
> +            }
> +        }
> +
> +        if (success) {
> +            if (*n_backends == n_allocated_backends) {
> +                lb_vip->backends = x2nrealloc(lb_vip->backends,
> +                                              &n_allocated_backends,
> +                                              sizeof *lb_vip->backends);
> +            }
> +
> +            struct ovn_lb_backend *lb_backend =
> +                &lb_vip->backends[(*n_backends)];
> +            lb_backend->ip_str = backend_ip;
> +            lb_backend->port_str = backend_port;
> +            lb_backend->port = 0;
> +            (*n_backends)++;
> +        } else {
> +            ds_put_format(&errors, "%s: should be a template of the form: "
> +                          "'^backendip_variable1[:^port_variable1|:port]', ",
> +                          atom);
> +        }
> +        free(atom);
> +    }
> +
> +    free(value);
> +    if (ds_last(&errors) != EOF) {
> +        ds_chomp(&errors, ' ');
> +        ds_chomp(&errors, ',');
> +        ds_put_char(&errors, '.');
> +        return ds_steal_cstr(&errors);
> +    }
> +    return NULL;
> +}
> +
> +/* Parses a VIP of a templated LB.
> + * For now only the following template forms are supported:
> + *   ^vip_variable[:^port_variable|:port]
> + */
> +static char *
> +ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip, const char *lb_key_,
> +                         const char *lb_value, int address_family)
> +{
> +    char *save_ptr = NULL;
> +    char *lb_key = xstrdup(lb_key_);
> +    bool success = false;
> +
> +    for (char *atom = strtok_r(lb_key, ":", &save_ptr); atom;
> +         atom = strtok_r(NULL, ":", &save_ptr)) {
> +        if (lb_vip->vip_str && lb_vip->port_str) {
> +            success = false;
> +            break;
> +        }
> +        success = true;
> +        if (!lb_vip->vip_str) {
> +            lb_vip->vip_str = xstrdup(atom);
> +        } else {
> +            lb_vip->port_str = xstrdup(atom);
> +        }
> +    }
> +    free(lb_key);
> +
> +    if (!success) {
> +        return xasprintf("%s: should be a template of the form: "
> +                         "'^vip_variable[:^port_variable|:port]'.",
> +                         lb_key_);
> +    }
> +
> +    lb_vip->address_family = address_family;
> +    return ovn_lb_backends_init_template(lb_vip, lb_value,
> +                                         &lb_vip->n_backends);
> +}
> +
> +/* Returns NULL on success, an error string on failure.  The caller is
> + * responsible for destroying 'lb_vip' in all cases.
> + */
> +char *
> +ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> +                const char *lb_value, bool template, int address_family)
> +{
> +    memset(lb_vip, 0, sizeof *lb_vip);
> +
> +    return !template
> +           ?  ovn_lb_vip_init_explicit(lb_vip, lb_key, lb_value)
> +           :  ovn_lb_vip_init_template(lb_vip, lb_key, lb_value,
> +                                       address_family);
> +}
> +
> +void
> +ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
>  {
>      free(vip->vip_str);
> +    free(vip->port_str);
>      for (size_t i = 0; i < vip->n_backends; i++) {
>          free(vip->backends[i].ip_str);
> +        free(vip->backends[i].port_str);
>      }
>      free(vip->backends);
>  }
>  
> +void
> +ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s, bool template)
> +{
> +    bool needs_brackets = vip->address_family == AF_INET6 && vip->port_str
> +                          && !template;
> +    if (needs_brackets) {
> +        ds_put_char(s, '[');
> +    }
> +    ds_put_cstr(s, vip->vip_str);
> +    if (needs_brackets) {
> +        ds_put_char(s, ']');
> +    }
> +    if (vip->port_str) {
> +        ds_put_format(s, ":%s", vip->port_str);
> +    }
> +}
> +
> +void
> +ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s,
> +                           bool template)
> +{
> +    bool needs_brackets = vip->address_family == AF_INET6 && vip->port_str
> +                          && !template;
> +    for (size_t i = 0; i < vip->n_backends; i++) {
> +        struct ovn_lb_backend *backend = &vip->backends[i];
> +
> +        if (needs_brackets) {
> +            ds_put_char(s, '[');
> +        }
> +        ds_put_cstr(s, backend->ip_str);
> +        if (needs_brackets) {
> +            ds_put_char(s, ']');
> +        }
> +        if (backend->port_str) {
> +            ds_put_format(s, ":%s", backend->port_str);
> +        }
> +        if (i != vip->n_backends - 1) {
> +            ds_put_char(s, ',');
> +        }
> +    }
> +}
> +
>  static
>  void ovn_northd_lb_vip_init(struct ovn_northd_lb_vip *lb_vip_nb,
>                              const struct ovn_lb_vip *lb_vip,
>                              const struct nbrec_load_balancer *nbrec_lb,
> -                            const char *vip_port_str, const char *backend_ips)
> +                            const char *vip_port_str, const char *backend_ips,
> +                            bool template)
>  {
>      lb_vip_nb->backend_ips = xstrdup(backend_ips);
>      lb_vip_nb->n_backends = lb_vip->n_backends;
>      lb_vip_nb->backends_nb = xcalloc(lb_vip_nb->n_backends,
>                                       sizeof *lb_vip_nb->backends_nb);
> -
> -    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, vip_port_str)) {
> -                lb_health_check = nbrec_lb->health_check[j];
> -                break;
> -            }
> -        }
> -    }
> -
> -    lb_vip_nb->lb_health_check = lb_health_check;
> +    lb_vip_nb->lb_health_check =
> +        ovn_lb_get_health_check(nbrec_lb, vip_port_str, template);
>  }
>  
>  static
> @@ -203,12 +418,111 @@ ovn_lb_get_hairpin_snat_ip(const struct uuid *lb_uuid,
>      }
>  }
>  
> +static bool
> +ovn_lb_get_routable_mode(const struct nbrec_load_balancer *nbrec_lb,
> +                         bool routable, bool template)
> +{
> +    if (template && routable) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +        VLOG_WARN_RL(&rl, "Template load balancer "UUID_FMT" does not suport "
> +                           "option 'add_route'.  Forcing it to disabled.",
> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
> +        return false;
> +    }
> +    return routable;
> +}
> +
> +static bool
> +ovn_lb_neigh_mode_is_valid(enum lb_neighbor_responder_mode mode, bool template)
> +{
> +    if (!template) {
> +        return true;
> +    }
> +
> +    switch (mode) {
> +    case LB_NEIGH_RESPOND_REACHABLE:
> +        return false;
> +    case LB_NEIGH_RESPOND_ALL:
> +    case LB_NEIGH_RESPOND_NONE:
> +        return true;
> +    }
> +}
> +
> +static enum lb_neighbor_responder_mode
> +ovn_lb_get_neigh_mode(const struct nbrec_load_balancer *nbrec_lb,
> +                      const char *mode, bool template)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +    enum lb_neighbor_responder_mode default_mode =
> +        template ? LB_NEIGH_RESPOND_NONE : LB_NEIGH_RESPOND_REACHABLE;
> +
> +    if (!mode) {
> +        mode = lb_neighbor_responder_mode_names[default_mode];
> +    }
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(lb_neighbor_responder_mode_names); i++) {
> +        if (!strcmp(mode, lb_neighbor_responder_mode_names[i])) {
> +            if (ovn_lb_neigh_mode_is_valid(i, template)) {
> +                return i;
> +            }
> +            break;
> +        }
> +    }
> +
> +    VLOG_WARN_RL(&rl, "Invalid neighbor responder mode %s for load balancer "
> +                       UUID_FMT", forcing it to %s",
> +                 mode, UUID_ARGS(&nbrec_lb->header_.uuid),
> +                 lb_neighbor_responder_mode_names[default_mode]);
> +    return default_mode;
> +}
> +
> +static struct nbrec_load_balancer_health_check *
> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
> +                        const char *vip_port_str, bool template)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +
> +    if (!nbrec_lb->n_health_check) {
> +        return NULL;
> +    }
> +
> +    if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) {
> +        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));
> +        return NULL;
> +    }
> +
> +    if (template) {
> +        VLOG_WARN_RL(&rl,
> +                     "Template load balancers do not currently support "
> +                     "health checks. Not creating health checks for "
> +                     "load balancer " UUID_FMT,
> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
> +        return NULL;
> +    }
> +
> +    for (size_t i = 0; i < nbrec_lb->n_health_check; i++) {
> +        if (!strcmp(nbrec_lb->health_check[i]->vip, vip_port_str)) {
> +            return nbrec_lb->health_check[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
>  struct ovn_northd_lb *
>  ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>  {
>      bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
>      bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol, "sctp");
>      struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> +    int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> +                                              "address-family", "ipv4"),
> +                                 "ipv4")
> +                         ? AF_INET
> +                         : AF_INET6;
>  
>      lb->nlb = nbrec_lb;
>      lb->proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
> @@ -216,12 +530,16 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>      lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips);
>      lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb);
>      lb->controller_event = smap_get_bool(&nbrec_lb->options, "event", false);
> -    lb->routable = smap_get_bool(&nbrec_lb->options, "add_route", false);
> +
> +    bool routable = smap_get_bool(&nbrec_lb->options, "add_route", false);
> +    lb->routable = ovn_lb_get_routable_mode(nbrec_lb, routable, lb->template);
> +
>      lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat", false);
> -    const char *mode =
> -        smap_get_def(&nbrec_lb->options, "neighbor_responder", "reachable");
> -    lb->neigh_mode = strcmp(mode, "all") ? LB_NEIGH_RESPOND_REACHABLE
> -                                         : LB_NEIGH_RESPOND_ALL;
> +    lb->template = smap_get_bool(&nbrec_lb->options, "template", false);
> +
> +    const char *mode = smap_get(&nbrec_lb->options, "neighbor_responder");
> +    lb->neigh_mode = ovn_lb_get_neigh_mode(nbrec_lb, mode, lb->template);
> +
>      sset_init(&lb->ips_v4);
>      sset_init(&lb->ips_v6);
>      struct smap_node *node;
> @@ -231,13 +549,19 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[n_vips];
>  
> -        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> -                                                  "reject", false);
> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
> +        char *error = ovn_lb_vip_init(lb_vip, node->key, node->value,
> +                                      lb->template, address_family);
> +        if (error) {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +            VLOG_WARN_RL(&rl, "Failed to initialize LB VIP: %s", error);
> +            ovn_lb_vip_destroy(lb_vip);
> +            free(error);
>              continue;
>          }
> +        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> +                                                  "reject", false);
>          ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> -                               node->key, node->value);
> +                               node->key, node->value, lb->template);
>          if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>              sset_add(&lb->ips_v4, lb_vip->vip_str);
>          } else {
> @@ -385,7 +709,8 @@ ovn_lb_group_find(const struct hmap *lb_groups, const struct uuid *uuid)
>  }
>  
>  struct ovn_controller_lb *
> -ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb)
> +ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb,
> +                         const struct smap *template_vars)
>  {
>      struct ovn_controller_lb *lb = xzalloc(sizeof *lb);
>  
> @@ -399,10 +724,20 @@ ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb)
>      SMAP_FOR_EACH (node, &sbrec_lb->vips) {
>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>  
> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
> -            continue;
> +        char *key_s = lexer_parse_template_string(xstrdup(node->key),
> +                                                  template_vars,
> +                                                  NULL);
> +        char *value_s = lexer_parse_template_string(xstrdup(node->value),
> +                                                    template_vars,
> +                                                    NULL);
> +        char *error = ovn_lb_vip_init_explicit(lb_vip, key_s, value_s);
> +        if (error) {
> +            free(error);
> +        } else {
> +            n_vips++;
>          }
> -        n_vips++;
> +        free(key_s);
> +        free(value_s);
>      }
>  
>      /* It's possible that parsing VIPs fails.  Update the lb->n_vips to the
> diff --git a/lib/lb.h b/lib/lb.h
> index 80ac03399..265a1215a 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -35,6 +35,7 @@ struct uuid;
>  enum lb_neighbor_responder_mode {
>      LB_NEIGH_RESPOND_REACHABLE,
>      LB_NEIGH_RESPOND_ALL,
> +    LB_NEIGH_RESPOND_NONE,
>  };
>  
>  /* The "routable" ssets are subsets of the load balancer IPs for which IP
> @@ -67,6 +68,7 @@ struct ovn_northd_lb {
>      bool controller_event;
>      bool routable;
>      bool skip_snat;
> +    bool template;
>  
>      struct sset ips_v4;
>      struct sset ips_v6;
> @@ -81,19 +83,31 @@ struct ovn_northd_lb {
>  };
>  
>  struct ovn_lb_vip {
> -    struct in6_addr vip;
> -    char *vip_str;
> -    uint16_t vip_port;
> -
> +    struct in6_addr vip; /* Only used in ovn-controller. */
> +    char *vip_str;       /* Actual VIP string representation (without port).
> +                          * To be used in ovn-northd.
> +                          */
> +    uint16_t vip_port;   /* Only used in ovn-controller. */
> +    char *port_str;      /* Actual port string representation.  To be used
> +                          * in ovn-controller.
> +                          */
>      struct ovn_lb_backend *backends;
>      size_t n_backends;
>      bool empty_backend_rej;
> +    int address_family;
>  };
>  
>  struct ovn_lb_backend {
> -    struct in6_addr ip;
> -    char *ip_str;
> -    uint16_t port;
> +    struct in6_addr ip;  /* Only used in ovn-controller. */
> +    char *ip_str;        /* Actual IP string representation. To be used in
> +                          * ovn-northd.
> +                          */
> +    uint16_t port;       /* Mostly used in ovn-controller but also for
> +                          * healthcheck in ovn-northd.
> +                          */
> +    char *port_str;      /* Actual port string representation. To be used
> +                          * in ovn-northd.
> +                          */
>  };
>  
>  /* ovn-northd specific backend information. */
> @@ -171,7 +185,16 @@ struct ovn_controller_lb {
>  };
>  
>  struct ovn_controller_lb *ovn_controller_lb_create(
> -    const struct sbrec_load_balancer *);
> +    const struct sbrec_load_balancer *,
> +    const struct smap *template_vars);
>  void ovn_controller_lb_destroy(struct ovn_controller_lb *);
>  
> +char *ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> +                      const char *lb_value, bool template, int address_family);
> +void ovn_lb_vip_destroy(struct ovn_lb_vip *vip);
> +void ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s,
> +                       bool template);
> +void ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s,
> +                                bool template);
> +
>  #endif /* OVN_LIB_LB_H 1 */
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 596e93129..1cc4c98b1 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -799,9 +799,6 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address,
>  {
>      struct sockaddr_storage ss;
>      if (!inet_parse_active(key, 0, &ss, false, NULL)) {
> -        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;
> diff --git a/northd/northd.c b/northd/northd.c
> index 63879ccd0..a93aee37e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3708,6 +3708,10 @@ static void
>  ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, struct ovn_northd_lb *lb,
>                    struct hmap *monitor_map, struct hmap *ports)
>  {
> +    if (lb->template) {
> +        return;
> +    }
> +
>      for (size_t i = 0; i < lb->n_vips; i++) {
>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
> @@ -4024,12 +4028,19 @@ static void
>  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
>                                 const struct ovn_northd_lb *lb)
>  {
> +    /* If configured to not reply to any neighbor requests for all VIPs
> +     * return early.
> +     */
> +    if (lb->neigh_mode == LB_NEIGH_RESPOND_NONE) {
> +        return;
> +    }
> +
>      /* If configured to reply to neighbor requests for all VIPs force them
>       * all to be considered "reachable".
>       */
>      if (lb->neigh_mode == LB_NEIGH_RESPOND_ALL) {
>          for (size_t i = 0; i < lb->n_vips; i++) {
> -            if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
> +            if (lb->vips[i].address_family == AF_INET) {
>                  sset_add(&od->lb_ips->ips_v4_reachable, lb->vips[i].vip_str);
>              } else {
>                  sset_add(&od->lb_ips->ips_v6_reachable, lb->vips[i].vip_str);
> @@ -4041,8 +4052,9 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
>      /* Otherwise, a VIP is reachable if there's at least one router
>       * subnet that includes it.
>       */
> +    ovs_assert(lb->neigh_mode == LB_NEIGH_RESPOND_REACHABLE);
>      for (size_t i = 0; i < lb->n_vips; i++) {
> -        if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
> +        if (lb->vips[i].address_family == AF_INET) {
>              ovs_be32 vip_ip4 = in6_addr_get_mapped_ipv4(&lb->vips[i].vip);
>              struct ovn_port *op;
>  
> @@ -5823,16 +5835,16 @@ build_empty_lb_event_flow(struct ovn_lb_vip *lb_vip,
>      ds_clear(action);
>      ds_clear(match);
>  
> -    bool ipv4 = IN6_IS_ADDR_V4MAPPED(&lb_vip->vip);
> +    bool ipv4 = lb_vip->address_family == AF_INET;
>  
>      ds_put_format(match, "ip%s.dst == %s && %s",
>                    ipv4 ? "4": "6", lb_vip->vip_str, lb->proto);
>  
>      char *vip = lb_vip->vip_str;
> -    if (lb_vip->vip_port) {
> -        ds_put_format(match, " && %s.dst == %u", lb->proto, lb_vip->vip_port);
> -        vip = xasprintf("%s%s%s:%u", ipv4 ? "" : "[", lb_vip->vip_str,
> -                        ipv4 ? "" : "]", lb_vip->vip_port);
> +    if (lb_vip->port_str) {
> +        ds_put_format(match, " && %s.dst == %s", lb->proto, lb_vip->port_str);
> +        vip = xasprintf("%s%s%s:%s", ipv4 ? "" : "[", lb_vip->vip_str,
> +                        ipv4 ? "" : "]", lb_vip->port_str);
>      }
>  
>      ds_put_format(action,
> @@ -5843,7 +5855,7 @@ build_empty_lb_event_flow(struct ovn_lb_vip *lb_vip,
>                    event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS),
>                    vip, lb->proto,
>                    UUID_ARGS(&lb->nlb->header_.uuid));
> -    if (lb_vip->vip_port) {
> +    if (lb_vip->port_str) {
>          free(vip);
>      }
>      return true;
> @@ -6899,7 +6911,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows, struct ovn_northd_lb *lb,
>          /* Store the original destination IP to be used when generating
>           * hairpin flows.
>           */
> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +        if (lb->vips[i].address_family == AF_INET) {
>              ip_match = "ip4";
>              ds_put_format(action, REG_ORIG_DIP_IPV4 " = %s; ",
>                            lb_vip->vip_str);
> @@ -6910,7 +6922,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows, struct ovn_northd_lb *lb,
>          }
>  
>          const char *proto = NULL;
> -        if (lb_vip->vip_port) {
> +        if (lb_vip->port_str) {
>              proto = "tcp";
>              if (lb->nlb->protocol) {
>                  if (!strcmp(lb->nlb->protocol, "udp")) {
> @@ -6923,8 +6935,8 @@ build_lb_rules_pre_stateful(struct hmap *lflows, struct ovn_northd_lb *lb,
>              /* Store the original destination port to be used when generating
>               * hairpin flows.
>               */
> -            ds_put_format(action, REG_ORIG_TP_DPORT " = %"PRIu16"; ",
> -                          lb_vip->vip_port);
> +            ds_put_format(action, REG_ORIG_TP_DPORT " = %s; ",
> +                          lb_vip->port_str);
>          }
>          ds_put_format(action, "%s;", ct_lb_mark ? "ct_lb_mark" : "ct_lb");
>  
> @@ -6962,24 +6974,12 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark,
>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
>          const char *ip_match = NULL;
> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +        if (lb_vip->address_family == AF_INET) {
>              ip_match = "ip4";
>          } else {
>              ip_match = "ip6";
>          }
>  
> -        const char *proto = NULL;
> -        if (lb_vip->vip_port) {
> -            proto = "tcp";
> -            if (lb->nlb->protocol) {
> -                if (!strcmp(lb->nlb->protocol, "udp")) {
> -                    proto = "udp";
> -                } else if (!strcmp(lb->nlb->protocol, "sctp")) {
> -                    proto = "sctp";
> -                }
> -            }
> -        }
> -
>          ds_clear(action);
>          ds_clear(match);
>  
> @@ -6997,8 +6997,9 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark,
>          ds_put_format(match, "ct.new && %s.dst == %s", ip_match,
>                        lb_vip->vip_str);
>          int priority = 110;
> -        if (lb_vip->vip_port) {
> -            ds_put_format(match, " && %s.dst == %d", proto, lb_vip->vip_port);
> +        if (lb_vip->port_str) {
> +            ds_put_format(match, " && %s.dst == %s", lb->proto,
> +                          lb_vip->port_str);
>              priority = 120;
>          }
>  
> @@ -9998,7 +9999,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
>       * of "ct_lb_mark($targets);". The other flow is for ct.est with
>       * an action of "next;".
>       */
> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +    if (lb_vip->address_family == AF_INET) {
>          ds_put_format(match, "ip4 && "REG_NEXT_HOP_IPV4" == %s",
>                        lb_vip->vip_str);
>      } else {
> @@ -10014,14 +10015,14 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
>      }
>  
>      int prio = 110;
> -    if (lb_vip->vip_port) {
> +    if (lb_vip->port_str) {
>          prio = 120;
>          new_match = xasprintf("ct.new && %s && %s && "
> -                              REG_ORIG_TP_DPORT_ROUTER" == %d",
> -                              ds_cstr(match), lb->proto, lb_vip->vip_port);
> +                              REG_ORIG_TP_DPORT_ROUTER" == %s",
> +                              ds_cstr(match), lb->proto, lb_vip->port_str);
>          est_match = xasprintf("ct.est && %s && %s && "
> -                              REG_ORIG_TP_DPORT_ROUTER" == %d && %s == 1",
> -                              ds_cstr(match), lb->proto, lb_vip->vip_port,
> +                              REG_ORIG_TP_DPORT_ROUTER" == %s && %s == 1",
> +                              ds_cstr(match), lb->proto, lb_vip->port_str,
>                                ct_natted);
>      } else {
>          new_match = xasprintf("ct.new && %s", ds_cstr(match));
> @@ -10030,7 +10031,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
>      }
>  
>      const char *ip_match = NULL;
> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +    if (lb_vip->address_family == AF_INET) {
>          ip_match = "ip4";
>      } else {
>          ip_match = "ip6";
> @@ -10048,9 +10049,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
>          ds_put_format(&undnat_match, "(%s.src == %s", ip_match,
>                        backend->ip_str);
>  
> -        if (backend->port) {
> -            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> -                          lb->proto, backend->port);
> +        if (backend->port_str) {
> +            ds_put_format(&undnat_match, " && %s.src == %s) || ",
> +                          lb->proto, backend->port_str);
>          } else {
>              ds_put_cstr(&undnat_match, ") || ");
>          }
> @@ -10063,9 +10064,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
>      struct ds unsnat_match = DS_EMPTY_INITIALIZER;
>      ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
>                    ip_match, ip_match, lb_vip->vip_str, lb->proto);
> -    if (lb_vip->vip_port) {
> -        ds_put_format(&unsnat_match, " && %s.dst == %d", lb->proto,
> -                      lb_vip->vip_port);
> +    if (lb_vip->port_str) {
> +        ds_put_format(&unsnat_match, " && %s.dst == %s", lb->proto,
> +                      lb_vip->port_str);
>      }
>  
>      struct ovn_datapath **gw_router_skip_snat =
> @@ -10305,7 +10306,7 @@ build_lrouter_defrag_flows_for_lb(struct ovn_northd_lb *lb,
>          ds_clear(&defrag_actions);
>          ds_clear(match);
>  
> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +        if (lb_vip->address_family == AF_INET) {
>              ds_put_format(match, "ip && ip4.dst == %s", lb_vip->vip_str);
>              ds_put_format(&defrag_actions, REG_NEXT_HOP_IPV4" = %s; ",
>                            lb_vip->vip_str);
> @@ -10315,7 +10316,7 @@ build_lrouter_defrag_flows_for_lb(struct ovn_northd_lb *lb,
>                            lb_vip->vip_str);
>          }
>  
> -        if (lb_vip->vip_port) {
> +        if (lb_vip->port_str) {
>              ds_put_format(match, " && %s", lb->proto);
>              prio = 110;
>  
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index bd28ac372..fb63b212e 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1908,6 +1908,18 @@
>          requests only for VIPs that are part of a router's subnet.  The default
>          value of this option, if not specified, is <code>reachable</code>.
>        </column>
> +
> +      <column name="options" key="template">
> +        Option to be set to <code>true</code>, if the load balancer is a
> +        template.  In this case the template variables used within the
> +        load balancer VIPs and backends will be instantiated differently
> +        on different chassis.
> +      </column>
> +
> +      <column name="options" key="address-family">
> +        Address family used by the load balancer.  This value is used only
> +        when the load balancer is a template.
> +      </column>
>      </group>
>    </table>
>  
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 6a5d52a5d..9b766d63e 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -857,23 +857,19 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:a80], [
>  [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
>  ])
>  
> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
> -[ovn-nbctl: 192.168.10.10:: should be an IP address.
> -])
> -
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
>  [ovn-nbctl: 192.168.10.1a: should be an IP address.
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +[ovn-nbctl: 192.168.10.10:80: should be an IP address, 192.168.10.20:80: should be an IP address.
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>  ])
>  
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:900 tcp], [1], [],
> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>  ])
>  
> @@ -1111,7 +1107,7 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0
>  
>  
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> -[ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80: should be an IP address.
>  ])
>  
>  
> @@ -1125,18 +1121,13 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1]
>  ])
>  
>  
> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
> -[ovn-nbctl: [[fd0f::10]]:: should be an IP address.
> -])
> -
> -
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
>  [ovn-nbctl: fd0f::1001a: should be an IP address.
>  ])
>  
>  
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80: should be an IP address.
>  ])
>  
>  
> @@ -1146,7 +1137,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10 tcp], [1], [],
>  
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:900 tcp], [1], [],
> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +[ovn-nbctl: [[fd0f::10]]:900: should be an IP address.
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 192.168.10.10], [1], [],
> @@ -1158,7 +1149,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 192.168.10.10], [1], [],
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 192.168.10.10:80], [1], [],
> -[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP [[ae0f::10]]:80.
> +[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP ae0f::10.
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10], [1], [],
> @@ -1166,7 +1157,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10], [1], [],
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 [[ae0f::10]]:80], [1], [],
> -[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP 30.0.0.10:80.
> +[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP 30.0.0.10.
>  ])
>  
>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10])
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index b6843956c..39032d269 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1828,6 +1828,11 @@ ovn-nbctl set Load_Balancer lb8 options:neighbor_responder=all
>  ovn-nbctl lb-add lb9 "[[4444::4444]]:8080" "[[10::10]]:8080" udp
>  ovn-nbctl set Load_Balancer lb9 options:neighbor_responder=all
>  
> +ovn-nbctl lb-add lb10 "55.55.55.55:8080" "10.0.0.8:8080" udp
> +ovn-nbctl set Load_Balancer lb10 options:neighbor_responder=none
> +ovn-nbctl lb-add lb11 "[[5555::5555]]:8080" "[[10::10]]:8080" udp
> +ovn-nbctl set Load_Balancer lb11 options:neighbor_responder=none
> +
>  ovn-nbctl lr-lb-add lr lb1
>  ovn-nbctl lr-lb-add lr lb2
>  ovn-nbctl lr-lb-add lr lb3
> @@ -1837,6 +1842,8 @@ ovn-nbctl lr-lb-add lr lb6
>  ovn-nbctl lr-lb-add lr lb7
>  ovn-nbctl lr-lb-add lr lb8
>  ovn-nbctl lr-lb-add lr lb9
> +ovn-nbctl lr-lb-add lr lb10
> +ovn-nbctl lr-lb-add lr lb11
>  
>  ovn-nbctl --wait=sb sync
>  lr_key=$(fetch_column sb:datapath_binding tunnel_key external_ids:name=lr)
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 811468dc6..4feae1db2 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -28,6 +28,7 @@
>  #include "openvswitch/json.h"
>  #include "lib/acl-log.h"
>  #include "lib/copp.h"
> +#include "lib/lb.h"
>  #include "lib/ovn-nb-idl.h"
>  #include "lib/ovn-util.h"
>  #include "memory.h"
> @@ -2837,6 +2838,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>      bool empty_backend_rej = shash_find(&ctx->options, "--reject") != NULL;
>      bool empty_backend_event = shash_find(&ctx->options, "--event") != NULL;
>      bool add_route = shash_find(&ctx->options, "--add-route") != NULL;
> +    bool template = shash_find(&ctx->options, "--template") != NULL;
>  
>      if (empty_backend_event && empty_backend_rej) {
>              ctl_error(ctx,
> @@ -2844,10 +2846,12 @@ nbctl_lb_add(struct ctl_context *ctx)
>              return;
>      }
>  
> +    const char *lb_address_family_str = "ipv4";
> +    int lb_address_family = AF_INET;
>      const char *lb_proto;
>      bool is_update_proto = false;
>  
> -    if (ctx->argc == 4) {
> +    if (ctx->argc <= 4) {
>          /* Default protocol. */
>          lb_proto = "tcp";
>      } else {
> @@ -2863,79 +2867,61 @@ nbctl_lb_add(struct ctl_context *ctx)
>          }
>      }
>  
> -    struct sockaddr_storage ss_vip;
> -    if (!inet_parse_active(lb_vip, 0, &ss_vip, false, NULL)) {
> -        ctl_error(ctx, "%s: should be an IP address (or an IP address "
> -                  "and a port number with : as a separator).", lb_vip);
> -        return;
> -    }
> -
> -    struct ds lb_vip_normalized_ds = DS_EMPTY_INITIALIZER;
> -    uint16_t lb_vip_port = ss_get_port(&ss_vip);
> -    if (lb_vip_port) {
> -        ss_format_address(&ss_vip, &lb_vip_normalized_ds);
> -        ds_put_format(&lb_vip_normalized_ds, ":%d", lb_vip_port);
> -    } else {
> -        ss_format_address_nobracks(&ss_vip, &lb_vip_normalized_ds);
> -    }
> -    const char *lb_vip_normalized = ds_cstr(&lb_vip_normalized_ds);
> +    if (ctx->argc > 5) {
> +        lb_address_family_str = ctx->argv[5];
> +        lb_address_family = !strcmp(lb_address_family_str, "ipv4")
> +                            ? AF_INET : AF_INET6;
>  
> -    if (!lb_vip_port && is_update_proto) {
> -        ds_destroy(&lb_vip_normalized_ds);
> -        ctl_error(ctx, "Protocol is unnecessary when no port of vip "
> -                  "is given.");
> -        return;
>      }
>  
> -    char *token = NULL, *save_ptr = NULL;
> +    struct ds lb_vip_normalized = DS_EMPTY_INITIALIZER;
>      struct ds lb_ips_new = DS_EMPTY_INITIALIZER;
> -    for (token = strtok_r(lb_ips, ",", &save_ptr);
> -            token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
> -        struct sockaddr_storage ss_dst;
> +    struct ovn_lb_vip lb_vip_parsed;
>  
> -        if (lb_vip_port) {
> -            if (!inet_parse_active(token, -1, &ss_dst, false, NULL)) {
> -                ctl_error(ctx, "%s: should be an IP address and a port "
> -                          "number with : as a separator.", token);
> -                goto out;
> -            }
> -        } else {
> -            if (!inet_parse_address(token, &ss_dst)) {
> -                ctl_error(ctx, "%s: should be an IP address.", token);
> -                goto out;
> -            }
> -        }
> +    char *error = ovn_lb_vip_init(&lb_vip_parsed, lb_vip, lb_ips, template,
> +                                  lb_address_family);
> +    if (error) {
> +        ctl_error(ctx, "%s", error);
> +        ovn_lb_vip_destroy(&lb_vip_parsed);
> +        free(error);
> +        return;
> +    }
>  
> -        if (ss_vip.ss_family != ss_dst.ss_family) {
> -            ctl_error(ctx, "%s: IP address family is different from VIP %s.",
> -                      token, lb_vip_normalized);
> -            goto out;
> -        }
> -        ds_put_format(&lb_ips_new, "%s%s",
> -                lb_ips_new.length ? "," : "", token);
> +    if (is_update_proto && !lb_vip_parsed.port_str) {
> +        ctl_error(ctx, "Protocol is unnecessary when no port of vip is "
> +                       "given.");
> +        ovn_lb_vip_destroy(&lb_vip_parsed);
> +        return;
>      }
>  
> +    ovn_lb_vip_format(&lb_vip_parsed, &lb_vip_normalized, template);
> +    ovn_lb_vip_backends_format(&lb_vip_parsed, &lb_ips_new, template);
> +    ovn_lb_vip_destroy(&lb_vip_parsed);
> +
>      const struct nbrec_load_balancer *lb = NULL;
>      if (!add_duplicate) {
> -        char *error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
> +        error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
>          if (error) {
>              ctx->error = error;
>              goto out;
>          }
>          if (lb) {
> -            if (smap_get(&lb->vips, lb_vip_normalized)) {
> +            if (smap_get(&lb->vips, ds_cstr(&lb_vip_normalized))) {
>                  if (!may_exist) {
>                      ctl_error(ctx, "%s: a load balancer with this vip (%s) "
> -                              "already exists", lb_name, lb_vip_normalized);
> +                              "already exists", lb_name,
> +                              ds_cstr(&lb_vip_normalized));
>                      goto out;
>                  }
>                  /* Update the vips. */
>                  smap_replace(CONST_CAST(struct smap *, &lb->vips),
> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
> +                             ds_cstr(&lb_vip_normalized),
> +                             ds_cstr(&lb_ips_new));
>              } else {
>                  /* Add the new vips. */
>                  smap_add(CONST_CAST(struct smap *, &lb->vips),
> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
> +                         ds_cstr(&lb_vip_normalized),
> +                         ds_cstr(&lb_ips_new));
>              }
>  
>              /* Update the load balancer. */
> @@ -2954,7 +2940,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>      nbrec_load_balancer_set_name(lb, lb_name);
>      nbrec_load_balancer_set_protocol(lb, lb_proto);
>      smap_add(CONST_CAST(struct smap *, &lb->vips),
> -            lb_vip_normalized, ds_cstr(&lb_ips_new));
> +             ds_cstr(&lb_vip_normalized), ds_cstr(&lb_ips_new));
>      nbrec_load_balancer_set_vips(lb, &lb->vips);
>      struct smap options = SMAP_INITIALIZER(&options);
>      if (empty_backend_rej) {
> @@ -2966,12 +2952,16 @@ nbctl_lb_add(struct ctl_context *ctx)
>      if (add_route) {
>          smap_add(&options, "add_route", "true");
>      }
> +    if (template) {
> +        smap_add(&options, "template", "true");
> +        smap_add(&options, "address-family", lb_address_family_str);
> +    }
>      nbrec_load_balancer_set_options(lb, &options);
>      smap_destroy(&options);
>  out:
>      ds_destroy(&lb_ips_new);
>  
> -    ds_destroy(&lb_vip_normalized_ds);
> +    ds_destroy(&lb_vip_normalized);
>  }
>  
>  static void
> @@ -3025,6 +3015,7 @@ static void
>  nbctl_pre_lb_list(struct ctl_context *ctx)
>  {
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>  }
> @@ -3033,6 +3024,7 @@ static void
>  lb_info_add_smap(const struct nbrec_load_balancer *lb,
>                   struct smap *lbs, int vip_width)
>  {
> +    bool template = smap_get_bool(&lb->options, "template", false);
>      const struct smap_node **nodes = smap_sort(&lb->vips);
>      if (!nodes) {
>          return;
> @@ -3041,13 +3033,24 @@ lb_info_add_smap(const struct nbrec_load_balancer *lb,
>      struct ds val = DS_EMPTY_INITIALIZER;
>      for (size_t i = 0; i < smap_count(&lb->vips); i++) {
>          const struct smap_node *node = nodes[i];
> +        const char *protocol = lb->protocol;
>  
> -        struct sockaddr_storage ss;
> -        if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
> -            continue;
> +        if (!template) {
> +            struct sockaddr_storage ss;
> +            if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
> +                continue;
> +            }
> +            protocol = ss_get_port(&ss) ? lb->protocol : "";
> +        } else {
> +            if (!lb->protocol) {
> +                VLOG_WARN("Load Balancer "UUID_FMT" (%s) is a template and "
> +                          "misses protocol", UUID_ARGS(&lb->header_.uuid),
> +                          lb->name);
> +                continue;
> +            }
> +            protocol = lb->protocol;
>          }
>  
> -        char *protocol = ss_get_port(&ss) ? lb->protocol : "";
>          if (i == 0) {
>              ds_put_format(&val, UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
>                            UUID_ARGS(&lb->header_.uuid),
> @@ -3239,6 +3242,7 @@ nbctl_pre_lr_lb_list(struct ctl_context *ctx)
>                           &nbrec_logical_router_col_load_balancer_group);
>  
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>  
> @@ -3402,6 +3406,7 @@ nbctl_pre_ls_lb_list(struct ctl_context *ctx)
>                           &nbrec_logical_switch_col_load_balancer_group);
>  
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>  
> @@ -7469,9 +7474,10 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>        nbctl_pre_lr_nat_set_ext_ips, nbctl_lr_nat_set_ext_ips,
>        NULL, "--is-exempted", RW},
>      /* load balancer commands. */
> -    { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]",
> +    { "lb-add", 3, 5, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL] [ADDRESS_FAMILY]",
>        nbctl_pre_lb_add, nbctl_lb_add, NULL,
> -      "--may-exist,--add-duplicate,--reject,--event,--add-route", RW },
> +      "--may-exist,--add-duplicate,--reject,--event,--add-route,--template",
> +      RW },
>      { "lb-del", 1, 2, "LB [VIP]", nbctl_pre_lb_del, nbctl_lb_del, NULL,
>          "--if-exists", RW },
>      { "lb-list", 0, 1, "[LB]", nbctl_pre_lb_list, nbctl_lb_list, NULL, "", RO },
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> Allow the CMS to configure template LBs.  The following configurations are
> supported:
> - VIPs of the form: ^vip_variable[:^port_variable|:port]
> - Backends of the form:
>
^backendip_variable1[:^port_variable1|:port],^backendip_variable2[:^port_variable2|:port]
>   OR
>   ^backends_variable1,^backends_variable2
>
> The CMS needs to provide a bit more information than with non-template
load
> balancers and must explicitly specify the address family to be used.
>
> There is currently no support for template load balancers with
> options:add_route=true set.  That is because ovn-northd does not
> instantiate template variables.  While this is a limitation in a way, its
> impact is not huge.  The load balancer 'add_route' option was added as a
> way to make the CMS life easier and to avoid having to explicitly add a
> route for the VIP.  The CMS can still achieve the same logical topology by
> explicitly adding the VIP route.
>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Hi Dumitru, sorry for the delayed response. Please see my comments below.

> ---
>  controller/lflow.c    |   18 +-
>  lib/lb.c              |  463
++++++++++++++++++++++++++++++++++++++++++-------
>  lib/lb.h              |   39 +++-
>  lib/ovn-util.c        |    3
>  northd/northd.c       |   85 +++++----
>  ovn-nb.xml            |   12 +
>  tests/ovn-nbctl.at    |   23 +-
>  tests/ovn-northd.at   |    7 +
>  utilities/ovn-nbctl.c |  122 +++++++------
>  9 files changed, 576 insertions(+), 196 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index ad58fcd3c..ffc8fdb6e 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1993,7 +1993,9 @@ add_lb_ct_snat_hairpin_flows(struct
ovn_controller_lb *lb,
>
>  static void
>  consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
> -                          const struct hmap *local_datapaths, bool
use_ct_mark,
> +                          const struct hmap *local_datapaths,
> +                          const struct smap *template_vars,

template_vars is added as an input, but I don't see I-P for template_vars
for hairpin flows in this patch, which makes this patch incomplete.
I see that it is taken care by the next patch. So they should be either
combined or switch the patch order, so that each incremental patch is
correct.
In addition, some basic tests may be added to this patch (which should
discover problems of hairpin flows when template vars are updated, due to
the missing I-P).

> +                          bool use_ct_mark,
>                            struct ovn_desired_flow_table *flow_table,
>                            struct simap *ids)
>  {
> @@ -2029,7 +2031,8 @@ consider_lb_hairpin_flows(const struct
sbrec_load_balancer *sbrec_lb,
>          return;
>      }
>
> -    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb);
> +    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb,
> +
 template_vars);
>      uint8_t lb_proto = IPPROTO_TCP;
>      if (lb->slb->protocol && lb->slb->protocol[0]) {
>          if (!strcmp(lb->slb->protocol, "udp")) {
> @@ -2059,7 +2062,9 @@ consider_lb_hairpin_flows(const struct
sbrec_load_balancer *sbrec_lb,
>   * backends to handle the load balanced hairpin traffic. */
>  static void
>  add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table,
> -                     const struct hmap *local_datapaths, bool
use_ct_mark,
> +                     const struct hmap *local_datapaths,
> +                     const struct smap *template_vars,
> +                     bool use_ct_mark,
>                       struct ovn_desired_flow_table *flow_table,
>                       struct simap *ids,
>                       struct id_pool *pool)
> @@ -2082,8 +2087,8 @@ add_lb_hairpin_flows(const struct
sbrec_load_balancer_table *lb_table,
>              ovs_assert(id_pool_alloc_id(pool, &id));
>              simap_put(ids, lb->name, id);
>          }
> -        consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark,
flow_table,
> -                                  ids);
> +        consider_lb_hairpin_flows(lb, local_datapaths, template_vars,
> +                                  use_ct_mark, flow_table, ids);
>      }
>  }
>
> @@ -2220,6 +2225,7 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
lflow_ctx_out *l_ctx_out)
>                         l_ctx_in->local_datapaths,
>                         l_ctx_out->flow_table);
>      add_lb_hairpin_flows(l_ctx_in->lb_table, l_ctx_in->local_datapaths,
> +                         l_ctx_in->template_vars,
>                           l_ctx_in->lb_hairpin_use_ct_mark,
>                           l_ctx_out->flow_table,
>                           l_ctx_out->hairpin_lb_ids,
> @@ -2351,6 +2357,7 @@ lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *dp,
>       * associated. */
>      for (size_t i = 0; i < n_dp_lbs; i++) {
>          consider_lb_hairpin_flows(dp_lbs[i], l_ctx_in->local_datapaths,
> +                                  l_ctx_in->template_vars,
>                                    l_ctx_in->lb_hairpin_use_ct_mark,
>                                    l_ctx_out->flow_table,
>                                    l_ctx_out->hairpin_lb_ids);
> @@ -2493,6 +2500,7 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
*l_ctx_in,
>          VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT,
>                   UUID_ARGS(&lb->header_.uuid));
>          consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> +                                  l_ctx_in->template_vars,
>                                    l_ctx_in->lb_hairpin_use_ct_mark,
>                                    l_ctx_out->flow_table,
>                                    l_ctx_out->hairpin_lb_ids);
> diff --git a/lib/lb.c b/lib/lb.c
> index 77674ea28..15cbf29ee 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -19,6 +19,7 @@
>  #include "lib/ovn-nb-idl.h"
>  #include "lib/ovn-sb-idl.h"
>  #include "lib/ovn-util.h"
> +#include "ovn/lex.h"
>
>  /* OpenvSwitch lib includes. */
>  #include "openvswitch/vlog.h"
> @@ -26,6 +27,16 @@
>
>  VLOG_DEFINE_THIS_MODULE(lb);
>
> +static const char *lb_neighbor_responder_mode_names[] = {
> +    [LB_NEIGH_RESPOND_REACHABLE] = "reachable",
> +    [LB_NEIGH_RESPOND_ALL] = "all",
> +    [LB_NEIGH_RESPOND_NONE] = "none",
> +};
> +
> +static struct nbrec_load_balancer_health_check *
> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
> +                        const char *vip_port_str, bool template);
> +
>  struct ovn_lb_ip_set *
>  ovn_lb_ip_set_create(void)
>  {
> @@ -71,108 +82,312 @@ ovn_lb_ip_set_clone(struct ovn_lb_ip_set *lb_ip_set)
>      return clone;
>  }
>
> -static
> -bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> -                     const char *lb_value)
> +/* Format for backend ips: "IP1:port1,IP2:port2,...". */
> +static char *
> +ovn_lb_backends_init_explicit(struct ovn_lb_vip *lb_vip, const char
*value,
> +                              size_t *n_backends)
>  {
> -    int addr_family;
> -
> -    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
> -                                         &lb_vip->vip_port,
&addr_family)) {
> -        return false;
> -    }
> -
> -    if (addr_family == AF_INET) {
> -        ovs_be32 vip4;
> -        ip_parse(lb_vip->vip_str, &vip4);
> -        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
> -    } else {
> -        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
> -    }
> -
> -    /* Format for backend ips: "IP1:port1,IP2:port2,...". */
> -    size_t n_backends = 0;
> +    struct ds errors = DS_EMPTY_INITIALIZER;
>      size_t n_allocated_backends = 0;
> -    char *tokstr = xstrdup(lb_value);
> +    char *tokstr = xstrdup(value);
>      char *save_ptr = NULL;
> +    *n_backends = 0;
> +
>      for (char *token = strtok_r(tokstr, ",", &save_ptr);
>          token != NULL;
>          token = strtok_r(NULL, ",", &save_ptr)) {
>
> -        if (n_backends == n_allocated_backends) {
> +        if (*n_backends == n_allocated_backends) {
>              lb_vip->backends = x2nrealloc(lb_vip->backends,
>                                            &n_allocated_backends,
>                                            sizeof *lb_vip->backends);
>          }
>
> -        struct ovn_lb_backend *backend = &lb_vip->backends[n_backends];
> +        struct ovn_lb_backend *backend =
&lb_vip->backends[(*n_backends)];
>          int backend_addr_family;
>          if (!ip_address_and_port_from_lb_key(token, &backend->ip_str,
>                                               &backend->port,
>                                               &backend_addr_family)) {
> +            if (lb_vip->port_str) {
> +                ds_put_format(&errors, "%s: should be an IP address and
a "
> +                                       "port number with : as a
separator, ",
> +                              token);
> +            } else {
> +                ds_put_format(&errors, "%s: should be an IP address, ",
token);
> +            }
>              continue;
>          }
>
> -        if (addr_family != backend_addr_family) {
> +        if (lb_vip->address_family != backend_addr_family) {
>              free(backend->ip_str);
> +            ds_put_format(&errors, "%s: IP address family is different
from "
> +                                   "VIP %s, ",
> +                          token, lb_vip->vip_str);
>              continue;
>          }
>
> -        if (addr_family == AF_INET) {
> +        if (lb_vip->port_str) {
> +            if (!backend->port) {
> +                free(backend->ip_str);
> +                ds_put_format(&errors, "%s: should be an IP address and "
> +                                       "a port number with : as a
separator, ",
> +                              token);
> +                continue;
> +            }
> +        } else {
> +            if (backend->port) {
> +                free(backend->ip_str);
> +                ds_put_format(&errors, "%s: should be an IP address, ",
token);
> +                continue;
> +            }
> +        }
> +
> +        backend->port_str =
> +            backend->port ? xasprintf("%"PRIu16, backend->port) : NULL;
> +
> +        if (lb_vip->address_family == AF_INET) {
>              ovs_be32 ip4;
>              ip_parse(backend->ip_str, &ip4);
>              in6_addr_set_mapped_ipv4(&backend->ip, ip4);
>          } else {
>              ipv6_parse(backend->ip_str, &backend->ip);
>          }
> -        n_backends++;
> +        (*n_backends)++;
>      }
>      free(tokstr);
> -    lb_vip->n_backends = n_backends;
> -    return true;
> +
> +    if (ds_last(&errors) != EOF) {
> +        ds_chomp(&errors, ' ');
> +        ds_chomp(&errors, ',');
> +        ds_put_char(&errors, '.');
> +        return ds_steal_cstr(&errors);
> +    }
> +    return NULL;
>  }
>
>  static
> -void ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
> +char *ovn_lb_vip_init_explicit(struct ovn_lb_vip *lb_vip, const char
*lb_key,
> +                               const char *lb_value)
> +{
> +    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
> +                                         &lb_vip->vip_port,
> +                                         &lb_vip->address_family)) {
> +        return xasprintf("%s: should be an IP address (or an IP address "
> +                         "and a port number with : as a separator).",
lb_key);
> +    }
> +
> +    lb_vip->port_str = lb_vip->vip_port
> +                       ? xasprintf("%"PRIu16, lb_vip->vip_port)
> +                       : NULL;
> +
> +    if (lb_vip->address_family == AF_INET) {
> +        ovs_be32 vip4;
> +        ip_parse(lb_vip->vip_str, &vip4);
> +        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
> +    } else {
> +        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
> +    }
> +    return ovn_lb_backends_init_explicit(lb_vip, lb_value,
> +                                         &lb_vip->n_backends);
> +}
> +
> +/* Parses backends of a templated LB VIP.
> + * For now only the following template forms are supported:
> + * A.
> + *   ^backendip_variable1[:^port_variable1|:port],
> + *   ^backendip_variable2[:^port_variable2|:port]
> + *
> + * B.
> + *   ^backends_variable1,^backends_variable2 is also a thing
> + *      where 'backends_variable1' may expand to IP1_1:PORT1_1 on
chassis-1
> + *                                               IP1_2:PORT1_2 on
chassis-2
> + *        and 'backends_variable2' may expand to IP2_1:PORT2_1 on
chassis-1
> + *                                               IP2_2:PORT2_2 on
chassis-2
> + */
> +static char *
> +ovn_lb_backends_init_template(struct ovn_lb_vip *lb_vip, const char
*value_,
> +                              size_t *n_backends)
> +{
> +    struct ds errors = DS_EMPTY_INITIALIZER;
> +    char *value = xstrdup(value_);
> +    char *save_ptr = NULL;
> +    size_t n_allocated_backends = 0;
> +    *n_backends = 0;
> +
> +    for (char *backend = strtok_r(value, ",", &save_ptr); backend;
> +         backend = strtok_r(NULL, ",", &save_ptr)) {
> +
> +        char *atom = xstrdup(backend);
> +        char *save_ptr2 = NULL;
> +        bool success = false;
> +        char *backend_ip = NULL;
> +        char *backend_port = NULL;
> +
> +        for (char *subatom = strtok_r(atom, ":", &save_ptr2); subatom;
> +             subatom = strtok_r(NULL, ":", &save_ptr2)) {
> +            if (backend_ip && backend_port) {
> +                success = false;
> +                break;
> +            }
> +            success = true;
> +            if (!backend_ip) {
> +                backend_ip = xstrdup(subatom);
> +            } else {
> +                backend_port = xstrdup(subatom);
> +            }
> +        }
> +
> +        if (success) {
> +            if (*n_backends == n_allocated_backends) {
> +                lb_vip->backends = x2nrealloc(lb_vip->backends,
> +                                              &n_allocated_backends,
> +                                              sizeof *lb_vip->backends);
> +            }
> +
> +            struct ovn_lb_backend *lb_backend =
> +                &lb_vip->backends[(*n_backends)];
> +            lb_backend->ip_str = backend_ip;
> +            lb_backend->port_str = backend_port;
> +            lb_backend->port = 0;
> +            (*n_backends)++;
> +        } else {
> +            ds_put_format(&errors, "%s: should be a template of the
form: "
> +
 "'^backendip_variable1[:^port_variable1|:port]', ",
> +                          atom);
> +        }
> +        free(atom);
> +    }
> +
> +    free(value);
> +    if (ds_last(&errors) != EOF) {
> +        ds_chomp(&errors, ' ');
> +        ds_chomp(&errors, ',');
> +        ds_put_char(&errors, '.');
> +        return ds_steal_cstr(&errors);
> +    }
> +    return NULL;
> +}
> +
> +/* Parses a VIP of a templated LB.
> + * For now only the following template forms are supported:
> + *   ^vip_variable[:^port_variable|:port]
> + */
> +static char *
> +ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip, const char *lb_key_,
> +                         const char *lb_value, int address_family)
> +{
> +    char *save_ptr = NULL;
> +    char *lb_key = xstrdup(lb_key_);
> +    bool success = false;
> +
> +    for (char *atom = strtok_r(lb_key, ":", &save_ptr); atom;
> +         atom = strtok_r(NULL, ":", &save_ptr)) {
> +        if (lb_vip->vip_str && lb_vip->port_str) {
> +            success = false;
> +            break;
> +        }
> +        success = true;
> +        if (!lb_vip->vip_str) {
> +            lb_vip->vip_str = xstrdup(atom);
> +        } else {
> +            lb_vip->port_str = xstrdup(atom);
> +        }
> +    }
> +    free(lb_key);
> +
> +    if (!success) {
> +        return xasprintf("%s: should be a template of the form: "
> +                         "'^vip_variable[:^port_variable|:port]'.",
> +                         lb_key_);
> +    }
> +
> +    lb_vip->address_family = address_family;
> +    return ovn_lb_backends_init_template(lb_vip, lb_value,
> +                                         &lb_vip->n_backends);
> +}
> +
> +/* Returns NULL on success, an error string on failure.  The caller is
> + * responsible for destroying 'lb_vip' in all cases.
> + */
> +char *
> +ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> +                const char *lb_value, bool template, int address_family)
> +{
> +    memset(lb_vip, 0, sizeof *lb_vip);
> +
> +    return !template
> +           ?  ovn_lb_vip_init_explicit(lb_vip, lb_key, lb_value)
> +           :  ovn_lb_vip_init_template(lb_vip, lb_key, lb_value,
> +                                       address_family);
> +}
> +
> +void
> +ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
>  {
>      free(vip->vip_str);
> +    free(vip->port_str);
>      for (size_t i = 0; i < vip->n_backends; i++) {
>          free(vip->backends[i].ip_str);
> +        free(vip->backends[i].port_str);
>      }
>      free(vip->backends);
>  }
>
> +void
> +ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s, bool
template)
> +{
> +    bool needs_brackets = vip->address_family == AF_INET6 &&
vip->port_str
> +                          && !template;
> +    if (needs_brackets) {
> +        ds_put_char(s, '[');
> +    }
> +    ds_put_cstr(s, vip->vip_str);
> +    if (needs_brackets) {
> +        ds_put_char(s, ']');
> +    }
> +    if (vip->port_str) {
> +        ds_put_format(s, ":%s", vip->port_str);
> +    }
> +}
> +
> +void
> +ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s,
> +                           bool template)
> +{
> +    bool needs_brackets = vip->address_family == AF_INET6 &&
vip->port_str
> +                          && !template;
> +    for (size_t i = 0; i < vip->n_backends; i++) {
> +        struct ovn_lb_backend *backend = &vip->backends[i];
> +
> +        if (needs_brackets) {
> +            ds_put_char(s, '[');
> +        }
> +        ds_put_cstr(s, backend->ip_str);
> +        if (needs_brackets) {
> +            ds_put_char(s, ']');
> +        }
> +        if (backend->port_str) {
> +            ds_put_format(s, ":%s", backend->port_str);
> +        }
> +        if (i != vip->n_backends - 1) {
> +            ds_put_char(s, ',');
> +        }
> +    }
> +}
> +
>  static
>  void ovn_northd_lb_vip_init(struct ovn_northd_lb_vip *lb_vip_nb,
>                              const struct ovn_lb_vip *lb_vip,
>                              const struct nbrec_load_balancer *nbrec_lb,
> -                            const char *vip_port_str, const char
*backend_ips)
> +                            const char *vip_port_str, const char
*backend_ips,
> +                            bool template)
>  {
>      lb_vip_nb->backend_ips = xstrdup(backend_ips);
>      lb_vip_nb->n_backends = lb_vip->n_backends;
>      lb_vip_nb->backends_nb = xcalloc(lb_vip_nb->n_backends,
>                                       sizeof *lb_vip_nb->backends_nb);
> -
> -    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, vip_port_str)) {
> -                lb_health_check = nbrec_lb->health_check[j];
> -                break;
> -            }
> -        }
> -    }
> -
> -    lb_vip_nb->lb_health_check = lb_health_check;
> +    lb_vip_nb->lb_health_check =
> +        ovn_lb_get_health_check(nbrec_lb, vip_port_str, template);
>  }
>
>  static
> @@ -203,12 +418,111 @@ ovn_lb_get_hairpin_snat_ip(const struct uuid
*lb_uuid,
>      }
>  }
>
> +static bool
> +ovn_lb_get_routable_mode(const struct nbrec_load_balancer *nbrec_lb,
> +                         bool routable, bool template)
> +{
> +    if (template && routable) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +        VLOG_WARN_RL(&rl, "Template load balancer "UUID_FMT" does not
suport "
> +                           "option 'add_route'.  Forcing it to
disabled.",
> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
> +        return false;
> +    }
> +    return routable;
> +}
> +
> +static bool
> +ovn_lb_neigh_mode_is_valid(enum lb_neighbor_responder_mode mode, bool
template)
> +{
> +    if (!template) {
> +        return true;
> +    }
> +
> +    switch (mode) {
> +    case LB_NEIGH_RESPOND_REACHABLE:
> +        return false;
> +    case LB_NEIGH_RESPOND_ALL:
> +    case LB_NEIGH_RESPOND_NONE:
> +        return true;
> +    }
> +}
> +
> +static enum lb_neighbor_responder_mode
> +ovn_lb_get_neigh_mode(const struct nbrec_load_balancer *nbrec_lb,
> +                      const char *mode, bool template)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +    enum lb_neighbor_responder_mode default_mode =
> +        template ? LB_NEIGH_RESPOND_NONE : LB_NEIGH_RESPOND_REACHABLE;
> +
> +    if (!mode) {
> +        mode = lb_neighbor_responder_mode_names[default_mode];
> +    }
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(lb_neighbor_responder_mode_names);
i++) {
> +        if (!strcmp(mode, lb_neighbor_responder_mode_names[i])) {
> +            if (ovn_lb_neigh_mode_is_valid(i, template)) {
> +                return i;
> +            }
> +            break;
> +        }
> +    }
> +
> +    VLOG_WARN_RL(&rl, "Invalid neighbor responder mode %s for load
balancer "
> +                       UUID_FMT", forcing it to %s",
> +                 mode, UUID_ARGS(&nbrec_lb->header_.uuid),
> +                 lb_neighbor_responder_mode_names[default_mode]);
> +    return default_mode;
> +}

This new limitation of the neighbor_responder mode needs to be documented
in ovn-nb.xml for the options:neighbor_responder.

Also curious why can't we support responders for templated LBs? Can the
relevant lflows be generated with the template vars used for the LB VIPs as
well?

> +
> +static struct nbrec_load_balancer_health_check *
> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
> +                        const char *vip_port_str, bool template)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +
> +    if (!nbrec_lb->n_health_check) {
> +        return NULL;
> +    }
> +
> +    if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) {
> +        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));
> +        return NULL;
> +    }
> +
> +    if (template) {
> +        VLOG_WARN_RL(&rl,
> +                     "Template load balancers do not currently support "
> +                     "health checks. Not creating health checks for "
> +                     "load balancer " UUID_FMT,
> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
> +        return NULL;
> +    }
> +
> +    for (size_t i = 0; i < nbrec_lb->n_health_check; i++) {
> +        if (!strcmp(nbrec_lb->health_check[i]->vip, vip_port_str)) {
> +            return nbrec_lb->health_check[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
>  struct ovn_northd_lb *
>  ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>  {
>      bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
>      bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol, "sctp");
>      struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> +    int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> +                                              "address-family", "ipv4"),
> +                                 "ipv4")
> +                         ? AF_INET
> +                         : AF_INET6;
>
>      lb->nlb = nbrec_lb;
>      lb->proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
> @@ -216,12 +530,16 @@ ovn_northd_lb_create(const struct
nbrec_load_balancer *nbrec_lb)
>      lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips);
>      lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb);
>      lb->controller_event = smap_get_bool(&nbrec_lb->options, "event",
false);
> -    lb->routable = smap_get_bool(&nbrec_lb->options, "add_route", false);
> +
> +    bool routable = smap_get_bool(&nbrec_lb->options, "add_route",
false);
> +    lb->routable = ovn_lb_get_routable_mode(nbrec_lb, routable,
lb->template);
> +
>      lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat",
false);
> -    const char *mode =
> -        smap_get_def(&nbrec_lb->options, "neighbor_responder",
"reachable");
> -    lb->neigh_mode = strcmp(mode, "all") ? LB_NEIGH_RESPOND_REACHABLE
> -                                         : LB_NEIGH_RESPOND_ALL;
> +    lb->template = smap_get_bool(&nbrec_lb->options, "template", false);
> +
> +    const char *mode = smap_get(&nbrec_lb->options,
"neighbor_responder");
> +    lb->neigh_mode = ovn_lb_get_neigh_mode(nbrec_lb, mode, lb->template);
> +
>      sset_init(&lb->ips_v4);
>      sset_init(&lb->ips_v6);
>      struct smap_node *node;
> @@ -231,13 +549,19 @@ ovn_northd_lb_create(const struct
nbrec_load_balancer *nbrec_lb)
>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[n_vips];
>
> -        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> -                                                  "reject", false);
> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
> +        char *error = ovn_lb_vip_init(lb_vip, node->key, node->value,
> +                                      lb->template, address_family);
> +        if (error) {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
> +            VLOG_WARN_RL(&rl, "Failed to initialize LB VIP: %s", error);
> +            ovn_lb_vip_destroy(lb_vip);
> +            free(error);
>              continue;
>          }
> +        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> +                                                  "reject", false);
>          ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> -                               node->key, node->value);
> +                               node->key, node->value, lb->template);
>          if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>              sset_add(&lb->ips_v4, lb_vip->vip_str);
>          } else {
> @@ -385,7 +709,8 @@ ovn_lb_group_find(const struct hmap *lb_groups, const
struct uuid *uuid)
>  }
>
>  struct ovn_controller_lb *
> -ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb)
> +ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb,
> +                         const struct smap *template_vars)
>  {
>      struct ovn_controller_lb *lb = xzalloc(sizeof *lb);
>
> @@ -399,10 +724,20 @@ ovn_controller_lb_create(const struct
sbrec_load_balancer *sbrec_lb)
>      SMAP_FOR_EACH (node, &sbrec_lb->vips) {
>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>
> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
> -            continue;
> +        char *key_s = lexer_parse_template_string(xstrdup(node->key),
> +                                                  template_vars,
> +                                                  NULL);
> +        char *value_s = lexer_parse_template_string(xstrdup(node->value),
> +                                                    template_vars,
> +                                                    NULL);

Shouldn't we check the options:use_template before parsing?

> +        char *error = ovn_lb_vip_init_explicit(lb_vip, key_s, value_s);
> +        if (error) {
> +            free(error);
> +        } else {
> +            n_vips++;
>          }
> -        n_vips++;
> +        free(key_s);
> +        free(value_s);
>      }
>
>      /* It's possible that parsing VIPs fails.  Update the lb->n_vips to
the
> diff --git a/lib/lb.h b/lib/lb.h
> index 80ac03399..265a1215a 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -35,6 +35,7 @@ struct uuid;
>  enum lb_neighbor_responder_mode {
>      LB_NEIGH_RESPOND_REACHABLE,
>      LB_NEIGH_RESPOND_ALL,
> +    LB_NEIGH_RESPOND_NONE,
>  };
>
>  /* The "routable" ssets are subsets of the load balancer IPs for which IP
> @@ -67,6 +68,7 @@ struct ovn_northd_lb {
>      bool controller_event;
>      bool routable;
>      bool skip_snat;
> +    bool template;
>
>      struct sset ips_v4;
>      struct sset ips_v6;
> @@ -81,19 +83,31 @@ struct ovn_northd_lb {
>  };
>
>  struct ovn_lb_vip {
> -    struct in6_addr vip;
> -    char *vip_str;
> -    uint16_t vip_port;
> -
> +    struct in6_addr vip; /* Only used in ovn-controller. */
> +    char *vip_str;       /* Actual VIP string representation (without
port).
> +                          * To be used in ovn-northd.
> +                          */
> +    uint16_t vip_port;   /* Only used in ovn-controller. */
> +    char *port_str;      /* Actual port string representation.  To be
used
> +                          * in ovn-controller.
> +                          */
>      struct ovn_lb_backend *backends;
>      size_t n_backends;
>      bool empty_backend_rej;
> +    int address_family;
>  };
>
>  struct ovn_lb_backend {
> -    struct in6_addr ip;
> -    char *ip_str;
> -    uint16_t port;
> +    struct in6_addr ip;  /* Only used in ovn-controller. */
> +    char *ip_str;        /* Actual IP string representation. To be used
in
> +                          * ovn-northd.
> +                          */
> +    uint16_t port;       /* Mostly used in ovn-controller but also for
> +                          * healthcheck in ovn-northd.
> +                          */
> +    char *port_str;      /* Actual port string representation. To be used
> +                          * in ovn-northd.
> +                          */
>  };
>
>  /* ovn-northd specific backend information. */
> @@ -171,7 +185,16 @@ struct ovn_controller_lb {
>  };
>
>  struct ovn_controller_lb *ovn_controller_lb_create(
> -    const struct sbrec_load_balancer *);
> +    const struct sbrec_load_balancer *,
> +    const struct smap *template_vars);
>  void ovn_controller_lb_destroy(struct ovn_controller_lb *);
>
> +char *ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> +                      const char *lb_value, bool template, int
address_family);
> +void ovn_lb_vip_destroy(struct ovn_lb_vip *vip);
> +void ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s,
> +                       bool template);
> +void ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds
*s,
> +                                bool template);
> +
>  #endif /* OVN_LIB_LB_H 1 */
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 596e93129..1cc4c98b1 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -799,9 +799,6 @@ ip_address_and_port_from_lb_key(const char *key, char
**ip_address,
>  {
>      struct sockaddr_storage ss;
>      if (!inet_parse_active(key, 0, &ss, false, NULL)) {
> -        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;
> diff --git a/northd/northd.c b/northd/northd.c
> index 63879ccd0..a93aee37e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3708,6 +3708,10 @@ static void
>  ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, struct ovn_northd_lb
*lb,
>                    struct hmap *monitor_map, struct hmap *ports)
>  {
> +    if (lb->template) {
> +        return;
> +    }
> +
>      for (size_t i = 0; i < lb->n_vips; i++) {
>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
> @@ -4024,12 +4028,19 @@ static void
>  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
>                                 const struct ovn_northd_lb *lb)
>  {
> +    /* If configured to not reply to any neighbor requests for all VIPs
> +     * return early.
> +     */
> +    if (lb->neigh_mode == LB_NEIGH_RESPOND_NONE) {
> +        return;
> +    }
> +
>      /* If configured to reply to neighbor requests for all VIPs force
them
>       * all to be considered "reachable".
>       */
>      if (lb->neigh_mode == LB_NEIGH_RESPOND_ALL) {
>          for (size_t i = 0; i < lb->n_vips; i++) {
> -            if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
> +            if (lb->vips[i].address_family == AF_INET) {
>                  sset_add(&od->lb_ips->ips_v4_reachable,
lb->vips[i].vip_str);
>              } else {
>                  sset_add(&od->lb_ips->ips_v6_reachable,
lb->vips[i].vip_str);
> @@ -4041,8 +4052,9 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath
*od,
>      /* Otherwise, a VIP is reachable if there's at least one router
>       * subnet that includes it.
>       */
> +    ovs_assert(lb->neigh_mode == LB_NEIGH_RESPOND_REACHABLE);
>      for (size_t i = 0; i < lb->n_vips; i++) {
> -        if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
> +        if (lb->vips[i].address_family == AF_INET) {
>              ovs_be32 vip_ip4 =
in6_addr_get_mapped_ipv4(&lb->vips[i].vip);
>              struct ovn_port *op;
>
> @@ -5823,16 +5835,16 @@ build_empty_lb_event_flow(struct ovn_lb_vip
*lb_vip,
>      ds_clear(action);
>      ds_clear(match);
>
> -    bool ipv4 = IN6_IS_ADDR_V4MAPPED(&lb_vip->vip);
> +    bool ipv4 = lb_vip->address_family == AF_INET;
>
>      ds_put_format(match, "ip%s.dst == %s && %s",
>                    ipv4 ? "4": "6", lb_vip->vip_str, lb->proto);
>
>      char *vip = lb_vip->vip_str;
> -    if (lb_vip->vip_port) {
> -        ds_put_format(match, " && %s.dst == %u", lb->proto,
lb_vip->vip_port);
> -        vip = xasprintf("%s%s%s:%u", ipv4 ? "" : "[", lb_vip->vip_str,
> -                        ipv4 ? "" : "]", lb_vip->vip_port);
> +    if (lb_vip->port_str) {
> +        ds_put_format(match, " && %s.dst == %s", lb->proto,
lb_vip->port_str);
> +        vip = xasprintf("%s%s%s:%s", ipv4 ? "" : "[", lb_vip->vip_str,
> +                        ipv4 ? "" : "]", lb_vip->port_str);
>      }
>
>      ds_put_format(action,
> @@ -5843,7 +5855,7 @@ build_empty_lb_event_flow(struct ovn_lb_vip *lb_vip,
>                    event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS),
>                    vip, lb->proto,
>                    UUID_ARGS(&lb->nlb->header_.uuid));
> -    if (lb_vip->vip_port) {
> +    if (lb_vip->port_str) {
>          free(vip);
>      }
>      return true;
> @@ -6899,7 +6911,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
struct ovn_northd_lb *lb,
>          /* Store the original destination IP to be used when generating
>           * hairpin flows.
>           */
> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +        if (lb->vips[i].address_family == AF_INET) {
>              ip_match = "ip4";
>              ds_put_format(action, REG_ORIG_DIP_IPV4 " = %s; ",
>                            lb_vip->vip_str);
> @@ -6910,7 +6922,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
struct ovn_northd_lb *lb,
>          }
>
>          const char *proto = NULL;
> -        if (lb_vip->vip_port) {
> +        if (lb_vip->port_str) {
>              proto = "tcp";
>              if (lb->nlb->protocol) {
>                  if (!strcmp(lb->nlb->protocol, "udp")) {
> @@ -6923,8 +6935,8 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
struct ovn_northd_lb *lb,
>              /* Store the original destination port to be used when
generating
>               * hairpin flows.
>               */
> -            ds_put_format(action, REG_ORIG_TP_DPORT " = %"PRIu16"; ",
> -                          lb_vip->vip_port);
> +            ds_put_format(action, REG_ORIG_TP_DPORT " = %s; ",
> +                          lb_vip->port_str);
>          }
>          ds_put_format(action, "%s;", ct_lb_mark ? "ct_lb_mark" :
"ct_lb");
>
> @@ -6962,24 +6974,12 @@ build_lb_rules(struct hmap *lflows, struct
ovn_northd_lb *lb, bool ct_lb_mark,
>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
>          const char *ip_match = NULL;
> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +        if (lb_vip->address_family == AF_INET) {
>              ip_match = "ip4";
>          } else {
>              ip_match = "ip6";
>          }
>
> -        const char *proto = NULL;
> -        if (lb_vip->vip_port) {
> -            proto = "tcp";
> -            if (lb->nlb->protocol) {
> -                if (!strcmp(lb->nlb->protocol, "udp")) {
> -                    proto = "udp";
> -                } else if (!strcmp(lb->nlb->protocol, "sctp")) {
> -                    proto = "sctp";
> -                }
> -            }
> -        }
> -
>          ds_clear(action);
>          ds_clear(match);
>
> @@ -6997,8 +6997,9 @@ build_lb_rules(struct hmap *lflows, struct
ovn_northd_lb *lb, bool ct_lb_mark,
>          ds_put_format(match, "ct.new && %s.dst == %s", ip_match,
>                        lb_vip->vip_str);
>          int priority = 110;
> -        if (lb_vip->vip_port) {
> -            ds_put_format(match, " && %s.dst == %d", proto,
lb_vip->vip_port);
> +        if (lb_vip->port_str) {
> +            ds_put_format(match, " && %s.dst == %s", lb->proto,
> +                          lb_vip->port_str);
>              priority = 120;
>          }
>
> @@ -9998,7 +9999,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
*lb_vip,
>       * of "ct_lb_mark($targets);". The other flow is for ct.est with
>       * an action of "next;".
>       */
> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +    if (lb_vip->address_family == AF_INET) {
>          ds_put_format(match, "ip4 && "REG_NEXT_HOP_IPV4" == %s",
>                        lb_vip->vip_str);
>      } else {
> @@ -10014,14 +10015,14 @@ build_lrouter_nat_flows_for_lb(struct
ovn_lb_vip *lb_vip,
>      }
>
>      int prio = 110;
> -    if (lb_vip->vip_port) {
> +    if (lb_vip->port_str) {
>          prio = 120;
>          new_match = xasprintf("ct.new && %s && %s && "
> -                              REG_ORIG_TP_DPORT_ROUTER" == %d",
> -                              ds_cstr(match), lb->proto,
lb_vip->vip_port);
> +                              REG_ORIG_TP_DPORT_ROUTER" == %s",
> +                              ds_cstr(match), lb->proto,
lb_vip->port_str);
>          est_match = xasprintf("ct.est && %s && %s && "
> -                              REG_ORIG_TP_DPORT_ROUTER" == %d && %s ==
1",
> -                              ds_cstr(match), lb->proto,
lb_vip->vip_port,
> +                              REG_ORIG_TP_DPORT_ROUTER" == %s && %s ==
1",
> +                              ds_cstr(match), lb->proto,
lb_vip->port_str,
>                                ct_natted);
>      } else {
>          new_match = xasprintf("ct.new && %s", ds_cstr(match));
> @@ -10030,7 +10031,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
*lb_vip,
>      }
>
>      const char *ip_match = NULL;
> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +    if (lb_vip->address_family == AF_INET) {
>          ip_match = "ip4";
>      } else {
>          ip_match = "ip6";
> @@ -10048,9 +10049,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
*lb_vip,
>          ds_put_format(&undnat_match, "(%s.src == %s", ip_match,
>                        backend->ip_str);
>
> -        if (backend->port) {
> -            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> -                          lb->proto, backend->port);
> +        if (backend->port_str) {
> +            ds_put_format(&undnat_match, " && %s.src == %s) || ",
> +                          lb->proto, backend->port_str);
>          } else {
>              ds_put_cstr(&undnat_match, ") || ");
>          }
> @@ -10063,9 +10064,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
*lb_vip,
>      struct ds unsnat_match = DS_EMPTY_INITIALIZER;
>      ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
>                    ip_match, ip_match, lb_vip->vip_str, lb->proto);
> -    if (lb_vip->vip_port) {
> -        ds_put_format(&unsnat_match, " && %s.dst == %d", lb->proto,
> -                      lb_vip->vip_port);
> +    if (lb_vip->port_str) {
> +        ds_put_format(&unsnat_match, " && %s.dst == %s", lb->proto,
> +                      lb_vip->port_str);
>      }
>
>      struct ovn_datapath **gw_router_skip_snat =
> @@ -10305,7 +10306,7 @@ build_lrouter_defrag_flows_for_lb(struct
ovn_northd_lb *lb,
>          ds_clear(&defrag_actions);
>          ds_clear(match);
>
> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> +        if (lb_vip->address_family == AF_INET) {
>              ds_put_format(match, "ip && ip4.dst == %s", lb_vip->vip_str);
>              ds_put_format(&defrag_actions, REG_NEXT_HOP_IPV4" = %s; ",
>                            lb_vip->vip_str);
> @@ -10315,7 +10316,7 @@ build_lrouter_defrag_flows_for_lb(struct
ovn_northd_lb *lb,
>                            lb_vip->vip_str);
>          }
>
> -        if (lb_vip->vip_port) {
> +        if (lb_vip->port_str) {
>              ds_put_format(match, " && %s", lb->proto);
>              prio = 110;
>
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index bd28ac372..fb63b212e 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1908,6 +1908,18 @@
>          requests only for VIPs that are part of a router's subnet.  The
default
>          value of this option, if not specified, is
<code>reachable</code>.
>        </column>
> +
> +      <column name="options" key="template">
> +        Option to be set to <code>true</code>, if the load balancer is a
> +        template.  In this case the template variables used within the
> +        load balancer VIPs and backends will be instantiated differently
> +        on different chassis.

It would be better to mention what are template variables, with a reference
to the chassis_template_var table. An simple example would be even better.

> +      </column>
> +
> +      <column name="options" key="address-family">
> +        Address family used by the load balancer.  This value is used
only
> +        when the load balancer is a template.

It would be better to state the keywords "ipv4" & "ipv6", and point out
that this is *required* if and only if the options:template is "true".

Thanks,
Han

> +      </column>
>      </group>
>    </table>
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 6a5d52a5d..9b766d63e 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -857,23 +857,19 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
30.0.0.10 192.168.10.10:a80], [
>  [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
>  ])
>
> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:],
[1], [],
> -[ovn-nbctl: 192.168.10.10:: should be an IP address.
> -])
> -
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10
192.168.10.1a], [1], [],
>  [ovn-nbctl: 192.168.10.1a: should be an IP address.
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,
192.168.10.20:80 tcp], [1], [],
> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +[ovn-nbctl: 192.168.10.10:80: should be an IP address, 192.168.10.20:80:
should be an IP address.
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>  ])
>
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:900 tcp], [1], [],
> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>  ])
>
> @@ -1111,7 +1107,7 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
ae0f::10fff [[fd0f::10]]:80,fd0
>
>
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
[[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> -[ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80:
should be an IP address.
>  ])
>
>
> @@ -1125,18 +1121,13 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
ae0f::10 [[fd0f::10]]:a80], [1]
>  ])
>
>
> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
[[fd0f::10]]:], [1], [],
> -[ovn-nbctl: [[fd0f::10]]:: should be an IP address.
> -])
> -
> -
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a],
[1], [],
>  [ovn-nbctl: fd0f::1001a: should be an IP address.
>  ])
>
>
>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:
[[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80:
should be an IP address.
>  ])
>
>
> @@ -1146,7 +1137,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10
tcp], [1], [],
>
>
>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:900 tcp], [1], [],
> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +[ovn-nbctl: [[fd0f::10]]:900: should be an IP address.
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 192.168.10.10], [1], [],
> @@ -1158,7 +1149,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10
192.168.10.10], [1], [],
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 192.168.10.10:80], [1],
[],
> -[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP
[[ae0f::10]]:80.
> +[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP
ae0f::10.
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10], [1], [],
> @@ -1166,7 +1157,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10],
[1], [],
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 [[ae0f::10]]:80], [1], [],
> -[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP
30.0.0.10:80.
> +[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP
30.0.0.10.
>  ])
>
>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10])
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index b6843956c..39032d269 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1828,6 +1828,11 @@ ovn-nbctl set Load_Balancer lb8
options:neighbor_responder=all
>  ovn-nbctl lb-add lb9 "[[4444::4444]]:8080" "[[10::10]]:8080" udp
>  ovn-nbctl set Load_Balancer lb9 options:neighbor_responder=all
>
> +ovn-nbctl lb-add lb10 "55.55.55.55:8080" "10.0.0.8:8080" udp
> +ovn-nbctl set Load_Balancer lb10 options:neighbor_responder=none
> +ovn-nbctl lb-add lb11 "[[5555::5555]]:8080" "[[10::10]]:8080" udp
> +ovn-nbctl set Load_Balancer lb11 options:neighbor_responder=none
> +
>  ovn-nbctl lr-lb-add lr lb1
>  ovn-nbctl lr-lb-add lr lb2
>  ovn-nbctl lr-lb-add lr lb3
> @@ -1837,6 +1842,8 @@ ovn-nbctl lr-lb-add lr lb6
>  ovn-nbctl lr-lb-add lr lb7
>  ovn-nbctl lr-lb-add lr lb8
>  ovn-nbctl lr-lb-add lr lb9
> +ovn-nbctl lr-lb-add lr lb10
> +ovn-nbctl lr-lb-add lr lb11
>
>  ovn-nbctl --wait=sb sync
>  lr_key=$(fetch_column sb:datapath_binding tunnel_key
external_ids:name=lr)
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 811468dc6..4feae1db2 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -28,6 +28,7 @@
>  #include "openvswitch/json.h"
>  #include "lib/acl-log.h"
>  #include "lib/copp.h"
> +#include "lib/lb.h"
>  #include "lib/ovn-nb-idl.h"
>  #include "lib/ovn-util.h"
>  #include "memory.h"
> @@ -2837,6 +2838,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>      bool empty_backend_rej = shash_find(&ctx->options, "--reject") !=
NULL;
>      bool empty_backend_event = shash_find(&ctx->options, "--event") !=
NULL;
>      bool add_route = shash_find(&ctx->options, "--add-route") != NULL;
> +    bool template = shash_find(&ctx->options, "--template") != NULL;
>
>      if (empty_backend_event && empty_backend_rej) {
>              ctl_error(ctx,
> @@ -2844,10 +2846,12 @@ nbctl_lb_add(struct ctl_context *ctx)
>              return;
>      }
>
> +    const char *lb_address_family_str = "ipv4";
> +    int lb_address_family = AF_INET;
>      const char *lb_proto;
>      bool is_update_proto = false;
>
> -    if (ctx->argc == 4) {
> +    if (ctx->argc <= 4) {
>          /* Default protocol. */
>          lb_proto = "tcp";
>      } else {
> @@ -2863,79 +2867,61 @@ nbctl_lb_add(struct ctl_context *ctx)
>          }
>      }
>
> -    struct sockaddr_storage ss_vip;
> -    if (!inet_parse_active(lb_vip, 0, &ss_vip, false, NULL)) {
> -        ctl_error(ctx, "%s: should be an IP address (or an IP address "
> -                  "and a port number with : as a separator).", lb_vip);
> -        return;
> -    }
> -
> -    struct ds lb_vip_normalized_ds = DS_EMPTY_INITIALIZER;
> -    uint16_t lb_vip_port = ss_get_port(&ss_vip);
> -    if (lb_vip_port) {
> -        ss_format_address(&ss_vip, &lb_vip_normalized_ds);
> -        ds_put_format(&lb_vip_normalized_ds, ":%d", lb_vip_port);
> -    } else {
> -        ss_format_address_nobracks(&ss_vip, &lb_vip_normalized_ds);
> -    }
> -    const char *lb_vip_normalized = ds_cstr(&lb_vip_normalized_ds);
> +    if (ctx->argc > 5) {
> +        lb_address_family_str = ctx->argv[5];
> +        lb_address_family = !strcmp(lb_address_family_str, "ipv4")
> +                            ? AF_INET : AF_INET6;
>
> -    if (!lb_vip_port && is_update_proto) {
> -        ds_destroy(&lb_vip_normalized_ds);
> -        ctl_error(ctx, "Protocol is unnecessary when no port of vip "
> -                  "is given.");
> -        return;
>      }
>
> -    char *token = NULL, *save_ptr = NULL;
> +    struct ds lb_vip_normalized = DS_EMPTY_INITIALIZER;
>      struct ds lb_ips_new = DS_EMPTY_INITIALIZER;
> -    for (token = strtok_r(lb_ips, ",", &save_ptr);
> -            token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
> -        struct sockaddr_storage ss_dst;
> +    struct ovn_lb_vip lb_vip_parsed;
>
> -        if (lb_vip_port) {
> -            if (!inet_parse_active(token, -1, &ss_dst, false, NULL)) {
> -                ctl_error(ctx, "%s: should be an IP address and a port "
> -                          "number with : as a separator.", token);
> -                goto out;
> -            }
> -        } else {
> -            if (!inet_parse_address(token, &ss_dst)) {
> -                ctl_error(ctx, "%s: should be an IP address.", token);
> -                goto out;
> -            }
> -        }
> +    char *error = ovn_lb_vip_init(&lb_vip_parsed, lb_vip, lb_ips,
template,
> +                                  lb_address_family);
> +    if (error) {
> +        ctl_error(ctx, "%s", error);
> +        ovn_lb_vip_destroy(&lb_vip_parsed);
> +        free(error);
> +        return;
> +    }
>
> -        if (ss_vip.ss_family != ss_dst.ss_family) {
> -            ctl_error(ctx, "%s: IP address family is different from VIP
%s.",
> -                      token, lb_vip_normalized);
> -            goto out;
> -        }
> -        ds_put_format(&lb_ips_new, "%s%s",
> -                lb_ips_new.length ? "," : "", token);
> +    if (is_update_proto && !lb_vip_parsed.port_str) {
> +        ctl_error(ctx, "Protocol is unnecessary when no port of vip is "
> +                       "given.");
> +        ovn_lb_vip_destroy(&lb_vip_parsed);
> +        return;
>      }
>
> +    ovn_lb_vip_format(&lb_vip_parsed, &lb_vip_normalized, template);
> +    ovn_lb_vip_backends_format(&lb_vip_parsed, &lb_ips_new, template);
> +    ovn_lb_vip_destroy(&lb_vip_parsed);
> +
>      const struct nbrec_load_balancer *lb = NULL;
>      if (!add_duplicate) {
> -        char *error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
> +        error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
>          if (error) {
>              ctx->error = error;
>              goto out;
>          }
>          if (lb) {
> -            if (smap_get(&lb->vips, lb_vip_normalized)) {
> +            if (smap_get(&lb->vips, ds_cstr(&lb_vip_normalized))) {
>                  if (!may_exist) {
>                      ctl_error(ctx, "%s: a load balancer with this vip
(%s) "
> -                              "already exists", lb_name,
lb_vip_normalized);
> +                              "already exists", lb_name,
> +                              ds_cstr(&lb_vip_normalized));
>                      goto out;
>                  }
>                  /* Update the vips. */
>                  smap_replace(CONST_CAST(struct smap *, &lb->vips),
> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
> +                             ds_cstr(&lb_vip_normalized),
> +                             ds_cstr(&lb_ips_new));
>              } else {
>                  /* Add the new vips. */
>                  smap_add(CONST_CAST(struct smap *, &lb->vips),
> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
> +                         ds_cstr(&lb_vip_normalized),
> +                         ds_cstr(&lb_ips_new));
>              }
>
>              /* Update the load balancer. */
> @@ -2954,7 +2940,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>      nbrec_load_balancer_set_name(lb, lb_name);
>      nbrec_load_balancer_set_protocol(lb, lb_proto);
>      smap_add(CONST_CAST(struct smap *, &lb->vips),
> -            lb_vip_normalized, ds_cstr(&lb_ips_new));
> +             ds_cstr(&lb_vip_normalized), ds_cstr(&lb_ips_new));
>      nbrec_load_balancer_set_vips(lb, &lb->vips);
>      struct smap options = SMAP_INITIALIZER(&options);
>      if (empty_backend_rej) {
> @@ -2966,12 +2952,16 @@ nbctl_lb_add(struct ctl_context *ctx)
>      if (add_route) {
>          smap_add(&options, "add_route", "true");
>      }
> +    if (template) {
> +        smap_add(&options, "template", "true");
> +        smap_add(&options, "address-family", lb_address_family_str);
> +    }
>      nbrec_load_balancer_set_options(lb, &options);
>      smap_destroy(&options);
>  out:
>      ds_destroy(&lb_ips_new);
>
> -    ds_destroy(&lb_vip_normalized_ds);
> +    ds_destroy(&lb_vip_normalized);
>  }
>
>  static void
> @@ -3025,6 +3015,7 @@ static void
>  nbctl_pre_lb_list(struct ctl_context *ctx)
>  {
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>  }
> @@ -3033,6 +3024,7 @@ static void
>  lb_info_add_smap(const struct nbrec_load_balancer *lb,
>                   struct smap *lbs, int vip_width)
>  {
> +    bool template = smap_get_bool(&lb->options, "template", false);
>      const struct smap_node **nodes = smap_sort(&lb->vips);
>      if (!nodes) {
>          return;
> @@ -3041,13 +3033,24 @@ lb_info_add_smap(const struct nbrec_load_balancer
*lb,
>      struct ds val = DS_EMPTY_INITIALIZER;
>      for (size_t i = 0; i < smap_count(&lb->vips); i++) {
>          const struct smap_node *node = nodes[i];
> +        const char *protocol = lb->protocol;
>
> -        struct sockaddr_storage ss;
> -        if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
> -            continue;
> +        if (!template) {
> +            struct sockaddr_storage ss;
> +            if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
> +                continue;
> +            }
> +            protocol = ss_get_port(&ss) ? lb->protocol : "";
> +        } else {
> +            if (!lb->protocol) {
> +                VLOG_WARN("Load Balancer "UUID_FMT" (%s) is a template
and "
> +                          "misses protocol",
UUID_ARGS(&lb->header_.uuid),
> +                          lb->name);
> +                continue;
> +            }
> +            protocol = lb->protocol;
>          }
>
> -        char *protocol = ss_get_port(&ss) ? lb->protocol : "";
>          if (i == 0) {
>              ds_put_format(&val, UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
>                            UUID_ARGS(&lb->header_.uuid),
> @@ -3239,6 +3242,7 @@ nbctl_pre_lr_lb_list(struct ctl_context *ctx)
>                           &nbrec_logical_router_col_load_balancer_group);
>
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>
> @@ -3402,6 +3406,7 @@ nbctl_pre_ls_lb_list(struct ctl_context *ctx)
>                           &nbrec_logical_switch_col_load_balancer_group);
>
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>
> @@ -7469,9 +7474,10 @@ static const struct ctl_command_syntax
nbctl_commands[] = {
>        nbctl_pre_lr_nat_set_ext_ips, nbctl_lr_nat_set_ext_ips,
>        NULL, "--is-exempted", RW},
>      /* load balancer commands. */
> -    { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]",
> +    { "lb-add", 3, 5, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]
[ADDRESS_FAMILY]",
>        nbctl_pre_lb_add, nbctl_lb_add, NULL,
> -      "--may-exist,--add-duplicate,--reject,--event,--add-route", RW },
> +
 "--may-exist,--add-duplicate,--reject,--event,--add-route,--template",
> +      RW },
>      { "lb-del", 1, 2, "LB [VIP]", nbctl_pre_lb_del, nbctl_lb_del, NULL,
>          "--if-exists", RW },
>      { "lb-list", 0, 1, "[LB]", nbctl_pre_lb_list, nbctl_lb_list, NULL,
"", RO },
>

On 10/21/22 07:47, Han Zhou wrote:
> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> Allow the CMS to configure template LBs.  The following configurations are
>> supported:
>> - VIPs of the form: ^vip_variable[:^port_variable|:port]
>> - Backends of the form:
>>
> ^backendip_variable1[:^port_variable1|:port],^backendip_variable2[:^port_variable2|:port]
>>   OR
>>   ^backends_variable1,^backends_variable2
>>
>> The CMS needs to provide a bit more information than with non-template
> load
>> balancers and must explicitly specify the address family to be used.
>>
>> There is currently no support for template load balancers with
>> options:add_route=true set.  That is because ovn-northd does not
>> instantiate template variables.  While this is a limitation in a way, its
>> impact is not huge.  The load balancer 'add_route' option was added as a
>> way to make the CMS life easier and to avoid having to explicitly add a
>> route for the VIP.  The CMS can still achieve the same logical topology by
>> explicitly adding the VIP route.
>>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> 
> Hi Dumitru, sorry for the delayed response. Please see my comments below.
> 

Hi Han,

No worries, thanks for the review!

>> ---
>>  controller/lflow.c    |   18 +-
>>  lib/lb.c              |  463
> ++++++++++++++++++++++++++++++++++++++++++-------
>>  lib/lb.h              |   39 +++-
>>  lib/ovn-util.c        |    3
>>  northd/northd.c       |   85 +++++----
>>  ovn-nb.xml            |   12 +
>>  tests/ovn-nbctl.at    |   23 +-
>>  tests/ovn-northd.at   |    7 +
>>  utilities/ovn-nbctl.c |  122 +++++++------
>>  9 files changed, 576 insertions(+), 196 deletions(-)
>>
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index ad58fcd3c..ffc8fdb6e 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -1993,7 +1993,9 @@ add_lb_ct_snat_hairpin_flows(struct
> ovn_controller_lb *lb,
>>
>>  static void
>>  consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
>> -                          const struct hmap *local_datapaths, bool
> use_ct_mark,
>> +                          const struct hmap *local_datapaths,
>> +                          const struct smap *template_vars,
> 
> template_vars is added as an input, but I don't see I-P for template_vars
> for hairpin flows in this patch, which makes this patch incomplete.
> I see that it is taken care by the next patch. So they should be either
> combined or switch the patch order, so that each incremental patch is
> correct.
> In addition, some basic tests may be added to this patch (which should
> discover problems of hairpin flows when template vars are updated, due to
> the missing I-P).
> 

I initially wanted to keep things simpler but you're right, now at patch
5/8 we have an incomplete implementation.  I'll squash the two patches
together and add more tests.

>> +                          bool use_ct_mark,
>>                            struct ovn_desired_flow_table *flow_table,
>>                            struct simap *ids)
>>  {
>> @@ -2029,7 +2031,8 @@ consider_lb_hairpin_flows(const struct
> sbrec_load_balancer *sbrec_lb,
>>          return;
>>      }
>>
>> -    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb);
>> +    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb,
>> +
>  template_vars);
>>      uint8_t lb_proto = IPPROTO_TCP;
>>      if (lb->slb->protocol && lb->slb->protocol[0]) {
>>          if (!strcmp(lb->slb->protocol, "udp")) {
>> @@ -2059,7 +2062,9 @@ consider_lb_hairpin_flows(const struct
> sbrec_load_balancer *sbrec_lb,
>>   * backends to handle the load balanced hairpin traffic. */
>>  static void
>>  add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table,
>> -                     const struct hmap *local_datapaths, bool
> use_ct_mark,
>> +                     const struct hmap *local_datapaths,
>> +                     const struct smap *template_vars,
>> +                     bool use_ct_mark,
>>                       struct ovn_desired_flow_table *flow_table,
>>                       struct simap *ids,
>>                       struct id_pool *pool)
>> @@ -2082,8 +2087,8 @@ add_lb_hairpin_flows(const struct
> sbrec_load_balancer_table *lb_table,
>>              ovs_assert(id_pool_alloc_id(pool, &id));
>>              simap_put(ids, lb->name, id);
>>          }
>> -        consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark,
> flow_table,
>> -                                  ids);
>> +        consider_lb_hairpin_flows(lb, local_datapaths, template_vars,
>> +                                  use_ct_mark, flow_table, ids);
>>      }
>>  }
>>
>> @@ -2220,6 +2225,7 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
> lflow_ctx_out *l_ctx_out)
>>                         l_ctx_in->local_datapaths,
>>                         l_ctx_out->flow_table);
>>      add_lb_hairpin_flows(l_ctx_in->lb_table, l_ctx_in->local_datapaths,
>> +                         l_ctx_in->template_vars,
>>                           l_ctx_in->lb_hairpin_use_ct_mark,
>>                           l_ctx_out->flow_table,
>>                           l_ctx_out->hairpin_lb_ids,
>> @@ -2351,6 +2357,7 @@ lflow_add_flows_for_datapath(const struct
> sbrec_datapath_binding *dp,
>>       * associated. */
>>      for (size_t i = 0; i < n_dp_lbs; i++) {
>>          consider_lb_hairpin_flows(dp_lbs[i], l_ctx_in->local_datapaths,
>> +                                  l_ctx_in->template_vars,
>>                                    l_ctx_in->lb_hairpin_use_ct_mark,
>>                                    l_ctx_out->flow_table,
>>                                    l_ctx_out->hairpin_lb_ids);
>> @@ -2493,6 +2500,7 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
> *l_ctx_in,
>>          VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT,
>>                   UUID_ARGS(&lb->header_.uuid));
>>          consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
>> +                                  l_ctx_in->template_vars,
>>                                    l_ctx_in->lb_hairpin_use_ct_mark,
>>                                    l_ctx_out->flow_table,
>>                                    l_ctx_out->hairpin_lb_ids);
>> diff --git a/lib/lb.c b/lib/lb.c
>> index 77674ea28..15cbf29ee 100644
>> --- a/lib/lb.c
>> +++ b/lib/lb.c
>> @@ -19,6 +19,7 @@
>>  #include "lib/ovn-nb-idl.h"
>>  #include "lib/ovn-sb-idl.h"
>>  #include "lib/ovn-util.h"
>> +#include "ovn/lex.h"
>>
>>  /* OpenvSwitch lib includes. */
>>  #include "openvswitch/vlog.h"
>> @@ -26,6 +27,16 @@
>>
>>  VLOG_DEFINE_THIS_MODULE(lb);
>>
>> +static const char *lb_neighbor_responder_mode_names[] = {
>> +    [LB_NEIGH_RESPOND_REACHABLE] = "reachable",
>> +    [LB_NEIGH_RESPOND_ALL] = "all",
>> +    [LB_NEIGH_RESPOND_NONE] = "none",
>> +};
>> +
>> +static struct nbrec_load_balancer_health_check *
>> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
>> +                        const char *vip_port_str, bool template);
>> +
>>  struct ovn_lb_ip_set *
>>  ovn_lb_ip_set_create(void)
>>  {
>> @@ -71,108 +82,312 @@ ovn_lb_ip_set_clone(struct ovn_lb_ip_set *lb_ip_set)
>>      return clone;
>>  }
>>
>> -static
>> -bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
>> -                     const char *lb_value)
>> +/* Format for backend ips: "IP1:port1,IP2:port2,...". */
>> +static char *
>> +ovn_lb_backends_init_explicit(struct ovn_lb_vip *lb_vip, const char
> *value,
>> +                              size_t *n_backends)
>>  {
>> -    int addr_family;
>> -
>> -    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
>> -                                         &lb_vip->vip_port,
> &addr_family)) {
>> -        return false;
>> -    }
>> -
>> -    if (addr_family == AF_INET) {
>> -        ovs_be32 vip4;
>> -        ip_parse(lb_vip->vip_str, &vip4);
>> -        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
>> -    } else {
>> -        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
>> -    }
>> -
>> -    /* Format for backend ips: "IP1:port1,IP2:port2,...". */
>> -    size_t n_backends = 0;
>> +    struct ds errors = DS_EMPTY_INITIALIZER;
>>      size_t n_allocated_backends = 0;
>> -    char *tokstr = xstrdup(lb_value);
>> +    char *tokstr = xstrdup(value);
>>      char *save_ptr = NULL;
>> +    *n_backends = 0;
>> +
>>      for (char *token = strtok_r(tokstr, ",", &save_ptr);
>>          token != NULL;
>>          token = strtok_r(NULL, ",", &save_ptr)) {
>>
>> -        if (n_backends == n_allocated_backends) {
>> +        if (*n_backends == n_allocated_backends) {
>>              lb_vip->backends = x2nrealloc(lb_vip->backends,
>>                                            &n_allocated_backends,
>>                                            sizeof *lb_vip->backends);
>>          }
>>
>> -        struct ovn_lb_backend *backend = &lb_vip->backends[n_backends];
>> +        struct ovn_lb_backend *backend =
> &lb_vip->backends[(*n_backends)];
>>          int backend_addr_family;
>>          if (!ip_address_and_port_from_lb_key(token, &backend->ip_str,
>>                                               &backend->port,
>>                                               &backend_addr_family)) {
>> +            if (lb_vip->port_str) {
>> +                ds_put_format(&errors, "%s: should be an IP address and
> a "
>> +                                       "port number with : as a
> separator, ",
>> +                              token);
>> +            } else {
>> +                ds_put_format(&errors, "%s: should be an IP address, ",
> token);
>> +            }
>>              continue;
>>          }
>>
>> -        if (addr_family != backend_addr_family) {
>> +        if (lb_vip->address_family != backend_addr_family) {
>>              free(backend->ip_str);
>> +            ds_put_format(&errors, "%s: IP address family is different
> from "
>> +                                   "VIP %s, ",
>> +                          token, lb_vip->vip_str);
>>              continue;
>>          }
>>
>> -        if (addr_family == AF_INET) {
>> +        if (lb_vip->port_str) {
>> +            if (!backend->port) {
>> +                free(backend->ip_str);
>> +                ds_put_format(&errors, "%s: should be an IP address and "
>> +                                       "a port number with : as a
> separator, ",
>> +                              token);
>> +                continue;
>> +            }
>> +        } else {
>> +            if (backend->port) {
>> +                free(backend->ip_str);
>> +                ds_put_format(&errors, "%s: should be an IP address, ",
> token);
>> +                continue;
>> +            }
>> +        }
>> +
>> +        backend->port_str =
>> +            backend->port ? xasprintf("%"PRIu16, backend->port) : NULL;
>> +
>> +        if (lb_vip->address_family == AF_INET) {
>>              ovs_be32 ip4;
>>              ip_parse(backend->ip_str, &ip4);
>>              in6_addr_set_mapped_ipv4(&backend->ip, ip4);
>>          } else {
>>              ipv6_parse(backend->ip_str, &backend->ip);
>>          }
>> -        n_backends++;
>> +        (*n_backends)++;
>>      }
>>      free(tokstr);
>> -    lb_vip->n_backends = n_backends;
>> -    return true;
>> +
>> +    if (ds_last(&errors) != EOF) {
>> +        ds_chomp(&errors, ' ');
>> +        ds_chomp(&errors, ',');
>> +        ds_put_char(&errors, '.');
>> +        return ds_steal_cstr(&errors);
>> +    }
>> +    return NULL;
>>  }
>>
>>  static
>> -void ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
>> +char *ovn_lb_vip_init_explicit(struct ovn_lb_vip *lb_vip, const char
> *lb_key,
>> +                               const char *lb_value)
>> +{
>> +    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
>> +                                         &lb_vip->vip_port,
>> +                                         &lb_vip->address_family)) {
>> +        return xasprintf("%s: should be an IP address (or an IP address "
>> +                         "and a port number with : as a separator).",
> lb_key);
>> +    }
>> +
>> +    lb_vip->port_str = lb_vip->vip_port
>> +                       ? xasprintf("%"PRIu16, lb_vip->vip_port)
>> +                       : NULL;
>> +
>> +    if (lb_vip->address_family == AF_INET) {
>> +        ovs_be32 vip4;
>> +        ip_parse(lb_vip->vip_str, &vip4);
>> +        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
>> +    } else {
>> +        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
>> +    }
>> +    return ovn_lb_backends_init_explicit(lb_vip, lb_value,
>> +                                         &lb_vip->n_backends);
>> +}
>> +
>> +/* Parses backends of a templated LB VIP.
>> + * For now only the following template forms are supported:
>> + * A.
>> + *   ^backendip_variable1[:^port_variable1|:port],
>> + *   ^backendip_variable2[:^port_variable2|:port]
>> + *
>> + * B.
>> + *   ^backends_variable1,^backends_variable2 is also a thing
>> + *      where 'backends_variable1' may expand to IP1_1:PORT1_1 on
> chassis-1
>> + *                                               IP1_2:PORT1_2 on
> chassis-2
>> + *        and 'backends_variable2' may expand to IP2_1:PORT2_1 on
> chassis-1
>> + *                                               IP2_2:PORT2_2 on
> chassis-2
>> + */
>> +static char *
>> +ovn_lb_backends_init_template(struct ovn_lb_vip *lb_vip, const char
> *value_,
>> +                              size_t *n_backends)
>> +{
>> +    struct ds errors = DS_EMPTY_INITIALIZER;
>> +    char *value = xstrdup(value_);
>> +    char *save_ptr = NULL;
>> +    size_t n_allocated_backends = 0;
>> +    *n_backends = 0;
>> +
>> +    for (char *backend = strtok_r(value, ",", &save_ptr); backend;
>> +         backend = strtok_r(NULL, ",", &save_ptr)) {
>> +
>> +        char *atom = xstrdup(backend);
>> +        char *save_ptr2 = NULL;
>> +        bool success = false;
>> +        char *backend_ip = NULL;
>> +        char *backend_port = NULL;
>> +
>> +        for (char *subatom = strtok_r(atom, ":", &save_ptr2); subatom;
>> +             subatom = strtok_r(NULL, ":", &save_ptr2)) {
>> +            if (backend_ip && backend_port) {
>> +                success = false;
>> +                break;
>> +            }
>> +            success = true;
>> +            if (!backend_ip) {
>> +                backend_ip = xstrdup(subatom);
>> +            } else {
>> +                backend_port = xstrdup(subatom);
>> +            }
>> +        }
>> +
>> +        if (success) {
>> +            if (*n_backends == n_allocated_backends) {
>> +                lb_vip->backends = x2nrealloc(lb_vip->backends,
>> +                                              &n_allocated_backends,
>> +                                              sizeof *lb_vip->backends);
>> +            }
>> +
>> +            struct ovn_lb_backend *lb_backend =
>> +                &lb_vip->backends[(*n_backends)];
>> +            lb_backend->ip_str = backend_ip;
>> +            lb_backend->port_str = backend_port;
>> +            lb_backend->port = 0;
>> +            (*n_backends)++;
>> +        } else {
>> +            ds_put_format(&errors, "%s: should be a template of the
> form: "
>> +
>  "'^backendip_variable1[:^port_variable1|:port]', ",
>> +                          atom);
>> +        }
>> +        free(atom);
>> +    }
>> +
>> +    free(value);
>> +    if (ds_last(&errors) != EOF) {
>> +        ds_chomp(&errors, ' ');
>> +        ds_chomp(&errors, ',');
>> +        ds_put_char(&errors, '.');
>> +        return ds_steal_cstr(&errors);
>> +    }
>> +    return NULL;
>> +}
>> +
>> +/* Parses a VIP of a templated LB.
>> + * For now only the following template forms are supported:
>> + *   ^vip_variable[:^port_variable|:port]
>> + */
>> +static char *
>> +ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip, const char *lb_key_,
>> +                         const char *lb_value, int address_family)
>> +{
>> +    char *save_ptr = NULL;
>> +    char *lb_key = xstrdup(lb_key_);
>> +    bool success = false;
>> +
>> +    for (char *atom = strtok_r(lb_key, ":", &save_ptr); atom;
>> +         atom = strtok_r(NULL, ":", &save_ptr)) {
>> +        if (lb_vip->vip_str && lb_vip->port_str) {
>> +            success = false;
>> +            break;
>> +        }
>> +        success = true;
>> +        if (!lb_vip->vip_str) {
>> +            lb_vip->vip_str = xstrdup(atom);
>> +        } else {
>> +            lb_vip->port_str = xstrdup(atom);
>> +        }
>> +    }
>> +    free(lb_key);
>> +
>> +    if (!success) {
>> +        return xasprintf("%s: should be a template of the form: "
>> +                         "'^vip_variable[:^port_variable|:port]'.",
>> +                         lb_key_);
>> +    }
>> +
>> +    lb_vip->address_family = address_family;
>> +    return ovn_lb_backends_init_template(lb_vip, lb_value,
>> +                                         &lb_vip->n_backends);
>> +}
>> +
>> +/* Returns NULL on success, an error string on failure.  The caller is
>> + * responsible for destroying 'lb_vip' in all cases.
>> + */
>> +char *
>> +ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
>> +                const char *lb_value, bool template, int address_family)
>> +{
>> +    memset(lb_vip, 0, sizeof *lb_vip);
>> +
>> +    return !template
>> +           ?  ovn_lb_vip_init_explicit(lb_vip, lb_key, lb_value)
>> +           :  ovn_lb_vip_init_template(lb_vip, lb_key, lb_value,
>> +                                       address_family);
>> +}
>> +
>> +void
>> +ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
>>  {
>>      free(vip->vip_str);
>> +    free(vip->port_str);
>>      for (size_t i = 0; i < vip->n_backends; i++) {
>>          free(vip->backends[i].ip_str);
>> +        free(vip->backends[i].port_str);
>>      }
>>      free(vip->backends);
>>  }
>>
>> +void
>> +ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s, bool
> template)
>> +{
>> +    bool needs_brackets = vip->address_family == AF_INET6 &&
> vip->port_str
>> +                          && !template;
>> +    if (needs_brackets) {
>> +        ds_put_char(s, '[');
>> +    }
>> +    ds_put_cstr(s, vip->vip_str);
>> +    if (needs_brackets) {
>> +        ds_put_char(s, ']');
>> +    }
>> +    if (vip->port_str) {
>> +        ds_put_format(s, ":%s", vip->port_str);
>> +    }
>> +}
>> +
>> +void
>> +ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s,
>> +                           bool template)
>> +{
>> +    bool needs_brackets = vip->address_family == AF_INET6 &&
> vip->port_str
>> +                          && !template;
>> +    for (size_t i = 0; i < vip->n_backends; i++) {
>> +        struct ovn_lb_backend *backend = &vip->backends[i];
>> +
>> +        if (needs_brackets) {
>> +            ds_put_char(s, '[');
>> +        }
>> +        ds_put_cstr(s, backend->ip_str);
>> +        if (needs_brackets) {
>> +            ds_put_char(s, ']');
>> +        }
>> +        if (backend->port_str) {
>> +            ds_put_format(s, ":%s", backend->port_str);
>> +        }
>> +        if (i != vip->n_backends - 1) {
>> +            ds_put_char(s, ',');
>> +        }
>> +    }
>> +}
>> +
>>  static
>>  void ovn_northd_lb_vip_init(struct ovn_northd_lb_vip *lb_vip_nb,
>>                              const struct ovn_lb_vip *lb_vip,
>>                              const struct nbrec_load_balancer *nbrec_lb,
>> -                            const char *vip_port_str, const char
> *backend_ips)
>> +                            const char *vip_port_str, const char
> *backend_ips,
>> +                            bool template)
>>  {
>>      lb_vip_nb->backend_ips = xstrdup(backend_ips);
>>      lb_vip_nb->n_backends = lb_vip->n_backends;
>>      lb_vip_nb->backends_nb = xcalloc(lb_vip_nb->n_backends,
>>                                       sizeof *lb_vip_nb->backends_nb);
>> -
>> -    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, vip_port_str)) {
>> -                lb_health_check = nbrec_lb->health_check[j];
>> -                break;
>> -            }
>> -        }
>> -    }
>> -
>> -    lb_vip_nb->lb_health_check = lb_health_check;
>> +    lb_vip_nb->lb_health_check =
>> +        ovn_lb_get_health_check(nbrec_lb, vip_port_str, template);
>>  }
>>
>>  static
>> @@ -203,12 +418,111 @@ ovn_lb_get_hairpin_snat_ip(const struct uuid
> *lb_uuid,
>>      }
>>  }
>>
>> +static bool
>> +ovn_lb_get_routable_mode(const struct nbrec_load_balancer *nbrec_lb,
>> +                         bool routable, bool template)
>> +{
>> +    if (template && routable) {
>> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> +        VLOG_WARN_RL(&rl, "Template load balancer "UUID_FMT" does not
> suport "
>> +                           "option 'add_route'.  Forcing it to
> disabled.",
>> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
>> +        return false;
>> +    }
>> +    return routable;
>> +}
>> +
>> +static bool
>> +ovn_lb_neigh_mode_is_valid(enum lb_neighbor_responder_mode mode, bool
> template)
>> +{
>> +    if (!template) {
>> +        return true;
>> +    }
>> +
>> +    switch (mode) {
>> +    case LB_NEIGH_RESPOND_REACHABLE:
>> +        return false;
>> +    case LB_NEIGH_RESPOND_ALL:
>> +    case LB_NEIGH_RESPOND_NONE:
>> +        return true;
>> +    }
>> +}
>> +
>> +static enum lb_neighbor_responder_mode
>> +ovn_lb_get_neigh_mode(const struct nbrec_load_balancer *nbrec_lb,
>> +                      const char *mode, bool template)
>> +{
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> +    enum lb_neighbor_responder_mode default_mode =
>> +        template ? LB_NEIGH_RESPOND_NONE : LB_NEIGH_RESPOND_REACHABLE;
>> +
>> +    if (!mode) {
>> +        mode = lb_neighbor_responder_mode_names[default_mode];
>> +    }
>> +
>> +    for (size_t i = 0; i < ARRAY_SIZE(lb_neighbor_responder_mode_names);
> i++) {
>> +        if (!strcmp(mode, lb_neighbor_responder_mode_names[i])) {
>> +            if (ovn_lb_neigh_mode_is_valid(i, template)) {
>> +                return i;
>> +            }
>> +            break;
>> +        }
>> +    }
>> +
>> +    VLOG_WARN_RL(&rl, "Invalid neighbor responder mode %s for load
> balancer "
>> +                       UUID_FMT", forcing it to %s",
>> +                 mode, UUID_ARGS(&nbrec_lb->header_.uuid),
>> +                 lb_neighbor_responder_mode_names[default_mode]);
>> +    return default_mode;
>> +}
> 
> This new limitation of the neighbor_responder mode needs to be documented
> in ovn-nb.xml for the options:neighbor_responder.
> 

You're right, I'll do that.

> Also curious why can't we support responders for templated LBs? Can the
> relevant lflows be generated with the template vars used for the LB VIPs as
> well?
> 

It seemed quite complex to implement correctly.  That's because in the
"neighbor_responder=reachable" mode we need to reply only to ARPs for
VIPs that are part of router owned subnets.  We currently implement that
in northd, by checking the router port subnets.  But we only have access
to the real VIP value quite late, after template instantiation, in
ovn-controller.

So I decided to not support neighbor_responder=reachable for now.  For
the use case I have in mind (node-port load balancers in ovn-kubernetes)
it doesn't matter because the VIP is the node IP for which we already
have ARP responder flows in place.  For the more generic case, it's up
to the CMS to explicitly request ARP responses (neighbor_responder=all)
or suppress all of them (neighbor_responder=none).

>> +
>> +static struct nbrec_load_balancer_health_check *
>> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
>> +                        const char *vip_port_str, bool template)
>> +{
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> +
>> +    if (!nbrec_lb->n_health_check) {
>> +        return NULL;
>> +    }
>> +
>> +    if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) {
>> +        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));
>> +        return NULL;
>> +    }
>> +
>> +    if (template) {
>> +        VLOG_WARN_RL(&rl,
>> +                     "Template load balancers do not currently support "
>> +                     "health checks. Not creating health checks for "
>> +                     "load balancer " UUID_FMT,
>> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
>> +        return NULL;
>> +    }
>> +
>> +    for (size_t i = 0; i < nbrec_lb->n_health_check; i++) {
>> +        if (!strcmp(nbrec_lb->health_check[i]->vip, vip_port_str)) {
>> +            return nbrec_lb->health_check[i];
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>>  struct ovn_northd_lb *
>>  ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>>  {
>>      bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
>>      bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol, "sctp");
>>      struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
>> +    int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
>> +                                              "address-family", "ipv4"),
>> +                                 "ipv4")
>> +                         ? AF_INET
>> +                         : AF_INET6;
>>
>>      lb->nlb = nbrec_lb;
>>      lb->proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
>> @@ -216,12 +530,16 @@ ovn_northd_lb_create(const struct
> nbrec_load_balancer *nbrec_lb)
>>      lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips);
>>      lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb);
>>      lb->controller_event = smap_get_bool(&nbrec_lb->options, "event",
> false);
>> -    lb->routable = smap_get_bool(&nbrec_lb->options, "add_route", false);
>> +
>> +    bool routable = smap_get_bool(&nbrec_lb->options, "add_route",
> false);
>> +    lb->routable = ovn_lb_get_routable_mode(nbrec_lb, routable,
> lb->template);
>> +
>>      lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat",
> false);
>> -    const char *mode =
>> -        smap_get_def(&nbrec_lb->options, "neighbor_responder",
> "reachable");
>> -    lb->neigh_mode = strcmp(mode, "all") ? LB_NEIGH_RESPOND_REACHABLE
>> -                                         : LB_NEIGH_RESPOND_ALL;
>> +    lb->template = smap_get_bool(&nbrec_lb->options, "template", false);
>> +
>> +    const char *mode = smap_get(&nbrec_lb->options,
> "neighbor_responder");
>> +    lb->neigh_mode = ovn_lb_get_neigh_mode(nbrec_lb, mode, lb->template);
>> +
>>      sset_init(&lb->ips_v4);
>>      sset_init(&lb->ips_v6);
>>      struct smap_node *node;
>> @@ -231,13 +549,19 @@ ovn_northd_lb_create(const struct
> nbrec_load_balancer *nbrec_lb)
>>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[n_vips];
>>
>> -        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
>> -                                                  "reject", false);
>> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
>> +        char *error = ovn_lb_vip_init(lb_vip, node->key, node->value,
>> +                                      lb->template, address_family);
>> +        if (error) {
>> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 1);
>> +            VLOG_WARN_RL(&rl, "Failed to initialize LB VIP: %s", error);
>> +            ovn_lb_vip_destroy(lb_vip);
>> +            free(error);
>>              continue;
>>          }
>> +        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
>> +                                                  "reject", false);
>>          ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
>> -                               node->key, node->value);
>> +                               node->key, node->value, lb->template);
>>          if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>>              sset_add(&lb->ips_v4, lb_vip->vip_str);
>>          } else {
>> @@ -385,7 +709,8 @@ ovn_lb_group_find(const struct hmap *lb_groups, const
> struct uuid *uuid)
>>  }
>>
>>  struct ovn_controller_lb *
>> -ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb)
>> +ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb,
>> +                         const struct smap *template_vars)
>>  {
>>      struct ovn_controller_lb *lb = xzalloc(sizeof *lb);
>>
>> @@ -399,10 +724,20 @@ ovn_controller_lb_create(const struct
> sbrec_load_balancer *sbrec_lb)
>>      SMAP_FOR_EACH (node, &sbrec_lb->vips) {
>>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>>
>> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
>> -            continue;
>> +        char *key_s = lexer_parse_template_string(xstrdup(node->key),
>> +                                                  template_vars,
>> +                                                  NULL);
>> +        char *value_s = lexer_parse_template_string(xstrdup(node->value),
>> +                                                    template_vars,
>> +                                                    NULL);
> 
> Shouldn't we check the options:use_template before parsing?
> 

That's better indeed!

>> +        char *error = ovn_lb_vip_init_explicit(lb_vip, key_s, value_s);
>> +        if (error) {
>> +            free(error);
>> +        } else {
>> +            n_vips++;
>>          }
>> -        n_vips++;
>> +        free(key_s);
>> +        free(value_s);
>>      }
>>
>>      /* It's possible that parsing VIPs fails.  Update the lb->n_vips to
> the
>> diff --git a/lib/lb.h b/lib/lb.h
>> index 80ac03399..265a1215a 100644
>> --- a/lib/lb.h
>> +++ b/lib/lb.h
>> @@ -35,6 +35,7 @@ struct uuid;
>>  enum lb_neighbor_responder_mode {
>>      LB_NEIGH_RESPOND_REACHABLE,
>>      LB_NEIGH_RESPOND_ALL,
>> +    LB_NEIGH_RESPOND_NONE,
>>  };
>>
>>  /* The "routable" ssets are subsets of the load balancer IPs for which IP
>> @@ -67,6 +68,7 @@ struct ovn_northd_lb {
>>      bool controller_event;
>>      bool routable;
>>      bool skip_snat;
>> +    bool template;
>>
>>      struct sset ips_v4;
>>      struct sset ips_v6;
>> @@ -81,19 +83,31 @@ struct ovn_northd_lb {
>>  };
>>
>>  struct ovn_lb_vip {
>> -    struct in6_addr vip;
>> -    char *vip_str;
>> -    uint16_t vip_port;
>> -
>> +    struct in6_addr vip; /* Only used in ovn-controller. */
>> +    char *vip_str;       /* Actual VIP string representation (without
> port).
>> +                          * To be used in ovn-northd.
>> +                          */
>> +    uint16_t vip_port;   /* Only used in ovn-controller. */
>> +    char *port_str;      /* Actual port string representation.  To be
> used
>> +                          * in ovn-controller.
>> +                          */
>>      struct ovn_lb_backend *backends;
>>      size_t n_backends;
>>      bool empty_backend_rej;
>> +    int address_family;
>>  };
>>
>>  struct ovn_lb_backend {
>> -    struct in6_addr ip;
>> -    char *ip_str;
>> -    uint16_t port;
>> +    struct in6_addr ip;  /* Only used in ovn-controller. */
>> +    char *ip_str;        /* Actual IP string representation. To be used
> in
>> +                          * ovn-northd.
>> +                          */
>> +    uint16_t port;       /* Mostly used in ovn-controller but also for
>> +                          * healthcheck in ovn-northd.
>> +                          */
>> +    char *port_str;      /* Actual port string representation. To be used
>> +                          * in ovn-northd.
>> +                          */
>>  };
>>
>>  /* ovn-northd specific backend information. */
>> @@ -171,7 +185,16 @@ struct ovn_controller_lb {
>>  };
>>
>>  struct ovn_controller_lb *ovn_controller_lb_create(
>> -    const struct sbrec_load_balancer *);
>> +    const struct sbrec_load_balancer *,
>> +    const struct smap *template_vars);
>>  void ovn_controller_lb_destroy(struct ovn_controller_lb *);
>>
>> +char *ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
>> +                      const char *lb_value, bool template, int
> address_family);
>> +void ovn_lb_vip_destroy(struct ovn_lb_vip *vip);
>> +void ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s,
>> +                       bool template);
>> +void ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds
> *s,
>> +                                bool template);
>> +
>>  #endif /* OVN_LIB_LB_H 1 */
>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
>> index 596e93129..1cc4c98b1 100644
>> --- a/lib/ovn-util.c
>> +++ b/lib/ovn-util.c
>> @@ -799,9 +799,6 @@ ip_address_and_port_from_lb_key(const char *key, char
> **ip_address,
>>  {
>>      struct sockaddr_storage ss;
>>      if (!inet_parse_active(key, 0, &ss, false, NULL)) {
>> -        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;
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 63879ccd0..a93aee37e 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -3708,6 +3708,10 @@ static void
>>  ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, struct ovn_northd_lb
> *lb,
>>                    struct hmap *monitor_map, struct hmap *ports)
>>  {
>> +    if (lb->template) {
>> +        return;
>> +    }
>> +
>>      for (size_t i = 0; i < lb->n_vips; i++) {
>>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
>>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
>> @@ -4024,12 +4028,19 @@ static void
>>  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
>>                                 const struct ovn_northd_lb *lb)
>>  {
>> +    /* If configured to not reply to any neighbor requests for all VIPs
>> +     * return early.
>> +     */
>> +    if (lb->neigh_mode == LB_NEIGH_RESPOND_NONE) {
>> +        return;
>> +    }
>> +
>>      /* If configured to reply to neighbor requests for all VIPs force
> them
>>       * all to be considered "reachable".
>>       */
>>      if (lb->neigh_mode == LB_NEIGH_RESPOND_ALL) {
>>          for (size_t i = 0; i < lb->n_vips; i++) {
>> -            if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
>> +            if (lb->vips[i].address_family == AF_INET) {
>>                  sset_add(&od->lb_ips->ips_v4_reachable,
> lb->vips[i].vip_str);
>>              } else {
>>                  sset_add(&od->lb_ips->ips_v6_reachable,
> lb->vips[i].vip_str);
>> @@ -4041,8 +4052,9 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath
> *od,
>>      /* Otherwise, a VIP is reachable if there's at least one router
>>       * subnet that includes it.
>>       */
>> +    ovs_assert(lb->neigh_mode == LB_NEIGH_RESPOND_REACHABLE);
>>      for (size_t i = 0; i < lb->n_vips; i++) {
>> -        if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
>> +        if (lb->vips[i].address_family == AF_INET) {
>>              ovs_be32 vip_ip4 =
> in6_addr_get_mapped_ipv4(&lb->vips[i].vip);
>>              struct ovn_port *op;
>>
>> @@ -5823,16 +5835,16 @@ build_empty_lb_event_flow(struct ovn_lb_vip
> *lb_vip,
>>      ds_clear(action);
>>      ds_clear(match);
>>
>> -    bool ipv4 = IN6_IS_ADDR_V4MAPPED(&lb_vip->vip);
>> +    bool ipv4 = lb_vip->address_family == AF_INET;
>>
>>      ds_put_format(match, "ip%s.dst == %s && %s",
>>                    ipv4 ? "4": "6", lb_vip->vip_str, lb->proto);
>>
>>      char *vip = lb_vip->vip_str;
>> -    if (lb_vip->vip_port) {
>> -        ds_put_format(match, " && %s.dst == %u", lb->proto,
> lb_vip->vip_port);
>> -        vip = xasprintf("%s%s%s:%u", ipv4 ? "" : "[", lb_vip->vip_str,
>> -                        ipv4 ? "" : "]", lb_vip->vip_port);
>> +    if (lb_vip->port_str) {
>> +        ds_put_format(match, " && %s.dst == %s", lb->proto,
> lb_vip->port_str);
>> +        vip = xasprintf("%s%s%s:%s", ipv4 ? "" : "[", lb_vip->vip_str,
>> +                        ipv4 ? "" : "]", lb_vip->port_str);
>>      }
>>
>>      ds_put_format(action,
>> @@ -5843,7 +5855,7 @@ build_empty_lb_event_flow(struct ovn_lb_vip *lb_vip,
>>                    event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS),
>>                    vip, lb->proto,
>>                    UUID_ARGS(&lb->nlb->header_.uuid));
>> -    if (lb_vip->vip_port) {
>> +    if (lb_vip->port_str) {
>>          free(vip);
>>      }
>>      return true;
>> @@ -6899,7 +6911,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
> struct ovn_northd_lb *lb,
>>          /* Store the original destination IP to be used when generating
>>           * hairpin flows.
>>           */
>> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>> +        if (lb->vips[i].address_family == AF_INET) {
>>              ip_match = "ip4";
>>              ds_put_format(action, REG_ORIG_DIP_IPV4 " = %s; ",
>>                            lb_vip->vip_str);
>> @@ -6910,7 +6922,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
> struct ovn_northd_lb *lb,
>>          }
>>
>>          const char *proto = NULL;
>> -        if (lb_vip->vip_port) {
>> +        if (lb_vip->port_str) {
>>              proto = "tcp";
>>              if (lb->nlb->protocol) {
>>                  if (!strcmp(lb->nlb->protocol, "udp")) {
>> @@ -6923,8 +6935,8 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
> struct ovn_northd_lb *lb,
>>              /* Store the original destination port to be used when
> generating
>>               * hairpin flows.
>>               */
>> -            ds_put_format(action, REG_ORIG_TP_DPORT " = %"PRIu16"; ",
>> -                          lb_vip->vip_port);
>> +            ds_put_format(action, REG_ORIG_TP_DPORT " = %s; ",
>> +                          lb_vip->port_str);
>>          }
>>          ds_put_format(action, "%s;", ct_lb_mark ? "ct_lb_mark" :
> "ct_lb");
>>
>> @@ -6962,24 +6974,12 @@ build_lb_rules(struct hmap *lflows, struct
> ovn_northd_lb *lb, bool ct_lb_mark,
>>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
>>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
>>          const char *ip_match = NULL;
>> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>> +        if (lb_vip->address_family == AF_INET) {
>>              ip_match = "ip4";
>>          } else {
>>              ip_match = "ip6";
>>          }
>>
>> -        const char *proto = NULL;
>> -        if (lb_vip->vip_port) {
>> -            proto = "tcp";
>> -            if (lb->nlb->protocol) {
>> -                if (!strcmp(lb->nlb->protocol, "udp")) {
>> -                    proto = "udp";
>> -                } else if (!strcmp(lb->nlb->protocol, "sctp")) {
>> -                    proto = "sctp";
>> -                }
>> -            }
>> -        }
>> -
>>          ds_clear(action);
>>          ds_clear(match);
>>
>> @@ -6997,8 +6997,9 @@ build_lb_rules(struct hmap *lflows, struct
> ovn_northd_lb *lb, bool ct_lb_mark,
>>          ds_put_format(match, "ct.new && %s.dst == %s", ip_match,
>>                        lb_vip->vip_str);
>>          int priority = 110;
>> -        if (lb_vip->vip_port) {
>> -            ds_put_format(match, " && %s.dst == %d", proto,
> lb_vip->vip_port);
>> +        if (lb_vip->port_str) {
>> +            ds_put_format(match, " && %s.dst == %s", lb->proto,
>> +                          lb_vip->port_str);
>>              priority = 120;
>>          }
>>
>> @@ -9998,7 +9999,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
>>       * of "ct_lb_mark($targets);". The other flow is for ct.est with
>>       * an action of "next;".
>>       */
>> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>> +    if (lb_vip->address_family == AF_INET) {
>>          ds_put_format(match, "ip4 && "REG_NEXT_HOP_IPV4" == %s",
>>                        lb_vip->vip_str);
>>      } else {
>> @@ -10014,14 +10015,14 @@ build_lrouter_nat_flows_for_lb(struct
> ovn_lb_vip *lb_vip,
>>      }
>>
>>      int prio = 110;
>> -    if (lb_vip->vip_port) {
>> +    if (lb_vip->port_str) {
>>          prio = 120;
>>          new_match = xasprintf("ct.new && %s && %s && "
>> -                              REG_ORIG_TP_DPORT_ROUTER" == %d",
>> -                              ds_cstr(match), lb->proto,
> lb_vip->vip_port);
>> +                              REG_ORIG_TP_DPORT_ROUTER" == %s",
>> +                              ds_cstr(match), lb->proto,
> lb_vip->port_str);
>>          est_match = xasprintf("ct.est && %s && %s && "
>> -                              REG_ORIG_TP_DPORT_ROUTER" == %d && %s ==
> 1",
>> -                              ds_cstr(match), lb->proto,
> lb_vip->vip_port,
>> +                              REG_ORIG_TP_DPORT_ROUTER" == %s && %s ==
> 1",
>> +                              ds_cstr(match), lb->proto,
> lb_vip->port_str,
>>                                ct_natted);
>>      } else {
>>          new_match = xasprintf("ct.new && %s", ds_cstr(match));
>> @@ -10030,7 +10031,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
>>      }
>>
>>      const char *ip_match = NULL;
>> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>> +    if (lb_vip->address_family == AF_INET) {
>>          ip_match = "ip4";
>>      } else {
>>          ip_match = "ip6";
>> @@ -10048,9 +10049,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
>>          ds_put_format(&undnat_match, "(%s.src == %s", ip_match,
>>                        backend->ip_str);
>>
>> -        if (backend->port) {
>> -            ds_put_format(&undnat_match, " && %s.src == %d) || ",
>> -                          lb->proto, backend->port);
>> +        if (backend->port_str) {
>> +            ds_put_format(&undnat_match, " && %s.src == %s) || ",
>> +                          lb->proto, backend->port_str);
>>          } else {
>>              ds_put_cstr(&undnat_match, ") || ");
>>          }
>> @@ -10063,9 +10064,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
>>      struct ds unsnat_match = DS_EMPTY_INITIALIZER;
>>      ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
>>                    ip_match, ip_match, lb_vip->vip_str, lb->proto);
>> -    if (lb_vip->vip_port) {
>> -        ds_put_format(&unsnat_match, " && %s.dst == %d", lb->proto,
>> -                      lb_vip->vip_port);
>> +    if (lb_vip->port_str) {
>> +        ds_put_format(&unsnat_match, " && %s.dst == %s", lb->proto,
>> +                      lb_vip->port_str);
>>      }
>>
>>      struct ovn_datapath **gw_router_skip_snat =
>> @@ -10305,7 +10306,7 @@ build_lrouter_defrag_flows_for_lb(struct
> ovn_northd_lb *lb,
>>          ds_clear(&defrag_actions);
>>          ds_clear(match);
>>
>> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>> +        if (lb_vip->address_family == AF_INET) {
>>              ds_put_format(match, "ip && ip4.dst == %s", lb_vip->vip_str);
>>              ds_put_format(&defrag_actions, REG_NEXT_HOP_IPV4" = %s; ",
>>                            lb_vip->vip_str);
>> @@ -10315,7 +10316,7 @@ build_lrouter_defrag_flows_for_lb(struct
> ovn_northd_lb *lb,
>>                            lb_vip->vip_str);
>>          }
>>
>> -        if (lb_vip->vip_port) {
>> +        if (lb_vip->port_str) {
>>              ds_put_format(match, " && %s", lb->proto);
>>              prio = 110;
>>
>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> index bd28ac372..fb63b212e 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -1908,6 +1908,18 @@
>>          requests only for VIPs that are part of a router's subnet.  The
> default
>>          value of this option, if not specified, is
> <code>reachable</code>.
>>        </column>
>> +
>> +      <column name="options" key="template">
>> +        Option to be set to <code>true</code>, if the load balancer is a
>> +        template.  In this case the template variables used within the
>> +        load balancer VIPs and backends will be instantiated differently
>> +        on different chassis.
> 
> It would be better to mention what are template variables, with a reference
> to the chassis_template_var table. An simple example would be even better.
> 

Sure, I'll do that.

>> +      </column>
>> +
>> +      <column name="options" key="address-family">
>> +        Address family used by the load balancer.  This value is used
> only
>> +        when the load balancer is a template.
> 
> It would be better to state the keywords "ipv4" & "ipv6", and point out
> that this is *required* if and only if the options:template is "true".
> 

Better indeed, I'll take care of it.

> Thanks,
> Han
> 

Thanks,
Dumitru

>> +      </column>
>>      </group>
>>    </table>
>>
>> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
>> index 6a5d52a5d..9b766d63e 100644
>> --- a/tests/ovn-nbctl.at
>> +++ b/tests/ovn-nbctl.at
>> @@ -857,23 +857,19 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
> 30.0.0.10 192.168.10.10:a80], [
>>  [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
>>  ])
>>
>> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:],
> [1], [],
>> -[ovn-nbctl: 192.168.10.10:: should be an IP address.
>> -])
>> -
>>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10
> 192.168.10.1a], [1], [],
>>  [ovn-nbctl: 192.168.10.1a: should be an IP address.
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,
> 192.168.10.20:80 tcp], [1], [],
>> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>> +[ovn-nbctl: 192.168.10.10:80: should be an IP address, 192.168.10.20:80:
> should be an IP address.
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>>  ])
>>
>> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:900 tcp], [1], [],
>> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>>  ])
>>
>> @@ -1111,7 +1107,7 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
> ae0f::10fff [[fd0f::10]]:80,fd0
>>
>>
>>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
> [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
>> -[ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
>> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80:
> should be an IP address.
>>  ])
>>
>>
>> @@ -1125,18 +1121,13 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
> ae0f::10 [[fd0f::10]]:a80], [1]
>>  ])
>>
>>
>> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
> [[fd0f::10]]:], [1], [],
>> -[ovn-nbctl: [[fd0f::10]]:: should be an IP address.
>> -])
>> -
>> -
>>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a],
> [1], [],
>>  [ovn-nbctl: fd0f::1001a: should be an IP address.
>>  ])
>>
>>
>>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:
> [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
>> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80:
> should be an IP address.
>>  ])
>>
>>
>> @@ -1146,7 +1137,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10
> tcp], [1], [],
>>
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:900 tcp], [1], [],
>> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>> +[ovn-nbctl: [[fd0f::10]]:900: should be an IP address.
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 192.168.10.10], [1], [],
>> @@ -1158,7 +1149,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10
> 192.168.10.10], [1], [],
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 192.168.10.10:80], [1],
> [],
>> -[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP
> [[ae0f::10]]:80.
>> +[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP
> ae0f::10.
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10], [1], [],
>> @@ -1166,7 +1157,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10],
> [1], [],
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 [[ae0f::10]]:80], [1], [],
>> -[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP
> 30.0.0.10:80.
>> +[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP
> 30.0.0.10.
>>  ])
>>
>>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10])
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index b6843956c..39032d269 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -1828,6 +1828,11 @@ ovn-nbctl set Load_Balancer lb8
> options:neighbor_responder=all
>>  ovn-nbctl lb-add lb9 "[[4444::4444]]:8080" "[[10::10]]:8080" udp
>>  ovn-nbctl set Load_Balancer lb9 options:neighbor_responder=all
>>
>> +ovn-nbctl lb-add lb10 "55.55.55.55:8080" "10.0.0.8:8080" udp
>> +ovn-nbctl set Load_Balancer lb10 options:neighbor_responder=none
>> +ovn-nbctl lb-add lb11 "[[5555::5555]]:8080" "[[10::10]]:8080" udp
>> +ovn-nbctl set Load_Balancer lb11 options:neighbor_responder=none
>> +
>>  ovn-nbctl lr-lb-add lr lb1
>>  ovn-nbctl lr-lb-add lr lb2
>>  ovn-nbctl lr-lb-add lr lb3
>> @@ -1837,6 +1842,8 @@ ovn-nbctl lr-lb-add lr lb6
>>  ovn-nbctl lr-lb-add lr lb7
>>  ovn-nbctl lr-lb-add lr lb8
>>  ovn-nbctl lr-lb-add lr lb9
>> +ovn-nbctl lr-lb-add lr lb10
>> +ovn-nbctl lr-lb-add lr lb11
>>
>>  ovn-nbctl --wait=sb sync
>>  lr_key=$(fetch_column sb:datapath_binding tunnel_key
> external_ids:name=lr)
>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
>> index 811468dc6..4feae1db2 100644
>> --- a/utilities/ovn-nbctl.c
>> +++ b/utilities/ovn-nbctl.c
>> @@ -28,6 +28,7 @@
>>  #include "openvswitch/json.h"
>>  #include "lib/acl-log.h"
>>  #include "lib/copp.h"
>> +#include "lib/lb.h"
>>  #include "lib/ovn-nb-idl.h"
>>  #include "lib/ovn-util.h"
>>  #include "memory.h"
>> @@ -2837,6 +2838,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>>      bool empty_backend_rej = shash_find(&ctx->options, "--reject") !=
> NULL;
>>      bool empty_backend_event = shash_find(&ctx->options, "--event") !=
> NULL;
>>      bool add_route = shash_find(&ctx->options, "--add-route") != NULL;
>> +    bool template = shash_find(&ctx->options, "--template") != NULL;
>>
>>      if (empty_backend_event && empty_backend_rej) {
>>              ctl_error(ctx,
>> @@ -2844,10 +2846,12 @@ nbctl_lb_add(struct ctl_context *ctx)
>>              return;
>>      }
>>
>> +    const char *lb_address_family_str = "ipv4";
>> +    int lb_address_family = AF_INET;
>>      const char *lb_proto;
>>      bool is_update_proto = false;
>>
>> -    if (ctx->argc == 4) {
>> +    if (ctx->argc <= 4) {
>>          /* Default protocol. */
>>          lb_proto = "tcp";
>>      } else {
>> @@ -2863,79 +2867,61 @@ nbctl_lb_add(struct ctl_context *ctx)
>>          }
>>      }
>>
>> -    struct sockaddr_storage ss_vip;
>> -    if (!inet_parse_active(lb_vip, 0, &ss_vip, false, NULL)) {
>> -        ctl_error(ctx, "%s: should be an IP address (or an IP address "
>> -                  "and a port number with : as a separator).", lb_vip);
>> -        return;
>> -    }
>> -
>> -    struct ds lb_vip_normalized_ds = DS_EMPTY_INITIALIZER;
>> -    uint16_t lb_vip_port = ss_get_port(&ss_vip);
>> -    if (lb_vip_port) {
>> -        ss_format_address(&ss_vip, &lb_vip_normalized_ds);
>> -        ds_put_format(&lb_vip_normalized_ds, ":%d", lb_vip_port);
>> -    } else {
>> -        ss_format_address_nobracks(&ss_vip, &lb_vip_normalized_ds);
>> -    }
>> -    const char *lb_vip_normalized = ds_cstr(&lb_vip_normalized_ds);
>> +    if (ctx->argc > 5) {
>> +        lb_address_family_str = ctx->argv[5];
>> +        lb_address_family = !strcmp(lb_address_family_str, "ipv4")
>> +                            ? AF_INET : AF_INET6;
>>
>> -    if (!lb_vip_port && is_update_proto) {
>> -        ds_destroy(&lb_vip_normalized_ds);
>> -        ctl_error(ctx, "Protocol is unnecessary when no port of vip "
>> -                  "is given.");
>> -        return;
>>      }
>>
>> -    char *token = NULL, *save_ptr = NULL;
>> +    struct ds lb_vip_normalized = DS_EMPTY_INITIALIZER;
>>      struct ds lb_ips_new = DS_EMPTY_INITIALIZER;
>> -    for (token = strtok_r(lb_ips, ",", &save_ptr);
>> -            token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
>> -        struct sockaddr_storage ss_dst;
>> +    struct ovn_lb_vip lb_vip_parsed;
>>
>> -        if (lb_vip_port) {
>> -            if (!inet_parse_active(token, -1, &ss_dst, false, NULL)) {
>> -                ctl_error(ctx, "%s: should be an IP address and a port "
>> -                          "number with : as a separator.", token);
>> -                goto out;
>> -            }
>> -        } else {
>> -            if (!inet_parse_address(token, &ss_dst)) {
>> -                ctl_error(ctx, "%s: should be an IP address.", token);
>> -                goto out;
>> -            }
>> -        }
>> +    char *error = ovn_lb_vip_init(&lb_vip_parsed, lb_vip, lb_ips,
> template,
>> +                                  lb_address_family);
>> +    if (error) {
>> +        ctl_error(ctx, "%s", error);
>> +        ovn_lb_vip_destroy(&lb_vip_parsed);
>> +        free(error);
>> +        return;
>> +    }
>>
>> -        if (ss_vip.ss_family != ss_dst.ss_family) {
>> -            ctl_error(ctx, "%s: IP address family is different from VIP
> %s.",
>> -                      token, lb_vip_normalized);
>> -            goto out;
>> -        }
>> -        ds_put_format(&lb_ips_new, "%s%s",
>> -                lb_ips_new.length ? "," : "", token);
>> +    if (is_update_proto && !lb_vip_parsed.port_str) {
>> +        ctl_error(ctx, "Protocol is unnecessary when no port of vip is "
>> +                       "given.");
>> +        ovn_lb_vip_destroy(&lb_vip_parsed);
>> +        return;
>>      }
>>
>> +    ovn_lb_vip_format(&lb_vip_parsed, &lb_vip_normalized, template);
>> +    ovn_lb_vip_backends_format(&lb_vip_parsed, &lb_ips_new, template);
>> +    ovn_lb_vip_destroy(&lb_vip_parsed);
>> +
>>      const struct nbrec_load_balancer *lb = NULL;
>>      if (!add_duplicate) {
>> -        char *error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
>> +        error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
>>          if (error) {
>>              ctx->error = error;
>>              goto out;
>>          }
>>          if (lb) {
>> -            if (smap_get(&lb->vips, lb_vip_normalized)) {
>> +            if (smap_get(&lb->vips, ds_cstr(&lb_vip_normalized))) {
>>                  if (!may_exist) {
>>                      ctl_error(ctx, "%s: a load balancer with this vip
> (%s) "
>> -                              "already exists", lb_name,
> lb_vip_normalized);
>> +                              "already exists", lb_name,
>> +                              ds_cstr(&lb_vip_normalized));
>>                      goto out;
>>                  }
>>                  /* Update the vips. */
>>                  smap_replace(CONST_CAST(struct smap *, &lb->vips),
>> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
>> +                             ds_cstr(&lb_vip_normalized),
>> +                             ds_cstr(&lb_ips_new));
>>              } else {
>>                  /* Add the new vips. */
>>                  smap_add(CONST_CAST(struct smap *, &lb->vips),
>> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
>> +                         ds_cstr(&lb_vip_normalized),
>> +                         ds_cstr(&lb_ips_new));
>>              }
>>
>>              /* Update the load balancer. */
>> @@ -2954,7 +2940,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>>      nbrec_load_balancer_set_name(lb, lb_name);
>>      nbrec_load_balancer_set_protocol(lb, lb_proto);
>>      smap_add(CONST_CAST(struct smap *, &lb->vips),
>> -            lb_vip_normalized, ds_cstr(&lb_ips_new));
>> +             ds_cstr(&lb_vip_normalized), ds_cstr(&lb_ips_new));
>>      nbrec_load_balancer_set_vips(lb, &lb->vips);
>>      struct smap options = SMAP_INITIALIZER(&options);
>>      if (empty_backend_rej) {
>> @@ -2966,12 +2952,16 @@ nbctl_lb_add(struct ctl_context *ctx)
>>      if (add_route) {
>>          smap_add(&options, "add_route", "true");
>>      }
>> +    if (template) {
>> +        smap_add(&options, "template", "true");
>> +        smap_add(&options, "address-family", lb_address_family_str);
>> +    }
>>      nbrec_load_balancer_set_options(lb, &options);
>>      smap_destroy(&options);
>>  out:
>>      ds_destroy(&lb_ips_new);
>>
>> -    ds_destroy(&lb_vip_normalized_ds);
>> +    ds_destroy(&lb_vip_normalized);
>>  }
>>
>>  static void
>> @@ -3025,6 +3015,7 @@ static void
>>  nbctl_pre_lb_list(struct ctl_context *ctx)
>>  {
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
>> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>>  }
>> @@ -3033,6 +3024,7 @@ static void
>>  lb_info_add_smap(const struct nbrec_load_balancer *lb,
>>                   struct smap *lbs, int vip_width)
>>  {
>> +    bool template = smap_get_bool(&lb->options, "template", false);
>>      const struct smap_node **nodes = smap_sort(&lb->vips);
>>      if (!nodes) {
>>          return;
>> @@ -3041,13 +3033,24 @@ lb_info_add_smap(const struct nbrec_load_balancer
> *lb,
>>      struct ds val = DS_EMPTY_INITIALIZER;
>>      for (size_t i = 0; i < smap_count(&lb->vips); i++) {
>>          const struct smap_node *node = nodes[i];
>> +        const char *protocol = lb->protocol;
>>
>> -        struct sockaddr_storage ss;
>> -        if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
>> -            continue;
>> +        if (!template) {
>> +            struct sockaddr_storage ss;
>> +            if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
>> +                continue;
>> +            }
>> +            protocol = ss_get_port(&ss) ? lb->protocol : "";
>> +        } else {
>> +            if (!lb->protocol) {
>> +                VLOG_WARN("Load Balancer "UUID_FMT" (%s) is a template
> and "
>> +                          "misses protocol",
> UUID_ARGS(&lb->header_.uuid),
>> +                          lb->name);
>> +                continue;
>> +            }
>> +            protocol = lb->protocol;
>>          }
>>
>> -        char *protocol = ss_get_port(&ss) ? lb->protocol : "";
>>          if (i == 0) {
>>              ds_put_format(&val, UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
>>                            UUID_ARGS(&lb->header_.uuid),
>> @@ -3239,6 +3242,7 @@ nbctl_pre_lr_lb_list(struct ctl_context *ctx)
>>                           &nbrec_logical_router_col_load_balancer_group);
>>
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
>> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>>
>> @@ -3402,6 +3406,7 @@ nbctl_pre_ls_lb_list(struct ctl_context *ctx)
>>                           &nbrec_logical_switch_col_load_balancer_group);
>>
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
>> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
>>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
>>
>> @@ -7469,9 +7474,10 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
>>        nbctl_pre_lr_nat_set_ext_ips, nbctl_lr_nat_set_ext_ips,
>>        NULL, "--is-exempted", RW},
>>      /* load balancer commands. */
>> -    { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]",
>> +    { "lb-add", 3, 5, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]
> [ADDRESS_FAMILY]",
>>        nbctl_pre_lb_add, nbctl_lb_add, NULL,
>> -      "--may-exist,--add-duplicate,--reject,--event,--add-route", RW },
>> +
>  "--may-exist,--add-duplicate,--reject,--event,--add-route,--template",
>> +      RW },
>>      { "lb-del", 1, 2, "LB [VIP]", nbctl_pre_lb_del, nbctl_lb_del, NULL,
>>          "--if-exists", RW },
>>      { "lb-list", 0, 1, "[LB]", nbctl_pre_lb_list, nbctl_lb_list, NULL,
> "", RO },
>>
>

On Fri, Oct 21, 2022 at 1:00 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 10/21/22 07:47, Han Zhou wrote:
> > On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >>
> >> Allow the CMS to configure template LBs.  The following configurations
are
> >> supported:
> >> - VIPs of the form: ^vip_variable[:^port_variable|:port]
> >> - Backends of the form:
> >>
> >
^backendip_variable1[:^port_variable1|:port],^backendip_variable2[:^port_variable2|:port]
> >>   OR
> >>   ^backends_variable1,^backends_variable2
> >>
> >> The CMS needs to provide a bit more information than with non-template
> > load
> >> balancers and must explicitly specify the address family to be used.
> >>
> >> There is currently no support for template load balancers with
> >> options:add_route=true set.  That is because ovn-northd does not
> >> instantiate template variables.  While this is a limitation in a way,
its
> >> impact is not huge.  The load balancer 'add_route' option was added as
a
> >> way to make the CMS life easier and to avoid having to explicitly add a
> >> route for the VIP.  The CMS can still achieve the same logical
topology by
> >> explicitly adding the VIP route.
> >>
> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> >
> > Hi Dumitru, sorry for the delayed response. Please see my comments
below.
> >
>
> Hi Han,
>
> No worries, thanks for the review!
>
> >> ---
> >>  controller/lflow.c    |   18 +-
> >>  lib/lb.c              |  463
> > ++++++++++++++++++++++++++++++++++++++++++-------
> >>  lib/lb.h              |   39 +++-
> >>  lib/ovn-util.c        |    3
> >>  northd/northd.c       |   85 +++++----
> >>  ovn-nb.xml            |   12 +
> >>  tests/ovn-nbctl.at    |   23 +-
> >>  tests/ovn-northd.at   |    7 +
> >>  utilities/ovn-nbctl.c |  122 +++++++------
> >>  9 files changed, 576 insertions(+), 196 deletions(-)
> >>
> >> diff --git a/controller/lflow.c b/controller/lflow.c
> >> index ad58fcd3c..ffc8fdb6e 100644
> >> --- a/controller/lflow.c
> >> +++ b/controller/lflow.c
> >> @@ -1993,7 +1993,9 @@ add_lb_ct_snat_hairpin_flows(struct
> > ovn_controller_lb *lb,
> >>
> >>  static void
> >>  consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
> >> -                          const struct hmap *local_datapaths, bool
> > use_ct_mark,
> >> +                          const struct hmap *local_datapaths,
> >> +                          const struct smap *template_vars,
> >
> > template_vars is added as an input, but I don't see I-P for
template_vars
> > for hairpin flows in this patch, which makes this patch incomplete.
> > I see that it is taken care by the next patch. So they should be either
> > combined or switch the patch order, so that each incremental patch is
> > correct.
> > In addition, some basic tests may be added to this patch (which should
> > discover problems of hairpin flows when template vars are updated, due
to
> > the missing I-P).
> >
>
> I initially wanted to keep things simpler but you're right, now at patch
> 5/8 we have an incomplete implementation.  I'll squash the two patches
> together and add more tests.
>
> >> +                          bool use_ct_mark,
> >>                            struct ovn_desired_flow_table *flow_table,
> >>                            struct simap *ids)
> >>  {
> >> @@ -2029,7 +2031,8 @@ consider_lb_hairpin_flows(const struct
> > sbrec_load_balancer *sbrec_lb,
> >>          return;
> >>      }
> >>
> >> -    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb);
> >> +    struct ovn_controller_lb *lb = ovn_controller_lb_create(sbrec_lb,
> >> +
> >  template_vars);
> >>      uint8_t lb_proto = IPPROTO_TCP;
> >>      if (lb->slb->protocol && lb->slb->protocol[0]) {
> >>          if (!strcmp(lb->slb->protocol, "udp")) {
> >> @@ -2059,7 +2062,9 @@ consider_lb_hairpin_flows(const struct
> > sbrec_load_balancer *sbrec_lb,
> >>   * backends to handle the load balanced hairpin traffic. */
> >>  static void
> >>  add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table,
> >> -                     const struct hmap *local_datapaths, bool
> > use_ct_mark,
> >> +                     const struct hmap *local_datapaths,
> >> +                     const struct smap *template_vars,
> >> +                     bool use_ct_mark,
> >>                       struct ovn_desired_flow_table *flow_table,
> >>                       struct simap *ids,
> >>                       struct id_pool *pool)
> >> @@ -2082,8 +2087,8 @@ add_lb_hairpin_flows(const struct
> > sbrec_load_balancer_table *lb_table,
> >>              ovs_assert(id_pool_alloc_id(pool, &id));
> >>              simap_put(ids, lb->name, id);
> >>          }
> >> -        consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark,
> > flow_table,
> >> -                                  ids);
> >> +        consider_lb_hairpin_flows(lb, local_datapaths, template_vars,
> >> +                                  use_ct_mark, flow_table, ids);
> >>      }
> >>  }
> >>
> >> @@ -2220,6 +2225,7 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
> > lflow_ctx_out *l_ctx_out)
> >>                         l_ctx_in->local_datapaths,
> >>                         l_ctx_out->flow_table);
> >>      add_lb_hairpin_flows(l_ctx_in->lb_table,
l_ctx_in->local_datapaths,
> >> +                         l_ctx_in->template_vars,
> >>                           l_ctx_in->lb_hairpin_use_ct_mark,
> >>                           l_ctx_out->flow_table,
> >>                           l_ctx_out->hairpin_lb_ids,
> >> @@ -2351,6 +2357,7 @@ lflow_add_flows_for_datapath(const struct
> > sbrec_datapath_binding *dp,
> >>       * associated. */
> >>      for (size_t i = 0; i < n_dp_lbs; i++) {
> >>          consider_lb_hairpin_flows(dp_lbs[i],
l_ctx_in->local_datapaths,
> >> +                                  l_ctx_in->template_vars,
> >>                                    l_ctx_in->lb_hairpin_use_ct_mark,
> >>                                    l_ctx_out->flow_table,
> >>                                    l_ctx_out->hairpin_lb_ids);
> >> @@ -2493,6 +2500,7 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
> > *l_ctx_in,
> >>          VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT,
> >>                   UUID_ARGS(&lb->header_.uuid));
> >>          consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> >> +                                  l_ctx_in->template_vars,
> >>                                    l_ctx_in->lb_hairpin_use_ct_mark,
> >>                                    l_ctx_out->flow_table,
> >>                                    l_ctx_out->hairpin_lb_ids);
> >> diff --git a/lib/lb.c b/lib/lb.c
> >> index 77674ea28..15cbf29ee 100644
> >> --- a/lib/lb.c
> >> +++ b/lib/lb.c
> >> @@ -19,6 +19,7 @@
> >>  #include "lib/ovn-nb-idl.h"
> >>  #include "lib/ovn-sb-idl.h"
> >>  #include "lib/ovn-util.h"
> >> +#include "ovn/lex.h"
> >>
> >>  /* OpenvSwitch lib includes. */
> >>  #include "openvswitch/vlog.h"
> >> @@ -26,6 +27,16 @@
> >>
> >>  VLOG_DEFINE_THIS_MODULE(lb);
> >>
> >> +static const char *lb_neighbor_responder_mode_names[] = {
> >> +    [LB_NEIGH_RESPOND_REACHABLE] = "reachable",
> >> +    [LB_NEIGH_RESPOND_ALL] = "all",
> >> +    [LB_NEIGH_RESPOND_NONE] = "none",
> >> +};
> >> +
> >> +static struct nbrec_load_balancer_health_check *
> >> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
> >> +                        const char *vip_port_str, bool template);
> >> +
> >>  struct ovn_lb_ip_set *
> >>  ovn_lb_ip_set_create(void)
> >>  {
> >> @@ -71,108 +82,312 @@ ovn_lb_ip_set_clone(struct ovn_lb_ip_set
*lb_ip_set)
> >>      return clone;
> >>  }
> >>
> >> -static
> >> -bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> >> -                     const char *lb_value)
> >> +/* Format for backend ips: "IP1:port1,IP2:port2,...". */
> >> +static char *
> >> +ovn_lb_backends_init_explicit(struct ovn_lb_vip *lb_vip, const char
> > *value,
> >> +                              size_t *n_backends)
> >>  {
> >> -    int addr_family;
> >> -
> >> -    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
> >> -                                         &lb_vip->vip_port,
> > &addr_family)) {
> >> -        return false;
> >> -    }
> >> -
> >> -    if (addr_family == AF_INET) {
> >> -        ovs_be32 vip4;
> >> -        ip_parse(lb_vip->vip_str, &vip4);
> >> -        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
> >> -    } else {
> >> -        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
> >> -    }
> >> -
> >> -    /* Format for backend ips: "IP1:port1,IP2:port2,...". */
> >> -    size_t n_backends = 0;
> >> +    struct ds errors = DS_EMPTY_INITIALIZER;
> >>      size_t n_allocated_backends = 0;
> >> -    char *tokstr = xstrdup(lb_value);
> >> +    char *tokstr = xstrdup(value);
> >>      char *save_ptr = NULL;
> >> +    *n_backends = 0;
> >> +
> >>      for (char *token = strtok_r(tokstr, ",", &save_ptr);
> >>          token != NULL;
> >>          token = strtok_r(NULL, ",", &save_ptr)) {
> >>
> >> -        if (n_backends == n_allocated_backends) {
> >> +        if (*n_backends == n_allocated_backends) {
> >>              lb_vip->backends = x2nrealloc(lb_vip->backends,
> >>                                            &n_allocated_backends,
> >>                                            sizeof *lb_vip->backends);
> >>          }
> >>
> >> -        struct ovn_lb_backend *backend =
&lb_vip->backends[n_backends];
> >> +        struct ovn_lb_backend *backend =
> > &lb_vip->backends[(*n_backends)];
> >>          int backend_addr_family;
> >>          if (!ip_address_and_port_from_lb_key(token, &backend->ip_str,
> >>                                               &backend->port,
> >>                                               &backend_addr_family)) {
> >> +            if (lb_vip->port_str) {
> >> +                ds_put_format(&errors, "%s: should be an IP address
and
> > a "
> >> +                                       "port number with : as a
> > separator, ",
> >> +                              token);
> >> +            } else {
> >> +                ds_put_format(&errors, "%s: should be an IP address,
",
> > token);
> >> +            }
> >>              continue;
> >>          }
> >>
> >> -        if (addr_family != backend_addr_family) {
> >> +        if (lb_vip->address_family != backend_addr_family) {
> >>              free(backend->ip_str);
> >> +            ds_put_format(&errors, "%s: IP address family is different
> > from "
> >> +                                   "VIP %s, ",
> >> +                          token, lb_vip->vip_str);
> >>              continue;
> >>          }
> >>
> >> -        if (addr_family == AF_INET) {
> >> +        if (lb_vip->port_str) {
> >> +            if (!backend->port) {
> >> +                free(backend->ip_str);
> >> +                ds_put_format(&errors, "%s: should be an IP address
and "
> >> +                                       "a port number with : as a
> > separator, ",
> >> +                              token);
> >> +                continue;
> >> +            }
> >> +        } else {
> >> +            if (backend->port) {
> >> +                free(backend->ip_str);
> >> +                ds_put_format(&errors, "%s: should be an IP address,
",
> > token);
> >> +                continue;
> >> +            }
> >> +        }
> >> +
> >> +        backend->port_str =
> >> +            backend->port ? xasprintf("%"PRIu16, backend->port) :
NULL;
> >> +
> >> +        if (lb_vip->address_family == AF_INET) {
> >>              ovs_be32 ip4;
> >>              ip_parse(backend->ip_str, &ip4);
> >>              in6_addr_set_mapped_ipv4(&backend->ip, ip4);
> >>          } else {
> >>              ipv6_parse(backend->ip_str, &backend->ip);
> >>          }
> >> -        n_backends++;
> >> +        (*n_backends)++;
> >>      }
> >>      free(tokstr);
> >> -    lb_vip->n_backends = n_backends;
> >> -    return true;
> >> +
> >> +    if (ds_last(&errors) != EOF) {
> >> +        ds_chomp(&errors, ' ');
> >> +        ds_chomp(&errors, ',');
> >> +        ds_put_char(&errors, '.');
> >> +        return ds_steal_cstr(&errors);
> >> +    }
> >> +    return NULL;
> >>  }
> >>
> >>  static
> >> -void ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
> >> +char *ovn_lb_vip_init_explicit(struct ovn_lb_vip *lb_vip, const char
> > *lb_key,
> >> +                               const char *lb_value)
> >> +{
> >> +    if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
> >> +                                         &lb_vip->vip_port,
> >> +                                         &lb_vip->address_family)) {
> >> +        return xasprintf("%s: should be an IP address (or an IP
address "
> >> +                         "and a port number with : as a separator).",
> > lb_key);
> >> +    }
> >> +
> >> +    lb_vip->port_str = lb_vip->vip_port
> >> +                       ? xasprintf("%"PRIu16, lb_vip->vip_port)
> >> +                       : NULL;
> >> +
> >> +    if (lb_vip->address_family == AF_INET) {
> >> +        ovs_be32 vip4;
> >> +        ip_parse(lb_vip->vip_str, &vip4);
> >> +        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
> >> +    } else {
> >> +        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
> >> +    }
> >> +    return ovn_lb_backends_init_explicit(lb_vip, lb_value,
> >> +                                         &lb_vip->n_backends);
> >> +}
> >> +
> >> +/* Parses backends of a templated LB VIP.
> >> + * For now only the following template forms are supported:
> >> + * A.
> >> + *   ^backendip_variable1[:^port_variable1|:port],
> >> + *   ^backendip_variable2[:^port_variable2|:port]
> >> + *
> >> + * B.
> >> + *   ^backends_variable1,^backends_variable2 is also a thing
> >> + *      where 'backends_variable1' may expand to IP1_1:PORT1_1 on
> > chassis-1
> >> + *                                               IP1_2:PORT1_2 on
> > chassis-2
> >> + *        and 'backends_variable2' may expand to IP2_1:PORT2_1 on
> > chassis-1
> >> + *                                               IP2_2:PORT2_2 on
> > chassis-2
> >> + */
> >> +static char *
> >> +ovn_lb_backends_init_template(struct ovn_lb_vip *lb_vip, const char
> > *value_,
> >> +                              size_t *n_backends)
> >> +{
> >> +    struct ds errors = DS_EMPTY_INITIALIZER;
> >> +    char *value = xstrdup(value_);
> >> +    char *save_ptr = NULL;
> >> +    size_t n_allocated_backends = 0;
> >> +    *n_backends = 0;
> >> +
> >> +    for (char *backend = strtok_r(value, ",", &save_ptr); backend;
> >> +         backend = strtok_r(NULL, ",", &save_ptr)) {
> >> +
> >> +        char *atom = xstrdup(backend);
> >> +        char *save_ptr2 = NULL;
> >> +        bool success = false;
> >> +        char *backend_ip = NULL;
> >> +        char *backend_port = NULL;
> >> +
> >> +        for (char *subatom = strtok_r(atom, ":", &save_ptr2); subatom;
> >> +             subatom = strtok_r(NULL, ":", &save_ptr2)) {
> >> +            if (backend_ip && backend_port) {
> >> +                success = false;
> >> +                break;
> >> +            }
> >> +            success = true;
> >> +            if (!backend_ip) {
> >> +                backend_ip = xstrdup(subatom);
> >> +            } else {
> >> +                backend_port = xstrdup(subatom);
> >> +            }
> >> +        }
> >> +
> >> +        if (success) {
> >> +            if (*n_backends == n_allocated_backends) {
> >> +                lb_vip->backends = x2nrealloc(lb_vip->backends,
> >> +                                              &n_allocated_backends,
> >> +                                              sizeof
*lb_vip->backends);
> >> +            }
> >> +
> >> +            struct ovn_lb_backend *lb_backend =
> >> +                &lb_vip->backends[(*n_backends)];
> >> +            lb_backend->ip_str = backend_ip;
> >> +            lb_backend->port_str = backend_port;
> >> +            lb_backend->port = 0;
> >> +            (*n_backends)++;
> >> +        } else {
> >> +            ds_put_format(&errors, "%s: should be a template of the
> > form: "
> >> +
> >  "'^backendip_variable1[:^port_variable1|:port]', ",
> >> +                          atom);
> >> +        }
> >> +        free(atom);
> >> +    }
> >> +
> >> +    free(value);
> >> +    if (ds_last(&errors) != EOF) {
> >> +        ds_chomp(&errors, ' ');
> >> +        ds_chomp(&errors, ',');
> >> +        ds_put_char(&errors, '.');
> >> +        return ds_steal_cstr(&errors);
> >> +    }
> >> +    return NULL;
> >> +}
> >> +
> >> +/* Parses a VIP of a templated LB.
> >> + * For now only the following template forms are supported:
> >> + *   ^vip_variable[:^port_variable|:port]
> >> + */
> >> +static char *
> >> +ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip, const char
*lb_key_,
> >> +                         const char *lb_value, int address_family)
> >> +{
> >> +    char *save_ptr = NULL;
> >> +    char *lb_key = xstrdup(lb_key_);
> >> +    bool success = false;
> >> +
> >> +    for (char *atom = strtok_r(lb_key, ":", &save_ptr); atom;
> >> +         atom = strtok_r(NULL, ":", &save_ptr)) {
> >> +        if (lb_vip->vip_str && lb_vip->port_str) {
> >> +            success = false;
> >> +            break;
> >> +        }
> >> +        success = true;
> >> +        if (!lb_vip->vip_str) {
> >> +            lb_vip->vip_str = xstrdup(atom);
> >> +        } else {
> >> +            lb_vip->port_str = xstrdup(atom);
> >> +        }
> >> +    }
> >> +    free(lb_key);
> >> +
> >> +    if (!success) {
> >> +        return xasprintf("%s: should be a template of the form: "
> >> +                         "'^vip_variable[:^port_variable|:port]'.",
> >> +                         lb_key_);
> >> +    }
> >> +
> >> +    lb_vip->address_family = address_family;
> >> +    return ovn_lb_backends_init_template(lb_vip, lb_value,
> >> +                                         &lb_vip->n_backends);
> >> +}
> >> +
> >> +/* Returns NULL on success, an error string on failure.  The caller is
> >> + * responsible for destroying 'lb_vip' in all cases.
> >> + */
> >> +char *
> >> +ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> >> +                const char *lb_value, bool template, int
address_family)
> >> +{
> >> +    memset(lb_vip, 0, sizeof *lb_vip);
> >> +
> >> +    return !template
> >> +           ?  ovn_lb_vip_init_explicit(lb_vip, lb_key, lb_value)
> >> +           :  ovn_lb_vip_init_template(lb_vip, lb_key, lb_value,
> >> +                                       address_family);
> >> +}
> >> +
> >> +void
> >> +ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
> >>  {
> >>      free(vip->vip_str);
> >> +    free(vip->port_str);
> >>      for (size_t i = 0; i < vip->n_backends; i++) {
> >>          free(vip->backends[i].ip_str);
> >> +        free(vip->backends[i].port_str);
> >>      }
> >>      free(vip->backends);
> >>  }
> >>
> >> +void
> >> +ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s, bool
> > template)
> >> +{
> >> +    bool needs_brackets = vip->address_family == AF_INET6 &&
> > vip->port_str
> >> +                          && !template;
> >> +    if (needs_brackets) {
> >> +        ds_put_char(s, '[');
> >> +    }
> >> +    ds_put_cstr(s, vip->vip_str);
> >> +    if (needs_brackets) {
> >> +        ds_put_char(s, ']');
> >> +    }
> >> +    if (vip->port_str) {
> >> +        ds_put_format(s, ":%s", vip->port_str);
> >> +    }
> >> +}
> >> +
> >> +void
> >> +ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s,
> >> +                           bool template)
> >> +{
> >> +    bool needs_brackets = vip->address_family == AF_INET6 &&
> > vip->port_str
> >> +                          && !template;
> >> +    for (size_t i = 0; i < vip->n_backends; i++) {
> >> +        struct ovn_lb_backend *backend = &vip->backends[i];
> >> +
> >> +        if (needs_brackets) {
> >> +            ds_put_char(s, '[');
> >> +        }
> >> +        ds_put_cstr(s, backend->ip_str);
> >> +        if (needs_brackets) {
> >> +            ds_put_char(s, ']');
> >> +        }
> >> +        if (backend->port_str) {
> >> +            ds_put_format(s, ":%s", backend->port_str);
> >> +        }
> >> +        if (i != vip->n_backends - 1) {
> >> +            ds_put_char(s, ',');
> >> +        }
> >> +    }
> >> +}
> >> +
> >>  static
> >>  void ovn_northd_lb_vip_init(struct ovn_northd_lb_vip *lb_vip_nb,
> >>                              const struct ovn_lb_vip *lb_vip,
> >>                              const struct nbrec_load_balancer
*nbrec_lb,
> >> -                            const char *vip_port_str, const char
> > *backend_ips)
> >> +                            const char *vip_port_str, const char
> > *backend_ips,
> >> +                            bool template)
> >>  {
> >>      lb_vip_nb->backend_ips = xstrdup(backend_ips);
> >>      lb_vip_nb->n_backends = lb_vip->n_backends;
> >>      lb_vip_nb->backends_nb = xcalloc(lb_vip_nb->n_backends,
> >>                                       sizeof *lb_vip_nb->backends_nb);
> >> -
> >> -    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,
vip_port_str)) {
> >> -                lb_health_check = nbrec_lb->health_check[j];
> >> -                break;
> >> -            }
> >> -        }
> >> -    }
> >> -
> >> -    lb_vip_nb->lb_health_check = lb_health_check;
> >> +    lb_vip_nb->lb_health_check =
> >> +        ovn_lb_get_health_check(nbrec_lb, vip_port_str, template);
> >>  }
> >>
> >>  static
> >> @@ -203,12 +418,111 @@ ovn_lb_get_hairpin_snat_ip(const struct uuid
> > *lb_uuid,
> >>      }
> >>  }
> >>
> >> +static bool
> >> +ovn_lb_get_routable_mode(const struct nbrec_load_balancer *nbrec_lb,
> >> +                         bool routable, bool template)
> >> +{
> >> +    if (template && routable) {
> >> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >> +        VLOG_WARN_RL(&rl, "Template load balancer "UUID_FMT" does not
> > suport "
> >> +                           "option 'add_route'.  Forcing it to
> > disabled.",
> >> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
> >> +        return false;
> >> +    }
> >> +    return routable;
> >> +}
> >> +
> >> +static bool
> >> +ovn_lb_neigh_mode_is_valid(enum lb_neighbor_responder_mode mode, bool
> > template)
> >> +{
> >> +    if (!template) {
> >> +        return true;
> >> +    }
> >> +
> >> +    switch (mode) {
> >> +    case LB_NEIGH_RESPOND_REACHABLE:
> >> +        return false;
> >> +    case LB_NEIGH_RESPOND_ALL:
> >> +    case LB_NEIGH_RESPOND_NONE:
> >> +        return true;
> >> +    }
> >> +}
> >> +
> >> +static enum lb_neighbor_responder_mode
> >> +ovn_lb_get_neigh_mode(const struct nbrec_load_balancer *nbrec_lb,
> >> +                      const char *mode, bool template)
> >> +{
> >> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >> +    enum lb_neighbor_responder_mode default_mode =
> >> +        template ? LB_NEIGH_RESPOND_NONE : LB_NEIGH_RESPOND_REACHABLE;
> >> +
> >> +    if (!mode) {
> >> +        mode = lb_neighbor_responder_mode_names[default_mode];
> >> +    }
> >> +
> >> +    for (size_t i = 0; i <
ARRAY_SIZE(lb_neighbor_responder_mode_names);
> > i++) {
> >> +        if (!strcmp(mode, lb_neighbor_responder_mode_names[i])) {
> >> +            if (ovn_lb_neigh_mode_is_valid(i, template)) {
> >> +                return i;
> >> +            }
> >> +            break;
> >> +        }
> >> +    }
> >> +
> >> +    VLOG_WARN_RL(&rl, "Invalid neighbor responder mode %s for load
> > balancer "
> >> +                       UUID_FMT", forcing it to %s",
> >> +                 mode, UUID_ARGS(&nbrec_lb->header_.uuid),
> >> +                 lb_neighbor_responder_mode_names[default_mode]);
> >> +    return default_mode;
> >> +}
> >
> > This new limitation of the neighbor_responder mode needs to be
documented
> > in ovn-nb.xml for the options:neighbor_responder.
> >
>
> You're right, I'll do that.
>
> > Also curious why can't we support responders for templated LBs? Can the
> > relevant lflows be generated with the template vars used for the LB
VIPs as
> > well?
> >
>
> It seemed quite complex to implement correctly.  That's because in the
> "neighbor_responder=reachable" mode we need to reply only to ARPs for
> VIPs that are part of router owned subnets.  We currently implement that
> in northd, by checking the router port subnets.  But we only have access
> to the real VIP value quite late, after template instantiation, in
> ovn-controller.
>
> So I decided to not support neighbor_responder=reachable for now.  For
> the use case I have in mind (node-port load balancers in ovn-kubernetes)
> it doesn't matter because the VIP is the node IP for which we already
> have ARP responder flows in place.  For the more generic case, it's up
> to the CMS to explicitly request ARP responses (neighbor_responder=all)
> or suppress all of them (neighbor_responder=none).
>
Make sense and I agree with you that it isn't worth the complexity.

Thanks,
Han

> >> +
> >> +static struct nbrec_load_balancer_health_check *
> >> +ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
> >> +                        const char *vip_port_str, bool template)
> >> +{
> >> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >> +
> >> +    if (!nbrec_lb->n_health_check) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) {
> >> +        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));
> >> +        return NULL;
> >> +    }
> >> +
> >> +    if (template) {
> >> +        VLOG_WARN_RL(&rl,
> >> +                     "Template load balancers do not currently
support "
> >> +                     "health checks. Not creating health checks for "
> >> +                     "load balancer " UUID_FMT,
> >> +                     UUID_ARGS(&nbrec_lb->header_.uuid));
> >> +        return NULL;
> >> +    }
> >> +
> >> +    for (size_t i = 0; i < nbrec_lb->n_health_check; i++) {
> >> +        if (!strcmp(nbrec_lb->health_check[i]->vip, vip_port_str)) {
> >> +            return nbrec_lb->health_check[i];
> >> +        }
> >> +    }
> >> +    return NULL;
> >> +}
> >> +
> >>  struct ovn_northd_lb *
> >>  ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> >>  {
> >>      bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
> >>      bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol,
"sctp");
> >>      struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> >> +    int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> >> +                                              "address-family",
"ipv4"),
> >> +                                 "ipv4")
> >> +                         ? AF_INET
> >> +                         : AF_INET6;
> >>
> >>      lb->nlb = nbrec_lb;
> >>      lb->proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
> >> @@ -216,12 +530,16 @@ ovn_northd_lb_create(const struct
> > nbrec_load_balancer *nbrec_lb)
> >>      lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips);
> >>      lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb);
> >>      lb->controller_event = smap_get_bool(&nbrec_lb->options, "event",
> > false);
> >> -    lb->routable = smap_get_bool(&nbrec_lb->options, "add_route",
false);
> >> +
> >> +    bool routable = smap_get_bool(&nbrec_lb->options, "add_route",
> > false);
> >> +    lb->routable = ovn_lb_get_routable_mode(nbrec_lb, routable,
> > lb->template);
> >> +
> >>      lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat",
> > false);
> >> -    const char *mode =
> >> -        smap_get_def(&nbrec_lb->options, "neighbor_responder",
> > "reachable");
> >> -    lb->neigh_mode = strcmp(mode, "all") ? LB_NEIGH_RESPOND_REACHABLE
> >> -                                         : LB_NEIGH_RESPOND_ALL;
> >> +    lb->template = smap_get_bool(&nbrec_lb->options, "template",
false);
> >> +
> >> +    const char *mode = smap_get(&nbrec_lb->options,
> > "neighbor_responder");
> >> +    lb->neigh_mode = ovn_lb_get_neigh_mode(nbrec_lb, mode,
lb->template);
> >> +
> >>      sset_init(&lb->ips_v4);
> >>      sset_init(&lb->ips_v6);
> >>      struct smap_node *node;
> >> @@ -231,13 +549,19 @@ ovn_northd_lb_create(const struct
> > nbrec_load_balancer *nbrec_lb)
> >>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
> >>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[n_vips];
> >>
> >> -        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> >> -                                                  "reject", false);
> >> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
> >> +        char *error = ovn_lb_vip_init(lb_vip, node->key, node->value,
> >> +                                      lb->template, address_family);
> >> +        if (error) {
> >> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> > 1);
> >> +            VLOG_WARN_RL(&rl, "Failed to initialize LB VIP: %s",
error);
> >> +            ovn_lb_vip_destroy(lb_vip);
> >> +            free(error);
> >>              continue;
> >>          }
> >> +        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> >> +                                                  "reject", false);
> >>          ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> >> -                               node->key, node->value);
> >> +                               node->key, node->value, lb->template);
> >>          if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >>              sset_add(&lb->ips_v4, lb_vip->vip_str);
> >>          } else {
> >> @@ -385,7 +709,8 @@ ovn_lb_group_find(const struct hmap *lb_groups,
const
> > struct uuid *uuid)
> >>  }
> >>
> >>  struct ovn_controller_lb *
> >> -ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb)
> >> +ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb,
> >> +                         const struct smap *template_vars)
> >>  {
> >>      struct ovn_controller_lb *lb = xzalloc(sizeof *lb);
> >>
> >> @@ -399,10 +724,20 @@ ovn_controller_lb_create(const struct
> > sbrec_load_balancer *sbrec_lb)
> >>      SMAP_FOR_EACH (node, &sbrec_lb->vips) {
> >>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
> >>
> >> -        if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
> >> -            continue;
> >> +        char *key_s = lexer_parse_template_string(xstrdup(node->key),
> >> +                                                  template_vars,
> >> +                                                  NULL);
> >> +        char *value_s =
lexer_parse_template_string(xstrdup(node->value),
> >> +                                                    template_vars,
> >> +                                                    NULL);
> >
> > Shouldn't we check the options:use_template before parsing?
> >
>
> That's better indeed!
>
> >> +        char *error = ovn_lb_vip_init_explicit(lb_vip, key_s,
value_s);
> >> +        if (error) {
> >> +            free(error);
> >> +        } else {
> >> +            n_vips++;
> >>          }
> >> -        n_vips++;
> >> +        free(key_s);
> >> +        free(value_s);
> >>      }
> >>
> >>      /* It's possible that parsing VIPs fails.  Update the lb->n_vips
to
> > the
> >> diff --git a/lib/lb.h b/lib/lb.h
> >> index 80ac03399..265a1215a 100644
> >> --- a/lib/lb.h
> >> +++ b/lib/lb.h
> >> @@ -35,6 +35,7 @@ struct uuid;
> >>  enum lb_neighbor_responder_mode {
> >>      LB_NEIGH_RESPOND_REACHABLE,
> >>      LB_NEIGH_RESPOND_ALL,
> >> +    LB_NEIGH_RESPOND_NONE,
> >>  };
> >>
> >>  /* The "routable" ssets are subsets of the load balancer IPs for
which IP
> >> @@ -67,6 +68,7 @@ struct ovn_northd_lb {
> >>      bool controller_event;
> >>      bool routable;
> >>      bool skip_snat;
> >> +    bool template;
> >>
> >>      struct sset ips_v4;
> >>      struct sset ips_v6;
> >> @@ -81,19 +83,31 @@ struct ovn_northd_lb {
> >>  };
> >>
> >>  struct ovn_lb_vip {
> >> -    struct in6_addr vip;
> >> -    char *vip_str;
> >> -    uint16_t vip_port;
> >> -
> >> +    struct in6_addr vip; /* Only used in ovn-controller. */
> >> +    char *vip_str;       /* Actual VIP string representation (without
> > port).
> >> +                          * To be used in ovn-northd.
> >> +                          */
> >> +    uint16_t vip_port;   /* Only used in ovn-controller. */
> >> +    char *port_str;      /* Actual port string representation.  To be
> > used
> >> +                          * in ovn-controller.
> >> +                          */
> >>      struct ovn_lb_backend *backends;
> >>      size_t n_backends;
> >>      bool empty_backend_rej;
> >> +    int address_family;
> >>  };
> >>
> >>  struct ovn_lb_backend {
> >> -    struct in6_addr ip;
> >> -    char *ip_str;
> >> -    uint16_t port;
> >> +    struct in6_addr ip;  /* Only used in ovn-controller. */
> >> +    char *ip_str;        /* Actual IP string representation. To be
used
> > in
> >> +                          * ovn-northd.
> >> +                          */
> >> +    uint16_t port;       /* Mostly used in ovn-controller but also for
> >> +                          * healthcheck in ovn-northd.
> >> +                          */
> >> +    char *port_str;      /* Actual port string representation. To be
used
> >> +                          * in ovn-northd.
> >> +                          */
> >>  };
> >>
> >>  /* ovn-northd specific backend information. */
> >> @@ -171,7 +185,16 @@ struct ovn_controller_lb {
> >>  };
> >>
> >>  struct ovn_controller_lb *ovn_controller_lb_create(
> >> -    const struct sbrec_load_balancer *);
> >> +    const struct sbrec_load_balancer *,
> >> +    const struct smap *template_vars);
> >>  void ovn_controller_lb_destroy(struct ovn_controller_lb *);
> >>
> >> +char *ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
> >> +                      const char *lb_value, bool template, int
> > address_family);
> >> +void ovn_lb_vip_destroy(struct ovn_lb_vip *vip);
> >> +void ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s,
> >> +                       bool template);
> >> +void ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct
ds
> > *s,
> >> +                                bool template);
> >> +
> >>  #endif /* OVN_LIB_LB_H 1 */
> >> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> >> index 596e93129..1cc4c98b1 100644
> >> --- a/lib/ovn-util.c
> >> +++ b/lib/ovn-util.c
> >> @@ -799,9 +799,6 @@ ip_address_and_port_from_lb_key(const char *key,
char
> > **ip_address,
> >>  {
> >>      struct sockaddr_storage ss;
> >>      if (!inet_parse_active(key, 0, &ss, false, NULL)) {
> >> -        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;
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index 63879ccd0..a93aee37e 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -3708,6 +3708,10 @@ static void
> >>  ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, struct
ovn_northd_lb
> > *lb,
> >>                    struct hmap *monitor_map, struct hmap *ports)
> >>  {
> >> +    if (lb->template) {
> >> +        return;
> >> +    }
> >> +
> >>      for (size_t i = 0; i < lb->n_vips; i++) {
> >>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
> >>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
> >> @@ -4024,12 +4028,19 @@ static void
> >>  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
> >>                                 const struct ovn_northd_lb *lb)
> >>  {
> >> +    /* If configured to not reply to any neighbor requests for all
VIPs
> >> +     * return early.
> >> +     */
> >> +    if (lb->neigh_mode == LB_NEIGH_RESPOND_NONE) {
> >> +        return;
> >> +    }
> >> +
> >>      /* If configured to reply to neighbor requests for all VIPs force
> > them
> >>       * all to be considered "reachable".
> >>       */
> >>      if (lb->neigh_mode == LB_NEIGH_RESPOND_ALL) {
> >>          for (size_t i = 0; i < lb->n_vips; i++) {
> >> -            if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
> >> +            if (lb->vips[i].address_family == AF_INET) {
> >>                  sset_add(&od->lb_ips->ips_v4_reachable,
> > lb->vips[i].vip_str);
> >>              } else {
> >>                  sset_add(&od->lb_ips->ips_v6_reachable,
> > lb->vips[i].vip_str);
> >> @@ -4041,8 +4052,9 @@ build_lrouter_lb_reachable_ips(struct
ovn_datapath
> > *od,
> >>      /* Otherwise, a VIP is reachable if there's at least one router
> >>       * subnet that includes it.
> >>       */
> >> +    ovs_assert(lb->neigh_mode == LB_NEIGH_RESPOND_REACHABLE);
> >>      for (size_t i = 0; i < lb->n_vips; i++) {
> >> -        if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) {
> >> +        if (lb->vips[i].address_family == AF_INET) {
> >>              ovs_be32 vip_ip4 =
> > in6_addr_get_mapped_ipv4(&lb->vips[i].vip);
> >>              struct ovn_port *op;
> >>
> >> @@ -5823,16 +5835,16 @@ build_empty_lb_event_flow(struct ovn_lb_vip
> > *lb_vip,
> >>      ds_clear(action);
> >>      ds_clear(match);
> >>
> >> -    bool ipv4 = IN6_IS_ADDR_V4MAPPED(&lb_vip->vip);
> >> +    bool ipv4 = lb_vip->address_family == AF_INET;
> >>
> >>      ds_put_format(match, "ip%s.dst == %s && %s",
> >>                    ipv4 ? "4": "6", lb_vip->vip_str, lb->proto);
> >>
> >>      char *vip = lb_vip->vip_str;
> >> -    if (lb_vip->vip_port) {
> >> -        ds_put_format(match, " && %s.dst == %u", lb->proto,
> > lb_vip->vip_port);
> >> -        vip = xasprintf("%s%s%s:%u", ipv4 ? "" : "[", lb_vip->vip_str,
> >> -                        ipv4 ? "" : "]", lb_vip->vip_port);
> >> +    if (lb_vip->port_str) {
> >> +        ds_put_format(match, " && %s.dst == %s", lb->proto,
> > lb_vip->port_str);
> >> +        vip = xasprintf("%s%s%s:%s", ipv4 ? "" : "[", lb_vip->vip_str,
> >> +                        ipv4 ? "" : "]", lb_vip->port_str);
> >>      }
> >>
> >>      ds_put_format(action,
> >> @@ -5843,7 +5855,7 @@ build_empty_lb_event_flow(struct ovn_lb_vip
*lb_vip,
> >>                    event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS),
> >>                    vip, lb->proto,
> >>                    UUID_ARGS(&lb->nlb->header_.uuid));
> >> -    if (lb_vip->vip_port) {
> >> +    if (lb_vip->port_str) {
> >>          free(vip);
> >>      }
> >>      return true;
> >> @@ -6899,7 +6911,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
> > struct ovn_northd_lb *lb,
> >>          /* Store the original destination IP to be used when
generating
> >>           * hairpin flows.
> >>           */
> >> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >> +        if (lb->vips[i].address_family == AF_INET) {
> >>              ip_match = "ip4";
> >>              ds_put_format(action, REG_ORIG_DIP_IPV4 " = %s; ",
> >>                            lb_vip->vip_str);
> >> @@ -6910,7 +6922,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
> > struct ovn_northd_lb *lb,
> >>          }
> >>
> >>          const char *proto = NULL;
> >> -        if (lb_vip->vip_port) {
> >> +        if (lb_vip->port_str) {
> >>              proto = "tcp";
> >>              if (lb->nlb->protocol) {
> >>                  if (!strcmp(lb->nlb->protocol, "udp")) {
> >> @@ -6923,8 +6935,8 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
> > struct ovn_northd_lb *lb,
> >>              /* Store the original destination port to be used when
> > generating
> >>               * hairpin flows.
> >>               */
> >> -            ds_put_format(action, REG_ORIG_TP_DPORT " = %"PRIu16"; ",
> >> -                          lb_vip->vip_port);
> >> +            ds_put_format(action, REG_ORIG_TP_DPORT " = %s; ",
> >> +                          lb_vip->port_str);
> >>          }
> >>          ds_put_format(action, "%s;", ct_lb_mark ? "ct_lb_mark" :
> > "ct_lb");
> >>
> >> @@ -6962,24 +6974,12 @@ build_lb_rules(struct hmap *lflows, struct
> > ovn_northd_lb *lb, bool ct_lb_mark,
> >>          struct ovn_lb_vip *lb_vip = &lb->vips[i];
> >>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[i];
> >>          const char *ip_match = NULL;
> >> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >> +        if (lb_vip->address_family == AF_INET) {
> >>              ip_match = "ip4";
> >>          } else {
> >>              ip_match = "ip6";
> >>          }
> >>
> >> -        const char *proto = NULL;
> >> -        if (lb_vip->vip_port) {
> >> -            proto = "tcp";
> >> -            if (lb->nlb->protocol) {
> >> -                if (!strcmp(lb->nlb->protocol, "udp")) {
> >> -                    proto = "udp";
> >> -                } else if (!strcmp(lb->nlb->protocol, "sctp")) {
> >> -                    proto = "sctp";
> >> -                }
> >> -            }
> >> -        }
> >> -
> >>          ds_clear(action);
> >>          ds_clear(match);
> >>
> >> @@ -6997,8 +6997,9 @@ build_lb_rules(struct hmap *lflows, struct
> > ovn_northd_lb *lb, bool ct_lb_mark,
> >>          ds_put_format(match, "ct.new && %s.dst == %s", ip_match,
> >>                        lb_vip->vip_str);
> >>          int priority = 110;
> >> -        if (lb_vip->vip_port) {
> >> -            ds_put_format(match, " && %s.dst == %d", proto,
> > lb_vip->vip_port);
> >> +        if (lb_vip->port_str) {
> >> +            ds_put_format(match, " && %s.dst == %s", lb->proto,
> >> +                          lb_vip->port_str);
> >>              priority = 120;
> >>          }
> >>
> >> @@ -9998,7 +9999,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> > *lb_vip,
> >>       * of "ct_lb_mark($targets);". The other flow is for ct.est with
> >>       * an action of "next;".
> >>       */
> >> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >> +    if (lb_vip->address_family == AF_INET) {
> >>          ds_put_format(match, "ip4 && "REG_NEXT_HOP_IPV4" == %s",
> >>                        lb_vip->vip_str);
> >>      } else {
> >> @@ -10014,14 +10015,14 @@ build_lrouter_nat_flows_for_lb(struct
> > ovn_lb_vip *lb_vip,
> >>      }
> >>
> >>      int prio = 110;
> >> -    if (lb_vip->vip_port) {
> >> +    if (lb_vip->port_str) {
> >>          prio = 120;
> >>          new_match = xasprintf("ct.new && %s && %s && "
> >> -                              REG_ORIG_TP_DPORT_ROUTER" == %d",
> >> -                              ds_cstr(match), lb->proto,
> > lb_vip->vip_port);
> >> +                              REG_ORIG_TP_DPORT_ROUTER" == %s",
> >> +                              ds_cstr(match), lb->proto,
> > lb_vip->port_str);
> >>          est_match = xasprintf("ct.est && %s && %s && "
> >> -                              REG_ORIG_TP_DPORT_ROUTER" == %d && %s ==
> > 1",
> >> -                              ds_cstr(match), lb->proto,
> > lb_vip->vip_port,
> >> +                              REG_ORIG_TP_DPORT_ROUTER" == %s && %s ==
> > 1",
> >> +                              ds_cstr(match), lb->proto,
> > lb_vip->port_str,
> >>                                ct_natted);
> >>      } else {
> >>          new_match = xasprintf("ct.new && %s", ds_cstr(match));
> >> @@ -10030,7 +10031,7 @@ build_lrouter_nat_flows_for_lb(struct
ovn_lb_vip
> > *lb_vip,
> >>      }
> >>
> >>      const char *ip_match = NULL;
> >> -    if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >> +    if (lb_vip->address_family == AF_INET) {
> >>          ip_match = "ip4";
> >>      } else {
> >>          ip_match = "ip6";
> >> @@ -10048,9 +10049,9 @@ build_lrouter_nat_flows_for_lb(struct
ovn_lb_vip
> > *lb_vip,
> >>          ds_put_format(&undnat_match, "(%s.src == %s", ip_match,
> >>                        backend->ip_str);
> >>
> >> -        if (backend->port) {
> >> -            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> >> -                          lb->proto, backend->port);
> >> +        if (backend->port_str) {
> >> +            ds_put_format(&undnat_match, " && %s.src == %s) || ",
> >> +                          lb->proto, backend->port_str);
> >>          } else {
> >>              ds_put_cstr(&undnat_match, ") || ");
> >>          }
> >> @@ -10063,9 +10064,9 @@ build_lrouter_nat_flows_for_lb(struct
ovn_lb_vip
> > *lb_vip,
> >>      struct ds unsnat_match = DS_EMPTY_INITIALIZER;
> >>      ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
> >>                    ip_match, ip_match, lb_vip->vip_str, lb->proto);
> >> -    if (lb_vip->vip_port) {
> >> -        ds_put_format(&unsnat_match, " && %s.dst == %d", lb->proto,
> >> -                      lb_vip->vip_port);
> >> +    if (lb_vip->port_str) {
> >> +        ds_put_format(&unsnat_match, " && %s.dst == %s", lb->proto,
> >> +                      lb_vip->port_str);
> >>      }
> >>
> >>      struct ovn_datapath **gw_router_skip_snat =
> >> @@ -10305,7 +10306,7 @@ build_lrouter_defrag_flows_for_lb(struct
> > ovn_northd_lb *lb,
> >>          ds_clear(&defrag_actions);
> >>          ds_clear(match);
> >>
> >> -        if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >> +        if (lb_vip->address_family == AF_INET) {
> >>              ds_put_format(match, "ip && ip4.dst == %s",
lb_vip->vip_str);
> >>              ds_put_format(&defrag_actions, REG_NEXT_HOP_IPV4" = %s; ",
> >>                            lb_vip->vip_str);
> >> @@ -10315,7 +10316,7 @@ build_lrouter_defrag_flows_for_lb(struct
> > ovn_northd_lb *lb,
> >>                            lb_vip->vip_str);
> >>          }
> >>
> >> -        if (lb_vip->vip_port) {
> >> +        if (lb_vip->port_str) {
> >>              ds_put_format(match, " && %s", lb->proto);
> >>              prio = 110;
> >>
> >> diff --git a/ovn-nb.xml b/ovn-nb.xml
> >> index bd28ac372..fb63b212e 100644
> >> --- a/ovn-nb.xml
> >> +++ b/ovn-nb.xml
> >> @@ -1908,6 +1908,18 @@
> >>          requests only for VIPs that are part of a router's subnet.
The
> > default
> >>          value of this option, if not specified, is
> > <code>reachable</code>.
> >>        </column>
> >> +
> >> +      <column name="options" key="template">
> >> +        Option to be set to <code>true</code>, if the load balancer
is a
> >> +        template.  In this case the template variables used within the
> >> +        load balancer VIPs and backends will be instantiated
differently
> >> +        on different chassis.
> >
> > It would be better to mention what are template variables, with a
reference
> > to the chassis_template_var table. An simple example would be even
better.
> >
>
> Sure, I'll do that.
>
> >> +      </column>
> >> +
> >> +      <column name="options" key="address-family">
> >> +        Address family used by the load balancer.  This value is used
> > only
> >> +        when the load balancer is a template.
> >
> > It would be better to state the keywords "ipv4" & "ipv6", and point out
> > that this is *required* if and only if the options:template is "true".
> >
>
> Better indeed, I'll take care of it.
>
> > Thanks,
> > Han
> >
>
> Thanks,
> Dumitru
>
> >> +      </column>
> >>      </group>
> >>    </table>
> >>
> >> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> >> index 6a5d52a5d..9b766d63e 100644
> >> --- a/tests/ovn-nbctl.at
> >> +++ b/tests/ovn-nbctl.at
> >> @@ -857,23 +857,19 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
> > 30.0.0.10 192.168.10.10:a80], [
> >>  [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
> >>  ])
> >>
> >> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10
192.168.10.10:],
> > [1], [],
> >> -[ovn-nbctl: 192.168.10.10:: should be an IP address.
> >> -])
> >> -
> >>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10
> > 192.168.10.1a], [1], [],
> >>  [ovn-nbctl: 192.168.10.1a: should be an IP address.
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,
> > 192.168.10.20:80 tcp], [1], [],
> >> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >> +[ovn-nbctl: 192.168.10.10:80: should be an IP address,
192.168.10.20:80:
> > should be an IP address.
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
> >>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >>  ])
> >>
> >> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:900 tcp], [1],
[],
> >> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
> >>  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >>  ])
> >>
> >> @@ -1111,7 +1107,7 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0
> > ae0f::10fff [[fd0f::10]]:80,fd0
> >>
> >>
> >>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
> > [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> >> -[ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
> >> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80:
> > should be an IP address.
> >>  ])
> >>
> >>
> >> @@ -1125,18 +1121,13 @@ AT_CHECK([ovn-nbctl -vsocket_util:off lb-add
lb0
> > ae0f::10 [[fd0f::10]]:a80], [1]
> >>  ])
> >>
> >>
> >> -AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
> > [[fd0f::10]]:], [1], [],
> >> -[ovn-nbctl: [[fd0f::10]]:: should be an IP address.
> >> -])
> >> -
> >> -
> >>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10
fd0f::1001a],
> > [1], [],
> >>  [ovn-nbctl: fd0f::1001a: should be an IP address.
> >>  ])
> >>
> >>
> >>  AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:
> > [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >> +[ovn-nbctl: [[fd0f::10]]:80: should be an IP address, [[fd0f::20]]:80:
> > should be an IP address.
> >>  ])
> >>
> >>
> >> @@ -1146,7 +1137,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10
> > tcp], [1], [],
> >>
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:900 tcp], [1],
[],
> >> -[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >> +[ovn-nbctl: [[fd0f::10]]:900: should be an IP address.
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 192.168.10.10], [1], [],
> >> @@ -1158,7 +1149,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10
> > 192.168.10.10], [1], [],
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 192.168.10.10:80], [1],
> > [],
> >> -[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP
> > [[ae0f::10]]:80.
> >> +[ovn-nbctl: 192.168.10.10:80: IP address family is different from VIP
> > ae0f::10.
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 ae0f::10], [1], [],
> >> @@ -1166,7 +1157,7 @@ AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10
ae0f::10],
> > [1], [],
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 [[ae0f::10]]:80], [1], [],
> >> -[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP
> > 30.0.0.10:80.
> >> +[ovn-nbctl: [[ae0f::10]]:80: IP address family is different from VIP
> > 30.0.0.10.
> >>  ])
> >>
> >>  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10])
> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >> index b6843956c..39032d269 100644
> >> --- a/tests/ovn-northd.at
> >> +++ b/tests/ovn-northd.at
> >> @@ -1828,6 +1828,11 @@ ovn-nbctl set Load_Balancer lb8
> > options:neighbor_responder=all
> >>  ovn-nbctl lb-add lb9 "[[4444::4444]]:8080" "[[10::10]]:8080" udp
> >>  ovn-nbctl set Load_Balancer lb9 options:neighbor_responder=all
> >>
> >> +ovn-nbctl lb-add lb10 "55.55.55.55:8080" "10.0.0.8:8080" udp
> >> +ovn-nbctl set Load_Balancer lb10 options:neighbor_responder=none
> >> +ovn-nbctl lb-add lb11 "[[5555::5555]]:8080" "[[10::10]]:8080" udp
> >> +ovn-nbctl set Load_Balancer lb11 options:neighbor_responder=none
> >> +
> >>  ovn-nbctl lr-lb-add lr lb1
> >>  ovn-nbctl lr-lb-add lr lb2
> >>  ovn-nbctl lr-lb-add lr lb3
> >> @@ -1837,6 +1842,8 @@ ovn-nbctl lr-lb-add lr lb6
> >>  ovn-nbctl lr-lb-add lr lb7
> >>  ovn-nbctl lr-lb-add lr lb8
> >>  ovn-nbctl lr-lb-add lr lb9
> >> +ovn-nbctl lr-lb-add lr lb10
> >> +ovn-nbctl lr-lb-add lr lb11
> >>
> >>  ovn-nbctl --wait=sb sync
> >>  lr_key=$(fetch_column sb:datapath_binding tunnel_key
> > external_ids:name=lr)
> >> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> >> index 811468dc6..4feae1db2 100644
> >> --- a/utilities/ovn-nbctl.c
> >> +++ b/utilities/ovn-nbctl.c
> >> @@ -28,6 +28,7 @@
> >>  #include "openvswitch/json.h"
> >>  #include "lib/acl-log.h"
> >>  #include "lib/copp.h"
> >> +#include "lib/lb.h"
> >>  #include "lib/ovn-nb-idl.h"
> >>  #include "lib/ovn-util.h"
> >>  #include "memory.h"
> >> @@ -2837,6 +2838,7 @@ nbctl_lb_add(struct ctl_context *ctx)
> >>      bool empty_backend_rej = shash_find(&ctx->options, "--reject") !=
> > NULL;
> >>      bool empty_backend_event = shash_find(&ctx->options, "--event") !=
> > NULL;
> >>      bool add_route = shash_find(&ctx->options, "--add-route") != NULL;
> >> +    bool template = shash_find(&ctx->options, "--template") != NULL;
> >>
> >>      if (empty_backend_event && empty_backend_rej) {
> >>              ctl_error(ctx,
> >> @@ -2844,10 +2846,12 @@ nbctl_lb_add(struct ctl_context *ctx)
> >>              return;
> >>      }
> >>
> >> +    const char *lb_address_family_str = "ipv4";
> >> +    int lb_address_family = AF_INET;
> >>      const char *lb_proto;
> >>      bool is_update_proto = false;
> >>
> >> -    if (ctx->argc == 4) {
> >> +    if (ctx->argc <= 4) {
> >>          /* Default protocol. */
> >>          lb_proto = "tcp";
> >>      } else {
> >> @@ -2863,79 +2867,61 @@ nbctl_lb_add(struct ctl_context *ctx)
> >>          }
> >>      }
> >>
> >> -    struct sockaddr_storage ss_vip;
> >> -    if (!inet_parse_active(lb_vip, 0, &ss_vip, false, NULL)) {
> >> -        ctl_error(ctx, "%s: should be an IP address (or an IP address
"
> >> -                  "and a port number with : as a separator).",
lb_vip);
> >> -        return;
> >> -    }
> >> -
> >> -    struct ds lb_vip_normalized_ds = DS_EMPTY_INITIALIZER;
> >> -    uint16_t lb_vip_port = ss_get_port(&ss_vip);
> >> -    if (lb_vip_port) {
> >> -        ss_format_address(&ss_vip, &lb_vip_normalized_ds);
> >> -        ds_put_format(&lb_vip_normalized_ds, ":%d", lb_vip_port);
> >> -    } else {
> >> -        ss_format_address_nobracks(&ss_vip, &lb_vip_normalized_ds);
> >> -    }
> >> -    const char *lb_vip_normalized = ds_cstr(&lb_vip_normalized_ds);
> >> +    if (ctx->argc > 5) {
> >> +        lb_address_family_str = ctx->argv[5];
> >> +        lb_address_family = !strcmp(lb_address_family_str, "ipv4")
> >> +                            ? AF_INET : AF_INET6;
> >>
> >> -    if (!lb_vip_port && is_update_proto) {
> >> -        ds_destroy(&lb_vip_normalized_ds);
> >> -        ctl_error(ctx, "Protocol is unnecessary when no port of vip "
> >> -                  "is given.");
> >> -        return;
> >>      }
> >>
> >> -    char *token = NULL, *save_ptr = NULL;
> >> +    struct ds lb_vip_normalized = DS_EMPTY_INITIALIZER;
> >>      struct ds lb_ips_new = DS_EMPTY_INITIALIZER;
> >> -    for (token = strtok_r(lb_ips, ",", &save_ptr);
> >> -            token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
> >> -        struct sockaddr_storage ss_dst;
> >> +    struct ovn_lb_vip lb_vip_parsed;
> >>
> >> -        if (lb_vip_port) {
> >> -            if (!inet_parse_active(token, -1, &ss_dst, false, NULL)) {
> >> -                ctl_error(ctx, "%s: should be an IP address and a
port "
> >> -                          "number with : as a separator.", token);
> >> -                goto out;
> >> -            }
> >> -        } else {
> >> -            if (!inet_parse_address(token, &ss_dst)) {
> >> -                ctl_error(ctx, "%s: should be an IP address.", token);
> >> -                goto out;
> >> -            }
> >> -        }
> >> +    char *error = ovn_lb_vip_init(&lb_vip_parsed, lb_vip, lb_ips,
> > template,
> >> +                                  lb_address_family);
> >> +    if (error) {
> >> +        ctl_error(ctx, "%s", error);
> >> +        ovn_lb_vip_destroy(&lb_vip_parsed);
> >> +        free(error);
> >> +        return;
> >> +    }
> >>
> >> -        if (ss_vip.ss_family != ss_dst.ss_family) {
> >> -            ctl_error(ctx, "%s: IP address family is different from
VIP
> > %s.",
> >> -                      token, lb_vip_normalized);
> >> -            goto out;
> >> -        }
> >> -        ds_put_format(&lb_ips_new, "%s%s",
> >> -                lb_ips_new.length ? "," : "", token);
> >> +    if (is_update_proto && !lb_vip_parsed.port_str) {
> >> +        ctl_error(ctx, "Protocol is unnecessary when no port of vip
is "
> >> +                       "given.");
> >> +        ovn_lb_vip_destroy(&lb_vip_parsed);
> >> +        return;
> >>      }
> >>
> >> +    ovn_lb_vip_format(&lb_vip_parsed, &lb_vip_normalized, template);
> >> +    ovn_lb_vip_backends_format(&lb_vip_parsed, &lb_ips_new, template);
> >> +    ovn_lb_vip_destroy(&lb_vip_parsed);
> >> +
> >>      const struct nbrec_load_balancer *lb = NULL;
> >>      if (!add_duplicate) {
> >> -        char *error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
> >> +        error = lb_by_name_or_uuid(ctx, lb_name, false, &lb);
> >>          if (error) {
> >>              ctx->error = error;
> >>              goto out;
> >>          }
> >>          if (lb) {
> >> -            if (smap_get(&lb->vips, lb_vip_normalized)) {
> >> +            if (smap_get(&lb->vips, ds_cstr(&lb_vip_normalized))) {
> >>                  if (!may_exist) {
> >>                      ctl_error(ctx, "%s: a load balancer with this vip
> > (%s) "
> >> -                              "already exists", lb_name,
> > lb_vip_normalized);
> >> +                              "already exists", lb_name,
> >> +                              ds_cstr(&lb_vip_normalized));
> >>                      goto out;
> >>                  }
> >>                  /* Update the vips. */
> >>                  smap_replace(CONST_CAST(struct smap *, &lb->vips),
> >> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
> >> +                             ds_cstr(&lb_vip_normalized),
> >> +                             ds_cstr(&lb_ips_new));
> >>              } else {
> >>                  /* Add the new vips. */
> >>                  smap_add(CONST_CAST(struct smap *, &lb->vips),
> >> -                        lb_vip_normalized, ds_cstr(&lb_ips_new));
> >> +                         ds_cstr(&lb_vip_normalized),
> >> +                         ds_cstr(&lb_ips_new));
> >>              }
> >>
> >>              /* Update the load balancer. */
> >> @@ -2954,7 +2940,7 @@ nbctl_lb_add(struct ctl_context *ctx)
> >>      nbrec_load_balancer_set_name(lb, lb_name);
> >>      nbrec_load_balancer_set_protocol(lb, lb_proto);
> >>      smap_add(CONST_CAST(struct smap *, &lb->vips),
> >> -            lb_vip_normalized, ds_cstr(&lb_ips_new));
> >> +             ds_cstr(&lb_vip_normalized), ds_cstr(&lb_ips_new));
> >>      nbrec_load_balancer_set_vips(lb, &lb->vips);
> >>      struct smap options = SMAP_INITIALIZER(&options);
> >>      if (empty_backend_rej) {
> >> @@ -2966,12 +2952,16 @@ nbctl_lb_add(struct ctl_context *ctx)
> >>      if (add_route) {
> >>          smap_add(&options, "add_route", "true");
> >>      }
> >> +    if (template) {
> >> +        smap_add(&options, "template", "true");
> >> +        smap_add(&options, "address-family", lb_address_family_str);
> >> +    }
> >>      nbrec_load_balancer_set_options(lb, &options);
> >>      smap_destroy(&options);
> >>  out:
> >>      ds_destroy(&lb_ips_new);
> >>
> >> -    ds_destroy(&lb_vip_normalized_ds);
> >> +    ds_destroy(&lb_vip_normalized);
> >>  }
> >>
> >>  static void
> >> @@ -3025,6 +3015,7 @@ static void
> >>  nbctl_pre_lb_list(struct ctl_context *ctx)
> >>  {
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> >> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
> >>  }
> >> @@ -3033,6 +3024,7 @@ static void
> >>  lb_info_add_smap(const struct nbrec_load_balancer *lb,
> >>                   struct smap *lbs, int vip_width)
> >>  {
> >> +    bool template = smap_get_bool(&lb->options, "template", false);
> >>      const struct smap_node **nodes = smap_sort(&lb->vips);
> >>      if (!nodes) {
> >>          return;
> >> @@ -3041,13 +3033,24 @@ lb_info_add_smap(const struct
nbrec_load_balancer
> > *lb,
> >>      struct ds val = DS_EMPTY_INITIALIZER;
> >>      for (size_t i = 0; i < smap_count(&lb->vips); i++) {
> >>          const struct smap_node *node = nodes[i];
> >> +        const char *protocol = lb->protocol;
> >>
> >> -        struct sockaddr_storage ss;
> >> -        if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
> >> -            continue;
> >> +        if (!template) {
> >> +            struct sockaddr_storage ss;
> >> +            if (!inet_parse_active(node->key, 0, &ss, false, NULL)) {
> >> +                continue;
> >> +            }
> >> +            protocol = ss_get_port(&ss) ? lb->protocol : "";
> >> +        } else {
> >> +            if (!lb->protocol) {
> >> +                VLOG_WARN("Load Balancer "UUID_FMT" (%s) is a template
> > and "
> >> +                          "misses protocol",
> > UUID_ARGS(&lb->header_.uuid),
> >> +                          lb->name);
> >> +                continue;
> >> +            }
> >> +            protocol = lb->protocol;
> >>          }
> >>
> >> -        char *protocol = ss_get_port(&ss) ? lb->protocol : "";
> >>          if (i == 0) {
> >>              ds_put_format(&val, UUID_FMT "
 %-20.16s%-11.7s%-*.*s%s",
> >>                            UUID_ARGS(&lb->header_.uuid),
> >> @@ -3239,6 +3242,7 @@ nbctl_pre_lr_lb_list(struct ctl_context *ctx)
> >>
&nbrec_logical_router_col_load_balancer_group);
> >>
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> >> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
> >>
> >> @@ -3402,6 +3406,7 @@ nbctl_pre_ls_lb_list(struct ctl_context *ctx)
> >>
&nbrec_logical_switch_col_load_balancer_group);
> >>
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_name);
> >> +    ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_options);
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_protocol);
> >>      ovsdb_idl_add_column(ctx->idl, &nbrec_load_balancer_col_vips);
> >>
> >> @@ -7469,9 +7474,10 @@ static const struct ctl_command_syntax
> > nbctl_commands[] = {
> >>        nbctl_pre_lr_nat_set_ext_ips, nbctl_lr_nat_set_ext_ips,
> >>        NULL, "--is-exempted", RW},
> >>      /* load balancer commands. */
> >> -    { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]",
> >> +    { "lb-add", 3, 5, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]
> > [ADDRESS_FAMILY]",
> >>        nbctl_pre_lb_add, nbctl_lb_add, NULL,
> >> -      "--may-exist,--add-duplicate,--reject,--event,--add-route", RW
},
> >> +
> >  "--may-exist,--add-duplicate,--reject,--event,--add-route,--template",
> >> +      RW },
> >>      { "lb-del", 1, 2, "LB [VIP]", nbctl_pre_lb_del, nbctl_lb_del,
NULL,
> >>          "--if-exists", RW },
> >>      { "lb-list", 0, 1, "[LB]", nbctl_pre_lb_list, nbctl_lb_list, NULL,
> > "", RO },
> >>
> >
>