You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ch...@apache.org on 2018/07/02 21:26:50 UTC
[1/3] mesos git commit: Added MESOS-8830 to the 1.4.2 CHANGELOG.
Repository: mesos
Updated Branches:
refs/heads/1.4.x c35e06b58 -> e7b117e0d
Added MESOS-8830 to the 1.4.2 CHANGELOG.
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/e7b117e0
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/e7b117e0
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/e7b117e0
Branch: refs/heads/1.4.x
Commit: e7b117e0d9072a50756dd90e12d691af572aed7c
Parents: 7af1fc0
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
Authored: Fri Jun 29 12:11:31 2018 -0700
Committer: Chun-Hung Hsiao <ch...@mesosphere.io>
Committed: Mon Jul 2 14:26:38 2018 -0700
----------------------------------------------------------------------
CHANGELOG | 1 +
1 file changed, 1 insertion(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/e7b117e0/CHANGELOG
----------------------------------------------------------------------
diff --git a/CHANGELOG b/CHANGELOG
index c3a4231..25f87cd 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -30,6 +30,7 @@ Release Notes - Mesos - Version 1.4.2 (WIP)
* [MESOS-8605] - Terminal task status update will not send if 'docker inspect' is hung.
* [MESOS-8651] - Potential memory leaks in the `volume/sandbox_path` isolator.
* [MESOS-8786] - CgroupIsolatorProcess accesses subsystem processes directly.
+ * [MESOS-8830] - Agent gc on old slave sandboxes could empty persistent volume data
* [MESOS-8876] - Normal exit of Docker container using rexray volume results in TASK_FAILED.
* [MESOS-8881] - Enable epoll backend in libevent integration.
* [MESOS-8885] - Disable libevent debug mode.
[3/3] mesos git commit: Unmounted any mount points in gc paths.
Posted by ch...@apache.org.
Unmounted any mount points in gc paths.
In various corner cases, agent may not get chance to properly unmount
persistent volumes mounted inside an executor's sandbox. When GC later
gets to these sandbox directories, permanent data loss can happen (see
MESOS-8830).
Currently, the only mounts in the host mount namespace under the sandbox
directories are persistent volumes, so this diff added protection to
unmount any dangling mount points before calling `rmdir` on the
directory.
NOTE: this means agent will not garbage collect any path if it cannot
read its own `mountinfo` table.
Review: https://reviews.apache.org/r/67264
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/809dd0ff
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/809dd0ff
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/809dd0ff
Branch: refs/heads/1.4.x
Commit: 809dd0ff12d8f65244eafc9362b79671ea35c387
Parents: c35e06b
Author: Zhitao Li <zh...@gmail.com>
Authored: Tue May 22 10:55:50 2018 -0700
Committer: Chun-Hung Hsiao <ch...@mesosphere.io>
Committed: Mon Jul 2 14:26:38 2018 -0700
----------------------------------------------------------------------
src/local/local.cpp | 2 +-
src/slave/gc.cpp | 72 ++++++++++++++++++++++++++++++++++++++-----
src/slave/gc.hpp | 9 ++++--
src/slave/main.cpp | 2 +-
src/tests/cluster.cpp | 2 +-
src/tests/gc_tests.cpp | 6 ++--
src/tests/mock_slave.cpp | 22 ++-----------
src/tests/mock_slave.hpp | 20 +-----------
8 files changed, 80 insertions(+), 55 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/local/local.cpp
----------------------------------------------------------------------
diff --git a/src/local/local.cpp b/src/local/local.cpp
index 99c1b08..3e671f6 100644
--- a/src/local/local.cpp
+++ b/src/local/local.cpp
@@ -407,7 +407,7 @@ PID<Master> launch(const Flags& flags, Allocator* _allocator)
<< slaveFlags.runtime_dir << "': " << mkdir.error();
}
- garbageCollectors->push_back(new GarbageCollector());
+ garbageCollectors->push_back(new GarbageCollector(slaveFlags.work_dir));
statusUpdateManagers->push_back(new StatusUpdateManager(slaveFlags));
fetchers->push_back(new Fetcher(slaveFlags));
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/slave/gc.cpp
----------------------------------------------------------------------
diff --git a/src/slave/gc.cpp b/src/slave/gc.cpp
index 86d94c8..415fe27 100644
--- a/src/slave/gc.cpp
+++ b/src/slave/gc.cpp
@@ -21,6 +21,7 @@
#include <process/delay.hpp>
#include <process/dispatch.hpp>
+#include <stout/adaptor.hpp>
#include <stout/foreach.hpp>
#include <stout/lambda.hpp>
@@ -28,6 +29,10 @@
#include "logging/logging.hpp"
+#ifdef __linux__
+#include "linux/fs.hpp"
+#endif
+
#include "slave/gc.hpp"
using namespace process;
@@ -159,7 +164,62 @@ void GarbageCollectorProcess::remove(const Timeout& removalTime)
info->removing = true;
}
- auto rmdirs = [infos]() {
+ const string _workDir = workDir;
+
+ auto rmdirs = [_workDir, infos]() mutable -> Future<Nothing> {
+#ifdef __linux__
+ // Clear any possible persistent volume mount points in `infos`. See
+ // MESOS-8830.
+ Try<fs::MountInfoTable> mountTable = fs::MountInfoTable::read();
+ if (mountTable.isError()) {
+ LOG(ERROR) << "Skipping any path deletion because of failure on read "
+ "MountInfoTable for agent process: "
+ << mountTable.error();
+
+ foreach (const Owned<PathInfo>& info, infos) {
+ info->promise.fail(mountTable.error());
+ }
+
+ return Failure(mountTable.error());
+ }
+
+ foreach (const fs::MountInfoTable::Entry& entry,
+ adaptor::reverse(mountTable->entries)) {
+ // Ignore mounts whose targets are not under `workDir`.
+ if (!strings::startsWith(
+ path::join(entry.target, ""),
+ path::join(_workDir, ""))) {
+ continue;
+ }
+
+ for (auto it = infos.begin(); it != infos.end(); ) {
+ const Owned<PathInfo>& info = *it;
+ // TODO(zhitao): Validate that both `info->path` and `workDir` are
+ // real paths.
+ if (strings::startsWith(
+ path::join(entry.target, ""), path::join(info->path, ""))) {
+ LOG(WARNING)
+ << "Unmounting dangling mount point '" << entry.target
+ << "' of persistent volume '" << entry.root
+ << "' inside garbage collected path '" << info->path << "'";
+
+ Try<Nothing> unmount = fs::unmount(entry.target);
+ if (unmount.isError()) {
+ LOG(WARNING) << "Skipping deletion of '"
+ << info->path << "' because unmount failed on '"
+ << entry.target << "': " << unmount.error();
+
+ info->promise.fail(unmount.error());
+ it = infos.erase(it);
+ continue;
+ }
+ }
+
+ it++;
+ }
+ }
+#endif // __linux__
+
foreach (const Owned<PathInfo>& info, infos) {
// Run the removal operation with 'continueOnError = true'.
// It's possible for tasks and isolators to lay down files
@@ -198,11 +258,9 @@ void GarbageCollectorProcess::remove(const Timeout& removalTime)
}
-void GarbageCollectorProcess::_remove(const Future<Nothing>& result,
- const list<Owned<PathInfo>> infos)
+void GarbageCollectorProcess::_remove(const Future<Nothing>& result,
+ const list<Owned<PathInfo>> infos)
{
- CHECK_READY(result);
-
// Remove path records from `paths` and `timeouts` data structures.
foreach (const Owned<PathInfo>& info, infos) {
CHECK(paths.remove(timeouts[info->path], info));
@@ -225,9 +283,9 @@ void GarbageCollectorProcess::prune(const Duration& d)
}
-GarbageCollector::GarbageCollector()
+GarbageCollector::GarbageCollector(const string& workDir)
{
- process = new GarbageCollectorProcess();
+ process = new GarbageCollectorProcess(workDir);
spawn(process);
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/slave/gc.hpp
----------------------------------------------------------------------
diff --git a/src/slave/gc.hpp b/src/slave/gc.hpp
index f23ffd3..518fb50 100644
--- a/src/slave/gc.hpp
+++ b/src/slave/gc.hpp
@@ -52,7 +52,7 @@ class GarbageCollectorProcess;
class GarbageCollector
{
public:
- GarbageCollector();
+ explicit GarbageCollector(const std::string& workDir);
virtual ~GarbageCollector();
// Schedules the specified path for removal after the specified
@@ -90,8 +90,9 @@ class GarbageCollectorProcess :
public process::Process<GarbageCollectorProcess>
{
public:
- GarbageCollectorProcess()
- : ProcessBase(process::ID::generate("agent-garbage-collector")) {}
+ explicit GarbageCollectorProcess(const std::string& _workDir)
+ : ProcessBase(process::ID::generate("agent-garbage-collector")),
+ workDir(_workDir) {}
virtual ~GarbageCollectorProcess();
@@ -131,6 +132,8 @@ private:
const process::Future<Nothing>& result,
const std::list<process::Owned<PathInfo>> infos);
+ const std::string workDir;
+
// Store all the timeouts and corresponding paths to delete.
// NOTE: We are using Multimap here instead of Multihashmap, because
// we need the keys of the map (deletion time) to be sorted.
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/slave/main.cpp
----------------------------------------------------------------------
diff --git a/src/slave/main.cpp b/src/slave/main.cpp
index a4a8ced..1892000 100644
--- a/src/slave/main.cpp
+++ b/src/slave/main.cpp
@@ -535,7 +535,7 @@ int main(int argc, char** argv)
}
Files* files = new Files(READONLY_HTTP_AUTHENTICATION_REALM, authorizer_);
- GarbageCollector* gc = new GarbageCollector();
+ GarbageCollector* gc = new GarbageCollector(flags.work_dir);
StatusUpdateManager* statusUpdateManager = new StatusUpdateManager(flags);
Try<ResourceEstimator*> resourceEstimator =
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/tests/cluster.cpp
----------------------------------------------------------------------
diff --git a/src/tests/cluster.cpp b/src/tests/cluster.cpp
index d657da6..a5e2037 100644
--- a/src/tests/cluster.cpp
+++ b/src/tests/cluster.cpp
@@ -474,7 +474,7 @@ Try<process::Owned<Slave>> Slave::start(
// If the garbage collector is not provided, create a default one.
if (gc.isNone()) {
- slave->gc.reset(new slave::GarbageCollector());
+ slave->gc.reset(new slave::GarbageCollector(flags.work_dir));
}
// If the resource estimator is not provided, create a default one.
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/tests/gc_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/gc_tests.cpp b/src/tests/gc_tests.cpp
index d4daad3..6906a6c 100644
--- a/src/tests/gc_tests.cpp
+++ b/src/tests/gc_tests.cpp
@@ -92,7 +92,7 @@ class GarbageCollectorTest : public TemporaryDirectoryTest {};
TEST_F(GarbageCollectorTest, Schedule)
{
- GarbageCollector gc;
+ GarbageCollector gc("work_dir");
// Make some temporary files to gc.
const string& file1 = "file1";
@@ -153,7 +153,7 @@ TEST_F(GarbageCollectorTest, Schedule)
TEST_F(GarbageCollectorTest, Unschedule)
{
- GarbageCollector gc;
+ GarbageCollector gc("work_dir");
// Attempt to unschedule a file that is not scheduled.
AWAIT_ASSERT_FALSE(gc.unschedule("bogus"));
@@ -202,7 +202,7 @@ TEST_F(GarbageCollectorTest, Unschedule)
TEST_F(GarbageCollectorTest, Prune)
{
- GarbageCollector gc;
+ GarbageCollector gc("work_dir");
// Make some temporary files to prune.
const string& file1 = "file1";
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/tests/mock_slave.cpp
----------------------------------------------------------------------
diff --git a/src/tests/mock_slave.cpp b/src/tests/mock_slave.cpp
index c435ec7..6e69fac 100644
--- a/src/tests/mock_slave.cpp
+++ b/src/tests/mock_slave.cpp
@@ -49,25 +49,6 @@ namespace mesos {
namespace internal {
namespace tests {
-MockGarbageCollector::MockGarbageCollector()
-{
- // NOTE: We use 'EXPECT_CALL' and 'WillRepeatedly' here instead of
- // 'ON_CALL' and 'WillByDefault'. See 'TestContainerizer::SetUp()'
- // for more details.
- EXPECT_CALL(*this, schedule(_, _))
- .WillRepeatedly(Return(Nothing()));
-
- EXPECT_CALL(*this, unschedule(_))
- .WillRepeatedly(Return(true));
-
- EXPECT_CALL(*this, prune(_))
- .WillRepeatedly(Return());
-}
-
-
-MockGarbageCollector::~MockGarbageCollector() {}
-
-
MockResourceEstimator::MockResourceEstimator()
{
ON_CALL(*this, initialize(_))
@@ -116,7 +97,7 @@ MockSlave::MockSlave(
detector,
containerizer,
&files,
- &gc,
+ gc = new slave::GarbageCollector(flags.work_dir),
statusUpdateManager = new slave::StatusUpdateManager(flags),
&resourceEstimator,
_qosController.isSome() ? _qosController.get() : &qosController,
@@ -150,6 +131,7 @@ MockSlave::MockSlave(
MockSlave::~MockSlave()
{
+ delete gc;
delete statusUpdateManager;
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/809dd0ff/src/tests/mock_slave.hpp
----------------------------------------------------------------------
diff --git a/src/tests/mock_slave.hpp b/src/tests/mock_slave.hpp
index 767ed3d..3e960ed 100644
--- a/src/tests/mock_slave.hpp
+++ b/src/tests/mock_slave.hpp
@@ -49,24 +49,6 @@ namespace mesos {
namespace internal {
namespace tests {
-class MockGarbageCollector : public slave::GarbageCollector
-{
-public:
- MockGarbageCollector();
- virtual ~MockGarbageCollector();
-
- MOCK_METHOD2(
- schedule,
- process::Future<Nothing>(const Duration& d, const std::string& path));
- MOCK_METHOD1(
- unschedule,
- process::Future<bool>(const std::string& path));
- MOCK_METHOD1(
- prune,
- void(const Duration& d));
-};
-
-
class MockResourceEstimator : public mesos::slave::ResourceEstimator
{
public:
@@ -213,7 +195,7 @@ public:
private:
Files files;
- MockGarbageCollector gc;
+ slave::GarbageCollector* gc;
MockResourceEstimator resourceEstimator;
MockQoSController qosController;
slave::StatusUpdateManager* statusUpdateManager;
[2/3] mesos git commit: Rewrote the `ROOT_BusyMountPoint` test to
reflect updated behavior.
Posted by ch...@apache.org.
Rewrote the `ROOT_BusyMountPoint` test to reflect updated behavior.
The current `ROOT_BusyMountPoint` test would fail because we added
support for unmounting dangling mount points in directory to gc. This
patch rewrote this test to reflect that after unmounting, the gc
succeeded, directory was gone and metrics were correctly reported.
Review: https://reviews.apache.org/r/67421
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/7af1fc08
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/7af1fc08
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/7af1fc08
Branch: refs/heads/1.4.x
Commit: 7af1fc08cb7f3f908aa754ee6ee0f24ac4370aa4
Parents: 809dd0f
Author: Zhitao Li <zh...@gmail.com>
Authored: Fri Jun 1 16:58:34 2018 -0700
Committer: Chun-Hung Hsiao <ch...@mesosphere.io>
Committed: Mon Jul 2 14:26:38 2018 -0700
----------------------------------------------------------------------
src/tests/gc_tests.cpp | 76 ++++++++++++++++++++++++++++++---------------
1 file changed, 51 insertions(+), 25 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/7af1fc08/src/tests/gc_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/gc_tests.cpp b/src/tests/gc_tests.cpp
index 6906a6c..d3060c3 100644
--- a/src/tests/gc_tests.cpp
+++ b/src/tests/gc_tests.cpp
@@ -873,25 +873,29 @@ TEST_F(GarbageCollectorIntegrationTest, Unschedule)
#ifdef __linux__
-// In Mesos it's possible for tasks and isolators to lay down files
-// that are not deletable by GC. This test runs a task that creates a busy
-// mount point which is not directly deletable by GC. We verify that
-// GC deletes all files that it's able to delete in the face of such errors.
-TEST_F(GarbageCollectorIntegrationTest, ROOT_BusyMountPoint)
+// This test creates a persistent volume and runs a task which mounts the volume
+// inside the sandbox, to simulate a dangling mount which agent failed to
+// clean up (see MESOS-8830). We verify that GC process will unmount the
+// dangling mount point successfully and report success in metrics.
+TEST_F(GarbageCollectorIntegrationTest, ROOT_DanglingMount)
{
Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
slave::Flags flags = CreateSlaveFlags();
+ flags.resources = strings::format("disk(%s):1024", DEFAULT_TEST_ROLE).get();
+
Owned<MasterDetector> detector = master.get()->createDetector();
Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), flags);
ASSERT_SOME(slave);
+ FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
+ frameworkInfo.set_roles(0, DEFAULT_TEST_ROLE);
MockScheduler sched;
MesosSchedulerDriver driver(
&sched,
- DEFAULT_FRAMEWORK_INFO,
+ frameworkInfo,
master.get()->pid,
DEFAULT_CREDENTIAL);
@@ -910,21 +914,33 @@ TEST_F(GarbageCollectorIntegrationTest, ROOT_BusyMountPoint)
AWAIT_READY(offers);
EXPECT_FALSE(offers->empty());
- const Offer& offer = offers.get()[0];
- const SlaveID& slaveId = offer.slave_id();
+ const Offer& offer = offers->at(0);
+
+ string persistenceId = "persistence-id";
+ string containerPath = "path";
+
+ Resource volume = createPersistentVolume(
+ Megabytes(1024),
+ DEFAULT_TEST_ROLE,
+ persistenceId,
+ containerPath,
+ None(),
+ None(),
+ frameworkInfo.principal());
+
+ string mountPoint = "dangling";
+
+ string hostPath = slave::paths::getPersistentVolumePath(
+ flags.work_dir, DEFAULT_TEST_ROLE, persistenceId);
- // The busy mount point goes before the regular file in GC's
- // directory traversal due to their names. This makes sure that
- // an error occurs before all deletable files are GCed.
- string mountPoint = "test1";
- string regularFile = "test2.txt";
+ string fileInVolume = "foo.txt";
TaskInfo task = createTask(
- slaveId,
- Resources::parse("cpus:1;mem:128;disk:1").get(),
- "touch "+ regularFile + "; "
+ offer.slave_id(),
+ Resources(volume),
+ "touch "+ path::join(containerPath, fileInVolume) + "; "
"mkdir " + mountPoint + "; "
- "mount --bind " + mountPoint + " " + mountPoint,
+ "mount --bind " + hostPath + " " + mountPoint,
None(),
"test-task123",
"test-task123");
@@ -938,7 +954,15 @@ TEST_F(GarbageCollectorIntegrationTest, ROOT_BusyMountPoint)
Future<Nothing> schedule = FUTURE_DISPATCH(
_, &GarbageCollectorProcess::schedule);
- driver.launchTasks(offer.id(), {task});
+ Future<Nothing> ack1 =
+ FUTURE_DISPATCH(_, &Slave::_statusUpdateAcknowledgement);
+
+ Future<Nothing> ack2 =
+ FUTURE_DISPATCH(_, &Slave::_statusUpdateAcknowledgement);
+
+ driver.acceptOffers(
+ {offer.id()},
+ {CREATE(volume), LAUNCH({task})});
AWAIT_READY(status1);
EXPECT_EQ(task.task_id(), status1->task_id());
@@ -948,23 +972,21 @@ TEST_F(GarbageCollectorIntegrationTest, ROOT_BusyMountPoint)
executorId.set_value("test-task123");
Result<string> _sandbox = os::realpath(slave::paths::getExecutorLatestRunPath(
flags.work_dir,
- slaveId,
+ offer.slave_id(),
frameworkId.get(),
executorId));
ASSERT_SOME(_sandbox);
string sandbox = _sandbox.get();
EXPECT_TRUE(os::exists(sandbox));
- // Wait for the task to create these paths.
+ // Wait for the task to create the dangling mount point.
Timeout timeout = Timeout::in(Seconds(15));
while (!os::exists(path::join(sandbox, mountPoint)) ||
- !os::exists(path::join(sandbox, regularFile)) ||
!timeout.expired()) {
os::sleep(Milliseconds(10));
}
ASSERT_TRUE(os::exists(path::join(sandbox, mountPoint)));
- ASSERT_TRUE(os::exists(path::join(sandbox, regularFile)));
AWAIT_READY(status2);
ASSERT_EQ(task.task_id(), status2->task_id());
@@ -972,13 +994,17 @@ TEST_F(GarbageCollectorIntegrationTest, ROOT_BusyMountPoint)
AWAIT_READY(schedule);
+ ASSERT_TRUE(os::exists(path::join(sandbox, mountPoint)));
+ ASSERT_TRUE(os::exists(path::join(hostPath, fileInVolume)));
+
+ AWAIT_READY(schedule);
+
Clock::pause();
Clock::advance(flags.gc_delay);
Clock::settle();
- EXPECT_TRUE(os::exists(sandbox));
- EXPECT_TRUE(os::exists(path::join(sandbox, mountPoint)));
- EXPECT_FALSE(os::exists(path::join(sandbox, regularFile)));
+ ASSERT_FALSE(os::exists(path::join(sandbox, mountPoint)));
+ ASSERT_TRUE(os::exists(path::join(hostPath, fileInVolume)));
Clock::resume();
driver.stop();