core/mount: stop generating mount units for cred mounts #32805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bluca
approved these changes
May 14, 2024
github-actions
bot
added
the
please-review
|
Important An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released. |
|
I have not reviewed this in detail, but the basic concept looks good to me. |
YHNdnzj
force-pushed
the
no-cred-mount-unit
branch
2 times, most recently
from
c9c5e21
to
722d59c
Compare
While @poettering wants to keep mount units for credential mounts, this has brought nothing but pain in real life. By generating mount units for each cred mount, we had trouble with default dependencies on them, which causes their stop jobs to race with unmounting through exec_context_destroy_credentials(). There were several attempts to workaround the problem, but none seems very graceful: systemd#26959, systemd#28787, systemd#28957, systemd#31360, systemd#32011. Also, we want to carry over credentials for services that survive soft-reboot to the new mount tree, and during the practice the stop of mount units are irritating. The mentioned problems are ultimately resolved by disabling default deps: systemd#32799. But after doing that, maybe the next question should be "why do we generate these mount units at all?" Let's revisit the whole concept here. First of all, the credential dirs are supposed to be opaque to users, and hence nobody should really reference to these mounts directly. Secondly, the lifetime of credentials is strictly bound to the service units, but nothing else. Moreover, as more and more users of credentials pop up, we could end up with hundreds of such mount units, which is something we handle poorly. And we emit useless UnitRemoved signals, etc... As discussed, it seems that eliminating these mount units is the correct way to go. No real use cases are impacted, and the lifetime management becomes sane again. Replaces systemd#32011
…voking umount command" This reverts commit 1e12256. This was an incomplete workaround of the race. Now that we stop generating mount units for credential mounts, the logic could be dropped.
YHNdnzj
force-pushed
the
no-cred-mount-unit
branch
from
722d59c
to
59614b7
Compare
keszybz
added
good-to-merge/after-next-release
and removed
please-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Material for v257.