gsadmin/rustfs
1
0
Fork 0
mirror of https://github.com/rustfs/rustfs.git synced 2026-08-05 21:07:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

chore(docs): refresh agent docs, guard doc paths, archive plans (#4203)

Browse Source 
Agent-instruction and architecture docs had drifted from the code:

- CLAUDE.md: slim to commands + pointers; fix wrong claim that
  `make pre-commit` is the full pre-PR gate (that is `make pre-pr`);
  drop stale pre-#3929 file paths and merged bug narratives
- AGENTS.md: drop dead `rust-refactor-helper` skill rule and the
  hand-maintained (already stale) scoped-AGENTS index; link
  architecture docs from Sources of Truth
- .github/AGENTS.md: replace the outdated copied CI command matrix
  with a pointer to ci.yml
- crates/AGENTS.md: merge duplicated Testing sections
- ARCHITECTURE.md: resolve the utils->config contradiction (edges are
  removed), mark volatile counts as snapshots, fix a bad path
- docs/architecture: add README router; move one-shot plans/trackers
  (rebalance-decommission phases, migration-progress ledger, PR
  template) to docs/superpowers/plans with archive headers; fix stale
  source paths in kept inventories (core/sets.rs, core/pools.rs,
  store/mod.rs, startup_* split from #3671)
- docs/operations/tier-ilm-debugging.md: extracted tier debugging
  playbook with corrected paths
- scripts/check_doc_paths.sh: new guard failing pre-commit/pre-pr when
  instruction/architecture docs reference nonexistent file paths
- .claude/skills: add tier-debug and arch-checks repo skills;
  .gitignore now keeps .claude/skills and docs/operations committable

Verification: ./scripts/check_doc_paths.sh,
./scripts/check_architecture_migration_rules.sh (both pass)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Zhengchao An
2026-07-02 23:30:13 +08:00
committed by GitHub
parent 29899f4731
commit d1db9a10cd
27 changed files with 454 additions and 241 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/superpowers/plans/migration-progress.md
+10016
View File
File diff suppressed because it is too large Load Diff
docs/superpowers/plans/rebalance-decommission-followup-review-plan.md
+1795
View File
File diff suppressed because it is too large Load Diff
docs/superpowers/plans/rebalance-decommission-implementation-plan-index.md
+81
View File
@@ -0,0 +1,81 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
# Rebalance and Decommission Implementation Plan Index
> This index is based on `docs/architecture/rebalance-decommission-remediation-plan.md`. The remediation scope is intentionally split into smaller implementation plans because the fixes touch independent risk areas: object-version safety, distributed operation semantics, data movement internals, and operational hardening.
## Why Split the Work
The remediation backlog has fourteen fix blocks. Implementing them in one PR would make review risky and would mix unrelated failure modes. The safer path is:
1. Fix the data semantics that can lose or corrupt object-version meaning.
2. Fix distributed start/stop/recovery semantics so operators can trust cluster state.
3. Fix resource and metadata correctness in shared data movement.
4. Add observability and compatibility hardening.
Each plan below should be reviewed and executed independently unless the plan explicitly says two fixes must share the same implementation.
## Plan Set
| Plan | Fixes | Status | Purpose |
| --- | --- | --- | --- |
| `rebalance-decommission-phase1-safety-plan.md` | F01, F02, F03, F04, F05 | Drafted | Protect object-version semantics and cluster operation safety |
| `rebalance-decommission-phase2-data-movement-plan.md` | F06, F07, F08, F09, F10 | Drafted | Stream multipart migration, preserve metadata, and improve convergence |
| `rebalance-decommission-phase3-hardening-plan.md` | F11, F12, F13, F14 | Drafted | Improve cleanup reporting, auditability, metadata decoding, and threshold docs |
| `rebalance-decommission-followup-review-plan.md` | R01-R16 | Reviewed | Close post-implementation review gaps found after F01-F14 |
## Execution Recommendation
Start with `rebalance-decommission-phase1-safety-plan.md`. Phase 1 contains the highest-risk issues and defines semantics that later fixes depend on.
Within Phase 1:
1. F01 should be analyzed first because rebalance delete marker and remote tiered behavior determines whether RustFS should move or skip these versions.
2. F02 can be implemented independently once the expected decommission overwrite semantics are confirmed.
3. F03, F04, and F05 can be designed together but should remain separate PRs unless a shared peer-failure helper is introduced.
For a long-running implementation task, use the phase plans as checkpoints:
1. Implement one fix block at a time.
2. Run the focused tests listed in that block.
3. Review the diff before moving to the next fix block.
4. Run the phase-level test matrix before considering the phase complete.
After the initial F01-F14 implementation pass, continue with `rebalance-decommission-followup-review-plan.md`. That follow-up plan is ordered by remaining risk and should be executed before treating the remediation as complete.
## Upstream Change Impact Notes
### 2026-06-17: `ed55857b refactor: move bucket operations contract (#3507)`
This upstream change moved `BucketOperations` from `crates/ecstore/src/store_api/traits.rs` into `crates/storage-api/src/bucket.rs` and re-exported it from `rustfs_storage_api`.
Impact on this remediation plan:
- No F01-F14 risk item is fixed by this upstream change.
- No planned fix block needs priority changes because of this upstream change.
- Implementation code that imports or bounds `BucketOperations` must now use `rustfs_storage_api::BucketOperations`.
- Generic implementations that need the ECStore error type should use an explicit associated error bound such as `BucketOperations<Error = crate::error::Error>`.
- The only touched planning-relevant files are import/boundary changes in paths such as `crates/ecstore/src/pools.rs`, `crates/ecstore/src/store.rs`, `crates/ecstore/src/set_disk.rs`, and `rustfs/src/admin/handlers/rebalance.rs`; the rebalance/decommission safety logic remains unchanged.
## Review Gates Before Implementation
Before any code patch starts for a fix block:
- Confirm the selected behavior in the corresponding plan.
- Identify the minimal set of files for that fix.
- Write or update failing tests first.
- Keep unrelated refactors out of scope.
- Run focused tests for the touched crate before broader checks.
## Verification Baseline
For any code PR generated from these plans:
- Run focused `cargo test` commands for touched crates and modules.
- Run `cargo fmt --all`.
- Run `cargo fmt --all --check`.
- Run `make pre-commit` before opening a PR when the implementation is ready.
- Clean generated build artifacts after build-based verification.
docs/superpowers/plans/rebalance-decommission-phase1-safety-plan.md
+381
View File
@@ -0,0 +1,381 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
# Rebalance and Decommission Phase 1 Safety Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Fix the highest-risk rebalance/decommission safety defects so object-version semantics and distributed operation state cannot silently diverge.
**Architecture:** Phase 1 keeps the existing ECStore/Rebalance/PoolMeta architecture and introduces narrowly scoped behavior changes. The plan favors fail-closed semantics for unsafe migration, peer propagation, and startup conflicts. Each fix block should be implemented as a separate PR unless a shared helper is explicitly called out.
**Tech Stack:** Rust, Tokio, ECStore, Axum admin handlers, tonic peer RPC, MessagePack metadata, existing RustFS test modules.
---
## Scope
This plan covers:
- F01: rebalance delete marker and remote tiered version migration semantics.
- F02: decommission `DataMovementOverwriteErr` cleanup safety.
- F03: decommission `pool_meta` reload as a start barrier.
- F04: store startup recovery ordering for pool meta and rebalance.
- F05: rebalance distributed start/stop semantics and `stopping` visibility.
This plan does not cover multipart streaming, checksum preservation, lifecycle-expired cleanup, overwrite convergence, cleanup warning UX, audit fields, or metadata decoding hardening. Those belong to later phase plans.
## Shared Principles
- Treat unknown target equivalence as unsafe.
- Do not clean a source entry unless the target state is proven complete or the version is intentionally skipped by a documented policy.
- Do not report distributed admin success when required peers failed.
- Do not auto-run rebalance and decommission together after restart.
- Prefer small local helpers near existing logic over broad refactors.
## F01: Rebalance Delete Marker and Remote Tiered Version Safety
### Decision Needed
Remote tiered versions need a product decision before coding:
- **Recommended short-term behavior:** Match MinIO and skip remote tiered versions during rebalance. This avoids changing remote-tier metadata during a balancing operation and removes the immediate source-cleanup data-loss risk.
- **Later behavior:** Implement explicit cross-pool target metadata movement for remote tiered versions only after tests prove remote references, lifecycle state, and cleanup semantics.
Delete markers should not be skipped. They preserve versioned delete semantics and must be copied to the selected target pool before source cleanup.
### Files
- Modify: `crates/ecstore/src/rebalance.rs`
- Possibly modify: `crates/ecstore/src/data_movement.rs`
- Avoid unless necessary: `crates/ecstore/src/set_disk.rs`
- Test: existing rebalance unit tests in `crates/ecstore/src/rebalance.rs`
### Design
1. Add an ECStore-level helper for rebalance delete marker movement.
- Input: source pool index, bucket, object name, source `FileInfo`, version ID.
- Behavior: choose a target pool using the same placement rules as normal data movement, excluding source/rebalancing/decommissioned pools.
- Write the delete marker metadata to the target pool with the original version ID, mod time, delete marker flag, and replication state.
- Return success only when the target write succeeds or an equivalent target delete marker is confirmed.
2. Change the delete marker branch in `migrate_entry_version_with_retry_wait`.
- Do not call `SetDisks::delete_object_for_migration` for rebalance delete markers.
- Call the ECStore-level helper via the `transfer`/backend abstraction or split delete marker handling out of the `SetDisks` backend path.
- Do not set `moved = true` unless the target helper succeeds.
3. Change remote tiered branch.
- Short term: return `ignored = true`, `cleanup_ignored = false`, `moved = false` for remote tiered versions so they do not contribute to full source cleanup.
- If skipping a remote tiered version prevents source cleanup, record a clear status reason so operators understand why the entry remains.
4. Source cleanup must remain blocked if any version was skipped without cleanup permission.
### Implementation Tasks
- [ ] Write a failing test proving a rebalance delete marker currently does not require target-pool metadata before counting complete.
- [ ] Write a failing test proving remote tiered versions do not allow source cleanup under the short-term skip policy.
- [ ] Implement the delete marker target write helper.
- [ ] Replace the rebalance delete marker source-set branch with the target write helper.
- [ ] Replace the remote tiered branch with the skip-without-cleanup policy.
- [ ] Run focused rebalance tests.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore rebalance_delete_marker --lib
cargo test -p rustfs-ecstore remote_tier --lib
cargo test -p rustfs-ecstore rebalance_entry --lib
```
### Acceptance Criteria
- Rebalance does not clean a source entry when a delete marker target write fails.
- Rebalance preserves versioned delete behavior after source cleanup.
- Remote tiered versions are either safely skipped without source cleanup or explicitly moved with verified target metadata.
- Existing non-delete-marker rebalance behavior remains unchanged.
### Risks
- The existing `MigrationBackend` trait is source-set oriented. If adapting it becomes invasive, prefer an ECStore-specific path in `rebalance_entry` rather than widening the trait for one branch.
- Skipping remote tiered versions may leave more source entries behind. That is safer than deleting unverified metadata and can be improved in a later PR.
---
## F02: Decommission `DataMovementOverwriteErr` Cleanup Safety
### Decision
`DataMovementOverwriteErr` must not be cleanup-safe by default. It can count as complete only after explicit target equivalence verification.
### Files
- Modify: `crates/ecstore/src/pools.rs`
- Possibly modify: `crates/ecstore/src/data_movement.rs`
- Test: decommission tests in `crates/ecstore/src/pools.rs`
### Design
1. Update delete marker and remote tiered decommission error handling.
- Keep object-not-found and version-not-found handling as cleanup-safe only when existing semantics justify it.
- Remove `is_err_data_movement_overwrite(&err)` from branches that set `cleanup_ignored = true`.
2. Add a narrow equivalence helper only if an existing target version can be inspected cheaply.
- Compare bucket, object, version ID, delete marker state, ETag where applicable, mod time, and key metadata.
- If comparison cannot be done reliably in the current code path, treat overwrite as failure for Phase 1.
3. Preserve source entry on unsafe overwrite.
- Set `failure = true`.
- Include a clear error stage in logs/status.
- Do not increment the decommissioned count.
### Implementation Tasks
- [ ] Write a failing unit test for delete marker decommission where `DataMovementOverwriteErr` must not count complete.
- [ ] Write a failing unit test for remote tiered decommission where `DataMovementOverwriteErr` must not count complete.
- [ ] Remove `is_err_data_movement_overwrite` from cleanup-safe conditions in those branches.
- [ ] Add an explicit helper or comment documenting why overwrite is unsafe without equivalence.
- [ ] Run focused decommission tests.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore decommission --lib
cargo test -p rustfs-ecstore DataMovementOverwriteErr --lib
```
### Acceptance Criteria
- `DataMovementOverwriteErr` does not set `cleanup_ignored = true` by default.
- Source cleanup does not run when overwrite equivalence is unknown.
- Logs make unsafe overwrite distinguishable from not-found cleanup-safe cases.
### Risks
- Tightening this behavior may cause more decommission retries/failures in clusters that previously masked unsafe state. That is intended; status must make the failure actionable.
---
## F03: Decommission Pool Meta Reload Barrier
### Decision
A successful decommission start must mean all required peers have acknowledged the updated pool meta or the API must return an explicit failure/degraded result.
### Files
- Modify: `crates/ecstore/src/pools.rs`
- Modify: `crates/ecstore/src/notification_sys.rs`
- Modify: `rustfs/src/admin/handlers/pools.rs`
- Possibly modify: `crates/ecstore/src/rpc/peer_rest_client.rs`
- Possibly modify: `rustfs/src/storage/rpc/node_service.rs`
### Design
1. Change `start_decommission` propagation behavior.
- After `pool_meta.save`, call `reload_pool_meta`.
- If peer reload returns an aggregate error, return that error to the admin handler.
- Do not silently proceed.
2. Decide rollback behavior.
- Preferred minimal Phase 1: fail the API and leave persisted `pool_meta` in decommission state only if rollback is unsafe or unavailable; status must show reload failure.
- Stronger option: persist a reverted pool meta before returning failure. This needs careful lock and persistence review and should not be attempted without tests.
3. Ensure decommission workers do not spawn after reload failure.
- If the caller starts workers after `start_decommission` returns, returning `Err` is sufficient.
- If any background path can resume from persisted metadata after a failed reload, status must clearly show pending/degraded state.
### Implementation Tasks
- [ ] Write a failing test for `start_decommission` with peer reload failure.
- [ ] Change reload failure from `warn` to returned error.
- [ ] Update admin handler error mapping so clients do not receive success.
- [ ] Add status/log context for peer reload failure.
- [ ] Run focused pool/decommission tests.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore start_decommission --lib
cargo test -p rustfs-ecstore reload_pool_meta --lib
cargo test -p rustfs decommission --lib
```
### Acceptance Criteria
- Admin start decommission does not return success when peer reload fails.
- Decommission workers do not start after reload failure through the admin path.
- Failure output includes enough peer context for operators.
### Risks
- Persisted pool meta may already be updated before reload failure is detected. Avoid adding rollback until the exact persistence safety is reviewed. Fail-closed reporting is the minimum safe fix.
---
## F04: Store Init Recovery Ordering
### Decision
Store init must load and install pool meta before deciding whether rebalance can auto-start.
### Files
- Modify: `crates/ecstore/src/store/init.rs`
- Possibly modify: `crates/ecstore/src/rebalance.rs`
- Possibly modify: `crates/ecstore/src/pools.rs`
### Design
1. Reorder init sequence.
- Initialize boot time.
- Load pool meta.
- Validate and install pool meta.
- Resolve resumable decommission pools.
- Load rebalance meta.
- Start rebalance only if no decommission is active or resumable.
2. Define conflict behavior.
- If active decommission and started rebalance metadata coexist, decommission wins for Phase 1.
- Rebalance should not start.
- Record or expose a clear deferred/conflict reason if existing status structures can carry it without broad schema changes.
3. Preserve existing happy path.
- Clusters with only rebalance metadata should still auto-start rebalance.
- Clusters with only decommission metadata should still resume decommission after the existing delay.
### Implementation Tasks
- [ ] Write a failing init test for active decommission plus rebalance metadata.
- [ ] Move pool meta load/validate/install before `load_rebalance_meta` and `start_rebalance`.
- [ ] Add a helper such as `should_auto_start_rebalance_after_init`.
- [ ] Ensure decommission resume still uses the installed pool meta.
- [ ] Run focused init tests.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore init --lib
cargo test -p rustfs-ecstore rebalance_meta --lib
cargo test -p rustfs-ecstore decommission --lib
```
### Acceptance Criteria
- Rebalance does not start when active decommission is loaded from pool meta.
- Rebalance still starts when no decommission is active.
- Decommission resume behavior is unchanged except that it no longer races with an already-started rebalance.
### Risks
- Init ordering can affect existing startup failure behavior. Keep error handling and stage names explicit so operators can identify which stage failed.
---
## F05: Rebalance Distributed Start/Stop Semantics
### Decision
Admin start/stop must not report plain success for partial cluster state. If stop remains asynchronous, status must expose that distinction.
### Files
- Modify: `rustfs/src/admin/handlers/rebalance.rs`
- Modify: `rustfs/src/storage/rpc/node_service.rs`
- Modify: `crates/ecstore/src/notification_sys.rs`
- Modify: `crates/ecstore/src/rpc/peer_rest_client.rs`
- Possibly modify: `crates/ecstore/src/rebalance.rs`
### Design
1. Fix peer RPC stop first.
- `node_service::stop_rebalance` must return `success=false` or gRPC error when `store.stop_rebalance().await` fails.
- `notification_sys.stop_rebalance` must propagate aggregate peer failure instead of warning and returning `Ok`.
2. Fix start propagation.
- Avoid returning success from peer `load_rebalance_meta(start=true)` before local start validation has completed.
- If fully synchronous worker startup is too invasive, split semantics:
- metadata loaded;
- start accepted;
- worker running observable through status.
- Admin start should fail when any required peer fails load/start acceptance.
3. Define rollback/degraded semantics.
- Preferred Phase 1: return failure on propagation failure and attempt local stop if local rebalance was already started.
- If rollback fails, return a degraded error and expose status.
4. Add status language for stop.
- If cancellation is requested but workers may still be running, status should not claim fully stopped.
- Minimal approach: add a `stopping` or equivalent field if existing response types allow it.
### Implementation Tasks
- [ ] Write a failing test for peer stop returning success despite local stop error.
- [ ] Update `node_service::stop_rebalance` to return failure details.
- [ ] Write a failing test for `notification_sys.stop_rebalance` aggregate peer failure.
- [ ] Update `notification_sys.stop_rebalance` to return aggregate error.
- [ ] Write a failing admin start propagation test.
- [ ] Update admin start to fail or rollback on propagation failure.
- [ ] Add status distinction for requested stop versus completed stop if response schema permits.
- [ ] Run focused admin/RPC/rebalance tests.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore stop_rebalance --lib
cargo test -p rustfs-ecstore load_rebalance_meta --lib
cargo test -p rustfs rebalance --lib
```
### Acceptance Criteria
- Peer stop failure is visible to callers.
- Admin stop fails or reports degraded state on peer failure.
- Admin start fails or reports degraded state on peer load/start failure.
- Stop status does not mislead operators while workers are still winding down.
### Risks
- API response schema changes may affect clients. Prefer backward-compatible additions where possible, and keep error responses compatible with existing admin error handling.
---
## Phase 1 Test Matrix
Run these once the individual fix tests pass:
```bash
cargo test -p rustfs-ecstore rebalance --lib
cargo test -p rustfs-ecstore decommission --lib
cargo test -p rustfs-ecstore pools --lib
cargo test -p rustfs rebalance --lib
cargo test -p rustfs pools --lib
cargo fmt --all --check
```
Before opening a PR with code changes:
```bash
cargo fmt --all
cargo fmt --all --check
make pre-commit
```
After build-based verification, clean generated build artifacts to avoid unnecessary disk usage.
## Suggested PR Split
1. PR 1: F02 only. Smallest fail-closed fix.
2. PR 2: F04 only. Startup ordering and conflict tests.
3. PR 3: F03 only. Decommission peer reload barrier.
4. PR 4: F05 stop semantics first, then start semantics if review scope remains manageable.
5. PR 5: F01 delete marker fix and remote tiered skip policy.
F01 is highest risk, but it may need the most design review. F02 and F04 are good first implementation candidates because they reduce safety risk with narrower code changes.
## Open Questions Before Coding
1. For F01 remote tiered versions, should Phase 1 match MinIO and skip them during rebalance, or should RustFS implement cross-pool remote metadata movement now?
2. For F03 reload failure, should the implementation attempt to rollback persisted pool meta, or fail closed and expose degraded metadata state?
3. For F05 start propagation, is a backward-compatible degraded error response acceptable, or must the API stay strictly compatible with current success/error shapes?
docs/superpowers/plans/rebalance-decommission-phase2-data-movement-plan.md
+368
View File
@@ -0,0 +1,368 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
# Rebalance and Decommission Phase 2 Data Movement Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Make shared data movement safe for large objects, metadata-preserving, and resilient to overwrite and cleanup races.
**Architecture:** Phase 2 keeps `crates/ecstore/src/data_movement.rs` as the shared migration core, but tightens its streaming, metadata, and equivalence behavior. Rebalance and decommission should share small helpers where their safety checks are identical, while keeping operation-specific policy decisions in `rebalance.rs` and `pools.rs`.
**Tech Stack:** Rust, Tokio async readers, ECStore object APIs, multipart APIs, lifecycle evaluator, filemeta metadata types, existing crate-local unit tests.
---
## Scope
This plan covers:
- F06: stream multipart data movement instead of buffering whole parts.
- F07: preserve and verify checksum, replication, version purge, and object-lock metadata.
- F08: align or explicitly define decommission lifecycle-expired cleanup behavior.
- F09: handle rebalance overwrite races with explicit target equivalence checks.
- F10: add final source cleanup verification before prefix deletion.
This plan assumes Phase 1 either has landed or its semantics are stable enough to build on.
## Shared Principles
- Streaming must keep memory bounded by buffer size, not part size.
- Metadata equivalence should be explicit and testable.
- A version is complete only when copied, intentionally skipped by policy, or proven equivalent on target.
- Source cleanup must be the last step and must verify the source version set has not changed.
- Fail closed when equivalence cannot be proven.
## F06: Stream Multipart Data Movement
### Decision
Multipart migration must not allocate a buffer equal to `part.size`. It should stream each part into `put_object_part`.
### Files
- Modify: `crates/ecstore/src/data_movement.rs`
- Possibly modify: `crates/ecstore/src/store/multipart.rs`
- Possibly modify: `crates/ecstore/src/store_api/readers.rs`
- Test: data movement tests in `crates/ecstore/src/data_movement.rs`
### Design
1. Replace per-part `Vec<u8>` allocation.
- Current behavior reads each part with `read_exact` into `chunk`.
- New behavior should create a bounded reader over the shared object stream for exactly `part.size` bytes.
2. Preserve part boundaries.
- Each `put_object_part` call must receive a reader that ends exactly at the part boundary.
- If a part stream ends early, return a staged `read_part` error and abort the multipart upload.
3. Preserve part index behavior.
- Continue decoding `part.index`.
- Preserve indexed reader behavior so erasure/indexed metadata remains compatible.
4. Preserve abort behavior.
- `abort_multipart_upload` must still run if any part upload or complete call fails before completion is marked.
### Implementation Tasks
- [ ] Add a failing data movement test with a fake large multipart stream that panics or fails if code tries to allocate/read the full part into memory.
- [ ] Add a test proving part boundary reads consume exactly the configured part sizes.
- [ ] Introduce a bounded async reader helper for a single multipart part.
- [ ] Replace `vec![0u8; part.size]` and `read_exact` in multipart migration with the bounded reader helper.
- [ ] Ensure part upload still wraps data in `PutObjReader` with correct size, actual size, and index.
- [ ] Keep existing abort-on-error behavior and add a focused test for failure during streamed part upload.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore data_movement --lib
cargo test -p rustfs-ecstore multipart --lib
```
### Acceptance Criteria
- Data movement memory is bounded and not proportional to multipart part size.
- Multipart part boundaries, sizes, ETags, and indexes remain correct.
- Failed streamed part upload aborts the temporary multipart upload.
### Risks
- Async reader composition can accidentally over-read into the next part. The part-boundary test is mandatory.
- If `put_object_part` requires a concrete reader type, introduce the smallest adapter needed rather than rewriting multipart internals.
---
## F07: Preserve and Verify Full Data Movement Metadata
### Decision
Checksum preservation is a confirmed fix. Replication, version purge, and object-lock behavior must be tested first; production changes should follow only where tests reveal loss or mismatch.
### Files
- Modify: `crates/ecstore/src/data_movement.rs`
- Possibly modify: `crates/ecstore/src/store_api/types.rs`
- Possibly modify: `crates/ecstore/src/set_disk.rs`
- Possibly modify: `crates/filemeta/src/fileinfo.rs`
- Possibly modify: `crates/filemeta/src/replication.rs`
- Test: data movement tests in `crates/ecstore/src/data_movement.rs`
### Design
1. Define metadata equivalence for data movement.
- Required fields: version ID, ETag, size, actual size, mod time, user metadata, storage class, checksum, multipart checksum, replication state, version purge state, object-lock mode/date, legal hold.
- Use a helper local to data movement tests first. Promote to production only if F09/F10 need it.
2. Preserve multipart checksum.
- When source part checksum exists, populate the corresponding `CompletePart` checksum fields.
- Preserve object-level multipart checksum metadata when complete metadata is written.
3. Validate replication state preservation.
- Current code copies `user_defined` and `set_disk` reconstructs `replication_state_internal` from metadata.
- Add tests to prove this works for replication status and version purge status.
- If tests fail, extend `ObjectOptions` or write path metadata handling narrowly.
4. Validate object lock preservation.
- Current code copies `user_defined`, which should carry object-lock headers.
- Add tests for retention mode/date and legal hold.
- If tests fail, fix the metadata copy path without changing normal user copy semantics.
### Implementation Tasks
- [ ] Add a metadata equivalence assertion helper in data movement tests.
- [ ] Add a failing multipart checksum preservation test.
- [ ] Populate `CompletePart` checksum fields from source part metadata.
- [ ] Add single-part checksum preservation test and fix object-level checksum metadata if needed.
- [ ] Add replication state and version purge state preservation tests.
- [ ] Add object-lock retention and legal hold preservation tests.
- [ ] Apply the minimal production fixes required by those tests.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore data_movement --lib
cargo test -p rustfs-ecstore checksum --lib
cargo test -p rustfs-ecstore replication --lib
cargo test -p rustfs-ecstore object_lock --lib
```
### Acceptance Criteria
- Migrated single-part and multipart objects preserve checksum behavior.
- Migrated multipart parts preserve per-part checksum metadata where RustFS stores it.
- Replication state and version purge state are equivalent after migration.
- Object-lock retention and legal hold remain unchanged after migration.
### Risks
- Some metadata may be derived rather than stored directly. Tests should compare externally observable object info, not only internal fields.
- Avoid broad `ObjectOptions` expansion unless current metadata copy cannot preserve a required field.
---
## F08: Lifecycle-Expired Version Cleanup Semantics
### Decision
RustFS should either align with MinIO by counting lifecycle-expired versions as decommission-complete, or explicitly document and expose retained expired source entries. The recommended behavior is to align with MinIO when lifecycle/object-lock/replication checks say the version is safe to expire.
### Files
- Modify: `crates/ecstore/src/pools.rs`
- Possibly modify: lifecycle evaluator under `crates/ecstore/src/bucket/lifecycle/`
- Test: decommission tests in `crates/ecstore/src/pools.rs`
### Design
1. Preserve existing lifecycle safety checks.
- Do not bypass object-lock or replication constraints.
- Keep `should_skip_lifecycle_for_data_movement` or equivalent evaluator as the source of truth.
2. Count safe expired versions as complete for source cleanup.
- Replace `expired == 0 && decommissioned == total_versions` with semantics that allow `decommissioned + expired == total_versions` when expired versions are safe.
3. Keep status honest.
- If a version is retained because expiration is not safe, status should show it as remaining, not completed.
### Implementation Tasks
- [ ] Write a failing test where one migrated version plus one lifecycle-expired version should allow source cleanup.
- [ ] Write a test where object-lock protected expired-looking version must not allow cleanup.
- [ ] Write a test where replication-pending version must not allow cleanup.
- [ ] Update source cleanup predicate to count safe expired versions.
- [ ] Update counters/status if existing fields distinguish expired from decommissioned.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore decommission --lib
cargo test -p rustfs-ecstore lifecycle --lib
cargo test -p rustfs-ecstore object_lock --lib
```
### Acceptance Criteria
- Source entry is cleaned when every version is migrated or safely lifecycle-expired.
- Protected versions still block cleanup.
- Decommission status remains understandable for migrated, expired, and blocked versions.
### Risks
- Counting expired versions as complete can hide lifecycle evaluator bugs. Tests must include protected negative cases.
---
## F09: Rebalance Overwrite Race Equivalence
### Decision
Unsafe `DataMovementOverwriteErr` remains a failure. Equivalent target overwrite can be counted complete only after explicit comparison.
### Files
- Modify: `crates/ecstore/src/data_movement.rs`
- Modify: `crates/ecstore/src/rebalance.rs`
- Possibly modify: `crates/ecstore/src/store/object.rs`
- Test: data movement and rebalance tests
### Design
1. Define target equivalence helper.
- Compare version ID, ETag, size, actual size, mod time, checksum, delete marker state, and selected user metadata.
- Reuse the test helper from F07 if appropriate, but production helper must be limited to fields needed for safety.
2. Apply helper only to overwrite race cases.
- If `DataMovementOverwriteErr` occurs and target pool differs from source pool, inspect target object/version.
- If equivalent, count migration as complete.
- If not equivalent or cannot inspect target, return failure.
3. Keep source-equals-target unsafe.
- Do not convert source-equals-target into success without independent target proof.
### Implementation Tasks
- [ ] Add a failing rebalance test where equivalent target version exists and overwrite should converge.
- [ ] Add a failing rebalance test where target version differs and overwrite must fail.
- [ ] Implement a minimal target equivalence helper.
- [ ] Wire the helper into data movement overwrite handling for rebalance.
- [ ] Keep decommission behavior aligned with F02.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore DataMovementOverwriteErr --lib
cargo test -p rustfs-ecstore rebalance --lib
cargo test -p rustfs-ecstore data_movement --lib
```
### Acceptance Criteria
- Equivalent overwrite does not block rebalance convergence.
- Non-equivalent overwrite does not clean source.
- Logs/status distinguish equivalent overwrite from unsafe overwrite.
### Risks
- Comparing too few fields can mark a corrupt target complete. Prefer strict comparison in Phase 2.
- Comparing too many volatile fields can prevent convergence. Use tests to calibrate.
---
## F10: Source Cleanup Preflight Verification
### Decision
Before deleting a source prefix, rebalance and decommission should verify the source version set still matches the migrated or intentionally skipped set.
### Files
- Modify: `crates/ecstore/src/rebalance.rs`
- Modify: `crates/ecstore/src/pools.rs`
- Possibly modify: `crates/ecstore/src/set_disk.rs`
- Test: rebalance and decommission cleanup tests
### Design
1. Capture source version identity at scan time.
- Version identity should include object name, version ID, delete marker flag, and enough metadata to detect a new or changed version.
2. Re-read before cleanup.
- Immediately before `delete_prefix`, read current source metadata.
- Compare current identity set to the expected cleanup set.
3. Defer on mismatch.
- Rebalance should defer/retry the entry with a clear last error.
- Decommission should fail the entry or retry according to existing decommission retry policy.
4. Keep not-found idempotent.
- If source entry is already gone, cleanup remains successful.
### Implementation Tasks
- [ ] Add helper to build a stable version identity set from `FileInfoVersions`.
- [ ] Add rebalance test where source metadata changes between migration and cleanup.
- [ ] Add decommission test where source metadata changes between migration and cleanup.
- [ ] Add cleanup preflight before rebalance source `delete_prefix`.
- [ ] Add cleanup preflight before decommission source `delete_prefix`.
- [ ] Return deferred/failure status with a clear reason on mismatch.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore rebalance_entry --lib
cargo test -p rustfs-ecstore decommission_entry --lib
cargo test -p rustfs-ecstore cleanup --lib
```
### Acceptance Criteria
- Cleanup does not delete a source entry if the version set changed after migration started.
- Already-deleted source remains idempotent.
- Mismatch errors are visible in status or last-error fields.
### Risks
- Extra metadata reads can add I/O to hot migration paths. Keep this as a cleanup-only check.
---
## Phase 2 Test Matrix
Run after individual fix tests pass:
```bash
cargo test -p rustfs-ecstore data_movement --lib
cargo test -p rustfs-ecstore multipart --lib
cargo test -p rustfs-ecstore rebalance --lib
cargo test -p rustfs-ecstore decommission --lib
cargo test -p rustfs-ecstore lifecycle --lib
cargo fmt --all --check
```
Before PR:
```bash
cargo fmt --all
cargo fmt --all --check
make pre-commit
```
Clean generated build artifacts after build-based verification.
## Suggested PR Split
1. PR 1: F06 streaming multipart migration.
2. PR 2: F07 checksum and metadata preservation tests/fixes.
3. PR 3: F08 lifecycle-expired cleanup semantics.
4. PR 4: F09 overwrite equivalence.
5. PR 5: F10 cleanup preflight verification.
F06 and F07 may combine only if checksum preservation requires the new streaming reader shape. F09 should wait for F07 if it depends on metadata equivalence helpers.
## Open Questions Before Coding
1. Which metadata fields are considered mandatory for equivalence in Phase 2: strict internal fields or externally observable fields only?
2. Should F08 fully match MinIO cleanup behavior, or should RustFS keep expired source entries but expose them as residual state?
3. Should F10 apply to both rebalance and decommission in the same PR, or should rebalance land first as the higher-risk cleanup path?
docs/superpowers/plans/rebalance-decommission-phase3-hardening-plan.md
+325
View File
@@ -0,0 +1,325 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
# Rebalance and Decommission Phase 3 Hardening Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Improve operator visibility, auditability, metadata robustness, and documented compatibility after the Phase 1 and Phase 2 safety fixes.
**Architecture:** Phase 3 should not change core data movement semantics unless a hardening test exposes a safety bug. It adds bounded status data, structured logs, metadata validation tests, and an explicit decision around rebalance completion tolerance.
**Tech Stack:** Rust, tracing structured logs, admin status DTOs, serde/rmp metadata decoding, existing guardrail scripts, crate-local unit tests.
---
## Scope
This plan covers:
- F11: improve rebalance cleanup failure reporting.
- F12: add structured audit fields for rebalance/decommission operations.
- F13: harden persisted rebalance and pool metadata decoding.
- F14: decide and document rebalance completion threshold behavior.
This plan assumes the safety semantics from Phase 1 and the data movement behavior from Phase 2 are stable.
## Shared Principles
- Hardening must not hide safety failures.
- Status fields should be bounded to avoid unbounded metadata growth.
- Logs must be structured, searchable, and free of secrets.
- Metadata compatibility must be explicit: strict where possible, legacy-compatible where necessary.
## F11: Rebalance Cleanup Failure Reporting
### Decision
Rebalance cleanup failures should not be invisible completion details. Admin status should expose whether completion had cleanup warnings and provide bounded object-level detail.
### Files
- Modify: `crates/ecstore/src/rebalance.rs`
- Modify: `rustfs/src/admin/handlers/rebalance.rs`
- Possibly modify: admin response DTOs in the same module or related madmin types
- Test: rebalance status and metadata tests
### Design
1. Extend cleanup warning metadata.
- Keep existing count.
- Add a bounded list of recent cleanup failures, for example last 10 entries.
- Each entry should include bucket, object, message, and timestamp.
2. Keep metadata bounded.
- Do not store every cleanup failure unboundedly in `rebalance.bin`.
- Use a ring-buffer style helper or truncate older entries.
3. Make terminal state visible.
- If rebalance completes with cleanup warnings, status should show warning count and details.
- Avoid reporting a clean completion when source cleanup failed.
### Implementation Tasks
- [ ] Add a cleanup warning entry struct with bounded retention.
- [ ] Add metadata compatibility defaults for older `rebalance.bin` without the new list.
- [ ] Update `record_rebalance_cleanup_warning_in_meta` to append bounded entries and update count/last fields.
- [ ] Update admin status serialization to expose warning count and entries.
- [ ] Add tests for one warning, multiple warnings, and bounded truncation.
- [ ] Add legacy metadata decode test.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore cleanup_warning --lib
cargo test -p rustfs-ecstore rebalance_meta --lib
cargo test -p rustfs rebalance --lib
```
### Acceptance Criteria
- Multiple cleanup failures are visible through count and bounded details.
- Legacy rebalance metadata still decodes.
- Status distinguishes clean completion from completion with cleanup warnings.
### Risks
- Adding status fields may affect clients if response schemas are strict. Prefer additive fields with defaults.
---
## F12: Structured Audit Fields
### Decision
Rebalance and decommission admin operations should have consistent structured logs for success, rejection, propagation failure, and partial/degraded state.
### Files
- Modify: `rustfs/src/admin/handlers/rebalance.rs`
- Modify: `rustfs/src/admin/handlers/pools.rs`
- Modify: `crates/ecstore/src/notification_sys.rs`
- Modify: `crates/ecstore/src/rpc/peer_rest_client.rs`
- Possibly modify: `scripts/check_logging_guardrails.sh` or related guardrails if logging rules require updates
### Design
1. Define standard fields.
- `event`
- `component`
- `subsystem`
- `action`
- `result`
- `request_id` when available
- `actor` or masked access key when available
- `remote_addr` when available
- `pool_indices` or `rebalance_id`
- `peer`
- `error`
2. Apply to admin handlers.
- Start, stop, cancel, status-affecting operations.
- Authorization failures should remain handled by existing auth paths but should have enough context if already logged.
3. Apply to peer propagation.
- Peer success and failure should use stable event names.
- Partial failure must be warn/error, not info-only.
4. Avoid secret leakage.
- Never log raw credentials, signatures, tokens, or full auth headers.
### Implementation Tasks
- [ ] Inventory existing rebalance/decommission log events and field names.
- [ ] Define a small local helper or convention for masked actor/request fields if one already exists.
- [ ] Update rebalance admin start/stop/status logs.
- [ ] Update decommission start/cancel/status logs.
- [ ] Update notification peer propagation failure logs.
- [ ] Add log-capture tests for success, partial failure, and rejected operation where feasible.
- [ ] Run logging guardrail script if the repository has one.
### Focused Test Commands
```bash
cargo test -p rustfs rebalance --lib
cargo test -p rustfs pools --lib
cargo test -p rustfs-ecstore notification --lib
scripts/check_logging_guardrails.sh
```
### Acceptance Criteria
- Operators can answer who requested an operation, what resource it targeted, and which peers failed.
- Partial failures are searchable through stable structured fields.
- Logs contain no secrets.
### Risks
- Logging changes can be noisy. Keep high-volume per-object logs out of admin audit events.
---
## F13: Persisted Metadata Decode Hardening
### Decision
Persisted rebalance and pool metadata should reject or surface unknown/corrupt fields where compatibility allows. Legacy compatibility must be tested explicitly.
### Files
- Modify: `crates/ecstore/src/rebalance.rs`
- Modify: `crates/ecstore/src/pools.rs`
- Possibly add fixtures under an existing test fixture directory if one exists
- Test: metadata decode tests in `rebalance.rs` and `pools.rs`
### Design
1. Identify persisted structs.
- Rebalance metadata and stats structs.
- Pool metadata and decommission info structs.
2. Choose strictness per struct.
- Use strict unknown-field rejection where metadata is not expected to carry forward-compatible fields.
- For legacy-compatible structs, keep decode lenient but log or validate unknown/unsupported state where possible.
3. Add state validation.
- Reject conflicting terminal states such as complete plus canceled plus running.
- Reject impossible counters or invalid pool indices where existing validation can catch them.
4. Preserve legacy fixtures.
- Existing old metadata must still decode if compatibility is required.
### Implementation Tasks
- [ ] List all rebalance and pool metadata structs currently deserialized from persisted bytes.
- [ ] Add fixture tests for unknown fields, missing critical fields, and conflicting terminal states.
- [ ] Add strict serde attributes only where fixtures show compatibility is safe.
- [ ] Add post-decode validation helpers for state conflicts.
- [ ] Update load paths to surface validation errors with actionable messages.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore rebalance_meta --lib
cargo test -p rustfs-ecstore pool_meta --lib
cargo test -p rustfs-ecstore metadata --lib
```
### Acceptance Criteria
- Unknown/corrupt metadata does not silently become an apparently valid operation state.
- Legacy metadata fixtures continue to decode through documented paths.
- Conflicting terminal state is rejected or quarantined.
### Risks
- Strict decode can block startup if old metadata contains benign extra fields. Start with tests and post-decode validation before broad strict attributes.
---
## F14: Rebalance Completion Threshold Decision
### Decision
RustFS must explicitly choose whether to match MinIO's tolerance-based rebalance completion or keep strict completion. The recommended default is to match MinIO unless RustFS has a documented reason to move more data.
### Files
- Modify: `crates/ecstore/src/rebalance.rs`
- Possibly modify: admin docs if present
- Test: rebalance goal/completion tests in `rebalance.rs`
### Design Options
1. **Match MinIO tolerance.**
- Complete when pool free-space ratio is within the MinIO-like tolerance of the goal.
- Reduces movement and operational time.
- Best for compatibility.
2. **Keep strict behavior and document it.**
- No code behavior change.
- Add tests proving strict behavior is intentional.
- Status/docs should avoid implying MinIO-compatible tolerance.
3. **Configurable tolerance.**
- Most flexible but not recommended unless operators need it.
- Adds configuration and support burden.
### Recommended Plan
Start with option 1 if compatibility is the goal. If maintainers prefer strictness, implement option 2 with explicit tests and documentation.
### Implementation Tasks
- [ ] Add tests that capture current strict behavior around the completion goal.
- [ ] Add tests for MinIO-like tolerance behavior.
- [ ] Confirm maintainers choose MinIO-compatible or strict behavior.
- [ ] Implement the selected threshold behavior.
- [ ] Update status/docs comments so completion semantics are clear.
### Focused Test Commands
```bash
cargo test -p rustfs-ecstore rebalance_goal --lib
cargo test -p rustfs-ecstore check_if_rebalance_done --lib
cargo test -p rustfs-ecstore rebalance --lib
```
### Acceptance Criteria
- Completion threshold behavior is covered by deterministic tests.
- Operators can understand whether RustFS matches MinIO tolerance.
- Empty-queue completion path remains valid.
### Risks
- Changing threshold may alter operational expectations for existing users. Document the change if behavior changes.
---
## Phase 3 Test Matrix
Run after individual fix tests pass:
```bash
cargo test -p rustfs-ecstore rebalance_meta --lib
cargo test -p rustfs-ecstore pool_meta --lib
cargo test -p rustfs-ecstore rebalance --lib
cargo test -p rustfs rebalance --lib
cargo test -p rustfs pools --lib
cargo fmt --all --check
```
If logging guardrails are relevant:
```bash
scripts/check_logging_guardrails.sh
```
Before PR:
```bash
cargo fmt --all
cargo fmt --all --check
make pre-commit
```
Clean generated build artifacts after build-based verification.
## Suggested PR Split
1. PR 1: F11 cleanup warning status.
2. PR 2: F12 structured audit fields.
3. PR 3: F13 metadata decode hardening.
4. PR 4: F14 completion threshold decision and tests.
F11 may depend on Phase 2 cleanup semantics if F10 changes retry behavior. F12 can be done anytime after Phase 1 defines distributed failure semantics.
## Open Questions Before Coding
1. What is the maximum number of cleanup warning entries to keep in persisted rebalance metadata?
2. Which actor identity is safe and useful to log for admin requests?
3. Should metadata unknown fields fail startup, warn and continue, or quarantine only the affected operation?
4. Should RustFS match MinIO's rebalance completion tolerance by default?
docs/superpowers/plans/rebalance-decommission-post-remediation-review-plan.md
+137
View File
@@ -0,0 +1,137 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
# Rebalance and Decommission Post-Remediation Review
> **Status:** Updated for branch `cxymds/rebalance-decommission-remediation`.
>
> **Scope:** Rebalance, decommission, shared data-movement helpers, source cleanup,
> delete-marker handling, lifecycle expiry, restart/cancel recovery, and CI coverage.
>
> **Conclusion:** The main code-level P1/P2 remediation set has been implemented on
> this branch. The remaining release gate is verification, not another known
> correctness rewrite: full CI must pass, `make pre-commit` must pass before the
> final PR state, and the data-movement e2e proof added to CI must complete in the
> GitHub runner environment.
## Review Corrections
The previous report needed two important corrections.
First, the R28 decommission permit issue is real but was overstated. The worker
permit was released before entry migration work, so `Workers` did not account for
entry lifetime and `wk.wait()` could not prove entry work had drained. However,
the current listing path awaits entry callbacks inline and bucket processing is
bounded, so the old "unbounded object movement" wording was not supported by the
code. The fix remains correct because permit lifetime now matches entry lifetime
and prevents future spawn-based regressions.
Second, SSE-C migration is related to raw data movement but has a distinct
failure mode. Before the raw migration path, SSE-C objects did not silently
corrupt through an empty `HeaderMap`; migration reads failed while resolving
SSE-C material because the customer SSE-C headers were missing. The raw internal
read path avoids requiring request customer headers for data movement.
## Implemented Remediation
| Area | Current Branch State | Representative Guards |
| --- | --- | --- |
| Raw migration reads | Rebalance and decommission use raw data-movement read options instead of normal transformed GET semantics. | `test_rebalance_object_migration_read_opts_are_raw_data_movement`, `test_decommission_object_migration_read_opts_are_raw_data_movement` |
| Raw write invariants | Data-movement writes preserve source version, ETag, tags, expires, part metadata, and checksums consistently. Single-part raw ETag validation no longer rejects preserved source ETags. | `test_data_movement_put_object_opts_preserves_version_and_etag`, `test_data_movement_opts_preserve_tags_and_expires`, `test_data_movement_single_part_raw_reader_does_not_validate_source_etag` |
| Terminal decommission recovery | Cancel/failed/complete paths cancel in-memory workers before terminal persistence, and terminal-save failure no longer leaves ghost cancelers. | decommission canceler unit tests in `crates/ecstore/src/pools.rs` |
| Rebalance operation identity | Rebalance merge/save/stop paths are id-aware and do not let stale operation state clobber a newer operation. | rebalance id-gate and merge tests |
| Start coordination | Rebalance and decommission starts cross-check persisted peer operation state before starting conflicting work. | start cross-check tests |
| Decommission worker accounting | Entry permits are held through entry completion. R28 is no longer a pending worker-accounting bug. | decommission worker permit tests |
| Rebalance rollback | Failed start rollback finalizes metadata instead of leaving `Started` plus `stopping=true` without a worker. | rollback tests |
| Decommission progress save | Periodic progress-save errors are best-effort; terminal saves remain strict. | progress-save tests |
| Queue semantics | Multi-pool decommission is queued, local-leader prefix scheduling is supported, completed restart is rejected, completed queue prefixes can advance, failed/canceled entries block automatic promotion, failed/canceled retry preserves bucket progress, and promoted queued pools are canceled if cancellation arrives before work starts. | queue/promotion/retry tests in `pools.rs` |
| Admin status and query hardening | Pool admin status exposes queued/progress state, dangerous pool/rebalance mutation queries reject unknown or ambiguous parameters, and pool status `by-id` parsing no longer falls back to pool 0. | `admin_pool_list_item`, `pools_handler_tests`, `rebalance_handler_tests` |
| Cancel/clear operations | Non-leader cancel intent is accepted and failed/canceled terminal decommission can be explicitly cleared. | remote cancel and clear tests |
| Resume equivalence | Delete-marker replication state, tiered-object metadata, multipart part numbers, tags, expires, and version counts are covered more strictly. | data-movement equivalence tests |
| Lifecycle expiry | Data movement no longer treats failed lifecycle expiry application as a successful skip. | `resolve_data_movement_lifecycle_expiry_result_rejects_apply_failure` |
| CI coverage | Existing RustFS e2e delete-marker migration proof is wired into the `e2e-tests` CI job with `--test-threads=1`, reusing the downloaded debug binary. | `.github/workflows/ci.yml` |
## Compatibility Decisions
The current RustFS decommission contract is no longer single-pool only. Multi-pool
requests are supported as queued operations on multi-pool deployments. The
current behavior is documented in
`docs/architecture/decommission-compatibility.md`.
Delete-marker behavior is intentionally characterized rather than guessed:
- a lone delete marker without replication is cleanup-only metadata and can be
skipped;
- delete markers with replication metadata remain eligible for movement;
- rebalance and decommission share the same predicate.
Lifecycle-expired versions are also explicit:
- decommission can count safely expired versions toward source cleanup only when
lifecycle expiry application succeeds;
- rebalance remains stricter and requires actual data-movement completion for
cleanup.
## Remaining Risks and Gates
There are no remaining known P1/P2 code changes in this remediation set, but the
branch is not final-release-ready until verification completes:
1. `make pre-commit` must pass before the PR is marked ready.
2. GitHub CI must pass, including the newly wired delete-marker migration e2e
proof.
3. Local full e2e verification was attempted but the machine ran out of disk
space while building `rustfs`; this is an environment blocker, not a test
assertion failure. The `target/` directory was cleaned afterward.
4. If product requires stronger end-to-end proof for encrypted and compressed
migration, add those scenarios to `crates/e2e_test` before release. Current
branch coverage is strongest at the internal option/invariant layer, with CI
e2e coverage focused on versioning/delete-marker semantics.
## Focused Verification Run During Implementation
Focused checks were run per task, including:
```bash
cargo test -p rustfs-ecstore data_movement_single_part_raw_reader --lib
cargo test -p rustfs-ecstore test_merge_rebalance_meta_preserves_stopping_stop_snapshot --lib
cargo test -p rustfs-ecstore test_merge_rebalance_pool_stats_clears_stopping_for_terminal_status --lib
cargo test -p rustfs-ecstore first_resumable_decommission_queue_indices --lib
cargo test -p rustfs-ecstore test_return_resumable_pools_skips_failed_decommission --lib
cargo test -p rustfs-ecstore resolve_data_movement_lifecycle_expiry_result --lib
cargo test -p rustfs-ecstore lifecycle_action_removes_data_movement_version --lib
cargo test -p rustfs-ecstore test_pool_meta_promoted_queued_decommission_can_be_canceled --lib
cargo test -p rustfs admin_pool_list_item --lib
cargo test -p rustfs pools_handler_tests --lib
cargo test -p rustfs rebalance_handler_tests --lib
cargo test -p rustfs admin_query_pool_status_by_id --lib
cargo fmt --all --check
```
The attempted local e2e command was:
```bash
cargo test -p e2e_test delete_marker_migration_semantics -- --nocapture
```
It failed because the local filesystem reached `No space left on device` while
building the RustFS debug binary. CI now runs the proof in the e2e job after the
debug binary artifact has been downloaded, and uses `--test-threads=1` to avoid
parallel test-server startup for that proof.
## Final PR Gate
Before marking the draft PR ready:
```bash
cargo fmt --all
cargo fmt --all --check
make pre-commit
```
If CI reports failures after the branch is pushed, inspect the failing job logs
instead of weakening the e2e gate. The delete-marker migration proof was added
because skipping it would leave the exact versioning regression class invisible
to CI.
docs/superpowers/plans/rebalance-decommission-remediation-plan.md
+551
View File
@@ -0,0 +1,551 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
# Rebalance and Decommission Remediation Plan
> This document turns `docs/architecture/expert-review-analysis.md` into an actionable remediation backlog. It is intentionally split into independent fix blocks so each item can be analyzed, assigned, implemented, and verified separately.
## Scope
This plan covers the confirmed and calibrated issues from the rebalance/decommission expert review:
- Critical and high-risk data integrity issues.
- Distributed state propagation and recovery semantics.
- Data movement resource usage and metadata preservation.
- Operational visibility, auditability, and hardening gaps.
This file is not a code patch. Each block below should be expanded into a focused implementation plan before code changes begin.
## Priority Order
| Order | Fix ID | Source | Priority | Main Risk |
| --- | --- | --- | --- | --- |
| 1 | F01 | P1.1 | Critical | Rebalance may drop delete marker or remote tiered metadata |
| 2 | F02 | P1.5 | High | Decommission may treat unsafe overwrite as cleanup-safe |
| 3 | F03 | P1.4 | High | Retiring pool may remain writable on stale peers |
| 4 | F04 | P1.2 | High | Rebalance and decommission may both resume after restart |
| 5 | F05 | P1.3, A4 | High | Admin start/stop may report success for partial cluster state |
| 6 | F06 | P2.1 | High | Multipart migration can allocate whole parts in memory |
| 7 | F07 | P2.2, A3 | Medium | Migrated objects may lose checksum or other metadata semantics |
| 8 | F08 | P2.4 | Medium | Decommission leaves lifecycle-expired source entries behind |
| 9 | F09 | P2.3 | Medium | Rebalance overwrite races may fail instead of converging |
| 10 | F10 | A2 | Medium | Source cleanup lacks a final version-set guard |
| 11 | F11 | A1 | Medium | Rebalance cleanup failures are completion warnings only |
| 12 | F12 | P3.2 | Low | Admin operations are not sufficiently auditable |
| 13 | F13 | P3.3 | Low | Persisted metadata accepts unknown or inconsistent fields |
| 14 | F14 | P3.4 | Low | Rebalance completion threshold differs from MinIO |
## Phase 1: Data Integrity and Safety
### F01: Fix rebalance delete marker and remote tiered version migration
**Source:** P1.1
**Decision:** Confirmed fix required.
**Target priority:** Critical.
**Problem:** `rebalance_entry` marks delete markers and remote tiered versions as moved, but those branches operate through the source `SetDisks` path rather than a confirmed cross-pool target write. Source cleanup can then remove the only metadata that preserves tombstone or tiered-object semantics.
**Primary files:**
- `crates/ecstore/src/rebalance.rs`
- `crates/ecstore/src/data_movement.rs`
- `crates/ecstore/src/set_disk.rs`
- Tests near existing rebalance migration tests in `crates/ecstore/src/rebalance.rs`
**Preferred fix direction:**
- Route rebalance delete marker movement through an `ECStore`-level data movement path that chooses a non-source target pool and writes the exact version metadata to that target.
- For remote tiered versions, either:
- align with MinIO and skip tiered versions during rebalance, or
- implement a target-pool metadata move that preserves the remote tier pointer and lifecycle state.
- Do not count the version as rebalanced until the target metadata is confirmed.
**Acceptance criteria:**
- A versioned object with a delete marker remains deleted after rebalance source cleanup.
- A remote tiered version remains readable or correctly listed after rebalance source cleanup.
- Rebalance does not mark delete marker or tiered versions as complete when target metadata was not written.
**Required tests:**
- Multi-pool rebalance test for a versioned object with a delete marker.
- Multi-pool rebalance test for a remote tiered version.
- Failure test where target metadata write fails and source cleanup must not happen.
**Dependencies:** None. This is the first safety fix.
---
### F02: Stop treating `DataMovementOverwriteErr` as cleanup-safe in decommission
**Source:** P1.5
**Decision:** Confirmed fix required.
**Target priority:** High.
**Problem:** Decommission treats `DataMovementOverwriteErr` like object-not-found/version-not-found and sets `cleanup_ignored = true`. That error only means source and destination pool are the same; it does not prove an equivalent target version exists.
**Primary files:**
- `crates/ecstore/src/pools.rs`
- `crates/ecstore/src/data_movement.rs`
- `crates/ecstore/src/store/object.rs`
**Preferred fix direction:**
- Remove `DataMovementOverwriteErr` from cleanup-safe branches for delete marker and remote tiered decommission paths.
- If overwrite occurs because an equivalent target version already exists, perform an explicit equivalence check before counting the version complete.
- Otherwise return a migration failure and keep the source entry.
**Acceptance criteria:**
- `DataMovementOverwriteErr` does not increment the decommissioned count unless equivalence is proven.
- Source cleanup is blocked when target equivalence is unknown.
- Logs/status clearly state whether overwrite was equivalent-complete or unsafe.
**Required tests:**
- Delete marker decommission returns `DataMovementOverwriteErr` and does not clean source.
- Remote tiered decommission returns `DataMovementOverwriteErr` and does not clean source.
- Equivalent target version exists; overwrite can be counted complete only after metadata equality check passes.
**Dependencies:** F01 clarifies remote/tombstone movement semantics, but F02 can be implemented independently for decommission.
---
### F03: Make decommission `pool_meta` reload a cluster-wide start barrier
**Source:** P1.4
**Decision:** Confirmed fix required.
**Target priority:** High.
**Problem:** `start_decommission` saves pool meta and then calls peer reload. Reload failure is logged but the operation still returns success, leaving stale peers able to write into the retiring pool.
**Primary files:**
- `crates/ecstore/src/pools.rs`
- `crates/ecstore/src/notification_sys.rs`
- `crates/ecstore/src/rpc/peer_rest_client.rs`
- `rustfs/src/admin/handlers/pools.rs`
- `rustfs/src/storage/rpc/node_service.rs`
**Preferred fix direction:**
- Treat reload failure as a failed start unless the system has an explicit partial/degraded operation state.
- Return peer failure information to the admin handler.
- Do not spawn or resume decommission workers until required peers acknowledge the updated pool meta.
**Acceptance criteria:**
- Admin decommission start fails or returns a clearly degraded response when peer reload fails.
- No peer can select the retiring pool for new writes after decommission start is reported successful.
- Status exposes peer reload failure details.
**Required tests:**
- Unit test for `start_decommission` with mocked `reload_pool_meta` failure.
- RPC/admin test showing start does not return plain success on peer failure.
- Write-placement test verifying retiring pools are skipped only after successful cluster reload.
**Dependencies:** None.
---
### F04: Reorder store init recovery to restore pool meta before rebalance
**Source:** P1.2
**Decision:** Confirmed fix required.
**Target priority:** High.
**Problem:** Store init loads and starts rebalance before loading persisted pool meta. If unfinished decommission exists on disk, `start_rebalance` can miss it and both processes may resume.
**Primary files:**
- `crates/ecstore/src/store/init.rs`
- `crates/ecstore/src/rebalance.rs`
- `crates/ecstore/src/pools.rs`
**Preferred fix direction:**
- Load and install `PoolMeta` before attempting to start rebalance.
- After pool meta is loaded, explicitly decide:
- if decommission is active, do not auto-start rebalance;
- if rebalance metadata exists but conflicts with decommission, mark rebalance as blocked/deferred and expose it in status.
- Keep existing decommission resume delay, but ensure rebalance is not already running when decommission resumes.
**Acceptance criteria:**
- Restart with active decommission metadata and rebalance metadata does not run both workers.
- Startup status explains which operation was deferred and why.
- Existing startup without decommission still resumes rebalance normally.
**Required tests:**
- Init test with persisted active decommission plus started rebalance metadata.
- Init test with only rebalance metadata still resumes rebalance.
- Init test with corrupt or missing pool meta follows existing error policy.
**Dependencies:** None, but coordinate with F05 status semantics.
---
### F05: Make rebalance start/stop distributed semantics explicit and fail-safe
**Source:** P1.3, A4
**Decision:** Confirmed fix required.
**Target priority:** High.
**Problem:** Rebalance start can return success even when peer propagation fails or peer background start later fails. Stop can return success while peer stop failed or local stop errors were ignored. Stop is also asynchronous, but admin semantics do not clearly expose `stopping` versus `stopped`.
**Primary files:**
- `rustfs/src/admin/handlers/rebalance.rs`
- `rustfs/src/storage/rpc/node_service.rs`
- `crates/ecstore/src/notification_sys.rs`
- `crates/ecstore/src/rpc/peer_rest_client.rs`
- `crates/ecstore/src/rebalance.rs`
**Preferred fix direction:**
- For start:
- propagate rebalance metadata to peers before returning success;
- have peer RPC synchronously validate/start or return a structured failure;
- on partial failure, rollback local start or report a degraded state.
- For stop:
- never ignore `store.stop_rebalance()` errors;
- aggregate peer stop failures and return them to admin;
- expose `stop_requested`, `stopping`, and `stopped` in status if worker shutdown remains asynchronous.
**Acceptance criteria:**
- Admin start does not return plain success when any required peer fails to load/start.
- Admin stop does not return plain success when any required peer fails to stop.
- Status can distinguish a requested stop from all workers fully stopped.
**Required tests:**
- Peer `load_rebalance_meta(true)` failure.
- Peer background `start_rebalance` failure.
- Peer `stop_rebalance` local save failure.
- Stop requested while a worker is in a long migration operation.
**Dependencies:** Coordinate with F04 so startup and admin semantics use the same conflict model.
---
## Phase 2: Data Movement Correctness and Resource Control
### F06: Stream multipart data movement instead of buffering whole parts
**Source:** P2.1
**Decision:** Confirmed fix required.
**Target priority:** High.
**Problem:** Multipart migration allocates `Vec<u8>` for each part and reads the whole part into memory. Large parts plus concurrent workers can OOM the process.
**Primary files:**
- `crates/ecstore/src/data_movement.rs`
- `crates/ecstore/src/store/multipart.rs`
- `crates/ecstore/src/store/object.rs`
**Preferred fix direction:**
- Replace whole-part buffering with bounded streaming.
- Preserve the current part index and checksum/etag behavior through the streaming reader.
- Ensure multipart abort still runs on any failed part or complete call.
**Acceptance criteria:**
- Migrating a large multipart object does not allocate memory proportional to part size.
- Part ETag, size, actual size, and index handling remain correct.
- Failed migration aborts the temporary multipart upload.
**Required tests:**
- Fake large multipart reader proving memory is bounded.
- Multipart migration preserves part order and ETags.
- Failure during part upload aborts the multipart upload.
**Dependencies:** Coordinate with F07 for checksum preservation.
---
### F07: Preserve and verify full data movement metadata
**Source:** P2.2, A3
**Decision:** Confirmed fix required for checksum; additional metadata requires verification and likely fixes.
**Target priority:** Medium.
**Problem:** Data movement preserves basic metadata but does not fully prove checksum, multipart checksum, replication state, version purge status, or object-lock metadata equivalence after migration.
**Primary files:**
- `crates/ecstore/src/data_movement.rs`
- `crates/ecstore/src/store_api/types.rs`
- `crates/ecstore/src/set_disk.rs`
- `crates/filemeta/src/fileinfo.rs`
- `crates/filemeta/src/replication.rs`
**Preferred fix direction:**
- For multipart migration, populate `CompletePart` checksum fields when source part checksum exists.
- Preserve or reconstruct object-level checksum metadata.
- Add migration equivalence checks for:
- ETag;
- checksum and multipart checksum;
- replication status and version purge status;
- object lock retention mode/date and legal hold;
- version ID and mod time.
- Only change production metadata handling after tests show a real loss path.
**Acceptance criteria:**
- Migrated object metadata matches source for the fields above.
- `GetObjectAttributes` checksum behavior remains correct after migration.
- Object lock retention/legal hold remains unchanged across migration.
**Required tests:**
- Single-part object with checksum metadata.
- Multipart object with per-part checksum metadata.
- Object with replication state and version purge state.
- Object with governance/compliance retention and legal hold.
**Dependencies:** F06 should land first or in the same PR if checksum streaming needs shared reader changes.
---
### F08: Align decommission lifecycle-expired version cleanup semantics
**Source:** P2.4
**Decision:** Confirmed fix required or explicit design decision required.
**Target priority:** Medium.
**Problem:** Decommission skips lifecycle-expired versions but does not clean the source entry if any expired version exists. MinIO counts expired versions as completed and can clean the source entry.
**Primary files:**
- `crates/ecstore/src/pools.rs`
- Lifecycle evaluator code under `crates/ecstore/src/bucket/lifecycle/`
**Preferred fix direction:**
- Treat lifecycle-expired versions as completed for decommission source cleanup when they are not required to remain accessible.
- Keep lifecycle/object-lock/replication checks in place before treating a version as expired.
- If RustFS intentionally keeps expired source versions, expose that state in status and exclude the pool from being considered fully clean.
**Acceptance criteria:**
- Decommission can clean a source entry when all remaining versions are either migrated or lifecycle-expired.
- Object-lock or replication-protected versions are not wrongly treated as cleanup-safe.
- Status accurately reflects residual expired-source entries if they are intentionally retained.
**Required tests:**
- Bucket with one migrated version plus one lifecycle-expired version.
- Expired version protected by object lock must not be cleaned.
- Replication-pending version must not be treated as cleanup-safe.
**Dependencies:** Coordinate with F07 metadata tests for object lock and replication.
---
### F09: Handle rebalance overwrite races using explicit equivalence checks
**Source:** P2.3
**Decision:** Fix likely required; first confirm exact race cases with tests.
**Target priority:** Medium.
**Problem:** RustFS treats `DataMovementOverwriteErr` as non-transient during rebalance, while MinIO ignores acceptable overwrite races. A strict failure can prevent rebalance convergence when the target already has an equivalent version.
**Primary files:**
- `crates/ecstore/src/rebalance.rs`
- `crates/ecstore/src/data_movement.rs`
- `crates/ecstore/src/store/object.rs`
**Preferred fix direction:**
- Keep unsafe source-equals-target cases as failures.
- If the target version already exists, compare version ID, ETag, size, mod time, checksum, and key metadata.
- Count as complete only when equivalence passes.
- Add last-error/status text when overwrite is unsafe.
**Acceptance criteria:**
- Equivalent target version allows rebalance to continue.
- Non-equivalent target version fails and does not clean source.
- Behavior is documented as compatible with MinIO's acceptable overwrite race handling.
**Required tests:**
- Rebalance overwrite with equivalent target version.
- Rebalance overwrite with mismatched checksum or metadata.
- Rebalance overwrite where source and target pool are the same and no equivalent target exists.
**Dependencies:** F07 defines the metadata equivalence fields.
---
### F10: Add source cleanup preflight verification before deleting migrated entries
**Source:** A2
**Decision:** Fix recommended as defense-in-depth.
**Target priority:** Medium.
**Problem:** After versions are migrated, source cleanup deletes the source prefix based on the version list read before migration. A final verification would protect against state propagation failures, recovery races, or repair processes changing metadata before cleanup.
**Primary files:**
- `crates/ecstore/src/rebalance.rs`
- `crates/ecstore/src/pools.rs`
- `crates/ecstore/src/set_disk.rs`
**Preferred fix direction:**
- Before source cleanup, re-read source metadata or a generation marker.
- Verify the version set scheduled for deletion equals the version set that was migrated or intentionally ignored.
- If the version set changed, defer/retry the entry instead of deleting the prefix.
**Acceptance criteria:**
- Cleanup does not delete versions created or discovered after the migration scan.
- Changed source metadata causes a retry/defer state with a clear status error.
- The guard applies to rebalance and decommission where source prefix deletion is used.
**Required tests:**
- Simulate source metadata changing between migration and cleanup.
- Simulate peer state mismatch where a write lands on a rebalancing/decommissioning pool.
- Confirm unchanged metadata still allows cleanup.
**Dependencies:** Works best after F03 and F05 reduce state propagation failures.
---
## Phase 3: Cleanup Semantics, Observability, and Hardening
### F11: Improve rebalance cleanup failure handling and reporting
**Source:** A1
**Decision:** Fix recommended.
**Target priority:** Medium.
**Problem:** Rebalance source cleanup failure is converted to a warning and the task can still complete. Only a count and last warning are preserved.
**Primary files:**
- `crates/ecstore/src/rebalance.rs`
- `rustfs/src/admin/handlers/rebalance.rs`
- Rebalance status DTOs and serialization paths
**Preferred fix direction:**
- Preserve a bounded list of cleanup failures, not only the last one.
- Add a clear terminal or partial terminal state when cleanup warnings exist.
- Decide whether cleanup failure should:
- defer the entry for retry, or
- allow completion but mark the pool as `completed_with_cleanup_warnings`.
**Acceptance criteria:**
- Admin status reports cleanup warning count and representative object list.
- Operators can identify which objects need manual cleanup or retry.
- Rebalance terminal state is not misleading when source data remains.
**Required tests:**
- Multiple cleanup failures preserve count and bounded object details.
- Status response includes cleanup warning data.
- Completion state reflects cleanup warnings.
**Dependencies:** Coordinate with F10 if cleanup failures become retryable.
---
### F12: Add structured audit fields for rebalance and decommission operations
**Source:** P3.2
**Decision:** Fix recommended.
**Target priority:** Low.
**Problem:** Critical admin operations do not consistently log actor, remote address, request ID, pool indices, peer host, result, and partial failure state.
**Primary files:**
- `rustfs/src/admin/handlers/rebalance.rs`
- `rustfs/src/admin/handlers/pools.rs`
- `crates/ecstore/src/notification_sys.rs`
- `crates/ecstore/src/rpc/peer_rest_client.rs`
- Logging guardrail scripts if applicable
**Preferred fix direction:**
- Standardize structured fields for start, stop, cancel, status-affecting propagation, and peer RPC failures.
- Mask sensitive actor material.
- Use warn/error for partial failure rather than info-only logs.
**Acceptance criteria:**
- Start/stop/cancel logs include actor, request ID, pool/rebalance ID, peer, result, and error when present.
- Logs do not expose secrets.
- Partial failures are searchable by stable event fields.
**Required tests:**
- Log-capture tests for success, rejection, and partial failure.
- Guardrail check if logging conventions are enforced by script.
**Dependencies:** F03 and F05 define partial-failure semantics.
---
### F13: Harden persisted rebalance and pool metadata decoding
**Source:** P3.3
**Decision:** Fix recommended after compatibility review.
**Target priority:** Low.
**Problem:** Persisted rebalance and pool metadata can accept unknown fields silently. This may be acceptable for compatibility, but it weakens corruption and typo detection.
**Primary files:**
- `crates/ecstore/src/rebalance.rs`
- `crates/ecstore/src/pools.rs`
- Metadata fixture tests near existing metadata tests
**Preferred fix direction:**
- For formats that can be strict, add `deny_unknown_fields`.
- Where backward compatibility requires leniency, detect and warn on unknown fields during legacy decode.
- Add explicit validation for conflicting terminal states.
**Acceptance criteria:**
- Unknown fields either fail decode or produce a warning through a documented compatibility path.
- Missing critical fields fail safely.
- Conflicting states such as completed plus running are rejected.
**Required tests:**
- Unknown field fixture.
- Missing critical field fixture.
- Conflicting terminal state fixture.
- Legacy fixture proving backward compatibility.
**Dependencies:** None.
---
### F14: Decide and document rebalance completion threshold behavior
**Source:** P3.4
**Decision:** Design decision required.
**Target priority:** Low.
**Problem:** RustFS uses a stricter rebalance completion goal than MinIO's tolerance-based behavior. This may increase movement work and operational time.
**Primary files:**
- `crates/ecstore/src/rebalance.rs`
- Admin status documentation if present
- Tests around rebalance goal calculation
**Preferred fix direction:**
- Decide whether RustFS should:
- match MinIO tolerance;
- keep strict behavior and document it;
- make tolerance configurable with a conservative default.
- Avoid configurability unless a real operational requirement exists.
**Acceptance criteria:**
- Completion behavior is intentional and covered by tests.
- Status/ETA reporting does not imply MinIO-compatible tolerance if strict behavior remains.
- Large-pool behavior is covered by a deterministic unit test.
**Required tests:**
- Rebalance goal just below target.
- Rebalance goal within MinIO-like tolerance.
- Empty queue completion path remains valid.
**Dependencies:** None.
---
## Cross-Cutting Test Matrix
Each fix should add focused tests, but the following end-to-end scenarios should also be covered before enabling production use:
1. Rebalance a versioned object with normal versions, delete marker, and source cleanup.
2. Rebalance a remote tiered version or verify it is intentionally skipped.
3. Decommission a pool while peer reload fails.
4. Restart with both active decommission metadata and rebalance metadata.
5. Stop rebalance while a large multipart migration is in progress.
6. Migrate a large multipart object without memory proportional to part size.
7. Migrate objects with checksum, replication state, retention, and legal hold metadata.
8. Trigger source cleanup failure and verify admin status exposes it.
9. Trigger overwrite race with equivalent and non-equivalent target versions.
10. Decommission lifecycle-expired versions with object-lock and replication constraints.
## Suggested Execution Strategy
1. Start with F01 and F02 because they protect object version semantics.
2. Implement F03, F04, and F05 before broader rollout because they define distributed operation safety.
3. Implement F06 before running large-scale data movement tests.
4. Implement F07 through F11 as compatibility, convergence, and observability hardening.
5. Keep F12 through F14 as final hardening unless release criteria require them earlier.
Each fix should be developed in a separate PR unless two adjacent fixes share the same core implementation. In particular:
- F01 and F09 should not be merged together unless the equivalence helper is clearly isolated.
- F06 and F07 may be combined only if streaming part migration and checksum preservation share the same reader changes.
- F03 and F05 may share peer failure aggregation utilities, but their admin behavior should be tested separately.
docs/superpowers/plans/site-replication-hardening-pr.md
+105
View File
@@ -0,0 +1,105 @@
> **Archived implementation plan/tracker** — moved from `docs/architecture/` (2026-07).
> Kept for history; not maintained. File paths inside may reflect the pre-#3929
> module layout (e.g. `crates/ecstore/src/rebalance.rs` is now
> `crates/ecstore/src/store/rebalance.rs`; `set_disk.rs` is now `set_disk/`).
## Related Issues
N/A
## Summary of Changes
This PR hardens the site replication control plane in small, reviewable commits. The work starts with guardrails identified during the MinIO comparison and multi-review pass: credential redaction, stricter diagnostic authorization, remove cleanup, and clearer status diagnostics.
### Implementation Log
1. `docs: plan site replication hardening`
- Purpose: establish the execution plan and PR draft before code changes.
- Reason: the site replication follow-up touches security, compatibility, operations, and tests, so reviewers need a durable record of each step and why it exists.
2. `fix: redact site replication target secrets`
- Purpose: prevent bucket replication target credentials from leaking through admin listing, bucket metadata export, and debug formatting.
- Reason: site replication stores the `site-replicator-0` secret in bucket targets today; keeping the internal storage format intact while redacting external surfaces reduces immediate blast radius without changing replication target loading.
3. `fix: require operation access for replication diagnostics`
- Purpose: require operation-level admin authorization for site replication diagnostic POST endpoints and clamp netperf duration.
- Reason: `devnull` and `netperf` read request bodies and exercise diagnostic work, so granting them through read-only site replication info permission was broader than intended.
4. `fix: clean site replication targets on remove`
- Purpose: remove bucket targets and `site-repl-*` replication rules that point at a removed deployment before completing pending remove.
- Reason: removing a site only from site replication state leaves bucket-level replication configuration behind, so object replication can continue attempting to reach a removed peer.
5. `feat: expose site replication status diagnostics`
- Purpose: add machine-readable peer fetch errors and pending operation progress to site replication status.
- Reason: operators previously saw only `Unknown` sync state or logs when peer metainfo fetches failed or remove/rotation was pending, which made automation and troubleshooting unnecessarily opaque.
6. `docs: expand site replication hardening scope`
- Purpose: keep the single-PR plan aligned with the expanded request to include MinIO compatibility, add preflight, bootstrap, lifecycle compatibility, retry, and repair work.
- Reason: the remaining work is larger than the first hardening pass, so reviewers need to see the intended sequence before more behavior changes land.
7. `fix: align site replication peer paths with minio`
- Purpose: send peer site-replication calls over MinIO-compatible admin paths and accept the MinIO-style peer join route.
- Reason: RustFS already accepts `/minio/admin` as an alias, so using MinIO wire paths improves mixed-cluster compatibility without removing RustFS route support.
8. `fix: validate site replication add topology`
- Purpose: preflight add requests with remote metainfo and IDP settings before creating service accounts, joining peers, or persisting state.
- Reason: MinIO rejects unsafe add topologies up front; RustFS now rejects duplicate deployment IDs, missing local deployment, IDP mismatch, multiple non-empty initial sites, and peers already configured with a different site-replication set.
9. `feat: bootstrap site replication metadata on add`
- Purpose: replay the local site replication snapshot to joined peers during add, before object backfill starts.
- Reason: existing hooks only replicate changes after site replication is enabled; existing policies, users, groups, bucket metadata, and buckets must be bootstrapped so a newly joined site does not start with an incomplete control-plane snapshot.
10. `fix: align lifecycle replication with minio semantics`
- Purpose: replicate bucket lifecycle metadata only when site replication has `replicate-ilm-expiry` enabled.
- Reason: MinIO keeps lifecycle expiry replication opt-in; RustFS should not replicate lifecycle metadata by default during live bucket metadata hooks or add-time bootstrap.
11. `feat: add site replication retry and repair MVP`
- Purpose: persist failed peer replication attempts as retry metadata, expose retry counts in status, and add an operation-level repair endpoint that replays the current local snapshot.
- Reason: transient peer failures were previously only visible through logs and required manual reconstruction; a durable, secret-safe queue plus repair replay gives operators a concrete recovery path without persisting sensitive request payloads.
12. `fix: satisfy site replication pre-pr checks`
- Purpose: remove a redundant MinIO admin-path branch and construct retry test fixtures directly.
- Reason: the full pre-PR gate enforces clippy warnings as errors; this keeps the final branch green without changing site replication behavior.
## Verification
Baseline started from `origin/main` at `758677da`, then was rebased onto the latest `origin/main` at `1d6a8259`.
- Passed: `cargo fmt --all --check`
- Passed: `cargo test -p rustfs-ecstore bucket_target --lib`
- Passed: `cargo test -p rustfs-madmin site_replication --lib`
- Passed: `cargo test -p rustfs route_policy --lib`
- Passed: `cargo test -p rustfs site_replication --lib`
- Passed: `cargo test -p rustfs route_policy --lib`
- Passed: `cargo test -p rustfs site_replication --lib`
- Passed: `cargo fmt --all`
- Passed: `cargo fmt --all --check`
- Passed: `cargo test -p rustfs site_replication --lib`
- Passed: `cargo fmt --all --check`
- Passed: `cargo test -p rustfs-madmin site_replication --lib`
- Passed: `cargo test -p rustfs route_policy --lib`
- Passed: `cargo test -p rustfs route_registration_test --lib`
- Passed: `cargo test -p rustfs site_replication --lib`
- Passed: `cargo fmt --all --check`
- Passed: `cargo test -p rustfs site_replication --lib`
- Passed: `cargo fmt --all --check`
- Passed: `cargo test -p rustfs site_replication --lib`
- Passed before the final main rebase: `make pre-pr`
- Not completed after rebasing onto `1d6a8259`: `make pre-pr` was interrupted during clippy at request time; formatting, unsafe-code, architecture, and logging guardrails had already passed in that run.
## Impact
The planned implementation is intended to reduce credential exposure, require stronger permission for diagnostic write-like endpoints, clean removed site replication targets, and make replication status more actionable. Compatibility-sensitive behavior is called out per commit in the implementation log.
The credential redaction step changes admin/export visibility of bucket target secrets. Stored target configuration is not migrated or reformatted by this PR step.
The diagnostic authorization step intentionally changes required permission for `POST /site-replication/devnull` and `POST /site-replication/netperf` from `SiteReplicationInfoAction` to `SiteReplicationOperationAction`.
The remove cleanup step preserves user-managed replication rules and non-replication targets, and only prunes targets/rules associated with removed site replication deployment IDs.
The status diagnostics step adds optional `PeerErrors` and `PendingOperation` fields to the status response. Existing clients can ignore them, while automation can use them to distinguish peer reachability/auth failures from content mismatch.
## Additional Notes
This draft PR includes bootstrap, durable retry, and repair work in one reviewable branch. It should remain draft until CI or a full local pre-PR run confirms the latest default branch baseline.
The expanded single-PR scope also includes MinIO wire-contract bootstrap validation, durable site-replication retry, full add-time IAM/bootstrap sync, lifecycle compatibility, and site-level repair.
The peer path compatibility step maps outbound peer requests to `/minio/admin/v3/site-replication/...`. Peer join uses the encrypted MinIO payload contract, while internal peer metadata/IAM/remove/edit requests remain plain JSON to match MinIO handlers.
The add preflight step performs remote reads before state mutation, so add fails earlier when a peer cannot report metainfo or IDP settings.
The bootstrap step replays IAM policies, built-in users with stored secret material, group membership/status, policy mappings, bucket creation, and bucket metadata through the existing peer replication handlers. It does not synthesize ordinary service-account secrets when the snapshot does not contain them; the site-replicator service account remains distributed through the join flow.
Lifecycle metadata is now skipped by default in site replication hooks and bootstrap snapshots unless the site replication peer state enables `replicate-ilm-expiry`.
The retry MVP intentionally stores peer/path/error/count metadata only. Repair regenerates payloads from the current local snapshot, avoiding persistent storage of user secrets, service-account secrets, or bucket target credentials in the retry queue.