Browse code

Merge pull request #33207 from cpuguy83/cherry_pick_mpsec_backport

[17.03] Fix issue backporting mount spec to pre-1.13 obj

Sebastiaan van Stijn authored on 2017/05/16 08:03:32
Showing 4 changed files
... ...
@@ -194,8 +194,9 @@ func (daemon *Daemon) restore() error {
194 194
 		wg.Add(1)
195 195
 		go func(c *container.Container) {
196 196
 			defer wg.Done()
197
-			if err := backportMountSpec(c); err != nil {
198
-				logrus.Error("Failed to migrate old mounts to use new spec format")
197
+			daemon.backportMountSpec(c)
198
+			if err := c.ToDiskLocking(); err != nil {
199
+				logrus.WithError(err).WithField("container", c.ID).Error("error saving backported mountspec to disk")
199 200
 			}
200 201
 
201 202
 			if c.IsRunning() || c.IsPaused() {
... ...
@@ -215,7 +216,6 @@ func (daemon *Daemon) restore() error {
215 215
 					// The error is only logged here.
216 216
 					logrus.Warnf("Failed to mount container on getting BaseFs path %v: %v", c.ID, err)
217 217
 				} else {
218
-					// if mount success, then unmount it
219 218
 					if err := daemon.Unmount(c); err != nil {
220 219
 						logrus.Warnf("Failed to umount container on getting BaseFs path %v: %v", c.ID, err)
221 220
 					}
... ...
@@ -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"
... ...
@@ -113,6 +114,7 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
113 113
 
114 114
 		for _, m := range c.MountPoints {
115 115
 			cp := &volume.MountPoint{
116
+				Type:        m.Type,
116 117
 				Name:        m.Name,
117 118
 				Source:      m.Source,
118 119
 				RW:          m.RW && volume.ReadWrite(mode),
... ...
@@ -243,48 +245,125 @@ func (daemon *Daemon) lazyInitializeVolume(containerID string, m *volume.MountPo
243 243
 	return nil
244 244
 }
245 245
 
246
-func backportMountSpec(container *container.Container) error {
247
-	for target, m := range container.MountPoints {
248
-		if m.Spec.Type != "" {
249
-			// if type is set on even one mount, no need to migrate
250
-			return nil
246
+// backportMountSpec resolves mount specs (introduced in 1.13) from pre-1.13
247
+// mount configurations
248
+// The container lock should not be held when calling this function.
249
+// Changes are only made in-memory and may make changes to containers referenced
250
+// by `container.HostConfig.VolumesFrom`
251
+func (daemon *Daemon) backportMountSpec(container *container.Container) {
252
+	container.Lock()
253
+	defer container.Unlock()
254
+
255
+	maybeUpdate := make(map[string]bool)
256
+	for _, mp := range container.MountPoints {
257
+		if mp.Spec.Source != "" && mp.Type != "" {
258
+			continue
251 259
 		}
252
-		if m.Name != "" {
253
-			m.Type = mounttypes.TypeVolume
254
-			m.Spec.Type = mounttypes.TypeVolume
260
+		maybeUpdate[mp.Destination] = true
261
+	}
262
+	if len(maybeUpdate) == 0 {
263
+		return
264
+	}
255 265
 
256
-			// make sure this is not an anyonmous volume before setting the spec source
257
-			if _, exists := container.Config.Volumes[target]; !exists {
258
-				m.Spec.Source = m.Name
266
+	mountSpecs := make(map[string]bool, len(container.HostConfig.Mounts))
267
+	for _, m := range container.HostConfig.Mounts {
268
+		mountSpecs[m.Target] = true
269
+	}
270
+
271
+	binds := make(map[string]*volume.MountPoint, len(container.HostConfig.Binds))
272
+	for _, rawSpec := range container.HostConfig.Binds {
273
+		mp, err := volume.ParseMountRaw(rawSpec, container.HostConfig.VolumeDriver)
274
+		if err != nil {
275
+			logrus.WithError(err).Error("Got unexpected error while re-parsing raw volume spec during spec backport")
276
+			continue
277
+		}
278
+		binds[mp.Destination] = mp
279
+	}
280
+
281
+	volumesFrom := make(map[string]volume.MountPoint)
282
+	for _, fromSpec := range container.HostConfig.VolumesFrom {
283
+		from, _, err := volume.ParseVolumesFrom(fromSpec)
284
+		if err != nil {
285
+			logrus.WithError(err).WithField("id", container.ID).Error("Error reading volumes-from spec during mount spec backport")
286
+			continue
287
+		}
288
+		fromC, err := daemon.GetContainer(from)
289
+		if err != nil {
290
+			logrus.WithError(err).WithField("from-container", from).Error("Error looking up volumes-from container")
291
+			continue
292
+		}
293
+
294
+		// make sure from container's specs have been backported
295
+		daemon.backportMountSpec(fromC)
296
+
297
+		fromC.Lock()
298
+		for t, mp := range fromC.MountPoints {
299
+			volumesFrom[t] = *mp
300
+		}
301
+		fromC.Unlock()
302
+	}
303
+
304
+	needsUpdate := func(containerMount, other *volume.MountPoint) bool {
305
+		if containerMount.Type != other.Type || !reflect.DeepEqual(containerMount.Spec, other.Spec) {
306
+			return true
307
+		}
308
+		return false
309
+	}
310
+
311
+	// main
312
+	for _, cm := range container.MountPoints {
313
+		if !maybeUpdate[cm.Destination] {
314
+			continue
315
+		}
316
+		// nothing to backport if from hostconfig.Mounts
317
+		if mountSpecs[cm.Destination] {
318
+			continue
319
+		}
320
+
321
+		if mp, exists := binds[cm.Destination]; exists {
322
+			if needsUpdate(cm, mp) {
323
+				cm.Spec = mp.Spec
324
+				cm.Type = mp.Type
259 325
 			}
260
-			if container.HostConfig.VolumeDriver != "" {
261
-				m.Spec.VolumeOptions = &mounttypes.VolumeOptions{
262
-					DriverConfig: &mounttypes.Driver{Name: container.HostConfig.VolumeDriver},
326
+			continue
327
+		}
328
+
329
+		if cm.Name != "" {
330
+			if mp, exists := volumesFrom[cm.Destination]; exists {
331
+				if needsUpdate(cm, &mp) {
332
+					cm.Spec = mp.Spec
333
+					cm.Type = mp.Type
263 334
 				}
335
+				continue
264 336
 			}
265
-			if strings.Contains(m.Mode, "nocopy") {
266
-				if m.Spec.VolumeOptions == nil {
267
-					m.Spec.VolumeOptions = &mounttypes.VolumeOptions{}
268
-				}
269
-				m.Spec.VolumeOptions.NoCopy = true
337
+
338
+			if cm.Type != "" {
339
+				// probably specified via the hostconfig.Mounts
340
+				continue
270 341
 			}
342
+
343
+			// anon volume
344
+			cm.Type = mounttypes.TypeVolume
345
+			cm.Spec.Type = mounttypes.TypeVolume
271 346
 		} else {
272
-			m.Type = mounttypes.TypeBind
273
-			m.Spec.Type = mounttypes.TypeBind
274
-			m.Spec.Source = m.Source
275
-			if m.Propagation != "" {
276
-				m.Spec.BindOptions = &mounttypes.BindOptions{
277
-					Propagation: m.Propagation,
347
+			if cm.Type != "" {
348
+				// already updated
349
+				continue
350
+			}
351
+
352
+			cm.Type = mounttypes.TypeBind
353
+			cm.Spec.Type = mounttypes.TypeBind
354
+			cm.Spec.Source = cm.Source
355
+			if cm.Propagation != "" {
356
+				cm.Spec.BindOptions = &mounttypes.BindOptions{
357
+					Propagation: cm.Propagation,
278 358
 				}
279 359
 			}
280 360
 		}
281 361
 
282
-		m.Spec.Target = m.Destination
283
-		if !m.RW {
284
-			m.Spec.ReadOnly = true
285
-		}
362
+		cm.Spec.Target = cm.Destination
363
+		cm.Spec.ReadOnly = !cm.RW
286 364
 	}
287
-	return container.ToDiskLocking()
288 365
 }
289 366
 
290 367
 func (daemon *Daemon) traverseLocalVolumes(fn func(volume.Volume) error) error {
291 368
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
+}