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

Fix flake with e2e test that checks detach while mount in progress #71429

Merged

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Nov 26, 2018

A volume can show up as in-use even before it gets attached
to the node.

Fixes #71426

/sig storage
/kind bug

cc @nikopen

NONE

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 26, 2018
@gnufied
Copy link
Member Author

gnufied commented Nov 26, 2018

while root cause is accurate, I need to tweak some other timings here to ensure this does not flake.

@gnufied
Copy link
Member Author

gnufied commented Nov 26, 2018

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2018
@nikopen
Copy link
Contributor

nikopen commented Nov 26, 2018

/priority critical-urgent
/milestone v1.13

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 26, 2018
@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Nov 26, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 26, 2018
@AishSundar
Copy link
Contributor

/test pull-kubernetes-e2e-gce-device-plugin-gpu

A volume can show up as in-use even before it gets attached
to the node.
@gnufied
Copy link
Member Author

gnufied commented Nov 26, 2018

So I have found another problem with this e2e - sometimes a pod can be deleted by e2e even before the kubelet starts mounting it. When this happens, the test will fail because it does not enter code path we are trying to test. To circumvent it, I had to add another sleep after pod creation. This fixes it but still isn't ideal solution.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2018
@saad-ali
Copy link
Member

@gnufied let me know when this is ready to review. If it is not possible to fix soon, we can consider moving it to flaky.

@gnufied
Copy link
Member Author

gnufied commented Nov 26, 2018

@saad-ali it is ready for review, but there is a timing issue that is kinda hard to fix because we don't know state of kubelet from outside. if a pod gets deleted before volume-manager starts mounting it, we run into case where the e2e will not reproduce the bug. I have added a sleep to prevent that and it should lessen that - but it is not a strong guarantee.

@saad-ali
Copy link
Member

Ack. Ok let's get this merged. If it is still flaky, we can just mark the test flaky.

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 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 Nov 26, 2018
@k8s-ci-robot k8s-ci-robot merged commit fad2399 into kubernetes:master Nov 27, 2018
k8s-ci-robot added a commit that referenced this pull request Dec 11, 2018
…#71429-upstream-release-1.10

Automated cherry pick of #71145: Fix bug with volume getting marked as not in-use with pending #71429: Fix flake with e2e test that checks detach while mount in
k8s-ci-robot added a commit that referenced this pull request Jan 17, 2019
…#71429-upstream-release-1.11

Automated cherry pick of #71145: Fix bug with volume getting marked as not in-use with pending #71429: Fix flake with e2e test that checks detach while mount in
k8s-ci-robot added a commit that referenced this pull request Jun 26, 2019
…#71429-upstream-release-1.12

Automated cherry pick of #71145: Fix bug with volume getting marked as not in-use with pending #71429: Fix flake with e2e test that checks detach while mount in
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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants