Use VSOCK when is available in the host. by jcvenegas · Pull Request #497 · kata-containers/runtime

jcvenegas · 2018-07-18T16:28:27Z

This PR completes the support to use vsock and no proxy configuration.

In order to create add VSOCK support we added:

coldplug support for VSOCK socket device.
Implemented a way to find a context ID based in a random search.
A random number is generated between 3 and max uint32 (4'294,967,295)
and uses it as context ID. The range is large enough to have a low collision probability. Using ioctl checks if the context ID is free, else find the immediate next free context ID.
When the hypervisor option use_vsock is true the runtime will check for vsock support . If vsock is supported, not proxy will be used and the shims will connect to the VM using VSOCKS. This flag is true by default, so will use VSOCK when possible.
The kata-env subcommand now shows information about if VSOCK will be used or not.

fixes #383

Signed-off-by: Jose Carlos Venegas Munoz jose.carlos.venegas.munoz@intel.com
Signed-off-by: Julio Montes julio.montes@intel.com

sboeuf · 2018-07-18T16:30:48Z

Thanks guys! Will take a look today ;)

egernst · 2018-07-18T16:33:01Z

virtcontainers/qemu.go

 	defer func(qemu *qemu) {
 		if q.qmpMonitorCh.qmp != nil {
 			q.qmpMonitorCh.qmp.Shutdown()
+			q.qmpMonitorCh.qmp = nil


is this specific to the vsock addition? Can you clarify why this is part of the initial commit?

I guess you're trying to fix a wrong behavior that you have seen somewhere? If this is needed, it should be part of a separate commit with the reason for this.

We use the qmp socket later to hotoplug the vsock device
Because we call Shutdown to close the qmp channel, later when we try to inialize the qmp connection again we dont do it because the qmp instance already exist but with the channel closed leading to fail to use it.

@jcvenegas This seems to be an existing bug, can you put this in a separate commit with the explanation you provided above as the commit message.

This is being taken care of by #501

sweet, not needed anymore.

Yeah #501 has been merged, you can simply remove the commit now :) (after rebasing obviously!)

egernst · 2018-07-18T16:37:40Z

virtcontainers/qemu.go

+			return 0, err
+		}
+
+		q.qmpMonitorCh.qmp = qmp


I see that we aren't consistent on the QMP channel setup throughout the file, and that this appears to be boiler plate code. Should we standardize? See hotplugAddMemory and hotplugBlockDevice. WDYT?

yes, I've seen the same with in the muitples places we setup a QMP channel. I agree we should refactor that to avoid code duplication.

Yes, this should be definitely done to avoid bugs. It can be done in another PR though. Please open an issue to make sure we don't loose track of this.

Or even as a first step, at least be consistent, since we seem to have a couple different patterns here...

sboeuf · 2018-07-18T16:40:50Z

virtcontainers/hypervisor.go

 	// memoryDevice is memory device type
 	memoryDev
+
+	// vSockDev is vsock device  type


there's two spaces between device and type

sboeuf · 2018-07-18T16:42:38Z

virtcontainers/qemu.go

 	defer func(qemu *qemu) {
 		if q.qmpMonitorCh.qmp != nil {
 			q.qmpMonitorCh.qmp.Shutdown()
+			q.qmpMonitorCh.qmp = nil


I guess you're trying to fix a wrong behavior that you have seen somewhere? If this is needed, it should be part of a separate commit with the reason for this.

sboeuf · 2018-07-18T16:48:07Z

virtcontainers/qemu.go


+func (q *qemu) hotplugVSock(op operation) (uint32, error) {
+	if op == removeDevice {
+		return 0, errors.New("Not implemented")


Could you generate a more verbose message here. Also, I don't think we would want to implement it since we won't change the way we communicate with the agent during the VM lifetime. So I would simply say that it's not supported because there's no reason to do so.

sboeuf · 2018-07-18T16:56:41Z

virtcontainers/qemu.go

+			return 0, err
+		}
+
+		q.qmpMonitorCh.qmp = qmp


Yes, this should be definitely done to avoid bugs. It can be done in another PR though. Please open an issue to make sure we don't loose track of this.

sboeuf · 2018-07-18T16:57:29Z

virtcontainers/qemu.go

+			q.qmpMonitorCh.qmp = nil
+		}()
+	}
+	// TODO: Find a free contextID


I guess you can remove this TODO since you solved the issue later in the other commits, right ?
And you should re-order the commits to avoid such hardcoding of the cid.

sboeuf · 2018-07-18T17:43:24Z

virtcontainers/kata_agent.go

 		}
 	case kataVSOCK:
-		// TODO Add an hypervisor vsock
+		k.Logger().Debug("Using vSock device will be plugged later")


This makes me think, we should maybe try to also hotplug the serial ports in case we don't use vsock.

nop, serial ports don't need a unique context ID, from #383

Since we don't know the context IDs that are being used by other processes, we have to check if the context ID we are about to use is free (ioctl VHOST_VSOCK_SET_GUEST_CID)
even implementing above points, we might face race conditions with other processes, because of ioctl VHOST_VSOCK_SET_GUEST_CID doesn't lock the context ID and other process can use it before we launch the QEMU process, to avoid this issue vsocks must be hot plugged

The idea is not increase the boot time for the serial port case, so we decied to not hotplug and detect if vsock will be used early in the setup. An extra validation that may help is to not add a serial port vsock is supported to reduce memory footprint.

Sorry if I was not clear but I'm just saying that in the case this switch find we'll use serial port, we could also hotplug the serial port, instead of cold plugging it.
Problem is, if we do that, the logic you introduced from the agent (first checking serial and then checking vsock) would not work. It'd be nice to have a way to indicate to the agent which type of communication is being used...

sounds good, but not for this PR

fair enough!

sboeuf · 2018-07-18T17:45:34Z

virtcontainers/kata_agent.go

 	type9pFs              = "9p"
 	vsockSocketScheme     = "vsock"
+	vSockDevicePath       = "/dev/vsock"
+	vSockPort             = 1024


This needs to be in sync with the agent, right? If that's the case, we should define it on the agent side and import this through the agent vendoring.

agree, will do in other PR

sboeuf · 2018-07-18T17:54:02Z

virtcontainers/qemu.go

-func findContextID() (uint32, error) {
-	// Fixme
-	return 3, nil
+func (q *qemu) findContextID() (uint32, error) {


Could you please add a big chunk of comment here to explain why this function is needed and what it does to work around the issue.

sboeuf · 2018-07-18T17:57:17Z

virtcontainers/kata_agent.go

 	return uint32(cid), uint32(port), nil
 }

-func supportsVsocks() bool {


Ah nice, I see that you have added the configuration I was mentioning in my previous comment, that's good !

sboeuf · 2018-07-18T17:59:20Z

cli/config.go

 	}

+	useVSOCK := false
+	if h.useVSOCK() && utils.SupportsVsocks() {


I think it could be good to print some log here in case the user wants to use vsock but the system actually does not allow it because there is no vsock support.
Otherwise, the user might think enabling the flag will work, but in reality the legacy case with the proxy and serial port will be used.

Something like this:

if h.useVSOCK() { if utils.SupportsVsocks() { kataLog.Info("vsock supported") useVSOCK = true } else { kataLog.Warn("No vsock support, falling back to legacy serial port") }

amshinde · 2018-07-18T18:08:01Z

virtcontainers/sandbox.go


 	s.Logger().Info("VM started")

+	data, err := s.hypervisor.hotplugAddDevice(nil, vSockDev)


Agree, this should be taken care by the agent.

amshinde · 2018-07-18T18:28:50Z

virtcontainers/kata_agent.go

 	return uint32(cid), uint32(port), nil
 }

-func supportsVsocks() bool {


So you have deleted function supportsVsocks introduced in a previous commit and added SupportsVsocks here.
Could you rebase/reorganise your commits so that we just have SupportsVsocks

amshinde · 2018-07-18T18:31:05Z

virtcontainers/kata_agent.go

 	return virtLog.WithField("subsystem", "kata_agent")
 }

-func parseVSOCKAddr(sock string) (uint32, uint32, error) {


Can you do add -p style commits so that this function is not introduced in the first place?

opendev-zuul · 2018-07-18T18:34:36Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 52m 50s

katacontainersbot · 2018-07-18T18:45:09Z

PSS Measurement:
Qemu: 170820 KB
Proxy: 5003 KB
Shim: 9112 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007540 KB

katacontainersbot · 2018-07-18T19:15:15Z

PSS Measurement:
Qemu: 163202 KB
Proxy: 4935 KB
Shim: 8832 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007220 KB

katacontainersbot · 2018-07-18T19:37:25Z

PSS Measurement:
Qemu: 162722 KB
Proxy: 4922 KB
Shim: 8827 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007392 KB

katacontainersbot · 2018-07-18T19:49:54Z

PSS Measurement:
Qemu: 168901 KB
Proxy: 4980 KB
Shim: 8856 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007236 KB

katacontainersbot · 2018-07-18T20:03:27Z

PSS Measurement:
Qemu: 165138 KB
Proxy: 4914 KB
Shim: 8820 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007408 KB

opendev-zuul · 2018-07-18T21:06:50Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 47m 21s

bergwolf · 2018-07-19T08:49:43Z

virtcontainers/kata_agent.go

+		sandbox.Logger().Info("Got context ID ", contextID)
+		s.contextID = contextID
+		s.port = uint32(vSockPort)
+		k.vmSocket = s


Please persist the vsock CID and port info so that we know how to reconnect to an existing guest.

@bergwolf it already save the URL to reconnect in the state.URL.

codecov · 2018-07-19T12:06:55Z

Codecov Report

Merging #497 into master will decrease coverage by 0.13%.
The diff coverage is 65.47%.

@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
- Coverage   66.66%   66.53%   -0.14%     
==========================================
  Files          93       94       +1     
  Lines        9608     9672      +64     
==========================================
+ Hits         6405     6435      +30     
- Misses       2516     2546      +30     
- Partials      687      691       +4

katacontainersbot · 2018-07-19T12:17:35Z

PSS Measurement:
Qemu: 162403 KB
Proxy: 4984 KB
Shim: 9004 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2006368 KB

katacontainersbot · 2018-07-19T12:41:27Z

PSS Measurement:
Qemu: 164649 KB
Proxy: 4988 KB
Shim: 9202 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007104 KB

opendev-zuul · 2018-07-19T13:22:52Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : POST_FAILURE in 40m 12s

katacontainersbot · 2018-07-19T14:00:41Z

PSS Measurement:
Qemu: 166697 KB
Proxy: 5043 KB
Shim: 8934 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007376 KB

opendev-zuul · 2018-07-19T14:29:20Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 39m 53s

sboeuf · 2018-07-30T15:40:43Z

CI looks good :)
@bergwolf waiting for your input on this one. Please merge if you're okay with this ;)

bergwolf · 2018-07-31T02:43:04Z

LGTM.

@devimc please fix up codecov/project 66.16% (-0.51%) then this can be merged. This is fair amount of change and it would be good that we do not lose codecov/project upon it.

add vhostfd and disable-modern to vhost-vsock-pci shortlog: 3830b44 qemu: add vhostfd and disable-modern to vhost-vsock-pci f700a97 qemu/qmp: implement function to hotplug vsock-pci Signed-off-by: Julio Montes <julio.montes@intel.com>

Implement function to check if the system has support for vsocks. This function looks for vsock and vhost-vsock devices returning true if those exist, otherwise false. Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com> Signed-off-by: Julio Montes <julio.montes@intel.com>

FindContextID generates a random number between 3 and max uint32 and uses it as context ID. Using ioctl findContextID checks if the context ID is free, if the context ID is being used by other process, this function iterates from over all valid context IDs until one is available. `/dev/vhost-vsock` is used to check what context IDs are free, we need it to ensure we are using a unique context ID to create the vsocks. Signed-off-by: Julio Montes <julio.montes@intel.com>

We already save the URL used to connect to the agent in the `state.URL` this variable is the used to connect the shim to agnet independently the socket type (VSOCK or serial) Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

katacontainersbot · 2018-07-31T15:48:54Z

PSS Measurement:
Qemu: 171993 KB
Proxy: 6077 KB
Shim: 9146 KB

Memory inside container:
Total Memory: 2043480 KB
Free Memory: 2003704 KB

Add `use_vsock` option to enable or disable the use of vsocks for communication between host and guest. Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com> Signed-off-by: Julio Montes <julio.montes@intel.com>

In order to see what proxy was started or not, we should log its type and the URL Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com> Signed-off-by: Julio Montes <julio.montes@intel.com>

add extra field in KataAgentConfig structure to specify if the kata agent have to use a vsock instead of serial port. Signed-off-by: Julio Montes <julio.montes@intel.com>

`appendVSockPCI` function can be used to cold plug vocks, vhost file descriptor holds the context ID and it's inherit by QEMU process, ID must be unique and disable-modern prevents qemu from relying on fast MMIO. Signed-off-by: Julio Montes <julio.montes@intel.com>

parseVSOCKAddr function is no more needed since now agent config contains a field to identify if vsocks should be used or not. Signed-off-by: Julio Montes <julio.montes@intel.com>

opendev-zuul · 2018-07-31T19:10:48Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : RETRY_LIMIT in 4m 46s

When the hypervisor option `use_vsock` is true the runtime will check for vsock support. If vsock is supported, not proxy will be used and the shims will connect to the VM using VSOCKS. This flag is true by default, so will use VSOCK when possible and no proxy will be started. fixes kata-containers#383 Signed-off-by: Jose Carlos Venegas Munoz jose.carlos.venegas.munoz@intel.com Signed-off-by: Julio Montes <julio.montes@intel.com>

katacontainersbot · 2018-07-31T20:54:22Z

PSS Measurement:
Qemu: 167976 KB
Proxy: 5843 KB
Shim: 9030 KB

Memory inside container:
Total Memory: 2043480 KB
Free Memory: 2003728 KB

opendev-zuul · 2018-07-31T22:03:53Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 54m 31s

bergwolf · 2018-08-01T01:54:49Z

CI green. Let's merge it ignoring the tiny codecov drop. It removes some dead code and the UT along with it. I guess that's why it's getting the tiny codecov/project drop.

sboeuf · 2018-08-01T05:51:23Z

@chavafg I think we should create a new Jenkins job for our CI so that we test in parallel the setup with and without VSOCK.
@grahamwhaley it'd be interesting to see what we get about density and global performances now that we can remove the proxy in case of vsock.

chavafg · 2018-08-02T17:39:23Z

Opened kata-containers/ci#54 to add the new job.

egernst reviewed Jul 18, 2018

View reviewed changes

devimc force-pushed the vsock-runtime5 branch 2 times, most recently from 2a0b5ff to 68dda5f Compare July 18, 2018 16:53

jcvenegas force-pushed the vsock-runtime5 branch 2 times, most recently from 0d57518 to 6cd9ae5 Compare July 18, 2018 17:32

sboeuf reviewed Jul 18, 2018

View reviewed changes

amshinde reviewed Jul 18, 2018

View reviewed changes

devimc force-pushed the vsock-runtime5 branch from 56f94d6 to f98e49b Compare July 18, 2018 19:26

devimc force-pushed the vsock-runtime5 branch from f98e49b to cd43c1a Compare July 18, 2018 19:40

devimc force-pushed the vsock-runtime5 branch from cd43c1a to 7924ba5 Compare July 18, 2018 19:53

bergwolf reviewed Jul 19, 2018

View reviewed changes

devimc force-pushed the vsock-runtime5 branch 2 times, most recently from 5f9d66a to 52ac223 Compare July 19, 2018 12:30

devimc force-pushed the vsock-runtime5 branch 2 times, most recently from c6cc48b to 4fa5a30 Compare July 19, 2018 13:48

Julio Montes and others added 4 commits July 31, 2018 08:42

vendor: update govmm

8ae2888

add vhostfd and disable-modern to vhost-vsock-pci shortlog: 3830b44 qemu: add vhostfd and disable-modern to vhost-vsock-pci f700a97 qemu/qmp: implement function to hotplug vsock-pci Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the vsock-runtime5 branch from 39fa8e1 to ce09fb2 Compare July 31, 2018 15:31

Julio Montes added 5 commits July 31, 2018 13:52

virtcontainers: log type of proxy started

3adc862

In order to see what proxy was started or not, we should log its type and the URL Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com> Signed-off-by: Julio Montes <julio.montes@intel.com>

virtcontainers: update KataAgentConfig to support vsocks

1515bd0

add extra field in KataAgentConfig structure to specify if the kata agent have to use a vsock instead of serial port. Signed-off-by: Julio Montes <julio.montes@intel.com>

virtcontainers: remove parseVSOCKAddr function

3c15bc5

parseVSOCKAddr function is no more needed since now agent config contains a field to identify if vsocks should be used or not. Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the vsock-runtime5 branch from ce09fb2 to 52d4b33 Compare July 31, 2018 18:53

devimc force-pushed the vsock-runtime5 branch from 52d4b33 to 7779beb Compare July 31, 2018 20:00

devimc force-pushed the vsock-runtime5 branch from 7779beb to 3364379 Compare July 31, 2018 20:38

egernst approved these changes Aug 1, 2018

View reviewed changes

bergwolf merged commit fc45d2e into kata-containers:master Aug 1, 2018

egernst mentioned this pull request Aug 22, 2018

Stable 1.1.1 candidate #621

Merged

jodh-intel mentioned this pull request Aug 23, 2018

Adding OpenTracing support for Kata kata-containers/kata-containers#27

Closed

jodh-intel mentioned this pull request Sep 5, 2018

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

Closed

sboeuf added the feature New functionality label Sep 12, 2018

jcvenegas deleted the vsock-runtime5 branch January 29, 2019 18:32


		s.Logger().Info("VM started")

		data, err := s.hypervisor.hotplugAddDevice(nil, vSockDev)

Conversation

jcvenegas commented Jul 18, 2018 • edited by devimc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf commented Jul 18, 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!

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!

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!

jcvenegas Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

jcvenegas commented Jul 18, 2018 •

edited by devimc

Loading

jcvenegas Jul 18, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading