From 2aed732d3a323110e7d166587424faacb9a4d138 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 30 Nov 2016 11:11:50 -0500 Subject: [PATCH] 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 (cherry picked from commit d8ce4a6e108f4f870228912f105eed8218e087e4) Signed-off-by: Victor Vieux --- ...er_cli_external_volume_driver_unix_test.go | 28 ++++++ volume/store/store.go | 89 ++++++++++--------- 2 files changed, 77 insertions(+), 40 deletions(-) diff --git a/integration-cli/docker_cli_external_volume_driver_unix_test.go b/integration-cli/docker_cli_external_volume_driver_unix_test.go index 2abd36389d..7673d6c113 100644 --- a/integration-cli/docker_cli_external_volume_driver_unix_test.go +++ b/integration-cli/docker_cli_external_volume_driver_unix_test.go @@ -568,8 +568,36 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverOutOfBandDelete(c *c // simulate out of band volume deletion on plugin level delete(p.vols, "test") + // test re-create with same driver + out, err = s.d.Cmd("volume", "create", "-d", driverName, "--opt", "foo=bar", "--name", "test") + c.Assert(err, checker.IsNil, check.Commentf(out)) + out, err = s.d.Cmd("volume", "inspect", "test") + c.Assert(err, checker.IsNil, check.Commentf(out)) + + var vs []types.Volume + err = json.Unmarshal([]byte(out), &vs) + c.Assert(err, checker.IsNil) + c.Assert(vs, checker.HasLen, 1) + c.Assert(vs[0].Driver, checker.Equals, driverName) + c.Assert(vs[0].Options, checker.NotNil) + c.Assert(vs[0].Options["foo"], checker.Equals, "bar") + c.Assert(vs[0].Driver, checker.Equals, driverName) + + // simulate out of band volume deletion on plugin level + delete(p.vols, "test") + + // test create with different driver out, err = s.d.Cmd("volume", "create", "-d", "local", "--name", "test") c.Assert(err, checker.IsNil, check.Commentf(out)) + + out, err = s.d.Cmd("volume", "inspect", "test") + c.Assert(err, checker.IsNil, check.Commentf(out)) + vs = nil + err = json.Unmarshal([]byte(out), &vs) + c.Assert(err, checker.IsNil) + c.Assert(vs, checker.HasLen, 1) + c.Assert(vs[0].Options, checker.HasLen, 0) + c.Assert(vs[0].Driver, checker.Equals, "local") } func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnMountFail(c *check.C) { diff --git a/volume/store/store.go b/volume/store/store.go index 40ae6ad6f1..06c0260308 100644 --- a/volume/store/store.go +++ b/volume/store/store.go @@ -284,56 +284,64 @@ func (s *VolumeStore) Create(name, driverName string, opts, labels map[string]st func (s *VolumeStore) checkConflict(name, driverName string) (volume.Volume, error) { // check the local cache v, _ := s.getNamed(name) - if v != nil { - vDriverName := v.DriverName() - if driverName != "" && vDriverName != driverName { - // we have what looks like a conflict - // let's see if there are existing refs to this volume, if so we don't need - // to go any further since we can assume the volume is legit. - if len(s.getRefs(name)) > 0 { - return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name) - } + if v == nil { + return nil, nil + } - // looks like there is a conflict, but nothing is referencing it... - // let's check if the found volume ref - // is stale by checking with the driver if it still exists - vd, err := volumedrivers.GetDriver(vDriverName) - if err != nil { - // play it safe and return the error - // TODO(cpuguy83): maybe when when v2 plugins are ubiquitous, we should - // just purge this from the cache - 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) - } + vDriverName := v.DriverName() + var conflict bool + if driverName != "" && vDriverName != driverName { + conflict = true + } - // now check if it still exists in the driver - v2, err := vd.Get(name) - err = errors.Cause(err) - if err != nil { - if _, ok := err.(net.Error); ok { - // got some error related to the driver connectivity - // play it safe and return the error - // TODO(cpuguy83): When when v2 plugins are ubiquitous, maybe we should - // just purge this from the cache - 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) - } + // let's check if the found volume ref + // is stale by checking with the driver if it still exists + exists, err := volumeExists(v) + if err != nil { + 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) + } - // a driver can return whatever it wants, so let's make sure this is nil - if v2 == nil { - // purge this reference from the cache - s.Purge(name) - return nil, nil - } - } - if v2 != nil { - return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name) - } + if exists { + if conflict { + return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name) } return v, nil } + if len(s.getRefs(v.Name())) > 0 { + // Containers are referencing this volume but it doesn't seem to exist anywhere. + // Return a conflict error here, the user can fix this with `docker volume rm -f` + 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) + } + + // doesn't exist, so purge it from the cache + s.Purge(name) return nil, nil } +// volumeExists returns if the volume is still present in the driver. +// An error is returned if there was an issue communicating with the driver. +func volumeExists(v volume.Volume) (bool, error) { + vd, err := volumedrivers.GetDriver(v.DriverName()) + if err != nil { + return false, errors.Wrapf(err, "error while checking if volume %q exists in driver %q", v.Name(), v.DriverName()) + } + exists, err := vd.Get(v.Name()) + if err != nil { + err = errors.Cause(err) + if _, ok := err.(net.Error); ok { + return false, errors.Wrapf(err, "error while checking if volume %q exists in driver %q", v.Name(), v.DriverName()) + } + + // At this point, the error could be anything from the driver, such as "no such volume" + // Let's not check an error here, and instead check if the driver returned a volume + } + if exists == nil { + return false, nil + } + return true, nil +} + // create asks the given driver to create a volume with the name/opts. // If a volume with the name is already known, it will ask the stored driver for the volume. // 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 if err != nil { return nil, err } + if v != nil { return v, nil }