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 2016/05/03 20:18:12 UTC

[3/8] mesos git commit: Simplified the rootfs preparation logic for command tasks.

Simplified the rootfs preparation logic for command tasks.

This patch simplifies the way we deal with rootfs for command tasks.
Here are the major simplifications:

1) We no longer do a bind mount (<rootfs> -> <sandbox>/.rootfs) for
command tasks. All isolators can now assume that for both custom
executors or command tasks, if they change root filesystems, the root
filesystems will be specified in 'containerConfig.rootfs'. This will
simplify isolators' logic.

2) The sandbox bind mount will be done in the container's mount
namespace. Currently, it's done in the host mount namespace. This
creates a lot of complications, especially during the cleanup and
recovery path.

3) All persistent volumes will be consistently mounted under the
container's sandbox directory. Currently, depending on whether the
container changes the root filesystem or not, the persistent volumes
will be mounted at different locations. This simplifies the cleanup and
recovery path as well.

*** Modified for 0.28.2 ***

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


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

Branch: refs/heads/0.28.x
Commit: a76d55799cf09dbc268bb5b964543fbdd4b83eaa
Parents: 5dc1064
Author: Jie Yu <yu...@gmail.com>
Authored: Thu Apr 28 16:39:53 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Tue May 3 10:05:15 2016 -0700

----------------------------------------------------------------------
 docs/container-image.md                         |   3 +-
 src/launcher/executor.cpp                       |  86 ++---
 src/slave/constants.hpp                         |   3 -
 src/slave/containerizer/mesos/containerizer.cpp | 123 ++++---
 src/slave/containerizer/mesos/containerizer.hpp |   2 +
 .../mesos/isolators/filesystem/linux.cpp        | 358 +++++--------------
 .../mesos/isolators/filesystem/linux.hpp        |  15 +-
 src/slave/slave.cpp                             |  43 +--
 8 files changed, 202 insertions(+), 431 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/docs/container-image.md
----------------------------------------------------------------------
diff --git a/docs/container-image.md b/docs/container-image.md
index db4b291..b269dcc 100644
--- a/docs/container-image.md
+++ b/docs/container-image.md
@@ -323,8 +323,7 @@ Instead, it can specify a `volume` whose source is an `Image`. Mesos
 containerizer will provision the `image` specified in the `volume`,
 and mount it under the sandbox directory. The executor can perform
 `pivot_root` or `chroot` itself to enter the container root
-filesystem. This is how we solve the command executor issue, avoiding
-the requirement that it has to exist in all container images.
+filesystem.
 
 
 ## References

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/src/launcher/executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
index 4149f08..02e15ca 100644
--- a/src/launcher/executor.cpp
+++ b/src/launcher/executor.cpp
@@ -79,8 +79,9 @@ class CommandExecutorProcess : public ProtobufProcess<CommandExecutorProcess>
 {
 public:
   CommandExecutorProcess(
-      const Option<char**>& override,
+      const Option<char**>& _override,
       const string& _healthCheckDir,
+      const Option<string>& _rootfs,
       const Option<string>& _sandboxDirectory,
       const Option<string>& _workingDirectory,
       const Option<string>& _user,
@@ -96,7 +97,8 @@ public:
       frameworkInfo(None()),
       taskId(None()),
       healthCheckDir(_healthCheckDir),
-      override(override),
+      override(_override),
+      rootfs(_rootfs),
       sandboxDirectory(_sandboxDirectory),
       workingDirectory(_workingDirectory),
       user(_user),
@@ -219,17 +221,10 @@ public:
       abort();
     }
 
-    Option<string> rootfs;
-    if (sandboxDirectory.isSome()) {
-      // If 'sandbox_diretory' is specified, that means the user
-      // task specifies a root filesystem, and that root filesystem has
-      // already been prepared at COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH.
-      // The command executor is responsible for mounting the sandbox
-      // into the root filesystem, chrooting into it and changing the
-      // user before exec-ing the user process.
-      //
-      // TODO(gilbert): Consider a better way to detect if a root
-      // filesystem is specified for the command task.
+    if (rootfs.isSome()) {
+      // The command executor is responsible for chrooting into the
+      // root filesystem and changing the user before exec-ing the
+      // user process.
 #ifdef __linux__
       Result<string> user = os::user();
       if (user.isError()) {
@@ -242,49 +237,6 @@ public:
         cerr << "The command executor requires root with rootfs" << endl;
         abort();
       }
-
-      rootfs = path::join(
-          os::getcwd(), COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH);
-
-      string sandbox = path::join(rootfs.get(), sandboxDirectory.get());
-      if (!os::exists(sandbox)) {
-        Try<Nothing> mkdir = os::mkdir(sandbox);
-        if (mkdir.isError()) {
-          cerr << "Failed to create sandbox mount point  at '"
-               << sandbox << "': " << mkdir.error() << endl;
-          abort();
-        }
-      }
-
-      // Mount the sandbox into the container rootfs.
-      // We need to perform a recursive mount because we want all the
-      // volume mounts in the sandbox to be also mounted in the container
-      // root filesystem. However, since the container root filesystem
-      // is also mounted in the sandbox, after the recursive mount we
-      // also need to unmount the root filesystem in the mounted sandbox.
-      Try<Nothing> mount = fs::mount(
-          os::getcwd(),
-          sandbox,
-          None(),
-          MS_BIND | MS_REC,
-          NULL);
-
-      if (mount.isError()) {
-        cerr << "Unable to mount the work directory into container "
-             << "rootfs: " << mount.error() << endl;;
-        abort();
-      }
-
-      // Umount the root filesystem path in the mounted sandbox after
-      // the recursive mount.
-      Try<Nothing> unmountAll = fs::unmountAll(path::join(
-          sandbox,
-          COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH));
-      if (unmountAll.isError()) {
-        cerr << "Unable to unmount rootfs under mounted sandbox: "
-             << unmountAll.error() << endl;
-        abort();
-      }
 #else
       cerr << "Not expecting root volume with non-linux platform." << endl;
       abort();
@@ -707,6 +659,7 @@ private:
   Option<TaskID> taskId;
   string healthCheckDir;
   Option<char**> override;
+  Option<string> rootfs;
   Option<string> sandboxDirectory;
   Option<string> workingDirectory;
   Option<string> user;
@@ -720,17 +673,20 @@ public:
   CommandExecutor(
       const Option<char**>& override,
       const string& healthCheckDir,
+      const Option<string>& rootfs,
       const Option<string>& sandboxDirectory,
       const Option<string>& workingDirectory,
       const Option<string>& user,
       const Option<string>& taskCommand)
   {
-    process = new CommandExecutorProcess(override,
-                                         healthCheckDir,
-                                         sandboxDirectory,
-                                         workingDirectory,
-                                         user,
-                                         taskCommand);
+    process = new CommandExecutorProcess(
+        override,
+        healthCheckDir,
+        rootfs,
+        sandboxDirectory,
+        workingDirectory,
+        user,
+        taskCommand);
 
     spawn(process);
   }
@@ -819,6 +775,10 @@ public:
         "subsequent 'argv' to be used with 'execvp'",
         false);
 
+    add(&rootfs,
+        "rootfs",
+        "The path to the root filesystem for the task");
+
     // The following flags are only applicable when a rootfs is
     // provisioned for this command.
     add(&sandbox_directory,
@@ -844,6 +804,7 @@ public:
   }
 
   bool override;
+  Option<string> rootfs;
   Option<string> sandbox_directory;
   Option<string> working_directory;
   Option<string> user;
@@ -889,6 +850,7 @@ int main(int argc, char** argv)
   mesos::internal::CommandExecutor executor(
       override,
       path,
+      flags.rootfs,
       flags.sandbox_directory,
       flags.working_directory,
       flags.user,

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/src/slave/constants.hpp
----------------------------------------------------------------------
diff --git a/src/slave/constants.hpp b/src/slave/constants.hpp
index bcbb140..d5ded8a 100644
--- a/src/slave/constants.hpp
+++ b/src/slave/constants.hpp
@@ -116,9 +116,6 @@ const int DOCKER_PS_MAX_INSPECT_CALLS = 100;
 // trigger a re-detection of the master to cause a re-registration.
 Duration DEFAULT_MASTER_PING_TIMEOUT();
 
-// Container path that the slave sets to mount the command executor rootfs to.
-extern const std::string COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH;
-
 } // namespace slave {
 } // namespace internal {
 } // namespace mesos {

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index fb6afe1..cb8aae0 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -675,29 +675,49 @@ Future<bool> MesosContainerizerProcess::launch(
   // We need to set the `launchInfos` to be a ready future initially
   // before we starting calling isolator->prepare() because otherwise,
   // the destroy will wait forever trying to wait for this future to
-  // be ready , which it never will. See MESOS-4878.
+  // be ready, which it never will. See MESOS-4878.
   container->launchInfos = list<Option<ContainerLaunchInfo>>();
 
   containers_.put(containerId, Owned<Container>(container));
 
+  // If 'container' is not set in 'executorInfo', one of the following
+  // is true:
+  //  1) This is a custom executor without ContainerInfo.
+  //  2) This is a command task without ContainerInfo (since we copy
+  //     ContainerInfo for command tasks if exists).
+  // In either of the above cases, no provisioning is needed.
+  // Therefore, we can go straight to 'prepare'.
   if (!executorInfo.has_container()) {
     return prepare(containerId, taskInfo, executorInfo, directory, user, None())
       .then(defer(self(),
                   &Self::__launch,
                   containerId,
+                  taskInfo,
                   executorInfo,
                   directory,
                   user,
                   slaveId,
                   slavePid,
                   checkpoint,
+                  None(),
                   lambda::_1));
   }
 
-  // Provision the root filesystem if needed.
-  CHECK_EQ(executorInfo.container().type(), ContainerInfo::MESOS);
+  // We'll first provision the image for the container , and then
+  // provision the images specified in Volumes.
+  Option<Image> containerImage;
+
+  if (taskInfo.isSome() &&
+      taskInfo->has_container() &&
+      taskInfo->container().mesos().has_image()) {
+    // Command task.
+    containerImage = taskInfo->container().mesos().image();
+  } else if (executorInfo.container().mesos().has_image()) {
+    // Custom executor.
+    containerImage = executorInfo.container().mesos().image();
+  }
 
-  if (!executorInfo.container().mesos().has_image()) {
+  if (containerImage.isNone()) {
     return _launch(containerId,
                    taskInfo,
                    executorInfo,
@@ -709,14 +729,13 @@ Future<bool> MesosContainerizerProcess::launch(
                    None());
   }
 
-  const Image& image = executorInfo.container().mesos().image();
-
-  Future<ProvisionInfo> future =
-    provisioner->provision(containerId, image);
+  Future<ProvisionInfo> provisioning = provisioner->provision(
+      containerId,
+      containerImage.get());
 
-  container->provisionInfos.push_back(future);
+  container->provisionInfos.push_back(provisioning);
 
-  return future
+  return provisioning
     .then(defer(PID<MesosContainerizerProcess>(this),
                 &MesosContainerizerProcess::_launch,
                 containerId,
@@ -765,15 +784,11 @@ Future<bool> MesosContainerizerProcess::_launch(
   // as well. We will mutate ContainerInfo::volumes to include the
   // paths to the provisioned root filesystems (by setting the
   // 'host_path') if the volume specifies an image as the source.
+  //
+  // TODO(gilbert): We need to figure out a way to support passing
+  // runtime configurations specified in the image to the container.
   Owned<ExecutorInfo> _executorInfo(new ExecutorInfo(executorInfo));
 
-  // NOTE: For the command task (i.e., taskInfo.isSome()), the
-  // filesystem image for the task is specified in a volume
-  // (COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH). We need to pass the
-  // provisionInfo from that image to isolators through 'prepare'.
-  Owned<Option<ProvisionInfo>> _provisionInfo(
-      new Option<ProvisionInfo>(provisionInfo));
-
   list<Future<Nothing>> futures;
 
   for (int i = 0; i < _executorInfo->container().volumes_size(); i++) {
@@ -788,25 +803,12 @@ Future<bool> MesosContainerizerProcess::_launch(
     Future<ProvisionInfo> future = provisioner->provision(containerId, image);
     containers_[containerId]->provisionInfos.push_back(future);
 
-    futures.push_back(future
-      .then([=](const ProvisionInfo& info) -> Future<Nothing> {
+    futures.push_back(future.then([=](const ProvisionInfo& info) {
         volume->set_host_path(info.rootfs);
-
-        if (taskInfo.isSome() &&
-            volume->container_path() ==
-              COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH) {
-          *_provisionInfo = info;
-        }
-
         return Nothing();
       }));
   }
 
-  // TODO(gilbert): For command executors, we modify the executorInfo
-  // so that the user specified image will be mounted in as a volume.
-  // However, we also need to figure out a way to support passing and
-  // handling those runtime configurations in the image.
-
   // We put `prepare` inside of a lambda expression, in order to get
   // _executorInfo object after host path set in volume.
   return collect(futures)
@@ -816,16 +818,18 @@ Future<bool> MesosContainerizerProcess::_launch(
                      *_executorInfo,
                      directory,
                      user,
-                     *_provisionInfo)
+                     provisionInfo)
         .then(defer(self(),
                     &Self::__launch,
                     containerId,
+                    taskInfo,
                     *_executorInfo,
                     directory,
                     user,
                     slaveId,
                     slavePid,
                     checkpoint,
+                    provisionInfo,
                     lambda::_1));
     }));
 }
@@ -953,12 +957,14 @@ Future<Nothing> MesosContainerizerProcess::fetch(
 
 Future<bool> MesosContainerizerProcess::__launch(
     const ContainerID& containerId,
+    const Option<TaskInfo>& taskInfo,
     const ExecutorInfo& executorInfo,
     const string& directory,
     const Option<string>& user,
     const SlaveID& slaveId,
     const PID<Slave>& slavePid,
     bool checkpoint,
+    const Option<ProvisionInfo>& provisionInfo,
     const list<Option<ContainerLaunchInfo>>& launchInfos)
 {
   if (!containers_.contains(containerId)) {
@@ -978,25 +984,19 @@ Future<bool> MesosContainerizerProcess::__launch(
       checkpoint,
       flags);
 
-  // Determine the root filesystem for the container. Only one
-  // isolator should return the container root filesystem.
-  Option<string> rootfs;
+  // Determine the root filesystem for the executor.
+  Option<string> executorRootfs;
+  if (taskInfo.isNone() && provisionInfo.isSome()) {
+    executorRootfs = provisionInfo->rootfs;
+  }
 
   // Determine the executor launch command for the container.
   // At most one command can be returned from docker runtime
-  // isolator if a docker image is specifed.
+  // isolator if a docker image is specified.
   Option<CommandInfo> executorLaunchCommand;
   Option<string> workingDirectory;
 
   foreach (const Option<ContainerLaunchInfo>& launchInfo, launchInfos) {
-    if (launchInfo.isSome() && launchInfo->has_rootfs()) {
-      if (rootfs.isSome()) {
-        return Failure("Only one isolator should return the container rootfs");
-      } else {
-        rootfs = launchInfo->rootfs();
-      }
-    }
-
     if (launchInfo.isSome() && launchInfo->has_environment()) {
       foreach (const Environment::Variable& variable,
                launchInfo->environment().variables()) {
@@ -1035,8 +1035,9 @@ Future<bool> MesosContainerizerProcess::__launch(
 
   // TODO(jieyu): Consider moving this to 'executorEnvironment' and
   // consolidating with docker containerizer.
-  environment["MESOS_SANDBOX"] =
-    rootfs.isSome() ? flags.sandbox_directory : directory;
+  environment["MESOS_SANDBOX"] = executorRootfs.isSome()
+    ? flags.sandbox_directory
+    : directory;
 
   // Include any enviroment variables from CommandInfo.
   foreach (const Environment::Variable& variable,
@@ -1074,6 +1075,16 @@ Future<bool> MesosContainerizerProcess::__launch(
   JSON::Object commands;
   commands.values["commands"] = commandArray;
 
+  if (executorLaunchCommand.isNone()) {
+    executorLaunchCommand = executorInfo.command();
+  }
+
+  // Inform the command executor about the rootfs of the task.
+  if (taskInfo.isSome() && provisionInfo.isSome()) {
+    CHECK_SOME(executorLaunchCommand);
+    executorLaunchCommand->add_arguments("--rootfs=" + provisionInfo->rootfs);
+  }
+
   return logger->prepare(executorInfo, directory)
     .then(defer(
         self(),
@@ -1089,18 +1100,16 @@ Future<bool> MesosContainerizerProcess::__launch(
     // Prepare the flags to pass to the launch process.
     MesosContainerizerLaunch::Flags launchFlags;
 
-    launchFlags.command = executorLaunchCommand.isSome()
-      ? JSON::protobuf(executorLaunchCommand.get())
-      : JSON::protobuf(executorInfo.command());
+    launchFlags.command = JSON::protobuf(executorLaunchCommand.get());
 
-    launchFlags.sandbox = rootfs.isSome()
+    launchFlags.sandbox = executorRootfs.isSome()
       ? flags.sandbox_directory
       : directory;
 
     // NOTE: If the executor shares the host filesystem, we should not
     // allow them to 'cd' into an arbitrary directory because that'll
     // create security issues.
-    if (rootfs.isNone() && workingDirectory.isSome()) {
+    if (executorRootfs.isNone() && workingDirectory.isSome()) {
       LOG(WARNING) << "Ignore working directory '" << workingDirectory.get()
                    << "' specified in container launch info for container "
                    << containerId << " since the executor is using the "
@@ -1110,19 +1119,19 @@ Future<bool> MesosContainerizerProcess::__launch(
     }
 
 #ifdef __WINDOWS__
-    if (!rootfs.isNone()) {
+    if (!executorRootfs.isNone()) {
       return Failure(
-          "`chroot` is not supported on Windows, but the `ContainerLaunchInfo` "
-          "provided a `rootfs` flag to the launcher");
+          "`chroot` is not supported on Windows, but the executor "
+          "specifies a root filesystem.");
     }
 
     if (!user.isNone()) {
       return Failure(
-          "`su` is not supported on Windows, but the `ContainerLauncherInfo` "
-          "provided a `user` to the launcher");
+          "`su` is not supported on Windows, but the executor "
+          "specifies a user.");
     }
 #else
-    launchFlags.rootfs = rootfs;
+    launchFlags.rootfs = executorRootfs;
     launchFlags.user = user;
 #endif // __WINDOWS__
     launchFlags.pipe_read = pipes[0];

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/src/slave/containerizer/mesos/containerizer.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp
index a6e3a8b..29a60ca 100644
--- a/src/slave/containerizer/mesos/containerizer.hpp
+++ b/src/slave/containerizer/mesos/containerizer.hpp
@@ -219,12 +219,14 @@ private:
 
   process::Future<bool> __launch(
       const ContainerID& containerId,
+      const Option<TaskInfo>& taskInfo,
       const ExecutorInfo& executorInfo,
       const std::string& directory,
       const Option<std::string>& user,
       const SlaveID& slaveId,
       const process::PID<Slave>& slavePid,
       bool checkpoint,
+      const Option<ProvisionInfo>& provisionInfo,
       const std::list<Option<mesos::slave::ContainerLaunchInfo>>& launchInfos);
 
   process::Future<bool> isolate(

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/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 9fc7c48..553de88 100644
--- a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
+++ b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
@@ -183,125 +183,51 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::recover(
     const list<ContainerState>& states,
     const hashset<ContainerID>& orphans)
 {
-  // Read the mount table in the host mount namespace to recover paths
-  // to containers' work directories if their root filesystems are
-  // changed. Method 'cleanup()' relies on this information to clean
-  // up mounts in the host mount namespace for each container.
-  Try<fs::MountInfoTable> table = fs::MountInfoTable::read();
-  if (table.isError()) {
-    return Failure("Failed to get mount table: " + table.error());
-  }
-
   foreach (const ContainerState& state, states) {
-    Owned<Info> info(new Info(state.directory()));
-
-    foreach (const fs::MountInfoTable::Entry& entry, table.get().entries) {
-      if (entry.root == info->directory) {
-        info->sandbox = entry.target;
-        break;
-      }
-    }
+    Owned<Info> info(new Info(
+        state.directory(),
+        state.executor_info()));
 
     infos.put(state.container_id(), info);
   }
 
-  // Recover both known and unknown orphans by scanning the mount
-  // table and finding those mounts whose roots are under slave's
-  // sandbox root directory. Those mounts are container's work
-  // directory mounts. Mounts from unknown orphans will be cleaned up
-  // immediately. Mounts from known orphans will be cleaned up when
-  // those known orphan containers are being destroyed by the slave.
-  //
-  // TODO(josephw): This does not deal with orphaned persistent
-  // volumes that are not included in a rootfs. There is no guarantee
-  // that an orphaned container without a rootfs was created by this
-  // isolator.  It may have been created by the `DockerContainerizer`.
-  hashset<ContainerID> unknownOrphans;
-
-  string sandboxRootDir = paths::getSandboxRootDir(flags.work_dir);
-
-  // NOTE: Mount table entries for rootfs are always created before
-  // entries for persistent volumes. We read the mount table in
-  // forward order and unmount in reverse order.
-  foreach (const fs::MountInfoTable::Entry& entry, table.get().entries) {
-    if (strings::startsWith(entry.root, sandboxRootDir)) {
-      // These are containers that specify a new root filesystem.
-      // Mesos will provision a rootfs and bind-mount the container's
-      // sandbox into the provisioned rootfs.
-      //
-      // TODO(jieyu): Here, we retrieve the container ID by taking the
-      // basename of 'entry.root'. This assumes that the slave's
-      // sandbox root directory are organized according to the
-      // comments in the beginning of slave/paths.hpp.
-      ContainerID containerId;
-      containerId.set_value(Path(entry.root).basename());
-
-      if (infos.contains(containerId)) {
-        continue;
-      }
-
-      Owned<Info> info(new Info(entry.root));
-
-      if (entry.root != entry.target) {
-        info->sandbox = entry.target;
-      }
-
-      infos.put(containerId, info);
+  // Remove orphaned persistent volume mounts.
+  Try<fs::MountInfoTable> table = fs::MountInfoTable::read();
+  if (table.isError()) {
+    return Failure("Failed to get mount table: " + table.error());
+  }
 
-      // Remember all the unknown orphan containers.
-      if (!orphans.contains(containerId)) {
-        unknownOrphans.insert(containerId);
-      }
-    } else {
-      // Check for mounts inside an executor's run path. These are
-      // persistent volumes that do not have a changed root
-      // filesystem. The combination of changed rootfs + persistent
-      // volumes is managed above in the 'if' branch.
-      Try<paths::ExecutorRunPath> runPath =
-        paths::parseExecutorRunPath(flags.work_dir, entry.target);
-
-      if (runPath.isSome()) {
-        // If there is already an `Info` about this `ContainerID`,
-        // this means this container is either a recoverable
-        // container, or a container with a changed rootfs (which was
-        // populated above).
-        if (infos.contains(runPath->containerId)) {
-          continue;
-        }
+  foreach (const fs::MountInfoTable::Entry& entry, table->entries) {
+    // Check for mounts inside an executor's run path. These are
+    // persistent volumes mounts.
+    Try<paths::ExecutorRunPath> runPath =
+      paths::parseExecutorRunPath(flags.work_dir, entry.target);
 
-        // TODO(josephw): We only track persistent volumes for known
-        // orphans as these orphans were presumably created by an
-        // earlier `MesosContainerizer`. Other persistent volumes may
-        // have been created by other actors, such as the
-        // `DockerContainerizer`.
-        if (!orphans.contains(runPath->containerId)) {
-          continue;
-        }
+    if (runPath.isError()) {
+      continue;
+    }
 
-        // NOTE: We do not need to set the `info->sandbox` because
-        // this mount corresponds to an orphan that only has a
-        // persistent volume and should not have a rootfs (otherwise,
-        // we should see a mount table entry for the rootfs earlier)..
-        Owned<Info> info(new Info(paths::getExecutorRunPath(
-            flags.work_dir,
-            runPath->slaveId,
-            runPath->frameworkId,
-            runPath->executorId,
-            runPath->containerId)));
-
-        infos.put(runPath->containerId, info);
-      }
+    if (infos.contains(runPath->containerId)) {
+      continue;
     }
-  }
 
-  // Cleanup mounts from unknown orphans.
-  list<Future<Nothing>> futures;
-  foreach (const ContainerID& containerId, unknownOrphans) {
-    futures.push_back(cleanup(containerId));
+    // TODO(josephw): We only track persistent volumes for known
+    // orphans as these orphans were presumably created by an earlier
+    // `MesosContainerizer`. Other persistent volumes may have been
+    // created by other actors, such as the `DockerContainerizer`.
+    if (orphans.contains(runPath->containerId)) {
+      Owned<Info> info(new Info(paths::getExecutorRunPath(
+          flags.work_dir,
+          runPath->slaveId,
+          runPath->frameworkId,
+          runPath->executorId,
+          runPath->containerId)));
+
+      infos.put(runPath->containerId, info);
+    }
   }
 
-  return collect(futures)
-    .then([]() -> Future<Nothing> { return Nothing(); });
+  return Nothing();
 }
 
 
@@ -320,91 +246,20 @@ Future<Option<ContainerLaunchInfo>> LinuxFilesystemIsolatorProcess::prepare(
     return Failure("Container has already been prepared");
   }
 
-  infos.put(containerId, Owned<Info>(new Info(directory)));
+  Owned<Info> info(new Info(
+      directory,
+      containerConfig.executor_info()));
 
-  const Owned<Info>& info = infos[containerId];
+  infos.put(containerId, info);
 
   ContainerLaunchInfo launchInfo;
   launchInfo.set_namespaces(CLONE_NEWNS);
 
-  if (!containerConfig.has_task_info() && containerConfig.has_rootfs()) {
-    // If the container changes its root filesystem, we need to mount
-    // the container's work directory into its root filesystem
-    // (creating it if needed) so that the executor and the task can
-    // access the work directory.
-    //
-    // NOTE: The mount of the work directory must be a shared mount in
-    // the host filesystem so that any mounts underneath it will
-    // propagate into the container's mount namespace. This is how we
-    // can update persistent volumes for the container.
-    const string rootfs = containerConfig.rootfs();
-
-    // This is the mount point of the work directory in the root filesystem.
-    const string sandbox = path::join(rootfs, flags.sandbox_directory);
-
-    // Save the path 'sandbox' which will be used in 'cleanup()'.
-    info->sandbox = sandbox;
-
-    Try<Nothing> mkdir = os::mkdir(sandbox);
-    if (mkdir.isError()) {
-      return Failure(
-          "Failed to create sandbox mount point at '" +
-          sandbox + "': " + mkdir.error());
-    }
-
-    LOG(INFO) << "Bind mounting work directory from '" << directory
-              << "' to '" << sandbox << "' for container " << containerId;
-
-    Try<Nothing> mount = fs::mount(
-        directory,
-        sandbox,
-        None(),
-        MS_BIND,
-        NULL);
-
-    if (mount.isError()) {
-      return Failure(
-          "Failed to mount work directory '" + directory +
-          "' to '" + sandbox + "': " + mount.error());
-    }
-
-    mount = fs::mount(
-        None(),
-        sandbox,
-        None(),
-        MS_SLAVE,
-        NULL);
-
-    if (mount.isError()) {
-      return Failure(
-          "Failed to mark sandbox '" + sandbox +
-          "' as a slave mount: " + mount.error());
-    }
-
-    mount = fs::mount(
-        None(),
-        sandbox,
-        None(),
-        MS_SHARED,
-        NULL);
-
-    if (mount.isError()) {
-      return Failure(
-          "Failed to mark sandbox '" + sandbox +
-          "' as a shared mount: " + mount.error());
-    }
-
-    launchInfo.set_rootfs(rootfs);
-  }
-
-  const ExecutorInfo& executorInfo = containerConfig.executor_info();
-
   // Prepare the commands that will be run in the container's mount
   // namespace right after forking the executor process. We use these
   // commands to mount those volumes specified in the container info
   // so that they don't pollute the host mount namespace.
-  Try<string> _script =
-    script(containerId, executorInfo, containerConfig);
+  Try<string> _script = script(containerId, containerConfig);
   if (_script.isError()) {
     return Failure("Failed to generate isolation script: " + _script.error());
   }
@@ -412,7 +267,7 @@ Future<Option<ContainerLaunchInfo>> LinuxFilesystemIsolatorProcess::prepare(
   CommandInfo* command = launchInfo.add_commands();
   command->set_value(_script.get());
 
-  return update(containerId, executorInfo.resources())
+  return update(containerId, containerConfig.executor_info().resources())
     .then([launchInfo]() -> Future<Option<ContainerLaunchInfo>> {
       return launchInfo;
     });
@@ -421,7 +276,6 @@ Future<Option<ContainerLaunchInfo>> LinuxFilesystemIsolatorProcess::prepare(
 
 Try<string> LinuxFilesystemIsolatorProcess::script(
     const ContainerID& containerId,
-    const ExecutorInfo& executorInfo,
     const ContainerConfig& containerConfig)
 {
   ostringstream out;
@@ -439,33 +293,29 @@ Try<string> LinuxFilesystemIsolatorProcess::script(
       << MesosContainerizerMount::NAME << " "
       << stringify(mountFlags) << "\n";
 
-  // Try to unmount work directory mounts and persistent volume mounts
-  // for other containers to release the extra references to them.
-  // NOTE:
-  // 1) This doesn't completely eliminate the race condition between
-  //    this container copying mount table and other containers being
-  //    cleaned up. This is instead a best-effort attempt.
-  // 2) This script assumes that all the mounts the container needs
-  //    under the slave work directory have its container ID in the
-  //    path either for the mount source (e.g. sandbox self-bind mount)
-  //    or the mount target (e.g. mounting sandbox into new rootfs).
-  //
-  // TODO(xujyan): This command may fail if --work_dir is not specified
-  // with a real path as real paths are used in the mount table. It
-  // doesn't work when the paths contain reserved characters such as
-  // spaces either because such characters in mount info are encoded
-  // in the escaped form (i.e. '\0xx').
-  out << "grep -E '" << flags.work_dir << "/.+' /proc/self/mountinfo | "
-      << "grep -v '" << containerId.value() << "' | "
-      << "cut -d' ' -f5 | " // '-f5' is the mount target. See MountInfoTable.
-      << "xargs --no-run-if-empty umount -l || "
-      << "true \n"; // We mask errors in this command.
-
-  if (!executorInfo.has_container()) {
+  if (!containerConfig.executor_info().has_container()) {
     return out.str();
   }
 
-  foreach (const Volume& volume, executorInfo.container().volumes()) {
+  // Bind mount the sandbox if the container specifies a rootfs.
+  if (containerConfig.has_rootfs()) {
+    string sandbox = path::join(
+        containerConfig.rootfs(),
+        flags.sandbox_directory);
+
+    Try<Nothing> mkdir = os::mkdir(sandbox);
+    if (mkdir.isError()) {
+      return Error(
+          "Failed to create sandbox mount point at '" +
+          sandbox + "': " + mkdir.error());
+    }
+
+    out << "mount -n --rbind '" << containerConfig.directory()
+        << "' '" << sandbox << "'\n";
+  }
+
+  foreach (const Volume& volume,
+           containerConfig.executor_info().container().volumes()) {
     if (!volume.has_host_path()) {
       return Error("A volume misses 'host_path'");
     }
@@ -511,26 +361,33 @@ Try<string> LinuxFilesystemIsolatorProcess::script(
     string target;
 
     if (strings::startsWith(volume.container_path(), "/")) {
-      if (!containerConfig.has_task_info() && containerConfig.has_rootfs()) {
+      if (containerConfig.has_rootfs()) {
         target = path::join(
             containerConfig.rootfs(),
             volume.container_path());
+
+        Try<Nothing> mkdir = os::mkdir(target);
+        if (mkdir.isError()) {
+          return Error(
+              "Failed to create the target of the mount at '" +
+              target + "': " + mkdir.error());
+        }
       } else {
         target = volume.container_path();
-      }
 
-      // An absolute path must already exist. This is because we want
-      // to avoid creating mount points outside the work directory in
-      // the host filesystem or in the container filesystem root.
-      if (!os::exists(target)) {
-        return Error("Absolute container path does not exist");
+        // An absolute path must already exist. This is because we
+        // want to avoid creating mount points outside the work
+        // directory in the host filesystem.
+        if (!os::exists(target)) {
+          return Error("Absolute container path does not exist");
+        }
       }
 
       // TODO(jieyu): We need to check that target resolves under
       // 'rootfs' because a user can potentially use a container path
       // like '/../../abc'.
     } else {
-      if (!containerConfig.has_task_info() && containerConfig.has_rootfs()) {
+      if (containerConfig.has_rootfs()) {
         target = path::join(containerConfig.rootfs(),
                             flags.sandbox_directory,
                             volume.container_path());
@@ -543,11 +400,19 @@ Try<string> LinuxFilesystemIsolatorProcess::script(
       // sandbox because a user can potentially use a container path
       // like '../../abc'.
 
-      Try<Nothing> mkdir = os::mkdir(target);
+      // NOTE: We cannot create the mount point at 'target' if
+      // container has rootfs defined. The bind mount of the sandbox
+      // will hide what's inside 'target'. So we should always create
+      // the mount point in 'directory'.
+      string mountPoint = path::join(
+          containerConfig.directory(),
+          volume.container_path());
+
+      Try<Nothing> mkdir = os::mkdir(mountPoint);
       if (mkdir.isError()) {
         return Error(
             "Failed to create the target of the mount at '" +
-            target + "': " + mkdir.error());
+            mountPoint + "': " + mkdir.error());
       }
     }
 
@@ -611,13 +476,7 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::update(
     }
 
     // Determine the target of the mount.
-    string target;
-
-    if (info->sandbox.isSome()) {
-      target = path::join(info->sandbox.get(), containerPath);
-    } else {
-      target = path::join(info->directory, containerPath);
-    }
+    string target = path::join(info->directory, containerPath);
 
     LOG(INFO) << "Removing mount '" << target << "' for persistent volume "
               << resource << " of container " << containerId;
@@ -693,13 +552,7 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::update(
     }
 
     // Determine the target of the mount.
-    string target;
-
-    if (info->sandbox.isSome()) {
-      target = path::join(info->sandbox.get(), containerPath);
-    } else {
-      target = path::join(info->directory, containerPath);
-    }
+    string target = path::join(info->directory, containerPath);
 
     if (os::exists(target)) {
       // NOTE: This is possible because 'info->resources' will be
@@ -766,13 +619,7 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::cleanup(
 
   // The path to the container' work directory which is the parent of
   // all the persistent volume mounts.
-  string sandbox;
-
-  if (info->sandbox.isSome()) {
-    sandbox = info->sandbox.get();
-  } else {
-    sandbox = info->directory;
-  }
+  string sandbox = info->directory;
 
   infos.erase(containerId);
 
@@ -784,17 +631,13 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::cleanup(
     return Failure("Failed to get mount table: " + table.error());
   }
 
-  bool sandboxMountExists = false;
-
   // Reverse unmount order to handle nested mount points.
   foreach (const fs::MountInfoTable::Entry& entry,
-           adaptor::reverse(table.get().entries)) {
+           adaptor::reverse(table->entries)) {
     // NOTE: All persistent volumes are mounted at targets under the
     // container's work directory. We unmount all the persistent
     // volumes before unmounting the sandbox/work directory mount.
-    if (entry.target == sandbox) {
-      sandboxMountExists = true;
-    } else if (strings::startsWith(entry.target, sandbox)) {
+    if (strings::startsWith(entry.target, sandbox)) {
       LOG(INFO) << "Unmounting volume '" << entry.target
                 << "' for container " << containerId;
 
@@ -807,24 +650,6 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::cleanup(
     }
   }
 
-  if (!sandboxMountExists) {
-    // This could happen if the container was not launched by this
-    // isolator (e.g., slaves prior to 0.25.0), or the container did
-    // not specify a root filesystem.
-    LOG(INFO) << "Ignoring unmounting sandbox/work directory"
-              << " for container " << containerId;
-  } else {
-    LOG(INFO) << "Unmounting sandbox/work directory '" << sandbox
-              << "' for container " << containerId;
-
-    Try<Nothing> unmount = fs::unmount(sandbox);
-    if (unmount.isError()) {
-      return Failure(
-          "Failed to unmount sandbox/work directory '" + sandbox +
-          "': " + unmount.error());
-    }
-  }
-
   return Nothing();
 }
 
@@ -850,7 +675,10 @@ double LinuxFilesystemIsolatorProcess::_containers_new_rootfs()
   double count = 0.0;
 
   foreachvalue (const Owned<Info>& info, infos) {
-    if (info->sandbox.isSome()) {
+    if (info->executor.isSome() &&
+        info->executor->has_container() &&
+        info->executor->container().type() == ContainerInfo::MESOS &&
+        info->executor->container().mesos().has_image()) {
       ++count;
     }
   }

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/src/slave/containerizer/mesos/isolators/filesystem/linux.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/isolators/filesystem/linux.hpp b/src/slave/containerizer/mesos/isolators/filesystem/linux.hpp
index cb18126..c99f33a 100644
--- a/src/slave/containerizer/mesos/isolators/filesystem/linux.hpp
+++ b/src/slave/containerizer/mesos/isolators/filesystem/linux.hpp
@@ -76,7 +76,6 @@ private:
 
   Try<std::string> script(
       const ContainerID& containerId,
-      const ExecutorInfo& executorInfo,
       const mesos::slave::ContainerConfig& containerConfig);
 
   const Flags flags;
@@ -85,17 +84,17 @@ private:
   {
     Info(const std::string& _directory) : directory(_directory) {}
 
-    const std::string directory;
+    Info(const std::string& _directory,
+         const ExecutorInfo& _executor)
+      : directory(_directory),
+        executor(_executor) {}
 
-    // The absolute path to the container's work directory mount point
-    // in the new root filesystem if the container changes its root
-    // filesystem (i.e., '<rootfs>/<flags.sandbox_directory>'). If the
-    // container does not specify a root filesystem, this field will
-    // not be set.
-    Option<std::string> sandbox;
+    const std::string directory;
 
     // Track resources so we can unmount unneeded persistent volumes.
     Resources resources;
+
+    Option<ExecutorInfo> executor;
   };
 
   hashmap<ContainerID, process::Owned<Info>> infos;

http://git-wip-us.apache.org/repos/asf/mesos/blob/a76d5579/src/slave/slave.cpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
index 840534f..1898441 100644
--- a/src/slave/slave.cpp
+++ b/src/slave/slave.cpp
@@ -3580,6 +3580,8 @@ ExecutorInfo Slave::getExecutorInfo(
       executor.mutable_container()->CopyFrom(task.container());
     }
 
+    // TODO(jieyu): We should move those Mesos containerizer specific
+    // logic (e.g., 'hasRootfs') to Mesos containerizer.
     bool hasRootfs = task.has_container() &&
                      task.container().type() == ContainerInfo::MESOS &&
                      task.container().mesos().has_image();
@@ -3587,43 +3589,16 @@ ExecutorInfo Slave::getExecutorInfo(
     if (hasRootfs) {
       ContainerInfo* container = executor.mutable_container();
 
-      // For command-tasks, we are now copying the entire `task.container` into
-      // the `executorInfo`. Thus, `executor.container` now has the image if
-      // `task.container` had one. However, in case of rootfs, we want to run
-      // the command executor in the host filesystem and prepare/mount the image
-      // into the container as a volume (command executor will use pivot_root to
-      // mount the image). For this reason, we need to strip the image in
+      // For command tasks, we are now copying the entire
+      // `task.container` into the `executorInfo`. Thus,
+      // `executor.container` now has the image if `task.container`
+      // had one. However, in the case of Mesos container with rootfs,
+      // we want to run the command executor in the host filesystem
+      // and let the command executor pivot_root to the rootfs for its
+      // task. For this reason, we need to strip the image in
       // `executor.container.mesos`.
       container->mutable_mesos()->clear_image();
 
-      // As we will chroot in the command executor into a new rootfs,
-      // we need to modify the volume mount points to be under the new rootfs
-      // so the container path that the task sees is correct.
-      // NOTE: We only need to modify volumes with absolute path since
-      // relative paths are mounted in the sandbox and will automatically be
-      // mounted into the rootfs.
-      for (int i = 0; i < container->volumes_size(); ++i) {
-        Volume* volume = container->mutable_volumes(i);
-        if (path::absolute(volume->container_path())) {
-          volume->set_container_path(path::join(
-              COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH,
-              volume->container_path()));
-        }
-      }
-
-      container->set_type(ContainerInfo::MESOS);
-      Volume* volume = container->add_volumes();
-      volume->mutable_image()->CopyFrom(task.container().mesos().image());
-      volume->set_container_path(COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH);
-      volume->set_mode(Volume::RW);
-
-      size_t volumesSize = container->volumes_size();
-      if (volumesSize > 1) {
-        // Move the rootfs volume to the front as the other volumes
-        // will be mounting under the rootfs directory that's added last.
-        container->mutable_volumes()->SwapElements(0, volumesSize - 1);
-      }
-
       // We need to set the executor user as root as it needs to
       // perform chroot (even when switch_user is set to false).
       executor.mutable_command()->set_user("root");