mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 44948: Add XFS disk resource isolator.
Date Thu, 24 Mar 2016 16:54:47 GMT

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




src/slave/containerizer/mesos/containerizer.cpp (line 217)
<https://reviews.apache.org/r/44948/#comment187725>

    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)
<https://reviews.apache.org/r/44948/#comment188068>

    It fits in one line.



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

    We usually don't omit argument names.



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

    This method aborts early when a error occurs, I think it's useful to note this.
    
    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`: http://pubs.opengroup.org/onlinepubs/9699919799/functions/nftw.html



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

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



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

    How does `openat` facilitate DFS better?



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

    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)
<https://reviews.apache.org/r/44948/#comment188027>

    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)
<https://reviews.apache.org/r/44948/#comment187927>

    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)
<https://reviews.apache.org/r/44948/#comment187928>

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



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

    s/process::Owned/Owned/



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

    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)
<https://reviews.apache.org/r/44948/#comment188025>

    Single quotes around `state.directory()`.



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

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



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

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



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

    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)
<https://reviews.apache.org/r/44948/#comment188006>

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



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

    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)
<https://reviews.apache.org/r/44948/#comment187733>

    s/" : "/": "/



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

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



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

    s/" : "/": "/



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

    Nit: this fits in one line.



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

    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)
<https://reviews.apache.org/r/44948/#comment188023>

    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)
<https://reviews.apache.org/r/44948/#comment188021>

    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)
<https://reviews.apache.org/r/44948/#comment188017>

    s/changes/changed/


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message