make sure kataAgent/createContainer can decode old specs.Spec #334

keloyang · 2018-05-23T07:34:16Z

In old specs.Spec, Capabilities is [] string, but we don't use CompatOCISpec for compatibility in kataAgent/createContainer, it lead to docker-1.11.2 failed to start kata-container, this pr will fix this.

before this pr,

[root@localhost linux]# docker run --rm -it --runtime kata-runtime busybox sh
docker: Error response from daemon: oci runtime error: json: cannot unmarshal array into Go struct field Process.capabilities of type specs.LinuxCapabilities.

after this pr

[root@localhost linux]# docker run --rm -it --runtime kata-runtime busybox sh
/ # exit

katabuilder · 2018-05-23T08:51:27Z

PSS Measurement:
Qemu: 141908 KB
Proxy: 6785 KB
Shim: 10897 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2011676 KB

jodh-intel · 2018-05-23T15:17:51Z

Hi @keloyang - thanks for raising. However, I'm not clear why we need to support docker 1.11 since:

that is a very old release now (https://github.com/moby/moby/releases/tag/v1.11.0).
docker 17 is available for Centos/RHEL 7, Ubuntu 16.04 LTS, etc...?

Also, note that this change breaks a number of the tests hence the CI failures.

keloyang · 2018-05-24T01:33:53Z

@jodh-intel thanks for your review.
The purpose of this pr is not only aim to the support for docker-1.11.2.
refer to https://github.com/kata-containers/runtime/blob/master/virtcontainers/pkg/oci/utils.go#L522 and https://github.com/kata-containers/runtime/blob/master/virtcontainers/kata_agent.go#L741
in ContainerConfig, we use CompatOCISpec and send it to kata-agent, but kata-agent use specs.Spec to decode it ， if docker send [] string as capabilities to kata-runtime, it will lead to a json decode err.
So before consuming capabilities, agent need translate capabilities to vc.LinuxCapabilities, or use CompatOCISpec to replace specs.Spec in kata-agent.

sboeuf

@keloyang This looks good to me, but just want to make sure about functionality here. Do you get the expected capabilities inside your container with Docker 1.11 ?

Also, this might be worth adding a test to ensure that we get the expected list of capabilities listed as a vc.LinuxCapabilities, no matter which spec version is used. This is important to make sure this PR solved an actual issue, and also to ensure this won't get broken later.

katabuilder · 2018-05-26T08:02:57Z

PSS Measurement:
Qemu: 144251 KB
Proxy: 4703 KB
Shim: 8811 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2012604 KB

keloyang · 2018-05-26T08:29:24Z

@sboeuf I have added the test cases. PTAL

katabuilder · 2018-05-26T08:37:23Z

PSS Measurement:
Qemu: 144025 KB
Proxy: 6728 KB
Shim: 10857 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2015388 KB

WeiZhang555 · 2018-05-29T06:32:32Z

CI is failing on these:

--- FAIL: TestCreateCreateSandboxFail (0.02s)
	assertions.go:239: 
                          
	Error Trace:	create_test.go:929
		
	Error:      	Should be true
=== RUN   TestCreateCreateContainerContainerConfigFail
--- FAIL: TestCreateCreateContainerContainerConfigFail (0.02s)
	assertions.go:239: 
                          
	Error Trace:	create_test.go:968
		
	Error:      	Should be true
	assertions.go:239: 
                          
	Error Trace:	create_test.go:968
		
	Error:      	Should be true
=== RUN   TestCreateCreateContainerFail
--- FAIL: TestCreateCreateContainerFail (0.02s)
	assertions.go:239: 
                          
	Error Trace:	create_test.go:1008
		
	Error:      	Should be true
	assertions.go:239: 
                          
	Error Trace:	create_test.go:1008
		
	Error:      	Should be true
=== RUN   TestCreateCreateContainer
--- FAIL: TestCreateCreateContainer (0.02s)
	assertions.go:239: 
                          
	Error Trace:	create_test.go:1055
		
	Error:      	Received unexpected error:
		
	            	Unexpected format for capabilities: map[permitted:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] ambient:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] bounding:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] effective:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] inheritable:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE]]
	assertions.go:239: 
                          
	Error Trace:	create_test.go:1055
		
	Error:      	Received unexpected error:
		
	            	Unexpected format for capabilities: map[inheritable:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] permitted:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] ambient:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] bounding:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE] effective:[CAP_AUDIT_WRITE CAP_KILL CAP_NET_BIND_SERVICE]]

I think they are real failing test cases, can you resolve this?

keloyang · 2018-05-30T03:49:13Z

@WeiZhang555 fixed, thanks.

katabuilder · 2018-05-30T04:22:56Z

PSS Measurement:
Qemu: 142563 KB
Proxy: 6591 KB
Shim: 8792 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2011340 KB

katabuilder · 2018-05-30T05:26:48Z

PSS Measurement:
Qemu: 142565 KB
Proxy: 6786 KB
Shim: 10880 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2013720 KB

amshinde · 2018-05-30T18:18:53Z

Looks like this broke with the kata-agent since the parsing is done in a couple of places now. I remember this working with clear-containers.

amshinde · 2018-05-30T18:21:15Z

cli/main_test.go

@@ -421,7 +421,13 @@ func readOCIConfigFile(configPath string) (oci.CompatOCISpec, error) {
 	if err := json.Unmarshal(data, &ociSpec); err != nil {
 		return oci.CompatOCISpec{}, err
 	}
-
+	if ociSpec.Process != nil {


I think it would be better to add this check for Process in containerCapabilities function itself. If the process is nil, return empty capabilities in that function itself. You can avoid checking for nil value of Process at multiple places.

If add the check in ContainerCapabilities, we don't know whether ociSpec.Process is nil, so we still need check outside of ContainerCapabilities.

@amshinde what do you mean, like the following ?

caps, err := containerCapabilities(ocispec) if err != nil { return CompatOCISpec{}, err } ocispec.Process.Capabilities = caps func containerCapabilities(s CompatOCISpec) (vc.LinuxCapabilities, error) { if ocispec.Process == nil { return vc.LinuxCapabilities{}, err } ... }

I think @amshinde 's suggestion is right.

func containerCapabilities(s CompatOCISpec) (vc.LinuxCapabilities, error) { if s.Process == nil { return vc.LinuxCapabilities{}, nil } ... }

This is also necessary for avoiding contianerCapabilities() to panic in case s.Process is nil

@keloyang Yes, basically what @WeiZhang555 pasted above.

sboeuf · 2018-05-31T22:02:00Z

@keloyang please rework as suggested by @WeiZhang555 and @amshinde. This LGTM when this will be done !

codecov · 2018-06-01T01:56:09Z

Codecov Report

Merging #334 into master will increase coverage by 0.14%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   63.74%   63.89%   +0.14%     
==========================================
  Files          87       87              
  Lines        8731     8633      -98     
==========================================
- Hits         5566     5516      -50     
+ Misses       2569     2532      -37     
+ Partials      596      585      -11

Impacted Files	Coverage Δ
virtcontainers/pkg/oci/utils.go	`77.64% <50%> (-0.76%)`	⬇️
virtcontainers/kata_agent.go	`28.61% <0%> (-1.17%)`	⬇️
virtcontainers/noop_agent.go	`90.9% <0%> (-0.76%)`	⬇️
virtcontainers/api.go	`63.12% <0%> (-0.62%)`	⬇️
virtcontainers/pkg/vcmock/mock.go	`95.18% <0%> (-0.43%)`	⬇️
cli/pause.go	`100% <0%> (ø)`	⬆️
virtcontainers/implementation.go	`0% <0%> (ø)`	⬆️
virtcontainers/sandbox.go	`67.45% <0%> (+0.34%)`	⬆️
virtcontainers/hyperstart_agent.go	`59.12% <0%> (+0.54%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f5f619...9a0434d. Read the comment docs.

keloyang · 2018-06-01T02:47:54Z

ping @sboeuf @WeiZhang555 @amshinde

katabuilder · 2018-06-01T06:18:29Z

PSS Measurement:
Qemu: 161906 KB
Proxy: 4664 KB
Shim: 12811 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 1996004 KB

sboeuf · 2018-06-01T06:29:10Z

Thanks @keloyang, this LGTM. I'm leaving the final words to @amshinde and @WeiZhang555 for merging this :)

in old specs.Spec, Capabilities is [] string, but we don't use CompatOCISpec for compatibility in kataAgent/createContainer. fixes #333 Signed-off-by: y00316549 <yangshukui@huawei.com>

keloyang · 2018-06-04T01:00:18Z

ping @WeiZhang555 @amshinde, PTAL, thanks.

bergwolf · 2018-06-04T02:20:11Z

lgtm!

ubuntu ci failure looks to be unrelated.

Makefile:35: recipe for target 'crio' failed
make: *** [crio] Error 1
Build step 'Execute shell' marked build as failure

WeiZhang555 · 2018-06-04T03:29:24Z

LGTM

keloyang force-pushed the spec-compatibility branch 4 times, most recently from 4a3e33f to e195a6f Compare May 23, 2018 08:13

sboeuf reviewed May 25, 2018

View reviewed changes

keloyang force-pushed the spec-compatibility branch from e195a6f to 043fa5f Compare May 26, 2018 06:39

keloyang closed this May 26, 2018

keloyang force-pushed the spec-compatibility branch from 043fa5f to 086d197 Compare May 26, 2018 06:54

keloyang reopened this May 26, 2018

keloyang force-pushed the spec-compatibility branch from c778371 to 29144d0 Compare May 30, 2018 03:47

keloyang force-pushed the spec-compatibility branch from 29144d0 to 1b592fa Compare May 30, 2018 04:49

jodh-intel mentioned this pull request May 30, 2018

unittest: tiny fix for incorrect parameters #348

Merged

amshinde reviewed May 30, 2018

View reviewed changes

keloyang force-pushed the spec-compatibility branch from 1b592fa to 16bf8bf Compare June 1, 2018 01:56

keloyang force-pushed the spec-compatibility branch from 16bf8bf to 9a0434d Compare June 1, 2018 05:46

sboeuf approved these changes Jun 1, 2018

View reviewed changes

virtcontainers: make kataAgent/createContainer can decode old specs.Spec

9a0434d

in old specs.Spec, Capabilities is [] string, but we don't use CompatOCISpec for compatibility in kataAgent/createContainer. fixes #333 Signed-off-by: y00316549 <yangshukui@huawei.com>

WeiZhang555 merged commit 813c8c3 into kata-containers:master Jun 4, 2018

jodh-intel mentioned this pull request Jun 19, 2018

document oldest docker version supported kata-containers/documentation#169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make sure kataAgent/createContainer can decode old specs.Spec #334

make sure kataAgent/createContainer can decode old specs.Spec #334

keloyang commented May 23, 2018

katabuilder commented May 23, 2018

jodh-intel commented May 23, 2018

keloyang commented May 24, 2018

sboeuf left a comment

katabuilder commented May 26, 2018

keloyang commented May 26, 2018

katabuilder commented May 26, 2018

WeiZhang555 commented May 29, 2018

keloyang commented May 30, 2018

katabuilder commented May 30, 2018

katabuilder commented May 30, 2018

amshinde commented May 30, 2018

amshinde May 30, 2018

keloyang May 31, 2018

keloyang May 31, 2018

WeiZhang555 May 31, 2018

amshinde May 31, 2018

sboeuf commented May 31, 2018

codecov bot commented Jun 1, 2018 •

edited

Loading

keloyang commented Jun 1, 2018

katabuilder commented Jun 1, 2018

sboeuf commented Jun 1, 2018

keloyang commented Jun 4, 2018

bergwolf commented Jun 4, 2018 •

edited by amshinde

Loading

WeiZhang555 commented Jun 4, 2018 •

edited by amshinde

Loading

make sure kataAgent/createContainer can decode old specs.Spec #334

make sure kataAgent/createContainer can decode old specs.Spec #334

Conversation

keloyang commented May 23, 2018

katabuilder commented May 23, 2018

jodh-intel commented May 23, 2018

keloyang commented May 24, 2018

sboeuf left a comment

Choose a reason for hiding this comment

katabuilder commented May 26, 2018

keloyang commented May 26, 2018

katabuilder commented May 26, 2018

WeiZhang555 commented May 29, 2018

keloyang commented May 30, 2018

katabuilder commented May 30, 2018

katabuilder commented May 30, 2018

amshinde commented May 30, 2018

amshinde May 30, 2018

Choose a reason for hiding this comment

keloyang May 31, 2018

Choose a reason for hiding this comment

keloyang May 31, 2018

Choose a reason for hiding this comment

WeiZhang555 May 31, 2018

Choose a reason for hiding this comment

amshinde May 31, 2018

Choose a reason for hiding this comment

sboeuf commented May 31, 2018

codecov bot commented Jun 1, 2018 • edited Loading

Codecov Report

keloyang commented Jun 1, 2018

katabuilder commented Jun 1, 2018

sboeuf commented Jun 1, 2018

keloyang commented Jun 4, 2018

bergwolf commented Jun 4, 2018 • edited by amshinde Loading

WeiZhang555 commented Jun 4, 2018 • edited by amshinde Loading

codecov bot commented Jun 1, 2018 •

edited

Loading

bergwolf commented Jun 4, 2018 •

edited by amshinde

Loading

WeiZhang555 commented Jun 4, 2018 •

edited by amshinde

Loading