diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index abe8fc99a5..b09f008b2e 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -114,6 +114,8 @@ const ( tokenPVNameKey = "pv.name" tokenPVCNameKey = "pvc.name" tokenPVCNameSpaceKey = "pvc.namespace" + + deleteVolumeRetryCount = 5 ) var ( @@ -550,10 +552,7 @@ func (p *csiProvisioner) ProvisionExt(options controller.ProvisionOptions) (*v1. delReq := &csi.DeleteVolumeRequest{ VolumeId: rep.GetVolume().GetVolumeId(), } - delReq.Secrets = provisionerCredentials - ctx, cancel := context.WithTimeout(context.Background(), p.timeout) - defer cancel() - _, err := p.csiClient.DeleteVolume(ctx, delReq) + err = cleanupVolume(p, delReq, provisionerCredentials) if err != nil { capErr = fmt.Errorf("%v. Cleanup of volume %s failed, volume is orphaned: %v", capErr, pvName, err) } @@ -561,6 +560,21 @@ func (p *csiProvisioner) ProvisionExt(options controller.ProvisionOptions) (*v1. return nil, controller.ProvisioningInBackground, capErr } + if options.PVC.Spec.DataSource != nil { + contentSource := rep.GetVolume().ContentSource + if contentSource == nil { + sourceErr := fmt.Errorf("volume content source missing") + delReq := &csi.DeleteVolumeRequest{ + VolumeId: rep.GetVolume().GetVolumeId(), + } + err = cleanupVolume(p, delReq, provisionerCredentials) + if err != nil { + sourceErr = fmt.Errorf("%v. cleanup of volume %s failed, volume is orphaned: %v", sourceErr, pvName, err) + } + return nil, controller.ProvisioningInBackground, sourceErr + } + } + pv := &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ Name: pvName, @@ -1074,3 +1088,17 @@ func isFinalError(err error) bool { // even start or failed. It is for sure not in progress. return true } + +func cleanupVolume(p *csiProvisioner, delReq *csi.DeleteVolumeRequest, provisionerCredentials map[string]string) error { + var err error = nil + delReq.Secrets = provisionerCredentials + ctx, cancel := context.WithTimeout(context.Background(), p.timeout) + defer cancel() + for i := 0; i < deleteVolumeRetryCount; i++ { + _, err = p.csiClient.DeleteVolume(ctx, delReq) + if err == nil { + break + } + } + return err +} diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index cc59770b08..8798b3fdd0 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -1610,6 +1610,7 @@ func TestProvisionFromSnapshot(t *testing.T) { snapshotStatusReady bool expectedPVSpec *pvSpec expectErr bool + notPopulated bool }{ "provision with volume snapshot data source": { volOpts: controller.ProvisionOptions{ @@ -1802,6 +1803,36 @@ func TestProvisionFromSnapshot(t *testing.T) { snapshotStatusReady: false, expectErr: true, }, + "fail not populated volume content source": { + volOpts: controller.ProvisionOptions{ + StorageClass: &storagev1.StorageClass{ + Parameters: map[string]string{}, + }, + PVName: "test-name", + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + Selector: nil, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: snapName, + Kind: "VolumeSnapshot", + APIGroup: &apiGrp, + }, + }, + }, + }, + snapshotStatusReady: true, + expectErr: true, + notPopulated: true, + }, } tmpdir := tempDir(t) @@ -1845,7 +1876,23 @@ func TestProvisionFromSnapshot(t *testing.T) { // When the following if condition is met, it is a valid create volume from snapshot // operation and CreateVolume is expected to be called. if tc.restoredVolSizeSmall == false && tc.wrongDataSource == false && tc.snapshotStatusReady { - controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + if tc.notPopulated { + out.Volume.ContentSource = nil + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + controllerServer.EXPECT().DeleteVolume(gomock.Any(), &csi.DeleteVolumeRequest{ + VolumeId: "test-volume-id", + }).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) + } else { + snapshotSource := csi.VolumeContentSource_Snapshot{ + Snapshot: &csi.VolumeContentSource_SnapshotSource{ + SnapshotId: "sid", + }, + } + out.Volume.ContentSource = &csi.VolumeContentSource{ + Type: &snapshotSource, + } + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + } } pv, err := csiProvisioner.Provision(tc.volOpts) @@ -1858,17 +1905,19 @@ func TestProvisionFromSnapshot(t *testing.T) { } if tc.expectedPVSpec != nil { - if pv.Name != tc.expectedPVSpec.Name { - t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) - } + if pv != nil { + if pv.Name != tc.expectedPVSpec.Name { + t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) + } - if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { - t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) - } + if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { + t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) + } - if tc.expectedPVSpec.CSIPVS != nil { - if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { - t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + if tc.expectedPVSpec.CSIPVS != nil { + if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { + t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + } } } } @@ -2611,8 +2660,15 @@ func TestProvisionFromPVC(t *testing.T) { } if !tc.expectErr { + volumeSource := csi.VolumeContentSource_Volume{ + Volume: &csi.VolumeContentSource_VolumeSource{ + VolumeId: tc.volOpts.PVC.Spec.DataSource.Name, + }, + } + out.Volume.ContentSource = &csi.VolumeContentSource{ + Type: &volumeSource, + } controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) - } if tc.restoredVolSizeSmall { controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) @@ -2639,17 +2695,19 @@ func TestProvisionFromPVC(t *testing.T) { } if tc.expectedPVSpec != nil { - if pv.Name != tc.expectedPVSpec.Name { - t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) - } + if pv != nil { + if pv.Name != tc.expectedPVSpec.Name { + t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) + } - if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { - t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) - } + if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { + t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) + } - if tc.expectedPVSpec.CSIPVS != nil { - if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { - t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + if tc.expectedPVSpec.CSIPVS != nil { + if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { + t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + } } } }