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 <cpuguy83@gmail.com>
(cherry picked from commit 3cf18596e95c19823387322cb0fc4e324958a341)
Signed-off-by: Brian Goff <cpuguy83@gmail.com>

daemon/daemon.go

@@ -194,8 +194,9 @@ 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")
 			}
 
 			if c.IsRunning() || c.IsPaused() {
@@ -215,7 +216,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)
 					}

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 <name>:/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 <name>:/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"))
 	}

daemon/volumes.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 
 	"github.com/Sirupsen/logrus"
@@ -113,6 +114,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),
@@ -243,48 +245,125 @@ 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
+// 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()
+
+	maybeUpdate := make(map[string]bool)
+	for _, mp := range container.MountPoints {
+		if mp.Spec.Source != "" && mp.Type != "" {
+			continue
 		}
-		if m.Name != "" {
-			m.Type = mounttypes.TypeVolume
-			m.Spec.Type = mounttypes.TypeVolume
+		maybeUpdate[mp.Destination] = true
+	}
+	if len(maybeUpdate) == 0 {
+		return
+	}
 
-			// make sure this is not an anyonmous volume before setting the spec source
-			if _, exists := container.Config.Volumes[target]; !exists {
-				m.Spec.Source = m.Name
+	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")
+			continue
+		}
+		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
 			}
-			if container.HostConfig.VolumeDriver != "" {
-				m.Spec.VolumeOptions = &mounttypes.VolumeOptions{
-					DriverConfig: &mounttypes.Driver{Name: container.HostConfig.VolumeDriver},
+			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 strings.Contains(m.Mode, "nocopy") {
-				if m.Spec.VolumeOptions == nil {
-					m.Spec.VolumeOptions = &mounttypes.VolumeOptions{}
-				}
-				m.Spec.VolumeOptions.NoCopy = true
+
+			if cm.Type != "" {
+				// probably specified via the hostconfig.Mounts
+				continue
 			}
+
+			// anon volume
+			cm.Type = mounttypes.TypeVolume
+			cm.Spec.Type = mounttypes.TypeVolume
 		} 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,
+			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,
 				}
 			}
 		}
 
-		m.Spec.Target = m.Destination
-		if !m.RW {
-			m.Spec.ReadOnly = true
-		}
+		cm.Spec.Target = cm.Destination
+		cm.Spec.ReadOnly = !cm.RW
 	}
-	return container.ToDiskLocking()
 }
 
 func (daemon *Daemon) traverseLocalVolumes(fn func(volume.Volume) error) error {

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))
+		}
+	}
+}