Return-Path: X-Original-To: apmail-mesos-commits-archive@www.apache.org Delivered-To: apmail-mesos-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 7DA54115CB for ; Mon, 4 Aug 2014 22:09:24 +0000 (UTC) Received: (qmail 77230 invoked by uid 500); 4 Aug 2014 22:09:24 -0000 Delivered-To: apmail-mesos-commits-archive@mesos.apache.org Received: (qmail 77180 invoked by uid 500); 4 Aug 2014 22:09:24 -0000 Mailing-List: contact commits-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mesos.apache.org Delivered-To: mailing list commits@mesos.apache.org Received: (qmail 76774 invoked by uid 99); 4 Aug 2014 22:09:24 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 04 Aug 2014 22:09:24 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id 0830491FB83; Mon, 4 Aug 2014 22:09:24 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: benh@apache.org To: commits@mesos.apache.org Date: Mon, 04 Aug 2014 22:10:01 -0000 Message-Id: In-Reply-To: <5cfb9554dcfc44a38cb58605b5fc3dce@git.apache.org> References: <5cfb9554dcfc44a38cb58605b5fc3dce@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [40/43] git commit: Addressing Docker review comments Addressing Docker review comments Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/48e7d4a4 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/48e7d4a4 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/48e7d4a4 Branch: refs/heads/master Commit: 48e7d4a4438331129d5604315788c0a421542093 Parents: 233e2d4 Author: Timothy Chen Authored: Thu Jul 24 10:57:34 2014 -0700 Committer: Benjamin Hindman Committed: Mon Aug 4 15:08:17 2014 -0700 ---------------------------------------------------------------------- src/Makefile.am | 4 - src/docker/docker.cpp | 447 ++++++++++++--------- src/docker/docker.hpp | 73 ++-- src/examples/docker_no_executor_framework.cpp | 2 +- src/slave/containerizer/containerizer.cpp | 2 +- src/slave/containerizer/docker.cpp | 240 +++++------ src/slave/containerizer/docker.hpp | 8 +- src/slave/flags.hpp | 2 +- src/tests/cgroups_tests.cpp | 13 + src/tests/docker_containerizer_tests.cpp | 56 +-- src/tests/docker_tests.cpp | 69 ++-- src/tests/environment.cpp | 30 +- 12 files changed, 495 insertions(+), 451 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/Makefile.am ---------------------------------------------------------------------- diff --git a/src/Makefile.am b/src/Makefile.am index 850fad3..04be4e0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1154,10 +1154,6 @@ EXTRA_DIST += examples/python/test_containerizer.py \ examples/python/test_framework.py -# Docker test executor image files. -EXTRA_DIST += tests/mesos_test_executor_docker_image/Dockerfile \ - tests/mesos_test_executor_docker_image/install.sh - dist_check_SCRIPTS += \ tests/balloon_framework_test.sh \ tests/low_level_scheduler_libprocess_test.sh \ http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/docker/docker.cpp ---------------------------------------------------------------------- diff --git a/src/docker/docker.cpp b/src/docker/docker.cpp index 4842cee..ee9c882 100644 --- a/src/docker/docker.cpp +++ b/src/docker/docker.cpp @@ -20,14 +20,17 @@ #include #include -#include - +#include #include +#include #include #include #include +#include + +#include "common/status_utils.hpp" #include "docker/docker.hpp" @@ -46,8 +49,66 @@ using std::string; using std::vector; -Try Docker::validate(const Docker &docker) +template +static Future failure( + const string& cmd, + int status, + const string& err) +{ + return Failure( + "Failed to '" + cmd + "': exit status = " + + WSTRINGIFY(status) + " stderr = " + err); +} + + +// Asynchronously read stderr from subprocess. +static Future err(const Subprocess& s) +{ + CHECK_SOME(s.err()); + + Try nonblock = os::nonblock(s.err().get()); + if (nonblock.isError()) { + return Failure("Cannot set nonblock for stderr: " + nonblock.error()); + } + + // TODO(tnachen): Although unlikely, it's possible to not capture + // the caller's failure message if io::read stderr fails. Can + // chain a callback to at least log. + return io::read(s.err().get()); +} + + +static Future _checkError(const string& cmd, const Subprocess& s) +{ + Option status = s.status().get(); + if (status.isNone()) { + return Failure("No status found for '" + cmd + "'"); + } + + if (status.get() != 0) { + // TODO(tnachen): Consider returning stdout as well. + return err(s).then( + lambda::bind(failure, cmd, status.get(), lambda::_1)); + } + + return Nothing(); +} + + +// Returns a failure if no status or non-zero status returned from +// subprocess. +static Future checkError(const string& cmd, const Subprocess& s) +{ + return s.status().then(lambda::bind(_checkError, cmd, s)); +} + + +Try Docker::create(const string& path, bool validate) { + if (!validate) { + return Docker(path); + } + // Make sure that cgroups are mounted, and at least the 'cpu' // subsystem is attached. Result hierarchy = cgroups::hierarchy("cpu"); @@ -58,76 +119,109 @@ Try Docker::validate(const Docker &docker) "to mount cgroups manually!"); } - Future info = docker.info(); + std::string cmd = path + " info"; + + Try s = subprocess( + cmd, + Subprocess::PATH("/dev/null"), + Subprocess::PIPE(), + Subprocess::PATH("/dev/null")); - if (!info.await(Seconds(3))) { - return Error("Failed to use Docker: Timed out"); - } else if (info.isFailed()) { - return Error("Failed to use Docker: " + info.failure()); + if (s.isError()) { + return Error(s.error()); } - return Nothing(); + Try nonblock = os::nonblock(s.get().out().get()); + if (nonblock.isError()) { + return Error("Failed to accept nonblock stdout:" + nonblock.error()); + } + + Future output = io::read(s.get().out().get()); + + if (!output.await(Seconds(5))) { + return Error("Docker info failed with time out"); + } else if (output.isFailed()) { + return Error("Docker info failed: " + output.failure()); + } + + return Docker(path); } -string Docker::Container::id() const +Try Docker::Container::create(const JSON::Object& json) { map::const_iterator entry = json.values.find("Id"); - CHECK(entry != json.values.end()); - JSON::Value value = entry->second; - CHECK(value.is()); - return value.as().value; -} + if (entry == json.values.end()) { + return Error("Unable to find Id in container"); + } -string Docker::Container::name() const -{ - map::const_iterator entry = - json.values.find("Name"); - CHECK(entry != json.values.end()); - JSON::Value value = entry->second; - CHECK(value.is()); - return value.as().value; -} + JSON::Value idValue = entry->second; + if (!idValue.is()) { + return Error("Id in container is not a string type"); + } -Option Docker::Container::pid() const -{ - map::const_iterator state = - json.values.find("State"); - CHECK(state != json.values.end()); - JSON::Value value = state->second; - CHECK(value.is()); + string id = idValue.as().value; - map::const_iterator entry = - value.as().values.find("Pid"); - CHECK(entry != json.values.end()); - // TODO(yifan) reload operator '=' to reuse the value variable above. + entry = json.values.find("Name"); + if (entry == json.values.end()) { + return Error("Unable to find Name in container"); + } + + JSON::Value nameValue = entry->second; + if (!nameValue.is()) { + return Error("Name in container is not string type"); + } + + string name = nameValue.as().value; + + entry = json.values.find("State"); + if (entry == json.values.end()) { + return Error("Unable to find State in container"); + } + + JSON::Value stateValue = entry->second; + if (!stateValue.is()) { + return Error("State in container is not object type"); + } + + entry = stateValue.as().values.find("Pid"); + if (entry == json.values.end()) { + return Error("Unable to find Pid in State"); + } + + // TODO(yifan): Reload operator '=' to reuse the value variable above. JSON::Value pidValue = entry->second; - CHECK(pidValue.is()); + if (!pidValue.is()) { + return Error("Pid in State is not number type"); + } pid_t pid = pid_t(pidValue.as().value); - if (pid == 0) { - return None(); + + Option optionalPid; + if (pid != 0) { + optionalPid = pid; } - return pid; + + return Docker::Container(id, name, optionalPid); } -Future > Docker::run( + +Future Docker::run( const string& image, const string& command, const string& name, const Option& resources, const Option >& env) const { - - string cmd = " run -d"; + string cmd = path + " run -d"; if (resources.isSome()) { // TODO(yifan): Support other resources (e.g. disk, ports). Option cpus = resources.get().cpus(); if (cpus.isSome()) { uint64_t cpuShare = - std::max((uint64_t) (CPU_SHARES_PER_CPU * cpus.get()), MIN_CPU_SHARES); + std::max((uint64_t) (CPU_SHARES_PER_CPU * cpus.get()), MIN_CPU_SHARES); cmd += " -c " + stringify(cpuShare); } @@ -139,6 +233,8 @@ Future > Docker::run( } if (env.isSome()) { + // TODO(tnachen): Use subprocess with args instead once we can + // handle splitting command string into args. foreachpair (string key, string value, env.get()) { key = strings::replace(key, "\"", "\\\""); value = strings::replace(value, "\"", "\\\""); @@ -148,88 +244,96 @@ Future > Docker::run( cmd += " --net=host --name=" + name + " " + image + " " + command; - VLOG(1) << "Running " << path << cmd; + VLOG(1) << "Running " << cmd; Try s = subprocess( - path + cmd, - Subprocess::PIPE(), - Subprocess::PIPE(), + cmd, + Subprocess::PATH("/dev/null"), + Subprocess::PATH("/dev/null"), Subprocess::PIPE()); if (s.isError()) { return Failure(s.error()); } - return s.get().status(); + + return checkError(cmd, s.get()); } -Future > Docker::kill(const string& container) const +Future Docker::kill(const string& container, bool remove) const { - VLOG(1) << "Running " << path << " kill " << container; + const string cmd = path + " kill " + container; + + VLOG(1) << "Running " << cmd; Try s = subprocess( - path + " kill " + container, - Subprocess::PIPE(), - Subprocess::PIPE(), + cmd, + Subprocess::PATH("/dev/null"), + Subprocess::PATH("/dev/null"), Subprocess::PIPE()); if (s.isError()) { return Failure(s.error()); } - return s.get().status(); + return s.get().status() + .then(lambda::bind( + &Docker::_kill, + *this, + container, + cmd, + s.get(), + remove)); } - -Future > Docker::rm( +Future Docker::_kill( + const Docker& docker, const string& container, - const bool force) const + const string& cmd, + const Subprocess& s, + bool remove) { - string cmd = force ? " rm -f " : " rm "; - - VLOG(1) << "Running " << path << cmd << container; - - Try s = subprocess( - path + cmd + container, - Subprocess::PIPE(), - Subprocess::PIPE(), - Subprocess::PIPE()); + Option status = s.status().get(); - if (s.isError()) { - return Failure(s.error()); + if (remove) { + bool force = !status.isSome() || status.get() != 0; + return docker.rm(container, force); } - return s.get().status(); + return checkError(cmd, s); } -Future > Docker::killAndRm(const string& container) const +Future Docker::rm( + const string& container, + bool force) const { - return kill(container) - .then(lambda::bind(Docker::_killAndRm, *this, container, lambda::_1)); -} + const string cmd = path + (force ? " rm -f " : " rm ") + container; + VLOG(1) << "Running " << cmd; -Future > Docker::_killAndRm( - const Docker& docker, - const string& container, - const Option& status) -{ - // If 'kill' fails, then do a 'rm -f'. - if (status.isNone()) { - return docker.rm(container, true); + Try s = subprocess( + cmd, + Subprocess::PATH("/dev/null"), + Subprocess::PATH("/dev/null"), + Subprocess::PIPE()); + + if (s.isError()) { + return Failure(s.error()); } - return docker.rm(container); + + return checkError(cmd, s.get()); } Future Docker::inspect(const string& container) const { - VLOG(1) << "Running " << path << " inspect " << container; + const string cmd = path + " inspect " + container; + VLOG(1) << "Running " << cmd; Try s = subprocess( - path + " inspect " + container, - Subprocess::PIPE(), + cmd, + Subprocess::PATH("/dev/null"), Subprocess::PIPE(), Subprocess::PIPE()); @@ -238,88 +342,61 @@ Future Docker::inspect(const string& container) const } return s.get().status() - .then(lambda::bind(&Docker::_inspect, s.get())); + .then(lambda::bind(&Docker::_inspect, cmd, s.get())); } -namespace os { - -inline Result read( - int fd, - Option size = None(), - size_t chunk = 16 * 4096) -{ - std::string result; - - while (size.isNone() || result.size() < size.get()) { - char buffer[chunk]; - ssize_t length = ::read(fd, buffer, chunk); - - if (length < 0) { - // TODO(bmahler): Handle a non-blocking fd? (EAGAIN, EWOULDBLOCK) - if (errno == EINTR) { - continue; - } - return ErrnoError(); - } else if (length == 0) { - // Reached EOF before expected! Only return as much data as - // available or None if we haven't read anything yet. - if (result.size() > 0) { - return result; - } - return None(); - } - - result.append(buffer, length); - } - - return result; -} - -} // namespace os { - - -Future Docker::_inspect(const Subprocess& s) +Future Docker::_inspect( + const string& cmd, + const Subprocess& s) { // Check the exit status of 'docker inspect'. CHECK_READY(s.status()); Option status = s.status().get(); - if (status.isSome() && status.get() != 0) { - // TODO(benh): Include stderr in error message. - Result read = os::read(s.err().get()); - return Failure("Failed to do 'docker inspect': " + - (read.isSome() - ? read.get() - : " exited with status " + stringify(status.get()))); + if (!status.isSome()) { + return Failure("No status found from '" + cmd + "'"); + } else if (status.get() != 0) { + return err(s).then( + lambda::bind( + failure, + cmd, + status.get(), + lambda::_1)); } // Read to EOF. - // TODO(benh): Read output asynchronously. CHECK_SOME(s.out()); - Result output = os::read(s.out().get()); - - if (output.isError()) { - // TODO(benh): Include stderr in error message. - return Failure("Failed to read output: " + output.error()); - } else if (output.isNone()) { - // TODO(benh): Include stderr in error message. - return Failure("No output available"); + Try nonblock = os::nonblock(s.out().get()); + if (nonblock.isError()) { + return Failure("Failed to accept nonblock stdout:" + nonblock.error()); } + Future output = io::read(s.out().get()); + return output.then(lambda::bind(&Docker::__inspect, lambda::_1)); +} - Try parse = JSON::parse(output.get()); + +Future Docker::__inspect(const string& output) +{ + Try parse = JSON::parse(output); if (parse.isError()) { return Failure("Failed to parse JSON: " + parse.error()); } JSON::Array array = parse.get(); - - // Skip the container if it no longer exists. + // Only return if only one container identified with name. if (array.values.size() == 1) { CHECK(array.values.front().is()); - return Docker::Container(array.values.front().as()); + Try container = + Docker::Container::create(array.values.front().as()); + + if (container.isError()) { + return Failure("Unable to create container: " + container.error()); + } + + return container.get(); } // TODO(benh): Handle the case where the short container ID was @@ -330,16 +407,16 @@ Future Docker::_inspect(const Subprocess& s) Future > Docker::ps( - const bool all, + bool all, const Option& prefix) const { - string cmd = all ? " ps -a" : " ps"; + string cmd = path + (all ? " ps -a" : " ps"); - VLOG(1) << "Running " << path << cmd; + VLOG(1) << "Running " << cmd; Try s = subprocess( - path + cmd, - Subprocess::PIPE(), + cmd, + Subprocess::PATH("/dev/null"), Subprocess::PIPE(), Subprocess::PIPE()); @@ -348,39 +425,46 @@ Future > Docker::ps( } return s.get().status() - .then(lambda::bind(&Docker::_ps, *this, s.get(), prefix)); + .then(lambda::bind(&Docker::_ps, *this, cmd, s.get(), prefix)); } Future > Docker::_ps( const Docker& docker, + const string& cmd, const Subprocess& s, const Option& prefix) { - // Check the exit status of 'docker ps'. - CHECK_READY(s.status()); - Option status = s.status().get(); - if (status.isSome() && status.get() != 0) { - // TODO(benh): Include stderr in error message. - return Failure("Failed to do 'docker ps'"); + if (!status.isSome()) { + return Failure("No status found from '" + cmd + "'"); + } else if (status.get() != 0) { + return err(s).then( + lambda::bind( + failure >, + cmd, + status.get(), + lambda::_1)); } // Read to EOF. - // TODO(benh): Read output asynchronously. CHECK_SOME(s.out()); - Result output = os::read(s.out().get()); - - if (output.isError()) { - // TODO(benh): Include stderr in error message. - return Failure("Failed to read output: " + output.error()); - } else if (output.isNone()) { - // TODO(benh): Include stderr in error message. - return Failure("No output available"); + Try nonblock = os::nonblock(s.out().get()); + if (nonblock.isError()) { + return Failure("Failed to accept nonblock stdout:" + nonblock.error()); } + Future output = io::read(s.out().get()); + return output.then(lambda::bind(&Docker::__ps, docker, prefix, lambda::_1)); +} - vector lines = strings::tokenize(output.get(), "\n"); + +Future > Docker::__ps( + const Docker& docker, + const Option& prefix, + const string& output) +{ + vector lines = strings::tokenize(output, "\n"); // Skip the header. CHECK(!lines.empty()); @@ -392,6 +476,7 @@ Future > Docker::_ps( // Inspect the containers that we are interested in depending on // whether or not a 'prefix' was specified. vector columns = strings::split(strings::trim(line), " "); + // We expect the name column to be the last column from ps. string name = columns[columns.size() - 1]; if (prefix.isNone()) { futures.push_back(docker.inspect(name)); @@ -402,33 +487,3 @@ Future > Docker::_ps( return collect(futures); } - - -Future Docker::info() const -{ - std::string cmd = path + " info"; - - VLOG(1) << "Running " << cmd; - - Try s = subprocess( - cmd, - Subprocess::PIPE(), - Subprocess::PIPE(), - Subprocess::PIPE()); - - if (s.isError()) { - return Failure(s.error()); - } - - Result output = os::read(s.get().out().get()); - - if (output.isError()) { - // TODO(benh): Include stderr in error message. - return Failure("Failed to read output: " + output.error()); - } else if (output.isNone()) { - // TODO(benh): Include stderr in error message. - return Failure("No output available"); - } - - return output.get(); -} http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/docker/docker.hpp ---------------------------------------------------------------------- diff --git a/src/docker/docker.hpp b/src/docker/docker.hpp index c4724de..98b2d60 100644 --- a/src/docker/docker.hpp +++ b/src/docker/docker.hpp @@ -37,55 +37,50 @@ class Docker { public: - // Validate Docker support - static Try validate(const Docker& docker); + // Create Docker abstraction and optionally validate docker. + static Try create(const std::string& path, bool validate = true); class Container { public: - Container(const JSON::Object& json) : json(json) {} + static Try create(const JSON::Object& json); // Returns the ID of the container. - std::string id() const; + std::string id; // Returns the name of the container. - std::string name() const; + std::string name; - // Returns the Pid of the container, or None if the container is + // Returns the pid of the container, or None if the container is // not running. - Option pid() const; + Option pid; private: - JSON::Object json; // JSON returned from 'docker inspect'. + Container( + const std::string& _id, + const std::string& _name, + const Option& _pid) + : id(_id), name(_name), pid(_pid) {} }; - // Uses the specified path to the Docker CLI tool. - Docker(const std::string& path) : path(path) {} - // Performs 'docker run IMAGE'. - process::Future > run( + process::Future run( const std::string& image, const std::string& command, const std::string& name, const Option& resources = None(), const Option >& env = None()) const; - // Performs 'docker kill CONTAINER'. - process::Future > kill( - const std::string& container) const; + // Performs 'docker kill CONTAINER'. If remove is true then a rm -f + // will be called when kill failed, otherwise a failure is returned. + process::Future kill( + const std::string& container, + bool remove = false) const; // Performs 'docker rm (-f) CONTAINER'. - process::Future > rm( + process::Future rm( const std::string& container, - const bool force = false) const; - - // Performs 'docker kill && docker rm' - // if 'docker kill' fails, then will do a 'docker rm -f'. - // - // TODO(yifan): Depreciate this when the docker provides - // something like 'docker rm --kill'. - process::Future > killAndRm( - const std::string& container) const; + bool force = false) const; // Performs 'docker inspect CONTAINER'. process::Future inspect( @@ -93,23 +88,37 @@ public: // Performs 'docker ps (-a)'. process::Future > ps( - const bool all = false, + bool all = false, const Option& prefix = None()) const; - process::Future info() const; - private: - // Continuations. + // Uses the specified path to the Docker CLI tool. + Docker(const std::string& _path) : path(_path) {}; + + static process::Future _kill( + const Docker& docker, + const std::string& container, + const std::string& cmd, + const process::Subprocess& s, + bool remove); + static process::Future _inspect( + const std::string& cmd, const process::Subprocess& s); + + static process::Future __inspect( + const std::string& output); + static process::Future > _ps( const Docker& docker, + const std::string& cmd, const process::Subprocess& s, const Option& prefix); - static process::Future > _killAndRm( + + static process::Future > __ps( const Docker& docker, - const std::string& container, - const Option& status); + const Option& prefix, + const std::string& output); const std::string path; }; http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/examples/docker_no_executor_framework.cpp ---------------------------------------------------------------------- diff --git a/src/examples/docker_no_executor_framework.cpp b/src/examples/docker_no_executor_framework.cpp index 3619405..d5385d9 100644 --- a/src/examples/docker_no_executor_framework.cpp +++ b/src/examples/docker_no_executor_framework.cpp @@ -176,7 +176,7 @@ int main(int argc, char** argv) FrameworkInfo framework; framework.set_user(""); // Have Mesos fill in the current user. - framework.set_name("No Executor Framework (C++)"); + framework.set_name("Docker No Executor Framework (C++)"); // TODO(vinod): Make checkpointing the default when it is default // on the slave. http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/slave/containerizer/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/containerizer.cpp b/src/slave/containerizer/containerizer.cpp index 003775b..c91ba38 100644 --- a/src/slave/containerizer/containerizer.cpp +++ b/src/slave/containerizer/containerizer.cpp @@ -173,7 +173,7 @@ Try Containerizer::create(const Flags& flags, bool local) } } else if (type == "docker") { Try containerizer = - DockerContainerizer::create(flags, local); + DockerContainerizer::create(flags); if (containerizer.isError()) { return Error("Could not create DockerContainerizer: " + containerizer.error()); http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/slave/containerizer/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index 294b4c2..904cdd3 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -28,8 +28,6 @@ #include #include -#include "common/status_utils.hpp" - #include "docker/docker.hpp" #ifdef __linux__ @@ -75,11 +73,10 @@ class DockerContainerizerProcess { public: DockerContainerizerProcess( - const Flags& flags, - bool local, - const Docker& docker) - : flags(flags), - docker(docker) {} + const Flags& _flags, + const Docker& _docker) + : flags(_flags), + docker(_docker) {} virtual process::Future recover( const Option& state); @@ -115,7 +112,7 @@ public: virtual void destroy( const ContainerID& containerId, - const bool& killed = true); + bool killed = true); // process is either killed or reaped. virtual process::Future > containers(); @@ -131,16 +128,14 @@ private: const std::string& directory, const SlaveID& slaveId, const PID& slavePid, - bool checkpoint, - const Option& status); + bool checkpoint); process::Future _launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const SlaveID& slaveId, const PID& slavePid, - bool checkpoint, - const Option& status); + bool checkpoint); process::Future __launch( const ContainerID& containerId, @@ -150,16 +145,15 @@ private: bool checkpoint, const Docker::Container& container); - void _destroy( const ContainerID& containerId, - const bool& killed, - const Future >& future); + bool killed, + const Future& future); void __destroy( const ContainerID& containerId, - const bool& killed, - const Future >& status); + bool killed, + const Future >& status); process::Future _update( const ContainerID& containerId, @@ -174,9 +168,7 @@ private: // container destroy. void reaped(const ContainerID& containerId); - // Parse the ContainerID from a Docker container and return None if - // the container was not launched from Mesos. - Option parse(const Docker::Container& container); + static std::string containerName(const ContainerID& containerId); const Flags flags; @@ -201,26 +193,48 @@ private: }; +// Parse the ContainerID from a Docker container and return None if +// the container was not launched from Mesos. +Option parse( + const Docker::Container& container) +{ + Option name = None(); + + if (strings::startsWith(container.name, DOCKER_NAME_PREFIX)) { + name = strings::remove( + container.name, DOCKER_NAME_PREFIX, strings::PREFIX); + } else if (strings::startsWith(container.name, "/" + DOCKER_NAME_PREFIX)) { + name = strings::remove( + container.name, "/" + DOCKER_NAME_PREFIX, strings::PREFIX); + } + + if (name.isSome()) { + ContainerID id; + id.set_value(name.get()); + return id; + } + + return None(); +} + + Try DockerContainerizer::create( - const Flags& flags, - bool local) + const Flags& flags) { - Docker docker(flags.docker); - Try validation = Docker::validate(docker); - if (validation.isError()) { - return Error(validation.error()); + Try docker = Docker::create(flags.docker); + if (docker.isError()) { + return Error(docker.error()); } - return new DockerContainerizer(flags, local, docker); + return new DockerContainerizer(flags, docker.get()); } DockerContainerizer::DockerContainerizer( const Flags& flags, - bool local, const Docker& docker) { - process = new DockerContainerizerProcess(flags, local, docker); + process = new DockerContainerizerProcess(flags, docker); spawn(process); } @@ -355,6 +369,12 @@ static int setup(const string& directory) } +string DockerContainerizerProcess::containerName(const ContainerID& containerId) +{ + return DOCKER_NAME_PREFIX + stringify(containerId); +} + + Future DockerContainerizerProcess::recover( const Option& state) { @@ -415,7 +435,6 @@ Future DockerContainerizerProcess::recover( promises.put(containerId, promise); - CHECK_SOME(run.get().forkedPid); pid_t pid = run.get().forkedPid.get(); statuses[containerId] = process::reap(pid); @@ -429,8 +448,7 @@ Future DockerContainerizerProcess::recover( // pid as one that just exited (highly unlikely) and the // slave dies after the new executor is launched but before // it hears about the termination of the earlier executor - // (also unlikely). Regardless, the launcher can't do - // anything sensible so this is considered an error. + // (also unlikely). return Failure( "Detected duplicate pid " + stringify(pid) + " for container " + stringify(containerId)); @@ -453,7 +471,7 @@ Future DockerContainerizerProcess::_recover( { foreach (const Docker::Container& container, containers) { VLOG(1) << "Checking if Docker container named '" - << container.name() << "' was started by Mesos"; + << container.name << "' was started by Mesos"; Option id = parse(container); @@ -470,7 +488,7 @@ Future DockerContainerizerProcess::_recover( if (!statuses.keys().contains(id.get())) { // TODO(benh): Retry 'docker rm -f' if it failed but the container // still exists (asynchronously). - docker.killAndRm(container.id()); + docker.kill(container.id, true); } } @@ -488,14 +506,13 @@ Future DockerContainerizerProcess::launch( bool checkpoint) { if (promises.contains(containerId)) { - LOG(ERROR) << "Cannot start already running container '" - << containerId << "'"; return Failure("Container already started"); } CommandInfo command = executorInfo.command(); if (!command.has_container()) { + LOG(INFO) << "No container info found, skipping launch"; return false; } @@ -503,6 +520,7 @@ Future DockerContainerizerProcess::launch( // Check if we should try and launch this command. if (!strings::startsWith(image, "docker:///")) { + LOG(INFO) << "No docker image found, skipping launch"; return false; } @@ -519,7 +537,7 @@ Future DockerContainerizerProcess::launch( image = strings::remove(image, "docker:///", strings::PREFIX); // Construct the Docker container name. - string name = DOCKER_NAME_PREFIX + stringify(containerId); + string name = containerName(containerId); map env = executorEnvironment( executorInfo, @@ -546,8 +564,7 @@ Future DockerContainerizerProcess::launch( executorInfo, slaveId, slavePid, - checkpoint, - lambda::_1)) + checkpoint)) .onFailed(defer(self(), &Self::destroy, containerId, false)); } @@ -563,12 +580,11 @@ Future DockerContainerizerProcess::launch( bool checkpoint) { if (promises.contains(containerId)) { - LOG(ERROR) << "Cannot start already running container '" - << containerId << "'"; return Failure("Container already started"); } if (!taskInfo.has_command()) { + LOG(WARNING) << "Not expecting call without command info"; return false; } @@ -577,6 +593,8 @@ Future DockerContainerizerProcess::launch( // Check if we should try and launch this command. if (!command.has_container() || !strings::startsWith(command.container().image(), "docker:///")) { + LOG(INFO) << "No container info or container image is not docker image, " + << "skipping launch"; return false; } @@ -598,7 +616,7 @@ Future DockerContainerizerProcess::launch( image = strings::remove(image, "docker:///", strings::PREFIX); // Construct the Docker container name. - string name = DOCKER_NAME_PREFIX + stringify(containerId); + string name = containerName(containerId); // Start a docker container then launch the executor (but destroy // the Docker container if launching the executor failed). @@ -611,8 +629,7 @@ Future DockerContainerizerProcess::launch( directory, slaveId, slavePid, - checkpoint, - lambda::_1)) + checkpoint)) .onFailed(defer(self(), &Self::destroy, containerId, false)); } @@ -624,17 +641,8 @@ Future DockerContainerizerProcess::_launch( const string& directory, const SlaveID& slaveId, const PID& slavePid, - bool checkpoint, - const Option& status) + bool checkpoint) { - // Try and see if the run failed. - if (status.isSome() && status.get() != 0) { - // Best effort kill and remove the container just in case. - docker.killAndRm(DOCKER_NAME_PREFIX + stringify(containerId)); - return Failure("Failed to run the container (" + - WSTRINGIFY(status.get()) + ")"); - } - // Prepare environment variables for the executor. map env = executorEnvironment( executorInfo, @@ -656,9 +664,8 @@ Future DockerContainerizerProcess::_launch( // don't want the exit status from 'docker wait' but rather the exit // status from the container, hence the use of /bin/bash. string override = - "/bin/bash -c 'exit `" + - flags.docker + " wait " + DOCKER_NAME_PREFIX + stringify(containerId) + - "`'"; + "/bin/sh -c 'exit `" + + flags.docker + " wait " + containerName(containerId) + "`'"; Try s = subprocess( executorInfo.command().value() + " --override " + override, @@ -708,9 +715,9 @@ Future DockerContainerizerProcess::_launch( errno == EINTR); if (length != sizeof(c)) { + string error = string(strerror(errno)); os::close(s.get().in().get()); - return Failure("Failed to synchronize with child process: " + - string(strerror(errno))); + return Failure("Failed to synchronize with child process: " + error); } // And finally watch for when the executor gets reaped. @@ -728,17 +735,9 @@ Future DockerContainerizerProcess::_launch( const ExecutorInfo& executorInfo, const SlaveID& slaveId, const PID& slavePid, - bool checkpoint, - const Option& status) + bool checkpoint) { - if (status.isSome() && status.get() != 0) { - // Best effort kill and remove the container just in case. - docker.killAndRm(DOCKER_NAME_PREFIX + stringify(containerId)); - return Failure("Failed to run the container (" + - WSTRINGIFY(status.get()) + ")"); - } - - return docker.inspect(DOCKER_NAME_PREFIX + stringify(containerId)) + return docker.inspect(containerName(containerId)) .then(defer(self(), &Self::__launch, containerId, @@ -758,18 +757,18 @@ Future DockerContainerizerProcess::__launch( bool checkpoint, const Docker::Container& container) { - Option pid = container.pid(); + Option pid = container.pid; if (!pid.isSome()) { return Failure("Unable to get executor pid after launch"); } if (checkpoint) { - // TODO(tnachen): We might not be able to checkpoint - // if the slave dies before it can checkpoint while - // the executor is still running. Optinally we can consider - // recording the slave id and executor id as part of the - // docker container name so we can recover from this. + // TODO(tnachen): We might not be able to checkpoint if the slave + // dies before it can checkpoint while the executor is still + // running. Optinally we can consider recording the slave id and + // executor id as part of the docker container name so we can + // recover from this. const string& path = slave::paths::getForkedPidPath( slave::paths::getMetaRootDir(flags.work_dir), @@ -806,22 +805,21 @@ Future DockerContainerizerProcess::update( const Resources& _resources) { if (!promises.contains(containerId)) { - LOG(WARNING) - << "Ignoring updating unknown container: " - << containerId.value(); + LOG(WARNING) << "Ignoring updating unknown container: " + << containerId; return Nothing(); } + // Store the resources for usage() + resources.put(containerId, _resources); + #ifdef __linux__ if (!_resources.cpus().isSome() && !_resources.mem().isSome()) { LOG(WARNING) << "Ignoring update as no supported resources are present"; return Nothing(); } - // Store the resources for usage() - resources.put(containerId, _resources); - - return docker.inspect(DOCKER_NAME_PREFIX + stringify(containerId)) + return docker.inspect(containerName(containerId)) .then(defer(self(), &Self::_update, containerId, _resources, lambda::_1)); #else return Nothing(); @@ -860,7 +858,7 @@ Future DockerContainerizerProcess::_update( // update the proper cgroup control files. // First check that this container still appears to be running. - Option pid = container.pid(); + Option pid = container.pid; if (pid.isNone()) { return Nothing(); } @@ -873,10 +871,9 @@ Future DockerContainerizerProcess::_update( return Failure("Failed to determine cgroup for the 'cpu' subsystem: " + cpuCgroup.error()); } else if (cpuCgroup.isNone()) { - LOG(WARNING) - << "Container " << containerId - << " does not appear to be a member of a cgroup " - << "where the 'cpu' subsystem is mounted"; + LOG(WARNING) << "Container " << containerId + << " does not appear to be a member of a cgroup " + << "where the 'cpu' subsystem is mounted"; } // And update the CPU shares (if applicable). @@ -895,10 +892,9 @@ Future DockerContainerizerProcess::_update( return Failure("Failed to update 'cpu.shares': " + write.error()); } - LOG(INFO) - << "Updated 'cpu.shares' to " << shares - << " at " << path::join(cpuHierarchy.get(), cpuCgroup.get()) - << " for container " << containerId; + LOG(INFO) << "Updated 'cpu.shares' to " << shares + << " at " << path::join(cpuHierarchy.get(), cpuCgroup.get()) + << " for container " << containerId; } // Now determine the cgroup for the 'memory' subsystem. @@ -908,16 +904,16 @@ Future DockerContainerizerProcess::_update( return Failure("Failed to determine cgroup for the 'memory' subsystem: " + memoryCgroup.error()); } else if (memoryCgroup.isNone()) { - LOG(WARNING) - << "Container " << containerId - << " does not appear to be a member of a cgroup " - << "where the 'memory' subsystem is mounted"; + LOG(WARNING) << "Container " << containerId + << " does not appear to be a member of a cgroup " + << "where the 'memory' subsystem is mounted"; } // And update the memory limits (if applicable). if (memoryHierarchy.isSome() && memoryCgroup.isSome() && _resources.mem().isSome()) { + // TODO(tnachen): investigate and handle OOM with docker. Bytes mem = _resources.mem().get(); Bytes limit = std::max(mem, MIN_MEMORY); @@ -931,9 +927,8 @@ Future DockerContainerizerProcess::_update( write.error()); } - LOG(INFO) - << "Updated 'memory.soft_limit_in_bytes' to " << limit - << " for container " << containerId; + LOG(INFO) << "Updated 'memory.soft_limit_in_bytes' to " << limit + << " for container " << containerId; // Read the existing limit. Try currentLimit = @@ -946,6 +941,9 @@ Future DockerContainerizerProcess::_update( } // Only update if new limit is higher. + // TODO(benh): Introduce a MemoryWatcherProcess which monitors the + // discrepancy between usage and soft limit and introduces a + // "manual oom" if necessary. if (limit > currentLimit.get()) { write = cgroups::memory::limit_in_bytes( memoryHierarchy.get(), memoryCgroup.get(), limit); @@ -955,10 +953,9 @@ Future DockerContainerizerProcess::_update( write.error()); } - LOG(INFO) - << "Updated 'memory.limit_in_bytes' to " << limit - << " at " << path::join(memoryHierarchy.get(), memoryCgroup.get()) - << " for container " << containerId; + LOG(INFO) << "Updated 'memory.limit_in_bytes' to " << limit << " at " + << path::join(memoryHierarchy.get(), memoryCgroup.get()) + << " for container " << containerId; } } #endif // __linux__ @@ -982,7 +979,7 @@ Future DockerContainerizerProcess::usage( } // Construct the Docker container name. - string name = DOCKER_NAME_PREFIX + stringify(containerId); + string name = containerName(containerId); return docker.inspect(name) .then(defer(self(), &Self::_usage, containerId, lambda::_1)); #endif // __linux__ @@ -993,7 +990,7 @@ Future DockerContainerizerProcess::_usage( const ContainerID& containerId, const Docker::Container& container) { - Option pid = container.pid(); + Option pid = container.pid; if (pid.isNone()) { return Failure("Container is not running"); } @@ -1038,7 +1035,7 @@ Future DockerContainerizerProcess::wait( void DockerContainerizerProcess::destroy( const ContainerID& containerId, - const bool& killed) + bool killed) { if (!promises.contains(containerId)) { LOG(WARNING) << "Ignoring destroy of unknown container: " << containerId; @@ -1071,15 +1068,15 @@ void DockerContainerizerProcess::destroy( // TODO(benh): Retry 'docker rm -f' if it failed but the container // still exists (asynchronously). - docker.killAndRm(DOCKER_NAME_PREFIX + stringify(containerId)) + docker.kill(containerName(containerId), true) .onAny(defer(self(), &Self::_destroy, containerId, killed, lambda::_1)); } void DockerContainerizerProcess::_destroy( const ContainerID& containerId, - const bool& killed, - const Future >& future) + bool killed, + const Future& future) { if (!future.isReady()) { promises[containerId]->fail( @@ -1108,7 +1105,7 @@ void DockerContainerizerProcess::_destroy( void DockerContainerizerProcess::__destroy( const ContainerID& containerId, - const bool& killed, + bool killed, const Future >& status) { containerizer::Termination termination; @@ -1116,6 +1113,8 @@ void DockerContainerizerProcess::__destroy( if (status.isReady() && status.get().isSome()) { termination.set_status(status.get().get()); } + termination.set_message(killed ? + "Docker task killed" : "Docker process terminated"); promises[containerId]->set(termination); @@ -1144,29 +1143,6 @@ void DockerContainerizerProcess::reaped(const ContainerID& containerId) } -Option DockerContainerizerProcess::parse( - const Docker::Container& container) -{ - Option name = None(); - - if (strings::startsWith(container.name(), DOCKER_NAME_PREFIX)) { - name = strings::remove( - container.name(), DOCKER_NAME_PREFIX, strings::PREFIX); - } else if (strings::startsWith(container.name(), "/" + DOCKER_NAME_PREFIX)) { - name = strings::remove( - container.name(), "/" + DOCKER_NAME_PREFIX, strings::PREFIX); - } - - if (name.isSome()) { - ContainerID id; - id.set_value(name.get()); - return id; - } - - return None(); -} - - } // namespace slave { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/slave/containerizer/docker.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.hpp b/src/slave/containerizer/docker.hpp index f4eb0ff..fbbd45d 100644 --- a/src/slave/containerizer/docker.hpp +++ b/src/slave/containerizer/docker.hpp @@ -41,15 +41,11 @@ class DockerContainerizerProcess; class DockerContainerizer : public Containerizer { public: - static Try create( - const Flags& flags, - bool local); - - static Try prepareCgroups(const Flags& flags); + static Try create(const Flags& flags); + // This is only public for tests. DockerContainerizer( const Flags& flags, - bool local, const Docker& docker); virtual ~DockerContainerizer(); http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/slave/flags.hpp ---------------------------------------------------------------------- diff --git a/src/slave/flags.hpp b/src/slave/flags.hpp index 66bba7c..c348109 100644 --- a/src/slave/flags.hpp +++ b/src/slave/flags.hpp @@ -279,7 +279,7 @@ public: add(&Flags::docker, "docker", - "The path to the docker executable for docker containerizer.", + "The absolute path to the docker executable for docker containerizer.", "docker"); #ifdef WITH_NETWORK_ISOLATOR http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/tests/cgroups_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/cgroups_tests.cpp b/src/tests/cgroups_tests.cpp index 01cf498..1d2fc36 100644 --- a/src/tests/cgroups_tests.cpp +++ b/src/tests/cgroups_tests.cpp @@ -288,6 +288,19 @@ TEST_F(CgroupsAnyHierarchyWithCpuMemoryTest, ROOT_CGROUPS_SubsystemsHierarchy) } +TEST_F(CgroupsAnyHierarchyWithCpuMemoryTest, ROOT_CGROUPS_FindCgroupSubsystems) +{ + pid_t pid = ::getpid(); + Result cpuHierarchy = cgroups::cpu::cgroup(pid); + EXPECT_FALSE(cpuHierarchy.isError()); + EXPECT_SOME(cpuHierarchy); + + Result memHierarchy = cgroups::memory::cgroup(pid); + EXPECT_FALSE(memHierarchy.isError()); + EXPECT_SOME(memHierarchy); +} + + TEST_F(CgroupsNoHierarchyTest, ROOT_CGROUPS_NOHIERARCHY_MountUnmountHierarchy) { EXPECT_ERROR(cgroups::mount("/tmp", "cpu")); http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/tests/docker_containerizer_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/docker_containerizer_tests.cpp b/src/tests/docker_containerizer_tests.cpp index 84324b0..e936e7e 100644 --- a/src/tests/docker_containerizer_tests.cpp +++ b/src/tests/docker_containerizer_tests.cpp @@ -57,15 +57,15 @@ using testing::Return; class DockerContainerizerTest : public MesosTest { public: - static bool containerExists( + static bool exists( const list& containers, const ContainerID& containerId) { - string expectedName = slave::DOCKER_NAME_PREFIX + containerId.value(); + string expectedName = slave::DOCKER_NAME_PREFIX + stringify(containerId); foreach (const Docker::Container& container, containers) { // Docker inspect name contains an extra slash in the beginning. - if (strings::contains(container.name(), expectedName)) { + if (strings::contains(container.name, expectedName)) { return true; } } @@ -79,9 +79,8 @@ class MockDockerContainerizer : public DockerContainerizer { public: MockDockerContainerizer( const slave::Flags& flags, - bool local, const Docker& docker) - : DockerContainerizer(flags, local, docker) + : DockerContainerizer(flags, docker) { EXPECT_CALL(*this, launch(_, _, _, _, _, _, _)) .WillRepeatedly(Invoke(this, &MockDockerContainerizer::_launchExecutor)); @@ -188,9 +187,9 @@ TEST_F(DockerContainerizerTest, DOCKER_Launch_Executor) slave::Flags flags = CreateSlaveFlags(); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker, false).get(); - MockDockerContainerizer dockerContainerizer(flags, true, docker); + MockDockerContainerizer dockerContainerizer(flags, docker); Try > slave = StartSlave(&dockerContainerizer); ASSERT_SOME(slave); @@ -262,7 +261,7 @@ TEST_F(DockerContainerizerTest, DOCKER_Launch_Executor) AWAIT_READY(containers); - ASSERT_TRUE(containerExists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), containerId.get())); Future termination = dockerContainerizer.wait(containerId.get()); @@ -275,7 +274,7 @@ TEST_F(DockerContainerizerTest, DOCKER_Launch_Executor) containers = docker.ps(true, slave::DOCKER_NAME_PREFIX); AWAIT_READY(containers); - ASSERT_FALSE(containerExists(containers.get(), containerId.get())); + ASSERT_FALSE(exists(containers.get(), containerId.get())); Shutdown(); } @@ -289,9 +288,9 @@ TEST_F(DockerContainerizerTest, DOCKER_Launch) slave::Flags flags = CreateSlaveFlags(); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker, false).get(); - MockDockerContainerizer dockerContainerizer(flags, true, docker); + MockDockerContainerizer dockerContainerizer(flags, docker); Try > slave = StartSlave(&dockerContainerizer); ASSERT_SOME(slave); @@ -358,7 +357,7 @@ TEST_F(DockerContainerizerTest, DOCKER_Launch) ASSERT_TRUE(containers.get().size() > 0); - ASSERT_TRUE(containerExists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), containerId.get())); dockerContainerizer.destroy(containerId.get()); @@ -376,9 +375,9 @@ TEST_F(DockerContainerizerTest, DOCKER_Kill) slave::Flags flags = CreateSlaveFlags(); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker, false).get(); - MockDockerContainerizer dockerContainerizer(flags, true, docker); + MockDockerContainerizer dockerContainerizer(flags, docker); Try > slave = StartSlave(&dockerContainerizer); ASSERT_SOME(slave); @@ -456,7 +455,7 @@ TEST_F(DockerContainerizerTest, DOCKER_Kill) AWAIT_READY(containers); - ASSERT_FALSE(containerExists(containers.get(), containerId.get())); + ASSERT_FALSE(exists(containers.get(), containerId.get())); driver.stop(); driver.join(); @@ -474,9 +473,9 @@ TEST_F(DockerContainerizerTest, DOCKER_Usage) slave::Flags flags = CreateSlaveFlags(); flags.resources = Option("cpus:2;mem:1024"); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker).get(); - MockDockerContainerizer dockerContainerizer(flags, true, docker); + MockDockerContainerizer dockerContainerizer(flags, docker); Try > slave = StartSlave(&dockerContainerizer, flags); ASSERT_SOME(slave); @@ -593,9 +592,9 @@ TEST_F(DockerContainerizerTest, DOCKER_Update) slave::Flags flags = CreateSlaveFlags(); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker).get(); - MockDockerContainerizer dockerContainerizer(flags, true, docker); + MockDockerContainerizer dockerContainerizer(flags, docker); Try > slave = StartSlave(&dockerContainerizer); ASSERT_SOME(slave); @@ -676,7 +675,7 @@ TEST_F(DockerContainerizerTest, DOCKER_Update) ASSERT_SOME(cpuHierarchy); ASSERT_SOME(memoryHierarchy); - Option pid = container.get().pid(); + Option pid = container.get().pid; ASSERT_SOME(pid); Result cpuCgroup = cgroups::cpu::cgroup(pid.get()); @@ -715,13 +714,20 @@ TEST_F(DockerContainerizerTest, DOCKER_Update) #endif //__linux__ -TEST_F(DockerContainerizerTest, DOCKER_Recover) +// Disabling recover test as the docker rm in recover is async. +// Even though we wait for the container to finish, when the wait +// returns docker rm might still be in progress. +// TODO(tnachen): Re-enable test when we wait for the async kill +// to finish. One way to do this is to mock the Docker interface +// and let the mocked docker collect all the remove futures and +// at the end of the test wait for all of them before the test exits. +TEST_F(DockerContainerizerTest, DISABLED_DOCKER_Recover) { slave::Flags flags = CreateSlaveFlags(); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker).get(); - MockDockerContainerizer dockerContainerizer(flags, true, docker); + MockDockerContainerizer dockerContainerizer(flags, docker); ContainerID containerId; containerId.set_value("c1"); @@ -730,14 +736,14 @@ TEST_F(DockerContainerizerTest, DOCKER_Recover) Resources resources = Resources::parse("cpus:1;mem:512").get(); - Future > d1 = + Future d1 = docker.run( "busybox", "sleep 360", slave::DOCKER_NAME_PREFIX + stringify(containerId), resources); - Future > d2 = + Future d2 = docker.run( "busybox", "sleep 360", http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/tests/docker_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/docker_tests.cpp b/src/tests/docker_tests.cpp index b7a7b6f..1951d9a 100644 --- a/src/tests/docker_tests.cpp +++ b/src/tests/docker_tests.cpp @@ -33,43 +33,40 @@ using namespace mesos; using namespace mesos::internal; -using process::Future; +using namespace process; using std::list; using std::string; -// This test tests the functionality of the -// docker's interfaces. +// This test tests the functionality of the docker's interfaces. TEST(DockerTest, DOCKER_interface) { string containerName = "mesos-docker-test"; Resources resources = Resources::parse("cpus:1;mem:512").get(); - Docker docker(tests::flags.docker); + Docker docker = Docker::create(tests::flags.docker, false).get(); - // Cleaning up the container first. - Future > status = docker.rm(containerName, true); - AWAIT_READY(status); - ASSERT_SOME(status.get()); + // Cleaning up the container first if it exists. + Future status = docker.rm(containerName, true); + ASSERT_TRUE(status.await(Seconds(10))); // Verify that we do not see the container. - Future > containers = docker.ps(true); + Future > containers = docker.ps(true, containerName); AWAIT_READY(containers); foreach (const Docker::Container& container, containers.get()) { - EXPECT_NE("/" + containerName, container.name()); + EXPECT_NE("/" + containerName, container.name); } // Start the container. status = docker.run("busybox", "sleep 120", containerName, resources); AWAIT_READY(status); - ASSERT_SOME(status.get()); // Should be able to see the container now. containers = docker.ps(); AWAIT_READY(containers); bool found = false; foreach (const Docker::Container& container, containers.get()) { - if ("/" + containerName == container.name()) { + if ("/" + containerName == container.name) { found = true; break; } @@ -80,75 +77,70 @@ TEST(DockerTest, DOCKER_interface) AWAIT_READY(container); // Test some fields of the container. - EXPECT_NE("", container.get().id()); - EXPECT_EQ("/" + containerName, container.get().name()); - EXPECT_SOME(container.get().pid()); + EXPECT_NE("", container.get().id); + EXPECT_EQ("/" + containerName, container.get().name); + EXPECT_SOME(container.get().pid); // Kill the container. status = docker.kill(containerName); AWAIT_READY(status); - ASSERT_SOME(status.get()); // Now, the container should not appear in the result of ps(). // But it should appear in the result of ps(true). containers = docker.ps(); AWAIT_READY(containers); foreach (const Docker::Container& container, containers.get()) { - EXPECT_NE("/" + containerName, container.name()); + EXPECT_NE("/" + containerName, container.name); } - containers = docker.ps(true); + containers = docker.ps(true, containerName); AWAIT_READY(containers); found = false; foreach (const Docker::Container& container, containers.get()) { - if ("/" + containerName == container.name()) { + if ("/" + containerName == container.name) { found = true; break; } } EXPECT_TRUE(found); - // Check the container's info, both id and name should remain - // the same since we haven't removed it, but the pid should be none + // Check the container's info, both id and name should remain the + // same since we haven't removed it, but the pid should be none // since it's not running. container = docker.inspect(containerName); AWAIT_READY(container); - EXPECT_NE("", container.get().id()); - EXPECT_EQ("/" + containerName, container.get().name()); - EXPECT_NONE(container.get().pid()); + EXPECT_NE("", container.get().id); + EXPECT_EQ("/" + containerName, container.get().name); + EXPECT_NONE(container.get().pid); // Remove the container. status = docker.rm(containerName); AWAIT_READY(status); - ASSERT_SOME(status.get()); // Should not be able to inspect the container. container = docker.inspect(containerName); AWAIT_FAILED(container); - // Also, now we should not be able to see the container - // by invoking ps(true). - containers = docker.ps(true); + // Also, now we should not be able to see the container by invoking + // ps(true). + containers = docker.ps(true, containerName); AWAIT_READY(containers); foreach (const Docker::Container& container, containers.get()) { - EXPECT_NE("/" + containerName, container.name()); + EXPECT_NE("/" + containerName, container.name); } // Start the container again, this time we will do a "rm -f" // directly, instead of killing and rm. - // - // First, Invoke docker.run() status = docker.run("busybox", "sleep 120", containerName, resources); AWAIT_READY(status); - ASSERT_SOME(status.get()); // Verify that the container is there. containers = docker.ps(); AWAIT_READY(containers); found = false; foreach (const Docker::Container& container, containers.get()) { - if ("/" + containerName == container.name()) { + if ("/" + containerName == container.name) { found = true; break; } @@ -158,18 +150,17 @@ TEST(DockerTest, DOCKER_interface) // Then do a "rm -f". status = docker.rm(containerName, true); AWAIT_READY(status); - ASSERT_SOME(status.get()); - // Verify that the container is totally removed, - // that is we can't find it by ps() or ps(true). + // Verify that the container is totally removed, that is we can't + // find it by ps() or ps(true). containers = docker.ps(); AWAIT_READY(containers); foreach (const Docker::Container& container, containers.get()) { - EXPECT_NE("/" + containerName, container.name()); + EXPECT_NE("/" + containerName, container.name); } - containers = docker.ps(true); + containers = docker.ps(true, containerName); AWAIT_READY(containers); foreach (const Docker::Container& container, containers.get()) { - EXPECT_NE("/" + containerName, container.name()); + EXPECT_NE("/" + containerName, container.name); } } http://git-wip-us.apache.org/repos/asf/mesos/blob/48e7d4a4/src/tests/environment.cpp ---------------------------------------------------------------------- diff --git a/src/tests/environment.cpp b/src/tests/environment.cpp index 6c80fa3..eec7d3e 100644 --- a/src/tests/environment.cpp +++ b/src/tests/environment.cpp @@ -29,7 +29,6 @@ #include #include -#include #include #include @@ -131,29 +130,30 @@ static bool enable(const ::testing::TestInfo& test) } #endif + // Filter out benchmark tests when we run 'make check'. + if (strings::contains(name, "BENCHMARK_") && !flags.benchmark) { + return false; + } + if (strings::contains(name, "DOCKER_")) { - Docker docker(flags.docker); - Try validate = Docker::validate(docker); - if (validate.isError()) { + Try docker = Docker::create(flags.docker); + if (docker.isError()) { std::cerr << "-------------------------------------------------------------\n" << "Skipping Docker tests because validation failed\n" - << "[Error] " + validate.error() + "\n" + << "[Error] " + docker.error() + "\n" << "-------------------------------------------------------------" << std::endl; + + return false; } #ifdef __linux__ - return user.get() == "root" && !validate.isError(); -#else - return !validate.isError(); + if (user.get() != "root") { + return false; + } #endif } - - // Filter out benchmark tests when we run 'make check'. - if (strings::contains(name, "BENCHMARK_") && !flags.benchmark) { - return false; - } } // Filter out regular tests when we run 'make bench', which @@ -170,7 +170,9 @@ static bool enable(const ::testing::TestInfo& test) const string& type = test.type_param(); if (strings::contains(type, "Cgroups")) { #ifdef __linux__ - return user.get() == "root" && cgroups::enabled(); + if (user.get() != "root" || !cgroups::enabled()) { + return false; + } #else return false; #endif