Config.Volume specification too restrictive.

[stevvooe]

Currently, the specification for the Config.Volumes declares the following:

If a file or folder exists within the image with the same path as a data volume, that file or folder will be replaced by the data volume and never be merged.

There are many possible behaviors for interacting with volumes that may make sense in practice. One may want to replace the volume data, seed it from the image or merge it from the image and the specification should not prevent this.

Please see the conversation at https://github.com/opencontainers/image-spec/pull/492/files/418573051225cd705be4414a249acd1fbf76db25#r117839121 for further details.

[stevvooe]

cc @cyphar @cpuguy83

[cyphar]

Also #504, which was trying to fix the same wording but for different reasons.

[runcom]

I'd add volumes can be actually overlays merge from host:container

[cyphar]

I'd still want to recommend that anything operating on an expanded rootfs not snapshot volumes. While I appreciate that we don't want to imply that people are going to be adding files by expanding the rootfs and doing diffs, we need to make it clear that some people rely on that property for security reasons and it would be a very bad idea to omit this from the spec.

[cpuguy83]

It's hard to codify because it's used for a lot of things, all of which are really just to signal to the runtime that the path is special and to do "something" different with it.

…

On Fri, May 26, 2017 at 3:12 PM, Aleksa Sarai ***@***.***> wrote: I'd still want to recommend that anything operating on an expanded rootfs *not* snapshot volumes. While I appreciate that we don't want to imply that people are going to be adding files by expanding the rootfs and doing diffs, we need to make it clear that some people rely on that property for security reasons and it would be a very bad idea to omit this from the spec. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#687 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwxZrLjtz2h-OwMTE_7Uz0xQNtF72Hmks5r9yQzgaJpZM4Nm5bM> .

-- - Brian Goff

[cyphar]

My main concern with diluting (or leaving vague) the meaning of Config.Volumes is that it no longer becomes clear what an implementation should do when they have Config.Volumes. Should the implementation mask any files that exist in the image already in that path? Maybe. Should the implementation include changes in future layers? Maybe. Should it just ignore Config.Volumes? Maybe.

In particular, there are several places where "data volume" is used but the term is not well-defined. We need to:

• Define "data-volume".
• Give at least some suggestions on what the intention of Config.Volumes is.

Currently there are quite a few other places where we have first-class fields but there's no description of what the intention of a particular field is meant to be. From the perspective of an implementer, all we really want to know is what is the intention behind a particular knob so that the tools we write actually convey that intention.

[stevvooe]

@cyphar One area that may be hanging you up is the fact that the field Config.Volumes does not enumerate "volumes", but really "the place where a volume may be mounted". That behavior is implementation dependent. There is no "right" way to specify it.

[erikh]

I still feel volumes are a largely unnecessary part of the spec and make builder writing about 1000x more complicated with them included. Personally I would like it if they did not exist at all. They are unnecessary for the purposes of building and given the way docker typically uses volumes largely unnecessary at image run time too. Given the confusion around these issues I'd argue it's the most prudent course to a release. Hopefully my unsolicited input is not bothersome.

…

On Tue, May 30, 2017 at 2:22 PM Stephen Day ***@***.***> wrote: @cyphar <https://github.com/cyphar> One area that may be hanging you up is the fact that the field Config.Volumes does not enumerate "volumes", but really "the place where a volume may be mounted". That behavior is implementation dependent. There is no "right" way to specify it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#687 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6_GpO9kbYohA3PjMuMgMZy1eLeXVks5r_IiwgaJpZM4Nm5bM> .

[cyphar]

@erikh They exist in the spec because they exist in the Docker image-spec (for compatibility reasons). I'm hoping in the future (post-1.0) we will be dropping most of the fields in Config that don't make sense (things like the CPU limits and memory limits).

[stevvooe]

CPU limits and memory limits

Are these not dropped? I don't think these are honored any more. Did we not drop them?

[stevvooe]

@erikh Your input is always welcome!

make builder writing about 1000x more complicated with them included.

How does this make it more complicated? Is it because we now need to include filtration logic in addition to file system diff logic?

[cyphar]

@stevvooe

Ah sorry, I completely forgot we dropped them. 😛

[stevvooe]

@cyphar No worries.

If there are any more that you want dropped, say so now. Last time I went through, it was looking like it is fairly sparse. I think the only glaring thing, that I'd prefer we left in, is that config has a config.

[erikh]

Hey, sorry for the late replies. the 1000x problem (which is clearly seen as hyperbole I hope) is referring to the logic of carrying a "VOLUME" parameter past multiple invocations of docker in a way that doesn't require you to rewrite the image at each step. In the case where we actually set the volume config parameter, we can keep it for each layer. However, at every "RUN" step in the chain a container will be spawned, which docker will use the config to create a volume that will immediately be orphaned, requiring us to clean up and mitigate any state transitional issues. Additionally, the files are lost if we don't feed the volume name back to it somehow, something I never bothered to figure out how to do, and then there are other layer merging issues with the volume's contents which present themselves. If we don't and set it at the end, we're doing better but then only one of the several relevant layers after the VOLUME appears. I realize this is a limitation of using docker as a client (externally to `docker build`) to build images, but alas, it is one I have to concern myself with right now. It is a colossal pain in the pajamas. For the rest of my anti-volume argument, I will reply to the main thread. :)

…

On Wed, May 31, 2017 at 10:54 AM, Stephen Day ***@***.***> wrote: @erikh <https://github.com/erikh> Your input is always welcome! make builder writing about 1000x more complicated with them included. How does this make it more complicated? Is it because we now need to include filtration logic in addition to file system diff logic? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#687 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ65a0HtycRwoNXsDjGu3gA-JbgVfzks5r_alhgaJpZM4Nm5bM> .

[erikh]

I have issues with Config.Volume existing, for the following reasons: * The term "Data Volume" is undefined in the specification, allowing it to be anything and have any logic whatsoever. * The runtime spec makes no obvious attempt to define what this is either * Volumes traditionally create (sometimes large) artifacts that need to be managed, making them hard to deal with when they are automatically provisioned; for many containers and redistributable images, this can be problematic. Defining this at the image level practically guarantees artifacts being generated by side-effect over and over again for no point other than it was in someone's image config. * For those of us writing builders, what exactly is a volume in the context of building an image? It's not really that clear. Anyways, I hope these arguments have merit and are worth discussing, note that I have no objections to bind mounts/volumes being specified at runtime, but it's important to fight this one for me I guess. It causes a lot of unnecessary UX problems. If I may propose an alternative, it would be to change Config.Volume's wording to something like this for the v1.0 release: "Reserved for for compatibility with Docker's image volume capabilities. <link to docker docs> for more information. MAY be used by OCI-compliant container engines. May be standardized in a future release." Then point them at annotations as a way to talk to specific engines with specific logic if necessary. Finally, aim at this when there are more runtimes and image builders with strong opinions on this. No offense to the docker crew implied, I just think that this is a special, complicated scenario that could easily be removed for simplicity's sake.

…

On Sat, Jun 3, 2017 at 12:07 PM, Erik Hollensbe ***@***.***> wrote: Hey, sorry for the late replies. the 1000x problem (which is clearly seen as hyperbole I hope) is referring to the logic of carrying a "VOLUME" parameter past multiple invocations of docker in a way that doesn't require you to rewrite the image at each step. In the case where we actually set the volume config parameter, we can keep it for each layer. However, at every "RUN" step in the chain a container will be spawned, which docker will use the config to create a volume that will immediately be orphaned, requiring us to clean up and mitigate any state transitional issues. Additionally, the files are lost if we don't feed the volume name back to it somehow, something I never bothered to figure out how to do, and then there are other layer merging issues with the volume's contents which present themselves. If we don't and set it at the end, we're doing better but then only one of the several relevant layers after the VOLUME appears. I realize this is a limitation of using docker as a client (externally to `docker build`) to build images, but alas, it is one I have to concern myself with right now. It is a colossal pain in the pajamas. For the rest of my anti-volume argument, I will reply to the main thread. :) On Wed, May 31, 2017 at 10:54 AM, Stephen Day ***@***.***> wrote: > @erikh <https://github.com/erikh> Your input is always welcome! > > make builder writing about 1000x more complicated with them included. > > How does this make it more complicated? Is it because we now need to > include filtration logic in addition to file system diff logic? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#687 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABJ65a0HtycRwoNXsDjGu3gA-JbgVfzks5r_alhgaJpZM4Nm5bM> > . >