Browse code

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>

Brian Goff authored on 2017/04/26 02:05:21
Showing 4 changed files
... ...
@@ -195,11 +195,12 @@ func (daemon *Daemon) restore() error {
195 195
 		wg.Add(1)
196 196
 		go func(c *container.Container) {
197 197
 			defer wg.Done()
198
-			if err := backportMountSpec(c); err != nil {
199
-				logrus.Error("Failed to migrate old mounts to use new spec format")
198
+			daemon.backportMountSpec(c)
199
+			if err := c.ToDiskLocking(); err != nil {
200
+				logrus.WithError(err).WithField("container", c.ID).Error("error saving backported mountspec to disk")
200 201
 			}
201
-			daemon.setStateCounter(c)
202 202
 
203
+			daemon.setStateCounter(c)
203 204
 			if c.IsRunning() || c.IsPaused() {
204 205
 				c.RestartManager().Cancel() // manually start containers because some need to wait for swarm networking
205 206
 				if err := daemon.containerd.Restore(c.ID, c.InitializeStdio); err != nil {
... ...
@@ -217,7 +218,6 @@ func (daemon *Daemon) restore() error {
217 217
 					// The error is only logged here.
218 218
 					logrus.Warnf("Failed to mount container on getting BaseFs path %v: %v", c.ID, err)
219 219
 				} else {
220
-					// if mount success, then unmount it
221 220
 					if err := daemon.Unmount(c); err != nil {
222 221
 						logrus.Warnf("Failed to umount container on getting BaseFs path %v: %v", c.ID, err)
223 222
 					}
... ...
@@ -4,6 +4,7 @@ import (
4 4
 	"fmt"
5 5
 	"strings"
6 6
 
7
+	mounttypes "github.com/docker/docker/api/types/mount"
7 8
 	"github.com/docker/docker/container"
8 9
 	volumestore "github.com/docker/docker/volume/store"
9 10
 )
... ...
@@ -20,27 +21,31 @@ func (daemon *Daemon) prepareMountPoints(container *container.Container) error {
20 20
 func (daemon *Daemon) removeMountPoints(container *container.Container, rm bool) error {
21 21
 	var rmErrors []string
22 22
 	for _, m := range container.MountPoints {
23
-		if m.Volume == nil {
23
+		if m.Type != mounttypes.TypeVolume || m.Volume == nil {
24 24
 			continue
25 25
 		}
26 26
 		daemon.volumes.Dereference(m.Volume, container.ID)
27
-		if rm {
28
-			// Do not remove named mountpoints
29
-			// these are mountpoints specified like `docker run -v <name>:/foo`
30
-			if m.Spec.Source != "" {
31
-				continue
32
-			}
33
-			err := daemon.volumes.Remove(m.Volume)
34
-			// Ignore volume in use errors because having this
35
-			// volume being referenced by other container is
36
-			// not an error, but an implementation detail.
37
-			// This prevents docker from logging "ERROR: Volume in use"
38
-			// where there is another container using the volume.
39
-			if err != nil && !volumestore.IsInUse(err) {
40
-				rmErrors = append(rmErrors, err.Error())
41
-			}
27
+		if !rm {
28
+			continue
29
+		}
30
+
31
+		// Do not remove named mountpoints
32
+		// these are mountpoints specified like `docker run -v <name>:/foo`
33
+		if m.Spec.Source != "" {
34
+			continue
35
+		}
36
+
37
+		err := daemon.volumes.Remove(m.Volume)
38
+		// Ignore volume in use errors because having this
39
+		// volume being referenced by other container is
40
+		// not an error, but an implementation detail.
41
+		// This prevents docker from logging "ERROR: Volume in use"
42
+		// where there is another container using the volume.
43
+		if err != nil && !volumestore.IsInUse(err) {
44
+			rmErrors = append(rmErrors, err.Error())
42 45
 		}
43 46
 	}
47
+
44 48
 	if len(rmErrors) > 0 {
45 49
 		return fmt.Errorf("Error removing volumes:\n%v", strings.Join(rmErrors, "\n"))
46 50
 	}
... ...
@@ -5,6 +5,7 @@ import (
5 5
 	"fmt"
6 6
 	"os"
7 7
 	"path/filepath"
8
+	"reflect"
8 9
 	"strings"
9 10
 
10 11
 	"github.com/Sirupsen/logrus"
... ...
@@ -112,6 +113,7 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
112 112
 
113 113
 		for _, m := range c.MountPoints {
114 114
 			cp := &volume.MountPoint{
115
+				Type:        m.Type,
115 116
 				Name:        m.Name,
116 117
 				Source:      m.Source,
117 118
 				RW:          m.RW && volume.ReadWrite(mode),
... ...
@@ -239,48 +241,124 @@ func (daemon *Daemon) lazyInitializeVolume(containerID string, m *volume.MountPo
239 239
 	return nil
240 240
 }
241 241
 
242
-func backportMountSpec(container *container.Container) error {
243
-	for target, m := range container.MountPoints {
244
-		if m.Spec.Type != "" {
245
-			// if type is set on even one mount, no need to migrate
246
-			return nil
242
+// backportMountSpec resolves mount specs (introduced in 1.13) from pre-1.13
243
+// mount configurations
244
+// The container lock should not be held when calling this function.
245
+// Changes are only made in-memory and may make changes to containers referenced
246
+// by `container.HostConfig.VolumesFrom`
247
+func (daemon *Daemon) backportMountSpec(container *container.Container) {
248
+	container.Lock()
249
+	defer container.Unlock()
250
+
251
+	maybeUpdate := make(map[string]bool)
252
+	for _, mp := range container.MountPoints {
253
+		if mp.Spec.Source != "" && mp.Type != "" {
254
+			continue
247 255
 		}
248
-		if m.Name != "" {
249
-			m.Type = mounttypes.TypeVolume
250
-			m.Spec.Type = mounttypes.TypeVolume
256
+		maybeUpdate[mp.Destination] = true
257
+	}
258
+	if len(maybeUpdate) == 0 {
259
+		return
260
+	}
251 261
 
252
-			// make sure this is not an anonymous volume before setting the spec source
253
-			if _, exists := container.Config.Volumes[target]; !exists {
254
-				m.Spec.Source = m.Name
262
+	mountSpecs := make(map[string]bool, len(container.HostConfig.Mounts))
263
+	for _, m := range container.HostConfig.Mounts {
264
+		mountSpecs[m.Target] = true
265
+	}
266
+
267
+	binds := make(map[string]*volume.MountPoint, len(container.HostConfig.Binds))
268
+	for _, rawSpec := range container.HostConfig.Binds {
269
+		mp, err := volume.ParseMountRaw(rawSpec, container.HostConfig.VolumeDriver)
270
+		if err != nil {
271
+			logrus.WithError(err).Error("Got unexpected error while re-parsing raw volume spec during spec backport")
272
+			continue
273
+		}
274
+		binds[mp.Destination] = mp
275
+	}
276
+
277
+	volumesFrom := make(map[string]volume.MountPoint)
278
+	for _, fromSpec := range container.HostConfig.VolumesFrom {
279
+		from, _, err := volume.ParseVolumesFrom(fromSpec)
280
+		if err != nil {
281
+			logrus.WithError(err).WithField("id", container.ID).Error("Error reading volumes-from spec during mount spec backport")
282
+		}
283
+		fromC, err := daemon.GetContainer(from)
284
+		if err != nil {
285
+			logrus.WithError(err).WithField("from-container", from).Error("Error looking up volumes-from container")
286
+			continue
287
+		}
288
+
289
+		// make sure from container's specs have been backported
290
+		daemon.backportMountSpec(fromC)
291
+
292
+		fromC.Lock()
293
+		for t, mp := range fromC.MountPoints {
294
+			volumesFrom[t] = *mp
295
+		}
296
+		fromC.Unlock()
297
+	}
298
+
299
+	needsUpdate := func(containerMount, other *volume.MountPoint) bool {
300
+		if containerMount.Type != other.Type || !reflect.DeepEqual(containerMount.Spec, other.Spec) {
301
+			return true
302
+		}
303
+		return false
304
+	}
305
+
306
+	// main
307
+	for _, cm := range container.MountPoints {
308
+		if !maybeUpdate[cm.Destination] {
309
+			continue
310
+		}
311
+		// nothing to backport if from hostconfig.Mounts
312
+		if mountSpecs[cm.Destination] {
313
+			continue
314
+		}
315
+
316
+		if mp, exists := binds[cm.Destination]; exists {
317
+			if needsUpdate(cm, mp) {
318
+				cm.Spec = mp.Spec
319
+				cm.Type = mp.Type
255 320
 			}
256
-			if container.HostConfig.VolumeDriver != "" {
257
-				m.Spec.VolumeOptions = &mounttypes.VolumeOptions{
258
-					DriverConfig: &mounttypes.Driver{Name: container.HostConfig.VolumeDriver},
321
+			continue
322
+		}
323
+
324
+		if cm.Name != "" {
325
+			if mp, exists := volumesFrom[cm.Destination]; exists {
326
+				if needsUpdate(cm, &mp) {
327
+					cm.Spec = mp.Spec
328
+					cm.Type = mp.Type
259 329
 				}
330
+				continue
260 331
 			}
261
-			if strings.Contains(m.Mode, "nocopy") {
262
-				if m.Spec.VolumeOptions == nil {
263
-					m.Spec.VolumeOptions = &mounttypes.VolumeOptions{}
264
-				}
265
-				m.Spec.VolumeOptions.NoCopy = true
332
+
333
+			if cm.Type != "" {
334
+				// probably specified via the hostconfig.Mounts
335
+				continue
266 336
 			}
337
+
338
+			// anon volume
339
+			cm.Type = mounttypes.TypeVolume
340
+			cm.Spec.Type = mounttypes.TypeVolume
267 341
 		} else {
268
-			m.Type = mounttypes.TypeBind
269
-			m.Spec.Type = mounttypes.TypeBind
270
-			m.Spec.Source = m.Source
271
-			if m.Propagation != "" {
272
-				m.Spec.BindOptions = &mounttypes.BindOptions{
273
-					Propagation: m.Propagation,
342
+			if cm.Type != "" {
343
+				// already updated
344
+				continue
345
+			}
346
+
347
+			cm.Type = mounttypes.TypeBind
348
+			cm.Spec.Type = mounttypes.TypeBind
349
+			cm.Spec.Source = cm.Source
350
+			if cm.Propagation != "" {
351
+				cm.Spec.BindOptions = &mounttypes.BindOptions{
352
+					Propagation: cm.Propagation,
274 353
 				}
275 354
 			}
276 355
 		}
277 356
 
278
-		m.Spec.Target = m.Destination
279
-		if !m.RW {
280
-			m.Spec.ReadOnly = true
281
-		}
357
+		cm.Spec.Target = cm.Destination
358
+		cm.Spec.ReadOnly = !cm.RW
282 359
 	}
283
-	return container.ToDiskLocking()
284 360
 }
285 361
 
286 362
 func (daemon *Daemon) traverseLocalVolumes(fn func(volume.Volume) error) error {
287 363
new file mode 100644
... ...
@@ -0,0 +1,259 @@
0
+// +build !windows
1
+
2
+package daemon
3
+
4
+import (
5
+	"encoding/json"
6
+	"fmt"
7
+	"reflect"
8
+	"testing"
9
+
10
+	containertypes "github.com/docker/docker/api/types/container"
11
+	mounttypes "github.com/docker/docker/api/types/mount"
12
+	"github.com/docker/docker/container"
13
+	"github.com/docker/docker/volume"
14
+)
15
+
16
+func TestBackportMountSpec(t *testing.T) {
17
+	d := Daemon{containers: container.NewMemoryStore()}
18
+
19
+	c := &container.Container{
20
+		CommonContainer: container.CommonContainer{
21
+			State: &container.State{},
22
+			MountPoints: map[string]*volume.MountPoint{
23
+				"/apple":      {Destination: "/apple", Source: "/var/lib/docker/volumes/12345678", Name: "12345678", RW: true, CopyData: true}, // anonymous volume
24
+				"/banana":     {Destination: "/banana", Source: "/var/lib/docker/volumes/data", Name: "data", RW: true, CopyData: true},        // named volume
25
+				"/cherry":     {Destination: "/cherry", Source: "/var/lib/docker/volumes/data", Name: "data", CopyData: true},                  // RO named volume
26
+				"/dates":      {Destination: "/dates", Source: "/var/lib/docker/volumes/data", Name: "data"},                                   // named volume nocopy
27
+				"/elderberry": {Destination: "/elderberry", Source: "/var/lib/docker/volumes/data", Name: "data"},                              // masks anon vol
28
+				"/fig":        {Destination: "/fig", Source: "/data", RW: true},                                                                // RW bind
29
+				"/guava":      {Destination: "/guava", Source: "/data", RW: false, Propagation: "shared"},                                      // RO bind + propagation
30
+				"/kumquat":    {Destination: "/kumquat", Name: "data", RW: false, CopyData: true},                                              // volumes-from
31
+
32
+				// partially configured mountpoint due to #32613
33
+				// specifically, `mp.Spec.Source` is not set
34
+				"/honeydew": {
35
+					Type:        mounttypes.TypeVolume,
36
+					Destination: "/honeydew",
37
+					Name:        "data",
38
+					Source:      "/var/lib/docker/volumes/data",
39
+					Spec:        mounttypes.Mount{Type: mounttypes.TypeVolume, Target: "/honeydew", VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true}},
40
+				},
41
+
42
+				// from hostconfig.Mounts
43
+				"/jambolan": {
44
+					Type:        mounttypes.TypeVolume,
45
+					Destination: "/jambolan",
46
+					Source:      "/var/lib/docker/volumes/data",
47
+					RW:          true,
48
+					Name:        "data",
49
+					Spec:        mounttypes.Mount{Type: mounttypes.TypeVolume, Target: "/jambolan", Source: "data"},
50
+				},
51
+			},
52
+			HostConfig: &containertypes.HostConfig{
53
+				Binds: []string{
54
+					"data:/banana",
55
+					"data:/cherry:ro",
56
+					"data:/dates:ro,nocopy",
57
+					"data:/elderberry:ro,nocopy",
58
+					"/data:/fig",
59
+					"/data:/guava:ro,shared",
60
+					"data:/honeydew:nocopy",
61
+				},
62
+				VolumesFrom: []string{"1:ro"},
63
+				Mounts: []mounttypes.Mount{
64
+					{Type: mounttypes.TypeVolume, Target: "/jambolan"},
65
+				},
66
+			},
67
+			Config: &containertypes.Config{Volumes: map[string]struct{}{
68
+				"/apple":      {},
69
+				"/elderberry": {},
70
+			}},
71
+		}}
72
+
73
+	d.containers.Add("1", &container.Container{
74
+		CommonContainer: container.CommonContainer{
75
+			State: &container.State{},
76
+			ID:    "1",
77
+			MountPoints: map[string]*volume.MountPoint{
78
+				"/kumquat": {Destination: "/kumquat", Name: "data", RW: false, CopyData: true},
79
+			},
80
+			HostConfig: &containertypes.HostConfig{
81
+				Binds: []string{
82
+					"data:/kumquat:ro",
83
+				},
84
+			},
85
+		},
86
+	})
87
+
88
+	type expected struct {
89
+		mp      *volume.MountPoint
90
+		comment string
91
+	}
92
+
93
+	pretty := func(mp *volume.MountPoint) string {
94
+		b, err := json.MarshalIndent(mp, "\t", "    ")
95
+		if err != nil {
96
+			return fmt.Sprintf("%#v", mp)
97
+		}
98
+		return string(b)
99
+	}
100
+
101
+	for _, x := range []expected{
102
+		{
103
+			mp: &volume.MountPoint{
104
+				Type:        mounttypes.TypeVolume,
105
+				Destination: "/apple",
106
+				RW:          true,
107
+				Name:        "12345678",
108
+				Source:      "/var/lib/docker/volumes/12345678",
109
+				CopyData:    true,
110
+				Spec: mounttypes.Mount{
111
+					Type:   mounttypes.TypeVolume,
112
+					Source: "",
113
+					Target: "/apple",
114
+				},
115
+			},
116
+			comment: "anonymous volume",
117
+		},
118
+		{
119
+			mp: &volume.MountPoint{
120
+				Type:        mounttypes.TypeVolume,
121
+				Destination: "/banana",
122
+				RW:          true,
123
+				Name:        "data",
124
+				Source:      "/var/lib/docker/volumes/data",
125
+				CopyData:    true,
126
+				Spec: mounttypes.Mount{
127
+					Type:   mounttypes.TypeVolume,
128
+					Source: "data",
129
+					Target: "/banana",
130
+				},
131
+			},
132
+			comment: "named volume",
133
+		},
134
+		{
135
+			mp: &volume.MountPoint{
136
+				Type:        mounttypes.TypeVolume,
137
+				Destination: "/cherry",
138
+				Name:        "data",
139
+				Source:      "/var/lib/docker/volumes/data",
140
+				CopyData:    true,
141
+				Spec: mounttypes.Mount{
142
+					Type:     mounttypes.TypeVolume,
143
+					Source:   "data",
144
+					Target:   "/cherry",
145
+					ReadOnly: true,
146
+				},
147
+			},
148
+			comment: "read-only named volume",
149
+		},
150
+		{
151
+			mp: &volume.MountPoint{
152
+				Type:        mounttypes.TypeVolume,
153
+				Destination: "/dates",
154
+				Name:        "data",
155
+				Source:      "/var/lib/docker/volumes/data",
156
+				Spec: mounttypes.Mount{
157
+					Type:          mounttypes.TypeVolume,
158
+					Source:        "data",
159
+					Target:        "/dates",
160
+					ReadOnly:      true,
161
+					VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true},
162
+				},
163
+			},
164
+			comment: "named volume with nocopy",
165
+		},
166
+		{
167
+			mp: &volume.MountPoint{
168
+				Type:        mounttypes.TypeVolume,
169
+				Destination: "/elderberry",
170
+				Name:        "data",
171
+				Source:      "/var/lib/docker/volumes/data",
172
+				Spec: mounttypes.Mount{
173
+					Type:          mounttypes.TypeVolume,
174
+					Source:        "data",
175
+					Target:        "/elderberry",
176
+					ReadOnly:      true,
177
+					VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true},
178
+				},
179
+			},
180
+			comment: "masks an anonymous volume",
181
+		},
182
+		{
183
+			mp: &volume.MountPoint{
184
+				Type:        mounttypes.TypeBind,
185
+				Destination: "/fig",
186
+				Source:      "/data",
187
+				RW:          true,
188
+				Spec: mounttypes.Mount{
189
+					Type:   mounttypes.TypeBind,
190
+					Source: "/data",
191
+					Target: "/fig",
192
+				},
193
+			},
194
+			comment: "bind mount with read/write",
195
+		},
196
+		{
197
+			mp: &volume.MountPoint{
198
+				Type:        mounttypes.TypeBind,
199
+				Destination: "/guava",
200
+				Source:      "/data",
201
+				RW:          false,
202
+				Propagation: "shared",
203
+				Spec: mounttypes.Mount{
204
+					Type:        mounttypes.TypeBind,
205
+					Source:      "/data",
206
+					Target:      "/guava",
207
+					ReadOnly:    true,
208
+					BindOptions: &mounttypes.BindOptions{Propagation: "shared"},
209
+				},
210
+			},
211
+			comment: "bind mount with read/write + shared propgation",
212
+		},
213
+		{
214
+			mp: &volume.MountPoint{
215
+				Type:        mounttypes.TypeVolume,
216
+				Destination: "/honeydew",
217
+				Source:      "/var/lib/docker/volumes/data",
218
+				RW:          true,
219
+				Propagation: "shared",
220
+				Spec: mounttypes.Mount{
221
+					Type:          mounttypes.TypeVolume,
222
+					Source:        "data",
223
+					Target:        "/honeydew",
224
+					VolumeOptions: &mounttypes.VolumeOptions{NoCopy: true},
225
+				},
226
+			},
227
+			comment: "partially configured named volume caused by #32613",
228
+		},
229
+		{
230
+			mp:      &(*c.MountPoints["/jambolan"]), // copy the mountpoint, expect no changes
231
+			comment: "volume defined in mounts API",
232
+		},
233
+		{
234
+			mp: &volume.MountPoint{
235
+				Type:        mounttypes.TypeVolume,
236
+				Destination: "/kumquat",
237
+				Source:      "/var/lib/docker/volumes/data",
238
+				RW:          false,
239
+				Name:        "data",
240
+				Spec: mounttypes.Mount{
241
+					Type:     mounttypes.TypeVolume,
242
+					Source:   "data",
243
+					Target:   "/kumquat",
244
+					ReadOnly: true,
245
+				},
246
+			},
247
+			comment: "partially configured named volume caused by #32613",
248
+		},
249
+	} {
250
+
251
+		mp := c.MountPoints[x.mp.Destination]
252
+		d.backportMountSpec(c)
253
+
254
+		if !reflect.DeepEqual(mp.Spec, x.mp.Spec) {
255
+			t.Fatalf("%s\nexpected:\n\t%s\n\ngot:\n\t%s", x.comment, pretty(x.mp), pretty(mp))
256
+		}
257
+	}
258
+}