You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2017/04/11 23:31:12 UTC
[1/4] mesos git commit: Added a test to verify persistent volume
mount points removal.
Repository: mesos
Updated Branches:
refs/heads/master 8e1596396 -> 559066638
Added a test to verify persistent volume mount points removal.
This test is used to catch regression related to MESOS-7366.
Review: https://reviews.apache.org/r/58280
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/55906663
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/55906663
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/55906663
Branch: refs/heads/master
Commit: 5590666384bcaab457a4d727de9a39818b84e2a3
Parents: cf7d1ae
Author: Jie Yu <yu...@gmail.com>
Authored: Fri Apr 7 16:41:45 2017 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue Apr 11 16:31:06 2017 -0700
----------------------------------------------------------------------
.../linux_filesystem_isolator_tests.cpp | 77 ++++++++++++++++++++
1 file changed, 77 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/55906663/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/linux_filesystem_isolator_tests.cpp b/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
index 5e489ef..70a0dce 100644
--- a/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
+++ b/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
@@ -811,6 +811,83 @@ TEST_F(LinuxFilesystemIsolatorTest, ROOT_WorkDirMountNeeded)
}
+// This test tries to catch the regression for MESOS-7366. It verifies
+// that the persistent volume mount points in the sandbox will be
+// cleaned up even if there is still reference to the volume.
+TEST_F(LinuxFilesystemIsolatorTest, ROOT_PersistentVolumeMountPointCleanup)
+{
+ slave::Flags flags = CreateSlaveFlags();
+ flags.isolation = "filesystem/linux";
+
+ Try<MesosContainerizer*> create =
+ MesosContainerizer::create(flags, true, &fetcher);
+
+ ASSERT_SOME(create);
+
+ Owned<Containerizer> containerizer(create.get());
+
+ ContainerID containerId;
+ containerId.set_value(UUID::random().toString());
+
+ ExecutorInfo executor = createExecutorInfo(
+ "test_executor",
+ "sleep 1000");
+
+ // Create a persistent volume.
+ executor.add_resources()->CopyFrom(createPersistentVolume(
+ Megabytes(32),
+ "test_role",
+ "persistent_volume_id",
+ "volume"));
+
+ string volume = slave::paths::getPersistentVolumePath(
+ flags.work_dir,
+ "test_role",
+ "persistent_volume_id");
+
+ ASSERT_SOME(os::mkdir(volume));
+
+ string directory = path::join(flags.work_dir, "sandbox");
+ ASSERT_SOME(os::mkdir(directory));
+
+ Future<bool> launch = containerizer->launch(
+ containerId,
+ None(),
+ executor,
+ directory,
+ None(),
+ SlaveID(),
+ map<string, string>(),
+ false);
+
+ AWAIT_READY(launch);
+
+ ASSERT_SOME(os::touch(path::join(directory, "volume", "abc")));
+
+ // This keeps a reference to the persistent volume mount.
+ Try<int_fd> fd = os::open(
+ path::join(directory, "volume", "abc"),
+ O_WRONLY | O_TRUNC | O_CLOEXEC,
+ S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+
+ ASSERT_SOME(fd);
+
+ containerizer->destroy(containerId);
+
+ Future<Option<ContainerTermination>> wait = containerizer->wait(containerId);
+
+ AWAIT_READY(wait);
+ ASSERT_SOME(wait.get());
+ ASSERT_TRUE(wait->get().has_status());
+ EXPECT_WTERMSIG_EQ(SIGKILL, wait.get()->status());
+
+ // Verifies that mount point has been removed.
+ EXPECT_FALSE(os::exists(path::join(directory, "volume", "abc")));
+
+ os::close(fd.get());
+}
+
+
// End to end Mesos integration tests for linux filesystem isolator.
class LinuxFilesystemIsolatorMesosTest : public LinuxFilesystemIsolatorTest {};
[2/4] mesos git commit: Lazily unmount persistent volumes in
MesosContainerizer.
Posted by ji...@apache.org.
Lazily unmount persistent volumes in MesosContainerizer.
Use MNT_DETACH when unmounting persistent volumes in Linux filesystem
isolator to workaround an issue of incorrect handling of container
destroy failures. Currently, if isolator cleanup returns a failure,
the slave will treat the container as terminated, and will schedule
the cleanup of the container's sandbox. Since the mount hasn't been
removed in the sandbox (e.g., due to EBUSY), that'll result in data in
the persistent volume being incorrectly deleted. Use MNT_DETACH so
that the mount point in the sandbox will be removed immediately. See
MESOS-7366 for more details.
Review: https://reviews.apache.org/r/58278
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/f96f5b6b
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/f96f5b6b
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/f96f5b6b
Branch: refs/heads/master
Commit: f96f5b6b25a444309fe021fa229b3b8286093b5e
Parents: 76d42c3
Author: Jie Yu <yu...@gmail.com>
Authored: Fri Apr 7 16:33:53 2017 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue Apr 11 16:31:06 2017 -0700
----------------------------------------------------------------------
.../mesos/isolators/filesystem/linux.cpp | 22 ++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/f96f5b6b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
index ae0031d..69804ee 100644
--- a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
+++ b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
@@ -818,6 +818,8 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::cleanup(
return Failure("Failed to get mount table: " + table.error());
}
+ vector<string> unmountErrors;
+
// Reverse unmount order to handle nested mount points.
foreach (const fs::MountInfoTable::Entry& entry,
adaptor::reverse(table->entries)) {
@@ -828,15 +830,31 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::cleanup(
LOG(INFO) << "Unmounting volume '" << entry.target
<< "' for container " << containerId;
- Try<Nothing> unmount = fs::unmount(entry.target);
+ // TODO(jieyu): Use MNT_DETACH here to workaround an issue of
+ // incorrect handling of container destroy failures. Currently,
+ // if isolator cleanup returns a failure, the slave will treat
+ // the container as terminated, and will schedule the cleanup of
+ // the container's sandbox. Since the mount hasn't been removed
+ // in the sandbox, that'll result in data in the persistent
+ // volume being incorrectly deleted. Use MNT_DETACH here so that
+ // the mount point in the sandbox will be removed immediately.
+ // See MESOS-7366 for more details.
+ Try<Nothing> unmount = fs::unmount(entry.target, MNT_DETACH);
if (unmount.isError()) {
- return Failure(
+ // NOTE: Instead of short circuit, we try to perform as many
+ // unmount as possible. We'll accumulate the errors together
+ // in the end.
+ unmountErrors.push_back(
"Failed to unmount volume '" + entry.target +
"': " + unmount.error());
}
}
}
+ if (!unmountErrors.empty()) {
+ return Failure(strings::join(", ", unmountErrors));
+ }
+
return Nothing();
}
Re: [4/4] mesos git commit: Removed unnecesary break statements in
local approver.
Posted by Jie Yu <yu...@gmail.com>.
oops. I think I might break something. I don't know why this happened :(
Let me try to fix it.
On Tue, Apr 11, 2017 at 4:31 PM, <ji...@apache.org> wrote:
> Removed unnecesary break statements in local approver.
>
> Removes `break` statements located in lines following a `return`
> statement since they are effectively unreachable code, don't improve
> readability nor make the code cleaner.
>
> Review: https://reviews.apache.org/r/58292/
>
>
> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/76d42c3b
> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/76d42c3b
> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/76d42c3b
>
> Branch: refs/heads/master
> Commit: 76d42c3bb8404b68656dfff4ed31cff9750f3e65
> Parents: 8e15963
> Author: Alexander Rojas <al...@mesosphere.io>
> Authored: Mon Apr 10 18:03:10 2017 -0700
> Committer: Jie Yu <yu...@gmail.com>
> Committed: Tue Apr 11 16:31:06 2017 -0700
>
> ----------------------------------------------------------------------
> src/common/validation.cpp | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/76d42c3b/
> src/common/validation.cpp
> ----------------------------------------------------------------------
> diff --git a/src/common/validation.cpp b/src/common/validation.cpp
> index 544d3a0..9c4f1de 100644
> --- a/src/common/validation.cpp
> +++ b/src/common/validation.cpp
> @@ -16,6 +16,8 @@
>
> #include "common/validation.hpp"
>
> +#include <limits.h>
> +
> #include <algorithm>
> #include <cctype>
>
> @@ -37,13 +39,19 @@ Option<Error> validateID(const string& id)
> return Error("ID must not be empty");
> }
>
> + if (id.length() > NAME_MAX) {
> + return Error(
> + "ID must not be greater than " +
> + stringify(NAME_MAX) + " characters");
> + }
> +
> // The ID cannot be exactly these special path components.
> if (id == "." || id == "..") {
> return Error("'" + id + "' is disallowed");
> }
>
> // Rules on invalid characters in the ID:
> - // - Control charaters are obviously not allowed.
> + // - Control characters are obviously not allowed.
> // - Slashes are disallowed as IDs are likely mapped to directories in
> Mesos.
> auto invalidCharacter = [](char c) {
> return iscntrl(c) ||
>
>
[4/4] mesos git commit: Removed unnecesary break statements in local
approver.
Posted by ji...@apache.org.
Removed unnecesary break statements in local approver.
Removes `break` statements located in lines following a `return`
statement since they are effectively unreachable code, don't improve
readability nor make the code cleaner.
Review: https://reviews.apache.org/r/58292/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/76d42c3b
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/76d42c3b
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/76d42c3b
Branch: refs/heads/master
Commit: 76d42c3bb8404b68656dfff4ed31cff9750f3e65
Parents: 8e15963
Author: Alexander Rojas <al...@mesosphere.io>
Authored: Mon Apr 10 18:03:10 2017 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue Apr 11 16:31:06 2017 -0700
----------------------------------------------------------------------
src/common/validation.cpp | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/76d42c3b/src/common/validation.cpp
----------------------------------------------------------------------
diff --git a/src/common/validation.cpp b/src/common/validation.cpp
index 544d3a0..9c4f1de 100644
--- a/src/common/validation.cpp
+++ b/src/common/validation.cpp
@@ -16,6 +16,8 @@
#include "common/validation.hpp"
+#include <limits.h>
+
#include <algorithm>
#include <cctype>
@@ -37,13 +39,19 @@ Option<Error> validateID(const string& id)
return Error("ID must not be empty");
}
+ if (id.length() > NAME_MAX) {
+ return Error(
+ "ID must not be greater than " +
+ stringify(NAME_MAX) + " characters");
+ }
+
// The ID cannot be exactly these special path components.
if (id == "." || id == "..") {
return Error("'" + id + "' is disallowed");
}
// Rules on invalid characters in the ID:
- // - Control charaters are obviously not allowed.
+ // - Control characters are obviously not allowed.
// - Slashes are disallowed as IDs are likely mapped to directories in Mesos.
auto invalidCharacter = [](char c) {
return iscntrl(c) ||
[3/4] mesos git commit: Lazily unmount persistent volumes in
DockerContainerizer.
Posted by ji...@apache.org.
Lazily unmount persistent volumes in DockerContainerizer.
Use MNT_DETACH to unmount persistent volumes in DockerContainerizer to
workaround an issue of incorrect handling of container destroy
failures. Currently, if unmount fails there, the containerizer will
still treat the container as terminated, and the agent will schedule
the cleanup of the container's sandbox. Since the mount hasn't been
removed in the sandbox (e.g., due to EBUSY), that'll result in data in
the persistent volume being incorrectly deleted. Use MNT_DETACH so
that the mount point in the sandbox will be removed immediately. See
MESOS-7366 for more details.
Review: https://reviews.apache.org/r/58279
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/cf7d1ae9
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/cf7d1ae9
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/cf7d1ae9
Branch: refs/heads/master
Commit: cf7d1ae9849d654e7e7eafefdff6824146a102b4
Parents: f96f5b6
Author: Jie Yu <yu...@gmail.com>
Authored: Fri Apr 7 16:38:00 2017 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue Apr 11 16:31:06 2017 -0700
----------------------------------------------------------------------
src/slave/containerizer/docker.cpp | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/cf7d1ae9/src/slave/containerizer/docker.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp
index 06751f1..ef04a21 100644
--- a/src/slave/containerizer/docker.cpp
+++ b/src/slave/containerizer/docker.cpp
@@ -645,6 +645,8 @@ Try<Nothing> DockerContainerizerProcess::unmountPersistentVolumes(
return Error("Failed to get mount table: " + table.error());
}
+ vector<string> unmountErrors;
+
foreach (const fs::MountInfoTable::Entry& entry,
adaptor::reverse(table.get().entries)) {
// TODO(tnachen): We assume there is only one docker container
@@ -666,13 +668,30 @@ Try<Nothing> DockerContainerizerProcess::unmountPersistentVolumes(
strings::contains(entry.target, containerId.value())) {
LOG(INFO) << "Unmounting volume for container '" << containerId << "'";
- Try<Nothing> unmount = fs::unmount(entry.target);
+ // TODO(jieyu): Use MNT_DETACH here to workaround an issue of
+ // incorrect handling of container destroy failures. Currently,
+ // if unmount fails there, the containerizer will still treat
+ // the container as terminated, and the agent will schedule the
+ // cleanup of the container's sandbox. Since the mount hasn't
+ // been removed in the sandbox, that'll result in data in the
+ // persistent volume being incorrectly deleted. Use MNT_DETACH
+ // here so that the mount point in the sandbox will be removed
+ // immediately. See MESOS-7366 for more details.
+ Try<Nothing> unmount = fs::unmount(entry.target, MNT_DETACH);
if (unmount.isError()) {
- return Error("Failed to unmount volume '" + entry.target +
- "': " + unmount.error());
+ // NOTE: Instead of short circuit, we try to perform as many
+ // unmount as possible. We'll accumulate the errors together
+ // in the end.
+ unmountErrors.push_back(
+ "Failed to unmount volume '" + entry.target +
+ "': " + unmount.error());
}
}
}
+
+ if (!unmountErrors.empty()) {
+ return Error(strings::join(", ", unmountErrors));
+ }
#endif // __linux__
return Nothing();
}