Add annotations to the state json #484
Conversation
Signed-off-by: Doug Davis <[email protected]>
|
On Thu, Jun 02, 2016 at 01:25:37PM -0700, Doug Davis wrote:
For previous work in this direction, see #188 (which was about adding When I suggested including config-author-specified content in the |
|
LGTM |
|
While this looks fine, we need to be clear about what we copy from the configuration to the state as it is essentially redundant. |
|
@mrunalp yes, Since annotations are more user defined "labels" for a container not part of the configuration that create the container and its settings it makes since for something like this to be added to the state allow it to be a queried |
|
@crosbymichael They could also potentially be used to implement extensions that aren't yet in the spec. |
|
@mrunalp yep, its something for external tooling, the runtime will just passthough and take no actions on the values. |
|
I am fine merging for now and we could maybe revisit if we see a need to move other values into the state. Thanks! |
|
LGTM |
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 <[email protected]>
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 <[email protected]>
Closes: #480
Signed-off-by: Doug Davis [email protected]