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

src/slave/containerizer/mesos/containerizer.cpp (line 217)

    IMO "xfs/disk" is probably better than "disk/xfs" (compared with 
"posix/disk") and "xfs/quota" is even better ("posix/quota" would have been 
more confusing but xfs being a filesystem, "xfs/quota" seem pretty clear to me 
what it means)

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

    It fits in one line.

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

    We usually don't omit argument names.

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

    This method aborts early when a error occurs, I think it's useful to note 
    Plus, I still think what we need in the isolator is a generic filesystem 
walk functionality. I'll help with creating something that utilize `nftw`: 

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

    `::dirfd` specially because you use dirfd as function arguments as well.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 156 - 157)

    How does `openat` facilitate DFS better?

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

    This helper method tries to do two things and it continues to do the second 
even if the first fails but the error reflects the first if it fails.
    It's a bit weird and we don't need this: we can do all this directly in:
    Future<Nothing> XfsDiskIsolatorProcess::cleanup(
        const ContainerID& containerId) {...}
    And in `recover()` we can call `cleanup(containerId)` directly.

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

    What's `nbytes`, number of bytes? We could simply call it `result`, to 
avoid name abbreviation.

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

    We should check if the user is root and the filesystem `flags.work_dir` is 
on is XFS here.

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

    To eliminate the need to use the sentinel value in the isolator we can add 
    bool validateProjectIds(IntervalSet<prid_t>);
    to the utils.

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


src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 310 - 312)

    Jie mentioned this too: it's customary for the isolators to take and store 
the flags directly, even if we have already derived `projectIds` from the flags.
    For `workDir` because it can be used directly from `flags.work_dir` with no 
conversion, we'd just use that in recovery.
    The argument for it is not strong but I would advocate for it.

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

    Single quotes around `state.directory()`.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 337 - 352)

    Jie mentioned this and I concur. The XFS API should interpret the sentinel 
value for us.
    Result<prid_t> getProjectId(directory);

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

    If this is not necessarily an error, I think LOG(WARNING) is more 

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 366 - 367)

    I don't think this is strictly true because the known orphans in the 
`orphans` list have not fully died yet and could in theory be manipulating 
files and causing race conditions that result in errors in cleanup operations.
    I'd follow the commonly used pattern to cleanup only unknown orphans (i.e., 
sandboxes that are neither in `states` or `orphans`) here and leave the 
`orphans` cleanup for the containerizer once it successfully kills the cgroup.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 381 - 382)

    This is not a valid indentation style. Use the following instead.
    return Failure("Failed to scan sandbox directories: " +

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 399 - 408)

    We are doing this twice if the sandbox didn't have a project ID assigned 
before the agent restart.
    Let's do the following:
    foreach (const string& sandbox, sandboxes.get()) {
      // Put the info for sandbox in infos regardless whether it is in 
`states`, `orphans` or not.
      // If sandbox is an unknown orphan, call cleanup(containerId).
    We may need to call cleanup asynchronously (i.e., `async(cleanup)`) because 
the cleanup could be expensive if there are a lot of files in the sandbox and 
we don't need to wait for it to complete (because it's an unknown orphan).

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

    s/" : "/": "/

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 501 - 503)

    If the new resources have no disk resources, shouldn't we cleanup the 

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

    s/" : "/": "/

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 552 - 553)

    Nit: this fits in one line.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 565 - 566)

    Recover at GC time: this is unlikely to be implemented given the way GC 
works right now and I don't see a major drawback in doing the cleanup there so 
the comment is a bit confusing. It's either a TODO or a description of the 
current behavior. If this is just to help readers understand the design 
decision, I would be explicit: "We could do ... but chose to do ... because ..."

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

    It seems that we don't need to modify `totalProjectIds` (which can be a 
const) here, this change doesn't survive agent restart anyways and we use 
`freeProjectIds` to assign IDs.

src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 588 - 589)

    It seems that if we trust `freeProjectIds` to be initialized correctly, 
then `projectId` is never going to be zero here. Normally we do such checks 
before we are about to use a variable in a way that assumes the CHECK condition 
holds so it doesn't look like we need to CHECK here.
    I am trying to see if we can eliminate the sentinel value from the isolator 
and confine it in the utils.

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


- Jiang Yan Xu

On March 22, 2016, 4:24 p.m., James Peach wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> -----------------------------------------------------------
> (Updated March 22, 2016, 4:24 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 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ee7a265975323ca891114a286357c8e42901560c 
>   src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
>   src/slave/flags.hpp 3067c128f1fded4f130f4d45f191584c2f30ad9c 
>   src/slave/flags.cpp ce028825ad99f54a231b4b18dde277b63aa0525c 
> Diff: https://reviews.apache.org/r/44948/diff/
> Testing
> -------
> Make check. Manual testing. Tests in subsequent patches.
> Thanks,
> James Peach

Reply via email to