-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust runc to new opencontainers/specs version #242
Conversation
How does `runc` integrate with the Open Container Initiative Specification? | ||
`runc` depends on the types specified in the | ||
[specs](https://github.com/opencontainers/specs) repository. Whenever the | ||
specification is updated and ready to be versioned `runc` will update it's dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/it's/its
@mrunalp fixed |
@@ -280,8 +352,8 @@ To test using Docker's `busybox` image follow these steps: | |||
mkdir rootfs | |||
tar -C rootfs -xf busybox.tar | |||
``` | |||
* Create a file called `config.json` using the example from above. You can also | |||
generate a spec using `runc spec`, redirecting the output into `config.json` | |||
* Create a files `config.json` and `runtime.json` using the example from above. You can also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a files//
or s/a/the/
@mrunalp fixed |
### OCF Container JSON Format: | ||
|
||
Below is a sample `config.json` configuration file. It assumes that | ||
Below are sample `config.json` and `runtime.json` configuration files. It assumes that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to move this generic OCS documentation into opencontainers/specs. Then you can link to that overview from opencontainers/runc, and not worry about keeping your OCS docs in sync with the upstream spec. Folks suggesting changes to the spec would just update the docs when PRing their changes, and the OCS maintainers would review the whole change as a cohesive unit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, let's do this in another PR though. It was supposed to be mostly code PR to get runc working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Tue, Sep 01, 2015 at 04:29:17PM -0700, Alexander Morozov wrote:
-### OCF Container JSON Format:
-Below is a sample
config.json
configuration file. It assumes that
+Below are sampleconfig.json
andruntime.json
configuration files. It assumes thatAgree, let's do this in another PR though. It was supposed to be
mostly code PR to get runc working.
Sounds good.
After looking at the configs in this PR I don't see how mounts in the config.json with Name are related to mounts in the RuntimeSpec? |
Yeah, need a check to make sure that all the mounts in config.json are satisfied in runtime.json. |
@mrunalp i think it's a problem in the spec. The Name is not used and I don't see how they can even relate to eachother |
I just think the |
@crosbymichael I agree. |
They related Path <-> Destination. If we'll have name in mounts, then Path is redundant. And then mounts probably should be a map. |
@crosbymichael 338 line in spec.go |
I think that it makes sense to add Name and take out destination. Sent from my iPhone
|
On Tue, Sep 01, 2015 at 06:25:10PM -0700, Alexander Morozov wrote:
I agree that we should use maps on both ends, and the current |
@wking I'll do it tomorrow. I'm working on docker integration and probably will notice more rough edges in implementation. |
On Tue, Sep 01, 2015 at 08:26:12PM -0700, Alexander Morozov wrote:
Ok. If we went with two maps, we'd need logic in the runtime to order |
} | ||
], | ||
} | ||
"mounts": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this example needs to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. update in a second
85119c3
to
149c67b
Compare
@LK4D4 Hmmm |
On Mon, Sep 14, 2015 at 12:38:16PM -0700, Alexander Morozov wrote:
Previous discussion about this alias in #210 (after 1). Maybe a |
@mrunalp I made |
@LK4D4 we can remove the alias now since this is a big update with the multiple spec files so it's breaking anyways |
@crosbymichael I already made flags global, so alias works. Not sure if you still want to remove it. |
@LK4D4 i would probably do it now since we are making large breaking changes anyways |
Sounds good to me. 👍 |
On Mon, Sep 14, 2015 at 01:22:35PM -0700, Michael Crosby wrote:
Updating the spec breaks the bundle-author API (which runC does |
@crosbymichael I dropped alias. Now it's only |
@LK4D4 ok but i don't think u pushed the commit |
@crosbymichael Pushed. Sorry for that. |
ea63e74
to
607c9d8
Compare
@crosbymichael I think now I pushed something. |
Looks fine to me but janky isn't working |
}, | ||
Action: func(context *cli.Context) { | ||
imagePath := context.String("image-path") | ||
if imagePath == "" { | ||
imagePath = getDefaultImagePath(context) | ||
} | ||
spec, err := loadSpec(context.Args().First()) | ||
spec, rspec, err := loadSpec(context.GlobalString("config-file"), context.GlobalString("runtime-file")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be GlobalString
here and just be String
I think maybe these changes are needed: diff --git a/main.go b/main.go
index 5f59783..33a86fc 100644
--- a/main.go
+++ b/main.go
@@ -5,10 +5,11 @@ import (
"github.com/Sirupsen/logrus"
"github.com/codegangsta/cli"
+ "github.com/opencontainers/specs"
)
const (
- version = "0.2"
+ version = "0.3"
usage = `Open Container Initiative runtime
runc is a command line client for running applications packaged according to
@@ -54,7 +55,7 @@ func main() {
},
cli.StringFlag{
Name: "root",
- Value: "/run/oci",
+ Value: specs.LinuxStateDirectory,
Usage: "root directory for storage of container state (this should be located in tmpfs)",
},
cli.StringFlag{
diff --git a/restore.go b/restore.go
index 094a4d3..bf12cb4 100644
--- a/restore.go
+++ b/restore.go
@@ -60,7 +60,7 @@ var restoreCommand = cli.Command{
if imagePath == "" {
imagePath = getDefaultImagePath(context)
}
- spec, rspec, err := loadSpec(context.GlobalString("config-file"), context.GlobalString("runtime-file"))
+ spec, rspec, err := loadSpec(context.String("config-file"), context.String("runtime-file"))
if err != nil {
fatal(err)
} |
I deleted possibility to specify config file from commands for now. Until we decide how it'll be done. Also I changed runc spec interface to write config files instead of output them. Signed-off-by: Alexander Morozov <lk4d4@docker.com>
@crosbymichael Incorporated your changes. |
LGTM |
1 similar comment
LGTM |
Adjust runc to new opencontainers/specs version
I deleted possibility to specify config file from commands for now.
Until we decide how it'll be done. Also I changed runc spec interface to
write config files instead of output them.