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 Tue, 27 Mar 2018 16:35:28 GMT

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




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

    Looke like we don't need this include?



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

    I don't think you need these. You'll probably get rid of the explicit `ControlFlow` usage,
and you only use `Continue` once.



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

    I think that this formulation makes the logic easier to read:
    
    ```
    xfs::QuotaPolicy quotaPolicy = xfs::QuotaPolicy::ACCOUNTING;
    
    if (flags.enforce_container_disk_quota) {
      quotaPolicy = flags.xfs_kill_containers
        ? xfs::QuotaPolicy::ENFORCING_ACTIVE
        : xfs::QuotaPolicy::ENFORCING_PASSIVE;
    }
    ```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 188 (original)
<https://reviews.apache.org/r/66001/#comment280649>

    Keep this newline. See the [new line style](https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#empty-lines)



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

    Double newline here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 306 (original), 325 (patched)
<https://reviews.apache.org/r/66001/#comment280648>

    Keep the existing failure. The continerizer shouldn't be asking for a container we don't
have.



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

    This starts a loop for each container. You want to do what `network/ports.cpp` does and
start a single loop once in `XfsDiskIsolatorProcess::initialize`.



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

    This should be simplified down to:
    ```
    process::loop(
      self(),
      [=]() {
        return process::after(watchInterval);
      },
      [=]() {
        check();
        return Continue();
      }
    ```
    
    Running the loop against `self()` causes it to dispatch the (serialized) lambdas against
the XFS process. This means that we can safely capture the `this` pointer and use it to access
member variables.
    
    After refactoring, you might find that you want to just move the code from `::check()`
into the body of the loop.



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

    All we need to do in this function is
    ```
    return infos[containerId]->limitation.future();
    ```
    
    The background check loop will satisfy the future if necessary.



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

    Just keep the newlines the way they were.



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

    Move this into the case statement block.



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

    Here's how this should be formatted:
    ```
          Bytes hardLimit = needed.get();
    
          // The purpose behind adding to the hard limit is so that the soft limit
          // can be exceeded thereby allowing for us to check if the limit has been
          // reached without allowing the process to allocate too much beyond the
          // desired limit.
          if (quotaPolicy == xfs::QuotaPolicy::ENFORCING_ACTIVE) {
            hardLimit += Megabytes(10);
          }
    
          Try<Nothing> status = xfs::setProjectQuota(
              info->directory, info->projectId, hardLimit, needed.get());
    
    ```
    
    Note the 4-space indent after the dangling `(`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 356 (original), 409 (patched)
<https://reviews.apache.org/r/66001/#comment280662>

    Let's format this as:
    "Set quota ... to " << softLimit << "/" << hardLimit;



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 362 (original), 414 (patched)
<https://reviews.apache.org/r/66001/#comment280550>

    `needed` is still the allocated quota so you don't need to use `hardLimit` here.



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

    It's unfortunately verbose, but we should do this:
    
    ```
      CHECK_NE(
          static_cast<int>(xfs::QuotaPolicy::ACCOUNTING),
          static_cast<int>(quotaPolicy));
    ```



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

    "Failed to check ..."



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

    Add newline here.



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

    Here, you need to deal with both old and new containers. However, in both cases, the soft
limit should have been set to the allocated disk resource for the container, so you can just
do:
    ```
    if (quotaInfo->used > quotaInfo->softLimit) {
      ...
    }
    ```



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

    The resource in the limitation shold be the actual usage, so 'quotaInfo->used'.



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

    I don't think we need this log message.



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

    Remove 'xfs' from this.



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

    Can we order these fields to be `softLimit`, `hardLimit`?



src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Line 80 (original), 82 (patched)
<https://reviews.apache.org/r/66001/#comment280581>

    This should be:
    ```
      return
        left.hardLimit == right.hardLimit &&
        left.softLimit == right.softLimit &&
        left.used == right.used;
    ```
    
    (2-space indent)



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

    Can we order this to be `softLimit`, `hardLimit`?



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

    Remove this internal helper. All the external APIs can call the same helper.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Line 253 (original), 259 (patched)
<https://reviews.apache.org/r/66001/#comment280646>

    Double newline after this function.



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

    Remove whitespace between `(` and `"`. Use a consistent error message, i.e.:
    
    ```
    if (hardLimit == 0) {
      return Error("Quota hard limit must be greater than 0");
    }
    
    if (softLimit == 0) {
      return Error("Quota soft limit must be greater than 0");
    }
    ```



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

    Double newline after this function.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Line 268 (original), 292 (patched)
<https://reviews.apache.org/r/66001/#comment280643>

    Not yours, but remove whitespace between `(` and `"`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Line 271 (original), 295 (patched)
<https://reviews.apache.org/r/66001/#comment280644>

    Just call the existing helper directly.
    ```
    return internal::setProjectQuota(path, projectId, hardLimit, hardLimit);
    ```


- James Peach


On March 23, 2018, 4:13 p.m., Harold Dost wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> -----------------------------------------------------------
> 
> (Updated March 23, 2018, 4:13 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 
>   src/tests/containerizer/xfs_quota_tests.cpp 64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe

> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Harold Dost
> 
>


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