feat(rc-replication)!: add target checks and bucket resync lifecycle#288
Conversation
e9ad04d to
e424398
Compare
0ffb003 to
0034965
Compare
|
Validation update for restacked commit
The branch is exactly one #1410 delta commit over the repaired #287 head. The Windows overflow-fixture repair inherited from #287 is confirmed here as well: the Windows test suite completed normally. Live RustFS container validation remains unavailable because the current environment has no Docker daemon. |
overtrue
left a comment
There was a problem hiding this comment.
Review: Request changes
bucket replication check, resync start, and resync status are implemented as signed S3 extension calls with a new output-v3 replication_operations family and unusually thorough tests. The safety-critical mechanics are well done: both destructive ops require --yes before any alias/network setup; the production client disables redirects and does not retry the PUT (both wire-tested); responses are hard-bounded to 1 MiB (declared and chunked); server error bodies have credentials redacted; cancel is deliberately omitted with rationale. The one substantive problem is how the --older-than blast-radius bound is serialized onto the wire.
Findings
[MAJOR] crates/s3/src/client.rs (start_bucket_replication_resync, the older-than query push) — the cutoff is serialized with humantime::format_duration, producing a form the server likely rejects
- Why:
humantime::format_durationemits full-word, space-separated units and decomposes into 30.44-day "months" and 365.25-day "years":30d→30days,7d10h31s→7days 10h 31s,31d→1month 13h 26m 24s. The only wire test covers1h— which is also the only category that survives a Go/MinIO-styletime.ParseDuration(it rejectsd/days/month/year). Every day-scale value the docs advertise (--older-than 30d, thereplicate.mdexamples) serializes to a form such a parser rejects. This bound is the blast-radius control on an expensive, disruptive whole-bucket re-replication: if the server rejects it, the documented commands fail (and a server 400 currently maps toconflictwith the misleading suggestion "Review the replication target configuration," not "bad duration"); if a server instead ignored an unparsedolder-than, it would resync more than the operator intended. - Fix: Serialize an unambiguous canonical form the server contract specifies (integer seconds, or Go-style e.g.
720h); add a wire round-trip test for a day/month-scale duration; align the help/doc examples to the server's grammar.
[MINOR] crates/cli/src/commands/replicate.rs (parse_resync_duration) — the range guard only rejects > i64::MAX seconds, and a test enshrines 1000y as valid
- Why: A Go-heritage server storing durations as
i64nanoseconds (time.Duration) overflows at ~292 years, so1000yis out of range yet accepted client-side. - Fix: Clamp against the real server limit (~292y if ns-int64) and drop/adjust the
1000y-is-ok assertion.
[NIT] crates/cli/src/commands/replicate.rs (setup_replication_operation_client) — the new --force flags gate on capabilities().replication, but S3Client::capabilities() hardcodes replication: true (client.rs:2624)
- Why: With no network probe the gate never fires, so
--forcehas no observable effect for these commands, yet its help ("even if generic replication capability detection fails") implies detection can fail here. - Fix: Drop
--force/the gate for these S3-route commands, or make the help accurate.
Test coverage
Strong (exit-code scenarios for confirmation/not-found/usage/unsupported, credential-redaction, oversized-body, no-retry, no-redirect, state preservation, schema fixtures, help contract). Gap: no wire/round-trip test for the day/month-scale --older-than values the docs promote (only 1h); no standalone golden fixture for check-success or resync_start-success.
Nice work
Credential redaction in extension-error mapping; strict 1 MiB bound on both declared and chunked bodies; redirects disabled in the production client and the PUT proven non-retrying/non-redirecting via wire tests; careful preservation of empty/future/unknown server states; cancel intentionally not implemented with a documented rationale.
Correction — retracting the MAJOR (and the related MINOR)I'm withdrawing the central finding of my previous review. It was wrong, and I apologize for the noise. The MAJOR ("
The premise that the server used Go-style The related MINOR (" Still valid — the NIT: Net verdict for this PR is therefore APPROVE-WITH-NITS, not request-changes. The strong parts I noted still stand (credential redaction in extension-error mapping, strict 1 MiB response bounding on declared + chunked bodies, redirects disabled and the PUT proven non-retrying via wire tests, |
Retracted — the older-than MAJOR was a false positive; the server parses older-than with humantime::parse_duration (inverse of the client's humantime::format_duration), so it round-trips. See correction comment. Net: approve-with-nits.
Closes rustfs/backlog#1410
Depends on #287.
Background
RustFS beta.10 exposes signed S3 extension routes for actively checking configured replication targets and for starting or reading bucket resync state. rc did not expose these workflows, and the server semantics include important safety and durability caveats that must not be hidden.
Solution
rc bucket replication check ... --yesactive validationrc bucket replication resync start ... --yeswith target ARN, older-than, and optional caller reset IDrc bucket replication resync statuswith optional target selectionThe documentation explicitly records beta.10 behavior: active checks may leave a remote probe object after target-side failure; check failures expose only the first server-selected error; overlapping resync starts are not atomically conflict-checked; error details are not reliably persisted; unknown target status returns an empty list; and no public cancellation route exists.
BREAKING CHANGE
The protected CLI reference contract gains check and resync lifecycle commands, and output-v3 gains the
replication_operationsfamily. Existing output-v1 and output-v2 contracts are unchanged. The output-v3 schema distinguishes the exactly-one, nonempty reset ID returned by start from legitimate empty reset IDs in persisted status.Validation
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace(1194 passed, 0 failed, 1 documented ignored doctest)All local checks passed. Live container validation was not run because no Docker daemon is available in the current environment.