From 1fed2b0912757d7652f1ef466c1770fe995ad296 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 14 Jun 2023 19:20:23 +0000 Subject: [PATCH] Restore active mount counts on live-restore When live-restoring a container the volume driver needs be notified that there is an active mount for the volume. Before this change the count is zero until the container stops and the uint64 overflows pretty much making it so the volume can never be removed until another daemon restart. Signed-off-by: Brian Goff (cherry picked from commit 647c2a6cdd86d79230df1bf690d0b6a2930d6db2) Signed-off-by: Bjorn Neergaard Signed-off-by: Sebastiaan van Stijn --- daemon/mounts.go | 12 +++++++++++ daemon/volumes.go | 7 ++++++ integration/daemon/daemon_test.go | 36 +++++++++++++++++++++++++++++++ volume/local/local.go | 28 ++++++++++++++++++++++++ volume/mounts/mounts.go | 29 +++++++++++++++++++++++++ volume/service/service.go | 15 +++++++++++++ volume/service/store.go | 9 ++++++++ volume/volume.go | 10 +++++++++ 8 files changed, 146 insertions(+) diff --git a/daemon/mounts.go b/daemon/mounts.go index 383a38e7ebe2b..ad637df03d073 100644 --- a/daemon/mounts.go +++ b/daemon/mounts.go @@ -5,16 +5,28 @@ import ( "fmt" "strings" + "github.com/containerd/containerd/log" mounttypes "github.com/docker/docker/api/types/mount" "github.com/docker/docker/container" volumesservice "github.com/docker/docker/volume/service" + "github.com/sirupsen/logrus" ) func (daemon *Daemon) prepareMountPoints(container *container.Container) error { + alive := container.IsRunning() for _, config := range container.MountPoints { if err := daemon.lazyInitializeVolume(container.ID, config); err != nil { return err } + if alive { + log.G(context.TODO()).WithFields(logrus.Fields{ + "container": container.ID, + "volume": config.Volume.Name(), + }).Debug("Live-restoring volume for alive container") + if err := config.LiveRestore(context.TODO()); err != nil { + return err + } + } } return nil } diff --git a/daemon/volumes.go b/daemon/volumes.go index 6e17e221c671f..09871e7ebe182 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -21,6 +21,8 @@ import ( "github.com/sirupsen/logrus" ) +var _ volume.LiveRestorer = (*volumeWrapper)(nil) + type mounts []container.Mount // Len returns the number of mounts. Used in sorting. @@ -257,6 +259,7 @@ func (daemon *Daemon) VolumesService() *service.VolumesService { type volumeMounter interface { Mount(ctx context.Context, v *volumetypes.Volume, ref string) (string, error) Unmount(ctx context.Context, v *volumetypes.Volume, ref string) error + LiveRestoreVolume(ctx context.Context, v *volumetypes.Volume, ref string) error } type volumeWrapper struct { @@ -291,3 +294,7 @@ func (v *volumeWrapper) CreatedAt() (time.Time, error) { func (v *volumeWrapper) Status() map[string]interface{} { return v.v.Status } + +func (v *volumeWrapper) LiveRestoreVolume(ctx context.Context, ref string) error { + return v.s.LiveRestoreVolume(ctx, v.v, ref) +} diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index 7ecb2c0aa4793..9dcaed374757b 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -400,6 +400,42 @@ func testLiveRestoreVolumeReferences(t *testing.T) { runTest(t, "on-failure") runTest(t, "no") }) + + // Make sure that the local volume driver's mount ref count is restored + // Addresses https://github.com/moby/moby/issues/44422 + t.Run("local volume with mount options", func(t *testing.T) { + v, err := c.VolumeCreate(ctx, volume.CreateOptions{ + Driver: "local", + Name: "test-live-restore-volume-references-local", + DriverOpts: map[string]string{ + "type": "tmpfs", + "device": "tmpfs", + }, + }) + assert.NilError(t, err) + m := mount.Mount{ + Type: mount.TypeVolume, + Source: v.Name, + Target: "/foo", + } + cID := container.Run(ctx, t, c, container.WithMount(m), container.WithCmd("top")) + defer c.ContainerRemove(ctx, cID, types.ContainerRemoveOptions{Force: true}) + + d.Restart(t, "--live-restore", "--iptables=false") + + // Try to remove the volume + // This should fail since its used by a container + err = c.VolumeRemove(ctx, v.Name, false) + assert.ErrorContains(t, err, "volume is in use") + + // Remove that container which should free the references in the volume + err = c.ContainerRemove(ctx, cID, types.ContainerRemoveOptions{Force: true}) + assert.NilError(t, err) + + // Now we should be able to remove the volume + err = c.VolumeRemove(ctx, v.Name, false) + assert.NilError(t, err) + }) } func TestDaemonDefaultBridgeWithFixedCidrButNoBip(t *testing.T) { diff --git a/volume/local/local.go b/volume/local/local.go index 512e666eb8cf5..f156ea2339bc3 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -4,6 +4,7 @@ package local // import "github.com/docker/docker/volume/local" import ( + "context" "encoding/json" "os" "path/filepath" @@ -11,6 +12,7 @@ import ( "strings" "sync" + "github.com/containerd/containerd/log" "github.com/docker/docker/daemon/names" "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/idtools" @@ -35,6 +37,8 @@ var ( // This name is used to create the bind directory, so we need to avoid characters that // would make the path to escape the root directory. volumeNameRegex = names.RestrictedNamePattern + + _ volume.LiveRestorer = (*localVolume)(nil) ) type activeMount struct { @@ -296,14 +300,17 @@ func (v *localVolume) CachedPath() string { func (v *localVolume) Mount(id string) (string, error) { v.m.Lock() defer v.m.Unlock() + logger := log.G(context.TODO()).WithField("volume", v.name) if v.needsMount() { if !v.active.mounted { + logger.Debug("Mounting volume") if err := v.mount(); err != nil { return "", errdefs.System(err) } v.active.mounted = true } v.active.count++ + logger.WithField("active mounts", v.active).Debug("Decremented active mount count") } if err := v.postMount(); err != nil { return "", err @@ -316,6 +323,7 @@ func (v *localVolume) Mount(id string) (string, error) { func (v *localVolume) Unmount(id string) error { v.m.Lock() defer v.m.Unlock() + logger := log.G(context.TODO()).WithField("volume", v.name) // Always decrement the count, even if the unmount fails // Essentially docker doesn't care if this fails, it will send an error, but @@ -323,12 +331,14 @@ func (v *localVolume) Unmount(id string) error { // this volume can never be removed until a daemon restart occurs. if v.needsMount() { v.active.count-- + logger.WithField("active mounts", v.active).Debug("Decremented active mount count") } if v.active.count > 0 { return nil } + logger.Debug("Unmounting volume") return v.unmount() } @@ -369,6 +379,24 @@ func (v *localVolume) saveOpts() error { return nil } +// LiveRestoreVolume restores reference counts for mounts +// It is assumed that the volume is already mounted since this is only called for active, live-restored containers. +func (v *localVolume) LiveRestoreVolume(ctx context.Context, _ string) error { + v.m.Lock() + defer v.m.Unlock() + + if !v.needsMount() { + return nil + } + v.active.count++ + v.active.mounted = true + log.G(ctx).WithFields(logrus.Fields{ + "volume": v.name, + "active mounts": v.active, + }).Debugf("Live restored volume") + return nil +} + // getAddress finds out address/hostname from options func getAddress(opts string) string { for _, opt := range strings.Split(opts, ",") { diff --git a/volume/mounts/mounts.go b/volume/mounts/mounts.go index c441e51ed9018..bc90bb9def9de 100644 --- a/volume/mounts/mounts.go +++ b/volume/mounts/mounts.go @@ -1,17 +1,20 @@ package mounts // import "github.com/docker/docker/volume/mounts" import ( + "context" "fmt" "os" "path/filepath" "syscall" + "github.com/containerd/containerd/log" mounttypes "github.com/docker/docker/api/types/mount" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/volume" "github.com/opencontainers/selinux/go-selinux/label" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) // MountPoint is the intersection point between a volume and a container. It @@ -164,6 +167,32 @@ func (m *MountPoint) Setup(mountLabel string, rootIDs idtools.Identity, checkFun return m.Source, nil } +func (m *MountPoint) LiveRestore(ctx context.Context) error { + if m.Volume == nil { + logrus.Debug("No volume to restore") + return nil + } + + lrv, ok := m.Volume.(volume.LiveRestorer) + if !ok { + log.G(ctx).WithField("volume", m.Volume.Name()).Debugf("Volume does not support live restore: %T", m.Volume) + return nil + } + + id := m.ID + if id == "" { + id = stringid.GenerateRandomID() + } + + if err := lrv.LiveRestoreVolume(ctx, id); err != nil { + return errors.Wrapf(err, "error while restoring volume '%s'", m.Source) + } + + m.ID = id + m.active++ + return nil +} + // Path returns the path of a volume in a mount point. func (m *MountPoint) Path() string { if m.Volume != nil { diff --git a/volume/service/service.go b/volume/service/service.go index 7030b2a32bed4..a5615fe73191b 100644 --- a/volume/service/service.go +++ b/volume/service/service.go @@ -274,3 +274,18 @@ func (s *VolumesService) List(ctx context.Context, filter filters.Args) (volumes func (s *VolumesService) Shutdown() error { return s.vs.Shutdown() } + +// LiveRestoreVolume passes through the LiveRestoreVolume call to the volume if it is implemented +// otherwise it is a no-op. +func (s *VolumesService) LiveRestoreVolume(ctx context.Context, vol *volumetypes.Volume, ref string) error { + v, err := s.vs.Get(ctx, vol.Name, opts.WithGetDriver(vol.Driver)) + if err != nil { + return err + } + rlv, ok := v.(volume.LiveRestorer) + if !ok { + log.G(ctx).WithField("volume", vol.Name).Debugf("volume does not implement LiveRestoreVolume: %T", v) + return nil + } + return rlv.LiveRestoreVolume(ctx, ref) +} diff --git a/volume/service/store.go b/volume/service/store.go index 8926866e1c4d7..9250c869674c9 100644 --- a/volume/service/store.go +++ b/volume/service/store.go @@ -24,6 +24,8 @@ const ( volumeDataDir = "volumes" ) +var _ volume.LiveRestorer = (*volumeWrapper)(nil) + type volumeWrapper struct { volume.Volume labels map[string]string @@ -67,6 +69,13 @@ func (v volumeWrapper) CachedPath() string { return v.Volume.Path() } +func (v volumeWrapper) LiveRestoreVolume(ctx context.Context, ref string) error { + if vv, ok := v.Volume.(volume.LiveRestorer); ok { + return vv.LiveRestoreVolume(ctx, ref) + } + return nil +} + // StoreOpt sets options for a VolumeStore type StoreOpt func(store *VolumeStore) error diff --git a/volume/volume.go b/volume/volume.go index 61c8243979f11..2dcbdebe16f78 100644 --- a/volume/volume.go +++ b/volume/volume.go @@ -1,6 +1,7 @@ package volume // import "github.com/docker/docker/volume" import ( + "context" "time" ) @@ -60,6 +61,15 @@ type Volume interface { Status() map[string]interface{} } +// LiveRestorer is an optional interface that can be implemented by a volume driver +// It is used to restore any resources that are necessary for a volume to be used by a live-restored container +type LiveRestorer interface { + // LiveRestoreVolume allows a volume driver which implements this interface to restore any necessary resources (such as reference counting) + // This is called only after the daemon is restarted with live-restored containers + // It is called once per live-restored container. + LiveRestoreVolume(_ context.Context, ref string) error +} + // DetailedVolume wraps a Volume with user-defined labels, options, and cluster scope (e.g., `local` or `global`) type DetailedVolume interface { Labels() map[string]string