cli: implement spec command by caoruidong · Pull Request #196 · kata-containers/runtime

caoruidong · 2018-04-10T15:48:13Z

Add spec command that that generates a basic config.json for kata.

fixes #188

Signed-off-by: Ruidong Cao caoruidong@huawei.com

codecov · 2018-04-10T17:18:42Z

Codecov Report

Merging #196 into master will decrease coverage by 0.07%.
The diff coverage is 38.09%.

@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
- Coverage   65.33%   65.26%   -0.08%     
==========================================
  Files          73       74       +1     
  Lines        7709     7730      +21     
==========================================
+ Hits         5037     5045       +8     
- Misses       2128     2135       +7     
- Partials      544      550       +6

Impacted Files	Coverage Δ
cli/main.go	`92.37% <ø> (ø)`	⬆️
cli/spec.go	`38.09% <38.09%> (ø)`

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 427b97c...b528ef2. Read the comment docs.

devimc · 2018-04-10T18:40:30Z

cli/spec.go

@@ -0,0 +1,108 @@
+package main


missing license

devimc · 2018-04-10T18:43:54Z

cli/spec.go

+		if err != nil {
+			return err
+		}
+		if err := ioutil.WriteFile(specConfig, data, 0666); err != nil {


pure evil ehh 😄

Change mode to 640?

runc uses 0664

I think runc is using 0666: https://github.com/opencontainers/runc/blob/master/spec.go#L110

But I don't know why, I think 0640 could be enough.

that's weird

$ docker-runc spec $ ls -l config.json -rw-rw-r-- 1 X X 597 Apr 11 10:33 config.json

Yes, let's do better than runc here, 0640 will be enough.

devimc · 2018-04-10T18:46:06Z

cli/spec.go

+			Usage: "generate a configuration for a rootless container",
+		},
+	},
+	Action: func(context *cli.Context) error {


please add an unit test for this

WeiZhang555 · 2018-04-11T09:01:04Z

cli/spec.go

+		cli.BoolFlag{
+			Name:  "rootless",
+			Usage: "generate a configuration for a rootless container",
+		},


I'm not sure if we can support "rootless" container in kata in future, as I know this is a runc feature that allows you to run a container without root privilege.

This flag needs to be removed if it's not supported yet.

WeiZhang555 · 2018-04-11T09:02:09Z

cli/spec.go

+		rootless := context.Bool("rootless")
+		if rootless {
+			specconv.ToRootless(spec)
+		}


This part needs to be removed if rootless container isn't supported yet

WeiZhang555 · 2018-04-11T09:11:01Z

I'm wondering if these vendor files added by dep tool? Please update the vendor with dep tool, one reference link is:
#141

And thanks for adding spec command, this is useful especially for following runc command line.

bergwolf · 2018-04-11T09:11:52Z

Could you add a test case the makes sure we can run a kata container using the default spec? There is an example in runv for creating a busybox bundle that might help you.

caoruidong · 2018-04-11T09:25:39Z

@bergwolf Sure. Thanks. I'm working on this.
@WeiZhang555 Yes. I will update with dep together with unit test.

caoruidong · 2018-04-12T05:40:16Z

@WeiZhang555 Vendor updated.
@bergwolf After testing, the default spec can work with busybox. Do you mean that add a test case in spec_test.go to explicitly call kata-runtime run with default spec and busybox bundle ?
@ALL Is runc's example spec too long or inappropriate for kata-runtime? for example https://github.com/opencontainers/runc/blob/3cbb2fa3c4e8f307817317c455268c6e58703e0b/libcontainer/specconv/example.go#L66
Hostname: "runc",
Do we need to write our own spec example?

bergwolf · 2018-04-12T06:37:59Z

@caoruidong I don't think we can do it in spec_test.go. I was thinking some place in https://github.com/kata-containers/tests. It does not block this PR though.

caoruidong · 2018-04-12T06:46:23Z

@bergwolf So I think test_data/Makefile should be moved to https://github.com/kata-containers/tests.

bergwolf · 2018-04-12T06:48:58Z

@caoruidong Feel free to use it when adding new tests. I just pointed to the runv tests for your reference.

sboeuf · 2018-04-12T13:48:05Z

@devimc PTAL.

devimc

lgtm

caoruidong · 2018-04-13T05:08:36Z

@jodh-intel @WeiZhang555 PTAL

devimc · 2018-04-13T15:21:46Z

@caoruidong rebase please

WeiZhang555 · 2018-04-13T15:51:19Z

LGTM after rebase.
Nice patch!

caoruidong · 2018-04-15T05:30:18Z

Rebased!

caoruidong · 2018-04-17T05:08:16Z

Can this be merged?

jodh-intel

lgtm. I'd better update kata-containers/documentation#48 to remove this limitation... ;)

Update the limitations document to remove the `spec` command limitation (implemented on kata-containers/runtime#196). Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2018-04-17T08:45:46Z

Done on kata-containers/documentation@2f2956e.

jodh-intel · 2018-04-17T08:55:03Z

Tests fine in stand-alone mode fwiw (using the strategy below for kata-runtime):

https://github.com/clearcontainers/runtime/wiki/Running-standalone

grahamwhaley · 2018-04-17T09:04:12Z

The only thing stopping the merge is the red codecov reports - if we are happy with that @jodh-intel , then let's merge...

grahamwhaley · 2018-04-17T10:03:11Z

cli/spec.go

+// Copyright (c) 2014,2015,2016,2017 Docker, Inc.
+// Copyright (c) 2018 Huawei Corporation.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");


Ah, I thought the standard for this project (the whole of kata) was the SPDX style copyright like:
https://github.com/kata-containers/runtime/blob/master/cli/fatal.go#L3
but, I note we have some non-SPDX copyright in the code base already, such as:
https://github.com/kata-containers/runtime/blob/master/cli/config.go#L3

@egernst @sboeuf @jodh-intel , thoughts?
And, maybe we can add a static check to catch any future deviations (whichever way we go) to the CI - maybe.

Other than that, this copyright looks correct to me afaict - the original code coming from https://github.com/opencontainers/runc/blob/master/spec.go I believe. That original file does not have an explicit copyright (it relies on the project wide LICENSE file I believe), so I presume we looked at the git logs to get the date range in the above - which I believe is fine and correct.

Maybe we can add a URL reference to that original file just below the copyright to make the historical origins clear?

btw, if we do all agree on SPDX (which we did I think at project inception, but we probably did not document well enough, and have not always caught transgressions), then I'm happy to Issue/PR to update/fix them all etc.

Good spot @grahamwhaley! Yep, I agree - we should ensure all our files have the SPDX header where possible.

I've raised kata-containers/tests#240 to add a CI check to enforce this for new / modified files, but would be great to have the existing codebase "SPDX-ified" 😄

caoruidong · 2018-04-17T14:33:40Z

@grahamwhaley It's green last time... I'll update again.
@jodh-intel SPDX-ing together.

Add spec command that generates a basic config.json for kata. fixes kata-containers#188 Signed-off-by: Ruidong Cao <caoruidong@huawei.com> Signed-off-by: Ruidong <caoruidong@huawei.com>

jodh-intel · 2018-04-17T17:07:37Z

yep - let's do this! ;)

Update the limitations document to remove the `spec` command limitation (implemented on kata-containers/runtime#196). Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

agent: Rollback properly when container creation fails

caoruidong force-pushed the add-spec branch 3 times, most recently from c7e05a3 to b2c2805 Compare April 10, 2018 16:05

devimc suggested changes Apr 10, 2018

View reviewed changes

devimc reviewed Apr 10, 2018

View reviewed changes

WeiZhang555 reviewed Apr 11, 2018

View reviewed changes

caoruidong force-pushed the add-spec branch from b2c2805 to 581fd45 Compare April 11, 2018 09:29

sboeuf requested a review from jodh-intel April 11, 2018 17:50

sboeuf approved these changes Apr 11, 2018

View reviewed changes

caoruidong force-pushed the add-spec branch 2 times, most recently from 2dad24c to e6e88d1 Compare April 12, 2018 05:34

caoruidong force-pushed the add-spec branch from e6e88d1 to df9fd8d Compare April 12, 2018 07:17

devimc approved these changes Apr 12, 2018

View reviewed changes

caoruidong force-pushed the add-spec branch from df9fd8d to 7bf2020 Compare April 15, 2018 04:12

jodh-intel approved these changes Apr 17, 2018

View reviewed changes

grahamwhaley reviewed Apr 17, 2018

View reviewed changes

jodh-intel mentioned this pull request Apr 17, 2018

CI: add license checker kata-containers/tests#240

Closed

cli: implement spec command

b528ef2

Add spec command that generates a basic config.json for kata. fixes kata-containers#188 Signed-off-by: Ruidong Cao <caoruidong@huawei.com> Signed-off-by: Ruidong <caoruidong@huawei.com>

caoruidong force-pushed the add-spec branch from 7bf2020 to b528ef2 Compare April 17, 2018 14:38

jodh-intel merged commit a4b7e20 into kata-containers:master Apr 17, 2018

caoruidong deleted the add-spec branch April 24, 2018 14:54

zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019

Merge pull request kata-containers#196 from sboeuf/fix_rollback

945b49c

agent: Rollback properly when container creation fails

Conversation

caoruidong commented Apr 10, 2018

Uh oh!

codecov bot commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergwolf commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caoruidong commented Apr 11, 2018

Uh oh!

caoruidong commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergwolf commented Apr 12, 2018

Uh oh!

caoruidong commented Apr 12, 2018

Uh oh!

bergwolf commented Apr 12, 2018

Uh oh!

sboeuf commented Apr 12, 2018

Uh oh!

devimc left a comment

Choose a reason for hiding this comment

Uh oh!

caoruidong commented Apr 13, 2018

Uh oh!

devimc commented Apr 13, 2018

Uh oh!

WeiZhang555 commented Apr 13, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caoruidong commented Apr 15, 2018

Uh oh!

caoruidong commented Apr 17, 2018

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Apr 17, 2018

Uh oh!

jodh-intel commented Apr 17, 2018

Uh oh!

grahamwhaley commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2018 •

edited

Loading

WeiZhang555 commented Apr 11, 2018 •

edited

Loading

bergwolf commented Apr 11, 2018 •

edited

Loading

caoruidong commented Apr 12, 2018 •

edited

Loading

WeiZhang555 commented Apr 13, 2018 •

edited by amshinde

Loading

caoruidong commented Apr 17, 2018 •

edited

Loading