From aef703fa1b85faffb6c13db8860b3deecbb9de2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 17 Aug 2023 16:44:22 +0200 Subject: [PATCH 1/2] integration/liveRestore: Check volume content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure that the content in the live-restored volume mounted in a new container is the same as the content in the old container. This checks if volume's _data directory doesn't get unmounted on startup. Signed-off-by: Paweł Gronowski --- integration/daemon/daemon_test.go | 52 ++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index b2821147c9..21632572a7 100644 --- a/integration/daemon/daemon_test.go +++ b/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" @@ -17,7 +20,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" @@ -432,9 +437,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 @@ -442,6 +466,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) From 26894844026d20938ec84ea22448172d3af9b652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 17 Aug 2023 16:44:28 +0200 Subject: [PATCH 2/2] volume/local: Don't unmount, restore mounted status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On startup all local volumes were unmounted as a cleanup mechanism for the non-clean exit of the last engine process. This caused live-restored volumes that used special volume opt mount flags to be broken. While the refcount was restored, the _data directory was just unmounted, so all new containers mounting this volume would just have the access to the empty _data directory instead of the real volume. With this patch, the mountpoint isn't unmounted. Instead, if the volume is already mounted, just mark it as mounted, so the next time Mount is called only the ref count is incremented, but no second attempt to mount it is performed. Signed-off-by: Paweł Gronowski --- volume/local/local.go | 17 +++++++++++++---- volume/local/local_unix.go | 27 +++++++++++++++++++++++---- volume/local/local_windows.go | 5 +++++ 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/volume/local/local.go b/volume/local/local.go index fc0dd7a8c4..d4e58c7976 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -82,13 +82,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 } @@ -337,6 +342,10 @@ func (v *localVolume) Unmount(id string) error { return nil } + if !v.active.mounted { + return nil + } + logger.Debug("Unmounting volume") return v.unmount() } diff --git a/volume/local/local_unix.go b/volume/local/local_unix.go index c116eceb88..05bf89f36a 100644 --- a/volume/local/local_unix.go +++ b/volume/local/local_unix.go @@ -98,10 +98,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 @@ -162,6 +158,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 { diff --git a/volume/local/local_windows.go b/volume/local/local_windows.go index 9eda81b514..78fdd45ccd 100644 --- a/volume/local/local_windows.go +++ b/volume/local/local_windows.go @@ -44,6 +44,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 {