cli: add configuration option to use or not use host netns by caoruidong · Pull Request #733 · kata-containers/runtime

caoruidong · 2018-09-14T10:07:36Z

If disable_new_netns set to true, create VM and shim processes in the host netns

Fixes #731

Signed-off-by: Ruidong Cao caoruidong@huawei.com

jodh-intel · 2018-09-14T10:14:10Z

This will be useful for tracing the agent (Adding OpenTracing support for Kata kata-containers#27), so thanks for raising! 😄
Your branch has conflicts so please rebase.
Please could you add a unit test for this new option.
The issues in your "Fixes" comment mentions disabling the ns for the shim too, but the code only applies to the hypervisor.

jodh-intel · 2018-09-14T10:15:25Z

Oh - also, this new option should be displayed in the output of kata-env ideally (remember to increase the value of formatVersion in kata-env.go).

codecov · 2018-09-15T07:33:54Z

Codecov Report

Merging #733 into master will increase coverage by 0.15%.
The diff coverage is 37.16%.

@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
+ Coverage    66.1%   66.26%   +0.15%     
==========================================
  Files          87       87              
  Lines       10525    10584      +59     
==========================================
+ Hits         6958     7013      +55     
+ Misses       2868     2866       -2     
- Partials      699      705       +6

caoruidong · 2018-09-15T08:00:03Z

Actually config.NetNSPath will influence both shim and VM processes. I have changed commit msg.

But this doesn't work for CNM model. Because we scan endpoints by default and try to attach them, this will break host networking. And libnetwork hook will get a error failed to set gateway while updating gateway: network is unreachable.
So just works with docker run -it --net=none

bergwolf

@caoruidong Did you try it with docker? I'm afraid it might do damage to host network (due to the oci hooks), if someone sets DisableNewNetNs and run it with docker.

bergwolf · 2018-09-17T07:21:55Z

cli/network.go

+			err error
+		)
+		if config.DisableNewNetNs {
+			n, err = ns.GetNS("/proc/1/ns/net")


Please use /proc/self/ns/net instead.

Or we can simply not to setup network namespace when DisableNewNetNs is set, in which case we can avoid setupNetworkNamespace() in kata cli and sandbox createNetwork()/removeNetwork().

Great! I have tried ns.GetCurrentNs() but forgot "/proc/self/ns/net".
I think it's better to skip setupNetworkNamespace(). Let's hear from others.

WeiZhang555 · 2018-09-17T10:39:21Z

This can be treated as an advanced feature and can only be enabled by users who really understand what this option means, I think if user want to use docker libnetwork, they should keep this flag disabled, and only enable it when they want to do custom operations for network. Based on this, I suggest we make a conflict rule:

disable_new_netns conflicts with enable_netmon
disable_new_netns conflicts with internetworking_model=bridge and internetworking_model=macvtap, instead we add a new network model none. disable_new_netns works only with internetworking_model=none

According to rule, any violation should got an error.

jodh-intel · 2018-09-17T10:49:41Z

Sounds good to me but let's also get input from @sboeuf, @amshinde, @mcastelino.

caoruidong · 2018-09-17T11:43:40Z

@bergwolf I have tried it with docker. I think it doesn't break host network but gets an error failed to set gateway while updating gateway: network is unreachable. due to hook.
It works with --net=none

mcastelino · 2018-09-17T16:47:27Z

@sboeuf @amshinde I am surprised that --net=host did not cause any issues. Did you add new code that disabled networking when --net=host is setup? I understand --net=none working.

amshinde · 2018-09-17T17:34:42Z

@mcastelino Yes, --net=host has been diasbled explicitly, explicitly showing an error that this is not supported.

@caoruidong Is this feature for your custom networking?

disable_new_netns conflicts with enable_netmon
disable_new_netns conflicts with internetworking_model=bridge and internetworking_model=macvtap, instead we add a new network model none. disable_new_netns works only with internetworking_model=none

I agree with the error scenarios @WeiZhang555 pointed out. In addition, I think It would be a good idea to skip running the hooks altogether when disable_new_netns is true.

sboeuf · 2018-09-17T18:20:07Z

@caoruidong based on input from @WeiZhang555 and @bergwolf , I think we need to add some documentation as part of this PR.
Because this option should only be used by ppl who know what they're doing, please add both documentation in the code and into a .md file.

Last thing, I would greatly appreciate if you could explain clearly the use case for such thing, since this is going against what we've been doing in Kata, and unless we have a strong use case for it, I don't know if we want it.

caoruidong · 2018-09-18T02:27:05Z

@mcastelino It is documented in https://github.com/kata-containers/documentation/blob/master/Limitations.md#docker---net=host.
This is kind of custom use case, but originally comes from what we discuss in #113 (comment). Thanks to gnawux, he draws a picture #113 (comment).
I will add some documents after we reach a consensus.

WeiZhang555 · 2018-09-18T03:30:10Z

I think It would be a good idea to skip running the hooks altogether when disable_new_netns is true.

@amshinde I'm not sure if there will be any new scenario depending on hook support in addition to "docker libnetwork", so maybe it's not good idea to disable hooks.

jodh-intel · 2018-10-18T08:56:05Z

cli/config/configuration.toml.in

 #     macvtap.
+#
+#   - none
+#     Used when customize network. Only creates a tap device. No veth pair.


I think it would be useful to add a link here to the following as that shows users visually more about how this will work:

https://github.com/kata-containers/documentation/blob/master/architecture.md#networking

OK. I will add a PR in documentation repo.

caoruidong · 2018-10-19T06:16:57Z

@bergwolf Comments addressed in this PR.

jshachm · 2018-10-22T01:32:54Z

LGTM , and waiting for that we fix the strange error of CI.

Refactor these functions so differernt types of endpoints can use a unified function to hotplug nics. Fixes kata-containers#731 Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

…point This model is for not creating a new net ns for VM and directly creating taps in the host net ns. Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

If `disable_new_netns` set to true, create VM and shim processes in the host netns Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

Add unit test for `disable_new_netns` Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

raravena80 · 2018-10-22T16:39:33Z

@caoruidong ping from your weekly Kata herder. Doing re-test?

caoruidong · 2018-10-23T06:32:22Z

@amshinde @sboeuf branch update

sboeuf

Thanks @caoruidong for all the reworks :)
LGTM

caoruidong · 2018-10-24T01:40:40Z

/test

WeiZhang555 · 2018-10-24T03:35:22Z

LGTM.

As we got 4 LGTMs, I'll merge this now.
The design has been discussed a lot so I think it's safe to merge, we can keep improving detail implementation later.

egernst added the review label Sep 14, 2018

caoruidong added the wip label Sep 14, 2018

sboeuf added the enhancement Improvement to an existing feature label Sep 14, 2018

caoruidong changed the title ~~cli: add configuration option to enable/disable hypervisor in host netns~~ cli: add configuration option to use or not use host netns Sep 14, 2018

caoruidong force-pushed the qemu-ns branch 2 times, most recently from 2ebbf5d to 53fb135 Compare September 15, 2018 03:57

kata-containers deleted a comment from katacontainersbot Sep 15, 2018

caoruidong force-pushed the qemu-ns branch 2 times, most recently from 790aa23 to 619c56d Compare September 15, 2018 06:55

caoruidong removed the wip label Sep 15, 2018

bergwolf reviewed Sep 17, 2018

View reviewed changes

jodh-intel added the feature New functionality label Sep 17, 2018

caoruidong force-pushed the qemu-ns branch from 619c56d to f1ff824 Compare September 17, 2018 08:58

caoruidong force-pushed the qemu-ns branch from f1ff824 to 0ad165f Compare September 17, 2018 11:36

caoruidong force-pushed the qemu-ns branch 3 times, most recently from 0e34cfc to 57b40ca Compare September 20, 2018 10:56

kata-containers deleted a comment from katacontainersbot Sep 20, 2018

jodh-intel mentioned this pull request Oct 18, 2018

[TEST PR - DNM] Nonetns #835

Closed

jodh-intel reviewed Oct 18, 2018

View reviewed changes

caoruidong force-pushed the qemu-ns branch 3 times, most recently from 214ffd1 to 1482f03 Compare October 19, 2018 03:29

caoruidong added the do-not-merge label Oct 19, 2018

caoruidong force-pushed the qemu-ns branch 2 times, most recently from 15430c9 to 0971987 Compare October 19, 2018 09:02

caoruidong removed the do-not-merge label Oct 19, 2018

jshachm approved these changes Oct 22, 2018

View reviewed changes

caoruidong added the do-not-merge label Oct 22, 2018

caoruidong force-pushed the qemu-ns branch from 0971987 to 7a5a57d Compare October 22, 2018 02:05

kata-containers deleted a comment from jshachm Oct 22, 2018

caoruidong added 4 commits October 22, 2018 21:06

virtcontainers: refactor hotplug qmp functions

f8f2962

Refactor these functions so differernt types of endpoints can use a unified function to hotplug nics. Fixes kata-containers#731 Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

network: add new NetInterworkingModel "none" and endpoint type TapEnd…

6935279

…point This model is for not creating a new net ns for VM and directly creating taps in the host net ns. Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

cli: add configuration option to use or not use host netns

14e5437

If `disable_new_netns` set to true, create VM and shim processes in the host netns Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

cli: test: add unit test for kata-env and kata-check

7a5a57d

Add unit test for `disable_new_netns` Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

caoruidong removed the do-not-merge label Oct 23, 2018

grahamwhaley mentioned this pull request Oct 23, 2018

CI: Make static check ignore local URLs kata-containers/tests#838

Merged

sboeuf approved these changes Oct 23, 2018

View reviewed changes

WeiZhang555 merged commit 5a8b738 into kata-containers:master Oct 24, 2018

caoruidong deleted the qemu-ns branch October 24, 2018 06:26

jodh-intel mentioned this pull request Nov 23, 2018

Cannot create container when disable_new_netns=true #935

Closed

Conversation

caoruidong commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jodh-intel commented Sep 14, 2018

Uh oh!

jodh-intel commented Sep 14, 2018

Uh oh!

codecov bot commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

caoruidong commented Sep 15, 2018

Uh oh!

bergwolf left a comment

Choose a reason for hiding this comment

Uh oh!

bergwolf Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

caoruidong Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 commented Sep 17, 2018

Uh oh!

jodh-intel commented Sep 17, 2018

Uh oh!

caoruidong commented Sep 17, 2018

Uh oh!

mcastelino commented Sep 17, 2018

Uh oh!

amshinde commented Sep 17, 2018

Uh oh!

sboeuf commented Sep 17, 2018

Uh oh!

caoruidong commented Sep 18, 2018

Uh oh!

WeiZhang555 commented Sep 18, 2018

Uh oh!

jodh-intel Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

caoruidong Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

caoruidong commented Oct 19, 2018

Uh oh!

jshachm commented Oct 22, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raravena80 commented Oct 22, 2018

Uh oh!

caoruidong commented Oct 23, 2018

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

caoruidong commented Oct 24, 2018

Uh oh!

WeiZhang555 commented Oct 24, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

caoruidong commented Sep 14, 2018 •

edited

Loading

codecov bot commented Sep 15, 2018 •

edited

Loading

jshachm commented Oct 22, 2018 •

edited by amshinde

Loading

WeiZhang555 commented Oct 24, 2018 •

edited by amshinde

Loading