test(e2e): End-to-end tests for the DB hot-row contention fixes - #720
Conversation
|
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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
1ff7f54 to
0381db9
Compare
0381db9 to
f863639
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in Go e2e test suite under src/tests/e2e/ intended to validate (against a live Harbor instance) the recent database hot-row contention mitigation stack: concurrent push burst behavior, quota enforcement correctness, usage convergence for unlimited projects, and blob Touch debouncing (plus an optional async-refresh convergence check).
Changes:
- Introduces a new
-tags e2etest suite that drives real registry/Harbor API interactions and asserts key outcomes for the contention fixes. - Implements a small, dependency-free OCI push client (blob upload + manifest PUT) used by the e2e tests.
- Documents how to run/configure the suite against the default slot-0 dev environment.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/tests/e2e/README.md | Documents running and configuring the new opt-in e2e test suite. |
| src/tests/e2e/contention_test.go | Adds end-to-end regression tests covering burst pushes, quota enforcement, usage convergence, blob Touch debounce, and async refresh convergence. |
| src/tests/e2e/client_test.go | Adds a minimal HTTP-based Harbor/OCI client used by the e2e tests (project creation, blob/manifest push, summary reads). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for time.Now().Before(deadline) { | ||
| used, err = e.projectUsedStorage(project) | ||
| if err == nil && used > 0 { | ||
| break | ||
| } | ||
| time.Sleep(3 * time.Second) | ||
| } | ||
| if err != nil { | ||
| t.Fatalf("summary: %v", err) | ||
| } | ||
| // used must cover at least the layer bytes (config+manifest add a bit) | ||
| var minExpected int64 | ||
| for _, l := range layers { | ||
| minExpected += int64(len(l.data)) | ||
| } | ||
| if used < minExpected { | ||
| t.Fatalf("usage did not converge: used=%d < expected>=%d", used, minExpected) | ||
| } |
There was a problem hiding this comment.
Fixed in bfffa01 - the loop now polls until used >= minExpected (the summed layer bytes), so a partially refreshed intermediate value cannot end the wait early.
| t.Fatalf("open db: %v", err) | ||
| } | ||
| defer db.Close() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
Fixed in bfffa01 - the outer 10s context now only guards the initial ping; each row read gets its own 10s per-query context.
|
|
||
| func randomBlob(size int) blob { | ||
| data := make([]byte, size) | ||
| _, _ = rand.Read(data) |
There was a problem hiding this comment.
crypto/rand.Read is documented (since Go 1.24) to never return an error - it always fills the buffer and crashes the program on entropy failure - so there is no error path to handle here.
| coreURL: get("E2E_CORE_URL", "http://localhost:8080"), | ||
| user: get("E2E_ADMIN_USER", "admin"), | ||
| password: get("E2E_ADMIN_PASSWORD", "Harbor12345"), | ||
| dbDSN: get("E2E_DB_DSN", "postgres://postgres:root123@localhost:5432/registry?sslmode=disable"), | ||
| http: &http.Client{Timeout: 60 * time.Second}, |
There was a problem hiding this comment.
These are Harbor's canonical dev-environment defaults, not real credentials: admin/Harbor12345 is hardcoded throughout the upstream apitests and every make/compose dev setup, and the DSN matches the throwaway dev database. Requiring the env vars would only add friction for the single environment this suite targets; anything non-default is already configurable via the documented env vars.
| E2E_CORE_URL http://localhost:8080 | ||
| E2E_ADMIN_USER admin | ||
| E2E_ADMIN_PASSWORD Harbor12345 | ||
| E2E_DB_DSN postgres://postgres:root123@localhost:5432/registry?sslmode=disable | ||
| E2E_ASYNC_REFRESH (unset) |
There was a problem hiding this comment.
Same as the client thread: these are the well-known Harbor dev defaults documented upstream, listed here precisely so a reader knows what the suite assumes and which env vars override them. Nothing secret is disclosed.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
f863639 to
bfffa01
Compare
goheader expects `http://www.apache.org/licenses/LICENSE-2.0` in the file header. #711 introduced `src/migration/authoritative.go` with the `https://` form, so `Go Lint` now fails on every PR targeting `main` (e.g. #716, #720). One-character fix. Signed-off-by: Prasanth Baskar <prasanth@8gears.com> Co-authored-by: Prasanth Baskar <prasanth@8gears.com>
|
Preview images for this PR are available in
Verify a preview image: Verify SBOM attestation: |
bfffa01 to
34e6ca2
Compare
34e6ca2 to
78add7a
Compare
78add7a to
41c6722
Compare
41c6722 to
66b3872
Compare
bupd
changed the title
66b3872 to
db029f9
Compare
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tests/e2e/client_test.go">
<violation number="1" location="src/tests/e2e/client_test.go:41">
P3: When E2E_CORE_URL is set with a trailing slash (http://host:8080/), concatenating it with the leading-slash paths builds double-slash URLs that Harbor's router rejects with 404, failing the whole suite. Normalize coreURL by trimming a trailing '/' in newEnv so configuration with or without the slash behaves identically.</violation>
</file>
<file name="src/tests/e2e/contention_test.go">
<violation number="1" location="src/tests/e2e/contention_test.go:216">
P2: When `QUOTA_ASYNC_REFRESH_DURATION` exceeds 10 seconds, this test can fail before three configured intervals elapse. Derive the deadline from the configured duration or enforce and document a maximum.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| // assume interval <= 10s; allow 3 intervals + slack | ||
| deadline := time.Now().Add(35 * time.Second) |
There was a problem hiding this comment.
P2: When QUOTA_ASYNC_REFRESH_DURATION exceeds 10 seconds, this test can fail before three configured intervals elapse. Derive the deadline from the configured duration or enforce and document a maximum.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tests/e2e/contention_test.go, line 216:
<comment>When `QUOTA_ASYNC_REFRESH_DURATION` exceeds 10 seconds, this test can fail before three configured intervals elapse. Derive the deadline from the configured duration or enforce and document a maximum.</comment>
<file context>
@@ -0,0 +1,226 @@
+ }
+
+ // assume interval <= 10s; allow 3 intervals + slack
+ deadline := time.Now().Add(35 * time.Second)
+ for time.Now().Before(deadline) {
+ used, err := e.projectUsedStorage(project)
</file context>
| http *http.Client | ||
| } | ||
|
|
||
| func newEnv() *env { |
There was a problem hiding this comment.
P3: When E2E_CORE_URL is set with a trailing slash (http://host:8080/), concatenating it with the leading-slash paths builds double-slash URLs that Harbor's router rejects with 404, failing the whole suite. Normalize coreURL by trimming a trailing '/' in newEnv so configuration with or without the slash behaves identically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tests/e2e/client_test.go, line 41:
<comment>When E2E_CORE_URL is set with a trailing slash (http://host:8080/), concatenating it with the leading-slash paths builds double-slash URLs that Harbor's router rejects with 404, failing the whole suite. Normalize coreURL by trimming a trailing '/' in newEnv so configuration with or without the slash behaves identically.</comment>
<file context>
@@ -0,0 +1,242 @@
+ http *http.Client
+}
+
+func newEnv() *env {
+ get := func(k, d string) string {
+ if v := os.Getenv(k); v != "" {
</file context>
Adds an opt-in e2e suite (go test -tags e2e ./tests/e2e/) that runs against a live Harbor - by default the slot-0 dev environment - and verifies the four fixes from this branch stack end to end: - TestConcurrentSharedLayerPushBurst: the incident shape - concurrent pushes into one unlimited project sharing base layers - completes with zero failures - TestQuotaEnforcementDeniesOverLimit: projects with a real storage limit still deny over-limit pushes synchronously (guards the unlimited-skip change) - TestUsageConvergesOnUnlimitedProject: with the reservation skipped, the usage figure still converges via the refresh path - TestTouchDebounce: repeated HEAD probes of a fresh blob leave its row untouched, verified directly against the blob table (version and update_time unchanged) - TestAsyncRefreshConvergence: opt-in (E2E_ASYNC_REFRESH=1) tighter convergence bound when core runs with QUOTA_ASYNC_REFRESH_DURATION The suite uses a dependency-free minimal OCI client (monolithic blob upload + manifest PUT) and mirrors real client behavior by HEAD-probing layers before upload. Signed-off-by: Vadim Bauer <vb@container-registry.com>
db029f9 to
383a263
Compare
What this PR does
Adds an opt-in end-to-end suite (
go test -tags e2e ./tests/e2e/) that runs against a live Harbor — by default the slot-0 dev environment — and verifies the four fixes in this stack behave correctly together.TestConcurrentSharedLayerPushBurstTestQuotaEnforcementDeniesOverLimitTestUsageConvergesOnUnlimitedProjectTestTouchDebounceblobtable (versionandupdate_timeunchanged)TestAsyncRefreshConvergenceE2E_ASYNC_REFRESH=1) tighter convergence bound withQUOTA_ASYNC_REFRESH_DURATIONsetWhy it earns its place
This suite caught a real design bug before it shipped. The first version of the unlimited-reservation skip assumed the usage figure would keep flowing through
RefreshMiddleware. It does not:RefreshForProjectMiddlewareis mounted only on artifact/repository DELETE routes (server/registry/route.go), never on manifest PUT — so on the push path the reservation is the only writer ofquota_usage. Skipping it alone would have silently frozen every unlimited project's storage figure, including the exporter metric.TestUsageConvergesOnUnlimitedProjectfailed withused=0, which is why that PR now ships the deferred coalesced refresh with it.Results against slot-0 running this stack
Database-level confirmation after a full push burst plus a HEAD storm: 0 of 963 blob rows had
version > 0.Notes
e2e, so it never runs in the normal unit-test sweep.E2E_CORE_URL,E2E_ADMIN_USER,E2E_ADMIN_PASSWORD,E2E_DB_DSN(slot-0 defaults).