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 745e0a6..7c043a4 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 446f4fc..449957a 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 */
         }