-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Linux Network Devices #4538
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
base: main
Are you sure you want to change the base?
Linux Network Devices #4538
Conversation
bdb31c1
to
0b771ca
Compare
07d3b0b
to
3833056
Compare
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.
LGTM. We are also interested in this use case for our accelerator devices.
67f12e0
to
d114afe
Compare
ec90a02
to
f4f5d02
Compare
735f9d5
to
ce1f612
Compare
6262c5e
to
c530772
Compare
4380e86
to
f53d263
Compare
@aojea friendly reminder that this should be ready soon, we are cutting 1.3.0-rc.1 soon (maybe this week). The spec part is still not sure when it will be merged? |
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.
@aojea I think the bug we were talking on slack is a bug in the validator, see the comments for more info.
f74c032
to
17c7056
Compare
9f4b286
to
20f65c5
Compare
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.
Thanks, it really helps to validate the runtime-spec change is not missing anything.
Left some nit comments. I don't think we should address them now, but before merging this :)
1193b12
to
0809631
Compare
14a603d
to
a92b31b
Compare
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.
Had a quick look, I'll review in detail later. Thanks again for the PR!
for name, netDevice := range p.config.Config.NetDevices { | ||
err := devChangeNetNamespace(name, nsPath, *netDevice) | ||
if err != nil { | ||
return err |
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 wonder what we should do if moving some ifaces work but some fail. Shall we move back to the init netns the ones that we moved to the container?
To be clear, this is an honest question, I don't know what would be the right thing to do. Maybe keep them in the new netns is the right thing, as the kernel will move it back whenever the netns is destroyed and we the code here is quite simple. But if we decide to do that, I'd put a comment here.
// host network pods does not move network devices. | ||
if !p.config.Config.Namespaces.Contains(configs.NEWNET) { | ||
return nil |
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 think you already fail in the config validate if there is no netns.
This might be useful for libct users, but for runc this should never happen.
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.
should I remove it then?
e67014a
to
fa56aeb
Compare
Change-Id: I2f000a2d8440257d9d81192bf073cc5d971039cc Signed-off-by: Antonio Ojea <[email protected]>
Implement support for passing Linux Network Devices to the container network namespace. The network device is passed during the creation of the container, before the process is started. It implements the logic defined in the OCI runtime specification. Signed-off-by: Antonio Ojea <[email protected]>
Implementation of opencontainers/runtime-spec#1271
It implements the new proposal to the OCI spec to be able to specify Network Devices that get attached/detached from the containers