[ovs-dev] [PATCH v2 ovn 1/3] Forwarding group to load balance l2 traffic with liveness detection

Numan Siddique numans at ovn.org
Thu Jan 16 15:36:41 UTC 2020


On Sat, Jan 11, 2020 at 6:11 AM Manoj Sharma <manoj.sharma at nutanix.com>
wrote:

> Add a forwarding group table and a reference to the logical switch it is
> configured on. The forwarding group is configured with a virtual IP,
> virtual
> MAC and a number of logical switch ports from a logical switch.
>
> Signed-off-by: Manoj Sharma <manoj.sharma at nutanix.com>
>

This patch has below checkpatch issues. Can you please fix them.

****
== Checking 198cdb9bab38 ("Forwarding group to load balance l2 traffic with
liveness detection") ==
WARNING: Line is 149 characters long (recommended limit is 79)
#125 FILE: utilities/ovn-nbctl.8.xml:489:
      <dt>[<code>--liveness</code>]<code>fwd-group-add</code>
<var>group</var> <var>switch</var> <var>vip</var> <var>vmac</var>
<var>ports</var></dt>

WARNING: Line is 85 characters long (recommended limit is 79)
#146 FILE: utilities/ovn-nbctl.8.xml:510:
      <dt>[<code>--if-exists</code>] <code>fwd-group-del</code>
<var>group</var></dt>

WARNING: Line lacks whitespace around operator
#172 FILE: utilities/ovn-nbctl.c:653:
  fwd-group-add GROUP SWITCH VIP VMAC PORTS...\n\

WARNING: Line lacks whitespace around operator
#174 FILE: utilities/ovn-nbctl.c:655:
  fwd-group-del GROUP       delete a forwarding group\n\

WARNING: Line lacks whitespace around operator
#175 FILE: utilities/ovn-nbctl.c:656:
  fwd-group-list [SWITCH]   print forwarding groups\n\

ERROR: Improper whitespace around control block
#196 FILE: utilities/ovn-nbctl.c:4742:
        NBREC_FORWARDING_GROUP_FOR_EACH(fwd_group, ctx->idl) {

ERROR: Improper whitespace around control block
#392 FILE: utilities/ovn-nbctl.c:4938:
    NBREC_FORWARDING_GROUP_FOR_EACH(fwd_group, ctx->idl) {
******

I think you can ignore the warnings in ovn-nbctl.c.

You can run these checks your self -  "utilities/checkpatch.py -1"

You need to bump  up the version in ovn-nb.ovsschema to 5.19.0

Also please move the relevant test cases from patch 3 to this one.

Thanks
Numan




> ---
>  ovn-nb.ovsschema          |  18 +++-
>  ovn-nb.xml                |  35 +++++++
>  utilities/ovn-nbctl.8.xml |  37 +++++++
>  utilities/ovn-nbctl.c     | 253
> ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 341 insertions(+), 2 deletions(-)
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 12999a4..99b6285 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
>      "version": "5.18.0",
> -    "cksum": "2806349485 24196",
> +    "cksum": "63300136 24879",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -59,7 +59,12 @@
>                               "min": 0, "max": "unlimited"}},
>                  "external_ids": {
>                      "type": {"key": "string", "value": "string",
> -                             "min": 0, "max": "unlimited"}}},
> +                             "min": 0, "max": "unlimited"}},
> +               "forwarding_groups": {
> +                    "type": {"key": {"type": "uuid",
> +                                     "refTable": "Forwarding_Group",
> +                                     "refType": "strong"},
> +                                     "min": 0, "max": "unlimited"}}},
>              "isRoot": true},
>          "Logical_Switch_Port": {
>              "columns": {
> @@ -113,6 +118,15 @@
>                               "min": 0, "max": "unlimited"}}},
>              "indexes": [["name"]],
>              "isRoot": false},
> +        "Forwarding_Group": {
> +            "columns": {
> +                "name": {"type": "string"},
> +                "vip": {"type": "string"},
> +                "vmac": {"type": "string"},
> +                "liveness": {"type": "boolean"},
> +                "child_port": {"type": {"key": "string",
> +                                        "min": 1, "max": "unlimited"}}},
> +            "isRoot": false},
>          "Address_Set": {
>              "columns": {
>                  "name": {"type": "string"},
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 5ae52bb..decb4ae 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -197,6 +197,11 @@
>        Please see the <ref table="DNS"/> table.
>      </column>
>
> +    <column name="forwarding_groups">
> +      Groups a set of logical port endpoints for traffic going out of the
> +      logical switch.
> +    </column>
> +
>      <group title="Naming">
>        <p>
>          These columns provide names for the logical switch.  From OVN's
> @@ -1152,6 +1157,36 @@
>      </group>
>    </table>
>
> +  <table name="Forwarding_Group" title="forwarding group">
> +    <p>
> +      Each row represents one forwarding group.
> +    </p>
> +
> +    <column name="name">
> +      A name for the forwarding group.  This name has no special meaning
> or
> +      purpose other than to provide convenience for human interaction with
> +      the ovn-nb database.
> +    </column>
> +
> +    <column name="vip">
> +      The virtual IP address assigned to the forwarding group. It will
> respond
> +      with vmac when an ARP request is sent for vip.
> +    </column>
> +
> +    <column name="vmac">
> +      The virtual MAC address assigned to the forwarding group.
> +    </column>
> +
> +    <column name="liveness">
> +      If set to <code>true</code>, liveness is enabled for child ports
> +      otherwise it is disabled.
> +    </column>
> +
> +    <column name="child_port">
> +      List of child ports in the forwarding group.
> +    </column>
> +  </table>
> +
>    <table name="Address_Set" title="Address Sets">
>      <p>
>        Each row in this table represents a named set of addresses.
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 88ebd13..2f3badd 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -483,6 +483,43 @@
>
>      </dl>
>
> +    <h1>Forwarding Group Commands</h1>
> +
> +    <dl>
> +      <dt>[<code>--liveness</code>]<code>fwd-group-add</code>
> <var>group</var> <var>switch</var> <var>vip</var> <var>vmac</var>
> <var>ports</var></dt>
> +      <dd>
> +        <p>
> +          Creates a new forwarding group named <var>group</var> as the
> name
> +          with the provided <var>vip</var> and <var>vmac</var>.
> <var>vip</var>
> +          should be a virtual IP address and <var>vmac</var> should be a
> +          virtual MAC address to access the forwarding group.
> <var>ports</var>
> +          are the logical switch port names that are put in the forwarding
> +          group. Example for <var>ports</var> is lsp1 lsp2 ...
> +          Traffic destined to virtual IP of the forwarding group will be
> load
> +          balanced to all the child ports.
> +        </p>
> +        <p>
> +          When <code>--liveness</code> is specified then child ports are
> +          expected to be bound to external devices like routers. BFD
> should
> +          be configured between hypervisors and the external devices.
> +          The child port selection will become dependent on BFD status
> with
> +          its external device.
> +        </p>
> +      </dd>
> +
> +      <dt>[<code>--if-exists</code>] <code>fwd-group-del</code>
> <var>group</var></dt>
> +      <dd>
> +        Deletes <var>group</var>.  It is an error if <var>group</var> does
> +        not exist, unless <code>--if-exists</code> is specified.
> +      </dd>
> +
> +      <dt><code>fwd-group-list</code> [<var>switch</var>]</dt>
> +      <dd>
> +        Lists all existing forwarding groups, If <var>switch</var> is
> specified
> +        then only the forwarding groups configured for <var>switch</var>
> will
> +        be listed.
> +      </dd>
> +    </dl>
>      <h1>Logical Router Commands</h1>
>
>      <dl>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 46ba3a9..39f53da 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -648,6 +648,13 @@ Logical switch port commands:\n\
>    lsp-get-dhcpv6-options PORT  get the dhcpv6 options for PORT\n\
>    lsp-get-ls PORT           get the logical switch which the port belongs
> to\n\
>  \n\
> +Forwarding group commands:\n\
> +  [--liveness]\n\
> +  fwd-group-add GROUP SWITCH VIP VMAC PORTS...\n\
> +                            add a forwarding group on SWITCH\n\
> +  fwd-group-del GROUP       delete a forwarding group\n\
> +  fwd-group-list [SWITCH]   print forwarding groups\n\
> +\n\
>  Logical router commands:\n\
>    lr-add [ROUTER]           create a logical router named ROUTER\n\
>    lr-del ROUTER             delete ROUTER and all its ports\n\
> @@ -4720,6 +4727,244 @@ nbctl_lrp_get_redirect_type(struct ctl_context
> *ctx)
>                    !redirect_type ? "overlay": redirect_type);
>  }
>
> +static const struct nbrec_forwarding_group *
> +fwd_group_by_name_or_uuid(struct ctl_context *ctx, const char *id)
> +{
> +    const struct nbrec_forwarding_group *fwd_group = NULL;
> +    struct uuid fwd_uuid;
> +
> +    bool is_uuid = uuid_from_string(&fwd_uuid, id);
> +    if (is_uuid) {
> +        fwd_group = nbrec_forwarding_group_get_for_uuid(ctx->idl,
> &fwd_uuid);
> +    }
> +
> +    if (!fwd_group) {
> +        NBREC_FORWARDING_GROUP_FOR_EACH(fwd_group, ctx->idl) {
> +            if (!strcmp(fwd_group->name, id)) {
> +                break;
> +            }
> +        }
> +    }
> +
> +    return fwd_group;
> +}
> +
> +static const struct nbrec_logical_switch *
> +fwd_group_to_logical_switch(struct ctl_context *ctx,
> +                            const struct nbrec_forwarding_group
> *fwd_group)
> +{
> +    if (!fwd_group) {
> +        return NULL;
> +    }
> +
> +    const struct nbrec_logical_switch_port *lsp;
> +    char *error = lsp_by_name_or_uuid(ctx, fwd_group->child_port[0],
> +                                      false, &lsp);
> +    if (error) {
> +        ctx->error = error;
> +        return NULL;
> +    }
> +    if (!lsp) {
> +        return NULL;
> +    }
> +
> +    const struct nbrec_logical_switch *ls;
> +    error = lsp_to_ls(ctx->idl, lsp, &ls);
> +    if (error) {
> +        ctx->error = error;
> +        return NULL;
> +    }
> +
> +    if (!ls) {
> +        return NULL;
> +    }
> +
> +    return ls;
> +}
> +
> +static void
> +nbctl_fwd_group_add(struct ctl_context *ctx)
> +{
> +    if (ctx->argc <= 5) {
> +        ctl_error(ctx, "Usage : ovn-nbctl fwd-group-add group switch vip
> vmac "
> +                  "child_ports...");
> +        return;
> +    }
> +
> +    /* Check if the forwarding group already exists */
> +    const char *fwd_group_name = ctx->argv[1];
> +    if (fwd_group_by_name_or_uuid(ctx, fwd_group_name)) {
> +        ctl_error(ctx, "%s: a forwarding group by this name already
> exists",
> +                  fwd_group_name);
> +        return;
> +    }
> +
> +    /* Check if the logical switch exists */
> +    const char *ls_name = ctx->argv[2];
> +    const struct nbrec_logical_switch *ls = NULL;
> +    char *error = ls_by_name_or_uuid(ctx, ls_name, true, &ls);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    /* Virtual IP for the group */
> +    ovs_be32 ipv4 = 0;
> +    const char *fwd_group_vip = ctx->argv[3];
> +    if (!ip_parse(fwd_group_vip, &ipv4)) {
> +        ctl_error(ctx, "invalid ip address %s", fwd_group_vip);
> +        return;
> +    }
> +
> +    /* Virtual MAC for the group */
> +    const char *fwd_group_vmac = ctx->argv[4];
> +    struct eth_addr ea;
> +    if (!eth_addr_from_string(fwd_group_vmac, &ea)) {
> +        ctl_error(ctx, "invalid mac address %s", fwd_group_vmac);
> +        return;
> +    }
> +
> +    /* Create the forwarding group */
> +    struct nbrec_forwarding_group *fwd_group = NULL;
> +    fwd_group = nbrec_forwarding_group_insert(ctx->txn);
> +    nbrec_forwarding_group_set_name(fwd_group, fwd_group_name);
> +    nbrec_forwarding_group_set_vip(fwd_group, fwd_group_vip);
> +    nbrec_forwarding_group_set_vmac(fwd_group, fwd_group_vmac);
> +
> +    int n_child_port = ctx->argc - 5;
> +    const char **child_port = (const char **)&ctx->argv[5];
> +
> +    /* Verify that child ports belong to the logical switch specified */
> +    for (int i = 5; i < ctx->argc; ++i) {
> +        const struct nbrec_logical_switch_port *lsp;
> +        const char *lsp_name = ctx->argv[i];
> +        error = lsp_by_name_or_uuid(ctx, lsp_name, false, &lsp);
> +        if (error) {
> +            ctx->error = error;
> +            return;
> +        }
> +        if (lsp) {
> +            error = lsp_to_ls(ctx->idl, lsp, &ls);
> +            if (error) {
> +                ctx->error = error;
> +                return;
> +            }
> +            if (strcmp(ls->name, ls_name)) {
> +                ctl_error(ctx, "%s: port already exists but in logical "
> +                          "switch %s", lsp_name, ls->name);
> +                return;
> +            }
> +        } else {
> +            ctl_error(ctx, "%s: logical switch port does not exist",
> lsp_name);
> +            return;
> +        }
> +    }
> +    nbrec_forwarding_group_set_child_port(fwd_group, child_port,
> n_child_port);
> +
> +    /* Liveness option */
> +    bool liveness = shash_find(&ctx->options, "--liveness") != NULL;
> +    if (liveness) {
> +      nbrec_forwarding_group_set_liveness(fwd_group, true);
> +    }
> +
> +    struct nbrec_forwarding_group **new_fwd_groups =
> +            xmalloc(sizeof(*new_fwd_groups) * (ls->n_forwarding_groups +
> 1));
> +    memcpy(new_fwd_groups, ls->forwarding_groups,
> +           sizeof *new_fwd_groups * ls->n_forwarding_groups);
> +    new_fwd_groups[ls->n_forwarding_groups] = fwd_group;
> +    nbrec_logical_switch_set_forwarding_groups(ls, new_fwd_groups,
> +                                               (ls->n_forwarding_groups +
> 1));
> +    free(new_fwd_groups);
> +
> +}
> +
> +static void
> +nbctl_fwd_group_del(struct ctl_context *ctx)
> +{
> +    const char *id = ctx->argv[1];
> +    const struct nbrec_forwarding_group *fwd_group = NULL;
> +
> +    fwd_group = fwd_group_by_name_or_uuid(ctx, id);
> +    if (!fwd_group) {
> +        return;
> +    }
> +
> +    const struct nbrec_logical_switch *ls = NULL;
> +    ls = fwd_group_to_logical_switch(ctx, fwd_group);
> +    if (!ls) {
> +      return;
> +    }
> +
> +    for (int i = 0; i < ls->n_forwarding_groups; ++i) {
> +        if (!strcmp(ls->forwarding_groups[i]->name, fwd_group->name)) {
> +            struct nbrec_forwarding_group **new_fwd_groups =
> +                xmemdup(ls->forwarding_groups,
> +                        sizeof *new_fwd_groups * ls->n_forwarding_groups);
> +            new_fwd_groups[i] =
> +                ls->forwarding_groups[ls->n_forwarding_groups - 1];
> +            nbrec_logical_switch_set_forwarding_groups(ls, new_fwd_groups,
> +                (ls->n_forwarding_groups - 1));
> +            free(new_fwd_groups);
> +            nbrec_forwarding_group_delete(fwd_group);
> +            return;
> +        }
> +    }
> +}
> +
> +static void
> +fwd_group_list_all(struct ctl_context *ctx, const char *ls_name)
> +{
> +    const struct nbrec_logical_switch *ls;
> +    struct ds *s = &ctx->output;
> +    const struct nbrec_forwarding_group *fwd_group = NULL;
> +
> +    if (ls_name) {
> +        char *error = ls_by_name_or_uuid(ctx, ls_name, true, &ls);
> +        if (error) {
> +            ctx->error = error;
> +            return;
> +        }
> +        if (!ls) {
> +            ctl_error(
> +                ctx, "%s: a logical switch with this name does not exist",
> +                ls_name);
> +            return;
> +        }
> +    }
> +
> +    ds_put_format(s, "%-16.16s%-14.16s%-16.7s%-22.21s%s\n",
> +                  "FWD_GROUP", "LS", "VIP", "VMAC", "CHILD_PORTS");
> +
> +    NBREC_FORWARDING_GROUP_FOR_EACH(fwd_group, ctx->idl) {
> +        ls = fwd_group_to_logical_switch(ctx, fwd_group);
> +        if (!ls) {
> +            continue;
> +        }
> +
> +        if (ls_name && (strcmp(ls->name, ls_name))) {
> +            continue;
> +        }
> +
> +        ds_put_format(s, "%-16.16s%-14.18s%-15.16s%-9.18s     ",
> +                      fwd_group->name, ls->name,
> +                      fwd_group->vip, fwd_group->vmac);
> +        for (int i = 0; i < fwd_group->n_child_port; ++i) {
> +            ds_put_format(s, " %s", fwd_group->child_port[i]);
> +        }
> +        ds_put_char(s, '\n');
> +    }
> +}
> +
> +static void
> +nbctl_fwd_group_list(struct ctl_context *ctx)
> +{
> +    if (ctx->argc == 1) {
> +        fwd_group_list_all(ctx, NULL);
> +    } else if (ctx->argc == 2) {
> +        fwd_group_list_all(ctx, ctx->argv[1]);
> +    }
> +}
> +
>
>  struct ipv4_route {
>      int priority;
> @@ -5704,6 +5949,14 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
>        nbctl_lsp_get_dhcpv6_options, NULL, "", RO },
>      { "lsp-get-ls", 1, 1, "PORT", NULL, nbctl_lsp_get_ls, NULL, "", RO },
>
> +    /* forwarding group commands. */
> +    { "fwd-group-add", 4, INT_MAX, "SWITCH GROUP VIP VMAC PORT...",
> +      NULL, nbctl_fwd_group_add, NULL, "--liveness", RW },
> +    { "fwd-group-del", 1, 1, "GROUP", NULL, nbctl_fwd_group_del, NULL,
> +      "--if-exists", RW },
> +    { "fwd-group-list", 0, 1, "[GROUP]", NULL, nbctl_fwd_group_list, NULL,
> +      "", RO },
> +
>      /* logical router commands. */
>      { "lr-add", 0, 1, "[ROUTER]", NULL, nbctl_lr_add, NULL,
>        "--may-exist,--add-duplicate", RW },
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list