You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Devin Leamy <dl...@x.com> on 2024/02/12 17:06:01 UTC
Review Request 74875: [cgroups2] List the available subsystems in a cgroup.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74875/
-----------------------------------------------------------
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 PRE-CREATION
src/linux/cgroups2.cpp PRE-CREATION
src/tests/containerizer/cgroups2_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/74875/diff/1/
Testing
-------
Thanks,
Devin Leamy
Re: Review Request 74875: [cgroups2] List the available subsystems in a cgroup.
Posted by Devin Leamy <dl...@x.com>.
> 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
>
>
Re: Review Request 74875: [cgroups2] List the available subsystems in a cgroup.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74875/#review226268
-----------------------------------------------------------
Fix it, then Ship it!
I'll take care of these for you and land it!
src/linux/cgroups2.cpp
Lines 75 (patched)
<https://reviews.apache.org/r/74875/#comment314500>
this needs an <iterator> include
src/linux/cgroups2.cpp
Lines 88 (patched)
<https://reviews.apache.org/r/74875/#comment314502>
this is const as well?
src/linux/cgroups2.cpp
Lines 93 (patched)
<https://reviews.apache.org/r/74875/#comment314504>
this is copying the vector
src/linux/cgroups2.cpp
Lines 116-131 (patched)
<https://reviews.apache.org/r/74875/#comment314508>
see comment below, this can just be a parse function rather than a read, and that way State provides parsing and stringification and the caller can compose the reading and writing to files (also winds up being more unit testable)
src/linux/cgroups2.cpp
Lines 127 (patched)
<https://reviews.apache.org/r/74875/#comment314506>
split needs a <stout/strings.hpp> include
src/linux/cgroups2.cpp
Lines 128 (patched)
<https://reviews.apache.org/r/74875/#comment314509>
move here as well? (note this doesn't matter since the strings are small and therefore not on the heap, but I saw you did it below as well)
src/linux/cgroups2.cpp
Lines 134-135 (patched)
<https://reviews.apache.org/r/74875/#comment314503>
what does m here represent?
src/linux/cgroups2.cpp
Lines 138 (patched)
<https://reviews.apache.org/r/74875/#comment314510>
nit: keep the &'s consistent (against the type)
src/linux/cgroups2.cpp
Lines 262-267 (patched)
<https://reviews.apache.org/r/74875/#comment314505>
why the extra function indirection here..?
src/linux/cgroups2.cpp
Lines 273-281 (patched)
<https://reviews.apache.org/r/74875/#comment314507>
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));
```
src/tests/containerizer/cgroups2_tests.cpp
Lines 41 (patched)
<https://reviews.apache.org/r/74875/#comment314512>
hm.. we don't need the extra boolean? we alrady have the state inside `mounted`?
src/tests/containerizer/cgroups2_tests.cpp
Lines 43-47 (patched)
<https://reviews.apache.org/r/74875/#comment314513>
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
src/tests/containerizer/cgroups2_tests.cpp
Lines 45 (patched)
<https://reviews.apache.org/r/74875/#comment314511>
hm.. not sure if we want the test to be mounting it, but let's keep it for now and revisit if needed
- Benjamin Mahler
On Feb. 28, 2024, 5:09 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, 5:09 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 ec7ba9e4028276ebcb0c40be18b8975bb03c217e
> src/tests/containerizer/cgroups2_tests.cpp 4981e9588b5f327e84172a31fb722484ab33ff18
>
>
> Diff: https://reviews.apache.org/r/74875/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Devin Leamy
>
>
Re: Review Request 74875: [cgroups2] List the available subsystems in a cgroup.
Posted by Devin Leamy <dl...@x.com>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/74875/diff/2-3/
Testing
-------
Thanks,
Devin Leamy
Re: Review Request 74875: [cgroups2] List the available subsystems in a cgroup.
Posted by Devin Leamy <dl...@x.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74875/
-----------------------------------------------------------
(Updated Feb. 28, 2024, 5:09 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 (updated)
-----
src/linux/cgroups2.hpp 0c54e464b26c2a72b5efc8c461066888c7442257
src/linux/cgroups2.cpp ec7ba9e4028276ebcb0c40be18b8975bb03c217e
src/tests/containerizer/cgroups2_tests.cpp 4981e9588b5f327e84172a31fb722484ab33ff18
Diff: https://reviews.apache.org/r/74875/diff/2/
Changes: https://reviews.apache.org/r/74875/diff/1-2/
Testing
-------
Thanks,
Devin Leamy