Merge pull request #32821 from cpuguy83/32613_fix_volspec_backport

Fix issue backporting mount spec to pre-1.13 obj

daemon/daemon.go

@@ -196,11 +196,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 {
@@ -218,7 +219,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"
@@ -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
+// 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 anonymous 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")
+		}
+		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))
+		}
+	}
+}