> On Feb. 28, 2024, 10:16 p.m., Benjamin Mahler wrote: > > src/linux/cgroups2.cpp > > Lines 134-135 (patched) > > <https://reviews.apache.org/r/74875/diff/2/?file=2285510#file2285510line134> > > > > what does m here represent?
Member. Otherwise there's name conflicts. > On Feb. 28, 2024, 10:16 p.m., Benjamin Mahler wrote: > > src/linux/cgroups2.cpp > > Lines 262-267 (patched) > > <https://reviews.apache.org/r/74875/diff/2/?file=2285510#file2285510line262> > > > > why the extra function indirection here..? One is the cgroups external API and the other is the internal interface to the control files. > On Feb. 28, 2024, 10:16 p.m., Benjamin Mahler wrote: > > src/linux/cgroups2.cpp > > Lines 273-281 (patched) > > <https://reviews.apache.org/r/74875/diff/2/?file=2285510#file2285510line273> > > > > hm.. it's a bit weird that we need another write for this specifically, > > looks more like we can just use the existing one: > > > > ``` > > return write(cgroup, control::SUBTREE_CONTROLLERS, stringify(*control)); > > ``` > > > > same for read here, it's more composable for the state to be able to > > parse, but not actually do the read (more unit testable as well): > > > > ``` > > Try<string> contents = cgroups2::read( > > cgroup, > > cgroups2::control::SUBTREE_CONTROLLERS); > > > > if (contents.isError()) { > > return Error(contents.error()); > > } > > > > State control = State::parse(*contents); > > control->enable(subsystems); > > return cgroups2::write( > > cgroup, > > control::SUBTREE_CONTROLLERS, > > stringify(control)); > > ``` I think your idea of making State::read() into State::parse() is a good one. > On Feb. 28, 2024, 10:16 p.m., Benjamin Mahler wrote: > > src/tests/containerizer/cgroups2_tests.cpp > > Lines 41 (patched) > > <https://reviews.apache.org/r/74875/diff/2/?file=2285511#file2285511line41> > > > > hm.. we don't need the extra boolean? we alrady have the state inside > > `mounted`? This reads as "the test mounted the cgroup file system". Not "is the cgroup file system mounted". `mounted()` is the latter. The boolean is used for the former. > On Feb. 28, 2024, 10:16 p.m., Benjamin Mahler wrote: > > src/tests/containerizer/cgroups2_tests.cpp > > Lines 43-47 (patched) > > <https://reviews.apache.org/r/74875/diff/2/?file=2285511#file2285511line43> > > > > use `ASSERT_` instead of `EXPECT_` where you need the test to stop on > > failure.. in particular here you don't want to derefrence the Try if it's > > none (will crash), same case below with the other Try Noted. > On Feb. 28, 2024, 10:16 p.m., Benjamin Mahler wrote: > > src/tests/containerizer/cgroups2_tests.cpp > > Lines 45 (patched) > > <https://reviews.apache.org/r/74875/diff/2/?file=2285511#file2285511line45> > > > > hm.. not sure if we want the test to be mounting it, but let's keep it > > for now and revisit if needed Alternatively we can refuse to do anything if it's cgroup2 is not already mounted on the host at /sys/fs/cgroup2. Would simply things and is a reasonable assumption. - Devin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74875/#review226268 ----------------------------------------------------------- On Feb. 28, 2024, 10:27 p.m., Devin Leamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74875/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2024, 10:27 p.m.) > > > Review request for mesos. > > > Repository: mesos > > > Description > ------- > > Creates an interface to "cgroups.controllers" to obtain a set of > the subsystems available on a host. > > > Diffs > ----- > > src/linux/cgroups2.hpp 0c54e464b26c2a72b5efc8c461066888c7442257 > src/linux/cgroups2.cpp 0a1da2cb4ac65da6681e18a0976310d741bc60f2 > src/tests/containerizer/cgroups2_tests.cpp > 4981e9588b5f327e84172a31fb722484ab33ff18 > > > Diff: https://reviews.apache.org/r/74875/diff/3/ > > > Testing > ------- > > > Thanks, > > Devin Leamy > >
