cgroupv1: refactor and optimize#3215
Conversation
We were checking if a unit is a slice two times. Consolidate those checks, and improve comments while we're at it. The code is the same in v1 and v2 but it's too complicated to factor it out, thus we just do the same changes in v1.go and v2.go. No functional change. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
No functional change. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It does not make sense to calculate slice and unit 10+ times. Move those out of the loop. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As ExpandSlice("system.slice") returns "/system.slice", there is no need
to call it for such paths (and the slash will be added by path.Join
anyway).
The same optimization was already done for v2 as part of commit
bf15cc9.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Now fs.go is not very readable as its public API functions are intermixed with internal stuff about getting cgroup paths. Move that out to paths.go, without changing any code. Same for the tests -- move paths-related tests to paths_test.go. This commit is separate to make the review easier. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case c.Path is set, c.Name and c.Parent are not used, and so calls to utils.CleanPath are entirely unnecessary. Move them to inside of the "if" statement body. Get rid of the intermediate cgPath variable, it is not needed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As this is called from the Apply() method, it's a natural name. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Dismantle and remove struct cgroupData. It contained three unrelated
entities (cgroup paths, pid, and resources), and made the code
harder to read. Most importantly, though, it is not needed.
Now, subsystems' Apply methods take path, resources, and pid.
To a reviewer -- the core of the changes is in fs.go and paths.go,
the rest of it is adapting to the new signatures and related test
changes.
2. Dismantle and remove struct cgroupTestUtil. This is a followup
to the previous item -- since cgroupData is gone, there is nothing
to hold in cgroupTestUtil. The change itself is very small (see
util_test.go), but this patch is big because of it -- mostly
because we had to replace helper.cgroup.Resources with
&config.Resources{}.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
left a comment
There was a problem hiding this comment.
I have re-reviewed this myself (as the code was written a few weeks ago) and it LGTM.
Most commits are easy to review, though the last one is not, merely because of its size. Its commit message gives some hints though.
cyphar
left a comment
There was a problem hiding this comment.
LGTM. In case you didn't know -- GitHub recently seems to have added commit-by-commit review capabilities -- if you open a single commit to view in a PR, it has previous and next arrows now.
|
|
||
| // if we create a slice, the parent is defined via a Wants= | ||
| if strings.HasSuffix(unitName, ".slice") { | ||
| // If we create a slice, the parent is defined via a Wants=. |
There was a problem hiding this comment.
I should probably add a nit here that If inside an if is describing the code 😇. Within an if/else branch it could be (e.g.);
| // If we create a slice, the parent is defined via a Wants=. | |
| // Create a slice; the parent is defined via a Wants=. |
(no need to change)
There was a problem hiding this comment.
We're not creating a slice right here, I think the older version of the comment is more comprehensible. Or something like "For a slice, the parent is defined via Wants=", but it's good enough as it is for me.
| cgParent := utils.CleanPath(c.Parent) | ||
| cgName := utils.CleanPath(c.Name) | ||
| innerPath = filepath.Join(cgParent, cgName) |
There was a problem hiding this comment.
Tempted to say; just inline them here (do we need a utils.CleanJoin() ?)
| cgParent := utils.CleanPath(c.Parent) | |
| cgName := utils.CleanPath(c.Name) | |
| innerPath = filepath.Join(cgParent, cgName) | |
| innerPath = filepath.Join(utils.CleanPath(c.Parent), utils.CleanPath(c.Name)) |
There was a problem hiding this comment.
The "clean" part is a bit more explicit the non-inlined way. I think this is why it was written the way it was -- to make it more explicit that we're cleaning all the input.
I guess the compiler optimizes this out anyway.
There was a problem hiding this comment.
Yeah the angry comment was added because CleanPath was deleted by someone inadvertently during a cleanup which reintroduced a vulnerability, so I tried to make it as clear as possible that the CleanPath is necessary and must not be removed.
|
@AkihiroSuda PTAL 🙏🏻 |
Used to be a part of #3131.
A few cgroup v1 optimizations and some refactoring. See individual commits for details.
This is a preparation for #3216.