bridge: When ports disappear from a datapath, add them back.

Before commit 2a73b1d73d4bdb (bridge: Reconfigure in single pass.), if a
port disappeared, for one reason or another, from a datapath, the next
bridge reconfiguration pass would notice and, if the port was still
configured in the database, add the port back to the datapath.  That
commit, however, removed the logic from bridge_refresh_ofp_port() that
did that and failed to add the same logic to the replacement function
bridge_delete_or_reconfigure_ports().  This commit fixes the problem.

To see this problem on a Linux kernel system:

ovs-vsctl add-br br0                             # 1
tunctl -t tap                                    # 2
ovs-vsctl add-port br0 tap                       # 3
ovs-dpctl show                                   # 4
tunctl -d tap                                    # 5
ovs-dpctl show                                   # 6
tunctl -t tap                                    # 7
ovs-vsctl del-port tap -- add-port br0 tap       # 8
ovs-dpctl show                                   # 9

Steps 1-4 create a bridge and a tap and add it to the bridge and
demonstrate that the tap is part of the datapath.  Step 5 and 6 delete
the tap and demonstrate that it has therefore disappeared from the
datapath.  Step 7 recreates a tap with the same name, and step 8
forces ovs-vswitchd to reconfigure.  Step 9 shows the effect of the
fix: without the fix, the new tap is not added back to the datapath;
with this fix, it is.

Special thanks to Gurucharan Shetty <gshetty@nicira.com> for finding a
simple reproduction case and then bisecting to find the commit that
introduced the problem.

Bug #1238467.
Reported-by: Ronald Lee <ronaldlee@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index f46d002..45a1491 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -252,6 +252,7 @@ static bool iface_is_internal(const struct ovsrec_interface *iface,
 static const char *iface_get_type(const struct ovsrec_interface *,
                                   const struct ovsrec_bridge *);
 static void iface_destroy(struct iface *);
+static void iface_destroy__(struct iface *);
 static struct iface *iface_lookup(const struct bridge *, const char *name);
 static struct iface *iface_find(const char *name);
 static struct iface *iface_from_ofp_port(const struct bridge *,
@@ -667,6 +668,9 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
     struct ofproto_port ofproto_port;
     struct ofproto_port_dump dump;
 
+    struct sset ofproto_ports;
+    struct port *port, *port_next;
+
     /* List of "ofp_port"s to delete.  We make a list instead of deleting them
      * right away because ofproto implementations aren't necessarily able to
      * iterate through a changing list of ports in an entirely robust way. */
@@ -676,11 +680,21 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 
     del = NULL;
     n = allocated = 0;
+    sset_init(&ofproto_ports);
 
+    /* Main task: Iterate over the ports in 'br->ofproto' and remove the ports
+     * that are not configured in the database.  (This commonly happens when
+     * ports have been deleted, e.g. with "ovs-vsctl del-port".)
+     *
+     * Side tasks: Reconfigure the ports that are still in 'br'.  Delete ports
+     * that have the wrong OpenFlow port number (and arrange to add them back
+     * with the correct OpenFlow port number). */
     OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
         ofp_port_t requested_ofp_port;
         struct iface *iface;
 
+        sset_add(&ofproto_ports, ofproto_port.name);
+
         iface = iface_lookup(br, ofproto_port.name);
         if (!iface) {
             /* No such iface is configured, so we should delete this
@@ -746,11 +760,37 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
         iface_destroy(iface);
         del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
     }
-
     for (i = 0; i < n; i++) {
         ofproto_port_del(br->ofproto, del[i]);
     }
     free(del);
+
+    /* Iterate over this module's idea of interfaces in 'br'.  Remove any ports
+     * that we didn't see when we iterated through the datapath, i.e. ports
+     * that disappeared underneath use.  This is an unusual situation, but it
+     * can happen in some cases:
+     *
+     *     - An admin runs a command like "ovs-dpctl del-port" (which is a bad
+     *       idea but could happen).
+     *
+     *     - The port represented a device that disappeared, e.g. a tuntap
+     *       device destroyed via "tunctl -d", a physical Ethernet device
+     *       whose module was just unloaded via "rmmod", or a virtual NIC for a
+     *       VM whose VM was just terminated. */
+    HMAP_FOR_EACH_SAFE (port, port_next, hmap_node, &br->ports) {
+        struct iface *iface, *iface_next;
+
+        LIST_FOR_EACH_SAFE (iface, iface_next, port_elem, &port->ifaces) {
+            if (!sset_contains(&ofproto_ports, iface->name)) {
+                iface_destroy__(iface);
+            }
+        }
+
+        if (list_is_empty(&port->ifaces)) {
+            port_destroy(port);
+        }
+    }
+    sset_destroy(&ofproto_ports);
 }
 
 static void
@@ -3145,8 +3185,6 @@ bridge_configure_dp_desc(struct bridge *br)
 \f
 /* Port functions. */
 
-static void iface_destroy__(struct iface *);
-
 static struct port *
 port_create(struct bridge *br, const struct ovsrec_port *cfg)
 {