Cloud Hypervisor: driver update number 2 #2274

ericooper · 2019-11-26T08:45:27Z

This PR introduces unit tests for clh driver #2205 and fixes for #2271 and #2270 noticed during development of the unit tests.

Merge #3 of kata-containers/runtime

added logging of stdout and stderr for failed hypervisor Fixes: #2271 Signed-off-by: Johan Kuijpers <[email protected]>

remove type in kill statement for virtiofsd Fixes: #2270 Signed-off-by: Johan Kuijpers <[email protected]>

jodh-intel

Thanks @ericooper! Great to see a large dump of new tests like this 😄

jodh-intel · 2019-11-26T09:53:26Z

virtcontainers/clh.go

-			supportedMinorVersion)
-		return errors.New(errorMessage)
+		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)",


How about changing the message to say something like: cloud hypervisor version too old (need atleast %d.%d, found %d.%d) or similar?

jodh-intel · 2019-11-26T09:55:42Z

virtcontainers/clh.go


 	if clh.config.Debug {
 		cmd.Env = os.Environ()
-		cmd.Env = append(cmd.Env, "RUST_BACKTRACE=FULL")
+		cmd.Env = append(cmd.Env, "RUST_BACKTRACE=full")


Random thought: we could probably do with this in virtcontainers/fc.go too.

I think so. But to me it looks like it only makes sense in combination with capturing stdout and stderr (of the exec/fork cmd output buffers) otherwise you still don't see what's going on.

jodh-intel · 2019-11-26T09:56:14Z

virtcontainers/clh.go

@@ -854,6 +863,11 @@ func (clh *cloudHypervisor) LaunchClh() (string, int, error) {
 	return errStr, cmd.Process.Pid, nil
 }

+// MaxAcrnVCPUs returns the maximum number of vCPUs supported


Comment doesn't match code (the static analysis script would detect this I believe).

the checks didn't report it but nevertheless this is obviously a cut and paste error and hence corrected.

jodh-intel · 2019-11-26T10:03:33Z

virtcontainers/clh_test.go

+// Cli helper functions
+//
+
+func cliToString(args []string) string {


You could simplify this (or remove it?) in favour of:

strings.TrimSpace(strings.Join(args, " "))

removed and renamed

jodh-intel · 2019-11-26T10:06:21Z

virtcontainers/clh_test.go

+	assert.NoError(err)
+	assert.NotNil(cli)
+
+	netarg, errr := getCliOption(cli.args, "--net")


Nit: should this be err? (same comment for other functions below).

jodh-intel · 2019-11-26T10:06:57Z

virtcontainers/clh_test.go

+// --cmdline <cmdline> Kernel command line
+//
+func TestClhCliKernelParameters(t *testing.T) {
+


Nit: blank line at start of lots of functions.

I think best practice to not start with newline.

best practice is what's good for my eyes :)
removed

jcvenegas

Could you do rebase instead of merge I see one commit is a merge - LGTM

ericooper · 2019-11-26T15:38:09Z

Could you do rebase instead of merge I see one commit is a merge - LGTM

I'm not sure what you mean with this. The merge you see is the one I do frequently to keep my fork up-to-date with kata-runtime/master ie in opposite direction. Shouldn't be a problem or?

jcvenegas · 2019-11-26T15:46:38Z

Could you do rebase instead of merge I see one commit is a merge - LGTM

I'm not sure what you mean with this. The merge you see is the one I do frequently to keep my fork up-to-date with kata-runtime/master ie in opposite direction. Shouldn't be a problem or?

Not a problem at all, common practice I use to not have merge commits after update my branch with master is rebase master - so the PR that will be merged only include the list of patches you added but not a blocker at all :)

git pull master
git checkout my-branch
git rebase master

- added clh unit tests - removed some inconsistencies in the cli builder to enable unit tests - suppressed version check for in startSandbox to enable unit tests - added clh related constants and methods to virtcontainer test - small corrections after review applied Fixes: #2205 Signed-off-by: Johan Kuijpers <[email protected]>

jcvenegas · 2019-12-02T17:23:28Z

/test

codecov · 2019-12-02T19:48:17Z

Codecov Report

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

@@           Coverage Diff            @@
##             master   #2274   +/-   ##
========================================
  Coverage          ?   50.5%           
========================================
  Files             ?     111           
  Lines             ?   16073           
  Branches          ?       0           
========================================
  Hits              ?    8118           
  Misses            ?    6968           
  Partials          ?     987

tedyu · 2019-12-03T14:22:20Z

virtcontainers/clh.go

-			supportedMajorVersion,
-			supportedMinorVersion)
-		return errors.New(errorMessage)
+		if clh.version.Major < supportedMajorVersion && clh.version.Minor < supportedMinorVersion {


supportedMajorVersion is 0.
How can clh.version.Major be lower than 0 ?

This check excludes supportedMinorVersion from being 0 as well (e.g. major == 1 and minor == 0).

I think the correct condition should be:

if clh.version.Major < supportedMajorVersion || (clh.version.Major == supportedMajorVersion && clh.version.Minor < supportedMinorVersion) {

Submitted #2312

ericooper and others added 3 commits November 25, 2019 09:37

Merge pull request #3 from kata-containers/master

7b8e15f

Merge #3 of kata-containers/runtime

clh: improve driver logging for failed hypervisor

6af127f

added logging of stdout and stderr for failed hypervisor Fixes: #2271 Signed-off-by: Johan Kuijpers <[email protected]>

clh: fixes erroneous killing of virtiofsd

3ef8f6c

remove type in kill statement for virtiofsd Fixes: #2270 Signed-off-by: Johan Kuijpers <[email protected]>

jodh-intel approved these changes Nov 26, 2019

View reviewed changes

devimc approved these changes Nov 26, 2019

View reviewed changes

jcvenegas reviewed Nov 26, 2019

View reviewed changes

jcvenegas merged commit d50eea6 into kata-containers:master Dec 2, 2019

tedyu reviewed Dec 3, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud Hypervisor: driver update number 2 #2274

Cloud Hypervisor: driver update number 2 #2274

ericooper commented Nov 26, 2019

jodh-intel left a comment

jodh-intel Nov 26, 2019

jodh-intel Nov 26, 2019

ericooper Nov 28, 2019

jodh-intel Nov 26, 2019

ericooper Nov 28, 2019

jodh-intel Nov 26, 2019

ericooper Nov 28, 2019

jodh-intel Nov 26, 2019

ericooper Nov 28, 2019

jodh-intel Nov 26, 2019

egernst Nov 26, 2019

ericooper Nov 28, 2019

jcvenegas left a comment

ericooper commented Nov 26, 2019

jcvenegas commented Nov 26, 2019

jcvenegas commented Dec 2, 2019

codecov bot commented Dec 2, 2019

tedyu Dec 3, 2019 •

edited

Loading

tedyu Dec 3, 2019 •

edited

Loading

Cloud Hypervisor: driver update number 2 #2274

Cloud Hypervisor: driver update number 2 #2274

Conversation

ericooper commented Nov 26, 2019

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

jcvenegas left a comment

Choose a reason for hiding this comment

ericooper commented Nov 26, 2019

jcvenegas commented Nov 26, 2019

jcvenegas commented Dec 2, 2019

codecov bot commented Dec 2, 2019

Codecov Report

tedyu Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

tedyu Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

tedyu Dec 3, 2019 •

edited

Loading

tedyu Dec 3, 2019 •

edited

Loading