[ovs-dev] [patch_v10] vtep: add source node replication support.
Darrell Ball
dlu998 at gmail.com
Tue May 10 23:07:19 UTC 2016
On Tue, May 10, 2016 at 1:39 PM, Justin Pettit <jpettit at ovn.org> wrote:
>
> > On May 7, 2016, at 9:21 AM, Darrell Ball <dlu998 at gmail.com> wrote:
> >
> > This patch 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 clearing the replication mode implicitly sets the
> replication
> > mode back to a default of service node.
>
> When running this through git-log, it's longer than 80 columns, so I'd
> adjust the commit message to 75 columns or so.
>
hmm; it looks like it got introduced after changing the commit message
recently
>
> > diff --git a/vtep/README.ovs-vtep.md b/vtep/README.ovs-vtep.md
> > index 6734dab..14477e7 100644
> > --- a/vtep/README.ovs-vtep.md
> > +++ b/vtep/README.ovs-vtep.md
> > @@ -166,13 +166,37 @@ 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. Direct unknown destinations out a tunnel.
> > +For handling L2 broadcast, multicast and unknown unicast traffic,
> packets
>
> I'd merge that first sentence into the rest of the paragraph.
>
That looks inconsistent with the rest of the list and pretty ugly with the
list item having no title per se.
Its not my first choice but if that going to block the commit, then sure.
>
> > +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.
>
> It might be nice to quote "unknown-dst".
>
It won't hurt to add quotes.
>
> > +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
>
> I think a comma might be called for after "called source node replication".
>
yes, a comma is needed
>
> > +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. Service node replication
>
> Super nit-pick: based on how "service node" and "source node" are written,
> it might be nice to italicize their introduction by surrounding them with
> "_".
>
might as well exercise the italics since we are paying for them :-)
>
> > +mode is considered a basic requirement because it only requires sending
> the
> > +packet to a single transport node. The following configuration is for
>
> I'm not sure the commentary about service node being a basic requirement
> because it's simpler is necessary. If you think it's important, it's fine,
> though.
>
> > +service node replication mode as only a single transport node
> destination
> > +is specified for the unknown-dst address:
> >
> > ```
> > vtep-ctl add-mcast-remote ls0 unknown-dst 10.2.2.2
> > ```
> >
> > -4. Direct unicast destinations out a different tunnel:
> > +
>
> This introduces an extra newline.
>
yes, it does
>
> > +4. Optionally, change the replication mode from a default of
> service_node to
> > + source_node, which can be done at the logical switch level:
>
> The "_" are used to indicate italics, so these modes don't show up
> properly in a markdown viewer. You probably want to escape them.
>
> I've found this markdown previewer to be handy:
>
> http://tmpvar.com/markdown.html
hmm; I thought retext and my browsers would have caught that formatting
I tried the one you pointed to now - thats useful - "_thanks_"
>
>
> > diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
> > index 31ff159..4d7129e 100755
> > --- a/vtep/ovs-vtep
> > +++ b/vtep/ovs-vtep
> >
> > @@ -293,8 +298,31 @@ class Logical_Switch(object):
> >
> > self.remote_macs = remote_macs
> >
> > + replication_mode = vtep_ctl("get logical_switch %s
> replication_mode"
> > + % self.name)
> > +
> > + # Replication mode is an optional column and if it is not set,
> > + # replication mode defaults to service_node.
> > + if replication_mode == "[]":
> > + replication_mode = "service_node"
> > +
> > + # If the logical switch level replication mode has changed then
> > + # update to that value.
> > + replication_mode_change = False
> > + if replication_mode != self.replication_mode:
> > + self.replication_mode = replication_mode
> > + vlog.info("%s replication mode changed to %s" %
> > + (self.name, self.replication_mode))
>
> This has an extra space.
>
remove the space
>
> > + replication_mode_change = True
> > +
> > + unknown_dsts_change = False
> > if (self.unknown_dsts != unknown_dsts):
> > self.unknown_dsts = unknown_dsts
> > + unknown_dsts_change = True
> > +
> > + # If either the replication mode has changed or the unknown
> > + # destinations set has changed, update the flooding decision.
> > + if replication_mode_change is True or unknown_dsts_change is
> True:
> > self.update_flood()
>
> Minor suggestion, but the logic could be slightly simplified by just using
> a single variable to indicate that the flood set should be updated.
>
thats fine
>
> > --- a/vtep/vtep-ctl.c
> > +++ b/vtep/vtep-ctl.c
> > @@ -335,6 +335,8 @@ Logical Switch commands:\n\
> > bind-ls PS PORT VLAN LS bind LS to VLAN on PORT\n\
> > unbind-ls PS PORT VLAN unbind logical switch on VLAN from PORT\n\
> > list-bindings PS PORT list bindings for PORT on PS\n\
> > + set-ls-replication-mode LS MODE set replication mode on LS\n\
> > + get-ls-replication-mode LS get replication mode on LS\n\
>
> I believe the commands don't actually have "ls" in them.
>
thats left over from when I wanted to differentiate _ls vs _ps
>
> > @@ -2459,6 +2496,10 @@ 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},
>
> I'd recommend making the functions have essentially the same name as the
> command (ie., drop the "ls").
>
you can remove the _ls; its an outdated qualifier for the name
>
> > diff --git a/vtep/vtep.xml b/vtep/vtep.xml
> > index a3a6988..f357edb 100644
> > --- a/vtep/vtep.xml
> > +++ b/vtep/vtep.xml
> > @@ -754,6 +776,36 @@
> > </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 the only option prior to
> > + version 1.6.0 of the VTEP schema. Service node replication
> mode is
> > + considered a basic requirement because it only requires sending
> the
> > + packet to a single transport node.
>
> We don't normally reference the version number, since it's not
> user-visible. This change is backwards compatible, so there's really no
> need to make an exception here. If people are really curious about when it
> was introduced, they can look at the git history. This feature might be
> worth adding a NEWS item, which would also make it easier to find when it
> was introduced.
>
alright; if its not user-visible, then not much point including it .....
>
> Let me know if you're happy with the diff below. If so, then I'll apply
> this to master.
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-=-=-
>
>
> diff --git a/vtep/README.ovs-vtep.md b/vtep/README.ovs-vtep.md
> index 14477e7..c5956b7 100644
> --- a/vtep/README.ovs-vtep.md
> +++ b/vtep/README.ovs-vtep.md
> @@ -166,16 +166,16 @@ vtep-ctl bind-ls br0 p0 0 ls0
> vtep-ctl set Logical_Switch ls0 tunnel_key=33
> ```
>
> -3. Direct unknown destinations out a tunnel.
> -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
> +3. Direct unknown destinations out a tunnel. 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
> +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. Service node replication
> @@ -188,9 +188,9 @@ is specified for the unknown-dst address:
> vtep-ctl add-mcast-remote ls0 unknown-dst 10.2.2.2
> ```
>
> -
> -4. Optionally, change the replication mode from a default of service_node
> to
> - source_node, which can be done at the logical switch level:
> +4. 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
> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
> index 4d7129e..a8ffb66 100755
> --- a/vtep/ovs-vtep
> +++ b/vtep/ovs-vtep
> @@ -308,21 +308,20 @@ class Logical_Switch(object):
>
> # If the logical switch level replication mode has changed then
> # update to that value.
> - replication_mode_change = False
> + update_flood_set = False
> if replication_mode != self.replication_mode:
> self.replication_mode = replication_mode
> vlog.info("%s replication mode changed to %s" %
> - (self.name, self.replication_mode))
> - replication_mode_change = True
> + (self.name, self.replication_mode))
> + update_flood_set = True
>
> - unknown_dsts_change = False
> if (self.unknown_dsts != unknown_dsts):
> self.unknown_dsts = unknown_dsts
> - unknown_dsts_change = True
> + update_flood_set = True
>
> # If either the replication mode has changed or the unknown
> # destinations set has changed, update the flooding decision.
> - if replication_mode_change is True or unknown_dsts_change is True:
> + if update_flood_set is True:
> self.update_flood()
>
> def update_stats(self):
> diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
> index ccd0df3..5c18971 100644
> --- a/vtep/vtep-ctl.c
> +++ b/vtep/vtep-ctl.c
> @@ -335,8 +335,8 @@ Logical Switch commands:\n\
> bind-ls PS PORT VLAN LS bind LS to VLAN on PORT\n\
> unbind-ls PS PORT VLAN unbind logical switch on VLAN from PORT\n\
> list-bindings PS PORT list bindings for PORT on PS\n\
> - set-ls-replication-mode LS MODE set replication mode on LS\n\
> - get-ls-replication-mode LS get replication mode on LS\n\
> + set-replication-mode LS MODE set replication mode on LS\n\
> + get-replication-mode LS get replication mode on LS\n\
> \n\
> Logical Router commands:\n\
> add-lr LR create a new logical router named LR\n\
> @@ -1528,7 +1528,7 @@ cmd_unbind_ls(struct ctl_context *ctx)
> }
>
> static void
> -cmd_set_ls_replication_mode(struct ctl_context *ctx)
> +cmd_set_replication_mode(struct ctl_context *ctx)
> {
> struct vtep_ctl_context *vtepctl_ctx = vtep_ctl_context_cast(ctx);
> struct vtep_ctl_lswitch *ls;
> @@ -1548,7 +1548,7 @@ cmd_set_ls_replication_mode(struct ctl_context *ctx)
> }
>
> static void
> -cmd_get_ls_replication_mode(struct ctl_context *ctx)
> +cmd_get_replication_mode(struct ctl_context *ctx)
> {
> struct vtep_ctl_context *vtepctl_ctx = vtep_ctl_context_cast(ctx);
> struct vtep_ctl_lswitch *ls;
> @@ -2497,9 +2497,9 @@ static const struct ctl_command_syntax
> vtep_commands[] = {
> {"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},
> + cmd_set_replication_mode, NULL, "", RW},
> {"get-replication-mode", 1, 1, "LS", pre_get_info,
> - cmd_get_ls_replication_mode, NULL, "", RO},
> + cmd_get_replication_mode, NULL, "", RO},
>
> /* Logical Router commands. */
> {"add-lr", 1, 1, NULL, pre_get_info, cmd_add_lr, NULL, "--may-exist",
> RW},
> diff --git a/vtep/vtep.xml b/vtep/vtep.xml
> index f357edb..ad014ab 100644
> --- a/vtep/vtep.xml
> +++ b/vtep/vtep.xml
> @@ -789,10 +789,9 @@
> 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 the only option prior to
> - version 1.6.0 of the VTEP schema. Service node replication mode
> is
> - considered a basic requirement because it only requires sending
> the
> - packet to a single transport node.
> + Service node replication mode is the default and considered a
> + basic requirement because it only requires sending the packet to
> + a single transport node.
> </p>
>
> <column name="replication_mode">
>
>
>
More information about the dev
mailing list