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

Manoj Sharma manoj.sharma at nutanix.com
Wed Jan 22 20:13:20 UTC 2020


Hi Numan,

I sent v3 just now. Please take a look.

Some of the issues reported by checkpatch can not be avoided. I fixed whatever could be broken into two lines.
The warnings below are for ‘-‘ in fwd-group-add/del.

Thanks,
Manoj

From: Numan Siddique <numans at ovn.org>
Date: Thursday, January 16, 2020 at 7:36 AM
To: Manoj Sharma <manoj.sharma at nutanix.com>
Cc: "ovs-dev at openvswitch.org" <ovs-dev at openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v2 ovn 1/3] Forwarding group to load balance l2 traffic with liveness detection



On Sat, Jan 11, 2020 at 6:11 AM Manoj Sharma <manoj.sharma at nutanix.com<mailto: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<mailto: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<mailto:dev at openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=9SN4tlEcxQzh-CvuC81YSi9I6ERNBOWQbg-05pnXlNw&m=TOBwZt_i97odPD5zXKnpvl9VbYN8mCv7cnxRrl8_B8w&s=09692xvVpG9GXDKLPPOBf7s056puQ-tvrpiaHrT0WWE&e=>


More information about the dev mailing list