Add e2e tests for CSI PVCDataSources #80117

j-griffith · 2019-07-13T00:36:49Z

/kind cleanup

What this PR does / why we need it:

Add an e2e test for the VolumePVCDataSource feature, test against the CSI Hostpath driver.

None

msau42 · 2019-07-18T20:12:06Z

test/e2e/storage/testsuites/provisioning.go

+	ginkgo.By("[Initialize dataSource]creating a source PVC")
+	sourcePVC, err := client.CoreV1().PersistentVolumeClaims(source.Namespace).Create(source)
+	framework.ExpectNoError(err)
+	err = framework.WaitForPersistentVolumeClaimPhase(v1.ClaimBound, client, sourcePVC.Namespace, sourcePVC.Name, framework.Poll, framework.ClaimProvisionTimeout)


To support delayed binding, we should create the pod first, and then we can wait for pvc bound.

Maybe better to check for delayed binding and react appropriately; then the calling test can sort out what to do (it may check the same way). My thought being that rather than just making the helper in the suite work in all conditions, make sure it's expclicitly checking based on the bind mode?

Unsure if this test really cares about the different between delayed and immediate. For most tests I think it'll be simpler to handle both the same way

msau42 · 2019-07-18T20:13:24Z

test/e2e/storage/testsuites/provisioning.go

+		defer cleanup()
+
+		dc := l.config.Framework.DynamicClient
+		dataSource, cleanupFunc := preparePVCDataSourceForProvisioning(framework.NodeSelection{Name: l.config.ClientNodeName}, l.cs, dc, l.pvc, l.sc)


L244 should be updated to be "pvc-datasource-tester"

msau42 · 2019-07-18T20:16:31Z

We also need to update https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-gcp/sig-gcp-gce-config.yaml#L132 to run tests with the new feature tag.

And I think this depends on #79955 to update the sidecar in the test.

msau42 · 2019-07-22T20:33:13Z

test/e2e/storage/testsuites/provisioning.go

+
+		dc := l.config.Framework.DynamicClient
+		dataSource, cleanupFunc := preparePVCDataSourceForProvisioning(framework.NodeSelection{Name: l.config.ClientNodeName}, l.cs, dc, l.pvc, l.sc)
+		defer cleanupFunc()


maybe call this dataSourceCleanup to be more clear

changed to "dataSourceCleanup", thanks.

msau42 · 2019-07-22T20:35:49Z

test/e2e/storage/testsuites/provisioning.go

+
+	wantStatus := v1.ClaimBound
+	if *class.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer {
+		wantStatus = v1.ClaimPending


Unsure what is the purpose of checking for state if it's just going to be pending. Since the purpose of this function is not to test PVC binding, what if we just don't check for PVC Phase and just wait for the pod to succeed to populate the data? If there actually is some binding failure, it will show up as pod failed to run.

Well, as you pointed out the code before was creating a PVC and waiting for "bound" status, as a result if one were to pass in a local-disk for example or anything using "FirstConsumer" binding, then this helper function in the test suite would fail.

So by adding the check above we "know" not to fail if the test is using a WaitForFirstConsumer PVC, and we don't fail here because it never went to Bound. That way it's up to the person implementing the test to decide how they want to deal with things like checking usage against a pod etc, but it makes this suite flexible enough to be used for both.

Sure, we could just omit this check altogether if you'd rather, however I think there was a test that consumed this and relied on this to determine if a volume was properly provisioned (by checking for status Bound).

This function is specific to preparing pvc data source, so I don't think it really needs to check pvc bound state. It only cares that the volume was successfully populated with some data.

If you still want to keep the pvc bound check, then I would move the check to after the pod is successful. This is how other tests handle both immediate and delayed binding.

That works, I'll just drop it out altogether, thanks.

msau42 · 2019-07-22T22:54:26Z

test/e2e/storage/testsuites/provisioning.go

+
+	ginkgo.By("[Initialize dataSource]checking the source PVC")
+	// Get new copy of the source
+	_, err = client.CoreV1().PersistentVolumeClaims(sourcePVC.Namespace).Get(sourcePVC.Name, metav1.GetOptions{})


unsure if we need this call? We only need pvc.namespace + name for this test, which should already be set in L729.

msau42 · 2019-07-23T00:11:44Z

test/e2e/storage/testsuites/provisioning.go

+	// write namespace to the /mnt/test (= the volume).
+	ginkgo.By("[Initialize dataSource]write data to volume")
+	command := fmt.Sprintf("echo '%s' > /mnt/test/initialData", sourcePVC.GetNamespace())
+	RunInPodWithVolume(client, sourcePVC.Namespace, sourcePVC.Name, "pvc-snapshot-writer", command, node)


This should be "pvc-datasource-writer"?

msau42 · 2019-07-23T00:15:26Z

test/e2e/storage/testsuites/provisioning.go

+		dataSource, dataSourceCleanup := preparePVCDataSourceForProvisioning(framework.NodeSelection{Name: l.config.ClientNodeName}, l.cs, dc, l.pvc, l.sc)
+		defer dataSourceCleanup()
+
+		l.pvc.Spec.DataSource = dataSource


Have you tried running this test manually against hostpath driver? I'm not sure if reusing l.pvc works here because we already used it to provision the source volume.

This won't work without: kubernetes-csi/external-provisioner#309 and kubernetes-csi/csi-driver-host-path#82.

We'll need to get those merged and backported or update the image in the e2e tests for this to pass.

I'm still not sure this works. l.pvc is used to create the source pvc. We need a new PVC for the target clone.

I'll clean it up, it's poorly written as is even if it did work.

@msau42 I added a new source PVC to the init, instrumented some thing and ran though it with the new images to confirm it's working as expected.

j-griffith · 2019-08-10T21:07:09Z

Dependent changes needed:
~~kubernetes-csi/external-provisioner#325~~ Merged
~~kubernetes-csi/csi-driver-host-path#82 new tag release~~
~~#81304 - update e2e image reference to include cloning fix~~

j-griffith · 2019-08-14T19:48:45Z

/retest

j-griffith · 2019-08-14T22:55:06Z

/retest

address unit test comments

j-griffith · 2019-08-15T21:06:08Z

/retest

msau42 · 2019-08-15T22:15:41Z

/lgtm
/approve

k8s-ci-robot · 2019-08-15T22:15:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: j-griffith, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/storage/OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

j-griffith · 2019-08-15T22:49:49Z

/retest

fejta-bot · 2019-08-16T03:09:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-08-16T10:51:56Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from copejon and verult July 13, 2019 00:38

msau42 reviewed Jul 18, 2019

View reviewed changes

j-griffith force-pushed the cloning_e2e_tests branch from 077227f to d9b1a67 Compare July 19, 2019 15:57

msau42 reviewed Jul 22, 2019

View reviewed changes

j-griffith force-pushed the cloning_e2e_tests branch from d4eea8a to 7bd418d Compare July 22, 2019 23:09

msau42 reviewed Jul 23, 2019

View reviewed changes

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 1, 2019

j-griffith mentioned this pull request Aug 12, 2019

Update hostpathplugin to include clone fixes #81303

Closed

Add e2e tests for CSI PVCDataSources

47facf9

address unit test comments

j-griffith force-pushed the cloning_e2e_tests branch from 7bd418d to 47facf9 Compare August 15, 2019 19:25

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2019

k8s-ci-robot assigned msau42 Aug 15, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2019

k8s-ci-robot merged commit 1955467 into kubernetes:master Aug 16, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add e2e tests for CSI PVCDataSources #80117

Add e2e tests for CSI PVCDataSources #80117

j-griffith commented Jul 13, 2019 •

edited

msau42 Jul 18, 2019

j-griffith Jul 19, 2019

msau42 Jul 19, 2019

msau42 Jul 18, 2019

msau42 commented Jul 18, 2019

msau42 Jul 22, 2019

j-griffith Jul 22, 2019

msau42 Jul 22, 2019

j-griffith Jul 22, 2019 •

edited

msau42 Jul 22, 2019

j-griffith Jul 22, 2019

msau42 Jul 22, 2019

j-griffith Jul 22, 2019

msau42 Jul 23, 2019

j-griffith Aug 15, 2019

msau42 Jul 23, 2019

j-griffith Jul 31, 2019 •

edited

msau42 Aug 15, 2019

j-griffith Aug 15, 2019

j-griffith Aug 15, 2019

j-griffith commented Aug 10, 2019 •

edited

j-griffith commented Aug 14, 2019

j-griffith commented Aug 14, 2019

j-griffith commented Aug 15, 2019

msau42 commented Aug 15, 2019

k8s-ci-robot commented Aug 15, 2019

j-griffith commented Aug 15, 2019

fejta-bot commented Aug 16, 2019

fejta-bot commented Aug 16, 2019

Add e2e tests for CSI PVCDataSources #80117

Add e2e tests for CSI PVCDataSources #80117

Conversation

j-griffith commented Jul 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith Jul 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith Jul 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith commented Aug 10, 2019 • edited

j-griffith commented Aug 14, 2019

j-griffith commented Aug 14, 2019

j-griffith commented Aug 15, 2019

msau42 commented Aug 15, 2019

k8s-ci-robot commented Aug 15, 2019

j-griffith commented Aug 15, 2019

fejta-bot commented Aug 16, 2019

fejta-bot commented Aug 16, 2019

j-griffith commented Jul 13, 2019 •

edited

j-griffith Jul 22, 2019 •

edited

j-griffith Jul 31, 2019 •

edited

j-griffith commented Aug 10, 2019 •

edited