diff --git a/cmd/csi-provisioner/csi-provisioner.go b/cmd/csi-provisioner/csi-provisioner.go index 7bbd8e1854..f1a40ca2bf 100644 --- a/cmd/csi-provisioner/csi-provisioner.go +++ b/cmd/csi-provisioner/csi-provisioner.go @@ -182,6 +182,7 @@ func main() { claimLister := factory.Core().V1().PersistentVolumeClaims().Lister() var csiNodeLister storagelistersv1.CSINodeLister + vaLister := factory.Storage().V1().VolumeAttachments().Lister() var nodeLister v1.NodeLister if ctrl.SupportsTopology(pluginCapabilities) { csiNodeLister = factory.Storage().V1().CSINodes().Lister() @@ -237,6 +238,7 @@ func main() { csiNodeLister, nodeLister, claimLister, + vaLister, *extraCreateMetadata, ) diff --git a/deploy/kubernetes/rbac.yaml b/deploy/kubernetes/rbac.yaml index 35b6880159..8d79962d26 100644 --- a/deploy/kubernetes/rbac.yaml +++ b/deploy/kubernetes/rbac.yaml @@ -50,6 +50,9 @@ rules: - apiGroups: [""] resources: ["nodes"] verbs: ["get", "list", "watch"] + - apiGroups: ["storage.k8s.io"] + resources: ["volumeattachments"] + verbs: ["get", "list", "watch"] --- kind: ClusterRoleBinding diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 417792040e..27b2c35af1 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -38,6 +38,7 @@ import ( storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" _ "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" @@ -219,6 +220,7 @@ type csiProvisioner struct { csiNodeLister storagelistersv1.CSINodeLister nodeLister corelisters.NodeLister claimLister corelisters.PersistentVolumeClaimLister + vaLister storagelistersv1.VolumeAttachmentLister extraCreateMetadata bool } @@ -284,6 +286,7 @@ func NewCSIProvisioner(client kubernetes.Interface, csiNodeLister storagelistersv1.CSINodeLister, nodeLister corelisters.NodeLister, claimLister corelisters.PersistentVolumeClaimLister, + vaLister storagelistersv1.VolumeAttachmentLister, extraCreateMetadata bool, ) controller.Provisioner { @@ -307,6 +310,7 @@ func NewCSIProvisioner(client kubernetes.Interface, csiNodeLister: csiNodeLister, nodeLister: nodeLister, claimLister: claimLister, + vaLister: vaLister, extraCreateMetadata: extraCreateMetadata, } return provisioner @@ -1006,7 +1010,6 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error { req := csi.DeleteVolumeRequest{ VolumeId: volumeId, } - // get secrets if StorageClass specifies it storageClassName := util.GetPersistentVolumeClass(volume) if len(storageClassName) != 0 { @@ -1035,6 +1038,18 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error { ctx, cancel := context.WithTimeout(context.Background(), p.timeout) defer cancel() + // Verify if volume is attached to a node before proceeding with deletion + vaList, err := p.vaLister.List(labels.Everything()) + if err != nil { + return fmt.Errorf("failed to list volumeattachments: %v", err) + } + + for _, va := range vaList { + if va.Spec.Source.PersistentVolumeName != nil && *va.Spec.Source.PersistentVolumeName == volume.Name { + return fmt.Errorf("persistentvolume %s is still attached to node %s", volume.Name, va.Spec.NodeName) + } + } + _, err = p.csiClient.DeleteVolume(ctx, &req) return err diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index f3227b575a..835406fd79 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -409,7 +409,7 @@ func TestCreateDriverReturnsInvalidCapacityDuringProvision(t *testing.T) { pluginCaps, controllerCaps := provisionCapabilities() csiProvisioner := NewCSIProvisioner(nil, 5*time.Second, "test-provisioner", "test", - 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, false) + 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false) // Requested PVC with requestedBytes storage deletePolicy := v1.PersistentVolumeReclaimDelete @@ -1674,7 +1674,7 @@ func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requested pluginCaps, controllerCaps := provisionCapabilities() csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, - nil, provisionDriverName, pluginCaps, controllerCaps, supportsMigrationFromInTreePluginName, false, csitrans.New(), nil, nil, nil, nil, tc.withExtraMetadata) + nil, provisionDriverName, pluginCaps, controllerCaps, supportsMigrationFromInTreePluginName, false, csitrans.New(), nil, nil, nil, nil, nil, tc.withExtraMetadata) out := &csi.CreateVolumeResponse{ Volume: &csi.Volume{ @@ -2410,7 +2410,7 @@ func TestProvisionFromSnapshot(t *testing.T) { pluginCaps, controllerCaps := provisionFromSnapshotCapabilities() csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, - client, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, false) + client, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false) out := &csi.CreateVolumeResponse{ Volume: &csi.Volume{ @@ -2580,11 +2580,11 @@ func TestProvisionWithTopologyEnabled(t *testing.T) { clientSet := fakeclientset.NewSimpleClientset(nodes, csiNodes) - scLister, csiNodeLister, nodeLister, claimLister, stopChan := listers(clientSet) + scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan := listers(clientSet) defer close(stopChan) csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, - csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, false) + csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false) pv, err := csiProvisioner.Provision(controller.ProvisionOptions{ StorageClass: &storagev1.StorageClass{}, @@ -2674,11 +2674,11 @@ func TestProvisionErrorHandling(t *testing.T) { nodes := buildNodes(nodeLabels, k8sTopologyBetaVersion.String()) csiNodes := buildCSINodes(topologyKeys) clientSet := fakeclientset.NewSimpleClientset(nodes, csiNodes) - scLister, csiNodeLister, nodeLister, claimLister, stopChan := listers(clientSet) + scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan := listers(clientSet) defer close(stopChan) csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, - csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, false) + csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false) csiProvisionerExt := csiProvisioner.(controller.ProvisionerExt) options := controller.ProvisionOptions{ @@ -2752,7 +2752,7 @@ func TestProvisionWithTopologyDisabled(t *testing.T) { clientSet := fakeclientset.NewSimpleClientset() pluginCaps, controllerCaps := provisionWithTopologyCapabilities() csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, - csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, false) + csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false) out := &csi.CreateVolumeResponse{ Volume: &csi.Volume{ @@ -2786,12 +2786,15 @@ func TestProvisionWithTopologyDisabled(t *testing.T) { type deleteTestcase struct { persistentVolume *v1.PersistentVolume storageClass *storagev1.StorageClass + volumeAttachment *storagev1.VolumeAttachment mockDelete bool expectErr bool } // TestDelete is a test of the delete operation func TestDelete(t *testing.T) { + pvName := "pv" + deletionTimestamp := metav1.NewTime(time.Now()) tt := map[string]deleteTestcase{ "fail - nil PV": { persistentVolume: nil, @@ -2838,6 +2841,127 @@ func TestDelete(t *testing.T) { }, expectErr: true, }, + "fail - delete when attached to node": { + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + ClaimRef: &v1.ObjectReference{ + Name: "sc-name", + }, + StorageClassName: "sc-name", + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}", + }, + }, + volumeAttachment: &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "va", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Source: storagev1.VolumeAttachmentSource{ + PersistentVolumeName: &pvName, + }, + NodeName: "node", + }, + Status: storagev1.VolumeAttachmentStatus{ + Attached: true, + }, + }, + expectErr: true, + }, + "fail - delete when volumeattachment exists but not attached to node": { + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + ClaimRef: &v1.ObjectReference{ + Name: "sc-name", + }, + StorageClassName: "sc-name", + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}", + }, + }, + volumeAttachment: &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "va", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Source: storagev1.VolumeAttachmentSource{ + PersistentVolumeName: &pvName, + }, + NodeName: "node", + }, + Status: storagev1.VolumeAttachmentStatus{ + Attached: false, + }, + }, + expectErr: true, + }, + "fail - delete when volumeattachment exists with deletionTimestamp set": { + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + ClaimRef: &v1.ObjectReference{ + Name: "sc-name", + }, + StorageClassName: "sc-name", + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}", + }, + }, + volumeAttachment: &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "va", + DeletionTimestamp: &deletionTimestamp, + }, + Spec: storagev1.VolumeAttachmentSpec{ + Source: storagev1.VolumeAttachmentSource{ + PersistentVolumeName: &pvName, + }, + NodeName: "node", + }, + }, + expectErr: true, + }, "simple - valid case": { persistentVolume: &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ @@ -2862,6 +2986,47 @@ func TestDelete(t *testing.T) { expectErr: false, mockDelete: true, }, + "simple - valid case with existing volumeattachment on different pv": { + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pv-without-attachment", + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + ClaimRef: &v1.ObjectReference{ + Name: "pvc-name", + }, + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}", + }, + }, + volumeAttachment: &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "va", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Source: storagev1.VolumeAttachmentSource{ + PersistentVolumeName: &pvName, + }, + NodeName: "node", + }, + Status: storagev1.VolumeAttachmentStatus{ + Attached: true, + }, + }, + expectErr: false, + mockDelete: true, + }, "simple - valid case with ClaimRef set": { persistentVolume: &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ @@ -2922,9 +3087,9 @@ func runDeleteTest(t *testing.T, k string, tc deleteTestcase) { } pluginCaps, controllerCaps := provisionCapabilities() - scLister, _, _, _, _ := listers(clientSet) + scLister, _, _, _, vaLister, _ := listers(clientSet) csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, - csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, nil, nil, nil, false) + csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, nil, nil, nil, vaLister, false) err = csiProvisioner.Delete(tc.persistentVolume) if tc.expectErr && err == nil { @@ -3341,11 +3506,11 @@ func TestProvisionFromPVC(t *testing.T) { }).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) } - _, _, _, claimLister, _ := listers(clientSet) + _, _, _, claimLister, _, _ := listers(clientSet) // Phase: execute the test csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, - nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, claimLister, false) + nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, claimLister, nil, false) pv, err = csiProvisioner.Provision(tc.volOpts) if tc.expectErr && err == nil { @@ -3463,7 +3628,7 @@ func TestProvisionWithMigration(t *testing.T) { pluginCaps, controllerCaps := provisionCapabilities() csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, - inTreePluginName, false, mockTranslator, nil, nil, nil, nil, false) + inTreePluginName, false, mockTranslator, nil, nil, nil, nil, nil, false) // Set up return values (AnyTimes to avoid overfitting on implementation) @@ -3621,9 +3786,11 @@ func TestDeleteMigration(t *testing.T) { defer driver.Stop() clientSet := fakeclientset.NewSimpleClientset() pluginCaps, controllerCaps := provisionCapabilities() + _, _, _, _, vaLister, stopCh := listers(clientSet) + defer close(stopCh) csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", - false, mockTranslator, nil, nil, nil, nil, false) + false, mockTranslator, nil, nil, nil, nil, vaLister, false) // Set mock return values (AnyTimes to avoid overfitting on implementation details) mockTranslator.EXPECT().IsPVMigratable(gomock.Any()).Return(tc.expectTranslation).AnyTimes() diff --git a/pkg/controller/topology_test.go b/pkg/controller/topology_test.go index e218d993c9..7fd4302941 100644 --- a/pkg/controller/topology_test.go +++ b/pkg/controller/topology_test.go @@ -393,7 +393,7 @@ func TestStatefulSetSpreading(t *testing.T) { kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes) - _, csiNodeLister, nodeLister, _, stopChan := listers(kubeClient) + _, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient) defer close(stopChan) for name, tc := range testcases { @@ -1083,7 +1083,7 @@ func TestTopologyAggregation(t *testing.T) { kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes) - _, csiNodeLister, nodeLister, _, stopChan := listers(kubeClient) + _, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient) defer close(stopChan) var selectedNode *v1.Node @@ -1403,7 +1403,7 @@ func TestPreferredTopologies(t *testing.T) { kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes) selectedNode := &nodes.Items[0] - _, csiNodeLister, nodeLister, _, stopChan := listers(kubeClient) + _, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient) defer close(stopChan) requirements, err := GenerateAccessibilityRequirements( @@ -1622,6 +1622,7 @@ func listers(kubeClient *fakeclientset.Clientset) ( storagelistersv1.CSINodeLister, corelisters.NodeLister, corelisters.PersistentVolumeClaimLister, + storagelistersv1.VolumeAttachmentLister, chan struct{}) { factory := informers.NewSharedInformerFactory(kubeClient, ResyncPeriodOfCsiNodeInformer) stopChan := make(chan struct{}) @@ -1629,7 +1630,8 @@ func listers(kubeClient *fakeclientset.Clientset) ( csiNodeLister := factory.Storage().V1().CSINodes().Lister() nodeLister := factory.Core().V1().Nodes().Lister() claimLister := factory.Core().V1().PersistentVolumeClaims().Lister() + vaLister := factory.Storage().V1().VolumeAttachments().Lister() factory.Start(stopChan) factory.WaitForCacheSync(stopChan) - return scLister, csiNodeLister, nodeLister, claimLister, stopChan + return scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan }