cgroups: fs: fix issues with .Apply() #60
Closed
Conversation
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
If a configuration option is set for a cgroup that isn't mounted, an error will be emitted. This is an issue, because newer cgroups aren't present in older kernels, and we can't break backcompat where a new config can't run on an older kernel. The issue arises because .Set() does no checks for whether the given path actually points to a mountpoint, so fix the code by just bailing if the cgroup mountpoint doesn't exist. Fixes: fc3981e Fixes: 606d906 Signed-off-by: Aleksa Sarai <[email protected]>
Member
Author
|
The reason behind this PR is flawed. Failing loudly is better than ignoring (potentially security-sensitive) options. |
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
config: minor cleanup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
If a configuration option is set for a cgroup that isn't mounted, an
error will be emitted. This is an issue, because newer cgroups aren't
present in older kernels, and we can't break backcompat where a new
config can't run on an older kernel.
The issue arises because .Set() does no checks for whether the given
path actually points to a mountpoint, so fix the code by just bailing if
the cgroup mountpoint doesn't exist.
Fixes: fc3981e
Fixes: 606d906
Signed-off-by: Aleksa Sarai [email protected]
This was already done in 606d906, and then reverted in fc3981e (see docker-archive/libcontainer#474). This is an issue I'm facing when testing #58, where the kernel doesn't support it but the config is also non-default. I don't agree that we should fail if we have a configuration option that applies to a cgroup which the host kernel doesn't support and also isn't a hard requirement. As it currently stands, the
cgroup.IsNotFound(err)check is essentially a no-op as we fail in.Set()anyway. The only time it is actually useful is where the cgroup has no options to set -- which is just a side-effect of.Set()doing nothing./cc @crosbymichael @vmarmol