Disable VHost when host does not support vhost-net by jiulongzaitian · Pull Request #422 · kata-containers/runtime

jiulongzaitian · 2018-06-20T02:54:05Z

#169
Disable VHost when host does not support vhost-net

Signed-off-by: zhangjie iamkadisi@163.com

katacontainersbot · 2018-06-20T03:06:05Z

PSS Measurement:
Qemu: 145835 KB
Proxy: 4602 KB
Shim: 8980 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007184 KB

katacontainersbot · 2018-06-20T03:21:51Z

PSS Measurement:
Qemu: 148899 KB
Proxy: 4656 KB
Shim: 8875 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007184 KB

egernst · 2018-06-20T03:36:23Z

.gitignore

 /virtcontainers/shim/mock/kata-shim/kata-shim
 /virtcontainers/utils/supportfiles
 /virtcontainers/profile.cov
+.idea/


this shouldn't be included?

Hah， it's goland IDE's trick~

This should go in its own separate commit.

Yes please make sure we don't introduce this into the PR.

egernst

Is it possible to add some test coverage for this? May need to create a namespace to work in for testing (so not to muck with the host's sysfs?)

jshachm · 2018-06-20T04:46:43Z

virtcontainers/network.go

+func haveVhostNetKernelModule() bool {
+	// First, check to see if the vhost module is already loaded
+	if fileExists("/sys/module/vhost_net") {
+		return true


maybe we don't need func fileExsits here since it's really simple : just a Stat and if we really need this func , it should be a utils not just for network itself.

codecov · 2018-06-20T05:20:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c324b55). Click here to learn what that means.
The diff coverage is 44.44%.

@@            Coverage Diff            @@
##             master     #422   +/-   ##
=========================================
  Coverage          ?   63.76%           
=========================================
  Files             ?       87           
  Lines             ?     8831           
  Branches          ?        0           
=========================================
  Hits              ?     5631           
  Misses            ?     2594           
  Partials          ?      606

Impacted Files	Coverage Δ
virtcontainers/qemu_arch_base.go	`77.52% <0%> (ø)`
virtcontainers/network.go	`48.43% <50%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c324b55...6dcad65. Read the comment docs.

katacontainersbot · 2018-06-20T05:34:25Z

PSS Measurement:
Qemu: 144011 KB
Proxy: 4513 KB
Shim: 9092 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007308 KB

WeiZhang555 · 2018-06-20T08:10:52Z

virtcontainers/network.go

+// (either loaded or available to be loaded)
+func haveVhostNetKernelModule() bool {
+	// First, check to see if the vhost module is already loaded
+	if _, err := os.Stat("/sys/module/vhost_net"); !os.IsNotExist(err) {


This isn't right, I think if Stat() returns error, no matter what the error is, it should return false

if _, err := os.Stat(vhostNetKernelModulePath); err != nil { return false }

Fixes: kata-containers#169 Signed-off-by: zhangjie <iamkadisi@163.com>

WeiZhang555 · 2018-06-20T08:22:10Z

virtcontainers/network.go

+	// First, check to see if the vhost module is already loaded
+	if _, err := os.Stat(vhostNetKernelModulePath); !os.IsNotExist(err) {
+		return true
+	}


This isn't right, I think if Stat() returns error, no matter what the error is, it should return false

Further, we have code to do this already in the runtime itself:

https://github.com/kata-containers/runtime/blob/master/cli/kata-check.go#L114..L127

You could potentially move that code into virtcontainers (maybe into virtcontainers/utils/utils.go) and then make the runtime:

import vcu "github.com/kata-containers/runtime/virtcontainers/utils" vcu.haveKernelModule()

wdyt @bergwolf, @sboeuf?

agree @jodh-intel If we have code for it, it is better to move it to virtcontainers/utils/utils.go and reuse it in both the places.

+1 for factorization and reusing the code with kata-check.

katacontainersbot · 2018-06-20T08:33:22Z

PSS Measurement:
Qemu: 146419 KB
Proxy: 4704 KB
Shim: 9001 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007572 KB

sboeuf · 2018-06-20T16:54:29Z

virtcontainers/network.go

+	// First, check to see if the vhost module is already loaded
+	if _, err := os.Stat(vhostNetKernelModulePath); !os.IsNotExist(err) {
+		return true
+	}


+1 for factorization and reusing the code with kata-check.

sboeuf · 2018-06-20T16:57:57Z

.gitignore

 /virtcontainers/shim/mock/kata-shim/kata-shim
 /virtcontainers/utils/supportfiles
 /virtcontainers/profile.cov
+.idea/


Yes please make sure we don't introduce this into the PR.

sboeuf · 2018-06-27T06:18:53Z

@jiulongzaitian any progress on this ?

amshinde · 2018-07-03T20:54:17Z

@jiulongzaitian Any updates on this?

jodh-intel · 2018-07-30T10:21:48Z

Hi @jiulongzaitian - please could you update this PR with the feedback provided (and to resolve the merge conflict)?

grahamwhaley · 2018-08-06T12:36:40Z

Maybe @jiulongzaitian is not available right now - we'll give this another week, and then decide how we push forwards with it...

bergwolf · 2018-08-13T08:07:13Z

@jiulongzaitian Any updates?

jodh-intel · 2018-08-28T13:52:49Z

Ping @jiulongzaitian.

Don't worry if you are not able to update this PR for whatever reason. But it would be useful if you could let us know if you do intend to update it, so we can decide what to do with this PR.

caoruidong · 2018-08-31T08:00:11Z

@jiulongzaitian If you are not available, we can assign another one take over this PR. That's all right

jiulongzaitian · 2018-09-05T08:24:50Z

@jodh-intel @caoruidong i am very very sorry, I have been on a business trip recently. i did not pay attention on it . maybe I haven’t had time recently， you can assign another one to take over this pr. thank you

jodh-intel · 2018-09-05T09:32:36Z

Hi @jiulongzaitian - no problem. Thanks for notifying us. In which case, I think we can use the Assisted PR Process here if anyone is willing to pick up this work?

https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#assisted-pr-workflow

We're happy to support anyone who wants to have a go :-)

https://github.com/kata-containers/community#join-us

jodh-intel · 2018-09-05T09:45:44Z

Note: this is similar to #497.

caoruidong · 2018-09-05T11:23:23Z

I can pick this PR.

jodh-intel · 2018-09-06T09:08:02Z

Thanks @caoruidong! I've assigned the PR (and the issue) to you too now.

Reuse code of kata-check. If host isn't able to load vhost_net.ko, set VHost to false and won't create vhost fds. This commit takes kata-containers#422 and addresses reviewers' comments. Fixes kata-containers#169, kata-containers#422 Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

sboeuf · 2018-09-10T17:33:00Z

@caoruidong I can see that you're working on this, please make sure you let us know when you're done reworking it, so that we can review it.

sboeuf · 2018-09-10T18:20:00Z

Actually, closing this PR, as #706 supersedes it.

Reuse code of kata-check. If host isn't able to load vhost_net.ko, set VHost to false and won't create vhost fds. This commit takes kata-containers#422 and addresses reviewers' comments. Fixes: kata-containers#169 Signed-off-by: zhangjie <iamkadisi@163.com> Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

So that the runtime can set guest time when needed. Fixes: kata-containers#422 Signed-off-by: Peng Tao <bergwolf@gmail.com>

Add function that creates new bridges to increase unit test coverage for virtcontainers/types/bridges. Also adds test for address formats. Fixes kata-containers#422 Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>

jiulongzaitian force-pushed the vhost branch from aa7ee37 to 7e415d2 Compare June 20, 2018 03:08

egernst reviewed Jun 20, 2018

View reviewed changes

jshachm reviewed Jun 20, 2018

View reviewed changes

jiulongzaitian force-pushed the vhost branch from 7e415d2 to 10ece12 Compare June 20, 2018 05:20

WeiZhang555 reviewed Jun 20, 2018

View reviewed changes

virtcontainers : Disable VHost when host does not support vhost-net

6dcad65

Fixes: kata-containers#169 Signed-off-by: zhangjie <iamkadisi@163.com>

jiulongzaitian force-pushed the vhost branch from 10ece12 to 6dcad65 Compare June 20, 2018 08:17

WeiZhang555 suggested changes Jun 20, 2018

View reviewed changes

sboeuf suggested changes Jun 20, 2018

View reviewed changes

jodh-intel assigned caoruidong Sep 6, 2018

caoruidong mentioned this pull request Sep 7, 2018

cli: add configuration option to enable/disable vhost_net #706

Merged

jodh-intel added the do-not-merge label Sep 7, 2018

sboeuf closed this Sep 10, 2018

zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019

grpc: add SetGuestDateTime API

31c6b6e

So that the runtime can set guest time when needed. Fixes: kata-containers#422 Signed-off-by: Peng Tao <bergwolf@gmail.com>

Conversation

jiulongzaitian commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katacontainersbot commented Jun 20, 2018

Uh oh!

katacontainersbot commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

katacontainersbot commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katacontainersbot commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf commented Jun 27, 2018

Uh oh!

amshinde commented Jul 3, 2018

Uh oh!

jodh-intel commented Jul 30, 2018

Uh oh!

grahamwhaley commented Aug 6, 2018

Uh oh!

bergwolf commented Aug 13, 2018

Uh oh!

jodh-intel commented Aug 28, 2018

Uh oh!

caoruidong commented Aug 31, 2018

Uh oh!

jiulongzaitian commented Sep 5, 2018

Uh oh!

jodh-intel commented Sep 5, 2018

Uh oh!

jodh-intel commented Sep 5, 2018

Uh oh!

caoruidong commented Sep 5, 2018

Uh oh!

jodh-intel commented Sep 6, 2018

Uh oh!

sboeuf commented Sep 10, 2018

Uh oh!

sboeuf commented Sep 10, 2018

Uh oh!

Reviewers

Assignees

Labels

jiulongzaitian commented Jun 20, 2018 •

edited

Loading

codecov bot commented Jun 20, 2018 •

edited

Loading