From cfafde983b1f8e31b5c34180a9824a2c8d75642e Mon Sep 17 00:00:00 2001 From: Vladimir Vivien Date: Thu, 4 Apr 2019 11:07:16 -0400 Subject: [PATCH] Volume AttachablePlugin.CanAttach() now returns both bool and error --- .../attachdetach/testing/testvolumespec.go | 4 +- pkg/volume/awsebs/attacher.go | 4 +- pkg/volume/azure_dd/azure_dd.go | 4 +- pkg/volume/cinder/attacher.go | 4 +- pkg/volume/csi/csi_attacher_test.go | 10 ++- pkg/volume/csi/csi_plugin.go | 18 ++-- pkg/volume/csi/csi_plugin_test.go | 88 ++++++++++++++++++- pkg/volume/fc/attacher.go | 4 +- pkg/volume/flexvolume/plugin.go | 4 +- pkg/volume/gcepd/attacher.go | 4 +- pkg/volume/iscsi/attacher.go | 4 +- pkg/volume/photon_pd/attacher.go | 4 +- pkg/volume/plugins.go | 6 +- pkg/volume/rbd/attacher.go | 4 +- pkg/volume/testing/testing.go | 8 +- pkg/volume/vsphere_volume/attacher.go | 4 +- 16 files changed, 131 insertions(+), 43 deletions(-) diff --git a/pkg/controller/volume/attachdetach/testing/testvolumespec.go b/pkg/controller/volume/attachdetach/testing/testvolumespec.go index 90d594f0b10..3e23162f3e5 100644 --- a/pkg/controller/volume/attachdetach/testing/testvolumespec.go +++ b/pkg/controller/volume/attachdetach/testing/testvolumespec.go @@ -308,8 +308,8 @@ func (plugin *TestPlugin) NewDetacher() (volume.Detacher, error) { return &detacher, nil } -func (plugin *TestPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *TestPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *TestPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/awsebs/attacher.go b/pkg/volume/awsebs/attacher.go index 88990e40ebc..f82ba083446 100644 --- a/pkg/volume/awsebs/attacher.go +++ b/pkg/volume/awsebs/attacher.go @@ -277,8 +277,8 @@ func (detacher *awsElasticBlockStoreDetacher) UnmountDevice(deviceMountPath stri return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false) } -func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *awsElasticBlockStorePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/azure_dd/azure_dd.go b/pkg/volume/azure_dd/azure_dd.go index dd12442e9ff..d0607423c2f 100644 --- a/pkg/volume/azure_dd/azure_dd.go +++ b/pkg/volume/azure_dd/azure_dd.go @@ -238,8 +238,8 @@ func (plugin *azureDataDiskPlugin) NewDetacher() (volume.Detacher, error) { }, nil } -func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *azureDataDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index 18c47569d26..9abbfa8b441 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -406,8 +406,8 @@ func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string) error return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false) } -func (plugin *cinderPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *cinderPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *cinderPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index ca7134113c8..8dae5f152e7 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -348,7 +348,10 @@ func TestAttacherWithCSIDriver(t *testing.T) { csiAttacher := attacher.(*csiAttacher) spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false) - pluginCanAttach := plug.CanAttach(spec) + pluginCanAttach, err := plug.CanAttach(spec) + if err != nil { + t.Fatalf("attacher.CanAttach failed: %s", err) + } if pluginCanAttach != test.expectVolumeAttachment { t.Errorf("attacher.CanAttach does not match expected attachment status %t", test.expectVolumeAttachment) } @@ -429,7 +432,10 @@ func TestAttacherWaitForVolumeAttachmentWithCSIDriver(t *testing.T) { csiAttacher := attacher.(*csiAttacher) spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false) - pluginCanAttach := plug.CanAttach(spec) + pluginCanAttach, err := plug.CanAttach(spec) + if err != nil { + t.Fatalf("plugin.CanAttach test failed: %s", err) + } if !pluginCanAttach { t.Log("plugin is not attachable") return diff --git a/pkg/volume/csi/csi_plugin.go b/pkg/volume/csi/csi_plugin.go index 963e0a58a51..68b50eccd3b 100644 --- a/pkg/volume/csi/csi_plugin.go +++ b/pkg/volume/csi/csi_plugin.go @@ -581,34 +581,30 @@ func (p *csiPlugin) NewDetacher() (volume.Detacher, error) { }, nil } -// TODO change CanAttach to return error to propagate ability -// to support Attachment or an error - see https://github.com/kubernetes/kubernetes/issues/74810 -func (p *csiPlugin) CanAttach(spec *volume.Spec) bool { +func (p *csiPlugin) CanAttach(spec *volume.Spec) (bool, error) { driverMode, err := p.getDriverMode(spec) if err != nil { - return false + return false, err } if driverMode == ephemeralDriverMode { - klog.V(4).Info(log("driver ephemeral mode detected for spec %v", spec.Name)) - return false + klog.V(5).Info(log("plugin.CanAttach = false, ephemeral mode detected for spec %v", spec.Name())) + return false, nil } pvSrc, err := getCSISourceFromSpec(spec) if err != nil { - klog.Error(log("plugin.CanAttach failed to get info from spec: %s", err)) - return false + return false, err } driverName := pvSrc.Driver skipAttach, err := p.skipAttach(driverName) if err != nil { - klog.Error(log("plugin.CanAttach error when calling plugin.skipAttach for driver %s: %s", driverName, err)) - return false + return false, err } - return !skipAttach + return !skipAttach, nil } // TODO (#75352) add proper logic to determine device moutability by inspecting the spec. diff --git a/pkg/volume/csi/csi_plugin_test.go b/pkg/volume/csi/csi_plugin_test.go index 4043178e01b..f8f13ddd579 100644 --- a/pkg/volume/csi/csi_plugin_test.go +++ b/pkg/volume/csi/csi_plugin_test.go @@ -880,6 +880,7 @@ func TestPluginCanAttach(t *testing.T) { driverName string spec *volume.Spec canAttach bool + shouldFail bool }{ { name: "non-attachable inline", @@ -893,6 +894,19 @@ func TestPluginCanAttach(t *testing.T) { spec: volume.NewSpecFromPersistentVolume(makeTestPV("test-vol", 20, "attachable-pv", testVol), true), canAttach: true, }, + { + name: "incomplete spec", + driverName: "attachable-pv", + spec: &volume.Spec{ReadOnly: true}, + canAttach: false, + shouldFail: true, + }, + { + name: "nil spec", + driverName: "attachable-pv", + canAttach: false, + shouldFail: true, + }, } for _, test := range tests { @@ -902,10 +916,80 @@ func TestPluginCanAttach(t *testing.T) { plug, tmpDir := newTestPlugin(t, fakeCSIClient) defer os.RemoveAll(tmpDir) - pluginCanAttach := plug.CanAttach(test.spec) + pluginCanAttach, err := plug.CanAttach(test.spec) + if err != nil && !test.shouldFail { + t.Fatalf("unexected plugin.CanAttach error: %s", err) + } if pluginCanAttach != test.canAttach { t.Fatalf("expecting plugin.CanAttach %t got %t", test.canAttach, pluginCanAttach) - return + } + }) + } +} + +func TestPluginFindAttachablePlugin(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIInlineVolume, true)() + tests := []struct { + name string + driverName string + spec *volume.Spec + canAttach bool + shouldFail bool + }{ + { + name: "non-attachable inline", + driverName: "attachable-inline", + spec: volume.NewSpecFromVolume(makeTestVol("test-vol", "attachable-inline")), + canAttach: false, + }, + { + name: "attachable PV", + driverName: "attachable-pv", + spec: volume.NewSpecFromPersistentVolume(makeTestPV("test-vol", 20, "attachable-pv", testVol), true), + canAttach: true, + }, + { + name: "incomplete spec", + driverName: "attachable-pv", + spec: &volume.Spec{ReadOnly: true}, + canAttach: false, + shouldFail: true, + }, + { + name: "nil spec", + driverName: "attachable-pv", + canAttach: false, + shouldFail: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + tmpDir, err := utiltesting.MkTmpdir("csi-test") + if err != nil { + t.Fatalf("can't create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + client := fakeclient.NewSimpleClientset(getCSIDriver(test.driverName, nil, &test.canAttach)) + factory := informers.NewSharedInformerFactory(client, csiResyncPeriod) + host := volumetest.NewFakeVolumeHostWithCSINodeName( + tmpDir, + client, + nil, + "fakeNode", + factory.Storage().V1beta1().CSIDrivers().Lister(), + ) + + plugMgr := &volume.VolumePluginMgr{} + plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, host) + + plugin, err := plugMgr.FindAttachablePluginBySpec(test.spec) + if err != nil && !test.shouldFail { + t.Fatalf("unexected error calling pluginMgr.FindAttachablePluginBySpec: %s", err) + } + if (plugin != nil) != test.canAttach { + t.Fatal("expecting attachable plugin, but got nil") } }) } diff --git a/pkg/volume/fc/attacher.go b/pkg/volume/fc/attacher.go index da6149efeca..356399330f1 100644 --- a/pkg/volume/fc/attacher.go +++ b/pkg/volume/fc/attacher.go @@ -175,8 +175,8 @@ func (detacher *fcDetacher) UnmountDevice(deviceMountPath string) error { return nil } -func (plugin *fcPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *fcPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *fcPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/flexvolume/plugin.go b/pkg/volume/flexvolume/plugin.go index 780f751cf15..aab52b4d9a6 100644 --- a/pkg/volume/flexvolume/plugin.go +++ b/pkg/volume/flexvolume/plugin.go @@ -256,8 +256,8 @@ func (plugin *flexVolumeAttachablePlugin) NewDeviceUnmounter() (volume.DeviceUnm return plugin.NewDetacher() } -func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *flexVolumeAttachablePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/gcepd/attacher.go b/pkg/volume/gcepd/attacher.go index 679eeeab39a..78d8a8b1f2b 100644 --- a/pkg/volume/gcepd/attacher.go +++ b/pkg/volume/gcepd/attacher.go @@ -350,8 +350,8 @@ func (detacher *gcePersistentDiskDetacher) UnmountDevice(deviceMountPath string) return mount.CleanupMountPoint(deviceMountPath, detacher.host.GetMounter(gcePersistentDiskPluginName), false) } -func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *gcePersistentDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/iscsi/attacher.go b/pkg/volume/iscsi/attacher.go index bbb20c29043..5231414116c 100644 --- a/pkg/volume/iscsi/attacher.go +++ b/pkg/volume/iscsi/attacher.go @@ -176,8 +176,8 @@ func (detacher *iscsiDetacher) UnmountDevice(deviceMountPath string) error { return nil } -func (plugin *iscsiPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *iscsiPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *iscsiPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/photon_pd/attacher.go b/pkg/volume/photon_pd/attacher.go index c25f948f0cd..f5c001d33a7 100644 --- a/pkg/volume/photon_pd/attacher.go +++ b/pkg/volume/photon_pd/attacher.go @@ -308,8 +308,8 @@ func (detacher *photonPersistentDiskDetacher) UnmountDevice(deviceMountPath stri return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false) } -func (plugin *photonPersistentDiskPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *photonPersistentDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *photonPersistentDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/plugins.go b/pkg/volume/plugins.go index 099b30897f8..8b9fce20e9a 100644 --- a/pkg/volume/plugins.go +++ b/pkg/volume/plugins.go @@ -235,7 +235,7 @@ type AttachableVolumePlugin interface { NewAttacher() (Attacher, error) NewDetacher() (Detacher, error) // CanAttach tests if provided volume spec is attachable - CanAttach(spec *Spec) bool + CanAttach(spec *Spec) (bool, error) } // DeviceMountableVolumePlugin is an extended interface of VolumePlugin and is used @@ -891,7 +891,9 @@ func (pm *VolumePluginMgr) FindAttachablePluginBySpec(spec *Spec) (AttachableVol return nil, err } if attachableVolumePlugin, ok := volumePlugin.(AttachableVolumePlugin); ok { - if attachableVolumePlugin.CanAttach(spec) { + if canAttach, err := attachableVolumePlugin.CanAttach(spec); err != nil { + return nil, err + } else if canAttach { return attachableVolumePlugin, nil } } diff --git a/pkg/volume/rbd/attacher.go b/pkg/volume/rbd/attacher.go index 54aadee7677..8a05302a14b 100644 --- a/pkg/volume/rbd/attacher.go +++ b/pkg/volume/rbd/attacher.go @@ -71,8 +71,8 @@ func (plugin *rbdPlugin) GetDeviceMountRefs(deviceMountPath string) ([]string, e return mounter.GetMountRefs(deviceMountPath) } -func (plugin *rbdPlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *rbdPlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *rbdPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) { diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index f29f4c32992..dd09407e01e 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -492,8 +492,8 @@ func (plugin *FakeVolumePlugin) GetNewDetacherCallCount() int { return plugin.NewDetacherCallCount } -func (plugin *FakeVolumePlugin) CanAttach(spec *Spec) bool { - return true +func (plugin *FakeVolumePlugin) CanAttach(spec *Spec) (bool, error) { + return true, nil } func (plugin *FakeVolumePlugin) CanDeviceMount(spec *Spec) (bool, error) { @@ -654,8 +654,8 @@ func (f *FakeAttachableVolumePlugin) NewDetacher() (Detacher, error) { return f.Plugin.NewDetacher() } -func (f *FakeAttachableVolumePlugin) CanAttach(spec *Spec) bool { - return true +func (f *FakeAttachableVolumePlugin) CanAttach(spec *Spec) (bool, error) { + return true, nil } var _ VolumePlugin = &FakeAttachableVolumePlugin{} diff --git a/pkg/volume/vsphere_volume/attacher.go b/pkg/volume/vsphere_volume/attacher.go index 136ffd48c27..26e298bb7b7 100644 --- a/pkg/volume/vsphere_volume/attacher.go +++ b/pkg/volume/vsphere_volume/attacher.go @@ -295,8 +295,8 @@ func (detacher *vsphereVMDKDetacher) UnmountDevice(deviceMountPath string) error return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false) } -func (plugin *vsphereVolumePlugin) CanAttach(spec *volume.Spec) bool { - return true +func (plugin *vsphereVolumePlugin) CanAttach(spec *volume.Spec) (bool, error) { + return true, nil } func (plugin *vsphereVolumePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {