Cannot get changes of FREEZERSTATE for unit via signal

[dofmind]

I tried to get changes of FREEZERSTATE for unit via signal - UnitPropertiesChanged or UnitStateChanged but they don't emit any signal when FREEZERSTATE for unit is changed.

The systemd seems not support to emit signal for changes of FREEZERSTATE for unit. Is there any way to detect changes of FREEZERSTATE for unit instead of polling the status of unit?

[engelmi]

Hi @dofmind,

I implemented the following python snippet to (kind of) verify that this is not due to BlueChi missing some signals: https://gist.github.com/engelmi/a1ed5af612dbecb51a27b28b83d1a33e

When running locally and freezing/thawing units (the cow.service in the example), it also doesn't trigger any property changed signals. Therefore, I think you are right - systemd doesn't emit signals for freezing/thawing even though it should accroding to its property definition: https://github.com/systemd/systemd/blob/main/src/core/dbus-unit.c#L873C3-L873C28

Maybe it emits the change on another service/object/interface since its actually not the unit itself, but the unit's cgroup that gets frozen - it would still be intuitive, I think, to receive property changed signals on the units interface.

So this seems to be a systemd related issue rather than a BlueChi one.

[mwperina]

Wouldn't it be to good to file issue about it to systemd?

[engelmi]

Wouldn't it be to good to file issue about it to systemd?

Definitely. I'll try to collect more information and compile the info to file a detailed issue at systemd. But it'll take a bit of time. If @dofmind or someone else wants to file the issue before, please add a link here :)

Update: Filed the issue https://github.com/systemd/systemd/issues/31115

[dofmind]

@engelmi After systemd fixes this issue, should changes of FREEZERSTATE be handled in UnitStateChanged? Currently UnitStateChanged does not consider FREEZERSTATE.

[engelmi]

Hi @dofmind, since FREEZERSTATE is a property on org.freedesktop.systemd1 /org/freedesktop/systemd1/unit/sleepy_2eservice org.freedesktop.systemd1.Unit, I'd expect a PropertiesChanged signal to be emitted. It would make sense to also emit the UnitStateChanged in the same go, but I don't have that much knowledge here - the systemd developers have more insight.

[dofmind]

Hi @dofmind, since FREEZERSTATE is a property on org.freedesktop.systemd1 /org/freedesktop/systemd1/unit/sleepy_2eservice org.freedesktop.systemd1.Unit, I'd expect a PropertiesChanged signal to be emitted. It would make sense to also emit the UnitStateChanged in the same go, but I don't have that much knowledge here - the systemd developers have more insight.

There is a patch on systemd to emit PropertiesChanged signal for FREEZERSTATE - https://github.com/YHNdnzj/systemd/commit/230534ec0aad7f0f4d9206049fb08bea8c543f18 I have backported the patch to apply on my systemd v250 and i will try to test it.

[dofmind]

A PR to resolve this issue was recently merged into systemd (https://github.com/systemd/systemd/pull/33092). Using the backport patch for systemd v250, I verified that the UnitPropertiesChanged signal emits when the FreezerState property of the unit changes.

From f484fb3fbdbba7f22a300a42b955e36ff272b9a2 Mon Sep 17 00:00:00 2001
From: Joonyoung Shim <joonyoung.shim@42dot.ai>
Date: Fri, 5 Jul 2024 11:39:35 +0900
Subject: [PATCH] core/unit: introduce unit_set_freezer_state, make logging
 consistent

Also, emit PropertiesChanged signal for FreezerState too.

Fixes systemd#31115

This backports the same fix from
https://github.com/YHNdnzj/systemd/commit/230534ec0aad7f0f4d9206049fb08bea8c543f18
for systemd upstream that we can't backport directly.

Signed-off-by: Joonyoung Shim <joonyoung.shim@42dot.ai>
---
 src/core/cgroup.c |  8 +++-----
 src/core/unit.c   | 18 ++++++++++++++++--
 src/core/unit.h   |  1 +
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 79681c65be..1f56047bce 100644
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -4125,7 +4125,7 @@ int unit_cgroup_freezer_action(Unit *u, FreezerAction action) {
                 log_unit_debug_errno(u, r, "Failed to obtain cgroup freezer state: %m");

         if (target == kernel) {
-                u->freezer_state = target;
+                unit_set_freezer_state(u, target);
                 return 0;
         }

@@ -4133,12 +4133,10 @@ int unit_cgroup_freezer_action(Unit *u, FreezerAction action) {
         if (r < 0)
                 return r;

-        log_unit_debug(u, "%s unit.", action == FREEZER_FREEZE ? "Freezing" : "Thawing");
-
         if (action == FREEZER_FREEZE)
-                u->freezer_state = FREEZER_FREEZING;
+                unit_set_freezer_state(u, FREEZER_FREEZING);
         else
-                u->freezer_state = FREEZER_THAWING;
+                unit_set_freezer_state(u, FREEZER_THAWING);

         r = write_string_file(path, one_zero(action == FREEZER_FREEZE), WRITE_STRING_FILE_DISABLE_BUFFER);
         if (r < 0)
diff --git a/src/core/unit.c b/src/core/unit.c
index 162ac1d5e2..1fe7f948f2 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -5804,10 +5804,24 @@ bool unit_can_freeze(Unit *u) {
         return UNIT_VTABLE(u)->freeze;
 }

+void unit_set_freezer_state(Unit *u, FreezerState state) {
+        assert(u);
+
+        if (u->freezer_state == state)
+                return;
+
+        log_unit_debug(u, "Freezer state changed %s -> %s",
+                       freezer_state_to_string(u->freezer_state), freezer_state_to_string(state));
+
+        u->freezer_state = state;
+
+        unit_add_to_dbus_queue(u);
+}
+
 void unit_frozen(Unit *u) {
         assert(u);

-        u->freezer_state = FREEZER_FROZEN;
+        unit_set_freezer_state(u, FREEZER_FROZEN);

         bus_unit_send_pending_freezer_message(u);
 }
@@ -5815,7 +5829,7 @@ void unit_frozen(Unit *u) {
 void unit_thawed(Unit *u) {
         assert(u);

-        u->freezer_state = FREEZER_RUNNING;
+        unit_set_freezer_state(u, FREEZER_RUNNING);

         bus_unit_send_pending_freezer_message(u);
 }
diff --git a/src/core/unit.h b/src/core/unit.h
index 94f2180951..ef8d89c940 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -980,6 +980,7 @@ int unit_clean(Unit *u, ExecCleanMask mask);
 int unit_can_clean(Unit *u, ExecCleanMask *ret_mask);

 bool unit_can_freeze(Unit *u);
+void unit_set_freezer_state(Unit *u, FreezerState state);
 int unit_freeze(Unit *u);
 void unit_frozen(Unit *u);

-- 
2.34.1

[engelmi]

Thats great news! Thanks for the update and verifying the fix works also with a backport patch! @dofmind I see that the related issue https://github.com/systemd/systemd/issues/31115 has also been closed. In that case, I think we can also close this issue here.

[dofmind]

Okey. Let's close this issue.