bridge: Support changing port numbers.

Feature #21293.
Requested-by: Anuprem Chalvadi <achalvadi@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

NEWS

@@ -38,6 +38,9 @@ Post-v2.0.0
      * OVS limits the OpenFlow port numbers it assigns to port 32767 and
        below, leaving port numbers above that range free for assignment
        by the controller.
+     * ovs-vswitchd now honors changes to the "ofport_request" column
+       in the Interface table by changing the port's OpenFlow port
+       number.
    - ovs-vswitchd.conf.db.5 man page will contain graphviz/dot
      diagram only if graphviz package was installed at the build time.
    - Support for Linux kernels up to 3.11

tests/ofproto.at

@@ -2152,3 +2152,58 @@ OFPT_BARRIER_REPLY (OF1.3):
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto - ofport_request])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2], [3])
+
+set_and_check_specific_ofports () {
+    ovs-vsctl set Interface p1 ofport_request="$1" -- \
+	      set Interface p2 ofport_request="$2" -- \
+	      set Interface p3 ofport_request="$3"
+    ofports=`ovs-vsctl get Interface p1 ofport -- \
+		       get Interface p2 ofport -- \
+		       get Interface p3 ofport`
+    AT_CHECK_UNQUOTED([echo $ofports], [0], [$1 $2 $3
+])
+}
+for pre in      '1 2 3' '1 3 2' '2 1 3' '2 3 1' '3 1 2' '3 2 1'; do
+    for post in '1 2 3' '1 3 2' '2 1 3' '2 3 1' '3 1 2' '3 2 1'; do
+        echo -----------------------------------------------------------
+        echo "Check changing port numbers from $pre to $post"
+	set_and_check_ofports $pre
+	set_and_check_ofports $post
+    done
+done
+
+ovs-vsctl del-port p3
+
+set_and_check_poorly_specified_ofports () {
+    ovs-vsctl set Interface p1 ofport_request="$1" -- \
+	      set Interface p2 ofport_request="$2"
+    p1=`ovs-vsctl get Interface p1 ofport`
+    p2=`ovs-vsctl get Interface p2 ofport`
+    echo $p1 $p2
+
+    AT_CHECK([test "$p1" != "$p2"])
+    if test "$1" = "$2" && test "$1" != '[[]]'; then
+        # One port number must be the requested one.
+	AT_CHECK([test "$p1" = "$1" || test "$p2" = "$1"])
+	# The other port number must be different (already tested above).
+    else
+        AT_CHECK([test "$1" = '[[]]' || test "$p1" == "$1"])
+        AT_CHECK([test "$2" = '[[]]' || test "$p2" == "$2"])
+    fi
+}
+for pre in      '1 2' '[[]] 2' '1 [[]]' '[[]] [[]]' '2 1' '[[]] 1' '2 [[]]' \
+                '1 1' '2 2'; do
+    for post in '1 2' '[[]] 2' '1 [[]]' '[[]] [[]]' '2 1' '[[]] 1' '2 [[]]' \
+                '1 1' '2 2'; do
+        echo -----------------------------------------------------------
+        echo "Check changing port numbers from $pre to $post"
+        set_and_check_poorly_specified_ofports $pre
+        set_and_check_poorly_specified_ofports $post
+    done
+done
+OVS_VSWITCHD_STOP
+AT_CLEANUP

vswitchd/bridge.c

@@ -255,6 +255,8 @@ static void iface_refresh_cfm_stats(struct iface *);
 static void iface_refresh_stats(struct iface *);
 static void iface_refresh_status(struct iface *);
 static bool iface_is_synthetic(const struct iface *);
+static ofp_port_t iface_get_requested_ofp_port(
+    const struct ovsrec_interface *);
 static ofp_port_t iface_pick_ofport(const struct ovsrec_interface *);
 
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
@@ -652,6 +654,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
     n = allocated = 0;
 
     OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
+        ofp_port_t requested_ofp_port;
         struct iface *iface;
 
         iface = iface_lookup(br, ofproto_port.name);
@@ -675,6 +678,43 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
             goto delete;
         }
 
+        /* If the requested OpenFlow port for 'iface' changed, and it's not
+         * already the correct port, then we might want to temporarily delete
+         * this interface, so we can add it back again with the new OpenFlow
+         * port number. */
+        requested_ofp_port = iface_get_requested_ofp_port(iface->cfg);
+        if (iface->ofp_port != OFPP_LOCAL &&
+            requested_ofp_port != OFPP_NONE &&
+            requested_ofp_port != iface->ofp_port) {
+            ofp_port_t victim_request;
+            struct iface *victim;
+
+            /* Check for an existing OpenFlow port currently occupying
+             * 'iface''s requested port number.  If there isn't one, then
+             * delete this port.  Otherwise we need to consider further. */
+            victim = iface_from_ofp_port(br, requested_ofp_port);
+            if (!victim) {
+                goto delete;
+            }
+
+            /* 'victim' is a port currently using 'iface''s requested port
+             * number.  Unless 'victim' specifically requested that port
+             * number, too, then we can delete both 'iface' and 'victim'
+             * temporarily.  (We'll add both of them back again later with new
+             * OpenFlow port numbers.)
+             *
+             * If 'victim' did request port number 'requested_ofp_port', just
+             * like 'iface', then that's a configuration inconsistency that we
+             * can't resolve.  We might as well let it keep its current port
+             * number. */
+            victim_request = iface_get_requested_ofp_port(victim->cfg);
+            if (victim_request != requested_ofp_port) {
+                del = add_ofp_port(victim->ofp_port, del, &n, &allocated);
+                iface_destroy(victim);
+                goto delete;
+            }
+        }
+
         /* Keep it. */
         continue;
 
@@ -690,7 +730,8 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 }
 
 static void
-bridge_add_ports(struct bridge *br, const struct shash *wanted_ports)
+bridge_add_ports__(struct bridge *br, const struct shash *wanted_ports,
+                   bool with_requested_port)
 {
     struct shash_node *port_node;
 
@@ -700,15 +741,32 @@ bridge_add_ports(struct bridge *br, const struct shash *wanted_ports)
 
         for (i = 0; i < port_cfg->n_interfaces; i++) {
             const struct ovsrec_interface *iface_cfg = port_cfg->interfaces[i];
-            struct iface *iface = iface_lookup(br, iface_cfg->name);
+            ofp_port_t requested_ofp_port;
 
-            if (!iface) {
-                iface_create(br, iface_cfg, port_cfg);
+            requested_ofp_port = iface_get_requested_ofp_port(iface_cfg);
+            if ((requested_ofp_port != OFPP_NONE) == with_requested_port) {
+                struct iface *iface = iface_lookup(br, iface_cfg->name);
+
+                if (!iface) {
+                    iface_create(br, iface_cfg, port_cfg);
+                }
             }
         }
     }
 }
 
+static void
+bridge_add_ports(struct bridge *br, const struct shash *wanted_ports)
+{
+    /* First add interfaces that request a particular port number. */
+    bridge_add_ports__(br, wanted_ports, true);
+
+    /* Then add interfaces that want automatic port number assignment.
+     * We add these afterward to avoid accidentally taking a specifically
+     * requested port number. */
+    bridge_add_ports__(br, wanted_ports, false);
+}
+
 static void
 port_configure(struct port *port)
 {
@@ -1458,7 +1516,7 @@ iface_create(struct bridge *br, const struct ovsrec_interface *iface_cfg,
 
             error = netdev_open(port->name, "internal", &netdev);
             if (!error) {
-                ofp_port_t fake_ofp_port = iface_pick_ofport(iface_cfg);
+                ofp_port_t fake_ofp_port = OFPP_NONE;
                 ofproto_port_add(br->ofproto, netdev, &fake_ofp_port);
                 netdev_close(netdev);
             } else {
@@ -3636,14 +3694,27 @@ iface_is_synthetic(const struct iface *iface)
 }
 
 static ofp_port_t
-iface_pick_ofport(const struct ovsrec_interface *cfg)
+iface_validate_ofport__(size_t n, int64_t *ofport)
 {
-    ofp_port_t ofport = cfg->n_ofport ? u16_to_ofp(*cfg->ofport)
-                                      : OFPP_NONE;
-    return cfg->n_ofport_request ? u16_to_ofp(*cfg->ofport_request)
-                                 : ofport;
+    return (n && *ofport >= 1 && *ofport < ofp_to_u16(OFPP_MAX)
+            ? u16_to_ofp(*ofport)
+            : OFPP_NONE);
 }
 
+static ofp_port_t
+iface_get_requested_ofp_port(const struct ovsrec_interface *cfg)
+{
+    return iface_validate_ofport__(cfg->n_ofport_request, cfg->ofport_request);
+}
+
+static ofp_port_t
+iface_pick_ofport(const struct ovsrec_interface *cfg)
+{
+    ofp_port_t requested_ofport = iface_get_requested_ofp_port(cfg);
+    return (requested_ofport != OFPP_NONE
+            ? requested_ofport
+            : iface_validate_ofport__(cfg->n_ofport, cfg->ofport));
+}
 
 /* Port mirroring. */
 

vswitchd/vswitch.xml

@@ -1306,14 +1306,19 @@
 	  </p>
 
 	  <p>
-	    Open vSwitch currently assigns the OpenFlow port number for an
-	    interface once, when the client first adds the interface.  It does
-	    not change the port number later if the client sets or changes or
-	    clears <ref column="ofport_request"/>.  Therefore, to ensure that
-	    <ref column="ofport_request"/> takes effect, the client should set
-	    it in the same database transaction that creates the interface.
-	    (Future versions of Open vSwitch might honor changes to <ref
-	    column="ofport_request"/>.)
+	    A client should ideally set this column's value in the same
+	    database transaction that it uses to create the interface.  Open
+	    vSwitch version 2.1 and later will honor a later request for a
+	    specific port number, althuogh it might confuse some controllers:
+	    OpenFlow does not have a way to announce a port number change, so
+	    Open vSwitch represents it over OpenFlow as a port deletion
+	    followed immediately by a port addition.
+	  </p>
+
+	  <p>
+	    If <ref column="ofport_request"/> is set or changed to some other
+	    port's automatically assigned port number, Open vSwitch chooses a
+	    new port number for the latter port.
 	  </p>
 	</column>
       </group>