64-gce-disk-removal.rules: delete #51
Conversation
This rule tries to clean up mount points after their backing block
devices have been detached.
However, it does not actually work on any recent systemd-based distro.
It's been almost ten years now since `systemd-udevd` has been set up to
run in a mount namespace[[1]]. (This has been replaced by
`PrivateMounts=yes` in more recent versions.)
This means that the `umount` done here will have no effect on the
system. I've verified this is the case in a Debian 11 instance on GCP
where I detached a device that was mounted. (It's worth noting that the
initial addition of this rule, which AFAICT is this commit[[2]],
predates the systemd patch by a year which means that this rule did work
in the beginning.)
Nowadays, mounting and unmounting should be done via systemd mount
units. systemd knows to clean up stale mount entries if their underlying
devices go away. If it really must be done from a udev rule, the
`systemd-mount` command is recommended, but unmounting with this tool
will not work for our purposes here since it assumes the device is still
present.
Using `mount` interactively is fine, but then it's reasonable to expect
users to also manually `umount` once they're done with it.
Also, note that the logic here is not quite correct. If `umount` fails
(e.g. because the device wasn't actually mounted), the overall exit code
of the shell command is nonzero, which systemd will log as a udev error
like:
(udev-worker)[534]: vda2: Process '/bin/sh -c '/bin/umount -fl
/dev/vda2 && /usr/bin/logger -p daemon.warn -s WARNING: hot-removed
/dev/vda2 that was still mounted, data may have been corrupted''
failed with exit code 32.
For all the above reasons, drop this rule.
[1]: systemd/systemd@c2c13f2
[2]: GoogleCloudPlatform/compute-image-packages@502f06e
|
Hi @jlebon. Thanks for your PR. I'm waiting for a GoogleCloudPlatform member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
|
/hold |
|
This change needs some investigation as these packages have to support a lot of old distros as well. One option for you is to just not include this udev rule in your package for Fedora CoreOS. We will have to investigate if this rule is actually needed in all other places. |
|
Thanks @zmarano. That's reasonable. |
|
Ack, thanks @zmarano. OOC, what are the oldest supported distros? |
|
Technically it is still RHEL 6 (because of ELS)... but realistically it will be RHEL 7. |
Thanks. RHEL 6 doesn't ship systemd, but if I had to guess, I'd say it's likely udevd is not executed in a mount namespace (so this rule would indeed have a effect). RHEL 7 ships systemd v219, which has the mentioned commit to make |
Any updates here? |
|
It actually looks good to me. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlebon, zmarano 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 |
|
/lgtm |
1 similar comment
|
/lgtm |
|
/retest |
|
/unhold |
This rule tries to clean up mount points after their backing block devices have been detached.
However, it does not actually work on any recent systemd-based distro. It's been almost ten years now since
systemd-udevdhas been set up to run in a mount namespace[1]. (This has been replaced byPrivateMounts=yesin more recent versions.)This means that the
umountdone here will have no effect on the system. I've verified this is the case in a Debian 11 instance on GCP where I detached a device that was mounted. (It's worth noting that the initial addition of this rule, which AFAICT is this commit[2], predates the systemd patch by a year which means that this rule did work in the beginning.)Nowadays, mounting and unmounting should be done via systemd mount units. systemd knows to clean up stale mount entries if their underlying devices go away. If it really must be done from a udev rule, the
systemd-mountcommand is recommended, but unmounting with this tool will not work for our purposes here since it assumes the device is still present.Using
mountinteractively is fine, but then it's reasonable to expect users to also manuallyumountonce they're done with it.Also, note that the logic here is not quite correct. If
umountfails (e.g. because the device wasn't actually mounted), the overall exit code of the shell command is nonzero, which systemd will log as a udev error like:For all the above reasons, drop this rule.