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

Add tmpfs as a valid volume source command. #13587

Merged
merged 1 commit into from Dec 2, 2015

Conversation

Projects
None yet
@rhatdan
Contributor

rhatdan commented May 29, 2015

This patch set will allow users to setup tmpfs mounts on top of
any directory. One of the big benefits of this would be
to allow --read-only to work when an application needs to write to /run or /tmp.

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 29, 2015

Contributor

This doesn't look like the correct UI.
Should use --volume-driver=tmpfs and provide a tmpfs driver to use.

Contributor

cpuguy83 commented May 29, 2015

This doesn't look like the correct UI.
Should use --volume-driver=tmpfs and provide a tmpfs driver to use.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan May 29, 2015

Contributor

@cpuguy83 HOw would I specify the equivalent of

docker run -ti -v tmpfs:/tmp -v tmpfs:/run --read-only fedora /bin/sh

Contributor

rhatdan commented May 29, 2015

@cpuguy83 HOw would I specify the equivalent of

docker run -ti -v tmpfs:/tmp -v tmpfs:/run --read-only fedora /bin/sh

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 29, 2015

Contributor

@rhatdan docker run -it -v /tmp -v /run --volume-driver tmpfs --read-only fedora /bin/sh

Contributor

cpuguy83 commented May 29, 2015

@rhatdan docker run -it -v /tmp -v /run --volume-driver tmpfs --read-only fedora /bin/sh

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 29, 2015

Contributor

The --volume-driver syntax isn't necessarily ideal at the moment since you can only have 1 volume driver per container... but this is what's in master right now with the experimental channel.

Contributor

cpuguy83 commented May 29, 2015

The --volume-driver syntax isn't necessarily ideal at the moment since you can only have 1 volume driver per container... but this is what's in master right now with the experimental channel.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan May 29, 2015

Contributor

Well I think that stinks, for several reasons,

  • It gives me no flexibility, I don't have any ability to have multiple volumes of different types.
  • It is not easy to understand. The -v syntax is well understood, and my change is intuitive, I belive, Similar individual mount commands
  • I don't believe the --volume-driver concept will tar up the contents of the underlying file system to be used in the new tmpfs, which is critical to using it for /run and I like the idea of using it for /var.
Contributor

rhatdan commented May 29, 2015

Well I think that stinks, for several reasons,

  • It gives me no flexibility, I don't have any ability to have multiple volumes of different types.
  • It is not easy to understand. The -v syntax is well understood, and my change is intuitive, I belive, Similar individual mount commands
  • I don't believe the --volume-driver concept will tar up the contents of the underlying file system to be used in the new tmpfs, which is critical to using it for /run and I like the idea of using it for /var.
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 29, 2015

Contributor

@rhatdan Feedback here #13420 would be appreciated.

Contributor

cpuguy83 commented May 29, 2015

@rhatdan Feedback here #13420 would be appreciated.

@eparis

This comment has been minimized.

Show comment
Hide comment
@eparis

eparis May 29, 2015

Contributor

how does this interact with docker run -memory-swap="1G"?

Contributor

eparis commented May 29, 2015

how does this interact with docker run -memory-swap="1G"?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan May 29, 2015

Contributor

@eparis ask questions on #13420

Contributor

rhatdan commented May 29, 2015

@eparis ask questions on #13420

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 29, 2015

Contributor

just curious if this is any different than the one @crosbymichael had open :)

Contributor

jessfraz commented May 29, 2015

just curious if this is any different than the one @crosbymichael had open :)

@calavera calavera closed this May 29, 2015

@calavera calavera reopened this May 29, 2015

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera May 29, 2015

Contributor

sorry, I didn't mean to close this. Fat fingers

Contributor

calavera commented May 29, 2015

sorry, I didn't mean to close this. Fat fingers

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan May 31, 2015

Contributor

@jfrazelle I only saw the --tmpfs /tmp one by @crosbymichael, did he do another?

Contributor

rhatdan commented May 31, 2015

@jfrazelle I only saw the --tmpfs /tmp one by @crosbymichael, did he do another?

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 1, 2015

Contributor

I understand that this is very useful in many cases and necessary in the case of systemd, but the implementation is far from ideal.

Leaving the discussion about how to set the driver name aside (I know it's a problem and I'm looking forward to find a good solution), I feel like adding more conditionals to that loop, in order to support more options in the mounting operation, is not the right solution.

The mount points or the exec driver mount configurations should be able to generate a valid configs.Mount based on the information they hold.

That loop should be reduced to something like this:

for _, m := range c.Mounts {
    container.Mounts = append(container.Mounts, m.Config())
}

Yes, there might be more lines of code to touch but it will be easier to maintain in the long term.

Contributor

calavera commented Jun 1, 2015

I understand that this is very useful in many cases and necessary in the case of systemd, but the implementation is far from ideal.

Leaving the discussion about how to set the driver name aside (I know it's a problem and I'm looking forward to find a good solution), I feel like adding more conditionals to that loop, in order to support more options in the mounting operation, is not the right solution.

The mount points or the exec driver mount configurations should be able to generate a valid configs.Mount based on the information they hold.

That loop should be reduced to something like this:

for _, m := range c.Mounts {
    container.Mounts = append(container.Mounts, m.Config())
}

Yes, there might be more lines of code to touch but it will be easier to maintain in the long term.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jun 1, 2015

Contributor

@calavera Is the updated patch what you had in mind?

Contributor

rhatdan commented Jun 1, 2015

@calavera Is the updated patch what you had in mind?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jun 10, 2015

Contributor

Any movement on this? @jfrazelle @crosbymichael @tianon @calavera

Contributor

rhatdan commented Jun 10, 2015

Any movement on this? @jfrazelle @crosbymichael @tianon @calavera

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 10, 2015

Contributor

@rhatdan are you sure you updated the patch? It looks like the latest commit is from before my latest comment.

Contributor

calavera commented Jun 10, 2015

@rhatdan are you sure you updated the patch? It looks like the latest commit is from before my latest comment.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jun 10, 2015

Contributor

@calavera I guess I had a separate tag, tmpfs. Sorry, replaced this package with updated tmpfs patch.

Contributor

rhatdan commented Jun 10, 2015

@calavera I guess I had a separate tag, tmpfs. Sorry, replaced this package with updated tmpfs patch.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 11, 2015

Contributor

I'm still not sold on the idea of making tmpfs a special case in the volume flag. In my mind, something mounted as tmpfs is not even a volume, why should we use the volume flag then?

I agree with @dqminh that it should not use an absolute path either. We already shell out to other commands using path lookup, why this should be different. I'm also thinking that having an absolute path is not going to work very well with other platforms, even shelling out to tar might not work very well 😸

Contributor

calavera commented Jun 11, 2015

I'm still not sold on the idea of making tmpfs a special case in the volume flag. In my mind, something mounted as tmpfs is not even a volume, why should we use the volume flag then?

I agree with @dqminh that it should not use an absolute path either. We already shell out to other commands using path lookup, why this should be different. I'm also thinking that having an absolute path is not going to work very well with other platforms, even shelling out to tar might not work very well 😸

@dqminh

This comment has been minimized.

Show comment
Hide comment
@dqminh

dqminh Jun 12, 2015

Contributor

something mounted as tmpfs is not even a volume, why should we use the volume flag then

Right, tmpfs is just a special type of mount, not really a volume (i.e., actual directory on the host ). But the problem is that volume is the only public interface in docker right now to specify mounts hence the overloading part of tmpfs as Source.

There's also #9586 which adds --tmpfs /tmpfs
Or maybe we can even add tmpfs to list of mount properties i.e, -v /tmpfs:[ro,rw,tmpfs] ?

Contributor

dqminh commented Jun 12, 2015

something mounted as tmpfs is not even a volume, why should we use the volume flag then

Right, tmpfs is just a special type of mount, not really a volume (i.e., actual directory on the host ). But the problem is that volume is the only public interface in docker right now to specify mounts hence the overloading part of tmpfs as Source.

There's also #9586 which adds --tmpfs /tmpfs
Or maybe we can even add tmpfs to list of mount properties i.e, -v /tmpfs:[ro,rw,tmpfs] ?

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Jun 12, 2015

Contributor

See that #9586 has several LGTM, I would expect the same approval if we reintroduce the flag here. I also think that having a Type in the mount points will be beneficial in the future, we might as well do it sooner rather than later.

Contributor

calavera commented Jun 12, 2015

See that #9586 has several LGTM, I would expect the same approval if we reintroduce the flag here. I also think that having a Type in the mount points will be beneficial in the future, we might as well do it sooner rather than later.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jun 13, 2015

Contributor

I would be fine with the --tmpfs proposal, or any proposal that would allow me to specify a tmpfs into a container. We are hearing a lot from customers that want IMMUTABLE containers, where we could run --read-only with /tmp and /run as tmpfs. We just need to standardize on the CLI. The beauty of this patch is that it is very small and I believe most users see volumes as a mechanism to mount something into a container. So using it for tmpfs and potentially other future file system types into the container feels natural.

Contributor

rhatdan commented Jun 13, 2015

I would be fine with the --tmpfs proposal, or any proposal that would allow me to specify a tmpfs into a container. We are hearing a lot from customers that want IMMUTABLE containers, where we could run --read-only with /tmp and /run as tmpfs. We just need to standardize on the CLI. The beauty of this patch is that it is very small and I believe most users see volumes as a mechanism to mount something into a container. So using it for tmpfs and potentially other future file system types into the container feels natural.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 17, 2015

Contributor

I think mount properties would be the best interface. Because really, tmpfs and vfs aren't exactly "volume drivers". They just change the mount options to specify what we are mounting where. If we set up volume options like "ethereal" to be set using mount properties, we can do stuff like --volume /tmp:ro,tmpfs -- combining several mount options from multiple volume drivers (this is a separate issue, but I think that combining different mount options would be useful when we start adding more volume drivers).

Contributor

cyphar commented Jun 17, 2015

I think mount properties would be the best interface. Because really, tmpfs and vfs aren't exactly "volume drivers". They just change the mount options to specify what we are mounting where. If we set up volume options like "ethereal" to be set using mount properties, we can do stuff like --volume /tmp:ro,tmpfs -- combining several mount options from multiple volume drivers (this is a separate issue, but I think that combining different mount options would be useful when we start adding more volume drivers).

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 17, 2015

Contributor

@rhatdan What happens if we stop a container and then we restart it? AFAICS, the data wouldn't persist -- I'd personally use this feature to store something like SSL certificates or something in the container, which I wouldn't want to commit with a docker commit but I would want to persist across restarts of a container. Was this feature envisioned to solve a separate problem?

Contributor

cyphar commented Jun 17, 2015

@rhatdan What happens if we stop a container and then we restart it? AFAICS, the data wouldn't persist -- I'd personally use this feature to store something like SSL certificates or something in the container, which I wouldn't want to commit with a docker commit but I would want to persist across restarts of a container. Was this feature envisioned to solve a separate problem?

janLo added a commit to janLo/docker-py that referenced this pull request Feb 11, 2016

Add support for Tmpfs declaration in Hostconfig.
This adds support for the Tmpfs option introduced in Docker 1.10.
See: moby/moby#13587

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>

janLo added a commit to janLo/docker-py that referenced this pull request Feb 11, 2016

Add support for Tmpfs declaration in Hostconfig.
This adds support for the Tmpfs option introduced in Docker 1.10.
See: moby/moby#13587

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>

janLo added a commit to janLo/docker-py that referenced this pull request Feb 11, 2016

Add support for Tmpfs declaration in Hostconfig.
This adds support for the Tmpfs option introduced in Docker 1.10.
See: moby/moby#13587

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
@Mirabis

This comment has been minimized.

Show comment
Hide comment
@Mirabis

Mirabis Feb 15, 2016

any idea how/if this maps to docker-compose?

Mirabis commented Feb 15, 2016

any idea how/if this maps to docker-compose?

@nfnty

This comment has been minimized.

Show comment
Hide comment
@nfnty

nfnty Feb 15, 2016

It's undocumented but the API supports it. Check out https://github.com/nfnty/dockerfiles/blob/master/containers.yaml to see how I'm using it.

nfnty commented Feb 15, 2016

It's undocumented but the API supports it. Check out https://github.com/nfnty/dockerfiles/blob/master/containers.yaml to see how I'm using it.

@Mirabis

This comment has been minimized.

Show comment
Hide comment
@Mirabis

Mirabis Feb 15, 2016

File gave me quite some insights, but "
Unsupported config option for services.xxx: 'Tmpfs'
" (tried lowercase too)..

But ur file is different entirely.. so yaml vs .yml etc...

Mirabis commented Feb 15, 2016

File gave me quite some insights, but "
Unsupported config option for services.xxx: 'Tmpfs'
" (tried lowercase too)..

But ur file is different entirely.. so yaml vs .yml etc...

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 15, 2016

Member

@Mirabis looks like @nfnty is not using Docker Compose, but a custom Python script; https://github.com/nfnty/dockerfiles/blob/master/scripts/containers.py. I suggest to search for existing issues, or open a new one in the Docker Compose issue tracker; https://github.com/docker/compose/issues

Member

thaJeztah commented Feb 15, 2016

@Mirabis looks like @nfnty is not using Docker Compose, but a custom Python script; https://github.com/nfnty/dockerfiles/blob/master/scripts/containers.py. I suggest to search for existing issues, or open a new one in the Docker Compose issue tracker; https://github.com/docker/compose/issues

aanand added a commit to aanand/docker-py that referenced this pull request Mar 16, 2016

Add support for Tmpfs declaration in Hostconfig.
This adds support for the Tmpfs option introduced in Docker 1.10.
See: moby/moby#13587

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>

@louy louy referenced this pull request Apr 30, 2016

Open

--tmpfs support #813

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Aug 4, 2016

Are there any plans for adding support for tmpfs at build time?

NikolausDemmel commented Aug 4, 2016

Are there any plans for adding support for tmpfs at build time?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 5, 2016

Contributor

What would be your use case?

Contributor

rhatdan commented Aug 5, 2016

What would be your use case?

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Aug 5, 2016

Use case 1: Storing secrets temporarily without them ending up in intermediate image (This IMHO hasn't really made progress in over a year of discussion):

COPY secrets/git-credentials /tmp/tmpfs/git-credentials
RUN git config ... /tmp/tmpfs/git-credentials ... && git clone ...
RUN rm /tmp/tmpfs/git-credentials

Use case 2: Copying files from context temporarily that otherwise bloat the image (This problem has been raised many times with various different suggestions and no real solution so far):

# 1. copy and extract tarball
COPY foo.tgz /tmp/tmpfs/foo.tgz
WORKDIR /foo/src
RUN tar -xf /tmp/tmpfs/foo.tgz && \
    rm -rf /tmp/tmpfs/foo.tgz

# 2. install pip package from local source into image
COPY src/foopkg /tmp/tmpfs/foopkg
RUN pip install /tmp/tmpfs/foopkg && \
    rm -rf /tmp/tmpfs/foopkg

# 3. copy latest commit of just the current branch as a git repo
COPY .git /tmp/tmpfs/foo.git
WORKDIR /foo/src
RUN git init && \
    git remote add origin /tmp/tmpfs/foo.git && \
    git fetch origin $(cut -d/ -f3- < /tmp/tmpfs/foo.git/HEAD) && \
    git checkout $(cut -d/ -f3- < /tmp/tmpfs/foo.git/HEAD) && \
    git remote rm origin && \
    rm .git/FETCH_HEAD && \
    rm -rf /tmp/tmpfs/foo.git

NikolausDemmel commented Aug 5, 2016

Use case 1: Storing secrets temporarily without them ending up in intermediate image (This IMHO hasn't really made progress in over a year of discussion):

COPY secrets/git-credentials /tmp/tmpfs/git-credentials
RUN git config ... /tmp/tmpfs/git-credentials ... && git clone ...
RUN rm /tmp/tmpfs/git-credentials

Use case 2: Copying files from context temporarily that otherwise bloat the image (This problem has been raised many times with various different suggestions and no real solution so far):

# 1. copy and extract tarball
COPY foo.tgz /tmp/tmpfs/foo.tgz
WORKDIR /foo/src
RUN tar -xf /tmp/tmpfs/foo.tgz && \
    rm -rf /tmp/tmpfs/foo.tgz

# 2. install pip package from local source into image
COPY src/foopkg /tmp/tmpfs/foopkg
RUN pip install /tmp/tmpfs/foopkg && \
    rm -rf /tmp/tmpfs/foopkg

# 3. copy latest commit of just the current branch as a git repo
COPY .git /tmp/tmpfs/foo.git
WORKDIR /foo/src
RUN git init && \
    git remote add origin /tmp/tmpfs/foo.git && \
    git fetch origin $(cut -d/ -f3- < /tmp/tmpfs/foo.git/HEAD) && \
    git checkout $(cut -d/ -f3- < /tmp/tmpfs/foo.git/HEAD) && \
    git remote rm origin && \
    rm .git/FETCH_HEAD && \
    rm -rf /tmp/tmpfs/foo.git
@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi
Member

aluzzardi commented Aug 5, 2016

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 7, 2016

Contributor

Would copying to /dev/shm solve your problem if that exists in a build container?

Contributor

rhatdan commented Aug 7, 2016

Would copying to /dev/shm solve your problem if that exists in a build container?

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Aug 7, 2016

Would copying to /dev/shm solve your problem if that exists in a build container?

I guess so, if it is mounted as a tmpfs and therfore does not get commited to one of the intermediate images. The question is how large it can get. For the "storing credentials temporarily" use case the size should not be a limit, but for the use case of copying large files (temporarily) from the context, a fixed size-limit could be a problem.

NikolausDemmel commented Aug 7, 2016

Would copying to /dev/shm solve your problem if that exists in a build container?

I guess so, if it is mounted as a tmpfs and therfore does not get commited to one of the intermediate images. The question is how large it can get. For the "storing credentials temporarily" use case the size should not be a limit, but for the use case of copying large files (temporarily) from the context, a fixed size-limit could be a problem.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 8, 2016

Contributor

I think they default it to 65k

Contributor

rhatdan commented Aug 8, 2016

I think they default it to 65k

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Aug 8, 2016

I think they default it to 65k

Ok, that would be enough for some simple credentials, but not for any serious file-copying.

Could /dev/shm be enabled for build containers easily?

NikolausDemmel commented Aug 8, 2016

I think they default it to 65k

Ok, that would be enough for some simple credentials, but not for any serious file-copying.

Could /dev/shm be enabled for build containers easily?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 8, 2016

Contributor

I think it is, I am not able to use docker right now, but try to copy something to /dev/shm and it should work and not show up in the container image

Contributor

rhatdan commented Aug 8, 2016

I think it is, I am not able to use docker right now, but try to copy something to /dev/shm and it should work and not show up in the container image

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Aug 8, 2016

I just checked, it is there (mount reports shm on /dev/shm type tmpfs (rw,nosuid,nodev,noexec,relatime,size=65536k)), but it is remounted at every step. I.e. at the beginning of every statement /dev/shm is empty. Given that, it is of course pointless to COPY foo /dev/shm/. We would need a something that persists over the while build process. I'm not sure tmpfs would actually be the preferred choice. Probably more something like an explicit bind mount of some folder on top of the COW filesystem that persists throughout the build and is discarded afterwards.

NikolausDemmel commented Aug 8, 2016

I just checked, it is there (mount reports shm on /dev/shm type tmpfs (rw,nosuid,nodev,noexec,relatime,size=65536k)), but it is remounted at every step. I.e. at the beginning of every statement /dev/shm is empty. Given that, it is of course pointless to COPY foo /dev/shm/. We would need a something that persists over the while build process. I'm not sure tmpfs would actually be the preferred choice. Probably more something like an explicit bind mount of some folder on top of the COW filesystem that persists throughout the build and is discarded afterwards.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 9, 2016

Contributor

We have added support for docker build -v /SOURCE:/Dest to our patches for projectatomic/docker. Sadly docker upstream refuses this patch. 👎 We added it because out customers wanted it for precisely the issue you are looking at.

Contributor

rhatdan commented Aug 9, 2016

We have added support for docker build -v /SOURCE:/Dest to our patches for projectatomic/docker. Sadly docker upstream refuses this patch. 👎 We added it because out customers wanted it for precisely the issue you are looking at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment