Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Thu, Apr 14, 2016 at 1:05 AM, Ben Pfaff wrote:
> On Mon, Apr 04, 2016 at 05:58:28AM -0700, Russell Bryant wrote:
> > This patch implements one approach to using ovn-controller to implement
> > a software l2 gateway between logical and physical networks.
> >
> > A new logical port type called "gateway" is introduced here. It is very
> > close to how localnet ports work, with the following exception:
> >
> > - A localnet port makes OVN use the physical network as the
> > transport between hypervisors instead of tunnels. A gateway port still
> > uses tunnels between all hypervisors, and packets only go to/from the
> > specified physical network as needed via the chassis the gateway port
> > is bound to.
> >
> > - A gateway port also gets bound to a chassis while a localnet port does
> > not. This binding is not done by ovn-controller. It is left as an
> > administrative function. In the case of OpenStack, the Neutron plugin
> > will do this.
> >
> > Signed-off-by: Russell Bryant
>
> This seems reasonable to me. Thanks for working on it.
>
> There's a stray " * " in the middle of the second line here:
>
Oops ... I must have joined 2 lines and forgot to clean it up.
> > + * ovn-controller does not bind gateway ports itself.
> > + * Choosing a chassis * for a gateway port is left
> > + * up to an entity external to OVN. */
> > +sset_add(&all_lports, binding_rec->logical_port);
> > +add_local_datapath(local_datapaths, binding_rec);
> > } else if (binding_rec->chassis == chassis_rec) {
> > if (ctx->ovnsb_idl_txn) {
> > VLOG_INFO("Releasing lport %s from this chassis.",
>
> The treatment of the "chassis" column in the Port_Binding database is
> quite different based on the type of the port:
>
> * For localnet ports, it's always empty.
>
> * For gateway ports, ovn-northd (or whoever adds the port, I
> guess) must set the chassis correctly.
>
My thinking for this was "something external to OVN", at least for now.
For OpenStack, we could have OpenStack choose the chassis.
Letting ovn-northd do it is an interesting idea, though. It sounds worth
considering in a future iteration.
> * For other ports, ovn-controller sets (and un-sets) the
> chassis.
>
> I don't think these differences can be gleaned from the documentation.
> I think that it should be made clear in ovn-sb.xml.
>
Thanks, yes that difference definitely could be more clear.
> I guess that you may have comments from others that you should address,
> but for me:
> Acked-by: Ben Pfaff
>
Thanks for the review! I didn't have any outstanding changes. Justin
asked that I wait for his review, though.
--
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Mon, Apr 04, 2016 at 05:58:28AM -0700, Russell Bryant wrote:
> This patch implements one approach to using ovn-controller to implement
> a software l2 gateway between logical and physical networks.
>
> A new logical port type called "gateway" is introduced here. It is very
> close to how localnet ports work, with the following exception:
>
> - A localnet port makes OVN use the physical network as the
> transport between hypervisors instead of tunnels. A gateway port still
> uses tunnels between all hypervisors, and packets only go to/from the
> specified physical network as needed via the chassis the gateway port
> is bound to.
>
> - A gateway port also gets bound to a chassis while a localnet port does
> not. This binding is not done by ovn-controller. It is left as an
> administrative function. In the case of OpenStack, the Neutron plugin
> will do this.
>
> Signed-off-by: Russell Bryant
This seems reasonable to me. Thanks for working on it.
There's a stray " * " in the middle of the second line here:
> + * ovn-controller does not bind gateway ports itself.
> + * Choosing a chassis * for a gateway port is left
> + * up to an entity external to OVN. */
> +sset_add(&all_lports, binding_rec->logical_port);
> +add_local_datapath(local_datapaths, binding_rec);
> } else if (binding_rec->chassis == chassis_rec) {
> if (ctx->ovnsb_idl_txn) {
> VLOG_INFO("Releasing lport %s from this chassis.",
The treatment of the "chassis" column in the Port_Binding database is
quite different based on the type of the port:
* For localnet ports, it's always empty.
* For gateway ports, ovn-northd (or whoever adds the port, I
guess) must set the chassis correctly.
* For other ports, ovn-controller sets (and un-sets) the
chassis.
I don't think these differences can be gleaned from the documentation.
I think that it should be made clear in ovn-sb.xml.
I guess that you may have comments from others that you should address,
but for me:
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Wed, Apr 6, 2016 at 5:43 PM, Ramu Ramamurthy wrote: > On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant wrote: > > This patch implements one approach to using ovn-controller to implement > > a software l2 gateway between logical and physical networks. > > > > A new logical port type called "gateway" is introduced here. It is very > > close to how localnet ports work, with the following exception: > > > > - A localnet port makes OVN use the physical network as the > > transport between hypervisors instead of tunnels. A gateway port still > > uses tunnels between all hypervisors, and packets only go to/from the > > specified physical network as needed via the chassis the gateway port > > is bound to. > > > > - A gateway port also gets bound to a chassis while a localnet port does > > not. This binding is not done by ovn-controller. It is left as an > > administrative function. In the case of OpenStack, the Neutron plugin > > will do this. > > > > Signed-off-by: Russell Bryant > > --- > > > > Can there be more than 1 gateway to the same l2 network (say for HA) ? > If gateway port G1 on chassis1 and gateway port G2 on chassis2, > Then, when a VM a chassis3 sends a broadcast, it will tunnel to > chassis1, and chassis2, > and then get sent out of G1 and G2 to the same L2 network, ie, Can > that broadcast packet > get sent in duplicate to the physical network ? If so, would you have > to run xSTP on the > gateway ports ? > You can only have a single gateway port for a given network at this stage. -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant wrote: > This patch implements one approach to using ovn-controller to implement > a software l2 gateway between logical and physical networks. > > A new logical port type called "gateway" is introduced here. It is very > close to how localnet ports work, with the following exception: > > - A localnet port makes OVN use the physical network as the > transport between hypervisors instead of tunnels. A gateway port still > uses tunnels between all hypervisors, and packets only go to/from the > specified physical network as needed via the chassis the gateway port > is bound to. > > - A gateway port also gets bound to a chassis while a localnet port does > not. This binding is not done by ovn-controller. It is left as an > administrative function. In the case of OpenStack, the Neutron plugin > will do this. > > Signed-off-by: Russell Bryant > --- > Can there be more than 1 gateway to the same l2 network (say for HA) ? If gateway port G1 on chassis1 and gateway port G2 on chassis2, Then, when a VM a chassis3 sends a broadcast, it will tunnel to chassis1, and chassis2, and then get sent out of G1 and G2 to the same L2 network, ie, Can that broadcast packet get sent in duplicate to the physical network ? If so, would you have to run xSTP on the gateway ports ? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Wed, Apr 6, 2016 at 10:49 AM, Han Zhou wrote: > > > On Wednesday, April 6, 2016, Russell Bryant wrote: > >> >> >> On Wed, Apr 6, 2016 at 3:10 AM, Han Zhou wrote: >> >>> >>> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant wrote: >>> > - Each localnet logical port is implemented as a >>> pair of >>> > - patch ports, one in the integration bridge, one in a >>> different >>> > - bridge, with the same >>> external-ids:ovn-localnet-port >>> > - value. >>> > + Each localnet and gateway logical >>> port >>> > + is implemented as a pair of patch ports, one in the >>> integration >>> > + bridge, one in a different bridge, with the same >>> > + external-ids:ovn-localnet-port value. >>> >>> Is this ovn-localnet-port a typo? >>> >> >> No. It's still using the same external-id value. I did that because >> "gateway" ports share almost all of the code for localnet ports, so it was >> just convenient. >> >> I thought about changing the name to avoid confusion, but hadn't decided >> on exactly what to change yet. >> >> I think it would worth a small code change in patch.c so that they use > different names to avoid confusion. > Sounds good. It'll need some changes in physical.c, as well, I believe. I'll work on it. Thanks, -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Wed, Apr 6, 2016 at 3:10 AM, Han Zhou wrote:
>
> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant wrote:
> > - Each localnet logical port is implemented as a
> pair of
> > - patch ports, one in the integration bridge, one in a different
> > - bridge, with the same
> external-ids:ovn-localnet-port
> > - value.
> > + Each localnet and gateway logical
> port
> > + is implemented as a pair of patch ports, one in the
> integration
> > + bridge, one in a different bridge, with the same
> > + external-ids:ovn-localnet-port value.
>
> Is this ovn-localnet-port a typo?
>
No. It's still using the same external-id value. I did that because
"gateway" ports share almost all of the code for localnet ports, so it was
just convenient.
I thought about changing the name to avoid confusion, but hadn't decided on
exactly what to change yet.
> > diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> > index 6027011..9bcda0d 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -326,7 +326,10 @@ main(int argc, char *argv[])
> > }
> >
> > if (br_int) {
> > -patch_run(&ctx, br_int, &local_datapaths,
> &patched_datapaths);
> > +if (chassis_id) {
>
> Shall we print an error log if chassis_id is NULL?
>
That's a good idea. It's already a problem today if chassis_id is NULL,
but we aren't logging anything about it. I may add that as a separate
patch.
> Otherwise looks good to me. I haven't tested it yet, but it is pretty
> straightforward approach.
>
--
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Wednesday, April 6, 2016, Russell Bryant > wrote:
>
>
> On Wed, Apr 6, 2016 at 3:10 AM, Han Zhou wrote:
>
>>
>> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant wrote:
>> > - Each localnet logical port is implemented as a
>> pair of
>> > - patch ports, one in the integration bridge, one in a
>> different
>> > - bridge, with the same
>> external-ids:ovn-localnet-port
>> > - value.
>> > + Each localnet and gateway logical
>> port
>> > + is implemented as a pair of patch ports, one in the
>> integration
>> > + bridge, one in a different bridge, with the same
>> > + external-ids:ovn-localnet-port value.
>>
>> Is this ovn-localnet-port a typo?
>>
>
> No. It's still using the same external-id value. I did that because
> "gateway" ports share almost all of the code for localnet ports, so it was
> just convenient.
>
> I thought about changing the name to avoid confusion, but hadn't decided
> on exactly what to change yet.
>
> I think it would worth a small code change in patch.c so that they use
different names to avoid confusion.
>
>> > diff --git a/ovn/controller/ovn-controller.c
>> b/ovn/controller/ovn-controller.c
>> > index 6027011..9bcda0d 100644
>> > --- a/ovn/controller/ovn-controller.c
>> > +++ b/ovn/controller/ovn-controller.c
>> > @@ -326,7 +326,10 @@ main(int argc, char *argv[])
>> > }
>> >
>> > if (br_int) {
>> > -patch_run(&ctx, br_int, &local_datapaths,
>> &patched_datapaths);
>> > +if (chassis_id) {
>>
>> Shall we print an error log if chassis_id is NULL?
>>
>
> That's a good idea. It's already a problem today if chassis_id is NULL,
> but we aren't logging anything about it. I may add that as a separate
> patch.
>
>
>> Otherwise looks good to me. I haven't tested it yet, but it is pretty
>> straightforward approach.
>>
>
> --
> Russell Bryant
>
--
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant wrote:
> - Each localnet logical port is implemented as a
pair of
> - patch ports, one in the integration bridge, one in a different
> - bridge, with the same
external-ids:ovn-localnet-port
> - value.
> + Each localnet and gateway logical
port
> + is implemented as a pair of patch ports, one in the integration
> + bridge, one in a different bridge, with the same
> + external-ids:ovn-localnet-port value.
Is this ovn-localnet-port a typo?
> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index 6027011..9bcda0d 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -326,7 +326,10 @@ main(int argc, char *argv[])
> }
>
> if (br_int) {
> -patch_run(&ctx, br_int, &local_datapaths,
&patched_datapaths);
> +if (chassis_id) {
Shall we print an error log if chassis_id is NULL?
Otherwise looks good to me. I haven't tested it yet, but it is pretty
straightforward approach.
--
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] ovn: Add software l2 gateway.
This patch implements one approach to using ovn-controller to implement
a software l2 gateway between logical and physical networks.
A new logical port type called "gateway" is introduced here. It is very
close to how localnet ports work, with the following exception:
- A localnet port makes OVN use the physical network as the
transport between hypervisors instead of tunnels. A gateway port still
uses tunnels between all hypervisors, and packets only go to/from the
specified physical network as needed via the chassis the gateway port
is bound to.
- A gateway port also gets bound to a chassis while a localnet port does
not. This binding is not done by ovn-controller. It is left as an
administrative function. In the case of OpenStack, the Neutron plugin
will do this.
Signed-off-by: Russell Bryant
---
v1->v2:
- Rebase and resolve conflicts with master.
- Fix doc typo when descriping gateway port type.
- Ensure chassis_id is non-NULL before calling patch_run().
ovn/controller/binding.c| 9 ++
ovn/controller/ovn-controller.8.xml | 15 ++--
ovn/controller/ovn-controller.c | 5 +-
ovn/controller/patch.c | 25 --
ovn/controller/patch.h | 3 +-
ovn/controller/physical.c | 10 ++-
ovn/ovn-nb.xml | 19 +
ovn/ovn-sb.xml | 41 -
tests/ovn.at| 164
9 files changed, 270 insertions(+), 21 deletions(-)
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index d3ca9c9..de18017 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -205,6 +205,15 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
}
sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
}
+} else if (!strcmp(binding_rec->type, "gateway")
+ && binding_rec->chassis == chassis_rec) {
+/* A locally bound gateway port.
+ *
+ * ovn-controller does not bind gateway ports itself.
+ * Choosing a chassis * for a gateway port is left
+ * up to an entity external to OVN. */
+sset_add(&all_lports, binding_rec->logical_port);
+add_local_datapath(local_datapaths, binding_rec);
} else if (binding_rec->chassis == chassis_rec) {
if (ctx->ovnsb_idl_txn) {
VLOG_INFO("Releasing lport %s from this chassis.",
diff --git a/ovn/controller/ovn-controller.8.xml
b/ovn/controller/ovn-controller.8.xml
index 1ee3a6e..4c11c4c 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -183,18 +183,19 @@
The presence of this key identifies a patch port as one created by
ovn-controller to connect the integration bridge and
- another bridge to implement a localnet logical port.
- Its value is the name of the logical port with type=localnet that
- the port implements.
+ another bridge to implement a localnet or
+ gateway logical port. Its value is the name of the
+ logical port with type set to localnet
+ or gateway that the port implements.
See external_ids:ovn-bridge-mappings, above,
for more information.
- Each localnet logical port is implemented as a pair of
- patch ports, one in the integration bridge, one in a different
- bridge, with the same external-ids:ovn-localnet-port
- value.
+ Each localnet and gateway logical port
+ is implemented as a pair of patch ports, one in the integration
+ bridge, one in a different bridge, with the same
+ external-ids:ovn-localnet-port value.
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 6027011..9bcda0d 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -326,7 +326,10 @@ main(int argc, char *argv[])
}
if (br_int) {
-patch_run(&ctx, br_int, &local_datapaths, &patched_datapaths);
+if (chassis_id) {
+patch_run(&ctx, br_int, &local_datapaths, &patched_datapaths,
+ chassis_id);
+}
struct lport_index lports;
struct mcgroup_index mcgroups;
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 943ac99..6c666b8 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -134,7 +134,8 @@ static void
add_bridge_mappings(struct controller_ctx *ctx,
const struct ovsrec_bridge *br_int,
struct shash *existing_ports,
-struct hmap *local_datapaths)
+struct hmap *local_datapaths,
+const char *chassis_id)
{
/* Get ovn-bridge-
