Set volume NeedsCopyUp to false iff data was copied up #12733

rhatdan · 2022-01-03T15:29:25Z

Currently Docker copies up the first volume on a mountpoint with
data.

Fixes: #12714

Also added NeedsCopyUP, NeedsChown and MountCount to the podman volume
inspect code.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

openshift-ci · 2022-01-03T15:29:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2022-01-03T15:36:41Z

test/system/160-volumes.bats

+    run_podman run --rm --volume $myvolume:/vol $IMAGE true
+    # Confirm that it shows up in 'volume ls', and confirm values
+    run_podman volume ls --format json
+    tests="
+Name           | $myvolume
+Driver         | local
+NeedsCopyUp    | true
+"
+    run_podman run --rm --volume $myvolume:/etc $IMAGE ls /etc/passwd
+    # Confirm that it shows up in 'volume ls', and confirm values
+    run_podman volume ls --format json
+    tests="
+Name           | $myvolume
+Driver         | local
+NeedsCopyUp    | false
+"


This looks like a NOP. Copy/paste artifact maybe?

No, second one has NeedsCopyUp set to False.

...but there's no parse_table. These last two definitions of test are not actually used.

Signing out for the day since, as I just learned, today is a holiday.

Went with inspect check.

edsantiago · 2022-01-04T13:37:41Z

test/system/160-volumes.bats

+    mountpoint="$output"
+    test -e "$mountpoint/passwd"
+    run_podman volume inspect --format '{{.MountCount}}' $myvolume
+    is "$output" "0" "Mount count should be 0"


How would you test a nonzero MountCount? If it's easy, would you mind adding a test? (I tried real quick but couldn't figure it out)

We probably should to this in a separate PR once local mounts are allowed on rootless. (My other PR).

sudo ./bin/podman volume create -o device=tmpfs --opt type=tmpfs dan $ sudo ./bin/podman volume inspect dan [ { "Name": "dan", "Driver": "local", "Mountpoint": "/var/lib/containers/storage/volumes/dan/_data", "CreatedAt": "2022-01-04T10:30:45.886433913-05:00", "Labels": {}, "Scope": "local", "Options": { "device": "tmpfs", "type": "tmpfs" }, "MountCount": 0, "NeedsCopyUp": true, "NeedsChown": true } ] $ sudo ./bin/podman run -d -v dan:/tmp alpine sleep infinity $ sudo ./bin/podman volume inspect dan [ { "Name": "dan", "Driver": "local", "Mountpoint": "/var/lib/containers/storage/volumes/dan/_data", "CreatedAt": "2022-01-04T10:30:45.886433913-05:00", "Labels": {}, "Scope": "local", "Options": { "device": "tmpfs", "type": "tmpfs" }, "MountCount": 1, "NeedsCopyUp": true } ] $ sudo podman stop -l -t 0 34cf2f16f45b28ac23a9b66178f82e1dca3c3ebeed6fe3cd6d5d5c17b1f8987b $ sudo ./bin/podman volume inspect dan [ { "Name": "dan", "Driver": "local", "Mountpoint": "/var/lib/containers/storage/volumes/dan/_data", "CreatedAt": "2022-01-04T10:30:45.886433913-05:00", "Labels": {}, "Scope": "local", "Options": { "device": "tmpfs", "type": "tmpfs" }, "MountCount": 0, "NeedsCopyUp": true } ]

edsantiago · 2022-01-04T13:39:41Z

test/system/160-volumes.bats

+
+    run_podman run --rm --volume $myvolume:/vol $IMAGE true
+    run_podman volume inspect --format '{{ .NeedsCopyUp }}' $myvolume
+    is "${output}" "true" "NeedsCopyUP should be true"


The description is tautological. What is it about this test case that makes NeedsCopyUp be true, and could the description be changed to express that? E.g. "volume .NeedsCopyUp after mounting on /vol". Similarly for the next one on line 349.

edsantiago · 2022-01-04T13:40:37Z

libpod/define/volume_inspect.go

+	// NeedsChown indicates that the next time the volume is mounted into
+	// a container, the container will chown the volume to the container process
+	// UID/GID.
+	NeedsChown bool `json:"NeedsChown,omitempty"`


Is there a way to test NeedsChown?

Added a test.

mheon · 2022-01-04T14:10:20Z

libpod/container_internal.go

@@ -1753,6 +1746,13 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
 			return vol, nil
 		}

+		// Set NeedsCopyUp to false since we are about to do first copy


Why is this moving up? This could cause us to trap the volume in a loop of attempting to copy up if reading the source to copy up fails.

Because we want to set the valure to false, Only if the copy up succeeded. Currently we set the copyup to false if there is nothing to copy up on the first mount. Whereas Docker only sets it on a successfully copy up.
$ docker run -v myvol:/myvol ... # No copyup
$ docker run -v myvol:/etc ... # Copyup

Versus we do
$ docker run -v myvol:/myvol ... # No copyup
$ docker run -v myvol:/etc ... # No copyup

The goal of this PR is to match Docker.

I'm still a bit worried about this one - it seems like we won't set NeedsCopyUp to false if the directory we want to copy up either does not exist or is empty.

If this matches Docker it should not a problem. In the first case you don't want the flag to be set.
If the first time there is an error, you fix the error, now the copyup will not happen, and you have to delete the volume and recreate it.

edsantiago · 2022-01-04T21:15:21Z

Tests LGTM; I'd like @mheon to confirm that his concerns are addressed.

Currently Docker copies up the first volume on a mountpoint with data. Fixes: containers#12714 Also added NeedsCopyUP, NeedsChown and MountCount to the podman volume inspect code. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

mheon · 2022-01-07T19:37:14Z

/lgtm

This behavior, frankly, makes no sense to me. But it matches what Docker does, so I guess we're obligated to follow.

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 3, 2022

edsantiago reviewed Jan 3, 2022

View reviewed changes

rhatdan force-pushed the copy branch from 269958e to b8ac44e Compare January 3, 2022 17:38

edsantiago reviewed Jan 4, 2022

View reviewed changes

mheon reviewed Jan 4, 2022

View reviewed changes

rhatdan force-pushed the copy branch 2 times, most recently from 996d0d6 to e2d8533 Compare January 4, 2022 15:34

Set volume NeedsCopyUp to false iff data was copied up

13f3fd2

Currently Docker copies up the first volume on a mountpoint with data. Fixes: containers#12714 Also added NeedsCopyUP, NeedsChown and MountCount to the podman volume inspect code. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the copy branch from e2d8533 to 13f3fd2 Compare January 6, 2022 15:42

openshift-ci bot assigned mheon Jan 7, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2022

openshift-merge-robot merged commit 41934ac into containers:main Jan 7, 2022

Ramblurr mentioned this pull request Jun 24, 2022

volume NeedsCopyUp breaking nfs volumes #14722

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set volume NeedsCopyUp to false iff data was copied up #12733

Set volume NeedsCopyUp to false iff data was copied up #12733

rhatdan commented Jan 3, 2022

openshift-ci bot commented Jan 3, 2022

edsantiago Jan 3, 2022

rhatdan Jan 3, 2022

edsantiago Jan 3, 2022

rhatdan Jan 4, 2022

edsantiago Jan 4, 2022

rhatdan Jan 4, 2022

edsantiago Jan 4, 2022

rhatdan Jan 4, 2022

edsantiago Jan 4, 2022

rhatdan Jan 4, 2022

mheon Jan 4, 2022

rhatdan Jan 4, 2022

mheon Jan 6, 2022

rhatdan Jan 6, 2022

edsantiago commented Jan 4, 2022

mheon commented Jan 7, 2022

Set volume NeedsCopyUp to false iff data was copied up #12733

Set volume NeedsCopyUp to false iff data was copied up #12733

Conversation

rhatdan commented Jan 3, 2022

openshift-ci bot commented Jan 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Jan 4, 2022

mheon commented Jan 7, 2022