diff mbox

[ovs-dev,v5] ovn: Add software l2 gateway.

Message ID CA+0q_Pi4LE-zb=BQYrkPtCVSMsRXPtw_GTakRshsATQ3vMfMkQ@mail.gmail.com
State Accepted
Headers show

Commit Message

Russell Bryant July 1, 2016, 9 p.m. UTC 
On Fri, Jun 24, 2016 at 9:29 PM, Justin Pettit <jpettit@ovn.org> wrote:

>
> > On Jun 23, 2016, at 12:34 PM, Russell Bryant <russell@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?
>

It's mentioned in ovn-sb.xml.  It may not be prominent enough.  The docs
for the "chassis" column of Port_Binding were updated to reflect how the
column is used for different port types, including "l2gateway".


>
> > 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.


Done.


>
> > +            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?
>

L2 gateways are bound by setting the "chassis" column in the corresponding
Port_Binding row of the southbound database.  The "ovn-bridge-mappings"
value is not related to chassis binding, but is used to know which local
OVS bridge provides connectivity to the L2 network the l2gateway port is
configured for.

To make them the same, we could support a "chassis" option for
Logical_Switch_Port that has a type of "l2gateway".  Right now we expect
some external entity to set the "chassis" column of the port binding in the
southbound database directly.  This would provide an interface to express
the same thing from the northbound database, which does seem nicer.  I'll
add a note about this to ovn/TODO.


> I know it's a hot-button issue, but I wonder if it's worth dusting off the
> logical-physical separation conversation again.
>

Does the above help clarify?  The "logical-physical separation" proposal
seems to affect L2 and L3 gateways the same.


> > 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"?
>

Yes.  I tried to fix them but I missed this file.

I also changed "ovn-gateway-port" to "ovn-l2gateway-port" here and in the
code.


> > 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".
>

Fixed.


>
> > +            patch_port_id = "ovn-gateway-port";
>
> I think it might be clearer to call this "ovs-l2gateway-port".
>

Agreed, fixed.


>
> > @@ -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"?
>

Changed.


>
> > 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.
>

Done.


>
> > 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.
>

Renaming it sounds good to me.  Guru, what do you think?  I also noticed
that the "gateway" port type isn't documented in ovn-nb.xml yet.

For now, I've at least added the gateway port type to this 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.
>

I tried to help clarify by changing this to "An L2 connection to ...".


>
> > +
> >           <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.
>

Done.


>
> > +      <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?
>

Yes.  There is a test case in tests/ovn-controller.at that tests the
behavior of setting ovn-bridge-mappings like this with 2 networks.


>
> > @@ -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.
>

Done.


> 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.
>

Thanks for the review!

I've done a pass through this to try to catch other cases where "gateway"
should be "L2 gateway".


> 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.
>

OK.  I elaborated in an earlier comment about this and am certainly happy
to talk more about proposals around this.


> Acked-by: Justin Pettit <jpettit@ovn.org>
>

Thanks!  I have applied this patch to master with the following incremental:


 diff --git a/ovn/TODO b/ovn/TODO
index 4f134a4..3f358c2 100644
           <dd>
             The physical location of this L2 gateway.  To successfully
identify a
@@ -1454,7 +1462,7 @@ tcp.flags = RST;

           <dt><code>l2gateway</code></dt>
           <dd>
-            A connection to a physical network.  The chassis this
+            An L2 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>.
@@ -1490,7 +1498,7 @@ tcp.flags = RST;
       </column>
     </group>

-    <group title="Gateway Options">
+    <group title="L3 Gateway Options">
       <p>
         These options apply to logical ports with <ref column="type"/> of
         <code>gateway</code>.
@@ -1542,7 +1550,7 @@ tcp.flags = RST;
       </column>
     </group>

-    <group title="Gateway Options">
+    <group title="L2 Gateway Options">
       <p>
         These options apply to logical ports with <ref column="type"/> of
         <code>l2gateway</code>.
@@ -1630,7 +1638,7 @@ 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)
-          or <code>l2gateway</code> (see <code>Gateway Options</code>,
above).
+          or <code>l2gateway</code> (see <code>L2 Gateway Options</code>,
above).
         </p>
       </column>
     </group>
diff mbox

Patch

--- a/ovn/TODO
+++ b/ovn/TODO
@@ -247,3 +247,14 @@  large.
 ** Support reject action.

 ** Support log option.
+
+* Software L2 gateway
+
+** Support "chassis" option in Logical_Switch_Port with type of
"l2gateway".
+
+   Right now an "l2gateway" port is bound to a chassis by setting the
"chassis"
+   column of the port binding in the southbound database directly.  We
should
+   support a "chassis" option in the "options" column of the
+   "Logical_Switch_Port" in the northbound database.  This would bring
+   "l2gateway" into alignment with how chassis binding is done for L3
gateways
+   (a "chassis" option for Logical_Router).
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index ac30298..8b439a6 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -224,7 +224,7 @@  consider_local_datapath(struct controller_ctx *ctx,
struct shash *lports,
         }
     } else if (!strcmp(binding_rec->type, "l2gateway")
                && binding_rec->chassis == chassis_rec) {
-        /* A locally bound gateway port.
+        /* A locally bound L2 gateway port.
          *
          * ovn-controller does not bind gateway ports itself.
          * Choosing a chassis for a gateway port is left
diff --git a/ovn/controller/ovn-controller.8.xml
b/ovn/controller/ovn-controller.8.xml
index 228a8cd..3fda8e7 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -199,24 +199,24 @@ 
       </dd>

       <dt>
-        <code>external-ids:ovn-gateway-port</code> in the <code>Port</code>
+        <code>external-ids:ovn-l2gateway-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.
+          another bridge to implement a <code>l2gateway</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
+          set to <code>l3gateway</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
+          Each <code>l2gateway</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>
+          bridge, with the same
<code>external-ids:ovn-l2gateway-port</code>
           value.
         </p>
       </dd>
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index c6782d6..589529e 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -207,14 +207,15 @@  add_bridge_mappings(struct controller_ctx *ctx,
             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. */
+            if (!binding->chassis
+                || strcmp(chassis_id, binding->chassis->name)) {
+                /* This L2 gateway port is not bound to this chassis,
+                 * so we should not create any patch ports for it. */
                 continue;
             }
-            patch_port_id = "ovn-gateway-port";
+            patch_port_id = "ovn-l2gateway-port";
         } else {
-            /* not a localnet or gateway port. */
+            /* not a localnet or L2 gateway port. */
             continue;
         }

@@ -347,7 +348,7 @@  patch_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
     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-gateway-port")
+            || smap_get(&port->external_ids, "ovn-l2gateway-port")
             || smap_get(&port->external_ids, "ovn-logical-patch-port")) {
             shash_add(&existing_ports, port->name, port);
         }
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index d83048e..d7637f2 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -169,8 +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");
+        const char *l2gateway = smap_get(&port_rec->external_ids,
+                                        "ovn-l2gateway-port");
         const char *logpatch = smap_get(&port_rec->external_ids,
                                         "ovn-logical-patch-port");

@@ -193,9 +193,9 @@  physical_run(struct controller_ctx *ctx, enum
mf_field_id mff_ovn_geneve,
                 /* localnet patch ports can be handled just like VIFs. */
                 simap_put(&localvif_to_ofport, localnet, ofport);
                 break;
-            } else if (is_patch && gateway) {
-                /* gateway patch ports can be handled just like VIFs. */
-                simap_put(&localvif_to_ofport, gateway, ofport);
+            } else if (is_patch && l2gateway) {
+                /* L2 gateway patch ports can be handled just like VIFs. */
+                simap_put(&localvif_to_ofport, l2gateway, ofport);
                 break;
             } else if (is_patch && logpatch) {
                 /* Logical patch ports can be handled just like VIFs. */
@@ -275,12 +275,12 @@  physical_run(struct controller_ctx *ctx, enum
mf_field_id mff_ovn_geneve,
          *       OpenFlow port for the VIF.  'tun' will be NULL.
          *
          *       The same logic handles logical patch ports, as well as
-         *       localnet and gateway patch ports.
+         *       localnet and L2 gateway patch ports.
          *
          *       For a container nested inside a VM and accessible via a
VLAN,
          *       'tag' is the VLAN ID; otherwise 'tag' is 0.
          *
-         *       For a localnet or gateway patch port, if a VLAN ID was
+         *       For a localnet or L2 gateway patch port, if a VLAN ID was
          *       configured, 'tag' is set to that VLAN ID; otherwise 'tag'
is 0.
          *
          *     - If the port is on a remote chassis, the OpenFlow port for
a
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 17881f5..02d34d5 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -188,7 +188,7 @@ 

         <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>,
+          port is connected.  The L2 gateway, via
<code>ovn-controller</code>,
           uses its local configuration to determine exactly how to connect
to
           this network.
         </column>
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index c2574e9..4814b0a 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1380,6 +1380,14 @@  tcp.flags = RST;
             connectivity to the corresponding physical network.
           </dd>

+          <dt>gateway</dt>
+          <dd>
+            The physical location of the L3 gateway.  To successfully
identify a
+            chassis, this column must be a <ref table="Chassis"/> record.
This is
+            populated by <code>ovn-controller</code> based on the value of
+            the <code>options:gateway-chassis</code> column in this table.
+          </dd>
+
           <dt>l2gateway</dt>