Modify URM Test Runner by kartnema · Pull Request #585 · qualcomm-linux/qcom-linux-testkit

Kartik Nema (kartnema) · 2026-09-21T03:59:03Z

URM service needs to be restarted before starting the tests,
so that the new test nodes staged in /tmp are processed
by the URM server.
Modify the flock concurrency control mechanism to enforce explicit
cleanup of the lock file
Timeout watcher and sleep process are explicitly terminated/waited
during normal completion and signal cleanup.

Kartik Nema (kartnema) · 2026-09-24T08:26:20Z

Summary of the changes made to address flock locking related issues

Kept flock as the primary concurrency control; genuine concurrent URM runs still result in SKIP.
Added explicit cleanup for the flock: flock -u 9 followed by exec 9>&-.
Consolidated lock cleanup and temporary node directory cleanup into one cleanup() function.
Removed the later trap replacement from the node-staging path so the lock cleanup trap is no longer overwritten.
Local timeout wrapper closes the lock FD before launching the URM binary, timeout watcher, and watcher sleep process.
Timeout watcher and sleep process are explicitly terminated/waited during normal completion and signal cleanup.
Replaced broad pgrep ... run.sh diagnostics with lslocks, fuser, and /proc/*/fd based diagnostics when available.

Srikanth Muppandam (smuppand) · 2026-09-24T17:07:46Z

+# Restart URM service so that the URM server is aware of the
+# test nodes staged in the temporary directory.
+log_info "Restarting URM service"
+if ! systemctl restart urm; then


Restart the selected service instead of the hard-coded urm unit. The runner documents and checks SERVICE_NAME, but this block restarts and verifies urm. With SERVICE_NAME=custom-urm.service, the configured daemon is never restarted while the logs claim that it is active, so tests can run against stale state or incorrectly SKIP. Use "$SERVICE_NAME" consistently for restart and readiness checks.

Replaced hard-coded urm operations with "$SERVICE_NAME" consistently.

Srikanth Muppandam (smuppand) · 2026-09-24T17:08:24Z

+# test nodes staged in the temporary directory.
+log_info "Restarting URM service"
+if ! systemctl restart urm; then
+    log_skip "[SERVICE] $SERVICE_NAME could not be restarted — overall SKIP"


A failed required restart must not produce a neutral SKIP. The service was already identified as applicable, and this change states that restarting it is required before the staged nodes can be tested.

Returning SKIP here, or when the service never becomes active, can leave CI green without executing the validation.

Record FAIL and retain systemctl status or journal evidence, reserve SKIP for a genuinely absent or non-applicable service.

Changed required restart failure and post-restart inactive state from neutral SKIP to FAIL.

Srikanth Muppandam (smuppand) · 2026-09-24T17:08:42Z

+    exit 0
+fi
+
+for i in $(seq 1 10); do


Do not depend on an unchecked seq command for service verification. seq is not included in the dependency check, and when it is unavailable the loop executes zero times and the script proceeds without confirming that URM became active. Use a POSIX arithmetic while loop or a shared bounded-wait helper.

Replaced unchecked seq usage with a usual while loop for service readiness polling.

Srikanth Muppandam (smuppand) · 2026-09-24T17:09:03Z


+# Restart URM service so that the URM server is aware of the
+# test nodes staged in the temporary directory.
+log_info "Restarting URM service"


Restart URM only when at least one suite is runnable and node staging succeeded. The current flow restarts the system service even when all binaries are missing or the configuration and nodes will cause every suite to SKIP.
Determine the runnable set first, or perform one guarded restart immediately before the first runnable test.

Deferred required service restart until immediately before the first runnable suite.

Avoided restarting the service when all suites will SKIP due to missing binary, missing config, or failed node staging.

Srikanth Muppandam (smuppand) · 2026-09-24T17:09:23Z

+        sleep_rc=$?
+        trap - INT TERM
+        rm -f "$sleep_pid_file" 2>/dev/null || true
+        if [ "$sleep_rc" -eq 0 ]; then


Record when the timeout watcher actually expires. Currently a timed-out binary returns a signal-derived status such as 143 or 137, which run_one reports only as “UNKNOWN RC” that is indistinguishable from an external signal or crash. Write a timeout marker or return a stable timeout status such as 124, then log the command and configured deadline.

Updated run_one() to classify 124 as TIMEOUT / FAIL.

Added timeout marker handling so helper-expired commands return stable 124 instead of ambiguous signal-derived statuses.

Added timeout logging with the configured deadline and full command.

Srikanth Muppandam (smuppand) · 2026-09-24T17:09:57Z

+# run_with_timeout() path here so the command, watcher and watcher sleep all
+# drop the lock FD before running, and so the watcher/sleep PIDs can be killed
+# and waited during normal completion or cleanup.
+run_cmd_with_timeout_no_lock_fd() {


Update the suite README for the changed runtime contract. It currently says timeout handling is conditional on the shared run_with_timeout helper and does not document the mandatory service restart, its result classification, or the new lock-cleanup diagnostics.

Also add the required function contract describing arguments, return statuses, spawned processes, retained files, and cleanup ownership for this timeout helper.

Updated the suite README to describe the current runtime contract, including selected SERVICE_NAME, required service restart, timeout behavior, lock cleanup, and diagnostics.

Kartik Nema (kartnema) · 2026-09-28T11:59:47Z

Hi Srikanth Muppandam (@smuppand) please let me know if additional changes are needed

Srikanth Muppandam (smuppand) · 2026-09-29T16:58:39Z

+}
+
+# shellcheck disable=SC2317  # Invoked indirectly by INT/TERM traps.
+cleanup_signal() {


Cleanup_signal() exits with 130/143 without writing or clearing the result file. The YAML ignores the runner exit code and always passes userspace-resource-manager.res to send-to-lava.sh. If an earlier run left PASS, an interrupted run can publish that stale PASS.

Initialize/remove the result at startup and write FAIL from the signal path, preferably through the shared result lifecycle.

Addressed stale result handling by initializing/removing userspace-resource-manager.res at startup via test_result_init when available, with a fallback rm -f. Added a shared write_result() helper and changed cleanup_signal() to write FAIL before cleanup and before exiting with 130/143. This prevents LAVA from publishing a stale PASS after an interrupted run.

Srikanth Muppandam (smuppand) · 2026-09-29T16:59:32Z

check_systemd_services() returns success when systemctl is absent or the unit is missing/disabled. The runner consequently logs the service as active, bypasses the absent-service branch, and later fails during mandatory restart.

Use systemd_service_exists() and systemd_service_is_active() explicitly before start/restart.

Replaced the initial service gate’s use of check_systemd_services() with explicit systemd_service_exists() and systemd_service_is_active() predicates. The runner now correctly distinguishes absent/non-applicable service as SKIP from applicable-but-inactive/unstartable service as FAIL. The mandatory restart path also validates service existence before restart and uses systemd_service_is_active() for readiness polling.

Srikanth Muppandam (smuppand) · 2026-09-29T16:59:58Z

+#   Retained files: temporary watcher sleep/timeout marker files under LOGDIR
+#                   while running only; this helper removes them before return,
+#                   and cleanup() owns removal on interruption.
+run_cmd_with_timeout_no_lock_fd() {


The timeout implementation is generic lifecycle behavior and duplicates shared utility responsibilities. It should be moved into or used to improve functestlib.sh.

My only concern being that I don't want to touch any core infra which impacts users and teams beyond URM, please advice.

My only concern being that I don't want to touch any core infra which impacts users and teams beyond URM, please advice.

That concern is valid. Please do not change the existing run_with_timeout() behavior in this PR, since it has other callers. Instead, add a new opt-in managed-timeout helper in functestlib.sh and use it only from URM. Keep the FD 9 closure and URM-specific cleanup wrapper local. This avoids regressions for other suites while keeping reusable timeout lifecycle code out of run.sh.

The other two functional findings remain unresolved at the current head:

Signal cleanup can publish a stale .res result because the result is neither initialized nor finalized as FAIL.

check_systemd_services() treats missing systemd or missing/disabled units as success, so it cannot be used for the initial applicability/active-state gate. Please use systemd_service_exists() and systemd_service_is_active() explicitly.

Addressed the timeout lifecycle review by moving the generic timeout lifecycle behavior out of the URM runner and into a new opt-in helper in functestlib.sh: run_with_managed_timeout(). The existing run_with_timeout() implementation was intentionally left unchanged to avoid impacting existing suites and callers.

URM now uses the new helper only through a thin local wrapper, run_cmd_with_timeout_no_lock_fd(). The wrapper keeps URM-specific behavior local by setting MANAGED_TIMEOUT_PRE_EXEC_HOOK=close_lock_fd_in_child, so the test command, timeout watcher, and watcher sleep close inherited FD 9 before running. This preserves the URM-specific lock handling without pushing FD 9 behavior into shared infrastructure.

URM cleanup also remains local. The shared managed-timeout helper exposes runtime state through MANAGED_TIMEOUT_* variables, and URM’s existing cleanup() trap uses those variables to kill/wait the command, watcher, watcher sleep, and remove timeout marker files on normal cleanup or interruption.

This keeps the reusable timeout process lifecycle in functestlib.sh, avoids changing legacy run_with_timeout() semantics, and limits behavioral change to URM as an explicit opt-in user.

- URM service needs to be restarted before starting the tests, so that the new test nodes staged in /tmp are processed by the URM server. - Modify the flock concurrency control mechanism to enforce explicit cleanup of the lock file - Timeout watcher and sleep process are explicitly terminated/waited during normal completion and signal cleanup. Signed-off-by: Kartik Nema <kartnema@qti.qualcomm.com>

Srikanth Muppandam (smuppand)

LGTM

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-service-restart branch from de6e138 to 213c768 Compare September 24, 2026 08:24

Kartik Nema (kartnema) marked this pull request as ready for review September 24, 2026 08:25

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-service-restart branch 3 times, most recently from 36bfbe3 to 5e64d8e Compare September 24, 2026 09:51

Srikanth Muppandam (smuppand) requested changes Sep 24, 2026

View reviewed changes

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-service-restart branch from 5e64d8e to 1e4e892 Compare September 25, 2026 06:06

Kartik Nema (kartnema) requested a review from Srikanth Muppandam (smuppand) September 25, 2026 06:24

Srikanth Muppandam (smuppand) requested changes Sep 29, 2026

View reviewed changes

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-service-restart branch 2 times, most recently from 13da9a5 to 84adb68 Compare September 30, 2026 05:49

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-service-restart branch from 84adb68 to 05f3bc4 Compare September 30, 2026 05:53

Kartik Nema (kartnema) requested a review from Srikanth Muppandam (smuppand) September 30, 2026 06:18

Srikanth Muppandam (smuppand) approved these changes Sep 30, 2026

View reviewed changes

Srikanth Muppandam (smuppand) merged commit bae1de9 into qualcomm-linux:main Sep 30, 2026
14 checks passed

Conversation

Kartik Nema (kartnema) commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kartik Nema (kartnema) commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kartik Nema (kartnema) Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kartik Nema (kartnema) Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kartik Nema (kartnema) commented Sep 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Srikanth Muppandam (smuppand) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kartik Nema (kartnema) commented Sep 21, 2026 •

edited

Loading

Kartik Nema (kartnema) commented Sep 24, 2026 •

edited

Loading

Kartik Nema (kartnema) Sep 25, 2026 •

edited

Loading

Kartik Nema (kartnema) Sep 25, 2026 •

edited

Loading