You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by vi...@apache.org on 2013/05/07 00:55:32 UTC
git commit: Fixed process isolator to safely setup launcher's
environment.
Updated Branches:
refs/heads/master 2e38010fb -> ac245899d
Fixed process isolator to safely setup launcher's environment.
Review: https://reviews.apache.org/r/10879
Project: http://git-wip-us.apache.org/repos/asf/incubator-mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-mesos/commit/ac245899
Tree: http://git-wip-us.apache.org/repos/asf/incubator-mesos/tree/ac245899
Diff: http://git-wip-us.apache.org/repos/asf/incubator-mesos/diff/ac245899
Branch: refs/heads/master
Commit: ac245899dce3efd94f562b4e2fb9dce682aea649
Parents: 2e38010
Author: Vinod Kone <vi...@twitter.com>
Authored: Tue Apr 30 20:03:24 2013 -0700
Committer: Vinod Kone <vi...@twitter.com>
Committed: Mon May 6 15:47:13 2013 -0700
----------------------------------------------------------------------
src/launcher/launcher.cpp | 78 ++++++++++++++++++++---------------
src/launcher/launcher.hpp | 17 +++++--
src/launcher/main.cpp | 2 +-
src/slave/process_isolator.cpp | 18 ++++----
4 files changed, 68 insertions(+), 47 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-mesos/blob/ac245899/src/launcher/launcher.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/launcher.cpp b/src/launcher/launcher.cpp
index 11cffc9..c7d2c30 100644
--- a/src/launcher/launcher.cpp
+++ b/src/launcher/launcher.cpp
@@ -23,6 +23,7 @@
#include <pwd.h>
#include <iostream>
+#include <map>
#include <sstream>
#include <sys/stat.h>
@@ -45,6 +46,7 @@
using std::cerr;
using std::cout;
using std::endl;
+using std::map;
using std::ostringstream;
using std::string;
@@ -382,48 +384,60 @@ int ExecutorLauncher::fetchExecutors()
}
+void ExecutorLauncher::switchUser()
+{
+ if (!os::su(user)) {
+ fatal("Failed to switch to user %s for executor %s of framework %s",
+ user.c_str(), executorId.value().c_str(), frameworkId.value().c_str());
+ }
+}
+
+
// Set up environment variables for launching a framework's executor.
void ExecutorLauncher::setupEnvironment()
{
+ foreachpair (const string& key, const string& value, getEnvironment()) {
+ os::setenv(key, value);
+ }
+}
+
+
+map<string, string> ExecutorLauncher::getEnvironment()
+{
+ map<string, string> env;
+
// Set LIBPROCESS_PORT so that we bind to a random free port (since
// this might have been set via --port option). We do this before
// the environment variables below in case it is included.
- os::setenv("LIBPROCESS_PORT", "0");
+ env["LIBPROCESS_PORT"] = "0";
// Set up the environment as specified in the ExecutorInfo.
if (commandInfo.has_environment()) {
foreach (const Environment::Variable& variable,
commandInfo.environment().variables()) {
- os::setenv(variable.name(), variable.value());
+ env[variable.name()] = variable.value();
}
}
// Set Mesos environment variables for slave ID, framework ID, etc.
- os::setenv("MESOS_DIRECTORY", workDirectory);
- os::setenv("MESOS_SLAVE_PID", slavePid);
- os::setenv("MESOS_SLAVE_ID", slaveId.value());
- os::setenv("MESOS_FRAMEWORK_ID", frameworkId.value());
- os::setenv("MESOS_EXECUTOR_ID", executorId.value());
- os::setenv("MESOS_EXECUTOR_UUID", uuid.toString());
- os::setenv("MESOS_CHECKPOINT", checkpoint ? "1" : "0");
+ env["MESOS_DIRECTORY"] = workDirectory;
+ env["MESOS_SLAVE_PID"] = slavePid;
+ env["MESOS_SLAVE_ID"] = slaveId.value();
+ env["MESOS_FRAMEWORK_ID"] = frameworkId.value();
+ env["MESOS_EXECUTOR_ID"] = executorId.value();
+ env["MESOS_EXECUTOR_UUID"] = uuid.toString();
+ env["MESOS_CHECKPOINT"] = checkpoint ? "1" : "0";
+
+ return env;
}
-void ExecutorLauncher::switchUser()
+// Get Mesos environment variables that launcher/main.cpp will
+// pass as arguments to an ExecutorLauncher there.
+map<string, string> ExecutorLauncher::getLauncherEnvironment()
{
- if (!os::su(user)) {
- fatal("Failed to switch to user %s for executor %s of framework %s",
- user.c_str(), executorId.value().c_str(), frameworkId.value().c_str());
- }
-}
-
-
-void ExecutorLauncher::setupEnvironmentForLauncherMain()
-{
- setupEnvironment();
+ map<string, string> env = getEnvironment();
- // Set up Mesos environment variables that launcher/main.cpp will
- // pass as arguments to an ExecutorLauncher there.
string uris = "";
foreach (const CommandInfo::URI& uri, commandInfo.uris()) {
uris += uri.value() + "+" +
@@ -436,17 +450,15 @@ void ExecutorLauncher::setupEnvironmentForLauncherMain()
uris = strings::trim(uris);
}
- os::setenv("MESOS_FRAMEWORK_ID", frameworkId.value());
- os::setenv("MESOS_SLAVE_ID", slaveId.value());
- os::setenv("MESOS_COMMAND", commandInfo.value());
- os::setenv("MESOS_EXECUTOR_URIS", uris);
- os::setenv("MESOS_USER", user);
- os::setenv("MESOS_WORK_DIRECTORY", workDirectory);
- os::setenv("MESOS_SLAVE_DIRECTORY", slaveDirectory);
- os::setenv("MESOS_SLAVE_PID", slavePid);
- os::setenv("MESOS_HADOOP_HOME", hadoopHome);
- os::setenv("MESOS_REDIRECT_IO", redirectIO ? "1" : "0");
- os::setenv("MESOS_SWITCH_USER", shouldSwitchUser ? "1" : "0");
+ env["MESOS_EXECUTOR_URIS"] = uris;
+ env["MESOS_COMMAND"] = commandInfo.value();
+ env["MESOS_USER"] = user;
+ env["MESOS_SLAVE_DIRECTORY"] = slaveDirectory;
+ env["MESOS_HADOOP_HOME"] = hadoopHome;
+ env["MESOS_REDIRECT_IO"] = redirectIO ? "1" : "0";
+ env["MESOS_SWITCH_USER"] = shouldSwitchUser ? "1" : "0";
+
+ return env;
}
} // namespace launcher {
http://git-wip-us.apache.org/repos/asf/incubator-mesos/blob/ac245899/src/launcher/launcher.hpp
----------------------------------------------------------------------
diff --git a/src/launcher/launcher.hpp b/src/launcher/launcher.hpp
index 9f0e247..637c9bc 100644
--- a/src/launcher/launcher.hpp
+++ b/src/launcher/launcher.hpp
@@ -19,6 +19,7 @@
#ifndef __LAUNCHER_HPP__
#define __LAUNCHER_HPP__
+#include <map>
#include <string>
#include <mesos/mesos.hpp>
@@ -72,10 +73,11 @@ public:
// Convenience function that calls setup() and then launch().
virtual int run();
- // Set up environment variables for exec'ing a launcher_main.cpp
- // (mesos-launcher binary) process. This is used by isolators that
- // cannot exec the user's executor directly.
- virtual void setupEnvironmentForLauncherMain();
+ // Return a map of environment variables for exec'ing a
+ // launch_main.cpp (mesos-launcher binary) process. This is used
+ // by isolators that cannot exec the user's executor directly
+ // (e.g., due to potential deadlocks in forked process).
+ virtual std::map<std::string, std::string> getLauncherEnvironment();
protected:
// Download the required files for the executor from the given set of URIs.
@@ -83,7 +85,12 @@ protected:
// This method is expected to place files in the workDirectory.
virtual int fetchExecutors();
- // Set up environment variables for launching a framework's executor.
+ // Return a map of environment variables for launching a
+ // framework's executor.
+ virtual std::map<std::string, std::string> getEnvironment();
+
+ // Set up environment variables for launching a
+ // framework's executor.
virtual void setupEnvironment();
// Switch to a framework's user in preparation for exec()'ing its executor.
http://git-wip-us.apache.org/repos/asf/incubator-mesos/blob/ac245899/src/launcher/main.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/main.cpp b/src/launcher/main.cpp
index 57d671e..5674afb 100644
--- a/src/launcher/main.cpp
+++ b/src/launcher/main.cpp
@@ -64,7 +64,7 @@ int main(int argc, char** argv)
UUID::fromString(os::getenv("MESOS_EXECUTOR_UUID")),
commandInfo,
os::getenv("MESOS_USER"),
- os::getenv("MESOS_WORK_DIRECTORY"),
+ os::getenv("MESOS_DIRECTORY"),
os::getenv("MESOS_SLAVE_DIRECTORY"),
os::getenv("MESOS_SLAVE_PID"),
os::getenv("MESOS_FRAMEWORKS_HOME", false),
http://git-wip-us.apache.org/repos/asf/incubator-mesos/blob/ac245899/src/slave/process_isolator.cpp
----------------------------------------------------------------------
diff --git a/src/slave/process_isolator.cpp b/src/slave/process_isolator.cpp
index 6938fbc..11a5d18 100644
--- a/src/slave/process_isolator.cpp
+++ b/src/slave/process_isolator.cpp
@@ -141,13 +141,7 @@ void ProcessIsolator::launchExecutor(
CHECK_SOME(cloexec) << "Error setting FD_CLOEXEC on pipe[1]";
// Create the ExecutorLauncher instance before the fork for the
- // child process to use. TODO(benh): Consider actually setting up
- // this slaves environment for launching mesos-launcher so that we
- // don't run the risk of
- // ExecutorLauncher::setupEnvironmentForLauncherMain having some
- // fork issues. Alternatively, we could imagine refactoring
- // ExecutorLauncher::setupEnvironmentForLauncherMain to carefully
- // only use fork "safe" functions.
+ // child process to use.
ExecutorLauncher launcher(
slaveId,
frameworkId,
@@ -164,6 +158,11 @@ void ProcessIsolator::launchExecutor(
flags.switch_user,
frameworkInfo.checkpoint());
+ // We get the environment map for launching mesos-launcher before
+ // the fork, because we have seen deadlock issues with ostringstream
+ // in the forked process before it calls exec.
+ map<string, string> env = launcher.getLauncherEnvironment();
+
pid_t pid;
if ((pid = fork()) == -1) {
PLOG(FATAL) << "Failed to fork to launch new executor";
@@ -219,7 +218,10 @@ void ProcessIsolator::launchExecutor(
close(pipes[1]);
- launcher.setupEnvironmentForLauncherMain();
+ // Setup the environment for launcher.
+ foreachpair (const string& key, const string& value, env) {
+ os::setenv(key, value);
+ }
const char** args = (const char**) new char*[2];
http://git-wip-us.apache.org/repos/asf/incubator-mesos/blob/ac245899/support/post-reviews.py
----------------------------------------------------------------------
diff --git a/support/post-reviews.py b/support/post-reviews.py
old mode 100644
new mode 100755