(#2156620) Allow drop-ins for transient units #381
Merged
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
changed the title
dtardon
force-pushed
the
bz2156620-drop-ins-transient
branch
4 times, most recently
from
d2e4327
to
5a2cf2a
Compare
systemd-rhel-bot
added
tracker/unapproved
systemd-rhel-bot
added
tracker/unapproved
systemd-rhel-bot
added
tracker/unapproved
|
@dtardon CI failures seem unrelated but please rerun, I'd feel better to give LGTM on this one once everything is green. |
dtardon
force-pushed
the
bz2156620-drop-ins-transient
branch
from
5a2cf2a
to
57ea65a
Compare
|
Tracker - 2156620 The following commits meet all requirements
|
|
@dtardon can you please fix the issue reported by the GH action, i.e. missing upstream reference. |
This is what upstream commit cc5549c has done, but just for a single shell file. RHEL-only Related: #2156620
We would miss anything created under a template instance. (cherry picked from commit 4e2ac45) Related: #2156620
to avoid possible word splitting. [dtardon: Dropped changes to other files.] (cherry picked from commit 3882526) Related: #2156620
Bash will generate a very nice message for us: /tmp/ff.sh: line 1: SOMEVAR: parameter null or not set Let's save some keystrokes by not replacing this with our own inferior messages. [dtardon: Dropped changes to other files.] (cherry picked from commit d7ff524) Related: #2156620
Not not IN_SET(…) is just too much for my poor brain. Let's invert the expression to make it easier to undertand. (cherry picked from commit b146a7345b69de16e88347acadb3783ffeeaad9d) Related: #2156620
In containers/podman#16107, starting of a transient slice unit fails because there's a "global" drop-in /usr/lib/systemd/user/slice.d/10-oomd-per-slice-defaults.conf (provided by systemd-oomd-defaults package to install some default oomd policy). This means that the unit_is_pristine() check fails and starting of the unit is forbidden. It seems pretty clear to me that dropins at any other level then the unit should be ignored in this check: we now have multiple layers of drop-ins (for each level of the cgroup path, and also "global" ones for a specific unit type). If we install a "global" drop-in, we wouldn't be able to start any transient units of that type, which seems undesired. In principle we could reject dropins at the unit level, but I don't think that is useful. The whole reason for drop-ins is that they are "add ons", and there isn't any particular reason to disallow them for transient units. It would also make things harder to implement and describe: one place for drop-ins is good, but another is bad. (And as a corner case: for instanciated units, a drop-in in the template would be acceptable, but a instance-specific drop-in bad?) Thus, $subject. While at it, adjust the message. All the conditions in unit_is_pristine() essentially mean that it wasn't loaded (e.g. it might be in an error state), and that it doesn't have a fragment path (now that drop-ins are acceptable). If there's a job for it, it necessarilly must have been loaded. If it is merged into another unit, it also was loaded and found to be an alias. Based on the discussion in the bugs, it seems that the current message is far from obvious ;) Fixes containers/podman#16107, https://bugzilla.redhat.com/show_bug.cgi?id=2133792. (cherry picked from commit 1f83244641f13a9cb28fdac7e3c17c5446242dfb) Resolves: #2156620
clear_services() is renamed to clear_units() and now takes a full unit name including the suffix as an argument. _clear_service() is renamed to clear_unit() and changed likewise. create_service() didn't have the same underscore prefix, and I don't think it's useful or needed for a local function, so it is removed. No functional change. (cherry picked from commit 5731e1378ad6256e34f3da33ee993343f025c075) Related: #2156620
Slices are worth testing too, because they don't need a fragment path so they behave slightly differently than service units. I'm making this a separate patch from the actual tests that I wanted to add later because it's complex enough on its own. (cherry picked from commit f80c874af376052b6b81f47cbbc43d7fecd98cd6) Related: #2156620
We want to test four things: - that the transient units are successfully started when drop-ins exist - that the transient setings override the defaults - the drop-ins override the transient settings (the same as for a normal unit) - that things are the same before and after a reload To make things more fun, we start and stop units in two different ways: via systemctl and via a direct busctl invocation. This gives us a bit more coverage of different code paths. (cherry picked from commit 6854434cfb5dda10c07d95835c38b75e5e71c2b5) Related: #2156620
(cherry picked from commit c3fa408dcc03bb6dbd11f180540fb9e684893c39) Related: #2156620
dtardon
force-pushed
the
bz2156620-drop-ins-transient
branch
from
57ea65a
to
b565b04
Compare
mergify
bot
added
pr/needs-ci
systemd-rhel-bot
changed the title
dtardon
changed the title
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.
Resolves: #2156620