----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54449/#review158517 -----------------------------------------------------------
Can we add a simple test for `isQuotaEnabled()` in ROOT_XFS_QuotaTest? configure.ac (line 1768) <https://reviews.apache.org/r/54449/#comment229535> Is this change for `Q_XGETQSTATV` and `FS_QSTATV_VERSION1`? At least explain this (and the change of variable names XFS_PROJ_QUOTA -> FS_PROJ_QUOTA in the review summary? src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 110 - 120) <https://reviews.apache.org/r/54449/#comment229537> We check ``` if (!xfs::pathIsXfs(flags.work_dir)) { return Error("'" + flags.work_dir + "' is not an XFS filesystem"); } ``` in `XfsDiskIsolatorProcess::create`. Seems like the similar check should go there if we want to "fail fast". src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 112 - 113) <https://reviews.apache.org/r/54449/#comment229538> Indent like ``` return Error( "Failed to get quota status for '" + flags.work_dir + "': " + enabled.error()); ``` or ``` return Error("Failed to get quota status for '" + flags.work_dir + "': " + enabled.error()); ``` src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 52) <https://reviews.apache.org/r/54449/#comment229536> Would `isQuotaEnabled` be a better name? It's obvious that the method above has set up the example but I guess `isPathXfs` would have been a better name too? src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 406) <https://reviews.apache.org/r/54449/#comment229539> Remove redundant space: ``` struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0}; ``` What's the `0` here? Do we need it? If we do, comment on it? src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 420) <https://reviews.apache.org/r/54449/#comment229540> We need a brief comment about "both accounting mode and enforcement mode count as quota enabled" but it probalby can go into the method declaration comment. - Jiang Yan Xu On Dec. 6, 2016, 1:21 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54449/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2016, 1:21 p.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 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4 > 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 > > Diff: https://reviews.apache.org/r/54449/diff/ > > > Testing > ------- > > Make check on Fedora 25. Manual test on F25 with mesos-execute. > > > Thanks, > > James Peach > >
