Browse code

Fix out-of-band vol delete+create for same driver

Fix issue where out-of-band deletions and then a `docker volume create`
on the same driver caused volume to not be re-created in the driver but
return as created since it was stored in the cache.

Previous fix only worked if the driver names did not match.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit d8ce4a6e108f4f870228912f105eed8218e087e4)
Signed-off-by: Victor Vieux <victorvieux@gmail.com>

Brian Goff authored on 2016/12/01 01:11:50
Showing 2 changed files
... ...
@@ -568,8 +568,36 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverOutOfBandDelete(c *c
568 568
 	// simulate out of band volume deletion on plugin level
569 569
 	delete(p.vols, "test")
570 570
 
571
+	// test re-create with same driver
572
+	out, err = s.d.Cmd("volume", "create", "-d", driverName, "--opt", "foo=bar", "--name", "test")
573
+	c.Assert(err, checker.IsNil, check.Commentf(out))
574
+	out, err = s.d.Cmd("volume", "inspect", "test")
575
+	c.Assert(err, checker.IsNil, check.Commentf(out))
576
+
577
+	var vs []types.Volume
578
+	err = json.Unmarshal([]byte(out), &vs)
579
+	c.Assert(err, checker.IsNil)
580
+	c.Assert(vs, checker.HasLen, 1)
581
+	c.Assert(vs[0].Driver, checker.Equals, driverName)
582
+	c.Assert(vs[0].Options, checker.NotNil)
583
+	c.Assert(vs[0].Options["foo"], checker.Equals, "bar")
584
+	c.Assert(vs[0].Driver, checker.Equals, driverName)
585
+
586
+	// simulate out of band volume deletion on plugin level
587
+	delete(p.vols, "test")
588
+
589
+	// test create with different driver
571 590
 	out, err = s.d.Cmd("volume", "create", "-d", "local", "--name", "test")
572 591
 	c.Assert(err, checker.IsNil, check.Commentf(out))
592
+
593
+	out, err = s.d.Cmd("volume", "inspect", "test")
594
+	c.Assert(err, checker.IsNil, check.Commentf(out))
595
+	vs = nil
596
+	err = json.Unmarshal([]byte(out), &vs)
597
+	c.Assert(err, checker.IsNil)
598
+	c.Assert(vs, checker.HasLen, 1)
599
+	c.Assert(vs[0].Options, checker.HasLen, 0)
600
+	c.Assert(vs[0].Driver, checker.Equals, "local")
573 601
 }
574 602
 
575 603
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnMountFail(c *check.C) {
... ...
@@ -284,56 +284,64 @@ func (s *VolumeStore) Create(name, driverName string, opts, labels map[string]st
284 284
 func (s *VolumeStore) checkConflict(name, driverName string) (volume.Volume, error) {
285 285
 	// check the local cache
286 286
 	v, _ := s.getNamed(name)
287
-	if v != nil {
288
-		vDriverName := v.DriverName()
289
-		if driverName != "" && vDriverName != driverName {
290
-			// we have what looks like a conflict
291
-			// let's see if there are existing refs to this volume, if so we don't need
292
-			// to go any further since we can assume the volume is legit.
293
-			if len(s.getRefs(name)) > 0 {
294
-				return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name)
295
-			}
287
+	if v == nil {
288
+		return nil, nil
289
+	}
296 290
 
297
-			// looks like there is a conflict, but nothing is referencing it...
298
-			// let's check if the found volume ref
299
-			// is stale by checking with the driver if it still exists
300
-			vd, err := volumedrivers.GetDriver(vDriverName)
301
-			if err != nil {
302
-				// play it safe and return the error
303
-				// TODO(cpuguy83): maybe when when v2 plugins are ubiquitous, we should
304
-				// just purge this from the cache
305
-				return nil, errors.Wrapf(errNameConflict, "found reference to volume '%s' in driver '%s', but got an error while checking the driver: %v", name, vDriverName, err)
306
-			}
291
+	vDriverName := v.DriverName()
292
+	var conflict bool
293
+	if driverName != "" && vDriverName != driverName {
294
+		conflict = true
295
+	}
307 296
 
308
-			// now check if it still exists in the driver
309
-			v2, err := vd.Get(name)
310
-			err = errors.Cause(err)
311
-			if err != nil {
312
-				if _, ok := err.(net.Error); ok {
313
-					// got some error related to the driver connectivity
314
-					// play it safe and return the error
315
-					// TODO(cpuguy83): When when v2 plugins are ubiquitous, maybe we should
316
-					// just purge this from the cache
317
-					return nil, errors.Wrapf(errNameConflict, "found reference to volume '%s' in driver '%s', but got an error while checking the driver: %v", name, vDriverName, err)
318
-				}
319
-
320
-				// a driver can return whatever it wants, so let's make sure this is nil
321
-				if v2 == nil {
322
-					// purge this reference from the cache
323
-					s.Purge(name)
324
-					return nil, nil
325
-				}
326
-			}
327
-			if v2 != nil {
328
-				return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name)
329
-			}
297
+	// let's check if the found volume ref
298
+	// is stale by checking with the driver if it still exists
299
+	exists, err := volumeExists(v)
300
+	if err != nil {
301
+		return nil, errors.Wrapf(errNameConflict, "found reference to volume '%s' in driver '%s', but got an error while checking the driver: %v", name, vDriverName, err)
302
+	}
303
+
304
+	if exists {
305
+		if conflict {
306
+			return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name)
330 307
 		}
331 308
 		return v, nil
332 309
 	}
333 310
 
311
+	if len(s.getRefs(v.Name())) > 0 {
312
+		// Containers are referencing this volume but it doesn't seem to exist anywhere.
313
+		// Return a conflict error here, the user can fix this with `docker volume rm -f`
314
+		return nil, errors.Wrapf(errNameConflict, "found references to volume '%s' in driver '%s' but the volume was not found in the driver -- you may need to remove containers referencing this volume or force remove the volume to re-create it", name, vDriverName)
315
+	}
316
+
317
+	// doesn't exist, so purge it from the cache
318
+	s.Purge(name)
334 319
 	return nil, nil
335 320
 }
336 321
 
322
+// volumeExists returns if the volume is still present in the driver.
323
+// An error is returned if there was an issue communicating with the driver.
324
+func volumeExists(v volume.Volume) (bool, error) {
325
+	vd, err := volumedrivers.GetDriver(v.DriverName())
326
+	if err != nil {
327
+		return false, errors.Wrapf(err, "error while checking if volume %q exists in driver %q", v.Name(), v.DriverName())
328
+	}
329
+	exists, err := vd.Get(v.Name())
330
+	if err != nil {
331
+		err = errors.Cause(err)
332
+		if _, ok := err.(net.Error); ok {
333
+			return false, errors.Wrapf(err, "error while checking if volume %q exists in driver %q", v.Name(), v.DriverName())
334
+		}
335
+
336
+		// At this point, the error could be anything from the driver, such as "no such volume"
337
+		// Let's not check an error here, and instead check if the driver returned a volume
338
+	}
339
+	if exists == nil {
340
+		return false, nil
341
+	}
342
+	return true, nil
343
+}
344
+
337 345
 // create asks the given driver to create a volume with the name/opts.
338 346
 // If a volume with the name is already known, it will ask the stored driver for the volume.
339 347
 // If the passed in driver name does not match the driver name which is stored
... ...
@@ -354,6 +362,7 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st
354 354
 	if err != nil {
355 355
 		return nil, err
356 356
 	}
357
+
357 358
 	if v != nil {
358 359
 		return v, nil
359 360
 	}