nsexec: replace usage of environment variable with netlink message #340
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
|
ping @crosbymichael @LK4D4 @avagin for review |
|
👍 to taking out runc from the names. |
dqminh
force-pushed
the
replace-env-netlink
branch
from
46eaf29 to
9c45132
Compare
| native := nl.NativeEndian() | ||
| native.PutUint16(buf[0:2], uint16(msg.Len())) | ||
| native.PutUint16(buf[2:4], msg.Type) | ||
| native.PutUint32(buf[4:8], msg.Value) |
There was a problem hiding this comment.
Why we can't do something like this https://github.com/vishvananda/netlink/blob/master/nl/tc_linux.go#L99 ?
There was a problem hiding this comment.
I dont think we should do that. It depends on an undocumented feature of the runtime i.e the struct fields' memory layout is in order (golang/go#10014). Doing this is much safer and future-proof imo.
add bootstrap data to setns process. If we have any bootstrap data then copy it to the bootstrap process (i.e. nsexec) using the sync pipe. This will allow us to eventually replace environment variable usage with more structured data to setup namespaces, write pid/gid map, setgroup etc. Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com>
dqminh
force-pushed
the
replace-env-netlink
branch
from
9c45132 to
b4c14ba
Compare
dqminh
force-pushed
the
replace-env-netlink
branch
from
b4c14ba to
c416a8d
Compare
dqminh
force-pushed
the
replace-env-netlink
branch
from
c416a8d to
5bc6526
Compare
| len = NLMSG_PAYLOAD(nh, 0); | ||
| char data[len]; | ||
| len = read(pipenum, data, len); | ||
| if (len <= 0) { |
There was a problem hiding this comment.
It's better to check that we get all data here
|
LGTM. Thanks! |
|
@mrunalp @crosbymichael @LK4D4 PTAL :D |
| @@ -1021,3 +1025,75 @@ func (c *linuxContainer) currentState() (*State, error) { | |||
| } | |||
| return state, nil | |||
| } | |||
|
|
|||
| const ( | |||
There was a problem hiding this comment.
Can we move this message code outside of this file and into maybe a message.go file? This file is already pretty large.
|
Nevermind the last comment. I see that it got removed in a later commit. |
dqminh
force-pushed
the
replace-env-netlink
branch
from
d46e0ca to
d7fcd15
Compare
dqminh
force-pushed
the
replace-env-netlink
branch
from
d7fcd15 to
7f7dff4
Compare
|
@crosbymichael @mrunalp moved the messages to message_linux.go and rebased. |
replace passing of pid and console path via environment variable with passing them with netlink message via an established pipe. this change requires us to set _LIBCONTAINER_INITTYPE and _LIBCONTAINER_INITPIPE as the env environment of the bootstrap process as we only send the bootstrap data for setns process right now. When init and setns bootstrap process are unified (i.e., init use nsexec instead of Go to clone new process), we can remove _LIBCONTAINER_INITTYPE. Note: - we read nlmsghdr first before reading the content so we can get the total length of the payload and allocate buffer properly instead of allocating one large buffer. - check read bytes vs the wanted number. It's an error if we failed to read the desired number of bytes from the pipe into the buffer. Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com>
dqminh
force-pushed
the
replace-env-netlink
branch
from
7f7dff4 to
7d423cb
Compare
|
LGTM |
1 similar comment
|
LGTM |
mrunalp
pushed a commit
that referenced
this pull request
Dec 9, 2015
nsexec: replace usage of environment variable with netlink message
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
add omitempty to 'Devices
stefanberger
pushed a commit
to stefanberger/runc
that referenced
this pull request
Sep 8, 2017
To match the omitempty which the Go property has had since 28cc423 (add omitempty to 'Device' and 'Namespace', 2016-03-10, opencontainers#340). Signed-off-by: W. Trevor King <wking@tremily.us>
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.
replace passing of pid and console path via environment variable with passing them with netlink message via an established pipe.
This is the first step in the process to unify the setup process between init and setns. We can use the netlink message to pass more structured data into the nsexec bootstrap process to eventually setup custom namespaces, setgroup, uid/gid map etc.