From: Ben Pfaff <blp@nicira.com>
Date: Mon, 30 Sep 2013 20:07:35 +0000 (-0700)
Subject: bridge: Let ofprotos run once before reporting configuration complete.
X-Git-Url: http://git.cascardo.eti.br/?a=commitdiff_plain;h=aeed7879059792858d27cd22619326dae214047a;p=cascardo%2Fovs.git

bridge: Let ofprotos run once before reporting configuration complete.

Occasionally in the unit tests the following race can happen:

   1. ovs-vsctl updates database
   2. ovs-vswitchd reconfigures, notifies ovs-vsctl that it is complete
   3. ovs-appctl ofproto/trace fails to see newly added port
   4. ovs-vswitchd main loop calls ofproto's ->type_run(), making the
      new port visible to translation.

This race may be seen in the failures of tests 5 and 624 here:
https://launchpadlibrarian.net/151884888/buildlog_ubuntu-precise-amd64.openvswitch_2.0~201309300804-1ppa1~precise_FAILEDTOBUILD.txt.gz

Reported-by: Vasiliy Tolstov <v.tolstov@selfip.ru>
Signed-off-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/AUTHORS b/AUTHORS
index 2632f108b..ab570f826 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -231,6 +231,7 @@ Teemu Koponen           koponen@nicira.com
 Timothy Chen            tchen@nicira.com
 Torbjorn Tornkvist      kruskakli@gmail.com
 Valentin Bud            valentin@hackaserver.com
+Vasiliy Tolstov         v.tolstov@selfip.ru
 Vishal Swarankar        vishal.swarnkar@gmail.com
 Vjekoslav Brajkovic     balkan@cs.washington.edu
 Voravit T.              voravit@kth.se
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index f325afa68..9282c59d2 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -176,6 +176,7 @@ static long long int iface_stats_timer = LLONG_MIN;
 #define OFP_PORT_ACTION_WINDOW 10
 
 static void add_del_bridges(const struct ovsrec_open_vswitch *);
+static void bridge_run__(void);
 static void bridge_create(const struct ovsrec_bridge *);
 static void bridge_destroy(struct bridge *);
 static struct bridge *bridge_lookup(const char *name);
@@ -601,6 +602,13 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
         }
     }
     free(managers);
+
+    /* The ofproto-dpif provider does some final reconfiguration in its
+     * ->type_run() function.  We have to call it before notifying the database
+     * client that reconfiguration is complete, otherwise there is a very
+     * narrow race window in which e.g. ofproto/trace will not recognize the
+     * new configuration (sometimes this causes unit test failures). */
+    bridge_run__();
 }
 
 /* Delete ofprotos which aren't configured or have the wrong type.  Create
@@ -2252,13 +2260,32 @@ instant_stats_wait(void)
     }
 }
 
+static void
+bridge_run__(void)
+{
+    struct bridge *br;
+    struct sset types;
+    const char *type;
+
+    /* Let each datapath type do the work that it needs to do. */
+    sset_init(&types);
+    ofproto_enumerate_types(&types);
+    SSET_FOR_EACH (type, &types) {
+        ofproto_type_run(type);
+    }
+    sset_destroy(&types);
+
+    /* Let each bridge do the work that it needs to do. */
+    HMAP_FOR_EACH (br, node, &all_bridges) {
+        ofproto_run(br->ofproto);
+    }
+}
+
 void
 bridge_run(void)
 {
     static struct ovsrec_open_vswitch null_cfg;
     const struct ovsrec_open_vswitch *cfg;
-    struct sset types;
-    const char *type;
 
     bool vlan_splinters_changed;
     struct bridge *br;
@@ -2301,18 +2328,7 @@ bridge_run(void)
                                         "flow-restore-wait", false));
     }
 
-    /* Let each datapath type do the work that it needs to do. */
-    sset_init(&types);
-    ofproto_enumerate_types(&types);
-    SSET_FOR_EACH (type, &types) {
-        ofproto_type_run(type);
-    }
-    sset_destroy(&types);
-
-    /* Let each bridge do the work that it needs to do. */
-    HMAP_FOR_EACH (br, node, &all_bridges) {
-        ofproto_run(br->ofproto);
-    }
+    bridge_run__();
 
     /* Re-configure SSL.  We do this on every trip through the main loop,
      * instead of just when the database changes, because the contents of the