Simplified launch continuations in DockerContainerizerProcess. Combined redundant code when launching a Docker container with and without a task.
Review: https://reviews.apache.org/r/26613 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/5c3d6f1a Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/5c3d6f1a Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/5c3d6f1a Branch: refs/heads/master Commit: 5c3d6f1a99e5b249cfc0e05d0fef349b1a90a131 Parents: c3e7f2a Author: Benjamin Hindman <[email protected]> Authored: Sat Oct 11 13:22:21 2014 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Fri Oct 31 15:05:38 2014 -0700 ---------------------------------------------------------------------- src/slave/containerizer/docker.cpp | 118 +++++++++++++------------------- 1 file changed, 47 insertions(+), 71 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/5c3d6f1a/src/slave/containerizer/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index 8873022..c46de8d 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -142,22 +142,9 @@ private: process::Future<Nothing> _recover( const std::list<Docker::Container>& containers); - process::Future<bool> _launch( + process::Future<Nothing> _launch( const ContainerID& containerId, - const TaskInfo& taskInfo, - const ExecutorInfo& executorInfo, - const std::string& directory, - const SlaveID& slaveId, - const PID<Slave>& slavePid, - bool checkpoint); - - process::Future<bool> _launch( - const ContainerID& containerId, - const ExecutorInfo& executorInfo, - const string& directory, - const SlaveID& slaveId, - const PID<Slave>& slavePid, - bool checkpoint); + const std::string& directory); process::Future<bool> __launch( const ContainerID& containerId, @@ -242,17 +229,36 @@ private: { static Try<Container*> create( const ContainerID& id, + const Option<TaskInfo>& taskInfo, + const ExecutorInfo& executorInfo, const std::string& directory, const Option<std::string>& user); Container(const ContainerID& id) : state(FETCHING), id(id) {} + Container(const ContainerID& id, + const Option<TaskInfo>& taskInfo, + const ExecutorInfo& executorInfo) + : state(FETCHING), + id(id), + task(taskInfo), + executor(executorInfo) {} + std::string name() { return DOCKER_NAME_PREFIX + stringify(id); } + std::string image() const + { + if (task.isSome()) { + return task.get().container().docker().image(); + } + + return executor.container().docker().image(); + } + // The DockerContainerier needs to be able to properly clean up // Docker containers, regardless of when they are destroyed. For // example, if a container gets destroyed while we are fetching, @@ -283,6 +289,8 @@ private: } state; ContainerID id; + Option<TaskInfo> task; + ExecutorInfo executor; // Promise for future returned from wait(). Promise<containerizer::Termination> termination; @@ -370,8 +378,11 @@ DockerContainerizer::~DockerContainerizer() } -Future<Nothing> DockerContainerizerProcess::Container::create( - const ContainerID& containerId, +Try<DockerContainerizerProcess::Container*> +DockerContainerizerProcess::Container::create( + const ContainerID& id, + const Option<TaskInfo>& taskInfo, + const ExecutorInfo& executorInfo, const string& directory, const Option<string>& user) { @@ -380,24 +391,24 @@ Future<Nothing> DockerContainerizerProcess::Container::create( Try<Nothing> touch = os::touch(path::join(directory, "stdout")); if (touch.isError()) { - return Failure("Failed to touch 'stdout': " + touch.error()); + return Error("Failed to touch 'stdout': " + touch.error()); } touch = os::touch(path::join(directory, "stderr")); if (touch.isError()) { - return Failure("Failed to touch 'stderr': " + touch.error()); + return Error("Failed to touch 'stderr': " + touch.error()); } if (user.isSome()) { Try<Nothing> chown = os::chown(user.get(), directory); if (chown.isError()) { - return Failure("Failed to chown: " + chown.error()); + return Error("Failed to chown: " + chown.error()); } } - return new Container(containerId); + return new Container(id, taskInfo, executorInfo); } @@ -751,7 +762,8 @@ Future<bool> DockerContainerizerProcess::launch( return false; } - Try<Container*> container = Container::create(containerId, directory, user); + Try<Container*> container = Container::create( + containerId, taskInfo, executorInfo, directory, user); if (container.isError()) { return Failure("Failed to create container: " + container.error()); @@ -765,8 +777,9 @@ Future<bool> DockerContainerizerProcess::launch( << "') of framework '" << executorInfo.framework_id() << "'"; return fetch(containerId, taskInfo.command(), directory) + .then(defer(self(), &Self::_launch, containerId, directory)) .then(defer(self(), - &Self::_launch, + &Self::__launch, containerId, taskInfo, executorInfo, @@ -778,14 +791,9 @@ Future<bool> DockerContainerizerProcess::launch( } -Future<bool> DockerContainerizerProcess::_launch( +Future<Nothing> DockerContainerizerProcess::_launch( const ContainerID& containerId, - const TaskInfo& taskInfo, - const ExecutorInfo& executorInfo, - const string& directory, - const SlaveID& slaveId, - const PID<Slave>& slavePid, - bool checkpoint) + const string& directory) { // Doing the fetch might have succeded but we were actually asked to // destroy the container, which we did, so don't continue. @@ -793,20 +801,14 @@ Future<bool> DockerContainerizerProcess::_launch( return Failure("Container was destroyed while launching"); } - containers_[containerId]->state = Container::PULLING; + Container* container = containers_[containerId]; - return pull(containerId, directory, taskInfo.container().docker().image()) - .then(defer(self(), - &Self::__launch, - containerId, - taskInfo, - executorInfo, - directory, - slaveId, - slavePid, - checkpoint)); + container->state = Container::PULLING; + + return pull(containerId, directory, container->image()); } + Future<bool> DockerContainerizerProcess::__launch( const ContainerID& containerId, const TaskInfo& taskInfo, @@ -975,7 +977,8 @@ Future<bool> DockerContainerizerProcess::launch( return false; } - Try<Container*> container = Container::create(containerId, directory, user); + Try<Container*> container = Container::create( + containerId, None(), executorInfo, directory, user); if (container.isError()) { return Failure("Failed to create container: " + container.error()); @@ -988,8 +991,9 @@ Future<bool> DockerContainerizerProcess::launch( << "' and framework '" << executorInfo.framework_id() << "'"; return fetch(containerId, executorInfo.command(), directory) + .then(defer(self(), &Self::_launch, containerId, directory)) .then(defer(self(), - &Self::_launch, + &Self::__launch, containerId, executorInfo, directory, @@ -1000,34 +1004,6 @@ Future<bool> DockerContainerizerProcess::launch( } -Future<bool> DockerContainerizerProcess::_launch( - const ContainerID& containerId, - const ExecutorInfo& executorInfo, - const string& directory, - const SlaveID& slaveId, - const PID<Slave>& slavePid, - bool checkpoint) -{ - // Doing the fetch might have succeded but we were actually asked to - // destroy the container, which we did, so don't continue. - if (!containers_.contains(containerId)) { - return Failure("Container was destroyed while launching"); - } - - containers_[containerId]->state = Container::PULLING; - - return pull(containerId, directory, executorInfo.container().docker().image()) - .then(defer(self(), - &Self::__launch, - containerId, - executorInfo, - directory, - slaveId, - slavePid, - checkpoint)); -} - - Future<bool> DockerContainerizerProcess::__launch( const ContainerID& containerId, const ExecutorInfo& executorInfo,
