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

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

    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)

    const Flags flags;

src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 57 - 58)

    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)

    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)

    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)

    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)

    This fits in one line.

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

    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)

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

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

    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)

    Single quote the sandbox.

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

    Move one space to the right.

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

    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)

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

    2 space indentation here.

src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 281 - 286)

    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)

    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)

    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)

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

- 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

Reply via email to