Add labels to the state

[gao-feng]

Labels is used to store some runtime specified informations,
these informations may be meaningless to other runtime, but it's
userful for the same runtime to operate this container.

Signed-off-by: Gao feng [email protected]

[wking]

On Mon, Sep 14, 2015 at 08:28:06PM -0700, Gao feng wrote:

1, clearly get the runtime of container, it's useful for statistic.

Hmm, what sort of statistics are you interested in? I think it would
be better to instrument your ‘start’ invocation (maybe for each
runtime on your system) to get this sort of thing. I'd like to have
different container-based runtimes be inter-operable (e.g. launch with
bocker but checkpoint with runC, etc.). There's really nothing
runtime-specific about the created container.

2, different kinds of runtime will not try to operate incompatible
container, such as, runc should not execute(runc exec) program on
runv container.

The current state layout doesn't really map to runV-launched
containers / applications anyway. For example, what is runV going to
put in the ‘pid’ field? I mentioned this in the 2015-08-26 minutes
1, and have fuzzy memories about the decision being “we're just
punting on that for now”.

[gao-feng]

@wking
Such as how many containers which type of runtime runs.
How about add two fields 'runtime-type" and 'runtime-name'? the same type of runtime can operate with each other.

Runv needs to store an unix socket path in state.json, we can name this socket as .sock. but this socket is totally meaningless for container based runtimes, they should not try to get useful information through the pid stored in state.json.

[wking]

On Mon, Sep 14, 2015 at 11:48:02PM -0700, Gao feng wrote:

Such as how many containers which type of runtime runs.

If you're interested in how many containers a given runtime starts,
I think your best bet is to instrument your start command 1. But
the start command seems like a small part of the container (or
hypervisor virtual machine, or whatever) lifecycle. Once your
container is started, who cares which runtime launched it? You should
be able to use other runtimes (assuming a compatible state) to perform
other lifecycle events.

Runv needs to store an unix socket path in state.json, we can name
this socket as .sock. but this socket is totally meaningless
for container based runtimes, they should not try to get useful
information through the pid stored in state.json.

We should address situations where there is a functional difference
(e.g. we don't have a useful application PID) directly (e.g. by
setting a ‘socket’ field and skipping the ‘pid’ field), and not
indirectly by putting the runtime name/type in the state file and
requiring consumers to map those names/types to the functional
differences.

[vishh]

I am not in favor of adding explicit Runtime specific information to the Spec or State. How about using runtime labels or annotations, which are arbitrary strings, for this purpose?

[vbatts]

Ah yeah
On Sep 16, 2015 13:47, "Vish Kannan" [email protected] wrote:

In config.go
#188 (comment):

@@ -67,4 +67,6 @@ type State struct {
Pid int json:"pid"
// Root is the path to the container's bundle directory.
Root string json:"root"

• // Runtime is the runtime of OCI container
• Runtime string json:"runtime"

I am not in favor of adding explicit Runtime specific information to the
Spec or State. How about using runtime labels or annotations, which are
arbitrary strings, for this purpose?

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/188/files#r39661833.

[laijs]

@gao-feng using labels instead is an excellent idea, allows the implementation to add the corresponding label. Could you update patch?

[gao-feng]

@vishh @vbatts @laijs @wking patch updated, please check

[wking]

Why restrict labels to the runtime itself? For example, see container labels in #108. I'd recommend we add an optional “labels” to config.json holding a list of arbitrary bundle-author-specified JSON. Then in the state JSON, we would have: "labels": { "runtime": …stuff specified by the runtime…, "bundle": …stuff from config.json's “labels”… }

[gao-feng]

@wking
I can not find out others which need to store informations in state.

"bundle": …stuff from config.json's “labels”…

it is already in config.json's "labels", right? why we add it to state.json again? the BundlePath in state.json will help us to access the config.json

[wking]

Maybe “Labels holds runtime-specified information that is not structured by this specification.”?

[wking]

On Fri, Oct 09, 2015 at 08:03:29PM -0700, Gao feng wrote:

"bundle": …stuff from config.json's “labels”…

it is already in config.json's "labels", right? why we add it to
state.json again? the BundlePath in state.json will help us to
access the config.json

Ah, fair enough. I guess I'm still convinced that putting the bundle
or rootfs path in the state JSON is a bad idea, but that's a different
discussion 1.

Sticking with the current spec's position that bundlePath is worth the
requirements it places on the stability and accessibility of the
bundle directory and its contents, the substance of a7eda6f looks
reasonable to me. I'd recommend we use json.RawMessage as the type,
to keep the values as opaque as possible, and I left some minor inline
copy-edits.

[gao-feng]

@wking Thanks for your suggestion, updated, please check. :)

[vbatts]

sorry, but why json.RawMessage? I presume for holding structures instead of just a string, but couldn't that be accomplished with a map[string]interface{} ?

[wking]

On Fri, Oct 16, 2015 at 06:51:30AM -0700, Vincent Batts wrote:

@@ -67,4 +67,6 @@ type State struct {
Pid int json:"pid"
// BundlePath is the path to the container's bundle directory.
BundlePath string json:"bundlePath"

• // Labels holds runtime-specified information that is not structured by this specification
• Labels map[string]json.RawMessage json:"labels"

sorry, but why json.RawMessage? I presume for holding structures
instead of just a string, but couldn't that be accomplished with a
map[string]interface{} ?

+1 to map[string]interface{}. Notes on using that from Go in 1.

[vishh]

Labels are typically used to grouping objects. See in docker and kubernetes.
I'm weary of overloading that concept to hold arbitrary data.
I'd say lets introduce a separate Annotation field that users can use to pass arbitrary data. Is this information expected to be persisted in the State?
Also if we switch to protobufs, extensions should let us pass arbitrary data and avoid having to include Annotations in the Spec.
Any thoughts @vbatts @philips ?

[vbatts]

There is value in labels or annotations in the mutable content IMO.

Though, even in it's current form, there is nothing stopping users from provided non-spec required fields in the config.json. i.e. the config is not invalid by having additional fields. And I feel that it should stay this way.
The spec will define behavior on the known fields (regardless required or optional). Should there be other fields added, it is undefined behavior, but not prohibited.

So rather than giving up to allow folks to shit all over the place, better to give a space like labels/annotations for them to dump content that is mutable and a part of the bundle's digest.

Now if there were to be an actual section for arbitrary runtime annotations, would that be separated from arbitrary data? I've not got a concrete example of this besides runtime hints of asking for 2g memory vs. some projects internal version numbering / git commit reference / etc

[vbatts]

Can we change this json.RawMessage to interface{}?

[gao-feng]

@vbatts Sure, updated, thanks for your comments.

[vbatts]

LGTM

[wking]

a878558 looks good to me too.

[vishh]

Labels are usually key value pairs elsewhere in docker and kubernetes. They are generally used for grouping. In this PR it looks like we are attempting to store metadata that is opaque to the spec. If that is the case can we store the metadata as a string? The users of the spec can then store anything in the string, including maps serialized to json format.
-1 on overloading the term 'labels' for storing arbitrary metadata.

[vbatts]

@vishh is it the field name of "labels" that feels like a collision to you? I am not attached to that name, but there is frustration with having to unmarshal content just to map in a string typed field.

[wking]

On Fri, Oct 30, 2015 at 08:37:29AM -0700, Vish Kannan wrote:

In this PR it looks like we are attempting to store metadata that is
opaque to the spec.

That's my preference, yeah. Why require a particular structure for
information that has no specified use? This is just a playground for
runtime-specific information, and I see no benefit to restricting its
content.

If that is the case can we store the metadata as a string? The
users of the spec can then store anything in the string, including
maps serialized to json format.

That seems like extra (de)serialization work for runtimes that use
this field. And I don't see a benefit. What do you gain by requiring
a string here?

-1 on overloading the term 'labels' for storing arbitrary metadata.

I don't really care what the key associated with this unstructured
runtime playground is, so +1 to switching to whatever key you propose
instead.

[vishh]

@vbatts: Got it. +1 on not using the keyword labels. How about Annotations.

I'm curious why we are choosing interface which is golang specific instead of map[string]string], which can be represented in other languages as well.

[wking]

On Fri, Oct 30, 2015 at 09:37:45AM -0700, Vish Kannan wrote:

@vbatts: Got it. +1 on not using the keyword labels. How about
Annotations.

Works for me, although I think all our existing JSON keys start with
lowercase.

I'm curious why we are choosing interface which is golang specific
instead of map[string]string], which can be represented in other
languages as well.

Because that doesn't allow deeper nesting if folks want to put more
structure into their field. interface{} is basically the C equivalent
of void*, and that's what I think we want here.

I don't think we have to worry about language support. Many languages
can parse arbitrary JSON, and anyone that's actually going to use
the data in this field will want to get a schema for the part they'll
use. We're leaving it unspecified here, but runtimes that populate
this information will likely provide their own specs for the data they
insert (if they expect third parties to use it).

[crosbymichael]

since it's json can you not just store whatever you want in there without having to add a field like this ?

i.e.

type MyRuntimeState {
    specs.State
    MyFields string `json:"myField"`
}

Your runtime can unmarshal and marshal exactly what you want, everyone else can ignore fields they don't know about.

[wking]

On Mon, Nov 16, 2015 at 01:59:02PM -0800, Michael Crosby wrote:

since it's json can you not just store whatever you want in there
without having to add a field like this ?

This PR allocates an approved namespace for that, otherwise you risk
conflicting with a spec-defined annotations (or whatever) down the
line.

[crosbymichael]

an approved namespace for putting unapproved things, makes sense ...

[vishh]

Is there a recommended format for the values? Is it byte encoded or string or something else?

[gao-feng]

@vishh @crosbymichael @wking Updated, thanks you guys.

[vishh]

What does an interface mean outside of golang?

[wking]

On Mon, Nov 16, 2015 at 07:59:57PM -0800, Vish Kannan wrote:

+* annotations: (interface) holds runtime-specified
information that is not structured by this specification.

What does an interface mean outside of golang?

Data with an unspecified schema. E.g. void* in C 1.

[jonboulle]

Eh, this is weird. We are already saying this is JSON encoded, so we should just say the type here is an arbitrary JSON type (either an object or a value). "interface" is unhelpful.

[vishh]

+1

On Wed, Dec 16, 2015 at 10:53 AM, Jonathan Boulle [email protected]
wrote:

In runtime.md
#188 (comment):

@@ -23,6 +23,9 @@ This allows the hooks to perform cleanup and teardown logic after the runtime de

• bundlePath: (string) is the absolute path to the container's bundle directory.
  This is provided so that consumers can find the container's configuration and root filesystem on the host.

+* annotations: (interface) holds runtime-specified information that is not structured by this specification.

Eh, this is weird. We are already saying this is JSON encoded
https://github.com/opencontainers/specs/pull/188/files#diff-b84a8d65d8ed53f4794cd2db7e8ea731L11,
so we should just say the type here is an arbitrary JSON type (either an
object or a value). "interface" is unhelpful.

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/188/files#r47816296.

[wking]

On Wed, Dec 16, 2015 at 10:53:41AM -0800, Jonathan Boulle wrote:

+* annotations: (interface) holds runtime-specified information that is not structured by this specification.

Eh, this is weird. We are already saying this is JSON
encoded,
so we should just say the type here is an arbitrary JSON type
(either an object or a value). "interface" is unhelpful.

“interface” is a Go-ism. So I'm fine with interface in the Go type
and “JSON” in these Markdown docs.

[wking]

161c98e looks good to me.

[vishh]

Just one nit. Otherwise LGTM

[philips]

If we do this I really really think we need to tell people to namespace their json fields. If the goal of the state.json is to encourage standard operations then having extensions here seems really dangerous without namespacing.

"annotations": {
  "example.com/funruntime": { "O_PONIES": true }
}

[wking]

On Tue, Dec 22, 2015 at 10:53:33PM -0800, Brandon Philips wrote:

If we do this I really really think we need to tell people to
namespace their json fields. If the goal of the state.json is to
encourage standard operations then having extensions here seems
really dangerous without namespacing.

"annotations": {
"example.com/funruntime": { "O_PONIES": true }
}

There is only one author (the runtime) that is allowed to write to
this field, so I don't see any change for collisions on writing. It
would be good to avoid ambiguity on reading though (e.g. two
different runtimes define annotations with overlapping keys but
distinct semantics. This field is (and should be) completely opaque
to the spec, but I'm open to an informative suggestion that runtime
authors insert a field so consumers can have good confidence that the
data they read comes from the runtime they expect. For example:

"annotations": {
"runtime": "funruntime",
"version": "0.1.0",
"O_PONIES": true
}

[philips]

I am not saying don't make it opaque to the spec, but we really should
encourage namespacing one way or the other as the readers will get confused.

On Wed, Dec 23, 2015 at 9:40 AM W. Trevor King [email protected]
wrote:

On Tue, Dec 22, 2015 at 10:53:33PM -0800, Brandon Philips wrote:

If we do this I really really think we need to tell people to
namespace their json fields. If the goal of the state.json is to
encourage standard operations then having extensions here seems
really dangerous without namespacing.

"annotations": {
"example.com/funruntime": { "O_PONIES": true }
}

There is only one author (the runtime) that is allowed to write to
this field, so I don't see any change for collisions on writing. It
would be good to avoid ambiguity on reading though (e.g. two
different runtimes define annotations with overlapping keys but
distinct semantics. This field is (and should be) completely opaque
to the spec, but I'm open to an informative suggestion that runtime
authors insert a field so consumers can have good confidence that the
data they read comes from the runtime they expect. For example:

"annotations": {
"runtime": "funruntime",
"version": "0.1.0",
"O_PONIES": true
}

—
Reply to this email directly or view it on GitHub
#188 (comment).

[vbatts]

@philips I agree on the encouragement. Perhaps that verbiage ought to start for annotations in the bundled config.json, since that will be from bundle authors (not just the runtime)

[wking]

On Mon, Jan 04, 2016 at 10:38:07AM -0800, Vincent Batts wrote:

Perhaps that verbiage ought to start for annotations in the bundled
config.json, since that will be from bundle authors (not just the
runtime)

I think this PR is just about the state JSON and runtime annotations.
Config annotations by bundle authors are [1](spun off from #108).

For this PR, I'd suggest phrasing like 2:

Runtimes are encouraged to mark and version any annotations so users
can consume them safely. For example:

"annotations": {"runtime": "fun-runtime", "version": "0.1.0", "O_PONIES": true}

Which will allow consumers to determine with high confidence that
those annotations should be interpreted according to v0.1.0 of a
specification published by the fun-runtime authors.

 Subject: Labels and extension meta data in containers #108
 Date: Tue, 11 Aug 2015 14:14:04 -0700
 Message-ID: <CAEMTkLbqUhyY97iPZ3_4axJ3yUwX9oOVOnQQr_Ks_aRr8R-1mg@mail.gmail.com>

[vishh]

Closing this in favor of #331. Kindly re-open if you find #331 to not satisfy all the requirements @gao-feng...

[wking]

On Wed, Mar 09, 2016 at 11:18:12AM -0800, Vish Kannan wrote:

Closing this in favor of #331.

I think we should re-open this PR or land something similar. #331
ended up dropping it's state JSON additions 1, and we'll want a
place for the runtime to store unspecified information. For example,
runV wants to store a socket path 2, and you'd also need a socket
path for proposal 3 in the create/start split discussion 3.

 Subject: Re: Splitting Start into create() and run()
 Date: Thu, 24 Mar 2016 15:04:14 -0700
 Message-ID: <[email protected]>