From b146a7345b69de16e88347acadb3783ffeeaad9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 14 Oct 2022 14:40:24 +0200 Subject: [PATCH 1/6] manager: reformat boolean expression in unit_is_pristine() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not not IN_SET(…) is just too much for my poor brain. Let's invert the expression to make it easier to undertand. --- src/core/unit.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index d6bea2080f..5016114cb4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4850,12 +4850,12 @@ bool unit_is_pristine(Unit *u) { * are marked UNIT_LOADED even though nothing was actually * loaded, as those unit types don't require a file on disk. */ - 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); + 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; } pid_t unit_control_pid(Unit *u) { From 1f83244641f13a9cb28fdac7e3c17c5446242dfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 14 Oct 2022 15:02:20 +0200 Subject: [PATCH 2/6] manager: allow transient units to have drop-ins In https://github.com/containers/podman/issues/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 https://github.com/containers/podman/issues/16107, https://bugzilla.redhat.com/show_bug.cgi?id=2133792. --- src/core/dbus-manager.c | 2 +- src/core/unit.c | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 73f739b12d..20156c58f5 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -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 */ diff --git a/src/core/unit.c b/src/core/unit.c index 5016114cb4..d0f7188613 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -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; } From 5731e1378ad6256e34f3da33ee993343f025c075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 16 Oct 2022 12:42:35 +0200 Subject: [PATCH 3/6] TEST-15: allow helper functions to accept other unit types 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. --- test/units/testsuite-15.sh | 50 ++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/test/units/testsuite-15.sh b/test/units/testsuite-15.sh index f847adac74..5f7387fb8b 100755 --- a/test/units/testsuite-15.sh +++ b/test/units/testsuite-15.sh @@ -3,30 +3,32 @@ set -eux set -o pipefail -_clear_service () { - local SERVICE_NAME="${1:?}" - systemctl stop "$SERVICE_NAME.service" 2>/dev/null || : - rm -f /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME".service - rm -fr /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME".service.d - rm -fr /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME".service.{wants,requires} - if [[ $SERVICE_NAME == *@ ]]; then - systemctl stop "$SERVICE_NAME"*.service 2>/dev/null || : - rm -f /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME"*.service - rm -fr /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME"*.service.d - rm -fr /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME"*.service.{wants,requires} +clear_unit () { + local UNIT_NAME="${1:?}" + systemctl stop "$UNIT_NAME" 2>/dev/null || : + rm -f /{etc,run,usr/lib}/systemd/system/"$UNIT_NAME" + rm -fr /{etc,run,usr/lib}/systemd/system/"$UNIT_NAME".d + rm -fr /{etc,run,usr/lib}/systemd/system/"$UNIT_NAME".{wants,requires} + if [[ $UNIT_NAME == *@ ]]; then + local base="${UNIT_NAME%@*}" + local suffix="${UNIT_NAME##*.}" + systemctl stop "$base@"*."$suffix" 2>/dev/null || : + rm -f /{etc,run,usr/lib}/systemd/system/"$base@"*."$suffix" + rm -fr /{etc,run,usr/lib}/systemd/system/"$base@"*."$suffix".d + rm -fr /{etc,run,usr/lib}/systemd/system/"$base@"*."$suffix".{wants,requires} fi } -clear_services () { +clear_units () { for u in "$@"; do - _clear_service "$u" + clear_unit "$u" done systemctl daemon-reload } create_service () { local SERVICE_NAME="${1:?}" - clear_services "$SERVICE_NAME" + clear_units "${SERVICE_NAME}".service cat >/etc/systemd/system/"$SERVICE_NAME".service < Date: Sun, 16 Oct 2022 12:54:34 +0200 Subject: [PATCH 4/6] TEST-15: also test hierarchical drop-ins for slices 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. --- test/units/testsuite-15.sh | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/test/units/testsuite-15.sh b/test/units/testsuite-15.sh index 5f7387fb8b..8b44d76982 100755 --- a/test/units/testsuite-15.sh +++ b/test/units/testsuite-15.sh @@ -177,8 +177,8 @@ test_template_alias() { clear_units test15-{a,b}@.service } -test_hierarchical_dropins () { - echo "Testing hierarchical dropins..." +test_hierarchical_service_dropins () { + echo "Testing hierarchical service dropins..." echo "*** test service.d/ top level drop-in" create_services a-b-c check_ko a-b-c ExecCondition "/bin/echo service.d" @@ -202,6 +202,36 @@ ExecCondition=/bin/echo $dropin clear_units a-b-c.service } +test_hierarchical_slice_dropins () { + echo "Testing hierarchical slice dropins..." + echo "*** test slice.d/ top level drop-in" + # Slice units don't even need a fragment, so we test the defaults here + check_ok a-b-c.slice Description "Slice /a/b/c" + check_ok a-b-c.slice MemoryMax "infinity" + + # Test drop-ins + for dropin in slice.d a-.slice.d a-b-.slice.d a-b-c.slice.d; do + mkdir -p /usr/lib/systemd/system/$dropin + echo " +[Slice] +MemoryMax=1000000000 + " >/usr/lib/systemd/system/$dropin/override.conf + systemctl daemon-reload + check_ok a-b-c.slice MemoryMax "1000000000" + rm /usr/lib/systemd/system/$dropin/override.conf + done + + # Test unit with a fragment + echo " +[Slice] +MemoryMax=1000000001 + " >/usr/lib/systemd/system/a-b-c.slice + systemctl daemon-reload + check_ok a-b-c.slice MemoryMax "1000000001" + + clear_units a-b-c.slice +} + test_template_dropins () { echo "Testing template dropins..." @@ -539,7 +569,8 @@ EOF test_basic_dropins test_linked_units test_template_alias -test_hierarchical_dropins +test_hierarchical_service_dropins +test_hierarchical_slice_dropins test_template_dropins test_alias_dropins test_masked_dropins From 6854434cfb5dda10c07d95835c38b75e5e71c2b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 16 Oct 2022 14:02:45 +0200 Subject: [PATCH 5/6] TEST-15: add test for transient units with drop-ins 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. --- test/units/testsuite-15.sh | 62 ++++++++++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/test/units/testsuite-15.sh b/test/units/testsuite-15.sh index 8b44d76982..ed6d5f838d 100755 --- a/test/units/testsuite-15.sh +++ b/test/units/testsuite-15.sh @@ -181,19 +181,40 @@ test_hierarchical_service_dropins () { echo "Testing hierarchical service dropins..." echo "*** test service.d/ top level drop-in" create_services a-b-c - check_ko a-b-c ExecCondition "/bin/echo service.d" - check_ko a-b-c ExecCondition "/bin/echo a-.service.d" - check_ko a-b-c ExecCondition "/bin/echo a-b-.service.d" - check_ko a-b-c ExecCondition "/bin/echo a-b-c.service.d" + check_ko a-b-c ExecCondition "echo service.d" + check_ko a-b-c ExecCondition "echo a-.service.d" + check_ko a-b-c ExecCondition "echo a-b-.service.d" + check_ko a-b-c ExecCondition "echo a-b-c.service.d" for dropin in service.d a-.service.d a-b-.service.d a-b-c.service.d; do mkdir -p /usr/lib/systemd/system/$dropin echo " [Service] -ExecCondition=/bin/echo $dropin +ExecCondition=echo $dropin " >/usr/lib/systemd/system/$dropin/override.conf systemctl daemon-reload - check_ok a-b-c ExecCondition "/bin/echo $dropin" + check_ok a-b-c ExecCondition "echo $dropin" + + # Check that we can start a transient service in presence of the drop-ins + systemd-run -u a-b-c2.service -p Description='sleepy' sleep infinity + + # The transient setting replaces the default + check_ok a-b-c2.service Description "sleepy" + + # The override takes precedence for ExecCondition + # (except the last iteration when it only applies to the other service) + if [ "$dropin" != "a-b-c.service.d" ]; then + check_ok a-b-c2.service ExecCondition "echo $dropin" + fi + + # Check that things are the same after a reload + systemctl daemon-reload + check_ok a-b-c2.service Description "sleepy" + if [ "$dropin" != "a-b-c.service.d" ]; then + check_ok a-b-c2.service ExecCondition "echo $dropin" + fi + + systemctl stop a-b-c2.service done for dropin in service.d a-.service.d a-b-.service.d a-b-c.service.d; do rm -rf /usr/lib/systemd/system/$dropin @@ -218,6 +239,35 @@ MemoryMax=1000000000 " >/usr/lib/systemd/system/$dropin/override.conf systemctl daemon-reload check_ok a-b-c.slice MemoryMax "1000000000" + + busctl call \ + org.freedesktop.systemd1 \ + /org/freedesktop/systemd1 \ + org.freedesktop.systemd1.Manager \ + StartTransientUnit 'ssa(sv)a(sa(sv))' \ + 'a-b-c.slice' 'replace' \ + 2 \ + 'Description' s 'slice too' \ + 'MemoryMax' t 1000000002 \ + 0 + + # The override takes precedence for MemoryMax + check_ok a-b-c.slice MemoryMax "1000000000" + # The transient setting replaces the default + check_ok a-b-c.slice Description "slice too" + + # Check that things are the same after a reload + systemctl daemon-reload + check_ok a-b-c.slice MemoryMax "1000000000" + check_ok a-b-c.slice Description "slice too" + + busctl call \ + org.freedesktop.systemd1 \ + /org/freedesktop/systemd1 \ + org.freedesktop.systemd1.Manager \ + StopUnit 'ss' \ + 'a-b-c.slice' 'replace' + rm /usr/lib/systemd/system/$dropin/override.conf done From c3fa408dcc03bb6dbd11f180540fb9e684893c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 16 Oct 2022 21:52:43 +0200 Subject: [PATCH 6/6] TEST-15: add one more test for drop-in precedence --- test/units/testsuite-15.sh | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/units/testsuite-15.sh b/test/units/testsuite-15.sh index ed6d5f838d..079c8b290e 100755 --- a/test/units/testsuite-15.sh +++ b/test/units/testsuite-15.sh @@ -282,6 +282,41 @@ MemoryMax=1000000001 clear_units a-b-c.slice } +test_transient_service_dropins () { + echo "Testing dropins for a transient service..." + echo "*** test transient service drop-ins" + + mkdir -p /etc/systemd/system/service.d + mkdir -p /etc/systemd/system/a-.service.d + mkdir -p /etc/systemd/system/a-b-.service.d + mkdir -p /etc/systemd/system/a-b-c.service.d + + echo -e '[Service]\nStandardInputText=aaa' >/etc/systemd/system/service.d/drop1.conf + echo -e '[Service]\nStandardInputText=bbb' >/etc/systemd/system/a-.service.d/drop2.conf + echo -e '[Service]\nStandardInputText=ccc' >/etc/systemd/system/a-b-.service.d/drop3.conf + echo -e '[Service]\nStandardInputText=ddd' >/etc/systemd/system/a-b-c.service.d/drop4.conf + + # There's no fragment yet, so this fails + systemctl cat a-b-c.service && exit 1 + + # xxx → eHh4Cg== + systemd-run -u a-b-c.service -p StandardInputData=eHh4Cg== sleep infinity + + data=$(systemctl show -P StandardInputData a-b-c.service) + # xxx\naaa\n\bbb\nccc\nddd\n → eHh4… + test "$data" = "eHh4CmFhYQpiYmIKY2NjCmRkZAo=" + + # Do a reload and check again + systemctl daemon-reload + data=$(systemctl show -P StandardInputData a-b-c.service) + test "$data" = "eHh4CmFhYQpiYmIKY2NjCmRkZAo=" + + clear_units a-b-c.service + rm /etc/systemd/system/service.d/drop1.conf \ + /etc/systemd/system/a-.service.d/drop2.conf \ + /etc/systemd/system/a-b-.service.d/drop3.conf +} + test_template_dropins () { echo "Testing template dropins..." @@ -621,6 +656,7 @@ test_linked_units test_template_alias test_hierarchical_service_dropins test_hierarchical_slice_dropins +test_transient_service_dropins test_template_dropins test_alias_dropins test_masked_dropins