> On Jun 23, 2016, at 12:34 PM, Russell Bryant <russ...@ovn.org> 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 "l2gateway" 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. An l2gateway port still
> uses tunnels between all hypervisors, and packets only go to/from the
> specified physical network as needed via the chassis the l2gateway port
> is bound to.
>
> - An l2gateway 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.
Do you think it's worth mentioning this in the documentation?
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index a07c327..dcfcd23 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -154,6 +154,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, "l2gateway")
> + && 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. */
I'd specify "L2 gateway in the comments above, since right below there's the L3
gateway.
> + sset_add(all_lports, binding_rec->logical_port);
> + add_local_datapath(local_datapaths, binding_rec);
> } else if (chassis_rec && binding_rec->chassis == chassis_rec
> && strcmp(binding_rec->type, "gateway")) {
> if (ctx->ovnsb_idl_txn) {
...
> + <group title="Options for l2gateway ports">
> + <p>
> + These options apply when <ref column="type"/> is
> + <code>l2gateway</code>.
> + </p>
> +
> + <column name="options" key="network_name">
> + Required. The name of the network to which the
> <code>l2gateway</code>
> + port is connected. The gateway, via <code>ovn-controller</code>,
> + uses its local configuration to determine exactly how to connect to
> + this network.
> + </column>
> + </group>
It looks like L3 gateways are bound by setting the "chassis" option in logical
routers, but L2 gateways are bound by setting "ovn-bridge-mappings" in the
Southbound database. It's a bit unfortunate that we're using pretty different
methods for similar actions. Do you think there's a way that we can make them
more similar?
I know it's a hot-button issue, but I wonder if it's worth dusting off the
logical-physical separation conversation again.
> diff --git a/ovn/controller/ovn-controller.8.xml
> b/ovn/controller/ovn-controller.8.xml
> index 1ee3a6e..228a8cd 100644
> --- a/ovn/controller/ovn-controller.8.xml
> +++ b/ovn/controller/ovn-controller.8.xml
> ...
> @@ -199,6 +199,29 @@
> </dd>
>
> <dt>
> + <code>external-ids:ovn-gateway-port</code> in the <code>Port</code>
> + table
> + </dt>
> + <dd>
> + <p>
> + The presence of this key identifies a patch port as one created by
> + <code>ovn-controller</code> to connect the integration bridge and
> + another bridge to implement a <code>gateway</code> logical port.
> + Its value is the name of the logical port with <code>type</code>
> + set to <code>gateway</code> that the port implements. See
> + <code>external_ids:ovn-bridge-mappings</code>, above, for more
> + information.
> + </p>
> +
> + <p>
> + Each <code>gateway</code> logical port is implemented as a pair
> + of patch ports, one in the integration bridge, one in a different
> + bridge, with the same <code>external-ids:ovn-gateway-port</code>
> + value.
> + </p>
Should these "gateway" references be renamed "l2gateway"?
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> index 652466b..edf3baf 100644
> --- a/ovn/controller/patch.c
> +++ b/ovn/controller/patch.c
> ...
> @@ -195,31 +197,40 @@ add_bridge_mappings(struct controller_ctx *ctx,
> continue;
> }
> ld->localnet_port = binding;
> + patch_port_id = "ovn-localnet-port";
> + } else if (!strcmp(binding->type, "l2gateway")) {
> + if (!binding->chassis || strcmp(chassis_id,
> binding->chassis->name)) {
> + /* This gateway port is not bound to this chassis, so we
> should
> + * not create any patch ports for it. */
> + continue;
> + }
The usual comments about referring to it as "L2 gateway". I'll be pointing out
a few more instances of this, but you might do a general pass to clarify spots
that just refer to "gateway".
> + patch_port_id = "ovn-gateway-port";
I think it might be clearer to call this "ovs-l2gateway-port".
> @@ -327,8 +338,9 @@ patch_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
> struct shash existing_ports = SHASH_INITIALIZER(&existing_ports);
> const struct ovsrec_port *port;
> OVSREC_PORT_FOR_EACH (port, ctx->ovs_idl) {
> - if (smap_get(&port->external_ids, "ovn-localnet-port") ||
> - smap_get(&port->external_ids, "ovn-logical-patch-port")) {
> + if (smap_get(&port->external_ids, "ovn-localnet-port")
> + || smap_get(&port->external_ids, "ovn-gateway-port")
> + || smap_get(&port->external_ids, "ovn-logical-patch-port")) {
"ovn-l2gateway-port"?
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 85528e0..d83048e 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -169,6 +169,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id
> mff_ovn_geneve,
>
> const char *localnet = smap_get(&port_rec->external_ids,
> "ovn-localnet-port");
> + const char *gateway = smap_get(&port_rec->external_ids,
> + "ovn-gateway-port");
Ditto. Also may want to rename the variable.
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index e9353f3..0387ed1 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1295,10 +1295,39 @@ tcp.flags = RST;
> </column>
>
> <column name="chassis">
> - The physical location of the logical port. To successfully identify
> a
> - chassis, this column must be a <ref table="Chassis"/> record. This
> is
> - populated by
> - <code>ovn-controller</code>/<code>ovn-controller-vtep</code>.
> + The meaning of this column depends on the value of the <ref
> column="type"/>
> + column. This is the meaning for each <ref column="type"/>
> +
> + <dl>
> + <dt>(empty string)</dt>
> + <dd>
> + The physical location of the logical port. To successfully
> identify a
> + chassis, this column must be a <ref table="Chassis"/> record.
> This is
> + populated by <code>ovn-controller</code>.
> + </dd>
> +
> + <dt>vtep</dt>
> + <dd>
> + The physical location of the hardware_vtep gateway. To
> successfully
> + identify a chassis, this column must be a <ref table="Chassis"/>
> record.
> + This is populated by <code>ovn-controller-vtep</code>.
> + </dd>
> +
> + <dt>localnet</dt>
> + <dd>
> + Always empty. A localnet port is realized on every chassis that
> has
> + connectivity to the corresponding physical network.
> + </dd>
> +
> + <dt>l2gateway</dt>
> + <dd>
> + The physical location of this L2 gateway. To successfully
> identify a
> + chassis, this column must be a <ref table="Chassis"/> record.
> + This is populated by an entity external to OVN, either manually
> or by
> + a CMS.
> + </dd>
It looks like we now have a "gateway" port type. First, we should probably
rename that. (l3gateway?) Second, you might want to add that to your list.
> @@ -1362,6 +1391,14 @@ tcp.flags = RST;
> to model direct connectivity to an existing network.
> </dd>
>
> + <dt><code>l2gateway</code></dt>
> + <dd>
> + A connection to a physical network. The chassis this
> + <ref table="Port_Binding"/> is bound to will serve as
> + an L2 gateway to the network named by
> + <ref column="options"
> table="Port_Binding"/>:<code>network_name</code>.
> + </dd>
As I mentioned earlier, there's now a "gateway" type. In addition to possibly
change the name, I think it would be good to clearly distinguish between the
two.
> +
> <dt><code>vtep</code></dt>
> <dd>
> A port to a logical switch on a VTEP gateway chassis. In order to
> @@ -1444,6 +1481,36 @@ tcp.flags = RST;
> </column>
> </group>
>
> + <group title="Gateway Options">
I think this is defining a second group of "Gateway Options". I assume you'll
want to prepend "L3" on the existing one and "L2" to this one.
> + <p>
> + These options apply to logical ports with <ref column="type"/> of
> + <code>l2gateway</code>.
> + </p>
> +
> + <column name="options" key="network_name">
> + Required. <code>ovn-controller</code> uses the configuration entry
> + <code>ovn-bridge-mappings</code> to determine how to connect to this
> + network. <code>ovn-bridge-mappings</code> is a list of network names
> + mapped to a local OVS bridge that provides access to that network.
> An
> + example of configuring <code>ovn-bridge-mappings</code> would be:
> +
> + <pre>$ ovs-vsctl set open .
> external-ids:ovn-bridge-mappings=physnet1:br-eth0,physnet2:br-eth1</pre>
Does it work in pratice to set more than one physical network?
> @@ -1501,7 +1568,8 @@ tcp.flags = RST;
>
> <p>
> This column is used for a different purpose when <ref
> column="type"/>
> - is <code>localnet</code> (see <code>Localnet Options</code>,
> above).
> + is <code>localnet</code> (see <code>Localnet Options</code>, above)
> + or <code>l2gateway</code> (see <code>Gateway Options</code>,
> above).
You'll want to update this to "L2 Gateway Options", I assume.
Thanks for implementing this. Sorry for taking so long to review it; I realize
that most of my feedback is related to L3 gateway going in before this work
that you did months and months ago. Still, I think it would be good to try to
be very clear about L2 vs L3 vs vtep gateways.
As for interfacing with L2 vs L3 gateways, I'm fine if you want to check this
in using the bridge mappings method. However, I do think it would be good to
start a conversation on whether we can come up with a consistent way to handle
logical and physical configuration.
Acked-by: Justin Pettit <jpet...@ovn.org>
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
