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>
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, |
Added the test (waiters time out by their deadline, other scopes and replicas keep working) and a hold-time histogram with a warning past 10 s. No enforced bound: a client-side deadline can't stop writes after release. The real fix, guarded writes in a transaction on the lock session, is tracked in #4307.
10 is SQLx's default. 4 is a judgment call from the reconnect-burst bench (about 3.3 busy). Neither is scale-tested, so both are configurable now (#2828 carried plus a lock pool knob), same defaults. On your merges: nothing from #4321 was lost, and I fixed the endpoint-status retry holding shutdown. |
9a94bc2 to
f44c126
Compare
|
LGTM! |
|
@pimlock can you take a glance at this |
f44c126 to
cfce8e0
Compare
|
/ok to test cfce8e0 |
|
@johntmyers can I have a review, please? |
Signed-off-by: divesh <dgude@nvidia.com>
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>
Signed-off-by: divesh <dgude@nvidia.com>
Signed-off-by: divesh <dgude@nvidia.com>
Signed-off-by: divesh <dgude@nvidia.com>
cfce8e0 to
f05203f
Compare
|
@EmilienM please make sure everything is addressed that has feedback comments and resolve them if so then we can look to merge |
This branch has not been deployed
5 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