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

pid1: add SurviveFinalKillSignal= to skip units on final sigterm/sigkill spree #28545

Merged
merged 3 commits into from Sep 28, 2023

Conversation

bluca
Copy link
Member

@bluca bluca commented Jul 28, 2023

Add a new boolean for units, SurviveFinalKillSignal=yes/no. Units that
set it will not have their process receive the final sigterm/sigkill in
the shutdown phase.

@bluca bluca added this to the v255 milestone Jul 28, 2023
@bluca bluca requested review from poettering and keszybz July 28, 2023 01:10
@github-actions github-actions bot added documentation util-lib tests units meson please-review PR is ready for (re-)review by a maintainer labels Jul 28, 2023
src/core/unit.c Outdated Show resolved Hide resolved
src/core/unit.c Outdated Show resolved Hide resolved
src/core/unit.c Outdated Show resolved Hide resolved
src/core/transaction.c Outdated Show resolved Hide resolved
src/core/transaction.c Outdated Show resolved Hide resolved
src/core/path.c Outdated Show resolved Hide resolved
@systemd systemd deleted a comment from github-actions bot Jul 28, 2023
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

superficial comments only.

src/basic/cgroup-util.c Outdated Show resolved Hide resolved
src/core/path.c Outdated Show resolved Hide resolved
src/core/path.c Outdated Show resolved Hide resolved
src/core/path.c Outdated Show resolved Hide resolved
src/core/scope.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
src/core/transaction.c Outdated Show resolved Hide resolved
src/core/path.c Outdated Show resolved Hide resolved
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jul 28, 2023
@github-actions github-actions bot added util-lib meson please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jul 28, 2023
@bluca bluca requested review from YHNdnzj and yuwata July 28, 2023 11:24
@keszybz
Copy link
Member

keszybz commented Jul 28, 2023

Hmm, so we'd be supposed to put services in a special slice. But slices are a mechanism for resource management, generally unrelated to how services behave during shutdown or other state transitions. I liked the previous approach with a setting that can be attached to any unit much more.

@bluca
Copy link
Member Author

bluca commented Jul 28, 2023

Hmm, so we'd be supposed to put services in a special slice. But slices are a mechanism for resource management, generally unrelated to how services behave during shutdown or other state transitions. I liked the previous approach with a setting that can be attached to any unit much more.

Well, ish - there's init.scope, and the forthcoming init-workers.scope, that also have semantic meaning outside of resource management. Also, there's the whole delegation mechanism, that is also not about resource management, but process management. The caveat for this feature is that I think managing processes by cgroup is the way to go, and the key thing we need to do for this is killing a subset of live processes on soft-reboot. I think it fits very well with the hierarchical cgroup model, and having to reimplement that manually (going unit by unit to check a boolean) would be much worse.

Then there's the issue that @poettering did not like the boolean, so I'm trying to get two birds with one stone here.

@keszybz
Copy link
Member

keszybz commented Jul 28, 2023

IMO that's a false similarity. Yes, we have a init.scope, but that's because the init process and its helpers form a unit which shares resources, so it makes sense to create a slice/scope for them. The fact that units should survive a reboot does not mean that they should share resources in any way.

@bluca
Copy link
Member Author

bluca commented Aug 30, 2023

Hence sorry, but these implicit deps might make sense in isolated usecases, but I doubt they are good defaults, nor are they desirable at all.

Yeah. Stuff that survives a reboot would generally be special, and would need to coordinate closely with other early-boot and late-shutdown services, so some careful crafting of ordering dependencies will be necessary anyway.

Do you have any such real-world examples? I have a dozen of them or so, and only one fits that description, but it's a one-shot remain-after-exit that configures hardware on early boot, so that also doesn't fit quite well. Everything else is a normal service from any other point of view.

After re-reading the discussion, I'm swayed by Lennart's arguments.

Please rework this, so that the knob just controls the xattr.

+1 to this. This would be a generally useful thing, and I think we all agree on having this.

We do not. I am very strongly against half-arsing this. It is very important to me and my use cases to have a first-class, well-defined, intelligible, supported and consistent interface for this.

This is not any different from many of the other well-defined, high level interfaces we have. For example, pretty much everything that deals with images, directories or mounts could be removed, and is doable with the single MountImage and BindPath settings. Everything else is redundant - RootImage, RootEphemeral, ExtensionImages, PrivateTmp, TemporaryFileSystem etc.
Same for the exec directories, everything is redundant - StateDirectory, RuntimeDirectory, etc.
Same for most of the input/output redirection, both old and new - OpenFiles, etc.
Heck, if you take this all the way through, I'm pretty sure 2/3 of all unit settings are redundant, as an inline script in ExecStartPre could very well replace most things.

Why do we have all of these redundant then? Because often it's not enough to enable some very low level functionality, but we want to offer a first class interface, that comes with a promise that whatever else we change, it will keep working as advertised, and models some common and specific behaviour that we want to encode and expose. For me, this is one of those cases.

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Sep 25, 2023
@bluca bluca changed the title pid1: add SurviveSystemTransition= for surviving services pid1: add SurviveFinalKillSignal= to skip units on final sigterm/sigkill spree Sep 25, 2023
man/systemd.unit.xml Outdated Show resolved Hide resolved
man/systemd.unit.xml Outdated Show resolved Hide resolved
src/core/cgroup.c Outdated Show resolved Hide resolved
src/shared/killall.c Outdated Show resolved Hide resolved
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Sep 28, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Sep 28, 2023
src/shared/killall.c Outdated Show resolved Hide resolved
@poettering poettering added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Sep 28, 2023
bluca and others added 3 commits September 28, 2023 13:48
…ill spree

Add a new boolean for units, SurviveFinalKillSignal=yes/no. Units that
set it will not have their process receive the final sigterm/sigkill in
the shutdown phase.

This is implemented by checking if a process is part of a cgroup marked
with a user.survive_final_kill_signal xattr (or a trusted xattr if we
can't set a user one, which were added only in kernel v5.7 and are not
supported in CentOS 8).
@bluca bluca 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 ci-failure-appears-unrelated and removed good-to-merge/with-minor-suggestions labels Sep 28, 2023
@bluca bluca merged commit 1e49f4e into systemd:main Sep 28, 2023
45 of 48 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 Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants