Skip to content
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

runc init: remove some code #3113

Merged
merged 2 commits into from
Jul 29, 2021
Merged

Conversation

kolyshkin
Copy link
Contributor

Remove some code from runc init which is not needed. Please review commit-by-commit.

  1. Remove option parsing from runc init.
  2. Remove cgroupns sync mechanism (as pointed out in (*initProcess).start: rm second Apply #2446 and in TODO).

@kolyshkin kolyshkin added the kind/refactor refactoring label Jul 27, 2021
@kolyshkin kolyshkin requested a review from cyphar July 27, 2021 18:33
runc init is special. For one thing, it needs to do a few things before
main(), so we have func init() that checks if we're init and does that.

What happens next is main() is called, which does some options parsing,
figures out it needs to call initCommand.Action and so it does.

Now, main() is entirely unnecessary -- we can do everything right from
init().

Hopefully the change makes things slightly less complicated.

From a user's perspective, the only change is runc help no longer
lists 'runc init` (which I think it also good).

Signed-off-by: Kir Kolyshkin <[email protected]>
As pointed out in TODO item added by commit 64bb59f, it is not
necessary to have a special sync mechanism for cgroupns, as the parent
adds runc init to cgroup way earlier (before sending nl bootstrap data.

This sync was added by commit df3fa11, which was also added a
second cgroup manager.Apply() call, later removed in commit
d1ba8e3. It seems the original author had the idea to wait for
that second Apply().

Fixes: df3fa11
Signed-off-by: Kir Kolyshkin <[email protected]>
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

)

func init() {
if len(os.Args) > 1 && os.Args[1] == "init" {
// This is the golang entry point for runc init, executed
// before main() but after libcontainer/nsenter's nsexec().
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not hurt to mention the function never returns (since this all ends in execve) so main never actually runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had something like that but eventually removed it as this is somewhat clear (for one thing, the code within the block ends with a panic).

I will try to improve this further in #3114 (which is a followup to this PR).

@kolyshkin kolyshkin merged commit 2aabb29 into opencontainers:master Jul 29, 2021
@kolyshkin kolyshkin deleted the init-rm-code branch December 2, 2021 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants