Repository navigation
feat(supervisor): recover shared instances with hash-based HA - #4864
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-4864.docs.buildwithfern.com/openshell |
matthewgrossman
left a comment
There was a problem hiding this comment.
Review of head 06cd2e2 from a multi-agent review panel (security, correctness, API/UX, and scope reviewers, plus a verification and triage step). I checked the one blocking finding against the code by hand. The reviewers read the code but didn't build it or run tests.
Summary: a large but coherent HA feature for #4860. The security reviewer found no new problems with the takeover grants, credential-epoch fencing, or the state preserved during adoption. One correctness issue should be fixed before merge. Everything else is polish or follow-up.
Fix before merge
- A refused pool launch leaves its assignment record and launch material behind, and another replica can later give it a takeover owner (
supervisor_pool.rs:1731).
Small fixes
- A config error message contains runs of spaces (
lib.rs:1876). - The pool/non-pool mismatch errors don't name the setting to change (
controller.rs:1037). - The HA docs don't say what an upgrade does to running sandboxes (
high-availability.mdx).
Follow-ups (fine as separate issues)
- Member freshness and report ordering compare wall-clock times written by different replicas.
- A design record (RFC) for pool ownership, takeover grants, and epoch fencing.
- Decide whether the non-pool shared-supervisor mode stays, since Helm always enables the pool.
Considered and not raised: splitting this ~24k-line PR into smaller PRs. It's one coherent, tested feature, so that's your call.
Details are in the inline comments.
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. cli — No explicit risk acceptance foundThe existing
2. rust-sdk — No explicit risk acceptance found
Review details
Advisory review; publication gates are unchanged. |
TaylorMutch
left a comment
There was a problem hiding this comment.
Reviewed head abffde246 locally. I found two correctness issues, each reproduced with a focused Rust test against this commit; details and reproduction steps are inline.
Validation: 30 supervisor Helm tests, the render-only 1/2/3-replica scaling check, 13 sandbox authentication tests, and the original 12 supervisor-control tests passed. The two additional regression assertions fail at the expected behavior checks. Temporary source changes were restored. These reproductions exercise the Rust controller and SQLite-backed pool; I did not run live Kubernetes recovery E2Es or PostgreSQL validation.
FrostGod
left a comment
There was a problem hiding this comment.
One inline comment on preserving snapshot acknowledgements across retries.
|
Label |
|
Label |
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
f59e8dc to
853684b
Compare
Summary
Add experimental high availability for the shared supervisor introduced by #4702. A hash ring places new sandbox generations, while durable ownership and gateway-signed takeover grants let a replacement supervisor adopt existing runtimes, processes, and memory without restarting workloads. Shared supervisors always use pool coordination, including at one replica.
Compute drivers are unchanged from base
9a85667d0. Coordination remains in the gateway and supervisor control layer.Related Issue
Closes #4860
Changes
[openshell.gateway.supervisor_pool]optionally tunes the recovery deadline (60 seconds by default); member identity is required, and configuration errors name the setting to fix. Per-sandbox supervision remains available.supervisor.podDisruptionBudget.enabledcan disable it for maintenance). Keep each supervisor resource in its own Helm template. Supervisor affinity defaults to{}; operators can setsupervisor.affinityindependently of gateway affinity, with a node-spreading example in the HA guide. Changing onlysupervisor.replicasscales 1 → 2 → 1 without changing gateway configuration or replacing surviving members.supervisor.autoscaling. The HPA owns replicas when enabled, with validated bounds and conservative downscale behavior; fixed-count deployments remain the default. Existing workloads stay with their live owners.Release Notes
Experimental shared supervisors always run as a pool; set
supervisor.replicasto change capacity. One replica has no spare for immediate failover. During takeover, workloads can briefly freeze and existing proxied connections or interactive sessions must reconnect. Workloads terminate if supervision is not restored within the recovery deadline.Testing
main(f07127e5d), preserving suspend/resume and required-middleware support. Updated two suspend test fixtures for the HA interfaces (853684bf6). All 3,103 affected Rust tests passed (33 skipped), full workspace all-target compile checks passed, Helm tests passed (405 gateway + 6 workspace), and normal commit checks passed. Rebuilt all three images from the rebased runtime and reran live 1 → 2 → 1: all eight tests passed (74.47 s), preserving workload identity and in-memory state. Readiness follow-up is tracked in bug(supervisor): gate rollout readiness on gateway attachment #4960.f59e8dc94): 401 gateway chart tests and 6 workspace chart tests passed, plus full Helm, docs, Markdown and normal commit checks. Semantic renders match the previous chart across four deployment configurations; disabling the PDB changes only that resource in fixed and HPA modes. Live disable/re-enable preserved Pod templates, UIDs and restart counts. Live 1 → 2 → 1 passed all eight tests (81.32 s), preserving workload identity and memory.6bee825e3): 394 gateway chart tests and 6 workspace chart tests passed, including custom-rule rendering and gateway/supervisor separation. Full Helm checks, docs validation, Markdown lint, and normal commit hooks passed. Live 1 → 2 → 1 with the empty affinity default passed all eight tests (82.75 s), preserving workload identity and memory.7cd665138): 2,686 Rust tests passed (33 skipped), strict affected-crate Clippy and normal commit hooks passed, and fullhelm:testpassed. Rebuilt all three images and reran live 1 → 2 → 1 against the current chart: all eight tests passed (77.25 s), preserving workload identity and memory. The final supervisor Deployment has one replica and no member-UID setting.helm:test, rendered-TOML loader validation, count-independent 1/2/3 renders, Helm lint/docs, affected Clippy with warnings denied, formatting, and Markdown lint passed.4a7f97f8f: 1 → 2 → 1 passed (81.75 s), plus all seven helpers. Workload Pod UID/generation/PID/start ticks/in-memory counter and gateway/surviving-supervisor identity stayed unchanged; a new sandbox worked after downscale. All exec and network probes succeeded, including the handed-off workload.Live Kubernetes validation uses one local k3d node and fixed-count scaling. The latest pool-identity scaling run used the current chart; the earlier scaling and abrupt-loss runs used the pre-autoscaling chart. The added HPA is covered by unit/render/config checks; metric-driven HPA scaling was not exercised live. Physical node loss and multi-gateway partitions are not exercised. Cross-replica wall-clock comparisons remain deferred. Aligning the ownership design record with this implementation is deferred to RFC 0018 (#4377).
Pool members share the supervisor credential, so declared member identity is within that trust boundary. Identity-bound provider placeholders without stable handles can fail closed after takeover if their provider revision changed. Service discovery adds bounded periodic probes per pool sandbox. A crash between credential creation and candidate staging can leave orphaned material, as in the existing SSH host-key path.
Checklist