This is an automatically generated e-mail. To reply, visit:

Partial review. Continuing on the latest revision.

It would be great to have two sets of tests. One that tests the XFS operations 
and one that tests the isolator. In fact XFS operations and the isolator can be 
splitted into two reviews so each is verified by tests.

configure.ac (line 923)

    Where is the corresponding AC_ARG_WITH?
    i.e. we should only do this if the user wants to use XFS.


    No need to kill this line as often two lines are used to delimit sections.

src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 38)

    Kill the line.

src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 84)

    About the type: should it be `prid_t` which more accurately describes what 
it is?
    About the name: unless this naming convention in univeral in XFS can we 
spell it out as `projectId`?

src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 87)

    Ditto about uint32_t vs prid_t.

src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 94)

    Kill the line.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 17)

    Add a link that explains this?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 60)

    Kill the line.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 65)

    { on a new line.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 69)

    Add a line.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 70)

    This comment is probably unnecessary.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 83)

    Add a line.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 128)

    Can this and other xfs* methods be put under a xfs namespace instead?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 154)

    This doesn't appear to be XFS specific. Can we pull this into stout? At 
lease name it such that it's clear.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 163 - 165)

    This utility method itself doesn't care if the path is a directory or not, 

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 167)

    Can we use `os::stat::dev()` directly?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 186)

    Use `devname.isError()`.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 190 - 194)

    Why list initialize it here?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 191 - 194)

    Why a union here?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 196 - 199)

    These require more comments.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 203)

    We are effectively not using the soft limits. Is it OK to not set them?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 205 - 206)

    This indentation style is unconventional. Consider this:
    if (::quotactl(QCMD(Q_XSETQLIM, PRJQUOTA), 
                   ptr.c) == -1) {

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 211 - 212)

    It would be more useful to include containerIds in the log lines and they 
can be put inside the isolator. This applies to a couple of other log lines as 

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 365)

    This looks like a generic utility which we can leverage & add to ls.hpp.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 396)

    It's unclear why this additional check is needed here so can you comment on 

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 456)

    The naming is inconsistent with the reset of xfs* methods but moreover, can 
we pull out things that aren't XFS specific to stout headers and group XFS 
methods under namespace `mesos::internal::slave::xfs` instead? In fact perhaps 
put them under `linux/xfs/`?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 470)

    Use `lambda::_1` instead.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 489 - 493)

    This feels like a TODO.
    FWIW currently persistent volumes are not removed at all. We can try to 
make the behavior consistent with posix/disk and also enforce quota on them and 
update its logic later when we work on the ticket that handles persitent volume 
    Otherwise such limitation needs to be prominently documentated. (e.g. in 
the class-level comments and user docs) with a TODO or JIRA.
    Let's evaluate what to do for now.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 513)

    What privledges does XFS isolator require?
    Looks to me some operations require CAP_SYS_ADMIN. We often just check if 
the user is root.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 531)

    Are we losing the last element of each range if the interval set is open on 
the right side?

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 553)

    We'll likely see this a lot when we first turn on this feature. 
LOG(WARNING) is better IMO.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 573)

    No biggie but it's more customary to add a `:` after TODO(jpeach).

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 573 - 576)

    We do need to handle orphans, as you've commented in `cleanup()`.
    `orphans` here are known to the containerizer so `cleanup()` will be called 
by it to remove the quota. However if there's not an entry for it in `infos`, 
`cleanup()` ignores it.
    The concern is that if the project ID of the orphan is not unassigned from 
the sandbox and the ID is not tracked by the isolator, next time it's used by a 
new task, it'll share the same quota with the orphan.

- Jiang Yan Xu

On March 7, 2016, 10:38 a.m., James Peach wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44342/
> -----------------------------------------------------------
> (Updated March 7, 2016, 10:38 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
> -----
>   configure.ac a20382e8d425eb297492a6e6c2c75ea59be097c2 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> Diff: https://reviews.apache.org/r/44342/diff/
> Testing
> -------
> Manual testing on Fedora 23 w/ XFS. Make check on Fedora and OS X.
> Thanks,
> James Peach

Reply via email to