Skip to content

Commit

Permalink
Need to check VolumeContentSource if creating volume from snapshot
Browse files Browse the repository at this point in the history 
bugfix-280 fix comments

add retry count

exec 'make -k all test' failed

update print for logs

modify comment
  • Loading branch information
@zhucan
zhucan authored and zhuc committed Sep 10, 2019
1 parent b42fe10 commit 6e3602d
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 24 deletions.
36 changes: 32 additions & 4 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ const (
tokenPVNameKey = "pv.name"
tokenPVCNameKey = "pvc.name"
tokenPVCNameSpaceKey = "pvc.namespace"

deleteVolumeRetryCount = 5
)

var (
Expand Down Expand Up @@ -550,17 +552,29 @@ 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)
}
// use InBackground to retry the call, hoping the volume is deleted correctly next time.
return nil, controller.ProvisioningInBackground, capErr
}

if options.PVC.Spec.DataSource != nil && (rc.clone || rc.snapshot) {
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,
Expand Down Expand Up @@ -1065,3 +1079,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
}
98 changes: 78 additions & 20 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1603,6 +1603,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
snapshotStatusReady bool
expectedPVSpec *pvSpec
expectErr bool
notPopulated bool
}{
"provision with volume snapshot data source": {
volOpts: controller.ProvisionOptions{
Expand Down Expand Up @@ -1795,6 +1796,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)
Expand Down Expand Up @@ -1838,7 +1869,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)
Expand All @@ -1851,17 +1898,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)
}
}
}
}
Expand Down Expand Up @@ -2572,8 +2621,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)

}
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false)

Expand All @@ -2587,17 +2643,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)
}
}
}
}
Expand Down

0 comments on commit 6e3602d

Please sign in to comment.