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

Config.Volume specification too restrictive. #687

Closed
stevvooe opened this issue May 25, 2017 · 16 comments
Closed

Config.Volume specification too restrictive. #687

stevvooe opened this issue May 25, 2017 · 16 comments

Comments

@stevvooe
Copy link
Contributor

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
Copy link
Contributor Author

cc @cyphar @cpuguy83

@cyphar
Copy link
Member

cyphar commented May 26, 2017

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

@runcom
Copy link
Member

runcom commented May 26, 2017

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

@cyphar
Copy link
Member

cyphar commented May 26, 2017

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
Copy link
Contributor

cpuguy83 commented May 26, 2017 via email

@cyphar
Copy link
Member

cyphar commented May 30, 2017

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
Copy link
Contributor Author

@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
Copy link
Contributor

erikh commented May 31, 2017 via email

@cyphar
Copy link
Member

cyphar commented May 31, 2017

@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
Copy link
Contributor Author

CPU limits and memory limits

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

@stevvooe
Copy link
Contributor Author

@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
Copy link
Member

cyphar commented May 31, 2017

@stevvooe

Ah sorry, I completely forgot we dropped them. 😛

@stevvooe
Copy link
Contributor Author

@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
Copy link
Contributor

erikh commented Jun 3, 2017 via email

@erikh
Copy link
Contributor

erikh commented Jun 3, 2017 via email

@stevvooe
Copy link
Contributor Author

stevvooe commented Jun 5, 2017

@erikh This is probably a non-starter. While we all have issues with the specification of Config.Volumes, removing it will just kick the can down the road.

What is so wrong with just weakening the language? Implementations can try to reproduce docker's behavior, or not. There is no need to reserve for a specific implementation.

The fact is, it identifies a good place where data may be written by the application that is specific to a particular instance, rather than something you want to snapshot.

  • 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.

Yes, and this is the common confusion that makes this problem 10x as hard. This isn't specifying "data volumes", the object, at all. Confounding these two things is what makes this confusing. They are not the same thing. The config.volumes field specifies places where one may want to mount a volume.

In general, it is going to make this conversation hard to have if most of your complaints are about the data object known as "volumes" when this specification is just calling out an area where instance-specific data may be written.

  • For those of us writing builders, what exactly is a volume in the context
    of building an image? It's not really that clear.

Again, this is not at all in the scope of this specification. This field is not defining the data object volumes.

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

5 participants