e2e: CSI Tests #56509

lpabon · 2017-11-28T20:10:19Z

What this PR does / why we need it:
This e2e test tests the CSI external attacher with a mock CSI plugin driver.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): kubernetes/enhancements#178

Special notes for your reviewer:

Tests features in Out-of-tree CSI Volume Plugins enhancements#178
Tests implementation of CSI Volume Plugins in Kubernetes Design Doc community#1258
Tests VolumeAttachment Object: Introduce new VolumeAttachment API Object #54463

Release note:

NONE

lpabon · 2017-11-28T20:13:25Z

/assign @msau42

k8s-ci-robot · 2017-11-28T20:14:05Z

@lpabon: GitHub didn't allow me to assign the following users: erinboyd.

Note that only kubernetes members can be assigned.

In response to this:

/assign @erinboyd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lpabon · 2017-11-28T20:15:09Z

cc @erinboyd @jsafrane

msau42 · 2017-11-28T20:18:34Z

/ok-to-test

lpabon · 2017-11-28T20:20:53Z

To test this using cluster/kube-up.sh you must enable the following:

export KUBE_RUNTIME_CONFIG="storage.k8s.io/v1alpha1=true"
export KUBE_FEATURE_GATES="CSIPersistentVolume=true

msau42 · 2017-11-28T22:49:25Z

@lpabon to enable these tests in CI, you need to modify these two places:
https://github.com/kubernetes/test-infra/blob/master/jobs/config.json#L4110
https://github.com/kubernetes/test-infra/blob/master/jobs/env/ci-kubernetes-e2e-gci-gce-alpha-features.env

Note that you cannot enable these tests in GKE until someone from google enables the v1alpha group.

lpabon · 2017-11-28T22:54:55Z

@msau42 nice! so I should add a skip if gke statement?

msau42 · 2017-11-28T22:57:04Z

you don't need a skip statement as long as you don't add it to the gke env/config files.

msau42 · 2017-11-28T23:00:49Z

test/e2e/storage/csi_volumes.go

+			By("creating a persistentvolume")
+			pv := externalAttacherPersistentVolume(cs, config, false, false, mockPluginName, mockPluginVolumeHandle)
+
+			By("creating a fake volume attacher")


shouldn't the attach detach controller be the one creating/deleting the AttachVolume object?

Currently that is not the task of the external-attacher. Instead is part of the plugin code created by #54529 . There will be another e2e test to test the K8S/CSI plugin creation of PV and volumeattach objects.

Since this test creates them manual pretending to be the k8s/csi plugin, then it is responsible to delete them.

E2E tests are supposed to test from the user's POV. If you're stubbing out different components, that is better suited for an integration or unit test.

Yeah I agree, but this is the only way to test a ...mostly.. e2e due to csi components. I will add to this test the e2e for #54529 in another PR. This one will set the foundation. Unless we wait until the full PR is ready?

Ok, I will add to this PR another commit for a full e2e test.

edisonxiang · 2017-11-29T08:28:28Z

test/e2e/storage/csi_volumes.go

+			{
+				APIGroups: []string{"storage.k8s.io"},
+				Resources: []string{"volumeattachments"},
+				Verbs:     []string{"get", "list", "watch", "update"},


Resource "nodes" need Verbs "update" from Line 423;
Resource "volumeattachments" need Verbs "delete" from Line 351.

This is policy for external-attacher started as a pod somewhere in this PR. It's not policy for the test itself.

@jsafrane Got it.

jsafrane · 2017-11-29T14:45:15Z

test/e2e/storage/csi_volumes.go

+			},
+			Volumes: []v1.Volume{
+				{
+					Name: "socket-dir",


I would expect some emptyDir here.

jsafrane · 2017-11-29T14:51:05Z

test/e2e/storage/csi_volumes.go

+		},
+	}
+
+	// Delete old VA with same name


jsafrane · 2017-11-29T14:56:31Z

test/e2e/storage/csi_volumes.go

+
+func setNodeAnnotation(
+	client clientset.Interface,
+	config framework.VolumeTestConfig,


config is not used

jsafrane · 2017-11-29T14:56:43Z

test/e2e/storage/csi_volumes.go

+
+func deleteNodeAnnotation(
+	client clientset.Interface,
+	config framework.VolumeTestConfig,


config is not used

jsafrane · 2017-11-29T14:56:53Z

test/e2e/storage/csi_volumes.go

+func waitForVolumeAttachementAttached(
+	client clientset.Interface,
+	name string,
+	ns string,


ns is not used

jsafrane · 2017-11-29T14:57:30Z

test/e2e/storage/csi_volumes.go

+		blob, err := json.Marshal(annotationMap)
+		framework.ExpectNoError(err, "Unable to marshal csi node mapping: %v", err)
+
+		n.Annotations[csiNodeAnnotationMappingKey] = string(blob)


Please remove the annotation if annotationMap is empty

lpabon · 2017-12-04T17:59:34Z

Thanks all. I will have an update with a full e2e update coming.

lpabon · 2017-12-13T21:09:47Z

/retest

lpabon · 2017-12-13T21:10:08Z

/assign @msau42

msau42 · 2017-12-14T00:18:21Z

test/e2e/storage/csi_volumes.go

+	csiDriverRegistrarImage     string = "docker.io/k8scsi/driver-registrar"
+)
+
+func externalAttacherServiceAccout(


msau42 · 2017-12-14T00:26:21Z

test/e2e/storage/csi_volumes.go

+
+			claim := newClaim(t, ns.GetName(), "")
+			class := newStorageClass(t, ns.GetName(), "")
+			testDynamicProvisioning(t, cs, claim, class)


Any plans to also test mount/unmount, attach/detach?

Right now, we would need a driver that supports attach/detach. But mount/unmount is tested by testDynamicProvisioning().

Actually, now that I think about it, the test does test it. It all depends on the driver, but the e2e test does send out all the CSI calls to attach/detach and mount/unmount. All the CSI drivers we have right now do attach/deatch and mount/unmount on the NodePublish/Unpublish calls done from the kubelet to the CSI driver. Probably when we get CSI drivers for the cloud vendors, the test would exercise attach/detach done from the external-attacher side car container to the CSI driver, then mount/unmount would be done by the kubelet to the CSI driver.

So, tldr: Yes, both are tested.

Now that I think about it, maybe I should change the text on line 227 to say something like: should provision, attach/detach, then mount/unmount storage with a hostPath CSI driver. Would that make sense?

lpabon · 2017-12-14T02:12:40Z

/retest

lpabon · 2017-12-14T02:28:09Z

/retest

lpabon · 2017-12-14T16:28:55Z

/retest

lpabon · 2017-12-14T19:51:59Z

/retest

This e2e test tests the CSI volume plugin in kubernetes with a CSI hostPath driver. It is also setup to be able to be tested with more drivers in the future.

lpabon · 2017-12-20T22:51:31Z

/retest

jsafrane

One minor nit, otherwise LGTM

jsafrane · 2018-01-08T12:10:39Z

test/e2e/storage/csi_volumes.go

+	}
+
+	clusterRoleClient.Delete(role.GetName(), &metav1.DeleteOptions{})
+	err := wait.Poll(2*time.Second, 10*time.Minute, func() (bool, error) {


I'd appreciate a constant for 10*time.Minute, as it is used on multiple times

saad-ali

/lgtm
/approve

saad-ali · 2018-01-12T07:39:42Z

test/e2e/storage/csi_hostpath.go

+					},
+				},
+				{
+					Name:            "driver-registrar",


I see that you bundled the all the external components and the driver in to the same pod. Not a huge deal, but our recommendation is two pods: 1) no more then one replica containing external-attacher, external-provisioner, and csi-driver and 2) on every node a driver-registrar and csi-driver.

We can consider this configuration for additional E2E tests in the future.

k8s-ci-robot · 2018-01-12T07:47:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lpabon, saad-ali

Associated issue: #178

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

Needs approval from an approver in each of these OWNERS Files:

~~test/e2e/storage/OWNERS~~ [saad-ali]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-01-12T07:48:21Z

/test all

Tests are more than 96 hours old. Re-running tests.

fejta-bot · 2018-01-12T11:11:18Z

/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 comment for consistent failures.

k8s-github-robot · 2018-01-12T11:56:21Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2018-01-13T04:34:54Z

Automatic merge from submit-queue (batch tested with PRs 57266, 58187, 58186, 46245, 56509). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned saad-ali and msau42 Nov 28, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 28, 2017

lpabon force-pushed the csi-e2e branch 2 times, most recently from 44e9932 to fab7712 Compare November 28, 2017 22:23

msau42 reviewed Nov 28, 2017

View reviewed changes

lpabon changed the title ~~e2e: CSI Attacher Tests~~ WIP: e2e: CSI Attacher Tests Nov 29, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2017

edisonxiang reviewed Nov 29, 2017

View reviewed changes

jsafrane reviewed Nov 29, 2017

View reviewed changes

lpabon changed the title ~~WIP: e2e: CSI Attacher Tests~~ WIP: e2e: CSI Tests Dec 4, 2017

lpabon force-pushed the csi-e2e branch from fab7712 to dfbf1fc Compare December 12, 2017 21:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 12, 2017

kubernetes deleted a comment from k8s-github-robot Dec 12, 2017

lpabon force-pushed the csi-e2e branch from e859106 to 3a8314e Compare December 13, 2017 17:12

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2017

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 13, 2017

lpabon changed the title ~~WIP: e2e: CSI Tests~~ e2e: CSI Tests Dec 13, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2017

lpabon force-pushed the csi-e2e branch from 3a8314e to 0679df4 Compare December 13, 2017 19:17

msau42 reviewed Dec 14, 2017

View reviewed changes

lpabon force-pushed the csi-e2e branch from 0679df4 to 8596d17 Compare December 14, 2017 02:08

lpabon force-pushed the csi-e2e branch from 8596d17 to c7600cf Compare December 14, 2017 16:38

e2e: CSI Volume tests

d1eb8a6

This e2e test tests the CSI volume plugin in kubernetes with a CSI hostPath driver. It is also setup to be able to be tested with more drivers in the future.

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2017

lpabon force-pushed the csi-e2e branch from c7600cf to d1eb8a6 Compare December 20, 2017 20:17

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2017

jsafrane reviewed Jan 8, 2018

View reviewed changes

saad-ali approved these changes Jan 12, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2018

k8s-github-robot merged commit 7dbb709 into kubernetes:master Jan 13, 2018

e2e: CSI Tests #56509

e2e: CSI Tests #56509

Conversation

lpabon commented Nov 28, 2017

lpabon commented Nov 28, 2017

k8s-ci-robot commented Nov 28, 2017

lpabon commented Nov 28, 2017

msau42 commented Nov 28, 2017

lpabon commented Nov 28, 2017

msau42 commented Nov 28, 2017

lpabon commented Nov 28, 2017

msau42 commented Nov 28, 2017

Choose a reason for hiding this comment

lpabon Nov 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpabon Nov 29, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpabon commented Dec 4, 2017

lpabon commented Dec 13, 2017

lpabon commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpabon Dec 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpabon commented Dec 14, 2017

lpabon commented Dec 14, 2017

lpabon commented Dec 14, 2017

lpabon commented Dec 14, 2017

lpabon commented Dec 20, 2017

jsafrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 12, 2018

k8s-github-robot commented Jan 12, 2018

fejta-bot commented Jan 12, 2018

k8s-github-robot commented Jan 12, 2018

k8s-github-robot commented Jan 13, 2018

lpabon Nov 28, 2017 •

edited

lpabon Nov 29, 2017 •

edited

lpabon Dec 14, 2017 •

edited