Repository navigation
Conversation
26c5f0d to
71b9c52
Compare
johntmyers
added
test:e2e
|
Label |
|
Label |
|
/ok to test 71b9c52 |
PR Review StatusThe independent review of scoped mutation locks, bounded waits, and provider cleanup found no blocking findings. Current-head Branch Checks, Helm Lint, Trivy Changes, and E2E workflows are queued or running; Gator will monitor their results. Blocking findings: None. Gator metadata
|
johntmyers
added
gator:watch-pipeline
71b9c52 to
51822ba
Compare
|
/ok to test 51822ba |
PR Review StatusThe independent follow-up review found no blocking findings in the author’s changes across the rebase, including settings row identity checks, provider-name validation, SSH identity lock-pool separation, and lifecycle lock adaptations. The stale test mirror has been refreshed, and current-head Branch Checks, Helm Lint, Trivy Changes, and E2E workflows are running; Gator will monitor their results. Blocking findings: None. Gator metadata
|
johntmyers
added
gator:watch-pipeline
| `OPENSHELL_REPLAY_TEST_DATABASE_URL` so legacy tests use that same database. | ||
| Never point it at a database that a running gateway uses: the tests take | ||
| fleet-wide advisory locks. | ||
| CI does not run these tests; the Kubernetes HA e2e suite covers PostgreSQL end |
There was a problem hiding this comment.
Can we run the focused PostgreSQL lock tests in CI? They check cancellation, connection reuse, and compatibility with old replicas
There was a problem hiding this comment.
Done. Branch Checks has a new "Rust PostgreSQL tests" job that runs the same runner script on a Linux runner, so the 15 postgres_* tests (cancellation, connection reuse, old-replica interop and the rest) run on every PR. The burst test is out of that suite now, see the other thread.
| ); | ||
| // Waits must stay far from the lock timeout, where requests fail. | ||
| assert!( | ||
| p99 * 5 < MUTATION_LOCK_TIMEOUT, |
There was a problem hiding this comment.
Can we move this latency assertion into a dedicated performance benchmark? In our earlier run, all 1,000 operations succeeded, but p99 was 5.35 seconds while other tests were running. The isolated rerun passed, so this threshold depends on machine load.
There was a problem hiding this comment.
Agreed, that one is a capacity check, so it shouldn't gate anything. It's now bench_postgres_lock_pool_absorbs_a_12ms_reconnect_burst: test:rust:postgres and CI skip it, and mise run test:rust:postgres:bench runs it on its own with the p99 check. The repo has no benchmark harness and the test uses crate-private APIs, so this seemed like the simplest place for it.
|
|
||
| let mut sandbox_settings = | ||
| load_sandbox_settings(state.store.as_ref(), &workspace, sandbox.object_name()).await?; | ||
| ensure_sandbox_keeps_name(state, &sandbox).await?; |
There was a problem hiding this comment.
Can we track the first settings write as a follow-up? A peer can delete the sandbox after this ownership check, and the first insert can still create settings inherited by a replacement using the same name. The row-ID check protects existing rows, but not the first insert.
There was a problem hiding this comment.
Right, the row-ID check only covers rows that already exist. Main has the same race (lifecycle deletes don't take a database lock), so it's in #4307 with the other follow-ups.
| /// Local S(global) S(workspace) X(sandbox), for provisioning-deadline | ||
| /// reconciliation, which re-derives configuration from provider and | ||
| /// profile records and must not interleave with their local writers. | ||
| pub(super) async fn lock_sandbox_local_in_workspace( |
There was a problem hiding this comment.
Please link a follow-up issue for bounding this wait and testing it. This path can hold the sandbox lock while waiting for a workspace lock, delaying start, stop, and delete for that sandbox.
There was a problem hiding this comment.
Tracked in #4307, including a test with a sandbox key that sorts before its workspace key.
The openshell-server tests that need a real PostgreSQL server are ignored by default and had no shared way to run. The mutation-replay test reads its own OPENSHELL_REPLAY_TEST_DATABASE_URL, so every contributor had to provision a database by hand, and the advisory-lock tests that follow in this series need the same setup. Add mise run test:rust:postgres. It runs every ignored postgres_* test in openshell-server against OPENSHELL_TEST_POSTGRES_URL, or starts a disposable PostgreSQL container with Docker or Podman (CONTAINER_ENGINE selects one) on the image pinned by the Kubernetes e2e fixture and removes it on exit. Tests run one at a time because advisory locks are database-wide. The runner always points the legacy replay variable at the selected database, so an inherited URL cannot send that test to a different server. test:postgres-runner checks that selection with a fake cargo and needs no database or container engine. CI does not run the PostgreSQL tests; TESTING.md documents the task. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
DeleteProvider checked that no sandbox referenced the provider and then deleted the record without holding the sandbox mutation guard. Sandbox create and provider attach take that guard while they write provider references, so one of them could add a reference after the attached sandbox check passed, and the delete then removed a provider that a sandbox spec still named. Take the guard before the attached sandbox check, as provider create and update already do, so the check and the delete run against a stable set of sandbox references, and reject an empty name after authorization but before the guard. A new test holds the guard, attaches the provider while the delete waits, and asserts that the delete fails with FailedPrecondition and leaves the provider in place. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
A provider credential refresh stages the minted values under new credential handles before it commits the provider update, and deletes those handles when validation or persistence fails. Two earlier returns skipped that cleanup. The minted expiry was converted to a protobuf timestamp only after staging, so an expiry outside the timestamp range failed the refresh and left the staged values in the credential driver. A failure to acquire the sandbox mutation guard returned the same way. Convert the expiry before staging anything, so a bad value fails before any handle exists, and delete the staged handles when the guard cannot be acquired. A new test refreshes a stored credential with an expiry of i64::MAX and asserts that the call fails, the provider keeps its original handles, and the credential driver holds the same number of values as before. The guard failure path has no test here, because the SQLite guard used by unit tests cannot fail. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
|
@johntmyers I think the CI failures are unrelated can you please /ok to test again? |
|
/ok to test c00b632 |
johntmyers
added
gator:watch-pipeline
johntmyers
added
gator:approval-needed
Signed-off-by: divesh <dgude@nvidia.com>
|
/ok to test 7663cc2 |
PR Review StatusThanks @EmilienM. I checked your request to refresh testing and posted Current-head Branch Checks, Helm Lint, Trivy Changes, and E2E are queued or running. Gator will inspect their results next cycle. Blocking findings: None. Gator metadata
|
johntmyers
added
gator:watch-pipeline
|
Can we bound how long guarded work can run, and test a stalled holder? On timeout, the operation must be unable to write further before its lock is released. |
|
what is the rationale behind 4 locks, and 10 data connectors, |
Signed-off-by: divesh <dgude@nvidia.com>
| //! 2. Process-local keys follow in ascending `i64` order. | ||
| //! 3. On `PostgreSQL`, the same keys follow as session-level advisory locks in | ||
| //! ascending order, all on one lock-pool connection. | ||
| //! 4. A task never acquires a mutation guard or a local lifecycle lock while |
There was a problem hiding this comment.
Rules 4 and 5 carry the whole deadlock-freedom argument but are prose only. Since the scheme is correct for disjoint keys, a future mutation_guard() call inside a guarded section could pass the full suite and only deadlock in production. A task-local depth counter with a debug_assert! in mutation_guard() would make that a test failure instead.
There was a problem hiding this comment.
Agreed, prose isn't enough here. A task_local depth counter doesn't fit tokio well: it needs a scope at every spawn, and guards move into the delete and failed-create workers, so the drop would hit the wrong counter. I added a debug-only check instead, keyed by tokio's task id (thread id for block_on roots like #[tokio::test] bodies). Each guard remembers the owner it registered under, and local keys, blocking lifecycle gates and the SSH identity lock now panic before waiting when they'd break rules 1, 4 or 5. Release builds compile it out. It found no real nesting, but startup endpoint-status reconciliation runs four guarded futures concurrently in one task, so that now goes through an explicit lock_order::branch(), and so do the tests (your SSH create test included) that hold a guard in the test body while driving the guarded path.
| /// open a lock connection, or Postgres `lock_timeout` (SQLSTATE 55P03). A lock connection that | ||
| /// Postgres does not open with at least `LOCK_CONNECTION_MIN_BUDGET` left is not counted. RPC | ||
| /// callers return the timeout as `Status::unavailable`. | ||
| pub fn record_lock_timeout(scope: LockScope) { |
There was a problem hiding this comment.
The docs say "alert on any timeout," but a lock connection Postgres doesn't open within LOCK_CONNECTION_MIN_BUDGET returns Database and intentionally skips this counter. So when the database is overloaded — exactly when you want the alert — real mutation failures bypass it. Worth either a separate counter or a note in the docs.
There was a problem hiding this comment.
Agreed. The catalog mentions it, but the HA guide says to alert on any timeout and leaves these failures with nothing to alert on, and readiness stays green because it pings the data pool. I added openshell_server_mutation_lock_errors_total{scope} for any guard acquisition that fails without timing out (a lock connection that never opened, or a failed lock statement, which also skipped the counter), with a row in the HA signals table, a PromQL example and a debug skill note. I kept it separate from the timeout counter because the response differs: contention for one, PostgreSQL capacity or connectivity for the other.
| /// (`(2 × replicas + surge) × 14`). Each guard holds | ||
| /// one lock connection, so a replica sustains about 4 / c guarded operations | ||
| /// per second, where c is how long one guard is held. | ||
| pub(super) const MUTATION_LOCK_POOL_MAX_CONNECTIONS: u32 = 4; |
There was a problem hiding this comment.
This comment states the ceiling (~4/c guarded ops/sec), and c can be seconds since guarded sections call the compute driver and profile sources. But nothing exports in-use lock connections, so operators can't see they're at 3 of 4 until requests fail. A pool gauge would make this a leading indicator.
There was a problem hiding this comment.
Makes sense. openshell_server_mutation_lock_connections_in_use is a GaugeSlot held by each checked-out lock connection, so it moves with the in-use count the pool already tracks and includes acquisitions waiting on a PostgreSQL lock and connections still being returned. openshell_server_mutation_lock_connections_capacity sits next to it so the ratio is one query. Both are PostgreSQL only, and capacity reads the configured pool size (server.dbLockMaxConnections, also in this PR).
| let mut set = MutationLockSet::default(); | ||
| match *self { | ||
| Self::Global => set.insert(MutationLockKey::Global, LockMode::Exclusive), | ||
| Self::Workspace("") => { |
There was a problem hiding this comment.
Workspace("") silently means a fleet-wide exclusive lock. Not reachable by accident today — provider paths all go through resolve_workspace(...).name — but note the asymmetry: Sandbox maps an empty workspace to default a few lines down, while here it escalates to global. An explicit MutationScope::Platform variant would make the escalation unreachable rather than just unreached.
There was a problem hiding this comment.
It's reached on purpose today: the platform profile writers (import, update, delete) get an empty name from authorize_and_resolve_profile_workspace and validate sandboxes in every workspace, so they need X(global), and mapping it to default like Sandbox would under-lock them. I made the escalation explicit with MutationScope::profiles(&workspace), which picks Global for the platform scope. Global already covers platform profiles with the same lock set and label, so a Platform variant would just be an alias. Workspace("") now trips a debug_assert, and release keeps X(global) as a fail-safe that over-locks rather than under-locks.
| } | ||
| } | ||
|
|
||
| fn lock_for(&self, key: i64) -> Arc<RwLock<()>> { |
There was a problem hiding this comment.
retain sweeps the whole table on every individual key acquisition while holding a blocking mutex — three sweeps per sandbox guard. Small in practice since the table is bounded by live keys, but it could be amortized rather than run on every call.
There was a problem hiding this comment.
Agreed, easy one. lock_for now only sweeps when a new key grows the table past max(64, 2x the live entries left by the last sweep), so it's amortized O(1) and dead entries from cancelled waiters stay bounded at about twice the peak live set. local_registry_drops_released_entries relied on the per-call sweep, so I replaced it with a churn test for that bound. The lifecycle gate registry on main does the same per-call sweep (one key per call); I left it alone to keep this PR scoped.
The mutation lock ordering rules were prose only, so a nested guard on a different key passed every test and could only deadlock under contention. Debug builds now register each held guard with the Tokio task (or the thread, outside a task) that acquired it, and panic before waiting when a local key, a blocking lifecycle gate or the SSH identity lock would break rules 1, 4 or 5. Startup endpoint-status reconciliation and the tests that hold a guard while driving guarded code run under lock_order::branch(), which marks deliberate concurrency within one task. Release builds compile the check out. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
When a replacement supervisor session failed its endpoint-status reset, the spawned cleanup task kept the session lifetime guard through retry_endpoint_status_after_supervisor_disconnect. That retry only ends once the reset is durable, so a gateway shutdown during a database or lock outage waited the full 10 s and reported ownership cleanup as incomplete. The guard now covers only the lifecycle demotion, as in finish_supervisor_session, and a SQLite test checks that shutdown completes while the retry is still blocked. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Platform-scope provider profile writers (import, update, delete) pass an
empty workspace on purpose and need X(global), because they validate
sandboxes in every workspace. MutationScope::profiles() now names that
escalation and picks Global for the platform scope, instead of relying
on MutationScope::Workspace("") to mean it. An empty workspace name now
trips a debug assertion, and release builds keep X(global) as a
fail-safe that over-locks rather than under-locks.
Part of NVIDIA#3528
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
LocalMutationLocks swept its whole table on every key lookup, so a burst of N concurrent acquisitions on one replica cost O(N^2) under a blocking mutex. The table is now swept only when a new key grows it past the larger of 64 and twice the live entries left by the last sweep, which makes acquisition amortized O(1). Released entries, including those of cancelled waiters, stay bounded at about twice the peak live set. The lifecycle gate registry keeps its per-call sweep. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Both stores hardcoded their pool ceiling: 10 connections for Postgres, 5 for on-disk SQLite. One pool is shared by every database-backed RPC, so that number is also the gateway's ceiling on concurrent database work. Once every connection is checked out, callers queue on acquire and sqlx logs "time to acquire exceeded slow threshold"; sandbox creates then time out and are retried, which adds load rather than shedding it. The right ceiling is deployment-specific — it depends on how many gateway replicas share the database and what max_connections the server itself allows — so it cannot be a single number baked into the binary. Expose it on the same config surface as the rest of the gateway's settings: --db-max-connections, OPENSHELL_DB_MAX_CONNECTIONS, and the TOML key database_max_connections, resolved in that precedence order and rendered by the Helm chart from server.dbMaxConnections. Omitting it keeps each backend's previous value, so existing deployments do not move. Values below 1 are rejected rather than silently replaced by the default: a zero pool would block every acquire, and quietly ignoring a typo would reproduce the ceiling the operator is trying to lift. An in-memory SQLite database stays pinned to one connection, since the database lives inside that connection. Refs: NVIDIA#2561 Signed-off-by: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> (cherry picked from commit 33517b3) Signed-off-by: Emilien Macchi <emacchi@redhat.com>
The mutation lock pool was fixed at 4 connections per replica. That number rests on one reconnect-burst bench rather than scale testing, so a gateway that serves many more sandboxes needs a way to raise it without a rebuild. Add --db-lock-max-connections, OPENSHELL_DB_LOCK_MAX_CONNECTIONS, the database_lock_max_connections gateway.toml key and the server.dbLockMaxConnections Helm value next to the data pool knob, resolved in the same order. The default stays 4, zero is rejected, SQLite ignores it with a warning, and the gateway logs both pool sizes at startup. PostgreSQL now needs at least 2 data connections, because the SSH identity lock keeps one while it queries the pool. The connection sizing docs, the HA guide formula and the debug skill now count data plus lock connections. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
A mutation lock acquisition that failed without timing out, such as a
lock connection PostgreSQL never opened or a failed lock statement,
only logged a warning, and readiness stays healthy because it pings the
data pool. Count those failures in
openshell_server_mutation_lock_errors_total{scope}, kept apart from
timeouts because they point at PostgreSQL capacity or connectivity
rather than contention. Export
openshell_server_mutation_lock_connections_in_use as a gauge slot held
by each checked-out lock connection, next to
openshell_server_mutation_lock_connections_capacity, which reads the
configured lock pool size; both exist only with PostgreSQL. The metrics
catalog, the HA guide's signals and PromQL examples, and the debug
skill cover the new series.
Part of NVIDIA#3528
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Nothing bounds how long a mutation guard is held, so a holder stuck in
a compute driver, credential backend or middleware call keeps its scope
locked while waiters time out. Each guard now records its hold in
openshell_server_mutation_lock_hold_seconds{scope} when it is released,
and logs a warning when the hold outlasted the lock wait timeout, so
operators can find the replica and scope behind those timeouts. A
PostgreSQL test with a stalled holder pins the current contract:
waiters on its sandbox, workspace and global scopes fail by their
deadline, other scopes proceed, the holder keeps its key, and a full
lock pool on one replica leaves the other working. The metrics catalog,
the HA guide and the debug skill describe the new signals, which
observe holds without bounding them.
Part of NVIDIA#3528
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
The test yes, an enforced bound not in this PR. There's now a PostgreSQL test with a stalled holder: waiters on its scope from another replica fail with MUTATION_LOCK_TIMEOUT by their deadline, other sandboxes and workspaces go through, the holder keeps its lock, and a replica whose lock pool fills up with stalled holders fails its next guarded operations by their deadline while the other replica keeps working. There's also openshell_server_mutation_lock_hold_seconds{scope}, plus a warning when a guard is released after being held past the 10 s wait timeout, so the replica and scope behind those timeouts are easy to find (neither shows a hold that is still in progress). I didn't add a hold deadline because a client-side one can't give the guarantee you're asking for. Timing out the work and releasing is what a cancelled RPC already does (on main too), and sqlx still flushes a statement that's sent or buffered, so it can commit after the unlock. Spawned work and driver or Vault side effects outlive the dropped future as well. Getting "no writes after release" means running guarded writes in a transaction on the lock session with idle_in_transaction_session_timeout as the bound, so PostgreSQL rolls back the writes and drops the locks together. That only works once driver and middleware calls are out of the guard, which #4307 already tracks, so I added it there next to that item.
Not much science behind either. The 10 isn't new: it's SQLx's default pool size, set explicitly when PostgreSQL support landed (5a15de6). The 4 is a judgment call. Each guard holds one lock connection, so a replica sustains about n/c guarded ops per second, and the reconnect-burst bench (1000 sessions landing on one replica over 12 s, about 170 ops/s at about 20 ms each) keeps about 3.3 busy, so 4 leaves headroom (p99 wait 39 ms locally) while staying well under the data pool. Since we haven't scale-tested either number, both are now configurable in this PR. I carried #2828 for the data pool (--db-max-connections, database_max_connections, server.dbMaxConnections) with bjw123's authorship and added a matching lock pool knob (--db-lock-max-connections, database_lock_max_connections, server.dbLockMaxConnections). Defaults stay 10 and 4. One new floor: on PostgreSQL the data pool can't go below 2, because the SSH identity lock keeps one data connection while it queries the pool, so the gateway rejects 1 at startup instead of hanging. Every extra lock slot costs 2 x replicas + surge connections during a rollout, so the HA guide's sizing formula now counts data plus lock connections, and the new in-use gauge shows when raising it is worth it. The rationale is in the MUTATION_LOCK_POOL_MAX_CONNECTIONS comment. On your merges: I checked the resolution against #4321 and nothing is lost. Gating the demotion on was_current only skips cases where it's a no-op or where stop/delete already owns the state, and the other abandon_session_setup call sites match main. One follow-up: the spawned task kept session_lifetime through the endpoint-status retry, which only ends once the reset is durable, so a shutdown during a DB or lock outage would wait the full 10 s and report cleanup incomplete. I scoped the lifetime to the demotion, like finish_supervisor_session, with a test. Your SSH create test needed one change for the new lock-order check: its holder loop takes a gate while still holding the previous guard, so the holders now go through lock_order::branch(). |
This branch has not been deployed
4 participants
Summary
Scope gateway mutation locks by workspace and sandbox and bound every lock wait, so a burst of supervisor reconnects (rollout, scale-down, redirects) no longer queues behind one fleet-wide lock.
Related Issue
Part of #3528. Follows #3978, now merged. Carries #2828 by @bjw123. Follow-ups are tracked in #4307.
Changes
sync_lockand the single fleet-wide advisory key with global, workspace and sandbox lock scopes (shared/exclusive) held on a dedicated PostgreSQL lock pool. Replicas still on the old release keep taking the global key, so a mixed-version rollout stays serialized. Debug builds check the lock ordering rules before every wait.UNAVAILABLEwith reasonMUTATION_LOCK_TIMEOUTand retry info. A lock connection PostgreSQL never opens returnsINTERNAL, so it doesn't read as contention.NOT_FOUNDorABORTEDinstead of writing into the newer sandbox's settings.DeleteProviderholds the mutation guard while it deletes, and staged provider credentials are released when a refresh fails early. On kind, the delete race left a dangling provider reference that crashlooped every gateway at startup.server.dbMaxConnections) and a matchingserver.dbLockMaxConnections, with flag, env andgateway.tomlforms. Defaults stay 10 and 4 (PostgreSQL needs at least 2 data connections), so each pod opens up to 14 PostgreSQL connections instead of 10; raisemax_connectionsbefore upgrading.mise run test:rust:postgresand a Branch Checks job, and document pool sizing. The reconnect-burst capacity check runs separately withmise run test:rust:postgres:bench, outside CI.Testing
mise run pre-commitandmise run docs:build:strictpass,mise run rust:lint, the Helm tests and the openshell-server tests pass, and every commit builds with its tests.mise run test:rust:postgres: 18 tests covering scope exclusion, interop with the old global key, cancellation, pool bounds and gauges, connection failures and a stalled holder.mise run test:rust:postgres:benchpasses on an idle machine.Checklist