Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs on e2e testing for CSI plugins #143

Merged
merged 1 commit into from
May 10, 2019

Conversation

mathu97
Copy link
Contributor

@mathu97 mathu97 commented Apr 10, 2019

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 10, 2019
@k8s-ci-robot k8s-ci-robot requested a review from lpabon April 10, 2019 13:02
@k8s-ci-robot 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 Apr 10, 2019
@k8s-ci-robot k8s-ci-robot requested a review from msau42 April 10, 2019 13:02
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @mathu97. Thanks for your PR.

I'm waiting for a kubernetes-csi or kubernetes 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 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.

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.

@mathu97 mathu97 force-pushed the csi-e2e-test-docs branch from 6d5882f to 73af8a2 Compare April 10, 2019 18:39
@msau42
Copy link
Collaborator

msau42 commented Apr 11, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 11, 2019
book/src/e2e-testing.md Outdated Show resolved Hide resolved
book/src/e2e-testing.md Outdated Show resolved Hide resolved
book/src/e2e-testing.md Outdated Show resolved Hide resolved

* For example the [NFS CSI plugin](https://github.com/kubernetes-csi/csi-driver-nfs) does not support dynamic provisoning, so we would want to skip those and run only pre-provisioned tests. For such cases, you would need to write your own testdriver, which is discussed below.

In-tree storage e2e tests could be used to test CSI storage plugins. Your repo could be setup similar to how this [NFS CSI plugin](https://github.com/mathu97/csi-driver-nfs/) is setup, where the testfiles are in a test directory and the main test file is in the cmd directory.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check this into the nfs driver repo so we have a stable path?

Copy link
Contributor Author

@mathu97 mathu97 Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I was thinking that too. I'll work on a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the ability to import e2e tests came in Kubernetes 1.14, and it would be required for the driver to use that. The nfs driver currently uses Kubernetes 1.12, so that would need to be updated. How would this work, if I wanted to update the kubernetes version for csi-driver-nfs?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump everything in Gopkg.toml to *1.14 from *1.12 and dep ensure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a PR to add this to the nfs driver repo. Would need to update the links.


In-tree storage e2e tests could be used to test CSI storage plugins. Your repo could be setup similar to how this [NFS CSI plugin](https://github.com/mathu97/csi-driver-nfs/) is setup, where the testfiles are in a test directory and the main test file is in the cmd directory.

To be able import the in-tree storage tests, CSI plugin authors would need to implement a [testdriver](https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/testsuites/testdriver.go#L31) for their CSI plugin. The testdriver provides required functionality that would help setup testcases for a particular plugin.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted somewhere that the version of the test library that you import is tied to the Kubernetes version. For example, the 1.13+ version of the e2e tests runs the raw block tests by default, since the feature went beta in 1.13. If you ran it against a 1.12 cluster, it would fail if you didn't enable the alpha feature.

- `SkipUnsupportedTest(pattern testpatterns.TestPattern)`
- `PrepareTest(f *framework.Framework) (*testsuites.PerTestConfig, func())`

The `PrepareTest` method is important as it is where you will write code to deploy your driver and setup other pre-requistes that your driver requires. The nfs and hostpath testdrivers deploy the plugin by simply [using the manifests files](https://github.com/mathu97/csi-driver-nfs/blob/master/test/nfs_driver.go#L91) that you would regularly use to deploy a plugin.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tree we bring up/down the driver per test case. For running out-of-tree in a driver-specific repo, I would encourage folks to bring up their driver at the beginning/before the test instead of per-test.


Depending on your plugin's specs, you would implement other driver interaces defined [here](https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/testsuites/testdriver.go#L61). For example the [NFS testdriver](https://github.com/mathu97/csi-driver-nfs/blob/master/test/nfs_driver.go) also implements PreprovisionedVolumeTestDriver, and PreprovisionedPVTestDriver interfaces, to enable pre-provisoned tests.

After implementing the tesdriver for your CSI plugin, you would create a `csi-volumes.go` file, where the implemented testdriver is used to run in-tree storage test suites [similar to how the NFS testdriver does](https://github.com/mathu97/csi-driver-nfs/blob/master/test/csi-volumes.go#L38).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: testdriver

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also mention here that this is where you define which test suites you want to run. And you can find the list of all test suites at __

@msau42
Copy link
Collaborator

msau42 commented Apr 11, 2019

/assign @pohly

@mathu97 mathu97 changed the title [WIP] docs on e2e testing for CSI plugins docs on e2e testing for CSI plugins May 7, 2019
@k8s-ci-robot
Copy link
Contributor

@mathu97: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 7, 2019
@mathu97
Copy link
Contributor Author

mathu97 commented May 7, 2019

@msau42 I pushed a commit according to your review. Let me know what you think.

Copy link
Collaborator

@msau42 msau42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly lgtm, just some minor comments. You can squash you commits

## Setting up End to End tests for your CSI Plugin

### Prerequisites:
* A Kubernetes v1.12+ Cluster
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated to 1.14?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works with 1.13 as well, but does not with 1.12. I will update it.


In-tree storage e2e tests could be used to test CSI storage plugins. Your repo should be setup similar to how the [NFS CSI plugin](https://github.com/kubernetes-csi/csi-driver-nfs/) is setup, where the testfiles are in a `test` directory and the main test file is in the `cmd` directory.

To be able import Kubernetes in-tree storage tests, the CSI plugin would need to use **Kubernetes v1.14+** (add to plugin's GoPkg.toml, since pluggable E2E tests become available in v1.14). CSI plugin authors would also be required to implement a [testdriver](https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/testsuites/testdriver.go#L31) for their CSI plugin. The testdriver provides required functionality that would help setup testcases for a particular plugin.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for any links that are pointing to specific line numbers, can you use an url with a specific commit/tag instead of master?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes more sense

* [Functional Testing](functional-testing.md)
* [End To End Testing](e2e-tests.md)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Kubernetes to the title and page name

* [Functional Testing](functional-testing.md)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functional page was intended for the K8s e2es I believe. Can we merge the two pages somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does. Should I copy over e2e-tests.md to this page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged the two pages and squashed the commits.

@k8s-ci-robot 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 May 9, 2019
@mathu97 mathu97 force-pushed the csi-e2e-test-docs branch from e34778f to 06f037d Compare May 9, 2019 15:23
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2019
@mathu97 mathu97 force-pushed the csi-e2e-test-docs branch from ec65bec to 00886f9 Compare May 9, 2019 15:25

In-tree storage e2e tests could be used to test CSI storage plugins. Your repo should be setup similar to how the [NFS CSI plugin](https://github.com/kubernetes-csi/csi-driver-nfs/) is setup, where the testfiles are in a `test` directory and the main test file is in the `cmd` directory.

To be able import Kubernetes in-tree storage tests, the CSI plugin would need to use **Kubernetes v1.14+** (add to plugin's GoPkg.toml, since pluggable E2E tests become available in v1.14). CSI plugin authors would also be required to implement a [testdriver](https://github.com/kubernetes/kubernetes/blob/release-1.14/test/e2e/storage/testsuites/testdriver.go#L31) for their CSI plugin. The testdriver provides required functionality that would help setup testcases for a particular plugin.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you actually use a specific commit hash or tag in the url so that it's not possible for the line number to change?

Same for all links to line numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msau42 I've updated the links, can you check if they're okay now?

@mathu97 mathu97 force-pushed the csi-e2e-test-docs branch from cd103e6 to 075dadb Compare May 10, 2019 14:02
* [Kubectl](https://kubernetes.io/docs/tasks/tools/install-kubectl/#install-kubectl)

There are two ways to run end-to-end tests for your CSI Plugin
1) use [Kubernetes E2E Tests](https://github.com/kubernetes/kubernetes/tree/e84271ed8ad04c94ce3c0a6fecd9f8dff834e861/test/e2e/storage/external), by providing a DriverDefinition YAML file via a parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these higher level links where there's no specific line number referenced, I think it's fine to refer to the master for latest instructions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mathu97 mathu97 force-pushed the csi-e2e-test-docs branch from 2d2a898 to fdc8de0 Compare May 10, 2019 15:33
@msau42
Copy link
Collaborator

msau42 commented May 10, 2019

/lgtm
/approve

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mathu97, 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2019
@mathu97
Copy link
Contributor Author

mathu97 commented May 10, 2019

@msau42 does this require a release note?

@msau42
Copy link
Collaborator

msau42 commented May 10, 2019

/release-note-none

@jsafrane is there a way we can turn off the bot for just this repo?

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 10, 2019
@k8s-ci-robot k8s-ci-robot merged commit d7fe3ed into kubernetes-csi:master May 10, 2019
@jsafrane
Copy link
Contributor

@msau42, prow config allows us to either include whole kubernetes-csi or list individual repos under kubernetes-csi. AFAIK there is no option for include whole kubernetes-csi except single repo. I wanted to avoid listing of all repos there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants