From 3763783aebbecd8b04c8cb36fc387661b53a40e4 Mon Sep 17 00:00:00 2001 From: Kiall Mac Innes Date: Wed, 21 Sep 2016 16:52:58 +0000 Subject: [PATCH] Don't rely on device name provided by Cinder See issue #33128 We can't rely on the device name provided by Cinder, and thus must perform detection based on the drive serial number (aka It's cinder ID) on the kubelet itself. This patch re-works the cinder volume attacher (the parts executed in kube-controller-manager) to return the volume ID, rather than the device name as advertised by Cinder. We then rework the cinder volume attacher (this time, the parts executed in kubelet) to accept this ID, and call the pre-existing GetDevicePath method, will will perform the discovery correctly. This is a break in the Attacher interface, which explicitly calls for the Attach method to return a device name. --- .../providers/openstack/openstack.go | 20 ------- .../providers/rackspace/rackspace.go | 20 ------- pkg/volume/cinder/attacher.go | 53 ++++++++++--------- pkg/volume/cinder/attacher_test.go | 45 ++-------------- pkg/volume/cinder/cinder.go | 1 - 5 files changed, 31 insertions(+), 108 deletions(-) diff --git a/pkg/cloudprovider/providers/openstack/openstack.go b/pkg/cloudprovider/providers/openstack/openstack.go index e44f7f4ae6263..fc82296f22b0a 100644 --- a/pkg/cloudprovider/providers/openstack/openstack.go +++ b/pkg/cloudprovider/providers/openstack/openstack.go @@ -683,26 +683,6 @@ func (os *OpenStack) DeleteVolume(volumeName string) error { return err } -// Get device path of attached volume to the compute running kubelet -func (os *OpenStack) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) { - disk, err := os.getVolume(diskName) - if err != nil { - return "", err - } - if len(disk.Attachments) > 0 && disk.Attachments[0]["server_id"] != nil { - if instanceID == disk.Attachments[0]["server_id"] { - // Attachment[0]["device"] points to the device path - // see http://developer.openstack.org/api-ref-blockstorage-v1.html - return disk.Attachments[0]["device"].(string), nil - } else { - errMsg := fmt.Sprintf("Disk %q is attached to a different compute: %q, should be detached before proceeding", diskName, disk.Attachments[0]["server_id"]) - glog.Errorf(errMsg) - return "", errors.New(errMsg) - } - } - return "", fmt.Errorf("volume %s is not attached to %s", diskName, instanceID) -} - // query if a volume is attached to a compute instance func (os *OpenStack) DiskIsAttached(diskName, instanceID string) (bool, error) { disk, err := os.getVolume(diskName) diff --git a/pkg/cloudprovider/providers/rackspace/rackspace.go b/pkg/cloudprovider/providers/rackspace/rackspace.go index 3f6f6a5d4b365..325ebb6b7a706 100644 --- a/pkg/cloudprovider/providers/rackspace/rackspace.go +++ b/pkg/cloudprovider/providers/rackspace/rackspace.go @@ -613,26 +613,6 @@ func (rs *Rackspace) DetachDisk(instanceID string, partialDiskId string) error { return nil } -// Get device path of attached volume to the compute running kubelet -func (rs *Rackspace) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) { - disk, err := rs.getVolume(diskName) - if err != nil { - return "", err - } - if len(disk.Attachments) > 0 && disk.Attachments[0]["server_id"] != nil { - if instanceID == disk.Attachments[0]["server_id"] { - // Attachment[0]["device"] points to the device path - // see http://developer.openstack.org/api-ref-blockstorage-v1.html - return disk.Attachments[0]["device"].(string), nil - } else { - errMsg := fmt.Sprintf("Disk %q is attached to a different compute: %q, should be detached before proceeding", diskName, disk.Attachments[0]["server_id"]) - glog.Errorf(errMsg) - return "", errors.New(errMsg) - } - } - return "", fmt.Errorf("volume %s is not attached to %s", diskName, instanceID) -} - // query if a volume is attached to a compute instance func (rs *Rackspace) DiskIsAttached(diskName, instanceID string) (bool, error) { disk, err := rs.getVolume(diskName) diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index 85712f3b61df2..4e7bcc4b20090 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -99,27 +99,12 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) ( } } - devicePath, err := attacher.cinderProvider.GetAttachmentDiskPath(instanceid, volumeID) - if err != nil { - glog.Infof("Attach volume %q to instance %q failed with %v", volumeID, instanceid, err) - return "", err - } - - return devicePath, err + // Fake the "devicePath" and actually just return the Cinder ID for the volume, allowing the kubelet to find + // the real device path (See Issue #33128) + return volumeID, err } -func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string, timeout time.Duration) (string, error) { - volumeSource, _, err := getVolumeSource(spec) - if err != nil { - return "", err - } - - volumeID := volumeSource.VolumeID - - if devicePath == "" { - return "", fmt.Errorf("WaitForAttach failed for Cinder disk %q: devicePath is empty.", volumeID) - } - +func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, volumeID string, timeout time.Duration) (string, error) { ticker := time.NewTicker(checkSleepDuration) defer ticker.Stop() timer := time.NewTimer(timeout) @@ -131,13 +116,16 @@ func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath case <-ticker.C: glog.V(5).Infof("Checking Cinder disk %q is attached.", volumeID) probeAttachedVolume() - exists, err := volumeutil.PathExists(devicePath) - if exists && err == nil { + + // Using the Cinder volume ID, find the real device path (See Issue #33128) + devicePath := attacher.cinderProvider.GetDevicePath(volumeID) + + if devicePath != "" { glog.Infof("Successfully found attached Cinder disk %q.", volumeID) return devicePath, nil } else { - //Log error, if any, and continue checking periodically - glog.Errorf("Error Stat Cinder disk (%q) is attached: %v", volumeID, err) + // Log an error, and continue checking periodically + glog.Errorf("Error could not find attached Cinder disk %q", volumeID) } case <-timer.C: return "", fmt.Errorf("Could not find attached Cinder disk %q. Timeout waiting for mount paths to be created.", volumeID) @@ -156,7 +144,14 @@ func (attacher *cinderDiskAttacher) GetDeviceMountPath( } // FIXME: this method can be further pruned. -func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string) error { +func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, volumeID string, deviceMountPath string) error { + // Using the Cinder volume ID, find the real device path (See Issue #33128) + devicePath := attacher.cinderProvider.GetDevicePath(volumeID) + + if devicePath == "" { + return fmt.Errorf("MountDevice failed for Cinder disk %q: devicePath is empty.", volumeID) + } + mounter := attacher.host.GetMounter() notMnt, err := mounter.IsLikelyNotMountPoint(deviceMountPath) if err != nil { @@ -241,12 +236,20 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, hostName stri return nil } -func (detacher *cinderDiskDetacher) WaitForDetach(devicePath string, timeout time.Duration) error { +func (detacher *cinderDiskDetacher) WaitForDetach(volumeID string, timeout time.Duration) error { ticker := time.NewTicker(checkSleepDuration) defer ticker.Stop() timer := time.NewTimer(timeout) defer timer.Stop() + // Using the Cinder volume ID, find the real device path (See Issue #33128) + devicePath := detacher.cinderProvider.GetDevicePath(volumeID) + + if devicePath == "" { + // Volume has already been unmounted + return nil + } + for { select { case <-ticker.C: diff --git a/pkg/volume/cinder/attacher_test.go b/pkg/volume/cinder/attacher_test.go index cf146b029b822..9ac766bf1dc9b 100644 --- a/pkg/volume/cinder/attacher_test.go +++ b/pkg/volume/cinder/attacher_test.go @@ -82,7 +82,6 @@ func TestAttachDetach(t *testing.T) { attachError := errors.New("Fake attach error") detachError := errors.New("Fake detach error") diskCheckError := errors.New("Fake DiskIsAttached error") - diskPathError := errors.New("Fake GetAttachmentDiskPath error") tests := []testcase{ // Successful Attach call { @@ -95,7 +94,7 @@ func TestAttachDetach(t *testing.T) { attacher := newAttacher(testcase) return attacher.Attach(spec, instanceID) }, - expectedDevice: "/dev/sda", + expectedDevice: diskName, }, // Disk is already attached @@ -108,7 +107,7 @@ func TestAttachDetach(t *testing.T) { attacher := newAttacher(testcase) return attacher.Attach(spec, instanceID) }, - expectedDevice: "/dev/sda", + expectedDevice: diskName, }, // DiskIsAttached fails and Attach succeeds @@ -122,7 +121,7 @@ func TestAttachDetach(t *testing.T) { attacher := newAttacher(testcase) return attacher.Attach(spec, instanceID) }, - expectedDevice: "/dev/sda", + expectedDevice: diskName, }, // Attach call fails @@ -138,20 +137,6 @@ func TestAttachDetach(t *testing.T) { expectedError: attachError, }, - // GetAttachmentDiskPath call fails - { - name: "Attach_Negative_DiskPatchFails", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, - attach: attachCall{diskName, instanceID, "", nil}, - diskPath: diskPathCall{diskName, instanceID, "", diskPathError}, - test: func(testcase *testcase) (string, error) { - attacher := newAttacher(testcase) - return attacher.Attach(spec, instanceID) - }, - expectedError: diskPathError, - }, - // Detach succeeds { name: "Detach_Positive", @@ -367,30 +352,6 @@ func (testcase *testcase) DiskIsAttached(diskName, instanceID string) (bool, err return expected.isAttached, expected.ret } -func (testcase *testcase) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) { - expected := &testcase.diskPath - if expected.diskName == "" && expected.instanceID == "" { - // testcase.diskPath looks uninitialized, test did not expect to - // call GetAttachmentDiskPath - testcase.t.Errorf("Unexpected GetAttachmentDiskPath call!") - return "", errors.New("Unexpected GetAttachmentDiskPath call!") - } - - if expected.diskName != diskName { - testcase.t.Errorf("Unexpected GetAttachmentDiskPath call: expected diskName %s, got %s", expected.diskName, diskName) - return "", errors.New("Unexpected GetAttachmentDiskPath call: wrong diskName") - } - - if expected.instanceID != instanceID { - testcase.t.Errorf("Unexpected GetAttachmentDiskPath call: expected instanceID %s, got %s", expected.instanceID, instanceID) - return "", errors.New("Unexpected GetAttachmentDiskPath call: wrong instanceID") - } - - glog.V(4).Infof("GetAttachmentDiskPath call: %s, %s, returning %v, %v", diskName, instanceID, expected.retPath, expected.ret) - - return expected.retPath, expected.ret -} - func (testcase *testcase) CreateVolume(name string, size int, tags *map[string]string) (volumeName string, err error) { return "", errors.New("Not implemented") } diff --git a/pkg/volume/cinder/cinder.go b/pkg/volume/cinder/cinder.go index 7b079d69041a6..73d2b4e7f4698 100644 --- a/pkg/volume/cinder/cinder.go +++ b/pkg/volume/cinder/cinder.go @@ -48,7 +48,6 @@ type CinderProvider interface { CreateVolume(name string, size int, tags *map[string]string) (volumeName string, err error) GetDevicePath(diskId string) string InstanceID() (string, error) - GetAttachmentDiskPath(instanceID string, diskName string) (string, error) DiskIsAttached(diskName, instanceID string) (bool, error) Instances() (cloudprovider.Instances, bool) }