oci-image-tool: implement create-runtime-bundle #114

s-urbaniak · 2016-06-02T14:07:43Z

This is a first shot towards a oci-image-tool create-runtime-bundle subcommand.

I found that converting towards the runtime bundle we have (obviously) quite some impedance mismatch here and there so I guess I surely missed something.

The most important questions for me are:

What ociVersion to I have to set?
How to map image Volumes to bundle mounts?
Is there anything else obvious I am missing?
We have redundant declarations of layers, one in the manifest, one in the image JSON, it is not clear to me which layer declaration is the one I should pick for unpacking to the bundle. This also applies to unpack. I opened Redunant definition of layers #115 for this.

Generally I have the impression that I can only fill up only a small subset of the bundle fields, hence I ask for advice how to proceed here.

s-urbaniak · 2016-06-02T14:17:26Z

@stevvooe @philips @vbatts PTAL

wking · 2016-06-02T16:52:57Z

On Thu, Jun 02, 2016 at 07:07:44AM -0700, Sergiusz Urbaniak wrote:

What ociVersion to I have to set?

For now, you probably want to set the tip ociVersion (0.5.0 at the
moment, but 1.0 rcs are coming out soon). Once we cut 1.0 and start
maintaining backwards compatibility, you should pick the lowest
runtime version that has everything you need (e.g. stay with 1.0
unless 1.1 lands a new feature you need).

wking · 2016-06-02T16:54:40Z

image/config.go

+
+func (c *config) runtimeSpec(rootfs string) specs.Spec {
+	var s specs.Spec
+	s.Version = "v0.5.0"


You don't want the v prefix (see this example).

ha, good catch, thanks!

fixed locally

wking · 2016-06-02T17:06:23Z

On Thu, Jun 02, 2016 at 07:07:44AM -0700, Sergiusz Urbaniak wrote:

How to map image Volumes to bundle mounts?

I'd fill in bind-mount entries and just leave ‘source’ entry null.
For example:

"mounts": [
{
"destination": "${path_from_image_config}",
"type": "bind",
"source": null,
"options": ["rbind"]
}
]

Then the runtime can fill in (or remove) the null-source entries
depending on ‘docker run -v a:b …’ etc.

wking · 2016-06-02T17:46:26Z

image/config.go

+	if ug := strings.Split(c.Config.User, ":"); len(ug) == 2 {
+		if uid, err := strconv.Atoi(ug[0]); err == nil {
+			s.Process.User.UID = uint32(uid)
+		}


These should probably be throwing “not supported yet” errors if it can't make the conversion until you add code to spin up a container with all the mounts and use getent or similar (see opencontainers/runtime-spec#38). Docker may be doing something like this already, so maybe you can grab their code. And this is hideous, ugly, and dangerous, which is why the runtime spec punted it to config authors.

ack, I'll return an error in this case, thanks for the suggestion 👍

s-urbaniak · 2016-06-03T12:42:06Z

@wking addressed review comments, PTAL and thanks for the review!

stevvooe · 2016-06-03T19:03:07Z

image/config.go

+	s.Platform.OS = c.OS
+	s.Platform.Arch = c.Architecture
+
+	if c.OS == "linux" {


Should this return an error when run on an unsupported OS?

On Fri, Jun 03, 2016 at 12:03:08PM -0700, Stephen Day wrote:

if c.OS == "linux" {

Should this return an error when run on an unsupported OS?

I don't think so. Perhaps the user is unpacking an image on one
machine that they intend to copy (via a shared filesystem?) and run on
another. It's probably not going to be a core use-case, but if the
image tool is capable of unpacking the image (regardless of its
target OS), I think it should unpack it.

The runtime, on the other hand, must die for unsupported platforms.
An explicit statement to that effect landed two weeks ago via
opencontainers/runtime-spec#441 1.

To be compatible, it needs to have a conversion for every single OS and needs to fail if it doesn't support that particular OS, as it may drop relevant data.

On Fri, Jun 03, 2016 at 02:51:22PM -0700, Stephen Day wrote:

if c.OS == "linux" {

To be compatible, it needs to have a conversion for every single OS…

I don't think you need that, if you have this:

… and needs to fail if it doesn't support that particular OS, as it
may drop relevant data.

So “die if you are asked to convert a format you can't handle
correctly”, not “die if you are asked to convert a format targetting
another platform”.

As far as I see it I am "dying if I am asked to convert a format I can't handle correctly", so does this looks good to you?

On Mon, Jun 06, 2016 at 04:12:27AM -0700, Sergiusz Urbaniak wrote:

if c.OS == "linux" {

As far as I see it I am "dying if I am asked to convert a format I
can't handle correctly", so does this looks good to you?

Yeah. This sub-thread was just clarifying @stevvooe's concern about a
blanket supported-OS check 1, and we both agree that you don't need
that blanket check 2.

As far as I see it I am "dying if I am asked to convert a format I can't handle correctly", so does this looks good to you?

Where is this happening? It looks like it checks for linux and then moves on if it doesn't understand the OS.

@stevvooe ok, I think I misunderstood ;-)

I'm merely dying when validation failed (https://github.com/opencontainers/image-spec/pull/114/files#diff-da07f373046a0a24e67b18699dc15a70R142)

I'll change the code to return an err if I get an OS != "linux".

wking · 2016-06-03T21:08:27Z

On Fri, Jun 03, 2016 at 05:42:07AM -0700, Sergiusz Urbaniak wrote:

@wking addressed review comments, PTAL and thanks for the review!

I haven't gone over the whole thing again, but 5a38e9f → cfd87a7 looks
good to me.

philips · 2016-06-07T06:01:52Z

LGTM overall

The ExposedPorts thing is an issue but I don't think we should bike shed over it and instead discuss on #87.

Fixes opencontainers#99 Signed-off-by: Sergiusz Urbaniak <[email protected]>

philips · 2016-06-08T03:27:05Z

Can anyone else review and give an LGTM on this one? @opencontainers/image-spec-maintainers

LGTM

vbatts · 2016-06-09T10:53:24Z

LGTM

stevvooe · 2016-06-12T00:49:49Z

Whoops, sorry.

LGTM, here.

…ed config The image-tools framework has attempted this since d3ffc1c (oci-image-tool: implement create-runtime-bundle, 2016-06-02, opencontainers#114), but we didn't specify whether the translation was required or what the output of the translation should be. That means downstream consumers of an unpacked image couldn't be sure if they'd find a config.json or (if they found one) which runtime-spec versions it would be compatible with. With this commit, the config output becomes specified, so consumers can post-process their config.json and/or invoke a runtime-spec 1.0.0-rc2-compatible runtime on it without worrying about the presence or version of the unpacked config.json. I've picked 1.0.0-rc2 as the most recent runtime-spec commit. As the runtime-spec moves forward with more RCs, I expect we'll want to bump this to keep up. Once runtime-spec hits 1.0, we can probably freeze the target, since post 1.0 releases in runtime-spec's 1.x line are unlikely to make translation from the config format easier, and any 1.x-compatible runtime will be able to handle 1.0 configs. Signed-off-by: W. Trevor King <[email protected]>

s-urbaniak force-pushed the bundle branch 2 times, most recently from 0aaa5e3 to 5a38e9f Compare June 2, 2016 14:17

wking reviewed Jun 2, 2016
View reviewed changes

s-urbaniak force-pushed the bundle branch from 5a38e9f to cfd87a7 Compare June 3, 2016 12:41

stevvooe reviewed Jun 3, 2016
View reviewed changes

s-urbaniak force-pushed the bundle branch 2 times, most recently from d5cf9d2 to 70a918d Compare June 6, 2016 10:23

oci-image-tool: implement create-runtime-bundle

d3ffc1c

Fixes opencontainers#99 Signed-off-by: Sergiusz Urbaniak <[email protected]>

s-urbaniak force-pushed the bundle branch from 70a918d to d3ffc1c Compare June 7, 2016 16:21

vbatts merged commit 1b0dafa into opencontainers:master Jun 9, 2016

wking mentioned this pull request Nov 8, 2016

manifest: Require unpackers to create a config.json from the referenced config #454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oci-image-tool: implement create-runtime-bundle #114

oci-image-tool: implement create-runtime-bundle #114

s-urbaniak commented Jun 2, 2016 •

edited

Loading

s-urbaniak commented Jun 2, 2016

wking commented Jun 2, 2016

wking Jun 2, 2016

s-urbaniak Jun 2, 2016

s-urbaniak Jun 3, 2016

wking commented Jun 2, 2016

wking Jun 2, 2016

s-urbaniak Jun 3, 2016

s-urbaniak commented Jun 3, 2016

stevvooe Jun 3, 2016

wking Jun 3, 2016

stevvooe Jun 3, 2016

wking Jun 3, 2016

s-urbaniak Jun 6, 2016

wking Jun 6, 2016

stevvooe Jun 6, 2016

s-urbaniak Jun 6, 2016

wking commented Jun 3, 2016

philips commented Jun 7, 2016 •

edited by caniszczyk

Loading

philips commented Jun 8, 2016 •

edited by caniszczyk

Loading

vbatts commented Jun 9, 2016 •

edited by caniszczyk

Loading

stevvooe commented Jun 12, 2016 •

edited by caniszczyk

Loading

oci-image-tool: implement create-runtime-bundle #114

oci-image-tool: implement create-runtime-bundle #114

Conversation

s-urbaniak commented Jun 2, 2016 • edited Loading

s-urbaniak commented Jun 2, 2016

wking commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-urbaniak commented Jun 3, 2016

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

wking commented Jun 3, 2016

philips commented Jun 7, 2016 • edited by caniszczyk Loading

philips commented Jun 8, 2016 • edited by caniszczyk Loading

vbatts commented Jun 9, 2016 • edited by caniszczyk Loading

stevvooe commented Jun 12, 2016 • edited by caniszczyk Loading

s-urbaniak commented Jun 2, 2016 •

edited

Loading

philips commented Jun 7, 2016 •

edited by caniszczyk

Loading

philips commented Jun 8, 2016 •

edited by caniszczyk

Loading

vbatts commented Jun 9, 2016 •

edited by caniszczyk

Loading

stevvooe commented Jun 12, 2016 •

edited by caniszczyk

Loading