Skip to content
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

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

Merged
merged 1 commit into from
Jan 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions libpod/container_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -1700,13 +1700,6 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
if vol.state.NeedsCopyUp {
logrus.Debugf("Copying up contents from container %s to volume %s", c.ID(), vol.Name())

// Set NeedsCopyUp to false immediately, so we don't try this
// again when there are already files copied.
vol.state.NeedsCopyUp = false
if err := vol.save(); err != nil {
return nil, err
}

// If the volume is not empty, we should not copy up.
volMount := vol.mountPoint()
contents, err := ioutil.ReadDir(volMount)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Do not copy second time.
vol.state.NeedsCopyUp = false
if err := vol.save(); err != nil {
return nil, err
}

// Buildah Copier accepts a reader, so we'll need a pipe.
reader, writer := io.Pipe()
defer reader.Close()
Expand Down
8 changes: 8 additions & 0 deletions libpod/define/volume_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,12 @@ type InspectVolumeData struct {
// volume for a specific container, and will be be removed when any
// container using it is removed.
Anonymous bool `json:"Anonymous,omitempty"`
// MountCount is the number of times this volume has been mounted.
MountCount uint `json:"MountCount"`
// NeedsCopyUp indicates that the next time the volume is mounted into
NeedsCopyUp bool `json:"NeedsCopyUp,omitempty"`
// 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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to test NeedsChown?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test.

}
3 changes: 3 additions & 0 deletions libpod/volume_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ func (v *Volume) Inspect() (*define.InspectVolumeData, error) {
data.UID = v.uid()
data.GID = v.gid()
data.Anonymous = v.config.IsAnon
data.MountCount = v.state.MountCount
data.NeedsCopyUp = v.state.NeedsCopyUp
data.NeedsChown = v.state.NeedsChown

return data, nil
}
40 changes: 40 additions & 0 deletions test/system/160-volumes.bats
Original file line number Diff line number Diff line change
Expand Up @@ -345,4 +345,44 @@ EOF
is "$output" "tmpfs" "volume should be tmpfs"
}

# Named volumes copyup
@test "podman volume create copyup" {
myvolume=myvol$(random_string)
mylabel=$(random_string)

# Create a named volume
run_podman volume create $myvolume
is "$output" "$myvolume" "output from volume create"

# Confirm that it shows up in 'volume ls', and confirm values
run_podman volume ls --format json
tests="
Name | $myvolume
Driver | local
NeedsCopyUp | true
NeedsChown | true
"
parse_table "$tests" | while read field expect; do
actual=$(jq -r ".[0].$field" <<<"$output")
is "$actual" "$expect" "volume ls .$field"
done

run_podman run --rm --volume $myvolume:/vol $IMAGE true
run_podman volume inspect --format '{{ .NeedsCopyUp }}' $myvolume
is "${output}" "true" "If content in dest '/vol' empty NeedsCopyUP should still be true"
run_podman volume inspect --format '{{ .NeedsChown }}' $myvolume
is "${output}" "false" "After first use within a container NeedsChown should still be false"

run_podman run --rm --volume $myvolume:/etc $IMAGE ls /etc/passwd
run_podman volume inspect --format '{{ .NeedsCopyUp }}' $myvolume
is "${output}" "false" "If content in dest '/etc' non-empty NeedsCopyUP should still have happend and be false"

run_podman volume inspect --format '{{.Mountpoint}}' $myvolume
mountpoint="$output"
test -e "$mountpoint/passwd"

# Clean up
run_podman volume rm $myvolume
}

# vim: filetype=sh