bridge: Update controller connection status correctly.

Updates to status-related columns in the Controller table can be lost if there
are multiple bridges with different sets of controllers. This commit fixes this
behavior by first accumulating status for all controllers on all bridges, then
making one pass over all rows in the Controller tables, updating the status of
each.

Bug #6185.
Reported-by: Michael Hu <mhu@nicira.com>

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 745e0a661f60bf311f4ec48a7fc953822e847a12..7c043a4aa8265bdf74b7bd5d6edc37bba02ab29f 100644 (file)
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -373,43 +373,45 @@ connmgr_get_controller_info(struct connmgr *mgr, struct shash *info)
 {
     const struct ofconn *ofconn;
 
-    shash_init(info);
-
     HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) {
         const struct rconn *rconn = ofconn->rconn;
-        time_t now = time_now();
-        time_t last_connection = rconn_get_last_connection(rconn);
-        time_t last_disconnect = rconn_get_last_disconnect(rconn);
-        int last_error = rconn_get_last_error(rconn);
-        struct ofproto_controller_info *cinfo = xmalloc(sizeof *cinfo);
+        const char *target = rconn_get_target(rconn);
 
-        shash_add(info, rconn_get_target(rconn), cinfo);
+        if (!shash_find(info, target)) {
+            struct ofproto_controller_info *cinfo = xmalloc(sizeof *cinfo);
+            time_t now = time_now();
+            time_t last_connection = rconn_get_last_connection(rconn);
+            time_t last_disconnect = rconn_get_last_disconnect(rconn);
+            int last_error = rconn_get_last_error(rconn);
 
-        cinfo->is_connected = rconn_is_connected(rconn);
-        cinfo->role = ofconn->role;
+            shash_add(info, target, cinfo);
 
-        cinfo->pairs.n = 0;
+            cinfo->is_connected = rconn_is_connected(rconn);
+            cinfo->role = ofconn->role;
 
-        if (last_error) {
-            cinfo->pairs.keys[cinfo->pairs.n] = "last_error";
-            cinfo->pairs.values[cinfo->pairs.n++] =
-                xstrdup(ovs_retval_to_string(last_error));
-        }
+            cinfo->pairs.n = 0;
 
-        cinfo->pairs.keys[cinfo->pairs.n] = "state";
-        cinfo->pairs.values[cinfo->pairs.n++] =
-            xstrdup(rconn_get_state(rconn));
+            if (last_error) {
+                cinfo->pairs.keys[cinfo->pairs.n] = "last_error";
+                cinfo->pairs.values[cinfo->pairs.n++]
+                    = xstrdup(ovs_retval_to_string(last_error));
+            }
 
-        if (last_connection != TIME_MIN) {
-            cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_connect";
+            cinfo->pairs.keys[cinfo->pairs.n] = "state";
             cinfo->pairs.values[cinfo->pairs.n++]
-                = xasprintf("%ld", (long int) (now - last_connection));
-        }
+                = xstrdup(rconn_get_state(rconn));
 
-        if (last_disconnect != TIME_MIN) {
-            cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_disconnect";
-            cinfo->pairs.values[cinfo->pairs.n++]
-                = xasprintf("%ld", (long int) (now - last_disconnect));
+            if (last_connection != TIME_MIN) {
+                cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_connect";
+                cinfo->pairs.values[cinfo->pairs.n++]
+                    = xasprintf("%ld", (long int) (now - last_connection));
+            }
+
+            if (last_disconnect != TIME_MIN) {
+                cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_disconnect";
+                cinfo->pairs.values[cinfo->pairs.n++]
+                    = xasprintf("%ld", (long int) (now - last_disconnect));
+            }
         }
     }
 }

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 446f4fc0c0516989e5516b7bb013c64f6a1f7dcd..449957a9f5372242a40fc1822e1cb5356f22b36c 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1348,13 +1348,20 @@ nx_role_to_str(enum nx_role role)
 }
 
 static void
-bridge_refresh_controller_status(const struct bridge *br)
+refresh_controller_status(void)
 {
+    struct bridge *br;
     struct shash info;
     const struct ovsrec_controller *cfg;
 
-    ofproto_get_ofproto_controller_info(br->ofproto, &info);
+    shash_init(&info);
+
+    /* Accumulate status for controllers on all bridges. */
+    HMAP_FOR_EACH (br, node, &all_bridges) {
+        ofproto_get_ofproto_controller_info(br->ofproto, &info);
+    }
 
+    /* Update each controller in the database with current status. */
     OVSREC_CONTROLLER_FOR_EACH(cfg, idl) {
         struct ofproto_controller_info *cinfo =
             shash_find_data(&info, cfg->target);
@@ -1448,9 +1455,9 @@ bridge_run(void)
                         iface_refresh_status(iface);
                     }
                 }
-                bridge_refresh_controller_status(br);
             }
             refresh_system_stats(cfg);
+            refresh_controller_status();
             ovsdb_idl_txn_commit(txn);
             ovsdb_idl_txn_destroy(txn); /* XXX */
         }