Skip to content

libcontainer: fix InitArgs options func to set factory.InitPath#1888

Closed
nickethier wants to merge 3 commits into
opencontainers:mainfrom
hashicorp:fix-init-args
Closed

libcontainer: fix InitArgs options func to set factory.InitPath#1888
nickethier wants to merge 3 commits into
opencontainers:mainfrom
hashicorp:fix-init-args

Conversation

@nickethier
Copy link
Copy Markdown

Greetings!

From the godocs it looks like InitArgs should be setting the full path and args for the init process:

InitArgs returns an options func to configure a LinuxFactory with the provided init binary path and arguments.

This isn't the case so here is a PR to fix that + a unit test.

Thanks!
-Nick

Signed-off-by: Nick Ethier <nethier@hashicorp.com>
Signed-off-by: Nick Ethier <nethier@hashicorp.com>
Signed-off-by: Nick Ethier <nethier@hashicorp.com>
@BooleanCat
Copy link
Copy Markdown

BooleanCat commented Dec 21, 2018

I don't think we should be changing the InitPath to something other than /proc/self/exe. How else would runc syncronise containerisation of the init process without this unless you provide another binary that can syncronize in the same way?

From my reading of runc's code the flow looks like:

  1. Set InitPath to /proc/self/exe, set the args to [init, ... other args]
  2. runc invokes a child process with /proc/self/exe init other args
  3. runc and the child communicate syncronisation for a bit while namespaces and such are set up
  4. the process defined in other_args[0] is execed

I think the intention of InitArgs is to enable you to change the init process that's eventually execed by the runc init child. Modying InitPath seems like a separate concern - perhaps the issue here is simply misleading wording in the comment?

The syncronisation dance I mentioned is document here in beautiful ASCII art: https://github.com/opencontainers/runc/blob/master/libcontainer/sync.go

@nickethier
Copy link
Copy Markdown
Author

@BooleanCat Thanks for the reply!

Under most circumstances I would agree that changing to anything other than /proc/self/exe is generally odd. The use case we have is in testing. When go test runs it compiles the tests into a package.test executable. In our test cases we're launching containers through libcontainer, so /proc/self/exe isn't actually the same binary/code path that we'd use outside of tests. I think we could get around this with a TestMain but it would mean launching containers goes through a slightly different code path in testing.

Concrete example:
In Nomad the libcontainer init is started by running nomad libcontainer-shim. We have a helper to discover the path to the binary (which works in tests as well) and pass it into the libcontainer factory: https://github.com/hashicorp/nomad/blob/03bf0bb65a8c2404b53f30328f2da262d0c42370/drivers/shared/executor/executor_linux.go#L123-L127
This way the init is hit when we launch a container inside a test (instead of running package.test init which would not work). https://github.com/hashicorp/nomad/blob/03bf0bb65a8c2404b53f30328f2da262d0c42370/drivers/shared/executor/executor_linux_test.go#L94

notnoop pushed a commit to hashicorp/nomad that referenced this pull request Mar 18, 2019
Use a custom initializer that enables us to use upstream runc, rather
than our fork.

Until opencontainers/runc#1888 is merged.
notnoop pushed a commit to hashicorp/nomad that referenced this pull request Mar 19, 2019
Use a custom initializer that enables us to use upstream runc, rather
than our fork.

Until opencontainers/runc#1888 is merged.
@nickethier nickethier closed this May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants