libct/cgroup: rm GetClosestMountpointAncestor using moby/sys/mountinfo parser#2401
Merged
mrunalp merged 2 commits intoopencontainers:masterfrom May 18, 2020
Merged
Conversation
This function is not very efficient, does not really belong to cgroup package, and is only used once (from fs/cpuset.go). Prepare to remove it by replacing with the implementation based on the parser from github.com/moby/sys/mountinfo parser. This commit is here to make sure the proposed replacement passes the unit test. Funny, but the unit test need to be slightly modified since it supplies the wrong mountinfo (space as the first character, empty line at the end). Validated by $ go test -v -run Ance === RUN TestGetClosestMountpointAncestor --- PASS: TestGetClosestMountpointAncestor (0.00s) PASS ok github.com/opencontainers/runc/libcontainer/cgroups 0.002s Signed-off-by: Kir Kolyshkin <[email protected]>
The function GetClosestMountpointAncestor is not very efficient, does not really belong to cgroup package, and is only used once (from fs/cpuset.go). Remove it, replacing with the implementation based on moby/sys/mountinfo parser. Signed-off-by: Kir Kolyshkin <[email protected]>
Contributor
Author
|
OK, CI passed, adding the second commit |
Contributor
Author
|
I quickly checked Docker and libpod sources -- this function is not used in there. |
Contributor
Author
|
@AkihiroSuda @mrunalp PTAL |
Member
|
LGTM, but I guess we may eventually move back the function to libct/cg . |
Contributor
Author
There is no need to mess with mountinfo for cgroup v2 case, and I don't think we'll make any major changes to cgroup v1 (except I'm still tinkering with how to decrease the ridiculous number of times we parse /proc/self/mountinfo). |
Contributor
|
LGTM |
AkihiroSuda
added a commit
to AkihiroSuda/runc
that referenced
this pull request
May 19, 2020
The compilation error had ocurred because of a bad rebase during opencontainers#2401 and opencontainers#2413 Signed-off-by: Akihiro Suda <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The function
GetClosestMountpointAncestoris not very efficient,does not really belong to cgroup package, and is only used once
(from fs/cpuset.go).
The removal is done in two stages/commits:
1
Prepare to remove it by replacing with the implementation based on
the
github.com/moby/sys/mountinfoparser.This commit is here to make sure the proposed replacement passes the
unit test.
Funny, but the unit test need to be slightly modified since it
supplies the wrong mountinfo (space as the first character, empty line
at the end).
Validated by
2
Actually remove
GetClosestMountpointAncestor, move the replacementcode to
libcontainer/cgroups/fs/cpuset.go.