[5/5] git commit: Save Docker container pid for subsequent containerizer updates.

[be...@apache.org]

Save Docker container pid for subsequent containerizer updates.

Review: https://reviews.apache.org/r/24768


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8cbb85c8
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8cbb85c8
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8cbb85c8

Branch: refs/heads/master
Commit: 8cbb85c8af6eae9453a868f67f2fb8dd387e18ba
Parents: a7b706a
Author: Timothy Chen <tn...@apache.org>
Authored: Sat Aug 16 07:53:55 2014 -0700
Committer: Benjamin Hindman <be...@gmail.com>
Committed: Sat Aug 16 08:25:40 2014 -0700

----------------------------------------------------------------------
 src/slave/containerizer/docker.cpp       | 55 ++++++++++++++++++++++-----
 src/tests/docker_containerizer_tests.cpp | 20 ++++++++++
 2 files changed, 66 insertions(+), 9 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/8cbb85c8/src/slave/containerizer/docker.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp
index 5fa0275..215c2b4 100644
--- a/src/slave/containerizer/docker.cpp
+++ b/src/slave/containerizer/docker.cpp
@@ -227,6 +227,11 @@ private:
       const Resources& resources,
       const Docker::Container& container);
 
+  process::Future<Nothing> __update(
+      const ContainerID& containerId,
+      const Resources& resources,
+      pid_t pid);
+
   Future<ResourceStatistics> _usage(
     const ContainerID& containerId,
     const Docker::Container& container);
@@ -301,6 +306,10 @@ private:
     // The docker pull subprocess is stored so we can killtree the
     // pid when destroy is called while docker is pulling the image.
     Option<Subprocess> pull;
+
+    // Once the container is running, this saves the pid of the
+    // running container.
+    Option<pid_t> pid;
   };
 
   hashmap<ContainerID, Container*> containers_;
@@ -1237,8 +1246,16 @@ Future<Nothing> DockerContainerizerProcess::update(
     return Nothing();
   }
 
+  Container* container = containers_[containerId];
+
+  if (container->state == Container::DESTROYING)  {
+    LOG(INFO) << "Ignoring updating container '" << containerId
+              << "' that is being destroyed";
+    return Nothing();
+  }
+
   // Store the resources for usage().
-  containers_[containerId]->resources = _resources;
+  container->resources = _resources;
 
 #ifdef __linux__
   if (!_resources.cpus().isSome() && !_resources.mem().isSome()) {
@@ -1246,6 +1263,11 @@ Future<Nothing> DockerContainerizerProcess::update(
     return Nothing();
   }
 
+  // Skip inspecting the docker container if we already have the pid.
+  if (container->pid.isSome()) {
+    return __update(containerId, _resources, container->pid.get());
+  }
+
   return docker.inspect(containerName(containerId))
     .then(defer(self(), &Self::_update, containerId, _resources, lambda::_1));
 #else
@@ -1259,6 +1281,27 @@ Future<Nothing> DockerContainerizerProcess::_update(
     const Resources& _resources,
     const Docker::Container& container)
 {
+  if (container.pid.isNone()) {
+    return Nothing();
+  }
+
+  if (!containers_.contains(containerId)) {
+    LOG(INFO) << "Container has been removed after docker inspect, "
+              << "skipping update";
+    return Nothing();
+  }
+
+  containers_[containerId]->pid = container.pid.get();
+
+  return __update(containerId, _resources, container.pid.get());
+}
+
+
+Future<Nothing> DockerContainerizerProcess::__update(
+    const ContainerID& containerId,
+    const Resources& _resources,
+    pid_t pid)
+{
 #ifdef __linux__
   // Determine the the cgroups hierarchies where the 'cpu' and
   // 'memory' subsystems are mounted (they may be the same). Note that
@@ -1284,15 +1327,9 @@ Future<Nothing> DockerContainerizerProcess::_update(
   // the hierarchy with the 'memory' subsystem attached so we can
   // update the proper cgroup control files.
 
-  // First check that this container still appears to be running.
-  Option<pid_t> pid = container.pid;
-  if (pid.isNone()) {
-    return Nothing();
-  }
-
   // Determine the cgroup for the 'cpu' subsystem (based on the
   // container's pid).
-  Result<string> cpuCgroup = cgroups::cpu::cgroup(pid.get());
+  Result<string> cpuCgroup = cgroups::cpu::cgroup(pid);
 
   if (cpuCgroup.isError()) {
     return Failure("Failed to determine cgroup for the 'cpu' subsystem: " +
@@ -1325,7 +1362,7 @@ Future<Nothing> DockerContainerizerProcess::_update(
   }
 
   // Now determine the cgroup for the 'memory' subsystem.
-  Result<string> memoryCgroup = cgroups::memory::cgroup(pid.get());
+  Result<string> memoryCgroup = cgroups::memory::cgroup(pid);
 
   if (memoryCgroup.isError()) {
     return Failure("Failed to determine cgroup for the 'memory' subsystem: " +

http://git-wip-us.apache.org/repos/asf/mesos/blob/8cbb85c8/src/tests/docker_containerizer_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/docker_containerizer_tests.cpp b/src/tests/docker_containerizer_tests.cpp
index 3a55f5e..c37bc52 100644
--- a/src/tests/docker_containerizer_tests.cpp
+++ b/src/tests/docker_containerizer_tests.cpp
@@ -734,6 +734,26 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Update)
   EXPECT_EQ(1024u, cpu.get());
   EXPECT_EQ(128u, mem.get().megabytes());
 
+  newResources = Resources::parse("cpus:1;mem:144");
+
+  // Issue second update that uses the cached pid instead of inspect.
+  update = dockerContainerizer.update(containerId.get(), newResources.get());
+
+  AWAIT_READY(update);
+
+  cpu = cgroups::cpu::shares(cpuHierarchy.get(), cpuCgroup.get());
+
+  ASSERT_SOME(cpu);
+
+  mem = cgroups::memory::soft_limit_in_bytes(
+      memoryHierarchy.get(),
+      memoryCgroup.get());
+
+  ASSERT_SOME(mem);
+
+  EXPECT_EQ(1024u, cpu.get());
+  EXPECT_EQ(144u, mem.get().megabytes());
+
   driver.stop();
   driver.join();