Fix uneccessary calls to volume.Unmount()

[cpuguy83]

Fixes #22564

When an error occurs on mount, there should not be any call later to
unmount. This can throw off refcounting in the underlying driver
unexpectedly.

Consider these two cases:

$ docker run -v foo:/bar busybox true

$ docker run -v foo:/bar -w /foo busybox true

In the first case, if mounting foo fails, the volume driver will
get a call to unmount (this is the incorrect behavior).

In the second case, the volume driver will not get a call to unmount
(correct behavior).

This occurs because in the first case, /bar does not exist in the
container, and as such there is no call to volume.Mount() during the
create phase. It will error out during the start phase.

In the second case /bar is created before dealing with the volume
because of the -w. Because of this, when the volume is being setup
docker will try to copy the image path contents in the volume, in which
case it will attempt to mount the volume and fail. This happens during
the create phase. This makes it so the container will not be created
(or at least fully created) and the user gets the error on create
instead of start. The error handling is different in these two phases.

Changed to only send unmount if the volume is mounted.

While investigating the cause of the reported issue I found some odd
behavior in unmount calls so I've cleaned those up a bit here as well.

Signed-off-by: Brian Goff [email protected]

[thaJeztah]

ping @tonistiigi @anusha-ragunathan PTAL

[tonistiigi]

I'm not sure I understand this. Do I understand it correctly that this calls umount(MNT_DETACH) ignoring the volume activity count. If this is the case then how is this safe for parallel cp.

[cpuguy83]

@tonistiigi This is the existing behavior.

[cpuguy83]

Also yes, I'm not sure why it's doing the syscall, I just cleaned up the call path for it.

[mlaventure]

LGTM

Umount doesn't seem to specify MNT_DETACH (see here)

[tonistiigi]

@mlaventure detachMounted does https://github.com/docker/docker/pull/27116/files#diff-5dfdabc4cd5100a2aee949c12d2afe6cR341

[vieux]

are we good on this one @mlaventure @tonistiigi ?

[tonistiigi]

@cpuguy83 When you do the rebase please add a comment to volumeMount.ID != "" that this is the best way to check had been actually mounted(if I understand this fix correctly). LGTM after that, although I think detach part needs to be fixed in follow up and volumes either shouldn't be in MountPoints if they are not mounted or they should do their own internal reference counting.

[cpuguy83]

Updated

[tonistiigi]

LGTM

[mlaventure]

LGTM