Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wking opened this issue Jun 8, 2016 · 10 comments
Open

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

wking opened this issue Jun 8, 2016 · 10 comments
Milestone

Comments

@wking
Copy link
Contributor

wking commented Jun 8, 2016

In opencontainers/image-spec#87, @philips is wondering whether the runtime-spec maintainers think network configuration and other metadata should go in config.json's annotations or in an external file. That sounds like something that deserves a SHOULD suggestion in this spec, so folks who don't already have an opinion can follow whatever the best practice is deemed to be without having to think to hard.

Besides the labels/annotation work in this repository with #188, #331, and #480, there was also some IRC discussion recorded here, with me arguing for external files and @vbatts arguing for embedding in config.json. More recently, @julz floated an external hooks.json (and @mrunalp came right back and suggested annotations). So possible OCI positions:

We really don't care

Language (in the annotations docs?) like:

Opaque metadata MAY also be stored in external files, and this specification makes no recommendation on whether to use external files or annotations.

Recommend external files for everything

Language like:

Information that does not target the OCI runtime implementation SHOULD be stored in separate files alongside the runtime's config.json.

This is my current preference, since it means the runtime is obviously not responsible for anything related to the extra data. But it means that having annotations in the config.json spec at all will be a bit confusing (I'd prefer removing it).

Recommend annotations for everything

Language like:

Information that does not target the OCI runtime implementation SHOULD be stored in annotations. Information that is structured should be flattened to a string, since the annotations map requires string values.

If we lift the string requirement and allow opaque data in annotations, we could drop the second sentence.

Recommend annotations for string data, and external files for structured data

Language like:

Information that does not target the OCI runtime implementation SHOULD be stored in annotation if it can be represented as a string without difficulty. Information that is more structured (e.g. JSON objects) SHOULD be stored in external files alongside the runtime's config.json.

Other ideas?

Are there breakdowns I'm missing? It's hard to say much about opaque data. And while @philips clearly has expectations about [downstream consumption of the image-spec annotations][6], I'm not sure how generic those expectations are (and maybe he doesn't think they apply to the runtime-spec's annotations anyway).

@JakeWarner
Copy link

JakeWarner commented Jun 8, 2016

I highly favor the 'Recommend external files for everything' approach.

To take it a step further, I'm still in favor of splitting some of the host-specific stuff out of the config.json file. I know at one point the OCI bundle contained a runtime.json and then it was decided against. I wasn't as involved within the community back at the time so I'm not fully sure what the criteria for what went into runtime.json vs config.json. That being said, having an immutable drop-in config that was specific to the container and didn't require any customization regardless of the host is very appealing as an implementor of the spec. It'd be great to be able to pull in an OCI-bundle, drop in my own custom "host-config" along side the container config and just immediately start the container. The fact that I currently have to unmarshal, modify, re-marshal and save the configurations for users importing containers to our platform just feels meh.

I fear that by adding more stuff to the config.json, we're going to end up with more confusion of "where does the spec start/end?"

To me, the ideal OCI-bundle structure would be something along the lines of:
/bundle/rootfs/
/bundle/config.json
/bundle/host-config.json
/bundle/plugins/NAME.json <-- not sure if 'plugins' is the best word here

'Plugins' would contain optional json configurations created by different tools/platforms that implemented the OCI-spec which could be distributed within the bundle itself.

(sorry, got a little off topic-ish)

@cyphar
Copy link
Member

cyphar commented Jun 8, 2016

@JakeWarner As another aside, I don't like JSON as a config format. Personally I think TOML would've been a better choice -- JSON has way too many quirks to make it a good choice of config format.

@wking
Copy link
Contributor Author

wking commented Jun 8, 2016

On Tue, Jun 07, 2016 at 11:16:00PM -0700, Jake Warner wrote:

I wasn't as involved within the community back at the time so I'm
not fully sure what the criteria for what went into runtime.json vs
config.json.

#284 has the big picture, and the list thread 1 has detailed
rebuttals of all the reasons put forward in favor of splitting the
config file.

/bundle/plugins/.json <-- not sure if 'plugins' is the best word here

'Plugins' would contain optional json configurations created by
different tools/platforms that implemented the OCI-spec.

The use-case that spawned this issue was “where do we put information
about exposed ports?” (opencontainers/image-spec#87). That's
targetting network managers, which are currently out-of-scope for the
OCI. So I'd caution against phrasing the full recommendation in terms
of OCI-spec tools; the extra configuration can be for anything,
OCI-related or not. Of course, folks may want to use OCI-related-ness
in the recommendation (e.g. suggesting OCI-related stuff go in
annotations, while OCI-independent stuff go in external files).

 Subject: Single, unified config file (i.e. rolling back specs#88)
 Date: Wed, 4 Nov 2015 09:53:20 -0800
 Message-ID: <20151104175320.GC24652@odin.tremily.us>

@wking
Copy link
Contributor Author

wking commented Jun 8, 2016

On Wed, Jun 08, 2016 at 12:38:45AM -0700, Aleksa Sarai wrote:

As another aside, I don't like JSON as a config format. Personally I
think TOML would've been a better choice…

Orthogonal? Can you open a new issue?

@JakeWarner
Copy link

JakeWarner commented Jun 8, 2016

#284 has the big picture, and the list thread [1] has detailed
rebuttals of all the reasons put forward in favor of splitting the
config file.

Will review.

The use-case that spawned this issue was “where do we put information
about exposed ports?” (opencontainers/image-spec#87). That's
targetting network managers, which are currently out-of-scope for the
OCI. So I'd caution against phrasing the full recommendation in terms
of OCI-spec tools; the extra configuration can be for anything,
OCI-related or not. Of course, folks may want to use OCI-related-ness
in the recommendation (e.g. suggesting OCI-related stuff go in
annotations, while OCI-independent stuff go in external files).

I'm all for ExposedPorts going into the config.json since almost everything requires it (probably should be part of the spec) but since networking can get so complex and it's not defined by the spec, why include it in the config.json, even as an annotation?

@wking
Copy link
Contributor Author

wking commented Jun 8, 2016

On Wed, Jun 08, 2016 at 10:41:16AM -0700, Jake Warner wrote:

I'm all for ExposedPorts going into the config.json since almost
everything requires it but since networking can get so complex and
it's not defined by the spec, why include it in the config.json,
even as an annotation?

After the discussion in today's meeting, I'm not sure there's a
consensus on the broader issue that I was targetting here. For
networking in particular, my impression of consensus was:

  • ExposedPorts is pretty simple, and the image-spec structure 1 can
    get packed down to a reasonable string 2, so this should get put
    in ‘annotations’ 3.
  • The rest of networking is complicated, and will likely be handled by
    the orchestration layer (reading from whatever it's particular
    image-format is) without writing any configuration to the bundle
    directory 4.

For the broader issue, @brendandburns also wants to allow structured
data in annotations 5, although that remains contentions. And I
didn't see anyone speaking up for separate files (I've already
explained my position on that in this issue).

@vbatts
Copy link
Member

vbatts commented Jun 9, 2016

As a nit, I did not recommend embedding the document, but rather
brainstorming possibilities.

On Wed, Jun 8, 2016, 13:32 W. Trevor King notifications@github.com wrote:

On Wed, Jun 08, 2016 at 10:41:16AM -0700, Jake Warner wrote:

I'm all for ExposedPorts going into the config.json since almost
everything requires it but since networking can get so complex and
it's not defined by the spec, why include it in the config.json,
even as an annotation?

After the discussion in today's meeting, I'm not sure there's a
consensus on the broader issue that I was targetting here. For
networking in particular, my impression of consensus was:

  • ExposedPorts is pretty simple, and the image-spec structure 1 can
    get packed down to a reasonable string 2, so this should get put
    in ‘annotations’ 3.
  • The rest of networking is complicated, and will likely be handled by
    the orchestration layer (reading from whatever it's particular
    image-format is) without writing any configuration to the bundle
    directory 4.

For the broader issue, @brendandburns also wants to allow structured
data in annotations 5, although that remains contentions. And I
didn't see anyone speaking up for separate files (I've already
explained my position on that in this issue).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#492 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAEF6ayW2mq1oXZEpF_rDEmcUcNEy-lOks5qJwqigaJpZM4IwlII
.

@philips
Copy link
Contributor

philips commented Jun 10, 2016

I am personally +1 for an annotation like:

{
    "ociVersion": "0.2.0",
...
    "annotations": {
        "opencontainers.org/image-spec/exposedPorts": "8080,53/udp"
    }
}

I think in general we should avoid forcing someone to parse and deal with additional files between OCI specs. And this is first case where the OCI Image Spec has data that it doesn't know how to give to the OCI Runtime Spec.

Original comment here: opencontainers/image-spec#87 (comment)

@wking
Copy link
Contributor Author

wking commented Jun 10, 2016

On Thu, Jun 09, 2016 at 07:39:20PM -0700, Brandon Philips wrote:

I think in general we should avoid forcing someone to parse and deal
with additional files between OCI specs. And this is first case
where the OCI Image Spec has data that it doesn't know how to give
to the OCI Runtime Spec.

So are you comfortable requiring external lookup tables (e.g. bundle
path → source image name / CAS ID) to efficiently build new images
1? If you don't copy that information (or embed the image
config/manifest) into the bundle somewhere, it's going to be hard to
build an efficient new image using just the bundle tree.

@wking
Copy link
Contributor Author

wking commented Apr 4, 2017

I wrote

So are you comfortable requiring external lookup tables…

To ground that in something concrete, umoci uses a parallel umoci.json which stores a reference to the original manifest. Then on repacking, umoci loads the original manifest and appends to the history, etc., preserving all the other complicated values that are too tedious to serialize into annotations. So combining umoci's parallel-file approach and opencontainers/image-spec#492's recommendation to populate a org.opencontainers.imageSpec.exposedPorts annotation, etc., I think the consensus position here seems to be either “We really don't care” or “Recommend annotations for string data, and external files for structured data” from my original set of possible positions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants