[ovs-dev] [PATCH ovn 1/2 v6] ovn-nbctl: Updates for container integration.

Justin Pettit jpettit at nicira.com
Fri Apr 17 21:55:20 UTC 2015


> On Apr 17, 2015, at 1:17 PM, Russell Bryant <rbryant at redhat.com> wrote:

Looks good, just some minor things.

>     <h1>Logical Port Commands</h1>
>     <dl>
> -      <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var></dt>
> +      <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var> [<var>parent</var>] [<var>tag</var>]</dt>
>       <dd>
>         Creates on <var>lswitch</var> a new logical port named
> -        <var>lport</var>.
> +        <var>lport</var>.  If this port is a child port (for a
> +        container running inside a VM), specify the parent port
> +        and tag for identifying this port's traffic.
>       </dd>

In "ovs-vsctl add-br", the case of specifying the parent is treated as a new command.  Instead of changing this definition, you could add another "lport-add" that handles the parent case.  Also, I wonder if we'll have other cases of using children, such as nested hypervisors.  What about a definition like the following:

      <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var> <var>parent</var> <var>tag</var></dt>
      <dd>
        Creates on <var>lswitch</var> a logical port named <var>lport</var>
        that is a child of <var>parent</var> that is identied with
        <var>tag</var>.  This is useful in cases such as virtualized
        container environments where Open vSwitch does not have a direct
        connection to the container's port and it must be shared with
        the virtual machine's port.
      </dd>

> Logical port commands:\n\
> -  lport-add LSWITCH LPORT   add logical port LPORT on LSWITCH\n\
> +  lport-add LSWITCH LPORT [PARENT] [TAG]\n\
> +                            add logical port LPORT on LSWITCH\n\

I think it might be clearer if you show it as two separate commands (there's some precedence with ovs-vsctl):

  lport-add LSWITCH LPORT   add logical port LPORT on LSWITCH
  lport-add LSWITCH LPORT PARENT TAG
                            add logical port LPORT on LSWITCH with
                            PARENT on TAG

>   lport-del LPORT           delete LPORT from its attached switch\n\
>   lport-list LSWITCH        print the names of all logical ports on LSWITCH\n\
> +  lport-get-parent LPORT    Get the parent port name if set\n\

To be consistent, I'd use lower-case for "get".  What about phrasing like:

  lport-get-parent LPORT    get parent of LPORT if set

> +  lport-get-tag LPORT       Get the port's tag if set\n\

Same here.  What about phrasing like:

  lport-get-tag LPORT       Get the LPORT's tag if set

> @@ -251,15 +255,37 @@ do_lport_add(struct ovs_cmdl_context *ctx)
>     struct nbctl_context *nb_ctx = ctx->pvt;
>     struct nbrec_logical_port *lport;
>     const struct nbrec_logical_switch *lswitch;
> +    int64_t tag;
> +
> +    /* Validate all of the input */

I don't know that this comment adds much.

> +    if (ctx->argc != 3 && ctx->argc != 5) {
> +        /* If a parent_name is specififed, a tag must be specified as well. */
> +        VLOG_WARN("Invalid arguments to lport-add.");
> +        return;
> +    }
> +
> +    if (ctx->argc == 5) {
> +        /* Validate tag. */
> +        if (!ovs_scan(ctx->argv[4], "%"SCNd64, &tag) || tag < 0 || tag > 4095) {
> +            VLOG_WARN("Invalid tag for logical port '%s'", ctx->argv[4]);

The phrasing makes it sound like the returned value is the logical port's name, not the tag.  What if you just drop "for logical port"?

Thanks,

--Justin





More information about the dev mailing list