Merge pull request #46366 from thaJeztah/24.0_backport_volume-local-restore-mounted-status

[24.0 backport] volume/local: Don't unmount, restore mounted status

integration/daemon/daemon_test.go

@@ -1,14 +1,17 @@
 package daemon // import "github.com/docker/docker/integration/daemon"
 
 import (
+	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"syscall"
 	"testing"
 
@@ -16,7 +19,9 @@ import (
 	"github.com/docker/docker/api/types/mount"
 	"github.com/docker/docker/api/types/volume"
 	"github.com/docker/docker/daemon/config"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/integration/internal/container"
+	"github.com/docker/docker/pkg/stdcopy"
 	"github.com/docker/docker/testutil/daemon"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -431,9 +436,28 @@ func testLiveRestoreVolumeReferences(t *testing.T) {
 			Source: v.Name,
 			Target: "/foo",
 		}
-		cID := container.Run(ctx, t, c, container.WithMount(m), container.WithCmd("top"))
+
+		const testContent = "hello"
+		cID := container.Run(ctx, t, c, container.WithMount(m), container.WithCmd("sh", "-c", "echo "+testContent+">>/foo/test.txt; sleep infinity"))
 		defer c.ContainerRemove(ctx, cID, types.ContainerRemoveOptions{Force: true})
 
+		// Wait until container creates a file in the volume.
+		poll.WaitOn(t, func(t poll.LogT) poll.Result {
+			stat, err := c.ContainerStatPath(ctx, cID, "/foo/test.txt")
+			if err != nil {
+				if errdefs.IsNotFound(err) {
+					return poll.Continue("file doesn't yet exist")
+				}
+				return poll.Error(err)
+			}
+
+			if int(stat.Size) != len(testContent)+1 {
+				return poll.Error(fmt.Errorf("unexpected test file size: %d", stat.Size))
+			}
+
+			return poll.Success()
+		})
+
 		d.Restart(t, "--live-restore", "--iptables=false")
 
 		// Try to remove the volume
@@ -441,6 +465,32 @@ func testLiveRestoreVolumeReferences(t *testing.T) {
 		err = c.VolumeRemove(ctx, v.Name, false)
 		assert.ErrorContains(t, err, "volume is in use")
 
+		t.Run("volume still mounted", func(t *testing.T) {
+			skip.If(t, testEnv.IsRootless(), "restarted rootless daemon has a new mount namespace and it won't have the previous mounts")
+
+			// Check if a new container with the same volume has access to the previous content.
+			// This fails if the volume gets unmounted at startup.
+			cID2 := container.Run(ctx, t, c, container.WithMount(m), container.WithCmd("cat", "/foo/test.txt"))
+			defer c.ContainerRemove(ctx, cID2, types.ContainerRemoveOptions{Force: true})
+
+			poll.WaitOn(t, container.IsStopped(ctx, c, cID2))
+
+			inspect, err := c.ContainerInspect(ctx, cID2)
+			if assert.Check(t, err) {
+				assert.Check(t, is.Equal(inspect.State.ExitCode, 0), "volume doesn't have the same file")
+			}
+
+			logs, err := c.ContainerLogs(ctx, cID2, types.ContainerLogsOptions{ShowStdout: true})
+			assert.NilError(t, err)
+			defer logs.Close()
+
+			var stdoutBuf bytes.Buffer
+			_, err = stdcopy.StdCopy(&stdoutBuf, io.Discard, logs)
+			assert.NilError(t, err)
+
+			assert.Check(t, is.Equal(strings.TrimSpace(stdoutBuf.String()), testContent))
+		})
+
 		// Remove that container which should free the references in the volume
 		err = c.ContainerRemove(ctx, cID, types.ContainerRemoveOptions{Force: true})
 		assert.NilError(t, err)

volume/local/local.go

@@ -83,13 +83,18 @@ func New(scope string, rootIdentity idtools.Identity) (*Root, error) {
 			quotaCtl:   r.quotaCtl,
 		}
 
-		// unmount anything that may still be mounted (for example, from an
-		// unclean shutdown). This is a no-op on windows
-		unmount(v.path)
-
 		if err := v.loadOpts(); err != nil {
 			return nil, err
 		}
+
+		if err := v.restoreIfMounted(); err != nil {
+			log.G(context.TODO()).WithFields(log.Fields{
+				"volume": v.name,
+				"path":   v.path,
+				"error":  err,
+			}).Warn("restoreIfMounted failed")
+		}
+
 		r.volumes[name] = v
 	}
 
@@ -338,6 +343,10 @@ func (v *localVolume) Unmount(id string) error {
 		return nil
 	}
 
+	if !v.active.mounted {
+		return nil
+	}
+
 	logger.Debug("Unmounting volume")
 	return v.unmount()
 }

volume/local/local_unix.go

@@ -99,10 +99,6 @@ func (v *localVolume) setOpts(opts map[string]string) error {
 	return v.saveOpts()
 }
 
-func unmount(path string) {
-	_ = mount.Unmount(path)
-}
-
 func (v *localVolume) needsMount() bool {
 	if v.opts == nil {
 		return false
@@ -163,6 +159,29 @@ func (v *localVolume) unmount() error {
 	return nil
 }
 
+// restoreIfMounted restores the mounted status if the _data directory is already mounted.
+func (v *localVolume) restoreIfMounted() error {
+	if v.needsMount() {
+		// Check if the _data directory is already mounted.
+		mounted, err := mountinfo.Mounted(v.path)
+		if err != nil {
+			return fmt.Errorf("failed to determine if volume _data path is already mounted: %w", err)
+		}
+
+		if mounted {
+			// Mark volume as mounted, but don't increment active count. If
+			// any container needs this, the refcount will be incremented
+			// by the live-restore (if enabled).
+			// In other case, refcount will be zero but the volume will
+			// already be considered as mounted when Mount is called, and
+			// only the refcount will be incremented.
+			v.active.mounted = true
+		}
+	}
+
+	return nil
+}
+
 func (v *localVolume) CreatedAt() (time.Time, error) {
 	fileInfo, err := os.Stat(v.rootPath)
 	if err != nil {

volume/local/local_windows.go

@@ -43,6 +43,11 @@ func (v *localVolume) postMount() error {
 	return nil
 }
 
+// restoreIfMounted is a no-op on Windows (because mounts are not supported).
+func (v *localVolume) restoreIfMounted() error {
+	return nil
+}
+
 func (v *localVolume) CreatedAt() (time.Time, error) {
 	fileInfo, err := os.Stat(v.rootPath)
 	if err != nil {