Attached/detached volume directory for task which has volume specified.

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


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

Branch: refs/heads/1.5.x
Commit: 345a9621f1021834d067ee591fb54b53753621c3
Parents: 8ae1e89
Author: Qian Zhang <zhq527...@gmail.com>
Authored: Wed Feb 14 00:17:37 2018 -0800
Committer: Gilbert Song <songzihao1...@gmail.com>
Committed: Wed Feb 14 00:33:51 2018 -0800

----------------------------------------------------------------------
 src/slave/slave.cpp | 144 ++++++++++++++++++++++++++++++++++++++++++++---
 src/slave/slave.hpp |  50 ++++++++++------
 2 files changed, 168 insertions(+), 26 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/345a9621/src/slave/slave.cpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
index 065b255..151c5eb 100644
--- a/src/slave/slave.cpp
+++ b/src/slave/slave.cpp
@@ -1031,6 +1031,7 @@ void Slave::attachTaskVolumeDirectory(
 
   CHECK_EQ(task.executor_id(), executorInfo.executor_id());
 
+  // This is the case that the task has disk resources specified.
   foreach (const Resource& resource, task.resources()) {
     // Ignore if there are no disk resources or if the
     // disk resources did not specify a volume mapping.
@@ -1040,15 +1041,15 @@ void Slave::attachTaskVolumeDirectory(
 
     const Volume& volume = resource.disk().volume();
 
-    const string executorDirectory = paths::getExecutorRunPath(
+    const string executorRunPath = paths::getExecutorRunPath(
         flags.work_dir,
         info.id(),
         task.framework_id(),
         task.executor_id(),
         executorContainerId);
 
-    const string executorVolumePath =
-      path::join(executorDirectory, volume.container_path());
+    const string executorDirectoryPath =
+      path::join(executorRunPath, volume.container_path());
 
     const string taskPath = paths::getTaskPath(
         flags.work_dir,
@@ -1058,16 +1059,86 @@ void Slave::attachTaskVolumeDirectory(
         executorContainerId,
         task.task_id());
 
-    const string taskVolumePath =
+    const string taskDirectoryPath =
       path::join(taskPath, volume.container_path());
 
-    files->attach(executorVolumePath, taskVolumePath)
+    files->attach(executorDirectoryPath, taskDirectoryPath)
       .onAny(defer(
           self(),
           &Self::fileAttached,
           lambda::_1,
-          executorVolumePath,
-          taskVolumePath));
+          executorDirectoryPath,
+          taskDirectoryPath));
+  }
+
+  // This is the case that the executor has disk resources specified
+  // and the task's ContainerInfo has a `SANDBOX_PATH` volume with type
+  // `PARENT` to share the executor's disk volume.
+  hashset<string> executorContainerPaths;
+  foreach (const Resource& resource, executorInfo.resources()) {
+    // Ignore if there are no disk resources or if the
+    // disk resources did not specify a volume mapping.
+    if (!resource.has_disk() || !resource.disk().has_volume()) {
+      continue;
+    }
+
+    const Volume& volume = resource.disk().volume();
+    executorContainerPaths.insert(volume.container_path());
+  }
+
+  if (executorContainerPaths.empty()) {
+    return;
+  }
+
+  if (task.has_container()) {
+    foreach (const Volume& volume, task.container().volumes()) {
+      if (!volume.has_source() ||
+          volume.source().type() != Volume::Source::SANDBOX_PATH) {
+        continue;
+      }
+
+      CHECK(volume.source().has_sandbox_path());
+
+      const Volume::Source::SandboxPath& sandboxPath =
+        volume.source().sandbox_path();
+
+      if (sandboxPath.type() != Volume::Source::SandboxPath::PARENT) {
+        continue;
+      }
+
+      if (!executorContainerPaths.contains(sandboxPath.path())) {
+        continue;
+      }
+
+      const string executorRunPath = paths::getExecutorRunPath(
+          flags.work_dir,
+          info.id(),
+          task.framework_id(),
+          task.executor_id(),
+          executorContainerId);
+
+      const string executorDirectoryPath =
+        path::join(executorRunPath, sandboxPath.path());
+
+      const string taskPath = paths::getTaskPath(
+          flags.work_dir,
+          info.id(),
+          task.framework_id(),
+          task.executor_id(),
+          executorContainerId,
+          task.task_id());
+
+      const string taskDirectoryPath =
+        path::join(taskPath, volume.container_path());
+
+      files->attach(executorDirectoryPath, taskDirectoryPath)
+        .onAny(defer(
+            self(),
+            &Self::fileAttached,
+            lambda::_1,
+            executorDirectoryPath,
+            taskDirectoryPath));
+    }
   }
 }
 
@@ -1083,9 +1154,22 @@ void Slave::detachTaskVolumeDirectories(
         (executorInfo.has_type() &&
          executorInfo.type() == ExecutorInfo::DEFAULT));
 
+  hashset<string> executorContainerPaths;
+  foreach (const Resource& resource, executorInfo.resources()) {
+    // Ignore if there are no disk resources or if the
+    // disk resources did not specify a volume mapping.
+    if (!resource.has_disk() || !resource.disk().has_volume()) {
+      continue;
+    }
+
+    const Volume& volume = resource.disk().volume();
+    executorContainerPaths.insert(volume.container_path());
+  }
+
   foreach (const Task& task, tasks) {
     CHECK_EQ(task.executor_id(), executorInfo.executor_id());
 
+    // This is the case that the task has disk resources specified.
     foreach (const Resource& resource, task.resources()) {
       // Ignore if there are no disk resources or if the
       // disk resources did not specify a volume mapping.
@@ -1103,10 +1187,52 @@ void Slave::detachTaskVolumeDirectories(
           executorContainerId,
           task.task_id());
 
-      const string taskVolumePath =
+      const string taskDirectoryPath =
         path::join(taskPath, volume.container_path());
 
-      files->detach(taskVolumePath);
+      files->detach(taskDirectoryPath);
+    }
+
+    if (executorContainerPaths.empty()) {
+      continue;
+    }
+
+    // This is the case that the executor has disk resources specified
+    // and the task's ContainerInfo has a `SANDBOX_PATH` volume with type
+    // `PARENT` to share the executor's disk volume.
+    if (task.has_container()) {
+      foreach (const Volume& volume, task.container().volumes()) {
+        if (!volume.has_source() ||
+            volume.source().type() != Volume::Source::SANDBOX_PATH) {
+          continue;
+        }
+
+        CHECK(volume.source().has_sandbox_path());
+
+        const Volume::Source::SandboxPath& sandboxPath =
+          volume.source().sandbox_path();
+
+        if (sandboxPath.type() != Volume::Source::SandboxPath::PARENT) {
+          continue;
+        }
+
+        if (!executorContainerPaths.contains(sandboxPath.path())) {
+          continue;
+        }
+
+        const string taskPath = paths::getTaskPath(
+            flags.work_dir,
+            info.id(),
+            task.framework_id(),
+            task.executor_id(),
+            executorContainerId,
+            task.task_id());
+
+        const string taskDirectoryPath =
+          path::join(taskPath, volume.container_path());
+
+        files->detach(taskDirectoryPath);
+      }
     }
   }
 }

http://git-wip-us.apache.org/repos/asf/mesos/blob/345a9621/src/slave/slave.hpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp
index 38808cb..45cea0d 100644
--- a/src/slave/slave.hpp
+++ b/src/slave/slave.hpp
@@ -409,30 +409,46 @@ public:
 
   Nothing detachFile(const std::string& path);
 
-  // TODO(qianzhang): This is a workaround to make the default executor
-  // task's volume directory visible in MESOS UI. In MESOS-7225, we made
-  // sure a task can access any volumes specified in its disk resources
-  // from its sandbox by introducing a workaround to the default executor,
-  // i.e., adding a `SANDBOX_PATH` volume with type `PARENT` to the
-  // corresponding nested container. This volume gets translated into a
-  // bind mount in the nested container's mount namespace, which is is not
-  // visible in Mesos UI because it operates in the host namespace. See
-  // Mesos-8279 for details.
+  // TODO(qianzhang): This is a workaround to make the default executor task's
+  // volume directory visible in MESOS UI. It handles two cases:
+  //   1. The task has disk resources specified. In this case any disk 
resources
+  //      specified for the task are mounted on the top level container since
+  //      currently all resources of nested containers are merged in the top
+  //      level executor container. To make sure the task can access any 
volumes
+  //      specified in its disk resources from its sandbox, a workaround was
+  //      introduced to the default executor in MESOS-7225, i.e., adding a
+  //      `SANDBOX_PATH` volume with type `PARENT` to the corresponding nested
+  //      container. This volume gets translated into a bind mount in the 
nested
+  //      container's mount namespace, which is not visible in Mesos UI because
+  //      it operates in the host namespace. See MESOS-8279 for details.
+  //   2. The executor has disk resources specified and the task's 
ContainerInfo
+  //      has a `SANDBOX_PATH` volume with type `PARENT` specified to share the
+  //      executor's disk volume. Similar to the first case, this 
`SANDBOX_PATH`
+  //      volume gets translated into a bind mount which is not visible in 
Mesos
+  //      UI. See MESOS-8565 for details.
   //
-  // To make the task's volume directory visible in Mesos UI, here we
-  // attach the executor's volume directory to it, so when users browse
-  // task's volume directory in Mesos UI, what they actually browse is the
-  // executor's volume directory. Note when calling `Files::attach()`, the
-  // third argument `authorized` is not specified because it is already
-  // specified when we do the attach for the executor's sandbox and it also
-  // applies to the executor's tasks.
+  // To make the task's volume directory visible in Mesos UI, here we attach 
the
+  // executor's volume directory to it so that it can be accessed via the 
/files
+  // endpoint. So when users browse task's volume directory in Mesos UI, what
+  // they actually browse is the executor's volume directory. Note when calling
+  // `Files::attach()`, the third argument `authorized` is not specified 
because
+  // it is already specified when we do the attach for the executor's sandbox
+  // and it also applies to the executor's tasks. Note that for the second case
+  // we can not do the attach when the task's ContainerInfo has a 
`SANDBOX_PATH`
+  // volume with type `PARENT` but the executor has NO disk resources, because
+  // in such case the attach will fail due to the executor's volume directory
+  // not existing which will actually be created by the `volume/sandbox_path`
+  // isolator when launching the nested container. But if the executor has disk
+  // resources, then we will not have this issue since the executor's volume
+  // directory will be created by the `filesystem/linux` isolator when 
launching
+  // the executor before we do the attach.
   void attachTaskVolumeDirectory(
       const ExecutorInfo& executorInfo,
       const ContainerID& executorContainerId,
       const Task& task);
 
   // TODO(qianzhang): Remove the task's volume directory from the /files
-  // endpoint. This is a workaround for MESOS-8279.
+  // endpoint. This is a workaround for MESOS-8279 and MESOS-8565.
   void detachTaskVolumeDirectories(
       const ExecutorInfo& executorInfo,
       const ContainerID& executorContainerId,

Reply via email to