mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.
Date Thu, 22 Mar 2018 18:19:33 GMT

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




src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 77 (patched)
<https://reviews.apache.org/r/66001/#comment280250>

    We don't need the const in the parameter list.



src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 84 (patched)
<https://reviews.apache.org/r/66001/#comment280249>

    Can you call this `check()` for symmetry with the ports isolator?



src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 104 (patched)
<https://reviews.apache.org/r/66001/#comment280252>

    I think that we should add a new quota policy enum to capture the killing semantic. What
do you think of this?
    
    ```
    enum class QuotaPolicy {
      ACCOUNTING
      ENFORCING_ACTIVE,
      ENFORCING_PASSIVE
    };
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 340 (patched)
<https://reviews.apache.org/r/66001/#comment280242>

    You only start the loop if we are killing containers and the policy is `ENFORCING`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 355 (patched)
<https://reviews.apache.org/r/66001/#comment280228>

    This starts a watch loop for every containers. What you want to do is to start a single
watch loop that checks all the containers in one pass. You can do this by overriding `ProcessBase::initialize`
like the ports isolator does.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 380 (patched)
<https://reviews.apache.org/r/66001/#comment280248>

    AFAICT `utils::copy` isn't needed.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 398 (patched)
<https://reviews.apache.org/r/66001/#comment280247>

    This deserves a comment describing our strategy for using the hard and soft limits.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 426 (patched)
<https://reviews.apache.org/r/66001/#comment280244>

    Document the invariants by adding
    ```
    CHECK_EQ(quotaPolicy, xfs::QuotaPolicy::ACCOUNTING);
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 432 (patched)
<https://reviews.apache.org/r/66001/#comment280237>

    Suggest:
    ```
    LOG(WARNING) << "Failed to check disk usage for container '"
                 << containerId << '": " << quotaInfo.error();
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 433 (patched)
<https://reviews.apache.org/r/66001/#comment280238>

    Better to `continue` here than do the `if/else`, i.e:
    
    ```
    if (quotaInfo.isError()) {
      ...
      continue;
    }
    
    if (quotaInfo.isNone()) {
      // No quota for this container? That's weird!
      ...
      continue;
    }
    
    // Now check the allocation against the usage.
    ...
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 435 (patched)
<https://reviews.apache.org/r/66001/#comment280236>

    We don't have a separate quota for each resource. In practice we are only tracking the
"anonymous" sandbox resource, which is why the `Info` struct only has the total bytes. In
the quota records, a quota is associated with a path, not a resource like you have here.
    
    I think the right approach is to revert `getDiskResources` and `getBytesNeeded` and just
compare the `Info::quota` value.
    
    Remember to explicitly comment (and handle if necesary) the case where the agent restarts
and the hard and soft limits are set to the same value.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 438 (patched)
<https://reviews.apache.org/r/66001/#comment280245>

    You can use `->` in place of `.get()`, like this:
    ```
    quotaInfo->used
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 444 (patched)
<https://reviews.apache.org/r/66001/#comment280246>

    The `Resource` here should be the amount the container consumed, so you need to create
it from the `QuotaInfo`.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 36 (patched)
<https://reviews.apache.org/r/66001/#comment280220>

    Let's call out the limits explicitly:
    ```
    Bytes hardLimit;
    Bytes softLimit;
    Bytes used;
    ```



src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 105 (patched)
<https://reviews.apache.org/r/66001/#comment280221>

    Parameter names should be
    ```
    Bytes hardLimit,
    Bytes softLimit);
    ```



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 52 (patched)
<https://reviews.apache.org/r/66001/#comment280222>

    Remove this newline.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 139 (patched)
<https://reviews.apache.org/r/66001/#comment280224>

    Update parameter names.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 169 (patched)
<https://reviews.apache.org/r/66001/#comment280226>

    Is this a copy/paste error? You need to update the `setProjectQuota` definition below
(near line 283).



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 176 (patched)
<https://reviews.apache.org/r/66001/#comment280225>

    This comment is no longer accurate. I think we can just keep the first sentence.


- James Peach


On March 22, 2018, 2:11 p.m., Harold Dost wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> -----------------------------------------------------------
> 
> (Updated March 22, 2018, 2:11 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-6575
>     https://issues.apache.org/jira/browse/MESOS-6575
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> New Flags for disk/xfs isolator:
> - xfs_kill_containers - This will create a 10 MB buffer between the soft
>   and hard limits, and when the soft limit is exceeded it will
>   subsequently be killed.
> 
> Functionality
> - This by default disabled behavior allows for the `disk/xfs` isolator
>   to kill containers which surpass their limit.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 07e68a777aefba4dd35066f2eb207bba7f199d83

>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 8d9f8f846866f9de377c59cb7fb311041283ba70

>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp e034133629a9c1cf58b776f8da2a93421332cee0

>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 2708524add1ff693b616d4fb241c4a0a3070520b

>   src/slave/flags.hpp 949a4783caf8aac9a246a98525a5287b0f8256d8 
>   src/slave/flags.cpp dd8dfb7a8a9f7c6030939c9eea841eb47deadfc4 
> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/10/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Harold Dost
> 
>


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