Add support for intel cloud hypervisor #2164

ericooper · 2019-11-05T15:17:04Z

Initial release of cloud hypervisor driver for kata-runtime

Related to issue 'Add support for cloud hypervisor 2046'

Signed-off-by: Johan Kuijpers [email protected]

egernst · 2019-11-05T15:53:12Z

/cc @sameo

jcvenegas · 2019-11-05T16:55:47Z

/test-ch

jcvenegas · 2019-11-05T17:11:54Z

test-ch

jcvenegas · 2019-11-05T17:12:01Z

/test-ch

jcvenegas · 2019-11-05T17:16:24Z

/test-ch

jcvenegas · 2019-11-05T17:18:26Z

/test-ch

jcvenegas

Initial review added, to make travis happy could you also add to your commit message

Fixes: #2046

cli/config/configuration-ich.toml

Makefile

jcvenegas · 2019-11-05T16:28:52Z

pkg/katautils/config.go

@@ -638,7 +639,6 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) {
 		GuestHookPath:           h.guestHookPath(),
 	}, nil
 }
-


Nit: not needed

I believe it's still here :)

pkg/rootless/rootless.go

Makefile

pkg/katautils/config.go

virtcontainers/ich_arch_base.go

virtcontainers/pkg/intel/govmm/ich/ich.go

Makefile

pkg/katautils/config.go

sameo

Overall this looks good and I think we should clean this PR up and merge it before switching to the HTTP based API. IMO this PR needs to be simplified:

We should get rid of the arch abstraction
We should remove the govmm dependency

With those 2 simplification steps, we'll pretty much have a self contained implementation for the Cloud Hypervisor support.

sameo · 2019-11-06T16:11:10Z

virtcontainers/ich.go

+type CloudHypervisorState struct {
+	sync.RWMutex
+	state ichState
+	PID int


The PID here is redundant with what's in CloudHypervisorInfo, I think.

I feel it would be cleaner to clearly separate the state from the information:

type CloudHypervisorInfo struct { PID int VirtiofsdPid int UUID string } type CloudHypervisorState struct { sync.RWMutex state ichState }

I've removed the govmm dependency and added a new CLI builder into the driver itself as agreed. I've also merged the info and state struct to simplify things and checked that the state in hypervisor.json is correct at runtime. I've also noticed that IF the hypervisor fails to start for whatever reason the virtiofsd obviously is not teared-down by the hypervisor as a consequence so I use now the stored virtiofsd PID to kill the it to avoid accumulating unused processes.

sameo · 2019-11-06T16:19:24Z

virtcontainers/ich_arch_base.go

+type ichArch interface {
+
+	// ichPath returns the path to the Ich binary
+	ichPath() (string, error)
+
+	// kernelParameters returns the kernel parameters
+	kernelParameters(debug bool) []Param
+
+	// appendImage appends an image to devices
+	appendImage(devices []govmmIch.Device, path string) ([]govmmIch.Device, error)
+
+	// appendNetwork appends a endpoint device to devices
+	appendNetwork(devices []govmmIch.Device, endpoint Endpoint) ([]govmmIch.Device, error)
+
+	// appendHybridVSock appends a hybrid vsock PCI to devices
+	appendHybridVSock(devices []govmmIch.Device, vsock types.HybridVSock) ([]govmmIch.Device, error)
+
+	appendRNGDevice(devices []govmmIch.Device, rngDev config.RNGDev) ([]govmmIch.Device, error)
+
+	appendSerialConsole(devices []govmmIch.Device, consoletype string) ([]govmmIch.Device, error)
+
+	appendVirtioConsole(devices []govmmIch.Device, consoletype string) ([]govmmIch.Device, error)
+
+	appendApiSocket(devices []govmmIch.Device, apiSocket string) ([]govmmIch.Device, error)
+
+	appendVirtualFilesystem(devices []govmmIch.Device, tag string, path string) ([]govmmIch.Device, error)
+
+	appendMemory(memorySize uint64, path string) (govmmIch.Memory, error)
+
+	appendProcessors(vcpu uint32) (govmmIch.VCPU, error)
+
+	//capabilities returns the capabilities supported by ICH
+	capabilities() types.Capabilities
+
+}


So I assume this is something that was taken from the qemu implementation, where the command line varies across architectures. With Cloud Hypervisor, we want to have a consistent HTTP API and hopefully command line, and I don't think adding such an abstraction is necessary.

Inline with your previous comment I can remove this but bear in mind that the main reason I added it was not the abstraction but the state aggregation into a device list before generating the final command line argument. This mechanism has to stay in the driver somehow because this builds by the 'add' calls into the driver (nic's are being added late in the process for instance) after which the state is ready to be translated into a working hypervisor. So to cut it short, I still need some state aggregation in the driver for devices if we cut off the iovmm logic.

How do you see this?

Oh yes, you're going to implement a way to aggregate devices and other parameters into a command line string for now (an HTTP payload later on). What I'm saying is that it should be part of the driver itself, not a govmm implementation.

Very good, then we're on the same page. I'll fix it.

sameo · 2019-11-06T16:22:35Z

virtcontainers/ich.go

+	ctx        context.Context
+	socketPath string
+	ichConfig  govmmIch.Config
+	arch 	   ichArchBase


As mentioned in another comment, we don't need an arch specific interface for Cloud Hypervisor.

sameo · 2019-11-06T16:25:26Z

virtcontainers/ich_arch_base.go

+}
+
+type ichArchBase struct {
+	machineType           string


For example, we don't expect to have different kind of machine types.

sameo · 2019-11-06T16:25:48Z

virtcontainers/ich_arch_base.go

+	kernelParams          []Param
+	memory				  govmmIch.Memory
+	vcpu				  govmmIch.VCPU
+}


And the rest of the structure is pretty much arch agnostic from a CH standpoint.

sameo · 2019-11-06T17:22:41Z

virtcontainers/ich.go

+	}
+
+	// Add memory to the cloud hypervisor
+	memory, err := ich.arch.appendMemory(ich.config.MemorySize, "/dev/shm")


Without the arch abstraction, this could be a cloudHypervisor method instead.

sameo · 2019-11-06T17:29:48Z

virtcontainers/ich.go

+	if err != nil {
+		return err
+	}
+	kernel := govmmIch.Kernel{


Another comment here: Similar to the arch abstraction, I think the govmm implementation is also modeled after the current qemu implementation.
For Cloud Hypervisor, I don't think there's a need to go through a govmm abstraction here. The cloudHypervisor structure would have a local array of cloudHypervisorDevice, etc.

The Cloud Hypervisor API is simple enough that we don't need to "hide" its complextiy behind the govmm package, and add a dependency between the Kata support for Cloud Hypervisor and the govmm package. This will simplify and speed the development process.

sameo · 2019-11-06T17:35:01Z

virtcontainers/ich.go

+//
+//#####################################
+
+func (ich *cloudHypervisor) createSandbox(ctx context.Context, id string, networkNS NetworkNamespace, hypervisorConfig *HypervisorConfig, vcStore *store.VCStore) error {


Given that we're going to eventually switch to an HTTP API based model, we should make sure that this routine creates a generic enough Cloud Hypervisor configuration for startSandbox to consume and either build a command line for the cloud-hypervisor binary or build an HTTP payload and then send it through an HTTP command.

egernst · 2019-11-06T22:52:37Z

virtcontainers/ich.go

+//	ichConfig.Devices, err = ich.arch.appendApiSocket(ichConfig.Devices, apiSocketPath)
+//	if err != nil {
+//		return err
+//	}


should remove commented out code

done. moved to rev 2 of the driver to be done next week

jodh-intel

Thanks for raising @ericooper!

Given the amount of new code, it would be great if you could add a few unit tests.

We have a doc explaining our table-driven approach and there are of course lots of examples in this repo and other Kata ones:

https://github.com/kata-containers/tests/blob/master/Unit-Test-Advice.md#table-driven-tests

jodh-intel · 2019-11-07T11:10:20Z

pkg/katautils/config.go

+		return vc.HypervisorConfig{}, err
+	}
+
+        if initrd != "" {


Nit: Odd indent compared to surrounding code (tabs/spaces issue maybe?)

will reformat

pkg/rootless/rootless.go

jodh-intel · 2019-11-07T11:13:08Z

virtcontainers/ich.go

+	"fmt"
+	"net"
+	"syscall"
+


Looking at this file with git diff shows a lot of extraneous whitespace (shows in red). I'd consider running gofmt over all the changed files.

As soon as this initial version of the driver isn't the subject of major requested structural changes, I'll start working on the unit tests for clh.go

jodh-intel · 2019-11-07T11:15:38Z

virtcontainers/ich.go

+	ichTimeout            = 10
+	ichSocket             = "ich.sock"
+	vfsdSocket            = "virtiofsd.sock"
+	apiSocket             = "cloud-hypervisor.9166"


What is 9166? Can this be simply cloud-hypervisor.sock?

it will be removed.

jodh-intel · 2019-11-07T11:20:06Z

virtcontainers/ich.go

+
+	{"root", "/dev/vda1"},
+	{"panic", "1"},
+	{"init", "/usr/lib/systemd/systemd"},


Is this necessary? /sbin/init always exists in our images and is either init binary or a link to it (systemd or the kata-agent).

Well, we use our own images for other reasons so I'll see what I can do.

jodh-intel · 2019-11-07T11:23:33Z

virtcontainers/ich.go

+	var thePid = 0
+	strErr, err, thePid = govmmIch.LaunchIch(ich.ichConfig)
+	if err != nil {
+		return fmt.Errorf("fail to launch cloud-hypervisor: %s, error messages from log: %s", err, strErr)


Nit: s/fail/failed/

jodh-intel · 2019-11-07T11:24:05Z

virtcontainers/ich.go

+}
+
+func (ich *cloudHypervisor) getSandboxConsole(id string) (string, error) {
+	ich.Logger().WithField("function", "getSandboxConsole").WithField("ID", id).Info("Get Sandbox Console")


Nit: Structured logging fields should be all lower case, so id here.

I didn't know. will change

jodh-intel · 2019-11-07T11:25:06Z

virtcontainers/ich.go

+
+func (ich *cloudHypervisor) getSandboxConsole(id string) (string, error) {
+	ich.Logger().WithField("function", "getSandboxConsole").WithField("ID", id).Info("Get Sandbox Console")
+	// TODO


Please can you add in a GitHub issue URL here so that the comment is in the form:

// FIXME: https://github.com/kata-containers/runtime/issues/...

That way we have both the issue and the standardised comment as a reminder of what remains to be done.

jodh-intel · 2019-11-07T11:28:22Z

virtcontainers/ich.go

+}
+
+func (ich *cloudHypervisor) getPids() []int {
+


Extraneous blank lines (ere and elsewhere).

jodh-intel · 2019-11-07T11:31:55Z

virtcontainers/pkg/intel/govmm/ich/ich.go

+
+func (config *Config) appendKernel() {
+
+	//config.ichParams = append(config.ichParams, "--cmdline", fmt.Sprintf("\"%s\"", config.Kernel.Params))


Commented out code.

this will be refactored.

egernst · 2019-11-06T22:56:24Z

virtcontainers/ich.go

+	case <-time.After(timeoutDuration):
+		err = fmt.Errorf("timed out waiting for cloud-hypervisor vsock")
+	}
+	// delay between vsock is available but hypervisor is bstill ooting and starting the kata-agent


Suggested change

// delay between vsock is available but hypervisor is bstill ooting and starting the kata-agent

// delay between vsock is available but hypervisor is still booting and starting the kata-agent

It must have been late ...

Nevertheless, this is an important issue I think. What I observed is that without this hard-coded wait the runtime shuts down the hypervisor even before it was able to start the agent. It's the time between the moment the vsock is available but the agent inside the hypervisor isn't ready yet. Somehow grpc doesn't wait during that time period but aborts immediately and causes a premature shutdown. This should be improved I think. Anyone an opinion on this?

amshinde · 2019-11-07T16:56:41Z

Makefile

+
+    # ICH-specific options (all should be suffixed by "_ICH")
+    # currently, huge pages are required for virtiofsd support
+    DEFENABLEHUGEPAGES_ICH := true


I dont think we want to set this as true, we got virtiofs to work without huge pages afaik.
cc @ganeshmaharaj

I was more thinking about DPDK which is widely adopted in the telco domain. How do you see this?

pkg/katautils/config.go

amshinde · 2019-11-07T18:20:34Z

pkg/katautils/config.go

@@ -1222,6 +1313,8 @@ func checkHypervisorConfig(config vc.HypervisorConfig) error {
 			}
 		}
 	}
+
+


Extra lines added. Please remove.

amshinde · 2019-11-07T18:33:50Z

virtcontainers/ich.go

+
+	{"root", "/dev/vda1"},
+	{"panic", "1"},
+	{"init", "/usr/lib/systemd/systemd"},


amshinde · 2019-11-07T18:48:06Z

virtcontainers/ich.go

+	if err != nil {
+		return err
+	}
+	ich.Logger().WithField("function", "startSandbox").Infof("Starting virtiofsd") 


Also, shouldn't this log line be inside the condition?

amshinde · 2019-11-07T18:49:16Z

virtcontainers/ich.go

+		}
+	}
+
+	ich.Logger().WithField("function", "startSandbox").Infof("virtiofsd starts sharing") 


Same as above, this should be logged only when ich.config.SharedFS = config.VirtioFS

virtcontainers/ich.go

jodh-intel · 2019-11-08T11:55:28Z

Hi @ericooper - thanks for updating. Given all the comments flying around, please could you post a comment here when you think the PR is generally ready for re-review.

jcvenegas

looking - adding some more comments to not forget in the refactor

jcvenegas · 2019-11-07T16:31:32Z

virtcontainers/ich.go

+			if ich.config.Debug {
+				ich.Logger().WithField("source", "virtiofsd").Debug(scanner.Text())
+			}
+			if !sent && strings.Contains(scanner.Text(), "Waiting for vhost-user socket connection...") {


This looks that would be broken at any time virtiofsd changes the log, I wonder if woudl be a better way to check is ready

this is not specific to my driver commit. it's the same in the other drivers using virtiofsd. I would like to keep it this way and focus on the essentials first until this is improved in general.

jcvenegas · 2019-11-07T19:38:07Z

virtcontainers/ich_arch_base.go

+			Tag:		   tag,
+			Path:          path,
+			NumQueues:	   1,
+			QueueSize:	   512,


TODO: Add config option for this

jcvenegas · 2019-11-07T19:42:44Z

virtcontainers/pkg/intel/govmm/ich/ich.go

+	cacheSize := "0Gib"
+	if(vfsdev.Dax) {
+		daxParam = "on"
+		cacheSize = "8Gib"


to big to be hardcoded

this is the default size of the hypervisor itself ie not my idea.

ericooper · 2019-11-10T09:47:09Z

Hi @ericooper - thanks for updating. Given all the comments flying around, please could you post a comment here when you think the PR is generally ready for re-review.

I think we have a misalignment of expectations here. This is a WIP for a hypervisor which itself is still in alfa stage and under heavy development. I've developed and published this driver to be able to start experimenting with cloud-hypervisor as soon as possible and to propell adoption of the cloud-hypervisor. I would like to focus first on the essentials and get it going and do the nitty gritty cosmetics at the end. This approach has proven to fail which is something I have to deal with.

sameo · 2019-11-10T13:15:06Z

Hi @ericooper - thanks for updating. Given all the comments flying around, please could you post a comment here when you think the PR is generally ready for re-review.

I think we have a misalignment of expectations here. This is a WIP for a hypervisor which itself is still in alfa stage and under heavy development. I've developed and published this driver to be able to start experimenting with cloud-hypervisor as soon as possible and to propell adoption of the cloud-hypervisor. I would like to focus first on the essentials and get it going and do the nitty gritty cosmetics at the end. This approach has proven to fail which is something I have to deal with.

I think @jodh-intel comment was very much aligned with that approach. He was saying that it's up to you to decide which comments you want to address and then decide when the PR is ready for a second round of reviews. I also agree with the approach of getting something that's acceptable and working merged first and then work on some of the nits/details from all the reviews that you got.
We only need to agree on what's acceptable as a mergeable PR, and I'm hoping that you can drive that discussion.

bergwolf · 2019-11-11T03:02:02Z

cli/config/configuration-ich.toml.in

+# Shared file system type:
+#   - virtio-9p (do not use)
+#   - virtio-fs (default & must)
+shared_fs = "@DEFSHAREDFS_ICH@"


IIUC there is no virtio-9p for cloud hypervisor.

I've removed both vsock and file-sharing options from Makefile, config template and hardwired them in newClhHypervisorConfig accordingly.

bergwolf · 2019-11-11T03:03:08Z

cli/config/configuration-ich.toml.in

+# If true and vsocks are supported, use vsocks to communicate directly
+# with the agent (no proxy is started).
+# Default true & must. cloud-hypervisor only supports vsock based agent communication
+use_vsock = true


Since cloud hypervisor must use vsock, we should not make it an option at all?

I've removed both vsock and file-sharing options from Makefile, config template and hardwired them in newClhHypervisorConfig accordingly.

jodh-intel · 2019-11-11T11:50:15Z

Hi @ericooper - this branch needs a rebase to resolve a conflict in rootless.go.

sboeuf · 2019-11-13T06:50:23Z

cli/config/configuration-ich.toml

+virtio_fs_daemon = "/usr/bin/virtiofsd-x86_64"
+
+# cloud-hypervisor does not support virtio-fs caching
+virtio_fs_cache = "none"


It does and it should be used for better performances.

sboeuf · 2019-11-13T06:51:30Z

cli/config/configuration-ich.toml

+[agent.kata]
+# If enabled, make the agent display debug-level messages.
+# (default: disabled)
+enable_debug = true


Do we need the debug to be enabled by default?

sboeuf · 2019-11-13T07:02:14Z

pkg/rootless/rootless.go

@@ -90,7 +90,7 @@ func setRootless() error {
 		if err != nil {
 			return parseError
 		}
-		rangeUID, err := strconv.ParseUint(ids[1], 10, 0)
+		rangeUID, err := strconv.ParseUint(ids[2], 10, 0)


Could you please submit this fix as its own separate commit since it's not directly related to Cloud-Hypervisor support.

This fix has already landed in master so this change is no longer needed.

virtcontainers/clh.go

sboeuf · 2019-11-13T07:06:46Z

virtcontainers/clh.go

+}
+
+func (clh *cloudHypervisor) hotplugAddDevice(devInfo interface{}, devType deviceType) (interface{}, error) {
+	clh.Logger().WithField("function", "hotplugAddDevice").Info("Add hotplug device")


The message should indicate that it's not supported. And I would suggest to use a Warn instead of Info.

virtcontainers/clh.go

sboeuf · 2019-11-13T07:07:54Z

virtcontainers/clh.go

+		"function": "resizeVCPUs",
+		"curr":     currentVCPUs,
+		"new":      newVCPUs,
+	}).Info("resize the VCPU's ")


We seem to have lost the explanation for what the ditto means here @sboeuf ;)

sboeuf · 2019-11-13T07:12:28Z

virtcontainers/clh.go

+	return nil
+}
+
+func (clh *cloudHypervisor) LaunchIch() (string, error, int) {


Another occurence of ich

sboeuf · 2019-11-13T07:19:48Z

virtcontainers/clh.go

+
+	// Add the hybrid vsock device to hypervisor
+	clh.cliBuilder.SetVsock(&CLIVsock{
+		cid:        3,


Shouldn't be the context ID provided by Kata? (instead of being hardcoded)

FWIW Firecracker also uses a hardcoded 3.

Doesn't that imply you cannot run a FC container and a CLH container on the same host then? Could we use VMADDR_CID_ANY (-1U) as documented in vsock(7)?

sameo

I am adding quite a few comments, but I think all of them are small/details/nits. I think the architecture looks good now, and I like the standalone clh.go driver. We're getting quite close, thanks.

Makefile

sameo · 2019-11-13T05:47:41Z

Makefile

@@ -600,11 +640,12 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit
 		-e "s|@SHIMPATH@|$(SHIMPATH)|g" \
 		-e "s|@DEFVCPUS@|$(DEFVCPUS)|g" \
 		-e "s|@DEFMAXVCPUS@|$(DEFMAXVCPUS)|g" \
-		-e "s|@DEFMAXVCPUS_ACRN@|$(DEFMAXVCPUS_ACRN)|g" \
+                -e "s|@DEFMAXVCPUS_ACRN@|$(DEFMAXVCPUS_ACRN)|g" \


Could you please avoid modifying this line?

I don't get it. Which line do you mean?

Hi @ericooper - the whitespace in Makefile (line 643 in your version) has been changed, but this looks like an oversight as the actual content hasn't changed.

sameo · 2019-11-13T05:52:36Z

cli/config/configuration-clh.toml.in

+# to the proxy logs, but only when proxy debug is also enabled.
+# 
+# Default false
+enable_debug = true


We want to comment that one out as a default.

cli/config/configuration-clh.toml.in

sameo · 2019-11-13T06:05:38Z

pkg/katautils/config.go

@@ -638,7 +639,6 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) {
 		GuestHookPath:           h.guestHookPath(),
 	}, nil
 }
-


I believe it's still here :)

virtcontainers/clh.go

sameo · 2019-11-13T07:18:04Z

virtcontainers/clh.go

+	// First take the default parameters defined by this driver
+	clh.cliBuilder.AddKernelParameters(clhKernelParams)
+
+	// Followed by extra debug parameters if debug enabled in configuration file
+	if clh.config.Debug == true {
+		clh.cliBuilder.AddKernelParameters(clhDebugKernelParams)
+	}
+
+	// Followed by extra debug parameters defined in the configuration file
+	clh.cliBuilder.AddKernelParameters(clh.config.KernelParams)


Maybe this could be grouped into a single func (d *DefaultCLIBuilder) AddKernelParameters(hypervisorConfig *HypervisorConfig) routine?

virtcontainers/clh.go

sameo

Only a few nits to address, and the virtiofs caching enabled by default. Getting closer.

Makefile

sameo · 2019-11-13T12:47:23Z

cli/config/configuration-clh.toml.in

+# cloud-hypervisor does not support virtio-fs caching
+virtio_fs_cache = "none"


Oh, right, the documentation is outdated (It would not be proper documentation otherwise, wouldn't it ;-)).
I can confirm that caching is now enabled, and we systematically test both cases as part of our CI. See https://github.com/intel/cloud-hypervisor/blob/master/src/main.rs#L1775 for example.

I'll open an issue against the doc and we'll fix it. Meanwhile, as suggested by @sboeuf, I think your PR should support caching by default. Performance is much better with DAX on.

sameo · 2019-11-13T12:54:15Z

pkg/katautils/config.go

@@ -1222,7 +1309,6 @@ func checkHypervisorConfig(config vc.HypervisorConfig) error {
 			}
 		}
 	}
-


I mean that you're removing an empty line here, which is not something that's related to the PR. It adds noise to the patch/PR, so I'm suggesting to keep the empty line there.

sameo · 2019-11-13T13:06:54Z

virtcontainers/clh.go

+	if clh.version.Major < supportedMajorVersion && clh.version.Minor < supportedMinorVersion {
+		errorMessage := fmt.Sprintf("Unsupported version: cloud-hypervisor %d.%d not supported by this driver version (%d.%d)",
+			clh.version.Major,
+			clh.version.Minor,
+			supportedMajorVersion,
+			supportedMinorVersion)
+		return errors.New(errorMessage)
+	}


But it is a fact that any binary below 0.3.0 the driver will fail to drive due to changes in the cli.

I agree. I don't have a strong opinion on that one, we can keep it.

I would actually like to see a different versioning for both cli and http api

That one I disagree with. It's ok to break the CLI/API between minor versions, especially with such a new project, but the CLI and HTTP APIs should be kept in sync when they map. There will be HTTP API endpoints that don't have a CLI counterpart (e.g. the hotplug ones), but for those who do they should be consistent.

sameo · 2019-11-13T13:11:09Z

virtcontainers/clh.go

+
+	// Add the hybrid vsock device to hypervisor
+	clh.cliBuilder.SetVsock(&CLIVsock{
+		cid:        3,


FWIW Firecracker also uses a hardcoded 3.

virtcontainers/clh.go

sameo · 2019-11-13T13:19:18Z

cli/config/configuration-clh.toml.in

+# cloud-hypervisor does not support virtio-fs caching
+virtio_fs_cache = "none"


See cloud-hypervisor/cloud-hypervisor#433

egernst

@ericooper: Can we set DisableVhostNet to true? I don't think we need it to be configurable, and today AFAICT the PR doesn't set it in the hypervisor config from TOML/Annotations anyway.

pkg/katautils/config.go

jcvenegas

@ericooper changed the kernel to use kata virtiofs kernel build, feel free to drop or squash the commit

sboeuf · 2019-11-14T21:12:52Z

@jcvenegas

@ericooper changed the kernel to use kata virtiofs kernel build, feel free to drop or squash the commit

Thanks for fixing the PR. The change should not be dropped since it is expected that if we want CloudHypervisor working with Kata, Kata will have to use virtio-fs.

jodh-intel

Thanks for updating again @ericooper. There are only a few unresolved questions now so we're getting close!

As shown in https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#updating-your-pr-based-on-review-comments, please can you squash all your commits down into one (since they all relate to the single task of adding CLH support). Once done, this PR will then have two commits: @jcvenegas's Makefile change and your single commit to add CLH support.

jodh-intel · 2019-11-15T10:49:40Z

virtcontainers/clh.go

@@ -416,7 +448,7 @@ func (clh *cloudHypervisor) addDevice(devInfo interface{}, devType deviceType) e
 		})

 	default:
-		clh.Logger().WithField("function", "addDevice").Warn("Add device of type %v is not supported.",  v)
+		clh.Logger().WithField("function", "addDevice").Warn(fmt.Sprintf("Add device of type %v is not supported.", v))


Nit: You can simplify this to use:

.Warnf("Add device of type %v is not supported.", v)

jodh-intel · 2019-11-15T10:53:49Z

pkg/rootless/rootless.go

@@ -90,7 +90,7 @@ func setRootless() error {
 		if err != nil {
 			return parseError
 		}
-		rangeUID, err := strconv.ParseUint(ids[1], 10, 0)
+		rangeUID, err := strconv.ParseUint(ids[2], 10, 0)


This fix has already landed in master so this change is no longer needed.

jodh-intel · 2019-11-15T10:58:34Z

virtcontainers/clh.go

+
+	// Add the hybrid vsock device to hypervisor
+	clh.cliBuilder.SetVsock(&CLIVsock{
+		cid:        3,


Doesn't that imply you cannot run a FC container and a CLH container on the same host then? Could we use VMADDR_CID_ANY (-1U) as documented in vsock(7)?

jodh-intel · 2019-11-15T11:04:10Z

virtcontainers/clh.go

+			clh.Logger().Info("Cloud Hypervisor stopped")
+			clh.state.PID = 0
+			clh.state.VirtiofsdPID = 0
+			clh.state.state = clhNotReady


Thought: This might benefit from a cloudHypervisor.reset() helper method (which possibly calls CloudHypervisorState.reset() or NewCloudHypervisorState() or similar). This might help avoid bugs if we ever extend CloudHypervisorState.

jodh-intel · 2019-11-15T11:16:56Z

virtcontainers/clh.go

+	}
+
+	// Send a SIGTERM to the VM process to try to stop it properly
+	if err = syscall.Kill(pid, syscall.SIGTERM); err != nil {


Having two kills like this is potentially racy. I'd remove the Kill(0) and instead return nil if Kill(SIGTERM) fails with ESRCH.

This can be handled with a follow-on PR since this idiom is used for the other hypervisor implementations too.

jodh-intel · 2019-11-15T11:20:33Z

virtcontainers/clh.go

+		return 0, err
+	}
+
+	clh.Logger().WithField("Path", clh.config.VirtioFSDaemon).Info()


Nit: All our log fields should be lower-case.

jodh-intel · 2019-11-15T11:23:06Z

virtcontainers/clh.go

+		"function": "resizeVCPUs",
+		"curr":     currentVCPUs,
+		"new":      newVCPUs,
+	}).Info("resize the VCPU's ")


We seem to have lost the explanation for what the ditto means here @sboeuf ;)

egernst · 2019-11-15T16:11:19Z

Thanks -- I'll fix the merge conflict, then let's merge!

egernst · 2019-11-15T16:20:50Z

/test

egernst · 2019-11-15T16:21:16Z

/test-ch

jodh-intel

Thanks @ericooper!

codecov · 2019-11-15T17:33:36Z

Codecov Report

Merging #2164 into master will decrease coverage by 1.89%.
The diff coverage is 6.94%.

@@            Coverage Diff            @@
##           master    #2164     +/-   ##
=========================================
- Coverage   51.25%   49.35%   -1.9%     
=========================================
  Files         110      111      +1     
  Lines       15114    15791    +677     
=========================================
+ Hits         7747     7794     +47     
- Misses       6410     7032    +622     
- Partials      957      965      +8

jcvenegas · 2019-11-15T18:00:31Z

/test

Initial release of cloud hypervisor driver for kata-runtime Fixes: #2046 Signed-off-by: Johan Kuijpers <[email protected]>

use virtiofs kernel to allow boot kata. Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>

jcvenegas · 2019-11-15T19:59:03Z

/test

tedyu · 2019-11-17T21:36:26Z

virtcontainers/clh.go

+	if elapsed < timeoutDuration {
+		timeout = timeout - int(elapsed.Seconds())
+	} else {
+		timeout = 0


Shouldn't error be returned in this case since the wait times out ?

tedyu · 2019-11-17T21:36:53Z

virtcontainers/clh.go

+
+	sourcePath := filepath.Join(kataHostSharedDir(), clh.id)
+	if _, err := os.Stat(sourcePath); os.IsNotExist(err) {
+		os.MkdirAll(sourcePath, os.ModePerm)


Better check return value and log accordingly.

tedyu · 2019-11-17T21:38:02Z

virtcontainers/clh.go

+		"-o", "source=" + sourcePath,
+		"-o", "cache=" + clh.config.VirtioFSCache}
+
+	if len(clh.config.VirtioFSExtraArgs) != 0 {


Should we check that VirtioFSExtraArgs contains none of the 3 args from line 665 to 667 ?

tedyu · 2019-11-17T21:40:25Z

virtcontainers/clh.go

+
+func (clh *cloudHypervisor) storeState() error {
+	if clh.store != nil {
+		if err := clh.store.Store(store.Hypervisor, clh.state); err != nil {


There is no need to check the return value - we can directly return it to caller.

tedyu · 2019-11-17T21:43:53Z

virtcontainers/clh.go

+			conn, err := net.DialUnix("unix", nil, addr)
+
+			if err != nil {
+				time.Sleep(50 * time.Millisecond)


Possibly log after continuously sleeping 200 times.

tedyu · 2019-11-17T21:44:27Z

virtcontainers/clh.go

+		err = fmt.Errorf("timed out waiting for cloud-hypervisor vsock")
+	}
+
+	time.Sleep(1000 * time.Millisecond)


We know the ready status, why is this needed ?

jcvenegas reviewed Nov 5, 2019

View reviewed changes

jcvenegas reviewed Nov 6, 2019

View reviewed changes

virtcontainers/pkg/intel/govmm/ich/ich.go Outdated Show resolved Hide resolved

sameo reviewed Nov 6, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

pkg/katautils/config.go Outdated Show resolved Hide resolved

sameo suggested changes Nov 6, 2019

View reviewed changes

egernst reviewed Nov 6, 2019

View reviewed changes

jodh-intel reviewed Nov 7, 2019

View reviewed changes

egernst reviewed Nov 7, 2019

View reviewed changes

amshinde reviewed Nov 7, 2019

View reviewed changes

jcvenegas reviewed Nov 8, 2019

View reviewed changes

bergwolf reviewed Nov 11, 2019

View reviewed changes

sboeuf reviewed Nov 13, 2019

View reviewed changes

sameo reviewed Nov 13, 2019

View reviewed changes

ericooper requested a review from sameo November 13, 2019 11:26

sameo reviewed Nov 13, 2019

View reviewed changes

egernst suggested changes Nov 13, 2019

View reviewed changes

egernst reviewed Nov 13, 2019

View reviewed changes

pkg/katautils/config.go Outdated Show resolved Hide resolved

jcvenegas reviewed Nov 14, 2019

View reviewed changes

jodh-intel reviewed Nov 15, 2019

View reviewed changes

egernst changed the title ~~[WIP] Add support for intel cloud hypervisor~~ Add support for intel cloud hypervisor Nov 15, 2019

egernst approved these changes Nov 15, 2019

View reviewed changes

sboeuf approved these changes Nov 15, 2019

View reviewed changes

sameo approved these changes Nov 15, 2019

View reviewed changes

jodh-intel approved these changes Nov 15, 2019

View reviewed changes

ericooper and others added 2 commits November 15, 2019 19:35

runtime: added cloud hypervisor driver

01713d5

Initial release of cloud hypervisor driver for kata-runtime Fixes: #2046 Signed-off-by: Johan Kuijpers <[email protected]>

Makefile: clh: Use virtiofs kernel

784066a

use virtiofs kernel to allow boot kata. Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>

sboeuf merged commit 547d580 into kata-containers:master Nov 15, 2019

tedyu reviewed Nov 17, 2019

View reviewed changes

likebreath mentioned this pull request Mar 21, 2020

clh: Summarize and track the status of cloud-hypervisor integration #2545

Closed

13 tasks


		func (config *Config) appendKernel() {

		//config.ichParams = append(config.ichParams, "--cmdline", fmt.Sprintf("\"%s\"", config.Kernel.Params))

	// delay between vsock is available but hypervisor is bstill ooting and starting the kata-agent
	// delay between vsock is available but hypervisor is still booting and starting the kata-agent

		# cloud-hypervisor does not support virtio-fs caching
		virtio_fs_cache = "none"

Add support for intel cloud hypervisor #2164

Add support for intel cloud hypervisor #2164

Conversation

ericooper commented Nov 5, 2019

egernst commented Nov 5, 2019

jcvenegas commented Nov 5, 2019

jcvenegas commented Nov 5, 2019

jcvenegas commented Nov 5, 2019

jcvenegas commented Nov 5, 2019

jcvenegas commented Nov 5, 2019

jcvenegas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericooper Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Nov 8, 2019

jcvenegas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericooper commented Nov 10, 2019

sameo commented Nov 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Nov 11, 2019

ericooper Nov 12, 2019 •

edited

Loading