----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29688/#review67709 -----------------------------------------------------------
Should there be a flag in here to make the enforcement optional for those that need to incrementally turn this on? src/slave/containerizer/isolators/disk_quota.hpp <https://reviews.apache.org/r/29688/#comment111793> Mind documenting why you need this? src/slave/containerizer/isolators/disk_quota.hpp <https://reviews.apache.org/r/29688/#comment111791> How about calling this 'usages'? src/slave/containerizer/isolators/disk_quota.hpp <https://reviews.apache.org/r/29688/#comment111794> It wasn't totally obvious that these strings were directories, mind adding a small comment? ``` // These contain the container's 'directory' above, and optionally // an entry corresponding to the path of each volume used by the // container. ``` Mind moving 'quotas' above 'usages'? ``` hashmap<std::string, Resources> quotas; hashmap<std::string, process::Future<Bytes>> usages; ``` src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111790> Could you avoid the non-const reference here and just make a copy? ``` foreachvalue (Future<Bytes> usage, usages) { usage.discard(); } ``` src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111801> Ditto here, not sure why you're distrusting Owned here. Also, Owned already provides this CHECK for you when invoking the -> operator. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111796> Why are you checking this? We should be trusting Owned, no? Ditto elsewhere. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111802> Just to be sure, we're validating that `has_disk() == true` with nothing set inside DiskInfo is disallowed by the master, right? src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111810> Could you do a consistency check here? It's not that obvious from the code: ``` CHECK_EQ(info->quotas.keys(), info->usages.keys()); ``` src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111803> This seems a bit tough to read with all the generic "checks" and "checker". How about calling it DiskUsageMonitor and s/check/usage/: ``` info->usages[path] = monitor.usage(path) .onAny(defer(..., &DiskQuotaIsolatorProcess::_usage, ...); ``` I also did `s/checked/_usage/` in the continuation. Note the wrapping consistency with most of our defer calls. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111800> Do you need to pull out the Owned here? Seems to be a bit more verbose. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111799> Remember to find all of these CHECK_NOTNULLs when cleaning this up. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111798> Ditto here. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111816> s/bytes/byte It's not clear what the 1K byte blocks means here, could you clarify? src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111817> Can you implement this? The consequence of it is that if the pipe fills, this stalls completely, and it's possible to get stderr output when there are permission issues! It actually seems like we could simplify the continuation structure here too: ``` process::await(s.get().status(), io::read(s.get().out().get()), io::read(s.get().err().get())) .onAny(defer(self() &Self::_schedule, s.get().status(), lambda::_1); ``` Then you don't need `__schedule` and `___schedule`, because everything will be ready within `_schedule` thoughts? (You'll need to add a 3-tuple overload of process::await) :) src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111822> Why do you pull this future out into a variable? src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111819> Why not discard the promise when the future is discarded? src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111824> Mind commenting what the expected output format is? Not obvious why you need the tab tokenizing here. src/slave/containerizer/isolators/disk_quota.cpp <https://reviews.apache.org/r/29688/#comment111811> You wrote "queue" but you used "list". src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/29688/#comment111806> I'm confused by the naming choice here historically. ``` // These are <technique>/<resource>: posix/cpu posix/mem posix/disk // Why not name it like this? // Ideally we would have prefixed all of these with "linux/". cgroups/cpu cgroups/mem cgroups/perf_event namespaces/pid // Some are <resource>/<technique>? filesystem/shared network/port_mapping ``` Any reason why you didn't call this posix/disk, in line with what we did for posix/mem? - Ben Mahler On Jan. 12, 2015, 6:46 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29688/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2015, 6:46 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Ian Downes. > > > Bugs: MESOS-1588 > https://issues.apache.org/jira/browse/MESOS-1588 > > > Repository: mesos-git > > > Description > ------- > > Added DiskQuotaIsolator to enforce disk quota. I created a DiskUsageChecker > to check disk usage by calling 'du'. The DiskUsageChecker is throttled (see > comments). The isolator uses DiskUsageChecker to enforce disk quota. > > > Diffs > ----- > > src/Makefile.am fc0c3227466ccf364353a739fec8d9532ea3c683 > src/slave/containerizer/isolators/disk_quota.hpp PRE-CREATION > src/slave/containerizer/isolators/disk_quota.cpp PRE-CREATION > src/slave/containerizer/mesos/containerizer.cpp > 5c014ebe360b9527b3edd505d47e57a4d5ce5c52 > src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6 > > Diff: https://reviews.apache.org/r/29688/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
