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.

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

units: do not soft-reboot before soft-reboot.target reached #32880

Merged
merged 2 commits into from
May 17, 2024
Merged

units: do not soft-reboot before soft-reboot.target reached #32880

merged 2 commits into from
May 17, 2024

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented May 17, 2024 •
edited by github-actions bot

Otherwise, at the time systemd-soft-reboot.service succeeds,
services which has Conflicts= and Before=soft-reboot.target may
not be stopped yet, and may be SIGKILLed.

Especially, systemd-journald.service has the dependencies, thus
journal may be corrupted. See #32223.

Follow-up for 13ffc60.
Fixes #32834.

@yuwata
units: drop dependencies of soft-reboot.target from systemd-journald@…
11a55b1 
….service

The service deos not have DefaultDependencies=no. Hence it has dependencies
of shutdown.target, and dependencies of soft-reboot.target are not
necessary.

Follow-up for f89985c.
@yuwata
units: do not soft-reboot before soft-reboot.target reached
4263d76 
Otherwise, at the time systemd-soft-reboot.service succeeds,
services which has Conflicts= and Before=soft-reboot.target may
not be stopped yet, and may be SIGKILLed.

Especially, systemd-journald.service has the dependencies, thus
journal may be corrupted. See systemd#32223.

Follow-up for 13ffc60.

Fixes systemd#32834.
@yuwata yuwata added the units label May 17, 2024
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label May 17, 2024
@yuwata yuwata requested review from poettering and bluca May 17, 2024 03:31
@yuwata yuwata mentioned this pull request May 17, 2024
Open
@github-actions GitHub Actions

This comment was marked as off-topic.

Sign in to view
@yuwata yuwata added this to the v256 milestone May 17, 2024
YHNdnzj
YHNdnzj approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@YHNdnzj YHNdnzj left a comment

Choose a reason for hiding this comment

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

This seems like the right thing to do. We also order initrd-switch-root.service after the target, as the operation must be performed after the synchronization point.

@YHNdnzj YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels May 17, 2024
@YHNdnzj YHNdnzj merged commit bf71770 into systemd:main May 17, 2024
31 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 17, 2024
@yuwata yuwata deleted the unit-soft-reboot branch May 17, 2024 06:11
@yuwata
Copy link
Member Author

yuwata commented May 17, 2024 •
edited

Hmm, other similar targets and relevant services e.g. reboot.target and systemd-reboot.service have the same problem.

But they are already documented in bootup(7). Maybe, we should keep the ordering of them and introduce another service after the target?
E.g.
systemd-raboot.service -> reboot.target -> systemd-reboot-really.service ?

@yuwata yuwata mentioned this pull request May 17, 2024
Open
@poettering
Copy link
Member

This seems wrong to me. Now systemd-soft-reboot.service is treated very differenty from the other kinds of reboot. This really should always be handled the same way.

@poettering poettering mentioned this pull request May 21, 2024
Closed
@poettering
Copy link
Member

Also, this drops shutdown.target as a dep? that makes really not much sense to me at all, as it means shutdown.target is now very different for the different types of shutdown.

@yuwata
Copy link
Member Author

yuwata commented May 21, 2024

Also, this drops shutdown.target as a dep? that makes really not much sense to me at all, as it means shutdown.target is now very different for the different types of shutdown.

No.

@yuwata
Copy link
Member Author

yuwata commented May 21, 2024

This seems wrong to me. Now systemd-soft-reboot.service is treated very differenty from the other kinds of reboot. This really should always be handled the same way.

-> #32895

@YHNdnzj
Copy link
Member

YHNdnzj commented May 21, 2024

Also, this drops shutdown.target as a dep? that makes really not much sense to me at all, as it means shutdown.target is now very different for the different types of shutdown.

Hmm? The ordering deps are moved to soft-reboot.target, no?

@yuwata
Copy link
Member Author

yuwata commented May 21, 2024

Also, this drops shutdown.target as a dep? that makes really not much sense to me at all, as it means shutdown.target is now very different for the different types of shutdown.

Hmm? The ordering deps are moved to soft-reboot.target, no?

Right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YHNdnzj YHNdnzj YHNdnzj approved these changes

@poettering poettering Awaiting requested review from poettering

@bluca bluca Awaiting requested review from bluca

Assignees
No one assigned
Labels
units
Milestone
v256
Development

Successfully merging this pull request may close these issues.

TEST-82-SOFTREBOOT is flaky in Github Actions
4 participants
@yuwata @poettering @YHNdnzj @bluca