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.

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

Fix device uncertain errors on reboot #122211

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Fix device uncertain errors on reboot #122211

merged 3 commits into from
Dec 15, 2023

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Dec 7, 2023 •
edited

Fixes #119608

The underlying reason of this issue is - when node that has pods with raw block volumes is shutdown and assuming node is shutdown for long enough time that - pods on the node get evicted (or deleted), then when the node comes back such pods will be usually stuck in terminating state.

The reason they are stuck in terminating state is because - although volume read from pod dir gets added to ASOW in uncertain state, a mount may still be performed for the volume and when that happens usually mount fails and that causes volume to be updated as unmounted in ASOW. When this happens, it also removes deviceMountPath from the volume and hence subsequent attempt to unmap the device fails because deviceMountPath is empty.

Allow deletion of pods that use raw block volumes on node reboot

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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 Dec 7, 2023
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.29 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.29.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Dec 6 22:15:04 UTC 2023.

@k8s-ci-robot k8s-ci-robot added area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/node Categorizes an issue or PR as relevant to SIG Node. approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 7, 2023
@gnufied gnufied force-pushed the fix-uncertain-raw-block-devices branch from d0eb7b0 to ed0faca Compare December 7, 2023 03:19
@bart0sh bart0sh added this to Triage in SIG Node PR Triage Dec 7, 2023
@bart0sh
Copy link
Contributor

bart0sh commented Dec 8, 2023

/kind bug
/priority important-soon
/triage accepted

@gnufied please provide a release note and a test coverage, thanks

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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 Dec 8, 2023
@bart0sh bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Dec 8, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 11, 2023
@jingxu97
Copy link
Contributor

jingxu97 commented Dec 14, 2023 •
edited

Could you help me understand why mount could fail after node is back, and also why mount fail will remove deviceMountPath from the volume? I forgot the details there.

Yeah so once node comes back volume still appears in DSOW and since volume was previously marked as uncertain during reconstruction, kubelet tries to once mount the volume. The mount usually at this fails because Driver may simply be unavailable yet - https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/operationexecutor/operation_generator.go#L1111

When this happens, we call markDeviceErrorState which in turn tries to device as unmounted because mount failed with a final error. But when MarkDeviceAsUnmounted is called - it clears the deiveMountPath and hence any attempt to unmount device will fail in future. @PhanLe1010 posted same analysis here - longhorn/longhorn#6919 (comment)

got it, thank you for the details! The only thing is when driver is not available, if we can identify this error, we should avoid changing any state such as MarkDeviceAsUnmounted. But this is be out of scope this issue.
btw: why this issue only for raw block?

@ejweber ejweber mentioned this pull request Dec 14, 2023
Closed
@gnufied
Copy link
Member Author

gnufied commented Dec 15, 2023

got it, thank you for the details! The only thing is when driver is not available, if we can identify this error, we should avoid changing any state such as MarkDeviceAsUnmounted. But this is be out of scope this issue.

yeah I agree. Lets file a follow up issue to clean things up. I want to keep things short in this PR, because we need to backport this to older versions.

@jsafrane
Copy link
Member

/lgtm

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

LGTM label has been added.

Git tree hash: 18bb9b2d4a484b6163937c66b7f5ea8b09732ae8

@k8s-ci-robot k8s-ci-robot merged commit c633ea7 into kubernetes:master Dec 15, 2023
18 checks passed
SIG Node PR Triage automation moved this from Needs Reviewer to Done Dec 15, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Dec 15, 2023
@gnufied gnufied mentioned this pull request Dec 15, 2023
Merged
@PhanLe1010
Copy link

PhanLe1010 commented Dec 15, 2023 •
edited

Hi @gnufied

I see that we are backporting this PR to v1.29. Can we also backport it into the active branchs 1.26, 1.27, and 1.28? A lot of users hitting this issue are on older versions and not a lot of them can upgrade to v1.29 immediately.

@brandond brandond mentioned this pull request Dec 15, 2023
Closed
This was referenced Dec 19, 2023
Merged
Merged
Merged
Merged
@gnufied
Copy link
Member Author

gnufied commented Dec 19, 2023

@PhanLe1010 yeah sure.

@gnufied
Copy link
Member Author

gnufied commented Dec 19, 2023

/cherry-pick release-1.28

This was referenced Dec 19, 2023
Merged
Merged
Merged
@PhanLe1010
Copy link

Thanks a lot @gnufied! We are very much appreciated your help

@openshift-ci-robot openshift-ci-robot mentioned this pull request Dec 20, 2023
Merged
@openshift-ci-robot openshift-ci-robot mentioned this pull request Jan 4, 2024
Closed
k8s-ci-robot added a commit that referenced this pull request Jan 10, 2024
@k8s-ci-robot
Merge pull request #122397 from gnufied/automated-cherry-pick-of-#122…
3fa2a2c 
…211-upstream-release-1.27

Automated cherry pick of #122211: Fix device uncertain errors on reboot
k8s-ci-robot added a commit that referenced this pull request Jan 10, 2024
@k8s-ci-robot
Merge pull request #122398 from gnufied/automated-cherry-pick-of-#122…
ac91ad8 
…211-upstream-release-1.26

Automated cherry pick of #122211: Fix device uncertain errors on reboot
k8s-ci-robot added a commit that referenced this pull request Jan 10, 2024
@k8s-ci-robot
Merge pull request #122396 from gnufied/automated-cherry-pick-of-#122…
95396c1 
…211-upstream-release-1.28

Automated cherry pick of #122211: Fix device uncertain errors on reboot
k8s-ci-robot added a commit that referenced this pull request Jan 10, 2024
@k8s-ci-robot
Merge pull request #122345 from gnufied/automated-cherry-pick-of-#122…
e1520ad 
…211-upstream-release-1.29

Automated cherry pick of #122211: Fix device uncertain errors on reboot
@gnufied gnufied mentioned this pull request Jan 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@jingxu97 jingxu97 jingxu97 left review comments

@ZhangShuaiyi ZhangShuaiyi ZhangShuaiyi left review comments

@mauriciopoppe mauriciopoppe Awaiting requested review from mauriciopoppe

@saikat-royc saikat-royc Awaiting requested review from saikat-royc

Assignees

@jsafrane jsafrane

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Archived in project
SIG Node PR Triage
Done
Milestone
v1.30
7 participants
@gnufied @k8s-ci-robot @bart0sh @jingxu97 @jsafrane @PhanLe1010 @ZhangShuaiyi