runtime: Document state annotations as a copy of config annotations #946
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
wking
force-pushed
the
state-annotations-source
branch
from
caec2e2 to
76217dc
Compare
|
Travis failure is #945. |
|
As an aside, regarding this:
One of the plans I have for |
|
Closing/reopening to bump Travis with #945 landed. |
|
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. |
|
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)?
|
|
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
<#946 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEF6SZ6Zp_W0kXozDyrWkpYlDMgVt5nks5tlUVlgaJpZM4RbW_K>.
|
|
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.
[1]: opencontainers/runc@cd1e7ab#diff-7b8effb45402944e445a664e4d9c296dR311
|
|
👎 |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
My preference would be to follow #118 and:
annotations.bundleto theconfig.jsonif they wanted configured annotations (or embed the wholeconfig.jsonin 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
labelsandannotationsto 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'sLooks like runc is removing their injectedLabelsmixes configured and runtime-specified annotations or not, so I'm not sure if it would be compatible with this change.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
annotationswas unspecified, so state consumers shouldn't have been relying on a particular semantic behavior.