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

Re-validate Mounts on container start #35833

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
@thaJeztah
Member

thaJeztah commented Dec 19, 2017

fixes #35821

Validation of Mounts was only performed on container creation, not on container start. As a result, if the host-path no longer existed when the container was started, a directory was created in the given location.

This is the wrong behavior, because when using the Mounts API, host paths should never be created, and an error should be produced instead.

This patch adds a validation step on container start, and produces an error if the host path is not found.

- How to verify it

Create a container that bind-mounts a directory that exists;

$ mkdir once-existed
$ docker create --name repro-35821 \
   --mount type=bind,source="$(pwd)"/once-existed,target=/app \
   busybox
a7b4d510d1afd3eabea59414b94e9565af2927606f8a79f27aa855f3a8ea2817

After creating, remove the directory, and verify the directory is gone;

$ rm -r once-existed
$ ls
(empty)

Verify that the container refuses to start because the directory no longer exists;

$ docker start repro-35821
Error response from daemon: invalid mount config for type "bind": bind source path does not exist

- Description for the changelog

* Validate Mount-specs on container start to prevent creating missing host-paths [moby/moby#35833](https://github.com/moby/moby/pull/35833)

- A picture of a cute animal (not mandatory but encouraged)

bird-sparrow

Re-validate Mounts on container start
Validation of Mounts was only performed on container _creation_, not on
container _start_. As a result, if the host-path no longer existed
when the container was started, a directory was created in the given
location.

This is the wrong behavior, because when using the `Mounts` API, host paths
should never be created, and an error should be produced instead.

This patch adds a validation step on container start, and produces an
error if the host path is not found.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
}
if runtime.GOOS == "windows" {

This comment has been minimized.

@thaJeztah
@thaJeztah

thaJeztah Dec 19, 2017

Member
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Dec 19, 2017

@vdemeester

LGTM 🦁

@h45rd

This comment has been minimized.

Show comment
Hide comment
@h45rd

h45rd Dec 19, 2017

Wow. That was fast. And thorough.
Thanks a bunch, @thaJeztah :)

h45rd commented Dec 19, 2017

Wow. That was fast. And thorough.
Thanks a bunch, @thaJeztah :)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 19, 2017

Member

Thank you for finding this one @h45rd! - it's easy to miss these things (as it's not a common scenario to have differences happen between "create" and "start")

Member

thaJeztah commented Dec 19, 2017

Thank you for finding this one @h45rd! - it's easy to miss these things (as it's not a common scenario to have differences happen between "create" and "start")

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 19, 2017

Member

"experimental" failure is being tracked here; #35825;

12:47:58 ----------------------------------------------------------------------
12:47:58 FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest
12:47:58 
12:47:58 check_test.go:371:
12:47:58     d.Stop(c)
12:47:58 daemon/daemon.go:395:
12:47:58     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
12:47:58 ... Error: Error while stopping the daemon dc53b7acb7a27 : exit status 130
12:47:58 
12:47:58 
12:47:58 ----------------------------------------------------------------------
12:47:58 PANIC: docker_api_swarm_service_test.go:201: DockerSwarmSuite.TestAPISwarmServicesUpdateStartFirst
12:47:58 
12:47:58 [dc53b7acb7a27] waiting for daemon to start
12:47:58 [dc53b7acb7a27] daemon started
12:47:58 
12:47:58 [dc53b7acb7a27] daemon started
12:47:58 Attempt #2: daemon is still running with pid 9575
12:47:58 Attempt #3: daemon is still running with pid 9575
12:47:58 Attempt #4: daemon is still running with pid 9575
12:47:58 [dc53b7acb7a27] exiting daemon
12:47:58 ... Panic: Fixture has panicked (see related PANIC)
12:47:58 
Member

thaJeztah commented Dec 19, 2017

"experimental" failure is being tracked here; #35825;

12:47:58 ----------------------------------------------------------------------
12:47:58 FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest
12:47:58 
12:47:58 check_test.go:371:
12:47:58     d.Stop(c)
12:47:58 daemon/daemon.go:395:
12:47:58     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
12:47:58 ... Error: Error while stopping the daemon dc53b7acb7a27 : exit status 130
12:47:58 
12:47:58 
12:47:58 ----------------------------------------------------------------------
12:47:58 PANIC: docker_api_swarm_service_test.go:201: DockerSwarmSuite.TestAPISwarmServicesUpdateStartFirst
12:47:58 
12:47:58 [dc53b7acb7a27] waiting for daemon to start
12:47:58 [dc53b7acb7a27] daemon started
12:47:58 
12:47:58 [dc53b7acb7a27] daemon started
12:47:58 Attempt #2: daemon is still running with pid 9575
12:47:58 Attempt #3: daemon is still running with pid 9575
12:47:58 Attempt #4: daemon is still running with pid 9575
12:47:58 [dc53b7acb7a27] exiting daemon
12:47:58 ... Panic: Fixture has panicked (see related PANIC)
12:47:58 
@boaz1337

LGTM 👍

@thaJeztah thaJeztah requested a review from cpuguy83 Dec 27, 2017

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Dec 28, 2017

Contributor

LGTM (not a maintainer)

Contributor

kolyshkin commented Dec 28, 2017

LGTM (not a maintainer)

@yongtang

LGTM

@coolljt0725

This comment has been minimized.

Show comment
Hide comment
@coolljt0725

coolljt0725 Jan 2, 2018

Contributor

LGTM

Contributor

coolljt0725 commented Jan 2, 2018

LGTM

@coolljt0725 coolljt0725 merged commit 9d9992b into moby:master Jan 2, 2018

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38524 has succeeded
Details
janky Jenkins build Docker-PRs 47218 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7595 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18730 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7436 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:fix-mount-creation-on-start branch Jan 2, 2018

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 2, 2018

Contributor

This works. I wonder if maybe we should move the validation to on start instead of on create anyway.

Contributor

cpuguy83 commented Jan 2, 2018

This works. I wonder if maybe we should move the validation to on start instead of on create anyway.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 2, 2018

Member

I was thinking about that, but thought it would be good to have it on both, so that when creating a container, I get direct feedback (as a user) that the path I'm trying to mount doesn't exist

Member

thaJeztah commented Jan 2, 2018

I was thinking about that, but thought it would be good to have it on both, so that when creating a container, I get direct feedback (as a user) that the path I'm trying to mount doesn't exist

@antoinetran

This comment has been minimized.

Show comment
Hide comment
@antoinetran

antoinetran Mar 7, 2018

Hi, if I understand correctly, this fix will prevent a container to start if the host directory does not exist. Will it retry with a restart policy set to always/unless-stopped/on-failure or just stop immediately? I have a use-case where we use a container to mount a distributed FS (like NFS) in host, and then mount the host directory to a second container, and this fix might be help us to make the second container wait for the host directory to be mounted. Thanks!

antoinetran commented Mar 7, 2018

Hi, if I understand correctly, this fix will prevent a container to start if the host directory does not exist. Will it retry with a restart policy set to always/unless-stopped/on-failure or just stop immediately? I have a use-case where we use a container to mount a distributed FS (like NFS) in host, and then mount the host directory to a second container, and this fix might be help us to make the second container wait for the host directory to be mounted. Thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 7, 2018

Member

@antoinetran I haven't tested that case, but this fix is included in docker 17.12.1, and Docker 18.01 and up, so you can give it a try 👍

Member

thaJeztah commented Mar 7, 2018

@antoinetran I haven't tested that case, but this fix is included in docker 17.12.1, and Docker 18.01 and up, so you can give it a try 👍

@antoinetran

This comment has been minimized.

Show comment
Hide comment
@antoinetran

antoinetran Mar 8, 2018

I tested it and it does seem this fix work. But I guess I tested wrong: the fix only applies to a special bind mount with "once-existed"? Because I tested it with regular docker volume mount "docker run -v src:dest:rw".

antoinetran commented Mar 8, 2018

I tested it and it does seem this fix work. But I guess I tested wrong: the fix only applies to a special bind mount with "once-existed"? Because I tested it with regular docker volume mount "docker run -v src:dest:rw".

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 8, 2018

Member

@antoinetran no the once-existed is just a name I picked for the test, but this fix is only for the --mount flag: the -v option will automatically create the host-path if it's missing (we have to keep that behaviour for backward compatibility). The --mount option on the other hand, will fail if the host-path is not found.

The fix in this PR makes sure that a container created with the --mount option will also fail if the path existed when it was created, but no longer exists when (re)starting the container.

Member

thaJeztah commented Mar 8, 2018

@antoinetran no the once-existed is just a name I picked for the test, but this fix is only for the --mount flag: the -v option will automatically create the host-path if it's missing (we have to keep that behaviour for backward compatibility). The --mount option on the other hand, will fail if the host-path is not found.

The fix in this PR makes sure that a container created with the --mount option will also fail if the path existed when it was created, but no longer exists when (re)starting the container.

@antoinetran

This comment has been minimized.

Show comment
Hide comment
@antoinetran

antoinetran Mar 9, 2018

@thaJeztah ok. Do you happen to know if this type of bind can be described in a docker-compose.yml v2? I couldn't find that answer easily, it does not seems like a common option. If yes, I can try that.

antoinetran commented Mar 9, 2018

@thaJeztah ok. Do you happen to know if this type of bind can be described in a docker-compose.yml v2? I couldn't find that answer easily, it does not seems like a common option. If yes, I can try that.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 9, 2018

Member

@antoinetran I think you can do that using the long syntax a described in the docker compose file reference.

I recommend checking if that does indeed follow the same semantics, as it may be possible that in the 2.x format it may still be auto-creating the host directories (see the discussion on docker/compose#5490 (comment)).

Member

thaJeztah commented Mar 9, 2018

@antoinetran I think you can do that using the long syntax a described in the docker compose file reference.

I recommend checking if that does indeed follow the same semantics, as it may be possible that in the 2.x format it may still be auto-creating the host directories (see the discussion on docker/compose#5490 (comment)).

@antoinetran

This comment has been minimized.

Show comment
Hide comment
@antoinetran

antoinetran Mar 19, 2018

@thaJeztah
Ok I extensively tested this bind mount in almost all latest: docker-ce-17.12.1-ce / CentOs 7.4.1708 / docker-compose 1.19.0 / compose ref 2.3 / Swarm classic 1.2.8.
What works:

  • When submitting to Swarm a docker-compose.yml with a service with bind mount, docker-compose refuses to run the service if the source directory does not exist.
  • If the container is started with the source directory mounted, if I stop the service, and remove the directory, the container cannot start again, complaining about missing directory.

What does not work:

  • If I stop the docker daemon, and remove the directory, then restart docker, then the container start and recreates the directori(es)!

antoinetran commented Mar 19, 2018

@thaJeztah
Ok I extensively tested this bind mount in almost all latest: docker-ce-17.12.1-ce / CentOs 7.4.1708 / docker-compose 1.19.0 / compose ref 2.3 / Swarm classic 1.2.8.
What works:

  • When submitting to Swarm a docker-compose.yml with a service with bind mount, docker-compose refuses to run the service if the source directory does not exist.
  • If the container is started with the source directory mounted, if I stop the service, and remove the directory, the container cannot start again, complaining about missing directory.

What does not work:

  • If I stop the docker daemon, and remove the directory, then restart docker, then the container start and recreates the directori(es)!
@antoinetran

This comment has been minimized.

Show comment
Hide comment
@antoinetran

antoinetran Mar 19, 2018

The expected result for me would be: if I restart docker, a container with restart policy set to unless-stopped/always will try starting many times and will effectively start when the bind mounts conditions will be fulfilled.

antoinetran commented Mar 19, 2018

The expected result for me would be: if I restart docker, a container with restart policy set to unless-stopped/always will try starting many times and will effectively start when the bind mounts conditions will be fulfilled.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 19, 2018

Member

Thanks for testing! Interesting; could you open an issue for that? Wondering if there's still some codepath somewhere that skips the validation (possibly on container restore after daemon start)

Member

thaJeztah commented Mar 19, 2018

Thanks for testing! Interesting; could you open an issue for that? Wondering if there's still some codepath somewhere that skips the validation (possibly on container restore after daemon start)

@agoulti

This comment has been minimized.

Show comment
Hide comment
@agoulti

agoulti May 16, 2018

I'm seeing a race condition which is likely related to this: if the directory is removed at a certain point while "docker run" is executing, a root-owned directory is created.

The following script more often than not produces a root-owned directory:

sudo rm -rf /tmp/try/nnn
mkdir -p /tmp/try/nnn
echo "before start:"  `ls -ld /tmp/try/nnn`
docker run -u 277174:89939 --mount type=bind,src=/tmp/try/nnn,dst=/test debian  sleep 3 &
sleep 0.5
rm -rf /tmp/try/nnn
echo "before sleep:"  `ls -ld /tmp/try/nnn`
sleep 3
echo "after sleep:"  `ls -ld /tmp/try/nnn`

My Docker version is 18.03.0-ce

agoulti commented May 16, 2018

I'm seeing a race condition which is likely related to this: if the directory is removed at a certain point while "docker run" is executing, a root-owned directory is created.

The following script more often than not produces a root-owned directory:

sudo rm -rf /tmp/try/nnn
mkdir -p /tmp/try/nnn
echo "before start:"  `ls -ld /tmp/try/nnn`
docker run -u 277174:89939 --mount type=bind,src=/tmp/try/nnn,dst=/test debian  sleep 3 &
sleep 0.5
rm -rf /tmp/try/nnn
echo "before sleep:"  `ls -ld /tmp/try/nnn`
sleep 3
echo "after sleep:"  `ls -ld /tmp/try/nnn`

My Docker version is 18.03.0-ce

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 16, 2018

Member

@agulti thanks! Could you open a new issue with details so that it can be looked into?

Member

thaJeztah commented May 16, 2018

@agulti thanks! Could you open a new issue with details so that it can be looked into?

@agoulti

This comment has been minimized.

Show comment
Hide comment
@agoulti

agoulti commented May 16, 2018

Done, #37083

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