From 647c2a6cdd86d79230df1bf690d0b6a2930d6db2 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 --- daemon/mounts.go | 12 +++++++++++ daemon/volumes.go | 7 ++++++ integration/daemon/daemon_test.go | 36 +++++++++++++++++++++++++++++++ volume/local/local.go | 27 +++++++++++++++++++++++ volume/mounts/mounts.go | 29 +++++++++++++++++++++++++ volume/service/service.go | 15 +++++++++++++ volume/service/store.go | 9 ++++++++ volume/volume.go | 10 +++++++++ 8 files changed, 145 insertions(+) diff --git a/daemon/mounts.go b/daemon/mounts.go index 383a38e7eb..ad637df03d 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 9d1ebee19c..a0667843f1 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -21,6 +21,8 @@ import ( "github.com/pkg/errors" ) +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 7ecb2c0aa4..9dcaed3747 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 306e974e57..d3dcc59947 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -19,6 +19,7 @@ import ( "github.com/docker/docker/quota" "github.com/docker/docker/volume" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) const ( @@ -36,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 { @@ -297,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 @@ -317,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 @@ -324,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() } @@ -370,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 c441e51ed9..bc90bb9def 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 cfac31823b..13cadf5376 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 a7b8a707d9..9ff8f53235 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 61c8243979..2dcbdebe16 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