[ovs-dev,13/23] ovn: Implement logical patch ports.
diff mbox

Message ID 1444450838-12150-2-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 10, 2015, 4:20 a.m. UTC
This implementation is suboptimal for several reasons.  First, it
creates an OVS port for every OVN logical patch port, not just for the
ones that are actually useful on this hypervisor.  Second, it's
wasteful to create an OVS patch port per OVN logical patch port, when
really there's no benefit to them beyond a way to identify how a
packet ingressed into a logical datapath.

There are two obvious ways to improve the situation here, by modifying
OVS:

    1. Add a way to configure in OVS which fields are preserved on a
       hop across an OVS patch port.  If MFF_LOG_DATAPATH and
       MFF_LOG_INPORT were preserved, then only a single pair of OVS
       patch ports would be required regardless of the number of OVN
       logical patch ports.

    2. Add a new OpenFlow extension action modeled on "resubmit" that
       also saves and restores the packet data and metadata (the
       inability to do this is the only reason that "resubmit" can't
       be used already).  Or add OpenFlow extension actions to
       otherwise save and restore packet data and metadata.

We should probably choose one of those in the medium to long term, but
I don't know which one.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 ovn/TODO                   | 37 +++++++++++----------------
 ovn/controller/patch.c     | 63 ++++++++++++++++++++++++++++++++++++++++------
 ovn/controller/physical.c  | 15 +++++++++--
 ovn/ovn-architecture.7.xml | 12 ++++++++-
 ovn/ovn-sb.xml             | 29 +++++++++++++++++++--
 tests/ovn-controller.at    | 40 ++++++++++++++++++++++++++++-
 6 files changed, 160 insertions(+), 36 deletions(-)

Comments

Justin Pettit Oct. 16, 2015, 12:48 a.m. UTC | #1
> On Oct 9, 2015, at 9:20 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> index 90c72ff..f25709c 100644
> --- a/ovn/controller/patch.c
> +++ b/ovn/controller/patch.c
> @@ -50,7 +50,7 @@ match_patch_port(const struct ovsrec_port *port, const char *peer)
> 
> static void
> create_patch_port(struct controller_ctx *ctx,
> -                  const char *network,
> +                  const char *key, const char *value,
>                   const struct ovsrec_bridge *src, const char *src_name,
>                   const struct ovsrec_bridge *dst, const char *dst_name,
>                   struct shash *existing_ports)

The names "key" and "value" seem awfully generic.

> static void
> create_patch_ports(struct controller_ctx *ctx,
> -                   const char *network,
> +                   const char *key, const char *value,

Same here about "key" and "value".

I wonder if this function is really needed.  This only has one caller and it doesn't do much.  The other function that creates patch ports just calls create_patch_port() directly.

> +            char *src_name = xasprintf("patch-%s-to-%s", local, peer);
> +            char *dst_name = xasprintf("patch-%s-to-%s", peer, local);

In the previous patch, you defined patch_port_name(), which could be used for this.

> +            create_patch_port(ctx, "ovn-logical-patch-port", local,
> +                              br_int, src_name, br_int, dst_name,
> +                              existing_ports);

A description of "ovn-localnet-port" is provided in the ovn-controller man page.  It might be nice to do the same for "ovn-logical-patch-port".

>     /* Add any patch ports that should exist but don't. */
>     parse_bridge_mappings(ctx, br_int, &existing_ports);
> +    add_logical_patch_ports(ctx, br_int, &existing_ports);

Since these are doing very similar things, do you think it's worth using more similar names?

> @@ -252,6 +262,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
>          *
>          *       The "localnet" port may be configured with a VLAN ID.  If so,
>          *       'tag' will be set to that VLAN ID; otherwise 'tag' is 0.
> +         *

Did you add this on purpose?

Acked-by: Justin Pettit <jpettit@nicira.com>

--Justin
Ben Pfaff Oct. 16, 2015, 8:31 p.m. UTC | #2
On Thu, Oct 15, 2015 at 05:48:30PM -0700, Justin Pettit wrote:
> 
> > On Oct 9, 2015, at 9:20 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> > index 90c72ff..f25709c 100644
> > --- a/ovn/controller/patch.c
> > +++ b/ovn/controller/patch.c
> > @@ -50,7 +50,7 @@ match_patch_port(const struct ovsrec_port *port, const char *peer)
> > 
> > static void
> > create_patch_port(struct controller_ctx *ctx,
> > -                  const char *network,
> > +                  const char *key, const char *value,
> >                   const struct ovsrec_bridge *src, const char *src_name,
> >                   const struct ovsrec_bridge *dst, const char *dst_name,
> >                   struct shash *existing_ports)
> 
> The names "key" and "value" seem awfully generic.

They are in fact a key and a value in the external-ids column.

There's now a comment to explain:

/* Creates a patch port in bridge 'src' named 'src_name', whose peer is
 * 'dst_name' in bridge 'dst'.  Initializes the patch port's external-ids:'key'
 * to 'key'.
 *
 * If such a patch port already exists, removes it from 'existing_ports'. */

> > static void
> > create_patch_ports(struct controller_ctx *ctx,
> > -                   const char *network,
> > +                   const char *key, const char *value,
> 
> Same here about "key" and "value".

Ditto:

/* Creates a pair of patch ports that connect bridges 'b1' and 'b2', using a
 * port named 'name1' and 'name2' in each respective bridge.
 * external-ids:'key' in each port is initialized to 'value'.
 *
 * If one or both of the ports already exists, leaves it there and removes it
 * from 'existing_ports'. */

> I wonder if this function is really needed.  This only has one caller
> and it doesn't do much.  The other function that creates patch ports
> just calls create_patch_port() directly.

I guess I see the obvious symmetry in the two lines of the function to
be a useful guide.

> > +            char *src_name = xasprintf("patch-%s-to-%s", local, peer);
> > +            char *dst_name = xasprintf("patch-%s-to-%s", peer, local);
> 
> In the previous patch, you defined patch_port_name(), which could be
> used for this.

The function couldn't be used exactly as written, since it takes two
bridges as parameters and uses the bridge names, but it's a reasonable
idea, so I changed it to take two string parameters instead.

> > +            create_patch_port(ctx, "ovn-logical-patch-port", local,
> > +                              br_int, src_name, br_int, dst_name,
> > +                              existing_ports);
> 
> A description of "ovn-localnet-port" is provided in the ovn-controller
> man page.  It might be nice to do the same for
> "ovn-logical-patch-port".

Good idea.  I added:

      <dt>
        <code>external-ids:ovn-logical-patch-port</code> in the
        <code>Port</code> table
      </dt>

      <dd>
        <p>
          This key identifies a patch port as one created by
          <code>ovn-controller</code> to implement an OVN logical patch port
          within the integration bridge.  Its value is the name of the OVN
          logical patch port that it implements.
        </p>
      </dd>

> >     /* Add any patch ports that should exist but don't. */
> >     parse_bridge_mappings(ctx, br_int, &existing_ports);
> > +    add_logical_patch_ports(ctx, br_int, &existing_ports);
> 
> Since these are doing very similar things, do you think it's worth using more similar names?

I changes parse_bridge_mappings() to add_bridge_mappings(), in a
previous patch.

> > @@ -252,6 +262,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
> >          *
> >          *       The "localnet" port may be configured with a VLAN ID.  If so,
> >          *       'tag' will be set to that VLAN ID; otherwise 'tag' is 0.
> > +         *
> 
> Did you add this on purpose?

No.  Reverted.

> Acked-by: Justin Pettit <jpettit@nicira.com>

Thanks!

Patch
diff mbox

diff --git a/ovn/TODO b/ovn/TODO
index b29df75..c914c10 100644
--- a/ovn/TODO
+++ b/ovn/TODO
@@ -18,30 +18,21 @@  one router to another, this doesn't seem to matter (just put more than
 one connection between them), but for connections between a router and
 a switch it might matter because a switch has only one router port.
 
-** OVN_SB schema
+*** Logical router port names in ACLs
+
+Currently the ACL table documents that the logical router port is
+always named "ROUTER".  This can't work directly using logical patch
+ports to connect a logical switch to its logical router, because every
+row in the Logical_Port table must have a unique name.  This probably
+means that we should change the convention for the ACL table so that
+the logical router port name is unique; for example, we could change
+the Logical_Router_Port table to require the 'name' column to be
+unique, and then use that name in the ACL table.
 
-*** Logical datapath interconnection
-
-There needs to be a way in the OVN_Southbound database to express
-connections between logical datapaths, so that packets can pass from a
-logical switch to its logical router (and vice versa) and from one
-logical router to another.
-
-One way to do this would be to introduce logical patch ports, closely
-modeled on the "physical" patch ports that OVS has had for ages.  Each
-logical patch port would consist of two rows in the Port_Binding table
-(one in each logical datapath), with type "patch" and an option "peer"
-that names the other logical port in the pair.
-
-If we do it this way then we'll need to figure out one odd special
-case.  Currently the ACL table documents that the logical router port
-is always named "ROUTER".  This can't be done directly with this patch
-port technique, because every row in the Logical_Port table must have
-a unique name.  This probably means that we should change the
-convention for the ACL table so that the logical router port name is
-unique; for example, we could change the Logical_Router_Port table to
-require the 'name' column to be unique, and then use that name in the
-ACL table.
+Another alternative would be to add a way to have aliases for logical
+ports, but I'm not sure that's a rathole we really want to go down.
+
+** OVN_SB schema
 
 *** Allow output to ingress port
 
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 90c72ff..f25709c 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -50,7 +50,7 @@  match_patch_port(const struct ovsrec_port *port, const char *peer)
 
 static void
 create_patch_port(struct controller_ctx *ctx,
-                  const char *network,
+                  const char *key, const char *value,
                   const struct ovsrec_bridge *src, const char *src_name,
                   const struct ovsrec_bridge *dst, const char *dst_name,
                   struct shash *existing_ports)
@@ -78,7 +78,7 @@  create_patch_port(struct controller_ctx *ctx,
     port = ovsrec_port_insert(ctx->ovs_idl_txn);
     ovsrec_port_set_name(port, src_name);
     ovsrec_port_set_interfaces(port, &iface, 1);
-    const struct smap ids = SMAP_CONST1(&ids, "ovn-localnet-port", network);
+    const struct smap ids = SMAP_CONST1(&ids, key, value);
     ovsrec_port_set_external_ids(port, &ids);
 
     struct ovsrec_port **ports;
@@ -93,13 +93,13 @@  create_patch_port(struct controller_ctx *ctx,
 
 static void
 create_patch_ports(struct controller_ctx *ctx,
-                   const char *network,
+                   const char *key, const char *value,
                    const struct ovsrec_bridge *b1, const char *name1,
                    const struct ovsrec_bridge *b2, const char *name2,
                    struct shash *existing_ports)
 {
-    create_patch_port(ctx, network, b1, name1, b2, name2, existing_ports);
-    create_patch_port(ctx, network, b2, name2, b1, name1, existing_ports);
+    create_patch_port(ctx, key, value, b1, name1, b2, name2, existing_ports);
+    create_patch_port(ctx, key, value, b2, name2, b1, name1, existing_ports);
 }
 
 static void
@@ -172,7 +172,7 @@  parse_bridge_mappings(struct controller_ctx *ctx,
 
         char *br_int_name = patch_port_name(br_int, ovs_bridge);
         char *ovs_bridge_name = patch_port_name(ovs_bridge, br_int);
-        create_patch_ports(ctx, network,
+        create_patch_ports(ctx, "ovn-localnet-port", network,
                            br_int, br_int_name,
                            ovs_bridge, ovs_bridge_name,
                            existing_ports);
@@ -182,6 +182,53 @@  parse_bridge_mappings(struct controller_ctx *ctx,
     free(start);
 }
 
+/* Add one OVS patch port for each OVN logical patch port.
+ *
+ * This is suboptimal for several reasons.  First, it creates an OVS port for
+ * every OVN logical patch port, not just for the ones that are actually useful
+ * on this hypervisor.  Second, it's wasteful to create an OVS patch port per
+ * OVN logical patch port, when really there's no benefit to them beyond a way
+ * to identify how a packet ingressed into a logical datapath.
+ *
+ * There are two obvious ways to improve the situation here, by modifying
+ * OVS:
+ *
+ *     1. Add a way to configure in OVS which fields are preserved on a hop
+ *        across an OVS patch port.  If MFF_LOG_DATAPATH and MFF_LOG_INPORT
+ *        were preserved, then only a single pair of OVS patch ports would be
+ *        required regardless of the number of OVN logical patch ports.
+ *
+ *     2. Add a new OpenFlow extension action modeled on "resubmit" that also
+ *        saves and restores the packet data and metadata (the inability to do
+ *        this is the only reason that "resubmit" can't be used already).  Or
+ *        add OpenFlow extension actions to otherwise save and restore packet
+ *        data and metadata.
+ */
+static void
+add_logical_patch_ports(struct controller_ctx *ctx,
+                        const struct ovsrec_bridge *br_int,
+                        struct shash *existing_ports)
+{
+    const struct sbrec_port_binding *binding;
+    SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
+        if (!strcmp(binding->type, "patch")) {
+            const char *local = binding->logical_port;
+            const char *peer = smap_get(&binding->options, "peer");
+            if (!peer) {
+                continue;
+            }
+
+            char *src_name = xasprintf("patch-%s-to-%s", local, peer);
+            char *dst_name = xasprintf("patch-%s-to-%s", peer, local);
+            create_patch_port(ctx, "ovn-logical-patch-port", local,
+                              br_int, src_name, br_int, dst_name,
+                              existing_ports);
+            free(dst_name);
+            free(src_name);
+        }
+    }
+}
+
 void
 patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int)
 {
@@ -193,13 +240,15 @@  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")) {
+        if (smap_get(&port->external_ids, "ovn-localnet-port") ||
+            smap_get(&port->external_ids, "ovn-logical-patch-port")) {
             shash_add(&existing_ports, port->name, port);
         }
     }
 
     /* Add any patch ports that should exist but don't. */
     parse_bridge_mappings(ctx, br_int, &existing_ports);
+    add_logical_patch_ports(ctx, br_int, &existing_ports);
 
     /* Delete any patch ports that do exist but shouldn't.  (Any that both
      * should and do exist were removed above.) */
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index a1f7fe5..2cf7d8c 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -156,6 +156,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 *logpatch = smap_get(&port_rec->external_ids,
+                                        "ovn-logical-patch-port");
 
         for (int j = 0; j < port_rec->n_interfaces; j++) {
             const struct ovsrec_interface *iface_rec = port_rec->interfaces[j];
@@ -169,10 +171,16 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 continue;
             }
 
-            /* Record as patch to local net, chassis, or local logical port. */
-            if (!strcmp(iface_rec->type, "patch") && localnet) {
+            /* Record as patch to local net, logical patch port, chassis, or
+             * local logical port. */
+            bool is_patch = !strcmp(iface_rec->type, "patch");
+            if (is_patch && localnet) {
                 simap_put(&localnet_to_ofport, localnet, ofport);
                 break;
+            } else if (is_patch && logpatch) {
+                /* Logical patch ports can be handled just like VIFs. */
+                simap_put(&localvif_to_ofport, logpatch, ofport);
+                break;
             } else if (chassis_id) {
                 enum chassis_tunnel_type tunnel_type;
                 if (!strcmp(iface_rec->type, "geneve")) {
@@ -242,6 +250,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
          *       and accessible via a VLAN, 'tag' is the VLAN ID; otherwise
          *       'tag' is 0.
          *
+         *       The same logic handles logical patch ports.
+         *
          *     - If the port is on a remote chassis, the OpenFlow port for a
          *       tunnel to the VIF's remote chassis.  'tun' identifies that
          *       tunnel.
@@ -252,6 +262,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
          *
          *       The "localnet" port may be configured with a VLAN ID.  If so,
          *       'tag' will be set to that VLAN ID; otherwise 'tag' is 0.
+         *
          */
 
         int tag = 0;
diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index a7ff674..364ccce 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -773,7 +773,9 @@ 
 
       <p>
         Flows in table 33 resemble those in table 32 but for logical ports that
-        reside locally rather than remotely.  For unicast logical output ports
+        reside locally rather than remotely.  (This includes logical patch
+        ports, which do not have a physical location and effectively reside on
+        every hypervisor.)  For unicast logical output ports
         on the local hypervisor, the actions just resubmit to table 34.  For
         multicast output ports that include one or more logical ports on the
         local hypervisor, for each such logical port <var>P</var>, the actions
@@ -814,6 +816,14 @@ 
         is a container nested with a VM, then before sending the packet the
         actions push on a VLAN header with an appropriate VLAN ID.
       </p>
+
+      <p>
+        If the logical egress port is a logical patch port, then table 64
+        outputs to an OVS patch port that represents the logical patch port.
+        The packet re-enters the OpenFlow flow table from the OVS patch port's
+        peer in table 0, which identifies the logical datapath and logical
+        input port based on the OVS patch port's OpenFlow port number.
+      </p>
     </li>
   </ol>
 
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 3cc96d4..bd116c3 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1039,8 +1039,9 @@  tcp.flags = RST;
 
   <table name="Port_Binding" title="Physical-Logical Port Bindings">
     <p>
-      Each row in this table identifies the physical location of a logical
-      port.
+      Most rows in this table identify the physical location of a logical port.
+      (The exceptions are logical patch ports, which do not have any physical
+      location.)
     </p>
 
     <p>
@@ -1127,6 +1128,14 @@  tcp.flags = RST;
         <dl>
           <dt>(empty string)</dt>
           <dd>VM (or VIF) interface.</dd>
+
+          <dt><code>patch</code></dt>
+          <dd>
+            One of a pair of logical ports that act as if connected by a patch
+            cable.  Useful for connecting two logical datapaths, e.g. to connect
+            a logical router to a logical switch or to another logical router.
+          </dd>
+
           <dt><code>localnet</code></dt>
           <dd>
             A connection to a locally accessible network from each
@@ -1150,6 +1159,22 @@  tcp.flags = RST;
       </column>
     </group>
 
+    <group title="Patch Options">
+      <p>
+        These options apply to logical ports with <ref column="type"/> of
+        <code>patch</code>.
+      </p>
+
+      <column name="options" key="peer">
+        The <ref column="logical_port"/> in the <ref table="Port_Binding"/>
+        record for the other side of the patch.  The named <ref
+        column="logical_port"/> must specify this <ref column="logical_port"/>
+        in its own <code>peer</code> option.  That is, the two patch logical
+        ports must have reversed <ref column="logical_port"/> and
+        <code>peer</code> values.
+      </column>
+    </group>
+
     <group title="Localnet Options">
       <p>
         These options apply to logical ports with <ref column="type"/> of
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 44206c1..773b3a7 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -64,8 +64,46 @@  check_patches \
     'br-eth1 patch-br-eth1-to-br-int patch-br-int-to-br-eth1' \
     'br-int  patch-br-int-to-br-eth1 patch-br-eth1-to-br-int'
 
-# Delete the mapping and the patch ports should go away.
+# Add logical patch ports.
+AT_CHECK([ovn-sbctl \
+    -- --id=@dp1 create Datapath_Binding tunnel_key=1 \
+    -- --id=@dp2 create Datapath_Binding tunnel_key=2 \
+    -- create Port_Binding datapath=@dp1 logical_port=foo tunnel_key=1 type=patch options:peer=bar \
+    -- create Port_Binding datapath=@dp2 logical_port=bar tunnel_key=2 type=patch options:peer=foo \
+| ${PERL} $srcdir/uuidfilt.pl], [0], [<0>
+<1>
+<2>
+<3>
+])
+check_patches \
+    'br-eth2 patch-br-eth2-to-br-int patch-br-int-to-br-eth2' \
+    'br-int  patch-br-int-to-br-eth2 patch-br-eth2-to-br-int' \
+    'br-eth1 patch-br-eth1-to-br-int patch-br-int-to-br-eth1' \
+    'br-int  patch-br-int-to-br-eth1 patch-br-eth1-to-br-int' \
+    'br-int  patch-foo-to-bar        patch-bar-to-foo' \
+    'br-int  patch-bar-to-foo        patch-foo-to-bar'
+
+# Delete the mapping and the ovn-bridge-mapping patch ports should go away;
+# the ones from the logical patch port remain.
 AT_CHECK([ovs-vsctl remove Open_vSwitch . external-ids ovn-bridge-mappings])
+check_patches \
+    'br-int patch-foo-to-bar patch-bar-to-foo' \
+    'br-int patch-bar-to-foo patch-foo-to-bar'
+
+# Change name of logical patch port, check that the OVS patch ports
+# get updated.
+AT_CHECK([ovn-sbctl \
+    -- set Port_Binding foo logical_port=quux options:peer=baz \
+    -- set Port_Binding bar logical_port=baz  options:peer=quux])
+check_patches \
+    'br-int patch-quux-to-baz patch-baz-to-quux' \
+    'br-int patch-baz-to-quux patch-quux-to-baz'
+
+# Change the logical patch ports to VIFs and verify that the OVS patch
+# ports disappear.
+AT_CHECK([ovn-sbctl \
+    -- set Port_Binding quux type='""' options='{}' \
+    -- set Port_Binding baz type='""' options='{}'])
 check_patches
 
 AT_CLEANUP