Updated Isolator::prepare to return list of required namespaces. This allows the Isolators to decide whether or not to provide resource isolation on a per-container level.
Review: https://reviews.apache.org/r/38365 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/6923bb3e Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/6923bb3e Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/6923bb3e Branch: refs/heads/master Commit: 6923bb3e8cfbddde9fbabc6ca4edc29d9fc96c06 Parents: fc541a9 Author: Kapil Arya <[email protected]> Authored: Wed Sep 16 17:01:16 2015 -0700 Committer: Niklas Q. Nielsen <[email protected]> Committed: Wed Sep 16 17:01:18 2015 -0700 ---------------------------------------------------------------------- include/mesos/slave/isolator.hpp | 9 ---- include/mesos/slave/isolator.proto | 4 ++ src/slave/containerizer/isolator.cpp | 6 --- src/slave/containerizer/isolator.hpp | 4 -- .../isolators/filesystem/linux.cpp | 7 +-- .../isolators/filesystem/linux.hpp | 2 - .../isolators/filesystem/shared.cpp | 7 +-- .../isolators/filesystem/shared.hpp | 2 - .../containerizer/isolators/namespaces/pid.cpp | 7 +-- .../containerizer/isolators/namespaces/pid.hpp | 2 - .../isolators/network/port_mapping.cpp | 18 +++---- .../isolators/network/port_mapping.hpp | 2 - src/slave/containerizer/mesos/containerizer.cpp | 14 ++---- src/tests/containerizer/isolator_tests.cpp | 51 ++++++++++++-------- 14 files changed, 50 insertions(+), 85 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/include/mesos/slave/isolator.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/slave/isolator.hpp b/include/mesos/slave/isolator.hpp index 4db23dc..ea14bff 100644 --- a/include/mesos/slave/isolator.hpp +++ b/include/mesos/slave/isolator.hpp @@ -44,15 +44,6 @@ class Isolator public: virtual ~Isolator() {} - // Returns the namespaces required by the isolator. The namespaces - // are created while launching the executor. Isolators may return - // a None() to indicate that they don't require any namespaces - // (e.g., Isolators for OS X). - // TODO(karya): Since namespaces are Linux-only, create a separate - // LinuxIsolator (and corresponding LinuxIsolatorProcess) class - // for Linux-specific isolators. - virtual process::Future<Option<int>> namespaces() { return None(); } - // Recover containers from the run states and the orphan containers // (known to the launcher but not known to the slave) detected by // the launcher. http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/include/mesos/slave/isolator.proto ---------------------------------------------------------------------- diff --git a/include/mesos/slave/isolator.proto b/include/mesos/slave/isolator.proto index 12ea6ac..9d38a25 100644 --- a/include/mesos/slave/isolator.proto +++ b/include/mesos/slave/isolator.proto @@ -71,4 +71,8 @@ message ContainerPrepareInfo // The root filesystem for the container. optional string rootfs = 3; + + // (Linux only) The namespaces required for the container. + // The namespaces are created while launching the executor. + optional uint32 namespaces = 4 [default = 0]; } http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolator.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolator.cpp b/src/slave/containerizer/isolator.cpp index 7973100..5a7bad1 100644 --- a/src/slave/containerizer/isolator.cpp +++ b/src/slave/containerizer/isolator.cpp @@ -47,12 +47,6 @@ MesosIsolator::~MesosIsolator() } -Future<Option<int>> MesosIsolator::namespaces() -{ - return dispatch(process.get(), &MesosIsolatorProcess::namespaces); -} - - Future<Nothing> MesosIsolator::recover( const list<ContainerState>& state, const hashset<ContainerID>& orphans) http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolator.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolator.hpp b/src/slave/containerizer/isolator.hpp index fbb7c8a..da58cbe 100644 --- a/src/slave/containerizer/isolator.hpp +++ b/src/slave/containerizer/isolator.hpp @@ -43,8 +43,6 @@ public: explicit MesosIsolator(process::Owned<MesosIsolatorProcess> process); virtual ~MesosIsolator(); - virtual process::Future<Option<int>> namespaces(); - virtual process::Future<Nothing> recover( const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans); @@ -82,8 +80,6 @@ class MesosIsolatorProcess : public process::Process<MesosIsolatorProcess> public: virtual ~MesosIsolatorProcess() {} - virtual process::Future<Option<int>> namespaces() { return None(); } - virtual process::Future<Nothing> recover( const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans) = 0; http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/filesystem/linux.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/filesystem/linux.cpp b/src/slave/containerizer/isolators/filesystem/linux.cpp index 297a296..d8a0e07 100644 --- a/src/slave/containerizer/isolators/filesystem/linux.cpp +++ b/src/slave/containerizer/isolators/filesystem/linux.cpp @@ -84,12 +84,6 @@ LinuxFilesystemIsolatorProcess::LinuxFilesystemIsolatorProcess( LinuxFilesystemIsolatorProcess::~LinuxFilesystemIsolatorProcess() {} -Future<Option<int>> LinuxFilesystemIsolatorProcess::namespaces() -{ - return CLONE_NEWNS; -} - - Future<Nothing> LinuxFilesystemIsolatorProcess::recover( const list<ContainerState>& states, const hashset<ContainerID>& orphans) @@ -268,6 +262,7 @@ Future<Option<ContainerPrepareInfo>> LinuxFilesystemIsolatorProcess::__prepare( const Owned<Info>& info = infos[containerId]; ContainerPrepareInfo prepareInfo; + prepareInfo.set_namespaces(CLONE_NEWNS); if (rootfs.isNone()) { // It the container does not change its root filesystem, we need http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/filesystem/linux.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/filesystem/linux.hpp b/src/slave/containerizer/isolators/filesystem/linux.hpp index b0016e5..99f939f 100644 --- a/src/slave/containerizer/isolators/filesystem/linux.hpp +++ b/src/slave/containerizer/isolators/filesystem/linux.hpp @@ -49,8 +49,6 @@ public: virtual ~LinuxFilesystemIsolatorProcess(); - virtual process::Future<Option<int>> namespaces(); - virtual process::Future<Nothing> recover( const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans); http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/filesystem/shared.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/filesystem/shared.cpp b/src/slave/containerizer/isolators/filesystem/shared.cpp index 4b4520e..73804ca 100644 --- a/src/slave/containerizer/isolators/filesystem/shared.cpp +++ b/src/slave/containerizer/isolators/filesystem/shared.cpp @@ -64,12 +64,6 @@ Try<Isolator*> SharedFilesystemIsolatorProcess::create(const Flags& flags) } -process::Future<Option<int>> SharedFilesystemIsolatorProcess::namespaces() -{ - return CLONE_NEWNS; -} - - Future<Nothing> SharedFilesystemIsolatorProcess::recover( const list<ContainerState>& states, const hashset<ContainerID>& orphans) @@ -108,6 +102,7 @@ Future<Option<ContainerPrepareInfo>> SharedFilesystemIsolatorProcess::prepare( containerPaths.insert(directory); ContainerPrepareInfo prepareInfo; + prepareInfo.set_namespaces(CLONE_NEWNS); foreach (const Volume& volume, executorInfo.container().volumes()) { // Because the filesystem is shared we require the container path http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/filesystem/shared.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/filesystem/shared.hpp b/src/slave/containerizer/isolators/filesystem/shared.hpp index a21bc79..3a2f7db 100644 --- a/src/slave/containerizer/isolators/filesystem/shared.hpp +++ b/src/slave/containerizer/isolators/filesystem/shared.hpp @@ -39,8 +39,6 @@ public: virtual ~SharedFilesystemIsolatorProcess(); - virtual process::Future<Option<int>> namespaces(); - virtual process::Future<Nothing> recover( const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans); http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/namespaces/pid.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/namespaces/pid.cpp b/src/slave/containerizer/isolators/namespaces/pid.cpp index 35cb664..a9823e0 100644 --- a/src/slave/containerizer/isolators/namespaces/pid.cpp +++ b/src/slave/containerizer/isolators/namespaces/pid.cpp @@ -121,12 +121,6 @@ Result<ino_t> NamespacesPidIsolatorProcess::getNamespace( } -process::Future<Option<int>> NamespacesPidIsolatorProcess::namespaces() -{ - return CLONE_NEWPID | CLONE_NEWNS; -} - - Future<Nothing> NamespacesPidIsolatorProcess::recover( const list<ContainerState>& states, const hashset<ContainerID>& orphans) @@ -166,6 +160,7 @@ Future<Option<ContainerPrepareInfo>> NamespacesPidIsolatorProcess::prepare( const Option<string>& user) { ContainerPrepareInfo prepareInfo; + prepareInfo.set_namespaces(CLONE_NEWPID | CLONE_NEWNS); // Mask the bind mount root directory in each container so // containers cannot see the namespace bind mount of other http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/namespaces/pid.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/namespaces/pid.hpp b/src/slave/containerizer/isolators/namespaces/pid.hpp index b22f5ba..87270d0 100644 --- a/src/slave/containerizer/isolators/namespaces/pid.hpp +++ b/src/slave/containerizer/isolators/namespaces/pid.hpp @@ -56,8 +56,6 @@ public: virtual ~NamespacesPidIsolatorProcess() {} - virtual process::Future<Option<int>> namespaces(); - virtual process::Future<Nothing> recover( const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans); http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/network/port_mapping.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/network/port_mapping.cpp b/src/slave/containerizer/isolators/network/port_mapping.cpp index 34ba229..e6bb75e 100644 --- a/src/slave/containerizer/isolators/network/port_mapping.cpp +++ b/src/slave/containerizer/isolators/network/port_mapping.cpp @@ -1619,17 +1619,6 @@ Try<Isolator*> PortMappingIsolatorProcess::create(const Flags& flags) } -process::Future<Option<int>> PortMappingIsolatorProcess::namespaces() -{ - // NOTE: the port mapping isolator itself doesn't require mount - // namespace. However, if mount namespace is enabled because of - // other isolators, we need to set mount sharing accordingly for - // PORT_MAPPING_BIND_MOUNT_ROOT to avoid races described in - // MESOS-1558. So we turn on mount namespace here for consistency. - return CLONE_NEWNET | CLONE_NEWNS; -} - - Future<Nothing> PortMappingIsolatorProcess::recover( const list<ContainerState>& states, const hashset<ContainerID>& orphans) @@ -2135,6 +2124,13 @@ Future<Option<ContainerPrepareInfo>> PortMappingIsolatorProcess::prepare( ContainerPrepareInfo prepareInfo; prepareInfo.add_commands()->set_value(scripts(infos[containerId])); + // NOTE: the port mapping isolator itself doesn't require mount + // namespace. However, if mount namespace is enabled because of + // other isolators, we need to set mount sharing accordingly for + // PORT_MAPPING_BIND_MOUNT_ROOT to avoid races described in + // MESOS-1558. So we turn on mount namespace here for consistency. + prepareInfo.set_namespaces(CLONE_NEWNET | CLONE_NEWNS); + return prepareInfo; } http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/isolators/network/port_mapping.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/network/port_mapping.hpp b/src/slave/containerizer/isolators/network/port_mapping.hpp index 4bca0b8..ae53c1b 100644 --- a/src/slave/containerizer/isolators/network/port_mapping.hpp +++ b/src/slave/containerizer/isolators/network/port_mapping.hpp @@ -152,8 +152,6 @@ public: virtual ~PortMappingIsolatorProcess() {} - virtual process::Future<Option<int>> namespaces(); - virtual process::Future<Nothing> recover( const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans); http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/slave/containerizer/mesos/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp index 3f0d8fb..e8a89bf 100644 --- a/src/slave/containerizer/mesos/containerizer.cpp +++ b/src/slave/containerizer/mesos/containerizer.cpp @@ -787,6 +787,7 @@ Future<bool> MesosContainerizerProcess::_launch( } JSON::Array commandArray; + int namespaces = 0; foreach (const Option<ContainerPrepareInfo>& prepareInfo, prepareInfos) { if (!prepareInfo.isSome()) { continue; @@ -805,6 +806,10 @@ Future<bool> MesosContainerizerProcess::_launch( environment[variable.name()] = variable.value(); } } + + if (prepareInfo.get().has_namespaces()) { + namespaces |= prepareInfo.get().namespaces(); + } } // TODO(jieyu): Use JSON::Array once we have generic parse support. @@ -830,15 +835,6 @@ Future<bool> MesosContainerizerProcess::_launch( launchFlags.pipe_write = pipes[1]; launchFlags.commands = commands; - // TODO(karya): Create ContainerPrepareInfo.namespaces and use that instead of - // Isolator::namespaces(). - int namespaces = 0; - foreach (const Owned<Isolator>& isolator, isolators) { - if (isolator->namespaces().get().isSome()) { - namespaces |= isolator->namespaces().get().get(); - } - } - // Fork the child using launcher. vector<string> argv(2); argv[0] = MESOS_CONTAINERIZER; http://git-wip-us.apache.org/repos/asf/mesos/blob/6923bb3e/src/tests/containerizer/isolator_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/isolator_tests.cpp b/src/tests/containerizer/isolator_tests.cpp index 1d0d41b..a25ae97 100644 --- a/src/tests/containerizer/isolator_tests.cpp +++ b/src/tests/containerizer/isolator_tests.cpp @@ -470,11 +470,14 @@ TEST_F(LimitedCpuIsolatorTest, ROOT_CGROUPS_Cfs) Try<string> dir = os::mkdtemp(path::join(os::getcwd(), "XXXXXX")); ASSERT_SOME(dir); - AWAIT_READY(isolator.get()->prepare( - containerId, - executorInfo, - dir.get(), - None())); + Future<Option<ContainerPrepareInfo>> prepare = + isolator.get()->prepare( + containerId, + executorInfo, + dir.get(), + None()); + + AWAIT_READY(prepare); // Generate random numbers to max out a single core. We'll run this for 0.5 // seconds of wall time so it should consume approximately 250 ms of total @@ -503,7 +506,7 @@ TEST_F(LimitedCpuIsolatorTest, ROOT_CGROUPS_Cfs) None(), None(), lambda::bind(&childSetup, pipes), - isolator.get()->namespaces().get()); + prepare.get().isSome() ? prepare.get().get().namespaces() : 0); ASSERT_SOME(pid); @@ -581,11 +584,14 @@ TEST_F(LimitedCpuIsolatorTest, ROOT_CGROUPS_Cfs_Big_Quota) Try<string> dir = os::mkdtemp(path::join(os::getcwd(), "XXXXXX")); ASSERT_SOME(dir); - AWAIT_READY(isolator.get()->prepare( - containerId, - executorInfo, - dir.get(), - None())); + Future<Option<ContainerPrepareInfo>> prepare = + isolator.get()->prepare( + containerId, + executorInfo, + dir.get(), + None()); + + AWAIT_READY(prepare); int pipes[2]; ASSERT_NE(-1, ::pipe(pipes)); @@ -605,7 +611,7 @@ TEST_F(LimitedCpuIsolatorTest, ROOT_CGROUPS_Cfs_Big_Quota) None(), None(), lambda::bind(&childSetup, pipes), - isolator.get()->namespaces().get()); + prepare.get().isSome() ? prepare.get().get().namespaces() : 0); ASSERT_SOME(pid); @@ -664,11 +670,14 @@ TEST_F(LimitedCpuIsolatorTest, ROOT_CGROUPS_Pids_and_Tids) Try<string> dir = os::mkdtemp(path::join(os::getcwd(), "XXXXXX")); ASSERT_SOME(dir); - AWAIT_READY(isolator.get()->prepare( - containerId, - executorInfo, - dir.get(), - None())); + Future<Option<ContainerPrepareInfo>> prepare = + isolator.get()->prepare( + containerId, + executorInfo, + dir.get(), + None()); + + AWAIT_READY(prepare); // Right after the creation of the cgroup, which happens in // 'prepare', we check that it is empty. @@ -695,7 +704,7 @@ TEST_F(LimitedCpuIsolatorTest, ROOT_CGROUPS_Pids_and_Tids) None(), None(), lambda::bind(&childSetup, pipes), - isolator.get()->namespaces().get()); + prepare.get().isSome() ? prepare.get().get().namespaces() : 0); ASSERT_SOME(pid); @@ -955,6 +964,7 @@ TEST_F(SharedFilesystemIsolatorTest, DISABLED_ROOT_RelativeVolume) AWAIT_READY(prepare); ASSERT_SOME(prepare.get()); ASSERT_EQ(1, prepare.get().get().commands().size()); + EXPECT_TRUE(prepare.get().get().has_namespaces()); // The test will touch a file in container path. const string file = path::join(containerPath, UUID::random().toString()); @@ -978,7 +988,7 @@ TEST_F(SharedFilesystemIsolatorTest, DISABLED_ROOT_RelativeVolume) None(), None(), None(), - isolator.get()->namespaces().get()); + prepare.get().get().namespaces()); ASSERT_SOME(pid); // Set up the reaper to wait on the forked child. @@ -1059,6 +1069,7 @@ TEST_F(SharedFilesystemIsolatorTest, DISABLED_ROOT_AbsoluteVolume) AWAIT_READY(prepare); ASSERT_SOME(prepare.get()); ASSERT_EQ(1, prepare.get().get().commands().size()); + EXPECT_TRUE(prepare.get().get().has_namespaces()); // Test the volume mounting by touching a file in the container's // /tmp, which should then be in flags.work_dir. @@ -1083,7 +1094,7 @@ TEST_F(SharedFilesystemIsolatorTest, DISABLED_ROOT_AbsoluteVolume) None(), None(), None(), - isolator.get()->namespaces().get()); + prepare.get().get().namespaces()); ASSERT_SOME(pid); // Set up the reaper to wait on the forked child.
