From 4942a57db6db36b715a18a67bafcf783bc25250e Mon Sep 17 00:00:00 2001 From: Kiall Mac Innes Date: Thu, 22 Sep 2016 11:07:13 +0100 Subject: [PATCH 1/2] Support OpenStack+ESXi Volumes in GetDevicePath GetDevicePath was currently coded to only support Nova+KVM style device paths, update so we also support Nova+ESXi and leave the code such that new pattern additions are easy. --- .../providers/openstack/openstack_volumes.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/cloudprovider/providers/openstack/openstack_volumes.go b/pkg/cloudprovider/providers/openstack/openstack_volumes.go index 7b65e522f43..8844897db92 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_volumes.go +++ b/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 "" } From ce8eda94dfe5b05c87cfcf26d67d204e0c8a8b41 Mon Sep 17 00:00:00 2001 From: Kiall Mac Innes Date: Thu, 22 Sep 2016 12:59:43 +0100 Subject: [PATCH 2/2] 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 | 11 +++++++++++ .../providers/openstack/openstack_test.go | 11 +++++++++++ .../providers/openstack/openstack_volumes.go | 9 ++++++++- pkg/cloudprovider/providers/rackspace/rackspace.go | 9 ++++++++- pkg/volume/cinder/attacher.go | 11 ++++++++--- pkg/volume/cinder/attacher_test.go | 4 ++++ pkg/volume/cinder/cinder.go | 1 + 7 files changed, 51 insertions(+), 5 deletions(-) diff --git a/pkg/cloudprovider/providers/openstack/openstack.go b/pkg/cloudprovider/providers/openstack/openstack.go index 5d18fd52ccf..afc47fa47dc 100644 --- a/pkg/cloudprovider/providers/openstack/openstack.go +++ b/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, } diff --git a/pkg/cloudprovider/providers/openstack/openstack_test.go b/pkg/cloudprovider/providers/openstack/openstack_test.go index 625455df882..bf8b7864acb 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_test.go +++ b/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) diff --git a/pkg/cloudprovider/providers/openstack/openstack_volumes.go b/pkg/cloudprovider/providers/openstack/openstack_volumes.go index 8844897db92..bde05d38734 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_volumes.go +++ b/pkg/cloudprovider/providers/openstack/openstack_volumes.go @@ -217,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 @@ -278,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 +} diff --git a/pkg/cloudprovider/providers/rackspace/rackspace.go b/pkg/cloudprovider/providers/rackspace/rackspace.go index b7e1a08f6de..b25b2ce5c9e 100644 --- a/pkg/cloudprovider/providers/rackspace/rackspace.go +++ b/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 +} diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index 064ab26a60b..a254090b003 100644 --- a/pkg/volume/cinder/attacher.go +++ b/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) diff --git a/pkg/volume/cinder/attacher_test.go b/pkg/volume/cinder/attacher_test.go index ded3156ebee..d26d068eca7 100644 --- a/pkg/volume/cinder/attacher_test.go +++ b/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") } diff --git a/pkg/volume/cinder/cinder.go b/pkg/volume/cinder/cinder.go index 817c4db486e..42ea53dea6b 100644 --- a/pkg/volume/cinder/cinder.go +++ b/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) }