mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-15 09:33:13 +00:00
fix(site-replication): route every state RMW through the locked transaction (#6097)
* fix(site-replication): route every state RMW through the locked transaction P1-15 PR2 (rustfs/backlog#1796, batch B2 of rustfs/backlog#1675), the follow-up promised by rustfs/rustfs#5882. PR1 left ~26 read-modify-write call sites on config/site-replication/state.json in the pre-transaction shape: a process-local mutex around load / mutate / save, each IO taking its own object lock. Nothing held a distributed lock across the whole sequence, so two nodes of one site still lost each other's updates, and the transitional mutex kept the old shape available to copy. Every remaining RMW now runs inside update_site_replication_state; read-only sites use load_site_replication_state, whose object read comes with the object-level read lock. SITE_REPLICATION_STATE_LOCK and its owner helper are gone, together with their architecture-guard allowlist entry and inventory row. The multi-stage flows (add / edit / peer join / peer edit / remove / rotate) keep their updated_at and pending-id CAS, but the CAS now runs inside the transaction that writes, against the state that transaction loaded. Peer probes, IAM work and fan-outs run between transactions and hold no lock at all — the add no longer blocks every writer of the site across its peer join round trips, and it re-checks the precondition right after the capability probes so the common race is rejected before any IAM write or remote join. When the add's commit CAS still fails, the error says the peers may already be joined and that re-running the add reconverges. The add adopts only the fields it computed (exhaustive destructure — adding a state field is a compile error until classified); fields owned by writers that do not bump updated_at keep their freshly loaded values. Ordering of peer-edit deliveries now rests on the generation fence landed in PR1 rather than on a guard that could never order two nodes: the add's finalize fan-out carries the generation allocated in its commit. An accepted peer join PRESERVES the applied-generation high-water marks — join fan-outs are routine (adds and rotations both deliver SRPeerJoin to existing peers), so wiping them would let stalled older edits land after any join; the unilateral-removal rejoin misfence that a wipe would have patched is pre-existing since the fence landed and needs an epoch in the fence instead. The rotation handler now takes the lifecycle guard: the background service-account reconciler runs its repair under a lifecycle try-acquire, and its pending-rotation precheck is only sound if a rotation cannot start mid-repair — an exclusion the removed process mutex used to provide as a side effect. update_site_replication_state_when_changed adds persist-or-skip so ack markers and pending-clearing paths stop rewriting the object on a miss — load-bearing, because the shared persist helper clears the whole object for a ≤1-peer pending-free state — and save_site_replication_state is now cfg(test): the pre-P1-15 shape can no longer be written in production code. No on-disk format change. Verification: cargo nextest run -p rustfs -E 'test(/admin::handlers::site_replication::/)' (181 passed); site-replication dual/three-node e2e (13 passed); cargo clippy -p rustfs --all-targets -D warnings; make pre-commit. Mutation checks: dropping the state-object lock from the boundary reds the separate-node concurrency tests; flipping a persist-or-skip miss to a persist reds test_missed_pending_clear_must_not_rewrite_the_state_object. Reviewed by three independent adversarial passes (correctness/concurrency, security/compatibility, simplicity/test-coverage); their confirmed findings are folded in. * fix(site-replication): serialize peer-join admission around its IAM write Review follow-up (overtrue): two joins accepted by the same node could interleave as "A checks a stale snapshot and pauses reading its body, B applies secret B and commits, A resumes, overwrites IAM with secret A, and A's commit is refused as superseded" — the persisted state advertised B's contract while IAM only accepted A's secret, failing every peer control-plane call. The pre-P1-15 process mutex serialized same-node joins end to end; removing it dropped that exclusion. admit_peer_join now runs the staleness check, the IAM upsert and the state commit under the lifecycle guard, with the authoritative pre-check taken against a load under that guard BEFORE IAM changes anything. The closing transaction still re-checks staleness: the guard is process-local (exactly as far as the old mutex reached) and the state-object lock arbitrates joins accepted by different nodes. The body is fully read before the guard so a stalling sender cannot block add/remove/rotate/reconciler. The IAM step is injected, and the gated-body regression test reproduces the review's ordering: join A is held mid-IAM while a newer join B arrives; B must wait at the guard, and both IAM order and the final persisted state end on B. Mutation-verified: removing the lifecycle guard from admit_peer_join turns the test red. Verification: cargo nextest run -p rustfs -E 'test(/admin::handlers::site_replication::/)' (182 passed); site-replication dual/three-node e2e (13 passed); cargo clippy -p rustfs --all-targets -D warnings; make pre-commit. * fix(site-replication): fence peer-join admission across nodes Review follow-up (overtrue, round 2): the lifecycle guard only serializes joins within one process. Node A could pass the staleness check for an older T1, node B write secret B to IAM and commit a newer T2, and node A then overwrite IAM with secret A while its own state commit is refused as superseded — state advertising T2's contract while IAM only accepts A's secret. The admission (staleness check -> IAM upsert -> state commit) now also runs under a distributed join-admission lock, a namespace-lock key with no backing object, following the repair execution lock's pattern — including its nesting of config-object locks (admission -> state), and delegating crash safety to the lock subsystem's lease expiry instead of a hand-rolled TTL. The staleness check runs against a load taken inside the lock, before IAM changes anything, so a superseded join exits without touching IAM. The closing transaction keeps its re-check for defence in depth and for old-version nodes that do not take the admission lock during a rolling upgrade (that mixed-version window keeps today's behavior and closes when the upgrade completes). admit_peer_join_across_nodes is the admission minus the process-local lifecycle guard — exactly what a second node runs — and the new separate-nodes regression test drives it directly with join A gated mid-IAM: join B must wait at the distributed lock, and both the IAM write order and the final persisted state end on B. Mutation-verified: removing the admission lock turns the test red while the same-node test (which drives the full admit_peer_join) stays green. Verification: cargo nextest run -p rustfs -E 'test(/admin::handlers::site_replication::/)' (183 passed); site-replication dual/three-node e2e (13 passed); cargo clippy -p rustfs --all-targets -D warnings; make pre-commit.
This commit is contained in:
唐小鸭
@@ -111,7 +111,7 @@ inventory. Generic function-local names such as `CACHE`, `LOCK`, `INIT`, and
|
||||
| `GET_OBJECT_BUFFER_THRESHOLD_WARNED`, `GET_READER_STREAM_BUFFER_SIZE_OVERRIDE`, function-local `ENABLED`, `OBJECT_SEEK_SUPPORT_THRESHOLD`, `OBJECT_SEEK_SUPPORT_CONCURRENCY_THRESHOLDS` | `rustfs/src/app/object_usecase.rs` | Cache or constant / owner-local cache | Object GET/seek tuning caches and warning guards stay private to object usecase helpers. |
|
||||
| `SUPPORTED_HEADERS` | `rustfs/src/storage/options.rs` | Cache or constant / owner-local constant | Supported-header lookup state stays private to storage option parsing. |
|
||||
| `AUDIT_TARGET_SPECS`, `NOTIFICATION_TARGET_SPECS` | `rustfs/src/admin/handlers/audit.rs`, `rustfs/src/admin/handlers/event.rs`, `rustfs/src/admin/handlers/plugins_instances.rs` | Cache or constant / owner-local constant | Admin target descriptor tables stay private to their handler owners. |
|
||||
| `SITE_REPLICATION_PEER_CLIENT`, `SITE_REPLICATION_STATE_LOCK` | `rustfs/src/admin/handlers/site_replication.rs` | Process-global owner-local cache / guard | Site-replication peer client cache and state lock stay private to site-replication handlers. |
|
||||
| `SITE_REPLICATION_PEER_CLIENT` | `rustfs/src/admin/handlers/site_replication.rs` | Process-global owner-local cache | Site-replication peer client cache stays private to site-replication handlers. The state RMW transaction holds no process-local mutex — see `rustfs/src/admin/site_replication_state.rs`. |
|
||||
| `AUDIT_MODULE_ENABLED`, `NOTIFY_MODULE_ENABLED`, `PERSISTED_NOTIFY_MODULE_ENABLED`, `PERSISTED_AUDIT_MODULE_ENABLED`, `PERSISTED_MODULE_SWITCH_CONFIGURED` | `rustfs/src/server/audit.rs`, `rustfs/src/server/event.rs`, `rustfs/src/server/module_switch.rs` | Process-global owner-local toggles | Audit/notify module snapshots stay private to the server module switch owners. |
|
||||
| `DELETE_TAIL_TOTAL`, `DELETE_CLEANUP_TOTAL`, `DELETE_REPLICATION_TOTAL`, `DELETE_NOTIFY_TOTAL` | `rustfs/src/delete_tail_activity.rs` | Process-global owner-local counters | Delete-tail activity counters stay private behind delete-tail activity helpers. |
|
||||
| `EMBEDDED_SERVER_STARTED` | `rustfs/src/startup_lifecycle.rs` | Process-global owner-local guard | Embedded startup single-start protection stays private to startup lifecycle. |
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -18,28 +18,21 @@
|
||||
//! `config/site-replication/state.json` is mutated by read-modify-write
|
||||
//! sequences spread over many call sites: admin handlers, the retry-event
|
||||
//! writers on every hook broadcast path, and the service-side reload driven
|
||||
//! over node RPC. Historically only some of them held the process-local
|
||||
//! mutex and none held a distributed lock across the whole RMW, so
|
||||
//! concurrent writers overwrote each other (single-process for the unlocked
|
||||
//! writers, cross-node for everyone).
|
||||
//! over node RPC.
|
||||
//!
|
||||
//! `with_site_replication_state_lock` is the single transaction boundary:
|
||||
//! it holds the process-local mutex AND the distributed config-object write
|
||||
//! lock (the pattern proven by the repair state,
|
||||
//! `update_site_replication_repair_state`) for the duration of the caller's
|
||||
//! closure. All IO inside the closure must use the `*_no_lock` config
|
||||
//! helpers — the locked variants would self-deadlock on the same object
|
||||
//! lock. Do not perform peer network calls or take other config locks
|
||||
//! it holds the distributed config-object write lock (the pattern proven by
|
||||
//! the repair state, `update_site_replication_repair_state`) for the
|
||||
//! duration of the caller's closure. The object lock is the sole mechanism —
|
||||
//! it is the only thing that can serialize two nodes of the same site, so a
|
||||
//! process-local lock must never be reintroduced in front of it as if it
|
||||
//! added protection. All IO inside the closure must use the `*_no_lock`
|
||||
//! config helpers — the locked variants would self-deadlock on the same
|
||||
//! object lock. Do not perform peer network calls or take other config locks
|
||||
//! inside the closure.
|
||||
//!
|
||||
//! The process-local mutex is transitional: call sites still outside this
|
||||
//! primitive serialize against migrated ones through it. Once every RMW
|
||||
//! call site goes through here (P1-15 PR2) it will be removed, leaving the
|
||||
//! object lock as the only mechanism.
|
||||
//!
|
||||
//! Lock order (unchanged from the historical comment next to the mutex):
|
||||
//! lifecycle -> bucket operation -> repair admission -> state (process
|
||||
//! mutex, then state object lock) -> per-bucket metadata.
|
||||
//! Lock order: lifecycle -> bucket operation -> repair admission
|
||||
//! -> state object lock -> per-bucket metadata.
|
||||
|
||||
use crate::admin::storage_api::runtime::ECStore;
|
||||
use crate::admin::storage_api::s3::{S3Error, S3ErrorCode, S3Result};
|
||||
@@ -53,24 +46,8 @@ use super::runtime_sources::current_object_store_handle;
|
||||
/// byte-level tolerant reload on the service side.
|
||||
pub(crate) const SITE_REPLICATION_STATE_PATH: &str = "config/site-replication/state.json";
|
||||
|
||||
/// Transitional process-local mutex — see the module docs. Stays private to
|
||||
/// this module (owner-local static, enforced by
|
||||
/// `scripts/check_architecture_migration_rules.sh`); callers go through
|
||||
/// [`site_replication_state_process_guard`].
|
||||
static SITE_REPLICATION_STATE_LOCK: std::sync::LazyLock<tokio::sync::Mutex<()>> =
|
||||
std::sync::LazyLock::new(|| tokio::sync::Mutex::new(()));
|
||||
|
||||
/// Owner helper for the transitional process mutex: the RMW call sites in
|
||||
/// `handlers::site_replication` that PR2 has not migrated to
|
||||
/// [`with_site_replication_state_lock`] yet hold this guard so they stay
|
||||
/// mutually exclusive with the migrated ones. Removed together with the
|
||||
/// mutex once every call site runs inside the transaction boundary.
|
||||
pub(crate) async fn site_replication_state_process_guard() -> tokio::sync::MutexGuard<'static, ()> {
|
||||
SITE_REPLICATION_STATE_LOCK.lock().await
|
||||
}
|
||||
|
||||
/// Run `operation` under the site-replication state transaction boundary:
|
||||
/// process mutex first, then the distributed state-object write lock.
|
||||
/// the distributed state-object write lock.
|
||||
pub(crate) async fn with_site_replication_state_lock<T, F, Fut>(operation: F) -> S3Result<T>
|
||||
where
|
||||
T: Send + 'static,
|
||||
@@ -83,21 +60,11 @@ where
|
||||
|
||||
/// Context-store variant for callers that resolve their store from an
|
||||
/// explicit [`AppContext`] (the service-side reload driven over node RPC).
|
||||
///
|
||||
/// This is the whole boundary: a state-object write lock serializes writers
|
||||
/// in *different* processes, which is what two nodes of one site are and
|
||||
/// what a process mutex could never cover.
|
||||
pub(crate) async fn with_site_replication_state_lock_on<T, F, Fut>(store: Arc<ECStore>, operation: F) -> S3Result<T>
|
||||
where
|
||||
T: Send + 'static,
|
||||
F: FnOnce() -> Fut + Send + 'static,
|
||||
Fut: std::future::Future<Output = S3Result<T>> + Send + 'static,
|
||||
{
|
||||
let _process_guard = SITE_REPLICATION_STATE_LOCK.lock().await;
|
||||
with_site_replication_state_object_lock(store, operation).await
|
||||
}
|
||||
|
||||
/// The distributed half of the boundary on its own: the state-object write
|
||||
/// lock, without the process mutex. This is the only thing that serializes
|
||||
/// writers in *different* processes (the mutex cannot), so it is also what
|
||||
/// the separate-nodes regression test drives.
|
||||
pub(crate) async fn with_site_replication_state_object_lock<T, F, Fut>(store: Arc<ECStore>, operation: F) -> S3Result<T>
|
||||
where
|
||||
T: Send + 'static,
|
||||
F: FnOnce() -> Fut + Send + 'static,
|
||||
|
||||
@@ -4033,7 +4033,7 @@ if [[ -s "$ECSTORE_REMOTE_TIER_DELETE_STATE_BYPASS_HITS_FILE" ]]; then
|
||||
report_failure "remote tier delete state access must stay behind ECStore tier sweeper owner helpers: $(paste -sd '; ' "$ECSTORE_REMOTE_TIER_DELETE_STATE_BYPASS_HITS_FILE")"
|
||||
fi
|
||||
|
||||
RUSTFS_OWNER_LOCAL_STATIC_NAMES='(KEYSTONE_AUTH|KEYSTONE_MAPPER|KEYSTONE_CONFIG|LICENSE_STATE|LICENSE_VERIFIER|CPU_CONT_GUARD|PROFILING_CANCEL_TOKEN|MEMORY_SYSTEM|DIAL9_TELEMETRY_GUARD|DISPLAY_CONFIG_SNAPSHOT|GLOBAL_CONFIG_SNAPSHOT|BUFFER_CONFIG_SINGLETON|BUFFER_PROFILE_ENABLED|LEGACY_CREDENTIAL_WARNED_KEYS|CONSOLE_CONFIG|ACTIVE_HTTP_REQUESTS|USE_STARSHARD_CACHE|BUCKET_CACHE_SMALL|BUCKET_CACHE_LARGE|GLOBAL_SSE_DEK_PROVIDER|SSE_TEST_LOCK|AUTH_FS|LOCK_STATS|DEADLOCK_DETECTOR|GET_OBJECT_BUFFER_THRESHOLD_WARNED|GET_READER_STREAM_BUFFER_SIZE_OVERRIDE|OBJECT_SEEK_SUPPORT_THRESHOLD|OBJECT_SEEK_SUPPORT_CONCURRENCY_THRESHOLDS|SUPPORTED_HEADERS|SITE_REPLICATION_PEER_CLIENT|SITE_REPLICATION_STATE_LOCK|AUDIT_MODULE_ENABLED|NOTIFY_MODULE_ENABLED|PERSISTED_NOTIFY_MODULE_ENABLED|PERSISTED_AUDIT_MODULE_ENABLED|PERSISTED_MODULE_SWITCH_CONFIGURED|DELETE_TAIL_TOTAL|DELETE_CLEANUP_TOTAL|DELETE_REPLICATION_TOTAL|DELETE_NOTIFY_TOTAL|EMBEDDED_SERVER_STARTED|TEST_OUTBOUND_TLS_GENERATION|TEST_REMAINING_FAILURES|CAPACITY_DIRTY_SCOPE_ENV|CAPACITY_DIRTY_SCOPE_INIT|GLOBAL_ENV)'
|
||||
RUSTFS_OWNER_LOCAL_STATIC_NAMES='(KEYSTONE_AUTH|KEYSTONE_MAPPER|KEYSTONE_CONFIG|LICENSE_STATE|LICENSE_VERIFIER|CPU_CONT_GUARD|PROFILING_CANCEL_TOKEN|MEMORY_SYSTEM|DIAL9_TELEMETRY_GUARD|DISPLAY_CONFIG_SNAPSHOT|GLOBAL_CONFIG_SNAPSHOT|BUFFER_CONFIG_SINGLETON|BUFFER_PROFILE_ENABLED|LEGACY_CREDENTIAL_WARNED_KEYS|CONSOLE_CONFIG|ACTIVE_HTTP_REQUESTS|USE_STARSHARD_CACHE|BUCKET_CACHE_SMALL|BUCKET_CACHE_LARGE|GLOBAL_SSE_DEK_PROVIDER|SSE_TEST_LOCK|AUTH_FS|LOCK_STATS|DEADLOCK_DETECTOR|GET_OBJECT_BUFFER_THRESHOLD_WARNED|GET_READER_STREAM_BUFFER_SIZE_OVERRIDE|OBJECT_SEEK_SUPPORT_THRESHOLD|OBJECT_SEEK_SUPPORT_CONCURRENCY_THRESHOLDS|SUPPORTED_HEADERS|SITE_REPLICATION_PEER_CLIENT|AUDIT_MODULE_ENABLED|NOTIFY_MODULE_ENABLED|PERSISTED_NOTIFY_MODULE_ENABLED|PERSISTED_AUDIT_MODULE_ENABLED|PERSISTED_MODULE_SWITCH_CONFIGURED|DELETE_TAIL_TOTAL|DELETE_CLEANUP_TOTAL|DELETE_REPLICATION_TOTAL|DELETE_NOTIFY_TOTAL|EMBEDDED_SERVER_STARTED|TEST_OUTBOUND_TLS_GENERATION|TEST_REMAINING_FAILURES|CAPACITY_DIRTY_SCOPE_ENV|CAPACITY_DIRTY_SCOPE_INIT|GLOBAL_ENV)'
|
||||
|
||||
(
|
||||
cd "$ROOT_DIR"
|
||||
|
||||
Reference in New Issue
Block a user