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 <[email protected]> Authored: Sun Sep 20 14:04:42 2015 -0400 Committer: Joris Van Remoortere <[email protected]> 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); + } } }
