Conversation
52014d2 to
14df55d
Compare
|
cc @drew |
|
/ok to test 14df55d |
|
Label |
|
/ok to test 14df55d |
|
just a heads up, there is slight design change to optimize the current gateway HA workflow. #3661 |
14df55d to
4097490
Compare
|
@FrostGod Thanks for the heads-up! I rebased on main (with #3644) and did a local test merge with #3661. They work together fine. There are a few small things for whoever lands second, and I listed them under "Merge coordination" in the description. The main one: #3661's membership worker should stop on |
4097490 to
42a0efe
Compare
|
I ran another round of adversarial code and architecture reviews with GPT-6-Astra xhigh on this iteration and addressed the findings. The PR is now ready for review. |
|
/ok to test 42a0efe |
mrunalp
added
test:e2e
|
Label |
|
/ok to test 42a0efe |
3f58e4d to
a8be556
Compare
Multi-replica gateways need capacity signals, paced supervisor handoff, and bounded shared-database contention for production scaling. On SIGTERM every gateway reports 503 "draining" on /readyz and /health. Peer-routed PostgreSQL gateways then drain their supervisor sessions before stopping their listener. Supervisors reset their reconnect backoff after an accepted session and jitter retries, and a new session receives relays only after SessionAccepted is queued. Expose per-replica session, relay, peer-RPC, mutation-lock, and watch-poller metrics with bounded labels. Replace the fleet-wide mutation guard with global/workspace/sandbox intention locks while preserving the legacy global key for rolling upgrades. Use a dedicated four-connection PostgreSQL lock pool with bounded acquisition and return. Lock timeouts fail with UNAVAILABLE, reason MUTATION_LOCK_TIMEOUT, and a one-second retry delay. Provider refresh validates the credential expiry before staging and removes staged credentials when lock acquisition fails. Reconcile startup endpoint status per sandbox and batch watched resource-version reads in groups of 1,000. Add optional autoscaling/v2 HPA support and document drain budgets against the chart's existing 30-second termination grace default. Give certgen hook pods their own labels so gateway selectors and the HPA no longer match them. Document placement, rollout, connection-pool, and HPA limitations. Add PostgreSQL concurrency and stalled-connection tests, credential cleanup coverage, Kubernetes rollout tests, and a test:rust:postgres task. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
a8be556 to
dbe29e6
Compare
|
/ok to test dbe29e6 |
|
Can someone add the |
|
Label |
|
The Kubernetes HA e2e lane passed on
The PR also makes Thanks for adding the label! |
|
Could we split this into smaller PRs, particularly separating the scoped-lock redesign, graceful draining and the observability additions? The lock changes alter concurrency guarantees across provider, policy, sandbox, and lifecycle operations, while draining changes shutdown and reconnect behavior and needs coordination with #3661. Reviewing those independently would make the invariants, validation, and rollback boundaries much clearer. Metrics can land first where they observe existing behavior; feature-specific metrics can accompany their implementation. HPA also looks suitable within the Metrics PR. |
| pods, and checks that the new pods' `openshell_server_supervisor_sessions` add | ||
| up to the Ready sandbox count. It scrapes each pod through | ||
| `kubectl get --raw /api/v1/namespaces/<ns>/pods/<pod>:9090/proxy/metrics`. | ||
| Gateway pods take up to 30 seconds to terminate because each drains its |
There was a problem hiding this comment.
[general obs] A lone PostgreSQL replica still drains even though no peer can receive its sessions, adding up to 15 seconds.
| propagation_delay_ms = supervisor_session::DRAIN_PROPAGATION_DELAY.as_millis(), | ||
| max_close_window_ms = supervisor_session::DRAIN_CLOSE_WINDOW.as_millis(), | ||
| "Draining supervisor sessions before stopping the gateway listener" | ||
| ); |
There was a problem hiding this comment.
Having peer routing configured does not guarantee another gateway is available. On a single-replica PostgreSQL StatefulSet, this adds shutdown delay without a destination for the sessions. Could we skip the paced drain when no other healthy replica is available?
|
|
||
| // Gauges | ||
| pub const SUPERVISOR_SESSIONS: &str = "openshell_server_supervisor_sessions"; | ||
| pub const DRAINING: &str = "openshell_server_draining"; |
There was a problem hiding this comment.
Do we need a separate draining metric? Short drains may finish between scrapes.
This branch has not been deployed
5 participants
Summary
Multi-replica gateways need capacity signals, bounded database contention, and a paced shutdown path. This PR adds per-replica metrics, scoped mutation locks, batched watch polling, graceful supervisor-session draining, and an optional Helm HPA.
The PR was initially drafted by Opus 5.5 xhigh and reviewed by GPT-6-Astra xhigh through independent adversarial code and architecture reviews.
Related Issue
Part of #3528: implements its capacity, scoped-lock, batched-watch, and planned-drain work. Draining moves sessions off a stopping replica; it does not balance established sessions after scale-out or decide which replica receives a reconnect. #3661 covers connect-time placement.
The issue has no acceptance or agent-workflow labels. Implementation and review were directly requested; labels are unchanged.
Changes
503 drainingon/readyzand/health;/healthzstays healthy. PostgreSQL gateways with a peer endpoint wait 3 seconds for endpoint propagation, then pace session closes over at most 12 seconds while the listener remains available. Ownership cleanup has a separate 10-second timeout. Compute-driver cleanup, trace export, and the gateway OCSF JSONL flush add time, so these budgets do not guarantee exit within the chart's existing 30-second termination grace period.SessionAcceptedbefore exposing a session to relay requests. A draining owner rejects relays promptly after its session becomes unusable.UNAVAILABLEwith reasonMUTATION_LOCK_TIMEOUT. Provider refresh validates expiry before staging credentials and cleans up staged handles when lock acquisition fails. Each replica can open 10 data connections plus 4 lock connections.autoscaling/v2HPA support with CPU/memory targets, custom metrics, and configurable scale-down behavior. With HPA enabled, omit workloadspec.replicasand validate againstmaxReplicas. Give certgen hook pods separate labels so workload selectors and HPA exclude them.Operational limits:
Provisioning, making new exec/SSH/forward requests fail withFAILED_PRECONDITION. On Kubernetes, a slower reconnect can also reportDependenciesNotReadywhile the workload continues running. Streams through an exiting gateway disconnect. Existing sandboxes retain old supervisor reconnect behavior until recreated.helm upgrade --reuse-valuesretains an older termination grace period unless explicitly overridden.Testing
Base:
mainat33a8eac1. Fresh verification for this review:cargo test -p openshell-server --features test-support: 1,949 unit tests and 31 integration tests passed; 24 tests ignored in this run.mise run test:rust:postgres: all 17 selected PostgreSQL tests passed against disposable PostgreSQL 17.10, including cancellation cleanup, mixed-version exclusion, contention, batched polling, and stalled-connection recovery.cargo test -p openshell-supervisor-process: 83 passed.mise run helm:test: 242 gateway and 6 workspace tests passed; chart ownership check passed.shellcheck -x tasks/scripts/run-postgres-tests.shandgit diff --checkpassed.mise run pre-commit: passed, including workspace/e2e/example Clippy, formatting, lockfile checks, Helm lint/docs checks, Markdown/Mermaid, protobuf, Python, TypeScript, and license checks.Previously reported integration results for the same runtime code; Kubernetes HA, Prometheus, and full CI were not rerun during this review:
sandbox_file_sync_survives_gateway_pod_rollsfailed once when immediate post-roll exec sawProvisioning, then passed five isolated reruns. The rollout test checks drain signaling, eventual Ready state, session accounting, and exec through replacement pods. Its observed exit intervals do not prove every pod avoided SIGKILL or handed off ownership within its termination grace period.promtool check metricsaccepted the new metrics and flagged two pre-existing readiness metrics for missing help text. HPA and PodMonitor were not exercised.mise run cistopped in two unchangedopenshell-binary-identitytests because/proc/<pid>/exeaccess was denied. This is not a full CI pass.The HA CI lane requires
test:e2e-kubernetes, which is not currently applied; PostgreSQL-specific tests have no CI job. Required GitHub checks still depend on maintainer validation of the fork PR.mise run pre-commitpasses.Checklist
architecture/was removed from the repository).Merge coordination
With #3661 (consistent-hash placement), whichever PR lands second:
draining_rxtospawn_membership_worker, so membership is removed when drain starts rather than after its 15-second budget. Peer ring refresh can take 10 seconds, longer than the 3-second propagation delay. After a redirected attempt fails beforeSessionAccepted, preserve the redirected flag for the fallback hello (redirected = redirected && !accepted) to avoid another redirect to the draining replica.OPENSHELL_PEER_TLS_SERVER_NAMEconfigures gateway-to-gateway calls, not the supervisor client.acceptedread before the match andReconnectBackoff::next_delay; retain feat(server): place supervisor sessions by consistent hash #3661's redirect arm and target reset, and drop its per-arm readiness reset.Store::list_by_typecall withlist_by_type_after(MEMBER_OBJECT_TYPE, None, MEMBER_LIST_LIMIT). A textual merge alone does not catch this compile failure.track_session, then useregister_awaiting_acceptandmark_accepts_relays. NewSupervisorHellotest literals already use..Default::default().#3384 may overlap in Helm values, helpers, and docs, including this PR's new top-level
autoscaling.*keys and documentation for the existingpodLifecycle.*settings.