Adjust runc to new opencontainers/specs version #242

LK4D4 · 2015-09-01T16:34:25Z

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.

mrunalp · 2015-09-01T21:22:18Z

README.md

+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


LK4D4 · 2015-09-01T21:31:20Z

@mrunalp fixed

mrunalp · 2015-09-01T21:34:46Z

README.md

@@ -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


s/a files//
or s/a/the/

LK4D4 · 2015-09-01T21:50:55Z

@mrunalp fixed

wking · 2015-09-01T23:20:23Z

README.md

-### 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


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.

Agree, let's do this in another PR though. It was supposed to be mostly code PR to get runc working.

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 sample config.json and runtime.json configuration files. It assumes that

Agree, let's do this in another PR though. It was supposed to be
mostly code PR to get runc working.

Sounds good.

crosbymichael · 2015-09-01T23:45:52Z

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?

mrunalp · 2015-09-01T23:53:18Z

Yeah, need a check to make sure that all the mounts in config.json are satisfied in runtime.json.

crosbymichael · 2015-09-01T23:58:21Z

@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

wking · 2015-09-02T00:02:32Z

On Tue, Sep 01, 2015 at 04:58:23PM -0700, Michael Crosby wrote:

@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 agree that it's a spec problem. See discussion yesterday on IRC
between @mrunalp, @LK4D4, and myself.

crosbymichael · 2015-09-02T00:23:39Z

I just think the Mount struct is missing Name string and that should fix the issues.

mrunalp · 2015-09-02T00:47:13Z

@crosbymichael I agree.

LK4D4 · 2015-09-02T01:25:07Z

They related Path <-> Destination. If we'll have name in mounts, then Path is redundant. And then mounts probably should be a map.

LK4D4 · 2015-09-02T01:28:03Z

@crosbymichael 338 line in spec.go

mrunalp · 2015-09-02T01:48:06Z

I think that it makes sense to add Name and take out destination.

Sent from my iPhone

On Sep 1, 2015, at 6:25 PM, Alexander Morozov notifications@github.com wrote:

They related Path <-> Destination. If we'll have name in mounts, then Path is redundant. And then mounts probably should be a map.

—
Reply to this email directly or view it on GitHub.

wking · 2015-09-02T03:05:40Z

On Tue, Sep 01, 2015 at 06:25:10PM -0700, Alexander Morozov wrote:

They related Path <-> Destination. If we'll have name in mounts,
then Path is redundant. And then mounts probably should be a map.

I agree that we should use maps on both ends, and the current
Mount.Destination and MountPoint.Name should become keys to those
maps. Shall I make an opencontainers/specs PR to that effect, or does
someone else want to handle it?

LK4D4 · 2015-09-02T03:26:10Z

@wking I'll do it tomorrow. I'm working on docker integration and probably will notice more rough edges in implementation.

wking · 2015-09-02T03:35:35Z

On Tue, Sep 01, 2015 at 08:26:12PM -0700, Alexander Morozov wrote:

I'll do it tomorrow…

Ok. If we went with two maps, we'd need logic in the runtime to order
the mounts, so it's probably easier to keep the MountPoints entry as
an array (because they know about the destinations) and turn the
Mounts entry into a map (so you can't duplicate keys). In fact, it
might make sense to allow duplicate Names in the MountPoints array (in
which case the same entry from the Mounts map would be mounted at both
destinations.

crosbymichael · 2015-09-03T18:29:00Z

README.md

-                }
-        ],
-    }
+	"mounts": [


I think this example needs to be updated.

You're right. update in a second

mrunalp · 2015-09-14T19:44:19Z

@LK4D4 Hmmm

wking · 2015-09-14T19:47:51Z

On Mon, Sep 14, 2015 at 12:38:16PM -0700, Alexander Morozov wrote:

@mrunalp It sorta hurts me that runc is alias to runc start. How
about to remove this alias?

Previous discussion about this alias in #210 (after 1). Maybe a
deprecation phase where you keep the alias but warn to stderr if folks
use it? Then remove the alias after a few months of that?

LK4D4 · 2015-09-14T20:00:10Z

@mrunalp I made -config-file and -runtime-file global flags, now runc should work.

crosbymichael · 2015-09-14T20:22:33Z

@LK4D4 we can remove the alias now since this is a big update with the multiple spec files so it's breaking anyways

LK4D4 · 2015-09-14T20:27:27Z

@crosbymichael I already made flags global, so alias works. Not sure if you still want to remove it.

crosbymichael · 2015-09-14T20:30:51Z

@LK4D4 i would probably do it now since we are making large breaking changes anyways

mrunalp · 2015-09-14T20:54:38Z

Sounds good to me. 👍

wking · 2015-09-14T20:56:25Z

On Mon, Sep 14, 2015 at 01:22:35PM -0700, Michael Crosby wrote:

@LK4D4 we can remove the alias now since this is a big update with
the multiple spec files so it's breaking anyways

Updating the spec breaks the bundle-author API (which runC does
frequently). Dropping the ‘runc’ → ‘runc start’ alias breaks the
runC-caller API (which runC hasn't done yet, at least for the bare
‘runc’ invocation). Still, I don't foresee much trouble with breaking
the runC-caller API, I just think it would be gentler to give folks a
window to adjust. But we can sort out things like that once we start
versioning runC ;).

LK4D4 · 2015-09-14T21:07:45Z

@crosbymichael I dropped alias. Now it's only runc start.

crosbymichael · 2015-09-14T21:59:48Z

@LK4D4 ok but i don't think u pushed the commit

LK4D4 · 2015-09-14T22:00:56Z

@crosbymichael Pushed. Sorry for that.

LK4D4 · 2015-09-14T22:22:51Z

@crosbymichael I think now I pushed something.

mrunalp · 2015-09-14T23:03:47Z

Looks fine to me but janky isn't working

crosbymichael · 2015-09-15T00:16:24Z

restore.go

 	},
 	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"))


This should not be GlobalString here and just be String

crosbymichael · 2015-09-15T00:21:29Z

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>

LK4D4 · 2015-09-15T15:36:03Z

@crosbymichael Incorporated your changes.

crosbymichael · 2015-09-15T17:20:40Z

LGTM

mrunalp · 2015-09-15T17:21:55Z

LGTM

Adjust runc to new opencontainers/specs version

GordonTheTurtle added the status/0-triage label Sep 1, 2015

LK4D4 force-pushed the adjust_spec branch from 70a08c9 to d7c46b2 Compare September 1, 2015 17:03

mrunalp reviewed Sep 1, 2015
View reviewed changes

LK4D4 force-pushed the adjust_spec branch from d7c46b2 to 1c6aafc Compare September 1, 2015 21:31

mrunalp reviewed Sep 1, 2015
View reviewed changes

LK4D4 force-pushed the adjust_spec branch from 1c6aafc to 2b41009 Compare September 1, 2015 21:50

wking reviewed Sep 1, 2015
View reviewed changes

LK4D4 mentioned this pull request Sep 2, 2015

Change layout of mountpoints and mounts opencontainers/runtime-spec#136

Merged

LK4D4 force-pushed the adjust_spec branch from 2b41009 to 882c610 Compare September 3, 2015 18:10

crosbymichael reviewed Sep 3, 2015
View reviewed changes

LK4D4 force-pushed the adjust_spec branch 3 times, most recently from 85119c3 to 149c67b Compare September 3, 2015 19:07

LK4D4 force-pushed the adjust_spec branch from 9bc01db to 1196889 Compare September 14, 2015 19:59

LK4D4 force-pushed the adjust_spec branch 2 times, most recently from ea63e74 to 607c9d8 Compare September 14, 2015 22:22

crosbymichael reviewed Sep 15, 2015
View reviewed changes

Adjust runc to new opencontainers/specs version

ea5032b

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>

LK4D4 force-pushed the adjust_spec branch from 607c9d8 to ea5032b Compare September 15, 2015 15:35

mrunalp pushed a commit that referenced this pull request Sep 15, 2015

Merge pull request #242 from LK4D4/adjust_spec

4ab1324

Adjust runc to new opencontainers/specs version

mrunalp merged commit 4ab1324 into opencontainers:master Sep 15, 2015

LK4D4 deleted the adjust_spec branch September 15, 2015 17:26

cyphar mentioned this pull request Sep 15, 2015

cgroups: add pids controller support #58

Merged

wking mentioned this pull request Sep 23, 2015

Move example configs to opencontainers/specs? #297

Closed

wking mentioned this pull request Oct 2, 2015

specifying a spec file on cmd line? #310

Closed

wking mentioned this pull request Dec 18, 2015

source and schema: differentiate with examples opencontainers/runtime-spec#276

Merged

Adjust runc to new opencontainers/specs version #242

Adjust runc to new opencontainers/specs version #242

Conversation

LK4D4 commented Sep 1, 2015

Choose a reason for hiding this comment

LK4D4 commented Sep 1, 2015

Choose a reason for hiding this comment

LK4D4 commented Sep 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

-### OCF Container JSON Format:

crosbymichael commented Sep 1, 2015

mrunalp commented Sep 1, 2015

crosbymichael commented Sep 1, 2015

wking commented Sep 2, 2015

crosbymichael commented Sep 2, 2015

mrunalp commented Sep 2, 2015

LK4D4 commented Sep 2, 2015

LK4D4 commented Sep 2, 2015

mrunalp commented Sep 2, 2015

wking commented Sep 2, 2015

LK4D4 commented Sep 2, 2015

wking commented Sep 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Sep 14, 2015

wking commented Sep 14, 2015

LK4D4 commented Sep 14, 2015

crosbymichael commented Sep 14, 2015

LK4D4 commented Sep 14, 2015

crosbymichael commented Sep 14, 2015

mrunalp commented Sep 14, 2015

wking commented Sep 14, 2015

LK4D4 commented Sep 14, 2015

crosbymichael commented Sep 14, 2015

LK4D4 commented Sep 14, 2015

LK4D4 commented Sep 14, 2015

mrunalp commented Sep 14, 2015

Choose a reason for hiding this comment

crosbymichael commented Sep 15, 2015

LK4D4 commented Sep 15, 2015

crosbymichael commented Sep 15, 2015

mrunalp commented Sep 15, 2015