test: tests for offloading#77
Conversation
dkharms
force-pushed
the
38-offloading-testing
branch
from
b141872 to
4c1209e
Compare
dkharms
changed the title
dkharms
force-pushed
the
38-offloading-testing
branch
from
4c1209e to
4e5dd19
Compare
dkharms
force-pushed
the
38-offloading-testing
branch
6 times, most recently
from
5c24662 to
9dd4e03
Compare
dkharms
force-pushed
the
38-offloading-testing
branch
from
9dd4e03 to
012801f
Compare
dkharms
force-pushed
the
38-offloading-testing
branch
from
012801f to
c0f110b
Compare
dkharms
force-pushed
the
38-offloading-testing
branch
from
c0f110b to
eeedacb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 69.96% 71.73% +1.77%
==========================================
Files 200 200
Lines 17986 18042 +56
==========================================
+ Hits 12584 12943 +359
+ Misses 4707 4382 -325
- Partials 695 717 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds S3 offloading test infrastructure (MinIO via Docker Compose and CI), introduces test helpers to force offloading, updates dependencies, and adds stringers for fractions. Implements S3 reader tests and an offloading integration suite, plus minor test cleanups. No runtime changes to workflows beyond metadata and MinIO setup. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/setup/env.go (1)
87-107: Avoid mutating the shared FracManagerConfig pointer across replicas.Current code sets DataDir on the shared pointer, which can cause surprising cross-replica effects. Copy before mutation.
-func (cfg *TestingEnvConfig) GetFracManagerConfig(replicaID string) fracmanager.Config { - c := cfg.FracManagerConfig - if c == nil { +func (cfg *TestingEnvConfig) GetFracManagerConfig(replicaID string) fracmanager.Config { + var conf fracmanager.Config + if cfg.FracManagerConfig == nil { // Fastest zstd compression, see: https://github.com/facebook/zstd/releases/tag/v1.3.4. const fastestZstdLevel = -5 - c = fracmanager.FillConfigWithDefault(&fracmanager.Config{ + conf = *fracmanager.FillConfigWithDefault(&fracmanager.Config{ FracSize: 256 * uint64(units.MiB), TotalSize: 1 * uint64(units.GiB), SealParams: frac.SealParams{ IDsZstdLevel: fastestZstdLevel, LIDsZstdLevel: fastestZstdLevel, TokenListZstdLevel: fastestZstdLevel, DocsPositionsZstdLevel: fastestZstdLevel, TokenTableZstdLevel: fastestZstdLevel, DocBlocksZstdLevel: fastestZstdLevel, DocBlockSize: int(units.MiB) * 4, }, }) - } - c.DataDir = filepath.Join(cfg.DataDir, replicaID) - return *c + } else { + conf = *cfg.FracManagerConfig // copy + } + conf.DataDir = filepath.Join(cfg.DataDir, replicaID) + return conf }
♻️ Duplicate comments (1)
tests/integration_tests/offloading_test.go (1)
1-1: Gate with an integration build tag.These tests require external deps (MinIO). Add a build tag to avoid running by default.
+//go:build integration +// +build integration package integration_tests
🧹 Nitpick comments (16)
fracmanager/config.go (1)
31-33: Clarify test-only flag and guard against accidental prod use.Consider documenting that OffloadingForced is for tests and ensure it’s not enabled in prod configs.
Apply this diff to annotate intent:
type Config struct { @@ - OffloadingEnabled bool - OffloadingForced bool + OffloadingEnabled bool + // OffloadingForced is intended for tests to force offloading paths. + // Must not be enabled in production. + OffloadingForced bool OffloadingRetention time.Duration }tests/docker-compose.yml (1)
3-12: Lock down ports and make MinIO creds explicitBind to localhost to avoid exposing services externally and set root credentials explicitly to prevent image-default changes from breaking tests.
Apply:
services: minio: image: quay.io/minio/minio:latest - ports: - - 9000:9000 # API - - 9001:9001 # WebUI + ports: + - "127.0.0.1:9000:9000" # API + - "127.0.0.1:9001:9001" # WebUI + environment: + MINIO_ROOT_USER: minioadmin + MINIO_ROOT_PASSWORD: minioadmin healthcheck: - test: [ "CMD-SHELL", "curl -f http://localhost:9000/minio/health/live"] + test: [ "CMD-SHELL", "curl -fsS http://localhost:9000/minio/health/live" ] interval: 5s timeout: 5s retries: 5 command: server /data --console-address ':9001'.github/workflows/ci.yml (1)
19-33: Harden MinIO step: set creds and wait for readinessSet root creds to avoid surprises and add a readiness wait to deflake tests.
Apply:
- name: Setup MinIO uses: addnab/docker-run-action@v3 with: registry: quay.io image: minio/minio:latest options: | --detach --publish 9000:9000 + -e MINIO_ROOT_USER=minioadmin + -e MINIO_ROOT_PASSWORD=minioadmin run: | minio \ server /data + + - name: Wait for MinIO readiness + run: | + for i in {1..30}; do + if curl -sf http://localhost:9000/minio/health/live > /dev/null; then exit 0; fi + sleep 2 + done + echo "MinIO not ready" >&2 + exit 1Please confirm the CI run is stable after this change.
fracmanager/proxy_frac.go (1)
199-201: Avoid%son possibly nilcur(); use Sprint
%sexpects a string and prints%!s(<nil>)for nil interfaces.fmt.Sprinthandles nils and respects Stringer.Apply:
-func (f *proxyFrac) String() string { - return fmt.Sprintf("%s", f.cur()) -} +func (f *proxyFrac) String() string { + return fmt.Sprint(f.cur()) +}storage/s3/reader.go (1)
179-183: Prefer clearer precondition message (minor)Keeping a panic is acceptable for an internal invariant; consider clarifying the message.
Apply:
-func rangeBytes(start, length int64) string { - if length <= 0 { - panic("length must be positive") - } +func rangeBytes(start, length int64) string { + if length <= 0 { + // Programming error: callers check len(p)>0 before invoking rangeBytes. + panic("s3.rangeBytes: length must be > 0") + } return fmt.Sprintf("bytes=%d-%d", start, start+length-1) }Makefile (1)
44-47: Wait for MinIO to be healthy to deflake tests
docker compose up -dreturns before health is green. Add a short wait loop keyed off the service health status.Apply:
test-deps: - @docker compose -f tests/docker-compose.yml up -d + @docker compose -f tests/docker-compose.yml up -d + @cid=$$(docker compose -f tests/docker-compose.yml ps -q minio); \ + echo "Waiting for MinIO to be healthy..."; \ + for i in $$(seq 1 30); do \ + status=$$(docker inspect -f '{{.State.Health.Status}}' $$cid 2>/dev/null || echo "starting"); \ + if [ "$$status" = "healthy" ]; then echo "MinIO is healthy"; exit 0; fi; \ + sleep 2; \ + done; \ + echo "MinIO failed to become healthy"; docker logs $$cid; exit 1storage/s3/reader_test.go (3)
33-37: Add cleanup to delete the object and bucket to avoid local MinIO clutter.Keeps the test environment tidy between runs.
Apply after bucket creation:
_, err = s3cli.cli.CreateBucket(t.Context(), &s3.CreateBucketInput{ Bucket: aws.String(bucket), }) require.NoError(t, err) + + // Best-effort cleanup + t.Cleanup(func() { + // Remove the uploaded object if present. + _, _ = s3cli.cli.DeleteObject(t.Context(), &s3.DeleteObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(path.Base(f.Name())), + }) + // Then remove the bucket. + _, _ = s3cli.cli.DeleteBucket(t.Context(), &s3.DeleteBucketInput{ + Bucket: aws.String(bucket), + }) + })
38-41: Close the temp file after upload.Avoids leaking a file descriptor in tests.
f, data, remove := generateFileWithRandomContent(t) defer remove() +defer f.Close()
46-51: Option: use io.ReadFull for deterministic full reads.Slightly reduces flake risk on partial reads from network-backed readers.
-_, err = reader.Read(readData) +_, err = io.ReadFull(reader, readData) require.NoError(t, err)fracmanager/fracmanager.go (1)
669-681: Forced offload relies on “outsiders”; verify it covers your test expectations.OffloadForcedForTests seals and then calls cleanupFractions, which offloads only fractions considered outsiders by determineOutsiders (i.e., when occupied size exceeds TotalSize). If tests intend to offload all sealed fracs regardless of capacity, this may no-op. Confirm config or consider extending the forced path to treat all sealed (non-active) fracs as offload candidates in tests.
tests/setup/env.go (1)
128-157: Optional: make S3 endpoint/creds configurable for local runs/CI.Environment overrides (e.g., S3_ENDPOINT, S3_REGION, S3_ACCESS_KEY, S3_SECRET_KEY) improve portability without hardcoding localhost/minioadmin.
tests/integration_tests/offloading_test.go (5)
40-42: Speed up CI: shrink fraction thresholds to trigger offload with fewer docs.Lower sizes so offloading happens fast and the suite is less flaky.
- FracSize: uint64(units.MiB), - TotalSize: 8 * uint64(units.MiB), + FracSize: 64 * uint64(units.KiB), + TotalSize: 1 * uint64(units.MiB),
133-140: Right-size the load for tests.1,024,000 docs is heavy for CI and not needed to validate offloading. Consider smaller batches.
- batchSize = 1024 - batches = 1000 + batchSize = 256 + batches = 64
86-101: Also assert match count for the prefix query and use idiomatic increment.Ensure there are no unexpected extra hits and use
k++.var k int for i := range allDocs { if !strings.Contains(allDocs[i], "yet-another-microservice-1") { continue } s.Assert().Equal(allDocs[i], string(found[k])) - k += 1 + k++ } + s.Require().Equal(k, len(found), "unexpected number of matches for prefix query")
72-76: Avoid brittle string-equality on full JSON blobs.If ingestion normalizes whitespace/field order, this will fail. Prefer comparing parsed fields (e.g., service and timestamp).
166-174: Confirm Testify API and fix message grammar.
- Verify
EventuallyWithTfexists in the pinned Testify; otherwise userequire.Eventually.- Minor: “fractions were not offloaded.”
- }, 30*time.Second, time.Second, "fraction were not offloaded") + }, 30*time.Second, time.Second, "fractions were not offloaded")Fallback (if needed):
s.Require().Eventually(func() bool { p := filepath.Join(env.Store(true).Config.FracManager.DataDir, "*"+consts.RemoteFractionSuffix) m, err := filepath.Glob(p) return s.Assert().NoError(err) && len(m) > 0 }, 30*time.Second, time.Second)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.github/workflows/cd.yml(1 hunks).github/workflows/ci.yml(2 hunks)Makefile(2 hunks)frac/remote.go(1 hunks)fracmanager/config.go(1 hunks)fracmanager/fracmanager.go(1 hunks)fracmanager/proxy_frac.go(2 hunks)fracmanager/sealed_frac_cache_test.go(1 hunks)go.mod(2 hunks)storage/s3/reader.go(3 hunks)storage/s3/reader_test.go(1 hunks)storeapi/store.go(1 hunks)tests/docker-compose.yml(1 hunks)tests/integration_tests/integration_test.go(1 hunks)tests/integration_tests/offloading_test.go(1 hunks)tests/setup/env.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
storage/s3/reader_test.go (2)
storage/s3/uploader.go (1)
NewUploader(24-26)storage/s3/reader.go (1)
NewReader(34-36)
storeapi/store.go (1)
fracmanager/fracmanager.go (1)
FracManager(34-60)
tests/integration_tests/offloading_test.go (5)
tests/suites/base.go (2)
Base(12-20)NewBase(22-24)tests/setup/env.go (7)
TestingEnvConfig(40-56)NewTestingEnv(159-216)WithTimeRange(574-579)WithOrder(581-585)NoFetch(525-529)WithAggQuery(551-566)TestingEnv(60-70)tests/setup/methods.go (1)
Bulk(58-61)storeapi/store.go (1)
Store(23-32)consts/consts.go (1)
RemoteFractionSuffix(64-64)
tests/setup/env.go (3)
storage/s3/client.go (1)
Client(16-19)fracmanager/config.go (2)
FillConfigWithDefault(35-81)Config(14-33)storeapi/store.go (3)
StoreConfig(34-37)NewStore(49-69)Store(23-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (15)
tests/integration_tests/integration_test.go (1)
764-765: Comment grammar tweak only — OK.No behavior change; tests remain deterministic.
fracmanager/sealed_frac_cache_test.go (1)
398-398: No-op whitespace change.Nothing to act on.
.github/workflows/cd.yml (1)
1-2: YAML schema hint is a nice DX improvement.Editor validation without runtime impact. LGTM.
fracmanager/config.go (1)
31-33: OffloadingForced scope verifiedOffloadingForced is only ever set to true in tests/integration_tests/offloading_test.go and is guarded at runtime by OffloadForcedForTests(), which itself only succeeds when OffloadingEnabled is also true. No other code paths toggle this flag.
frac/remote.go (1)
186-189: Stringer for Remote — LGTM.Consistent with other fractions; useful for logs and debugging.
storage/s3/reader.go (1)
46-47: Refactor to package-levelrangeByteslooks goodCall sites updated correctly to
rangeBytes(...). No functional regressions spotted.Also applies to: 117-118
Makefile (1)
82-83: Nice: test targets depend ontest-depsThis improves local parity with CI and reduces setup friction.
Also applies to: 90-91
go.mod (1)
8-8: Unable to verify locally – please confirm Go toolchain and buildsThe sandbox couldn’t locate
go.mod(due toskip_cloning), so please run these checks in a cloned copy to ensure compatibility:
- Confirm your CI is using Go 1.24+ and that the
godirective ingo.modmatches (e.g.awk '/^go /{print $2}' go.mod).- Run
go mod tidyandgo list -m allto ensure no dependency errors.- Verify cross-platform builds succeed:
GOOS=linux GOARCH=amd64 go build ./...GOOS=darwin GOARCH=arm64 go build ./...storage/s3/reader_test.go (1)
16-21: LGTM: range formatting is well covered.Checks the usual ranges and zero-length panic. Nice.
storeapi/store.go (1)
116-118: Test helper OffloadAll mirrors existing helpers—good addition.Matches SealAll/ResetCache patterns and keeps test API consistent.
tests/setup/env.go (5)
55-56: S3 client on TestingEnvConfig: good wiring point for tests.Keeps S3 plumbing explicit and reusable across hot/cold store creation.
183-194: On-demand S3 client creation looks good.Creates a dedicated bucket per env; simple and effective for isolation.
258-261: MakeStores signature change is consistent; all call sites updated.Pass-through of s3cli keeps store construction explicit.
467-471: Stores.OffloadAll helper aligns with SealAll/ResetCache.Good API symmetry for tests.
492-496: TestingEnv.OffloadAll helper is consistent and convenient.Small but valuable addition for test readability.
4 participants
Final PR that adds suite for offloading testing.
Definitely it's far from perfect but I have some ideas for future follow-ups.
Summary by CodeRabbit