runtime: Document state annotations as a copy of config annotations

[wking]

The spec was not very clear on how state annotations are related to config annotations. In the pull-request that landed state annotations (#484), it sounds like these were supposed to be copied opaquely from the config. It's still not clear to me why we'd copy annotations but not the rest of the config, but I'm leaving that alone for now.

There was previous interest in runtime-specified annotations (e.g. a RunV socket path), 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 #118 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 this, 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, and while it's not as elegant as a full config copy, the labels/annotations approach is still viable.

Related to opencontainers/runc#1687. ~~I'm not clear on whether libcontainer's Labels mixes configured and runtime-specified annotations or not, so I'm not sure if it would be compatible with this change.~~ Looks like runc is removing their injected bundle, so opencontainers/runc#1687 would be compatible with this PR as it stands.

I don't think this is a breaking change, because the previous content of annotations was unspecified, so state consumers shouldn't have been relying on a particular semantic behavior.

[wking]

Travis failure is #945.

[cyphar]

As an aside, regarding this:

Require state readers to follow bundle to the config.json if they wanted configured annotations (or embed the whole config.json in the state).

One of the plans I have for runc is to change our on-disk state storage so that it only stores the PID1 and a copy of the config, both for our own sanity but also to have a single source of truth for the state of the container. This would make this sort of requirement easier to deal with if we want to make it a requirement.

[wking]

Closing/reopening to bump Travis with #945 landed.

[vbatts]

it was intentionally vague as that annotations field for runtime state was literally a holding bin so that implementations would just dump arbitrary values across the state. I'm not inclined on this PR

[wking]

On Wed, Apr 04, 2018 at 10:03:03PM +0000, Vincent Batts wrote:

it was intentionally vague as that annotations field for runtime state was literally a holding bin so that implementations would just dump arbitrary values across the state. I'm not inclined on this PR

So are there any compliance conditions on state annotations, or are those completely free-form? It sounds like you'd consider runc compliant with or without opencontainers/runc#1687, but if there are no constraints at all it makes state annotations pretty useless from a portable-hook standpoint. Or do you have some constraints on the values (e.g. they must include at least the keys from the config annotations, although runtimes are free to clobber the values and add additional keys)?

[vbatts]

the state annotations were different from the bundle annotations.

On 4/4/18 6:12 PM, W. Trevor King wrote:

On Wed, Apr 04, 2018 at 10:03:03PM +0000, Vincent Batts wrote:

it was intentionally vague as that annotations field for runtime state was literally a holding bin so that implementations would just dump arbitrary values across the state. I'm not inclined on this PR

So are there any compliance conditions on state annotations, or are those completely free-form? It sounds like you'd consider runc compliant with or without opencontainers/runc#1687, but if there are no constraints at all it makes state annotations pretty useless from a portable-hook standpoint. Or do you have some constraints on the values (e.g. they must include at least the keys from the config annotations, although runtimes are free to clobber the values and add additional keys)?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/opencontainers/runtime-spec/pull/946#issuecomment-378761726, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEF6SZ6Zp_W0kXozDyrWkpYlDMgVt5nks5tlUVlgaJpZM4RbW_K.

[wking]

On Wed, Apr 04, 2018 at 03:15:14PM -0700, Vincent Batts wrote:

the state annotations were different from the bundle annotations.

Is that just a historical note? Runc has them identical since 1, so I don't see a current issue. We can land this PR without impacting runc compliance. Not landing this PR, on the other hand, leaves hook authors and other state consumers with very little information about the intended semantics and values of state annotations.

[vbatts]

:-1: I'm okay with this being vague and not imposing a workflow here