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