[5/5] mesos git commit: Synchronized global environment manipulation in TestContainerizer.

[jo...@apache.org]

Synchronized global environment manipulation in TestContainerizer.

This fixes non-deterministic behavior in the TestContainerizer. There
is still an open issue mentioned in the comments and documented in
MESOS-3475.


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/a92ff3cd
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/a92ff3cd
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/a92ff3cd

Branch: refs/heads/master
Commit: a92ff3cd7388cfcf948e4ffa3dabcad98a29e3a8
Parents: 31defd4
Author: Joris Van Remoortere <jo...@gmail.com>
Authored: Sun Sep 20 14:04:42 2015 -0400
Committer: Joris Van Remoortere <jo...@gmail.com>
Committed: Sun Sep 20 14:21:28 2015 -0400

----------------------------------------------------------------------
 src/tests/containerizer.cpp | 105 +++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 43 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/a92ff3cd/src/tests/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer.cpp b/src/tests/containerizer.cpp
index 5134e63..1f74315 100644
--- a/src/tests/containerizer.cpp
+++ b/src/tests/containerizer.cpp
@@ -17,6 +17,11 @@
  */
 
 #include "tests/containerizer.hpp"
+
+#include <mutex>
+
+#include "stout/synchronized.hpp"
+
 #include "tests/mesos.hpp"
 
 using std::map;
@@ -99,56 +104,70 @@ Future<bool> TestContainerizer::_launch(
   containers_[key] = containerId;
 
   Executor* executor = executors[executorInfo.executor_id()];
-  Owned<MesosExecutorDriver> driver(new MesosExecutorDriver(executor));
-  drivers[containerId] = driver;
-
-  // Prepare additional environment variables for the executor.
-  // TODO(benh): Need to get flags passed into the TestContainerizer
-  // in order to properly use here.
-  slave::Flags flags;
-  flags.recovery_timeout = Duration::zero();
-
-  // We need to save the original set of environment variables so we
-  // can reset the environment after calling 'driver->start()' below.
-  hashmap<string, string> original = os::environment();
-
-  const map<string, string> environment = executorEnvironment(
-      executorInfo,
-      directory,
-      slaveId,
-      slavePid,
-      checkpoint,
-      flags);
 
-  foreachpair (const string& name, const string variable, environment) {
-    os::setenv(name, variable);
-  }
+  // We need to synchronize all reads and writes to the environment as this is
+  // global state.
+  // TODO(jmlvanre): Even this is not sufficient, as other aspects of the code
+  // may read an environment variable while we are manipulating it. The better
+  // solution is to pass the environment variables into the fork, or to set them
+  // on the command line. See MESOS-3475.
+  static std::mutex mutex;
+
+  synchronized(mutex) {
+    // Since the constructor for `MesosExecutorDriver` reads environment
+    // variables to load flags, even it needs to be within this synchronization
+    // section.
+    Owned<MesosExecutorDriver> driver(new MesosExecutorDriver(executor));
+    drivers[containerId] = driver;
+
+    // Prepare additional environment variables for the executor.
+    // TODO(benh): Need to get flags passed into the TestContainerizer
+    // in order to properly use here.
+    slave::Flags flags;
+    flags.recovery_timeout = Duration::zero();
+
+    // We need to save the original set of environment variables so we
+    // can reset the environment after calling 'driver->start()' below.
+    hashmap<string, string> original = os::environment();
+
+    const map<string, string> environment = executorEnvironment(
+        executorInfo,
+        directory,
+        slaveId,
+        slavePid,
+        checkpoint,
+        flags);
+
+    foreachpair (const string& name, const string variable, environment) {
+      os::setenv(name, variable);
+    }
 
-  // TODO(benh): Can this be removed and done exlusively in the
-  // 'executorEnvironment()' function? There are other places in the
-  // code where we do this as well and it's likely we can do this once
-  // in 'executorEnvironment()'.
-  foreach (const Environment::Variable& variable,
-           executorInfo.command().environment().variables()) {
-    os::setenv(variable.name(), variable.value());
-  }
+    // TODO(benh): Can this be removed and done exlusively in the
+    // 'executorEnvironment()' function? There are other places in the
+    // code where we do this as well and it's likely we can do this once
+    // in 'executorEnvironment()'.
+    foreach (const Environment::Variable& variable,
+            executorInfo.command().environment().variables()) {
+      os::setenv(variable.name(), variable.value());
+    }
 
-  os::setenv("MESOS_LOCAL", "1");
+    os::setenv("MESOS_LOCAL", "1");
 
-  driver->start();
+    driver->start();
 
-  os::unsetenv("MESOS_LOCAL");
+    os::unsetenv("MESOS_LOCAL");
 
-  // Unset the environment variables we set by resetting them to their
-  // original values and also removing any that were not part of the
-  // original environment.
-  foreachpair (const string& name, const string& value, original) {
-    os::setenv(name, value);
-  }
+    // Unset the environment variables we set by resetting them to their
+    // original values and also removing any that were not part of the
+    // original environment.
+    foreachpair (const string& name, const string& value, original) {
+      os::setenv(name, value);
+    }
 
-  foreachkey (const string& name, environment) {
-    if (!original.contains(name)) {
-      os::unsetenv(name);
+    foreachkey (const string& name, environment) {
+      if (!original.contains(name)) {
+        os::unsetenv(name);
+      }
     }
   }