You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by vi...@apache.org on 2016/09/29 22:35:16 UTC
[1/2] mesos git commit: Fixed logging in composing containerizer.
Repository: mesos
Updated Branches:
refs/heads/master 9dc41c492 -> 700f04002
Fixed logging in composing containerizer.
In addition to removing quotes, removed Container ID in the
returned error messages because the caller knows it already.
Review: https://reviews.apache.org/r/52329
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4dd3bd01
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4dd3bd01
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4dd3bd01
Branch: refs/heads/master
Commit: 4dd3bd01f1a74ead5c9ea9d9357fb5273ef18d8d
Parents: 9dc41c4
Author: Vinod Kone <vi...@gmail.com>
Authored: Tue Sep 27 17:38:45 2016 -0700
Committer: Vinod Kone <vi...@gmail.com>
Committed: Thu Sep 29 13:17:14 2016 -0700
----------------------------------------------------------------------
src/slave/containerizer/composing.cpp | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/4dd3bd01/src/slave/containerizer/composing.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/composing.cpp b/src/slave/containerizer/composing.cpp
index 1031326..7d34d39 100644
--- a/src/slave/containerizer/composing.cpp
+++ b/src/slave/containerizer/composing.cpp
@@ -401,8 +401,7 @@ Future<bool> ComposingContainerizerProcess::launch(
bool checkpoint)
{
if (containers_.contains(containerId)) {
- return Failure("Container '" + stringify(containerId) +
- "' is already launching");
+ return Failure("Duplicate container found");
}
// Try each containerizer. If none of them handle the
@@ -449,7 +448,7 @@ Future<bool> ComposingContainerizerProcess::launch(
if (!containers_.contains(rootContainerId)) {
return Failure(
- "Root container '" + rootContainerId.value() + "' not found");
+ "Root container " + stringify(rootContainerId) + " not found");
}
// Use the containerizer that launched the root container to launch
@@ -507,7 +506,7 @@ Future<Nothing> ComposingContainerizerProcess::update(
const Resources& resources)
{
if (!containers_.contains(containerId)) {
- return Failure("Container '" + containerId.value() + "' not found");
+ return Failure("Container not found");
}
return containers_[containerId]->containerizer->update(
@@ -519,7 +518,7 @@ Future<ResourceStatistics> ComposingContainerizerProcess::usage(
const ContainerID& containerId)
{
if (!containers_.contains(containerId)) {
- return Failure("Container '" + containerId.value() + "' not found");
+ return Failure("Container not found");
}
return containers_[containerId]->containerizer->usage(containerId);
@@ -530,7 +529,7 @@ Future<ContainerStatus> ComposingContainerizerProcess::status(
const ContainerID& containerId)
{
if (!containers_.contains(containerId)) {
- return Failure("Container " + containerId.value() + " not found");
+ return Failure("Container not found");
}
return containers_[containerId]->containerizer->status(containerId);
[2/2] mesos git commit: Fixed the race between launch and destroy in
composing containerizer.
Posted by vi...@apache.org.
Fixed the race between launch and destroy in composing containerizer.
Review: https://reviews.apache.org/r/52330
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/700f0400
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/700f0400
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/700f0400
Branch: refs/heads/master
Commit: 700f04002bc78ec1b8fd813ea307b0f1f95770e9
Parents: 4dd3bd0
Author: Vinod Kone <vi...@gmail.com>
Authored: Tue Sep 27 20:12:53 2016 -0700
Committer: Vinod Kone <vi...@gmail.com>
Committed: Thu Sep 29 13:32:57 2016 -0700
----------------------------------------------------------------------
src/slave/containerizer/composing.cpp | 107 +++++++++----
.../composing_containerizer_tests.cpp | 153 ++++++++++++++++++-
2 files changed, 221 insertions(+), 39 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/700f0400/src/slave/containerizer/composing.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/composing.cpp b/src/slave/containerizer/composing.cpp
index 7d34d39..0c5489f 100644
--- a/src/slave/containerizer/composing.cpp
+++ b/src/slave/containerizer/composing.cpp
@@ -336,33 +336,65 @@ Future<bool> ComposingContainerizerProcess::_launch(
vector<Containerizer*>::iterator containerizer,
bool launched)
{
- // The container struct will not be cleaned up by destroy
- // when the container is in the LAUNCHING state.
- CHECK(containers_.contains(containerId));
- Container* container = containers_.at(containerId);
-
- // A destroy arrived in the interim.
- if (container->state == DESTROYING) {
- container->destroyed.set(true);
- containers_.erase(containerId);
- delete container;
- return Failure("Container was destroyed while launching");
+ if (!containers_.contains(containerId)) {
+ // If we are here a destroy started and finished in the interim.
+ return launched;
}
+ Container* container = containers_.at(containerId);
+
if (launched) {
- container->state = LAUNCHED;
+ // Note that we don't update the state if a destroy is in progress.
+ if (container->state == LAUNCHING) {
+ container->state = LAUNCHED;
+ }
+
+ // Note that the return value is not impacted
+ // by whether a destroy is currently in progress.
return true;
}
+ // If we are here, the launch is not supported by `containerizer`.
+
// Try the next containerizer.
++containerizer;
if (containerizer == containerizers_.end()) {
+ // If we are here none of the containerizers support the launch.
+
+ // We set this to `false` because the container has no chance of
+ // getting launched by any containerizer. This is similar to what
+ // would happen if the destroy "started" after launch returned false.
+ container->destroyed.set(false);
+
+ // We destroy the container irrespective whether
+ // a destroy is already in progress, for simplicity.
containers_.erase(containerId);
delete container;
+
+ // We return false here because none of the
+ // containerizers support the launch.
return false;
}
+ if (container->state == DESTROYING) {
+ // If we are here there is at least one more containerizer that could
+ // potentially launch this container. But since a destroy is in progress
+ // we do not try any other containerizers.
+
+ // We set this to `true` because the destroy-in-progress stopped an
+ // launch-in-progress (using the next containerizer).
+ container->destroyed.set(true);
+
+ containers_.erase(containerId);
+ delete container;
+
+ // We return failure here because there is a chance some other
+ // containerizer might be able to launch this container but
+ // we are not trying it because a destroy is in progress.
+ return Failure("Container was destroyed while launching");
+ }
+
container->containerizer = *containerizer;
return (*containerizer)->launch(
@@ -474,30 +506,41 @@ Future<bool> ComposingContainerizerProcess::launch(
Future<bool> ComposingContainerizerProcess::_launch(
- const ContainerID& containerId,
- bool launched)
+ const ContainerID& containerId,
+ bool launched)
{
- // The container struct will not be cleaned up by destroy
- // when the container is in the LAUNCHING state.
- CHECK(containers_.contains(containerId));
- Container* container = containers_.at(containerId);
-
- // A destroy arrived in the interim.
- if (container->state == DESTROYING) {
- container->destroyed.set(true);
- containers_.erase(containerId);
- delete container;
- return Failure("Container was destroyed while launching");
+ if (!containers_.contains(containerId)) {
+ // If we are here a destroy started and finished in the interim.
+ return launched;
}
+ Container* container = containers_.at(containerId);
+
if (launched) {
- container->state = LAUNCHED;
+ // Note that we don't update the state if a destroy is in progress.
+ if (container->state == LAUNCHING) {
+ container->state = LAUNCHED;
+ }
+
+ // Note that the return value is not impacted
+ // by whether a destroy is currently in progress.
return true;
- } else {
- containers_.erase(containerId);
- delete container;
- return false;
}
+
+ // If we are here, the launch is not supported by the containerizer.
+
+ // We set this to `false` because the container has no chance of
+ // getting launched. This is similar to what would happen if the
+ // destroy "started" after launch returned false.
+ container->destroyed.set(false);
+
+ // We destroy the container irrespective whether
+ // a destroy is already in progress, for simplicity.
+ containers_.erase(containerId);
+ delete container;
+
+ // We return false here because the launch is not supported.
+ return false;
}
@@ -586,10 +629,12 @@ Future<bool> ComposingContainerizerProcess::destroy(
// `Container.destroy` in `_launch()` will be a no-op;
// this might result in users waiting on the future
// incorrectly thinking that the destroy failed when in
- // fact the destory is implicitly successful because the
+ // fact the destroy is implicitly successful because the
// launch failed.
if (containers_.contains(containerId)) {
containers_.at(containerId)->destroyed.associate(destroy);
+ delete containers_.at(containerId);
+ containers_.erase(containerId);
}
}));
http://git-wip-us.apache.org/repos/asf/mesos/blob/700f0400/src/tests/containerizer/composing_containerizer_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/composing_containerizer_tests.cpp b/src/tests/containerizer/composing_containerizer_tests.cpp
index 631328a..934824c 100644
--- a/src/tests/containerizer/composing_containerizer_tests.cpp
+++ b/src/tests/containerizer/composing_containerizer_tests.cpp
@@ -58,9 +58,10 @@ class ComposingContainerizerTest : public MesosTest {};
// launch loop. The composing containerizer still calls the
// underlying containerizer's destroy (because it's not sure
// if the containerizer can handle the type of container being
-// launched). Once the launch is rejected, the composing
-// containerizer should stop the launch loop.
-TEST_F(ComposingContainerizerTest, DestroyDuringLaunchLoop)
+// launched). If the launch is not supported by the 1st containerizer,
+// the composing containerizer should stop the launch loop and
+// set the value of destroy future to true.
+TEST_F(ComposingContainerizerTest, DestroyDuringUnsupportedLaunchLoop)
{
vector<Containerizer*> containerizers;
@@ -84,12 +85,82 @@ TEST_F(ComposingContainerizerTest, DestroyDuringLaunchLoop)
.WillOnce(Return(launchPromise.future()));
Future<Nothing> destroy;
+ Promise<bool> destroyPromise;
+ EXPECT_CALL(*mockContainerizer1, destroy(_))
+ .WillOnce(DoAll(FutureSatisfy(&destroy),
+ Return(destroyPromise.future())));
+
+ Future<bool> launched = containerizer.launch(
+ containerId,
+ taskInfo,
+ executorInfo,
+ "dir",
+ "user",
+ slaveId,
+ environment,
+ false);
+
+ Resources resources = Resources::parse("cpus:1;mem:256").get();
+
+ EXPECT_TRUE(launched.isPending());
+
+ Future<bool> destroyed = containerizer.destroy(containerId);
+
+ EXPECT_CALL(*mockContainerizer2, launch(_, _, _, _, _, _, _, _))
+ .Times(0);
+
+ // We make sure the destroy is being called on the first containerizer.
+ // The second containerizer shouldn't be called as well since the
+ // container is already destroyed.
+ AWAIT_READY(destroy);
+
+ launchPromise.set(false);
+ destroyPromise.set(false);
+
+ // `launched` should be a failure and `destroyed` should be true
+ // because the launch was stopped from being tried on the 2nd
+ // containerizer because of the destroy.
+ AWAIT_FAILED(launched);
+ AWAIT_EXPECT_EQ(true, destroyed);
+}
+
+
+// This test ensures that destroy can be called while in the
+// launch loop. The composing containerizer still calls the
+// underlying containerizer's destroy (because it's not sure
+// if the containerizer can handle the type of container being
+// launched). If the launch is successful the destroy future
+// value depends on the containerizer's destroy.
+TEST_F(ComposingContainerizerTest, DestroyDuringSupportedLaunchLoop)
+{
+ vector<Containerizer*> containerizers;
+
+ MockContainerizer* mockContainerizer1 = new MockContainerizer();
+ MockContainerizer* mockContainerizer2 = new MockContainerizer();
+
+ containerizers.push_back(mockContainerizer1);
+ containerizers.push_back(mockContainerizer2);
+
+ ComposingContainerizer containerizer(containerizers);
+ ContainerID containerId;
+ containerId.set_value("container");
+ TaskInfo taskInfo;
+ ExecutorInfo executorInfo;
+ SlaveID slaveId;
+ std::map<std::string, std::string> environment;
+
+ Promise<bool> launchPromise;
+ EXPECT_CALL(*mockContainerizer1, launch(_, _, _, _, _, _, _, _))
+ .WillOnce(Return(launchPromise.future()));
+
+ Future<Nothing> destroy;
+ Promise<bool> destroyPromise;
EXPECT_CALL(*mockContainerizer1, destroy(_))
.WillOnce(DoAll(FutureSatisfy(&destroy),
- Return(Future<bool>(false))));
+ Return(destroyPromise.future())));
- Future<bool> launch = containerizer.launch(
+ Future<bool> launched = containerizer.launch(
containerId,
taskInfo,
executorInfo,
@@ -101,9 +172,9 @@ TEST_F(ComposingContainerizerTest, DestroyDuringLaunchLoop)
Resources resources = Resources::parse("cpus:1;mem:256").get();
- EXPECT_TRUE(launch.isPending());
+ EXPECT_TRUE(launched.isPending());
- containerizer.destroy(containerId);
+ Future<bool> destroyed = containerizer.destroy(containerId);
EXPECT_CALL(*mockContainerizer2, launch(_, _, _, _, _, _, _, _))
.Times(0);
@@ -113,8 +184,74 @@ TEST_F(ComposingContainerizerTest, DestroyDuringLaunchLoop)
// container is already destroyed.
AWAIT_READY(destroy);
+ launchPromise.set(true);
+ destroyPromise.set(false);
+
+ // `launched` should return true and `destroyed` should return false
+ // because the launch succeeded and `destroyPromise` was set to false.
+ AWAIT_EXPECT_EQ(true, launched);
+ AWAIT_EXPECT_EQ(false, destroyed);
+}
+
+
+// This test ensures that destroy can be called at the end of the
+// launch loop. The composing containerizer still calls the
+// underlying containerizer's destroy (because it's not sure
+// if the containerizer can handle the type of container being
+// launched). If the launch is not supported by any containerizers
+// both the launch and destroy futures should be false.
+TEST_F(ComposingContainerizerTest, DestroyAfterLaunchLoop)
+{
+ vector<Containerizer*> containerizers;
+
+ MockContainerizer* mockContainerizer1 = new MockContainerizer();
+ containerizers.push_back(mockContainerizer1);
+
+ ComposingContainerizer containerizer(containerizers);
+ ContainerID containerId;
+ containerId.set_value("container");
+ TaskInfo taskInfo;
+ ExecutorInfo executorInfo;
+ SlaveID slaveId;
+ std::map<std::string, std::string> environment;
+
+ Promise<bool> launchPromise;
+
+ EXPECT_CALL(*mockContainerizer1, launch(_, _, _, _, _, _, _, _))
+ .WillOnce(Return(launchPromise.future()));
+
+ Future<Nothing> destroy;
+ Promise<bool> destroyPromise;
+ EXPECT_CALL(*mockContainerizer1, destroy(_))
+ .WillOnce(DoAll(FutureSatisfy(&destroy),
+ Return(destroyPromise.future())));
+
+ Future<bool> launched = containerizer.launch(
+ containerId,
+ taskInfo,
+ executorInfo,
+ "dir",
+ "user",
+ slaveId,
+ environment,
+ false);
+
+ Resources resources = Resources::parse("cpus:1;mem:256").get();
+
+ EXPECT_TRUE(launched.isPending());
+
+ Future<bool> destroyed = containerizer.destroy(containerId);
+
+ // We make sure the destroy is being called on the containerizer.
+ AWAIT_READY(destroy);
+
launchPromise.set(false);
- AWAIT_FAILED(launch);
+ destroyPromise.set(false);
+
+ // `launch` should return false and `destroyed` should return false
+ // because none of the containerizers support the launch.
+ AWAIT_EXPECT_EQ(false, launched);
+ AWAIT_EXPECT_EQ(false, destroyed);
}