> On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 55 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line55> > > > > It would be great to include the mount error too (for easier debugging > > by the human trying to use this stuff). For example, maybe: > > > > return Try<bool>::error("Failed to create hierarchy " + hierarchy + ": > > " + result.error()); > > > > I think the error will include the "Failed to mount" bit, so no need to > > repeat that here (in fact, in many cases you can get away with something > > like 'return Try<bool>::error(result.error());'.
Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 65 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line65> > > > > We don't use snake case, except when working with protobuf generated > > code: s/unmount_rc/result/ is probably better. > > > > Also, does it make sense to make the flags passed to unmount default to > > 0? Done. Add another interface "unmount" without flags (google c++ style says default values are not recommended). > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 47 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line47> > > > > s/mount_rc/result Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 110 > > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line110> > > > > s/val/value/ Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 67 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line67> > > > > Again, the actual unmount error would probably be helpful here. Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 111 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line111> > > > > s/val/value/ Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/mount.hpp, line 45 > > <https://reviews.apache.org/r/5211/diff/2/?file=110055#file110055line45> > > > > Any reason not to make this a 'const string&'? The glibc interface uses void * here. I guess in some cases, the parameter is not a string (e.g. mount a very special file system). > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/proc.hpp, line 221 > > <https://reviews.apache.org/r/5211/diff/2/?file=110057#file110057line221> > > > > Indent is +4 here please. Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/proc.hpp, line 244 > > <https://reviews.apache.org/r/5211/diff/2/?file=110057#file110057line244> > > > > No need for the typedef, we prefer explicit types as much as possible. Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/proc.hpp, line 246 > > <https://reviews.apache.org/r/5211/diff/2/?file=110057#file110057line246> > > > > You should just call this 'entries'. Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 80 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line80> > > > > This indentation should be +4 please. Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 51 > > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line51> > > > > s/WH/Wh Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 79 > > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line79> > > > > Maybe it makes sense to take a third argument that specifies whether to > > create the hierarchy if it doesn't exist defaulted to true? I want to keep these interfaces as simple as possible. Other richer functions can be built on top of these primitives. What do you think? > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 39 > > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line39> > > > > What about calling it a 'control' instead? Has the cgroups community > > adopted any terminology themselves? Is it knob? Done. > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 62 > > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line62> > > > > This is awesome, but what about two sub-namespaces: 'hierarchies' and > > 'controls'. E.g., from a clients perspective: > > > > cgroups::hierarchies::create(...); > > cgroups::hierarchies::destroy(...); > > > > cgroups::create(hierarchy, cgroup); > > > > cgroups::controls::read(hierarchy, cgroup, control); > > cgroups::controls::write(...); I prefer to keep the current interfaces. The way you suggested makes me feel that hierarchies are contained in cgroups. But in fact, a cgroup is a node in a hierarchy. What do you think? > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 78 > > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line78> > > > > Why use ::mkdir? If you want to make sure it already doesn't exist, you > > should just do os::exists first. If it's because you want better error > > information, it probably makes sense to update os::mkdir and have it return > > a Try. Either way, you can defer this to a later review but add a comment > > (either here or in utils.hpp next to mkdir and rmdir and any of the other > > ones that were insufficient for your use case). The reason I use ::mkdir (non-recursive version) here is the following: (in fact, we may need to add a non-recursive mkdir in common/utils.hpp) Each directory in a hierarchy is treated as a cgroup. Say your hierarchy is mounted at "/sys/fs/cgroups". You want to create a cgroup "students/undergrads". If the parent cgroups "students/" does not exists, the recursive version of mkdir (the one I don't use) will create two cgroups: "students/" and "students/undergrads". Consider the situation in which "students/" is created successfully and "students/undergrads" is not. You may want to return an error and remove the cgroup "students/" to make the operation atomic. I don't think such a complex logic should be in these primitive functions (we can always build rich functionalities based on these primitives). The take away here is: we should add a new function "utils::os::mkdir" in common/utils.hpp, which is just a wrapper of ::mkdir (non-recursieve). What's you opinion? > On 2012-05-23 23:50:20, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 35 > > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line35> > > > > Is there any reason for the hierarchy? For example, are any controls > > applied in a hierarchical fashion? Any details like this would be great to > > add for posterity. Done. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5211/#review8069 ----------------------------------------------------------- On 2012-05-23 22:30:37, Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5211/ > ----------------------------------------------------------- > > (Updated 2012-05-23 22:30:37) > > > Review request for mesos and Benjamin Hindman. > > > Summary > ------- > > This patch contains the following work: > > 1) Adding interfaces to control linux cgroups directly (see > src/linux/cgroups.h|cpp) > > 2) Adding interfaces to mount/unmount file systems on linux (see > src/linux/mount.h|cpp) > > 3) Adding more interfaces in src/linux/proc to examine cgroups info and mount > table. > > > Diffs > ----- > > src/Makefile.am d6eff8a > src/linux/cgroups.hpp PRE-CREATION > src/linux/cgroups.cpp PRE-CREATION > src/linux/mount.hpp PRE-CREATION > src/linux/mount.cpp PRE-CREATION > src/linux/proc.hpp 2f5c02a > src/linux/proc.cpp ed1beff > src/tests/cgroups_tests.cpp PRE-CREATION > src/tests/mount_tests.cpp PRE-CREATION > src/tests/proc_tests.cpp b9dcb91 > > Diff: https://reviews.apache.org/r/5211/diff > > > Testing > ------- > > Manual tests are done for those operations that need root permission. > > Proc related tests are done in google test framework (ProcTest). > > On linux machines, make check. > > > Thanks, > > Jie > >
