Wait for dbus and FEATURE subscriptions#73
Conversation
Signed-off-by: gpunathilell <gpunathilell@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a startup race in system-health’s Sysmonitor where the initial full service scan could run before the systemd DBus JobRemoved listener and the STATE_DB FEATURE subscription are registered, causing ALL_SERVICE_STATUS to remain stuck at Starting for fast-starting units (e.g., chrony).
Changes:
- Add “subscription ready”
threading.Eventsignaling to the systemd DBus monitor thread and STATE_DB FEATURE monitor thread. - Block the initial
update_system_status()scan until both monitor subscriptions are reported ready (with a timeout). - Extend unit tests to cover subscription readiness signaling and
_wait_for_monitor_subscriptions()behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/system-health/health_checker/sysmonitor.py |
Adds readiness events, a startup wait helper, and wires events into both monitor tasks before the initial scan. |
src/system-health/tests/test_system_health.py |
Adds/adjusts tests to validate readiness event behavior and prevent blocking in test_system_service. |
| sys.exit(1) | ||
| if not statedb_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | ||
| logger.log_error( | ||
| "Timeout: STATE_DB FEATURE monitor did not finish subscribing within {}s".format(SUBSCRIPTION_READY_TIMEOUT_SEC)) | ||
| sys.exit(1) |
There was a problem hiding this comment.
sys.exit(1) here is executed inside the Sysmonitor background thread (started via task_run() in scripts/healthd). In non-main threads, sys.exit() only terminates that thread, leaving the healthd process (and monitor subthreads) running without a queue consumer, which can hide the failure and leak queued events. If the intent is to fail the service so systemd restarts it, propagate the failure to the main thread or explicitly terminate the process (e.g., signal the PID / set a shared fatal flag the main loop checks).
| sys.exit(1) | |
| if not statedb_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | |
| logger.log_error( | |
| "Timeout: STATE_DB FEATURE monitor did not finish subscribing within {}s".format(SUBSCRIPTION_READY_TIMEOUT_SEC)) | |
| sys.exit(1) | |
| os.kill(os.getpid(), signal.SIGTERM) | |
| if not statedb_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | |
| logger.log_error( | |
| "Timeout: STATE_DB FEATURE monitor did not finish subscribing within {}s".format(SUBSCRIPTION_READY_TIMEOUT_SEC)) | |
| os.kill(os.getpid(), signal.SIGTERM) |
| if not dbus_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | ||
| logger.log_error( | ||
| "Timeout: systemd dbus monitor did not finish Subscribe within {}s".format(SUBSCRIPTION_READY_TIMEOUT_SEC)) | ||
| sys.exit(1) | ||
| if not statedb_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): |
There was a problem hiding this comment.
The two .wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC) calls are sequential, so total startup blocking can reach ~2× SUBSCRIPTION_READY_TIMEOUT_SEC (dbus wait + STATE_DB wait). If the intent is a single overall maximum, consider using a deadline and passing the remaining time into the second wait, or waiting on both events with one overall timeout.
| if not dbus_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | |
| logger.log_error( | |
| "Timeout: systemd dbus monitor did not finish Subscribe within {}s".format(SUBSCRIPTION_READY_TIMEOUT_SEC)) | |
| sys.exit(1) | |
| if not statedb_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | |
| # Enforce a single overall maximum wait for both subscriptions combined. | |
| deadline = time.time() + SUBSCRIPTION_READY_TIMEOUT_SEC | |
| if not dbus_ready.wait(timeout=SUBSCRIPTION_READY_TIMEOUT_SEC): | |
| logger.log_error( | |
| "Timeout: systemd dbus monitor did not finish Subscribe within {}s".format(SUBSCRIPTION_READY_TIMEOUT_SEC)) | |
| sys.exit(1) | |
| remaining = deadline - time.time() | |
| if remaining <= 0 or not statedb_ready.wait(timeout=remaining): |
Signed-off-by: gpunathilell <gpunathilell@nvidia.com>
Signed-off-by: gpunathilell <gpunathilell@nvidia.com>
| if not ev.wait(timeout=max(0.0, remaining)): | ||
| logger.log_error( | ||
| "Timeout: {} within {}s (combined budget)".format(detail, SUBSCRIPTION_READY_TIMEOUT_SEC)) | ||
| if monitor_system_bus is not None: | ||
| monitor_system_bus.task_stop() | ||
| if monitor_statedb_table is not None: | ||
| monitor_statedb_table.task_stop() | ||
| sys.exit(1) |
There was a problem hiding this comment.
_wait_for_monitor_subscriptions() calls sys.exit(1) on timeout, but Sysmonitor.system_service() runs inside the Sysmonitor worker thread (started via Sysmonitor.task_run() in scripts/healthd). sys.exit() from a non-main thread only raises SystemExit in that thread and will not terminate the healthd process, leaving the daemon running without sysmonitor (and potentially leaving monitor threads running). Consider propagating the failure to the main thread (e.g., raise an exception that ThreadTaskBase reports, set a shared stop/fatal event that healthd checks and then exits, or use a process-level exit like os._exit(1) if that’s the intended behavior).
| with patch('dbus.mainloop.glib.DBusGMainLoop', MagicMock()), \ | ||
| patch('dbus.SystemBus', MagicMock()), \ | ||
| patch('dbus.Interface') as mock_interface, \ | ||
| patch('gi.repository.GLib.MainLoop') as mock_main_loop: | ||
| manager = MagicMock() | ||
| mock_interface.return_value = manager | ||
| mock_main_loop.return_value.run = MagicMock() | ||
| task.subscribe_sysbus() |
There was a problem hiding this comment.
This test patches dbus.* and gi.repository.* by import path, which requires the dbus and gi modules to be importable in the unit-test environment. setup.py doesn’t declare these as test dependencies, so this test can fail with ModuleNotFoundError in minimal CI environments. Consider stubbing these modules via patch.dict(sys.modules, {...}) (or using patch(..., create=True) with a known-importable target) so the test doesn’t depend on system packages being installed.
…sonic-net#26524) #### Why I did it src/dhcpmon ``` * c0e1938 - (HEAD -> master, origin/master, origin/HEAD) fix: migrate CI from Bookworm to Trixie and fix g++14 build (#73) (13 hours ago) [Xichen96] * be2d5b7 - fix: add -std=c++17 to compile flags for C++17 feature support (#70) (2 days ago) [Xichen96] * 57036c7 - [dhcpmon] Add DHCPv6 monitoring support (#62) (9 days ago) [Xichen96] ``` #### How I did it #### How to verify it #### Description for the changelog
Why I did it
System-health’s sysmonitor can record a service as Starting in
ALL_SERVICE_STATUSduring the initial full scan while systemd still reports activating, then never refresh that row if JobRemoved for that unit’s start job was emitted before the dbus monitor thread finished Subscribe() and connect_to_signal(JobRemoved). Updates are otherwise event-driven only (no periodic rescan), so STATE_DB can stay wrong indefinitely even though the unit is active (running)—commonly observed with chrony andshow system-health sysready-status.Work item tracking
How I did it
Subscribe()andconnect_to_signal('JobRemoved', ...), the dbus monitor thread sets athreading.Event.SubscriberStateTableis added to the select set, the FEATURE monitor thread sets a secondthreading.Event.wait()s on both (withSUBSCRIPTION_READY_TIMEOUT_SEC, currently 60s) before callingupdate_system_status(), so the first service scan runs only after both listeners are registered.sys.exit(1)(same severity as existing monitor startup failures).test_system_health.py: coverage for_wait_for_monitor_subscriptions, ready events insubscribe_statedb/subscribe_sysbus, and_wait_for_monitor_subscriptionsmocked intest_system_serviceso the test does not block.How to verify it
src/system-health/tests, e.g.pytest test_system_health.py -k "wait_for_monitor or subscription_ready or test_system_service" -vsystem-health/healthd; confirmshow system-health sysready-statusreflects OK for units that reach active quickly (e.g. chrony) without requiring a manualsystemctl restart chronyto force a refresh.Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Wait for systemd dbus and STATE_DB FEATURE subscriptions before the first sysready service scan, fixing stale Starting status for fast-starting units (e.g. chrony).
Link to config_db schema for YANG module changes
N/A — no YANG or CONFIG_DB schema changes.
A picture of a cute animal (not mandatory but encouraged)