perf: add guarded large-cluster qualification harness - #202
Conversation
There was a problem hiding this comment.
Pull request overview
Adds deterministic and guarded live large-cluster qualification infrastructure using the real application path.
Changes:
- Adds 1k/10k/50k workload profiles and replay/live benchmark CLIs.
- Adds metrics, Kubernetes read telemetry, guarded churn, and retry handling.
- Adds extensive tests and qualification documentation.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds psutil development dependency. |
uv.lock |
Locks psutil packages. |
src/korvid/k8s/client.py |
Instruments Kubernetes reads. |
src/korvid/k8s/errors.py |
Preserves retry delay metadata. |
src/korvid/k8s/telemetry.py |
Defines read telemetry events. |
tests/k8s/test_client.py |
Tests telemetry behavior. |
tests/ui/waits.py |
Adds operational timeout exception. |
tests/ui/test_waits.py |
Tests timeout semantics. |
tests/performance/__init__.py |
Defines performance test package. |
tests/performance/cli.py |
Implements benchmark CLI. |
tests/performance/live.py |
Implements guarded AKS replay. |
tests/performance/manifests.py |
Generates live seed manifests. |
tests/performance/metrics.py |
Collects and renders measurements. |
tests/performance/profile.py |
Loads and validates profiles. |
tests/performance/replay.py |
Drives deterministic real-app replay. |
tests/performance/workload.py |
Generates deterministic workloads. |
tests/performance/test_cli.py |
Tests CLI behavior. |
tests/performance/test_live.py |
Tests live safety and replay. |
tests/performance/test_manifests.py |
Tests manifest generation. |
tests/performance/test_metrics.py |
Tests metric calculations. |
tests/performance/test_profile.py |
Tests profile validation. |
tests/performance/test_replay.py |
Tests application-path replay. |
tests/performance/test_workload.py |
Tests workload determinism. |
tests/performance/profiles/aks-1k.json |
Defines AKS comparison profile. |
tests/performance/profiles/aks-live-1k.json |
Defines 30-minute live profile. |
tests/performance/profiles/burst-50k.json |
Defines 50k burst profile. |
tests/performance/profiles/smoke-1k.json |
Defines CI smoke profile. |
tests/performance/profiles/steady-10k.json |
Defines 10k steady profile. |
docs/dev/specs/2026-08-06-large-cluster-performance-qualification-design.md |
Documents qualification protocol. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cb89109 to
dd06aa4
Compare
Address all inline review findings for the large-cluster qualification harness: - generated Pods tolerate the documented korvid.dev/performance taint - fail-closed AKS gate validates the immutable dedicated-test resource group, cluster name, and required test-only tags from az aks show before clients - ownership/preflight rejects non-Running/non-Ready owned Pods (exactly 1000) - explicit read/setup connection timeout on identity, harness, and app connects - resolve the real korvid SHA (GITHUB_SHA/git HEAD); live fails closed if none - live-specific manifest records context/ARM id, Kubernetes and node-pool version/count metadata, persisted to JSON and Markdown - separate LIST-to-populated-table/startup phases from watch event-to-render - machine-readable process-start-to-interactive, LIST-to-populated-table, max backlog depth, and post-burst drain summaries - drive filter, sort, namespace switch, split pane, describe, and multi-log UI-at-scale scenarios through the real pilot during churn, recording outcomes - extend failure vocabulary with metrics_unavailable and slow_logs plus report evidence - require exactly four run-labelled live artifacts for replay-live - persist expected/final digest and an explicit match flag - fit the RSS slope only over post-warm-up steady-state samples - correct the design namespace contract to match the generator The audit-injection finding is rejected: the audit invariant governs product agent write tools, not this dedicated performance harness; the stronger cluster/ownership/UID JSON-Patch guards are preserved and tested. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
tests/performance/profile.py:109
- Profiles can declare multiple failures at the same
at_event, butrun_replayconverts them to a dictionary keyed by event number, silently retaining only the last one. Reject duplicate event positions so a profile cannot claim failure injections that were never exercised.
return tuple(sorted(result, key=lambda failure: failure.at_event))
tests/performance/live.py:1205
- The live path never calls
BenchmarkRecorder.mark_burst_end(the only production call is in the offline replay source), so every live report records an empty drain list andmax_post_burst_drain_seconds=None. That makes the published ≤3s live burst-drain budget impossible to evaluate. Schedule burst-end marks on the live clock while churn is running, including the case where the backlog is already empty.
events = scheduled_events(profile)
state.progress.requested_events = len(events)
churn_task = asyncio.create_task(
drive_live_churn(
events,
docs/dev/specs/2026-08-06-large-cluster-performance-qualification-design.md:85
- This documented
replay-livecommand is guaranteed to fail validation because the CLI requires all four live artifacts, while the example supplies only--jsonand--out. Include run-labelled--cpu-profileand--allocation-snapshotdestinations so operators can execute the published protocol.
uv run python -m tests.performance.cli replay-live \
--profile tests/performance/profiles/aks-live-1k.json \
--context aks-korvid-contract-test \
--expected-cluster-id <ARM resource id> --run-id <run> \
--json live.json --out live.md
tests/performance/cli.py:272
- An unwritable allocation-snapshot destination raises
OSErrorhere, but both callers invoke this helper from afinallyblock after their surroundingexcept OSErrorhandler. The error therefore escapes with a traceback (and can replace the original replay failure) instead of returning the documented operational exit code 1. Move snapshot error handling into the protected path while preserving any active replay exception.
snapshot = tracemalloc.take_snapshot()
stats = snapshot.statistics("lineno")[:100]
Path(path).write_text("\n".join(str(stat) for stat in stats))
tracemalloc.stop()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
tests/performance/live.py:1047
- The live driver never calls
BenchmarkRecorder.mark_burst_end(the only call is in the offline replay source), so every live report leavespost_burst_drain_secondsempty andmax_post_burst_drain_secondsunset. That makes the published ≤3-second live burst-drain budget impossible to evaluate. Mark each profile burst boundary on the same clock used by live render recording and ensure an already-empty backlog records a zero drain.
for event in events:
elapsed = now() - start
delay = event.offset_seconds * options.time_scale - elapsed
if delay > 0:
await sleep(delay)
tests/performance/profile.py:109
- Duplicate
at_eventvalues are accepted here, butrun_replaylater builds{f.at_event: f}, silently discarding all but one declared failure. The profile hash/report can therefore claim multiple injections that were never exercised. Reject duplicate failure event positions during profile loading.
return tuple(sorted(result, key=lambda failure: failure.at_event))
Review round on PR #202 found the UI-at-scale evidence was hollow: a scenario was marked `ok=True` whenever `pilot.press` returned, without checking any resulting state. In the live wiring that meant three of the six scenarios could not have done anything and still "passed": - `describe` and `multi_log` bail out with an "unavailable" warning because no manifest/log provider was wired; - the namespace toggle was a no-op, because the configured namespace was already `ALL_NAMESPACES`, so `0` navigated to the scope it was on. None of those raises. Combined with the previous commit folding scenario outcomes into the exit status, a run could have "passed" on no evidence. Each scenario is now an ordered list of steps that each declare the observable state they must reach (filter pattern, active sort, current scope, pane count, describe screen, log pane) and wait for it; a step that does not get there records `ok=False`. The remaining steps still run so the workspace is restored and the digest/row-count convergence checks stay intact. To make the scenarios reachable the live app now gets read-only providers on the harness connection - a pods-only `get_manifest` and the read client's `stream_logs` - plus a favorite namespace so `1`/`0` really scopes down to a seeded namespace and back. That scope change restarts the application watch, which the happy-path test now pins explicitly: three watches, all still cluster-wide, because `make_live_watch_source` keeps the read pinned to the owned namespace set regardless of UI scope. Two further findings from the same round: - The live churn driver never marked burst boundaries, so every live report left `post_burst_drain_seconds` empty and printed "n/a" for the published <=3-second burst-drain budget. It now marks each boundary on the same clock the deterministic driver uses. - `load_profile` accepted duplicate `at_event` positions, which `run_replay` silently collapses into one injection while the profile hash and report still claim both. Duplicates are now rejected at load. Also extends the earlier node-shell audit-race fix: four more tests waited on a notification and then read an audit outcome that is appended afterwards on a separate `asyncio.to_thread` hop. They now share one helper that waits for the outcome entry itself. Tests: tests/performance/test_live.py:: test_ui_scenarios_are_not_marked_ok_when_the_app_state_never_changes test_run_live_replay_times_the_post_burst_drain tests/performance/test_profile.py:: test_load_profile_rejects_duplicate_failure_event_positions Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 33 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
tests/performance/live.py:1544
- The production composition root wires
MetricsPoller(kube.list_pod_metrics)(src/korvid/__main__.py:1060), but this measured app omitsmetrics. Consequently the 30-minute live run does not exercise metrics polling or measure responsiveness/API load while metrics updates are active, despite the qualification requirements. Wire the real poller through the telemetry-enabled client.
app = MeasuredKorvidApp(
config=KorvidConfig(
namespace=ALL_NAMESPACES,
# Key 1 scopes to a real seeded namespace; key 0 returns
# to all namespaces. Without a favorite the toggle is a
# no-op (the configured namespace already is
# ALL_NAMESPACES) and the scenario proves nothing.
favorite_namespaces=(manifests.namespace_name(run_id, 0),),
),
store=store,
watch_manager=watch_manager,
recorder=recorder,
docs/dev/specs/2026-08-06-large-cluster-performance-qualification-design.md:96
- The generator does not assign UIDs or resource versions:
initial_podsleavesPodSummary.uidat its empty default and the event model has no resource-version field. Remove those claims or add them to the versioned workload and hashes; otherwise the reproducibility documentation overstates what is deterministic.
The deterministic generator emits stable names, namespaces, UIDs, resource
versions, and event order from the profile seed. Repeating a profile with the
same seed must produce the same object and event hashes.
tests/performance/metrics.py:198
reconnectscounts every WATCH open after the first on a path, including deliberate stop/start cycles. The live namespace scenario necessarily opens/api/v1/podsthree times (test_live.py:1052-1064), so a healthy run is reported as having two reconnects. Emit/record reconnects explicitly from retry recovery instead of inferring them from aggregated opens.
reconnects = sum(max(counts.get("watch_open", 0) - 1, 0) for counts in paths.values())
tests/performance/metrics.py:456
- When the backlog is already empty at the burst boundary, there is nothing to drain and this sample should be
0.0. Keeping the marker pending makes the next unrelated steady-state render produce a positive drain time (or leaves the sample missing if no later render occurs), corrupting the burst-drain budget.
self._burst_end_pending.append(at)
tests/performance/replay.py:596
- The report's “final” digest is computed only from
ResourceStore, not from the rendered table. A stale/missedResourceTablecell—especially in the newly cached in-place diff path—can therefore pass with matching digests and zero drops. Validate the table's rendered state independently before reporting digest correctness.
# Compute final digest from the store (actual state).
final_digest = summary_digest(cast(Iterable[PodSummary], store.get("pods", ALL_NAMESPACES)))
tests/performance/cli.py:272
- An unwritable
--allocation-snapshotdestination raisesOSErrorhere from afinallyblock, outside the command's error handler. After a long run this produces a traceback and can mask the original replay failure, unlike the other artifact writes that return exit 1. Handle snapshot write failures without overriding an in-flight exception.
Path(path).write_text("\n".join(str(stat) for stat in stats))
tracemalloc.stop()
docs/dev/specs/2026-08-06-large-cluster-performance-qualification-design.md:85
- This documented live command is rejected because
replay-livenow requires all four artifacts and requires each filename to include the run ID. Add--cpu-profileand--allocation-snapshot, and use run-labelled names for every output so the published protocol is executable.
This issue also appears on line 94 of the same file.
uv run python -m tests.performance.cli replay-live \
--profile tests/performance/profiles/aks-live-1k.json \
--context aks-korvid-contract-test \
--expected-cluster-id <ARM resource id> --run-id <run> \
--json live.json --out live.md
…ects Second review round on PR #202. Three of these are defects the previous round's changes created or exposed. **The digest criterion never checked the table.** `expected_digest` and `final_digest` are both store digests, so a table showing 1,000 stale rows satisfied the published store/table digest criterion — and the row count was the only thing asserted about the rendering. That is exactly the guarantee the new cached in-place diff needs, since it now diffs against its own record of what it last wrote instead of reading the table back. `check_rendered_rows` projects each owned Pod onto the strings its row must display and checks them against the cells the `DataTable` actually holds. It is written from the Pod summary rather than by calling the widget's row builder on purpose: reusing the builder would only prove the widget agrees with itself. **Reconnects counted deliberate restarts.** `reconnects` was inferred as `watch_open - 1` per path. The `namespace_switch` scenario added last commit legitimately re-opens `/api/v1/pods` twice, so a perfectly healthy run reported two reconnects. A reconnect is now counted from recovery: a `watch_open` on a path whose stream previously errored. **An empty backlog at a burst boundary produced a bogus drain.** `mark_burst_end` always queued a pending marker, so with nothing to drain the next unrelated steady-state render reported its own latency as the drain (or the sample vanished if no later render arrived). Nothing to drain now records `0.0` immediately. **`_flush_allocation_snapshot` could mask a run failure.** It runs from a `finally`, so an unwritable destination raised `OSError` past the command's handler: a traceback after a 30-minute run, hiding whatever actually failed. It now reports and returns a status, tracing is always stopped, and the failure surfaces as exit 1. Docs: the published live command was no longer executable (all four artifacts are mandatory and each filename must carry the run id), and the determinism claim listed uids and resource versions, which the generator does not assign — on a live run they are whatever the cluster issued. Tests: tests/performance/test_live.py::test_rendered_rows_check_rejects_a_stale_cell tests/performance/test_metrics.py:: test_reconnects_count_only_watch_reopens_after_an_error test_reconnects_count_a_watch_reopened_after_a_dropped_stream test_mark_burst_end_records_zero_when_the_backlog_is_already_empty tests/performance/test_cli.py:: test_cli_replay_live_reports_an_unwritable_allocation_snapshot Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (5)
tests/performance/replay.py:596
- The reported
final_digestis computed fromResourceStore, not from the renderedResourceTable. A stale or skipped table update therefore still produces digest parity and a successful replay, despite the report claiming store/table correctness and despite this PR changing the table diff cache. Validate the rendered rows against the expected summaries while the app/table is still available, as the live path now does.
# Compute final digest from the store (actual state).
final_digest = summary_digest(cast(Iterable[PodSummary], store.get("pods", ALL_NAMESPACES)))
tests/performance/cli.py:378
- The live command can still exit successfully after violating most of the documented hard budgets. This shared check only enforces dropped updates and digest parity (with UI failures handled separately), but it never rejects missing/over-budget startup, LIST-to-table, event/input p95, burst drain, RSS slope/peak, or unexpected GET metrics from the hard-budget table. Add a live-profile budget evaluator that treats missing required measurements and threshold violations as failures.
if replay.dropped_updates > 0 or replay.expected_digest != replay.final_digest:
return 1
tests/performance/cli.py:490
replay-liverequires--cpu-profile, so every successful qualification takes this branch and runs the entire measured window under deterministiccProfile. That instrumentation materially changes CPU and event/input latency, making the resulting values unsuitable for the user-facing hard budgets the run is supposed to qualify. Collect the profile in a separate companion run (same SHA/profile) or use low-overhead sampling so the budget run itself remains unprofiled.
if args.cpu_profile:
replay = _run_live_with_cpu_profile(
profile,
options,
context=args.context,
expected_cluster_id=args.expected_cluster_id,
run_id=args.run_id,
cpu_profile_path=args.cpu_profile,
tests/performance/replay.py:361
next_burstresets to zero on every watch generation. After a 410/429 reconnect resumes later in the schedule, the first event re-marks every burst that already ended, producing duplicate and time-shifted post-burst drain samples. Persist the next burst index across source invocations (or initialize it from the resumed event offset).
burst_ends = burst_end_offsets(self._profile)
next_burst = 0
tests/performance/metrics.py:384
- If the sampler task exits with a
psutil/tracemallocerror, awaiting it raises before managed tracing is released. Callers then also skip their subsequent watch-manager cleanup, so a sampling failure can leak both tracing state and benchmark tasks. Release the owned tracing state in afinallyblock.
task.cancel()
with suppress(asyncio.CancelledError):
await task
if self._uses_managed_tracing:
self._release_tracemalloc()
…failure Third review round on PR #202 raised no blocking comments; these are the suppressed findings that turned out to be real correctness bugs, two of them in the burst-drain machinery the previous commits touched. **A reconnect re-marked every burst that had already ended.** `_ReplaySource` restarts at `_next_event_index` on a new watch generation, but the burst cursor was a local reset to zero each time. After a 410 drops the stream mid-schedule, the first resumed event re-marked every closed burst. Reproduced with a single-burst profile and a 410 at event 50: two drain samples for one burst, the second a spurious `0.0`. The cursor now lives on the source, alongside `_next_event_index`. **A failed sampler leaked managed tracing.** `ProcessSampler.stop()` awaits the sampler task and released tracemalloc afterwards, so a `psutil`/`tracemalloc` error inside the task raised past the release - and past the caller's own watch-manager teardown. The release now runs from a `finally`. **The offline replay never checked its rendering either.** Both digests are computed from the store, so a table full of stale cells passed - the same hole `28dec94` closed on the live path, and the one that matters for the cached in-place diff. `check_rendered_rows` moved from `live.py` to `replay.py` (`live.py` already imports from it, so this avoids an import cycle) and now runs on the offline path too, inside the app block where the table still exists, and only when the run was not aborted by an injected failure. Tests: tests/performance/test_replay.py:: test_replay_does_not_re_mark_bursts_after_a_watch_reconnect tests/performance/test_metrics.py:: test_sampler_stop_releases_tracing_even_when_the_task_failed Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 3 addressed in Fixed
Not taken, with reasons
Per the review-loop policy this is the first round with no blocking findings. |
Define deterministic replay, guarded 1,000-Pod AKS load, evidence-gated optimization, cleanup, and publication requirements for #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Define strict deterministic profile inputs for issue #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Schema v1 fixes the initial state to all Running/Ready Pods; future distributions need a new field.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make object and event streams reproducible from the issue #186 profile seed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Measure logical API load for issue #186 without changing the unobserved runtime path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid duplicate generic watch summary projection, clean up reused item lists, and document the verified exception flow from review follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add stable JSON and Markdown measurements for issue #186 comparisons. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Freeze published API aggregates, harden ProcessSampler lifecycle, and count relists only after 410 recovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ate parameters Add test_replay_gone_reconnects_with_time_scale_1 which proves the relay harness correctly handles a HTTP 410 reconnect when time_scale=1 is active: the post-reconnect watch generation must continue using the original _replay_start (set at churn_start, not reset on reconnect) so event offsets remain relative to the global churn origin rather than re-sleeping the full absolute offset from the reconnect timestamp. RED/GREEN sensitivity confirmed: a temporary mutation that applied absolute- offset sleep for gen>0 (instead of offset*scale - elapsed) caused the churn to exceed the 30s until() timeout in the full test suite: AssertionError: churn complete and all events rendered not met within 30.0s Correct two adjacent test event-rate parameters: - test_replay_time_scale_1_uses_relative_inter_event_delays: 20 eps → 3 eps (sum-of-absolute-offsets with 20 eps = 90s, reliably showing the bug; 3 eps gives ~10s offsets, still > 30s with the bug, passes in 3.8s clean) - test_replay_gone_reconnects_and_digest_matches: 3 eps → 20 eps (reconnect test needs high event density to stress re-list correctness) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…aces Finding 1 (Critical): test_replay_time_scale_1_uses_relative_inter_event_delays was insensitive to the absolute-offset bug after steady_events_per_second was changed 20->3 in commit 2082aaf. With 9 events the sum_of_offsets is only 15 s < 30 s until() guard, so the test PASSES with the bug (false-GREEN confirmed: PASSED in 12.93 s with mutation). Fix: restore steady_events_per_second=20 (60 events, sum_of_offsets=91.5 s >> 30 s). RED confirmed: FAILED in 39.66 s with mutation. Finding 2 (Important): test_replay_gone_reconnects_with_time_scale_1 with duration_seconds=2 had only ~10.25 s margin above the 30 s until() guard, leaving ~1 s true safety when pilot.press() overhead is subtracted. Fix: increase duration_seconds=2->5 (95 post-failure events, sum_of_offsets=251.75 s, margin=221.75 s) for deterministic RED even in isolation. RED confirmed in isolation: FAILED in 39.67 s with reconnect mutation. Both tests now also use a sleep_callback (new ReplayOptions field) to accumulate total scheduled sleep time and assert total_sleep < duration_seconds * 3, providing a scale-independent deterministic check independent of the until() wall-clock guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the sleep_callback recording mechanism with an injected monotonic clock (monotonic_fn) and async sleeper (async_sleep) on ReplayOptions. _ReplaySource stores self._now and self._sleep, resolved at construction from the options; production runs use time.monotonic and _sleep_default (a thin asyncio.sleep wrapper). Tests supply virtual_monotonic / virtual_sleep closures that advance a shared virtual_time float and do asyncio.sleep(0) to yield without real wall time. Both time_scale=1 tests now complete in ~0.01 s instead of 3–5 s and their total_sleep assertions fire immediately (2.59 s total) under the historical absolute-offset mutation, with no dependence on the until() 30 s guard. RED evidence (mutation: delay = offset * time_scale, no elapsed subtraction): test_replay_time_scale_1_uses_relative_inter_event_delays: assert 88.5 < 9 FAIL test_replay_gone_reconnects_with_time_scale_1: assert 247.5 < 15 FAIL Both fail in 2.59 s wall time. GREEN: 4 passed in 3.97 s (all replay tests, correct implementation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Require virtual sleep totals to match the final scheduled offset and assert the first post-410 delay. This makes the issue #186 regression fail for reconnect-origin resets and omitted sleeping, not only the original absolute-offset mutation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add tests/performance/live.py: run_live_replay drives the real production stack (KubeClient -> WatchManager -> ResourceStore -> MeasuredKorvidApp) against an already-seeded, uniquely-owned 20-namespace/1,000-pod AKS topology (built by manifests.build_seed_manifests). Fail-closed before any mutation: - Cluster identity gate: active kubeconfig context, resolved API-server hostname, and an independent `az aks show --ids` lookup must all agree. - Ownership gate: every expected namespace and pod must already carry both ownership labels; every mismatch is collected before raising. - Guarded churn: each mutation is a JSON-Patch that `test`s the target pod's UID and both ownership labels before `replace`-ing status.phase - a failed `test` op aborts the whole run, with no unguarded fallback. Reuses replay.py/metrics.py/workload.py/manifests.py/profile.py unchanged. Promoted namespace_name/pod_name/validate_run_id/MANAGED_BY_LABEL/ MANAGED_BY_VALUE/RUN_LABEL to public names in manifests.py so live.py shares the exact seeding contract instead of duplicating it. Extend cli.py with a `replay-live` subcommand (mandatory --profile/--context/ --expected-cluster-id/--run-id; optional --duration overriding only duration_seconds; no --time-scale - live churn always replays at real wall-clock time), reusing the same Markdown/JSON output and exit-code rules as `replay`. Add tests/performance/test_live.py (26 tests) and extend test_cli.py (+15 tests) covering: identity-gate rejections (wrong context/resource-id/ hostname/malformed JSON/nonzero exit/missing executable), topology mismatch, ownership-gate rejections, deterministic index-to-live-object mapping, guarded-patch construction, guarded-churn success/abort, namespace-filtered watch source, full happy-path digest parity, guard-failure and CancelledError propagation with client/watch teardown, and CLI argument/ error-handling coverage. Followed RED-GREEN-REFACTOR throughout; see the task report for RED evidence and exact verification results. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…PI telemetry Address both Task 8.2 review findings in tests/performance/live.py: MEDIUM: run_live_replay wired a single KubeClient with recorder.record_api to both the ownership gate, the pre-churn uid snapshot, the app watch source, and the post-churn independent read, so ReplayReport.api mixed ~60 harness-only LISTs with the real application LIST/WATCH telemetry. LiveDependencies now exposes a second seam, harness_kube_client_factory, that constructs a non-instrumented KubeReadClient used only for _verify_ownership and the final independent re-read. The telemetry-wired client (kube_client_factory) is now only ever handed to make_live_watch_source, so ReplayReport.api reports exactly the production application read path. Identity ordering is preserved and tightened: the app-path client is now constructed only after the harness ownership gate passes (previously it was constructed and connected up front), and mutation_client_factory is still only called after ownership verification. LOW: _verify_ownership now returns the validated (namespace, name) -> PodSummary snapshot it already collected while checking ownership, and run_live_replay reuses that snapshot as the pre-churn uid snapshot instead of immediately re-listing every namespace's Pods a second time. Tests (TDD): _FakeKubeClient now models per-call telemetry on list_objects/list_pods (mirroring the real KubeClient) and tracks list_pods/list_objects/watch_pods call counts; _happy_deps constructs separate harness/app-path fake client instances sharing the same underlying namespaces/pods dicts. New/updated coverage proves: report.api.operations["list"] == 1 (only the app watch's own LIST, none of the harness's ~60 reads); the harness client is list_pods-ed exactly twice per namespace (ownership gate + final read, no redundant third uid pass); the app-path client's list_pods is never called; the harness client's watch_pods is never called; the application-path client is never even constructed when the ownership gate rejects; and _verify_ownership's return value matches the seeded topology. All prior Task 8.2 gate/churn/CLI tests are preserved and updated only where the LiveDependencies/_FakeKubeClient signatures changed. RED: reverting live.py to its pre-fix state with only the updated tests/performance/test_live.py in place produced "20 failed, 8 passed in 1.32s" (uv run pytest -p no:tach tests/performance/test_live.py -q), all failing on the new harness_kube_client_factory field/behavior. GREEN: uv run pytest -p no:tach tests/performance/test_live.py tests/performance/test_cli.py -q -> 55 passed; uv run pytest -p no:tach tests/performance/ -q -> 102 passed; uv run ruff check/format, uv run mypy tests/performance/live.py tests/performance/cli.py, and uv run tach check all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ss trustworthy Independent whole-branch review of the #186 performance harness found three blocking defects, seven important gaps, and fifteen quality findings. This closes every credible one. Blockers - Record event-to-render at owned MODIFIED *watch receipt* instead of after the patch acknowledgement. The watch event and the patch response race over independent connections, so recording at ack could append a pending entry after its own render (reproduced: a 60s opaque wait timeout and false dropped updates) and silently folded the write round trip into a read-path latency metric that is compared against the 1k/10k/50k baselines. - Churn a dedicated non-ownership `korvid.dev/performance-tick` label on the Pod's own metadata instead of the kubelet-owned `status.phase`, keeping the atomic UID and both ownership-label JSON Patch `test` operations. Read the ground-truth cluster snapshot while the watch is still live, revalidate ownership, wait for the store digest to converge, and re-assert the exact row count before teardown. - Drive mutations with explicit bounded concurrency and a bounded per-attempt timeout, retry only HTTP 429 with a bounded policy re-issuing the identical guarded patch, and report requested events/rate next to observed events, churn wall time, achieved rate, and mutation throttles (counted separately from application read telemetry). Important - Add `aks-live-1k` (1,800s at 20 events/s with three 30s bursts at 100 events/s), matching the published live plan, and point the command help and design doc at it; `aks-1k` stays the deterministic comparison schedule. - Revalidate profile invariants after `--duration` in both the CLI and `run_live_replay`, with an explicit error before any identity/ownership work. - Cancel and drain the churn task (and every mutation task inside its task group) before any client is closed, proven by a real outer-task cancellation test. - Reject unexpected Pods in owned namespaces, and reject any expected Pod that lost its UID identity or either ownership label on the post-churn read. - Make an injected `forbidden` replay failure a named terminal abort instead of a 30-second render timeout; add direct throttled/forbidden/slow coverage. - Name the recorded API errors in wait/convergence timeouts. Quality - Remove dead `current_digest`; add `BenchmarkRecorder.pending_count()`/ `api_errors()`; replace the tautological churn/input ordering flag with a real emitted/dispatched-event signal; give `until` a dedicated `WaitTimeout`; add `get_object` error telemetry; reuse the bound LIST payload items; use the injected clock for live input latency; handle output-path `OSError`; untrack the `.superpowers/sdd/task-4-report.md` session artifact; promote `build_manifest`; connect the mutation client eagerly under a bounded timeout; carry `object_index` on `ScheduledEvent` instead of parsing Pod names; bound the overall churn wait; count only resource-update renders. - Keep exact decoded-byte accounting: measured at 8.8ms for a 1,000-Pod LIST (0.4% of the 2s budget) and 9us per watch event, while the suggested `len(str(payload))` saves 1.3ms and reports Python repr characters instead of bytes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve Kubernetes 429 retry metadata, bound retry delays, and spread concurrent workers with deterministic target-specific jitter.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Treat Retry-After as a server floor and add stable target-specific jitter before applying the configured ceiling, preventing workers from retrying in lockstep when the hint dominates exponential backoff.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Handle expected terminal replay failures as clean CLI errors while preserving propagation of unexpected programmer defects. Add a valid forbidden-failure profile regression test that proves no traceback is emitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address all inline review findings for the large-cluster qualification harness: - generated Pods tolerate the documented korvid.dev/performance taint - fail-closed AKS gate validates the immutable dedicated-test resource group, cluster name, and required test-only tags from az aks show before clients - ownership/preflight rejects non-Running/non-Ready owned Pods (exactly 1000) - explicit read/setup connection timeout on identity, harness, and app connects - resolve the real korvid SHA (GITHUB_SHA/git HEAD); live fails closed if none - live-specific manifest records context/ARM id, Kubernetes and node-pool version/count metadata, persisted to JSON and Markdown - separate LIST-to-populated-table/startup phases from watch event-to-render - machine-readable process-start-to-interactive, LIST-to-populated-table, max backlog depth, and post-burst drain summaries - drive filter, sort, namespace switch, split pane, describe, and multi-log UI-at-scale scenarios through the real pilot during churn, recording outcomes - extend failure vocabulary with metrics_unavailable and slow_logs plus report evidence - require exactly four run-labelled live artifacts for replay-live - persist expected/final digest and an explicit match flag - fit the RSS slope only over post-warm-up steady-state samples - correct the design namespace contract to match the generator The audit-injection finding is rejected: the audit invariant governs product agent write tools, not this dedicated performance harness; the stronger cluster/ownership/UID JSON-Patch guards are preserved and tested. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The fail-closed live cluster identity gate awaited the first external `az aks show` command without a timeout, so a stuck kubeconfig exec / credential plugin could hang the gate indefinitely before any client was constructed. Wrap the command_runner await in `asyncio.timeout(limits.read_connect_timeout_seconds)`, consistent with the adjacent context-host lookup and setup connects. TDD: added test_run_live_replay_bounds_the_az_aks_show_lookup mirroring the context-host bounding test (RED: hung ~31s and raised ValueError; GREEN: bounded, raises TimeoutError in <0.05s). Full tests/performance/test_live.py: 72 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Long-running sessions against AKS died with HTTP 401 after ~22 minutes: kubelogin's exec credential expired and nothing re-ran the exec plugin. `load_refreshable_kube_config()` now installs a `refresh_api_key_hook` that re-runs the kubeconfig loader shortly before the exec credential expires. Because `Configuration.set_default()` and a default-constructed `ApiClient()` both deep-copy the configuration, a shared refresh closure alone is not enough - each copy owns its own `api_key`. The hook therefore tracks a shared refresh generation and re-applies the loader's cached credential to any copy that has fallen behind, so every clone converges without re-spawning the exec plugin. Refresh is serialized behind a lock, bounded by the existing probe timeout, and errors propagate instead of being swallowed. A failed refresh deliberately leaves the generation untouched so other copies still attempt a refresh rather than trusting a stale token. `connect()`, `switch_context()`, `open_pod_exec()` and the live performance mutation client now pass the active configuration explicitly to `ApiClient`/`WsApiClient` so refreshed credentials actually reach the transport. Tests: tests/k8s/test_client.py (refresh, serialization, stale-copy propagation, static-token no-op, generation-invariant on failure, connect/switch/exec wiring) and tests/performance/test_live.py::test_mutation_client_connect_uses_refreshable_kube_config Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`test_node_shell_cleanup_failure_warns_and_audits` waited on the failure notification and then read the audit log after the app context closed. The outcome entry is appended *after* that notification, on a separate `asyncio.to_thread` hop, so a loaded full-suite run could tear the app down first and find only the `intent` entry. It now waits for the audit record itself. This originally also carried a `_rendered_cells` diff cache for `ResourceTable`; #209 landed the same optimisation on main as `_emitted`, with a memo and pruning on top, so that part is dropped in favour of the upstream implementation. Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…facts Two review findings on the live qualification command: `drive_ui_scenarios` deliberately records a key sequence that never reached its target state as `ScenarioResult(ok=False)` instead of raising, so the scenario outcomes never reached the exit status. A live run whose filter, split-pane, describe, or multi-log evidence failed still reported success — exactly the case where the qualification has nothing to show. `replay-live` now folds failed scenarios into its exit status and names them on stderr. Artifact distinctness compared raw path strings, so aliases such as `sub/../run.json` and `run.json` passed the check while resolving to the same file, letting a later artifact write silently destroy an earlier one. Distinctness is now decided on resolved paths. The design doc records both: failed UI-at-scale scenarios join the hard budget table at 0. Tests: tests/performance/test_cli.py:: test_cli_replay_live_fails_when_a_ui_scenario_did_not_pass test_cli_replay_live_rejects_artifact_paths_that_alias_one_file Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review round on PR #202 found the UI-at-scale evidence was hollow: a scenario was marked `ok=True` whenever `pilot.press` returned, without checking any resulting state. In the live wiring that meant three of the six scenarios could not have done anything and still "passed": - `describe` and `multi_log` bail out with an "unavailable" warning because no manifest/log provider was wired; - the namespace toggle was a no-op, because the configured namespace was already `ALL_NAMESPACES`, so `0` navigated to the scope it was on. None of those raises. Combined with the previous commit folding scenario outcomes into the exit status, a run could have "passed" on no evidence. Each scenario is now an ordered list of steps that each declare the observable state they must reach (filter pattern, active sort, current scope, pane count, describe screen, log pane) and wait for it; a step that does not get there records `ok=False`. The remaining steps still run so the workspace is restored and the digest/row-count convergence checks stay intact. To make the scenarios reachable the live app now gets read-only providers on the harness connection - a pods-only `get_manifest` and the read client's `stream_logs` - plus a favorite namespace so `1`/`0` really scopes down to a seeded namespace and back. That scope change restarts the application watch, which the happy-path test now pins explicitly: three watches, all still cluster-wide, because `make_live_watch_source` keeps the read pinned to the owned namespace set regardless of UI scope. Two further findings from the same round: - The live churn driver never marked burst boundaries, so every live report left `post_burst_drain_seconds` empty and printed "n/a" for the published <=3-second burst-drain budget. It now marks each boundary on the same clock the deterministic driver uses. - `load_profile` accepted duplicate `at_event` positions, which `run_replay` silently collapses into one injection while the profile hash and report still claim both. Duplicates are now rejected at load. Also extends the earlier node-shell audit-race fix: four more tests waited on a notification and then read an audit outcome that is appended afterwards on a separate `asyncio.to_thread` hop. They now share one helper that waits for the outcome entry itself. Tests: tests/performance/test_live.py:: test_ui_scenarios_are_not_marked_ok_when_the_app_state_never_changes test_run_live_replay_times_the_post_burst_drain tests/performance/test_profile.py:: test_load_profile_rejects_duplicate_failure_event_positions Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ects Second review round on PR #202. Three of these are defects the previous round's changes created or exposed. **The digest criterion never checked the table.** `expected_digest` and `final_digest` are both store digests, so a table showing 1,000 stale rows satisfied the published store/table digest criterion — and the row count was the only thing asserted about the rendering. That is exactly the guarantee the new cached in-place diff needs, since it now diffs against its own record of what it last wrote instead of reading the table back. `check_rendered_rows` projects each owned Pod onto the strings its row must display and checks them against the cells the `DataTable` actually holds. It is written from the Pod summary rather than by calling the widget's row builder on purpose: reusing the builder would only prove the widget agrees with itself. **Reconnects counted deliberate restarts.** `reconnects` was inferred as `watch_open - 1` per path. The `namespace_switch` scenario added last commit legitimately re-opens `/api/v1/pods` twice, so a perfectly healthy run reported two reconnects. A reconnect is now counted from recovery: a `watch_open` on a path whose stream previously errored. **An empty backlog at a burst boundary produced a bogus drain.** `mark_burst_end` always queued a pending marker, so with nothing to drain the next unrelated steady-state render reported its own latency as the drain (or the sample vanished if no later render arrived). Nothing to drain now records `0.0` immediately. **`_flush_allocation_snapshot` could mask a run failure.** It runs from a `finally`, so an unwritable destination raised `OSError` past the command's handler: a traceback after a 30-minute run, hiding whatever actually failed. It now reports and returns a status, tracing is always stopped, and the failure surfaces as exit 1. Docs: the published live command was no longer executable (all four artifacts are mandatory and each filename must carry the run id), and the determinism claim listed uids and resource versions, which the generator does not assign — on a live run they are whatever the cluster issued. Tests: tests/performance/test_live.py::test_rendered_rows_check_rejects_a_stale_cell tests/performance/test_metrics.py:: test_reconnects_count_only_watch_reopens_after_an_error test_reconnects_count_a_watch_reopened_after_a_dropped_stream test_mark_burst_end_records_zero_when_the_backlog_is_already_empty tests/performance/test_cli.py:: test_cli_replay_live_reports_an_unwritable_allocation_snapshot Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…failure Third review round on PR #202 raised no blocking comments; these are the suppressed findings that turned out to be real correctness bugs, two of them in the burst-drain machinery the previous commits touched. **A reconnect re-marked every burst that had already ended.** `_ReplaySource` restarts at `_next_event_index` on a new watch generation, but the burst cursor was a local reset to zero each time. After a 410 drops the stream mid-schedule, the first resumed event re-marked every closed burst. Reproduced with a single-burst profile and a 410 at event 50: two drain samples for one burst, the second a spurious `0.0`. The cursor now lives on the source, alongside `_next_event_index`. **A failed sampler leaked managed tracing.** `ProcessSampler.stop()` awaits the sampler task and released tracemalloc afterwards, so a `psutil`/`tracemalloc` error inside the task raised past the release - and past the caller's own watch-manager teardown. The release now runs from a `finally`. **The offline replay never checked its rendering either.** Both digests are computed from the store, so a table full of stale cells passed - the same hole `28dec94` closed on the live path, and the one that matters for the cached in-place diff. `check_rendered_rows` moved from `live.py` to `replay.py` (`live.py` already imports from it, so this avoids an import cycle) and now runs on the offline path too, inside the app block where the table still exists, and only when the run was not aborted by an injected failure. Tests: tests/performance/test_replay.py:: test_replay_does_not_re_mark_bursts_after_a_watch_reconnect tests/performance/test_metrics.py:: test_sampler_stop_releases_tracing_even_when_the_task_failed Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
00beec4 to
4e38fb8
Compare
|
Rebased onto Specifically: my Resolution: took Note for the issue #186 numbers: the live before/after I published measured Full gate green after the rebase: 4305 passed, ruff/mypy/tach clean. |
The qualification design says main carries compact baseline and optimized summaries plus the supported scale envelope and known limits. It did not. This adds them from the recorded live runs rather than leaving the numbers only in the issue thread. Records both budgets that pass and the two that miss: event-to-render p95 (299ms against a 250ms budget, but measured at 24 ev/s against a budget written at 20) and cursor-input p95 (2.4s against 100ms, more than 20x over and untouched by the render work). Also flags what is not trustworthy yet: the UI-at-scale interaction timings predate the harness fix that makes those scenarios wait for the target state, so they are upper bounds taken under CPU saturation, not measurements. Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Status: ready to merge, waiting on GitHub Actions. Everything on the branch is done — rebased onto CI on the current head has passed everything that could get a runner: GitHub has had Actions in Local full gate on this exact commit: 4305 passed, 21 skipped, ruff / mypy / tach clean. Not merging until |
The CI run for ade1fd0 has been stuck in queued for ~12 hours since the GitHub Actions major outage on 2026-08-06; its jobs API returns zero jobs and it can neither be cancelled nor re-run. Pushing an empty commit to get a clean run now that hosted runner queues have drained. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Retry-Afterpreservation, bounded deterministic jitter, and desynchronized retriesWhy
Issue #186 requires evidence-driven large-cluster qualification before changing production hot paths. This harness establishes reproducible offline baselines and a fail-closed live workflow so optimizations are made only when measured AKS evidence demonstrates a budget violation or bottleneck.
Safety and review notes
Validation
make checkCloses #186