Merge pull request #73789 from vladimirvivien/volume-plugin-can-attach

pkg/controller/volume/attachdetach/testing/testvolumespec.go

@@ -310,6 +310,10 @@ func (plugin *TestPlugin) NewDetacher() (volume.Detacher, error) {
 	return &detacher, nil
 }
 
+func (plugin *TestPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 func (plugin *TestPlugin) NewDeviceUnmounter() (volume.DeviceUnmounter, error) {
 	return plugin.NewDetacher()
 }

pkg/volume/awsebs/attacher.go

@@ -277,6 +277,10 @@ func (detacher *awsElasticBlockStoreDetacher) UnmountDevice(deviceMountPath stri
 	return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false)
 }
 
+func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 func setNodeDisk(
 	nodeDiskMap map[types.NodeName]map[*volume.Spec]bool,
 	volumeSpec *volume.Spec,

pkg/volume/azure_dd/azure_dd.go

@@ -235,6 +235,10 @@ func (plugin *azureDataDiskPlugin) NewDetacher() (volume.Detacher, error) {
 	}, nil
 }
 
+func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 func (plugin *azureDataDiskPlugin) NewDeleter(spec *volume.Spec) (volume.Deleter, error) {
 	volumeSource, _, err := getVolumeSource(spec)
 	if err != nil {

pkg/volume/cinder/attacher.go

@@ -406,6 +406,10 @@ 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 (attacher *cinderDiskAttacher) nodeInstanceID(nodeName types.NodeName) (string, error) {
 	instances, res := attacher.cinderProvider.Instances()
 	if !res {

pkg/volume/csi/csi_attacher.go

@@ -69,16 +69,6 @@ func (c *csiAttacher) Attach(spec *volume.Spec, nodeName types.NodeName) (string
 		return "", err
 	}
 
-	skip, err := c.plugin.skipAttach(csiSource.Driver)
-	if err != nil {
-		klog.Error(log("attacher.Attach failed to find if driver is attachable: %v", err))
-		return "", err
-	}
-	if skip {
-		klog.V(4).Infof(log("skipping attach for driver %s", csiSource.Driver))
-		return "", nil
-	}
-
 	node := string(nodeName)
 	pvName := spec.PersistentVolume.GetName()
 	attachID := getAttachmentName(csiSource.VolumeHandle, csiSource.Driver, node)
@@ -131,16 +121,6 @@ func (c *csiAttacher) WaitForAttach(spec *volume.Spec, _ string, pod *v1.Pod, ti
 
 	attachID := getAttachmentName(source.VolumeHandle, source.Driver, string(c.plugin.host.GetNodeName()))
 
-	skip, err := c.plugin.skipAttach(source.Driver)
-	if err != nil {
-		klog.Error(log("attacher.Attach failed to find if driver is attachable: %v", err))
-		return "", err
-	}
-	if skip {
-		klog.V(4).Infof(log("Driver is not attachable, skip waiting for attach"))
-		return "", nil
-	}
-
 	return c.waitForVolumeAttachment(source.VolumeHandle, attachID, timeout)
 }
 

pkg/volume/csi/csi_attacher_test.go

@@ -251,6 +251,16 @@ func TestAttacherWithCSIDriver(t *testing.T) {
 			csiAttacher := attacher.(*csiAttacher)
 			spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false)
 
+			pluginCanAttach := plug.CanAttach(spec)
+			if pluginCanAttach != test.expectVolumeAttachment {
+				t.Errorf("attacher.CanAttach does not match expected attachment status %t", test.expectVolumeAttachment)
+			}
+
+			if !pluginCanAttach {
+				t.Log("plugin is not attachable")
+				return
+			}
+
 			expectedAttachID := getAttachmentName("test-vol", test.driver, "node")
 			status := storage.VolumeAttachmentStatus{
 				Attached: true,
@@ -258,6 +268,7 @@ func TestAttacherWithCSIDriver(t *testing.T) {
 			if test.expectVolumeAttachment {
 				go markVolumeAttached(t, csiAttacher.k8s, fakeWatcher, expectedAttachID, status)
 			}
+
 			attachID, err := csiAttacher.Attach(spec, types.NodeName("node"))
 			if err != nil {
 				t.Errorf("Attach() failed: %s", err)
@@ -321,6 +332,13 @@ func TestAttacherWaitForVolumeAttachmentWithCSIDriver(t *testing.T) {
 			}
 			csiAttacher := attacher.(*csiAttacher)
 			spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false)
+
+			pluginCanAttach := plug.CanAttach(spec)
+			if !pluginCanAttach {
+				t.Log("plugin is not attachable")
+				return
+			}
+
 			_, err = csiAttacher.WaitForAttach(spec, "", nil, time.Second)
 			if err != nil && !test.expectError {
 				t.Errorf("Unexpected error: %s", err)

pkg/volume/csi/csi_plugin.go

@@ -439,6 +439,29 @@ func (p *csiPlugin) NewDetacher() (volume.Detacher, error) {
 	}, nil
 }
 
+func (p *csiPlugin) CanAttach(spec *volume.Spec) bool {
+	if spec.PersistentVolume == nil {
+		klog.Error(log("plugin.CanAttach test failed, spec missing PersistentVolume"))
+		return false
+	}
+
+	var driverName string
+	if spec.PersistentVolume.Spec.CSI != nil {
+		driverName = spec.PersistentVolume.Spec.CSI.Driver
+	} else {
+		klog.Error(log("plugin.CanAttach test failed, spec missing CSIPersistentVolume"))
+		return false
+	}
+
+	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 !skipAttach
+}
+
 func (p *csiPlugin) NewDeviceUnmounter() (volume.DeviceUnmounter, error) {
 	return p.NewDetacher()
 }

pkg/volume/csi/csi_plugin_test.go

@@ -375,6 +375,36 @@ func TestPluginNewDetacher(t *testing.T) {
 	}
 }
 
+func TestPluginCanAttach(t *testing.T) {
+	tests := []struct {
+		name       string
+		driverName string
+		canAttach  bool
+	}{
+		{
+			name:       "attachable",
+			driverName: "attachble-driver",
+			canAttach:  true,
+		},
+	}
+
+	for _, test := range tests {
+		csiDriver := getCSIDriver(test.driverName, nil, &test.canAttach)
+		t.Run(test.name, func(t *testing.T) {
+			fakeCSIClient := fakecsi.NewSimpleClientset(csiDriver)
+			plug, tmpDir := newTestPlugin(t, nil, fakeCSIClient)
+			defer os.RemoveAll(tmpDir)
+			spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driverName, "test-vol"), false)
+
+			pluginCanAttach := plug.CanAttach(spec)
+			if pluginCanAttach != test.canAttach {
+				t.Fatalf("expecting plugin.CanAttach %t got %t", test.canAttach, pluginCanAttach)
+				return
+			}
+		})
+	}
+}
+
 func TestPluginNewBlockMapper(t *testing.T) {
 	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIBlockVolume, true)()
 

pkg/volume/fc/attacher.go

@@ -175,6 +175,10 @@ func (detacher *fcDetacher) UnmountDevice(deviceMountPath string) error {
 	return nil
 }
 
+func (plugin *fcPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 func volumeSpecToMounter(spec *volume.Spec, host volume.VolumeHost) (*fcDiskMounter, error) {
 	fc, readOnly, err := getVolumeSource(spec)
 	if err != nil {

pkg/volume/flexvolume/plugin.go

@@ -256,6 +256,10 @@ func (plugin *flexVolumeAttachablePlugin) NewDeviceUnmounter() (volume.DeviceUnm
 	return plugin.NewDetacher()
 }
 
+func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 // ConstructVolumeSpec is part of the volume.AttachableVolumePlugin interface.
 func (plugin *flexVolumePlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volume.Spec, error) {
 	flexVolume := &api.Volume{

pkg/volume/gcepd/attacher.go

@@ -288,3 +288,7 @@ func (detacher *gcePersistentDiskDetacher) Detach(volumeName string, nodeName ty
 func (detacher *gcePersistentDiskDetacher) UnmountDevice(deviceMountPath string) error {
 	return mount.CleanupMountPoint(deviceMountPath, detacher.host.GetMounter(gcePersistentDiskPluginName), false)
 }
+
+func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}

pkg/volume/iscsi/attacher.go

@@ -176,6 +176,10 @@ func (detacher *iscsiDetacher) UnmountDevice(deviceMountPath string) error {
 	return nil
 }
 
+func (plugin *iscsiPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 func volumeSpecToMounter(spec *volume.Spec, host volume.VolumeHost, targetLocks keymutex.KeyMutex, pod *v1.Pod) (*iscsiDiskMounter, error) {
 	var secret map[string]string
 	readOnly, fsType, err := getISCSIVolumeInfo(spec)

pkg/volume/photon_pd/attacher.go

@@ -307,3 +307,7 @@ func (detacher *photonPersistentDiskDetacher) WaitForDetach(devicePath string, t
 func (detacher *photonPersistentDiskDetacher) UnmountDevice(deviceMountPath string) error {
 	return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false)
 }
+
+func (plugin *photonPersistentDiskPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}

pkg/volume/plugins.go

@@ -210,6 +210,8 @@ type AttachableVolumePlugin interface {
 	DeviceMountableVolumePlugin
 	NewAttacher() (Attacher, error)
 	NewDetacher() (Detacher, error)
+	// CanAttach tests if provided volume spec is attachable
+	CanAttach(spec *Spec) bool
 }
 
 // DeviceMountableVolumePlugin is an extended interface of VolumePlugin and is used
@@ -823,7 +825,9 @@ func (pm *VolumePluginMgr) FindAttachablePluginBySpec(spec *Spec) (AttachableVol
 		return nil, err
 	}
 	if attachableVolumePlugin, ok := volumePlugin.(AttachableVolumePlugin); ok {
-		return attachableVolumePlugin, nil
+		if attachableVolumePlugin.CanAttach(spec) {
+			return attachableVolumePlugin, nil
+		}
 	}
 	return nil, nil
 }

pkg/volume/rbd/attacher.go

@@ -71,6 +71,10 @@ func (plugin *rbdPlugin) GetDeviceMountRefs(deviceMountPath string) ([]string, e
 	return mounter.GetMountRefs(deviceMountPath)
 }
 
+func (plugin *rbdPlugin) CanAttach(spec *volume.Spec) bool {
+	return true
+}
+
 // rbdAttacher implements volume.Attacher interface.
 type rbdAttacher struct {
 	plugin  *rbdPlugin

pkg/volume/testing/testing.go

@@ -484,6 +484,10 @@ func (plugin *FakeVolumePlugin) GetNewDetacherCallCount() int {
 	return plugin.NewDetacherCallCount
 }
 
+func (plugin *FakeVolumePlugin) CanAttach(spec *Spec) bool {
+	return true
+}
+
 func (plugin *FakeVolumePlugin) Recycle(pvName string, spec *Spec, eventRecorder recyclerclient.RecycleEventRecorder) error {
 	return nil
 }
@@ -634,6 +638,10 @@ func (f *FakeAttachableVolumePlugin) NewDetacher() (Detacher, error) {
 	return f.Plugin.NewDetacher()
 }
 
+func (f *FakeAttachableVolumePlugin) CanAttach(spec *Spec) bool {
+	return true
+}
+
 var _ VolumePlugin = &FakeAttachableVolumePlugin{}
 var _ AttachableVolumePlugin = &FakeAttachableVolumePlugin{}
 

pkg/volume/vsphere_volume/attacher.go

@@ -295,6 +295,10 @@ 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 setNodeVolume(
 	nodeVolumeMap map[types.NodeName]map[*volume.Spec]bool,
 	volumeSpec *volume.Spec,