Skip to content

Modify URM Test Runner - #585

Open
Kartik Nema (kartnema) wants to merge 1 commit into
qualcomm-linux:mainfrom
kartnema:modify-urm-test-runner-with-service-restart
Open

Kartik Nema (kartnema) wants to merge 1 commit into
qualcomm-linux:mainfrom
kartnema:modify-urm-test-runner-with-service-restart

Conversation

@kartnema

@kartnema Kartik Nema (kartnema) commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor
  • 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.

@kartnema
Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-service-restart branch from de6e138 to 213c768 Compare September 24, 2026 08:24
@kartnema
Kartik Nema (kartnema) marked this pull request as ready for review September 24, 2026 08:25
@kartnema

Kartik Nema (kartnema) commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

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.

@kartnema
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
# 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

exit 0
fi

for i in $(seq 1 10); do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kartnema Kartik Nema (kartnema) Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added timeout logging with the configured deadline and full command.

# 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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kartnema Kartik Nema (kartnema) Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- 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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants