fix(storage): fail fast on connection-pool exhaustion, not just lock contention#345
Open
matthyx wants to merge 2 commits into
Open
fix(storage): fail fast on connection-pool exhaustion, not just lock contention#345matthyx wants to merge 2 commits into
matthyx wants to merge 2 commits into
Conversation
matthyx
added
the
ai-assisted
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the storage layer’s “fail fast on internal contention” behavior from per-key lock acquisition to SQLite connection-pool acquisition, aiming to return a consistent apierrors.ServerTimeout + Retry-After signal under pool exhaustion (instead of stalling until upstream timeouts). It also adds regression and diagnostic tests to cover both lock contention and pool contention scenarios.
Changes:
- Shrinks connection-pool acquisition timeout to 5s (mirroring
lockTimeout) and generalizes the timeout error helper (newLockTimeoutError→newContentionTimeoutError). - Updates storage operations to return
ServerTimeouton contention and adds a deterministic regression test for pool exhaustion. - Adds a gated load/stress benchmark and a deterministic lock fail-fast regression test for ContainerProfile contention patterns.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| pkg/registry/file/storage.go | Introduces poolTimeout and routes pool/lock contention to a generalized ServerTimeout constructor. |
| pkg/registry/file/storage_test.go | Renames the timeout-error unit test and adds a pool-contention regression test. |
| pkg/registry/file/containerprofile_load_test.go | Adds a gated mixed-load benchmark plus a deterministic lock fail-fast regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // retry logic honors. It is used for both lock acquisition (s.locks.Lock/RLock) and | ||
| // connection-pool acquisition (s.pool.Take) timeouts: both are internal contention points | ||
| // that should fail fast with the same backoff-friendly signal rather than hang to the outer | ||
| // request deadline. op is "create"/"get"/"update"/"delete"/"list". |
Comment on lines
311
to
+315
| poolCtx, cancel := poolContext() | ||
| defer cancel() | ||
| conn, err := s.pool.Take(poolCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("take connection: %w", err) | ||
| return newContentionTimeoutError("create", key, err) |
Comment on lines
390
to
+394
| poolCtx, cancel := poolContext() | ||
| defer cancel() | ||
| conn, err := s.pool.Take(poolCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("take connection: %w", err) | ||
| return newContentionTimeoutError("delete", key, err) |
Comment on lines
468
to
+472
| poolCtx, cancel := poolContext() | ||
| defer cancel() | ||
| conn, err := s.pool.Take(poolCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("take connection: %w", err) | ||
| return newContentionTimeoutError("get", key, err) |
Comment on lines
713
to
+717
| poolCtx, cancel := poolContext() | ||
| defer cancel() | ||
| conn, err := s.pool.Take(poolCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("take connection: %w", err) | ||
| return newContentionTimeoutError("list", key, err) |
Comment on lines
898
to
+902
| poolCtx, cancel := poolContext() | ||
| defer cancel() | ||
| conn, err := s.pool.Take(poolCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("take connection: %w", err) | ||
| return newContentionTimeoutError("update", key, err) |
Comment on lines
1078
to
+1082
| poolCtx, cancel := poolContext() | ||
| defer cancel() | ||
| conn, err := s.pool.Take(poolCtx) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("take connection: %w", err) | ||
| return 0, newContentionTimeoutError("count", key, err) |
Comment on lines
+443
to
+445
| // Shrink the backstop so the fail-fast path resolves quickly; this exercises | ||
| // the real child-context timeout -> newLockTimeoutError code path, just with | ||
| // a smaller bound (same technique as TestStorageImpl_LockContentionReturnsServerTimeout). |
| // Each REST-facing call gets a request context whose deadline models the | ||
| // apiserver's request timeout. Pre-fix, a contended lock blocks up to this | ||
| // deadline; post-fix the 5s lockTimeout fails fast well under it. We keep it | ||
| // generous (30s) so we measure the *actual* wait, not an artificial cap. |
Comment on lines
+285
to
+291
| seedCtx, seedCancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| for _, tpl := range templates { | ||
| p := tpl.profile.DeepCopy() | ||
| key := "/spdx.softwarecomposition.kubescape.io/containerprofile/" + p.Namespace + "/" + p.Name | ||
| _ = s.Create(seedCtx, key, p, nil, 0) | ||
| } | ||
| seedCancel() |
|
Summary:
|
matthyx
force-pushed
the
fix/consolidator-connection-scoping
branch
from
8983b7f to
9546fdf
Compare
…lock backstop Adds TestContainerProfileLockFailFast: 20 concurrent GETs contend for a key whose write lock is held elsewhere; asserts every request resolves within a 2s bound (well above the shrunk 500ms lockTimeout used for determinism, far below the 10s request deadline) as apierrors.IsServerTimeout, not a plain timeout. Deterministic (~0.5s, no flakiness) and runs in the normal suite -- this is the closest thing to an AC4 latency-bound regression test that's practical to assert on reliably in CI. Also adds TestContainerProfileLoad, a mixed-load diagnostic (background consolidation + concurrent REST create/get/update contending for a small connection pool) gated behind LOAD_TEST=1 and skipped by default. Before/after measurement against the pre-fix baseline showed the fail-fast test's signal cleanly (all 20 gets go from hanging to the ~10s deadline pre-fix to resolving in ~1s post-fix); the mixed-load numbers were directionally consistent (higher read throughput, shorter consolidation passes) but too noisy under its deliberately adversarial pool/concurrency ratio to assert on, hence it stays a manual diagnostic rather than a CI gate. Two items surfaced during this measurement remain open, not addressed here: - The mixed-load harness's writes are time-series profiles, which early- return in PreSave before reaching CollapseSettings(), so it does not confirm PR2's in-lock CollapseConfig Get removal is a genuine hot path under load -- that AC4 sub-claim is still unverified. - The SQLite connection pool has no WAL mode or busy_timeout configured (NewPool sets neither Flags nor PrepareConn), which under concurrent writes to different keys can surface "database is locked" errors independent of anything these commits touch (they operate above SQLite). Worth its own follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
…contention Live Helm e2e validation of the prior 3 commits showed relevantCVEs now passing cleanly, but network_policy_known_servers still failing with a 504 after a ~34.5s stall and a generic apiserver Reason:Timeout -- not the ServerTimeout this package's own lock-contention fix produces. That test runs node-agent with a 10s updatePeriod (vs 5m for the CVE test) and polls the same CRDs every 5-10s, sustaining far more consolidator/REST contention than the CVE test ever exercises. The timing and error shape point at s.pool.Take(poolCtx), which still used the original 60s poolTimeout untouched by the prior commits: long enough that a pool-exhaustion stall can blow past the k8s apiserver's own outer non-long-running-request deadline (~34s) before this package's error ever fires, surfacing as an opaque client-side 504 instead of our fail-fast signal. Bound s.pool.Take the same way lock acquisition is already bounded: shrink poolTimeout from a 1-minute const to a 5s var (mirroring lockTimeout), and return the same ServerTimeout+Retry-After:1 error instead of an opaque "take connection" error on expiry. Generalized the error constructor (renamed newLockTimeoutError -> newContentionTimeoutError) since it now covers both lock and connection-pool contention -- both are internal contention points that should fail fast with the same backoff-friendly signal rather than hang to the outer request deadline. Adds TestStorageImpl_PoolContentionReturnsServerTimeout, the connection-pool analogue of the existing lock-contention regression test: exhausts a size-1 pool, asserts a subsequent Get fails fast as ServerTimeout+Retry-After rather than hanging to the old ~60s bound. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/pool-contention-fail-fast
branch
from
97014cb to
9c515a2
Compare
|
Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TestContainerProfileLockFailFast, a deterministic regression test for fix(storage): fail fast on lock contention instead of hanging to the request timeout #342: 20 concurrent GETs contend for a key whose write lock is held elsewhere; asserts every request resolves within a 2s bound asapierrors.IsServerTimeout, not a plain timeout. Also addsTestContainerProfileLoad, a mixed-load diagnostic (background consolidation + concurrent REST create/get/update) gated behindLOAD_TEST=1and skipped by default — too timing-sensitive under its deliberately adversarial pool/concurrency ratio to assert on reliably in CI.relevantCVEspassing cleanly, butnetwork_policy_known_serversstill failing with a 504 after a ~34.5s stall and a generic apiserverReason:Timeout— not theServerTimeoutfix(storage): fail fast on lock contention instead of hanging to the request timeout #342's lock-contention fix produces. That test runs node-agent with a 10supdatePeriod(vs 5m for the CVE test) and polls the same CRDs every 5-10s, sustaining far more consolidator/REST contention than the CVE test ever exercises.s.pool.Take(poolCtx), which still used the original 60spoolTimeoutuntouched by the prior PRs in this stack: long enough that a pool-exhaustion stall can blow past the k8s apiserver's own outer non-long-running-request deadline (~34s) before this package's error ever fires, surfacing as an opaque client-side 504 instead of our fail-fast signal.s.pool.Takethe same way lock acquisition is bounded in fix(storage): fail fast on lock contention instead of hanging to the request timeout #342: shrinkspoolTimeoutfrom a 1-minute const to a 5s var (mirroringlockTimeout), and returns the sameServerTimeout+Retry-After:1error instead of an opaque "take connection" error on expiry. Generalizes the error constructor (renamednewLockTimeoutError→newContentionTimeoutError) since it now covers both lock and connection-pool contention.TestStorageImpl_PoolContentionReturnsServerTimeout, the connection-pool analogue of the existing lock-contention regression test.Context
Stacked on #344 (consolidator connection scoping). Final PR in the stack for the ContainerProfile 504 regression; see #342 for the shared root-cause context.
Test plan
go build ./...,go vet,gofmtcleango test -race ./pkg/registry/file/...passesnetwork_policy_known_serversnow passes cleanly (2 consecutive clean runs, ~5min each, no retries) — was failing with 504s before this PRrelevantCVEsunaffected, still passingrelevant_data_is_appended— a stale CVE fixture, identical failure present in the pre-fix baseline run too, unrelated to locking/pooling)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com