Repository navigation
Conversation
StreamPartSize grows the 64 MiB part size until the expected stream fits in 9,000 parts, so a >1.5 TB archive no longer hits the 640 GiB ceiling. NewS3Client exposes the package's client so other packages reuse it.
Publishes a stopped datadir in the snapshots.ethpandaops.io layout: tar | zstd streamed into a multipart upload, hashed every 1 GiB, verified by size and random ranges (through a public base or S3), then the head block, metadata and, with --write-latest, latest last. A failed tar or zstd aborts the upload.
![redpandabot[bot]](https://avatars.githubusercontent.com/in/3749543?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The PR adds benchmarkoor snapshot export, streaming a stopped client datadir through tar | zstd into an S3 multipart upload, hashing 1 GiB blocks, verifying random ranges, then writing the head-block/metadata/latest objects. The layout, packing, hashing and abort-on-truncation logic are well structured and tested. Two issues remain: Ctrl-C does not actually abort the multipart upload, and a negative --verify-blocks panics.
Issues
- 🟡
pkg/upload/stream.go:57— Ctrl-C leaves the multipart upload un-aborted — see the thread on that line - 🟢
pkg/snapshot/verify.go:91— Negative --verify-blocks panics — see the thread on that line
Reviewed @ ad96ab38
"If debugging is the process of removing software bugs, then programming must be the process of putting them in." — Edsger Dijkstra
| u.Concurrency = concurrency | ||
| }) | ||
|
|
||
| _, err := u.Upload(ctx, &s3.PutObjectInput{ //nolint:staticcheck // SA1019: successor is pre-v1 |
There was a problem hiding this comment.
🟡 Ctrl-C leaves the multipart upload un-aborted
UploadStream passes the CLI's cancellable context straight into manager.Uploader.Upload. On SIGINT/SIGTERM cmd/benchmarkoor/snapshot.go cancels that same context, and the SDK's failure path aborts via AbortMultipartUpload(u.ctx, ...) with the already-canceled context, so the abort request never reaches the store and the incomplete multipart upload is orphaned — the opposite of the documented "Ctrl-C does the same". Abort with a detached context (e.g. context.WithoutCancel, or the UploadID from the returned error) so cleanup survives the interrupt.
There was a problem hiding this comment.
Fixed in 26000da (with a test that fails without the fix).
There was a problem hiding this comment.
This was my finding, and 26000da addresses it. UploadStream now re-aborts on a context that outlives the canceled one (pkg/upload/stream.go:65-80):
if err != nil {
var mu manager.MultiUploadFailure
if errors.As(err, &mu) && mu.UploadID() != "" {
abortCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), time.Minute)
defer cancel()
if _, aerr := client.AbortMultipartUpload(abortCtx, &s3.AbortMultipartUploadInput{...}); aerr != nil {
err = errors.Join(err, fmt.Errorf("aborting upload %s: %w", mu.UploadID(), aerr))
}
}
...
}I checked the pinned SDK (feature/s3/manager v1.22.5): its fail() aborts with u.ctx, i.e. the canceled context, and Upload returns an error implementing manager.MultiUploadFailure that carries the upload ID, so errors.As + mu.UploadID() is the correct recovery. The signal context from cmd/benchmarkoor/snapshot.go:106 does reach pkg/snapshot/export.go:120, so this is exactly the path that was broken — the finding was right and is now fixed.
The new TestUploadStream_CanceledAbortsUpload (pkg/upload/stream_test.go:38) cancels on the first part and asserts the abort DELETE for up-1. I couldn't execute it here (no Go toolchain in the checkout), but the control flow is direct and it targets the right request.
One small nit, not a blocker: the SDK's own fail() already attempts the abort first, so on a non-cancel failure where that abort succeeds, this second call can return NoSuchUpload and get joined into the returned error. Cleanup is unaffected; only the error text changes.
@ 26000daa
| return fmt.Errorf("object is %d bytes, uploaded %d", got, size) | ||
| } | ||
|
|
||
| for _, i := range rand.Perm(len(sums))[:min(n, len(sums))] { |
There was a problem hiding this comment.
🟢 Negative --verify-blocks panics
rand.Perm(len(sums))[:min(n, len(sums))] with a negative n (e.g. --verify-blocks -1) indexes the slice with a negative high bound and panics; VerifyBlocks is never validated in ExportOptions.Validate or the CLI. Clamp n to >= 0 or reject the flag value.
There was a problem hiding this comment.
Fixed in 26000da (with a test that fails without the fix).
There was a problem hiding this comment.
Addressed in 26000da. ExportOptions.Validate now rejects the value (pkg/snapshot/export.go:54-56):
if o.VerifyBlocks < 0 {
return fmt.Errorf("verify blocks must be >= 0, got %d", o.VerifyBlocks)
}Export calls it first thing (pkg/snapshot/export.go:73), before it reaches Verify at line 138, and the CLI path goes through Export (cmd/benchmarkoor/snapshot.go:109), so --verify-blocks -1 is now rejected rather than panicking. TestExportOptionsValidate (pkg/snapshot/layout_test.go:47-55) pins the negative case. Confirmed addressed.
@ 26000daa
…-verify-blocks The SDK aborts on the upload's own context, which a Ctrl-C has already canceled, so the abort never reached the store. Abort again on a context that outlives it. A negative --verify-blocks panicked in Verify.
There was a problem hiding this comment.
Adds benchmarkoor snapshot export: it walks a stopped datadir, streams a deterministic tar|zstd straight into a multipart S3 upload (part size derived from the datadir), hashes 1 GiB blocks, verifies size plus random ranges through S3 or a public URL, then writes the head-block/metadata and optional latest. The follow-up commit validates a negative --verify-blocks and retries a Ctrl-C-canceled multipart abort on a detached context; both previous findings are fixed and I found nothing new.
Reviewed 17 changed file(s) @ 26000daa — no blocking issues found.
"If debugging is the process of removing software bugs, then programming must be the process of putting them in." — Edsger Dijkstra
snapshots.ethpandaops.io publishes geth (mainnet and jochemnet alike) as the contents of <datadir>/geth, and consumers extract into <datadir>/geth. A geth/-prefixed archive extracted that way nests a second geth/, and extracted flat geth opens chaindata/ but misses triedb/merkle.journal and rewinds its head (found by the e2e prototype on the jochemnet geth snapshot).
jochemnet's nethermind tarball has mainnet/ at its root and every shadowfork inventory extracts it into <datadir>/nethermind_db; the CI hosts hold the same tree under <dir>/nethermind_db/mainnet, so packing the datadir would have nested a second nethermind_db/ (found by the e2e prototype).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A 1.2 TB export logged one line at its start and nothing for the next hours (found by the e2e prototype: geth's export, ~120 MB/s compressed, ~2.5 h). Also gofmt pack.go.
The published geth image carried .snapshot_fetcher_started and download_snapshot.sh from the source tarball (found by the e2e prototype).
There was a problem hiding this comment.
Summary
This PR adds benchmarkoor snapshot export, a streamed tar|zstd-to-multipart-S3 publisher with random-range verification, plus the upload/verification helpers and tests behind it. The two previously-flagged issues (Ctrl-C leaving the multipart upload open, and a negative --verify-blocks panicking) are now fixed, but one open finding remains: the docs example still passes the geth instance dir as --datadir. I also found one new silent-failure edge case when the archive root is a symlink.
Issues
- 🟡
pkg/snapshot/pack.go:121— a symlinked archive root yields a silently empty snapshot — see the thread on that line - 🟡
docs/snapshot-export.md:18— example passes the geth instance dir as --datadir — see the thread on that line - 🟢
pkg/upload/stream.go:69— re-abort on a path the SDK already aborted — The aws-sdk-go-v2 manager already calls AbortMultipartUpload before returning aMultiUploadFailurewhen the upload context is not canceled, so on every non-Ctrl-C failure (e.g. a tar read error) this second abort getsNoSuchUploadand joins it into the error. Harmless to the outcome, but it makes real failures noisier.
Reviewed @ c6540790
"If debugging is the process of removing software bugs, then programming must be the process of putting them in." — Edsger Dijkstra
|
|
||
| m := &Manifest{Dir: root} | ||
|
|
||
| err = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { |
There was a problem hiding this comment.
🟡 a symlinked archive root yields a silently empty snapshot
os.Stat(root) follows a symlink and accepts a symlinked directory, but filepath.WalkDir uses os.Lstat on the root: a symlinked root is a non-directory DirEntry, so nothing is descended and m.Members ends up as just .. tar --no-recursion then emits only the ./ entry (I reproduced this: piping a lone . member produces an archive containing only ./), so an empty archive is uploaded, it passes size/range verification against itself, and latest is written. Resolve the root with filepath.EvalSymlinks (or walk with os.ReadDir) before building the manifest. Only triggers when --datadir (or <datadir>/geth, <datadir>/nethermind_db) is a symlink to the real directory.
ethrex keeps its chain-id-1 db at <datadir>/chain-1 (sst, blob, metadata.json at its root). jochemnet's archive is that directory's contents and msf-2 extracts it into /data/ethrex/chain-1; packing the datadir would have nested a second chain-1/ (the same class as geth's and nethermind's roots, found on the 2026-10-11 build).
1 participant
Adds
benchmarkoor snapshot export, which publishes a stopped client datadir in the snapshots.ethpandaops.io layout so that ethereum-package'snetwork_sync_base_urland thesnapshot_fetcherrole can read it unchanged. It replaces thepublish.sh/verify.sh/blockhash.py/metadata.shscripts the shadowfork image builds have used so far.Credentials come from
S3_ENDPOINT_URL,AWS_ACCESS_KEY_IDandAWS_SECRET_ACCESS_KEY. Full docs are indocs/snapshot-export.md.1. Streamed, never a local archive
The command walks the datadir, drops the excludes and pipes the sorted member list through
tar --null --no-recursion -T - | zstd -<level> -T0straight into a multipart upload. Members are intar --sort=nameorder, so the same datadir always gives the same archive.upload.UploadStream/upload.StreamPartSizeinpkg/upload, built on the existing S3 client and manager rather than a second one.2. Hashed and verified
--verify-public-base, the ranges are read through the public URL as consumers read them, and each must be a206._snapshot_eth_getBlockByNumber.json(published byte-for-byte, so Amsterdam header fields survive; itsresult.numbermust equal--block) and_snapshot_metadata.jsonare written only after verification passes.latestis written only with--write-latest, and last.{"data_size_bytes": …}with--metadata-filemerged over it, nested objects key by key, soshadowfork.amsterdam_timeanddocker_imagepass through.3. Per-client packing
nodekey,LOCK,nodes/,logs/,_snapshot_*<datadir>/geth, no prefix (snapshots.ethpandaops.io's layout; consumers extract into<datadir>/geth)snapshot.tar.zst./discovery-secret,known-peers.jsonsnapshot-v2.tar.zst./snapshot-pruned.tar.zst./keysnapshot.tar.zst./node.key,node_config.jsonsnapshot.tar.zst<datadir>/nethermind_db, no prefix (mainnet/at the root, as jochemnet's tarball; consumers extract into<datadir>/nethermind_db)*/peers,*/discoveryNodessnapshot.tar.zst--archive-nameoverrides them, e.g.snapshot.tar.zstfor reth to match snapshots.ethpandaops.io.LOCK(e.g.geth/chaindata/LOCK) is kept.The image gains
tarandzstd.Verified
make test-core):Range(refused on its 200);make test-integration-core, added to CI). It usescgr.dev/chainguard/minio, becauseminio/miniois no longer on Docker Hub. It checks that:-3);latestis absent unless requested;block 6);latestand no open multipart upload.--write-latest; verification through an anonymous-read bucket via--verify-public-base; and fail-fast on a wrong--block, missing credentials and a public base that 403s.ethpandaops-shadowfork-images.