You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2016/03/17 04:39:06 UTC

Review Request 44949: Add XFS disk isolator tests.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44949/
-----------------------------------------------------------

Review request for mesos, Jie Yu and Jiang Yan Xu.


Bugs: MESOS-4828
    https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
-------

Add basic XFS disk isolator tests by cloning the POSIX disk isolator
tests and making minor changes for the differences in semantics.


Diffs
-----

  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44949/diff/


Testing
-------

Make check. Manual testing.


Thanks,

James Peach

Re: Review Request 44949: Add XFS disk isolator tests.

Posted by Jiang Yan Xu <ya...@jxu.me>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44949/#review127471
-----------------------------------------------------------




src/tests/containerizer/xfs_quota_tests.cpp (line 338)
<https://reviews.apache.org/r/44949/#comment190850>

    These more complex and integration oriented tests should all have a header comment so it makes it easier for 
    
    Something like:
    ```
    // This test verifies that a task fails to write to disk when it 
    // exceeds the requested disk quota.
    ```



src/tests/containerizer/xfs_quota_tests.cpp (line 345)
<https://reviews.apache.org/r/44949/#comment190811>

    Two spaces to continue after `=`.
    
    Here and elsewhere in this test.



src/tests/containerizer/xfs_quota_tests.cpp (line 357)
<https://reviews.apache.org/r/44949/#comment190812>

    One space between `;` and `//`.
    
    I saw some other test code that does this but it's probably due to copying and pasting of bad examples. Unless it's to align with some other comments, use one space.
    
    Here and elsewhere.



src/tests/containerizer/xfs_quota_tests.cpp (line 390)
<https://reviews.apache.org/r/44949/#comment190818>

    s/a/an.



src/tests/containerizer/xfs_quota_tests.cpp (line 425)
<https://reviews.apache.org/r/44949/#comment190837>

    `s/Future<vector<Offer> >/Future<vector<Offer>>/` here and elsewhere.



src/tests/containerizer/xfs_quota_tests.cpp (line 461)
<https://reviews.apache.org/r/44949/#comment190838>

    `Timeout::in()` and `Timeout::expired()` is more suitable for this.



src/tests/containerizer/xfs_quota_tests.cpp (line 486)
<https://reviews.apache.org/r/44949/#comment190851>

    With Timeout you don't need to do this.



src/tests/containerizer/xfs_quota_tests.cpp (lines 526 - 531)
<https://reviews.apache.org/r/44949/#comment190917>

    Here we seem to be implying that we are expecting the task to die because it exceeds the disk quota but in fact we are just testing slave recovery and the task doesn't die because of the `;`.
    
    How about we just request 1MB and use 1MB (or not use anything at all (just sleep) since we are mainly verifying if the project ID is cleared afterwards.



src/tests/containerizer/xfs_quota_tests.cpp (line 545)
<https://reviews.apache.org/r/44949/#comment190852>

    Two spaces.



src/tests/containerizer/xfs_quota_tests.cpp (line 551)
<https://reviews.apache.org/r/44949/#comment190915>

    This is not guaranteed if `dd` is slow to write to the disk and the rest of the test proceeds quickly.
    
    I think it's OK if we don't verify this here because the two tests above already verified that disk usage is correctly reported and correctly capped at the limit.



src/tests/containerizer/xfs_quota_tests.cpp (lines 565 - 566)
<https://reviews.apache.org/r/44949/#comment190857>

    This is not necessary if we
    
    ```
    AWAIT_READY(slaveReregisteredMessage);
    ```
    
    because reregistration only happens after `Slave::_recover`.



src/tests/containerizer/xfs_quota_tests.cpp (line 590)
<https://reviews.apache.org/r/44949/#comment190925>

    Additionally we should assert there is exactly one container. Otherwise the foreach below can fall through without triggering any expectations if `sandboxees` is empty.



src/tests/containerizer/xfs_quota_tests.cpp (line 593)
<https://reviews.apache.org/r/44949/#comment190926>

    `s/foreach(/foreach (/`



src/tests/containerizer/xfs_quota_tests.cpp (lines 680 - 691)
<https://reviews.apache.org/r/44949/#comment190922>

    This part is not necessary. Slave considers itself fully recovered when the containers are fully recovered, not when the executors have all reregistered. Also the exchange of reconnect and reregister messages doesn't go through a delay so no need to advance (only the `reregisterExecutorTimeout` goes through a delay).
    
    Simply removing these lines should do it.



src/tests/containerizer/xfs_quota_tests.cpp (line 702)
<https://reviews.apache.org/r/44949/#comment190923>

    This is again not guaranteed. I think it's good enough if we just verify the limits.



src/tests/containerizer/xfs_quota_tests.cpp (line 718)
<https://reviews.apache.org/r/44949/#comment190927>

    `s/foreach(/foreach (/`


- Jiang Yan Xu


On April 6, 2016, 3:37 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44949/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 3:37 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
>     https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add basic XFS disk isolator tests by cloning the POSIX disk isolator
> tests and making minor changes for the differences in semantics.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44949/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44949: Add XFS disk isolator tests.

Posted by Jiang Yan Xu <ya...@jxu.me>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44949/#review127881
-----------------------------------------------------------


Ship it!




Ship It!

- Jiang Yan Xu


On April 7, 2016, 2:50 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44949/
> -----------------------------------------------------------
> 
> (Updated April 7, 2016, 2:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
>     https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add basic XFS disk isolator tests by cloning the POSIX disk isolator
> tests and making minor changes for the differences in semantics.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44949/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>