> On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > Should there be a flag in here to make the enforcement optional for those > > that need to incrementally turn this on?
Will do it in a following patch. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.hpp, line 108 > > <https://reviews.apache.org/r/29688/diff/4/?file=816761#file816761line108> > > > > Mind documenting why you need this? Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.hpp, lines 112-113 > > <https://reviews.apache.org/r/29688/diff/4/?file=816761#file816761line112> > > > > 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; > > ``` Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 61 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line61> > > > > Could you avoid the non-const reference here and just make a copy? > > > > ``` > > foreachvalue (Future<Bytes> usage, usages) { > > usage.discard(); > > } > > ``` Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 134 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line134> > > > > Ditto here, not sure why you're distrusting Owned here. Also, Owned > > already provides this CHECK for you when invoking the -> operator. Removed. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 153 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line153> > > > > Why are you checking this? We should be trusting Owned, no? > > > > Ditto elsewhere. Removed. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 166 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line166> > > > > Just to be sure, we're validating that `has_disk() == true` with > > nothing set inside DiskInfo is disallowed by the master, right? Yes. Added a comment. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 202 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line202> > > > > Could you do a consistency check here? It's not that obvious from the > > code: > > > > ``` > > CHECK_EQ(info->quotas.keys(), info->usages.keys()); > > ``` Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 249 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line249> > > > > 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. Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, lines 279-280 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line279> > > > > Do you need to pull out the Owned here? Seems to be a bit more verbose. Killed. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 280 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line280> > > > > Remember to find all of these CHECK_NOTNULLs when cleaning this up. Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 282 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line282> > > > > Ditto here. Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 368 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line368> > > > > s/bytes/byte > > > > It's not clear what the 1K byte blocks means here, could you clarify? Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, line 395 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line395> > > > > Why do you pull this future out into a variable? Killed. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, lines 406-413 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line406> > > > > Why not discard the promise when the future is discarded? We do not expect someone to discard 'status', so I use failure here. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, lines 459-460 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line459> > > > > Mind commenting what the expected output format is? Not obvious why you > > need the tab tokenizing here. Done. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/mesos/containerizer.cpp, line 110 > > <https://reviews.apache.org/r/29688/diff/4/?file=816763#file816763line110> > > > > 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? I liked it. I renamed the isolator as well. > On Jan. 12, 2015, 11:11 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/disk_quota.cpp, lines 392-393 > > <https://reviews.apache.org/r/29688/diff/4/?file=816762#file816762line392> > > > > 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) :) Good idea! Done. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29688/#review67709 ----------------------------------------------------------- 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 > >
