Repository: mesos Updated Branches: refs/heads/master cfb2c1660 -> 3593dd76e
Check quotas are enabled in the XFS disk isolator. The XFS disk isolator checks that the filesystem is XFS, but doesn't check whether project quotas are actually enabled. This means that an invalid configuration will start but will always fail when tasks are launched. Add a check to test whether project quotas are enabled on the work directory and fail hard if they are not. Review: https://reviews.apache.org/r/54449/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/3593dd76 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/3593dd76 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/3593dd76 Branch: refs/heads/master Commit: 3593dd76eb96bfd6fc9977611442ad95fcd8316a Parents: cfb2c16 Author: James Peach <[email protected]> Authored: Fri Mar 10 10:51:59 2017 -0800 Committer: Jiang Yan Xu <[email protected]> Committed: Fri Mar 10 11:02:27 2017 -0800 ---------------------------------------------------------------------- configure.ac | 2 +- .../containerizer/mesos/isolators/xfs/disk.cpp | 19 +++++- .../containerizer/mesos/isolators/xfs/utils.cpp | 38 ++++++++++-- .../containerizer/mesos/isolators/xfs/utils.hpp | 8 ++- src/tests/containerizer/xfs_quota_tests.cpp | 61 +++++++++++++++++++- 5 files changed, 117 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/3593dd76/configure.ac ---------------------------------------------------------------------- diff --git a/configure.ac b/configure.ac index 1e47bab..090783a 100644 --- a/configure.ac +++ b/configure.ac @@ -1789,7 +1789,7 @@ The XFS disk isolator is only supported on Linux. # Check for build dependencies for the XFS disk isolator. We only # enable this if all the needed headers and libraries are present. - AC_CHECK_HEADERS([xfs/xfs.h xfs/xqm.h linux/quota.h sys/quota.h], + AC_CHECK_HEADERS([xfs/xfs.h linux/dqblk_xfs.h linux/quota.h sys/quota.h], [], [AC_MSG_ERROR([missing XFS quota headers ------------------------------------------------------------------- Please install the Linux kernel headers and xfsprogs development http://git-wip-us.apache.org/repos/asf/mesos/blob/3593dd76/src/slave/containerizer/mesos/isolators/xfs/disk.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/xfs/disk.cpp b/src/slave/containerizer/mesos/isolators/xfs/disk.cpp index dd4df86..40f1049 100644 --- a/src/slave/containerizer/mesos/isolators/xfs/disk.cpp +++ b/src/slave/containerizer/mesos/isolators/xfs/disk.cpp @@ -103,10 +103,23 @@ static Option<Bytes> getDiskResource( Try<Isolator*> XfsDiskIsolatorProcess::create(const Flags& flags) { - if (!xfs::pathIsXfs(flags.work_dir)) { + if (!xfs::isPathXfs(flags.work_dir)) { return Error("'" + flags.work_dir + "' is not an XFS filesystem"); } + Try<bool> enabled = xfs::isQuotaEnabled(flags.work_dir); + if (enabled.isError()) { + return Error( + "Failed to get quota status for '" + + flags.work_dir + "': " + enabled.error()); + } + + if (!enabled.get()) { + return Error( + "XFS project quotas are not enabled on '" + + flags.work_dir + "'"); + } + Result<uid_t> uid = os::getuid(); CHECK_SOME(uid) << "getuid(2) doesn't fail"; @@ -120,8 +133,7 @@ Try<Isolator*> XfsDiskIsolatorProcess::create(const Flags& flags) if (projects.isError()) { return Error( "Failed to parse XFS project range '" + - flags.xfs_project_range + - "'"); + flags.xfs_project_range + "'"); } if (projects.get().type() != Value::RANGES) { @@ -419,6 +431,7 @@ Option<prid_t> XfsDiskIsolatorProcess::nextProjectId() return projectId; } + void XfsDiskIsolatorProcess::returnProjectId( prid_t projectId) { http://git-wip-us.apache.org/repos/asf/mesos/blob/3593dd76/src/slave/containerizer/mesos/isolators/xfs/utils.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/xfs/utils.cpp b/src/slave/containerizer/mesos/isolators/xfs/utils.cpp index b9d8e7d..8018ad3 100644 --- a/src/slave/containerizer/mesos/isolators/xfs/utils.cpp +++ b/src/slave/containerizer/mesos/isolators/xfs/utils.cpp @@ -23,7 +23,6 @@ // use textdomain(), compilation errors ensue. #define ENABLE_GETTEXT #include <xfs/xfs.h> -#include <xfs/xqm.h> #undef ENABLE_GETTEXT // xfs/platform_defs-x86_64.h defines min() and max() macros which conflict @@ -34,6 +33,7 @@ #include <fts.h> #include <blkid/blkid.h> +#include <linux/dqblk_xfs.h> #include <linux/quota.h> #include <sys/quota.h> @@ -152,7 +152,7 @@ static Try<Nothing> setProjectQuota( // Specify that we are setting a project quota for this ID. quota.d_id = projectId; - quota.d_flags = XFS_PROJ_QUOTA; + quota.d_flags = FS_PROJ_QUOTA; // Set both the hard and the soft limit to the same quota, just // for consistency. Functionally all we need is the hard quota. @@ -229,7 +229,7 @@ Result<QuotaInfo> getProjectQuota( quota.d_version = FS_DQUOT_VERSION; quota.d_id = projectId; - quota.d_flags = XFS_PROJ_QUOTA; + quota.d_flags = FS_PROJ_QUOTA; // In principle, we should issue a Q_XQUOTASYNC to get an accurate accounting. // However, we don't want to affect performance by continually syncing the @@ -390,11 +390,41 @@ Option<Error> validateProjectIds(const IntervalSet<prid_t>& projectRange) } -bool pathIsXfs(const string& path) +bool isPathXfs(const string& path) { return ::platform_test_xfs_path(path.c_str()) == 1; } + +Try<bool> isQuotaEnabled(const string& path) +{ + Try<string> devname = getDeviceForPath(path); + if (devname.isError()) { + return Error(devname.error()); + } + + struct fs_quota_statv statv = {FS_QSTATV_VERSION1}; + + // The quota `type` argument to QCMD() doesn't apply to QCMD_XGETQSTATV + // since it is for quota subsystem information that can include all + // types of quotas. Equally, the quotactl() `id` argument doesn't apply + // because we are getting global information rather than information for + // a specific identity (eg. a projectId). + if (::quotactl(QCMD(Q_XGETQSTATV, 0), + devname.get().c_str(), + 0, // id + reinterpret_cast<caddr_t>(&statv)) == -1) { + // ENOSYS means that quotas are not enabled at all. + if (errno == ENOSYS) { + return false; + } + + return ErrnoError(); + } + + return statv.qs_flags & (FS_QUOTA_PDQ_ACCT | FS_QUOTA_PDQ_ENFD); +} + } // namespace xfs { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/3593dd76/src/slave/containerizer/mesos/isolators/xfs/utils.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/xfs/utils.hpp b/src/slave/containerizer/mesos/isolators/xfs/utils.hpp index 7602fe3..eddd4c3 100644 --- a/src/slave/containerizer/mesos/isolators/xfs/utils.hpp +++ b/src/slave/containerizer/mesos/isolators/xfs/utils.hpp @@ -46,7 +46,13 @@ inline bool operator==(const QuotaInfo& left, const QuotaInfo& right) Option<Error> validateProjectIds(const IntervalSet<prid_t>& projectRange); -bool pathIsXfs(const std::string& path); +bool isPathXfs(const std::string& path); + + +// Test whether XFS project quotas are enabled on the filesystem at the +// given path. This does not imply that quotas are being enforced, just +// that they are enabled. +Try<bool> isQuotaEnabled(const std::string& path); Result<QuotaInfo> getProjectQuota( http://git-wip-us.apache.org/repos/asf/mesos/blob/3593dd76/src/tests/containerizer/xfs_quota_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/xfs_quota_tests.cpp b/src/tests/containerizer/xfs_quota_tests.cpp index 0fbaddd..bb19311 100644 --- a/src/tests/containerizer/xfs_quota_tests.cpp +++ b/src/tests/containerizer/xfs_quota_tests.cpp @@ -42,6 +42,7 @@ #include "slave/containerizer/fetcher.hpp" #include "slave/containerizer/mesos/containerizer.hpp" +#include "slave/containerizer/mesos/isolators/xfs/disk.hpp" #include "slave/containerizer/mesos/isolators/xfs/utils.hpp" #include "tests/environment.hpp" @@ -64,6 +65,7 @@ using mesos::internal::slave::Fetcher; using mesos::internal::slave::MesosContainerizer; using mesos::internal::slave::MesosContainerizerProcess; using mesos::internal::slave::Slave; +using mesos::internal::slave::XfsDiskIsolatorProcess; using mesos::master::detector::MasterDetector; @@ -79,9 +81,12 @@ static QuotaInfo makeQuotaInfo( } -class ROOT_XFS_QuotaTest : public MesosTest +class ROOT_XFS_TestBase : public MesosTest { public: + explicit ROOT_XFS_TestBase(const string& _xfsOptions) + : xfsOptions(_xfsOptions) {} + virtual void SetUp() { MesosTest::SetUp(); @@ -126,7 +131,7 @@ public: mntPath, "xfs", 0, // Flags. - "prjquota")); + xfsOptions)); mountPoint = mntPath; ASSERT_SOME(os::chdir(mountPoint.get())) @@ -206,11 +211,39 @@ public: return string("/dev/loop") + stringify(devno); } + string xfsOptions; Option<string> loopDevice; // The loop device we attached. Option<string> mountPoint; // XFS filesystem mountpoint. }; +// ROOT_XFS_QuotaTest is our standard fixture that sets up a +// XFS filesystem on loopback with project quotas enabled. +class ROOT_XFS_QuotaTest : public ROOT_XFS_TestBase +{ +public: + ROOT_XFS_QuotaTest() : ROOT_XFS_TestBase("prjquota") {} +}; + + +// ROOT_XFS_NoQuota sets up an XFS filesystem on loopback +// with no quotas enabled. +class ROOT_XFS_NoQuota : public ROOT_XFS_TestBase +{ +public: + ROOT_XFS_NoQuota() : ROOT_XFS_TestBase("noquota") {} +}; + + +// ROOT_XFS_NoProjectQuota sets up an XFS filesystem on loopback +// with all the quota types except project quotas enabled. +class ROOT_XFS_NoProjectQuota : public ROOT_XFS_TestBase +{ +public: + ROOT_XFS_NoProjectQuota() : ROOT_XFS_TestBase("usrquota,grpquota") {} +}; + + TEST_F(ROOT_XFS_QuotaTest, QuotaGetSet) { prid_t projectId = 44; @@ -878,6 +911,30 @@ TEST_F(ROOT_XFS_QuotaTest, IsolatorFlags) ASSERT_ERROR(StartSlave(detector.get(), flags)); } + +// Verify that we correctly detect when quotas are not enabled at all. +TEST_F(ROOT_XFS_NoQuota, CheckQuotaEnabled) +{ + EXPECT_SOME_EQ(false, xfs::isQuotaEnabled(mountPoint.get())); + EXPECT_ERROR(XfsDiskIsolatorProcess::create(CreateSlaveFlags())); +} + + +// Verify that we correctly detect when quotas are enabled but project +// quotas are not enabled. +TEST_F(ROOT_XFS_NoProjectQuota, CheckQuotaEnabled) +{ + EXPECT_SOME_EQ(false, xfs::isQuotaEnabled(mountPoint.get())); + EXPECT_ERROR(XfsDiskIsolatorProcess::create(CreateSlaveFlags())); +} + + +// Verify that we correctly detect that project quotas are enabled. +TEST_F(ROOT_XFS_QuotaTest, CheckQuotaEnabled) +{ + EXPECT_SOME_EQ(true, xfs::isQuotaEnabled(mountPoint.get())); +} + } // namespace tests { } // namespace internal { } // namespace mesos {
