----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54449/#review168207 -----------------------------------------------------------
Fix it, then Ship it! The test refactor looks great! src/tests/containerizer/xfs_quota_tests.cpp Line 82 (original), 82 (patched) <https://reviews.apache.org/r/54449/#comment240402> How about ROOT_XFS_TestBase here and use the name ROOT_XFS_QuotaTest for the "standard test" below? Although not required by gtest, it's a convention to end test fixtures with Test or TestBase (the latter when it is purely a base class with no tests of itself). src/tests/containerizer/xfs_quota_tests.cpp Lines 85 (patched) <https://reviews.apache.org/r/54449/#comment240403> s/quotaOpt/quotaOptions/ (full spelling) (or s/quotaOpt/xfsOptions/ which is more generic since this class is just a base class) src/tests/containerizer/xfs_quota_tests.cpp Lines 227 (patched) <https://reviews.apache.org/r/54449/#comment240785> s/a/an/ here and below? src/tests/containerizer/xfs_quota_tests.cpp Lines 229 (patched) <https://reviews.apache.org/r/54449/#comment240399> IMO `ROOT_XFS_NoQuota` reads better: it's literally the mount option and it's pretty clear. :) src/tests/containerizer/xfs_quota_tests.cpp Lines 237 (patched) <https://reviews.apache.org/r/54449/#comment240394> s/oeoject/project/? src/tests/containerizer/xfs_quota_tests.cpp Lines 238 (patched) <https://reviews.apache.org/r/54449/#comment240396> The comment says `ROOT_XFS_QuotaNoProj` but here it's `ROOT_XFS_QuotaOther`. IMO `ROOT_XFS_NoProjectQuota` is more straighforward than "QuotaOther". src/tests/containerizer/xfs_quota_tests.cpp Lines 916 (patched) <https://reviews.apache.org/r/54449/#comment240781> Testing `xfs::isQuotaEnabled` is approximate, but the most direct test would be on `XfsDiskIsolatorProcess::create` or even `MesosContainerizer::create`? - Jiang Yan Xu On March 6, 2017, 11:10 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54449/ > ----------------------------------------------------------- > > (Updated March 6, 2017, 11:10 a.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-6732 > https://issues.apache.org/jira/browse/MESOS-6732 > > > Repository: mesos > > > Description > ------- > > 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. > > > Diffs > ----- > > configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671 > src/slave/containerizer/mesos/isolators/xfs/disk.cpp > dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 > src/slave/containerizer/mesos/isolators/xfs/utils.hpp > 7602fe3b6ab069db643397418732e773d0417f8a > src/slave/containerizer/mesos/isolators/xfs/utils.cpp > b9d8e7dc999ba3064bee7105eff0f9553d825df8 > src/tests/containerizer/xfs_quota_tests.cpp > 0fbaddd68af55c51c106962377be20afa599fb97 > > > Diff: https://reviews.apache.org/r/54449/diff/4/ > > > Testing > ------- > > Make check on Fedora 25. Manual test on F25 with mesos-execute. > > > Thanks, > > James Peach > >
