[ovs-dev] [PATCH] Add basic PG commands.

Mark Michelson mmichels at redhat.com
Fri Oct 5 20:05:22 UTC 2018


On 10/04/2018 06:08 PM, Han Zhou wrote:
> 
> 
> On Wed, Oct 3, 2018 at 2:14 PM Mark Michelson <mmichels at redhat.com 
> <mailto:mmichels at redhat.com>> wrote:
>  >
>  > Signed-off-by: Mark Michelson <mmichels at redhat.com 
> <mailto:mmichels at redhat.com>>
>  > ---
>  >  ovn/utilities/ovn-nbctl.8.xml | 22 +++++++++++++
>  >  ovn/utilities/ovn-nbctl.c     | 72 
> +++++++++++++++++++++++++++++++++++++++++++
>  >  tests/ovn.at <http://ovn.at>                  | 37 
> ++++++++++++++++++++++
>  >  3 files changed, 131 insertions(+)
>  >
>  > 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 eabd30308..b586db502 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.at <http://ovn.at> b/tests/ovn.at <http://ovn.at>
>  > index 769e09f81..81dff3da2 100644
>  > --- a/tests/ovn.at <http://ovn.at>
>  > +++ b/tests/ovn.at <http://ovn.at>
>  > @@ -11253,3 +11253,40 @@ 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
>  > +
>  > +AT_SETUP([ovn -- ovn-nbctl port group commands])
>  > +ovn_start
>  > +
>  > +ovn-nbctl pg-add pg1
>  > +AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],
>  > +[pg1
>  > +])
>  > +
>  > +ovn-nbctl pg-del pg1
>  > +AT_CHECK([ovn-nbctl list port_group], [0], [])
>  > +
>  > +ovn-nbctl ls-add sw1
>  > +ovn-nbctl lsp-add sw1 sw1-p1
>  > +SW1P1=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port 
> sw1-p1)
>  > +ovn-nbctl lsp-add sw1 sw1-p2
>  > +SW1P2=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port 
> sw1-p2)
>  > +
>  > +ovn-nbctl list logical_switch_port
>  > +
>  > +ovn-nbctl pg-add pg1 sw1-p1
>  > +AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],
>  > +[pg1
>  > +])
>  > +AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group 
> pg1], [0],
>  > +[[$SW1P1
>  > +]])
>  > +
>  > +ovn-nbctl pg-set-ports pg1 sw1-p2
>  > +AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group 
> pg1], [0],
>  > +[[$SW1P2
>  > +]])
>  > +
>  > +ovn-nbctl pg-del pg1
>  > +AT_CHECK([ovn-nbctl list port_group], [0], [])
>  > +
>  > +AT_CLEANUP
>  > --
>  > 2.14.4
>  >
>  > _______________________________________________
>  > dev mailing list
>  > dev at openvswitch.org <mailto:dev at openvswitch.org>
>  > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> Thanks Mark for improving the commands for port-groups. It looks good to 
> me, except that the nbctl related tests may be better to stay in 
> ovn-nbctl.at <http://ovn-nbctl.at>.
> 
> Han

Thanks for the feedback Han. Interestingly, when I move the tests from 
ovn.at to ovn-nbctl.at, the tests no longer pass. When running in 
non-daemon mode, for whatever reason, trying to get the port group's 
ports from the database results in an empty return. I can't reproduce 
this in the sandbox, so I'll need to dive in and see what's going wrong.

When run in daemon mode, interestingly, it appears that there are a 
couple of legitimate bugs I'm running into. First, there's an extra 
blank line at the beginning before the output of the database output. 
Second, it appears the --bare flag is ignored. I can reproduce both of 
these in the sandbox.

So I think this is going to result in another patch from me to fix the 
daemon mode bugs mentioned above.


More information about the dev mailing list