[ovs-dev] [PATCH ovn v3] Add SCTP support to load balancers.

Numan Siddique nusiddiq at redhat.com
Mon Mar 30 08:26:21 UTC 2020


On Sat, Mar 28, 2020 at 12:54 AM Mark Michelson <mmichels at redhat.com> wrote:
>
> This allows for load balancers to use SCTP as a supported protocol in
> addition to the already-supported UDP and TCP.
>
> With this patch, health checks are not supported for SCTP load
> balancers. A test has been added to ensure that this is the case. Health
> checks should be added for SCTP load balancers in the near future. When
> that's done, the existing test can be updated to ensure that the SCTP
> health check works properly.
>
> Signed-off-by: Mark Michelson <mmichels at redhat.com>


Acked-by: Numan Siddique <numans at ovn.org>

The only thing which is missing is the system-ovn tests for this.
I think the file tests/test-l7.py can be enhanced to add support for sctp.
We can probably use pysctp for this. It's worth exploring this as a
follow up patch.

Thanks
Numan

> ---
> v2 -> v3
> Rebased. Now includes Numan's CT fix for NAT+LB
>
> v1 -> v2
> * Fixed checkpatch complaints
> * Fixed NULL pointer dereference in ovn-northd causing system tests to
>   fail
> * Fixed broken undnat flow for sctp
> ---
>
>  lib/actions.c         |   8 +--
>  northd/ovn-northd.c   |  55 +++++++++++--------
>  ovn-nb.ovsschema      |   6 +--
>  ovn-nb.xml            |  10 ++--
>  tests/ovn.at          | 121 +++++++++++++++++++++++++++++++++++++++++-
>  utilities/ovn-nbctl.c |   8 +--
>  6 files changed, 170 insertions(+), 38 deletions(-)
>
> diff --git a/lib/actions.c b/lib/actions.c
> index f22acddff..6351db765 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1957,10 +1957,12 @@ validate_empty_lb_backends(struct action_context *ctx,
>              }
>              break;
>          case EMPTY_LB_PROTOCOL:
> -            if (strcmp(c->string, "tcp") && strcmp(c->string, "udp")) {
> +            if (strcmp(c->string, "tcp") &&
> +                strcmp(c->string, "udp") &&
> +                strcmp(c->string, "sctp")) {
>                  lexer_error(ctx->lexer,
> -                    "Load balancer protocol '%s' is not 'tcp' or 'udp'",
> -                    c->string);
> +                    "Load balancer protocol '%s' is not 'tcp', 'udp', "
> +                    "or 'sctp'", c->string);
>                  return;
>              }
>              break;
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 7ef049310..076278197 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3177,10 +3177,21 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs,
>          lb->vips[n_vips].backend_ips = xstrdup(node->value);
>
>          struct nbrec_load_balancer_health_check *lb_health_check = NULL;
> -        for (size_t i = 0; i < nbrec_lb->n_health_check; i++) {
> -            if (!strcmp(nbrec_lb->health_check[i]->vip, node->key)) {
> -                lb_health_check = nbrec_lb->health_check[i];
> -                break;
> +        if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) {
> +            if (nbrec_lb->n_health_check > 0) {
> +                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +                VLOG_WARN_RL(&rl,
> +                             "SCTP load balancers do not currently support "
> +                             "health checks. Not creating health checks for "
> +                             "load balancer " UUID_FMT,
> +                             UUID_ARGS(&nbrec_lb->header_.uuid));
> +            }
> +        } else {
> +            for (size_t i = 0; i < nbrec_lb->n_health_check; i++) {
> +                if (!strcmp(nbrec_lb->health_check[i]->vip, node->key)) {
> +                    lb_health_check = nbrec_lb->health_check[i];
> +                    break;
> +                }
>              }
>          }
>
> @@ -5562,10 +5573,13 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb)
>
>          const char *proto = NULL;
>          if (lb_vip->vip_port) {
> -            if (lb->nlb->protocol && !strcmp(lb->nlb->protocol, "udp")) {
> -                proto = "udp";
> -            } else {
> -                proto = "tcp";
> +            proto = "tcp";
> +            if (lb->nlb->protocol) {
> +                if (!strcmp(lb->nlb->protocol, "udp")) {
> +                    proto = "udp";
> +                } else if (!strcmp(lb->nlb->protocol, "sctp")) {
> +                    proto = "sctp";
> +                }
>              }
>          }
>
> @@ -7573,7 +7587,7 @@ static void
>  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>                     struct ds *match, struct ds *actions, int priority,
>                     const char *lb_force_snat_ip, struct lb_vip *lb_vip,
> -                   bool is_udp, struct nbrec_load_balancer *lb,
> +                   const char *proto, struct nbrec_load_balancer *lb,
>                     struct shash *meter_groups, struct sset *nat_entries)
>  {
>      build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT,
> @@ -7628,11 +7642,10 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>           * S_ROUTER_IN_DNAT stage. */
>          struct ds unsnat_match = DS_EMPTY_INITIALIZER;
>          ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
> -                      ip_match, ip_match, lb_vip->vip,
> -                      is_udp ? "udp" : "tcp");
> +                      ip_match, ip_match, lb_vip->vip, proto);
>          if (lb_vip->vip_port) {
> -            ds_put_format(&unsnat_match, " && %s.dst == %d",
> -                          is_udp ? "udp" : "tcp", lb_vip->vip_port);
> +            ds_put_format(&unsnat_match, " && %s.dst == %d", proto,
> +                          lb_vip->vip_port);
>          }
>
>          ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120,
> @@ -7658,7 +7671,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>
>          if (backend->port) {
>              ds_put_format(&undnat_match, " && %s.src == %d) || ",
> -                          is_udp ? "udp" : "tcp", backend->port);
> +                          proto, backend->port);
>          } else {
>              ds_put_cstr(&undnat_match, ") || ");
>          }
> @@ -9207,15 +9220,13 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>
>                  int prio = 110;
>                  bool is_udp = nullable_string_is_equal(nb_lb->protocol, "udp");
> +                bool is_sctp = nullable_string_is_equal(nb_lb->protocol,
> +                                                        "sctp");
> +                const char *proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
>
>                  if (lb_vip->vip_port) {
> -                    if (is_udp) {
> -                        ds_put_format(&match, " && udp && udp.dst == %d",
> -                                      lb_vip->vip_port);
> -                    } else {
> -                        ds_put_format(&match, " && tcp && tcp.dst == %d",
> -                                      lb_vip->vip_port);
> -                    }
> +                    ds_put_format(&match, " && %s && %s.dst == %d", proto,
> +                                  proto, lb_vip->vip_port);
>                      prio = 120;
>                  }
>
> @@ -9224,7 +9235,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                                    od->l3redirect_port->json_key);
>                  }
>                  add_router_lb_flow(lflows, od, &match, &actions, prio,
> -                                   lb_force_snat_ip, lb_vip, is_udp,
> +                                   lb_force_snat_ip, lb_vip, proto,
>                                     nb_lb, meter_groups, &nat_entries);
>              }
>          }
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 843e979db..ea6f4e354 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.20.0",
> -    "cksum": "2846067333 25243",
> +    "version": "5.20.1",
> +    "cksum": "721375950 25251",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -168,7 +168,7 @@
>                               "min": 0, "max": "unlimited"}},
>                  "protocol": {
>                      "type": {"key": {"type": "string",
> -                             "enum": ["set", ["tcp", "udp"]]},
> +                             "enum": ["set", ["tcp", "udp", "sctp"]]},
>                               "min": 0, "max": 1}},
>                  "health_check": {"type": {
>                      "key": {"type": "uuid",
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index f4ecc1c1e..541ec20c1 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1474,11 +1474,11 @@
>
>      <column name="protocol">
>        <p>
> -        Valid protocols are <code>tcp</code> or <code>udp</code>.  This column
> -        is useful when a port number is provided as part of the
> -        <code>vips</code> column.  If this column is empty and a port number
> -        is provided as part of <code>vips</code> column, OVN assumes the
> -        protocol to be <code>tcp</code>.
> +        Valid protocols are <code>tcp</code>, <code>udp</code>, or
> +        <code>sctp</code>.  This column is useful when a port number is
> +        provided as part of the <code>vips</code> column.  If this column is
> +        empty and a port number is provided as part of <code>vips</code>
> +        column, OVN assumes the protocol to be <code>tcp</code>.
>        </p>
>      </column>
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9a44f0a6f..81d649603 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1423,8 +1423,8 @@ trigger_event(event = "empty_lb_backends", meter="event-elb" vip = "10.0.0.1:80"
>      encodes as controller(userdata=00.00.00.0f.00.00.00.00.00.00.00.00.00.01.00.0b.31.30.2e.30.2e.30.2e.31.3a.38.30.00.02.00.03.74.63.70.00.03.00.24.31.32.33.34.35.36.37.38.2d.61.62.63.64.2d.39.38.37.36.2d.66.65.64.63.2d.31.31.31.31.39.66.38.65.37.64.36.63,meter_id=5)
>
>  # Testing invalid vip results in extra error messages from socket-util.c
> -trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "sctp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
> -    Load balancer protocol 'sctp' is not 'tcp' or 'udp'
> +trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "aarp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
> +    Load balancer protocol 'aarp' is not 'tcp', 'udp', or 'sctp'
>  trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "tcp", load_balancer = "bacon");
>      Load balancer 'bacon' is not a UUID
>
> @@ -17876,6 +17876,123 @@ AT_CHECK([cat lflows.txt], [0], [dnl
>  OVN_CLEANUP([hv1], [hv2])
>  AT_CLEANUP
>
> +AT_SETUP([ovn -- SCTP Load balancer health checks])
> +AT_KEYWORDS([lb sctp])
> +
> +# Currently this test just ensures that no service monitors get created when
> +# An SCTP load balancer is configured to use health checks. Once SCTP load
> +# balancers are modified to allow health checks, this test should be altered
> +# to ensure the health check succeeds.
> +
> +ovn_start
> +
> +# Set up same network as previous health check test. As long as health checks
> +# aren't allowed for SCTP load balancers, the network will not be used for
> +# much. However, having the network in place will make it easy to alter when
> +# health checks are allowed.
> +
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> +    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> +    options:tx_pcap=hv1/vif1-tx.pcap \
> +    options:rxq_pcap=hv1/vif1-rx.pcap \
> +    ofport-request=1
> +ovs-vsctl -- add-port br-int hv1-vif2 -- \
> +    set interface hv1-vif2 external-ids:iface-id=sw0-p2 \
> +    options:tx_pcap=hv1/vif2-tx.pcap \
> +    options:rxq_pcap=hv1/vif2-rx.pcap \
> +    ofport-request=2
> +
> +sim_add hv2
> +as hv2
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.2
> +ovs-vsctl -- add-port br-int hv2-vif1 -- \
> +    set interface hv2-vif1 external-ids:iface-id=sw1-p1 \
> +    options:tx_pcap=hv2/vif1-tx.pcap \
> +    options:rxq_pcap=hv2/vif1-rx.pcap \
> +    ofport-request=1
> +
> +ovn-nbctl ls-add sw0
> +
> +ovn-nbctl lsp-add sw0 sw0-p1
> +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> +
> +ovn-nbctl lsp-add sw0 sw0-p2
> +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> +
> +# Create the second logical switch with one port
> +ovn-nbctl ls-add sw1
> +ovn-nbctl lsp-add sw1 sw1-p1
> +ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3"
> +ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3"
> +
> +# Create a logical router and attach both logical switches
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> +ovn-nbctl lsp-add sw0 sw0-lr0
> +ovn-nbctl lsp-set-type sw0-lr0 router
> +ovn-nbctl lsp-set-addresses sw0-lr0 router
> +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
> +
> +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24
> +ovn-nbctl lsp-add sw1 sw1-lr0
> +ovn-nbctl lsp-set-type sw1-lr0 router
> +ovn-nbctl lsp-set-addresses sw1-lr0 router
> +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
> +
> +ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80 sctp
> +
> +ovn-nbctl --wait=sb set load_balancer . ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> +ovn-nbctl --wait=sb set load_balancer . ip_port_mappings:20.0.0.3=sw1-p1:10.0.0.2
> +
> +ovn-nbctl --wait=sb -- --id=@hc create \
> +Load_Balancer_Health_Check vip="10.0.0.10\:80" -- add Load_Balancer . \
> +health_check @hc
> +
> +ovn-nbctl --wait=sb ls-lb-add sw0 lb1
> +ovn-nbctl --wait=sb ls-lb-add sw1 lb1
> +ovn-nbctl --wait=sb lr-lb-add lr0 lb1
> +
> +ovn-nbctl ls-add public
> +ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> +ovn-nbctl lsp-add public public-lr0
> +ovn-nbctl lsp-set-type public-lr0 router
> +ovn-nbctl lsp-set-addresses public-lr0 router
> +ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> +
> +# localnet port
> +ovn-nbctl lsp-add public ln-public
> +ovn-nbctl lsp-set-type ln-public localnet
> +ovn-nbctl lsp-set-addresses ln-public unknown
> +ovn-nbctl lsp-set-options ln-public network_name=public
> +
> +# schedule the gw router port to a chassis. Change the name of the chassis
> +ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20
> +
> +OVN_POPULATE_ARP
> +ovn-nbctl --wait=hv sync
> +
> +# And now for the anticlimax. We need to ensure that there is no
> +# service monitor in the southbound db.
> +
> +AT_CHECK([test 0 = `ovn-sbctl --bare --columns _uuid find \
> +service_monitor | sed '/^$/d' | wc -l`])
> +
> +# Let's also be sure the warning message about SCTP load balancers is
> +# is in the ovn-northd log
> +
> +AT_CHECK([test 1 = `grep -c "SCTP load balancers do not currently support health checks" northd/ovn-northd.log`])
> +
> +AT_CLEANUP
> +
>  AT_SETUP([ovn -- ARP/ND request broadcast limiting])
>  ovn_start
>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index e80058e61..59abe0051 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -2734,9 +2734,11 @@ nbctl_lb_add(struct ctl_context *ctx)
>          /* Validate protocol. */
>          lb_proto = ctx->argv[4];
>          is_update_proto = true;
> -        if (strcmp(lb_proto, "tcp") && strcmp(lb_proto, "udp")) {
> -            ctl_error(ctx, "%s: protocol must be one of \"tcp\", \"udp\".",
> -                      lb_proto);
> +        if (strcmp(lb_proto, "tcp") &&
> +            strcmp(lb_proto, "udp") &&
> +            strcmp(lb_proto, "sctp")) {
> +            ctl_error(ctx, "%s: protocol must be one of \"tcp\", \"udp\", "
> +                      " or \"sctp\".", lb_proto);
>              return;
>          }
>      }
> --
> 2.24.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>



More information about the dev mailing list