Add physical snapshot object publication#1133
Conversation
|
Current head: Evidence:
Validation:
Please review the immutable object commit protocol, manifest trust boundary, conditional S3 semantics, download filesystem safety, and offline restore flow. @codex review |
|
Warning Review limit reached
Next review available in: 1 second Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthrough物理スナップショットをマニフェストとペイロードに分離し、S3互換オブジェクトストアへ検証付きで公開・取得・復元する機能を追加した。CLIにはpublish、download、restoreモード、SSE検証、クラスター整合性確認、復元処理を実装した。 Changes物理スナップショットオフロード
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Publisher
participant ObjectStore
participant Downloader
participant Restore
CLI->>Publisher: publishモードでスナップショットを公開
Publisher->>ObjectStore: ペイロードとマニフェストを条件付き保存
CLI->>Downloader: downloadモードでマニフェストを指定
Downloader->>ObjectStore: マニフェストとペイロードを取得
Downloader-->>CLI: 検証済みファイルを返却
CLI->>Restore: restoreモードで取得済みペイロードを処理
Restore-->>CLI: 復元用DataDirとFSMPathを返却
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the elastickv-physical-snapshot command-line utility and its supporting snapshotoffload package to enable publishing, downloading, and restoring Raft/FSM snapshots using S3-compatible object storage. Feedback on the changes highlights a critical issue in the membership validation logic that incorrectly rejects valid joint consensus states where a node ID appears in both voters and outgoing voters. Additionally, the reviewer recommended using deferred Close() calls in download.go and publisher.go to prevent resource leaks during panics and to ensure that errors during cleanup are properly logged or propagated.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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)
docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md (1)
143-154: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winファイル名とヘッダーを
partialに揃えてください
M0/M1 は完了済みで、M2/M3 は未完了です。docs/design/README.mdの定義ではこの段階は*_partial_*.mdに該当するため、2026_07_19_partial_physical_snapshot_object_offload.mdへgit mvし、Status: Partialに更新してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md` around lines 143 - 154, Rename the design document with git mv to 2026_07_19_partial_physical_snapshot_object_offload.md and update its header status to Partial, reflecting completed M0/M1 and pending M2/M3. Update every reference, including docs/design/README.md references if applicable, and verify no references to the old proposed filename remain.Source: Coding guidelines
🧹 Nitpick comments (1)
cmd/elastickv-physical-snapshot/operations.go (1)
97-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win必須フィールド検証ロジックが3箇所で重複し、mapのイテレーション順序が非決定的
parsePublishFlags、validateDownloadConfig、parseRestoreFlagsで同じ「map[string]stringを作ってrequiredを呼ぶ」パターンが重複しています。Goの map イテレーション順序は非決定的なため、複数の必須フラグが同時に欠落した場合、実行ごとに異なるエラーメッセージが返る可能性があります。順序付きの
[]struct{name, value string}を使う共通ヘルパーに抽出することで、重複を解消し、エラーメッセージを決定的にできます。♻️ 修正案(例)
+func requireAll(fields ...[2]string) error { + for _, f := range fields { + if err := required(f[0], strings.TrimSpace(f[1])); err != nil { + return err + } + } + return nil +}呼び出し側:
- for name, value := range map[string]string{ - "data-dir": cfg.dataDir, - "cluster-id": cfg.clusterID, - "prefix": cfg.prefix, - } { - if err := required(name, strings.TrimSpace(value)); err != nil { - return nil, err - } - } + if err := requireAll( + [2]string{"data-dir", cfg.dataDir}, + [2]string{"cluster-id", cfg.clusterID}, + [2]string{"prefix", cfg.prefix}, + ); err != nil { + return nil, err + }Also applies to: 170-178, 273-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/elastickv-physical-snapshot/operations.go` around lines 97 - 105, Extract the duplicated required-field validation from parsePublishFlags, validateDownloadConfig, and parseRestoreFlags into a shared helper accepting an ordered []struct{name, value string}. Preserve each caller’s existing field order and required validation behavior so the first missing field and resulting error are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/elastickv-physical-snapshot/main.go`:
- Around line 63-68: Update the required function to create its validation error
with the file’s existing github.com/cockroachdb/errors package instead of
fmt.Errorf, preserving the current message and required-value behavior.
---
Outside diff comments:
In `@docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md`:
- Around line 143-154: Rename the design document with git mv to
2026_07_19_partial_physical_snapshot_object_offload.md and update its header
status to Partial, reflecting completed M0/M1 and pending M2/M3. Update every
reference, including docs/design/README.md references if applicable, and verify
no references to the old proposed filename remain.
---
Nitpick comments:
In `@cmd/elastickv-physical-snapshot/operations.go`:
- Around line 97-105: Extract the duplicated required-field validation from
parsePublishFlags, validateDownloadConfig, and parseRestoreFlags into a shared
helper accepting an ordered []struct{name, value string}. Preserve each caller’s
existing field order and required validation behavior so the first missing field
and resulting error are deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31fd3fc0-015b-4441-8d13-770c2e174df2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/elastickv-physical-snapshot/config.gocmd/elastickv-physical-snapshot/main.gocmd/elastickv-physical-snapshot/main_test.gocmd/elastickv-physical-snapshot/operations.godocs/design/2026_07_19_proposed_physical_snapshot_object_offload.mdgo.modinternal/snapshotoffload/download.gointernal/snapshotoffload/manifest.gointernal/snapshotoffload/manifest_test.gointernal/snapshotoffload/publisher.gointernal/snapshotoffload/publisher_test.gointernal/snapshotoffload/s3_store.gointernal/snapshotoffload/s3_store_test.gointernal/snapshotoffload/store.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14056d6226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Current head: Addressed all nine root findings from the prior head:
Additional design status work:
Semantic caller audit:
Validation:
The full @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce379ceff5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return errors.Wrapf(ErrRemoteIntegrity, "key=%s kind=%q expected=%q", key, metadataKind, kind) | ||
| case encryption != "" && info.Encryption != encryption: | ||
| return errors.Wrapf(ErrRemoteIntegrity, "key=%s encryption=%q expected=%q", key, info.Encryption, encryption) | ||
| case encryption == EncryptionKMS && strings.TrimSpace(info.KMSKeyID) != strings.TrimSpace(kmsKeyID): |
There was a problem hiding this comment.
When operators pass a KMS alias or bare key id to --kms-key-id (values S3 accepts for SSE-KMS uploads), S3 can report the canonical key identifier/ARN on HEAD/GET; this exact string comparison then rejects the object after a successful publish, and later downloads fail with ErrRemoteIntegrity, even though the requested key was used. Either require a canonical ARN/key ID at validation time or resolve aliases before comparing.
Useful? React with 👍 / 👎.
| s3DefaultMultipartPart = int64(64 * 1024 * 1024) | ||
| s3MaxMultipartPart = int64(5 * 1024 * 1024 * 1024) | ||
| s3MaxMultipartParts = int64(10_000) | ||
| s3MaxObjectBytes = int64(5 * 1024 * 1024 * 1024 * 1024) |
There was a problem hiding this comment.
For physical snapshots above 5 TiB, multipartPartSize returns ErrObjectInvalid before trying multipart upload, but the current S3 multipart limits allow substantially larger objects with up to 10,000 parts (AWS docs). Since the code already scales part size up to the 5 GiB part limit, this hard cap unnecessarily blocks valid large snapshot uploads; raise the object limit to match S3 or make it backend-configurable.
Useful? React with 👍 / 👎.
| } | ||
| if prior, ok := seen[id]; ok { | ||
| jointVoter := prior == "voters" && names[i] == "voters_outgoing" | ||
| if !jointVoter && prior != "learners_next" && names[i] != "learners_next" { |
There was a problem hiding this comment.
When a manifest has learners_next containing an incoming voter/learner, this condition exempts the duplicate solely because one side is learners_next, so voters:[1], learners_next:[1] passes DecodeManifest as a validated snapshot witness. In etcd joint consensus, LearnersNext is only for outgoing voters being demoted, so accepting these impossible ConfStates makes corrupted or hand-written manifests look valid instead of failing closed.
Useful? React with 👍 / 👎.
| if _, ok := learners[peers[i].ID]; ok { | ||
| peers[i].Suffrage = etcd.SuffrageLearner | ||
| } else { | ||
| peers[i].Suffrage = etcd.SuffrageVoter | ||
| } |
There was a problem hiding this comment.
If --learners names every peer, this loop marks the whole restore membership as learners and returns success, so runRestore proceeds to fetch the snapshot from S3 and only later fails inside PreparePhysicalSnapshotRestore with “at least one voter is required”. For large physical snapshots this turns a locally detectable flag error into an unnecessary full download; count voters here and fail before contacting the object store.
Useful? React with 👍 / 👎.
1 participant
Summary
EKVSSTI1payload through the SST ingest receiver contractSafety
0600, fsync, and an atomic no-replace link in the destination directoryDependencies
Validation
go test ./internal/snapshotoffload ./cmd/elastickv-physical-snapshot -count=1 -timeout=20mgo test -race ./internal/snapshotoffload ./cmd/elastickv-physical-snapshot -count=1 -timeout=20mgolangci-lint run ./... --timeout=10m --allow-parallel-runnersgit diff --checkAuthor: bootjp
Summary by CodeRabbit
新機能
ドキュメント
テスト