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

[broadcom]: respect the current network namespace when creating netdev #3896

Merged
merged 1 commit into from
Dec 23, 2019

Conversation

ishidawataru
Copy link
Collaborator

@ishidawataru ishidawataru commented Dec 13, 2019

Broadcom-Switch/OpenNSL#26

Signed-off-by: Wataru Ishida [email protected]

@lguohan
Copy link
Collaborator

lguohan commented Dec 13, 2019

retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented Dec 13, 2019

I do not see other netdev drivers have this patch. Do you know if this is a standard behavior in kernel module?

@ishidawataru
Copy link
Collaborator Author

It depends on the device.
TUN device which I think similar to KNET device sets the current netns when creating netdevice.

https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L2789

ref: list of dev_net_set usage

https://elixir.bootlin.com/linux/latest/ident/dev_net_set

This patch won't change the current SONiC behavior since all SONiC containers are running with --net=host.
However, I think it is better to run containers which need access to front panel ports (like syncd/swss/frr/lldp) in a separate network namespace so that we can avoid configuring the netdevs directly without using SONiC toolchains from the default network namespace (e.g. assign an IP address via ip command which is not recommended in SONiC ).

@lguohan
Copy link
Collaborator

lguohan commented Dec 13, 2019

you assume the syncd container will be in the same namespace as swss. it is not always the case. a more generic solution is the create the netdev in global namespace and have a user space daemon to move the netdev to the right namespace.

@ishidawataru
Copy link
Collaborator Author

ishidawataru commented Dec 13, 2019

Yes, I was assuming putting syncd and swss ( and other daemons that need front panel port access ) in the same netns.
What would be the motivation to put syncd and swss in different namespaces?

I think it's not consistent with other virtual netdev like TUN or veth if we create KNET netdev in a network namespace other than the default and it pops out in the default one.
I feel more natural that the KNET will get created in the netns of the user.

If we need to move it to another namespace, we can do that after creating it.

@lguohan
Copy link
Collaborator

lguohan commented Dec 14, 2019

we are working on a case a single syncd support multiple asic, in that case, all the netdevs created by syncd will likely not in the same namespace. but I think your have a point. this knet driver is more like a tun/tap device than other traditional netdev.

@lguohan
Copy link
Collaborator

lguohan commented Dec 16, 2019

@judyjoseph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants