Add annotations and labels to the Spec. by vishh · Pull Request #331 · opencontainers/runtime-spec

vishh · 2016-03-04T19:14:36Z

Fixes #108

mrunalp · 2016-03-04T20:00:41Z

Do we need Labels and Annotations in both spec and state? I thought Labels were more of a runtime grouping mechanism so belong to the state and Annotations static and hence should be in the spec config.

vbatts · 2016-03-04T20:51:12Z

@mrunalp the labels are useful in both places. And as far as annotations, there may be runtimes that need to stash runtime specific metadata, and it would provide a clean place to do so.

wking · 2016-03-04T21:34:47Z

It looks like the runtime will be ignoring ‘annotations’ 1. Why not
follow the approach #188 proposes and use a single, opaque field
(‘interface{}’ in Go and ‘JSON’ or ‘object’ in the Markdown 2)?
Then the higher-level tools that will be consuming this information
can specify their own schema, Go types, etc. I don't see a benefit to
requiring a particular schema for data that the runtime will ignore,
and requring a map[string]string seems like it conflicts with “This
information may be … structured” 1.

I'm -1 on labels (which do impact the runtime interface) until we have
clarity on that interface. For example, the ‘list’ command used as an
example 3 is not one of our current operations 4. If we want to
add label-based identifiers for stop and exec, I'd rather have links
from those actions to the labels documentation. And “This operation
MUST generate an error if it is not provided the container ID” 5
would probably need to be softened.

vishh · 2016-03-04T21:54:48Z

How much do we care about the state from a Spec perspective?

On Fri, Mar 4, 2016 at 1:34 PM, W. Trevor King notifications@github.com
wrote:

It looks like the runtime will be ignoring ‘annotations’ 1. Why not
follow the approach #188 proposes and use a single, opaque field
(‘interface{}’ in Go and ‘JSON’ or ‘object’ in the Markdown 2)?
Then the higher-level tools that will be consuming this information
can specify their own schema, Go types, etc. I don't see a benefit to
requiring a particular schema for data that the runtime will ignore,
and requring a map[string]string seems like it conflicts with “This
information may be … structured” 1.

I'm -1 on labels (which do impact the runtime interface) until we have
clarity on that interface. For example, the ‘list’ command used as an
example 3 is not one of our current operations 4. If we want to
add label-based identifiers for stop and exec, I'd rather have links
from those actions to the labels documentation. And “This operation
MUST generate an error if it is not provided the container ID” 5
would probably need to be softened.

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

wking · 2016-03-04T22:02:33Z

On Fri, Mar 04, 2016 at 01:54:50PM -0800, Vish Kannan wrote:

How much do we care about the state from a Spec perspective?

I care because it is the key that allows container access without
going through an established runtime operation 1. If you need to
adjust something that doesn't have an OCI knob, attach some monitoring
that doesn't have an OCI integration, etc., the state gives you the
platform primitives you need to interact with the container directly.

vishh · 2016-03-05T00:25:58Z

@philips @crosbymichael @mrunalp @vbatts: How do we want to move forward on this PR?

mrunalp · 2016-03-07T18:53:18Z

@vishh Could you give use cases for each as we had discussed in the call? I think that will help move this.

crosbymichael · 2016-03-08T18:42:36Z

I'm still unclear why we need to. its like 2x the complexity and ppl could be confused when and why to use each one.

wking · 2016-03-08T18:55:32Z

On Tue, Mar 08, 2016 at 10:42:38AM -0800, Michael Crosby wrote:

I'm still unclear why we need to. its like 2x the complexity and
ppl could be confused when and why to use each one.

One is for opaque-to-the-runtime data, and the other is for grouping
(as defined in this PR). I think that's a clear enough distinction.
I'd prefer folks used other files for opaque-to-the-runtime data, but
I'm ok with a blessed field in config.json for opaque-to-the-runtime
data 1.

 Subject: Re: Labels and extension meta data in containers #108
 Date: Wed, 30 Dec 2015 20:54:37 -0800
 Message-ID: <20151231045437.GA6362@odin.tremily.us>

Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh · 2016-03-09T19:12:39Z

Updates PR to not include Labels. Removed Annotations from state.go

crosbymichael · 2016-03-09T19:14:20Z

LGTM

mrunalp · 2016-03-09T19:15:45Z

LGTM

Add annotations and labels to the Spec.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>

wking · 2016-03-09T19:30:34Z

config.go

 	// Hooks are the commands run at various lifecycle events of the container.
 	Hooks Hooks `json:"hooks"`
+	// Annotations is an unstructured key value map that may be set by external tools to store and retrieve arbitrary metadata.
+	Annotations map[string]string `json:"annotations,omitempty"`


Are we sure we don't want an opaque type here? Or will we just revisit that later when somebody needs something that doesn't fit into a map[string]string?

The spec now has opaque 'annotations', although: * It's a map[string]string and I would have prefered an opaque-in-the-runtime-spec type for an opaque-to-the-runtime field [1]. * It doesn't address arbitrary bundle content [2], but I have a separate tag for that [3]. [1]: opencontainers/runtime-spec#331 (comment) [2]: Message-ID: <20151231045437.GA6362@odin.tremily.us> Subject: Re: Labels and extension meta data in containers #108 Date: Wed, 30 Dec 2015 20:54:37 -0800 [3]: Message-ID: <20150826195447.GX21585@odin.tremily.us> Subject: Dropping the rootfs requirement and restoring arbitrary bundle content Date: Wed, 26 Aug 2015 12:54:47 -0700

Change made with: $ sed -i 's/\t/ /g' config.md fixing tabs that were added with 1c49f4d (Add annotations and labels to the Spec, 2016-03-04, opencontainers#331). Signed-off-by: W. Trevor King <wking@tremily.us>

The spec was not very clear on how state annotations are related to [config annotations. In the pull-request that landed state annotations, it sounds like these were supposed to be copied opaquely from the config [1]. It's still not clear to me why we'd copy annotations but not the rest of the config [2], but I'm leaving that alone for now. There was previous interest in runtime-specified annotations [3,4] (e.g. a RunV socket path [5]), but this commit does not allow runtimes to inject additional entries because I don't like: * Relying on config authors to avoid squatting on the namespace used by the runtime (if ties are broken in favor of the config) or * Silently clobbering configured annotations (if ties are broken in favor of the runtime). My preference would be to follow [3] and: * Only include runtime-specified information in the state annotations. * Require state readers to follow 'bundle' to the config.json if they wanted configured annotations (or embed the whole config.json in the state). But with 1.0 released and spec-maintainer comments like [1], I think it's too late to return to that approach. If we want to expose runtime-specified annotations, I think we'll need a new state property. There has been previous discussion of using "labels" and "annotations" to carry both types of information in the state [6], and while it's not as elegant as a full config copy, the labels/annotations approach is still viable. [1]: opencontainers#484 (comment) [2]: opencontainers#484 (comment) [3]: opencontainers#188 [4]: opencontainers#331 (comment) [5]: opencontainers#188 (comment) [6]: opencontainers#331 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

The spec was not very clear on how state annotations are related to config annotations. In the pull-request that landed state annotations, it sounds like these were supposed to be copied opaquely from the config [1]. It's still not clear to me why we'd copy annotations but not the rest of the config [2], but I'm leaving that alone for now. There was previous interest in runtime-specified annotations [3,4] (e.g. a RunV socket path [5]), but this commit does not allow runtimes to inject additional entries because I don't like: * Relying on config authors to avoid squatting on the namespace used by the runtime (if ties are broken in favor of the config) or * Silently clobbering configured annotations (if ties are broken in favor of the runtime). My preference would be to follow [3] and: * Only include runtime-specified information in the state annotations. * Require state readers to follow 'bundle' to the config.json if they wanted configured annotations (or embed the whole config.json in the state). But with 1.0 released and spec-maintainer comments like [1], I think it's too late to return to that approach. If we want to expose runtime-specified annotations, I think we'll need a new state property. There has been previous discussion of using "labels" and "annotations" to carry both types of information in the state [6], and while it's not as elegant as a full config copy, the labels/annotations approach is still viable. [1]: opencontainers#484 (comment) [2]: opencontainers#484 (comment) [3]: opencontainers#188 [4]: opencontainers#331 (comment) [5]: opencontainers#188 (comment) [6]: opencontainers#331 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

vishh added this to the v0.4.0 milestone Mar 4, 2016

Add annotations and labels to the Spec.

1c49f4d

Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh force-pushed the labels-annotations branch from ce688a7 to 1c49f4d Compare March 9, 2016 19:11

mrunalp pushed a commit that referenced this pull request Mar 9, 2016

Merge pull request #331 from vishh/labels-annotations

dae09c6

Add annotations and labels to the Spec.

mrunalp merged commit dae09c6 into opencontainers:master Mar 9, 2016

vishh mentioned this pull request Mar 9, 2016

Add labels to the state #188

Closed

vbatts added a commit to vbatts/oci-runtime-spec that referenced this pull request Mar 9, 2016

schema: add annotations field from opencontainers#331

084ff67

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>

wking reviewed Mar 9, 2016
View reviewed changes

wking mentioned this pull request Mar 9, 2016

Bump v0.4.0 #336

Merged

wking mentioned this pull request Mar 24, 2016

Is PORTS field neccessary for oci specs? #178

Closed

wking mentioned this pull request Apr 13, 2016

config.md: Rename ociVersion -> ociRuntimeVersion #371

Closed

wking mentioned this pull request Apr 27, 2016

manifest: add annotations opencontainers/image-spec#44

Merged

wking mentioned this pull request May 6, 2016

config: Use spaces (not tabs) to indent JSON #426

Merged

This was referenced Jun 2, 2016

Add annotations to the state json #484

Merged

Guidelines for config.json annotations vs. an external file? #492

Open

wking mentioned this pull request Jul 7, 2016

Add text about extensions #510

Merged

wking mentioned this pull request Sep 15, 2017

Question about OCI Index Template xiekeyang/oci-discovery#15

Closed

wking mentioned this pull request Jan 11, 2018

runtime: Document state annotations as a copy of config annotations #946

Open

Conversation

vishh commented Mar 4, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

vbatts commented Mar 4, 2016

Uh oh!

wking commented Mar 4, 2016

Uh oh!

vishh commented Mar 4, 2016

Uh oh!

wking commented Mar 4, 2016

Uh oh!

vishh commented Mar 5, 2016

Uh oh!

mrunalp commented Mar 7, 2016

Uh oh!

crosbymichael commented Mar 8, 2016

Uh oh!

wking commented Mar 8, 2016

Uh oh!

vishh commented Mar 9, 2016

Uh oh!

crosbymichael commented Mar 9, 2016

Uh oh!

mrunalp commented Mar 9, 2016

Uh oh!

wking Mar 9, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants