add repetition loop to test idempotency

[okartau]

Repeated volume operations helps to test the idempotency requirement.

What type of PR is this?
/kind feature

What this PR does / why we need it:

CSI spec requires driver to tolerate repeated volume operations by the idempotency requirements.
In pmem-csi driver context we discovered that some volume operations failed when requests were repeated. i.e. we failed to conform to idempotency requirements. However, the test suite does not repeat volume options in most cases, so here is attempt to add this.
There is config option that makes it possible to select repeat count, or bypass the repeated part.

Does this PR introduce a user-facing change?:

If a component using csi-test does not implement all idempotency requirements, this addition may start to expose some new failures.

Added (optional) repeated operations to test idempotency

[k8s-ci-robot]

Welcome @okartau!

It looks like this is your first PR to kubernetes-csi/csi-test 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

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.

You can also check if kubernetes-csi/csi-test has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @okartau. 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.

[pohly]

/ok-to-test

[pohly]

Please add a release note in the PR description. This is worth mentioning when doing the next csi-test release.

The test-fmt checking failed in the Travis CI.

[okartau]

The failure in
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-test/229/pull-kubernetes-csi-csi-test/1185891170576764931
seems to point out the idempotency related issue in the hostpath driver, where repated NodeUnpublish does not succeed?
We had a similar one in pmem-csi when I started to add those repeated tests.

[pohly]

The failure in
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-test/229/pull-kubernetes-csi-csi-test/1185891170576764931
seems to point out the idempotency related issue in the hostpath driver, where repated NodeUnpublish does not succeed?

In that case we'll have to fix the hostpath driver first such that it passes these new tests.

[pohly]

Please add a release note in the PR description.

Still open.

[pohly]

/assign

[okartau]

Please add a release note in the PR description.

Still open.

I added a short note, is this good?

[pohly]

repeated Node Volume operations

That note made me realized that the repetition count really is only applied to node operations - somehow I missed that earlier.

All operations must be idempotent, including CreateVolume and ControllerPublishVolume. Can you add that and update the changelog entry accordingly?

It would be nicer if the idempotency test wasn't under node.go because some CSI driver might only support the controller operations (for example, during initial development), but I think it's okay as it is because eventually the node operations are needed and then one can run the test.

[okartau]

Pushed version with Volume lifecycle separated as function which can be called with repeat-count argument. Also, removing the duplicate part from Unpublish side, in separate commit.
I think we can improve It and By strings a bit, currently used
It("should return appropriate values (no optional values added)"
(which I did not changed yet in this PR)
seems to be used as copied around template used in many places and not a good fit,
as we are not checking any optional values in that code.

[okartau]

This lacked skip check so it failed the CI run. Added skip andtry pushed.
Q: Is the location of those what I called "Lifecycle" tests now good, it's between Publish and Unpublish tests. Or would it be better to put it after Unpublish part.

[pohly]

Is the location of those what I called "Lifecycle" tests now good, it's between Publish and Unpublish tests. Or would it be better to put it after Unpublish part.

I don't have a strong preference either way. Let's just keep it where it is.

Some minor nits, otherwise looking good now.

[okartau]

addressed latest comments and force-pushed.

[pohly]

/lgtm

[pohly]

Let's see if someone else has an opinion about this. If not, then I'll approve.

[pohly]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: okartau, pohly

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:

• OWNERS [pohly]

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