Update URM Test Runner by kartnema · Pull Request #568 · qualcomm-linux/qcom-linux-testkit

Kartik Nema (kartnema) · 2026-09-02T14:43:23Z

Update test configs and test nodes selection order to reflect the
movement of these files to /usr/share/urm/tests in latest URM
release
Stage the test nodes in a temporary directory, created using mktemp -d
for the duration of the test execution and clean them up once done.

Srikanth Muppandam (smuppand)

Kartik Nema (@kartnema) Please update the commit message to reflect what changes are being introduced as well.

Srikanth Muppandam (smuppand) · 2026-09-02T15:28:49Z

-COMMON_CONFIGS_DIR="$URM_CONFIG_DIR/common"
-TEST_CONFIGS_DIR="$URM_CONFIG_DIR/tests/configs"
-TEST_NODES_DIR="$URM_CONFIG_DIR/tests/nodes"
+URM_COMMON_CONFIG_DIR="${URM_COMMON_CONFIG_DIR:-/etc/urm}"


URM_CONFIG_DIR is a documented public override, but this replaces it with URM_COMMON_CONFIG_DIR and silently ignores existing callers.

A CI job using URM_CONFIG_DIR=/opt/urm will now inspect /etc/urm and /usr/share/urm instead, producing an incorrect SKIP.

Preserve URM_CONFIG_DIR as the compatibility fallback for all three roots, while allowing the new per-root overrides to take precedence.

I am not sure I understood it totally.
Could you please explain this part:
"Preserve URM_CONFIG_DIR as the compatibility fallback for all three roots, while allowing the new per-root overrides to take precedence."

I am not sure I understood it totally. Could you please explain this part: "Preserve URM_CONFIG_DIR as the compatibility fallback for all three roots, while allowing the new per-root overrides to take precedence."

It means keep old users working while adding finer control. Before this PR, one variable controlled every URM asset path:

URM_CONFIG_DIR=/opt/urm

The runner then looked for:
/opt/urm/common
/opt/urm/tests/configs
/opt/urm/tests/nodes

This PR removes that variable and instead uses three new variables:
URM_COMMON_CONFIG_DIR
URM_TESTS_CONFIG_DIR
URM_TEST_NODES_DIR

So an existing CI job that sets URM_CONFIG_DIR=/opt/urm is silently ignored. That is the compatibility break.
For example:

URM_COMMON_CONFIG_DIR="${URM_COMMON_CONFIG_DIR:-${URM_CONFIG_DIR:-/etc/urm}}"
URM_TESTS_CONFIG_DIR="${URM_TESTS_CONFIG_DIR:-${URM_CONFIG_DIR:-/usr/share/urm}}"
URM_TEST_NODES_DIR="${URM_TEST_NODES_DIR:-${URM_CONFIG_DIR:-/usr/share/urm}}"

Result:

Existing URM_CONFIG_DIR=/opt/urm users continue to work.

A user can override only one location when packages split assets:

URM_TEST_NODES_DIR=/var/lib/urm ./run.sh

When neither is set, the runner uses the package defaults.

Got it, I have made some changes to fix this. please check

Srikanth Muppandam (smuppand) · 2026-09-02T15:29:09Z

+
+# nodes will be available either in /var/lib or /usr/share
+URM_TEST_NODES_DIR="${URM_TEST_NODES_DIR:-/usr/share/urm}"
+if [ -d "/var/lib/urm/tests" ]; then


This unconditionally overrides an explicitly supplied URM_TEST_NODES_DIR whenever /var/lib/urm/tests exists. It also selects /var/lib solely because the directory exists, even if it is empty while the package-provided /usr/share/urm/tests/nodes is valid.

Resolve candidates in priority order: explicit override first, then valid populated runtime location, then valid package location. Do not replace an operator’s explicit selection.

Srikanth Muppandam (smuppand) · 2026-09-02T15:29:31Z

+# Setup: Move all nodes to /var/lib/urm/tests to have a consistent start point.
+TEST_NODES_DEST_DIR="/run/urm/tests/"
+mkdir -p "$TEST_NODES_DEST_DIR"
+cp -r "$TEST_NODES_DIR" "$TEST_NODES_DEST_DIR"


The new copy is not consumed by either run_one or the test binaries: the runner validates TEST_NODES_DIR, copies it to /run/urm/tests/nodes, then invokes the binaries without an argument, environment variable, or working-directory change that points to /run.

The comment says /var/lib/urm/tests, which is a third, different path. This adds state mutation without fixing non-standard discovery. Remove the copy unless the binary has a documented runtime-node override; if it does, pass that exact destination explicitly and verify the copy before starting tests.

Srikanth Muppandam (smuppand) · 2026-09-02T15:29:52Z

 done

+# Cleanup, delete the copied nodes
+rm -rf "$TEST_NODES_DEST_DIR"


rm -rf /run/urm/tests/ deletes a fixed shared directory rather than test-owned state, and it is not protected by the existing exit trap. A failure, interrupt, or pre-existing daemon-owned content can leave stale data or delete unrelated runtime nodes.

Do not recursively remove this shared path. If a temporary copy is genuinely required, create a test-owned directory with mktemp -d, register cleanup immediately with trap, and remove only that owned directory. If the binary requires a fixed path, snapshot and restore only the state created by this test.

Srikanth Muppandam (smuppand) · 2026-09-02T15:30:08Z


 ### 3) Test test nodes
-`/etc/urm/tests/nodes` must exist and be non‑empty for **`/usr/bin/UrmIntegrationTests`** and **`/usr/bin/UrmComponentTests`**. If missing/empty → **SKIP only that suite**.
+`/usr/share/tests/nodes` must exist and be non‑empty for **`/usr/bin/UrmIntegrationTests`** and **`/usr/bin/UrmComponentTests`**. If missing/empty → **SKIP only that suite**.


The documentation says /usr/share/tests/nodes, while the current code checks /usr/share/urm/tests/nodes; it also still documents the removed URM_CONFIG_DIR interface below. This directs users to the wrong path and makes overrides appear supported when they are ignored. Document the actual candidate order and the retained compatibility/new override variables.

Srikanth Muppandam (smuppand) · 2026-09-03T03:55:38Z

Please address the comments where you’ve already provided fixes so they’re easier to review.

Srikanth Muppandam (smuppand) · 2026-09-03T04:28:35Z

+# immediately so it runs on exit, interrupt, or termination.
+RUNTIME_NODES_DIR="/run/urm/tests/nodes"
+if [ "$TEST_NODES_OK" -eq 1 ]; then
+    mkdir -p "$RUNTIME_NODES_DIR"


mkdir -p accepts an existing /run/urm/tests/nodes, but this test does not establish that it created or exclusively owns that directory. A previous interrupted run, another service, or a manual diagnostic can leave nodes there; the subsequent copy merges its files with stale content and the exit trap later deletes the entire directory.
That can produce invalid test inputs or remove state outside this run. Require an absent/test-owned destination before staging, or snapshot and restore pre-existing state. The cleanup must remove only assets this invocation created.

The staging block now checks whether /run/urm/tests/nodes already exists before copying, using [ -e "$RUNTIME_NODES_DIR" ]. If it does, staging is refused and suites are skipped cleanly rather than running against merged or stale content. The copy only proceeds when the destination is confirmed absent, so this invocation exclusively owns everything it creates. If the copy fails partway, the partially-created directory is immediately cleaned up. The exit trap is only registered after a fully successful copy, ensuring it removes exactly what this run created and nothing else.

Should be addressed more clearly now, since we are using mktemp -d

Srikanth Muppandam (smuppand) · 2026-09-03T04:28:58Z

+RUNTIME_NODES_DIR="/run/urm/tests/nodes"
+if [ "$TEST_NODES_OK" -eq 1 ]; then
+    mkdir -p "$RUNTIME_NODES_DIR"
+    if cp -r "$TEST_NODES_DIR/"* "$RUNTIME_NODES_DIR/"; then


The copy occurs before the cleanup trap is installed and its destination is not cleared or made atomic. If cp partially copies and then fails, TEST_NODES_OK=0 skips the suites but leaves a partial /run/urm/tests/nodes; a later run can merge with and execute against that stale partial set. Register a cleanup handler immediately after establishing owned staging state, and stage atomically, for example, copy to a private temporary directory, validate it, then publish it only when the fixed runtime path is safely owned.

The staging block now copies into a sibling temporary directory /run/urm/tests/nodes.staging first, registers the cleanup trap immediately after creating it, then atomically renames it into the final /run/urm/tests/nodes path via mv. This ensures the trap is in place before any content is written, so an interrupt at any point during the copy is handled cleanly. The final path is either fully present or absent — never partial — since mv on the same tmpfs is atomic. The trap covers both the staging and final directories, so cleanup is correct regardless of which stage the script was interrupted at.

Srikanth Muppandam (smuppand) · 2026-09-03T04:29:12Z

+        log_info "[NODES] Staged test nodes from $TEST_NODES_DIR to $RUNTIME_NODES_DIR"
+        # Extend the existing trap to also clean up the staged nodes.
+        if [ "$lock_flock" -eq 1 ]; then
+            trap 'rm -rf "$RUNTIME_NODES_DIR"; exec 9>&-' EXIT INT TERM


The trap uses rm -rf "$RUNTIME_NODES_DIR" for every successful staging run, including when mkdir -p reused a pre-existing directory. This contradicts the comment that cleanup “removes only the directory we created.” Track ownership explicitly and fail/skip rather than deleting a path that was already present; otherwise an interrupted or concurrent system component can lose its runtime nodes.

Kartik Nema (kartnema) · 2026-09-07T08:38:46Z

Got a new suggestion from meta-qcom reviewers to move the nodes to /tmp, let me verify if those changes are needed. Will reopen for reviews once verified.

Kartik Nema (kartnema) · 2026-09-08T11:02:51Z

Instead of pushing the nodes to a static directory like /run/urm, we'll create a temporary directory (using mktemp -d) and keep the nodes there. This should ease up the ownership concerns as well, since any directory created with the system call will be unique. The runner needs to pass the path to the URM test binaries, since it will be variable run to run.

Essentially, we'll create a directory along the lines of /tmp/tmp.SD1gPNdmiV/urm/tests/nodes/, which is guaranteed to be unique and move the nodes there.

Srikanth Muppandam (smuppand)

Kartik Nema (@kartnema) The current version looks good. If you can address the latest comment, we’re all set.

Srikanth Muppandam (smuppand) · 2026-09-09T12:17:39Z

+# on exit, interrupt, or termination.
+RUNTIME_NODES_DIR=""
+if [ "$TEST_NODES_OK" -eq 1 ]; then
+    nodes_tmp_base="$(mktemp -d)"


mktemp -d is unchecked. If it fails, nodes_tmp_base is empty and line 458 turns the destination into /urm/tests/nodes; a root-run test will then create and copy files outside test-owned temporary state. The cleanup trap removes "$nodes_tmp_base"—an empty path—not that accidental directory.

Check mktemp -d before deriving RUNTIME_NODES_DIR; log a clean skip/failure on error, and register cleanup only for a confirmed private directory.

addressed now

Also added checks for verifying if mkdir -p "$RUNTIME_NODES_DIR"; succeeded.

- Update test configs and test nodes selection order to reflect the movement of these files to /usr/share/urm/tests in latest URM release - Stage the test nodes in a temporary directory, created using mktemp -d for the duration of the test execution and clean them up once done. Signed-off-by: Kartik Nema <kartnema@qti.qualcomm.com>

Srikanth Muppandam (smuppand) requested changes Sep 2, 2026

View reviewed changes

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-new-paths branch from 98fe447 to f7c7f93 Compare September 2, 2026 16:39

Kartik Nema (kartnema) requested a review from Srikanth Muppandam (smuppand) September 2, 2026 16:43

Srikanth Muppandam (smuppand) requested changes Sep 3, 2026

View reviewed changes

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-new-paths branch from f7c7f93 to ef80785 Compare September 7, 2026 08:28

Kartik Nema (kartnema) marked this pull request as draft September 7, 2026 09:22

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-new-paths branch from ef80785 to 6fa48ae Compare September 8, 2026 11:00

Kartik Nema (kartnema) marked this pull request as ready for review September 8, 2026 11:00

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-new-paths branch 2 times, most recently from 00a7acf to 7022f52 Compare September 8, 2026 11:46

Kartik Nema (kartnema) requested a review from Srikanth Muppandam (smuppand) September 8, 2026 11:55

Srikanth Muppandam (smuppand) requested changes Sep 9, 2026

View reviewed changes

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-new-paths branch from 7022f52 to e584422 Compare September 9, 2026 12:40

Kartik Nema (kartnema) requested a review from Srikanth Muppandam (smuppand) September 9, 2026 12:41

Kartik Nema (kartnema) force-pushed the modify-urm-test-runner-with-new-paths branch from e584422 to c7964a5 Compare September 10, 2026 09:30

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Srikanth Muppandam (smuppand) left a comment

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!

Srikanth Muppandam (smuppand) commented Sep 3, 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!

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

Uh oh!

Uh oh!

Kartik Nema (kartnema) commented Sep 8, 2026

Uh oh!

Srikanth Muppandam (smuppand) left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kartik Nema (kartnema) commented Sep 2, 2026 •

edited

Loading

Kartik Nema (kartnema) commented Sep 7, 2026 •

edited

Loading