From da81a108653e2ef19102698dbc0184bd18b084d9 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 10 Oct 2024 21:16:05 +0200 Subject: [PATCH 1/4] core/manager: still send out STATUS=Ready for user manager This effectively reverts 37d15cd132f3a8a0bf42fb252c1604e804171ff2. The offending commit wrongly assumed that the second READY=1 notification is for system scope only, but it also serves the purpose of flushing out previous STATUS= containing user unit job status. --- src/core/manager.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 2789f0e3d0c9c..456ad46135b72 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3885,7 +3885,7 @@ static void manager_notify_finished(Manager *m) { log_taint_string(m); } -static void manager_send_ready_user_scope(Manager *m) { +static void manager_send_ready_on_basic_target(Manager *m) { int r; assert(m); @@ -3904,18 +3904,18 @@ static void manager_send_ready_user_scope(Manager *m) { m->status_ready = false; } -static void manager_send_ready_system_scope(Manager *m) { +static void manager_send_ready_on_idle(Manager *m) { int r; assert(m); - if (!MANAGER_IS_SYSTEM(m)) - return; - /* Skip the notification if nothing changed. */ if (m->ready_sent && m->status_ready) return; + /* Note that for user managers, we might have already sent READY=1 in manager_send_ready_user_scope(). + * But we still need to flush STATUS=. The second READY=1 will be treated as a noop so it doesn't + * hurt to send it twice. */ r = sd_notify(/* unset_environment= */ false, "READY=1\n" "STATUS=Ready."); @@ -3940,7 +3940,7 @@ static void manager_check_basic_target(Manager *m) { return; /* For user managers, send out READY=1 as soon as we reach basic.target */ - manager_send_ready_user_scope(m); + manager_send_ready_on_basic_target(m); /* Log the taint string as soon as we reach basic.target */ log_taint_string(m); @@ -3971,7 +3971,7 @@ void manager_check_finished(Manager *m) { if (hashmap_buckets(m->jobs) > hashmap_size(m->units) / 10) m->jobs = hashmap_free(m->jobs); - manager_send_ready_system_scope(m); + manager_send_ready_on_idle(m); /* Notify Type=idle units that we are done now */ manager_close_idle_pipe(m); From 155098a702c4f6de6b1dca534661492625773fed Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 10 Oct 2024 21:06:35 +0200 Subject: [PATCH 2/4] core/manager-serialize: drop serialization for Manager.ready_sent This field indicates whether READY=1 has been sent to the service manager/supervisor. Whenever we reload/reexec/soft-reboot, manager_send_reloading() always resets it to false first, so that READY=1 is sent after reloading finishes. Hence we utterly get "false" at all times. Kill it. --- src/core/manager-serialize.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/core/manager-serialize.c b/src/core/manager-serialize.c index 62dfce93a0a85..3f624619dfd19 100644 --- a/src/core/manager-serialize.c +++ b/src/core/manager-serialize.c @@ -92,7 +92,6 @@ int manager_serialize( (void) serialize_item_format(f, "current-job-id", "%" PRIu32, m->current_job_id); (void) serialize_item_format(f, "n-installed-jobs", "%u", m->n_installed_jobs); (void) serialize_item_format(f, "n-failed-jobs", "%u", m->n_failed_jobs); - (void) serialize_bool(f, "ready-sent", m->ready_sent); (void) serialize_bool(f, "taint-logged", m->taint_logged); (void) serialize_bool(f, "service-watchdogs", m->service_watchdogs); @@ -356,15 +355,6 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { else m->n_failed_jobs += n; - } else if ((val = startswith(l, "ready-sent="))) { - int b; - - b = parse_boolean(val); - if (b < 0) - log_notice("Failed to parse ready-sent flag '%s', ignoring.", val); - else - m->ready_sent = m->ready_sent || b; - } else if ((val = startswith(l, "taint-logged="))) { int b; @@ -558,7 +548,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { if (q < _MANAGER_TIMESTAMP_MAX) /* found it */ (void) deserialize_dual_timestamp(val, m->timestamps + q); - else if (!STARTSWITH_SET(l, "kdbus-fd=", "honor-device-enumeration=")) /* ignore deprecated values */ + else if (!STARTSWITH_SET(l, "kdbus-fd=", "honor-device-enumeration=", "ready-sent=")) /* ignore deprecated values */ log_notice("Unknown serialization item '%s', ignoring.", l); } } From a375e145190482e8a2f0971bffb332e31211622f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 10 Oct 2024 21:32:17 +0200 Subject: [PATCH 3/4] units/{user,capsule}@.service: issue daemon-reexec when notify-reloading Closes #28367 (but not really in the exact form, see below) We have the problem of restarting all user manager instances after upgrade. Current approaches involve systemctl kill with SIGRTMIN+25, which is async and feels rather ugly [1][2]; or systemctl --machine=user@ --user, which requires entering each user session. Neither is particularly elegant. Instead, let's just signal daemon-reexec when user@.service is reloaded from system manager. Our long goal of dropping daemon-reload in favor of reexec (see TODO) is unlikely to happen due to user dbus restrictions, but here the synchronization is done via READY=1. [1] https://gitlab.archlinux.org/archlinux/packaging/packages/systemd/-/blob/main/systemd.install?ref_type=heads#L37 [2] https://salsa.debian.org/systemd-team/systemd/-/blob/debian/master/debian/systemd.postinst#L24 #28367 would not really work for us now I come to think about it, because all processes will be reparented to pid1 as soon as original user manager process exits. This alternative approach seems good enough for our use case. --- units/capsule@.service.in | 4 ++++ units/user@.service.in | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/units/capsule@.service.in b/units/capsule@.service.in index f2bb9e3a45a83..a64298786e490 100644 --- a/units/capsule@.service.in +++ b/units/capsule@.service.in @@ -23,6 +23,10 @@ StateDirectory=capsules/%i RuntimeDirectory=capsules/%i LogExtraFields=CAPSULE=%i Slice=capsule.slice +# Reexecute the manager on service reload, instead of reloading. +# This provides a synchronous method for restarting all user manager +# instances after upgrade. +ReloadSignal=RTMIN+25 KillMode=mixed Delegate=pids memory cpu DelegateSubgroup=init.scope diff --git a/units/user@.service.in b/units/user@.service.in index 5695465747217..381ab2a0db54e 100644 --- a/units/user@.service.in +++ b/units/user@.service.in @@ -20,6 +20,10 @@ PAMName=systemd-user Type=notify-reload ExecStart={{LIBEXECDIR}}/systemd --user Slice=user-%i.slice +# Reexecute the manager on service reload, instead of reloading. +# This provides a synchronous method for restarting all user manager +# instances after upgrade. +ReloadSignal=RTMIN+25 KillMode=mixed Delegate=pids memory cpu DelegateSubgroup=init.scope From 2d0af8bc354f4a1429cebedfb387af72c88720a0 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Thu, 10 Oct 2024 22:37:39 +0200 Subject: [PATCH 4/4] rpm/systemd-update-helper: Use systemctl reload to reexec/reload user managers Let's always use systemctl reload to reexec and reload user managers now that it always implies a reexec. This moves all the job management logic to pid 1 instead of bash and reduces the complexity of the logic as we remove systemd-run, pam and systemd-stdio-bridge from the equation. --- src/rpm/systemd-update-helper.in | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/rpm/systemd-update-helper.in b/src/rpm/systemd-update-helper.in index c81e16c3d3ffb..8af914935261a 100755 --- a/src/rpm/systemd-update-helper.in +++ b/src/rpm/systemd-update-helper.in @@ -107,25 +107,13 @@ case "$command" in [ -d /run/systemd/system ] || exit 0 - users=$(systemctl list-units 'user@*' --legend=no | sed -n -r 's/.*user@([0-9]+).service.*/\1/p') - - if [[ "$command" =~ reexec ]]; then - for user in $users; do - SYSTEMD_BUS_TIMEOUT={{UPDATE_HELPER_USER_TIMEOUT_SEC}}s \ - systemctl --user -M "$user@" daemon-reexec & - done - wait - fi - - if [[ "$command" =~ reload ]]; then - for user in $users; do - SYSTEMD_BUS_TIMEOUT={{UPDATE_HELPER_USER_TIMEOUT_SEC}}s \ - systemctl --user -M "$user@" daemon-reload & - done - wait + if [[ "$command" =~ reexec|reload ]]; then + SYSTEMD_BUS_TIMEOUT={{UPDATE_HELPER_USER_TIMEOUT_SEC}}s systemctl reload "user@*.service" fi if [[ "$command" =~ restart ]]; then + users=$(systemctl list-units 'user@*' --legend=no | sed -n -r 's/.*user@([0-9]+).service.*/\1/p') + for user in $users; do SYSTEMD_BUS_TIMEOUT={{UPDATE_HELPER_USER_TIMEOUT_SEC}}s \ systemctl --user -M "$user@" reload-or-restart --marked &