Fix issue backporting mount spec to pre-1.13 obj by cpuguy83 · Pull Request #32821 · moby/moby

cpuguy83 · 2017-04-25T18:38:26Z

In some cases a mount spec would not be properly backported which could
lead to accidental removal of the underlying volume on container remove
(which should never happen with named volumes).

Adds unit tests for this as well. Unfortunately I had to add a daemon
depdency for the backport function due to looking up VolumesFrom
specs.

Fixes #32613

thaJeztah · 2017-04-28T23:26:21Z

@cpuguy83 tests are failing, and looks related

cpuguy83 · 2017-05-04T12:43:51Z

All green

thaJeztah · 2017-05-09T19:32:57Z

ping @cpuguy83 looks like it needs a rebase 😢

cpuguy83 · 2017-05-09T19:39:48Z

rebased

thaJeztah · 2017-05-09T20:13:54Z

ping @dnephin @stevvooe PTAL

vdemeester

LGTM 🐯

thaJeztah

left some questions ☺️

thaJeztah · 2017-05-10T12:28:07Z

daemon/volumes.go

nit:lowercase Error ?

Looks like we should continue here, as we still try to use fromC even if we got an error

Yes need continue (or something), nice catch.

Logged errors are typically capitalized.

In that case, capitalize the error at line 298 👍

thaJeztah · 2017-05-10T12:49:56Z

daemon/volumes.go

perhaps remove the updated variable here, as it's easily mistaken for the updated output variable?

if daemon.backportMountSpec(fromC) { fromC.ToDisk() }

Also, wondering if we should

log errors resulting from fromC.ToDisk() here, like we to here https://github.com/moby/moby/pull/32821/files#diff-1a1f3e7ad9b1d7584e2d3e7d0c4c3db9R201

if the locking and ToDisk() should be done inside the backportMountSpec function? Looks like we always Lock, and write to Disk when calling this function, and if updated is true?

thaJeztah · 2017-05-10T12:56:31Z

daemon/volumes.go

"probably specified view the hostconfig.Mounts" - sorry, couldn't "grasp" what you tried to say there

thaJeztah · 2017-05-10T13:17:43Z

daemon/volumes.go

Will this be an issue if both the container itself, and the --volumes-from container specify a volume for the same destination? (and the --volumes-from needs updating)

-volumes-from is lowest precedence.
Can't have two mountpoints with the same destination.

thaJeztah

found a typo, and some questions 😅

thaJeztah · 2017-05-11T11:21:06Z

daemon/volumes.go

typo s/unepxected/unexpected/

thaJeztah · 2017-05-11T11:41:42Z

daemon/volumes.go

Wondering in what situation this would need an update; cm.Destination is only added to volumesFrom if we entered the for _, fromSpec := range container.HostConfig.VolumesFrom { loop below, in which case the needsUpdate() check is already performed.

Should this just be continue?

Let me just move this stuff up to make the code simpler.
I was trying to save some time and not process volumes-from if not-neccessary, but the code is very messy.

thaJeztah · 2017-05-11T11:47:50Z

daemon/volumes.go

We no longer write updated mount-specs to disk for volumes-from containers, only in-memory, correct?

In this function yes, they are persisted to disk in the caller.
I can add a note to that in the docstring.

In some cases a mount spec would not be properly backported which could lead to accidental removal of the underlying volume on container remove (which should never happen with named volumes). Adds unit tests for this as well. Unfortunately I had to add a daemon depdency for the backport function due to looking up `VolumesFrom` specs. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2017-05-12T17:05:15Z

This is updated and green.

thaJeztah

LGTM

thaJeztah · 2017-05-15T13:58:26Z

All green! Checked with @vdemeester earlier, and this still looked good to him

thaJeztah · 2017-05-15T19:49:40Z

daemon/volumes.go

+	for _, fromSpec := range container.HostConfig.VolumesFrom {
+		from, _, err := volume.ParseVolumesFrom(fromSpec)
+		if err != nil {
+			logrus.WithError(err).WithField("id", container.ID).Error("Error reading volumes-from spec during mount spec backport")


Missed this in my review; needs a continue (see #33207 (comment))

thaJeztah · 2017-05-15T19:59:45Z

cherry-picked for 17.03.2 in #33207

Reuse existing structures and rely on json serialization to deep copy Container objects. Also consolidate all "save" operations on container.CheckpointTo, which now both saves a serialized json to disk, and replicates state to the ACID in-memory store. Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit edad527) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - daemon/container_operations.go: missing moby#30117 - daemon/daemon.go: missing moby#32821 - daemon/list.go: missing moby#27557, moby#33241 etc.

GordonTheTurtle added the status/0-triage label Apr 25, 2017

cpuguy83 mentioned this pull request Apr 25, 2017

Named volume incorrectly removed after engine upgrade #32613

Closed

cpuguy83 added status/2-code-review and removed status/0-triage labels Apr 25, 2017

cpuguy83 added this to the 17.06 milestone Apr 25, 2017

cpuguy83 force-pushed the 32613_fix_volspec_backport branch from d649909 to 47999e9 Compare April 26, 2017 16:58

cpuguy83 added the rebuild/* label Apr 27, 2017

GordonTheTurtle removed the rebuild/* label Apr 27, 2017

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 28, 2017

cpuguy83 force-pushed the 32613_fix_volspec_backport branch 2 times, most recently from 7bc5580 to 6c4ec35 Compare May 1, 2017 17:59

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 1, 2017

cpuguy83 added the rebuild/powerpc label May 1, 2017

GordonTheTurtle removed the rebuild/powerpc label May 1, 2017

cpuguy83 force-pushed the 32613_fix_volspec_backport branch from 6c4ec35 to 10ae0e7 Compare May 9, 2017 19:39

GordonTheTurtle assigned vdemeester May 10, 2017

vdemeester approved these changes May 10, 2017

View reviewed changes

thaJeztah requested changes May 10, 2017

View reviewed changes

cpuguy83 force-pushed the 32613_fix_volspec_backport branch from 10ae0e7 to f482626 Compare May 10, 2017 14:57

cpuguy83 added the rebuild/z label May 10, 2017

GordonTheTurtle removed the rebuild/z label May 10, 2017

thaJeztah requested changes May 11, 2017

View reviewed changes

cpuguy83 force-pushed the 32613_fix_volspec_backport branch from f482626 to 3cf1859 Compare May 11, 2017 16:32

thaJeztah approved these changes May 15, 2017

View reviewed changes

thaJeztah added the status/4-merge label May 15, 2017

thaJeztah merged commit 6cea2e5 into moby:master May 15, 2017

cpuguy83 mentioned this pull request May 15, 2017

[17.03] Fix issue backporting mount spec to pre-1.13 obj #33207

Merged

thaJeztah reviewed May 15, 2017

View reviewed changes

cpuguy83 deleted the 32613_fix_volspec_backport branch September 20, 2017 16:54

kolyshkin mentioned this pull request May 2, 2020

backport: No container locks on docker ps projectatomic/docker#375

Merged

Conversation

cpuguy83 commented Apr 25, 2017

Uh oh!

thaJeztah commented Apr 28, 2017

Uh oh!

cpuguy83 commented May 4, 2017

Uh oh!

thaJeztah commented May 9, 2017

Uh oh!

cpuguy83 commented May 9, 2017

Uh oh!

thaJeztah commented May 9, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 12, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants