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 7b65e522f43..bde05d38734 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 "" } @@ -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 +} 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) }