Skip to content

Commit a319671

Browse files
zhucanzhuc
zhucan
authored and
zhuc
committed
Need to check VolumeContentSource if creating volume from snapshot
bugfix-280 fix comments add retry count exec 'make -k all test' failed update print for logs modify comment No need to check the rc again
1 parent b42fe10 commit a319671

File tree

2 files changed

+110
-24
lines changed

2 files changed

+110
-24
lines changed

‎pkg/controller/controller.go

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,8 @@ const (
114114
tokenPVNameKey = "pv.name"
115115
tokenPVCNameKey = "pvc.name"
116116
tokenPVCNameSpaceKey = "pvc.namespace"
117+
118+
deleteVolumeRetryCount = 5
117119
)
118120

119121
var (
@@ -550,17 +552,29 @@ func (p *csiProvisioner) ProvisionExt(options controller.ProvisionOptions) (*v1.
550552
delReq := &csi.DeleteVolumeRequest{
551553
VolumeId: rep.GetVolume().GetVolumeId(),
552554
}
553-
delReq.Secrets = provisionerCredentials
554-
ctx, cancel := context.WithTimeout(context.Background(), p.timeout)
555-
defer cancel()
556-
_, err := p.csiClient.DeleteVolume(ctx, delReq)
555+
err = cleanupVolume(p, delReq, provisionerCredentials)
557556
if err != nil {
558557
capErr = fmt.Errorf("%v. Cleanup of volume %s failed, volume is orphaned: %v", capErr, pvName, err)
559558
}
560559
// use InBackground to retry the call, hoping the volume is deleted correctly next time.
561560
return nil, controller.ProvisioningInBackground, capErr
562561
}
563562

563+
if options.PVC.Spec.DataSource != nil {
564+
contentSource := rep.GetVolume().ContentSource
565+
if contentSource == nil {
566+
sourceErr := fmt.Errorf("volume content source missing")
567+
delReq := &csi.DeleteVolumeRequest{
568+
VolumeId: rep.GetVolume().GetVolumeId(),
569+
}
570+
err = cleanupVolume(p, delReq, provisionerCredentials)
571+
if err != nil {
572+
sourceErr = fmt.Errorf("%v. cleanup of volume %s failed, volume is orphaned: %v", sourceErr, pvName, err)
573+
}
574+
return nil, controller.ProvisioningInBackground, sourceErr
575+
}
576+
}
577+
564578
pv := &v1.PersistentVolume{
565579
ObjectMeta: metav1.ObjectMeta{
566580
Name: pvName,
@@ -1065,3 +1079,17 @@ func isFinalError(err error) bool {
10651079
// even start or failed. It is for sure not in progress.
10661080
return true
10671081
}
1082+
1083+
func cleanupVolume(p *csiProvisioner, delReq *csi.DeleteVolumeRequest, provisionerCredentials map[string]string) error {
1084+
var err error = nil
1085+
delReq.Secrets = provisionerCredentials
1086+
ctx, cancel := context.WithTimeout(context.Background(), p.timeout)
1087+
defer cancel()
1088+
for i := 0; i < deleteVolumeRetryCount; i++ {
1089+
_, err = p.csiClient.DeleteVolume(ctx, delReq)
1090+
if err == nil {
1091+
break
1092+
}
1093+
}
1094+
return err
1095+
}

‎pkg/controller/controller_test.go

Lines changed: 78 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1603,6 +1603,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
16031603
snapshotStatusReady bool
16041604
expectedPVSpec *pvSpec
16051605
expectErr bool
1606+
notPopulated bool
16061607
}{
16071608
"provision with volume snapshot data source": {
16081609
volOpts: controller.ProvisionOptions{
@@ -1795,6 +1796,36 @@ func TestProvisionFromSnapshot(t *testing.T) {
17951796
snapshotStatusReady: false,
17961797
expectErr: true,
17971798
},
1799+
"fail not populated volume content source": {
1800+
volOpts: controller.ProvisionOptions{
1801+
StorageClass: &storagev1.StorageClass{
1802+
Parameters: map[string]string{},
1803+
},
1804+
PVName: "test-name",
1805+
PVC: &v1.PersistentVolumeClaim{
1806+
ObjectMeta: metav1.ObjectMeta{
1807+
UID: "testid",
1808+
},
1809+
Spec: v1.PersistentVolumeClaimSpec{
1810+
Selector: nil,
1811+
Resources: v1.ResourceRequirements{
1812+
Requests: v1.ResourceList{
1813+
v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)),
1814+
},
1815+
},
1816+
AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce},
1817+
DataSource: &v1.TypedLocalObjectReference{
1818+
Name: snapName,
1819+
Kind: "VolumeSnapshot",
1820+
APIGroup: &apiGrp,
1821+
},
1822+
},
1823+
},
1824+
},
1825+
snapshotStatusReady: true,
1826+
expectErr: true,
1827+
notPopulated: true,
1828+
},
17981829
}
17991830

18001831
tmpdir := tempDir(t)
@@ -1838,7 +1869,23 @@ func TestProvisionFromSnapshot(t *testing.T) {
18381869
// When the following if condition is met, it is a valid create volume from snapshot
18391870
// operation and CreateVolume is expected to be called.
18401871
if tc.restoredVolSizeSmall == false && tc.wrongDataSource == false && tc.snapshotStatusReady {
1841-
controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
1872+
if tc.notPopulated {
1873+
out.Volume.ContentSource = nil
1874+
controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
1875+
controllerServer.EXPECT().DeleteVolume(gomock.Any(), &csi.DeleteVolumeRequest{
1876+
VolumeId: "test-volume-id",
1877+
}).Return(&csi.DeleteVolumeResponse{}, nil).Times(1)
1878+
} else {
1879+
snapshotSource := csi.VolumeContentSource_Snapshot{
1880+
Snapshot: &csi.VolumeContentSource_SnapshotSource{
1881+
SnapshotId: "sid",
1882+
},
1883+
}
1884+
out.Volume.ContentSource = &csi.VolumeContentSource{
1885+
Type: &snapshotSource,
1886+
}
1887+
controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
1888+
}
18421889
}
18431890

18441891
pv, err := csiProvisioner.Provision(tc.volOpts)
@@ -1851,17 +1898,19 @@ func TestProvisionFromSnapshot(t *testing.T) {
18511898
}
18521899

18531900
if tc.expectedPVSpec != nil {
1854-
if pv.Name != tc.expectedPVSpec.Name {
1855-
t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name)
1856-
}
1901+
if pv != nil {
1902+
if pv.Name != tc.expectedPVSpec.Name {
1903+
t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name)
1904+
}
18571905

1858-
if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) {
1859-
t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity)
1860-
}
1906+
if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) {
1907+
t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity)
1908+
}
18611909

1862-
if tc.expectedPVSpec.CSIPVS != nil {
1863-
if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) {
1864-
t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI)
1910+
if tc.expectedPVSpec.CSIPVS != nil {
1911+
if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) {
1912+
t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI)
1913+
}
18651914
}
18661915
}
18671916
}
@@ -2572,8 +2621,15 @@ func TestProvisionFromPVC(t *testing.T) {
25722621

25732622
}
25742623
if !tc.expectErr {
2624+
volumeSource := csi.VolumeContentSource_Volume{
2625+
Volume: &csi.VolumeContentSource_VolumeSource{
2626+
VolumeId: tc.volOpts.PVC.Spec.DataSource.Name,
2627+
},
2628+
}
2629+
out.Volume.ContentSource = &csi.VolumeContentSource{
2630+
Type: &volumeSource,
2631+
}
25752632
controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
2576-
25772633
}
25782634
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false)
25792635

@@ -2587,17 +2643,19 @@ func TestProvisionFromPVC(t *testing.T) {
25872643
}
25882644

25892645
if tc.expectedPVSpec != nil {
2590-
if pv.Name != tc.expectedPVSpec.Name {
2591-
t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name)
2592-
}
2646+
if pv != nil {
2647+
if pv.Name != tc.expectedPVSpec.Name {
2648+
t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name)
2649+
}
25932650

2594-
if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) {
2595-
t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity)
2596-
}
2651+
if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) {
2652+
t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity)
2653+
}
25972654

2598-
if tc.expectedPVSpec.CSIPVS != nil {
2599-
if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) {
2600-
t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI)
2655+
if tc.expectedPVSpec.CSIPVS != nil {
2656+
if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) {
2657+
t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI)
2658+
}
26012659
}
26022660
}
26032661
}

0 commit comments

Comments
 (0)