bridge: Fix controller status update

When multiple bridges connects to the same controller, the controller status should be maintained separately for each bridge. Current logic pushes status updates only based on the connection string, which is the same across multiple bridges when connecting to the same controller. Report-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-May/044412.html Reported-by: Tulio Ribeiro <[email protected]> Signed-off-by: Andy Zhou <[email protected]> Reviewed-by: Greg Rose <gvrose@[email protected]>
keyman9848 · May 15, 2017 · 85c5577 · 85c5577
1 parent f5f6455
commit 85c5577
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 18 deletions.
diff --git a/AUTHORS.rst b/AUTHORS.rst
@@ -530,6 +530,7 @@ Teemu Koponen                   [email protected]
 Thomas Morin                    [email protected]
 Timothy Chen                    [email protected]
 Torbjorn Tornkvist              [email protected]
+Tulio Ribeiro                   [email protected]
 Tytus Kurek                     [email protected]
 Valentin Bud                    [email protected]
 Vasiliy Tolstov                 [email protected]

diff --git a/tests/bridge.at b/tests/bridge.at
@@ -38,3 +38,36 @@ dummy@ovs-dummy: hit:0 missed:0
 OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 AT_CLEANUP
+
+dnl When multiple bridges are connected to the same controller, make
+dnl sure their status are tracked independently.
+AT_SETUP([bridge - multiple bridges share a controller])
+OVS_VSWITCHD_START(
+   [add-br br1 -- \
+    set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
+    set bridge br1 datapath-type=dummy other-config:datapath-id=1234 ])
+
+dnl Start ovs-testcontroller
+AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore])
+
+dnl Add the controller to both bridges, 5 seconds apart.
+AT_CHECK([ovs-vsctl set-controller br0 unix:controller])
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-vsctl set-controller br1 unix:controller])
+
+dnl Wait for the controller connection to come up
+for i in `seq 0 7`
+do
+    AT_CHECK([ovs-appctl time/warp 10], [0], [ignore])
+done
+
+dnl Make sure the connection status are different
+AT_CHECK([ovs-vsctl --columns=status list controller | sort], [0], [dnl
+
+status              : {sec_since_connect="0", state=ACTIVE}
+status              : {sec_since_connect="5", state=ACTIVE}
+])
+
+OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+AT_CLEANUP
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
@@ -2704,34 +2704,31 @@ static void
 refresh_controller_status(void)
 {
     struct bridge *br;
-    struct shash info;
-    const struct ovsrec_controller *cfg;
-
-    shash_init(&info);
 
     /* Accumulate status for controllers on all bridges. */
     HMAP_FOR_EACH (br, node, &all_bridges) {
+        struct shash info = SHASH_INITIALIZER(&info);
         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);
+        /* Update each controller of the bridge in the database with
+         * current status. */
+        struct ovsrec_controller **controllers;
+        size_t n_controllers = bridge_get_controllers(br, &controllers);
+        size_t i;
+        for (i = 0; i < n_controllers; i++) {
+            struct ovsrec_controller *cfg = controllers[i];
+            struct ofproto_controller_info *cinfo =
+                shash_find_data(&info, cfg->target);
 
-        if (cinfo) {
+            ovs_assert(cinfo);
             ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
-            ovsrec_controller_set_role(cfg, ofp12_controller_role_to_str(
-                                           cinfo->role));
+            const char *role = ofp12_controller_role_to_str(cinfo->role);
+            ovsrec_controller_set_role(cfg, role);
             ovsrec_controller_set_status(cfg, &cinfo->pairs);
-        } else {
-            ovsrec_controller_set_is_connected(cfg, false);
-            ovsrec_controller_set_role(cfg, NULL);
-            ovsrec_controller_set_status(cfg, NULL);
         }
-    }
 
-    ofproto_free_ofproto_controller_info(&info);
+        ofproto_free_ofproto_controller_info(&info);
+    }
 }
 
 /* Update interface and mirror statistics if necessary. */