kata-check: Add version consistency check #2376

Pennyzct · 2019-12-25T07:16:30Z

Which feature do you think can be improved?
We may need multiple kata components to launch one kata container. The longest legacy one could have kata-shim, kata-proxy, kata-runtime, and kata-agent.
For users, especially for developers, we often encounter confused runtime error due to each kata component at different version.
E.g. Laterly, I've encountered following error in launching kata container with firecracker, and it's because of stale version of kata-shim, which is lower than v1.9.0.

$ docker run --runtime kata-runtime busybox
docker: Error response from daemon: cannot start a stopped process: unknown.
ERRO[0002] error waiting for container: context canceled

How can it be improved?
So, here, I want to introduce version consistency check in kata-runtime kata-check.
If we have kata components at different version, it should issue a warning, like this:

$ kata-runtime kata-check
System is capable of running Kata Containers
System can currently create Kata Containers
Warning: there exists version inconsistency in kata components. kata-proxy: v1.10, kata-shim: v1.7, kata-runtime: v1.10

Otherwise, it should output like this:

$ kata-runtime kata-check
INFO[0000] VSOCK supported, configure to not use proxy
System is capable of running Kata Containers
System can currently create Kata Containers
Version consistency of Kata Containers is verified

Updates:
Following upstream comments to use --strict/-s option to perform version consistency check:

$ kata-runtime kata-check --strict
INFO[0000] VSOCK supported, configure to not use proxy
System is capable of running Kata Containers
System can currently create Kata Containers
Version consistency of Kata Containers is verified

$ kata-runtime kata-check --strict         
INFO[0000] VSOCK supported, configure to not use proxy
System is capable of running Kata Containers
System can currently create Kata Containers
ERRO[0000] there exists version inconsistency in Kata Components. kata-shim: v1.8.1, kata-runtime: v1.10.0
  arch=arm64 name=kata-runtime pid=35284 source=runtime
there exists version inconsistency in Kata Components. kata-shim: v1.8.1, kata-runtime: v1.10.0

Pennyzct · 2019-12-25T07:18:56Z

Hi @lifupan
I finally detected the root cause of that error....
stale kata-shim...🤬🤮

Pennyzct · 2019-12-25T07:40:37Z

/test

codecov · 2019-12-25T08:12:42Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2376   +/-   ##
=========================================
  Coverage          ?   50.79%           
=========================================
  Files             ?      114           
  Lines             ?    16381           
  Branches          ?        0           
=========================================
  Hits              ?     8321           
  Misses            ?     7035           
  Partials          ?     1025

jodh-intel

Thanks @Pennyzct - this is a good idea. A few comments...

jodh-intel · 2020-01-02T15:21:09Z

cli/kata-check.go

@@ -454,3 +459,45 @@ func genericCheckKVMExtensions(extensions map[string]kvmExtension) (map[string]i

 	return results, nil
 }
+
+// func checkVersionConsistencyInComponents checks version consistency in kata components.


Nit: The comment should start with the name of the function (without func).

jodh-intel · 2020-01-02T15:23:18Z

cli/kata-check.go

+		return errors.New("cannot determine runtime config")
+	}
+
+	proxyInfo, _ := getProxyInfo(config)


It might be worth removing the error return value for getProxyInfo() as it's always nil (same for getNetmonInfo()).

jodh-intel · 2020-01-02T15:25:28Z

cli/kata-check.go

@@ -357,6 +358,10 @@ var kataCheckCLICommand = cli.Command{
 			fmt.Println(successMessageCreate)
 		}

+		if err := checkVersionConsistencyInComponents(context); err != nil {


You could simplify this block to just:

return checkVersionConsistencyInComponents(context)

jodh-intel · 2020-01-02T15:26:13Z

cli/kata-check.go

+	}
+
+	var runtimeVersion VersionInfo
+	versions := strings.Split(version, ".")


This works but it may be better to use the already-vendored semver package (github.com/blang/semver).

jodh-intel · 2020-01-02T15:29:23Z

cli/kata-check.go

+
+	if proxyInfo.Version.Version != "" {
+		if !versionCompare(proxyInfo.Version, runtimeVersion) || !versionCompare(shimInfo.Version, runtimeVersion) {
+			fmt.Printf("Warning: there exists version inconsistency in kata components. kata-proxy: v%s.%s, kata-shim: v%s.%s, kata-runtime: v%s.%s\n", proxyInfo.Version.Major, proxyInfo.Version.Minor,


s/kata components/Kata components/

We use semver consistently so I think we should display all three parts of the version here (major, minor + patch) , or just the entire version string with a single %s.

Random thought: this would expand the scope of this PR a little, but I wonder if we want to add a --strict option so that if you run kata-runtime kata-check --strict and one of these checks fails, it returns 1 rather than 0. That could be more useful for scripts, etc which shouldn't have to parse stderr. That would imply this function would then return an actual error which itself would make it easier to unit test.

jodh-intel · 2020-01-02T15:33:34Z

cli/kata-env.go

@@ -34,6 +34,12 @@ type MetaInfo struct {
 	Version string
 }

+type VersionInfo struct {
+	Version string


This is a little confusing - what is Version? I think it's the raw output of $cmd --version so I'd make that clearer by adding a comment or renaming the field. But again, I'd look at the semver package to avoid "re-inventing the wheel" - you could just store a semver.Version as that contains everything we'd need.

Ya, same question, what is the Version here? Shouldnt this be Major, Minor and Patch instead?
Are we not comparing the "Patch" version at all?

jodh-intel · 2020-01-02T15:40:31Z

cli/kata-env_test.go

@@ -30,8 +30,9 @@ import (
 	"github.com/stretchr/testify/assert"
 )

-const testProxyVersion = "proxy version 0.1"
-const testShimVersion = "shim version 0.1"
+var testProxyVersion = VersionInfo{Version: "proxy version 0.1", Major: "0", Minor: "1"}


Great to see some new tests. It would be great though if you could test a larger number of scenarios:

runtime older than proxy or shim

runtime older than proxy and shim

runtime newer than proxy or shim

runtime newer than proxy and shim

etc

I think the easiest way to add in a table-based set of semi-exhaustive tests would be to add in that strict option to allow you to test for error in checkVersionConsistencyInComponents().

amshinde · 2020-01-03T18:31:01Z

cli/kata-check.go

+	}
+
+	if proxyInfo.Version.Version != "" {
+		if !versionCompare(proxyInfo.Version, runtimeVersion) || !versionCompare(shimInfo.Version, runtimeVersion) {


Add a condition comparing the shim and proxy versions here?

amshinde · 2020-01-03T18:37:49Z

cli/kata-env.go

@@ -34,6 +34,12 @@ type MetaInfo struct {
 	Version string
 }

+type VersionInfo struct {
+	Version string


Ya, same question, what is the Version here? Shouldnt this be Major, Minor and Patch instead?
Are we not comparing the "Patch" version at all?

Pennyzct · 2020-01-06T05:18:42Z

Hi guys @jodh-intel @amshinde
Thanks for all your detailed comments, really appreciated. ;) 😚
I'm with the @jodh-intel's suggestion to add a --strict option to do the binary-check, instead of just outputting the warning.
And I'll use semver package to replace the original design. ;).
Update ASAP...

jodh-intel · 2020-01-06T08:54:35Z

Thanks @Pennyzct !

Pennyzct · 2020-01-08T09:17:33Z

Hi guys
@jodh-intel @amshinde
I've updated my code, Plz take a review~~~~;)

Pennyzct · 2020-01-09T03:30:03Z

/test

WeiZhang555 · 2020-01-14T01:37:28Z

/test

jodh-intel

Thanks @Pennyzct - very nice.

lgtm

jodh-intel · 2020-01-20T10:41:44Z

/test

Pennyzct · 2020-01-27T12:25:47Z

Hi~ guys
FC failed as usual, and should be irrelevant with this patch.
ARM CI failed because of PR kata-containers/tests/#2236.
And I need one reviewer here~~~ any one cares to take a look?? @kata-containers/runtime

devimc · 2020-01-27T14:58:28Z

restarting ubuntu-ci

Pennyzct · 2020-01-30T04:28:03Z

/test-fc

Pennyzct · 2020-01-30T04:33:43Z

ARM CI failed on:

10:42:56 pkg/rootless/rootless.go:57:2: comment on exported var `IsRootless` should be of the form `IsRootless ...` (golint)
10:42:56 	//The function is declared this way for mocking in unit tests
10:42:56 	^
10:43:01 Build step 'Execute shell' marked build as failure

It should be related to the recently merged PR #2418. @devimc Would you like to refine this comment by yourself or I just do the refinement here? ;).

amshinde · 2020-02-07T19:55:08Z

Rerunning ARM CI now that #2435 is merged.

amshinde · 2020-02-07T22:02:33Z

ARM CI is still failing @Pennyzct

Pennyzct · 2020-02-10T06:31:27Z

Hi~ @amshinde
Sorry for the ARM CI failure, and I'm already raised an PR kata-containers/test#2294 to fix them.
But it should be irrelevant with this PR. ;).

We import new struct VersionInfo for better organizing version info of kata components, in order to follow Semantic Versioning Specification. Fixes: kata-containers#2375 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Use `kata-runtime kata-check --strict/-s` to perform version consistency check. Only if major version number, minor version number and Patch number are all the same, we determine those two kata components are version-consistent. Fixes: kata-containers#2375 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

we need to refine unit tests due to previous two commits and add new test for new func checkVersionConsistencyInComponents. Fixes: kata-containers#2375 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct · 2020-02-17T02:15:21Z

re-based. ;)
/test

Pennyzct · 2020-02-18T12:05:23Z

/test

Pennyzct · 2020-02-19T01:30:42Z

Hi~ guys. Got all green here~~~~~. ;).

Pennyzct force-pushed the version_compatibility branch from 331eb90 to 6605630 Compare December 25, 2019 07:34

jodh-intel reviewed Jan 2, 2020

View reviewed changes

amshinde reviewed Jan 3, 2020

View reviewed changes

Pennyzct added the wip Work in Progress (PR incomplete - needs more work or rework) label Jan 8, 2020

Pennyzct force-pushed the version_compatibility branch from 6605630 to d04ac42 Compare January 8, 2020 09:08

Pennyzct removed the wip Work in Progress (PR incomplete - needs more work or rework) label Jan 8, 2020

Pennyzct force-pushed the version_compatibility branch 3 times, most recently from b668117 to 16ee745 Compare January 9, 2020 03:21

Pennyzct mentioned this pull request Jan 13, 2020

FC: Update Firecracker to v0.20.0 #2379

Merged

jodh-intel approved these changes Jan 20, 2020

View reviewed changes

devimc approved these changes Jan 27, 2020

View reviewed changes

Pennyzct added 2 commits February 17, 2020 10:13

kata-env: import new struct VersionInfo

a4b3c65

We import new struct VersionInfo for better organizing version info of kata components, in order to follow Semantic Versioning Specification. Fixes: kata-containers#2375 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

unit-test: refine unit tests

1c1e7cc

we need to refine unit tests due to previous two commits and add new test for new func checkVersionConsistencyInComponents. Fixes: kata-containers#2375 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct force-pushed the version_compatibility branch from 16ee745 to 1c1e7cc Compare February 17, 2020 02:14

amshinde merged commit 44b0967 into kata-containers:master Feb 19, 2020

jcvenegas mentioned this pull request May 8, 2020

kata-env: Version check is broken for stable releases #2674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kata-check: Add version consistency check #2376

kata-check: Add version consistency check #2376

Pennyzct commented Dec 25, 2019 •

edited

Loading

Pennyzct commented Dec 25, 2019

Pennyzct commented Dec 25, 2019

codecov bot commented Dec 25, 2019 •

edited

Loading

jodh-intel left a comment

jodh-intel Jan 2, 2020

jodh-intel Jan 2, 2020

jodh-intel Jan 2, 2020

jodh-intel Jan 2, 2020

jodh-intel Jan 2, 2020

jodh-intel Jan 2, 2020

amshinde Jan 3, 2020

jodh-intel Jan 2, 2020

amshinde Jan 3, 2020

amshinde Jan 3, 2020

Pennyzct commented Jan 6, 2020 •

edited

Loading

jodh-intel commented Jan 6, 2020

Pennyzct commented Jan 8, 2020

Pennyzct commented Jan 9, 2020

WeiZhang555 commented Jan 14, 2020

jodh-intel left a comment

jodh-intel commented Jan 20, 2020

Pennyzct commented Jan 27, 2020

devimc commented Jan 27, 2020

Pennyzct commented Jan 30, 2020

Pennyzct commented Jan 30, 2020 •

edited

Loading

amshinde commented Feb 7, 2020

amshinde commented Feb 7, 2020

Pennyzct commented Feb 10, 2020

Pennyzct commented Feb 17, 2020

Pennyzct commented Feb 18, 2020

Pennyzct commented Feb 19, 2020

kata-check: Add version consistency check #2376

kata-check: Add version consistency check #2376

Conversation

Pennyzct commented Dec 25, 2019 • edited Loading

Pennyzct commented Dec 25, 2019

Pennyzct commented Dec 25, 2019

codecov bot commented Dec 25, 2019 • edited Loading

Codecov Report

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

Pennyzct commented Jan 6, 2020 • edited Loading

jodh-intel commented Jan 6, 2020

Pennyzct commented Jan 8, 2020

Pennyzct commented Jan 9, 2020

WeiZhang555 commented Jan 14, 2020

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel commented Jan 20, 2020

Pennyzct commented Jan 27, 2020

devimc commented Jan 27, 2020

Pennyzct commented Jan 30, 2020

Pennyzct commented Jan 30, 2020 • edited Loading

amshinde commented Feb 7, 2020

amshinde commented Feb 7, 2020

Pennyzct commented Feb 10, 2020

Pennyzct commented Feb 17, 2020

Pennyzct commented Feb 18, 2020

Pennyzct commented Feb 19, 2020

Pennyzct commented Dec 25, 2019 •

edited

Loading

codecov bot commented Dec 25, 2019 •

edited

Loading

Pennyzct commented Jan 6, 2020 •

edited

Loading

Pennyzct commented Jan 30, 2020 •

edited

Loading