[ovs-dev,v2] ovn-nbctl: Add basic port group commands.

Message ID 20181008202353.29585-1-mmichels@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev,v2] ovn-nbctl: Add basic port group commands.
Related show

Commit Message

Mark Michelson Oct. 8, 2018, 8:23 p.m.
This adds the following commands:

pg-add: Add a new port group, optionally adding switch ports at
creation.
pg-set-ports: Sets the logical switch ports on a port group
pg-del: Remove a port group.

The main motivation for these commands is that it allows for adding
logical switch ports by name rather than UUID.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
v1 -> v2
Moved tests to tests/ovn-nbctl.at. This revealed issues when listing
database information in ovn-nbctl daemon mode. These issues are fixed in
series https://patchwork.ozlabs.org/project/openvswitch/list/?series=69636
Therefore this patch is dependent on the linked series.
---
 ovn/utilities/ovn-nbctl.8.xml | 22 +++++++++++++
 ovn/utilities/ovn-nbctl.c     | 72 +++++++++++++++++++++++++++++++++++++++++++
 tests/ovn-nbctl.at            | 34 +++++++++++++++++++-
 tests/ovn.at                  |  1 +
 4 files changed, 128 insertions(+), 1 deletion(-)

Comments

0-day Robot Oct. 8, 2018, 8:54 p.m. | #1
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
<stdin>:194: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Failed to merge in the changes.
Patch failed at 0001 ovn-nbctl: Add basic port group commands.
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Han Zhou Oct. 8, 2018, 9:20 p.m. | #2
On Mon, Oct 8, 2018 at 1:24 PM Mark Michelson <mmichels@redhat.com> wrote:
>
> This adds the following commands:
>
> pg-add: Add a new port group, optionally adding switch ports at
> creation.
> pg-set-ports: Sets the logical switch ports on a port group
> pg-del: Remove a port group.
>
> The main motivation for these commands is that it allows for adding
> logical switch ports by name rather than UUID.
>
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---
> v1 -> v2
> Moved tests to tests/ovn-nbctl.at. This revealed issues when listing
> database information in ovn-nbctl daemon mode. These issues are fixed in
> series https://patchwork.ozlabs.org/project/openvswitch/list/?series=69636
> Therefore this patch is dependent on the linked series.
> ---
>  ovn/utilities/ovn-nbctl.8.xml | 22 +++++++++++++
>  ovn/utilities/ovn-nbctl.c     | 72
+++++++++++++++++++++++++++++++++++++++++++
>  tests/ovn-nbctl.at            | 34 +++++++++++++++++++-
>  tests/ovn.at                  |  1 +
>  4 files changed, 128 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
> index 5e18fa7c0..14cc02f53 100644
> --- a/ovn/utilities/ovn-nbctl.8.xml
> +++ b/ovn/utilities/ovn-nbctl.8.xml
> @@ -881,6 +881,28 @@
>        </dd>
>      </dl>
>
> +    <h1>Port Group commands</h1>
> +
> +    <dl>
> +      <dt><code>pg-add</code> <var>group</var> [<var>port</var>]...</dt>
> +      <dd>
> +        Creates a new port group in the <code>Port_Group</code> table
named
> +        <code>group</code> with optional <code>ports</code> added to the
group.
> +      </dd>
> +
> +      <dt><code>pg-set-ports</code> <var>group</var>
<var>port</var>...</dt>
> +      <dd>
> +        Sets <code>ports</code> on the port group named
<code>group</code>. It
> +        is an error if <code>group</code> does not exist.
> +      </dd>
> +
> +      <dt><code>pg-del</code> <var>group</var></dt>
> +      <dd>
> +        Deletes port group <code>group</code>. It is an error if
> +        <code>group</code> does not exist.
> +      </dd>
> +    </dl>
> +
>      <h1>Database Commands</h1>
>      <p>These commands query and modify the contents of
<code>ovsdb</code> tables.
>      They are a slight abstraction of the <code>ovsdb</code> interface and
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index bff6d1380..89f0d6213 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -4655,6 +4655,73 @@ cmd_set_ssl(struct ctl_context *ctx)
>      nbrec_nb_global_set_ssl(nb_global, ssl);
>  }
>
> +static char *
> +set_ports_on_pg(struct ctl_context *ctx, const struct nbrec_port_group
*pg,
> +                char **new_ports, size_t num_new_ports)
> +{
> +    struct nbrec_logical_switch_port **lports;
> +    lports = xmalloc(sizeof *lports * num_new_ports);
> +
> +    size_t i;
> +    char *error = NULL;
> +    for (i = 0; i < num_new_ports; i++) {
> +        const struct nbrec_logical_switch_port *lsp;
> +        error = lsp_by_name_or_uuid(ctx, new_ports[i], true, &lsp);
> +        if (error) {
> +            goto out;
> +        }
> +        lports[i] = (struct nbrec_logical_switch_port *) lsp;
> +    }
> +
> +    nbrec_port_group_set_ports(pg, lports, num_new_ports);
> +
> +out:
> +    free(lports);
> +    return error;
> +}
> +
> +static void
> +cmd_pg_add(struct ctl_context *ctx)
> +{
> +    const struct nbrec_port_group *pg;
> +
> +    pg = nbrec_port_group_insert(ctx->txn);
> +    nbrec_port_group_set_name(pg, ctx->argv[1]);
> +    if (ctx->argc > 2) {
> +        ctx->error = set_ports_on_pg(ctx, pg, ctx->argv + 2, ctx->argc -
2);
> +    }
> +}
> +
> +static void
> +cmd_pg_set_ports(struct ctl_context *ctx)
> +{
> +    const struct nbrec_port_group *pg;
> +
> +    char *error;
> +    error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, &pg);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    ctx->error = set_ports_on_pg(ctx, pg, ctx->argv + 2, ctx->argc - 2);
> +}
> +
> +static void
> +cmd_pg_del(struct ctl_context *ctx)
> +{
> +    const struct nbrec_port_group *pg;
> +
> +    char *error;
> +    error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, &pg);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    nbrec_port_group_delete(pg);
> +}
> +
>  static const struct ctl_table_class tables[NBREC_N_TABLES] = {
>      [NBREC_TABLE_DHCP_OPTIONS].row_ids
>      = {{&nbrec_logical_switch_port_col_name, NULL,
> @@ -5123,6 +5190,11 @@ static const struct ctl_command_syntax
nbctl_commands[] = {
>          "PRIVATE-KEY CERTIFICATE CA-CERT [SSL-PROTOS [SSL-CIPHERS]]",
>          pre_cmd_set_ssl, cmd_set_ssl, NULL, "--bootstrap", RW},
>
> +    /* Port Group Commands */
> +    {"pg-add", 1, INT_MAX, "", NULL, cmd_pg_add, NULL, "", RW },
> +    {"pg-set-ports", 2, INT_MAX, "", NULL, cmd_pg_set_ports, NULL, "",
RW },
> +    {"pg-del", 1, 1, "", NULL, cmd_pg_del, NULL, "", RW },
> +
>      {NULL, 0, 0, NULL, NULL, NULL, NULL, "", RO},
>  };
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 3f89874ba..25414b8bd 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -1518,7 +1518,6 @@ AT_CHECK([ovn-nbctl get Port_Group pg0 name], [0],
[dnl
>  "pg0"
>  ])])
>
> -
>  OVN_NBCTL_TEST([ovn_nbctl_extra_newlines], [extra newlines], [
>  dnl This test addresses a specific issue seen when running ovn-nbctl in
>  dnl daemon mode. All we have to do is ensure that each time we list
database
> @@ -1539,3 +1538,36 @@ AT_CHECK([ovn-nbctl ls-add sw1], [0], [ignore])
>  AT_CHECK([ovn-nbctl --bare --columns=name list logical_switch sw1], [0],
[dnl
>  sw1
>  ])])
> +dnl ---------------------------------------------------------------------
> +
> +OVN_NBCTL_TEST([ovn_nbctl_port_group_commands], [port group commands], [
> +AT_CHECK([ovn-nbctl pg-add pg1], [0], [ignore])
> +AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],
> +[pg1
> +])
> +
> +AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore])
> +AT_CHECK([ovn-nbctl list port_group], [0], [])
> +
> +AT_CHECK([ovn-nbctl ls-add sw1], [0], [ignore])
> +AT_CHECK([ovn-nbctl lsp-add sw1 sw1-p1], [0], [ignore])
> +SW1P1=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port sw1-p1)
> +AT_CHECK([ovn-nbctl lsp-add sw1 sw1-p2], [0], [ignore])
> +SW1P2=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port sw1-p2)
> +
> +AT_CHECK([ovn-nbctl pg-add pg1 sw1-p1], [0], [ignore])
> +AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],[dnl
> +pg1
> +])
> +AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group
pg1], [0], [dnl
> +$SW1P1
> +])
> +
> +AT_CHECK([ovn-nbctl pg-set-ports pg1 sw1-p2], [0], [ignore])
> +AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group
pg1], [0], [dnl
> +$SW1P2
> +])
> +
> +AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore])
> +AT_CHECK([ovn-nbctl list port_group], [0], [])
> +])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 769e09f81..b747a7992 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11253,3 +11253,4 @@ AT_CHECK([ovn-nbctl lsp-set-addresses sw2-p1
"00:00:00:00:00:04 192.168.0.3"])
>  AT_CHECK([ovn-nbctl lsp-set-addresses sw2-p1 "00:00:00:00:00:04
aef0::1"])
>
>  AT_CLEANUP
> +
> --
> 2.14.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks Mark. There is an unnecessary file change - ovn.at, where only a
blank line is added. Otherwise:

Acked-by: Han Zhou <hzhou8@ebay.com>
Ben Pfaff Oct. 9, 2018, 12:32 a.m. | #3
On Mon, Oct 08, 2018 at 04:23:53PM -0400, Mark Michelson wrote:
> This adds the following commands:
> 
> pg-add: Add a new port group, optionally adding switch ports at
> creation.
> pg-set-ports: Sets the logical switch ports on a port group
> pg-del: Remove a port group.
> 
> The main motivation for these commands is that it allows for adding
> logical switch ports by name rather than UUID.
> 
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---
> v1 -> v2
> Moved tests to tests/ovn-nbctl.at. This revealed issues when listing
> database information in ovn-nbctl daemon mode. These issues are fixed in
> series https://patchwork.ozlabs.org/project/openvswitch/list/?series=69636
> Therefore this patch is dependent on the linked series.

Besides Han's minor comment, would you mind updating the usage message
also?

Thanks,

Ben.

Patch

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 5e18fa7c0..14cc02f53 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -881,6 +881,28 @@ 
       </dd>
     </dl>
 
+    <h1>Port Group commands</h1>
+
+    <dl>
+      <dt><code>pg-add</code> <var>group</var> [<var>port</var>]...</dt>
+      <dd>
+        Creates a new port group in the <code>Port_Group</code> table named
+        <code>group</code> with optional <code>ports</code> added to the group.
+      </dd>
+
+      <dt><code>pg-set-ports</code> <var>group</var> <var>port</var>...</dt>
+      <dd>
+        Sets <code>ports</code> on the port group named <code>group</code>. It
+        is an error if <code>group</code> does not exist.
+      </dd>
+
+      <dt><code>pg-del</code> <var>group</var></dt>
+      <dd>
+        Deletes port group <code>group</code>. It is an error if
+        <code>group</code> does not exist.
+      </dd>
+    </dl>
+
     <h1>Database Commands</h1>
     <p>These commands query and modify the contents of <code>ovsdb</code> tables.
     They are a slight abstraction of the <code>ovsdb</code> interface and
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index bff6d1380..89f0d6213 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -4655,6 +4655,73 @@  cmd_set_ssl(struct ctl_context *ctx)
     nbrec_nb_global_set_ssl(nb_global, ssl);
 }
 
+static char *
+set_ports_on_pg(struct ctl_context *ctx, const struct nbrec_port_group *pg,
+                char **new_ports, size_t num_new_ports)
+{
+    struct nbrec_logical_switch_port **lports;
+    lports = xmalloc(sizeof *lports * num_new_ports);
+
+    size_t i;
+    char *error = NULL;
+    for (i = 0; i < num_new_ports; i++) {
+        const struct nbrec_logical_switch_port *lsp;
+        error = lsp_by_name_or_uuid(ctx, new_ports[i], true, &lsp);
+        if (error) {
+            goto out;
+        }
+        lports[i] = (struct nbrec_logical_switch_port *) lsp;
+    }
+
+    nbrec_port_group_set_ports(pg, lports, num_new_ports);
+
+out:
+    free(lports);
+    return error;
+}
+
+static void
+cmd_pg_add(struct ctl_context *ctx)
+{
+    const struct nbrec_port_group *pg;
+
+    pg = nbrec_port_group_insert(ctx->txn);
+    nbrec_port_group_set_name(pg, ctx->argv[1]);
+    if (ctx->argc > 2) {
+        ctx->error = set_ports_on_pg(ctx, pg, ctx->argv + 2, ctx->argc - 2);
+    }
+}
+
+static void
+cmd_pg_set_ports(struct ctl_context *ctx)
+{
+    const struct nbrec_port_group *pg;
+
+    char *error;
+    error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, &pg);
+    if (error) {
+        ctx->error = error;
+        return;
+    }
+
+    ctx->error = set_ports_on_pg(ctx, pg, ctx->argv + 2, ctx->argc - 2);
+}
+
+static void
+cmd_pg_del(struct ctl_context *ctx)
+{
+    const struct nbrec_port_group *pg;
+
+    char *error;
+    error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, &pg);
+    if (error) {
+        ctx->error = error;
+        return;
+    }
+
+    nbrec_port_group_delete(pg);
+}
+
 static const struct ctl_table_class tables[NBREC_N_TABLES] = {
     [NBREC_TABLE_DHCP_OPTIONS].row_ids
     = {{&nbrec_logical_switch_port_col_name, NULL,
@@ -5123,6 +5190,11 @@  static const struct ctl_command_syntax nbctl_commands[] = {
         "PRIVATE-KEY CERTIFICATE CA-CERT [SSL-PROTOS [SSL-CIPHERS]]",
         pre_cmd_set_ssl, cmd_set_ssl, NULL, "--bootstrap", RW},
 
+    /* Port Group Commands */
+    {"pg-add", 1, INT_MAX, "", NULL, cmd_pg_add, NULL, "", RW },
+    {"pg-set-ports", 2, INT_MAX, "", NULL, cmd_pg_set_ports, NULL, "", RW },
+    {"pg-del", 1, 1, "", NULL, cmd_pg_del, NULL, "", RW },
+
     {NULL, 0, 0, NULL, NULL, NULL, NULL, "", RO},
 };
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 3f89874ba..25414b8bd 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -1518,7 +1518,6 @@  AT_CHECK([ovn-nbctl get Port_Group pg0 name], [0], [dnl
 "pg0"
 ])])
 
-
 OVN_NBCTL_TEST([ovn_nbctl_extra_newlines], [extra newlines], [
 dnl This test addresses a specific issue seen when running ovn-nbctl in
 dnl daemon mode. All we have to do is ensure that each time we list database
@@ -1539,3 +1538,36 @@  AT_CHECK([ovn-nbctl ls-add sw1], [0], [ignore])
 AT_CHECK([ovn-nbctl --bare --columns=name list logical_switch sw1], [0], [dnl
 sw1
 ])])
+dnl ---------------------------------------------------------------------
+
+OVN_NBCTL_TEST([ovn_nbctl_port_group_commands], [port group commands], [
+AT_CHECK([ovn-nbctl pg-add pg1], [0], [ignore])
+AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],
+[pg1
+])
+
+AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore])
+AT_CHECK([ovn-nbctl list port_group], [0], [])
+
+AT_CHECK([ovn-nbctl ls-add sw1], [0], [ignore])
+AT_CHECK([ovn-nbctl lsp-add sw1 sw1-p1], [0], [ignore])
+SW1P1=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port sw1-p1)
+AT_CHECK([ovn-nbctl lsp-add sw1 sw1-p2], [0], [ignore])
+SW1P2=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port sw1-p2)
+
+AT_CHECK([ovn-nbctl pg-add pg1 sw1-p1], [0], [ignore])
+AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],[dnl
+pg1
+])
+AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group pg1], [0], [dnl
+$SW1P1
+])
+
+AT_CHECK([ovn-nbctl pg-set-ports pg1 sw1-p2], [0], [ignore])
+AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group pg1], [0], [dnl
+$SW1P2
+])
+
+AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore])
+AT_CHECK([ovn-nbctl list port_group], [0], [])
+])
diff --git a/tests/ovn.at b/tests/ovn.at
index 769e09f81..b747a7992 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -11253,3 +11253,4 @@  AT_CHECK([ovn-nbctl lsp-set-addresses sw2-p1 "00:00:00:00:00:04 192.168.0.3"])
 AT_CHECK([ovn-nbctl lsp-set-addresses sw2-p1 "00:00:00:00:00:04 aef0::1"])
 
 AT_CLEANUP
+