Skip to content

test(rocprofv3): add interactive attach detach tests - #12055

Merged
minseobshin11 merged 6 commits into
developfrom
users/mishin/attach-interactive-detach
Nov 3, 2026
Merged

minseobshin11 merged 6 commits into
developfrom
users/mishin/attach-interactive-detach

Conversation

@minseobshin11

@minseobshin11 minseobshin11 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

rocprofv3 supports two interactive ways to end an attachment: pressing Enter in the terminal where it runs, or sending it SIGINT. This is the documented default, with --attach-duration-msec as the timed alternative.

Every existing attachment suite passes --attach-duration-msec, so only the timed path is exercised. A regression in the interactive path would break the documented default workflow while the suites stayed green.

Technical Details

Adds tests/rocprofv3/attachment/attach-detach/, registering two scenarios from one driver. Both omit --attach-duration-msec, which is what selects the interactive path, and hold the profiler's stdin open through a FIFO so the prompt blocks rather than reaching EOF:

  • enter writes a newline to the FIFO to answer the Press Enter to detach... prompt
  • sigint sends SIGINT to rocprofv3 itself

Each scenario waits for the Press Enter to detach... prompt before starting its settle interval. The prompt is printed only once attachment has completed and the SIGINT handler is installed, so a slow attach cannot consume the interval or receive the trigger early.

The distinction from the existing suites is the signal target. They send SIGINT to the profiled application after the profiler has already detached on its timer; this sends it to the profiler, which is the path rocprof-attach installs a handler for.

Each scenario asserts the profiler exits cleanly, the target process is still running afterwards, and the flushed output contains agent, kernel dispatch, and HSA API records. --attach-sync-output is retained so output timing stays deterministic.

The driver carries its own readiness wait and ptrace permission guard, matching the sibling attachment drivers, and skips when ptrace is unavailable.

Issue Tracking

JIRA ID: AIROCVAL-70

Test Plan

ctest -R "attach-detach"

Requires a GPU and the ability to ptrace a non-descendant process; in a container the SYS_PTRACE capability is needed, as described in the process attachment guide. The suite skips when that is unavailable.

Test Result

Run on an 8x MI300X (gfx942) node. Both scenarios attach, detach on their interactive trigger, and leave the target process running; all six validators pass against the captured output. With attachment artificially delayed beyond the settle interval, both scenarios still wait for the prompt and pass.

Submission Checklist

@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@minseobshin11
minseobshin11 marked this pull request as ready for review September 24, 2026 14:56
@minseobshin11
minseobshin11 requested review from a team as code owners September 24, 2026 14:56
@venkat1361
venkat1361 requested a balanced review from Copilot September 28, 2026 15:10

Copilot AI left a comment

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.

Copilot review overview

🟡 Changes recommended

Readiness races and an ineffective zombie-process check can make the tests flaky or allow false positives.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds coverage for rocprofv3’s interactive Enter- and SIGINT-based detach workflows.

Changes:

  • Registers Enter and SIGINT attachment scenarios.
  • Adds execution, permission-skip, cleanup, and validation logic.
  • Validates agent, kernel-dispatch, and HSA API output.
File Description
attachment/​CMakeLists.txt Registers the new suite.
attach-detach/​CMakeLists.txt Defines execution and validation tests.
run_attachment_detach_test.sh Drives both interactive detach modes.
validate.py Validates captured profiling data.
conftest.py Loads JSON and handles skips.
pytest.ini Configures pytest discovery.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

  • Add new commits
  • Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

@github-actions github-actions Bot added the Stale PR has no activity for 25+ days label Nov 2, 2026
@minseobshin11
minseobshin11 force-pushed the users/mishin/attach-interactive-detach branch from 7d63acb to 9f4d504 Compare November 2, 2026 06:07
@peymanr

peymanr commented Nov 2, 2026

Copy link
Copy Markdown

PR Pundit review

(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)

# Finding Where
1 🟠 Detach tests pass even if detach never stops tracing: validate.py checks only record presence projects/rocprofiler-sdk/tests/rocprofv3/attachment/attach-detach/validate.py:41
2 🟠 Worst-case detach reports as ctest timeout and loses rocprofv3 log: unbounded waits under TIMEOUT 90 projects/rocprofiler-sdk/tests/rocprofv3/attachment/attach-detach/run_attachment_detach_test.sh:177

1. 🟠 Should fix: Detach tests pass even if detach never stops tracing: validate.py checks only record presence

projects/rocprofiler-sdk/tests/rocprofv3/attachment/attach-detach/validate.py:41

validate.py only checks that agents, kernel_dispatch and hsa_api records exist, so it does not show that detach stopped collection. run_attachment_detach_test.sh reads the JSON only after the app has been sent SIGINT and exited. If tool_detach never ran, tool_fini writes the output itself (skip_output = detach_output_generated is false). A broken or no-op detach (or a broken --attach-sync-output path) would therefore still produce a passing JSON, as long as rocattach_detach returned 0. Consider checking that the JSON exists before the app is killed, or that no record timestamp is later than the detach point.

Who is affected: Only test coverage: a detach regression would pass in CI; no user-visible behaviour.

Checked against the code by an independent pass.

2. 🟠 Should fix: Worst-case detach reports as ctest timeout and loses rocprofv3 log: unbounded waits under TIMEOUT 90

projects/rocprofiler-sdk/tests/rocprofv3/attachment/attach-detach/run_attachment_detach_test.sh:177

wait ${ROCPROF_PID} and wait $APP_PID have no bound of their own, so a detach that runs past what is left of TIMEOUT 90 ends as a ctest timeout rather than a script failure. On a normal run the two 30s polls return early (their full 30s happens only on failure paths that exit), so roughly 85s remain for the detach. When the timeout does fire, rocprofv3's output is lost: it goes only to rocprofv3.log, which is cat'ed after wait returns. Consider bounding the waits with a watchdog, as terminate_and_wait does in attach-roctx-pause-resume (#10433). The sibling attach-twice and attach-once-att tests use the same unbounded wait with TIMEOUT 90.

Who is affected: CI runs of rocprofv3-test-attachment-attach-detach-{enter,sigint} on a slow or loaded GPU host where rocattach_detach takes more than about 85s.

Checked against the code by an independent pass.

Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at 9f4d504a; findings checked and posted by @peymanr; points already raised on this PR are left out.

Copilot AI left a comment

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.

🟡 Changes recommended

The test can falsely pass without Enter-triggered detachment, kill valid slow detachments, and fail instead of skipping in root containers lacking CAP_SYS_PTRACE.

3 open issues
1 resolved since last review

Review effort: Balanced

@github-actions github-actions Bot removed the Stale PR has no activity for 25+ days label Nov 3, 2026
@minseobshin11
minseobshin11 merged commit 2624718 into develop Nov 3, 2026
129 of 136 checks passed
@minseobshin11
minseobshin11 deleted the users/mishin/attach-interactive-detach branch November 3, 2026 21:23
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants