Repository: kubernetes-csi/external-provisioner
Pull request #409 (2020-02-21 18:54:35)

Add more tests for provsionsing from PVC as DataSource
**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:

This commit adds 6 more tests to the existing unit tests
for creating PVC given a DataSource as another PVC, IOW
cloning feature.

The tests are added in 2 categories,
- When source PVCs Status.Phase is different than,
  ClaimBound
- When clone PV source's Status.Phase is other than,
  VolumeBound

The tests did open up a code issue, where PV Status was
not checked for VolumeBound status. This is also fixed
in this commit.

Tested using `make test-go` against go compiler
version go1.13.6 linux/amd64

**Which issue(s) this PR fixes**:
NA, Added to help move clone feature to GA.

**Special notes for your reviewer**:
- If PV `Spec.ClaimRef` is non-nil, can `Status.Phase` be anything other than bound? IOW, is this test valid and needed?

**Does this PR introduce a user-facing change?**:
```release-note
NONE
```


k8s-ci-robot: Welcome @ShyamsundarR! <br><br>It looks like this is your first PR to <a href='https://github.com/kubernetes-csi/external-provisioner'>kubernetes-csi/external-provisioner</a> 🎉. Please refer to our [pull request process documentation](https://git.k8s.io/community/contributors/guide/pull-requests.md) to help your PR have a smooth ride to approval. <br><br>You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. [Here is the bot commands documentation](https://go.k8s.io/bot-commands). <br><br>You can also check if kubernetes-csi/external-provisioner has [its own contribution guidelines](https://github.com/kubernetes-csi/external-provisioner/tree/master/CONTRIBUTING.md). <br><br>You may want to refer to our [testing guide](https://git.k8s.io/community/contributors/devel/sig-testing/testing.md) if you run into trouble with your tests not passing. <br><br>If you are having difficulty getting your pull request seen, please follow the [recommended escalation practices](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#why-is-my-pull-request-not-getting-reviewed). Also, for tips and tricks in the contribution process you may want to read the [Kubernetes contributor cheat sheet](https://git.k8s.io/community/contributors/guide/contributor-cheatsheet/README.md). We want to make sure your contribution gets all the attention it needs! <br><br>Thank you, and welcome to Kubernetes. :smiley:
k8s-ci-robot: Hi @ShyamsundarR. Thanks for your PR.

I'm waiting for a [kubernetes-csi](https://github.com/orgs/kubernetes-csi/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-provisioner/pull/409#" title="Author self-approved">ShyamsundarR</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **saad-ali**
You can assign the PR to them by writing `/assign @saad-ali` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-provisioner/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["saad-ali"]} -->
ShyamsundarR: /assign @j-griffith 
/assign @msau42 

msau42: /ok-to-test
jsafrane: lgtm-ish, but refactoring to #411 - style would be helpful.
ShyamsundarR: > lgtm-ish, but refactoring to #411 - style would be helpful.

Considering the order in which PRs were submitted, I was thinking if #409 and #410 were merged first, I would rebase #411 PR against the merged master and refactor the newly added tests as well.

We could also get #411 in first and rework the other two based on that. Let me know what is preferred and I can sequence it accordingly. Thanks.

Patch:
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index 1dca7a465..cd5034939 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -822,6 +822,11 @@ func (p *csiProvisioner) getPVCSource(options controller.ProvisionOptions) (*csi
 		return nil, fmt.Errorf("claim in dataSource not bound or invalid")
 	}
 
+	if sourcePV.Status.Phase != v1.VolumeBound {
+		klog.Warningf("the source volume %s for PVC %s/%s status is \"%s\", should instead be \"%s\"", sourcePVC.Spec.VolumeName, sourcePVC.Namespace, sourcePVC.Name, sourcePV.Status.Phase, v1.VolumeBound)
+		return nil, fmt.Errorf("claim in dataSource not bound or invalid")
+	}
+
 	volumeSource := csi.VolumeContentSource_Volume{
 		Volume: &csi.VolumeContentSource_VolumeSource{
 			VolumeId: sourcePV.Spec.CSI.VolumeHandle,


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index 947e8511c..42d83134f 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -2857,7 +2857,9 @@ func TestProvisionFromPVC(t *testing.T) {
 	fakeSc2 := "fake-sc-2"
 	srcName := "fake-pvc"
 	srcNamespace := "fake-pvc-namespace"
-	invalidPVC := "invalid-pvc"
+	invalidPVC := "invalid-pv"
+	lostPVC := "lost-pvc"
+	pendingPVC := "pending-pvc"
 	pvName := "test-testi"
 	unboundPVName := "unbound-pv"
 	anotherDriverPVName := "another-class"
@@ -2875,12 +2877,13 @@ func TestProvisionFromPVC(t *testing.T) {
 
 	testcases := map[string]struct {
 		volOpts              controller.ProvisionOptions
-		clonePVName          string  // name of the PV that srcName PVC has a claim on
-		restoredVolSizeSmall bool    // set to request a larger volSize than source PVC, default false
-		restoredVolSizeBig   bool    // set to request a smaller volSize than source PVC, default false
-		expectedPVSpec       *pvSpec // set to expected PVSpec on success, for deep comparison, default nil
-		cloneUnsupported     bool    // set to state clone feature not supported in capabilities, default false
-		expectErr            bool    // set to state, test is expected to return errors, default false
+		clonePVName          string                   // name of the PV that srcName PVC has a claim on
+		restoredVolSizeSmall bool                     // set to request a larger volSize than source PVC, default false
+		restoredVolSizeBig   bool                     // set to request a smaller volSize than source PVC, default false
+		expectedPVSpec       *pvSpec                  // set to expected PVSpec on success, for deep comparison, default nil
+		cloneUnsupported     bool                     // set to state clone feature not supported in capabilities, default false
+		sourcePVStatusPhase  v1.PersistentVolumePhase // set to change source PV Status.Phase, default "Bound"
+		expectErr            bool                     // set to state, test is expected to return errors, default false
 	}{
 		"provision with pvc data source": {
 			clonePVName: pvName,
@@ -2945,6 +2948,40 @@ func TestProvisionFromPVC(t *testing.T) {
 			volOpts:     generatePVCForProvisionFromPVC(srcNamespace, invalidPVC, fakeSc1, requestedBytes),
 			expectErr:   true,
 		},
+		"provision with pvc data source when pvc status is claim pending": {
+			clonePVName: pvName,
+			volOpts:     generatePVCForProvisionFromPVC(srcNamespace, pendingPVC, fakeSc1, requestedBytes),
+			expectErr:   true,
+		},
+		"provision with pvc data source when pvc status is claim lost": {
+			clonePVName: pvName,
+			volOpts:     generatePVCForProvisionFromPVC(srcNamespace, lostPVC, fakeSc1, requestedBytes),
+			expectErr:   true,
+		},
+		"provision with pvc data source when clone pv has released status": {
+			clonePVName:         pvName,
+			volOpts:             generatePVCForProvisionFromPVC(srcNamespace, srcName, fakeSc1, requestedBytes),
+			sourcePVStatusPhase: v1.VolumeReleased,
+			expectErr:           true,
+		},
+		"provision with pvc data source when clone pv has failed status": {
+			clonePVName:         pvName,
+			volOpts:             generatePVCForProvisionFromPVC(srcNamespace, srcName, fakeSc1, requestedBytes),
+			sourcePVStatusPhase: v1.VolumeFailed,
+			expectErr:           true,
+		},
+		"provision with pvc data source when clone pv has pending status": {
+			clonePVName:         pvName,
+			volOpts:             generatePVCForProvisionFromPVC(srcNamespace, srcName, fakeSc1, requestedBytes),
+			sourcePVStatusPhase: v1.VolumePending,
+			expectErr:           true,
+		},
+		"provision with pvc data source when clone pv has available status": {
+			clonePVName:         pvName,
+			volOpts:             generatePVCForProvisionFromPVC(srcNamespace, srcName, fakeSc1, requestedBytes),
+			sourcePVStatusPhase: v1.VolumeAvailable,
+			expectErr:           true,
+		},
 		"provision with PVC using unbound PV": {
 			clonePVName: unboundPVName,
 			volOpts:     generatePVCForProvisionFromPVC(srcNamespace, srcName, fakeSc1, requestedBytes),
@@ -2989,6 +3026,10 @@ func TestProvisionFromPVC(t *testing.T) {
 			defer driver.Stop()
 
 			// Phase: setup fake objects for test
+			pvPhase := v1.VolumeBound
+			if tc.sourcePVStatusPhase != "" {
+				pvPhase = tc.sourcePVStatusPhase
+			}
 			pv := &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: pvName,
@@ -3012,6 +3053,9 @@ func TestProvisionFromPVC(t *testing.T) {
 					},
 					StorageClassName: fakeSc1,
 				},
+				Status: v1.PersistentVolumeStatus{
+					Phase: pvPhase,
+				},
 			}
 
 			unboundPV := pv.DeepCopy()
@@ -3038,9 +3082,13 @@ func TestProvisionFromPVC(t *testing.T) {
 			claim := fakeClaim(srcName, srcNamespace, "fake-claim-uid", requestedBytes, tc.clonePVName, v1.ClaimBound, &fakeSc1)
 			// Create a fake claim with invalid PV
 			invalidClaim := fakeClaim(invalidPVC, srcNamespace, "fake-claim-uid", requestedBytes, "pv-not-present", v1.ClaimBound, &fakeSc1)
-			/// Create a fake claim as source PVC storageclass nil
+			// Create a fake claim as source PVC storageclass nil
 			scNilClaim := fakeClaim("pvc-sc-nil", srcNamespace, "fake-claim-uid", requestedBytes, pvName, v1.ClaimBound, nil)
-			clientSet = fakeclientset.NewSimpleClientset(claim, scNilClaim, pv, invalidClaim, unboundPV, anotherDriverPV, pvBoundToAnotherPVCUID, pvBoundToAnotherPVCNamespace, pvBoundToAnotherPVCName)
+			// Create a fake claim, with source PVC having a lost claim status
+			lostClaim := fakeClaim(lostPVC, srcNamespace, "fake-claim-uid", requestedBytes, tc.clonePVName, v1.ClaimLost, &fakeSc1)
+			// Create a fake claim, with source PVC having a pending claim status
+			pendingClaim := fakeClaim(pendingPVC, srcNamespace, "fake-claim-uid", requestedBytes, tc.clonePVName, v1.ClaimPending, &fakeSc1)
+			clientSet = fakeclientset.NewSimpleClientset(claim, scNilClaim, pv, invalidClaim, unboundPV, anotherDriverPV, pvBoundToAnotherPVCUID, pvBoundToAnotherPVCNamespace, pvBoundToAnotherPVCName, lostClaim, pendingClaim)
 
 			// Phase: setup responses based on test case parameters
 			out := &csi.CreateVolumeResponse{