Fix docker container naming and tests. Review: https://reviews.apache.org/r/29335
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/597b8a25 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/597b8a25 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/597b8a25 Branch: refs/heads/master Commit: 597b8a2544463f368833616c10f70ecb2a846c17 Parents: 280e465 Author: Timothy Chen <[email protected]> Authored: Thu Dec 4 02:15:33 2014 +0000 Committer: Timothy Chen <[email protected]> Committed: Fri May 22 23:13:50 2015 -0700 ---------------------------------------------------------------------- src/slave/containerizer/docker.cpp | 24 ++++---- src/slave/containerizer/docker.hpp | 11 +++- src/tests/docker_containerizer_tests.cpp | 84 +++++++++++++++------------ 3 files changed, 68 insertions(+), 51 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/597b8a25/src/slave/containerizer/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index c996afe..0154c4b 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -75,6 +75,9 @@ using state::RunState; const string DOCKER_NAME_PREFIX = "mesos-"; // Declared in header, see explanation there. +const string DOCKER_NAME_SEPERATOR = "."; + +// Declared in header, see explanation there. const string DOCKER_SYMLINK_DIRECTORY = "docker/links"; // Parse the ContainerID from a Docker container and return None if @@ -92,20 +95,20 @@ Option<ContainerID> parse(const Docker::Container& container) } if (name.isSome()) { - // For Mesos version <= 0.21.0, the docker container name format - // was DOCKER_NAME_PREFIX + containerId, and starting with 0.22.0 - // it is changed to DOCKER_NAME_PREFIX + slaveId + "/" + - // containerId. - // To still be backward compatible during upgrade, we still need - // to load the previous format. + // For Mesos version < 0.23.0, the docker container name format + // was DOCKER_NAME_PREFIX + containerId, and starting with 0.23.0 + // it is changed to DOCKER_NAME_PREFIX + slaveId + + // DOCKER_NAME_SEPERATOR + containerId. + // To be backward compatible during upgrade, we still to support + // the previous format. // TODO(tnachen): Remove this check after deprecation cycle. - if (!strings::contains(name.get(), "/")) { + if (!strings::contains(name.get(), DOCKER_NAME_SEPERATOR)) { ContainerID id; id.set_value(name.get()); return id; } - vector<string> parts = strings::split(name.get(), "/"); + vector<string> parts = strings::split(name.get(), DOCKER_NAME_SEPERATOR); if (parts.size() == 2) { ContainerID id; id.set_value(parts[1]); @@ -537,7 +540,7 @@ Future<Nothing> DockerContainerizerProcess::_recover( // Create and store a container. Container* container = new Container(containerId); containers_[containerId] = container; - + container->slaveId = state.get().id; container->state = Container::RUNNING; pid_t pid = run.get().forkedPid.get(); @@ -1063,9 +1066,6 @@ Future<bool> DockerContainerizerProcess::_______launch( container->status.future().get() .onAny(defer(self(), &Self::reaped, containerId)); - // TODO(benh): Check failure of Docker::logs. - docker->logs(container->name(), container->directory); - return true; } http://git-wip-us.apache.org/repos/asf/mesos/blob/597b8a25/src/slave/containerizer/docker.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.hpp b/src/slave/containerizer/docker.hpp index a54e0d4..16c7775 100644 --- a/src/slave/containerizer/docker.hpp +++ b/src/slave/containerizer/docker.hpp @@ -35,6 +35,10 @@ namespace slave { // created by Mesos from those created manually. extern const std::string DOCKER_NAME_PREFIX; +// Seperator used to compose docker container name, which is made up +// of slave id and container id. +extern const std::string DOCKER_NAME_SEPERATOR; + // Directory that stores all the symlinked sandboxes that is mapped // into Docker containers. This is a relative directory that will // joined with the slave path. Only sandbox paths that contains a @@ -341,17 +345,18 @@ private: std::string name() { - return DOCKER_NAME_PREFIX + slaveId.value() + "/" + stringify(id); + return DOCKER_NAME_PREFIX + slaveId.value() + DOCKER_NAME_SEPERATOR + + stringify(id); } std::string logName() { - return name() + "/log"; + return name() + DOCKER_NAME_SEPERATOR + "log"; } std::string executorName() { - return name() + "/executor"; + return name() + DOCKER_NAME_SEPERATOR + "executor"; } std::string image() const http://git-wip-us.apache.org/repos/asf/mesos/blob/597b8a25/src/tests/docker_containerizer_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/docker_containerizer_tests.cpp b/src/tests/docker_containerizer_tests.cpp index 36ccc6a..3fa663e 100644 --- a/src/tests/docker_containerizer_tests.cpp +++ b/src/tests/docker_containerizer_tests.cpp @@ -119,7 +119,8 @@ public: stop, process::Future<Nothing>( const string&, - const Duration&, bool)); + const Duration&, + bool)); process::Future<Nothing> _run( const mesos::ContainerInfo& containerInfo, @@ -168,11 +169,20 @@ public: class DockerContainerizerTest : public MesosTest { public: + static string containerName( + const SlaveID& slaveId, + const ContainerID& containerId) + { + return slave::DOCKER_NAME_PREFIX + slaveId.value() + + slave::DOCKER_NAME_SEPERATOR + containerId.value(); + } + static bool exists( const list<Docker::Container>& containers, + const SlaveID& slaveId, const ContainerID& containerId) { - string expectedName = slave::DOCKER_NAME_PREFIX + stringify(containerId); + string expectedName = containerName(slaveId, containerId); foreach (const Docker::Container& container, containers) { // Docker inspect name contains an extra slash in the beginning. @@ -213,7 +223,7 @@ public: // Cleanup all mesos launched containers. foreach (const Docker::Container& container, containers.get()) { - AWAIT_READY_FOR(docker.get()->rm(container.id, true), Seconds(30)); + ASSERT_TRUE(docker.get()->rm(container.id, true).await(Seconds(30))); } delete docker.get(); @@ -435,6 +445,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Launch_Executor) const Offer& offer = offers.get()[0]; + SlaveID slaveId = offer.slave_id(); + TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); @@ -489,7 +501,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Launch_Executor) AWAIT_READY(containers); - ASSERT_TRUE(exists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), slaveId, containerId.get())); Future<containerizer::Termination> termination = dockerContainerizer.wait(containerId.get()); @@ -565,6 +577,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Launch_Executor_Bridged) const Offer& offer = offers.get()[0]; + SlaveID slaveId = offer.slave_id(); + TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); @@ -620,7 +634,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Launch_Executor_Bridged) AWAIT_READY(containers); - ASSERT_TRUE(exists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), slaveId, containerId.get())); Future<containerizer::Termination> termination = dockerContainerizer.wait(containerId.get()); @@ -691,6 +705,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Launch) const Offer& offer = offers.get()[0]; + SlaveID slaveId = offer.slave_id(); + TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); @@ -737,7 +753,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Launch) ASSERT_TRUE(containers.get().size() > 0); - ASSERT_TRUE(exists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), slaveId, containerId.get())); Future<containerizer::Termination> termination = dockerContainerizer.wait(containerId.get()); @@ -1067,6 +1083,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Update) const Offer& offer = offers.get()[0]; + SlaveID slaveId = offer.slave_id(); + TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); @@ -1107,8 +1125,9 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Update) AWAIT_READY_FOR(statusRunning, Seconds(60)); EXPECT_EQ(TASK_RUNNING, statusRunning.get().state()); - string containerName = slave::DOCKER_NAME_PREFIX + containerId.get().value(); - Future<Docker::Container> container = docker->inspect(containerName); + string name = containerName(slaveId, containerId.get()); + + Future<Docker::Container> container = docker->inspect(name); AWAIT_READY(container); @@ -1193,11 +1212,20 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) MockDockerContainerizer dockerContainerizer(flags, &fetcher, docker); + SlaveID slaveId; + slaveId.set_value("s1"); ContainerID containerId; containerId.set_value("c1"); ContainerID reapedContainerId; reapedContainerId.set_value("c2"); + string container1 = containerName(slaveId, containerId); + string container2 = containerName(slaveId, reapedContainerId); + + // Clean up artifacts if containers still exists. + ASSERT_TRUE(docker->rm(container1, true).await(Seconds(30))); + ASSERT_TRUE(docker->rm(container2, true).await(Seconds(30))); + Resources resources = Resources::parse("cpus:1;mem:512").get(); ContainerInfo containerInfo; @@ -1214,7 +1242,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) docker->run( containerInfo, commandInfo, - slave::DOCKER_NAME_PREFIX + stringify(containerId), + container1, flags.work_dir, flags.docker_sandbox_directory, resources); @@ -1223,7 +1251,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) docker->run( containerInfo, commandInfo, - slave::DOCKER_NAME_PREFIX + stringify(reapedContainerId), + container2, flags.work_dir, flags.docker_sandbox_directory, resources); @@ -1232,6 +1260,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) AWAIT_READY(d2); SlaveState slaveState; + slaveState.id = slaveId; FrameworkState frameworkState; ExecutorID execId; @@ -1243,18 +1272,12 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) execState.latest = containerId; Try<process::Subprocess> wait = - process::subprocess( - tests::flags.docker + " wait " + - slave::DOCKER_NAME_PREFIX + - stringify(containerId)); + process::subprocess(tests::flags.docker + " wait " + container1); ASSERT_SOME(wait); Try<process::Subprocess> reaped = - process::subprocess( - tests::flags.docker + " wait " + - slave::DOCKER_NAME_PREFIX + - stringify(reapedContainerId)); + process::subprocess(tests::flags.docker + " wait " + container2); ASSERT_SOME(reaped); @@ -1268,15 +1291,6 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) slaveState.frameworks.put(frameworkId, frameworkState); - // We need to capture and await on the stop future so that we can - // ensure there is no child process at the end of the test. - // The stop future is being awaited at teardown. - Future<Nothing> stop; - EXPECT_CALL(*mockDocker, stop(_, _, _)) - .WillOnce(FutureResult( - &stop, - Invoke((MockDocker*) docker.get(), &MockDocker::_stop))); - Future<Nothing> recover = dockerContainerizer.recover(slaveState); AWAIT_READY(recover); @@ -1288,14 +1302,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover) AWAIT_FAILED(dockerContainerizer.wait(reapedContainerId)); - dockerContainerizer.destroy(containerId); - - AWAIT_READY(termination); - AWAIT_READY(reaped.get().status()); - AWAIT_READY_FOR(stop, Seconds(30)); - Shutdown(); } @@ -1935,6 +1943,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_SlaveRecoveryTaskContainer) const Offer& offer = offers.get()[0]; + SlaveID slaveId = offer.slave_id(); + TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); @@ -2015,7 +2025,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_SlaveRecoveryTaskContainer) AWAIT_READY(containers); - ASSERT_TRUE(exists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), slaveId, containerId.get())); Future<containerizer::Termination> termination = dockerContainerizer2->wait(containerId.get()); @@ -2208,7 +2218,7 @@ TEST_F(DockerContainerizerTest, AWAIT_READY(containers); - ASSERT_TRUE(exists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), slaveId.get(), containerId.get())); driver.stop(); driver.join(); @@ -2417,6 +2427,8 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_LaunchSandboxWithColon) const Offer& offer = offers.get()[0]; + SlaveID slaveId = offer.slave_id(); + TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("test:colon"); @@ -2463,7 +2475,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_LaunchSandboxWithColon) ASSERT_TRUE(containers.get().size() > 0); - ASSERT_TRUE(exists(containers.get(), containerId.get())); + ASSERT_TRUE(exists(containers.get(), slaveId, containerId.get())); Future<containerizer::Termination> termination = dockerContainerizer.wait(containerId.get());
