Skip to content

Manually create udev links if udev trigger doesn't work #2216

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
merged 1 commit into from
Nov 6, 2025
Merged

Manually create udev links if udev trigger doesn't work #2216

merged 1 commit into from
Nov 6, 2025
+167 −17

Conversation

@mattcary
Copy link
Contributor

@mattcary mattcary commented Oct 27, 2025

/kind bug

What this PR does / why we need it:
We have seen some cases in the field where udevadm --trigger may not function correctly, even when disk serial numbers are found. This PR makes sure a proper symlink is created even if udevadm --trigger doesn't work.

Manually create device paths when udevadm --trigger fails.

/assign @sunnylovestiramisu

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 27, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary

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 Oct 27, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 27, 2025
sunnylovestiramisu
sunnylovestiramisu reviewed Oct 28, 2025
View reviewed changes
test/e2e/tests/setup_e2e_test.go
)

const (
noMachineType = "none"
Copy link
Contributor

@sunnylovestiramisu sunnylovestiramisu Oct 28, 2025

Choose a reason for hiding this comment

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

Is this change related? Seems like the change for ARM test fix

Copy link
Contributor Author

@mattcary mattcary Oct 28, 2025

Choose a reason for hiding this comment

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

Weird, that was in a different PR. Let me check.

Copy link
Contributor Author

@mattcary mattcary Oct 28, 2025

Choose a reason for hiding this comment

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

Ah, I must have forked off of my arm fix rather than master. Fixed now at any rate.

@sunnylovestiramisu
Copy link
Contributor

sunnylovestiramisu commented Oct 28, 2025

Is there anyway we can test this end to end?

@mattcary
Copy link
Contributor Author

mattcary commented Oct 28, 2025

Is there anyway we can test this end to end?

Yeah, I'm thinking how to do that. I was going to at least run the regular tests and see what happened.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2025
@mattcary mattcary marked this pull request as draft October 28, 2025 20:30
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2025
@mattcary
Copy link
Contributor Author

mattcary commented Oct 28, 2025

Holding while I see if I can get an e2e test on a node with udev disabled.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2025
@mattcary mattcary marked this pull request as ready for review November 5, 2025 00:53
@k8s-ci-robot 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 Nov 5, 2025
@mattcary
Copy link
Contributor Author

mattcary commented Nov 5, 2025

/retest

@mattcary mattcary force-pushed the udev branch 3 times, most recently from 0dfa742 to 04b34fa Compare November 5, 2025 17:54
@mattcary
Manually create udev links if udev trigger doesn't work
242cee2 
Change-Id: I26bffb3eda447c8343ce39e69c34cf31616120e3
msau42
msau42 reviewed Nov 6, 2025
View reviewed changes
test/e2e/tests/single_zone_e2e_test.go
Expect(err).To(BeNil(), "Failed to rm file path %s: %v", fp, err)
})

It("Should mount if udev disabled, and remount if it's enabled again", func() {
Copy link
Contributor

@msau42 msau42 Nov 6, 2025

Choose a reason for hiding this comment

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

Can we verify a few additional scenarios?

  1. Detaching a disk with a manually created symlink cleans up the links
  2. After re-enabling udev, could it override the manually created link?

Copy link
Contributor Author

@mattcary mattcary Nov 6, 2025

Choose a reason for hiding this comment

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

I think those should be covered already in the scenario below?

@msau42
Copy link
Contributor

msau42 commented Nov 6, 2025

/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 6, 2025
@k8s-ci-robot k8s-ci-robot merged commit c3b6db3 into kubernetes-sigs:master Nov 6, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msau42 msau42 msau42 left review comments

@sunnylovestiramisu sunnylovestiramisu sunnylovestiramisu left review comments

@tyuchn tyuchn Awaiting requested review from tyuchn

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

@tonyzhc tonyzhc Awaiting requested review from tonyzhc

Assignees

@msau42 msau42

@sunnylovestiramisu sunnylovestiramisu

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

4 participants

@mattcary @k8s-ci-robot @sunnylovestiramisu @msau42