feat(e2e): performance harness with incremental checkpoints - #212
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
e174d67 to
65f52c7
Compare
Amber reviewStatus: In progress Amber started a review of commit 65f52c7 at 2026-08-27T18:46:49Z. |
818056e to
89739c3
Compare
Amber reviewStatus: Complete VerdictCOMMENT (approve-with-nits). This is a well-structured, well-tested local/manual performance harness plus a partial OpenShift e2e driver and an I reviewed against Findings
Cross-PR coordinationTwo open pull requests require a maintainer decision or a defined merge order relative to this one:
Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT (approve-with-nits). This is a well-structured, well-tested local/manual performance harness plus a partial OpenShift e2e driver and an E2E_MODE=short|long split; there is no production Go code, long remains the default so existing CI is untouched, and both cluster-free unit suites (tests/e2e/perf/lib_test.sh, tests/e2e/openshift_driver_test.sh) pass locally. My findings are all Minor (worker credential handling, a small JSON-schema default inconsistency, and nondeterministic seed selection); none block merge, but a couple of cross-PR coordination points below need a maintainer decision.
I reviewed against CLAUDE.md, security.spec.md, control-plane/conventions.spec.md, and the review checklists. I paid particular attention to Test Diff Scrutiny: the pre-existing assertions in tests/e2e/e2e-openshell.sh are gated behind if e2e_step long, and because E2E_MODE defaults to long, every previously-unconditional assertion still runs in the default/CI path — no guarantee is silently removed. The CA-extraction assertion that changed for the openshift branch is a new-driver code path, not a weakened contract for the existing kind path.
Findings
-
[Minor] Credentials spliced into
bash -cworker command strings (argv exposure).
Intests/e2e/e2e-performance.sh, both the scale-up workers (export E2E_OIDC_PASSWORD="'"${E2E_OIDC_PASSWORD}"'"and friends) and the teardown workers build abash -c '...'string with the secret values interpolated as literals. Those values then appear in the child process argv (visible viaps//proc/<pid>/cmdline) and the single-quote splice breaks on any value containing a quote or shell metacharacter. These are dev/test credentials so severity is Minor, but the safer pattern is toexportthe variables in the parent shell and let the backgrounded subshell inherit them — which the code already relies on forOPENSHIFT_NAMESPACE/E2E_HS_NAMESPACE. That would also remove the current asymmetry where OIDC vars are forwarded explicitly butOPENSHIFT_NAMESPACE(needed by the openshift driver'sacquire_oidc_token) is only inherited. Confidence: High. -
[Minor]
perf_results_initdefault latency objects omit theavgkey.
tests/e2e/perf/lib.sh:404-405initializePERF_RES_CREATE_JSON/PERF_RES_TTR_JSONto{"p50": null, ...}withoutavg, whereas the module-level defaults (lines 283-284), the documented schema, and completed-run writes all includeavg. A run that is interrupted before scale-up metrics are computed therefore writes a history file whosetime_to_running_secondsobject has noavg. This happens to render as-(matching the spec's "Partial Run Shows Dashes" scenario, so it is harmless in practice), but the two init strings should include"avg": nullfor schema consistency. Confidence: High. -
[Minor] Nondeterministic seed selection.
e2e_discover_seed_ids/e2e_json_first_id(tests/e2e/lib.sh) bind to the first fleet/cluster/release returned by the API. That is correct for the single-seed dev/CI environment the harness targets, but on any environment with more than one fleet/cluster/release the harness silently binds to an arbitrary one determined by API ordering. Consider selecting by a known name or documenting the single-seed assumption at the discovery call site. Confidence: Medium.
Cross-PR coordination
Two open pull requests require a maintainer decision or a defined merge order relative to this one:
-
#194 (adopt upstream OpenShell Helm chart for gateway deployments) changes the gateway deployment mechanism and renames in-namespace gateway resources (e.g.
openshell-ca→openshell-gateway-ca,openshell-server→openshell-gateway-server) while editing the same gateway-verification region oftests/e2e/e2e-openshell.shthat this PR reworks into short/long steps. This PR's new OpenShift driver (discover_gateway_endpoint,wait_for_gateway_route) and the performance harness readiness checks assume the current provisioning path and resource names. The two make incompatible assumptions about how a gateway is deployed and what its in-namespace objects are called; whichever merges second must reconcile the resource names and re-validate the harness/driver readiness probes against the Helm-based deployment. Both PRs also rewrite the sharedskills/RECONCILE.mdcheckpoint state, so the coverage/gap tables will need to be reconciled rather than blindly merged. A merge-order decision is needed. -
#200 (define control plane reconciliation contract) revises the gateway-deletion and managed-namespace-GC contract in the same
specs/platform/e2e-testing.spec.md, restating deletion as finalizer-driven cleanup. This PR adds new normative scenarios and a short-mode "throwaway gateway" GC test premised on the delete-driven GC semantics (accepts204/404and polls the namespace to disappearance). The two edits describe the same behavior from different angles; maintainers should confirm which deletion/GC wording is authoritative and align this PR's new short-mode GC assertions with #200's finalization model, deciding the merge order accordingly.
Findings Summary (ordered by severity, highest first)
- [Minor] Credentials interpolated into
bash -cworker argv - Security (tests/e2e/e2e-performance.sh) - [Minor]
perf_results_initdefault latency objects omitavg- Spec/Schema Consistency (tests/e2e/perf/lib.sh:404-405) - [Minor] Nondeterministic seed fleet/cluster/release selection - Robustness (
tests/e2e/lib.sh)
Convention Checklist
| Convention | Result |
|---|---|
| Conventional commit messages | Pass |
Input validated (E2E_MODE validated, fails fast) |
Pass |
| Reconcile / reuse-or-create pattern (not create-or-skip) | Pass |
Test Diff Scrutiny (default long preserves existing assertions) |
Pass |
| No secrets in logs or command arguments | Fail (Minor - argv splice) |
| Config separate from code (env-driven knobs) | Pass |
| export E2E_OIDC_ISSUER="'"${E2E_OIDC_ISSUER}"'" | ||
| export E2E_OIDC_CLIENT_ID="'"${E2E_OIDC_CLIENT_ID}"'" | ||
| export E2E_OIDC_USERNAME="'"${E2E_OIDC_USERNAME}"'" | ||
| export E2E_OIDC_PASSWORD="'"${E2E_OIDC_PASSWORD}"'" |
There was a problem hiding this comment.
[Minor - Security] The scale-up worker interpolates E2E_OIDC_PASSWORD (and the other OIDC vars) as a literal inside the bash -c '...' string, so the secret ends up in the child process argv (ps//proc/<pid>/cmdline) and the single-quote splice breaks on any value containing a quote/metacharacter. The teardown worker (~line 182) does the same. Prefer export-ing these in the parent shell and letting the backgrounded subshell inherit them — as this code already does for OPENSHIFT_NAMESPACE. That also removes the asymmetry where OPENSHIFT_NAMESPACE (needed by the openshift driver's acquire_oidc_token) is only inherited, not forwarded.
There was a problem hiding this comment.
Addressed in 85c7a8c. Scale-up and teardown workers now inherit API_HOST, OIDC, seed ids, and OPENSHIFT_NAMESPACE from the parent via perf_export_child_env. The bash -c strings no longer interpolate secrets into argv.
| PERF_RES_WALL="null" | ||
| PERF_RES_THROUGHPUT="null" | ||
| PERF_RES_CREATE_JSON='{"p50": null, "p90": null, "p99": null, "max": null}' | ||
| PERF_RES_TTR_JSON='{"p50": null, "p90": null, "p99": null, "max": null}' |
There was a problem hiding this comment.
[Minor - Schema consistency] These init defaults omit the avg key, unlike the module-level defaults (lines 283-284), the documented schema, and completed-run writes, which all include avg. An interrupted run therefore writes time_to_running_seconds with no avg. It renders as - (matching the spec's "Partial Run Shows Dashes" scenario, so harmless), but include "avg": null here for schema consistency.
There was a problem hiding this comment.
Addressed in 85c7a8c. perf_results_init now writes the same {"avg": null, "p50": null, "p90": null, "p99": null, "max": null} objects as the module-level defaults and the schema.
1ca6671 to
8e0a159
Compare
Implements the HYPERSHELL-18 performance-test spec: adds E2E_MODE (short/long) step depth to the e2e suite, an infra-agnostic tests/e2e/e2e-performance.sh harness that scales up a gateway fleet in batches with per-batch short-mode checkpoints, tests/e2e/perf/lib.sh utilities (timing, percentiles, bounded concurrency, JSON results I/O), scripts/perf-report.sh for local reporting, and the make e2e-performance / e2e-performance-report targets. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Short-mode throwaway gateways failed every performance run because cluster and release ids were not forwarded and discovery skipped when fleet was already set. Report output now documents the table, prints wall clock as HH:MM:SS, and aligns checkpoint rows. Assisted-by: Cursor Grok 4.6
Export OIDC and API env in the parent so bash -c workers inherit credentials instead of taking them on argv. Include avg in init latency JSON. Kind seed discovery selects default/local-kind/ dev-release by name unless the caller overrides. Assisted-by: Cursor Grok 4.6
Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
8e0a159 to
c298b9e
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured, infra-agnostic bash performance harness (batched scale-up, canary checkpoints, functional gate, optional SLOs, schema_version=1 JSON) plus a manual OpenShift e2e driver and E2E_MODE=short|long step tagging; it is backward-compatible (long remains the default) and touches only test/build/spec files, no Go production code. The changes are carefully written around security (secrets go through the environment, never argv), are covered by runnable unit tests that pass locally, and the spec/RECONCILE updates track the behavior — I have only minor comments and one cross-PR coordination item.
Amber Assessment
Confidence: High. I read the full diff, ran bash -n on every changed script, and executed both tests/e2e/perf/lib_test.sh (28 tests) and tests/e2e/openshift_driver_test.sh (9 tests) — all pass with no cluster required.
What's good
- No secrets in argv/logs.
perf_export_child_envdeliberately passes OIDC credentials through the child environment rather than splicing them into thebash -ccommand string, and there is a unit test asserting the password never reaches worker argv. Good instinct. - Backward compatible.
E2E_MODEdefaults tolong, and for thekinddriver_driver_curlremainscurl -sk, so existing CI behavior is unchanged. The OpenShift driver correctly verifies TLS (curl -sS). - Signal-safe teardown. The EXIT trap masks INT/TERM during cleanup, cancels workers before removing temp state, deletes via the API first and then reaps tracked namespaces under one bounded timeout. This is thoughtful.
- Removes hardcoded ids. Gateway create bodies now discover seeded cluster/release ids via the API instead of the previous hardcoded
e2e-cluster/e2e-release, and the head commit correctly drops Fleet seeding to match the post-Fleet data model onmain.
Findings (minor)
See inline comments. Neither blocks merge.
- [Minor]
e2e_lookup_gateway_by_nameinterpolates the gateway name into thesearch=name%3D${name}query string without URL-encoding the value. Safe today (names are DNS labels and the Python filter re-checks exact name), but a value containing&/space would corrupt the query. Robustness (tests/e2e/lib.sh). - [Minor]
perf_provision_onerunsacquire_oidc_token 2>/dev/null || true, so a systemic auth failure surfaces only as generic per-gatewayfailrecords with no reason recorded. Consider a dedicatedauthstage/record. Observability (tests/e2e/perf/lib.sh).
Cross-PR coordination
Another open pull request (#232, [HYPERSHELL-44] OpenShift development) implements OpenShift support for the same HYPERSHELL-44 / openshift-development.spec.md scope as this PR's OpenShift slice. The maintainers/owner should coordinate the following before both land:
- Overlapping ownership of the OpenShift effort and its specs. This PR adds a manual OpenShift e2e/perf driver (
tests/e2e/drivers/openshift.sh) framed as a partial implementation ofopenshift-development.spec.md, while #232 adds the OpenShift cluster lifecycle (scripts/cluster/drivers/openshift.sh,make openshift-up/down/swap). The two introduce distinctopenshift.sh"driver" abstractions and both rewrite the same coverage tables inskills/RECONCILE.mdand editspecs/platform/e2e-testing.spec.md,specs/platform/local-development.spec.md, andtests/e2e/e2e-openshell.sh. A merge order and an agreed ownership boundary are needed so the coverage tables and spec sections converge rather than overwrite each other. - Conflicting data-model assumption about Fleet. This PR's head commit removes Fleet seeding from e2e/local dev to match
main(Fleet was deleted from the stack). #232 still seeds aFleetresource as part ofopenshift-up. That assumption contradicts the current data model and must be reconciled — a decision is required on which direction is authoritative before #232 merges.
Findings Summary (ordered by severity, highest first):
- [Minor] Gateway name not URL-encoded in
search=query - Robustness (tests/e2e/lib.shL225) - [Minor] Silent auth-failure swallowing in provisioning worker - Observability (
tests/e2e/perf/lib.shL641)
Convention Checklist (only conventions applicable to this diff):
| Convention | Result |
|---|---|
| No secrets in logs/argv | Pass |
| Input validated (URL/query construction) | Minor gap |
| Errors propagated, not silently swallowed | Minor gap |
| Config separate from code (env-driven, no hardcoded ids) | Pass |
| Conventional commit messages | Pass |
| Tests accompany new behavior | Pass |
| Spec/RECONCILE updated to match behavior | Pass |
| _GW_NAMESPACE="" | ||
| _GW_PHASE="" | ||
| local resp | ||
| resp=$(api_curl "${API_HOST}/api/hypershell/v1/gateways?search=name%3D${name}" 2>/dev/null || true) |
There was a problem hiding this comment.
The gateway name is interpolated straight into the query string (search=name%3D${name}) without URL-encoding the value. It's safe for the current callers (names are K8s DNS labels and the Python block below re-filters on an exact name match), so this is not a blocker — but a value containing &, space, or # would silently corrupt the query. Consider URL-encoding the value (e.g. via the existing python3 helper) if this lookup is ever reused for arbitrary names.
| running_s="null" | ||
| printf '%s\t%s\n' "$name" "authenticating" > "${record}.state" | ||
|
|
||
| acquire_oidc_token 2>/dev/null || true |
There was a problem hiding this comment.
acquire_oidc_token 2>/dev/null || true swallows all auth output and errors here. Under a systemic auth failure (expired admin creds, Keycloak down) every worker will still proceed and record a generic fail with no reason, making triage harder during a large scale-up. Consider recording an explicit auth-failure stage/record (as perf_delete_gateway_by_name already does with its auth marker) so the diagnostics distinguish auth failures from provisioning timeouts.
2 participants
What
Implements the HYPERSHELL-18 performance-test spec: an infra-agnostic e2e performance harness with batched, checkpointed gateway scale-up.
Highlights
E2E_MODE=short|longstep-depth tagging to the e2e suite (tests/e2e/lib.sh,tests/e2e/e2e-openshell.sh); long remains the default so existing CI is unchanged.tests/e2e/e2e-performance.shharness: batched gateway scale-up with bounded concurrency, per-batch short-mode checkpoints, a final long-mode functional gate, optional SLO gating, and EXIT-trap teardown.tests/e2e/perf/lib.shutilities (timing, latency percentiles, bounded concurrency, schema_version=1 JSON results I/O) plustests/e2e/perf/lib_test.shunit tests (no cluster required).scripts/perf-report.shandmake e2e-performance/make e2e-performance-reporttargets to run the harness and tabulate local run history.bash, notpython3/jqas previously stated.Scope
Local/manual performance testing only — not wired into PR CI (too slow/heavy for the 20-minute gate). The OpenShift driver itself remains out of scope (tracked under HYPERSHELL-44).