Repository navigation
kind: run the rate limiters on Redis by default, as deployments do - #480
Conversation
f1e7708 to
9a508e3
Compare
9a508e3 to
a629528
Compare
a629528 to
409f142
Compare
nickdujay-pplx
left a comment
There was a problem hiding this comment.
Reviewed 409f142 against #485. The Kind wiring and limiter recovery flow look reasonable, and all four Kind CI shards passed. Requesting one small correction for the other consumer of these charts: the caller-provided-cluster installer now silently inherits a Redis endpoint it does not provision. Details inline.
I reproduced the before/after with the documented umbrella-chart example values: the parent renders no SPACE_REDIS_URL; this head renders redis://space-redis:6379 on API, admin API and cluster-controller, with no Redis Service. Please keep the Kind default while making the umbrella's Redis configuration intentional.
Kind deployed space-redis in both bring-up paths, but no chart set SPACE_REDIS_URL, so the cluster activation limiter and the per-client concurrency limiter ran on their fail-open (`misconfigured`) arms in every run except the one rate-limiter scenario, which patched the URL into space-api itself. The charts now render SPACE_REDIS_URL=redis://space-redis:6379 where the code reads it: space-api and space-admin-api (both limiters are built above the --surface branch, in AppState::from_pools_with_lifecycle) and space-cluster-controller (NativePublicOperationExecutor builds its own activation limiter from ClusterSandboxAdmissionConfig::from_env). The internal-api binary links no reader, so it gets nothing. The activation limiter runs at the binary defaults and per-client concurrency runs in `observe`, which deployments pin; observe needs no allowlist, since unrecognised ids are metered in the shared `unspecified` bucket. The infra/space/cluster installer reuses these charts but provisions no Redis, so its defaults keep redis.url empty for both, and an empty URL now renders no SPACE_REDIS_URL: an install that names no store renders as before, with both limiters off. Its values.example.yaml shows how to name the caller's store, and templates/validate.yaml refuses the Kind endpoint and API and controller URLs that differ (they share the activation bucket). Redis stays plaintext: the workspace's Redis clients are built with `tls-rustls-webpki-roots`, so they trust only webpki roots and would refuse a per-cluster CA, and the `#insecure` fragment would be an environment-only switch. The README's Differences table records that and the internal-api omission. Redis gets a PING readiness probe, and Tilt now starts space-api after it (bootstrap.sh already waited for its rollout before the API). local.rate_limiters_enforce_and_fail_open no longer writes the URL. It checks the API has one, shrinks the public API's caps (keeping the burst test hook, since the default burst of 60 would take 60+ VM boots to drain) and switches per-client concurrency to enforce, scales Redis to zero for the fail-open half, and adds a recovery phase: once Redis is back, a connect burst draws a per-client 429 again. The controller's copy of the activation caps is left at the defaults, and the scenario says why: it sends only ECW requests, which the API admits itself. The scenario's faults are recoverable when its own cleanup does not run or fails: - The public-API mutation Lease records the standing limiter env and the Redis replica count before either changes. A record is cleared only once its restore succeeded. - Every acquisition of that Lease, including of a released one, puts back whatever is still recorded. A Lease with a repair can also be taken over from a holder proven dead (same user and host, pid gone), so a killed run does not block the next one for the Lease's hour. - Every write under the Lease first renews it in one patch guarded by uid, resourceVersion and holder, which also sets or clears the records. A runner resumed after a successor took over the Lease fails there instead of undoing the successor's fault or erasing its records. - Before every scenario on the kind backend, `space_smoke run` reads the Lease. If a fault is recorded, it acquires the Lease to repair it, or refuses to run while a live holder has it up. The worker-cordon Lease's repair also runs on a released Lease now. Also removes kind_stack_ensure_redis, whose only caller script is gone, and corrects the reference README's sentence about how Kind sets the URL.
409f142 to
ed4554e
Compare
nickdujay-pplx
left a comment
There was a problem hiding this comment.
Re-reviewed ed4554e against the previously reviewed head. My Redis installer blocker is resolved: the shared cluster chart defaults omit SPACE_REDIS_URL on API, admin API, and controller, preserving operation without a Redis store. Callers can supply matching external URLs; mismatched URLs and the absent Kind store are rejected.
Validation: all 45 existing chart tests and 11 disposable render checks passed. The Kind API/controller default renders are semantically identical to the previously reviewed head. This follow-up is limited to Helm defaults, validation, and conditional environment rendering; I found no further correctness, deployment, or rollback blocker.
Pull CI is green. The four Kind smoke shards and Rust CodeQL are still running at review time; their completion remains part of the merge checks.
What Kind now does like deployments
Kind already deployed
space-redisin both bring-up paths, but no chart setSPACE_REDIS_URL. So the cluster-activation admission limiter and the per-client concurrency limiter ran on their fail-openmisconfiguredarms in every run. The one exception waslocal.rate_limiters_enforce_and_fail_open, which patched the URL intodeploy/space-apiitself. Onmain, with no URL rendered, every activation and per-client decision outside that scenario isaccepted|admitted / misconfigured. On this round's bench, no decision wasmisconfigured. Each was one of three kinds:none(made against the store), a refusal a scenario provokes on purpose, orstore_unavailablewhile a drill or the scenario had Redis down.With this change Kind renders
SPACE_REDIS_URL=redis://space-redis:6379wherever the code reads it, in both bring-up paths (Tilt andbootstrap.shrender the same charts):space-api,space-admin-apiAppState::from_pools_with_lifecyclebuilds both limiters above the--surfacebranch:ClusterSandboxAdmissionConfig::from_envandClientConcurrencyConfig::from_env.space-cluster-controllerNativePublicOperationExecutor::new) builds its own activation limiter fromClusterSandboxAdmissionConfig::from_env. It shares the same bucket key, so the URL must match the API's.space-internal-apispace-internal-api-svclinks readsSPACE_REDIS_URL(the readers live only incrates/libs/api). See the README row.Limiter configuration matches deployments:
observe, pinned explicitly in the chart, because deployments pin it rather than inherit it.SPACE_API_CLIENT_CONCURRENCY_KNOWN_CLIENTS: stays empty. Observe needs no allowlist to classify traffic: an unrecognised or missingX-Client-IDgoes to the sharedunspecifiedbucket, which is metered like any other (ClientKey::classify).Code paths this exercises in every Kind run:
EVALSHAon every cold create, restore and wake, in both the API and the controller's async dispatcher;would_rejectmetering;Redis placement and health: the store stays on the infra node. Its readiness probe is now
redis-cli pinginstead of a TCP check. Tilt now startsspace-apiafterspace-redis;bootstrap.shalready waited for its rollout before the API.Rate-limiter scenario
local.rate_limiters_enforce_and_fail_openno longer writes the URL. It now:SPACE_CLUSTER_ACTIVATION_ADMISSION_BURSTtest hook, since a default burst of 60 would need 60+ VM boots to drain;The controller's copy of the activation caps stays at the chart defaults. The token-bucket script refills by each caller's own burst and rate, so a native activation during the drain would top the bucket up. None runs then: the scenario sends only ECW requests, which the API admits itself, and the suite runs one scenario at a time. The scenario's comment says so.
When a run that does not clean up is repaired, and by whom. The scenario's own teardown restores the env and scales Redis back; a restore that fails once is retried by the teardown. Beyond that:
space_smoke runreads that Lease before every scenario on the kind backend; a suite runs each scenario through it. If nothing is recorded, that one read is all it costs. If a fault is recorded, it acquires the Lease, repairs, printsREPAIRED:and runs the scenario. If a live holder has the fault up, it refuses to run (exit 2).resourceVersionand holder. A runner that resumes after a successor took over its expired Lease fails at that patch, and leaves the successor's fault and records alone.The generic Lease helper now runs a repair on every acquisition, which also closes the same gap for the worker-cordon Lease: a node a failed uncordon left cordoned is uncordoned by the next holder.
Also removed:
kind_stack_ensure_redis, whose only caller script no longer exists.This supersedes the opt-in direction of #36: Redis is now on by default. The rate-limiter scenario still needs a way to take the store away for its fail-open half, which #36's Lease-fenced store handling also addressed. Here it is the Lease-recorded scale-to-zero described above.
What stays different (README "Differences from the EKS deployment modules")
rediss://; Kind uses plaintextredis://. The workspace's Redis clients are built withtls-rustls-webpki-roots. With that feature, redis-rs builds its root store from webpki roots only and never loads native or custom roots, so a per-cluster CA would be refused without a code change. The#insecureURL fragment would be an environment-only switch. Kind also renders the URL as a literal, where the EKS module delivers it through thespace-redisSecret.SPACE_REDIS_URLon internal-api: deployments set it; Kind doesn't, because no code in that binary reads it.infra/space/aws/reference/README.mdsaid Kind sets the URL. That is true now, and the sentence names the services.Validation
Commits
main)8936b980f1d3be(#486)d06d56e, GitHub's merge of8936b98into0f1d3be, same tree as8936b980f1d3bef1e7708, the same commit rebased9254f09(#470)The rebase onto
9254f09was clean. The upstream commits in between (#470, #312, #165 and three docs commits) change no file this PR changes. They do touch other Kind and harness files (host-path checks, the template-build containerd check). Onf1e7708I reran the harness unit tests, and the PR's Kind CI ran the full typed suite on it (table below).Bench
A fresh Kind cluster on one Linux/KVM devbox, with CI's settings:
SPACE_KIND_TELEMETRY_MODE=otelandDATA_PLANE_URL=http://127.0.0.1:8090;kind-ci.yml;SPACE_KIND_RUST_ARTIFACTS=prebuilt, using the Rust artifact image that this PR's CI producer built ford06d56e. I loaded it withrust-artifact-handoff.py load, which checks the archive digest and the image ID. The bring-up ran from a checkout ofd06d56e.Before the suite, a health gate: Tilt quiet, no ReplicaSet younger than 180 s, every node Ready and published, then a 120 s settle.
Environment notes:
SPACE_KIND_DATA_ROOT). A Docker host overlay directory does not work there: kubeadm's control plane never started on it./varvolume. Nothing the scenarios exercise depends on where containerd keeps its images.Provenance:
version d06d56ee2da8.space-api,space-admin-apiandspace-cluster-controllercarriedSPACE_REDIS_URL=redis://space-redis:6379;space-internal-apicarried none. The API ran per-client concurrency inobserve.bash scripts/tests/smoke-kind-e2e.shon the fresh clusterlocal.rate_limiters_enforce_and_fail_open×3local.node_group_boundary_schedules(takes the Lease)local.node_selector_extras_render_and_enforce(takes the Lease)ecw.command_round_trip,ecw.sandbox_info(the drill (d) suite),ecw.filesystem_round_trip,ecw.pause_connectlocal.telemetry_reaches_local_backendsecw.command_round_trip,native_contract.token_scope,native.resident_paused06d56e(4 shards, full typed suite)REPAIRED:line in any shardf1e7708(GitHub's merge onto9254f09)REPAIRED:line in any sharduv run pytest space_smoke/tests -q8936b98; 544 passed onf1e7708Fault drills (disposable, against the live bench)
"Standing" below means: the public API's limiter env back to the chart's values (
SPACE_API_CLIENT_CONCURRENCY_MODE=observe, the enable flag empty, the cap variables unset), Redis at 1/1, and the Lease released with no records. Commands:space_smoke run <scenario>with CI's environment;space_smoke.suite.run_scenarios, the runner behindrun --all, over two named scenarios;setsid.kill -9 -- -<pgid>.ecw.command_round_tripandecw.sandbox_info.REPAIRED:: it took the Lease from the provably dead holder and restored Redis and the env. Then both scenarios passed. Standing afterwards.kubectlwrapper onPATHfailed the firstset envthat wroteSPACE_API_CLIENT_CONCURRENCY_MODE=observe.REPAIRED:. It recorded the true standing env (observe, notenforce) as its baseline, and passed. Standing afterwards.ecw.sandbox_infowhile a limiter run held its fault.Failures seen, and their classification
wait_public_api_rolloutand the/readyzwait return before the old pods have drained. This PR changes neither. It did not recur in the other limiter runs here or in CI. I classify it ENVIRONMENTAL (a slow bench under memory pressure from the tmpfs stores), with the race noted as a possible pre-existing harness weakness. It is not fixed here.local.telemetry_reaches_local_backends. Loki had not yet ingested the sandbox's log line. It passed on rerun 40 s later; this PR changes nothing in its path. ENVIRONMENTAL.Review follow-up
Second review: the
infra/space/clusterinstaller reuses these charts but provisions no Redis, so it would have rendered the Kind endpoint with no store behind it. Fixed: its defaults keepredis.urlempty for both charts, and an empty URL renders noSPACE_REDIS_URL, so the installer renders exactly as it did before this PR.values.example.yamlshows how to name the caller's store, andtemplates/validate.yamlrefuses the Kind endpoint and API and controller URLs that differ. Kind's own renders are unchanged.Restack onto #485: no conflicts; the commit's content is unchanged.
Earlier rounds:
resourceVersionand holder. Drill (c).space_smoke runchecks the Lease before every kind-backend scenario. It repairs under acquired ownership, including from a provably dead holder, or refuses to run. Drills (d) and (e).Rollback
Reverting removes
SPACE_REDIS_URLfrom the Kind charts, and both limiters fail open as they did before. Redis holds only short-lived limiter keys, so there is nothing to clean up. Theinfra/space/clusterinstaller is unchanged either way: its defaults keep the URL empty.This change and its validation were produced with an AI coding assistant (Claude Code) working unattended; the operator reviews before it leaves draft.
🤖 Generated with Claude Code