On Fri, Oct 21, 2022 at 1:00 AM Dumitru Ceara <[email protected]> wrote:
>
> On 10/21/22 07:47, Han Zhou wrote:
> > On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <[email protected]> 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 <[email protected]>
> >
> > 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 },
> >>
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
