Skip to content

Mark volume as uncertain after Unmount* fails #100183

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.

Sign up for GitHub

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

Jump to bottom
Merged
k8s-ci-robot merged 6 commits into from
Jun 18, 2021
Merged

Mark volume as uncertain after Unmount* fails #100183

k8s-ci-robot merged 6 commits into from
Jun 18, 2021

Conversation

@jsafrane
Copy link
Member

@jsafrane jsafrane commented Mar 12, 2021 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Mark volume device / volume mount as uncertain after UnmountDevice / UnmountVolume operation fails, because kubelet can't be sure what's the actual state of the volume. Kubelet will then call MountDevice / SetUp to make sure the volume is fully mounted when a new pod wants to use the same volume.

Similarly, update Unmap* calls for block volumes.

Which issue(s) this PR fixes:

Fixes: #100182

Special notes for your reviewer:

  • Added e2e test for NodeUnstage errors, which exercises the UnmountDevice error path.
  • Both transient and final errors result in the mount marked as uncertain. IMO it's not safe to assume that a mount is usable even after UnmountVolume / UnmountDevice final error. Better to call SetUp / MountDevice again when the mount is needed.

Does this PR introduce a user-facing change?

Fixed starting new pods after previous pod timed out unmounting its volumes.

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 12, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 12, 2021

@jsafrane: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 12, 2021
@jsafrane jsafrane force-pushed the fix-unstage-retry branch 2 times, most recently from 3da7652 to fe7d5bd Compare March 12, 2021 13:19
@yangjunmyfm192085
Copy link
Contributor

yangjunmyfm192085 commented Mar 12, 2021

/test pull-kubernetes-unit

@yangjunmyfm192085
Copy link
Contributor

yangjunmyfm192085 commented Mar 12, 2021

/test pull-kubernetes-e2e-kind

@jsafrane
Copy link
Member Author

jsafrane commented Mar 12, 2021

I tried to add e2e tests for the block mode, but the mock driver does not support block volumes.

@jsafrane
Copy link
Member Author

jsafrane commented Mar 12, 2021

Ready for review (I tried to add e2e tests for block mode, but the mock driver does not support block)

cc @gnufied @msau42 @jingxu97 PTAL

jingxu97
jingxu97 reviewed Mar 12, 2021
View reviewed changes
test/e2e/storage/csi_mock_volume.go Outdated
Copy link
Contributor

@jingxu97 jingxu97 Mar 12, 2021

Choose a reason for hiding this comment

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

For InvalidArgument, do we need to mark as uncertain?

Copy link
Member Author

@jsafrane jsafrane Mar 12, 2021

Choose a reason for hiding this comment

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

It's an example of a "final" response, where we can be reasonably sure that e.g. provisioning is not in progress. Not sure if we can apply the same logic to Unstage.

@jingxu97
Copy link
Contributor

jingxu97 commented Mar 12, 2021

/approve

@jingxu97
Copy link
Contributor

jingxu97 commented Mar 12, 2021

PR lgtm. just want to make sure other reviewers have a chance to take a look.

@jingxu97
Copy link
Contributor

jingxu97 commented Mar 25, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2021
@jingxu97 jingxu97 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 25, 2021
jsafrane added 6 commits June 16, 2021 18:39
@jsafrane
Add e2e test for NodeUnstage error cases
54ad7e4
@jsafrane
Fix UnmountDevice error cases
f4b41c0 
When UnmountDevice fails, kubelet treat the volume mount as uncertain,
because it does not know at which stage UnmountDevice failed. It may be
already partially unmonted / destroyed.

As result, MountDevice will be performer when a new Pod is started on the
node after UnmountDevice faiure.
@jsafrane
Add GetPossiblyMountedVolumesForPod to let kubelet know all volumes w…
ca934b8 
…ere unmounted

podVolumesExist() should consider also uncertain volumes (where kubelet
does not know if a volume was fully unmounted) when checking for pod's
volumes. Added GetPossiblyMountedVolumesForPod for that.

Adding uncertain mounts to GetMountedVolumesForPod would potentially break
other callers (e.g. `verifyVolumesMountedFunc`).
@jsafrane
Add PodRemovedFromVolume
2fcb5e9 
To know when a volume has been fully unmounted (incl. uncertain mounts).
@jsafrane
Refactor dswp unit tests
f795b02 
Change existing desiredStateOfWorldPopulator.findAndAddNewPods tests to use
a common initialization function.
@jsafrane
Add unit test for DSWP with uncertain volume
d5da730 
desiredStateOfWorldPopulator.findAndRemoveDeletedPods() should remove
volumes from DSW when a pod is deleted on the API server and the volume is
uncertain in ASW.
@jsafrane jsafrane force-pushed the fix-unstage-retry branch from fca9dc1 to d5da730 Compare June 16, 2021 16:42
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2021
@gnufied
Copy link
Member

gnufied commented Jun 17, 2021

/retest
/lgtm

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

k8s-ci-robot commented Jun 17, 2021

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-storage-slow d5da730 link /test pull-kubernetes-e2e-gce-storage-slow

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.

@mrunalp
Copy link
Contributor

mrunalp commented Jun 18, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, jsafrane, mrunalp

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:

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 Jun 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 4afb72a into kubernetes:master Jun 18, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 18, 2021
@jsafrane jsafrane mentioned this pull request Jun 23, 2021
Closed
@ehashman ehashman mentioned this pull request Jun 24, 2021
Closed
@msau42 msau42 mentioned this pull request Jul 7, 2021
Merged
@timebertt
Copy link
Contributor

timebertt commented Oct 1, 2021

Any chance we can backport this change?
We observed #100182 multiple times on v1.20 clusters.

@timebertt timebertt mentioned this pull request Oct 1, 2021
Closed
This was referenced Dec 14, 2021
Merged
Closed
k8s-ci-robot added a commit that referenced this pull request Jan 7, 2022
@k8s-ci-robot
Merge pull request #107014 from jsafrane/automated-cherry-pick-of-#10…
90fdd27 
…0183-upstream-release-1.21

Automated cherry pick of #100183: Mark volume as uncertain after Unmount* fails
@ialidzhikov ialidzhikov mentioned this pull request Mar 30, 2022
Closed
@ialidzhikov ialidzhikov mentioned this pull request May 25, 2022
Closed
@ankitjain28may
Copy link

ankitjain28may commented Sep 28, 2022

Any updates, can we get to know if this is getting backported to 1.20 since we are facing a lot of such issues causing production downtimes?

@torredil torredil mentioned this pull request Jan 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saad-ali saad-ali Awaiting requested review from saad-ali

3 more reviewers

@gnufied gnufied gnufied left review comments

@jingxu97 jingxu97 jingxu97 left review comments

@mrunalp mrunalp mrunalp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gnufied gnufied

@ehashman ehashman

@Random-Liu Random-Liu

@derekwaynecarr derekwaynecarr

@jingxu97 jingxu97

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test 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/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project

Milestone

v1.22

Development

Successfully merging this pull request may close these issues.

NodeStage is not called after NodeUnstage error