Repository: mesos Updated Branches: refs/heads/master fe09d7de7 -> d9315d97a
Fixed sandbox ownership bug for executors without URIs. During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root). Review: https://reviews.apache.org/r/32911 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d9315d97 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d9315d97 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d9315d97 Branch: refs/heads/master Commit: d9315d97afd08d52b75d8c36efa2148988e7db88 Parents: fe09d7d Author: Niklas Nielsen <[email protected]> Authored: Tue Apr 7 11:54:08 2015 -0700 Committer: Niklas Q. Nielsen <[email protected]> Committed: Tue Apr 7 11:54:08 2015 -0700 ---------------------------------------------------------------------- .../containerizer/external_containerizer.cpp | 17 ++++++----- src/slave/paths.cpp | 21 ++++++++++++- src/slave/paths.hpp | 3 +- src/slave/slave.cpp | 32 +++++++++++++------- 4 files changed, 52 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/d9315d97/src/slave/containerizer/external_containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/external_containerizer.cpp b/src/slave/containerizer/external_containerizer.cpp index 1bbd61c..9a72acb 100644 --- a/src/slave/containerizer/external_containerizer.cpp +++ b/src/slave/containerizer/external_containerizer.cpp @@ -349,14 +349,6 @@ Future<Nothing> ExternalContainerizerProcess::__recover( << "' for executor '" << executor.id << "' of framework " << framework.id; - // Re-create the sandbox for this container. - const string& directory = paths::createExecutorDirectory( - flags.work_dir, - state.get().id, - framework.id, - executor.id, - containerId); - Option<string> user = None(); if (flags.switch_user) { // The command (either in form of task or executor command) @@ -370,6 +362,15 @@ Future<Nothing> ExternalContainerizerProcess::__recover( } } + // Re-create the sandbox for this container. + const string directory = paths::createExecutorDirectory( + flags.work_dir, + state.get().id, + framework.id, + executor.id, + containerId, + user); + Sandbox sandbox(directory, user); // Collect this container as being active. http://git-wip-us.apache.org/repos/asf/mesos/blob/d9315d97/src/slave/paths.cpp ---------------------------------------------------------------------- diff --git a/src/slave/paths.cpp b/src/slave/paths.cpp index 01ea856..0741616 100644 --- a/src/slave/paths.cpp +++ b/src/slave/paths.cpp @@ -368,7 +368,8 @@ string createExecutorDirectory( const SlaveID& slaveId, const FrameworkID& frameworkId, const ExecutorID& executorId, - const ContainerID& containerId) + const ContainerID& containerId, + const Option<string>& user) { const string directory = getExecutorRunPath(rootDir, slaveId, frameworkId, executorId, containerId); @@ -394,6 +395,24 @@ string createExecutorDirectory( << "Failed to symlink directory '" << directory << "' to '" << latest << "'"; + if (user.isSome()) { + // Per MESOS-2592, we need to set the ownership of the executor + // directory during its creation. We should not rely on subsequent + // phases of the executor creation to ensure the ownership as + // those may be conditional and in some cases leave the executor + // directory owned by the slave user instead of the specified + // framework or per-executor user. + Try<Nothing> chown = os::chown(user.get(), directory); + if (chown.isError()) { + // TODO(nnielsen): We currently have tests which depend on using + // user names which may not be available on the test machines. + // Therefore, we cannot make the chown validation a hard + // CHECK(). + LOG(WARNING) << "Failed to chown executor directory '" << directory + << "': " << chown.error(); + } + } + return directory; } http://git-wip-us.apache.org/repos/asf/mesos/blob/d9315d97/src/slave/paths.hpp ---------------------------------------------------------------------- diff --git a/src/slave/paths.hpp b/src/slave/paths.hpp index 1618439..00476f1 100644 --- a/src/slave/paths.hpp +++ b/src/slave/paths.hpp @@ -254,7 +254,8 @@ std::string createExecutorDirectory( const SlaveID& slaveId, const FrameworkID& frameworkId, const ExecutorID& executorId, - const ContainerID& containerId); + const ContainerID& containerId, + const Option<std::string>& user = None()); std::string createSlaveDirectory( http://git-wip-us.apache.org/repos/asf/mesos/blob/d9315d97/src/slave/slave.cpp ---------------------------------------------------------------------- diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp index 521624c..9fec023 100644 --- a/src/slave/slave.cpp +++ b/src/slave/slave.cpp @@ -4157,13 +4157,31 @@ Executor* Framework::launchExecutor( ContainerID containerId; containerId.set_value(UUID::random().toString()); + Option<string> user = None(); + if (slave->flags.switch_user) { + // The command (either in form of task or executor command) can + // define a specific user to run as. If present, this precedes the + // framework user value. The selected user will have been verified by + // the master at this point through the active ACLs. + // NOTE: The global invariant is that the executor info at this + // point is (1) the user provided task.executor() or (2) a command + // executor constructed by the slave from the task.command(). + // If this changes, we need to check the user in both + // task.command() and task.executor().command() below. + user = info.user(); + if (executorInfo.command().has_user()) { + user = executorInfo.command().user(); + } + } + // Create a directory for the executor. const string& directory = paths::createExecutorDirectory( slave->flags.work_dir, slave->info.id(), id, executorInfo.executor_id(), - containerId); + containerId, + user); Executor* executor = new Executor( slave, id, executorInfo, containerId, directory, info.checkpoint()); @@ -4197,14 +4215,6 @@ Executor* Framework::launchExecutor( resources += taskInfo.resources(); executorInfo_.mutable_resources()->CopyFrom(resources); - // The command (either in form of task or executor command) can - // define a specific user to run as. If present, this precedes the - // framework user value. - string user = info.user(); - if (executor->info.command().has_user()) { - user = executor->info.command().user(); - } - // Launch the container. Future<bool> launch; if (!executor->isCommandExecutor()) { @@ -4216,7 +4226,7 @@ Executor* Framework::launchExecutor( containerId, executorInfo_, // modified to include the task's resources. executor->directory, - slave->flags.switch_user ? Option<string>(user) : None(), + user, slave->info.id(), slave->self(), info.checkpoint()); @@ -4234,7 +4244,7 @@ Executor* Framework::launchExecutor( taskInfo, executorInfo_, executor->directory, - slave->flags.switch_user ? Option<string>(user) : None(), + user, slave->info.id(), slave->self(), info.checkpoint());
