> On May 4, 2016, at 12:03 PM, Darrell Ball <dlu...@gmail.com> wrote:
>
> This patch series updates the vtep schema, vtep-ctl commands and vtep
It's not really a series. Even if it were, when looking at the commit history
once its committed, it wouldn't show as a series.
> diff --git a/vtep/README.ovs-vtep.md b/vtep/README.ovs-vtep.md
> index 6734dab..86eeb0f 100644
> --- a/vtep/README.ovs-vtep.md
> +++ b/vtep/README.ovs-vtep.md
> @@ -166,13 +166,58 @@ vtep-ctl bind-ls br0 p0 0 ls0
> vtep-ctl set Logical_Switch ls0 tunnel_key=33
> ```
>
> -3. Direct unknown destinations out a tunnel:
> +3. Optionally, change the replication mode from a default of service_node to
> + source_node, which can be done at the logical switch level:
> +
> + ```
> +vtep-ctl set-replication-mode ls0 source_node
> + ```
I would swap steps 3 and 4, since the replication mode hasn't been defined yet.
> +
> +The replication mode can also be reset back to the default of service node
> +replication, if needed, at the logical switch level:
> +
> + ```
> +vtep-ctl reset-replication-mode ls0
> + ```
> +
> +The replication mode may be explicitly set to the default value of
> +service_node replication, via the set command, if needed:
> +
> + ```
> +vtep-ctl set-replication-mode ls0 service_node
> + ```
> +
> +The replication mode can also be queried using the command:
> +
> + ```
> +vtep-ctl get-replication-mode ls0
> + ```
I think all this discussion about resetting and retrieving the replication mode
can be dropped. This should be a quick reference. The man page describes
these commands well enough.
> +4. Direct unknown destinations out a tunnel:
The colon is usually followed by the command to run, but in this case, it's
just a further descriptive paragraph. I'd just replace it with a period and
make it part of the same paragraph.
> +
> +For handling L2 broadcast, multicast and unknown unicast traffic, packets
> +can be sent to all members of a logical switch referenced by a physical
> +switch. The unknown-dst address below is used to represent these packets.
> +There are different modes to replicate the packets. The default mode of
> +replication is to send the traffic to a service node, which can be a
> +hypervisor, server or appliance, and let the service node handle
> +replication to other transport nodes (hypervisors or other VTEP physical
> +switches). This mode is called service node replication. An alternate
> +mode of replication, called source node replication involves the source
> +node sending to all other transport nodes. Hypervisors are always
> +responsible for doing their own replication for locally attached VMs in
> +both modes. Service node mode is the default and was the only option for
> +prior versions of the VTEP schema. Service node replication mode is
I would drop reference to the prior versions of the VTEP schema, because it
will become meaningless over time.
> +considered a basic requirement because it only requires sending the
> +packet to a single transport node. The following configuration is for
> +service node replication mode as only a single transport node destination
> +is specified for the unknown-dst address.
I think you can put the colon here.
> diff --git a/vtep/vtep-ctl.8.in b/vtep/vtep-ctl.8.in
> index 129c7ed..fa0c242 100644
> --- a/vtep/vtep-ctl.8.in
> +++ b/vtep/vtep-ctl.8.in
> @@ -195,6 +195,37 @@ combination on the physical switch \fIpswitch\fR.
> List the logical switch bindings for \fIport\fR on the physical switch
> \fIpswitch\fR.
> .
> +.IP "\fBset\-replication\-mode \fIlswitch replication\-mode\fR"
> +Set logical switch \fIlswitch\fR replication mode to
> +\fIreplication\-mode\fR; the only valid values presently for replication
I think you can drop "presently".
> +mode are "service_node" and "source_node".
> +.
> +For handling L2 broadcast, multicast and unknown unicast traffic,
> +packets can be sent to all members of a logical switch referenced by
> +a physical switch. There are different modes to replicate the
> +packets. The default mode of replication is to send the traffic to
> +a service node, which can be a hypervisor, server or appliance, and
> +let the service node handle replication to other transport nodes
> +(hypervisors or other VTEP physical switches). This mode is called
> +service node replication. An alternate mode of replication, called
> +source node replication involves the source node sending to all
> +other transport nodes. Hypervisors are always responsible for doing
> +their own replication for locally attached VMs in both modes.
> +Service node mode is the default, if the replication mode is not
> +explicitly set. Service node replication mode is considered a basic
> +requirement because it only requires sending the packet to a single
> +transport node.
> +.
> +.IP "\fBget\-replication\-mode \fIlswitch\fR"
> +Get logical switch \fIlswitch\fR replication mode. The only valid values
> +presently for replication mode are "service_node" and "source_node".
Same here.
> +A return value of NULL indicates the replication mode column is not set
> +and therefore a default of "service_node" is implied.
I'm not sure that "return value of NULL" is exactly a correct description.
Perhaps "an empty response"?
> diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
> index 29d9a17..2e5aab1 100644
> --- a/vtep/vtep-ctl.c
> +++ b/vtep/vtep-ctl.c
>
> +static void
> +cmd_get_ls_replication_mode(struct ctl_context *ctx)
> +{
> + struct vtep_ctl_context *vtepctl_ctx = vtep_ctl_context_cast(ctx);
> + struct vtep_ctl_lswitch *ls;
> + const char *ls_name = ctx->argv[1];
> +
> + vtep_ctl_context_populate_cache(ctx);
> +
> + ls = find_lswitch(vtepctl_ctx, ls_name, true);
> + ds_put_format(&ctx->output, "%s\n", ls->ls_cfg->replication_mode);
> +
> +}
There's an unnecessary blank line here.
> @@ -2459,6 +2513,12 @@ static const struct ctl_command_syntax vtep_commands[]
> = {
> {"list-bindings", 2, 2, NULL, pre_get_info, cmd_list_bindings, NULL, "",
> RO},
> {"bind-ls", 4, 4, NULL, pre_get_info, cmd_bind_ls, NULL, "", RO},
> {"unbind-ls", 3, 3, NULL, pre_get_info, cmd_unbind_ls, NULL, "", RO},
> + {"set-replication-mode", 2, 2, "LS MODE", pre_get_info,
> + cmd_set_ls_replication_mode, NULL, "", RW},
> + {"get-replication-mode", 1, 1, "LS", pre_get_info,
> + cmd_get_ls_replication_mode, NULL, "", RO},
> + {"reset-replication-mode", 1, 1, "LS", pre_get_info,
> + cmd_reset_ls_replication_mode, NULL, "", RW},
As I mentioned before, we use "del" elsewhere in the codebase. I understand
that you think it's less clear, but I think introducing new terms will be more
confusing. Alternatively, you could drop this entirely, since people can just
run "vtep-ctl clear Logical_Switch <LS> replication-mode".
> diff --git a/vtep/vtep.xml b/vtep/vtep.xml
> index a3a6988..e2d7ad6 100644
> --- a/vtep/vtep.xml
> +++ b/vtep/vtep.xml
> @@ -357,6 +357,28 @@
> Indicates that an error has occurred in the switch but that no
> more specific information is available.
> </column>
> +
> + <column name="switch_fault_status"
> + key="unsupported_source_node_replication">
> + Indicates that the requested source node replication mode cannot be
> + supported by the physical switch; this specifically means in this
> + context that the physical switch lacks the capability to support
> + source node replication mode. This error occurs when a controller
> + attempts to set source node replication mode for one of the logical
> + switches that the physical switch is keeping context for. An NVC
> + that observes this error should take appropriate action (for example
> + reverting the logical switch to service node replication mode).
> + It is recommended that an NVC be proactive and test for support of
> + source node replication by using a test logical switch on vtep
> + physical switch nodes and then trying to change the replication mode
> + to source node on this logical switch, checking for error. The NVC
> + could remember this capability per vtep physical switch. Using
> + mixed replication modes on a given logical switch is not recommended.
> + Service node replication mode is considered a basic requirement
> + since it only requires sending a packet to a single transport node,
> + hence its not expected that a switch should report that service node
s/its/it's/
> @@ -754,6 +776,37 @@
> </column>
> </group>
>
> + <group title="Replication Mode">
> + <p>
> + For handling L2 broadcast, multicast and unknown unicast traffic,
> + packets can be sent to all members of a logical switch referenced by
> + a physical switch. There are different modes to replicate the
> + packets. The default mode of replication is to send the traffic to
> + a service node, which can be a hypervisor, server or appliance, and
> + let the service node handle replication to other transport nodes
> + (hypervisors or other VTEP physical switches). This mode is called
> + service node replication. An alternate mode of replication, called
> + source node replication involves the source node sending to all
> + other transport nodes. Hypervisors are always responsible for doing
> + their own replication for locally attached VMs in both modes.
> + Service node mode is the default and was the only option for prior
> + versions of the schema. Service node replication mode is considered
Once again, I wouldn't mention prior versions of the schema.
Thanks,
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
