(#1860899) Avoid a race when exiting user service #213
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
systemd-rhel-bot
added
pr/needs-ci
systemd-rhel-bot
changed the title
systemd-rhel-bot
added
pr/needs-ci
|
@dtardon Could you please rebase. Thanks |
|
@mrc0mmand Could you please have a look at CI. Thanks |
systemd-rhel-bot
added
tracker/unapproved
The service would always be in state == SERVICE_INACTIVE, but it needs to go through state == SERVICE_START so that SuccessAction/FailureAction are executed. (cherry picked from commit ef5ae8e) Related: #1860899
FailureAction=/SuccessAction= were added later then StartLimitAction=, so it was easiest to refer to the existing description. But those two settings are somewhat simpler (they just execute the action unconditionally) while StartLimitAction= has additional timing and burst parameters, and they are about to take on a more prominent role, so let's move the description of allowed values. (cherry picked from commit 454dd6c) Related: #1860899
…accept that We would accept e.g. FailureAction=reboot-force in user units and then do an exit in the user manager. Let's be stricter, and define "exit"/"exit-force" as the only supported actions in user units. v2: - rename 'exit' to 'exit-force' and add new 'exit' - add test for the parsing function (cherry picked from commit 54fcb61) Related: #1860899
Before we would only accept those "system" values, so there wasn't other chocie. Let's provide backwards compatiblity in case somebody made use of this functionality in user mode. v2: use 'exit-force' not 'exit' v3: use error value in log_syntax (cherry picked from commit 469f76f) Related: #1860899
(cherry picked from commit 3f00d37) Related: #1860899
The setting is now only looked at when considering an action for a job timeout or unit start limit. It is ignored for ctrl-alt-del, SuccessAction, SuccessFailure. v2: turn the parameter into a flag field v3: rename Options to Flags (cherry picked from commit 1710d4b) Related: #1860899
Fixes #10414. v2: - rename .service.in to .service - rename 'exit' to 'exit-force' (cherry picked from commit 631c9b7) Resolves: #1860899
(cherry picked from commit d85515e) Related: #1860899
Explicit systemctl calls remain in systemd-halt.service and the system systemd-exit.service. To convert systemd-halt, we'd need to add SuccessAction=halt-force. Halting doesn't make much sense, so let's just leave that is. systemd-exit.service will be converted in the next commit. (cherry picked from commit afa6206) Related: #1860899
…service C.f. 287419c: 'systemctl exit 42' can be used to set an exit value and pulls in exit.target, which pulls in systemd-exit.service, which calls org.fdo.Manager.Exit, which calls method_exit(), which sets the objective to MANAGER_EXIT. Allow the same to happen through SuccessAction=exit. v2: update for 'exit' and 'exit-force' (cherry picked from commit a400bd8) Related: #1860899
For example in a container we'd log: Oct 17 17:01:10 rawhide systemd[1]: Started Power-Off. Oct 17 17:01:10 rawhide systemd[1]: Forcibly powering off: unit succeeded Oct 17 17:01:10 rawhide systemd[1]: Reached target Power-Off. Oct 17 17:01:10 rawhide systemd[1]: Shutting down. and on the console we'd write (in red) [ !! ] Forcibly powering off: unit succeeded This is not useful in any way, and the fact that we're calling an "emergency action" is an internal implementation detail. Let's log about c-a-d and the watchdog actions only. (cherry picked from commit c7adcb1) Related: #1860899
dtardon
force-pushed
the
bz1860899-user-exit-race
branch
from
f94cf9a
to
fafa71a
Compare
systemd-rhel-bot
added
pr/needs-review
|
@lnykryn interesting, the GH API (as well as GH in general) had an outage and your bot just assumed that all CIs passed ;) |
|
@mrc0mmand That's known issue. We should check both check-runs and check-suites, but we check only |
I see, thank you. In the meantime, I fixed the CI issue in systemd/systemd-centos-ci#441, so the CIs should be green once again. |
|
@mrc0mmand @systemd-rhel-bot shouldn't be able to merge PR with failing CI for now. I protected all branches and CI is now required together with one review. |
Excellent, thanks! |
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.
No description provided.