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