[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