> On Apr 28, 2016, at 8:46 PM, Darrell Ball <dlu...@gmail.com> wrote:
>
> This patch series updates the vtep schema, vtep-ctl commands and vtep
> simulator to support source node replication in addition to service node
> replication per logical switch. The default replication mode is service node
> as that was the only mode previously supported. Source node replication
> mode is optionally configurable and resetting the replication mode implicitly
> sets the replication mode back to a default of service node.
>
> Signed-off-by: Darrell Ball <dlu...@gmail.com>
I believe Bruce acked v6, so if you didn't change the patch in ways that would
likely cause him to revoke that, you can put his Acked-by with the new patch.
It makes it easier to track what's been already been reviewed--especially in
large patch series that may have patches acked at different times.
(That said, if you and Bruce agree with my suggested changes, we will need a
new ack.)
> diff --git a/tests/vtep-ctl.at b/tests/vtep-ctl.at
> index 99e97e8..d2323a0 100644
> --- a/tests/vtep-ctl.at
> +++ b/tests/vtep-ctl.at
> @@ -318,6 +318,23 @@ CHECK_LSWITCHES([a])
> VTEP_CTL_CLEANUP
> AT_CLEANUP
>
> +AT_SETUP([add-ls a, set-ls-replication-mode a source_node])
> +AT_KEYWORDS([vtep-ctl])
> +VTEP_CTL_SETUP
> +AT_CHECK([RUN_VTEP_CTL(
> + [add-ls a],[set-ls-replication-mode a source_node])],
Thanks for writing tests! It would probably be good to make sure that this
change actually modified the configuration, though.
> diff --git a/vtep/README.ovs-vtep.md b/vtep/README.ovs-vtep.md
> index 6734dab..74900f1 100644
> --- a/vtep/README.ovs-vtep.md
> +++ b/vtep/README.ovs-vtep.md
> ...
> +4. The alternate replication mode can also be reset back to the default of
> + service node replication, at the logical switch level:
> +
> + ```
> +vtep-ctl reset-ls-replication-mode ls0
> + ```
I wonder about the utility of having a reset at all. Regardless, I think it's
likely confusing to have it listed as a step in the instructions about setting
the emulator up to work with an NVC. I'd just drop this step.
> diff --git a/vtep/vtep-ctl.8.in b/vtep/vtep-ctl.8.in
> index 129c7ed..25deebd 100644
> --- a/vtep/vtep-ctl.8.in
> +++ b/vtep/vtep-ctl.8.in
> @@ -195,6 +195,15 @@ combination on the physical switch \fIpswitch\fR.
> List the logical switch bindings for \fIport\fR on the physical switch
> \fIpswitch\fR.
> .
> +.IP "\fBset\-ls\-replication\-mode \fIlswitch replication\-mode\fR"
> +Set logical switch \fIlswitch\fR alternate replication mode to
> +\fIreplication\-mode\fR; the only valid value presently for alternate
> +replication mode is "source_node".
> +.
> +.IP "\fBreset\-ls\-replication\-mode \fIlswitch\fR"
> +Reset a logical switch \fIlswitch\fR alternate replication mode to the
> +default of "service_node".
In vtep-ctl, I would drop support for reset-ls-replication entirely. I'd
suggest that you add a get/set/del group of actions. If someone chooses
del-ls-replication, then it would go back to the default. This is how
fail-mode is handled in ovs-vsctl, which has similar requirements.
> diff --git a/vtep/vtep.ovsschema b/vtep/vtep.ovsschema
> index 533fd2e..a0e27fd 100644
> --- a/vtep/vtep.ovsschema
> +++ b/vtep/vtep.ovsschema
> @@ -96,6 +96,11 @@
> "name": {"type": "string"},
> "description": {"type": "string"},
> "tunnel_key": {"type": {"key": "integer", "min": 0, "max": 1}},
> + "alt_replication_mode": {
What about just calling this "replication_mode"? It feels like we're casting
judgment on whether source or service node replication is better.
> + "type": {
> + "key": {
> + "enum": ["set", ["source_node"]],
> + "type": "string"},"min": 0, "max": 1}},
I would add "service_node" to the enums. This way people can choose one of the
two modes, and if they don't specify one, we can use a default. This is
similar to how we handle "fail_mode" for ovs-vswitchd.
> @@ -296,4 +301,4 @@
> "ephemeral": true}},
> "indexes": [["target"]],
> "isRoot": false}},
> - "version": "1.5.1"}
> + "version": "1.6.1"}
I would have expected this to be "1.6.0".
> diff --git a/vtep/vtep.xml b/vtep/vtep.xml
> index a3a6988..a2fab02 100644
> --- a/vtep/vtep.xml
> +++ b/vtep/vtep.xml
> @@ -357,6 +357,24 @@
> 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
Not supporting source node replication is certainly the only possibility with
current vtep devices, but will that always be the case? It seems like the user
should be able to figure out that the configured value doesn't work, so the
opposite one should. I would have thought this language would be a bit more
general.
> + <group title="Alternate Replication Mode">
> + <p>
> + For handling broadcast, multicast (in default manner) 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. Source node mode is an alternate replication mode
> + that may be configured using this column.
> + </p>
> +
> + <column name="alt_replication_mode">
> + <p>
> + This optional column defines the alternate replication mode per
> + <ref table="Logical_Switch"/>. There is one valid value presently,
> + <code>source_node</code>.
> + </p>
Once again, calling this mode "alternate" feels negative to me.
> @@ -911,9 +958,12 @@
>
> <column name="locator_set">
> The physical locator set to be used to reach this MAC address. In
> - this table, the physical locator set will be either a service node IP
> - address or a set of tunnel IP addresses of hypervisors (and
> - potentially other VTEPs).
> + this table, the physical locator set will be either a set of service
> + node when service node replication is used or the set of transport
s/node/nodes/
Thanks,
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
