Skip to content

Commit

Permalink
manager: allow transient units to have drop-ins
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
keszybz committed Oct 16, 2022
1 parent b146a73 commit 1f83244
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 7 deletions.
2 changes: 1 addition & 1 deletion src/core/dbus-manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ static int transient_unit_from_message(

if (!unit_is_pristine(u))
return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS,
"Unit %s already exists.", name);
"Unit %s was already loaded or has a fragment file.", name);

/* OK, the unit failed to load and is unreferenced, now let's
* fill in the transient data instead */
Expand Down
14 changes: 8 additions & 6 deletions src/core/unit.c
Original file line number Diff line number Diff line change
Expand Up @@ -4844,16 +4844,18 @@ int unit_fail_if_noncanonical(Unit *u, const char* where) {
bool unit_is_pristine(Unit *u) {
assert(u);

/* Check if the unit already exists or is already around,
* in a number of different ways. Note that to cater for unit
* types such as slice, we are generally fine with units that
* are marked UNIT_LOADED even though nothing was actually
* loaded, as those unit types don't require a file on disk. */
/* Check if the unit already exists or is already around, in a number of different ways. Note that to
* cater for unit types such as slice, we are generally fine with units that are marked UNIT_LOADED
* even though nothing was actually loaded, as those unit types don't require a file on disk.
*
* Note that we don't check for drop-ins here, because we allow drop-ins for transient units
* identically to non-transient units, both unit-specific and hierarchical. E.g. for a-b-c.service:
* service.d/….conf, a-.service.d/….conf, a-b-.service.d/….conf, a-b-c.service.d/….conf.
*/

return IN_SET(u->load_state, UNIT_NOT_FOUND, UNIT_LOADED) &&
!u->fragment_path &&
!u->source_path &&
strv_isempty(u->dropin_paths) &&
!u->job &&
!u->merged_into;
}
Expand Down

0 comments on commit 1f83244

Please sign in to comment.