----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55897/#review173432 -----------------------------------------------------------
LGTM! I also verified that setting limits to zero doesn't lead to the quota accounting system to be turned off. src/slave/containerizer/mesos/isolators/xfs/disk.cpp Lines 334 (patched) <https://reviews.apache.org/r/55897/#comment246424> Too much indentation. src/slave/containerizer/mesos/isolators/xfs/disk.cpp Lines 325-326 (original), 340-343 (patched) <https://reviews.apache.org/r/55897/#comment246425> Use a `if ... else if` here? You have two exclusive cases here, we can make it explicit. Perhpas a `switch` wouldn't overkill either. src/slave/containerizer/mesos/isolators/xfs/disk.cpp Line 327 (original), 344 (patched) <https://reviews.apache.org/r/55897/#comment246450> What happens if we were in accounting mode but now in enforce mode? Should we update the quota (this can potentially kill existing containers) even if it's the same? What does the posix isolator do? src/slave/containerizer/mesos/isolators/xfs/disk.cpp Lines 346 (patched) <https://reviews.apache.org/r/55897/#comment246426> Indentation. src/slave/containerizer/mesos/isolators/xfs/disk.cpp Line 339 (original), 355 (patched) <https://reviews.apache.org/r/55897/#comment246438> `info->quota` is no longer what you set to now that you've moved the assignment statement. src/tests/containerizer/xfs_quota_tests.cpp Line 259 (original), 258-259 (patched) <https://reviews.apache.org/r/55897/#comment246439> Revert it? src/tests/containerizer/xfs_quota_tests.cpp Line 443 (original), 443 (patched) <https://reviews.apache.org/r/55897/#comment246440> A brief comment? To reduce redundancy you can point to the similar test: "Similar to `DiskUsageExceedsQuota` above but the task doesn't fail in this test due to quota not being enforced". src/tests/containerizer/xfs_quota_tests.cpp Lines 444 (patched) <https://reviews.apache.org/r/55897/#comment246441> s/DiskUsageNoEnforce/DiskUsageExceedsQuotaNoEnforce/ ? "ExceedsQuota" is kind of important for summarizing the test IMO. src/tests/containerizer/xfs_quota_tests.cpp Line 513 (original), 574 (patched) <https://reviews.apache.org/r/55897/#comment246442> It will always have the limit? src/tests/containerizer/xfs_quota_tests.cpp Lines 601 (patched) <https://reviews.apache.org/r/55897/#comment246443> Comment? Something that says "Verify that disk usage statistic still works even without enforcement"? src/tests/containerizer/xfs_quota_tests.cpp Lines 632 (patched) <https://reviews.apache.org/r/55897/#comment246444> Use just one space before the comment. src/tests/containerizer/xfs_quota_tests.cpp Lines 641 (patched) <https://reviews.apache.org/r/55897/#comment246447> Comment like the one above `ResourceStatistics` but make clear we expect all 4MBs to be successfully written? src/tests/containerizer/xfs_quota_tests.cpp Lines 668-670 (patched) <https://reviews.apache.org/r/55897/#comment246446> It will always have the limit? src/tests/containerizer/xfs_quota_tests.cpp Lines 674-675 (patched) <https://reviews.apache.org/r/55897/#comment246448> An expectation more fitting than LOG(INFO)? The loop not ending up in ASSERT_FAILURE already means it has succeeded but to be more explicit, we can put ``` EXPECT_LE(usage->disk_used_bytes(), Megabytes(4).bytes()); ``` below the loop? I think that's what you wanted to express with the log but this wouldn't require a human to read the test logs? src/tests/containerizer/xfs_quota_tests.cpp Lines 680 (patched) <https://reviews.apache.org/r/55897/#comment246449> On the subject of expectation messages, the following is what I consider useful message to append to the expectation because I'd like to know what `usage->disk_used_bytes()` is even if I know it's below `usage->has_disk_limit_bytes()`: it being zero vs. not may help me debug. ``` ASSERT_FALSE(timeout.expired()) << "Disk usage failed to reach " << usage->has_disk_limit_bytes() << " when timeout expires, current value: " << usage->disk_used_bytes(); ``` - Jiang Yan Xu On April 25, 2017, 3:37 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55897/ > ----------------------------------------------------------- > > (Updated April 25, 2017, 3:37 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-5116 > https://issues.apache.org/jira/browse/MESOS-5116 > > > Repository: mesos > > > Description > ------- > > Add XFS disk isolator support for not enforcing disk quotas on > containers. While there is a global filesystem configuration option > to turn off quota enforcement, we should not automatically toggle > that because we don't know why the operator might have changed that > configuration. Instead, we just apply an unlimited (0) quota, which > engages XFS space accounting without enforcing any limit. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/xfs/disk.hpp > 52f0459421a45b01ce38b17c689633301cd97982 > src/slave/containerizer/mesos/isolators/xfs/disk.cpp > 40f1049358ad99d3f213289e36def81c580f07f3 > src/slave/containerizer/mesos/isolators/xfs/utils.hpp > eddd4c37fb42339ca21ecb392dea47acf6b277bb > src/tests/containerizer/xfs_quota_tests.cpp > 7beb60b059910a0d4451b1ace895a35dc974a043 > > > Diff: https://reviews.apache.org/r/55897/diff/5/ > > > Testing > ------- > > sudo make check (Fedora 25) > > > Thanks, > > James Peach > >
