[ovs-dev,v8,2/2] ovn: Add "localnet" logical port type.
diff mbox

Message ID 20150908185152.GD3766@nicira.com
State Not Applicable
Headers show

Commit Message

Ben Pfaff Sept. 8, 2015, 6:51 p.m. UTC
On Thu, Sep 03, 2015 at 12:45:01PM -0400, Russell Bryant wrote:
> Introduce a new logical port type called "localnet".  A logical port
> with this type also has an option called "network_name".  A "localnet"
> logical port represents a connection to a network that is locally
> accessible from each chassis running ovn-controller.  ovn-controller
> will use the ovn-bridge-mappings configuration to figure out which
> patch port on br-int should be used for this port.

Thanks!  I folded in the following incrementals, which are mostly for
style (I updated one comment that had rotted, adjusted line lengths,
etc.).  Please do read them to make sure that you agree.  Then I applied
this to master.

Comments

Russell Bryant Sept. 9, 2015, 12:55 a.m. UTC | #1
On 09/08/2015 02:51 PM, Ben Pfaff wrote:
> On Thu, Sep 03, 2015 at 12:45:01PM -0400, Russell Bryant wrote:
>> Introduce a new logical port type called "localnet".  A logical port
>> with this type also has an option called "network_name".  A "localnet"
>> logical port represents a connection to a network that is locally
>> accessible from each chassis running ovn-controller.  ovn-controller
>> will use the ovn-bridge-mappings configuration to figure out which
>> patch port on br-int should be used for this port.
> 
> Thanks!  I folded in the following incrementals, which are mostly for
> style (I updated one comment that had rotted, adjusted line lengths,
> etc.).  Please do read them to make sure that you agree.  Then I applied
> this to master.

Thanks for the great feedback along the way!  I agree with all of the
changes you made.

Now I'll just have to do the Neutron integration with this and that will
be another feature gap closed with the existing ovs support.

Patch
diff mbox

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index ba2cddf..bdb02da 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -214,22 +214,34 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
         ofp_port_t ofport;
         struct ovs_list bindings;
     };
-    /* A list of localnet port bindings hashed by network name */
+    /* Maps from network name to "struct localnet_bindings". */
     struct shash localnet_inputs = SHASH_INITIALIZER(&localnet_inputs);
 
-    /* Datapaths with at least one local port binding */
+    /* Contains bare "struct hmap_node"s whose hash values are the tunnel_key
+     * of datapaths with at least one local port binding. */
     struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
 
     /* Set up flows in table 0 for physical-to-logical translation and in table
      * 64 for logical-to-physical translation. */
     const struct sbrec_port_binding *binding;
     SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
-        /* Find the OpenFlow port for the logical port, as 'ofport'.  If it's
-         * on a remote chassis, this is the OpenFlow port for the tunnel to
-         * that chassis (and set 'local' to false).  Otherwise, if it's on the
-         * chassis we're managing, this is the OpenFlow port for the vif itself
-         * (and set 'local' to true). When 'parent_port' is set for a binding,
-         * it implies a container sitting inside a VM reachable via a 'tag'.
+        /* Find the OpenFlow port for the logical port, as 'ofport'.  This is
+         * one of:
+         *
+         *     - If the port is a VIF on the chassis we're managing, the
+         *       OpenFlow port for the VIF.  'tun' will be NULL.
+         *
+         *       In this or the next case, for a container nested inside a VM
+         *       and accessible via a VLAN, 'tag' is the VLAN ID; otherwise
+         *       'tag' is 0.
+         *
+         *     - If the port is on a remote chassis, the OpenFlow port for a
+         *       tunnel to the VIF's remote chassis.  'tun' identifies that
+         *       tunnel.
+         *
+         *     - If the port is a "localnet" port for a network that is
+         *       attached to the chassis we're managing, the OpenFlow port for
+         *       the localnet port (a patch port).
          */
 
         int tag = 0;
@@ -287,18 +299,17 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                  * attached to more than one logical datapath, so keep track of
                  * all associated bindings and add a flow at the end. */
 
-                const char *network = smap_get(&binding->options, "network_name");
-                struct shash_node *node;
+                const char *network
+                    = smap_get(&binding->options, "network_name");
                 struct localnet_bindings *ln_bindings;
 
-                node = shash_find(&localnet_inputs, network);
-                if (!node) {
+                ln_bindings = shash_find_data(&localnet_inputs, network);
+                if (!ln_bindings) {
                     ln_bindings = xmalloc(sizeof *ln_bindings);
                     ln_bindings->ofport = ofport;
                     list_init(&ln_bindings->bindings);
-                    node = shash_add(&localnet_inputs, network, ln_bindings);
+                    shash_add(&localnet_inputs, network, ln_bindings);
                 }
-                ln_bindings = node->data;
 
                 struct binding_elem *b = xmalloc(sizeof *b);
                 b->binding = binding;
@@ -594,14 +605,14 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     /* Table 0, Priority 100
      * =====================
      *
-     * We have now determined the full set of port bindings associated with each
-     * "localnet" network.  Only create flows for datapaths that have another
-     * local binding.  Otherwise, we know it would just be dropped.
+     * We have now determined the full set of port bindings associated with
+     * each "localnet" network.  Only create flows for datapaths that have
+     * another local binding.  Otherwise, we know it would just be dropped.
      */
     struct shash_node *ln_bindings_node, *ln_bindings_node_next;
-    struct localnet_bindings *ln_bindings;
-    SHASH_FOR_EACH_SAFE (ln_bindings_node, ln_bindings_node_next, &localnet_inputs) {
-        ln_bindings = ln_bindings_node->data;
+    SHASH_FOR_EACH_SAFE (ln_bindings_node, ln_bindings_node_next,
+                         &localnet_inputs) {
+        struct localnet_bindings *ln_bindings = ln_bindings_node->data;
 
         struct match match;
         match_init_catchall(&match);
@@ -610,16 +621,12 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
         struct ofpbuf ofpacts;
         ofpbuf_init(&ofpacts, 0);
 
-        bool found_local = false;
         struct binding_elem *b;
         LIST_FOR_EACH_POP (b, list_elem, &ln_bindings->bindings) {
             struct hmap_node *ld;
             ld = hmap_first_with_hash(&local_datapaths,
                                       b->binding->datapath->tunnel_key);
             if (ld) {
-                /* This datapath has local port bindings. */
-                found_local = true;
-
                 /* Set MFF_LOG_DATAPATH and MFF_LOG_INPORT. */
                 put_load(b->binding->datapath->tunnel_key, MFF_LOG_DATAPATH,
                          0, 64, &ofpacts);
@@ -631,7 +638,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
             free(b);
         }
 
-        if (found_local) {
+        if (ofpacts.size) {
             ofctrl_add_flow(flow_table, 0, 100, &match, &ofpacts);
         }
 
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 0dfdab5..42a94b9 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -116,24 +116,26 @@ 
       </p>
 
       <p>
-      When this column is set to <em>localnet</em>, this logical port represents a
-      connection to a locally accessible network from each ovn-controller instance.
-      A logical switch can only have a single <em>localnet</em> port attached
-      and at most one regular logical port.  This is used to model direct
-      connectivity to an existing network.
+        When this column is set to <code>localnet</code>, this logical port
+        represents a connection to a locally accessible network from each
+        <code>ovn-controller</code> instance.  A logical switch can only have a
+        single <code>localnet</code> port attached and at most one regular
+        logical port.  This is used to model direct connectivity to an existing
+        network.
       </p>
     </column>
 
     <column name="options">
       <p>
-      This column provides key/value settings specific to the logical port
-      <ref column="type"/>.
+        This column provides key/value settings specific to the logical port
+        <ref column="type"/>.
       </p>
 
       <p>
-      When <ref column="type"/> is set to <em>localnet</em>, you must set the option
-      <em>network_name</em>.  ovn-controller uses local configuration to determine
-      exactly how to connect to this locally accessible network.
+        When <ref column="type"/> is set to <code>localnet</code>, you must set
+        the option <code>network_name</code>.  <code>ovn-controller</code> uses
+        local configuration to determine exactly how to connect to this locally
+        accessible network.
       </p>
     </column>
 
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index d696745..74631f9 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -964,39 +964,42 @@ 
       </p>
 
       <p>
-      When this column is set to <em>localnet</em>, this logical port represents a
-      connection to a locally accessible network from each ovn-controller instance.
-      A logical switch can only have a single <em>localnet</em> port attached
-      and at most one regular logical port.  This is used to model direct
-      connectivity to an existing network.
+        When this column is set to <code>localnet</code>, this logical port
+        represents a connection to a locally accessible network from each
+        ovn-controller instance.  A logical switch can only have a single
+        <code>localnet</code> port attached and at most one regular logical
+        port.  This is used to model direct connectivity to an existing
+        network.
       </p>
     </column>
 
     <column name="options">
       <p>
-      This column provides key/value settings specific to the logical port
-      <ref column="type"/>.
+        This column provides key/value settings specific to the logical port
+        <ref column="type"/>.
       </p>
 
       <p>
-      When <ref column="type"/> is set to <em>localnet</em>, you must set the option
-      <em>network_name</em>.  ovn-controller uses the configuration entry
-      <em>ovn-bridge-mappings</em> to determine how to connect to this network.
-      <em>ovn-bridge-mappings</em> is a list of network names mapped to a local
-      OVS bridge that provides access to that network.  An example of configuring
-      <em>ovn-bridge-mappings</em> would be:
+        When <ref column="type"/> is set to <code>localnet</code>, you must set
+        the option <code>network_name</code>.  <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:
       </p>
 
       <p>
-      <em>$ ovs-vsctl set open . external-ids:ovn-bridge-mappings=physnet1:br-eth0,physnet2:br-eth1</em>
+        <code>$ ovs-vsctl set open
+        . external-ids:ovn-bridge-mappings=physnet1:br-eth0,physnet2:br-eth1</code>
       </p>
 
       <p>
-      Also note that when a logical switch has a <em>localnet</em> port attached,
-      every chassis that may have a local vif attached to that logical switch
-      must have a bridge mapping configured to reach that <em>localnet</em>.
-      Traffic that arrives on a <em>localnet</em> port is never forwarded over a tunnel
-      to another chassis.
+        Also note that when a logical switch has a <code>localnet</code> port
+        attached, every chassis that may have a local vif attached to that
+        logical switch must have a bridge mapping configured to reach that
+        <code>localnet</code>.  Traffic that arrives on a <code>localnet</code>
+        port is never forwarded over a tunnel to another chassis.
       </p>
     </column>