Skip to content

Commit

Permalink
Add VolumeAttachment Lister to CSI Provisioner
Browse files Browse the repository at this point in the history 
Addresses #84226
This change prevents calling DeleteVolume on the CSI plugin for volumes that are still attached to a kubernetes node.
  • Loading branch information
@RaunakShah
RaunakShah committed May 22, 2020
1 parent 94aaacb commit a34a7c0
Show file tree
Hide file tree
Showing 5 changed files with 208 additions and 19 deletions.
2 changes: 2 additions & 0 deletions cmd/csi-provisioner/csi-provisioner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -237,6 +238,7 @@ func main() {
csiNodeLister,
nodeLister,
claimLister,
vaLister,
*extraCreateMetadata,
)

Expand Down
3 changes: 3 additions & 0 deletions deploy/kubernetes/rbac.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ rules:
- apiGroups: [""]
resources: ["nodes"]
verbs: ["get", "list", "watch"]
- apiGroups: ["storage.k8s.io"]
resources: ["volumeattachments"]
verbs: ["get", "list", "watch"]

---
kind: ClusterRoleBinding
Expand Down
17 changes: 16 additions & 1 deletion pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -219,6 +220,7 @@ type csiProvisioner struct {
csiNodeLister storagelistersv1.CSINodeLister
nodeLister corelisters.NodeLister
claimLister corelisters.PersistentVolumeClaimLister
vaLister storagelistersv1.VolumeAttachmentLister
extraCreateMetadata bool
}

Expand Down Expand Up @@ -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 {

Expand All @@ -307,6 +310,7 @@ func NewCSIProvisioner(client kubernetes.Interface,
csiNodeLister: csiNodeLister,
nodeLister: nodeLister,
claimLister: claimLister,
vaLister: vaLister,
extraCreateMetadata: extraCreateMetadata,
}
return provisioner
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
195 changes: 181 additions & 14 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{},
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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()
Expand Down
Loading

0 comments on commit a34a7c0

Please sign in to comment.