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

Use lsblk to safeguard against outdated symlinks #1878

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Use lsblk to safeguard against outdated symlinks #1878

merged 1 commit into from
Jan 11, 2024

Conversation

ConnorJC3
Copy link
Contributor

@ConnorJC3 ConnorJC3 commented Dec 21, 2023 •
edited
Loading

Is this a bug fix or adding new feature?

Bug fix

What is this PR about? / Why do we need it?

Fixes #1224

What testing is done?

I1221 18:37:12.769528       1 node_linux.go:140] "Comparing volume serial" expected="vol0b99a3d34724acd2b" actual="vol0b99a3d34724acd2b"

CI/Added new unit tests

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 21, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 21, 2023
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 21, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 21, 2023 •
edited
Loading

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/node_linux.go 75.7% 77.6% 1.9

@ConnorJC3
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 21, 2023
@ConnorJC3
Copy link
Contributor Author

/retest

torredil
torredil reviewed Jan 3, 2024
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

Is this fix still necessary? it appears the issue leading to #1224 has been addressed via:

@ConnorJC3
Copy link
Contributor Author

@torredil yes, I believe so:

Firstly, that fix is relatively recent and it is unlikely that slow-moving distros have picked it up. Non-security fixes can often take months or years to be distributed.

Secondly, even when all users have that fix, that issue does not cause the race condition but only exacerbates it (by increasing the chance users hit it by hogging the CPU during the hotplug). Thus, I think this fix is still valuable because:

  • The cost of the fix is very cheap (a single lsblk call per staged volume).
  • Even if the race condition that causes it is very rarely lost, the bug avoided is catastrophic (the EBS CSI Driver mounts the wrong volume, which could very easily lead to data corruption or loss).
  • The fix is very conservative, so it should not affect users except when the driver is 100% confident the wrong volume is being mounted.

torredil
torredil reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

Even if the race condition that causes it is very rarely lost, the bug avoided is catastrophic

That makes sense. In that case, agree that paying the lsblk fee is worthwhile.

/lgtm

pkg/driver/node_linux.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2024
@torredil
Copy link
Member

torredil commented Jan 8, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2024
@ConnorJC3
Copy link
Contributor Author

/retest

1 similar comment
@ConnorJC3
Copy link
Contributor Author

/retest

rdpsin
rdpsin reviewed Jan 9, 2024
View reviewed changes
pkg/driver/node_linux.go
}
} else {
// If the command fails (for example, because lsblk is not available), silently ignore the error and proceed
klog.V(5).ErrorS(err, "Ignoring lsblk failure", "canonicalDevicePath", canonicalDevicePath, "strippedVolumeName", strippedVolumeName)
Copy link
Contributor

Choose a reason for hiding this comment

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

np: How about increasing the severity? This can help customers (and us) debug this issue faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated with myself about this, so I'd like to hear other's opinions.

On one hand, a broken lsblk is an arguably incorrect installation environment.

On the other hand, it feels unhelpful to flood the logs with the same error for every staged volume if lsblk is missing from the container or similar.

Copy link
Member

Choose a reason for hiding this comment

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

Thought about this quite a bit an ultimately landed on the following as well:

it feels unhelpful to flood the logs with the same error for every staged volume if lsblk is missing from the container or similar.

AndrewSirenko
AndrewSirenko approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Cleanly done, thank you!

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 9, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024
@ConnorJC3
Copy link
Contributor Author

/retest

torredil
torredil approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3981294 into kubernetes-sigs:master Jan 11, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdpsin rdpsin rdpsin left review comments

@torredil torredil torredil approved these changes

@AndrewSirenko AndrewSirenko AndrewSirenko approved these changes

Assignees

@AndrewSirenko AndrewSirenko

@torredil torredil

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Safeguard against outdated /dev/disk/by-id/ symlinks that can lead Pod to mount the wrong volume
5 participants
@ConnorJC3 @torredil @k8s-ci-robot @AndrewSirenko @rdpsin