[ovs-dev] [patch_v9] vtep: add source node replication support.

Justin Pettit jpettit at ovn.org
Sat May 7 00:57:26 UTC 2016


> On May 4, 2016, at 12:03 PM, Darrell Ball <dlu998 at 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





More information about the dev mailing list