mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 55897: Add support for not enforcing XFS quotas.
Date Mon, 01 May 2017 05:38:52 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message