-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deps: upgrade opencontainer/runc to 1.2 #25138
Draft
pkazmierczak
wants to merge
7
commits into
main
Choose a base branch
from
f-runc-upgrade
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
010c11e
deps: upgrade opencontainer/runc to 1.2
pkazmierczak 2c2bf3b
fix nsenter init
pkazmierczak fab5714
go mod tidy
pkazmierczak 39d12bc
path correction
pkazmierczak 8efe0c9
remove shim
pkazmierczak 670a4a1
nosemgrep on time after leak
pkazmierczak ef8948a
adds init process and cgroup hack
mismithhisler File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we dropping the shim argument in the process? I think we need that to trigger the
init()
function indrivers/shared/executor/libcontainer_nsenter_linux.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fixing this, sorry got distracted by other stuff today :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. apparently we can't
InitArgs
anymore :/opencontainers/runc@630c0d7
I will admit that I'm not understanding the context here, the libcontainer-shim intended behavior is a mystery to me here. Is this perhaps something we can obsolete/remove? Otherwise the best course of action would be to stay at runc 1.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Nomad client is running with as many "M-threads" as there are cores on the host because that's the default behavior of the Go runtime. But we don't want that to be the case with the
executor
process we create, to reduce resource overhead. So we inject a shim between to change the gomaxprocs and pin to the main thread for correctness in signal handling.Even if the "init args" is broken, there's probably still a place for us to hook in somewhere here, as
runc
is basically just a giant pile of these shims that get called in order (that's what we're attempting to hook in #20017 for example). We probably can't leaverunc
at 1.1 for very long, as it's a security-sensitive bit of our stack.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a little more context ahead of our sidebar discussion, here's what the relevant parts of my process tree looks like if I run a job with the
exec
task driver:But if I use
execsnoop
(I'm using a slightly hacked version) I can see that we actually end up exec'ing into a series of processes from the executor:Looking at that, I'm a lot less sure about what the shim is actually doing for us here. Let's dig into this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our sidebar discussion: it looks like the only thing the libcontainer-shim does for us today is reconfigures the logging we get from libcontainer. But it also looks like that the logging environment in the init shim has improved in libcontainer quite a bit since this was written. So @pkazmierczak is going to test out removing the shim and verifying we get some reasonable logs out from libcontainer failures.