cgroupv1: refactor and optimize #3215
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
No functional change. Signed-off-by: Kir Kolyshkin <[email protected]>
It does not make sense to calculate slice and unit 10+ times. Move those out of the loop. Signed-off-by: Kir Kolyshkin <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
As this is called from the Apply() method, it's a natural name. Signed-off-by: Kir Kolyshkin <[email protected]>
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 <[email protected]>
5 tasks
kolyshkin
requested review from
AkihiroSuda,
cyphar and
thaJeztah
and removed request for
AkihiroSuda
kolyshkin
commented
Sep 15, 2021
cyphar
reviewed
Sep 19, 2021
cyphar
approved these changes
Sep 19, 2021
thaJeztah
reviewed
Sep 20, 2021
| 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.);
Suggested change
| // 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.
libcontainer/cgroups/fs/paths.go
Outdated
Comment on lines
128
to
130
| 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() ?)
Suggested change
| 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 🙏🏻 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.