mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From b...@apache.org
Subject [40/43] git commit: Addressing Docker review comments
Date Mon, 04 Aug 2014 22:10:01 GMT
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 <tnachen@gmail.com>
Authored: Thu Jul 24 10:57:34 2014 -0700
Committer: Benjamin Hindman <benjamin.hindman@gmail.com>
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 <vector>
 
 #include <stout/lambda.hpp>
-#include <stout/strings.hpp>
-
+#include <stout/os.hpp>
 #include <stout/result.hpp>
+#include <stout/strings.hpp>
 
 #include <stout/os/read.hpp>
 
 #include <process/check.hpp>
 #include <process/collect.hpp>
+#include <process/io.hpp>
+
+#include "common/status_utils.hpp"
 
 #include "docker/docker.hpp"
 
@@ -46,8 +49,66 @@ using std::string;
 using std::vector;
 
 
-Try<Nothing> Docker::validate(const Docker &docker)
+template<class T>
+static Future<T> 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<string> err(const Subprocess& s)
+{
+  CHECK_SOME(s.err());
+
+  Try<Nothing> 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<Nothing> _checkError(const string& cmd, const Subprocess& s)
+{
+  Option<int> 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<Nothing>, cmd, status.get(), lambda::_1));
+  }
+
+  return Nothing();
+}
+
+
+// Returns a failure if no status or non-zero status returned from
+// subprocess.
+static Future<Nothing> checkError(const string& cmd, const Subprocess& s)
+{
+  return s.status().then(lambda::bind(_checkError, cmd, s));
+}
+
+
+Try<Docker> 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<string> hierarchy = cgroups::hierarchy("cpu");
@@ -58,76 +119,109 @@ Try<Nothing> Docker::validate(const Docker &docker)
                  "to mount cgroups manually!");
   }
 
-  Future<std::string> info = docker.info();
+  std::string cmd = path + " info";
+
+  Try<Subprocess> 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<Nothing> nonblock = os::nonblock(s.get().out().get());
+  if (nonblock.isError()) {
+    return Error("Failed to accept nonblock stdout:" + nonblock.error());
+  }
+
+  Future<string> 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> Docker::Container::create(const JSON::Object& json)
 {
   map<string, JSON::Value>::const_iterator entry =
     json.values.find("Id");
-  CHECK(entry != json.values.end());
-  JSON::Value value = entry->second;
-  CHECK(value.is<JSON::String>());
-  return value.as<JSON::String>().value;
-}
+  if (entry == json.values.end()) {
+    return Error("Unable to find Id in container");
+  }
 
-string Docker::Container::name() const
-{
-  map<string, JSON::Value>::const_iterator entry =
-    json.values.find("Name");
-  CHECK(entry != json.values.end());
-  JSON::Value value = entry->second;
-  CHECK(value.is<JSON::String>());
-  return value.as<JSON::String>().value;
-}
+  JSON::Value idValue = entry->second;
+  if (!idValue.is<JSON::String>()) {
+    return Error("Id in container is not a string type");
+  }
 
-Option<pid_t> Docker::Container::pid() const
-{
-  map<string, JSON::Value>::const_iterator state =
-    json.values.find("State");
-  CHECK(state != json.values.end());
-  JSON::Value value = state->second;
-  CHECK(value.is<JSON::Object>());
+  string id = idValue.as<JSON::String>().value;
 
-  map<string, JSON::Value>::const_iterator entry =
-    value.as<JSON::Object>().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<JSON::String>()) {
+    return Error("Name in container is not string type");
+  }
+
+  string name = nameValue.as<JSON::String>().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<JSON::Object>()) {
+    return Error("State in container is not object type");
+  }
+
+  entry = stateValue.as<JSON::Object>().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<JSON::Number>());
+  if (!pidValue.is<JSON::Number>()) {
+    return Error("Pid in State is not number type");
+  }
 
   pid_t pid = pid_t(pidValue.as<JSON::Number>().value);
-  if (pid == 0) {
-    return None();
+
+  Option<pid_t> optionalPid;
+  if (pid != 0) {
+    optionalPid = pid;
   }
-  return pid;
+
+  return Docker::Container(id, name, optionalPid);
 }
 
-Future<Option<int> > Docker::run(
+
+Future<Nothing> Docker::run(
     const string& image,
     const string& command,
     const string& name,
     const Option<mesos::Resources>& resources,
     const Option<map<string, string> >& env) const
 {
-
-  string cmd = " run -d";
+  string cmd = path + " run -d";
 
   if (resources.isSome()) {
     // TODO(yifan): Support other resources (e.g. disk, ports).
     Option<double> 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<Option<int> > 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<Option<int> > Docker::run(
 
   cmd += " --net=host --name=" + name + " " + image + " " + command;
 
-  VLOG(1) << "Running " << path << cmd;
+  VLOG(1) << "Running " << cmd;
 
   Try<Subprocess> 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<Option<int> > Docker::kill(const string& container) const
+Future<Nothing> 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<Subprocess> 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<Option<int> > Docker::rm(
+Future<Nothing> 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<Subprocess> s = subprocess(
-      path + cmd + container,
-      Subprocess::PIPE(),
-      Subprocess::PIPE(),
-      Subprocess::PIPE());
+  Option<int> 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<Option<int> > Docker::killAndRm(const string& container) const
+Future<Nothing> 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<Option<int> > Docker::_killAndRm(
-    const Docker& docker,
-    const string& container,
-    const Option<int>& status)
-{
-  // If 'kill' fails, then do a 'rm -f'.
-  if (status.isNone()) {
-    return docker.rm(container, true);
+  Try<Subprocess> 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::Container> Docker::inspect(const string& container) const
 {
-  VLOG(1) << "Running " << path << " inspect " << container;
+  const string cmd =  path + " inspect " + container;
+  VLOG(1) << "Running " << cmd;
 
   Try<Subprocess> s = subprocess(
-      path + " inspect " + container,
-      Subprocess::PIPE(),
+      cmd,
+      Subprocess::PATH("/dev/null"),
       Subprocess::PIPE(),
       Subprocess::PIPE());
 
@@ -238,88 +342,61 @@ Future<Docker::Container> 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<std::string> read(
-    int fd,
-    Option<size_t> 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::Container> Docker::_inspect(const Subprocess& s)
+Future<Docker::Container> Docker::_inspect(
+    const string& cmd,
+    const Subprocess& s)
 {
   // Check the exit status of 'docker inspect'.
   CHECK_READY(s.status());
 
   Option<int> status = s.status().get();
 
-  if (status.isSome() && status.get() != 0) {
-    // TODO(benh): Include stderr in error message.
-    Result<string> 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<Docker::Container>,
+            cmd,
+            status.get(),
+            lambda::_1));
   }
 
   // Read to EOF.
-  // TODO(benh): Read output asynchronously.
   CHECK_SOME(s.out());
-  Result<string> 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<Nothing> nonblock = os::nonblock(s.out().get());
+  if (nonblock.isError()) {
+    return Failure("Failed to accept nonblock stdout:" + nonblock.error());
   }
+  Future<string> output = io::read(s.out().get());
+  return output.then(lambda::bind(&Docker::__inspect, lambda::_1));
+}
 
-  Try<JSON::Array> parse = JSON::parse<JSON::Array>(output.get());
+
+Future<Docker::Container> Docker::__inspect(const string& output)
+{
+  Try<JSON::Array> parse = JSON::parse<JSON::Array>(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<JSON::Object>());
-    return Docker::Container(array.values.front().as<JSON::Object>());
+    Try<Docker::Container> container =
+      Docker::Container::create(array.values.front().as<JSON::Object>());
+
+    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::Container> Docker::_inspect(const Subprocess& s)
 
 
 Future<list<Docker::Container> > Docker::ps(
-    const bool all,
+    bool all,
     const Option<string>& 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<Subprocess> s = subprocess(
-      path + cmd,
-      Subprocess::PIPE(),
+      cmd,
+      Subprocess::PATH("/dev/null"),
       Subprocess::PIPE(),
       Subprocess::PIPE());
 
@@ -348,39 +425,46 @@ Future<list<Docker::Container> > 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<list<Docker::Container> > Docker::_ps(
     const Docker& docker,
+    const string& cmd,
     const Subprocess& s,
     const Option<string>& prefix)
 {
-  // Check the exit status of 'docker ps'.
-  CHECK_READY(s.status());
-
   Option<int> 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<list<Docker::Container> >,
+            cmd,
+            status.get(),
+            lambda::_1));
   }
 
   // Read to EOF.
-  // TODO(benh): Read output asynchronously.
   CHECK_SOME(s.out());
-  Result<string> 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<Nothing> nonblock = os::nonblock(s.out().get());
+  if (nonblock.isError()) {
+    return Failure("Failed to accept nonblock stdout:" + nonblock.error());
   }
+  Future<string> output = io::read(s.out().get());
+  return output.then(lambda::bind(&Docker::__ps, docker, prefix, lambda::_1));
+}
 
-  vector<string> lines = strings::tokenize(output.get(), "\n");
+
+Future<list<Docker::Container> > Docker::__ps(
+    const Docker& docker,
+    const Option<string>& prefix,
+    const string& output)
+{
+  vector<string> lines = strings::tokenize(output, "\n");
 
   // Skip the header.
   CHECK(!lines.empty());
@@ -392,6 +476,7 @@ Future<list<Docker::Container> > Docker::_ps(
     // Inspect the containers that we are interested in depending on
     // whether or not a 'prefix' was specified.
     vector<string> 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<list<Docker::Container> > Docker::_ps(
 
   return collect(futures);
 }
-
-
-Future<std::string> Docker::info() const
-{
-  std::string cmd = path + " info";
-
-  VLOG(1) << "Running " << cmd;
-
-  Try<Subprocess> s = subprocess(
-      cmd,
-      Subprocess::PIPE(),
-      Subprocess::PIPE(),
-      Subprocess::PIPE());
-
-  if (s.isError()) {
-    return Failure(s.error());
-  }
-
-  Result<string> 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<Nothing> validate(const Docker& docker);
+  // Create Docker abstraction and optionally validate docker.
+  static Try<Docker> create(const std::string& path, bool validate = true);
 
   class Container
   {
   public:
-    Container(const JSON::Object& json) : json(json) {}
+    static Try<Container> 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_t> pid() const;
+    Option<pid_t> pid;
 
   private:
-    JSON::Object json; // JSON returned from 'docker inspect'.
+    Container(
+        const std::string& _id,
+        const std::string& _name,
+        const Option<pid_t>& _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<Option<int> > run(
+  process::Future<Nothing> run(
       const std::string& image,
       const std::string& command,
       const std::string& name,
       const Option<mesos::Resources>& resources = None(),
       const Option<std::map<std::string, std::string> >& env = None()) const;
 
-  // Performs 'docker kill CONTAINER'.
-  process::Future<Option<int> > 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<Nothing> kill(
+      const std::string& container,
+      bool remove = false) const;
 
   // Performs 'docker rm (-f) CONTAINER'.
-  process::Future<Option<int> > rm(
+  process::Future<Nothing> 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<Option<int> > killAndRm(
-      const std::string& container) const;
+      bool force = false) const;
 
   // Performs 'docker inspect CONTAINER'.
   process::Future<Container> inspect(
@@ -93,23 +88,37 @@ public:
 
   // Performs 'docker ps (-a)'.
   process::Future<std::list<Container> > ps(
-      const bool all = false,
+      bool all = false,
       const Option<std::string>& prefix = None()) const;
 
-  process::Future<std::string> info() const;
-
 private:
-  // Continuations.
+  // Uses the specified path to the Docker CLI tool.
+  Docker(const std::string& _path) : path(_path) {};
+
+  static process::Future<Nothing> _kill(
+      const Docker& docker,
+      const std::string& container,
+      const std::string& cmd,
+      const process::Subprocess& s,
+      bool remove);
+
   static process::Future<Container> _inspect(
+      const std::string& cmd,
       const process::Subprocess& s);
+
+  static process::Future<Container> __inspect(
+      const std::string& output);
+
   static process::Future<std::list<Container> > _ps(
       const Docker& docker,
+      const std::string& cmd,
       const process::Subprocess& s,
       const Option<std::string>& prefix);
-  static process::Future<Option<int> > _killAndRm(
+
+  static process::Future<std::list<Container> > __ps(
       const Docker& docker,
-      const std::string& container,
-      const Option<int>& status);
+      const Option<std::string>& 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*> Containerizer::create(const Flags& flags, bool local)
       }
     } else if (type == "docker") {
       Try<DockerContainerizer*> 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 <stout/hashset.hpp>
 #include <stout/os.hpp>
 
-#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<Nothing> recover(
       const Option<state::SlaveState>& 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<hashset<ContainerID> > containers();
 
@@ -131,16 +128,14 @@ private:
       const std::string& directory,
       const SlaveID& slaveId,
       const PID<Slave>& slavePid,
-      bool checkpoint,
-      const Option<int>& status);
+      bool checkpoint);
 
   process::Future<bool> _launch(
       const ContainerID& containerId,
       const ExecutorInfo& executorInfo,
       const SlaveID& slaveId,
       const PID<Slave>& slavePid,
-      bool checkpoint,
-      const Option<int>& status);
+      bool checkpoint);
 
   process::Future<bool> __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<Option<int> >& future);
+      bool killed,
+      const Future<Nothing>& future);
 
   void __destroy(
       const ContainerID& containerId,
-      const bool& killed,
-      const Future<Option<int > >& status);
+      bool killed,
+      const Future<Option<int> >& status);
 
   process::Future<Nothing> _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<ContainerID> 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<ContainerID> parse(
+    const Docker::Container& container)
+{
+  Option<string> 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*> DockerContainerizer::create(
-    const Flags& flags,
-    bool local)
+    const Flags& flags)
 {
-  Docker docker(flags.docker);
-  Try<Nothing> validation = Docker::validate(docker);
-  if (validation.isError()) {
-    return Error(validation.error());
+  Try<Docker> 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<Nothing> DockerContainerizerProcess::recover(
     const Option<SlaveState>& state)
 {
@@ -415,7 +435,6 @@ Future<Nothing> 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<Nothing> 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<Nothing> 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<ContainerID> id = parse(container);
 
@@ -470,7 +488,7 @@ Future<Nothing> 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<bool> 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<bool> 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<bool> 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<string, string> env = executorEnvironment(
       executorInfo,
@@ -546,8 +564,7 @@ Future<bool> DockerContainerizerProcess::launch(
                executorInfo,
                slaveId,
                slavePid,
-               checkpoint,
-               lambda::_1))
+               checkpoint))
     .onFailed(defer(self(), &Self::destroy, containerId, false));
 }
 
@@ -563,12 +580,11 @@ Future<bool> 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<bool> 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<bool> 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<bool> DockerContainerizerProcess::launch(
                 directory,
                 slaveId,
                 slavePid,
-                checkpoint,
-                lambda::_1))
+                checkpoint))
     .onFailed(defer(self(), &Self::destroy, containerId, false));
 }
 
@@ -624,17 +641,8 @@ Future<bool> DockerContainerizerProcess::_launch(
     const string& directory,
     const SlaveID& slaveId,
     const PID<Slave>& slavePid,
-    bool checkpoint,
-    const Option<int>& 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<string, string> env = executorEnvironment(
       executorInfo,
@@ -656,9 +664,8 @@ Future<bool> 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<Subprocess> s = subprocess(
       executorInfo.command().value() + " --override " + override,
@@ -708,9 +715,9 @@ Future<bool> 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<bool> DockerContainerizerProcess::_launch(
     const ExecutorInfo& executorInfo,
     const SlaveID& slaveId,
     const PID<Slave>& slavePid,
-    bool checkpoint,
-    const Option<int>& 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<bool> DockerContainerizerProcess::__launch(
     bool checkpoint,
     const Docker::Container& container)
 {
-  Option<int> pid = container.pid();
+  Option<int> 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<Nothing> 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<Nothing> DockerContainerizerProcess::_update(
   // update the proper cgroup control files.
 
   // First check that this container still appears to be running.
-  Option<pid_t> pid = container.pid();
+  Option<pid_t> pid = container.pid;
   if (pid.isNone()) {
     return Nothing();
   }
@@ -873,10 +871,9 @@ Future<Nothing> 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<Nothing> 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<Nothing> 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<Nothing> 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<Bytes> currentLimit =
@@ -946,6 +941,9 @@ Future<Nothing> 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<Nothing> 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<ResourceStatistics> 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<ResourceStatistics> DockerContainerizerProcess::_usage(
     const ContainerID& containerId,
     const Docker::Container& container)
 {
-  Option<pid_t> pid = container.pid();
+  Option<pid_t> pid = container.pid;
   if (pid.isNone()) {
     return Failure("Container is not running");
   }
@@ -1038,7 +1035,7 @@ Future<containerizer::Termination> 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<Option<int> >& future)
+    bool killed,
+    const Future<Nothing>& 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<Option<int> >& 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<ContainerID> DockerContainerizerProcess::parse(
-    const Docker::Container& container)
-{
-  Option<string> 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<DockerContainerizer*> create(
-      const Flags& flags,
-      bool local);
-
-  static Try<Nothing> prepareCgroups(const Flags& flags);
+  static Try<DockerContainerizer*> 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<std::string> cpuHierarchy = cgroups::cpu::cgroup(pid);
+  EXPECT_FALSE(cpuHierarchy.isError());
+  EXPECT_SOME(cpuHierarchy);
+
+  Result<std::string> 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<Docker::Container>& 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<PID<Slave> > 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<containerizer::Termination> 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<PID<Slave> > 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<PID<Slave> > 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<string>("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<PID<Slave> > 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<PID<Slave> > slave = StartSlave(&dockerContainerizer);
   ASSERT_SOME(slave);
@@ -676,7 +675,7 @@ TEST_F(DockerContainerizerTest, DOCKER_Update)
   ASSERT_SOME(cpuHierarchy);
   ASSERT_SOME(memoryHierarchy);
 
-  Option<pid_t> pid = container.get().pid();
+  Option<pid_t> pid = container.get().pid;
   ASSERT_SOME(pid);
 
   Result<string> 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<Option<int> > d1 =
+  Future<Nothing> d1 =
     docker.run(
         "busybox",
         "sleep 360",
         slave::DOCKER_NAME_PREFIX + stringify(containerId),
         resources);
 
-  Future<Option<int> > d2 =
+  Future<Nothing> 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<Option<int> > status = docker.rm(containerName, true);
-  AWAIT_READY(status);
-  ASSERT_SOME(status.get());
+  // Cleaning up the container first if it exists.
+  Future<Nothing> status = docker.rm(containerName, true);
+  ASSERT_TRUE(status.await(Seconds(10)));
 
   // Verify that we do not see the container.
-  Future<list<Docker::Container> > containers = docker.ps(true);
+  Future<list<Docker::Container> > 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 <process/gmock.hpp>
 #include <process/gtest.hpp>
-#include <process/subprocess.hpp>
 
 #include <stout/check.hpp>
 #include <stout/error.hpp>
@@ -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<Nothing> validate = Docker::validate(docker);
-      if (validate.isError()) {
+      Try<Docker> 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


Mime
View raw message