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

Jump to bottom

core/device: drop nonexistent devlink units #24522

Merged
merged 10 commits into from Sep 21, 2022
Merged

core/device: drop nonexistent devlink units #24522

merged 10 commits into from Sep 21, 2022

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Aug 31, 2022

Fixes #24518.

@yuwata yuwata added the pid1 label Aug 31, 2022
github-code-scanning[bot]
github-code-scanning bot found potential problems Aug 31, 2022
View reviewed changes
test/units/testsuite-17.08.sh Fixed Show resolved Hide resolved
test/units/testsuite-17.08.sh Fixed Show fixed Hide fixed
test/units/testsuite-17.08.sh Fixed Show fixed Hide fixed
test/units/testsuite-17.08.sh Fixed Show fixed Hide fixed
test/units/testsuite-64.sh Fixed Show fixed Hide fixed
test/units/testsuite-64.sh Fixed Show fixed Hide fixed
test/units/testsuite-64.sh Fixed Show fixed Hide fixed
@yuwata yuwata mentioned this pull request Aug 31, 2022
Closed
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch 3 times, most recently from ab0c221 to 52a4aa5 Compare September 1, 2022 01:37
github-code-scanning[bot]
github-code-scanning bot found potential problems Sep 1, 2022
View reviewed changes
test/units/testsuite-17.08.sh Fixed Show fixed Hide fixed
test/units/testsuite-17.08.sh Fixed Show fixed Hide fixed
test/units/testsuite-17.08.sh Fixed Show fixed Hide fixed
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch 3 times, most recently from 8635cb0 to a0689e0 Compare September 1, 2022 02:51
@systemd systemd deleted a comment from lgtm-com bot Sep 1, 2022
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from a0689e0 to f05bb82 Compare September 1, 2022 08:14
keszybz
keszybz reviewed Sep 8, 2022
View reviewed changes
src/core/device.c Outdated Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from f05bb82 to d955008 Compare September 8, 2022 21:08
@yuwata
Copy link
Member Author

yuwata commented Sep 8, 2022

@keszybz Thank you for the comments. All comments are addressed. PTAL.

@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from d955008 to ffdac17 Compare September 12, 2022 19:26
@keszybz
Copy link
Member

keszybz commented Sep 19, 2022

Looks reasonable. But needs a rebase now.

@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from ffdac17 to 2af5960 Compare September 19, 2022 13:05
@yuwata
Copy link
Member Author

yuwata commented Sep 19, 2022

@keszybz Rebased. PTAL.

@DaanDeMeyer
Copy link
Contributor

DaanDeMeyer commented Sep 20, 2022 •
edited

Can you moves the "Fixes" from the 5th commit to the 6th commit? I was extremely confused trying to figure out how the 5th commit could possibly fix the issue until I moved on to the 6th commit and noticed that that was the actual fix.

DaanDeMeyer
DaanDeMeyer reviewed Sep 20, 2022
View reviewed changes
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Some questions and suggestions. This one is hard to review because I lack knowledge about this part of systemd.

src/core/device.c Show resolved Hide resolved
src/core/device.c Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
src/core/device.c Outdated Show resolved Hide resolved
@DaanDeMeyer DaanDeMeyer added this to the v252 milestone Sep 20, 2022
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch 2 times, most recently from dbe277f to 675fd33 Compare September 20, 2022 13:11
@yuwata
Copy link
Member Author

yuwata commented Sep 20, 2022

@DaanDeMeyer Thank you for the review. Addressed the suggestions, and add/update comments to answer the questions.

DaanDeMeyer
DaanDeMeyer reviewed Sep 20, 2022
View reviewed changes
src/core/device.c Outdated Show resolved Hide resolved
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from 675fd33 to 57d9bdc Compare September 20, 2022 18:33
DaanDeMeyer
DaanDeMeyer requested changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Found a few more things

src/core/device.c Show resolved Hide resolved
test/units/testsuite-17.08.sh Outdated Show resolved Hide resolved
test/units/testsuite-17.08.sh Outdated Show resolved Hide resolved
test/units/testsuite-17.08.sh Outdated Show resolved Hide resolved
test/units/testsuite-64.sh Show resolved Hide resolved
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from 57d9bdc to b38765e Compare September 20, 2022 20:19
@yuwata
core/device: always update existing devlink or alias units on uevent
4228306 
Previously, existing device units for devlinks or aliases were not
removed unless the main device unit is removed. This makes all existing
device units for devlinks and aliases are checked if they are still
required, and remove if not necessary anymore.

Fixes systemd#24518.
@yuwata
core/device: refuse alias with ".."
c352110
@yuwata
core/device: check that no unit is ready and not simultaneously
a4cb8af 
This should not happen, just for safety.
@yuwata
test-64-udev-storage: check device units
9c126b2
@yuwata
test-64-storage: add test for renaming lvm volume
ede0fe0
@yuwata
test-17-udev: test that device units for nonexistent devlink are removed
fb227a3 
For issue systemd#24518.
@yuwata yuwata force-pushed the core-device-drop-nonexistent-devlink-unit branch from b38765e to fb227a3 Compare September 20, 2022 20:59
@yuwata
Copy link
Member Author

yuwata commented Sep 20, 2022

Sad NEWS.
LVM sets ENV{SYSTEMD_ALIAS}="/dev/block/$major:$minor" to forcibly creates the 'devlink' units.
So, we cannot judge where a virtual unit comes from, SYMLINK= or ENV{SYSTEMD_ALIAS}=.

Now, device_setup_devlink_units() and device_setup_alias_units() are merged into device_setup_extra_units().

PTAL.

@DaanDeMeyer DaanDeMeyer merged commit 0178ee6 into systemd:main Sep 21, 2022
44 of 45 checks passed
@keszybz
Copy link
Member

keszybz commented Sep 21, 2022

focal-amd64 failed in "upstream": TEST-38-FREEZER: FAIL ( 98 s), TEST-29-PORTABLE: FAIL (217 s). The system seems to be very slow, so I hope it's some timeout issue, unrelated to this PR.

@yuwata yuwata deleted the core-device-drop-nonexistent-devlink-unit branch September 21, 2022 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keszybz keszybz keszybz left review comments

@jamacku jamacku jamacku left review comments

@DaanDeMeyer DaanDeMeyer DaanDeMeyer approved these changes

Assignees
No one assigned
Labels
ci-failure-appears-unrelated pid1
Milestone
v252
Development

Successfully merging this pull request may close these issues.

Stale devlink device units are not cleaned up while dispatching UDEV change events
4 participants
@yuwata @keszybz @DaanDeMeyer @jamacku