Merge pull request #33270 from hpcloud/bug/33128

Automatic merge from submit-queue

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 to ignore the supplied
deviceName, and instead defer to the pre-existing GetDevicePath method to
discover the device path based on it's serial number and /dev/disk/by-id
mapping.

This new behavior is controller by a config option, as falling back
to the cinder value when we can't discover a device would risk devices
not showing up, falling back to cinder's guess, and detecting the wrong
disk as attached.

pkg/cloudprovider/providers/openstack/openstack.go

@@ -86,11 +86,16 @@ type LoadBalancerOpts struct {
 	NodeSecurityGroupID  string     `gcfg:"node-security-group"`
 }
 
+type BlockStorageOpts struct {
+	TrustDevicePath bool `gcfg:"trust-device-path"` // See Issue #33128
+}
+
 // OpenStack is an implementation of cloud provider Interface for OpenStack.
 type OpenStack struct {
 	provider *gophercloud.ProviderClient
 	region   string
 	lbOpts   LoadBalancerOpts
+	bsOpts   BlockStorageOpts
 	// InstanceID of the server where this OpenStack object is instantiated.
 	localInstanceID string
 }
@@ -110,6 +115,7 @@ type Config struct {
 		Region     string
 	}
 	LoadBalancer LoadBalancerOpts
+	BlockStorage BlockStorageOpts
 }
 
 func init() {
@@ -146,6 +152,10 @@ func readConfig(config io.Reader) (Config, error) {
 	}
 
 	var cfg Config
+
+	// Set default values for config params
+	cfg.BlockStorage.TrustDevicePath = false
+
 	err := gcfg.ReadInto(&cfg, config)
 	return cfg, err
 }
@@ -200,6 +210,7 @@ func newOpenStack(cfg Config) (*OpenStack, error) {
 		provider:        provider,
 		region:          cfg.Global.Region,
 		lbOpts:          cfg.LoadBalancer,
+		bsOpts:          cfg.BlockStorage,
 		localInstanceID: id,
 	}
 

pkg/cloudprovider/providers/openstack/openstack_test.go

@@ -76,6 +76,8 @@ func TestReadConfig(t *testing.T) {
  monitor-delay = 1m
  monitor-timeout = 30s
  monitor-max-retries = 3
+ [BlockStorage]
+ trust-device-path = yes
  `))
 	if err != nil {
 		t.Fatalf("Should succeed when a valid config is provided: %s", err)
@@ -96,6 +98,9 @@ func TestReadConfig(t *testing.T) {
 	if cfg.LoadBalancer.MonitorMaxRetries != 3 {
 		t.Errorf("incorrect lb.monitormaxretries: %d", cfg.LoadBalancer.MonitorMaxRetries)
 	}
+	if cfg.BlockStorage.TrustDevicePath != true {
+		t.Errorf("incorrect bs.trustdevicepath: %v", cfg.BlockStorage.TrustDevicePath)
+	}
 }
 
 func TestToAuthOptions(t *testing.T) {
@@ -291,6 +296,12 @@ func TestVolumes(t *testing.T) {
 
 	WaitForVolumeStatus(t, os, vol, volumeInUseStatus, volumeCreateTimeoutSeconds)
 
+	devicePath := os.GetDevicePath(diskId)
+	if !strings.HasPrefix(devicePath, "/dev/disk/by-id/") {
+		t.Fatalf("GetDevicePath returned and unexpected path for Cinder volume %s, returned %s", vol, devicePath)
+	}
+	t.Logf("Volume (%s) found at path: %s\n", vol, devicePath)
+
 	err = os.DetachDisk(os.localInstanceID, vol)
 	if err != nil {
 		t.Fatalf("Cannot DetachDisk Cinder volume %s: %v", vol, err)

pkg/cloudprovider/providers/openstack/openstack_volumes.go

@@ -169,16 +169,25 @@ func (os *OpenStack) CreateVolume(name string, size int, vtype, availability str
 
 // GetDevicePath returns the path of an attached block storage volume, specified by its id.
 func (os *OpenStack) GetDevicePath(diskId string) string {
+	// Build a list of candidate device paths
+	candidateDeviceNodes := []string{
+		// KVM
+		fmt.Sprintf("virtio-%s", diskId[:20]),
+		// ESXi
+		fmt.Sprintf("wwn-0x%s", strings.Replace(diskId, "-", "", -1)),
+	}
+
 	files, _ := ioutil.ReadDir("/dev/disk/by-id/")
+
 	for _, f := range files {
-		if strings.Contains(f.Name(), "virtio-") {
-			devid_prefix := f.Name()[len("virtio-"):len(f.Name())]
-			if strings.Contains(diskId, devid_prefix) {
+		for _, c := range candidateDeviceNodes {
+			if c == f.Name() {
 				glog.V(4).Infof("Found disk attached as %q; full devicepath: %s\n", f.Name(), path.Join("/dev/disk/by-id/", f.Name()))
 				return path.Join("/dev/disk/by-id/", f.Name())
 			}
 		}
 	}
+
 	glog.Warningf("Failed to find device for the diskid: %q\n", diskId)
 	return ""
 }
@@ -208,8 +217,10 @@ func (os *OpenStack) DeleteVolume(volumeName string) error {
 	return err
 }
 
-// Get device path of attached volume to the compute running kubelet
+// Get device path of attached volume to the compute running kubelet, as known by cinder
 func (os *OpenStack) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) {
+	// See issue #33128 - Cinder does not always tell you the right device path, as such
+	// we must only use this value as a last resort.
 	disk, err := os.getVolume(diskName)
 	if err != nil {
 		return "", err
@@ -269,3 +280,8 @@ func (os *OpenStack) diskIsUsed(diskName string) (bool, error) {
 	}
 	return false, nil
 }
+
+// query if we should trust the cinder provide deviceName, See issue #33128
+func (os *OpenStack) ShouldTrustDevicePath() bool {
+	return os.bsOpts.TrustDevicePath
+}

pkg/cloudprovider/providers/rackspace/rackspace.go

@@ -629,8 +629,10 @@ func (rs *Rackspace) DetachDisk(instanceID string, partialDiskId string) error {
 	return nil
 }
 
-// Get device path of attached volume to the compute running kubelet
+// Get device path of attached volume to the compute running kubelet, as known by cinder
 func (rs *Rackspace) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) {
+	// See issue #33128 - Cinder does not always tell you the right device path, as such
+	// we must only use this value as a last resort.
 	disk, err := rs.getVolume(diskName)
 	if err != nil {
 		return "", err
@@ -681,3 +683,8 @@ func (rs *Rackspace) DisksAreAttached(diskNames []string, instanceID string) (ma
 	}
 	return attached, returnedErr
 }
+
+// query if we should trust the cinder provide deviceName, See issue #33128
+func (rs *Rackspace) ShouldTrustDevicePath() bool {
+	return true
+}

pkg/volume/cinder/attacher.go

@@ -144,6 +144,7 @@ func (attacher *cinderDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nod
 }
 
 func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string, timeout time.Duration) (string, error) {
+	// NOTE: devicePath is is path as reported by Cinder, which may be incorrect and should not be used. See Issue #33128
 	volumeSource, _, err := getVolumeSource(spec)
 	if err != nil {
 		return "", err
@@ -166,13 +167,17 @@ func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath
 		case <-ticker.C:
 			glog.V(5).Infof("Checking Cinder disk %q is attached.", volumeID)
 			probeAttachedVolume()
+			if !attacher.cinderProvider.ShouldTrustDevicePath() {
+				// Using the Cinder volume ID, find the real device path (See Issue #33128)
+				devicePath = attacher.cinderProvider.GetDevicePath(volumeID)
+			}
 			exists, err := volumeutil.PathExists(devicePath)
 			if exists && err == nil {
-				glog.Infof("Successfully found attached Cinder disk %q.", volumeID)
+				glog.Infof("Successfully found attached Cinder disk %q at %v.", volumeID, devicePath)
 				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: %v", volumeID, err)
 			}
 		case <-timer.C:
 			return "", fmt.Errorf("Could not find attached Cinder disk %q. Timeout waiting for mount paths to be created.", volumeID)

pkg/volume/cinder/attacher_test.go

@@ -393,6 +393,10 @@ func (testcase *testcase) GetAttachmentDiskPath(instanceID string, diskName stri
 	return expected.retPath, expected.ret
 }
 
+func (testcase *testcase) ShouldTrustDevicePath() bool {
+	return true
+}
+
 func (testcase *testcase) CreateVolume(name string, size int, vtype, availability string, tags *map[string]string) (volumeName string, err error) {
 	return "", errors.New("Not implemented")
 }

pkg/volume/cinder/cinder.go

@@ -51,6 +51,7 @@ type CinderProvider interface {
 	GetAttachmentDiskPath(instanceID string, diskName string) (string, error)
 	DiskIsAttached(diskName, instanceID string) (bool, error)
 	DisksAreAttached(diskNames []string, instanceID string) (map[string]bool, error)
+	ShouldTrustDevicePath() bool
 	Instances() (cloudprovider.Instances, bool)
 }