[TRTLLM-12648][test] implement disagg cancellation injector thread - #14920
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
dbbcceb to
6b3d90f
Compare
…log_scanner Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #51922 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughThis PR implements a YAML-driven stress injection system for the disaggregated cancellation harness, enabling scheduled signal injection (SIGSTOP, SIGKILL) into worker processes with optional respawn and health verification. Updates include harness API extensions, comprehensive test coverage, and documentation reflecting the incremental implementation progress. ChangesInjector Stress System
Sequence DiagramsequenceDiagram
participant Harness
participant InjectorThread as Injector Thread
participant Worker as Worker Process
participant Health as Health Check
Harness->>Harness: parse_injection_schedule()
Harness->>InjectorThread: start thread
loop for each injection
InjectorThread->>InjectorThread: wait until scheduled at_min
InjectorThread->>InjectorThread: resolve_injection_target()
InjectorThread->>Worker: signal_process(SIGSTOP or SIGKILL)
Worker-->>InjectorThread: signal delivered
alt SIGKILL with respawn_within_s
Worker-->>InjectorThread: process exits
InjectorThread->>InjectorThread: _respawn_tracked_worker()
InjectorThread->>Health: _wait_for_worker_health()
alt health check succeeds
Health-->>InjectorThread: HTTP 200
InjectorThread->>Harness: record_injection_event(respawned=True)
else health check timeout
Health-->>InjectorThread: timeout
InjectorThread->>Harness: fail_event set, respawned=False
end
else SIGSTOP only
InjectorThread->>Harness: record_injection_event(skipped=False)
end
end
InjectorThread-->>Harness: thread exits on stop/fail
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/defs/stress_test/disagg_cancel/harness.py`:
- Around line 1055-1058: The bug: updating spec.log_path after respawning a
worker leaves the previously snapshotted _LogSource.path objects (used by
_log_scanner_thread_body()) pointing at the old file, so the scanner misses the
new worker's logs. Fix by updating the live log-source objects when you replace
tracked.wrapper: after setting tracked.wrapper = new_wrapper and spec.log_path =
new_wrapper.log_path, locate the worker's existing _LogSource instances (the
collection created at startup that _log_scanner_thread_body() uses) and update
their .path attributes to spec.log_path (or recreate/synchronize those
_LogSource objects) so the scanner tails the new file; ensure this update logic
is invoked in _run_worker() or the respawn code path that handles
tracked.wrapper changes.
- Around line 1055-1068: The respawn health check is always probing localhost
instead of the actual worker host; update calls to _wait_for_worker_health to
use the respawned worker's host (tracked.host or spec.host from the
WorkerLaunchSpec) along with new_wrapper.port when building the health probe
endpoint (replace any hardcoded 'localhost' usage). Make the same change for the
second occurrence around the 1070-1085 block so both respawn paths call
_wait_for_worker_health(new_wrapper.port, host=tracked.host or spec.host,
timeout_s=timeout_s) or the equivalent parameters expected by
_wait_for_worker_health.
- Around line 357-389: In _parse_injection_schedule(), after converting
at_min/duration_s/respawn_within_s to floats, validate their signs and skip
malformed entries: reject entries with at_min < 0 (log and continue), reject
sigstop entries with duration_s < 0 (log and continue), and reject sigkill
respawn_within_s values that are <= 0 (log and continue); update the existing
logger.error calls to reflect these specific validation failures and ensure the
function continues past the bad entry when these checks fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5630068d-0f67-4f56-89c5-88558191bb2f
📒 Files selected for processing (4)
tests/integration/defs/stress_test/disagg_cancel/README.mdtests/integration/defs/stress_test/disagg_cancel/harness.pytests/integration/defs/stress_test/disagg_cancel/test_injector.pytests/integration/defs/stress_test/disagg_cancel/test_log_scanner.py
|
PR_Github #51922 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52134 [ run ] triggered by Bot. Commit: |
|
PR_Github #52134 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52163 [ run ] triggered by Bot. Commit: |
|
PR_Github #52163 [ run ] completed with state |
…VIDIA#14920) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Signed-off-by: NVFB <186336021+NVFB@users.noreply.github.com>
…VIDIA#14920) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…te for PR NVIDIA#15015 merge PR NVIDIA#15015 (canary_thread) merged on 2026-06-08; 4 of 5 thread bodies now in upstream/main. The two phase0 docs (spec + implementation plan) were originally split as WHAT-to-build vs HOW-to-build, which made sense while implementing. Now that the implementation is mostly done, the impl plan is mostly historical record and its still-useful content (decisions beyond spec, risk mitigations) folds into the spec as an Implementation notes appendix. Changes: * Delete phase0-implementation-plan.md (252 lines). * phase0-stress-test-suite.md status header: skeleton + 4 thread bodies merged (only load_thread + marathon entry pending). * PR chain table: mark PR NVIDIA#14920 (injector) and PR NVIDIA#15015 (canary) as merged with dates. * Replace 'Open questions for the implementing agent' (all TBD at writing, all resolved during implementation) with an 'Implementation notes' section capturing decisions beyond the spec (serial marathon execution, per-thread PR split not per-marathon, kill-only SIGKILL fallback, canary token-equivalence fallback chain, --smoke mode) and the risks the implementation pinned in code. * Rewrite 'Acceptance criteria' from single-PR checklist to cumulative PR-chain checklist with per-PR landed-in attribution. No README updates needed — the parent README's 'Detailed design documents' section already references only the spec, not the impl plan. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Summary by CodeRabbit
New Features
Documentation
Tests
Summary
Third thread body in the disaggregated cancellation stress-test suite — the marathon-style (hours-running) harness that gates regressions of the bug class fixed by PR #13713. The skeleton +
log_scanner_threadlanded in PR #14375, and themetrics_threadin PR #14807; this PR adds theinjector_thread.This PR ships:
injector_threadbody — reads thestress_config.injectionsschedule, waits until each entry'sat_minoffset from marathon start, and fires the failure injection against the tracked workers. Each fired event (withat_min,elapsed_s,target,role,index, signal outcomes) is appended to_injection_eventsfor the end-of-marathon injection-count / recovery assertions. Only the log scanner is fail-fast; the injector trips fail-fast solely on a respawn-health timeout._parse_injection_schedule— validates and normalizes the YAMLinjections:list into sorted_InjectionSpecentries; malformed entries are logged at ERROR and skipped (a typo in one slot can't abort the marathon). Enforces thatsigstoprequiresduration_s, and onlysigstop/sigkilltypes are accepted._resolve_injection_target— mapsgen_worker_random,ctx_worker_random, and indexed{ctx,gen}_worker_<N>targets to a tracked worker; unknown / unmatched targets raise and are skipped without fail-fast._execute_sigstop_pause— SIGSTOP → bounded pause → SIGCONT. The pause is interruptible bystop_event/failed_event, and SIGCONT is always sent in afinallyso a worker is never left stopped during teardown._execute_sigkill+_respawn_tracked_worker— SIGKILL the worker, then optionally relaunch it on a freshly allocated port viadisagg_test_utils._run_worker, with a bounded/healthpoll (_wait_for_worker_health). Respawn launch errors and health timeouts returnFalse→ fail-fast.WorkerLaunchSpec+bind_tracked_workers— shadow-tracked launch context recorded at cluster setup so the injector can relaunch a SIGKILLed worker without modifying the sharedProcessWrapperinfrastructure.injector_poll_interval_s— new ctor parameter onDisaggCancellationStressHarness, matching the existinglog_scanner_poll_interval_s/metrics_scrape_interval_spattern. Tests pass small values for fast turnaround; the marathon uses defaults.The thread sits inside the lifecycle wired up by PR [TRTLLM-12648][test] add disagg cancellation stress-test harness skeleton #14375 —
start()already spawns it,stop()joins it,collect_results()already returnsinjection_events. Previous PRs left it as a no-op stub;this PR makes it actually inject.
PR chain plan
The final PR flips
setup_disagg_clusterto launch workers with explicit per-worker ports +save_log=Trueand callsbind_tracked_workers()— at which point the injector acts on real worker subprocesses. Today it correctly no-ops when its inputs are absent: with the cluster launch still stubbed,_tracked_workersis empty and the thread logs a warning and exits.Tests added
All deterministic, GPU-free, second-scale. Under
tests/integration/defs/stress_test/disagg_cancel/.test_injector.py— unit tests across three layers:respawned=True,stop_eventexits before a distant injection, no-tracked-workers / empty-schedule exit cleanly, unknown target is skipped without fail-fast, and respawn uses the allocated port with a bounded health wait (isolated via a fakedisagg_test_utilsmodule so no GPU / venv is required).Run locally (no GPU, no cluster):
```bash
PYTHONPATH=tests/integration/defs:tests/integration/defs/disaggregated
python3 -m pytest -c /dev/null -o addopts=
--confcutdir=tests/integration/defs/stress_test
tests/integration/defs/stress_test/disagg_cancel/test_injector.py -v
```
Out of scope (deferred to later PRs in this chain)
injector_poll_interval_s(constructor-only for now; the marathon uses defaults).canary,load.setup_disagg_clusterinvocation +bind_tracked_workers()(cluster launch is still a stub, so_tracked_workersis empty in the lifecycle smoke; the injector logs a warning and exits in that case)./healthon a fresh port, but does not yet assert the disagg server re-registers it into the routable pool (design open question Kaiyu/update main #5). This is validated whensetup()+ load / canary land.qa/llm_function_stress.txtfor nightly / weekly CI.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.