Fix process/config properties merging #4585
Open
+180
−64
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
kolyshkin
force-pushed
the
per-process-properties
branch
from
a12d2da to
ed015c7
Compare
kolyshkin
force-pushed
the
per-process-properties
branch
6 times, most recently
from
b39f21b to
8bab6ac
Compare
kolyshkin
changed the title
|
|
rata
reviewed
Jan 21, 2025
There was a problem hiding this comment.
@kolyshkin thanks for tackling this, very nice cleanups!
Do we want to backport this to 1.2?
Left some minor questions, this mostly LGTM.
My view here is, unless someone will report this, let's view these as improvements rather than bug fixes -- thus no backport. |
Another reason is, for backporting I'd do it differently (minimal set of fixes rather than what I did here, helping the future me to figure out this mess) |
kolyshkin
commented
Jan 30, 2025
kolyshkin
force-pushed
the
per-process-properties
branch
from
8bab6ac to
bad011e
Compare
|
Oh my, this needs a manual rebase since #3999 is merged. Update: rebased |
This is one of the dark corners of runc / libcontainer, so let's shed some light on it. initConfig is a structure which is filled in [mostly] by newInitConfig, and one of its hidden aspects is it contains a process config which is the result of merge between the container and the process configs. Let's document how all this happens, where the fields are coming from, which one has a preference, and how it all works. Signed-off-by: Kir Kolyshkin <[email protected]>
They are passed in initConfig twice, so it does not make sense. NB: the alternative to that would be to remove Config field from initConfig, but it results in a much bigger patch and more maintenance down the road. Signed-off-by: Kir Kolyshkin <[email protected]>
In runtime-spec, capabilities property is optional, but libcontainer/capabilities panics when New(nil) is called. Because of this, there's a kludge in finalizeNamespace to ensure capabilities.New is not called with nil argument, and there's a TestProcessEmptyCaps to ensure runc won't panic. Let's fix this at the source, allowing libct/cap to work with nil capabilities. (The caller is fixed by the next commit.) Signed-off-by: Kir Kolyshkin <[email protected]>
For all other properties that are available in both Config and Process, the merging is performed by newInitConfig. Let's do the same for Capabilities for the sake of code uniformity. Also, thanks to the previous commit, we no longer have to make sure we do not call capabilities.New(nil). Signed-off-by: Kir Kolyshkin <[email protected]>
Commit bfbd030 added IOPriority field into both Config and Process, but forgot to add a mechanism to actually use Process.IOPriority. As a result, runc exec does not set Process.IOPriority ever. Fix it, and a test case (which fails before the fix). Signed-off-by: Kir Kolyshkin <[email protected]>
Commit 770728e added Scheduler field into both Config and Process, but forgot to add a mechanism to actually use Process.Scheduler. As a result, runc exec does not set Process.Scheduler ever. Fix it, and a test case (which fails before the fix). Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
per-process-properties
branch
from
bad011e to
7e26176
Compare
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.
runc exec (as well as libcontainer's
container.Run/container.Start) has an interesting feature of deriving various process' properties from the runtime spec for container init (theProcessentry inconfig.jsonaka spec).This is mostly implemented via merging
ConfigandProcessproperties intoinitConfig, which happens in libcontainer/container_linux.go, func newInitConfig. Note howinitConfigfields are taken fromc.configfirst (they are there from spec'sProcess) and then are overwritten by properties of a particular process being run. TheseinitConfigfields are then used to set up various process attributes.Alas, this functionality (of merging process and config properties) is
This PR is an attempt to improve the situation,