On Fri, May 6, 2016 at 5:57 PM, Justin Pettit <jpet...@ovn.org> wrote:
>
> > 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.
>
I actually did not think it mattered.
series == set, which could have a single member :-)
I'll remove the series part.
>
> > 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.
>
agreed; it bothered me as well and I forgot to revisit
>
> > +
> > +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.
>
sure; the context here does not add much c.f. the man page for reset and get
>
> > +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.
>
sure
>
> > +
> > +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.
>
I have the same opinion about "prior versions"
However; the wording "was the only option for prior versions of the VTEP
schema" came from a previous suggestion.
I will remove it and revisit if it becomes contentious
>
> > +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.
>
thanks
>
> > 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".
>
Yes, it can be removed; it creep in somehow after discussions regarding
possible additional modes.
>
> > +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.
>
Yes, it can be removed; it creep in somehow after discussions regarding
possible additional modes.
>
> > +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"?
>
hmm, "An empty reply for replication mode implies a default of
"service_node"".
>
> > 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.
>
thanks
>
> > @@ -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".
>
I would never have expected for the word "reset" used for "set back to
default" to cause such a kerfuffle.
I understand the concern about prior art, however.
I think I will just remove it since there are already a couple ways for the
operator to set back to
service mode - using the set-replication-mode command and the raw database
wrapper; it is less code too.
>
>
> > 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/
>
s/its/"it is"
>
> > @@ -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.
>
I have the same opinion about "prior versions"
However; the wording "was the only option for prior versions of the VTEP
schema" was a previous suggestion
I will remove it and revisit if it becomes contentious.
>
> Thanks,
>
> --Justin
>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
