clh: use http client #2275

jcvenegas · 2019-11-26T16:37:21Z

Generate API client using OpenAPI tools ( similar to fc implementation does)
Instead of build a command, use Cloud Hypervisor http API

Despite of all the noise in the PR for generated code, the main changes are in

virtcontainers/clh.go

Depends-on: github.com/kata-containers/tests#2100

Signed-off-by: Bo Chen chen.bo@intel.com
Signed-off-by: Jose Carlos Venegas Munoz jose.carlos.venegas.munoz@intel.com

jcvenegas · 2019-11-26T16:42:18Z

\cc @ericooper could you review this PR - please notice that cli builder will be gone, so we may want to wait adding more unit test on current code base.

Gopkg.lock

egernst

A couple of questions/comments on the openAPI bits. Review of the rest is ongoing.

Thanks guys.

virtcontainers/pkg/cloud-hypervisor/Makefile

virtcontainers/clh.go

egernst · 2019-11-26T22:15:57Z

For at least the functions which implement the Hypervisor interface, it'd be helpful to describe exactly what is being done with respect to the configuration, the VMM and the VM itself within each function. See comments in fc.go as a reference for what I'm hoping to read.

virtcontainers/clh.go

egernst · 2019-11-26T22:34:51Z

virtcontainers/clh.go

+		clh.vmconfig.Vsock = []chclient.VsockConfig{{Cid: defaultGuestVSockCID, Sock: v.UdsPath}}
+	case types.Volume:
+
+		if clh.config.SharedFS != config.VirtioFS {


does it support block based volumes?

I have not tried but is not possible to hotAdd volumes, for K8s this the only way to go.

virtcontainers/clh.go

vendor/golang.org/x/net/context/ctxhttp/ctxhttp_pre17.go

virtcontainers/pkg/cloud-hypervisor/Makefile

virtcontainers/clh.go

jcvenegas · 2019-11-27T21:53:11Z

/test-ch

jcvenegas · 2019-11-29T13:47:38Z

/test-ch

jcvenegas · 2019-11-29T14:48:06Z

/test-ch

jcvenegas · 2019-11-29T16:31:51Z

/test-ch

egernst · 2019-12-04T19:35:07Z

nit on 1dd2135

Can we be consistent on naming (s/CH/CLH/)

codecov · 2019-12-05T22:54:20Z

Codecov Report

Merging #2275 into master will increase coverage by 3.66%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2275      +/-   ##
==========================================
+ Coverage   50.38%   54.04%   +3.66%     
==========================================
  Files         111       45      -66     
  Lines       16067     4613   -11454     
==========================================
- Hits         8095     2493    -5602     
+ Misses       6984     1917    -5067     
+ Partials      988      203     -785

sboeuf

@jcvenegas @likebreath
The PR looks good so far! I just have a few nits ;)

virtcontainers/pkg/cloud-hypervisor/cloud-hypervisor.yaml

virtcontainers/clh.go

sboeuf · 2019-12-05T22:43:02Z

virtcontainers/clh.go

+	clh.vmconfig.Memory.File = "/dev/shm"
+	// Set initial amount of cpu's for the virtual machine
+	clh.vmconfig.Cpus = chclient.CpuConfig{
+		// cast to int32, as openAPI has a limitation that it does not support unsigned values


Yes unfortunately :(

sboeuf · 2019-12-05T22:44:29Z

virtcontainers/clh.go

+	// Set initial amount of cpu's for the virtual machine
+	clh.vmconfig.Cpus = chclient.CpuConfig{
+		// cast to int32, as openAPI has a limitation that it does not support unsigned values
+		CpuCount: int32(clh.config.NumVCPUs),


FYI, this changed into boot_vcpus on latest CH codebase.

Yes, I say I needed to lock the hypervisor to a commit previous to that, you want we update now or after merge this PR, the functionality for the version that is lock this PR should be enough for what it cover this PR.

It's really up to you, but I don't think you should worry about it right now. A follow up PR will be fine. I simply wanted to let you know that this part had changed very recently :)

sboeuf · 2019-12-05T23:02:27Z

virtcontainers/clh.go

 	if err := clh.waitVMM(clhTimeout); err != nil {
 		clh.Logger().WithField("error", err).WithField("output", clh.cmdOutput.String()).Warn("cloud-hypervisor init failed")
 		clh.shutdownVirtiofsd()
 		return err
 	}

-	clh.state.PID = pid
+	if err := clh.bootVM(ctx); err != nil {


One comment here. You chose to create the VMM and boot the VM in the same startSandbox() function, but you could have done it slightly differently too. The VMM can be started with only the API socket as argument (from createSandbox()), and later on, you can issue a VmCreate(vmConfig) to start the VM from startSandbox().

I think matching FC.go is okay for now. We can look to see if there are any measurable benefits in a follow up.

virtcontainers/clh.go

jcvenegas · 2019-12-05T23:30:49Z

/test-ch

jcvenegas · 2019-12-05T23:45:13Z

/test

sboeuf · 2019-12-06T08:04:21Z

virtcontainers/clh.go

+				NumQueues: clhFsQueues,
+				QueueSize: clhFsQueueSize,
+			},
+		}


This part is fine and will work as expected 👍

sboeuf · 2019-12-06T08:11:20Z

virtcontainers/clh.go

+				NumQueues: clhFsQueues,
+				QueueSize: clhFsQueueSize,
+			},
+		}


I'm not sure about the behavior from this part. By not providing the CacheSize, and because the OpenAPI says that it expects an int64, I think the default value is going to be 0. But that will be translated into Some(0) for the Rust code, meaning it will still try to use DAX with the shared memory region when really what you want is no cache at all.
Please give it a try and let me know if that's the case, but TBH I think that's something we need to fix in CH.

@jcvenegas I've just opened cloud-hypervisor/cloud-hypervisor#515 to fix this. It'd be nice if you could give it a shot as I think this is the right way to configure FsConfig.
Note that because I added dax default to true and cache_size default to 8G, you shouldn't need to specify dax and cache_size when the only information you get from your user is virtioFsCacheAlways = "always".

sboeuf · 2019-12-06T15:24:56Z

virtcontainers/clh.go

+	// remove after PR is merged:
+	// https://github.com/cloud-hypervisor/cloud-hypervisor/pull/480
+	ip := "0.0.0.0"
+	mask := "0.0.0.0"


@jcvenegas @likebreath I've just submitted cloud-hypervisor/cloud-hypervisor#516 which should solve this issue. Let me know if that works for you.

#516 has been merged.
Can the above be removed ?

egernst

Looking good. Doing one more pass for sanity :)

likebreath · 2019-12-06T17:21:28Z

/test-ch

egernst · 2019-12-06T17:30:54Z

/test

egernst

Thanks @ericooper for the base, and @jcvenegas and @likebreath for this API. Neither was light lifting!

Looks good, let's put this in, and add improvements in follow on PRs.

jcvenegas · 2019-12-06T17:46:32Z

/test

Instead of build a command, use Cloud Hypervisor http API. Fixes: kata-containers#2165 Signed-off-by: Bo Chen <chen.bo@intel.com> Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

Remove cli builder code as now that we use http client Signed-off-by: Bo Chen <chen.bo@intel.com> Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

test with recent API changes of CH. Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

remove dirtory created for VM. This should be refactored in all hypervisors Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

Add initial unit test around http client Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

jcvenegas · 2019-12-06T21:28:07Z

/test

jcvenegas · 2019-12-06T21:45:27Z

/test-ch

jcvenegas · 2019-12-07T00:26:12Z

/test-ch

egernst · 2019-12-07T01:06:43Z

@jcvenegas @likebreath can you PTAL at the CI failure?

jcvenegas · 2019-12-07T01:19:08Z

For ARM I just check and is not related with it, I restarted the job.

The CLH CI fail is failing in different docker run every time, this is due to kata-containers/tests#2141 , so this is expected to fail until we find the root cause. I am trying seems that is a race condition with the agent, but I could not reproduce it recently, lets ignore clh CI until is more stable.

egernst · 2019-12-07T01:23:14Z

Since this occurs w CLH on master already today (not a regression), let’s get this change in and focus on resolving the test failures. Thanks folks.

tedyu · 2019-12-09T16:36:53Z

virtcontainers/clh.go

+	timeStart := time.Now()
+	cl := clh.client()
+	for {
+		ctx, cancel := context.WithTimeout(context.Background(), clhAPITimeout*time.Second)


If, at the end of iteration N, the remaining time (w.r.t. timeout) is shorter than clhAPITimeout, we would still wait for clhAPITimeout in iteration N+1. This would make the total wait longer than timeout.

Is this Okay ?

jcvenegas requested review from sameo and sboeuf November 26, 2019 16:37

jcvenegas requested a review from a team as a code owner November 26, 2019 16:37

egernst self-assigned this Nov 26, 2019

jcvenegas requested a review from egernst November 26, 2019 17:27

egernst reviewed Nov 26, 2019

View reviewed changes

Gopkg.lock Show resolved Hide resolved

egernst reviewed Nov 26, 2019

View reviewed changes

virtcontainers/pkg/cloud-hypervisor/Makefile Outdated Show resolved Hide resolved

egernst reviewed Nov 26, 2019

View reviewed changes

virtcontainers/clh.go Outdated Show resolved Hide resolved

virtcontainers/clh.go Outdated Show resolved Hide resolved

virtcontainers/clh.go Show resolved Hide resolved

egernst assigned egernst and unassigned egernst Nov 26, 2019

egernst reviewed Nov 26, 2019

View reviewed changes

jodh-intel reviewed Nov 27, 2019

View reviewed changes

jcvenegas force-pushed the ch-api-support branch 3 times, most recently from 706f2a3 to ef1352c Compare November 27, 2019 21:15

jcvenegas force-pushed the ch-api-support branch 3 times, most recently from 2680524 to 2fa74a4 Compare November 28, 2019 18:29

jcvenegas mentioned this pull request Nov 29, 2019

tests: Improve tracing test kata-containers/tests#2129

Closed

jcvenegas force-pushed the ch-api-support branch 2 times, most recently from 8378145 to c9e9d80 Compare December 4, 2019 17:57

jcvenegas changed the title ~~WIP: clh: use http client~~ clh: use http client Dec 4, 2019

jcvenegas force-pushed the ch-api-support branch from c9e9d80 to 08efa97 Compare December 4, 2019 19:45

jcvenegas force-pushed the ch-api-support branch from 621757a to 795bcc9 Compare December 5, 2019 23:06

sboeuf approved these changes Dec 5, 2019

View reviewed changes

jcvenegas force-pushed the ch-api-support branch from 795bcc9 to 10b25f6 Compare December 6, 2019 06:12

sboeuf reviewed Dec 6, 2019

View reviewed changes

jcvenegas force-pushed the ch-api-support branch from 10b25f6 to 99f3519 Compare December 6, 2019 14:01

sboeuf reviewed Dec 6, 2019

View reviewed changes

egernst reviewed Dec 6, 2019

View reviewed changes

egernst self-requested a review December 6, 2019 16:41

egernst approved these changes Dec 6, 2019

View reviewed changes

jcvenegas added 5 commits December 6, 2019 21:26

clh: use http client

f73723a

Instead of build a command, use Cloud Hypervisor http API. Fixes: kata-containers#2165 Signed-off-by: Bo Chen <chen.bo@intel.com> Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

clh: remove cli builder

9f15dd2

Remove cli builder code as now that we use http client Signed-off-by: Bo Chen <chen.bo@intel.com> Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

version: CH bump to master

c688a15

test with recent API changes of CH. Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

clh: cleanup VM dir

42061f6

remove dirtory created for VM. This should be refactored in all hypervisors Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

clh: clh http unit test

0afeb52

Add initial unit test around http client Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

jcvenegas force-pushed the ch-api-support branch from 99f3519 to 0afeb52 Compare December 6, 2019 21:26

egernst merged commit a660d80 into kata-containers:master Dec 7, 2019

tedyu reviewed Dec 9, 2019

View reviewed changes

jcvenegas deleted the ch-api-support branch January 23, 2020 19:44

clh: use http client #2275

clh: use http client #2275

Conversation

jcvenegas commented Nov 26, 2019 • edited Loading

jcvenegas commented Nov 26, 2019

egernst left a comment

Choose a reason for hiding this comment

egernst commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcvenegas commented Nov 27, 2019

jcvenegas commented Nov 29, 2019

jcvenegas commented Nov 29, 2019

jcvenegas commented Nov 29, 2019

egernst commented Dec 4, 2019

codecov bot commented Dec 5, 2019 • edited Loading

Codecov Report

sboeuf 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

jcvenegas commented Dec 5, 2019

jcvenegas commented Dec 5, 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

Choose a reason for hiding this comment

egernst left a comment

Choose a reason for hiding this comment

likebreath commented Dec 6, 2019

egernst commented Dec 6, 2019

egernst left a comment

Choose a reason for hiding this comment

jcvenegas commented Dec 6, 2019

jcvenegas commented Dec 6, 2019

jcvenegas commented Dec 6, 2019

jcvenegas commented Dec 7, 2019

egernst commented Dec 7, 2019

jcvenegas commented Dec 7, 2019

egernst commented Dec 7, 2019

Choose a reason for hiding this comment

jcvenegas commented Nov 26, 2019 •

edited

Loading

codecov bot commented Dec 5, 2019 •

edited

Loading