Add NodeStage error tests by jsafrane · Pull Request #89041 · kubernetes/kubernetes

jsafrane · 2020-03-11T12:10:29Z

What this PR does / why we need it:
Add some test for NodeStage error handling. The main purpose is to test that:

NodeUnstage is called after NodeStage transient error && corresponding pod is deleted.
NodeUnstage is not called after NodeStage final error && corresponding pod is deleted.

The test (and whole CSI mock output handling) becomes quite complex.

This is just an exercise how to use the javascript hooks. If we decide this is useful, it can be extended to test also NodePublish (+ block mode of both).

@gnufied @tsmetana @pohly @msau42 @jingxu97, is it useful? We already have unit tests for this behavior.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/kind cleanup
/sig storage

jsafrane · 2020-03-12T10:22:32Z

the newly introduced tests are flaky, https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/89041/pull-kubernetes-e2e-gce-storage-slow/1237750118531207170/:

test/e2e/storage/csi_mock_volume.go:664
Mar 11 15:26:33.230: while waiting for initial CSI calls
Unexpected error:
    <*errors.errorString | 0xc001da7a10>: {
        s: "could not load CSI driver logs: the server rejected our request for an unknown reason (get pods csi-mockplugin-0)",
    }
    could not load CSI driver logs: the server rejected our request for an unknown reason (get pods csi-mockplugin-0)
occurred
test/e2e/storage/csi_mock_volume.go:688

/hold
/retest

jsafrane · 2020-03-12T12:58:26Z

/test pull-kubernetes-e2e-gce-storage-slow

jsafrane · 2020-03-12T15:05:33Z

/test pull-kubernetes-e2e-gce-storage-slow

jsafrane · 2020-03-20T11:20:59Z

Reworked to merged version of javascript hooks.

The only WIP item is bump of csi-mock driver version to canary. We should do a proper release before merging this PR.

k8s-ci-robot · 2020-03-20T11:21:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/e2e/storage/OWNERS~~ [jsafrane]
~~test/e2e/testing-manifests/storage-csi/OWNERS~~ [jsafrane]

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

jsafrane · 2020-03-20T12:15:10Z

/test pull-kubernetes-e2e-gce-storage-slow

jsafrane · 2020-03-24T11:24:19Z

/test pull-kubernetes-e2e-gce-storage-slow
/retest

jsafrane · 2020-03-24T13:21:41Z

/test pull-kubernetes-e2e-gce-storage-slow

jsafrane · 2020-03-24T16:40:23Z

/test pull-kubernetes-e2e-gce-storage-slow

jsafrane · 2020-04-01T09:39:25Z

/hold cancel
Rebased to mock v3.1.0 with the javascript hooks.

pohly · 2020-04-01T11:02:51Z

Is the new test flaky? There's a "could not load CSI driver logs: the server rejected our request for an unknown reason (get pods csi-mockplugin-0)" in https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/89041/pull-kubernetes-e2e-gce-storage-slow/1245284273221537792/.

/retest

pohly · 2020-04-01T11:07:42Z

This failed because "the server rejected our request for an unknown reason (get pods csi-mockplugin-0)" (https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/89041/pull-kubernetes-e2e-gce-storage-slow/1245284273221537792/).

Perhaps a retry loop would help?

There is already a retry loop in the caller (via wait.Poll). Perhaps failures to retrieve log output should simply be treated here as "no output", i.e. return nil, nil?

Maybe the pod is not Running yet. The new mock test calls createPod() and immediately after that it reads the pod logs. I added wait for PVC to get bound in between - the driver must be fully operational to provision a PV.

Sorry about the failed build noise. Stupid typo...

I don't get it. The mock driver provisioned a PV, yet it gets error from API server:

could not load CSI driver logs: the server rejected our request for an unknown reason (get pods csi-mockplugin-0)

There is already a retry loop in the caller (via wait.Poll). Perhaps failures to retrieve log output should simply be treated here as "no output", i.e. return nil, nil?

I've implemented that idea in #88114 (3cf9ab1) and got all tests to pass. The log does indeed show that "the server rejected our request for an unknown reason" occurred, but tests succeeded after ignoring that error (https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/88114/pull-kubernetes-e2e-kind/1245706940810530817/build-log.txt).

I added your last commit from #88114, hoping it helps.

I can't reproduce "the server rejected our request for an unknown reason" on any of my clusters.

I can't reproduce "the server rejected our request for an unknown reason" on any of my clusters.

Me neither. I think saw it once while working on some other test, but as far as I remember, that then turned out to be because I was I was asking for logs after the pod had just been deleted. Perhaps here we have the inverse, asking for a very recently started pod? Just wondering.

gnufied · 2020-04-01T11:59:52Z

/assign

Especially related to "uncertain" global mounts. A large refactoring of CSI mock tests were necessary: - to be able to script the driver to return errors as required by the test - to parse the CSI driver logs to check kubelet called the right CSI calls

As seen in some test runs (https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/89041), retrieving output can fail with "the server rejected our request for an unknown reason (get pods csi-mockplugin-0)". If this truly an intermittent error, then the existing retry logic in the callers can deal with this.

pohly · 2020-04-06T14:38:21Z

/retest

jsafrane · 2020-04-07T07:04:43Z

/retest

k8s-ci-robot · 2020-04-07T08:13:22Z

@jsafrane: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	`981aae3`	link	`/test pull-kubernetes-node-e2e-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

pohly

/lgtm

k8s-ci-robot requested review from copejon and mkumatag March 11, 2020 12:12

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 11, 2020

jsafrane force-pushed the stage-error-tests branch 3 times, most recently from 869f30a to 6e4686d Compare March 11, 2020 14:39

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2020

jsafrane mentioned this pull request Mar 20, 2020

Log full error code kubernetes-csi/csi-test#254

Merged

jsafrane force-pushed the stage-error-tests branch from d23a444 to 5b6ea42 Compare March 20, 2020 11:20

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2020

pohly mentioned this pull request Mar 24, 2020

Mock topology testing kubernetes-csi/csi-test#249

Merged

This was referenced Mar 26, 2020

e2e/storage: check result of WaitForPersistentVolumeDeleted #88118

Merged

CHANGELOG-3.1.md: v3.1.0 release kubernetes-csi/csi-test#256

Merged

jsafrane force-pushed the stage-error-tests branch from 5b6ea42 to 0cc6363 Compare April 1, 2020 09:37

jsafrane changed the title ~~WIP: Add NodeStage error tests~~ Add NodeStage error tests Apr 1, 2020

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 Apr 1, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

pohly reviewed Apr 1, 2020

View reviewed changes

k8s-ci-robot assigned gnufied Apr 1, 2020

jsafrane force-pushed the stage-error-tests branch 3 times, most recently from 366b04d to 12fdc04 Compare April 1, 2020 14:44

pohly mentioned this pull request Apr 2, 2020

storage capacity tests #88114

Merged

jsafrane and others added 3 commits April 6, 2020 15:03

Test NodeStage error cases

a4f0808

Especially related to "uncertain" global mounts. A large refactoring of CSI mock tests were necessary: - to be able to script the driver to return errors as required by the test - to parse the CSI driver logs to check kubelet called the right CSI calls

Update to new javascript

e23a26a

jsafrane force-pushed the stage-error-tests branch from 12fdc04 to 981aae3 Compare April 6, 2020 13:07

pohly reviewed Apr 7, 2020

View reviewed changes

k8s-ci-robot assigned pohly Apr 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020

k8s-ci-robot merged commit 15bb54c into kubernetes:master Apr 7, 2020

k8s-ci-robot added this to the v1.19 milestone Apr 7, 2020

Conversation

jsafrane commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsafrane commented Mar 12, 2020

Uh oh!

jsafrane commented Mar 12, 2020

Uh oh!

jsafrane commented Mar 12, 2020

Uh oh!

jsafrane commented Mar 20, 2020

Uh oh!

k8s-ci-robot commented Mar 20, 2020

Uh oh!

jsafrane commented Mar 20, 2020

Uh oh!

jsafrane commented Mar 24, 2020

Uh oh!

jsafrane commented Mar 24, 2020

Uh oh!

jsafrane commented Mar 24, 2020

Uh oh!

jsafrane commented Apr 1, 2020

Uh oh!

pohly commented Apr 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Apr 1, 2020

Uh oh!

pohly commented Apr 6, 2020

Uh oh!

jsafrane commented Apr 7, 2020

Uh oh!

k8s-ci-robot commented Apr 7, 2020

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsafrane commented Mar 11, 2020 •

edited

Loading