Skip to content

[TRTLLM-12648][test] implement disagg cancel stress metrics_thread#14807

Merged
chienchunhung merged 1 commit into
NVIDIA:mainfrom
chienchunhung:phase0-metrics-thread
Jun 2, 2026
Merged

[TRTLLM-12648][test] implement disagg cancel stress metrics_thread#14807
chienchunhung merged 1 commit into
NVIDIA:mainfrom
chienchunhung:phase0-metrics-thread

Conversation

@chienchunhung

@chienchunhung chienchunhung commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Second 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_thread landed previously in PR #14375; this PR adds the metrics_thread.

This PR ships:

  • metrics_thread body — scrapes every worker's trtllm_kv_cache_utilization gauge from /prometheus/metrics on a fixed cadence (default 30 s, ctor-configurable), and appends timestamped samples (role, index, host, port, utilization, error) to _kv_utilization_samples for the end-of-marathon leak-detection assertion. Transport / parse failures are recorded as misses (worker mid-restart from SIGKILL, mid-SIGSTOP pause, endpoint not wired for backend) rather than fail-fast — only the log scanner is fail-fast.
  • _parse_kv_cache_utilization — pure-function Prometheus text-exposition parser: tolerates # HELP / # TYPE comments, label sets ({instance="ctx_0",pool="kv"}), and scientific notation.
  • _fetch_kv_cache_utilizationurllib-based one-shot scrape that folds every transport error (URLError, HTTPError, TimeoutError, OSError) into a (None, error_string) return so the thread can never raise.
  • WorkerLaunchSpec.host — new optional field (default "localhost") so a future multi-host config can carry a non-localhost hostname through to the scraper without further API churn.
  • metrics_scrape_interval_s / metrics_scrape_timeout_s — new ctor parameters on DisaggCancellationStressHarness, matching the existing log_scanner_poll_interval_s pattern. Tests pass small values for fast turnaround; the marathon uses defaults.
  • Shared test-helper module (_testing.py) — DUMMY_YAML + make_spec(role, index, *, port, host, log_path) factored out of the two test files that had drifted into copy-paste. test_log_scanner.py is migrated in this PR (touches ~80 lines but removes ~80 lines of dup).

The thread sits inside the lifecycle wired up by PR #14375start() already spawns it, stop() joins it, collect_results() already returns kv_utilization_samples. PR #14375 left it as a no-op stub; this PR makes it actually scrape.

PR chain plan

PR Adds Marathon assertion it enables
PR #14375 skeleton + log_scanner hard-zero log patterns
this PR metrics_thread KV-cache utilization growth bound
next injector_thread SIGSTOP / SIGCONT / SIGKILL+respawn; injection_events count
next + 1 canary_thread canary error rate + token-equivalence + recovery time
next + 2 load_thread + real setup() with save_log=True sustained load over duration_min; marathon entry point becomes a real marathon

The final PR also flips setup_disagg_cluster to save_log=True with explicit per-worker ports — at which point both the log_scanner (from PR #14375) and the metrics_thread (this PR) start scraping real worker endpoints. Today both correctly no-op when their inputs are absent: scanner skips log_path=None with a warning, scraper records scrape misses.

Tests added

All deterministic, GPU-free, millisecond-scale. Under tests/integration/defs/stress_test/disagg_cancel/.

  • test_metrics_thread.py — 16 unit tests across three layers:
    • Parser-only (6): simple gauge, gauge with labels, scientific notation, missing metric, comment-line skipping, first-sample-wins on multi-sample exposition.
    • Fetch-only (4): in-process http.server.HTTPServer validates success + missing-metric path; closed port validates connection-refused; 503 response validates HTTP error path.
    • Thread-body integration (6): multi-cycle sample collection, exit-on-no-specs, scrape-miss recording on connection-refused, mid-run worker-recovery (initial 503 → later 200, polled deterministically via a _wait_until helper rather than time.sleep), multi-worker per-cycle coverage, prompt exit on failed_event.
  • test_log_scanner.py — migrated to use the new _testing.py helpers. No new assertions, no behaviour change; verifies via the existing 17 tests.

Out of scope (deferred to later PRs in this chain)

  • YAML schema wiring of metrics_scrape_interval_s / metrics_scrape_timeout_s (constructor-only for now; the marathon uses defaults).
  • The remaining three thread bodies: injector, canary, load.
  • Real setup_disagg_cluster invocation (cluster launch is still a stub, so _worker_specs is empty in the lifecycle smoke; the metrics thread correctly logs a warning and exits in that case).
  • Registration in qa/llm_function_stress.txt for 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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

@chienchunhung
chienchunhung requested a review from a team as a code owner June 1, 2026 04:13
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR extends a disaggregation cancellation stress-test harness with Prometheus-based metrics collection. A new host field in WorkerLaunchSpec enables per-worker targeting, two utility functions parse and fetch KV-cache utilization from /prometheus/metrics, and the harness constructor accepts scraping cadence and timeout parameters. The previously stubbed metrics thread now runs a live scraper loop. A comprehensive test suite validates parsing, HTTP fetching, and multi-worker end-to-end collection with failure recovery.

Changes

Prometheus KV-cache metrics collection

Layer / File(s) Summary
Data contracts and imports
tests/integration/defs/stress_test/disagg_cancel/harness.py
Added urllib.error and urllib.request imports; extended WorkerLaunchSpec with host: str = "localhost" field to direct metrics scraping targets.
Prometheus scraping utilities
tests/integration/defs/stress_test/disagg_cancel/harness.py
Introduced _parse_kv_cache_utilization (regex-based parser extracting trtllm_kv_cache_utilization samples) and _fetch_kv_cache_utilization (HTTP fetcher returning (utilization, error) tuples with error-to-string conversion for network/parse failures).
Harness metrics integration
tests/integration/defs/stress_test/disagg_cancel/harness.py
Extended DisaggCancellationStressHarness.__init__ with metrics_scrape_interval_s and metrics_scrape_timeout_s keyword-only parameters, stored them on instance, and replaced the stubbed _metrics_thread_body with an active loop that periodically scrapes all workers, parses utilization samples, and appends timestamped records (including host, port, utilization, and error).
Test fixtures and infrastructure
tests/integration/defs/stress_test/disagg_cancel/test_metrics_thread.py
Added module-level test setup: in-process HTTP server with configurable Prometheus metrics body and HTTP 503 simulation; helpers to construct WorkerLaunchSpec instances and minimal harness from temporary YAML; and utilities to run the metrics thread with controlled shutdown.
Scraping utility validation
tests/integration/defs/stress_test/disagg_cancel/test_metrics_thread.py
Unit and integration tests for _parse_kv_cache_utilization (gauge parsing, labeled samples, scientific notation, metadata filtering, missing metric handling) and _fetch_kv_cache_utilization (success, metric_absent error, url_error categorization for connection failures and HTTP 503 responses).
Metrics thread integration tests
tests/integration/defs/stress_test/disagg_cancel/test_metrics_thread.py
End-to-end tests for _metrics_thread_body: periodic sample recording per worker spec (with role/index/host/port/utilization/error/timestamp fields), immediate exit when no workers configured, error recording and mid-run recovery from scrape failures, multi-worker coverage verification, and prompt exit on failed_event signal.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.93% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the specific component being implemented (metrics_thread), the ticket reference (TRTLLM-12648), and the change type (test/implementation), directly matching the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed PR description comprehensively explains objectives, changes, test coverage, and defers appropriately to future PRs.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
@chienchunhung
chienchunhung force-pushed the phase0-metrics-thread branch from 3bbfa0f to 5c5a0f5 Compare June 1, 2026 04:43
@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51308 [ run ] triggered by Bot. Commit: 5c5a0f5 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51308 [ run ] completed with state SUCCESS. Commit: 5c5a0f5
/LLM/main/L0_MergeRequest_PR pipeline #40725 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51394 [ run ] triggered by Bot. Commit: 5c5a0f5 Link to invocation

@chienchunhung
chienchunhung requested a review from pcastonguay June 1, 2026 19:35
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51394 [ run ] completed with state FAILURE. Commit: 5c5a0f5
/LLM/main/L0_MergeRequest_PR pipeline #40805 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51424 [ run ] triggered by Bot. Commit: 5c5a0f5 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51424 [ run ] completed with state SUCCESS. Commit: 5c5a0f5
/LLM/main/L0_MergeRequest_PR pipeline #40834 completed with status: 'SUCCESS'

CI Report

Link to invocation

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.

3 participants