From d2c5696e29fcbf2c1d953ca7aca6351a57ba5bcc Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 25 Apr 2017 13:05:21 -0400 Subject: [PATCH] Fix issue backporting mount spec to pre-1.13 obj In some cases a mount spec would not be properly backported which could lead to accidental removal of the underlying volume on container remove (which should never happen with named volumes). Adds unit tests for this as well. Unfortunately I had to add a daemon depdency for the backport function due to looking up `VolumesFrom` specs. Signed-off-by: Brian Goff Upstream-commit: 3cf18596e95c19823387322cb0fc4e324958a341 Component: engine --- components/engine/daemon/daemon.go | 8 +- components/engine/daemon/mounts.go | 37 +-- components/engine/daemon/volumes.go | 156 ++++++++--- components/engine/daemon/volumes_unix_test.go | 259 ++++++++++++++++++ 4 files changed, 401 insertions(+), 59 deletions(-) create mode 100644 components/engine/daemon/volumes_unix_test.go diff --git a/components/engine/daemon/daemon.go b/components/engine/daemon/daemon.go index 35ff2a66ef..1082e76404 100644 --- a/components/engine/daemon/daemon.go +++ b/components/engine/daemon/daemon.go @@ -195,11 +195,12 @@ func (daemon *Daemon) restore() error { wg.Add(1) go func(c *container.Container) { defer wg.Done() - if err := backportMountSpec(c); err != nil { - logrus.Error("Failed to migrate old mounts to use new spec format") + daemon.backportMountSpec(c) + if err := c.ToDiskLocking(); err != nil { + logrus.WithError(err).WithField("container", c.ID).Error("error saving backported mountspec to disk") } - daemon.setStateCounter(c) + daemon.setStateCounter(c) if c.IsRunning() || c.IsPaused() { c.RestartManager().Cancel() // manually start containers because some need to wait for swarm networking if err := daemon.containerd.Restore(c.ID, c.InitializeStdio); err != nil { @@ -217,7 +218,6 @@ func (daemon *Daemon) restore() error { // The error is only logged here. logrus.Warnf("Failed to mount container on getting BaseFs path %v: %v", c.ID, err) } else { - // if mount success, then unmount it if err := daemon.Unmount(c); err != nil { logrus.Warnf("Failed to umount container on getting BaseFs path %v: %v", c.ID, err) } diff --git a/components/engine/daemon/mounts.go b/components/engine/daemon/mounts.go index 1c11f86a80..35c6ed59a6 100644 --- a/components/engine/daemon/mounts.go +++ b/components/engine/daemon/mounts.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + mounttypes "github.com/docker/docker/api/types/mount" "github.com/docker/docker/container" volumestore "github.com/docker/docker/volume/store" ) @@ -20,27 +21,31 @@ func (daemon *Daemon) prepareMountPoints(container *container.Container) error { func (daemon *Daemon) removeMountPoints(container *container.Container, rm bool) error { var rmErrors []string for _, m := range container.MountPoints { - if m.Volume == nil { + if m.Type != mounttypes.TypeVolume || m.Volume == nil { continue } daemon.volumes.Dereference(m.Volume, container.ID) - if rm { - // Do not remove named mountpoints - // these are mountpoints specified like `docker run -v :/foo` - if m.Spec.Source != "" { - continue - } - err := daemon.volumes.Remove(m.Volume) - // Ignore volume in use errors because having this - // volume being referenced by other container is - // not an error, but an implementation detail. - // This prevents docker from logging "ERROR: Volume in use" - // where there is another container using the volume. - if err != nil && !volumestore.IsInUse(err) { - rmErrors = append(rmErrors, err.Error()) - } + if !rm { + continue + } + + // Do not remove named mountpoints + // these are mountpoints specified like `docker run -v :/foo` + if m.Spec.Source != "" { + continue + } + + err := daemon.volumes.Remove(m.Volume) + // Ignore volume in use errors because having this + // volume being referenced by other container is + // not an error, but an implementation detail. + // This prevents docker from logging "ERROR: Volume in use" + // where there is another container using the volume. + if err != nil && !volumestore.IsInUse(err) { + rmErrors = append(rmErrors, err.Error()) } } + if len(rmErrors) > 0 { return fmt.Errorf("Error removing volumes:\n%v", strings.Join(rmErrors, "\n")) } diff --git a/components/engine/daemon/volumes.go b/components/engine/daemon/volumes.go index 9f0468e1a4..a01e1369bb 100644 --- a/components/engine/daemon/volumes.go +++ b/components/engine/daemon/volumes.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "strings" "github.com/Sirupsen/logrus" @@ -112,6 +113,7 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo for _, m := range c.MountPoints { cp := &volume.MountPoint{ + Type: m.Type, Name: m.Name, Source: m.Source, RW: m.RW && volume.ReadWrite(mode), @@ -239,48 +241,124 @@ func (daemon *Daemon) lazyInitializeVolume(containerID string, m *volume.MountPo return nil } -func backportMountSpec(container *container.Container) error { - for target, m := range container.MountPoints { - if m.Spec.Type != "" { - // if type is set on even one mount, no need to migrate - return nil - } - if m.Name != "" { - m.Type = mounttypes.TypeVolume - m.Spec.Type = mounttypes.TypeVolume +// backportMountSpec resolves mount specs (introduced in 1.13) from pre-1.13 +// mount configurations +// The container lock should not be held when calling this function. +// Changes are only made in-memory and may make changes to containers referenced +// by `container.HostConfig.VolumesFrom` +func (daemon *Daemon) backportMountSpec(container *container.Container) { + container.Lock() + defer container.Unlock() - // make sure this is not an anonymous volume before setting the spec source - if _, exists := container.Config.Volumes[target]; !exists { - m.Spec.Source = m.Name - } - if container.HostConfig.VolumeDriver != "" { - m.Spec.VolumeOptions = &mounttypes.VolumeOptions{ - DriverConfig: &mounttypes.Driver{Name: container.HostConfig.VolumeDriver}, - } - } - if strings.Contains(m.Mode, "nocopy") { - if m.Spec.VolumeOptions == nil { - m.Spec.VolumeOptions = &mounttypes.VolumeOptions{} - } - m.Spec.VolumeOptions.NoCopy = true - } - } else { - m.Type = mounttypes.TypeBind - m.Spec.Type = mounttypes.TypeBind - m.Spec.Source = m.Source - if m.Propagation != "" { - m.Spec.BindOptions = &mounttypes.BindOptions{ - Propagation: m.Propagation, - } - } - } - - m.Spec.Target = m.Destination - if !m.RW { - m.Spec.ReadOnly = true + maybeUpdate := make(map[string]bool) + for _, mp := range container.MountPoints { + if mp.Spec.Source != "" && mp.Type != "" { + continue } + maybeUpdate[mp.Destination] = true + } + if len(maybeUpdate) == 0 { + return + } + + mountSpecs := make(map[string]bool, len(container.HostConfig.Mounts)) + for _, m := range container.HostConfig.Mounts { + mountSpecs[m.Target] = true + } + + binds := make(map[string]*volume.MountPoint, len(container.HostConfig.Binds)) + for _, rawSpec := range container.HostConfig.Binds { + mp, err := volume.ParseMountRaw(rawSpec, container.HostConfig.VolumeDriver) + if err != nil { + logrus.WithError(err).Error("Got unexpected error while re-parsing raw volume spec during spec backport") + continue + } + binds[mp.Destination] = mp + } + + volumesFrom := make(map[string]volume.MountPoint) + for _, fromSpec := range container.HostConfig.VolumesFrom { + from, _, err := volume.ParseVolumesFrom(fromSpec) + if err != nil { + logrus.WithError(err).WithField("id", container.ID).Error("Error reading volumes-from spec during mount spec backport") + } + fromC, err := daemon.GetContainer(from) + if err != nil { + logrus.WithError(err).WithField("from-container", from).Error("Error looking up volumes-from container") + continue + } + + // make sure from container's specs have been backported + daemon.backportMountSpec(fromC) + + fromC.Lock() + for t, mp := range fromC.MountPoints { + volumesFrom[t] = *mp + } + fromC.Unlock() + } + + needsUpdate := func(containerMount, other *volume.MountPoint) bool { + if containerMount.Type != other.Type || !reflect.DeepEqual(containerMount.Spec, other.Spec) { + return true + } + return false + } + + // main + for _, cm := range container.MountPoints { + if !maybeUpdate[cm.Destination] { + continue + } + // nothing to backport if from hostconfig.Mounts + if mountSpecs[cm.Destination] { + continue + } + + if mp, exists := binds[cm.Destination]; exists { + if needsUpdate(cm, mp) { + cm.Spec = mp.Spec + cm.Type = mp.Type + } + continue + } + + if cm.Name != "" { + if mp, exists := volumesFrom[cm.Destination]; exists { + if needsUpdate(cm, &mp) { + cm.Spec = mp.Spec + cm.Type = mp.Type + } + continue + } + + if cm.Type != "" { + // probably specified via the hostconfig.Mounts + continue + } + + // anon volume + cm.Type = mounttypes.TypeVolume + cm.Spec.Type = mounttypes.TypeVolume + } else { + if cm.Type != "" { + // already updated + continue + } + + cm.Type = mounttypes.TypeBind + cm.Spec.Type = mounttypes.TypeBind + cm.Spec.Source = cm.Source + if cm.Propagation != "" { + cm.Spec.BindOptions = &mounttypes.BindOptions{ + Propagation: cm.Propagation, + } + } + } + + cm.Spec.Target = cm.Destination + cm.Spec.ReadOnly = !cm.RW } - return container.ToDiskLocking() } func (daemon *Daemon) traverseLocalVolumes(fn func(volume.Volume) error) error { diff --git a/components/engine/daemon/volumes_unix_test.go b/components/engine/daemon/volumes_unix_test.go new file mode 100644 index 0000000000..4be7719fe7 --- /dev/null +++ b/components/engine/daemon/volumes_unix_test.go @@ -0,0 +1,259 @@ +// +build !windows + +package daemon + +import ( + "encoding/json" + "fmt" + "reflect" + "testing" + + containertypes "github.com/docker/docker/api/types/container" + mounttypes "github.com/docker/docker/api/types/mount" + "github.com/docker/docker/container" + "github.com/docker/docker/volume" +) + +func TestBackportMountSpec(t *testing.T) { + d := Daemon{containers: container.NewMemoryStore()} + + c := &container.Container{ + CommonContainer: container.CommonContainer{ + State: &container.State{}, + MountPoints: map[string]*volume.MountPoint{ + "/apple": {Destination: "/apple", Source: "/var/lib/docker/volumes/12345678", Name: "12345678", RW: true, CopyData: true}, // anonymous volume + "/banana": {Destination: "/banana", Source: "/var/lib/docker/volumes/data", Name: "data", RW: true, CopyData: true}, // named volume + "/cherry": {Destination: "/cherry", Source: "/var/lib/docker/volumes/data", Name: "data", CopyData: true}, // RO named volume + "/dates": {Destination: "/dates", Source: "/var/lib/docker/volumes/data", Name: "data"}, // named volume nocopy + "/elderberry": {Destination: "/elderberry", Source: "/var/lib/docker/volumes/data", Name: "data"}, // masks anon vol + "/fig": {Destination: "/fig", Source: "/data", RW: true}, // RW bind + "/guava": {Destination: "/guava", Source: "/data", RW: false, Propagation: "shared"}, // RO bind + propagation + "/kumquat": {Destination: "/kumquat", Name: "data", RW: false, CopyData: true}, // volumes-from + + // partially configured mountpoint due to #32613 + // specifically, `mp.Spec.Source` is not set + "/honeydew": { + Type: mounttypes.TypeVolume, + Destination: "/honeydew", + Name: "data", + Source: "/var/lib/docker/volumes/data", + Spec: mounttypes.Mount{Type: mounttypes.TypeVolume, Target: "/honeydew", VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true}}, + }, + + // from hostconfig.Mounts + "/jambolan": { + Type: mounttypes.TypeVolume, + Destination: "/jambolan", + Source: "/var/lib/docker/volumes/data", + RW: true, + Name: "data", + Spec: mounttypes.Mount{Type: mounttypes.TypeVolume, Target: "/jambolan", Source: "data"}, + }, + }, + HostConfig: &containertypes.HostConfig{ + Binds: []string{ + "data:/banana", + "data:/cherry:ro", + "data:/dates:ro,nocopy", + "data:/elderberry:ro,nocopy", + "/data:/fig", + "/data:/guava:ro,shared", + "data:/honeydew:nocopy", + }, + VolumesFrom: []string{"1:ro"}, + Mounts: []mounttypes.Mount{ + {Type: mounttypes.TypeVolume, Target: "/jambolan"}, + }, + }, + Config: &containertypes.Config{Volumes: map[string]struct{}{ + "/apple": {}, + "/elderberry": {}, + }}, + }} + + d.containers.Add("1", &container.Container{ + CommonContainer: container.CommonContainer{ + State: &container.State{}, + ID: "1", + MountPoints: map[string]*volume.MountPoint{ + "/kumquat": {Destination: "/kumquat", Name: "data", RW: false, CopyData: true}, + }, + HostConfig: &containertypes.HostConfig{ + Binds: []string{ + "data:/kumquat:ro", + }, + }, + }, + }) + + type expected struct { + mp *volume.MountPoint + comment string + } + + pretty := func(mp *volume.MountPoint) string { + b, err := json.MarshalIndent(mp, "\t", " ") + if err != nil { + return fmt.Sprintf("%#v", mp) + } + return string(b) + } + + for _, x := range []expected{ + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/apple", + RW: true, + Name: "12345678", + Source: "/var/lib/docker/volumes/12345678", + CopyData: true, + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "", + Target: "/apple", + }, + }, + comment: "anonymous volume", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/banana", + RW: true, + Name: "data", + Source: "/var/lib/docker/volumes/data", + CopyData: true, + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "data", + Target: "/banana", + }, + }, + comment: "named volume", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/cherry", + Name: "data", + Source: "/var/lib/docker/volumes/data", + CopyData: true, + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "data", + Target: "/cherry", + ReadOnly: true, + }, + }, + comment: "read-only named volume", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/dates", + Name: "data", + Source: "/var/lib/docker/volumes/data", + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "data", + Target: "/dates", + ReadOnly: true, + VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true}, + }, + }, + comment: "named volume with nocopy", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/elderberry", + Name: "data", + Source: "/var/lib/docker/volumes/data", + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "data", + Target: "/elderberry", + ReadOnly: true, + VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true}, + }, + }, + comment: "masks an anonymous volume", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeBind, + Destination: "/fig", + Source: "/data", + RW: true, + Spec: mounttypes.Mount{ + Type: mounttypes.TypeBind, + Source: "/data", + Target: "/fig", + }, + }, + comment: "bind mount with read/write", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeBind, + Destination: "/guava", + Source: "/data", + RW: false, + Propagation: "shared", + Spec: mounttypes.Mount{ + Type: mounttypes.TypeBind, + Source: "/data", + Target: "/guava", + ReadOnly: true, + BindOptions: &mounttypes.BindOptions{Propagation: "shared"}, + }, + }, + comment: "bind mount with read/write + shared propgation", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/honeydew", + Source: "/var/lib/docker/volumes/data", + RW: true, + Propagation: "shared", + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "data", + Target: "/honeydew", + VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true}, + }, + }, + comment: "partially configured named volume caused by #32613", + }, + { + mp: &(*c.MountPoints["/jambolan"]), // copy the mountpoint, expect no changes + comment: "volume defined in mounts API", + }, + { + mp: &volume.MountPoint{ + Type: mounttypes.TypeVolume, + Destination: "/kumquat", + Source: "/var/lib/docker/volumes/data", + RW: false, + Name: "data", + Spec: mounttypes.Mount{ + Type: mounttypes.TypeVolume, + Source: "data", + Target: "/kumquat", + ReadOnly: true, + }, + }, + comment: "partially configured named volume caused by #32613", + }, + } { + + mp := c.MountPoints[x.mp.Destination] + d.backportMountSpec(c) + + if !reflect.DeepEqual(mp.Spec, x.mp.Spec) { + t.Fatalf("%s\nexpected:\n\t%s\n\ngot:\n\t%s", x.comment, pretty(x.mp), pretty(mp)) + } + } +}