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/04/01 02:01:24 UTC

Re: Review Request 44948: Add XFS disk resource isolator.

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

(Updated April 1, 2016, 12:01 a.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Track sandbox directory usage by dynamically assigning XFS project
quotas. We track a range of XFS project IDs, assigning a project ID
and a project quota to each sandbox as it is created. When the task
reaches the quota, writes will fail with EDQUOT, and the task will have
an opportunity to handle that.

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2
src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0
src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION
src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04
src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 7, 2016, 9:50 p.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

src/Makefile.am 71c4308cccfa5c56b93f6c3928dd2a1cf3ba9741
src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0
src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION
src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6
src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

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

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


Ship it!




Ship It!

- Jiang Yan Xu


On April 6, 2016, 3:36 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 3:36 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
> -------
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ba9cc8b683bba2ae8fe9d9c58642690f5b80afaf 
>   src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
>   src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 6, 2016, 10:36 p.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

src/Makefile.am ba9cc8b683bba2ae8fe9d9c58642690f5b80afaf
src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0
src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION
src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe
src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.


> On April 6, 2016, 9:07 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp, line 407
> > <https://reviews.apache.org/r/44948/diff/15/?file=1327115#file1327115line407>
> >
> >     Should we assume that freeProjectId already doesn't include `info->projectId`?

No, ``freeProjectIds`` is initialized from ``totalProjectIds``, so we have to remove it because it is still considered in use.


- James


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


On April 6, 2016, 6:43 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 6:43 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
> -------
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
>   src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
>   src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

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

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



Mostly LGTM. Just a few additional comments.


src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 21 - 22)
<https://reviews.apache.org/r/44948/#comment190657>

    Not used anymore.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 26)
<https://reviews.apache.org/r/44948/#comment190784>

    This is not used.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 28 - 30)
<https://reviews.apache.org/r/44948/#comment190658>

    One blank line above and below as per convention.
    
    And I don't think any of the three are still used?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 31)
<https://reviews.apache.org/r/44948/#comment190783>

    This is not used either.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 38 - 39)
<https://reviews.apache.org/r/44948/#comment190656>

    Not used anymore.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 63 - 66)
<https://reviews.apache.org/r/44948/#comment190679>

    There is some awkwardness with the -1: we don't use the last element in the range so the check needs to be `ranges.range(i).end() - 1` as well.
    
    However, I am thinking if we can just use the inclusive right bounds. It seems to me the open right bound is because of the empty interval set check, see my comments below.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 69)
<https://reviews.apache.org/r/44948/#comment190680>

    s/open/closed/ if we agree on the comment on `nextProjectId()`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 116)
<https://reviews.apache.org/r/44948/#comment190694>

    We should always add a CHECK if we expect it to never be none. In fact it'll never be error either because it simply returns `::getuid()`.
    
    It's just being clear that we intentionally don't check other conditions: `CHECK_SOME(uid);`, Kinda like "This page is intentionally left blank".
    
    At this point it seems there is not much value to use `os::getuid()` rather than `::getuid()` but let's just keep the use of `os::getuid()` with this CHECK for the sake of "There should be one-- and preferably only one --obvious way" to get uid.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 199)
<https://reviews.apache.org/r/44948/#comment190699>

    s/known/alive/
    
    Otherwise it sounds like it encompasses both alive and known orphans and we should merge it with `orphans`. Keeping them separate is more explict so we should keep `alive` IMO.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 214 - 217)
<https://reviews.apache.org/r/44948/#comment190705>

    Unless there is a collision in UUID this should never happen. I trust UUID but we can do a CHECK here instead. :)
    
    ```
    CHECK(!infos.contains(containerId)) << "ContainerIDs should never collide";
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 221)
<https://reviews.apache.org/r/44948/#comment190731>

    Add a comment here. Somthing along the lines of:
    
    ```
    // We fail the isolator recovery upon failure in any container because
    // failing to get the project ID usually suggests some fatal issue
    // on the host.
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 234)
<https://reviews.apache.org/r/44948/#comment190734>

    s/message/call later/ because it's not a 'message'.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 236)
<https://reviews.apache.org/r/44948/#comment190762>

    Add 
    
    ```
    // Note that we don't wait for the result of the cleanups as we don't 
    // want to block agent recovery for unknown orphans.
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 406 - 413)
<https://reviews.apache.org/r/44948/#comment190782>

    We make a best effort to cleanup but we should still propagate the failure to the containerizer so it can do some containerizer-wide logging and metrics update.
    
    How about:
    ```
    infos.erase(containerId);
    
    if (quotaStatus.isError() || projectStatus.isError()) {
      return Failure("Failed to cleanup '" + info->directory + "'");
    } else {
      returnProjectId(info->projectId);
      return Nothing();
    }
    ```
    
    ?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 407)
<https://reviews.apache.org/r/44948/#comment190771>

    Should we assume that freeProjectId already doesn't include `info->projectId`?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 419 - 430)
<https://reviews.apache.org/r/44948/#comment190678>

    This seems better:
    
    ```
    if (freeProjectIds.empty()) {
      return None();
    }
    
    prid_t projectId = freeProjectIds.begin()->lower();
    freeProjectIds -= projectId;
    return projectId;
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 420 - 422)
<https://reviews.apache.org/r/44948/#comment190672>

    Took another look at this: it seems that this won't happen. A few tests I wrote (plus the ones already in interval_tests.cpp show that empty intervals get eliminated automatically.



src/slave/flags.cpp (line 777)
<https://reviews.apache.org/r/44948/#comment190651>

    s/range/ranges/ since this is what we actually support.


- Jiang Yan Xu


On April 6, 2016, 11:43 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 11:43 a.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
> -------
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
>   src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
>   src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 6, 2016, 6:43 p.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0
src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0
src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION
src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe
src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 5, 2016, 11:41 p.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2
src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0
src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION
src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe
src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 5, 2016, 11:15 p.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2
src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0
src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION
src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe
src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 5, 2016, 10:59 p.m.)

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

Changes
-------

Rebased and addresses review comments.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.


> On April 5, 2016, 4:52 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp, line 33
> > <https://reviews.apache.org/r/44948/diff/11/?file=1321655#file1321655line33>
> >
> >     Doesn't look like the utils header is used in this header?
> >     
> >     Otherwise add a blank line above.

Added the blank. The header is needed for the definition of ``prid_t``.


> On April 5, 2016, 4:52 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp, lines 112-114
> > <https://reviews.apache.org/r/44948/diff/11/?file=1321656#file1321656line112>
> >
> >     There is also the `uid.isNone()` case.
> >     
> >     We can just 
> >     
> >     ```
> >     if (!user.isSome()) {
> >       return Error("Failed to determine user: " +
> >                    (uid.isError() ? uid.error() : "uid not found"));
> >     }
> >     ```

``getuid(2)`` can't fail or return ``None``.


> On April 5, 2016, 4:52 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp, lines 281-286
> > <https://reviews.apache.org/r/44948/diff/11/?file=1321656#file1321656line281>
> >
> >     This is sort of safe in our case because we expect an empty sandbox. 
> >     
> >     In a general case `xfs::setProjectId()` could partially fail which makes it unsafe to return the project ID.
> >     
> >     I think we can just do a hard CHECK before calling to make sure the directory is empty. If no such method is directory usable right now, add a TODO and let's add one to stout.

As discussed, we keep the container Info record and depend on cleanup to return the project ID.


- James


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


On April 5, 2016, 10:59 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> 
> (Updated April 5, 2016, 10:59 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
> -------
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

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

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




src/slave/containerizer/mesos/isolators/xfs/disk.hpp (line 33)
<https://reviews.apache.org/r/44948/#comment190019>

    Doesn't look like the utils header is used in this header?
    
    Otherwise add a blank line above.



src/slave/containerizer/mesos/isolators/xfs/disk.hpp (line 100)
<https://reviews.apache.org/r/44948/#comment190020>

    const Flags flags;



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 57 - 58)
<https://reviews.apache.org/r/44948/#comment189960>

    We should probably also validate the upper bound of the ranges because they are `uint64`.
    
    ```
    static Try<IntervalSet<prid_t>> getIntervalSet(
        const Value::Ranges& ranges)
    ```
    
    ?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 112 - 114)
<https://reviews.apache.org/r/44948/#comment189938>

    There is also the `uid.isNone()` case.
    
    We can just 
    
    ```
    if (!user.isSome()) {
      return Error("Failed to determine user: " +
                   (uid.isError() ? uid.error() : "uid not found"));
    }
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 140 - 144)
<https://reviews.apache.org/r/44948/#comment189953>

    I suggested this earlier and this was marked as resolved:
    
    If the utils provdes 
    
    ```
    Option<Error> validateProjectIds(IntervalSet<prid_t>);
    ```
    
    Then the isolator doesn't need to know about `NON_PROJECT_ID` anymore and it can stay as a constant in utils.cpp.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 172 - 199)
<https://reviews.apache.org/r/44948/#comment190032>

    We can consolidate this with the orphan case because in both cases we need to do all of these to read its project ID and construct an `info`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 216 - 217)
<https://reviews.apache.org/r/44948/#comment189977>

    This fits in one line.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 222)
<https://reviews.apache.org/r/44948/#comment190044>

    IMO it's pretty clean and explicit to consolicate the recovery for both live containers and known orphans if we add comments here to make it clear that
    
    ```
    // We recover the sandboxes for both live containers and known orphans 
    // and only clean up the unknown orphans here.
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 231 - 234)
<https://reviews.apache.org/r/44948/#comment190041>

    This case shouldn't happen anymore if we consolidate. We can do a CHECK here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 237)
<https://reviews.apache.org/r/44948/#comment190040>

    In the code above you had
    
    ```
    // We should always be able to get a project ID. Maybe the directory
    // is not on an XFS filesystem, or something equally fatal happened.
    ```
    
    And you return a failure. I think this applies here as well regarless of whether the sandbox is alive or known/unkown orphans because this indicates problems that affect more than just this one container.
    
    We should return a failure here as well.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 238 - 239)
<https://reviews.apache.org/r/44948/#comment190018>

    Single quote the sandbox.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 239)
<https://reviews.apache.org/r/44948/#comment190039>

    Move one space to the right.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 243)
<https://reviews.apache.org/r/44948/#comment190035>

    Let's move over the comments you had above.
    
    ```
    // If there is no project ID, don't worry about it. This can happen the
    // first time an operator enables the XFS disk isolator and we recover a
    // set of containers that we did not isolate.
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 253)
<https://reviews.apache.org/r/44948/#comment190053>

    I don't think we need to wait for the result of the unknown orphan recovery because the cleanup could be expensive as it processes every file in the sandbox.
    
    We can call `cleanup(containerId)` for every unknown orphan without collecting them. At the bottom we just `return Nothing()`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 258)
<https://reviews.apache.org/r/44948/#comment189942>

    2 space indentation here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 281 - 286)
<https://reviews.apache.org/r/44948/#comment190256>

    This is sort of safe in our case because we expect an empty sandbox. 
    
    In a general case `xfs::setProjectId()` could partially fail which makes it unsafe to return the project ID.
    
    I think we can just do a hard CHECK before calling to make sure the directory is empty. If no such method is directory usable right now, add a TODO and let's add one to stout.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 332 - 333)
<https://reviews.apache.org/r/44948/#comment190201>

    The 'resources' here can be long as it includes all resources. We probably don't need to log here as we are already logging all outcomes.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 338 - 340)
<https://reviews.apache.org/r/44948/#comment190203>

    We can take care of this in a subsequent review but let's insert a LOG(WARNING) here and add a TODO: semantially we should set the quota to 0 but given XFS' enforceability limitation it's going to be limited at 1 basic block.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 436 - 440)
<https://reviews.apache.org/r/44948/#comment190204>

    We don't need to dispatch again. `_cleanup`'s content can just be moved here.


- Jiang Yan Xu


On April 4, 2016, 10:28 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> 
> (Updated April 4, 2016, 10:28 a.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
> -------
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

Posted by James Peach <jp...@apache.org>.

(Updated April 4, 2016, 5:28 p.m.)

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

Changes
-------

Rebased.

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

Repository: mesos

Description
-------

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

$ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

$ xfs_io -r -c stat PATH

Diffs (updated)
-----

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

Testing
-------

Make check. Manual testing. Tests in subsequent patches.

Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

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

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




src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 501 - 503)
<https://reviews.apache.org/r/44948/#comment189628>

    Chatted offline. 
    
    Feels like the right thing to do here is to treat a container with no disk resource as `limit == 0`, whether the container started with zero disk resource or is updated to zero.
    
    Given XFS' lack of ability to enforce disk quota below a basic block (512bytes), I think it's OK to do so and in the user doc make it clear that "with XFS isolator when no disk resource or zero disk resource is given, XFS will limit it at 512bytes (the size of a basic block)." In fact, XFS cannot do enforcement at a granularity below 512bytes. However since Mesos' disk resource granularity is more coarse than it (at 1KB, or 0.001 of 1MB), this shouldn't violate any expectations.
    
    Given that this is a pretty strict behavior (in most cases the task will fail immediately because of stdout/stderr/downloads) I think we should act this way based on a flag, maybe only if `flags.enforce_container_disk_quota == true`, but it raises another question of whether XFS isolator should support monitoring-only mode. Therefore I think it can be punted to a later review, I'll raise a JIRA intead.


- Jiang Yan Xu


On March 31, 2016, 5:01 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> 
> (Updated March 31, 2016, 5:01 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
> -------
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>