diff --git a/CHANGELOG.md b/CHANGELOG.md index 05b19e177..70954aaba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Helm Ingress**: `customAnnotations` are now merged with class-specific annotations (nginx/traefik) instead of being ignored when `ingress.className` is set. +- **Per-pool erasure parity**: Erasure parity (STANDARD and reduced-redundancy) is now resolved independently for every pool instead of reusing the first pool's value. A heterogeneous topology — for example a 4-drive pool plus a 2-drive pool created during expansion — previously inherited the first pool's parity and could resolve to zero data shards in the smaller pool, panicking Reed-Solomon construction on write. Automatic parity now resolves per pool (for example `2+2` in the 4-drive pool and `1+1` in the 2-drive pool). Fixes #4801. ### Added - **NATS JetStream Publish Path**: Opt-in at-least-once delivery for the NATS notify and audit targets. A NATS Core publish flushes to the connection without awaiting a broker acknowledgement, so an event can be lost across a broker restart or a reconnect after the send queue has already cleared it. A queued event now clears only after the JetStream `PublishAck`, so bucket notifications survive those interruptions. Off by default and byte-identical to the NATS Core path when disabled. @@ -38,6 +39,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **HTTP Server Stack**: Integrated `KeystoneAuthLayer` middleware from `rustfs-keystone` crate into service stack (positioned after ReadinessGateLayer) +- **Storage-class validation on startup (upgrade note)**: A persisted explicit storage class (`RUSTFS_STORAGE_CLASS_STANDARD` / `RUSTFS_STORAGE_CLASS_RRS`, for example `EC:2`) is now validated against the actual per-pool drive counts at startup and rejected when a pool cannot satisfy it. This is fail-closed and correct, but a cluster that persisted a storage class larger than a small or heterogeneous pool can hold (for example `EC:2` alongside a 2-drive pool), which earlier releases accepted and silently resolved to an invalid layout, will now refuse to start after upgrade. To recover, unset `RUSTFS_STORAGE_CLASS_STANDARD` so the server derives a valid per-pool default automatically, or set it to a value every pool can satisfy. - **IAMAuth**: Enhanced `get_secret_key()` to return empty secret for Keystone credentials (bypasses signature validation) - **Auth Module**: Modified `check_key_valid()` to retrieve Keystone credentials from task-local storage and determine admin status - **`StorageBackend` trait**: extended with multipart upload methods (`create_multipart_upload`, `upload_part`, `complete_multipart_upload`, `abort_multipart_upload`) plus `upload_part_copy`. Streaming-upload code path is now available to FTPS, WebDAV, and Swift drivers as well. diff --git a/crates/ecstore/src/cluster/rpc/remote_disk.rs b/crates/ecstore/src/cluster/rpc/remote_disk.rs index 92801429a..6e4f1613a 100644 --- a/crates/ecstore/src/cluster/rpc/remote_disk.rs +++ b/crates/ecstore/src/cluster/rpc/remote_disk.rs @@ -1129,6 +1129,9 @@ fn decode_batch_read_version_response_items( let resp = decode_msgpack_or_json::(buf, "", "BatchReadVersionResp").map_err(|err| { Error::other(format!("decode BatchReadVersionResp msgpack item {index} from {endpoint} failed: {err}")) })?; + if resp.success { + validate_decoded_file_info(&resp.file_info)?; + } batch_read_version_resps.push(resp); } return Ok(batch_read_version_resps); @@ -1143,12 +1146,19 @@ fn decode_batch_read_version_response_items( let resp = serde_json::from_str::(json_str).map_err(|err| { Error::other(format!("decode BatchReadVersionResp json item {index} from {endpoint} failed: {err}")) })?; + if resp.success { + validate_decoded_file_info(&resp.file_info)?; + } batch_read_version_resps.push(resp); } Ok(batch_read_version_resps) } +fn validate_decoded_file_info(file_info: &FileInfo) -> Result<()> { + file_info.validate_for_metadata_read().map_err(Into::into) +} + #[async_trait::async_trait] impl DiskAPI for RemoteDisk { #[tracing::instrument(level = "trace", skip_all)] @@ -1825,6 +1835,7 @@ impl DiskAPI for RemoteDisk { } let file_info = decode_msgpack_or_json::(&response.file_info_bin, &response.file_info, "FileInfo")?; + validate_decoded_file_info(&file_info)?; Ok(file_info) }, @@ -2712,6 +2723,25 @@ mod tests { static INIT: Once = Once::new(); + #[test] + fn decoded_remote_metadata_rejects_default_like_delete_marker() { + let forged = FileInfo { + deleted: true, + ..Default::default() + }; + assert!(matches!(validate_decoded_file_info(&forged), Err(DiskError::FileCorrupt))); + + let marker = FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(Uuid::new_v4()), + deleted: true, + mod_time: Some(::time::OffsetDateTime::now_utc()), + ..Default::default() + }; + validate_decoded_file_info(&marker).expect("canonical remote delete marker should validate"); + } + #[derive(Clone, Default)] struct CapturedLogs { buffer: Arc>>, @@ -3021,17 +3051,19 @@ mod tests { } fn sample_batch_read_version_resp(index: usize, path: &str, success: bool) -> BatchReadVersionResp { + let mut file_info = FileInfo::new(path, 1, 0); + file_info.erasure.index = 1; BatchReadVersionResp { index, path: path.to_string(), version_id: "version-a".to_string(), success, + file_info, error: if success { String::new() } else { "file version not found".to_string() }, - ..Default::default() } } @@ -3055,6 +3087,25 @@ mod tests { assert!(decoded[0].success); } + #[test] + fn batch_read_version_response_rejects_invalid_success_metadata() { + let endpoint = sample_remote_endpoint(); + let mut response_item = sample_batch_read_version_resp(0, "invalid-object", true); + response_item.file_info.erasure.data_blocks = 0; + response_item.file_info.erasure.parity_blocks = 2; + let response = BatchReadVersionResponse { + success: true, + batch_read_version_resps: Vec::new(), + batch_read_version_resps_bin: vec![encode_msgpack(&response_item).expect("msgpack response should encode").into()], + error: None, + }; + + let err = decode_batch_read_version_response_items(response, &endpoint) + .expect_err("successful remote response with invalid metadata must fail closed"); + + assert_eq!(err, DiskError::FileCorrupt); + } + #[test] fn batch_read_version_response_decode_reports_corrupt_msgpack_item() { let endpoint = sample_remote_endpoint(); diff --git a/crates/ecstore/src/config/storageclass.rs b/crates/ecstore/src/config/storageclass.rs index 56cea2486..050da21b8 100644 --- a/crates/ecstore/src/config/storageclass.rs +++ b/crates/ecstore/src/config/storageclass.rs @@ -69,6 +69,8 @@ pub const MIN_PARITY_DRIVES: usize = 0; // Default RRS parity is always minimum parity. pub const DEFAULT_RRS_PARITY: usize = 1; +const DEFAULT_RRS_STORAGE_CLASS: &str = "EC:1"; +const ZERO_SET_DRIVE_COUNT_ERROR: &str = "set drive count must be greater than zero"; pub static DEFAULT_INLINE_BLOCK: usize = 128 * 1024; @@ -81,7 +83,7 @@ pub static DEFAULT_KVS: LazyLock = LazyLock::new(|| { }, KV { key: CLASS_RRS.to_owned(), - value: "EC:1".to_owned(), + value: DEFAULT_RRS_STORAGE_CLASS.to_owned(), hidden_if_empty: false, }, KV { @@ -314,7 +316,7 @@ fn rrs_policy(kvs: &KVS, value: Option) -> Result { Some(value) => Ok(ParityPolicy::Explicit(parse_storage_class(&value)?.parity)), None => { let value = kvs.get(CLASS_RRS); - if value.is_empty() || value == "EC:1" { + if value.is_empty() || value == DEFAULT_RRS_STORAGE_CLASS { Ok(ParityPolicy::LegacyRrs) } else { Ok(ParityPolicy::Explicit(parse_storage_class(&value)?.parity)) @@ -442,7 +444,7 @@ pub fn validate_parity(ss_parity: usize, set_drive_count: usize) -> Result<()> { // } if set_drive_count == 0 { - return Err(Error::other("set drive count must be greater than zero")); + return Err(Error::other(ZERO_SET_DRIVE_COUNT_ERROR)); } if ss_parity > set_drive_count / 2 { @@ -475,7 +477,7 @@ pub fn validate_parity_inner(ss_parity: usize, rrs_parity: usize, set_drive_coun // } if set_drive_count == 0 { - return Err(Error::other("set drive count must be greater than zero")); + return Err(Error::other(ZERO_SET_DRIVE_COUNT_ERROR)); } if ss_parity > set_drive_count / 2 { diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index 15ff12815..c89226723 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -39,7 +39,7 @@ use metrics::gauge; use parking_lot::RwLock as ParkingLotRwLock; use rustfs_filemeta::{ Cache, FileInfo, FileInfoOpts, FileMeta, MetaCacheEntry, MetacacheWriter, ObjectPartInfo, Opts, RawFileInfo, UpdateFn, - get_file_info, read_xl_meta_no_data_sync, + ValidationMode, get_file_info, read_xl_meta_no_data_sync, }; use rustfs_utils::HashAlgorithm; use rustfs_utils::os::get_info; @@ -133,6 +133,43 @@ fn rollback_committed_rename_std( Ok(()) } +fn rollback_inline_metadata_commit_std( + dst_file_path: &Path, + rollback_data_dir: Option, + local_rollback_path: Option<&Path>, +) -> std::io::Result<()> { + if let Some(backup_path) = local_rollback_path { + // The commit immediately before this rollback renamed the staged + // xl.meta from the same directory as `backup_path` onto + // `dst_file_path`, proving both paths are on the same filesystem. + // Unix rename atomically replaces the committed destination; never + // unlink it first or an interrupted rollback could lose xl.meta. + std::fs::rename(backup_path, dst_file_path)?; + } else { + rollback_committed_rename_std(dst_file_path, None, rollback_data_dir)?; + } + Ok(()) +} + +fn create_local_inline_rollback_backup( + dst_file_path: &Path, + staging_file_path: &Path, + old_metadata: &[u8], +) -> std::io::Result { + let Some(staging_parent) = staging_file_path.parent() else { + return Err(std::io::Error::new(ErrorKind::InvalidInput, "missing staging metadata parent")); + }; + let backup_path = staging_parent.join(STORAGE_FORMAT_FILE_BACKUP); + remove_file_if_exists(&backup_path)?; + if (should_fail_local_inline_rollback_hardlink(dst_file_path) || std::fs::hard_link(dst_file_path, &backup_path).is_err()) + && let Err(err) = std::fs::write(&backup_path, old_metadata) + { + let _ = remove_file_if_exists(&backup_path); + return Err(err); + } + Ok(backup_path) +} + async fn write_metadata_rollback_backup(object_dir: &Path, rollback_dir: Uuid, data: &[u8]) -> Result<()> { let backup_dir = object_dir.join(rollback_dir.to_string()); fs::create_dir_all(&backup_dir).await.map_err(to_file_error)?; @@ -1429,6 +1466,10 @@ static RENAME_DATA_FAIL_BEFORE_OLD_METADATA_BACKUP: std::sync::Mutex> = std::sync::Mutex::new(None); #[cfg(test)] +static RENAME_DATA_FAIL_COMMIT_RENAME: std::sync::Mutex> = std::sync::Mutex::new(None); +#[cfg(test)] +static LOCAL_INLINE_ROLLBACK_HARDLINK_FAILURE: std::sync::Mutex> = std::sync::Mutex::new(None); +#[cfg(test)] static DELETE_VERSION_FAIL_AFTER_DATA_STAGED: std::sync::Mutex> = std::sync::Mutex::new(Vec::new()); #[cfg(test)] @@ -1445,6 +1486,20 @@ fn set_rename_data_fail_after_metadata_commit(dst_path: &str) { .expect("test failpoint lock should not be poisoned") = Some(dst_path.to_string()); } +#[cfg(test)] +fn set_rename_data_fail_commit_rename(dst_path: &str) { + *RENAME_DATA_FAIL_COMMIT_RENAME + .lock() + .expect("test failpoint lock should not be poisoned") = Some(dst_path.to_string()); +} + +#[cfg(test)] +fn set_local_inline_rollback_hardlink_failure(dst_path: &Path) { + *LOCAL_INLINE_ROLLBACK_HARDLINK_FAILURE + .lock() + .expect("test failpoint lock should not be poisoned") = Some(dst_path.to_path_buf()); +} + #[cfg(test)] fn set_delete_version_fail_after_data_staged(path: &str) { DELETE_VERSION_FAIL_AFTER_DATA_STAGED @@ -1479,6 +1534,32 @@ fn should_fail_after_metadata_commit(dst_path: &str) -> bool { } } +#[cfg(test)] +fn should_fail_commit_rename(dst_path: &str) -> bool { + let mut target = RENAME_DATA_FAIL_COMMIT_RENAME + .lock() + .expect("test failpoint lock should not be poisoned"); + if target.as_deref() == Some(dst_path) { + target.take(); + true + } else { + false + } +} + +#[cfg(test)] +fn should_fail_local_inline_rollback_hardlink(dst_path: &Path) -> bool { + let mut target = LOCAL_INLINE_ROLLBACK_HARDLINK_FAILURE + .lock() + .expect("test failpoint lock should not be poisoned"); + if target.as_deref() == Some(dst_path) { + target.take(); + true + } else { + false + } +} + #[cfg(test)] fn should_fail_after_delete_data_staged(path: &str) -> bool { let mut targets = DELETE_VERSION_FAIL_AFTER_DATA_STAGED @@ -1502,6 +1583,16 @@ fn should_fail_after_metadata_commit(_dst_path: &str) -> bool { false } +#[cfg(not(test))] +fn should_fail_commit_rename(_dst_path: &str) -> bool { + false +} + +#[cfg(not(test))] +fn should_fail_local_inline_rollback_hardlink(_dst_path: &Path) -> bool { + false +} + #[cfg(not(test))] fn should_fail_after_delete_data_staged(_path: &str) -> bool { false @@ -5969,6 +6060,7 @@ impl DiskAPI for LocalDisk { fi.uses_legacy_checksum, ) .map_err(DiskError::from)?; + fi.validate(ValidationMode::RequireErasure)?; for (i, part) in fi.parts.iter().enumerate() { let checksum_info = erasure.get_checksum_info(part.number); let checksum_algo = if fi.uses_legacy_checksum && checksum_info.algorithm == HashAlgorithm::HighwayHash256S { @@ -6095,6 +6187,7 @@ impl DiskAPI for LocalDisk { } #[tracing::instrument(level = "trace", skip_all)] async fn check_parts(&self, volume: &str, path: &str, fi: &FileInfo) -> Result { + let layout = fi.validate(ValidationMode::RequireErasure)?.ok_or(DiskError::FileCorrupt)?; let volume_dir = self.get_bucket_path(volume)?; let file_path = self.get_object_path(volume, path)?; check_path_length(file_path.to_string_lossy().as_ref())?; @@ -6129,7 +6222,9 @@ impl DiskAPI for LocalDisk { resp.results[i] = CHECK_PART_FILE_NOT_FOUND; continue; } - if (st.len() as i64) < fi.erasure.shard_file_size(part.size as i64) { + let expected_size = layout.shard_file_size(part.size).ok_or(DiskError::FileCorrupt)?; + let expected_size = u64::try_from(expected_size).map_err(|_| DiskError::FileCorrupt)?; + if st.len() < expected_size { resp.results[i] = CHECK_PART_FILE_CORRUPT; continue; } @@ -6590,11 +6685,15 @@ impl DiskAPI for LocalDisk { &self, src_volume: &str, src_path: &str, - fi: FileInfo, + mut fi: FileInfo, dst_volume: &str, dst_path: &str, ) -> Result { crate::hp_guard!("LocalDisk::rename_data"); + if fi.is_legacy_indexed_delete_marker() { + fi.erasure.index = 0; + } + fi.validate_for_metadata_read()?; // Snapshot the destination part paths before `fi` is consumed below. These // are the descriptors a reader may hold for the version this call is about // to replace (backlog#1145); readers build the identical string in @@ -7038,6 +7137,8 @@ impl DiskAPI for LocalDisk { None } }); + let sync = durability.syncs_commit_metadata(); + let mut local_rollback_path = None; if let Some(d) = old_data_dir.as_ref() { let _ = xlmeta.data.remove_two(version_id, *d); } @@ -7053,7 +7154,6 @@ impl DiskAPI for LocalDisk { if let Some(parent) = src.parent() { std::fs::create_dir_all(parent)?; } - let sync = durability.syncs_commit_metadata(); let mut f = std::fs::OpenOptions::new() .create(true) .write(true) @@ -7091,23 +7191,37 @@ impl DiskAPI for LocalDisk { os::fsync_dir_std(old_parent).map_err(to_file_error)?; } } + } else if let Some(ref old_metadata) = has_dst_buf + && (sync || cfg!(test)) + { + local_rollback_path = Some(create_local_inline_rollback_backup(&dst, &src, old_metadata)?); } - match std::fs::rename(&src, &dst) { - Ok(()) => Ok(()), - Err(err) if err.kind() == ErrorKind::NotFound && !src.exists() => Ok(()), - Err(err) if err.kind() == ErrorKind::NotFound => { - if let Some(parent) = dst.parent() { - std::fs::create_dir_all(parent)?; + let commit_result = if should_fail_commit_rename(&dst_path_for_failpoint) { + Err(std::io::Error::other("test fail during metadata commit rename")) + } else { + match std::fs::rename(&src, &dst) { + Ok(()) => Ok(()), + Err(err) if err.kind() == ErrorKind::NotFound && !src.exists() => Ok(()), + Err(err) if err.kind() == ErrorKind::NotFound => { + if let Some(parent) = dst.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::rename(&src, &dst).map_err(to_file_error)?; + Ok(()) } - std::fs::rename(&src, &dst).map_err(to_file_error)?; - Ok(()) + Err(err) => Err(to_file_error(err)), } - Err(err) => Err(to_file_error(err)), - }?; + }; + if let Err(err) = commit_result { + if let Some(backup_path) = local_rollback_path.as_deref() { + let _ = remove_file_if_exists(backup_path); + } + return Err(err); + } if should_fail_after_metadata_commit(&dst_path_for_failpoint) { - rollback_committed_rename_std(&dst, None, rollback_data_dir)?; + rollback_inline_metadata_commit_std(&dst, rollback_data_dir, local_rollback_path.as_deref())?; return Err(std::io::Error::other("test fail after metadata commit")); } @@ -7116,7 +7230,7 @@ impl DiskAPI for LocalDisk { && let Some(dst_parent) = dst.parent() && let Err(err) = os::fsync_dir_std(dst_parent) { - rollback_committed_rename_std(&dst, None, rollback_data_dir)?; + rollback_inline_metadata_commit_std(&dst, rollback_data_dir, local_rollback_path.as_deref())?; return Err(err); } @@ -7134,7 +7248,7 @@ impl DiskAPI for LocalDisk { break; } if let Err(err) = os::fsync_dir_std(ancestor_dir) { - rollback_committed_rename_std(&dst, None, rollback_data_dir)?; + rollback_inline_metadata_commit_std(&dst, rollback_data_dir, local_rollback_path.as_deref())?; return Err(err); } if ancestor_dir == bucket_dir.as_path() { @@ -7144,6 +7258,10 @@ impl DiskAPI for LocalDisk { } } + if let Some(backup_path) = local_rollback_path.as_deref() { + let _ = remove_file_if_exists(backup_path); + } + Ok::<(Option, Option>, Option), std::io::Error>(( rollback_data_dir, version_signature, @@ -7349,6 +7467,7 @@ impl DiskAPI for LocalDisk { #[tracing::instrument(level = "trace", skip_all)] async fn write_metadata(&self, _org_volume: &str, volume: &str, path: &str, fi: FileInfo) -> Result<()> { crate::hp_guard!("LocalDisk::write_metadata"); + fi.validate_for_metadata_read()?; let p = self.get_object_path(volume, format!("{path}/{STORAGE_FORMAT_FILE}").as_str())?; let mut meta = FileMeta::new(); @@ -7421,6 +7540,11 @@ impl DiskAPI for LocalDisk { }, )?; + fi.validate_for_metadata_read()?; + if fi.is_canonical_delete_marker() { + return Ok(fi); + } + if opts.read_data { if fi.data.as_ref().is_some_and(|d| !d.is_empty()) || fi.size == 0 { if fi.inline_data() { @@ -7969,15 +8093,15 @@ mod test { .as_ref() .map(|data| i64::try_from(data.len()).expect("test data length should fit i64")) .unwrap_or(1); - FileInfo { - name: name.to_string(), - version_id: Some(version_id), - data_dir, - data, - size, - mod_time: Some(OffsetDateTime::now_utc()), - ..Default::default() - } + let mut file_info = FileInfo::new(name, 1, 0); + file_info.erasure.index = 1; + file_info.name = name.to_string(); + file_info.version_id = Some(version_id); + file_info.data_dir = data_dir; + file_info.data = data; + file_info.size = size; + file_info.mod_time = Some(OffsetDateTime::now_utc()); + file_info } fn test_meta(fi: FileInfo) -> Vec { @@ -8001,6 +8125,27 @@ mod test { assert!(!rollback_dir.is_nil()); } + #[test] + fn local_inline_rollback_backup_falls_back_when_hardlink_fails() { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let object_dir = dir.path().join("object"); + std::fs::create_dir(&object_dir).expect("object dir should be created"); + let xl_path = object_dir.join(STORAGE_FORMAT_FILE); + let staging_dir = dir.path().join("staging"); + std::fs::create_dir(&staging_dir).expect("staging dir should be created"); + let staging_path = staging_dir.join(STORAGE_FORMAT_FILE); + let old_metadata = b"old metadata"; + std::fs::write(&xl_path, old_metadata).expect("old metadata should be written"); + set_local_inline_rollback_hardlink_failure(&xl_path); + + let rollback_path = create_local_inline_rollback_backup(&xl_path, &staging_path, old_metadata) + .expect("copy fallback should create rollback backup"); + let backup = std::fs::read(&rollback_path).expect("fallback backup should be readable"); + + assert_eq!(backup, old_metadata); + assert_eq!(rollback_path.parent(), Some(staging_dir.as_path())); + } + // Call-site guards for rustfs/rustfs#4978. On Linux/macOS CI a real // non-empty rmdir yields ENOTEMPTY (already tolerated by the pre-fix paths), // so an end-to-end delete test cannot detect a call-site regression. These @@ -8052,6 +8197,156 @@ mod test { } } + #[tokio::test] + async fn read_version_rejects_zero_data_geometry_before_inline_shard_math() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let bucket = "bucket"; + let object = "invalid-erasure"; + ensure_test_volume(&disk, bucket).await; + + let object_dir = dir.path().join(bucket).join(object); + fs::create_dir_all(&object_dir).await.expect("object dir should be created"); + let mut file_info = test_file_info(object, Uuid::new_v4(), Some(Uuid::new_v4()), None); + file_info.parts = vec![ObjectPartInfo { + number: 1, + size: 1, + actual_size: 1, + ..Default::default() + }]; + file_info.erasure.data_blocks = 0; + file_info.erasure.parity_blocks = 2; + file_info.erasure.block_size = 1; + file_info.erasure.index = 1; + file_info.erasure.distribution = vec![1, 2]; + fs::write(object_dir.join(STORAGE_FORMAT_FILE), test_meta(file_info)) + .await + .expect("invalid metadata should be written for the read regression"); + + let err = disk + .read_version( + "", + bucket, + object, + "", + &ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .expect_err("invalid erasure geometry must fail before shard size calculation"); + + assert_eq!(err, DiskError::FileCorrupt); + } + + #[tokio::test] + async fn read_version_delete_marker_never_enters_inline_shard_math() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let bucket = "bucket"; + let object = "delete-marker"; + ensure_test_volume(&disk, bucket).await; + + let object_dir = dir.path().join(bucket).join(object); + fs::create_dir_all(&object_dir).await.expect("object dir should be created"); + let file_info = FileInfo { + name: object.to_string(), + version_id: Some(Uuid::new_v4()), + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + fs::write(object_dir.join(STORAGE_FORMAT_FILE), test_meta(file_info)) + .await + .expect("delete marker metadata should be written"); + + let file_info = disk + .read_version( + "", + bucket, + object, + "", + &ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .expect("delete marker must return before payload shard math"); + + assert!(file_info.deleted); + assert_eq!(file_info.erasure.data_blocks, 0); + } + + #[tokio::test] + async fn read_version_purge_pending_payload_still_loads_inline_candidate_data() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let bucket = "bucket"; + let object = "purge-pending-object"; + let version_id = Uuid::new_v4(); + let data_dir = Uuid::new_v4(); + let payload = b"purge-pending payload"; + ensure_test_volume(&disk, bucket).await; + + let object_dir = dir.path().join(bucket).join(object); + let part_dir = object_dir.join(data_dir.to_string()); + fs::create_dir_all(&part_dir) + .await + .expect("object data dir should be created"); + fs::write(part_dir.join("part.1"), payload) + .await + .expect("payload part should be written"); + + let mut file_info = test_file_info(object, version_id, Some(data_dir), None); + file_info.size = payload.len() as i64; + file_info.add_object_part( + 1, + "part-etag".to_string(), + payload.len(), + file_info.mod_time, + payload.len() as i64, + None, + None, + ); + rustfs_utils::http::insert_str( + &mut file_info.metadata, + rustfs_utils::http::SUFFIX_PURGESTATUS, + "target=PENDING;".to_string(), + ); + fs::write(object_dir.join(STORAGE_FORMAT_FILE), test_meta(file_info)) + .await + .expect("purge-pending object metadata should be written"); + + let file_info = disk + .read_version( + "", + bucket, + object, + "", + &ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .expect("purge-pending object remains an erasure payload at the disk boundary"); + + assert!(file_info.deleted, "version purge state should retain its logical deleted flag"); + assert!(!file_info.is_canonical_delete_marker()); + assert_eq!(file_info.data.as_deref(), Some(payload.as_slice())); + } + /// Regression coverage for the disk-layer delete/rename fixes: /// - move_to_trash must propagate real rename failures instead of silently /// reporting success (rustfs/backlog#948, ECA-07). @@ -8645,6 +8940,25 @@ mod test { assert_eq!(payload, b"read-only-payload"); } + #[tokio::test] + async fn write_metadata_rejects_default_like_delete_marker() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let forged = FileInfo { + deleted: true, + ..Default::default() + }; + + let err = disk + .write_metadata("bucket", "bucket", "object", forged) + .await + .expect_err("default-like delete marker must be rejected before persistence"); + assert_eq!(err, DiskError::FileCorrupt); + } + #[tokio::test] async fn write_metadata_replaces_corrupt_existing_xl_meta_without_losing_new_version() { use tempfile::tempdir; @@ -8698,6 +9012,7 @@ mod test { data_blocks: 2, parity_blocks: 2, block_size: 4, + index: 1, distribution: vec![1, 2, 3, 4], ..Default::default() }, @@ -10341,6 +10656,184 @@ mod test { assert_eq!(restored_meta, old_meta); } + #[tokio::test] + async fn rename_delete_marker_post_commit_error_restores_other_version_metadata() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + + let bucket = "bucket"; + let object = "delete-marker-post-commit-object"; + let old_version_id = Uuid::parse_str("77777777-7777-7777-7777-777777777777").expect("version id should parse"); + let marker_version_id = Uuid::parse_str("88888888-8888-8888-8888-888888888888").expect("version id should parse"); + + ensure_test_volume(&disk, bucket).await; + ensure_test_volume(&disk, RUSTFS_META_TMP_BUCKET).await; + + let old_meta = test_meta(test_file_info(object, old_version_id, None, Some(Bytes::from_static(b"inline-old")))); + let dst_object_dir = dir.path().join(bucket).join(object); + fs::create_dir_all(&dst_object_dir) + .await + .expect("object dir should be created"); + fs::write(dst_object_dir.join(STORAGE_FORMAT_FILE), old_meta.clone()) + .await + .expect("old metadata should be written"); + + let marker = FileInfo { + volume: bucket.to_string(), + name: object.to_string(), + version_id: Some(marker_version_id), + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + let xl_path = dst_object_dir.join(STORAGE_FORMAT_FILE); + set_local_inline_rollback_hardlink_failure(&xl_path); + set_rename_data_fail_after_metadata_commit(object); + let result = disk + .rename_data(RUSTFS_META_TMP_BUCKET, "tmp-delete-marker", marker, bucket, object) + .await; + + assert!(result.is_err()); + let restored_meta = fs::read(dst_object_dir.join(STORAGE_FORMAT_FILE)) + .await + .expect("old metadata should still be readable"); + assert_eq!(restored_meta, old_meta); + let mut entries = fs::read_dir(&dst_object_dir) + .await + .expect("object directory should remain readable"); + while let Some(entry) = entries.next_entry().await.expect("object directory entry should be readable") { + assert!(!entry.path().is_dir(), "local rollback directory should be removed"); + } + assert!( + !dir.path() + .join(RUSTFS_META_TMP_BUCKET) + .join("tmp-delete-marker") + .join(STORAGE_FORMAT_FILE_BACKUP) + .exists(), + "copy fallback backup should be consumed by atomic rollback" + ); + } + + #[tokio::test] + async fn rename_commit_failure_cleans_local_rollback_backup() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let bucket = "bucket"; + let object = "commit-rename-failure-object"; + ensure_test_volume(&disk, bucket).await; + ensure_test_volume(&disk, RUSTFS_META_TMP_BUCKET).await; + + let old_meta = test_meta(test_file_info(object, Uuid::new_v4(), None, Some(Bytes::from_static(b"old")))); + let dst_object_dir = dir.path().join(bucket).join(object); + fs::create_dir_all(&dst_object_dir) + .await + .expect("object dir should be created"); + fs::write(dst_object_dir.join(STORAGE_FORMAT_FILE), old_meta.clone()) + .await + .expect("old metadata should be written"); + + set_rename_data_fail_commit_rename(object); + let result = disk + .rename_data( + RUSTFS_META_TMP_BUCKET, + "tmp-commit-failure", + FileInfo { + volume: bucket.to_string(), + name: object.to_string(), + version_id: Some(Uuid::new_v4()), + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }, + bucket, + object, + ) + .await; + + assert!(result.is_err()); + assert_eq!( + fs::read(dst_object_dir.join(STORAGE_FORMAT_FILE)) + .await + .expect("old metadata should remain readable"), + old_meta + ); + let mut entries = fs::read_dir(&dst_object_dir) + .await + .expect("object directory should remain readable"); + while let Some(entry) = entries.next_entry().await.expect("object directory entry should be readable") { + assert!(!entry.path().is_dir(), "failed commit must clean local rollback directory"); + } + } + + #[tokio::test] + async fn rename_purge_pending_payload_stays_object_and_cleans_local_backup() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + + let bucket = "bucket"; + let object = "purge-pending-rename-object"; + let old_version_id = Uuid::new_v4(); + let purge_version_id = Uuid::new_v4(); + ensure_test_volume(&disk, bucket).await; + ensure_test_volume(&disk, RUSTFS_META_TMP_BUCKET).await; + + let dst_object_dir = dir.path().join(bucket).join(object); + fs::create_dir_all(&dst_object_dir) + .await + .expect("object dir should be created"); + fs::write( + dst_object_dir.join(STORAGE_FORMAT_FILE), + test_meta(test_file_info(object, old_version_id, None, Some(Bytes::from_static(b"old")))), + ) + .await + .expect("old metadata should be written"); + + let mut purge_pending = test_file_info(object, purge_version_id, None, Some(Bytes::from_static(b"purge-pending"))); + rustfs_utils::http::insert_str( + &mut purge_pending.metadata, + rustfs_utils::http::SUFFIX_PURGESTATUS, + "target=PENDING;".to_string(), + ); + purge_pending.deleted = true; + disk.rename_data(RUSTFS_META_TMP_BUCKET, "tmp-purge-pending", purge_pending, bucket, object) + .await + .expect("purge-pending erasure payload should commit"); + + let stored = disk + .read_version( + "", + bucket, + object, + &purge_version_id.to_string(), + &ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .expect("purge-pending payload should remain readable"); + assert!(stored.deleted); + assert!(!stored.is_canonical_delete_marker()); + assert_eq!(stored.erasure.data_blocks, 1); + assert_eq!(stored.size, 13); + + let mut entries = fs::read_dir(&dst_object_dir) + .await + .expect("object directory should remain readable"); + while let Some(entry) = entries.next_entry().await.expect("object directory entry should be readable") { + assert!(!entry.path().is_dir(), "successful local rollback directory should be removed"); + } + } + #[tokio::test] async fn test_delete_version_undo_restores_backup_to_object_root() { use tempfile::tempdir; @@ -13627,6 +14120,49 @@ mod test { assert!(construction_source.is::()); } + #[tokio::test] + async fn local_disk_check_parts_rejects_zero_data_geometry_before_shard_math() { + use tempfile::tempdir; + + let root_dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(root_dir.path().to_string_lossy().as_ref()).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let volume = "check-parts-volume"; + let object = "object.bin"; + let data_dir = Uuid::new_v4(); + ensure_test_volume(&disk, volume).await; + + let part_path = path_join_buf(&[object, &data_dir.to_string(), "part.1"]); + disk.write_all(volume, &part_path, Bytes::from_static(b"shard")) + .await + .expect("test shard should be written"); + let file_info = FileInfo { + data_dir: Some(data_dir), + parts: vec![ObjectPartInfo { + number: 1, + size: 1, + actual_size: 1, + ..Default::default() + }], + erasure: ErasureInfo { + data_blocks: 0, + parity_blocks: 2, + block_size: 1, + index: 1, + distribution: vec![1, 2], + ..Default::default() + }, + ..Default::default() + }; + + let err = disk + .check_parts(volume, object, &file_info) + .await + .expect_err("invalid erasure metadata must fail before shard size calculation"); + + assert_eq!(err, DiskError::FileCorrupt); + } + #[tokio::test] async fn local_disk_read_file_verifier_reports_bitrot_mismatch() { use crate::erasure::coding::BitrotWriter; diff --git a/crates/ecstore/src/object_api/types.rs b/crates/ecstore/src/object_api/types.rs index ae2c09353..77e8af41a 100644 --- a/crates/ecstore/src/object_api/types.rs +++ b/crates/ecstore/src/object_api/types.rs @@ -307,9 +307,9 @@ impl ObjectInfo { /// The inline fast path decodes erasure-coded data entirely in memory, /// bypassing disk I/O, duplex pipes, and the disk-read semaphore. /// - /// The `inlined` flag is the primary signal — it is set during PUT by - /// `storage_class_should_inline()` which already applies the correct - /// version-aware threshold (128 KiB non-versioned, 16 KiB versioned). + /// The `inlined` flag is the primary signal — PUT sets it through the + /// captured storage-class snapshot's `Config::should_inline`, which applies + /// the correct version-aware threshold (128 KiB non-versioned, 16 KiB versioned). /// The size check below is a safety net using the same thresholds. /// /// Additional conditions: diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index 2764bbf44..9d86328bf 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -164,7 +164,7 @@ pub(in crate::set_disk) struct MetadataFanoutObservation { impl MetadataFanoutObservation { pub(in crate::set_disk) fn from_file_info(file_info: &FileInfo, elapsed: Duration) -> Self { - if file_info.is_valid() { + if file_info_is_valid_for_metadata(file_info) { Self { outcome: GET_METADATA_RESPONSE_VALID, elapsed, @@ -301,6 +301,7 @@ pub(in crate::set_disk) struct MetadataQuorumAccumulator { pub(in crate::set_disk) candidate_votes: usize, pub(in crate::set_disk) conflicting_metadata: bool, pub(in crate::set_disk) delete_marker_seen: bool, + pub(in crate::set_disk) delete_marker_candidates: Vec<(FileInfo, usize)>, pub(in crate::set_disk) delete_marker_votes: usize, pub(in crate::set_disk) requested_version_id: String, pub(in crate::set_disk) matching_version_votes: usize, @@ -321,6 +322,7 @@ impl MetadataQuorumAccumulator { candidate_votes: 0, conflicting_metadata: false, delete_marker_seen: false, + delete_marker_candidates: Vec::new(), delete_marker_votes: 0, requested_version_id: String::new(), matching_version_votes: 0, @@ -333,7 +335,7 @@ impl MetadataQuorumAccumulator { } pub(in crate::set_disk) fn observe_file_info(&mut self, file_info: &FileInfo) { - if !file_info.is_valid() { + if !file_info_is_valid_for_metadata(file_info) { self.hard_errors = self.hard_errors.saturating_add(1); return; } @@ -348,9 +350,24 @@ impl MetadataQuorumAccumulator { self.matching_version_votes = self.matching_version_votes.saturating_add(1); } - if file_info.deleted { - self.delete_marker_votes = self.delete_marker_votes.saturating_add(1); + if file_info.is_canonical_delete_marker() { self.delete_marker_seen = true; + if let Some((_, votes)) = self + .delete_marker_candidates + .iter_mut() + .find(|(candidate, _)| metadata_early_stop_candidate_matches(candidate, file_info)) + { + *votes = votes.saturating_add(1); + } else { + self.delete_marker_candidates.push((file_info.clone(), 1)); + } + self.delete_marker_votes = self + .delete_marker_candidates + .iter() + .map(|(_, votes)| *votes) + .max() + .unwrap_or_default(); + self.conflicting_metadata |= self.delete_marker_candidates.len() > 1; return; } @@ -477,10 +494,15 @@ impl MetadataQuorumAccumulator { if self.default_parity_count == 0 { return Some(self.total_disks); } - if candidate.deleted || candidate.size == 0 || candidate.erasure.parity_blocks >= self.total_disks { + if candidate.is_canonical_delete_marker() || candidate.size == 0 || candidate.erasure.parity_blocks >= self.total_disks { return None; } - Some(candidate.write_quorum(self.default_write_quorum())) + let data_blocks = candidate.erasure.data_blocks; + Some(if data_blocks == candidate.erasure.parity_blocks { + data_blocks.saturating_add(1) + } else { + data_blocks + }) } pub(in crate::set_disk) fn default_write_quorum(&self) -> usize { @@ -518,13 +540,22 @@ pub(in crate::set_disk) fn metadata_early_stop_candidate_matches(left: &FileInfo && left.is_latest == right.is_latest && left.deleted == right.deleted && left.mark_deleted == right.mark_deleted + && left.transition_status == right.transition_status + && left.transitioned_objname == right.transitioned_objname + && left.transition_tier == right.transition_tier + && left.transition_version_id == right.transition_version_id + && left.expire_restored == right.expire_restored && left.size == right.size && left.mod_time == right.mod_time && left.mode == right.mode + && left.written_by_version == right.written_by_version && left.metadata == right.metadata + && left.replication_state_internal == right.replication_state_internal && left.parts == right.parts && left.checksum == right.checksum && left.versioned == right.versioned + && left.num_versions == right.num_versions + && left.successor_mod_time == right.successor_mod_time && left.data_dir == right.data_dir && left.erasure.algorithm == right.erasure.algorithm && left.erasure.data_blocks == right.erasure.data_blocks @@ -2257,13 +2288,24 @@ impl SetDisks { ))); } - FileMeta { + let file_info_versions = FileMeta { versions, ..Default::default() } .get_all_file_info_versions(bucket, object, true) - .map(Some) - .map_err(|err| Error::other(format!("exact object versions decode failed for {bucket}/{object}: {err}"))) + .map_err(|err| Error::other(format!("exact object versions decode failed for {bucket}/{object}: {err}")))?; + + for file_info in file_info_versions + .versions + .iter() + .chain(file_info_versions.free_versions.iter()) + { + file_info + .validate_for_metadata_read() + .map_err(|err| Error::other(format!("exact object versions validation failed for {bucket}/{object}: {err}")))?; + } + + Ok(Some(file_info_versions)) } pub(in crate::set_disk) async fn read_all_raw_file_info( @@ -2375,7 +2417,7 @@ impl SetDisks { // `into_fileinfo` with an empty version_id selects the first non-free version // (see FileMeta::into_fileinfo); replicate that selection from the header here. let vid = match meta.into_fileinfo(bucket, object, "", true, incl_free_vers, true) { - Ok(finfo) if finfo.is_valid() => finfo.version_id.unwrap_or(Uuid::nil()), + Ok(finfo) if file_info_is_valid_for_metadata(&finfo) => finfo.version_id.unwrap_or(Uuid::nil()), _ => match meta .versions .iter() @@ -2398,7 +2440,10 @@ impl SetDisks { for (idx, meta_op) in metadata_array.iter().enumerate() { if let Some(meta) = meta_op { match meta.into_fileinfo(bucket, object, vid.to_string().as_str(), read_data, incl_free_vers, true) { - Ok(res) => meta_file_infos[idx] = res, + Ok(res) => match res.validate_for_metadata_read() { + Ok(_) => meta_file_infos[idx] = res, + Err(err) => errs[idx] = Some(err.into()), + }, Err(err) => errs[idx] = Some(err.into()), } } @@ -2608,6 +2653,21 @@ impl SetDisks { Vec>, Option, )> { + if let Some(file_info) = disks + .iter() + .zip(file_infos.iter()) + .find_map(|(disk, file_info)| disk.as_ref().map(|_| file_info)) + { + // Newly encoded metadata does not acquire its per-disk shard index + // until the fanout below. Validate the shared metadata shape once, + // using an online slot because shuffled offline slots contain the + // default placeholder, then validate each assigned geometry in its task. + if file_info.is_canonical_delete_marker() { + file_info.validate_for_metadata_read()?; + } else { + file_info.validate_for_erasure_write()?; + } + } let mut futures = Vec::with_capacity(disks.len()); let mut errs = Vec::with_capacity(disks.len()); @@ -2631,11 +2691,16 @@ impl SetDisks { #[allow(clippy::let_unit_value)] let _fanout_task_guard = Self::rename_fanout_task_guard(&dst_object); + let Some(disk) = disk else { + return Err(DiskError::DiskNotFound); + }; + + let is_delete_marker = file_info.is_canonical_delete_marker(); if file_info.erasure.index == 0 { file_info.erasure.index = i + 1; } - if !file_info.is_valid() { + if !is_delete_marker && !file_info.has_valid_erasure_geometry() { return Err(DiskError::FileCorrupt); } @@ -2643,12 +2708,8 @@ impl SetDisks { // A no-op immediately-ready future in production. Self::rename_fanout_barrier(&dst_object, i, rename_fanout_barrier_phase::RENAME).await; - if let Some(disk) = disk { - disk.rename_data(&src_bucket, &src_object, file_info, &dst_bucket, &dst_object) - .await - } else { - Err(DiskError::DiskNotFound) - } + disk.rename_data(&src_bucket, &src_object, file_info, &dst_bucket, &dst_object) + .await })); } @@ -3391,7 +3452,7 @@ impl SetDisks { // that were never made durable), and deleting the surviving shards right away // turns a partial loss into a total one. Skip deletion and leave the object // for a later heal/scanner pass to re-evaluate. - if m.is_valid() + if file_info_is_valid_for_metadata(&m) && let Some(mod_time) = m.mod_time { let grace = dangling_delete_grace(); @@ -3412,7 +3473,7 @@ impl SetDisks { tags.insert("pool".to_string(), self.pool_index.to_string()); tags.insert("merrs".to_string(), join_errs(errs)); tags.insert("derrs".to_string(), format!("{data_errs_by_part:?}")); - if m.is_valid() { + if file_info_is_valid_for_metadata(&m) { tags.insert("sz".to_string(), m.size.to_string()); tags.insert( "mt".to_string(), @@ -3502,7 +3563,7 @@ impl SetDisks { } } - let write_quorum = if m.is_valid() { + let write_quorum = if file_info_is_valid_for_metadata(&m) { m.write_quorum(self.default_write_quorum()) } else { self.default_write_quorum() @@ -4375,6 +4436,17 @@ mod tests { fi } + fn metadata_test_delete_marker(object: &str, version_id: Uuid, mod_time: OffsetDateTime) -> FileInfo { + FileInfo { + volume: "bucket".to_string(), + name: object.to_string(), + version_id: Some(version_id), + deleted: true, + mod_time: Some(mod_time), + ..Default::default() + } + } + fn read_part_test_part(number: usize, etag: &str) -> ObjectPartInfo { ObjectPartInfo { number, @@ -4434,6 +4506,21 @@ mod tests { .await } + async fn write_raw_file_meta_unchecked(disk: &DiskStore, bucket: &str, object: &str, metadata: FileMeta) { + let encoded = metadata.marshal_msg().expect("raw regression metadata should serialize"); + disk.write_all(bucket, &format!("{object}/{STORAGE_FORMAT_FILE}"), Bytes::from(encoded)) + .await + .expect("raw regression metadata should be installed"); + } + + async fn write_raw_file_info_unchecked(disk: &DiskStore, bucket: &str, object: &str, file_info: FileInfo) { + let mut metadata = FileMeta::new(); + metadata + .add_version(file_info) + .expect("raw regression metadata should encode"); + write_raw_file_meta_unchecked(disk, bucket, object, metadata).await; + } + fn failed_read_repair_submitter(_request: rustfs_common::heal_channel::HealChannelRequest) -> ReadRepairAdmissionFuture { Box::pin(async { ReadRepairAdmissionOutcome::Failed("injected submit failure".to_string()) }) } @@ -4591,6 +4678,153 @@ mod tests { (0..count).map(|_| metadata_test_fileinfo(object)).collect() } + #[tokio::test] + async fn rename_data_skips_offline_placeholder_when_validating_new_metadata() { + let (_dirs, mut online_disks) = call_counter_local_disks("rename-validation-bucket", 1).await; + let online_disk = online_disks.pop().expect("one test disk should be present"); + let mut file_info = metadata_test_fileinfo("rename-unassigned-index"); + file_info.erasure.index = 0; + + let err = SetDisks::rename_data( + &[None, online_disk], + RUSTFS_META_TMP_BUCKET, + "source", + &[FileInfo::default(), file_info], + "bucket", + "object", + 1, + ) + .await + .expect_err("the missing staged source must fail after metadata validation"); + + assert_ne!(err, DiskError::FileCorrupt); + } + + #[tokio::test] + async fn rename_data_accepts_canonical_delete_marker() { + let bucket = "rename-delete-marker-bucket"; + let object = "object"; + let (_dirs, mut online_disks) = call_counter_local_disks(bucket, 1).await; + let online_disk = online_disks.pop().expect("one test disk slot should be present"); + let disk = online_disk.as_ref().expect("test disk should be online"); + match disk.make_volume(RUSTFS_META_TMP_BUCKET).await { + Ok(()) | Err(DiskError::VolumeExists) => {} + Err(err) => panic!("temporary metadata volume should be available: {err:?}"), + } + let version_id = Uuid::new_v4(); + let mut marker = metadata_test_delete_marker(object, version_id, OffsetDateTime::now_utc()); + marker + .metadata + .insert("x-rustfs-internal-purgestatus".to_string(), "pending".to_string()); + + SetDisks::rename_data( + &[None, online_disk.clone()], + RUSTFS_META_TMP_BUCKET, + "source", + &[FileInfo::default(), marker], + bucket, + object, + 1, + ) + .await + .expect("canonical delete marker should commit without erasure payload"); + + let stored = disk + .read_version("", bucket, object, &version_id.to_string(), &ReadOptions::default()) + .await + .expect("committed delete marker should be readable"); + + assert!(stored.deleted); + assert_eq!(stored.version_id, Some(version_id)); + assert_eq!(stored.erasure.index, 0); + assert_eq!(stored.metadata.get("x-rustfs-internal-purgestatus").map(String::as_str), Some("pending")); + } + + #[tokio::test] + async fn rename_data_preserves_null_delete_marker_type() { + let bucket = "rename-null-marker-bucket"; + let object = "object"; + let (_dirs, mut online_disks) = call_counter_local_disks(bucket, 1).await; + let online_disk = online_disks.pop().expect("one test disk slot should be present"); + let disk = online_disk.as_ref().expect("test disk should be online"); + match disk.make_volume(RUSTFS_META_TMP_BUCKET).await { + Ok(()) | Err(DiskError::VolumeExists) => {} + Err(err) => panic!("temporary metadata volume should be available: {err:?}"), + } + let marker = metadata_test_delete_marker(object, Uuid::new_v4(), OffsetDateTime::now_utc()); + let marker = FileInfo { + version_id: None, + ..marker + }; + + SetDisks::rename_data( + std::slice::from_ref(&online_disk), + RUSTFS_META_TMP_BUCKET, + "source", + &[marker], + bucket, + object, + 1, + ) + .await + .expect("null delete marker should commit"); + + let stored = disk + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("null delete marker should remain readable"); + assert!(stored.deleted); + assert_eq!(stored.version_id, None); + assert!(stored.is_canonical_delete_marker()); + } + + #[tokio::test] + async fn rename_delete_marker_quorum_failure_restores_existing_metadata() { + let bucket = "rename-marker-quorum-bucket"; + let object = "object"; + let (_dirs, mut online_disks) = call_counter_local_disks(bucket, 1).await; + let online_disk = online_disks.pop().expect("one test disk slot should be present"); + let disk = online_disk.as_ref().expect("test disk should be online"); + match disk.make_volume(RUSTFS_META_TMP_BUCKET).await { + Ok(()) | Err(DiskError::VolumeExists) => {} + Err(err) => panic!("temporary metadata volume should be available: {err:?}"), + } + let old_version_id = Uuid::new_v4(); + let mut old = metadata_test_fileinfo(object); + old.version_id = Some(old_version_id); + old.mod_time = Some(OffsetDateTime::now_utc()); + disk.write_metadata(bucket, bucket, object, old) + .await + .expect("old metadata should be written"); + let marker_version_id = Uuid::new_v4(); + let marker = metadata_test_delete_marker(object, marker_version_id, OffsetDateTime::now_utc()); + + let err = SetDisks::rename_data( + &[online_disk.clone(), None], + RUSTFS_META_TMP_BUCKET, + "source", + &[marker, FileInfo::default()], + bucket, + object, + 2, + ) + .await + .expect_err("quorum-minus-one marker commit should fail"); + + assert_eq!(err, DiskError::ErasureWriteQuorum); + let restored = disk + .read_version("", bucket, object, &old_version_id.to_string(), &ReadOptions::default()) + .await + .expect("old metadata should remain after rollback"); + assert!(!restored.deleted); + assert_eq!(restored.version_id, Some(old_version_id)); + assert!(matches!( + disk.read_version("", bucket, object, &marker_version_id.to_string(), &ReadOptions::default()) + .await, + Err(DiskError::FileVersionNotFound) + )); + } + /// Demo / regression guard for the backlog#1325 rename fan-out pause barrier /// and background-task introspection. Serves the barrier-style acceptance of /// #1312 ("assert no background disk write remains after release"). @@ -4793,6 +5027,47 @@ mod tests { assert_eq!(accumulator.final_miss_reason(), GET_METADATA_EARLY_STOP_REASON_ERROR); } + #[test] + fn metadata_quorum_accumulator_early_stops_on_one_delete_marker_majority() { + let marker = metadata_test_delete_marker("object", Uuid::new_v4(), OffsetDateTime::now_utc()); + let mut accumulator = MetadataQuorumAccumulator::new(6, 3, true); + + for _ in 0..4 { + accumulator.observe_file_info(&marker); + } + + assert_eq!(accumulator.default_write_quorum(), 4); + assert_eq!(accumulator.delete_marker_votes, 4); + assert_eq!( + accumulator.early_stop_decision(), + Some(MetadataEarlyStopDecision { + reason: GET_METADATA_EARLY_STOP_REASON_DELETE_MARKER, + }) + ); + } + + #[test] + fn metadata_quorum_accumulator_does_not_combine_distinct_delete_markers() { + let now = OffsetDateTime::now_utc(); + let first = metadata_test_delete_marker("object", Uuid::new_v4(), now); + let second = metadata_test_delete_marker("object", Uuid::new_v4(), now + time::Duration::seconds(1)); + let third = metadata_test_delete_marker("object", Uuid::new_v4(), now + time::Duration::seconds(2)); + let mut accumulator = MetadataQuorumAccumulator::new(4, 2, true); + + accumulator.observe_file_info(&first); + accumulator.observe_file_info(&second); + accumulator.observe_file_info(&third); + + assert_eq!(accumulator.delete_marker_votes, 1); + assert_eq!(accumulator.delete_marker_candidates.len(), 3); + assert_eq!(accumulator.early_stop_decision(), None); + + accumulator.observe_file_info(&second); + accumulator.observe_file_info(&second); + assert_eq!(accumulator.delete_marker_votes, 3); + assert!(accumulator.early_stop_decision().is_some()); + } + #[test] fn metadata_quorum_accumulator_candidate_latest_quorum_handles_zero_parity_and_invalid_candidates() { let accumulator = MetadataQuorumAccumulator::new(4, 0, true); @@ -4803,7 +5078,10 @@ mod tests { let accumulator = MetadataQuorumAccumulator::new(4, 2, true); let mut deleted = candidate.clone(); deleted.deleted = true; - assert_eq!(accumulator.candidate_latest_quorum(&deleted), None); + assert_eq!(accumulator.candidate_latest_quorum(&deleted), Some(3)); + + let marker = metadata_test_delete_marker("object", Uuid::new_v4(), OffsetDateTime::now_utc()); + assert_eq!(accumulator.candidate_latest_quorum(&marker), None); let mut empty = candidate.clone(); empty.size = 0; @@ -5052,6 +5330,59 @@ mod tests { assert_eq!(versions.versions[0].name, object); } + #[tokio::test] + async fn load_file_info_versions_exact_rejects_transitioned_duplicate_parts() { + let bucket = "exact-versions-bucket"; + let object = "poisoned-transitioned-object"; + let (_dir, disk) = read_multiple_test_disk(bucket, &[]).await; + let mut file_info = metadata_test_fileinfo(object); + file_info.version_id = Some(Uuid::new_v4()); + file_info.mod_time = Some(OffsetDateTime::now_utc()); + file_info.transition_status = TRANSITION_COMPLETE.to_string(); + file_info.transitioned_objname = "remote/object".to_string(); + file_info.transition_tier = "WARM".to_string(); + file_info.parts.push(file_info.parts[0].clone()); + write_raw_file_info_unchecked(&disk, bucket, object, file_info).await; + let set = io_primitives_test_set(vec![Some(disk)], 0).await; + + let err = set + .load_file_info_versions_exact(bucket, object) + .await + .expect_err("exact loader must reject metadata that would poison decommission"); + + assert!(err.to_string().contains("validation failed"), "unexpected error: {err}"); + } + + #[tokio::test] + async fn load_file_info_versions_exact_rejects_default_like_delete_marker() { + let bucket = "exact-versions-bucket"; + let object = "forged-delete-marker"; + let (_dir, disk) = read_multiple_test_disk(bucket, &[]).await; + let forged_version = rustfs_filemeta::FileMetaVersion { + version_type: rustfs_filemeta::VersionType::Delete, + delete_marker: Some(rustfs_filemeta::MetaDeleteMarker { + version_id: Some(Uuid::new_v4()), + mod_time: None, + ..Default::default() + }), + write_version: 1, + ..Default::default() + }; + let mut forged_meta = FileMeta::new(); + forged_meta + .versions + .push(FileMetaShallowVersion::try_from(forged_version).expect("forged marker body should encode")); + write_raw_file_meta_unchecked(&disk, bucket, object, forged_meta).await; + let set = io_primitives_test_set(vec![Some(disk)], 0).await; + + let err = set + .load_file_info_versions_exact(bucket, object) + .await + .expect_err("default-like delete marker must be rejected at the exact loader boundary"); + + assert!(err.to_string().contains("exact object versions decode failed"), "unexpected error: {err}"); + } + #[tokio::test] async fn commit_rename_data_dir_reclaims_old_data_dir_and_reports_receipt() { let bucket = "commit-rename-bucket"; diff --git a/crates/ecstore/src/set_disk/metadata.rs b/crates/ecstore/src/set_disk/metadata.rs index 060669956..136a6cf7e 100644 --- a/crates/ecstore/src/set_disk/metadata.rs +++ b/crates/ecstore/src/set_disk/metadata.rs @@ -245,12 +245,12 @@ impl SetDisks { continue; } - if !metadata.is_valid() { + if !file_info_is_valid_for_metadata(metadata) { parities[index] = -1; continue; } - if metadata.deleted || metadata.size == 0 { + if metadata.is_canonical_delete_marker() || metadata.size == 0 { parities[index] = half; } else if metadata.transition_status == TRANSITION_COMPLETE { let majority_metadata_parity = total_shards_i32 - (half + 1); @@ -327,7 +327,7 @@ impl SetDisks { for (i, etag_item) in etags.iter().enumerate() { if let Some(etag_item) = etag_item && etag_item == &etag - && parts_metadata[i].is_valid() + && file_info_is_valid_for_metadata(&parts_metadata[i]) { new_disk[i].clone_from(&disks[i]); } @@ -340,7 +340,7 @@ impl SetDisks { let mut new_disk = vec![None; disks.len()]; for (i, &t) in mod_times.iter().enumerate() { - if parts_metadata[i].is_valid() && mod_time == t { + if file_info_is_valid_for_metadata(&parts_metadata[i]) && mod_time == t { new_disk[i].clone_from(&disks[i]); } } @@ -357,7 +357,7 @@ impl SetDisks { continue; } - if meta.is_valid() { + if file_info_is_valid_for_metadata(meta) { usable_metadata += 1; } } @@ -388,7 +388,7 @@ impl SetDisks { let mut identity_counts = HashMap::with_capacity(usable_metadata); for (meta, err) in parts_metadata.iter().zip(errs.iter()) { - if err.is_some() || !meta.is_valid() { + if err.is_some() || !file_info_is_valid_for_metadata(meta) { continue; } @@ -613,7 +613,7 @@ impl SetDisks { } } - if !meta.deleted && meta.size != 0 { + if !meta.is_canonical_delete_marker() && meta.size != 0 { hasher.update(meta.erasure.data_blocks.to_le_bytes()); hasher.update(meta.erasure.parity_blocks.to_le_bytes()); hasher.update(meta.erasure.distribution.len().to_le_bytes()); @@ -626,7 +626,7 @@ impl SetDisks { fn latest_fileinfo_identity_groups(parts_metadata: &[FileInfo], errs: &[Option]) -> Vec { let mut groups: Vec = Vec::with_capacity(parts_metadata.len()); for (meta, err) in parts_metadata.iter().zip(errs.iter()) { - if err.is_some() || !meta.is_valid() { + if err.is_some() || !file_info_is_valid_for_metadata(meta) { continue; } @@ -658,7 +658,7 @@ impl SetDisks { let mut count = 0; for (i, ((meta, err), disk)) in parts_metadata.iter().zip(errs.iter()).zip(disks.iter()).enumerate() { - if err.is_some() || !meta.is_valid() || Self::file_info_quorum_hash(meta) != hash { + if err.is_some() || !file_info_is_valid_for_metadata(meta) || Self::file_info_quorum_hash(meta) != hash { continue; } @@ -731,7 +731,7 @@ impl SetDisks { let mut meta_hashes = vec![None; metas.len()]; for (i, meta) in metas.iter().enumerate() { - if !meta.is_valid() { + if !file_info_is_valid_for_metadata(meta) { debug!( index = i, valid = false, @@ -807,7 +807,7 @@ impl SetDisks { if let Some(hash) = op_hash && let Some(max_hash) = max_val && *hash == max_hash - && metas[i].is_valid() + && file_info_is_valid_for_metadata(&metas[i]) { if !found { found_fi = Some(metas[i].clone()); @@ -861,7 +861,7 @@ impl SetDisks { let mut inconsistent = 0; for (k, v) in parts_metadata.iter().enumerate() { - if disks[k].is_none() || !v.is_valid() || distribution[k] != v.erasure.index { + if disks[k].is_none() || !v.has_valid_erasure_geometry() || distribution[k] != v.erasure.index { inconsistent += 1; } } @@ -877,9 +877,9 @@ impl SetDisks { continue; } let eligible = if use_by_index { - parts_metadata[k].is_valid() && distribution[k] == parts_metadata[k].erasure.index + parts_metadata[k].has_valid_erasure_geometry() && distribution[k] == parts_metadata[k].erasure.index } else { - init || parts_metadata[k].is_valid() + init || parts_metadata[k].has_valid_erasure_geometry() }; if !eligible { continue; @@ -917,7 +917,7 @@ impl SetDisks { continue; } - if !v.is_valid() { + if !v.has_valid_erasure_geometry() { inconsistent += 1; continue; } @@ -963,7 +963,7 @@ impl SetDisks { continue; } - if !init && !parts_metadata[k].is_valid() { + if !init && !parts_metadata[k].has_valid_erasure_geometry() { continue; } @@ -1156,6 +1156,43 @@ mod tests { assert_ne!(SetDisks::file_info_quorum_hash(&left), SetDisks::file_info_quorum_hash(&right)); } + #[test] + fn purge_pending_quorum_hash_keeps_erasure_layouts_separate() { + let mod_time = OffsetDateTime::from_unix_timestamp(1_705_312_300).expect("valid timestamp"); + let version_id = Uuid::new_v4(); + let data_dir = Uuid::new_v4(); + let mut honest = FileInfo::new("bucket/object", 5, 1); + honest.name = "bucket/object".to_string(); + honest.version_id = Some(version_id); + honest.data_dir = Some(data_dir); + honest.mod_time = Some(mod_time); + honest.size = 1; + honest.deleted = true; + honest.add_object_part(1, "part-etag".to_string(), 1, Some(mod_time), 1, None, None); + + let mut parts_metadata = (1..=6) + .map(|index| { + let mut metadata = honest.clone(); + metadata.erasure.index = index; + metadata + }) + .collect::>(); + let mut tampered_layout = FileInfo::new("bucket/object", 3, 3).erasure; + tampered_layout.index = 1; + parts_metadata[0].erasure = tampered_layout; + let errs = vec![None; 6]; + + assert_eq!( + SetDisks::object_quorum_from_meta(&parts_metadata, &errs, 3) + .expect("five honest EC:1 payload copies should determine object quorum"), + (5, 5) + ); + let selected = SetDisks::find_file_info_in_quorum(&parts_metadata, &Some(mod_time), &None, 5) + .expect("the five matching EC:1 payload copies should determine metadata identity"); + assert_eq!(selected.erasure.data_blocks, 5); + assert_eq!(selected.erasure.parity_blocks, 1); + } + #[test] fn quorum_helpers_reject_zero_quorum_and_shuffle_check_parts_by_distribution() { let err = SetDisks::find_file_info_in_quorum(&[], &None, &None, 0).expect_err("zero quorum cannot select metadata"); diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index 80bbc82e0..ef9b3c955 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -224,6 +224,14 @@ pub(crate) const RUSTFS_MULTIPART_BUCKET_KEY: &str = "x-rustfs-internal-multipar pub(crate) const RUSTFS_MULTIPART_OBJECT_KEY: &str = "x-rustfs-internal-multipart-object"; const ENV_ISSUE3031_DIAG_ENABLE: &str = "RUSTFS_ISSUE3031_DIAG_ENABLE"; +/// Validate disk metadata at a boundary that may legitimately return a delete +/// marker. Disk/RPC decode boundaries perform the full collection validation +/// once; repeated quorum passes use the cheap erasure-geometry predicate for +/// payload entries and the canonical marker predicate for pure delete markers. +pub(in crate::set_disk) fn file_info_is_valid_for_metadata(file_info: &FileInfo) -> bool { + file_info.has_valid_metadata_shape() +} + struct ObjectLockDiagGuard { guard: NamespaceLockGuard, enabled: bool, @@ -1971,25 +1979,209 @@ fn issue3031_diag_enabled() -> bool { rustfs_utils::get_env_bool(ENV_ISSUE3031_DIAG_ENABLE, false) } -fn build_tiered_decommission_file_info( - bucket: &str, - object: &str, - fi: &FileInfo, - disk_count: usize, - default_parity_count: usize, - storage_class: Option<&str>, -) -> (FileInfo, usize) { - let parity_drives = runtime_sources::storage_class_parity(storage_class).unwrap_or(default_parity_count); - let data_drives = disk_count - parity_drives; - let mut write_quorum = data_drives; - if data_drives == parity_drives { - write_quorum += 1; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) struct WriteLayout { + data_drives: usize, + parity_drives: usize, + write_quorum: usize, +} + +impl WriteLayout { + fn from_parity(drive_count: usize, parity_drives: usize) -> Result { + let max_parity = drive_count / 2; + if parity_drives > max_parity { + return Err(Error::other(format!( + "write parity {parity_drives} exceeds the maximum {max_parity} for {drive_count} drives" + ))); + } + + let data_drives = drive_count + .checked_sub(parity_drives) + .filter(|&data_drives| data_drives > 0 && parity_drives <= data_drives) + .ok_or_else(|| Error::other(format!("invalid write layout with {drive_count} drives and parity {parity_drives}")))?; + let write_quorum = data_drives + .checked_add(usize::from(data_drives == parity_drives)) + .filter(|&write_quorum| write_quorum <= drive_count) + .ok_or_else(|| Error::other(format!("invalid write quorum for {drive_count} drives and parity {parity_drives}")))?; + + Ok(Self { + data_drives, + parity_drives, + write_quorum, + }) } +} + +pub(super) fn resolve_write_layout( + config: &storageclass::Config, + pool_index: usize, + drive_count: usize, + fallback_parity: usize, + storage_class: Option<&str>, + max_parity: bool, +) -> Result { + let configured_parity = if config.is_initialized() { + config + .parity_for_pool(storage_class.unwrap_or_default(), pool_index, drive_count) + .ok_or_else(|| { + Error::other(format!("storage class layout does not match pool {pool_index} with {drive_count} drives")) + })? + } else { + fallback_parity + }; + let parity_drives = if max_parity { drive_count / 2 } else { configured_parity }; + + WriteLayout::from_parity(drive_count, parity_drives) +} + +#[cfg(test)] +mod write_layout_tests { + use super::{WriteLayout, resolve_write_layout}; + use crate::config::storageclass::{ + CLASS_RRS, CLASS_STANDARD, INLINE_BLOCK_ENV, OPTIMIZE_ENV, RRS, RRS_ENV, STANDARD_ENV, lookup_config_for_pools, + lookup_config_for_pools_without_env, + }; + use arc_swap::ArcSwap; + use rustfs_config::server_config::KVS; + use std::sync::Arc; + + #[test] + fn automatic_standard_layout_is_resolved_per_pool() { + let config = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]) + .expect("automatic storage class should resolve for both pools"); + + assert_eq!( + resolve_write_layout(&config, 0, 4, 2, None, false).expect("first pool should resolve"), + WriteLayout { + data_drives: 2, + parity_drives: 2, + write_quorum: 3, + } + ); + assert_eq!( + resolve_write_layout(&config, 1, 2, 1, None, false).expect("second pool should resolve"), + WriteLayout { + data_drives: 1, + parity_drives: 1, + write_quorum: 2, + } + ); + } + + #[test] + fn reduced_redundancy_layout_allows_single_disk_zero_parity() { + let config = + lookup_config_for_pools_without_env(&KVS::new(), &[4, 1]).expect("reduced redundancy should resolve for both pools"); + + assert_eq!( + resolve_write_layout(&config, 0, 4, 2, Some(RRS), false).expect("four-drive RRS pool should resolve"), + WriteLayout { + data_drives: 3, + parity_drives: 1, + write_quorum: 3, + } + ); + assert_eq!( + resolve_write_layout(&config, 1, 1, 0, Some(RRS), false).expect("single-drive RRS pool should resolve"), + WriteLayout { + data_drives: 1, + parity_drives: 0, + write_quorum: 1, + } + ); + } + + #[test] + fn write_layout_rejects_unknown_topology_and_invalid_parity() { + let config = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("test storage class should resolve"); + + assert!(resolve_write_layout(&config, 2, 2, 1, None, false).is_err()); + assert!(resolve_write_layout(&config, 1, 4, 2, None, false).is_err()); + assert!(WriteLayout::from_parity(4, 3).is_err()); + assert!(WriteLayout::from_parity(0, 0).is_err()); + + let mut zero_parity_kvs = KVS::new(); + zero_parity_kvs.insert(CLASS_STANDARD.to_string(), "EC:0".to_string()); + zero_parity_kvs.insert(CLASS_RRS.to_string(), "EC:0".to_string()); + let zero_parity = lookup_config_for_pools_without_env(&zero_parity_kvs, &[4]).expect("zero-parity config should resolve"); + assert_eq!( + resolve_write_layout(&zero_parity, 0, 4, 2, None, true).expect("max parity should override configured parity"), + WriteLayout { + data_drives: 2, + parity_drives: 2, + write_quorum: 3, + } + ); + } + + #[test] + fn only_uninitialized_config_falls_back_to_pool_startup_parity() { + let uninitialized = crate::config::storageclass::Config::default(); + assert_eq!( + resolve_write_layout(&uninitialized, 99, 2, 0, None, false) + .expect("uninitialized config should preserve the pool's startup fallback"), + WriteLayout { + data_drives: 2, + parity_drives: 0, + write_quorum: 2, + } + ); + + let initialized = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("initialized config should resolve"); + assert!(resolve_write_layout(&initialized, 99, 2, 1, None, false).is_err()); + assert!(resolve_write_layout(&initialized, 1, 4, 2, None, false).is_err()); + } + + #[test] + #[serial_test::serial(storage_class_env)] + fn held_snapshot_keeps_parity_and_inline_policy_consistent_across_reload() { + let old = temp_env::with_vars( + [ + (STANDARD_ENV, Some("")), + (RRS_ENV, Some("")), + (OPTIMIZE_ENV, None), + (INLINE_BLOCK_ENV, Some("1KiB")), + ], + || lookup_config_for_pools(&KVS::new(), &[4, 2]), + ) + .expect("old config should resolve"); + let new = temp_env::with_vars( + [ + (STANDARD_ENV, Some("EC:1")), + (RRS_ENV, Some("EC:1")), + (OPTIMIZE_ENV, None), + (INLINE_BLOCK_ENV, Some("0B")), + ], + || lookup_config_for_pools(&KVS::new(), &[4, 2]), + ) + .expect("new config should resolve"); + + let published = ArcSwap::from_pointee(old); + let held = published.load_full(); + published.store(Arc::new(new)); + + let held_layout = resolve_write_layout(&held, 0, 4, 2, None, false).expect("held snapshot should remain valid"); + assert_eq!(held_layout.parity_drives, 2); + assert!(held.should_inline(512, false)); + + let current = published.load_full(); + let current_layout = resolve_write_layout(¤t, 0, 4, 2, None, false).expect("new snapshot should resolve"); + assert_eq!(current_layout.parity_drives, 1); + assert!(!current.should_inline(512, false)); + } +} + +fn build_tiered_decommission_file_info(bucket: &str, object: &str, fi: &FileInfo, layout: WriteLayout) -> FileInfo { + let WriteLayout { + data_drives, + parity_drives, + .. + } = layout; let mut updated = fi.clone(); updated.erasure = FileInfo::new([bucket, object].join("/").as_str(), data_drives, parity_drives).erasure; - (updated, write_quorum) + updated } fn resolve_tiered_decommission_write_quorum_result( @@ -2036,6 +2228,8 @@ pub struct SetDisks { /// writes skip the global registry mutex (backlog#1315). `Arc` so clones of /// a set share one generation marker. capacity_dirty_generation: Arc, + #[cfg(test)] + storage_class_config_override: Arc>>>, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -2168,6 +2362,28 @@ impl DiskHealthEntry { } impl SetDisks { + fn storage_class_config_snapshot(&self) -> Arc { + #[cfg(test)] + if let Some(config) = self + .storage_class_config_override + .read() + .expect("test storage class override lock should not be poisoned") + .as_ref() + { + return config.clone(); + } + + runtime_sources::storage_class_config_snapshot() + } + + #[cfg(test)] + pub(crate) fn set_test_storage_class_config(&self, config: storageclass::Config) { + *self + .storage_class_config_override + .write() + .expect("test storage class override lock should not be poisoned") = Some(Arc::new(config)); + } + fn get_object_metadata_cache_hash(&self, bucket: &str, object: &str) -> u64 { let mut hasher = self.get_object_metadata_cache_hash_builder.build_hasher(); bucket.hash(&mut hasher); @@ -2372,6 +2588,8 @@ impl SetDisks { ctx, capacity_scope_cache: Arc::new(std::sync::RwLock::new(CapacityScopeCache::default())), capacity_dirty_generation: Arc::new(AtomicU64::new(u64::MAX)), + #[cfg(test)] + storage_class_config_override: Arc::new(std::sync::RwLock::new(None)), }) } @@ -2888,7 +3106,7 @@ fn collect_inline_data_shard_fileinfos_by_index<'a>( if block_index == 0 || block_index > data_shards { continue; } - if !file_info.is_valid() { + if !file_info.has_valid_erasure_geometry() { continue; } if file_info.data.as_ref().is_none_or(|data| data.is_empty()) { @@ -3322,6 +3540,7 @@ impl SetDisks { fi: &FileInfo, opts: &ObjectOptions, ) -> Result<()> { + let storage_class_config = self.storage_class_config_snapshot(); let _lock_guard = if !opts.no_lock { Some( self.new_ns_lock(bucket, object) @@ -3336,8 +3555,16 @@ impl SetDisks { let disks = self.disks.read().await.clone(); let storage_class = opts.user_defined.get(AMZ_STORAGE_CLASS).map(String::as_str); - let (fi, write_quorum) = - build_tiered_decommission_file_info(bucket, object, fi, disks.len(), self.default_parity_count, storage_class); + let layout = resolve_write_layout( + &storage_class_config, + self.pool_index, + disks.len(), + self.default_parity_count, + storage_class, + opts.max_parity, + )?; + let fi = build_tiered_decommission_file_info(bucket, object, fi, layout); + let write_quorum = layout.write_quorum; let parts_metadata = vec![fi.clone(); disks.len()]; let (shuffle_disks, parts_metadata) = Self::shuffle_disks_and_parts_metadata(&disks, &parts_metadata, &fi); @@ -3407,13 +3634,13 @@ fn is_object_dangling( let mut valid_meta = FileInfo::default(); for fi in meta_arr.iter() { - if fi.is_valid() { + if file_info_is_valid_for_metadata(fi) { valid_meta = fi.clone(); break; } } - if !valid_meta.is_valid() { + if !file_info_is_valid_for_metadata(&valid_meta) { let data_blocks = meta_arr.len().div_ceil(2); if not_found_parts_errs > data_blocks { return (valid_meta, true); @@ -3426,7 +3653,7 @@ fn is_object_dangling( return (valid_meta, false); } - if valid_meta.deleted { + if valid_meta.is_canonical_delete_marker() { let data_blocks = errs.len().div_ceil(2); return (valid_meta, not_found_meta_errs > data_blocks); } @@ -3539,12 +3766,12 @@ async fn disks_with_all_parts( // Check for inconsistent erasure distribution let mut inconsistent = 0; for (index, meta) in parts_metadata.iter().enumerate() { - if !meta.is_valid() { + if !file_info_is_valid_for_metadata(meta) { // Since for majority of the cases erasure.Index matches with erasure.Distribution we can // consider the offline disks as consistent. continue; } - if !meta.deleted { + if !meta.is_canonical_delete_marker() { if meta.erasure.distribution.len() != online_disks.len() { // Erasure distribution seems to have lesser // number of items than number of online disks. @@ -3604,7 +3831,7 @@ async fn disks_with_all_parts( } if erasure_distribution_reliable { - if !meta.is_valid() { + if !file_info_is_valid_for_metadata(meta) { info!( "disks_with_all_partsv2: metadata is not valid, object_name={}, index: {index}", object_name @@ -3615,7 +3842,7 @@ async fn disks_with_all_parts( continue; } - if !meta.deleted && meta.erasure.distribution.len() != online_disks_len { + if !meta.is_canonical_delete_marker() && meta.erasure.distribution.len() != online_disks_len { // Erasure distribution is not the same as onlineDisks // attempt a fix if possible, assuming other entries // might have the right erasure distribution. @@ -3658,7 +3885,7 @@ async fn disks_with_all_parts( }; let meta = &mut parts_metadata[index]; - if meta.deleted || meta.is_remote() { + if meta.is_canonical_delete_marker() || meta.is_remote() { continue; } @@ -3788,7 +4015,7 @@ pub fn should_heal_object_on_disk( return (true, true, Some(DiskError::OutdatedXLMeta)); } - if !meta.deleted && !meta.is_remote() { + if !meta.is_canonical_delete_marker() && !meta.is_remote() { let err_vec = [CHECK_PART_FILE_NOT_FOUND, CHECK_PART_FILE_CORRUPT]; for part_err in parts_errs.iter() { if err_vec.contains(part_err) { @@ -6356,6 +6583,7 @@ mod tests { erasure: ErasureInfo { data_blocks: 4, parity_blocks: 2, + block_size: 4, index: 1, // Must be > 0 for is_valid() to return true distribution: vec![1, 2, 3, 4, 5, 6], // Must match data_blocks + parity_blocks ..Default::default() @@ -6368,6 +6596,7 @@ mod tests { erasure: ErasureInfo { data_blocks: 6, parity_blocks: 3, + block_size: 4, index: 1, // Must be > 0 for is_valid() to return true distribution: vec![1, 2, 3, 4, 5, 6, 7, 8, 9], // Must match data_blocks + parity_blocks ..Default::default() @@ -6380,6 +6609,7 @@ mod tests { erasure: ErasureInfo { data_blocks: 2, parity_blocks: 1, + block_size: 4, index: 1, // Must be > 0 for is_valid() to return true distribution: vec![1, 2, 3], // Must match data_blocks + parity_blocks ..Default::default() @@ -6399,6 +6629,102 @@ mod tests { assert_eq!(parities[2], 1); // half of total shards (3/2 = 1) for zero size file } + #[test] + fn delete_markers_participate_in_four_disk_metadata_quorum_without_erasure_geometry() { + let marker = FileInfo { + name: "bucket/deleted".to_string(), + deleted: true, + version_id: Some(Uuid::new_v4()), + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + let parts_metadata = vec![marker; 4]; + let errs = vec![None; 4]; + + assert!(parts_metadata.iter().all(file_info_is_valid_for_metadata)); + assert!(parts_metadata.iter().all(|metadata| !metadata.is_valid())); + assert_eq!(SetDisks::list_object_parities(&parts_metadata, &errs), vec![2; 4]); + assert_eq!( + SetDisks::object_quorum_from_meta(&parts_metadata, &errs, 2) + .expect("four matching delete markers must reach metadata quorum"), + (2, 3) + ); + } + + #[test] + fn metadata_boundary_does_not_relax_non_delete_or_malformed_delete_metadata() { + assert!(!file_info_is_valid_for_metadata(&FileInfo::default())); + + let mut transitioned = FileInfo::new("bucket/transitioned", 2, 2); + transitioned.erasure.index = 1; + transitioned.transition_status = TRANSITION_COMPLETE.to_string(); + assert!(file_info_is_valid_for_metadata(&transitioned)); + + transitioned.erasure = ErasureInfo::default(); + assert!( + !file_info_is_valid_for_metadata(&transitioned), + "transition state must not relax local erasure validation" + ); + + let mut purge_pending = FileInfo::new("bucket/purge-pending", 2, 2); + purge_pending.erasure.index = 1; + purge_pending.deleted = true; + purge_pending.parts.push(ObjectPartInfo { + number: 1, + ..Default::default() + }); + assert!( + file_info_is_valid_for_metadata(&purge_pending), + "purge-pending payload metadata must retain its valid erasure vote" + ); + assert!(!purge_pending.is_canonical_delete_marker()); + + let mut malformed_marker = FileInfo { + deleted: true, + ..Default::default() + }; + malformed_marker.parts = vec![ + ObjectPartInfo { + number: 1, + ..Default::default() + }, + ObjectPartInfo { + number: 1, + ..Default::default() + }, + ]; + assert!(!file_info_is_valid_for_metadata(&malformed_marker)); + } + + #[test] + fn purge_pending_payload_uses_its_erasure_parity_for_metadata_quorum() { + let version_id = Uuid::new_v4(); + let mod_time = OffsetDateTime::now_utc(); + let parts_metadata = (1..=6) + .map(|disk_index| { + let mut purge_pending = FileInfo::new("bucket/purge-pending", 5, 1); + purge_pending.name = "bucket/purge-pending".to_string(); + purge_pending.version_id = Some(version_id); + purge_pending.mod_time = Some(mod_time); + purge_pending.size = 1; + purge_pending.deleted = true; + purge_pending.erasure.index = disk_index; + purge_pending.add_object_part(1, "part-etag-1".to_string(), 1, None, 1, None, None); + purge_pending + }) + .collect::>(); + let errs = vec![None; 6]; + + assert!(parts_metadata.iter().all(file_info_is_valid_for_metadata)); + assert!(parts_metadata.iter().all(|metadata| !metadata.is_canonical_delete_marker())); + assert_eq!(SetDisks::list_object_parities(&parts_metadata, &errs), vec![1; 6]); + assert_eq!( + SetDisks::object_quorum_from_meta(&parts_metadata, &errs, 3) + .expect("purge-pending payload should retain its EC:1 quorum"), + (5, 5) + ); + } + #[test] fn test_conv_part_err_to_int() { // Test error conversion to integer codes @@ -7141,7 +7467,8 @@ mod tests { ..Default::default() }; - let (updated, write_quorum) = build_tiered_decommission_file_info("bucket", "object", &original, 16, 4, None); + let layout = WriteLayout::from_parity(16, 4).expect("tiered write layout should be valid"); + let updated = build_tiered_decommission_file_info("bucket", "object", &original, layout); assert_eq!(updated.version_id, original.version_id); assert_eq!(updated.transition_status, original.transition_status); @@ -7150,7 +7477,7 @@ mod tests { assert_eq!(updated.transition_version_id, original.transition_version_id); assert_eq!(updated.erasure.data_blocks, 12); assert_eq!(updated.erasure.parity_blocks, 4); - assert_eq!(write_quorum, 12); + assert_eq!(layout.write_quorum, 12); assert_ne!(updated.erasure.distribution, original.erasure.distribution); } @@ -8395,11 +8722,15 @@ mod tests { } async fn make_local_bucket_test_set_disks() -> Arc { - let format = FormatV3::new(1, 2); + make_local_bucket_test_set_disks_with_drive_count(2).await + } + + async fn make_local_bucket_test_set_disks_with_drive_count(drive_count: usize) -> Arc { + let format = FormatV3::new(1, drive_count); let mut endpoints = Vec::new(); let mut disks = Vec::new(); - for disk_idx in 0..2 { + for disk_idx in 0..drive_count { let dir = tempfile::tempdir().expect("tempdir should be created"); let mut endpoint = Endpoint::try_from(dir.path().to_str().expect("tempdir path should be utf8")).expect("endpoint should parse"); @@ -8428,18 +8759,23 @@ mod tests { disks.push(Some(disk)); } - SetDisks::new( + let set_disks = SetDisks::new( "test-owner".to_string(), Arc::new(RwLock::new(disks)), - 2, - 1, + drive_count, + drive_count / 2, 0, 0, endpoints, format, Vec::new(), ) - .await + .await; + set_disks.set_test_storage_class_config( + storageclass::lookup_config_for_pools_without_env(&rustfs_config::server_config::KVS::new(), &[drive_count]) + .expect("test storage class should resolve for the local drive count"), + ); + set_disks } async fn make_local_bucket_test_set_disks_with_missing_format() -> Arc { @@ -8909,7 +9245,7 @@ mod tests { #[tokio::test] async fn set_level_versioned_delete_marker_hides_object_without_corrupting_version_metadata() { - let set_disks = make_local_bucket_test_set_disks().await; + let set_disks = make_local_bucket_test_set_disks_with_drive_count(4).await; let bucket = "bucket-versioned-delete"; let object = "object.txt"; let opts = ObjectOptions { diff --git a/crates/ecstore/src/set_disk/ops/heal.rs b/crates/ecstore/src/set_disk/ops/heal.rs index 4df265558..5d02a2f5f 100644 --- a/crates/ecstore/src/set_disk/ops/heal.rs +++ b/crates/ecstore/src/set_disk/ops/heal.rs @@ -771,7 +771,7 @@ impl SetDisks { // A surviving valid, non-deleted, non-remote data FileInfo to rebuild from. let Some(surviving) = parts_metadata .iter() - .find(|fi| fi.is_valid() && !fi.deleted && !fi.is_remote()) + .find(|fi| fi.has_valid_erasure_geometry() && !fi.deleted && !fi.is_remote()) .cloned() else { return Ok(false); @@ -1013,7 +1013,13 @@ impl SetDisks { ..Default::default() }; - if lfi.is_valid() { + // Report the object's own parity only when it actually carries erasure + // geometry; delete markers and geometry-less versions fall back to the + // pool default. Uses `has_valid_erasure_geometry()` (not `is_valid()`) + // to stay in step with the rest of the metadata-predicate migration — + // `is_valid()` now requires full payload validation and returns `false` + // for delete markers, which would misreport their parity here. + if lfi.has_valid_erasure_geometry() { result.parity_blocks = lfi.erasure.parity_blocks; } else { result.parity_blocks = self.default_parity_count; diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index f281549d8..e246e72e9 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -845,6 +845,7 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { #[tracing::instrument(skip(self))] async fn new_multipart_upload(&self, bucket: &str, object: &str, opts: &ObjectOptions) -> Result { crate::hp_guard!("SetDisks::new_multipart_upload"); + let storage_class_config = self.storage_class_config_snapshot(); let mut _object_lock_guard = None; if opts.http_preconditions.is_some() { @@ -876,18 +877,18 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { let _ = user_defined.remove(AMZ_STORAGE_CLASS); } - let sc_parity_drives = runtime_sources::storage_class_parity(user_defined.get(AMZ_STORAGE_CLASS).map(String::as_str)); - - let mut parity_drives = sc_parity_drives.unwrap_or(self.default_parity_count); - if opts.max_parity { - parity_drives = disks.len() / 2; - } - - let data_drives = disks.len() - parity_drives; - let mut write_quorum = data_drives; - if data_drives == parity_drives { - write_quorum += 1 - } + let WriteLayout { + data_drives, + parity_drives, + write_quorum, + } = resolve_write_layout( + &storage_class_config, + self.pool_index, + disks.len(), + self.default_parity_count, + user_defined.get(AMZ_STORAGE_CLASS).map(String::as_str), + opts.max_parity, + )?; let mut fi = FileInfo::new([bucket, object].join("/").as_str(), data_drives, parity_drives); @@ -1369,7 +1370,7 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { } for meta in parts_metadatas.iter_mut() { - if meta.is_valid() { + if meta.has_valid_erasure_geometry() { meta.size = fi.size; meta.mod_time = fi.mod_time; meta.parts.clone_from(&fi.parts); @@ -1556,10 +1557,14 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { #[cfg(test)] mod tests { use super::*; + use crate::config::storageclass::lookup_config_for_pools_without_env; use crate::disk::DiskAPI as _; use crate::disk::{endpoint::Endpoint, format::FormatV3}; - use crate::set_disk::ops::object::hermetic_set_disks_support::hermetic_set_disks; + use crate::set_disk::ops::object::hermetic_set_disks_support::{ + hermetic_set_disks, hermetic_set_disks_for_pool_with_default_parity, + }; use crate::storage_api_contracts::namespace::NamespaceLocking as _; + use rustfs_config::server_config::KVS; use rustfs_lock::{LockClient, client::local::LocalClient}; use serial_test::serial; use tempfile::TempDir; @@ -1890,6 +1895,65 @@ mod tests { ); } + #[tokio::test] + async fn second_pool_multipart_uses_its_own_layout_and_round_trips() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_for_pool_with_default_parity(2, 1, 2).await; + set_disks.set_test_storage_class_config( + lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("heterogeneous pool storage class should resolve"), + ); + + let bucket = "multipart-second-pool-bucket"; + let object = "object"; + for disk in &disk_stores { + disk.make_volume(bucket).await.expect("bucket volume should be created"); + } + + let upload = set_disks + .new_multipart_upload(bucket, object, &ObjectOptions::default()) + .await + .expect("second-pool multipart upload should be created"); + let (upload_info, _) = set_disks + .check_upload_id_exists(bucket, object, &upload.upload_id, false) + .await + .expect("stored multipart layout should be readable"); + assert_eq!(upload_info.erasure.data_blocks, 1); + assert_eq!(upload_info.erasure.parity_blocks, 1); + + let payload = vec![0x5a; 4096]; + let mut reader = PutObjReader::from_vec(payload.clone()); + let part = set_disks + .put_object_part(bucket, object, &upload.upload_id, 1, &mut reader, &ObjectOptions::default()) + .await + .expect("second-pool part should encode without zero data shards"); + set_disks + .clone() + .complete_multipart_upload( + bucket, + object, + &upload.upload_id, + vec![CompletePart { + part_num: part.part_num, + etag: part.etag, + ..Default::default() + }], + &ObjectOptions::default(), + ) + .await + .expect("second-pool multipart upload should complete"); + + let mut object_reader = set_disks + .get_object_reader(bucket, object, None, HeaderMap::new(), &ObjectOptions::default()) + .await + .expect("completed second-pool object should be readable"); + let mut restored = Vec::new(); + object_reader + .stream + .read_to_end(&mut restored) + .await + .expect("completed second-pool object should stream"); + assert_eq!(restored, payload); + } + #[tokio::test] async fn list_multipart_uploads_caps_each_page_at_max_uploads_and_paginates_cleanly() { let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index e35c8904a..27cc00da7 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -702,6 +702,7 @@ impl SetDisks { opts: &ObjectOptions, ) -> Result<(ObjectInfo, Option)> { crate::hp_guard!("SetDisks::put_object"); + let storage_class_config = self.storage_class_config_snapshot(); self.invalidate_get_object_metadata_cache(bucket, object).await; let disks = self.get_disks_internal().await; @@ -727,18 +728,18 @@ impl SetDisks { user_defined.insert(key.clone(), value.clone()); } } - let sc_parity_drives = runtime_sources::storage_class_parity(user_defined.get(AMZ_STORAGE_CLASS).map(String::as_str)); - - let mut parity_drives = sc_parity_drives.unwrap_or(self.default_parity_count); - if opts.max_parity { - parity_drives = disks.len() / 2; - } - - let data_drives = disks.len() - parity_drives; - let mut write_quorum = data_drives; - if data_drives == parity_drives { - write_quorum += 1 - } + let WriteLayout { + data_drives, + parity_drives, + write_quorum, + } = resolve_write_layout( + &storage_class_config, + self.pool_index, + disks.len(), + self.default_parity_count, + user_defined.get(AMZ_STORAGE_CLASS).map(String::as_str), + opts.max_parity, + )?; // if filtered_online < write_quorum { // warn!( @@ -776,8 +777,7 @@ impl SetDisks { let erasure = erasure_from_file_info(&fi, false)?; let put_object_size = known_put_object_storage_size(data.size()); - let is_inline_buffer = - runtime_sources::storage_class_should_inline(erasure.shard_file_size(put_object_size), opts.versioned); + let is_inline_buffer = storage_class_config.should_inline(erasure.shard_file_size(put_object_size), opts.versioned); let shard_file_size = erasure.shard_file_size(put_object_size); let shard_size = erasure.shard_size(); @@ -1946,7 +1946,7 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { let inline_data = fi.inline_data(); for fi in metas.iter_mut() { - if fi.is_valid() { + if fi.has_valid_erasure_geometry() { fi.metadata = (*src_info.user_defined).clone(); if let Some(etag) = &src_info.etag { fi.metadata.insert("etag".to_owned(), etag.clone()); @@ -3402,14 +3402,15 @@ pub(in crate::set_disk::ops) mod hermetic_set_disks_support { use tempfile::TempDir; use tokio::sync::RwLock; - pub(in crate::set_disk::ops) async fn make_formatted_local_disk( + async fn make_formatted_local_disk_for_pool( disk_idx: usize, + pool_index: usize, format: &FormatV3, ) -> (TempDir, Endpoint, DiskStore) { let dir = tempfile::tempdir().expect("tempdir should be created"); let mut endpoint = Endpoint::try_from(dir.path().to_str().expect("tempdir path should be utf8")).expect("endpoint should parse"); - endpoint.set_pool_index(0); + endpoint.set_pool_index(pool_index); endpoint.set_set_index(0); endpoint.set_disk_index(disk_idx); @@ -3433,6 +3434,14 @@ pub(in crate::set_disk::ops) mod hermetic_set_disks_support { } pub(in crate::set_disk::ops) async fn hermetic_set_disks(disk_count: usize) -> (Vec, Vec, Arc) { + hermetic_set_disks_for_pool_with_default_parity(disk_count, 0, disk_count / 2).await + } + + pub(in crate::set_disk::ops) async fn hermetic_set_disks_for_pool_with_default_parity( + disk_count: usize, + pool_index: usize, + default_parity_count: usize, + ) -> (Vec, Vec, Arc) { let format = FormatV3::new(1, disk_count); let mut temp_dirs = Vec::with_capacity(disk_count); @@ -3441,7 +3450,7 @@ pub(in crate::set_disk::ops) mod hermetic_set_disks_support { let mut disks = Vec::with_capacity(disk_count); for disk_idx in 0..disk_count { - let (temp_dir, endpoint, disk) = make_formatted_local_disk(disk_idx, &format).await; + let (temp_dir, endpoint, disk) = make_formatted_local_disk_for_pool(disk_idx, pool_index, &format).await; temp_dirs.push(temp_dir); endpoints.push(endpoint); disk_stores.push(disk.clone()); @@ -3452,9 +3461,9 @@ pub(in crate::set_disk::ops) mod hermetic_set_disks_support { "hermetic-ops-test-owner".to_string(), Arc::new(RwLock::new(disks)), disk_count, - disk_count / 2, - 0, + default_parity_count, 0, + pool_index, endpoints, format, Vec::new(), @@ -4201,6 +4210,61 @@ mod transition_source_identity_matrix_tests { } } +#[cfg(test)] +mod heterogeneous_pool_put_tests { + use super::hermetic_set_disks_support::hermetic_set_disks_for_pool_with_default_parity; + use super::*; + use crate::config::storageclass::lookup_config_for_pools_without_env; + use crate::disk::{DiskAPI as _, ReadOptions}; + use rustfs_config::server_config::KVS; + use tokio::io::AsyncReadExt; + + #[tokio::test] + async fn second_pool_regular_put_uses_its_own_layout_and_round_trips() { + // Deliberately inject the first pool's invalid scalar fallback. The + // test can pass only if the production PUT uses the held [4, 2] + // storage-class snapshot and resolves pool 1 to parity 1. + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_for_pool_with_default_parity(2, 1, 2).await; + set_disks.set_test_storage_class_config( + lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("heterogeneous pool storage class should resolve"), + ); + + let bucket = "regular-put-second-pool-bucket"; + let object = "object"; + for disk in &disk_stores { + disk.make_volume(bucket).await.expect("bucket volume should be created"); + } + + let payload = vec![0x3c; 4096]; + let mut reader = PutObjReader::from_vec(payload.clone()); + set_disks + .put_object(bucket, object, &mut reader, &ObjectOptions::default()) + .await + .expect("second-pool regular PUT should encode without zero data shards"); + + for (disk_index, disk) in disk_stores.iter().enumerate() { + let file_info = disk + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .unwrap_or_else(|err| panic!("disk {disk_index} should persist valid second-pool metadata: {err}")); + assert_eq!(file_info.erasure.data_blocks, 1); + assert_eq!(file_info.erasure.parity_blocks, 1); + } + + let mut object_reader = set_disks + .get_object_reader(bucket, object, None, HeaderMap::new(), &ObjectOptions::default()) + .await + .expect("second-pool regular PUT should be readable"); + let mut restored = Vec::new(); + object_reader + .stream + .read_to_end(&mut restored) + .await + .expect("second-pool regular PUT should stream"); + assert_eq!(restored, payload); + } +} + #[cfg(test)] mod put_object_tmp_cleanup_tests { //! Regression coverage for backlog#924 (HP-3): the speculative tmp-dir diff --git a/crates/ecstore/src/set_disk/read.rs b/crates/ecstore/src/set_disk/read.rs index 880e1421d..bc7f37736 100644 --- a/crates/ecstore/src/set_disk/read.rs +++ b/crates/ecstore/src/set_disk/read.rs @@ -121,7 +121,7 @@ impl SetDisks { online_disks: &[Option], read_quorum: usize, ) { - if fi.deleted || !fi.is_valid() { + if fi.deleted || !fi.has_valid_erasure_geometry() { return; } let (bucket, object) = identity; @@ -3192,10 +3192,13 @@ mod tests { let mut historical = metadata_fanout_test_fileinfo("object"); historical.version_id = Some(Uuid::parse_str("00000000-0000-0000-0000-000000000002").expect("static uuid should parse")); - let mut delete_marker = metadata_fanout_test_fileinfo("object"); - delete_marker.deleted = true; - delete_marker.version_id = - Some(Uuid::parse_str("00000000-0000-0000-0000-000000000003").expect("static uuid should parse")); + let delete_marker = FileInfo { + name: "object".to_string(), + deleted: true, + version_id: Some(Uuid::parse_str("00000000-0000-0000-0000-000000000003").expect("static uuid should parse")), + mod_time: Some(OffsetDateTime::from_unix_timestamp(3).expect("static timestamp should parse")), + ..Default::default() + }; let diagnostics = MetadataFanoutDiagnostics::new( Duration::from_millis(9), @@ -3296,6 +3299,47 @@ mod tests { assert_eq!(accumulator.final_miss_reason(), GET_METADATA_EARLY_STOP_REASON_CONFLICTING_METADATA); } + #[test] + fn metadata_quorum_accumulator_rejects_semantic_field_splits() { + type FileInfoMutation = fn(&mut FileInfo); + + let mutations: &[(&str, FileInfoMutation)] = &[ + ("transition_status", |fi| fi.transition_status = "complete".to_string()), + ("transitioned_objname", |fi| fi.transitioned_objname = "remote-object".to_string()), + ("transition_tier", |fi| fi.transition_tier = "WARM".to_string()), + ("transition_version_id", |fi| fi.transition_version_id = Some(Uuid::from_u128(10))), + ("expire_restored", |fi| fi.expire_restored = true), + ("written_by_version", |fi| fi.written_by_version = Some(1)), + ("replication_state_internal", |fi| { + fi.replication_state_internal = Some(Default::default()) + }), + ("num_versions", |fi| fi.num_versions = 2), + ("successor_mod_time", |fi| { + fi.successor_mod_time = Some(OffsetDateTime::from_unix_timestamp(10).expect("static timestamp should parse")); + }), + ]; + + for (field, mutate) in mutations { + let mut accumulator = metadata_early_stop_accumulator(); + let first = metadata_early_stop_candidate("object", 1); + let mut second = metadata_early_stop_candidate("object", 2); + let mut third = metadata_early_stop_candidate("object", 3); + mutate(&mut second); + mutate(&mut third); + + accumulator.observe_file_info(&first); + accumulator.observe_file_info(&second); + accumulator.observe_file_info(&third); + + assert!(accumulator.conflicting_metadata, "split {field} must block metadata early-stop"); + assert!( + accumulator.early_stop_decision().is_none(), + "split {field} must not be mistaken for three matching votes" + ); + assert_eq!(accumulator.final_miss_reason(), GET_METADATA_EARLY_STOP_REASON_CONFLICTING_METADATA); + } + } + #[test] fn metadata_quorum_accumulator_falls_back_on_split_data_dir() { let mut accumulator = metadata_early_stop_accumulator(); @@ -3325,8 +3369,15 @@ mod tests { #[test] fn metadata_quorum_accumulator_hits_delete_marker_quorum_early_stop() { let mut accumulator = metadata_early_stop_accumulator(); - let mut deleted = metadata_early_stop_candidate("object", 1); - deleted.deleted = true; + let deleted = FileInfo { + name: "object".to_string(), + deleted: true, + version_id: Some(Uuid::new_v4()), + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + + assert!(!deleted.is_valid(), "real delete markers do not carry erasure geometry"); accumulator.observe_file_info(&deleted); accumulator.observe_file_info(&deleted); @@ -3344,8 +3395,13 @@ mod tests { #[test] fn metadata_quorum_accumulator_falls_back_on_delete_marker_below_quorum() { let mut accumulator = metadata_early_stop_accumulator(); - let mut deleted = metadata_early_stop_candidate("object", 1); - deleted.deleted = true; + let deleted = FileInfo { + name: "object".to_string(), + deleted: true, + version_id: Some(Uuid::parse_str("00000000-0000-0000-0000-000000000004").expect("static uuid should parse")), + mod_time: Some(OffsetDateTime::from_unix_timestamp(4).expect("static timestamp should parse")), + ..Default::default() + }; accumulator.observe_file_info(&deleted); @@ -3353,6 +3409,49 @@ mod tests { assert_eq!(accumulator.final_miss_reason(), GET_METADATA_EARLY_STOP_REASON_DELETE_MARKER); } + #[test] + fn metadata_quorum_accumulator_does_not_treat_purge_pending_payload_as_delete_marker() { + let mut accumulator = MetadataQuorumAccumulator::new(6, 3, true); + let version_id = Uuid::parse_str("00000000-0000-0000-0000-000000000005").expect("static uuid should parse"); + + for disk_index in 1..=4 { + let mut purge_pending = FileInfo::new("object", 5, 1); + purge_pending.name = "object".to_string(); + purge_pending.version_id = Some(version_id); + purge_pending.mod_time = Some(OffsetDateTime::from_unix_timestamp(5).expect("static timestamp should parse")); + purge_pending.size = 1; + purge_pending.deleted = true; + purge_pending.erasure.index = disk_index; + purge_pending.add_object_part(1, "part-etag-1".to_string(), 1, None, 1, None, None); + + accumulator.observe_file_info(&purge_pending); + } + + assert!(!accumulator.delete_marker_seen); + assert_eq!(accumulator.candidate_votes, 4); + assert!( + accumulator.early_stop_decision().is_none(), + "EC:1 purge-pending payload on six disks still requires five matching payload votes" + ); + + let mut fifth = FileInfo::new("object", 5, 1); + fifth.name = "object".to_string(); + fifth.version_id = Some(version_id); + fifth.mod_time = Some(OffsetDateTime::from_unix_timestamp(5).expect("static timestamp should parse")); + fifth.size = 1; + fifth.deleted = true; + fifth.erasure.index = 5; + fifth.add_object_part(1, "part-etag-1".to_string(), 1, None, 1, None, None); + accumulator.observe_file_info(&fifth); + + assert_eq!( + accumulator.early_stop_decision(), + Some(MetadataEarlyStopDecision { + reason: GET_METADATA_EARLY_STOP_REASON_VALID_QUORUM + }) + ); + } + #[test] fn metadata_quorum_accumulator_falls_back_on_object_not_found_quorum() { let mut accumulator = metadata_early_stop_accumulator(); diff --git a/crates/ecstore/src/store/bucket.rs b/crates/ecstore/src/store/bucket.rs index 04b3aea33..a2968b3b5 100644 --- a/crates/ecstore/src/store/bucket.rs +++ b/crates/ecstore/src/store/bucket.rs @@ -317,11 +317,12 @@ mod tests { use crate::disk::{BUCKET_META_PREFIX, RUSTFS_META_BUCKET}; use crate::error::StorageError; use crate::object_api::{ObjectOptions, PutObjReader}; + use crate::runtime::instance::InstanceContext; use crate::storage_api_contracts::{ bucket::{BucketOperations as _, DeleteBucketOptions, MakeBucketOptions, SRBucketDeleteOp}, object::{ObjectIO as _, ObjectOperations as _}, }; - use crate::store::{ECStore, init_local_disks}; + use crate::store::{ECStore, init_local_disks_with_instance_ctx}; use crate::{ disk::endpoint::Endpoint, layout::endpoints::{EndpointServerPools, Endpoints, PoolEndpoints}, @@ -373,18 +374,31 @@ mod tests { platform: format!("OS: {} | Arch: {}", std::env::consts::OS, std::env::consts::ARCH), }]); - init_local_disks(endpoint_pools.clone()) + let instance_ctx = Arc::new(InstanceContext::new()); + init_local_disks_with_instance_ctx(&instance_ctx, endpoint_pools.clone()) .await .expect("local disks should initialize"); - let ecstore = - ECStore::new("127.0.0.1:0".parse().expect("test address"), endpoint_pools, CancellationToken::new()) - .await - .expect("ECStore should initialize"); - - if metadata_sys::get_global_bucket_metadata_sys().is_none() { - metadata_sys::init_bucket_metadata_sys(ecstore.clone(), Vec::new()).await; + let ecstore = ECStore::new_with_instance_ctx( + "127.0.0.1:0".parse().expect("test address"), + endpoint_pools, + CancellationToken::new(), + instance_ctx, + ) + .await + .expect("ECStore should initialize"); + let storage_class = crate::config::storageclass::lookup_config_for_pools_without_env( + &rustfs_config::server_config::KVS::new(), + &[4], + ) + .expect("bucket test storage class should match its four-disk pool"); + for pool in &ecstore.pools { + for set in &pool.disk_set { + set.set_test_storage_class_config(storage_class.clone()); + } } + metadata_sys::init_bucket_metadata_sys(ecstore.clone(), Vec::new()).await; + (disk_paths, ecstore) }) .await @@ -490,17 +504,8 @@ mod tests { ); } - // #[serial] with the crate-wide default key: these tests drive make_bucket / - // delete_bucket through the process-global local-disk registry and lock - // client (see crates/ecstore/src/runtime/global.rs), and through Sets::new - // which reads the process-global erasure mode. Running them concurrently with - // other tests that touch those globals races make_bucket into - // InsufficientWriteQuorum under `cargo test` (single process). serial_test - // serializes them across the - // in-process suite; nextest's per-test processes are covered separately by the - // ecstore-serial-flaky test-group in .config/nextest.toml (backlog #937). - // Full instance-level isolation is blocked on the InstanceContext migration - // (backlog #939) and is not attempted here. + // These tests share one isolated instance and mutate its bucket metadata; + // serialize them so their assertions cannot observe each other's operations. #[tokio::test] #[serial] async fn bucket_delete_mark_delete_marks_metadata_deleted_without_physical_object_delete() { @@ -509,7 +514,7 @@ mod tests { let object = "object.txt"; create_bucket_with_object(&ecstore, &bucket, object).await; - assert!(metadata_sys::get(&bucket).await.is_ok()); + assert!(metadata_sys::get_in(&ecstore.ctx, &bucket).await.is_ok()); let generation_before_delete = ecstore.scanner_namespace_mutation_generation(); ecstore @@ -537,7 +542,7 @@ mod tests { "MarkDelete should persist the deleted-bucket marker" ); assert!( - metadata_sys::get(&bucket).await.is_err(), + metadata_sys::get_in(&ecstore.ctx, &bucket).await.is_err(), "deleted bucket metadata must be removed from the local cache" ); } @@ -578,7 +583,7 @@ mod tests { "Purge should remove bucket metadata prefix" ); assert!( - metadata_sys::get(&bucket).await.is_err(), + metadata_sys::get_in(&ecstore.ctx, &bucket).await.is_err(), "purged bucket metadata must be removed from the local cache" ); } @@ -609,7 +614,7 @@ mod tests { "failed default S3 DeleteBucket must keep object data" ); assert!( - metadata_sys::get(&bucket).await.is_ok(), + metadata_sys::get_in(&ecstore.ctx, &bucket).await.is_ok(), "failed default S3 DeleteBucket must keep metadata cache" ); } diff --git a/crates/ecstore/src/store/init.rs b/crates/ecstore/src/store/init.rs index 14ca9d754..e6e090364 100644 --- a/crates/ecstore/src/store/init.rs +++ b/crates/ecstore/src/store/init.rs @@ -212,7 +212,8 @@ impl ECStore { // Validate topology and environment overrides before opening any disk. // The values stored on SetDisks remain pure per-pool topology defaults; - // the runtime storage-class snapshot is published later from config. + // payload writes use the runtime storage-class snapshot published later + // from config before the store is marked ready. let default_pool_parities = resolve_startup_pool_defaults(&endpoint_pools)?; let mut deployment_id = None; @@ -857,6 +858,28 @@ mod tests { assert!(err.to_string().contains("pool 1") && err.to_string().contains("2 drives")); } + #[test] + #[serial_test::serial(storage_class_env)] + fn startup_pool_defaults_validate_environment_without_changing_metadata_fallback() { + temp_env::with_vars( + [ + (crate::config::storageclass::STANDARD_ENV, Some("EC:1")), + (crate::config::storageclass::RRS_ENV, None), + (crate::config::storageclass::OPTIMIZE_ENV, None), + (crate::config::storageclass::INLINE_BLOCK_ENV, None), + ], + || { + let runtime = crate::config::storageclass::lookup_config_for_pools(&KVS::new(), &[6, 4]) + .expect("explicit standard parity must resolve the runtime candidate"); + assert_eq!(runtime.parities_for_sc(crate::config::storageclass::STANDARD), Some(vec![1, 1])); + + let defaults = super::resolve_startup_pool_defaults(&endpoint_pools_with_drive_counts(&[6, 4])) + .expect("explicit standard parity must validate for every pool"); + assert_eq!(defaults, vec![3, 2]); + }, + ); + } + async fn without_storage_class_env(future: F) -> F::Output { temp_env::async_with_vars( [ diff --git a/crates/ecstore/src/store/rebalance.rs b/crates/ecstore/src/store/rebalance.rs index c3d626b0b..68f2e1e67 100644 --- a/crates/ecstore/src/store/rebalance.rs +++ b/crates/ecstore/src/store/rebalance.rs @@ -13,6 +13,7 @@ // limitations under the License. use super::*; +use crate::config::storageclass; use crate::layout::pool_space::{ServerPoolsAvailableSpace, build_server_pools_available_space}; use crate::runtime::sources as runtime_sources; use crate::storage_api_contracts::{admin::StorageAdminApi, namespace::NamespaceLocking as _, object::ObjectOperations as _}; @@ -23,6 +24,142 @@ use support::{ resolve_rebalance_delete_from_all_pools_results, resolve_store_rebalance_pool_meta_reload_result, }; +#[derive(Debug, Default, Eq, PartialEq)] +struct BackendStorageClassInfo { + standard_sc_data: Vec, + standard_sc_parities: Vec, + standard_sc_parity: Option, + rr_sc_data: Vec, + rr_sc_parities: Vec, + rr_sc_parity: Option, +} + +fn resolve_pool_layout( + drives_per_set: &[usize], + mut parity_for_pool: impl FnMut(usize, usize) -> Option, +) -> Option<(Vec, Vec)> { + let mut parities = Vec::with_capacity(drives_per_set.len()); + let mut data = Vec::with_capacity(drives_per_set.len()); + + for (pool_index, &drives) in drives_per_set.iter().enumerate() { + if drives == 0 { + return None; + } + + let parity = parity_for_pool(pool_index, drives)?; + let data_drives = drives.checked_sub(parity)?; + if data_drives == 0 || parity > data_drives { + return None; + } + + data.push(data_drives); + parities.push(parity); + } + + Some((parities, data)) +} + +fn homogeneous_parity(parities: &[usize]) -> Option { + let first = *parities.first()?; + parities.iter().all(|&parity| parity == first).then_some(first) +} + +fn resolve_complete_pool_layouts( + drives_per_set: &[usize], + standard_parity_for_pool: impl FnMut(usize, usize) -> Option, + rr_parity_for_pool: impl FnMut(usize, usize) -> Option, +) -> Option { + let (standard_sc_parities, standard_sc_data) = resolve_pool_layout(drives_per_set, standard_parity_for_pool)?; + let (rr_sc_parities, rr_sc_data) = resolve_pool_layout(drives_per_set, rr_parity_for_pool)?; + + for ((&standard_parity, &rr_parity), &drives) in standard_sc_parities.iter().zip(&rr_sc_parities).zip(drives_per_set) { + storageclass::validate_parity_inner(standard_parity, rr_parity, drives).ok()?; + } + + Some(BackendStorageClassInfo { + standard_sc_parity: homogeneous_parity(&standard_sc_parities), + standard_sc_data, + standard_sc_parities, + rr_sc_parity: homogeneous_parity(&rr_sc_parities), + rr_sc_data, + rr_sc_parities, + }) +} + +fn resolve_backend_storage_class_info( + config: &storageclass::Config, + drives_per_set: &[usize], + default_standard_parities: &[usize], +) -> BackendStorageClassInfo { + if drives_per_set.len() != default_standard_parities.len() { + return BackendStorageClassInfo::default(); + } + + if !config.is_initialized() { + let Some((standard_sc_parities, standard_sc_data)) = + resolve_pool_layout(drives_per_set, |pool_index, _| default_standard_parities.get(pool_index).copied()) + else { + return BackendStorageClassInfo::default(); + }; + + return BackendStorageClassInfo { + standard_sc_parity: homogeneous_parity(&standard_sc_parities), + standard_sc_data, + standard_sc_parities, + ..Default::default() + }; + } + + match (config.parities_for_sc(storageclass::STANDARD), config.parities_for_sc(storageclass::RRS)) { + (Some(standard), Some(rr)) if standard.len() == drives_per_set.len() && rr.len() == drives_per_set.len() => { + resolve_complete_pool_layouts( + drives_per_set, + |pool_index, drives| config.parity_for_pool(storageclass::STANDARD, pool_index, drives), + |pool_index, drives| config.parity_for_pool(storageclass::RRS, pool_index, drives), + ) + .unwrap_or_default() + } + (None, None) => { + let Some(standard) = config.get_parity_for_sc(storageclass::STANDARD) else { + return BackendStorageClassInfo::default(); + }; + let Some(rr) = config.get_parity_for_sc(storageclass::RRS) else { + return BackendStorageClassInfo::default(); + }; + + resolve_complete_pool_layouts(drives_per_set, |_, _| Some(standard), |_, _| Some(rr)).unwrap_or_default() + } + _ => BackendStorageClassInfo::default(), + } +} + +fn build_backend_info( + config: &storageclass::Config, + drives_per_set: &[usize], + default_standard_parities: &[usize], + total_sets: &[usize], +) -> rustfs_madmin::BackendInfo { + let storage_class_info = if total_sets.len() == drives_per_set.len() { + resolve_backend_storage_class_info(config, drives_per_set, default_standard_parities) + } else { + BackendStorageClassInfo::default() + }; + + rustfs_madmin::BackendInfo { + backend_type: rustfs_madmin::BackendByte::Erasure, + online_disks: rustfs_madmin::BackendDisks::new(), + offline_disks: rustfs_madmin::BackendDisks::new(), + standard_sc_data: storage_class_info.standard_sc_data, + standard_sc_parities: storage_class_info.standard_sc_parities, + standard_sc_parity: storage_class_info.standard_sc_parity, + rr_sc_data: storage_class_info.rr_sc_data, + rr_sc_parities: storage_class_info.rr_sc_parities, + rr_sc_parity: storage_class_info.rr_sc_parity, + total_sets: total_sets.to_vec(), + drives_per_set: drives_per_set.to_vec(), + } +} + impl ECStore { #[instrument(level = "debug", skip(self))] pub(super) async fn delete_all(&self, bucket: &str, prefix: &str) -> Result<()> { @@ -509,37 +646,12 @@ impl ECStore { #[instrument(skip(self))] pub(super) async fn handle_backend_info(&self) -> rustfs_madmin::BackendInfo { - let (standard_sc_parity, rr_sc_parity) = - runtime_sources::backend_storage_class_parities(self.pools[0].default_parity_count); + let drives_per_set = StorageAdminApi::set_drive_counts(self); + let default_standard_parities = self.pools.iter().map(|pool| pool.default_parity_count).collect::>(); + let storage_class = runtime_sources::storage_class_config_snapshot(); + let total_sets = self.pools.iter().map(|pool| pool.set_count).collect::>(); - let mut standard_sc_data = Vec::new(); - let mut rr_sc_data = Vec::new(); - let mut drives_per_set = Vec::new(); - let mut total_sets = Vec::new(); - - for (idx, set_count) in StorageAdminApi::set_drive_counts(self).iter().enumerate() { - if let Some(sc_parity) = standard_sc_parity { - standard_sc_data.push(set_count - sc_parity); - } - if let Some(sc_parity) = rr_sc_parity { - rr_sc_data.push(set_count - sc_parity); - } - total_sets.push(self.pools[idx].set_count); - drives_per_set.push(*set_count); - } - - rustfs_madmin::BackendInfo { - backend_type: rustfs_madmin::BackendByte::Erasure, - online_disks: rustfs_madmin::BackendDisks::new(), - offline_disks: rustfs_madmin::BackendDisks::new(), - standard_sc_data, - standard_sc_parity, - rr_sc_data, - rr_sc_parity, - total_sets, - drives_per_set, - ..Default::default() - } + build_backend_info(&storage_class, &drives_per_set, &default_standard_parities, &total_sets) } #[instrument(skip(self))] @@ -635,6 +747,166 @@ impl ECStore { #[cfg(test)] mod tests { use super::*; + use crate::config::storageclass::{CLASS_RRS, CLASS_STANDARD, lookup_config_for_pools_without_env}; + use arc_swap::ArcSwap; + use rustfs_config::server_config::KVS; + use std::sync::Arc; + + fn assert_backend_layout_empty(info: &rustfs_madmin::BackendInfo) { + assert!(info.standard_sc_parities.is_empty()); + assert!(info.standard_sc_data.is_empty()); + assert_eq!(info.standard_sc_parity, None); + assert!(info.rr_sc_parities.is_empty()); + assert!(info.rr_sc_data.is_empty()); + assert_eq!(info.rr_sc_parity, None); + } + + #[test] + fn build_backend_info_reports_heterogeneous_automatic_config_in_pool_order() { + let config = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("automatic storage class should resolve"); + + let info = build_backend_info(&config, &[4, 2], &[2, 1], &[7, 3]); + + assert!(matches!(info.backend_type, rustfs_madmin::BackendByte::Erasure)); + assert_eq!(info.standard_sc_parities, vec![2, 1]); + assert_eq!(info.standard_sc_data, vec![2, 1]); + assert_eq!(info.standard_sc_parity, None); + assert_eq!(info.rr_sc_parities, vec![1, 1]); + assert_eq!(info.rr_sc_data, vec![3, 1]); + assert_eq!(info.rr_sc_parity, Some(1)); + assert_eq!(info.drives_per_set, vec![4, 2]); + assert_eq!(info.total_sets, vec![7, 3]); + } + + #[test] + fn build_backend_info_keeps_truthful_homogeneous_scalar() { + let mut kvs = KVS::new(); + kvs.insert(CLASS_STANDARD.to_string(), "EC:2".to_string()); + let config = + lookup_config_for_pools_without_env(&kvs, &[4, 6]).expect("explicit storage class should resolve for every pool"); + + let info = build_backend_info(&config, &[4, 6], &[2, 3], &[1, 1]); + + assert_eq!(info.standard_sc_parities, vec![2, 2]); + assert_eq!(info.standard_sc_data, vec![2, 4]); + assert_eq!(info.standard_sc_parity, Some(2)); + assert_eq!(info.rr_sc_parities, vec![1, 1]); + assert_eq!(info.rr_sc_data, vec![3, 5]); + assert_eq!(info.rr_sc_parity, Some(1)); + } + + #[test] + fn build_backend_info_reports_single_disk_pool_without_inventing_parity() { + let config = lookup_config_for_pools_without_env(&KVS::new(), &[4, 1]).expect("single disk pool should resolve"); + + let info = build_backend_info(&config, &[4, 1], &[2, 0], &[1, 1]); + + assert_eq!(info.standard_sc_parities, vec![2, 0]); + assert_eq!(info.standard_sc_data, vec![2, 1]); + assert_eq!(info.standard_sc_parity, None); + assert_eq!(info.rr_sc_parities, vec![1, 0]); + assert_eq!(info.rr_sc_data, vec![3, 1]); + assert_eq!(info.rr_sc_parity, None); + } + + #[test] + fn build_backend_info_uses_pool_defaults_only_when_truly_uninitialized() { + let info = build_backend_info(&Default::default(), &[4, 2], &[1, 0], &[1, 1]); + + assert_eq!(info.standard_sc_parities, vec![1, 0]); + assert_eq!(info.standard_sc_data, vec![3, 2]); + assert_eq!(info.standard_sc_parity, None); + assert!(info.rr_sc_parities.is_empty()); + assert!(info.rr_sc_data.is_empty()); + assert_eq!(info.rr_sc_parity, None); + } + + #[test] + fn build_backend_info_fails_closed_on_initialized_snapshot_mismatch() { + let config = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("automatic storage class should resolve"); + + let info = build_backend_info(&config, &[4, 6], &[1, 2], &[1, 1]); + + assert_backend_layout_empty(&info); + assert_eq!(info.drives_per_set, vec![4, 6]); + assert_eq!(info.total_sets, vec![1, 1]); + } + + #[test] + fn build_backend_info_expands_valid_legacy_scalar_snapshot() { + let mut kvs = KVS::new(); + kvs.insert(CLASS_STANDARD.to_string(), "EC:2".to_string()); + kvs.insert(CLASS_RRS.to_string(), "EC:1".to_string()); + let current = + lookup_config_for_pools_without_env(&kvs, &[4, 6]).expect("distinct legacy scalars should resolve for every pool"); + let encoded = serde_json::to_string(¤t).expect("config should serialize"); + let legacy: storageclass::Config = serde_json::from_str(&encoded).expect("legacy scalar config should deserialize"); + + let info = build_backend_info(&legacy, &[4, 6], &[2, 3], &[1, 1]); + + assert_eq!(info.standard_sc_parities, vec![2, 2]); + assert_eq!(info.standard_sc_data, vec![2, 4]); + assert_eq!(info.standard_sc_parity, Some(2)); + assert_eq!(info.rr_sc_parities, vec![1, 1]); + assert_eq!(info.rr_sc_data, vec![3, 5]); + assert_eq!(info.rr_sc_parity, Some(1)); + } + + #[test] + fn build_backend_info_rejects_legacy_scalar_invalid_for_later_pool() { + let current = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("automatic config should resolve"); + let encoded = serde_json::to_string(¤t).expect("config should serialize"); + let legacy: storageclass::Config = serde_json::from_str(&encoded).expect("legacy scalar config should deserialize"); + + let info = build_backend_info(&legacy, &[4, 2], &[2, 1], &[1, 1]); + + assert_backend_layout_empty(&info); + } + + #[test] + fn build_backend_info_never_reports_invalid_geometry() { + let config = storageclass::Config::default(); + + assert_backend_layout_empty(&build_backend_info(&config, &[4, 2], &[5, 1], &[1, 1])); + assert_backend_layout_empty(&build_backend_info(&config, &[0], &[0], &[1])); + assert_backend_layout_empty(&build_backend_info(&config, &[4], &[3], &[1])); + } + + #[test] + fn build_backend_info_rejects_mismatched_topology_lengths() { + let config = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("automatic storage class should resolve"); + + assert_backend_layout_empty(&build_backend_info(&config, &[4, 2], &[2], &[1, 1])); + assert_backend_layout_empty(&build_backend_info(&config, &[4, 2], &[2, 1], &[1])); + + let empty = build_backend_info(&Default::default(), &[], &[], &[]); + assert_backend_layout_empty(&empty); + assert!(empty.drives_per_set.is_empty()); + assert!(empty.total_sets.is_empty()); + } + + #[test] + fn build_backend_info_uses_one_complete_arc_swap_snapshot() { + let old = lookup_config_for_pools_without_env(&KVS::new(), &[4, 2]).expect("old config should resolve"); + let mut new_kvs = KVS::new(); + new_kvs.insert(CLASS_STANDARD.to_string(), "EC:1".to_string()); + let new = lookup_config_for_pools_without_env(&new_kvs, &[4, 2]).expect("new config should resolve"); + let snapshots = ArcSwap::from_pointee(old); + let held_old = snapshots.load_full(); + snapshots.store(Arc::new(new)); + + let old_info = build_backend_info(&held_old, &[4, 2], &[2, 1], &[1, 1]); + let new_info = build_backend_info(&snapshots.load_full(), &[4, 2], &[2, 1], &[1, 1]); + + assert_eq!(old_info.standard_sc_parities, vec![2, 1]); + assert_eq!(old_info.standard_sc_data, vec![2, 1]); + assert_eq!(old_info.standard_sc_parity, None); + assert_eq!(old_info.rr_sc_parities, vec![1, 1]); + assert_eq!(new_info.standard_sc_parities, vec![1, 1]); + assert_eq!(new_info.standard_sc_data, vec![3, 1]); + assert_eq!(new_info.standard_sc_parity, Some(1)); + assert_eq!(new_info.rr_sc_parities, vec![1, 1]); + } fn object_info_with_mod_time(unix_ts: i64, delete_marker: bool) -> ObjectInfo { ObjectInfo { diff --git a/crates/filemeta/src/fileinfo.rs b/crates/filemeta/src/fileinfo.rs index 2cbe993a3..98a39c8a9 100644 --- a/crates/filemeta/src/fileinfo.rs +++ b/crates/filemeta/src/fileinfo.rs @@ -17,8 +17,8 @@ use bytes::Bytes; use rmp_serde::Serializer; use rustfs_utils::HashAlgorithm; use rustfs_utils::http::{ - SUFFIX_COMPRESSION, SUFFIX_DATA_MOVED, SUFFIX_HEALING, SUFFIX_INLINE_DATA, SUFFIX_TIER_FV_ID, SUFFIX_TIER_FV_MARKER, - SUFFIX_TIER_SKIP_FV_ID, contains_key_str, get_str, insert_str, + SUFFIX_COMPRESSION, SUFFIX_DATA_MOVED, SUFFIX_FREE_VERSION, SUFFIX_HEALING, SUFFIX_INLINE_DATA, SUFFIX_TIER_FV_ID, + SUFFIX_TIER_FV_MARKER, SUFFIX_TIER_SKIP_FV_ID, contains_key_str, get_str, has_internal_suffix, insert_str, }; use s3s::dto::{RestoreStatus, Timestamp}; use s3s::header::X_AMZ_RESTORE; @@ -31,6 +31,12 @@ use uuid::Uuid; pub const ERASURE_ALGORITHM: &str = "rs-vandermonde"; pub const BLOCK_SIZE_V2: usize = 1024 * 1024; // 1M +const MAX_ERASURE_SHARDS: usize = 16; +const MAX_FILEINFO_PARTS: usize = 10_000; +const MAX_FILEINFO_CHECKSUMS: usize = 10_000; +const FILEINFO_PART_BITMAP_WORD_BITS: usize = std::mem::size_of::() * 8; +const FILEINFO_PART_BITMAP_WORDS: usize = MAX_FILEINFO_PARTS.div_ceil(FILEINFO_PART_BITMAP_WORD_BITS); + // Additional constants from Go version pub const NULL_VERSION_ID: &str = "null"; // pub const RUSTFS_ERASURE_UPGRADED: &str = "x-rustfs-internal-erasure-upgraded"; @@ -137,6 +143,14 @@ pub fn calc_shard_size(block_size: usize, data_shards: usize) -> usize { (block_size.div_ceil(data_shards) + 1) & !1 } +fn checked_calc_shard_size(block_size: usize, data_shards: usize) -> Option { + if data_shards == 0 { + return None; + } + + block_size.div_ceil(data_shards).checked_add(1).map(|size| size & !1) +} + impl ErasureInfo { pub fn get_checksum_info(&self, part_number: usize) -> ChecksumInfo { for sum in &self.checksums { @@ -243,6 +257,55 @@ pub struct FileInfo { pub uses_legacy_checksum: bool, } +/// Selects the validation policy for a trusted operation boundary. +/// +/// This mode is deliberately caller-selected and is never inferred from +/// serialized [`FileInfo`] state such as `deleted` or `transition_status`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ValidationMode { + /// Validate the complete storage erasure layout before payload access. + RequireErasure, + /// Validate delete metadata without requiring a payload erasure layout. + DeleteOnly, +} + +/// Erasure geometry that passed the storage-layout validation policy. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ValidatedErasureLayout { + data_blocks: usize, + block_size: usize, + shard_size: usize, +} + +impl ValidatedErasureLayout { + /// Calculates a shard file size without overflowing the metadata's integer format. + pub fn shard_file_size(&self, total_length: usize) -> Option { + let full_blocks = total_length / self.block_size; + let last_block_size = total_length % self.block_size; + let last_shard_size = checked_calc_shard_size(last_block_size, self.data_blocks)?; + let shard_file_size = full_blocks.checked_mul(self.shard_size)?.checked_add(last_shard_size)?; + i64::try_from(shard_file_size).ok() + } +} + +fn mark_fileinfo_part(bitmap: &mut [u64; FILEINFO_PART_BITMAP_WORDS], part_number: usize) -> bool { + let Some(index) = part_number.checked_sub(1).filter(|&index| index < MAX_FILEINFO_PARTS) else { + return false; + }; + let word = index / FILEINFO_PART_BITMAP_WORD_BITS; + let mask = 1u64 << (index % FILEINFO_PART_BITMAP_WORD_BITS); + let was_absent = bitmap[word] & mask == 0; + bitmap[word] |= mask; + was_absent +} + +fn has_fileinfo_part(bitmap: &[u64; FILEINFO_PART_BITMAP_WORDS], part_number: usize) -> bool { + let Some(index) = part_number.checked_sub(1).filter(|&index| index < MAX_FILEINFO_PARTS) else { + return false; + }; + bitmap[index / FILEINFO_PART_BITMAP_WORD_BITS] & (1u64 << (index % FILEINFO_PART_BITMAP_WORD_BITS)) != 0 +} + /// Validates that an erasure `distribution` is a permutation of `1..=n`. /// /// A well-formed distribution has exactly `n` entries and each 1-based slot @@ -252,11 +315,11 @@ pub struct FileInfo { /// `usize` underflow / out-of-bounds panic. Rejecting such distributions here /// lets the metadata surface as a clean quorum/corruption error instead. pub(crate) fn is_valid_distribution(distribution: &[usize], n: usize) -> bool { - if n == 0 || distribution.len() != n { + if n == 0 || n > MAX_ERASURE_SHARDS || distribution.len() != n { return false; } - let mut seen = vec![false; n]; + let mut seen = [false; MAX_ERASURE_SHARDS]; for &block_idx in distribution { // Valid 1-based slots are `1..=n`; anything else (including `0`) is invalid. if block_idx < 1 || block_idx > n { @@ -304,19 +367,264 @@ impl FileInfo { } } - pub fn is_valid(&self) -> bool { - if self.deleted { - return true; + fn validate_erasure_geometry_with_index(&self, index: usize) -> Result { + let erasure = &self.erasure; + if erasure.data_blocks == 0 || erasure.block_size == 0 || erasure.data_blocks < erasure.parity_blocks { + return Err(Error::FileCorrupt); } - let data_blocks = self.erasure.data_blocks; - let parity_blocks = self.erasure.parity_blocks; + let total_blocks = erasure + .data_blocks + .checked_add(erasure.parity_blocks) + .ok_or(Error::FileCorrupt)?; + if total_blocks > MAX_ERASURE_SHARDS + || index == 0 + || index > total_blocks + || !is_valid_distribution(&erasure.distribution, total_blocks) + { + return Err(Error::FileCorrupt); + } - (data_blocks >= parity_blocks) - && (data_blocks > 0) - && (self.erasure.index > 0 - && self.erasure.index <= data_blocks + parity_blocks - && is_valid_distribution(&self.erasure.distribution, data_blocks + parity_blocks)) + let shard_size = checked_calc_shard_size(erasure.block_size, erasure.data_blocks) + .filter(|&size| i64::try_from(size).is_ok()) + .ok_or(Error::FileCorrupt)?; + i64::try_from(erasure.block_size).map_err(|_| Error::FileCorrupt)?; + + let layout = ValidatedErasureLayout { + data_blocks: erasure.data_blocks, + block_size: erasure.block_size, + shard_size, + }; + let object_size = usize::try_from(self.size).map_err(|_| Error::FileCorrupt)?; + layout.shard_file_size(object_size).ok_or(Error::FileCorrupt)?; + Ok(layout) + } + + fn validate_erasure_geometry(&self) -> Result { + self.validate_erasure_geometry_with_index(self.erasure.index) + } + + fn validate_collection_bounds(&self) -> Result<()> { + if self.parts.len() > MAX_FILEINFO_PARTS + || self.erasure.checksums.len() > MAX_FILEINFO_CHECKSUMS + || self.erasure.distribution.len() > MAX_ERASURE_SHARDS + { + return Err(Error::FileCorrupt); + } + Ok(()) + } + + fn validate_collection_contents(&self, layout: Option<&ValidatedErasureLayout>) -> Result<()> { + let mut part_numbers = [0u64; FILEINFO_PART_BITMAP_WORDS]; + for part in &self.parts { + if let Some(layout) = layout { + i64::try_from(part.size).map_err(|_| Error::FileCorrupt)?; + layout.shard_file_size(part.size).ok_or(Error::FileCorrupt)?; + + // A negative `actual_size` is the documented "unknown size" sentinel for + // compressed streaming objects (see `ObjectPartInfo::actual_size` / + // `ObjectInfo::get_actual_size`), written to xl.meta by both RustFS and + // MinIO. Only shard-validate a real, non-negative size; rejecting the + // sentinel would make legitimate compressed objects unreadable. + if let Ok(actual_size) = usize::try_from(part.actual_size) { + layout.shard_file_size(actual_size).ok_or(Error::FileCorrupt)?; + } + } + if !mark_fileinfo_part(&mut part_numbers, part.number) { + return Err(Error::FileCorrupt); + } + } + + let mut checksum_parts = [0u64; FILEINFO_PART_BITMAP_WORDS]; + for checksum in &self.erasure.checksums { + if !has_fileinfo_part(&part_numbers, checksum.part_number) + || !mark_fileinfo_part(&mut checksum_parts, checksum.part_number) + { + return Err(Error::FileCorrupt); + } + } + Ok(()) + } + + fn validate_delete_marker_shape(&self, expected_index: usize, allow_nil_version_id: bool) -> Result<()> { + if !self.deleted { + return Err(Error::FileCorrupt); + } + let erasure = &self.erasure; + let stored_free_version = contains_key_str(&self.metadata, SUFFIX_FREE_VERSION); + if self.mod_time.is_none_or(|mod_time| mod_time <= OffsetDateTime::UNIX_EPOCH) + || (!allow_nil_version_id && self.version_id.is_some_and(|version_id| version_id.is_nil())) + || self.transition_version_id.is_some_and(|version_id| version_id.is_nil()) + || self.size != 0 + || self.data_dir.is_some() + || self.mode.is_some() + || self.written_by_version.is_some() + || self.data.is_some() + || self.checksum.is_some() + || !self.parts.is_empty() + || !erasure.algorithm.is_empty() + || erasure.data_blocks != 0 + || erasure.parity_blocks != 0 + || erasure.block_size != 0 + || erasure.index != expected_index + || !erasure.distribution.is_empty() + || !erasure.checksums.is_empty() + || stored_free_version != self.tier_free_version() + || (stored_free_version && (self.transition_tier.is_empty() || self.transitioned_objname.is_empty())) + { + return Err(Error::FileCorrupt); + } + Ok(()) + } + + fn validate_tier_free_version_delete_shape(&self) -> Result<()> { + let erasure = &self.erasure; + if !self.deleted + || !self.tier_free_version() + || self.version_id.is_none_or(|version_id| version_id.is_nil()) + || self.mod_time.is_some() + || self.is_latest + || !self.transition_status.is_empty() + || !self.transitioned_objname.is_empty() + || !self.transition_tier.is_empty() + || self.transition_version_id.is_some() + || self.expire_restored + || self.size != 0 + || self.data_dir.is_some() + || self.mode.is_some() + || self.written_by_version.is_some() + || self.mark_deleted + || self.replication_state_internal.is_some() + || self.data.is_some() + || self.num_versions != 0 + || self.successor_mod_time.is_some() + || self.fresh + || self.idx != 0 + || self.checksum.is_some() + || self.versioned + || self.uses_legacy_checksum + || !self.parts.is_empty() + || self.metadata.is_empty() + || self.metadata.len() > 2 + || self + .metadata + .iter() + .any(|(key, value)| !has_internal_suffix(key, SUFFIX_TIER_FV_MARKER) || !value.is_empty()) + || !erasure.algorithm.is_empty() + || erasure.data_blocks != 0 + || erasure.parity_blocks != 0 + || erasure.block_size != 0 + || erasure.index != 0 + || !erasure.distribution.is_empty() + || !erasure.checksums.is_empty() + { + return Err(Error::FileCorrupt); + } + Ok(()) + } + + /// Validates this metadata for the caller-selected operation boundary and + /// returns the payload erasure layout when one is established. + /// + /// All modes enforce collection bounds and checksum-to-part associations. + /// Only [`ValidationMode::RequireErasure`] establishes a payload erasure + /// layout; the other modes cannot be upgraded based on serialized flags and + /// return `None`. + pub fn validate(&self, mode: ValidationMode) -> Result> { + self.validate_collection_bounds()?; + + let erasure_layout = match mode { + ValidationMode::RequireErasure => Some(self.validate_erasure_geometry()?), + ValidationMode::DeleteOnly => { + self.validate_delete_marker_shape(0, false)?; + None + } + }; + self.validate_collection_contents(erasure_layout.as_ref())?; + + Ok(erasure_layout) + } + + /// Validate metadata returned by a disk or peer as a strict tagged union. + /// Payload entries require complete erasure geometry, including + /// purge-pending object versions whose replication state sets `deleted`. + /// Only entries that are not valid payloads may fall back to the canonical + /// delete-marker shape, so `deleted` cannot bypass payload validation. + pub fn validate_for_metadata_read(&self) -> Result<()> { + if self.validate(ValidationMode::RequireErasure).is_ok() { + return Ok(()); + } + + self.validate(ValidationMode::DeleteOnly).map(|_| ()) + } + + /// Cheap shape check for metadata that already passed + /// [`Self::validate_for_metadata_read`] at its decode boundary. + pub fn has_valid_metadata_shape(&self) -> bool { + self.has_valid_erasure_geometry() || self.is_canonical_delete_marker() + } + + /// Returns whether this is a canonical delete marker rather than an + /// erasure-backed object version in a purge-pending state. + pub fn is_canonical_delete_marker(&self) -> bool { + self.validate_delete_marker_shape(0, false).is_ok() + } + + /// Storage-only marker classification after an absent version ID has been + /// normalized to the internal nil UUID. Network and disk boundaries must + /// continue to use [`Self::is_canonical_delete_marker`]. + pub(crate) fn is_storage_delete_marker(&self) -> bool { + self.validate_delete_marker_shape(0, true).is_ok() + } + + /// Whether this is a canonical delete marker carrying only the per-disk + /// index assigned by an older rename coordinator. + pub fn is_legacy_indexed_delete_marker(&self) -> bool { + self.erasure.index > 0 + && self.erasure.index <= MAX_ERASURE_SHARDS + && self.validate_delete_marker_shape(self.erasure.index, false).is_ok() + } + + /// Validates complete metadata before a write fanout assigns its per-disk + /// shard index. An index of zero is treated only as the pending assignment; + /// every other geometry and collection invariant remains mandatory. + pub fn validate_for_erasure_write(&self) -> Result<()> { + self.validate_collection_bounds()?; + let index = if self.erasure.index == 0 { 1 } else { self.erasure.index }; + let layout = self.validate_erasure_geometry_with_index(index)?; + self.validate_collection_contents(Some(&layout)) + } + + /// Validates metadata used to mutate a version list during a delete. + /// Delete-marker creation requires the canonical marker shape; other + /// delete operations do not access payload shards but still validate all + /// collection bounds and checksum-to-part associations before mutation. + pub fn validate_for_delete_operation(&self) -> Result<()> { + self.validate_collection_bounds()?; + + if self.deleted && self.tier_free_version() { + return self.validate_tier_free_version_delete_shape(); + } + + if self.deleted { + return self.validate(ValidationMode::DeleteOnly).map(|_| ()); + } + + self.validate_collection_contents(None) + } + + /// Performs the cheap layout check used by quorum selection after a decode + /// boundary has already called [`Self::validate`]. + pub fn has_valid_erasure_geometry(&self) -> bool { + self.validate_erasure_geometry().is_ok() + } + + /// Validates the complete payload metadata shape. + /// + /// Repeated quorum passes over metadata that was already fully validated + /// should use [`Self::has_valid_erasure_geometry`] instead. + pub fn is_valid(&self) -> bool { + self.validate(ValidationMode::RequireErasure).is_ok() } pub fn get_etag(&self) -> Option { @@ -324,7 +632,7 @@ impl FileInfo { } pub fn write_quorum(&self, quorum: usize) -> usize { - if self.deleted { + if self.deleted && !self.has_valid_erasure_geometry() { return quorum; } @@ -817,6 +1125,419 @@ mod tests { assert!(!fi.is_valid()); } + fn validation_test_fileinfo() -> FileInfo { + let mut fi = FileInfo::new("bucket/object", 4, 2); + fi.erasure.index = 1; + fi.parts = vec![ + ObjectPartInfo { + number: 1, + ..Default::default() + }, + ObjectPartInfo { + number: 2, + ..Default::default() + }, + ]; + fi.erasure.checksums = vec![ + ChecksumInfo { + part_number: 1, + algorithm: HashAlgorithm::HighwayHash256S, + hash: Bytes::from_static(b"checksum-one"), + }, + ChecksumInfo { + part_number: 2, + algorithm: HashAlgorithm::SHA256, + hash: Bytes::from_static(b"checksum-two"), + }, + ]; + fi + } + + fn assert_file_corrupt(fi: &FileInfo, mode: ValidationMode) { + assert_eq!(fi.validate(mode).expect_err("invalid FileInfo must be rejected"), Error::FileCorrupt); + } + + #[test] + fn validate_require_erasure_returns_checked_layout() { + let fi = validation_test_fileinfo(); + let layout = fi + .validate(ValidationMode::RequireErasure) + .expect("well-formed erasure metadata must validate") + .expect("strict validation must return an erasure layout"); + assert_eq!(layout.data_blocks, 4); + assert_eq!(layout.block_size, BLOCK_SIZE_V2); + assert_eq!(layout.shard_size, calc_shard_size(BLOCK_SIZE_V2, 4)); + let shard_size = i64::try_from(layout.shard_size).expect("validated shard size must fit i64"); + assert_eq!(layout.shard_file_size(0), Some(0)); + assert_eq!(layout.shard_file_size(BLOCK_SIZE_V2 - 1), Some(shard_size)); + assert_eq!(layout.shard_file_size(BLOCK_SIZE_V2), Some(shard_size)); + assert_eq!(layout.shard_file_size(BLOCK_SIZE_V2 + 1), Some(shard_size + 2)); + } + + #[test] + fn validate_require_erasure_accepts_zero_parity() { + let mut fi = FileInfo::new("bucket/object", 16, 0); + fi.erasure.index = 1; + + let layout = fi + .validate(ValidationMode::RequireErasure) + .expect("zero parity is a valid storage layout"); + assert_eq!(layout.expect("strict layout must be present").data_blocks, 16); + } + + #[test] + fn validate_for_erasure_write_only_relaxes_pending_shard_index() { + let mut fi = validation_test_fileinfo(); + fi.erasure.index = 0; + + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + fi.validate_for_erasure_write() + .expect("write preparation must accept an index that fanout has not assigned yet"); + + fi.erasure.index = fi.erasure.data_blocks + fi.erasure.parity_blocks + 1; + assert_eq!(fi.validate_for_erasure_write(), Err(Error::FileCorrupt)); + + fi.erasure.index = 0; + fi.parts.push(fi.parts[0].clone()); + assert_eq!(fi.validate_for_erasure_write(), Err(Error::FileCorrupt)); + } + + #[test] + fn validate_require_erasure_rejects_invalid_geometry() { + let mut fi = validation_test_fileinfo(); + fi.erasure.data_blocks = 0; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.block_size = 0; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.data_blocks = usize::MAX; + fi.erasure.parity_blocks = 1; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.data_blocks = 9; + fi.erasure.parity_blocks = 8; + fi.erasure.index = 1; + fi.erasure.distribution = (1..=17).collect(); + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.data_blocks = 1; + fi.erasure.parity_blocks = 2; + fi.erasure.index = 1; + fi.erasure.distribution = vec![1, 2, 3]; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.index = 0; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.index = 7; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = validation_test_fileinfo(); + fi.erasure.distribution = vec![1, 2, 3, 4, 5, 5]; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + } + + fn one_shard_validation_fileinfo(block_size: usize) -> FileInfo { + let mut fi = FileInfo::new("bucket/object", 1, 0); + fi.erasure.index = 1; + fi.erasure.block_size = block_size; + fi + } + + #[test] + fn validate_require_erasure_rejects_unrepresentable_shard_sizes() { + let fi = one_shard_validation_fileinfo(usize::MAX); + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + if let Some((max_i64, above_i64_max)) = usize::try_from(i64::MAX) + .ok() + .and_then(|max_i64| max_i64.checked_add(1).map(|above_i64_max| (max_i64, above_i64_max))) + { + let mut fi = FileInfo::new("bucket/object", 2, 0); + fi.erasure.index = 1; + fi.erasure.block_size = above_i64_max; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = FileInfo::new("bucket/object", 2, 0); + fi.erasure.index = 1; + fi.erasure.block_size = max_i64; + fi.parts = vec![ObjectPartInfo { + number: 1, + size: above_i64_max, + ..Default::default() + }]; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + } + + let mut fi = one_shard_validation_fileinfo(2); + fi.size = -1; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = one_shard_validation_fileinfo(2); + fi.size = i64::MAX; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + let mut fi = one_shard_validation_fileinfo(2); + fi.parts = vec![ObjectPartInfo { + number: 1, + size: usize::MAX, + ..Default::default() + }]; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + + // A negative `actual_size` is the compressed "unknown size" sentinel, not an + // unrepresentable size: it must stay readable so existing compressed objects + // remain accessible. Only real (non-negative) sizes are shard-validated. + let mut fi = one_shard_validation_fileinfo(2); + fi.parts = vec![ObjectPartInfo { + number: 1, + actual_size: -1, + ..Default::default() + }]; + fi.validate(ValidationMode::RequireErasure) + .expect("negative actual_size sentinel (compressed objects) must remain readable"); + + let mut fi = one_shard_validation_fileinfo(2); + fi.parts = vec![ObjectPartInfo { + number: 1, + actual_size: i64::MAX, + ..Default::default() + }]; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + } + + #[test] + fn metadata_read_validation_rejects_flags_that_try_to_relax_payload_validation() { + let mut fi = FileInfo::new("bucket/legacy", 0, 2); + fi.erasure.index = 1; + fi.deleted = true; + fi.transition_status = TRANSITION_COMPLETE.to_owned(); + fi.transitioned_objname = "remote/object".to_owned(); + fi.transition_tier = "WARM".to_owned(); + + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert!(!fi.is_valid(), "wire-controlled state flags must not relax is_valid"); + assert!(matches!(fi.validate_for_metadata_read(), Err(Error::FileCorrupt))); + assert_file_corrupt(&fi, ValidationMode::DeleteOnly); + } + + #[test] + fn metadata_read_validation_requires_canonical_delete_marker_shape() { + let marker = FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(Uuid::new_v4()), + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + marker + .validate_for_metadata_read() + .expect("canonical delete marker should validate"); + + let mut missing_time = marker.clone(); + missing_time.mod_time = None; + assert!(matches!(missing_time.validate_for_metadata_read(), Err(Error::FileCorrupt))); + + let mut epoch_time = marker.clone(); + epoch_time.mod_time = Some(OffsetDateTime::UNIX_EPOCH); + assert!(matches!(epoch_time.validate_for_metadata_read(), Err(Error::FileCorrupt))); + + let mut disguised_payload = marker.clone(); + disguised_payload.erasure.data_blocks = 1; + assert!(matches!(disguised_payload.validate_for_metadata_read(), Err(Error::FileCorrupt))); + + let mut marker_with_part = marker; + marker_with_part.parts.push(ObjectPartInfo { + number: 1, + ..Default::default() + }); + assert!(matches!(marker_with_part.validate_for_metadata_read(), Err(Error::FileCorrupt))); + + let mut purge_pending = validation_test_fileinfo(); + purge_pending.deleted = true; + purge_pending + .validate_for_metadata_read() + .expect("erasure-backed purge-pending objects remain valid payload metadata"); + assert!(!purge_pending.is_canonical_delete_marker()); + } + + #[test] + fn rename_validation_accepts_only_legacy_assigned_delete_marker_index() { + let marker = FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(Uuid::new_v4()), + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + + let mut legacy_marker = marker.clone(); + legacy_marker.erasure.index = 2; + assert_eq!(legacy_marker.validate_for_metadata_read(), Err(Error::FileCorrupt)); + assert!(legacy_marker.is_legacy_indexed_delete_marker()); + legacy_marker.erasure.index = 0; + legacy_marker + .validate_for_metadata_read() + .expect("normalized legacy marker should pass strict metadata validation"); + + let mut malformed_marker = legacy_marker; + malformed_marker.erasure.index = 2; + malformed_marker.erasure.data_blocks = 1; + assert!(!malformed_marker.is_legacy_indexed_delete_marker()); + assert_eq!(malformed_marker.validate_for_metadata_read(), Err(Error::FileCorrupt)); + + let mut forged_free_marker = marker.clone(); + insert_str(&mut forged_free_marker.metadata, SUFFIX_FREE_VERSION, String::new()); + assert_eq!(forged_free_marker.validate_for_metadata_read(), Err(Error::FileCorrupt)); + + let mut free_marker = marker.clone(); + insert_str(&mut free_marker.metadata, SUFFIX_FREE_VERSION, String::new()); + free_marker.set_tier_free_version(); + free_marker.transition_tier = "WARM".to_string(); + free_marker.transitioned_objname = "remote/object".to_string(); + free_marker + .validate_for_metadata_read() + .expect("complete tier free-version marker should remain valid"); + + let mut out_of_range_marker = marker; + out_of_range_marker.erasure.index = MAX_ERASURE_SHARDS + 1; + assert!(!out_of_range_marker.is_legacy_indexed_delete_marker()); + assert_eq!(out_of_range_marker.validate_for_metadata_read(), Err(Error::FileCorrupt)); + } + + #[test] + fn delete_operation_accepts_only_the_worker_tier_free_version_request_shape() { + let mut worker_request = FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(Uuid::new_v4()), + deleted: true, + ..Default::default() + }; + worker_request.set_tier_free_version(); + worker_request + .validate_for_delete_operation() + .expect("the lifecycle worker request shape must remain valid"); + + let mut timestamped = worker_request.clone(); + timestamped.mod_time = Some(OffsetDateTime::now_utc()); + assert_eq!(timestamped.validate_for_delete_operation(), Err(Error::FileCorrupt)); + + let mut with_payload = worker_request.clone(); + with_payload.size = 1; + assert_eq!(with_payload.validate_for_delete_operation(), Err(Error::FileCorrupt)); + + let mut with_unrelated_metadata = worker_request; + with_unrelated_metadata + .metadata + .insert("unexpected".to_string(), "value".to_string()); + assert_eq!(with_unrelated_metadata.validate_for_delete_operation(), Err(Error::FileCorrupt)); + } + + #[test] + fn erasure_and_delete_operation_validation_enforce_basic_collection_bounds() { + let mut fi = validation_test_fileinfo(); + fi.parts = (1..=10_001) + .map(|number| ObjectPartInfo { + number, + ..Default::default() + }) + .collect(); + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert_eq!(fi.validate_for_delete_operation(), Err(Error::FileCorrupt)); + + let mut fi = validation_test_fileinfo(); + fi.erasure.checksums = (1..=10_001) + .map(|part_number| ChecksumInfo { + part_number, + ..Default::default() + }) + .collect(); + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert_eq!(fi.validate_for_delete_operation(), Err(Error::FileCorrupt)); + + let mut fi = validation_test_fileinfo(); + fi.erasure.distribution = vec![1; 17]; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert_eq!(fi.validate_for_delete_operation(), Err(Error::FileCorrupt)); + } + + #[test] + fn validate_accepts_exact_collection_limits() { + let mut fi = validation_test_fileinfo(); + fi.parts = (1..=10_000) + .map(|number| ObjectPartInfo { + number, + ..Default::default() + }) + .collect(); + fi.erasure.checksums = (1..=10_000) + .map(|part_number| ChecksumInfo { + part_number, + ..Default::default() + }) + .collect(); + + fi.validate(ValidationMode::RequireErasure) + .expect("exact collection limits must remain valid"); + fi.validate_for_delete_operation() + .expect("non-marker delete requests must accept exact collection limits"); + } + + #[test] + fn erasure_and_delete_operation_validation_reject_checksum_association_errors() { + let mut fi = validation_test_fileinfo(); + fi.erasure.checksums[0].part_number = 99; + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert_eq!(fi.validate_for_delete_operation(), Err(Error::FileCorrupt)); + + let mut fi = validation_test_fileinfo(); + let duplicate = fi.erasure.checksums[0].clone(); + fi.erasure.checksums.push(duplicate); + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert_eq!(fi.validate_for_delete_operation(), Err(Error::FileCorrupt)); + } + + #[test] + fn erasure_and_delete_operation_validation_reject_duplicate_parts() { + let mut fi = validation_test_fileinfo(); + fi.parts[1].number = fi.parts[0].number; + fi.erasure.checksums.clear(); + + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + assert_eq!(fi.validate_for_delete_operation(), Err(Error::FileCorrupt)); + } + + #[test] + fn write_quorum_distinguishes_purge_pending_payload_from_delete_marker() { + let mut purge_pending = FileInfo::new("bucket/object", 5, 1); + purge_pending.erasure.index = 1; + purge_pending.deleted = true; + assert_eq!(purge_pending.write_quorum(4), 5); + + let marker = FileInfo { + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + assert_eq!(marker.write_quorum(4), 4); + + let malformed_marker = FileInfo { + deleted: true, + ..Default::default() + }; + assert_eq!(malformed_marker.write_quorum(4), 4, "invalid marker metadata must never reduce quorum"); + } + fn small_string_strategy() -> impl Strategy { proptest::string::string_regex("[A-Za-z0-9._/-]{0,16}").expect("small string regex should compile") } diff --git a/crates/filemeta/src/filemeta.rs b/crates/filemeta/src/filemeta.rs index 77dd3ceff..74f85dea4 100644 --- a/crates/filemeta/src/filemeta.rs +++ b/crates/filemeta/src/filemeta.rs @@ -395,7 +395,24 @@ impl FileMeta { // delete_version deletes version, returns data_dir #[tracing::instrument(level = "debug", skip(self))] pub fn delete_version(&mut self, fi: &FileInfo) -> Result> { + fi.validate_for_delete_operation()?; + let vid = Some(fi.version_id.unwrap_or(Uuid::nil())); + if fi.deleted && fi.tier_free_version() { + let Some(index) = self.versions.iter().position(|version| { + version.header.version_id == vid + && version.header.version_type == VersionType::Delete + && version.header.free_version() + && version + .parse_version_meta() + .is_ok_and(|decoded| decoded.free_version() && decoded.header().version_id == vid) + }) else { + return Err(Error::FileVersionNotFound); + }; + self.versions.remove(index); + return Ok(None); + } + let target_is_delete_marker = self .versions .iter() @@ -410,10 +427,6 @@ impl FileMeta { mod_time: fi.mod_time, ..Default::default() }); - - if !fi.is_valid() { - return Err(Error::other("invalid file meta version")); - } } let mut update_version = false; @@ -978,6 +991,52 @@ mod test { assert!(!fi.parts.is_empty(), "multipart/part layout present"); } + /// Compatibility guard for the tightened `validate_for_metadata_read()` decode + /// boundary (the rolling-upgrade / MinIO-migration risk): every version of + /// every real, historically-written `xl.meta` fixture must still be *accepted* + /// on read, never rejected as `FileCorrupt`. This is the empirical companion to + /// the code-reasoned decode-tolerance invariants in + /// `docs/architecture/erasure-coding.md` §11 — reverting the tolerant handling + /// (delete-marker shape, legacy per-part checksums, string/short + /// `transitioned-versionID`, negative `actual_size`) turns one of these red. + /// It covers real MinIO-written objects (inline, versioned incl. a delete + /// marker, and multipart), a legacy V1 (`xl.json`-derived) object, and a legacy + /// meta_ver 2 object. + #[test] + fn real_historical_xlmeta_versions_pass_metadata_read_validation() { + let fixtures: [(&str, Vec); 5] = [ + ("minio-small-inline", create_minio_small_object_xlmeta().expect("small fixture")), + ( + "minio-versioned+delete-marker", + create_minio_versioned_object_xlmeta().expect("versioned fixture"), + ), + ("minio-large-multipart", create_minio_large_object_xlmeta().expect("large fixture")), + ("legacy-v1-object", create_legacy_v1_object_xlmeta().expect("legacy v1 fixture")), + ( + "legacy-meta-v2-object", + create_issue_2265_legacy_meta_v2_object_xlmeta().expect("legacy meta v2 fixture"), + ), + ]; + + for (label, bytes) in fixtures { + let fm = FileMeta::load(&bytes).unwrap_or_else(|e| panic!("{label}: load real xl.meta failed: {e}")); + // `all_parts = true` materializes the part arrays so the checksum/part and + // shard-size checks in `validate_for_metadata_read` are actually exercised. + let versions = fm + .list_versions("interop", "object", true) + .unwrap_or_else(|e| panic!("{label}: list_versions failed: {e}")); + assert!(!versions.is_empty(), "{label}: fixture must contain at least one version"); + for (i, fi) in versions.iter().enumerate() { + fi.validate_for_metadata_read().unwrap_or_else(|e| { + panic!( + "{label}: version {i} (deleted={}) rejected by validate_for_metadata_read: {e:?}", + fi.deleted + ) + }); + } + } + } + /// Wraps a raw meta block in a valid XL2 container (header, bin32 length /// prefix, and CRC trailer) so decode tests exercise the meta parsing /// itself rather than the envelope checks. @@ -2072,6 +2131,152 @@ mod test { ); } + #[test] + fn delete_version_accepts_delete_only_marker_and_free_version_paths() { + let marker_version_id = Uuid::new_v4(); + let mut marker_meta = FileMeta::new(); + marker_meta + .delete_version(&FileInfo { + version_id: Some(marker_version_id), + deleted: true, + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }) + .expect("delete-marker metadata must not require a payload erasure layout"); + assert_eq!(marker_meta.versions.len(), 1); + assert_eq!(marker_meta.versions[0].header.version_type, VersionType::Delete); + + let object_version_id = Uuid::new_v4(); + let remote_version_id = Uuid::new_v4(); + let free_version_id = Uuid::new_v4(); + let mut free_meta = FileMeta::new(); + free_meta + .add_version(FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(object_version_id), + transition_status: TRANSITION_COMPLETE.to_string(), + transitioned_objname: "remote/object".to_string(), + transition_version_id: Some(remote_version_id), + transition_tier: "WARM".to_string(), + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }) + .expect("transitioned object version must be seeded"); + + let mut transition_delete = FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(object_version_id), + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }; + transition_delete.set_tier_free_version_id(&free_version_id.to_string()); + free_meta + .delete_version(&transition_delete) + .expect("transitioned delete must persist free-version cleanup metadata"); + + let mut free_version_delete = FileInfo { + volume: "bucket".to_string(), + name: "object".to_string(), + version_id: Some(free_version_id), + deleted: true, + ..Default::default() + }; + free_version_delete.set_tier_free_version(); + free_meta + .delete_version(&free_version_delete) + .expect("free-version cleanup must not require a payload erasure layout"); + + let versions = free_meta + .get_file_info_versions("bucket", "object", false) + .expect("versions must remain readable after free-version cleanup"); + assert!(versions.free_versions.is_empty()); + } + + #[test] + fn delete_version_validates_requests_before_mutation() { + let version_id = Uuid::new_v4(); + let mut fm = FileMeta::new(); + fm.add_version(FileInfo { + version_id: Some(version_id), + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }) + .expect("object version must be seeded"); + let original = fm.clone(); + + let err = fm + .delete_version(&FileInfo { + version_id: Some(version_id), + parts: vec![ + ObjectPartInfo { + number: 1, + ..Default::default() + }, + ObjectPartInfo { + number: 1, + ..Default::default() + }, + ], + ..Default::default() + }) + .expect_err("non-deleted requests must not bypass boundary validation"); + + assert_eq!(err, Error::FileCorrupt); + assert_eq!(fm, original, "failed validation must happen before metadata mutation"); + + let err = fm + .delete_version(&FileInfo { + version_id: Some(version_id), + deleted: true, + mod_time: None, + ..Default::default() + }) + .expect_err("malformed delete markers must fail before removing the existing object version"); + + assert_eq!(err, Error::FileCorrupt); + assert_eq!(fm, original, "malformed delete-marker validation must precede metadata mutation"); + + let mut free_version_delete = FileInfo { + version_id: Some(version_id), + deleted: true, + ..Default::default() + }; + free_version_delete.set_tier_free_version(); + let err = fm + .delete_version(&free_version_delete) + .expect_err("tier free-version cleanup must not remove an ordinary object version"); + + assert_eq!(err, Error::FileVersionNotFound); + assert_eq!(fm, original, "missing free-version cleanup must not mutate ordinary metadata"); + + let mut poisoned_object_header = fm.clone(); + poisoned_object_header.versions[0].header.flags |= XL_FLAG_FREE_VERSION; + let poisoned_original = poisoned_object_header.clone(); + let err = poisoned_object_header + .delete_version(&free_version_delete) + .expect_err("a free-version flag on an Object header must not authorize cleanup deletion"); + assert_eq!(err, Error::FileVersionNotFound); + assert_eq!( + poisoned_object_header, poisoned_original, + "a poisoned Object header must remain unchanged after rejected cleanup" + ); + + let mut mismatched_delete_header = fm.clone(); + mismatched_delete_header.versions[0].header.flags |= XL_FLAG_FREE_VERSION; + mismatched_delete_header.versions[0].header.version_type = VersionType::Delete; + let mismatched_original = mismatched_delete_header.clone(); + let err = mismatched_delete_header + .delete_version(&free_version_delete) + .expect_err("a Delete/free header over an Object body must fail closed"); + assert_eq!(err, Error::FileVersionNotFound); + assert_eq!( + mismatched_delete_header, mismatched_original, + "header/body mismatch must not mutate metadata" + ); + } + #[test] fn get_file_info_versions_excludes_free_versions_from_num_versions() { let object_version_id = Uuid::new_v4(); diff --git a/crates/filemeta/src/filemeta/version.rs b/crates/filemeta/src/filemeta/version.rs index d42da89ae..f491e75b9 100644 --- a/crates/filemeta/src/filemeta/version.rs +++ b/crates/filemeta/src/filemeta/version.rs @@ -29,10 +29,10 @@ use super::*; use crate::ChecksumInfo; use rustfs_utils::HashAlgorithm; use rustfs_utils::http::{ - SUFFIX_CRC, SUFFIX_FREE_VERSION, SUFFIX_INLINE_DATA, SUFFIX_PURGESTATUS, SUFFIX_TIER_FV_ID, SUFFIX_TIER_FV_MARKER, - SUFFIX_TRANSITION_STATUS, SUFFIX_TRANSITION_TIER, SUFFIX_TRANSITION_TIER_DESTINATION_ID, SUFFIX_TRANSITIONED_OBJECTNAME, - SUFFIX_TRANSITIONED_VERSION_ID, contains_key_bytes, get_bytes, get_consistent_bytes, get_str, has_internal_suffix, - insert_bytes, is_internal_key, remove_bytes, strip_internal_prefix, + RUSTFS_INTERNAL_PREFIX, SUFFIX_CRC, SUFFIX_FREE_VERSION, SUFFIX_INLINE_DATA, SUFFIX_PURGESTATUS, SUFFIX_TIER_FV_ID, + SUFFIX_TIER_FV_MARKER, SUFFIX_TRANSITION_STATUS, SUFFIX_TRANSITION_TIER, SUFFIX_TRANSITION_TIER_DESTINATION_ID, + SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, contains_key_bytes, get_bytes, get_consistent_bytes, get_str, + has_internal_suffix, insert_bytes, is_internal_key, remove_bytes, strip_internal_prefix, }; const MSGPACK_EXT8: u8 = 0xc7; @@ -249,6 +249,27 @@ fn parse_legacy_uuid_bytes(bytes: &[u8], field: &str) -> Result> { Ok((!id.is_nil()).then_some(id)) } +/// Decode a stored transitioned-version-id from a version's `meta_sys`. +/// +/// RustFS writes it as 16 raw UUID bytes; MinIO-migrated tiered objects store +/// the remote tier's version id as a UUID *string*. Accept both, and treat any +/// absent / nil / otherwise-unparseable value as "no tier version" (matching the +/// tolerant pre-hardening behavior) rather than failing the whole object read — +/// a malformed tier id must not make an otherwise-readable object unreadable. +fn transitioned_version_id_from_meta_sys(meta_sys: &HashMap>) -> Option { + let value = get_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID)?; + if value.is_empty() { + return None; + } + if let Ok(id) = Uuid::from_slice(&value) { + return (!id.is_nil()).then_some(id); + } + std::str::from_utf8(&value) + .ok() + .and_then(|s| Uuid::parse_str(s.trim()).ok()) + .filter(|id| !id.is_nil()) +} + fn parse_legacy_erasure_algo(value: &str) -> ErasureAlgo { match value { "ReedSolomon" => ErasureAlgo::ReedSolomon, @@ -807,7 +828,7 @@ impl TryFrom for FileMetaVersion { impl From for FileMetaVersion { fn from(value: FileInfo) -> Self { - if value.deleted { + if value.is_storage_delete_marker() { FileMetaVersion { version_type: VersionType::Delete, legacy_object: None, @@ -817,6 +838,22 @@ impl From for FileMetaVersion { uses_legacy_checksum: false, } } else { + // A `deleted` FileInfo that is not a canonical delete marker is only + // legitimate as a purge-pending payload, which carries real erasure + // geometry and is intentionally serialized as an Object. A `deleted` + // FileInfo with neither a canonical-marker shape nor valid erasure + // geometry would silently serialize as a zero-geometry MetaObject that + // later fails `validate_for_metadata_read`. Write paths validate first + // (`validate_for_erasure_write` / `validate_for_metadata_read`), so this + // is a caller bug — surface it rather than writing malformed metadata + // silently. (`From` is infallible, so this cannot return an error.) + if value.deleted && !value.has_valid_erasure_geometry() { + tracing::warn!( + event = "filemeta_non_canonical_deleted_fileinfo_as_object", + component = "filemeta", + "serializing a deleted FileInfo that is neither a canonical delete marker nor a valid erasure payload as an Object version; upstream validation should have rejected it" + ); + } FileMetaVersion { version_type: VersionType::Object, legacy_object: None, @@ -2361,9 +2398,7 @@ impl MetaObject { let transitioned_objname = get_bytes(&self.meta_sys, SUFFIX_TRANSITIONED_OBJECTNAME) .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - let transition_version_id = get_bytes(&self.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID) - .and_then(|v| Uuid::from_slice(v.as_slice()).ok()) - .filter(|u| !u.is_nil()); + let transition_version_id = transitioned_version_id_from_meta_sys(&self.meta_sys); let transition_tier = get_bytes(&self.meta_sys, SUFFIX_TRANSITION_TIER) .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); @@ -2671,9 +2706,7 @@ impl MetaDeleteMarker { .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - fi.transition_version_id = get_bytes(&self.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID) - .and_then(|v| Uuid::from_slice(v.as_slice()).ok()) - .filter(|u| !u.is_nil()); + fi.transition_version_id = transitioned_version_id_from_meta_sys(&self.meta_sys); } fi @@ -2788,10 +2821,54 @@ impl MetaDeleteMarker { impl From for MetaDeleteMarker { fn from(value: FileInfo) -> Self { + let mut meta_sys = HashMap::new(); + let mut durable_metadata: HashMap = HashMap::new(); + for (key, metadata_value) in &value.metadata { + if !is_internal_key(key) || is_skip_meta_key(key) { + continue; + } + let Some(suffix) = strip_internal_prefix(key) else { + continue; + }; + let rustfs_preferred = key + .get(..RUSTFS_INTERNAL_PREFIX.len()) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case(RUSTFS_INTERNAL_PREFIX)); + match durable_metadata.entry(suffix) { + std::collections::hash_map::Entry::Vacant(entry) => { + entry.insert((metadata_value, rustfs_preferred)); + } + std::collections::hash_map::Entry::Occupied(mut entry) if rustfs_preferred && !entry.get().1 => { + entry.insert((metadata_value, true)); + } + std::collections::hash_map::Entry::Occupied(_) => {} + } + } + for (suffix, (metadata_value, _)) in durable_metadata { + insert_bytes(&mut meta_sys, &suffix, metadata_value.as_bytes().to_vec()); + } + if value.tier_free_version() { + insert_bytes(&mut meta_sys, SUFFIX_FREE_VERSION, vec![]); + } + if !value.transition_status.is_empty() { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITION_STATUS, value.transition_status.as_bytes().to_vec()); + } + if !value.transitioned_objname.is_empty() { + insert_bytes( + &mut meta_sys, + SUFFIX_TRANSITIONED_OBJECTNAME, + value.transitioned_objname.as_bytes().to_vec(), + ); + } + if let Some(version_id) = value.transition_version_id { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, version_id.as_bytes().to_vec()); + } + if !value.transition_tier.is_empty() { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITION_TIER, value.transition_tier.as_bytes().to_vec()); + } Self { version_id: value.version_id, mod_time: value.mod_time, - meta_sys: HashMap::new(), + meta_sys, } } } @@ -3324,6 +3401,31 @@ mod tests { use super::*; use serde::Serialize; + #[test] + fn delete_marker_conversion_preserves_only_durable_internal_metadata() { + let mut marker = FileInfo::default(); + marker + .metadata + .insert("x-minio-internal-purgestatus".to_string(), "pending".to_string()); + marker + .metadata + .insert("x-rustfs-internal-healing".to_string(), "true".to_string()); + marker.metadata.insert("content-type".to_string(), "text/plain".to_string()); + let remote_version_id = Uuid::new_v4(); + marker.transition_version_id = Some(remote_version_id); + + let converted = MetaDeleteMarker::from(marker); + + assert_eq!(converted.meta_sys.get("x-rustfs-internal-purgestatus"), Some(&b"pending".to_vec())); + assert_eq!(converted.meta_sys.get("x-minio-internal-purgestatus"), Some(&b"pending".to_vec())); + assert_eq!( + get_bytes(&converted.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID), + Some(remote_version_id.as_bytes().to_vec()) + ); + assert!(!converted.meta_sys.contains_key("x-rustfs-internal-healing")); + assert!(!converted.meta_sys.contains_key("content-type")); + } + #[derive(Serialize)] enum LegacyDeleteVersionTypeFixture { #[serde(rename = "DeleteMarker")] @@ -3997,6 +4099,32 @@ mod tests { assert_eq!(fi.transition_version_id, Some(id)); } + #[test] + fn meta_object_transition_version_id_unparseable_stays_readable_as_none() { + // A non-UUID / non-16-byte tier version id must NOT make the object + // unreadable; it is tolerated as "no tier version" (compat with + // pre-hardening behavior and foreign/edge metadata). + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"not-a-uuid".to_vec()); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("unparseable transition version id must not fail the object read"); + assert_eq!(fi.transition_version_id, None); + } + + #[test] + fn meta_object_transition_version_id_minio_string_form_is_recovered() { + // MinIO-migrated tiered objects store the remote tier's version id as a + // UUID string, not 16 raw bytes; recover it instead of dropping it. + let id = sample_version_id(); + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, id.to_string().into_bytes()); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("string-form transition version id must decode"); + assert_eq!(fi.transition_version_id, Some(id)); + } + #[test] fn delete_marker_free_version_transition_version_id_nil_uuid_yields_none() { let mut sys = HashMap::new(); @@ -4026,6 +4154,47 @@ mod tests { assert_eq!(fi.transition_version_id, Some(id)); } + #[test] + fn delete_marker_free_version_transition_version_id_unparseable_stays_readable() { + // A malformed tier version id must not make a free-version record corrupt: + // it decodes to None and stays readable. Otherwise free-version expiry + // fails and the remote-tier object leaks. + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_FREE_VERSION, vec![]); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"not-a-uuid".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITION_TIER, b"WARM".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_OBJECTNAME, b"remote-object".to_vec()); + let fi = MetaDeleteMarker { + version_id: Some(sample_version_id()), + mod_time: Some(sample_mod_time()), + meta_sys: sys, + } + .into_fileinfo("b", "k", false); + + assert_eq!(fi.transition_version_id, None); + fi.validate_for_metadata_read() + .expect("free-version record with an unparseable tier id must remain readable"); + } + + #[test] + fn delete_marker_free_version_transition_version_id_minio_string_form_is_recovered() { + // MinIO stores the tier version id as a UUID string; recover it. + let id = sample_version_id(); + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_FREE_VERSION, vec![]); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, id.to_string().into_bytes()); + insert_bytes(&mut sys, SUFFIX_TRANSITION_TIER, b"WARM".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_OBJECTNAME, b"remote-object".to_vec()); + let fi = MetaDeleteMarker { + version_id: Some(sample_version_id()), + mod_time: Some(sample_mod_time()), + meta_sys: sys, + } + .into_fileinfo("b", "k", false); + + assert_eq!(fi.transition_version_id, Some(id)); + } + #[test] fn version_header_sorts_before_prefers_object_over_delete_marker_on_equal_mod_time() { let object = FileMetaVersionHeader { diff --git a/crates/filemeta/src/metacache.rs b/crates/filemeta/src/metacache.rs index 3a8d18e38..ab602e396 100644 --- a/crates/filemeta/src/metacache.rs +++ b/crates/filemeta/src/metacache.rs @@ -1333,7 +1333,6 @@ mod tests { name: "object".to_string(), version_id: Some(free_version_id), deleted: true, - mod_time: Some(OffsetDateTime::now_utc()), ..Default::default() }; free_delete_fi.set_tier_free_version(); diff --git a/crates/obs/src/metrics/stats_collector.rs b/crates/obs/src/metrics/stats_collector.rs index 45a9d7f40..a0c6c19c4 100644 --- a/crates/obs/src/metrics/stats_collector.rs +++ b/crates/obs/src/metrics/stats_collector.rs @@ -50,6 +50,7 @@ const LOG_SUBSYSTEM_METRICS_COLLECTOR: &str = "metrics_collector"; const EVENT_METRICS_COLLECTOR_STATE: &str = "metrics_collector_state"; type ObsStorageInfo = ::StorageInfo; +type ObsBackendInfo = ::BackendInfo; struct ObsDataUsageInfo { last_update: Option, @@ -789,43 +790,86 @@ pub fn collect_internode_network_stats() -> Option { } /// Collect cluster config metrics from backend parity configuration. +fn cluster_config_stats_from_backend_parities( + rr_sc_parity: Option, + standard_sc_parity: Option, +) -> Option { + Some(ClusterConfigStats { + rrs_parity: u32::try_from(rr_sc_parity?).ok()?, + standard_parity: u32::try_from(standard_sc_parity?).ok()?, + }) +} + pub async fn collect_cluster_config_stats() -> Option { let store = resolve_obs_object_store_handle()?; let backend = StorageAdminApi::backend_info(store.as_ref()).await; - Some(ClusterConfigStats { - rrs_parity: backend.rr_sc_parity.unwrap_or_default() as u32, - standard_parity: backend.standard_sc_parity.unwrap_or_default() as u32, - }) + cluster_config_stats_from_backend_parities(backend.rr_sc_parity, backend.standard_sc_parity) } -/// Collect cluster erasure set metrics from storage and backend topology info. -pub async fn collect_erasure_set_stats() -> Vec { - let Some(store) = resolve_obs_object_store_handle() else { - return Vec::new(); +fn standard_erasure_layout_from_backend(backend: &ObsBackendInfo, pool_idx: usize) -> Option<(usize, usize)> { + let drives_per_set = backend.drives_per_set.get(pool_idx).copied()?; + if drives_per_set == 0 + || (!backend.standard_sc_data.is_empty() && backend.standard_sc_data.len() != backend.drives_per_set.len()) + || (!backend.standard_sc_parities.is_empty() && backend.standard_sc_parities.len() != backend.drives_per_set.len()) + { + return None; + } + + let has_data = !backend.standard_sc_data.is_empty(); + let has_parities = !backend.standard_sc_parities.is_empty(); + let (data, parity) = match (has_data, has_parities) { + (true, true) => ( + backend.standard_sc_data.get(pool_idx).copied()?, + backend.standard_sc_parities.get(pool_idx).copied()?, + ), + (true, false) => { + let data = backend.standard_sc_data.get(pool_idx).copied()?; + (data, drives_per_set.checked_sub(data)?) + } + (false, true) => { + let parity = backend.standard_sc_parities.get(pool_idx).copied()?; + (drives_per_set.checked_sub(parity)?, parity) + } + (false, false) => { + let parity = backend.standard_sc_parity?; + (drives_per_set.checked_sub(parity)?, parity) + } }; - let storage_info = StorageAdminApi::storage_info(store.as_ref()).await; - let backend = StorageAdminApi::backend_info(store.as_ref()).await; + if data == 0 || parity > data || data.checked_add(parity) != Some(drives_per_set) { + return None; + } + + Some((data, parity)) +} + +fn erasure_set_stats_from_backend(storage_info: &ObsStorageInfo, backend: &ObsBackendInfo) -> Vec { let mut grouped: HashMap<(usize, usize), ErasureSetStats> = HashMap::new(); for disk in &storage_info.disks { - let pool_idx = disk.pool_index.max(0) as usize; - let set_idx = disk.set_index.max(0) as usize; - let set_drive_count = backend.drives_per_set.get(pool_idx).copied().unwrap_or_default(); - let parity = backend - .standard_sc_parities - .get(pool_idx) - .copied() - .or(backend.standard_sc_parity) - .unwrap_or(set_drive_count / 2); + let (Ok(pool_idx), Ok(set_idx)) = (usize::try_from(disk.pool_index), usize::try_from(disk.set_index)) else { + continue; + }; + let Some((_, parity)) = standard_erasure_layout_from_backend(backend, pool_idx) else { + continue; + }; + let set_drive_count = backend.drives_per_set[pool_idx]; + let (Ok(pool_id), Ok(set_id), Ok(size), Ok(parity_metric)) = ( + u32::try_from(pool_idx), + u32::try_from(set_idx), + u32::try_from(set_drive_count), + u32::try_from(parity), + ) else { + continue; + }; let quorum_shape = derive_erasure_set_quorum_shape(set_drive_count, parity); let entry = grouped.entry((pool_idx, set_idx)).or_insert_with(|| ErasureSetStats { - pool_id: pool_idx as u32, - set_id: set_idx as u32, - size: set_drive_count as u32, - parity: parity as u32, + pool_id, + set_id, + size, + parity: parity_metric, data_shards: quorum_shape.data_shards, read_quorum: quorum_shape.read_quorum, write_quorum: quorum_shape.write_quorum, @@ -855,6 +899,17 @@ pub async fn collect_erasure_set_stats() -> Vec { stats } +/// Collect cluster erasure set metrics from storage and backend topology info. +pub async fn collect_erasure_set_stats() -> Vec { + let Some(store) = resolve_obs_object_store_handle() else { + return Vec::new(); + }; + + let storage_info = StorageAdminApi::storage_info(store.as_ref()).await; + let backend = StorageAdminApi::backend_info(store.as_ref()).await; + erasure_set_stats_from_backend(&storage_info, &backend) +} + pub async fn collect_iam_stats() -> Option { let snapshot = iam_metrics_snapshot()?; @@ -1119,6 +1174,100 @@ mod tests { use std::thread; use std::time::Duration; + fn storage_info_with_one_online_disk() -> ObsStorageInfo { + let mut info = ObsStorageInfo::default(); + info.disks.push(Default::default()); + let disk = info.disks.last_mut().expect("inserted disk should exist"); + disk.pool_index = 0; + disk.set_index = 0; + disk.disk_index = 0; + disk.state = DRIVE_STATE_OK.to_string(); + disk.runtime_state = Some(DRIVE_STATE_ONLINE.to_string()); + info + } + + #[test] + fn cluster_config_stats_accept_homogeneous_backend_parities() { + let stats = cluster_config_stats_from_backend_parities(Some(1), Some(2)) + .expect("homogeneous scalar parities should produce cluster config metrics"); + + assert_eq!(stats.rrs_parity, 1); + assert_eq!(stats.standard_parity, 2); + } + + #[test] + fn cluster_config_stats_skip_heterogeneous_backend_parities() { + assert!(cluster_config_stats_from_backend_parities(Some(1), None).is_none()); + assert!(cluster_config_stats_from_backend_parities(None, Some(2)).is_none()); + } + + #[test] + fn cluster_config_stats_reject_parity_larger_than_u32() { + let Ok(overflow) = usize::try_from(u64::from(u32::MAX) + 1) else { + return; + }; + + assert!(cluster_config_stats_from_backend_parities(Some(overflow), Some(2)).is_none()); + assert!(cluster_config_stats_from_backend_parities(Some(1), Some(overflow)).is_none()); + } + + #[test] + fn erasure_set_stats_skip_unknown_backend_layout() { + let storage_info = storage_info_with_one_online_disk(); + let backend = ObsBackendInfo { + drives_per_set: vec![8], + ..Default::default() + }; + + assert!(erasure_set_stats_from_backend(&storage_info, &backend).is_empty()); + } + + #[test] + fn erasure_set_stats_skip_inconsistent_exact_layout_without_scalar_fallback() { + let storage_info = storage_info_with_one_online_disk(); + let backend = ObsBackendInfo { + standard_sc_data: vec![6], + standard_sc_parities: vec![4], + standard_sc_parity: Some(2), + drives_per_set: vec![8], + ..Default::default() + }; + + assert!(erasure_set_stats_from_backend(&storage_info, &backend).is_empty()); + } + + #[test] + fn erasure_set_stats_accept_truthful_legacy_scalar_layout() { + let storage_info = storage_info_with_one_online_disk(); + let backend = ObsBackendInfo { + standard_sc_parity: Some(2), + drives_per_set: vec![8], + ..Default::default() + }; + + let stats = erasure_set_stats_from_backend(&storage_info, &backend); + assert_eq!(stats.len(), 1); + assert_eq!(stats[0].size, 8); + assert_eq!(stats[0].parity, 2); + assert_eq!(stats[0].data_shards, 6); + } + + #[test] + fn erasure_set_stats_exact_data_ignores_stale_scalar() { + let storage_info = storage_info_with_one_online_disk(); + let backend = ObsBackendInfo { + standard_sc_data: vec![6], + standard_sc_parity: Some(4), + drives_per_set: vec![8], + ..Default::default() + }; + + let stats = erasure_set_stats_from_backend(&storage_info, &backend); + assert_eq!(stats.len(), 1); + assert_eq!(stats[0].parity, 2); + assert_eq!(stats[0].data_shards, 6); + } + fn generate_loopback_traffic() -> std::io::Result<()> { let listener = TcpListener::bind(("127.0.0.1", 0))?; let addr = listener.local_addr()?; diff --git a/crates/scanner/src/scanner_io.rs b/crates/scanner/src/scanner_io.rs index f5d4a5bf2..056a227f3 100644 --- a/crates/scanner/src/scanner_io.rs +++ b/crates/scanner/src/scanner_io.rs @@ -2487,10 +2487,16 @@ mod tests { meta.add_version(fi).expect("object version should be added"); } - let mut delete_marker = FileInfo::new(object, 1, 1); - delete_marker.version_id = Some(Uuid::new_v4()); - delete_marker.mod_time = Some(OffsetDateTime::from_unix_timestamp(30).expect("timestamp should be valid")); - delete_marker.deleted = true; + // A real delete marker carries no erasure geometry (delete paths build it as + // `FileInfo { deleted: true, .. }`). Construct it that way so it classifies as a + // storage delete marker rather than a purge-pending payload object. + let delete_marker = FileInfo { + name: object.to_string(), + version_id: Some(Uuid::new_v4()), + mod_time: Some(OffsetDateTime::from_unix_timestamp(30).expect("timestamp should be valid")), + deleted: true, + ..Default::default() + }; meta.add_version(delete_marker).expect("delete marker should be added"); tokio::fs::write(&metadata_path, meta.marshal_msg().expect("metadata should marshal")) diff --git a/rustfs/src/admin/handlers/config_admin.rs b/rustfs/src/admin/handlers/config_admin.rs index 0ce0a69aa..c51966ca9 100644 --- a/rustfs/src/admin/handlers/config_admin.rs +++ b/rustfs/src/admin/handlers/config_admin.rs @@ -18,8 +18,9 @@ use crate::admin::runtime_sources::{ current_app_context, current_object_store_handle_for_context, current_server_config_for_context, publish_server_config, }; use crate::admin::service::config::{ - apply_dynamic_config_for_subsystem, is_dynamic_config_subsystem, signal_config_snapshot_reload, signal_dynamic_config_reload, - validate_server_config, + CONFIG_WORKER_RELOAD_FAILURE_STATE, EVENT_CONFIG_WORKER_RELOAD_FAILED, FULL_CONFIG_WORKER_SUBSYSTEMS, LOG_COMPONENT_ADMIN, + LOG_SUBSYSTEM_CONFIG, PreparedRuntimeConfig, apply_dynamic_config_for_subsystem, is_dynamic_config_subsystem, + prepare_server_config, signal_config_snapshot_reload, signal_dynamic_config_reload, }; use crate::admin::storage_api::config::storageclass::{INLINE_BLOCK_ENV, OPTIMIZE_ENV, RRS_ENV, STANDARD_ENV}; use crate::admin::storage_api::config::{ @@ -81,7 +82,9 @@ use s3s::{Body, S3Error, S3ErrorCode, S3Request, S3Response, S3Result, s3_error} use serde::Serialize; use std::collections::{BTreeSet, HashMap}; use std::env; +use std::future::Future; use time::OffsetDateTime; +use tracing::warn; use uuid::Uuid; const REDACTED_VALUE: &str = "*redacted*"; @@ -94,7 +97,6 @@ const CONFIG_APPLIED_HEADER: &str = "x-rustfs-config-applied"; const CONFIG_APPLIED_COMPAT_HEADER: &str = "x-minio-config-applied"; const CONFIG_APPLIED_TRUE: &str = "true"; const DEFAULT_COMMENT_DESCRIPTION: &str = "optionally add a comment to this setting"; - #[derive(Debug, Clone, PartialEq, Eq)] struct ConfigEntry { key: String, @@ -1542,20 +1544,37 @@ fn build_help_response(sub_system: Option<&str>, key: Option<&str>, env_only: bo }) } -/// Re-apply all dynamic subsystems from the given config and signal peers to reload. -/// Used after full-config operations (restore, set-config) where the leader replaces -/// the entire config and must ensure runtime state (e.g. GLOBAL_STORAGE_CLASS) is -/// refreshed on both the leader and all peers. -async fn apply_and_signal_dynamic_subsystems(config: &ServerConfig) { - for sub_system in [ - STORAGE_CLASS_SUB_SYS, - AUDIT_WEBHOOK_SUB_SYS, - AUDIT_MQTT_SUB_SYS, - SCANNER_SUB_SYS, - HEAL_SUB_SYS, - ] { - if apply_dynamic_config_for_subsystem(config, sub_system).await.unwrap_or(false) { - signal_dynamic_config_reload(sub_system).await; +fn publish_prepared_config_snapshots(config: ServerConfig, prepared: PreparedRuntimeConfig) -> S3Result<()> { + prepared.publish_storage_class()?; + publish_server_config(config); + Ok(()) +} + +async fn commit_prepared_config( + config: ServerConfig, + prepared: PreparedRuntimeConfig, + persist: impl Future>, + publish: impl FnOnce(ServerConfig, PreparedRuntimeConfig) -> S3Result<()>, +) -> S3Result<()> { + persist.await?; + publish(config, prepared) +} + +/// Re-apply local mutable worker families after a full-config replacement. +/// Peers receive one full-snapshot signal after this returns; signaling each +/// family here as well would recreate audit/scanner targets twice per peer. +async fn apply_dynamic_subsystems(config: &ServerConfig) { + for sub_system in FULL_CONFIG_WORKER_SUBSYSTEMS { + if let Err(err) = apply_dynamic_config_for_subsystem(config, sub_system).await { + warn!( + event = EVENT_CONFIG_WORKER_RELOAD_FAILED, + component = LOG_COMPONENT_ADMIN, + subsystem = LOG_SUBSYSTEM_CONFIG, + config_subsystem = sub_system, + state = CONFIG_WORKER_RELOAD_FAILURE_STATE, + error = %err, + "Published server config but failed to reload a local worker subsystem" + ); } } } @@ -1595,21 +1614,34 @@ impl Operation for SetConfigKVHandler { let sub_system = config_update_sub_system(&directives)?; let mut config = load_server_config_from_store().await?; apply_set_directives(&mut config, &directives)?; - validate_server_config(&config, sub_system).await?; + let prepared = prepare_server_config(&config, sub_system).await?; save_server_config_history(&body).await?; - save_server_config_to_store(&config).await?; - publish_server_config(config.clone()); - let mut config_applied = false; - if let Some(sub_system) = sub_system - && is_dynamic_config_subsystem(sub_system) - { - config_applied = apply_dynamic_config_for_subsystem(&config, sub_system).await?; - if config_applied { - signal_dynamic_config_reload(sub_system).await; - } + let config_applied = if sub_system == Some(STORAGE_CLASS_SUB_SYS) { + commit_prepared_config( + config.clone(), + prepared, + save_server_config_to_store(&config), + publish_prepared_config_snapshots, + ) + .await?; + signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await; + true } else { - signal_config_snapshot_reload().await; - } + save_server_config_to_store(&config).await?; + publish_server_config(config.clone()); + if let Some(sub_system) = sub_system + && is_dynamic_config_subsystem(sub_system) + { + let config_applied = apply_dynamic_config_for_subsystem(&config, sub_system).await?; + if config_applied { + signal_dynamic_config_reload(sub_system).await; + } + config_applied + } else { + signal_config_snapshot_reload().await; + false + } + }; success_response(config_applied) } @@ -1631,21 +1663,34 @@ impl Operation for DelConfigKVHandler { let sub_system = config_update_sub_system(&directives)?; let mut config = load_server_config_from_store().await?; apply_delete_directives(&mut config, &directives); - validate_server_config(&config, sub_system).await?; + let prepared = prepare_server_config(&config, sub_system).await?; save_server_config_history(&body).await?; - save_server_config_to_store(&config).await?; - publish_server_config(config.clone()); - let mut config_applied = false; - if let Some(sub_system) = sub_system - && is_dynamic_config_subsystem(sub_system) - { - config_applied = apply_dynamic_config_for_subsystem(&config, sub_system).await?; - if config_applied { - signal_dynamic_config_reload(sub_system).await; - } + let config_applied = if sub_system == Some(STORAGE_CLASS_SUB_SYS) { + commit_prepared_config( + config.clone(), + prepared, + save_server_config_to_store(&config), + publish_prepared_config_snapshots, + ) + .await?; + signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await; + true } else { - signal_config_snapshot_reload().await; - } + save_server_config_to_store(&config).await?; + publish_server_config(config.clone()); + if let Some(sub_system) = sub_system + && is_dynamic_config_subsystem(sub_system) + { + let config_applied = apply_dynamic_config_for_subsystem(&config, sub_system).await?; + if config_applied { + signal_dynamic_config_reload(sub_system).await; + } + config_applied + } else { + signal_config_snapshot_reload().await; + false + } + }; success_response(config_applied) } @@ -1730,10 +1775,16 @@ impl Operation for RestoreConfigHistoryKVHandler { let mut config = ServerConfig::new(); apply_set_directives(&mut config, &directives)?; - validate_server_config(&config, None).await?; - save_server_config_to_store(&config).await?; - publish_server_config(config.clone()); - apply_and_signal_dynamic_subsystems(&config).await; + let prepared = prepare_server_config(&config, None).await?; + commit_prepared_config( + config.clone(), + prepared, + save_server_config_to_store(&config), + publish_prepared_config_snapshots, + ) + .await?; + signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await; + apply_dynamic_subsystems(&config).await; signal_config_snapshot_reload().await; success_response(false) @@ -1768,11 +1819,17 @@ impl Operation for SetConfigHandler { let mut config = ServerConfig::new(); apply_set_directives(&mut config, &directives)?; - validate_server_config(&config, None).await?; + let prepared = prepare_server_config(&config, None).await?; save_server_config_history(&body).await?; - save_server_config_to_store(&config).await?; - publish_server_config(config.clone()); - apply_and_signal_dynamic_subsystems(&config).await; + commit_prepared_config( + config.clone(), + prepared, + save_server_config_to_store(&config), + publish_prepared_config_snapshots, + ) + .await?; + signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await; + apply_dynamic_subsystems(&config).await; signal_config_snapshot_reload().await; success_response(false) @@ -1782,6 +1839,142 @@ impl Operation for SetConfigHandler { #[cfg(test)] mod tests { use super::*; + use std::sync::{Arc, Mutex}; + + #[tokio::test] + async fn prepared_config_commit_persists_before_publish() { + let events = Arc::new(Mutex::new(Vec::new())); + let persist_events = events.clone(); + let publish_events = events.clone(); + + commit_prepared_config( + ServerConfig::new(), + PreparedRuntimeConfig::default(), + async move { + persist_events.lock().expect("persist events lock").push("persist"); + Ok(()) + }, + move |_, _| { + publish_events.lock().expect("publish events lock").push("publish"); + Ok(()) + }, + ) + .await + .expect("prepared config commit"); + + assert_eq!(*events.lock().expect("result events lock"), ["persist", "publish"]); + } + + #[tokio::test] + async fn prepared_config_commit_does_not_publish_after_persist_failure() { + let events = Arc::new(Mutex::new(Vec::new())); + let persist_events = events.clone(); + let publish_events = events.clone(); + + let err = commit_prepared_config( + ServerConfig::new(), + PreparedRuntimeConfig::default(), + async move { + persist_events.lock().expect("persist events lock").push("persist"); + Err(s3_error!(InternalError, "injected persistence failure")) + }, + move |_, _| { + publish_events.lock().expect("publish events lock").push("publish"); + Ok(()) + }, + ) + .await + .expect_err("persistence failure must propagate"); + + assert_eq!(err.code(), &S3ErrorCode::InternalError); + assert_eq!(*events.lock().expect("result events lock"), ["persist"]); + } + + #[test] + fn storage_config_write_handlers_persist_before_publishing() { + const SOURCE: &str = include_str!("config_admin.rs"); + + for (handler, next_handler, follow_up) in [ + ( + "SetConfigKVHandler", + "DelConfigKVHandler", + "signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await", + ), + ( + "DelConfigKVHandler", + "HelpConfigKVHandler", + "signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await", + ), + ( + "RestoreConfigHistoryKVHandler", + "GetConfigHandler", + "signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await", + ), + ( + "SetConfigHandler", + "#[cfg(test)]", + "signal_dynamic_config_reload(STORAGE_CLASS_SUB_SYS).await", + ), + ] { + let start_marker = format!("impl Operation for {handler}"); + let start = SOURCE + .find(&start_marker) + .unwrap_or_else(|| panic!("missing {handler} implementation")); + let tail = &SOURCE[start..]; + let end = tail + .find(next_handler) + .unwrap_or_else(|| panic!("missing {next_handler} after {handler}")); + let implementation = &tail[..end]; + let prepared_commit_path = if matches!(handler, "SetConfigKVHandler" | "DelConfigKVHandler") { + let branch_start = implementation + .find("if sub_system == Some(STORAGE_CLASS_SUB_SYS)") + .unwrap_or_else(|| panic!("missing storage-class branch in {handler}")); + let branch = &implementation[branch_start..]; + let branch_end = branch + .find("} else {") + .unwrap_or_else(|| panic!("missing non-storage branch in {handler}")); + &branch[..branch_end] + } else { + implementation + }; + + assert_eq!(prepared_commit_path.matches("commit_prepared_config(").count(), 1, "{handler}"); + let commit_start = prepared_commit_path.find("commit_prepared_config(").expect("commit call"); + let commit_end = prepared_commit_path[commit_start..].find(';').expect("commit terminator") + commit_start; + let commit_statement = &prepared_commit_path[commit_start..=commit_end]; + assert!(commit_statement.contains(".await?;"), "{handler} must propagate commit failure"); + + let follow_up_start = prepared_commit_path + .find(follow_up) + .unwrap_or_else(|| panic!("missing follow-up in {handler}")); + assert!(follow_up_start > commit_end, "{handler} must run follow-up only after commit"); + assert!( + !prepared_commit_path.contains("publish_server_config("), + "{handler} must not publish directly" + ); + assert!( + !prepared_commit_path.contains(".publish_storage_class("), + "{handler} must not publish directly" + ); + + if matches!(handler, "RestoreConfigHistoryKVHandler" | "SetConfigHandler") { + let worker_start = prepared_commit_path + .find("apply_dynamic_subsystems(&config).await") + .unwrap_or_else(|| panic!("missing local worker apply in {handler}")); + let signal_start = prepared_commit_path + .find("signal_config_snapshot_reload().await") + .unwrap_or_else(|| panic!("missing snapshot signal in {handler}")); + assert!( + worker_start > follow_up_start, + "{handler} must converge peer parity before local worker apply" + ); + assert!( + signal_start > worker_start, + "{handler} must signal the full snapshot after local worker apply" + ); + } + } + } #[test] fn tokenize_config_line_handles_quotes_and_escapes() { diff --git a/rustfs/src/admin/runtime_sources.rs b/rustfs/src/admin/runtime_sources.rs index 50e6d9f28..43e5dc0ed 100644 --- a/rustfs/src/admin/runtime_sources.rs +++ b/rustfs/src/admin/runtime_sources.rs @@ -29,6 +29,8 @@ pub(crate) use crate::runtime_sources::{ current_region, current_replication_pool_handle, current_replication_stats_handle, current_server_config_for_context, current_token_signing_key, }; +#[cfg(test)] +pub(crate) use crate::runtime_sources::{IamInterface, KmsInterface, ServerConfigInterface, StorageClassInterface}; use rustfs_config::server_config::Config; use rustfs_kms::KmsServiceManager; use rustfs_tls_runtime::{GlobalPublishedOutboundTlsState, TlsGeneration}; diff --git a/rustfs/src/admin/service/config.rs b/rustfs/src/admin/service/config.rs index 85222e6b6..e2dc44ecc 100644 --- a/rustfs/src/admin/service/config.rs +++ b/rustfs/src/admin/service/config.rs @@ -33,6 +33,7 @@ use rustfs_targets::config::{ validate_postgres_config, validate_pulsar_config, validate_redis_config, validate_webhook_config, }; use s3s::{S3Error, S3ErrorCode, S3Result}; +use std::future::Future; use tracing::warn; use url::Url; @@ -43,6 +44,12 @@ pub fn is_dynamic_config_subsystem(sub_system: &str) -> bool { ) } +pub(crate) const FULL_CONFIG_WORKER_SUBSYSTEMS: [&str; 2] = [AUDIT_WEBHOOK_SUB_SYS, SCANNER_SUB_SYS]; +pub(crate) const EVENT_CONFIG_WORKER_RELOAD_FAILED: &str = "config_worker_reload_failed"; +pub(crate) const LOG_COMPONENT_ADMIN: &str = "admin"; +pub(crate) const LOG_SUBSYSTEM_CONFIG: &str = "config"; +pub(crate) const CONFIG_WORKER_RELOAD_FAILURE_STATE: &str = "best_effort_reload_failed"; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum DynamicConfigWorkerMutation { None, @@ -71,39 +78,87 @@ fn resolve_runtime_config_store_for_context(context: Option<&AppContext>) -> S3R current_object_store_handle_for_context(context).ok_or_else(|| internal_error("storage layer not initialized")) } -async fn apply_storage_class_runtime_config_for_context(context: Option<&AppContext>, config: &ServerConfig) -> S3Result<()> { - let store = resolve_runtime_config_store_for_context(context)?; - - let kvs = config.get_value(STORAGE_CLASS_SUB_SYS, DEFAULT_DELIMITER).unwrap_or_default(); - let set_drive_count = StorageAdminApi::set_drive_counts(store.as_ref()) - .into_iter() - .next() - .unwrap_or(1); - let parsed = storageclass::lookup_config(&kvs, set_drive_count) - .map_err(|err| internal_error(format!("failed to apply storage class config: {err}")))?; - publish_storage_class_config(parsed); - Ok(()) +#[derive(Debug, Default)] +pub(crate) struct PreparedRuntimeConfig { + storage_class: Option, } -fn validate_storage_class_kvs(kvs: &KVS, set_drive_counts: &[usize]) -> S3Result<()> { - for count in set_drive_counts { - storageclass::lookup_config(kvs, *count) - .map_err(|err| invalid_request(format!("invalid storage class config: {err}")))?; +impl PreparedRuntimeConfig { + fn publish_storage_class_with(self, publish: impl FnOnce(storageclass::Config)) -> bool { + let Some(config) = self.storage_class else { + return false; + }; + + publish(config); + true } - Ok(()) + fn publish_storage_class_for_context_with( + self, + context: Option<&AppContext>, + publish_fallback: impl FnOnce(storageclass::Config), + ) -> S3Result<()> { + if self.publish_storage_class_with(|config| { + if let Some(context) = context { + context.storage_class().set(config); + } else { + publish_fallback(config); + } + }) { + Ok(()) + } else { + Err(internal_error("prepared storage class candidate is missing")) + } + } + + fn publish_storage_class_for_context(self, context: Option<&AppContext>) -> S3Result<()> { + self.publish_storage_class_for_context_with(context, publish_storage_class_config) + } + + pub(crate) fn publish_storage_class(self) -> S3Result<()> { + let context = current_app_context(); + self.publish_storage_class_for_context(context.as_deref()) + } } -async fn validate_storage_class_config_for_context(context: Option<&AppContext>, config: &ServerConfig) -> S3Result<()> { - let store = resolve_runtime_config_store_for_context(context)?; +fn publish_server_config_for_context(context: Option<&AppContext>, config: ServerConfig) { + if let Some(context) = context { + context.server_config().set(config); + } else { + publish_server_config(config); + } +} +fn prepare_storage_class_kvs(kvs: &KVS, set_drive_counts: &[usize]) -> S3Result { + storageclass::lookup_config_for_pools(kvs, set_drive_counts) + .map_err(|err| invalid_request(format!("invalid storage class config: {err}"))) +} + +async fn prepare_storage_class_runtime_config_for_context( + context: Option<&AppContext>, + config: &ServerConfig, +) -> S3Result { + let store = resolve_runtime_config_store_for_context(context)?; let kvs = config.get_value(STORAGE_CLASS_SUB_SYS, DEFAULT_DELIMITER).unwrap_or_default(); let set_drive_counts = StorageAdminApi::set_drive_counts(store.as_ref()); - if set_drive_counts.is_empty() { - return validate_storage_class_kvs(&kvs, &[1]); - } - validate_storage_class_kvs(&kvs, &set_drive_counts) + prepare_storage_class_kvs(&kvs, &set_drive_counts) +} + +async fn apply_storage_class_runtime_config_for_context(context: Option<&AppContext>, config: &ServerConfig) -> S3Result<()> { + let parsed = prepare_storage_class_runtime_config_for_context(context, config) + .await + .map_err(|err| internal_error(format!("failed to apply storage class config: {err}")))?; + PreparedRuntimeConfig { + storage_class: Some(parsed), + } + .publish_storage_class_for_context(context)?; + Ok(()) +} + +#[cfg(test)] +fn validate_storage_class_kvs(kvs: &KVS, set_drive_counts: &[usize]) -> S3Result<()> { + prepare_storage_class_kvs(kvs, set_drive_counts).map(|_| ()) } fn target_enabled(kvs: &KVS) -> bool { @@ -248,23 +303,27 @@ fn validate_identity_openid_config(config: &ServerConfig) -> S3Result<()> { Ok(()) } -pub async fn validate_server_config_for_context( +pub(crate) async fn prepare_server_config_for_context( context: Option<&AppContext>, config: &ServerConfig, sub_system: Option<&str>, -) -> S3Result<()> { +) -> S3Result { + let mut prepared = PreparedRuntimeConfig::default(); + match sub_system { - Some(STORAGE_CLASS_SUB_SYS) => validate_storage_class_config_for_context(context, config).await, - Some(NOTIFY_WEBHOOK_SUB_SYS) => validate_notify_subsystem_config(config, NOTIFY_WEBHOOK_SUB_SYS), - Some(NOTIFY_MQTT_SUB_SYS) => validate_notify_subsystem_config(config, NOTIFY_MQTT_SUB_SYS), - Some(AUDIT_WEBHOOK_SUB_SYS) => validate_audit_subsystem_config(config, AUDIT_WEBHOOK_SUB_SYS), - Some(AUDIT_MQTT_SUB_SYS) => validate_audit_subsystem_config(config, AUDIT_MQTT_SUB_SYS), - Some(IDENTITY_OPENID_SUB_SYS) => validate_identity_openid_config(config), + Some(STORAGE_CLASS_SUB_SYS) => { + prepared.storage_class = Some(prepare_storage_class_runtime_config_for_context(context, config).await?); + } + Some(NOTIFY_WEBHOOK_SUB_SYS) => validate_notify_subsystem_config(config, NOTIFY_WEBHOOK_SUB_SYS)?, + Some(NOTIFY_MQTT_SUB_SYS) => validate_notify_subsystem_config(config, NOTIFY_MQTT_SUB_SYS)?, + Some(AUDIT_WEBHOOK_SUB_SYS) => validate_audit_subsystem_config(config, AUDIT_WEBHOOK_SUB_SYS)?, + Some(AUDIT_MQTT_SUB_SYS) => validate_audit_subsystem_config(config, AUDIT_MQTT_SUB_SYS)?, + Some(IDENTITY_OPENID_SUB_SYS) => validate_identity_openid_config(config)?, Some(SCANNER_SUB_SYS | HEAL_SUB_SYS) => rustfs_scanner::validate_scanner_runtime_config(config) - .map_err(|err| invalid_request(format!("invalid scanner config: {err}"))), - Some(_) => Ok(()), + .map_err(|err| invalid_request(format!("invalid scanner config: {err}")))?, + Some(_) => {} None => { - validate_storage_class_config_for_context(context, config).await?; + prepared.storage_class = Some(prepare_storage_class_runtime_config_for_context(context, config).await?); validate_notify_subsystem_config(config, NOTIFY_WEBHOOK_SUB_SYS)?; validate_notify_subsystem_config(config, NOTIFY_MQTT_SUB_SYS)?; validate_audit_subsystem_config(config, AUDIT_WEBHOOK_SUB_SYS)?; @@ -272,9 +331,25 @@ pub async fn validate_server_config_for_context( validate_identity_openid_config(config)?; rustfs_scanner::validate_scanner_runtime_config(config) .map_err(|err| invalid_request(format!("invalid scanner config: {err}")))?; - Ok(()) } } + + Ok(prepared) +} + +pub async fn validate_server_config_for_context( + context: Option<&AppContext>, + config: &ServerConfig, + sub_system: Option<&str>, +) -> S3Result<()> { + prepare_server_config_for_context(context, config, sub_system) + .await + .map(|_| ()) +} + +pub(crate) async fn prepare_server_config(config: &ServerConfig, sub_system: Option<&str>) -> S3Result { + let context = current_app_context(); + prepare_server_config_for_context(context.as_deref(), config, sub_system).await } pub async fn validate_server_config(config: &ServerConfig, sub_system: Option<&str>) -> S3Result<()> { @@ -317,7 +392,6 @@ pub async fn reload_dynamic_config_runtime_state_for_context(context: Option<&Ap } let store = resolve_runtime_config_store_for_context(context)?; - let config = read_admin_config_without_migrate(store).await.map_err(|err| { warn!("peer reload_dynamic_config: failed to load server config for {sub_system}: {err}"); internal_error(format!("failed to load server config: {err}")) @@ -336,30 +410,81 @@ pub async fn reload_dynamic_config_runtime_state(sub_system: &str) -> S3Result<( reload_dynamic_config_runtime_state_for_context(context.as_deref(), sub_system).await } +async fn reload_runtime_config_snapshot_with( + read: ReadFuture, + prepare: Prepare, + publish: Publish, + apply_workers: ApplyWorkers, +) -> S3Result<()> +where + ReadFuture: Future>, + Prepare: FnOnce(ServerConfig) -> PrepareFuture, + PrepareFuture: Future>, + Publish: FnOnce(&ServerConfig, PreparedRuntimeConfig) -> S3Result<()>, + ApplyWorkers: FnOnce(ServerConfig) -> ApplyWorkersFuture, + ApplyWorkersFuture: Future>, +{ + let config = read.await?; + let (config, prepared) = prepare(config).await?; + publish(&config, prepared)?; + + // Worker reloads mutate live state and have no rollback contract. They are + // therefore best-effort after the validated storage/server snapshots are + // published; a transient worker failure must not leave this peer on stale + // erasure geometry. + if let Err(err) = apply_workers(config).await { + warn!( + event = EVENT_CONFIG_WORKER_RELOAD_FAILED, + component = LOG_COMPONENT_ADMIN, + subsystem = LOG_SUBSYSTEM_CONFIG, + state = CONFIG_WORKER_RELOAD_FAILURE_STATE, + error = ?err, + "Runtime config snapshot was published but a worker reload failed" + ); + } + Ok(()) +} + pub async fn reload_runtime_config_snapshot_for_context(context: Option<&AppContext>) -> S3Result<()> { let store = resolve_runtime_config_store_for_context(context)?; - let config = read_admin_config_without_migrate(store).await.map_err(|err| { - warn!("peer reload_runtime_config_snapshot: failed to load server config: {err}"); - internal_error(format!("failed to load server config: {err}")) - })?; - - // Re-apply dynamic subsystems before publishing the snapshot, so that - // runtime state (e.g. GLOBAL_STORAGE_CLASS) is refreshed on this peer. - for sub_system in [ - STORAGE_CLASS_SUB_SYS, - AUDIT_WEBHOOK_SUB_SYS, - AUDIT_MQTT_SUB_SYS, - SCANNER_SUB_SYS, - HEAL_SUB_SYS, - ] { - if let Err(err) = apply_dynamic_config_for_subsystem_for_context(context, &config, sub_system).await { - warn!("peer reload_runtime_config_snapshot: failed to apply {sub_system}: {err}"); - } - } - - publish_server_config(config); - Ok(()) + reload_runtime_config_snapshot_with( + async move { + read_admin_config_without_migrate(store).await.map_err(|err| { + warn!("peer reload_runtime_config_snapshot: failed to load server config: {err}"); + internal_error(format!("failed to load server config: {err}")) + }) + }, + |config| async move { + let prepared = prepare_server_config_for_context(context, &config, None).await.map_err(|_| { + warn!("peer reload_runtime_config_snapshot: failed to prepare server config"); + internal_error("failed to prepare server config") + })?; + Ok((config, prepared)) + }, + |config, prepared| { + prepared.publish_storage_class_for_context(context)?; + publish_server_config_for_context(context, config.clone()); + Ok(()) + }, + |config| async move { + for sub_system in FULL_CONFIG_WORKER_SUBSYSTEMS { + if let Err(err) = apply_dynamic_config_for_subsystem_for_context(context, &config, sub_system).await { + warn!( + event = EVENT_CONFIG_WORKER_RELOAD_FAILED, + component = LOG_COMPONENT_ADMIN, + subsystem = LOG_SUBSYSTEM_CONFIG, + config_subsystem = sub_system, + state = CONFIG_WORKER_RELOAD_FAILURE_STATE, + error = ?err, + "Peer runtime config snapshot was published but a subsystem worker reload failed" + ); + } + } + Ok(()) + }, + ) + .await } pub async fn reload_runtime_config_snapshot() -> S3Result<()> { @@ -408,15 +533,336 @@ pub async fn signal_config_snapshot_reload() { #[cfg(test)] mod tests { use super::*; + use crate::admin::runtime_sources::{IamInterface, KmsInterface, ServerConfigInterface, StorageClassInterface}; use crate::admin::storage_api::bucket::metadata::{BUCKET_LIFECYCLE_CONFIG, BUCKET_REPLICATION_CONFIG}; + use crate::admin::storage_api::config::save_admin_server_config; + use crate::storage_api::cluster::{Endpoint, EndpointServerPools, Endpoints, PoolEndpoints}; + use crate::storage_api::startup::storage::{init_local_disks_with_instance_ctx, new_instance_ctx}; use rustfs_config::notify::NOTIFY_WEBHOOK_SUB_SYS; use rustfs_config::oidc::{OIDC_CLIENT_ID, OIDC_CONFIG_URL, OIDC_SCOPES}; use rustfs_config::{HEAL_SUB_SYS, SCANNER_SUB_SYS}; use rustfs_config::{MQTT_BROKER, MQTT_QUEUE_DIR, MQTT_TOPIC, WEBHOOK_ENDPOINT, WEBHOOK_QUEUE_DIR}; + use rustfs_iam::{store::object::ObjectStore, sys::IamSys}; + use rustfs_kms::KmsServiceManager; + use std::collections::HashMap; + use std::path::Path; + use std::sync::{ + Arc, Mutex, + atomic::{AtomicUsize, Ordering}, + }; + use tempfile::TempDir; + use tokio_util::sync::CancellationToken; const LIFECYCLE_RELOAD_LABEL: &str = "lifecycle"; const REPLICATION_RELOAD_LABEL: &str = "replication"; + fn without_storage_class_env(f: impl FnOnce() -> R) -> R { + temp_env::with_vars_unset( + [ + storageclass::STANDARD_ENV, + storageclass::RRS_ENV, + storageclass::OPTIMIZE_ENV, + storageclass::INLINE_BLOCK_ENV, + ], + f, + ) + } + + struct TestIamInterface; + + impl IamInterface for TestIamInterface { + fn handle(&self) -> Arc> { + unreachable!("runtime config reload tests do not use IAM") + } + + fn is_ready(&self) -> bool { + false + } + } + + struct TestKmsInterface { + manager: Arc, + } + + impl KmsInterface for TestKmsInterface { + fn handle(&self) -> Arc { + self.manager.clone() + } + } + + struct TestServerConfigInterface { + snapshot: Arc>>, + set_calls: Arc, + } + + impl ServerConfigInterface for TestServerConfigInterface { + fn get(&self) -> Option { + self.snapshot.lock().expect("server config snapshot lock").clone() + } + + fn set(&self, config: ServerConfig) { + self.set_calls.fetch_add(1, Ordering::SeqCst); + *self.snapshot.lock().expect("server config snapshot lock") = Some(config); + } + } + + struct TestStorageClassInterface { + snapshot: Arc>>, + set_calls: Arc, + } + + impl StorageClassInterface for TestStorageClassInterface { + fn set(&self, config: storageclass::Config) { + self.set_calls.fetch_add(1, Ordering::SeqCst); + *self.snapshot.lock().expect("storage class snapshot lock") = Arc::new(config); + } + } + + struct RuntimeConfigReloadFixture { + _temp_dir: TempDir, + context: AppContext, + baseline_server: ServerConfig, + baseline_storage_class: Arc, + server_snapshot: Arc>>, + storage_class_snapshot: Arc>>, + server_set_calls: Arc, + storage_class_set_calls: Arc, + } + + impl RuntimeConfigReloadFixture { + fn assert_snapshots_unchanged(&self) { + assert_eq!(self.server_set_calls.load(Ordering::SeqCst), 0); + assert_eq!(self.storage_class_set_calls.load(Ordering::SeqCst), 0); + assert_eq!( + *self.server_snapshot.lock().expect("server config result lock"), + Some(self.baseline_server.clone()) + ); + let storage_class_snapshot = self.storage_class_snapshot.lock().expect("storage class result lock"); + assert!(Arc::ptr_eq(&*storage_class_snapshot, &self.baseline_storage_class)); + for storage_class in [storageclass::STANDARD, storageclass::RRS] { + assert_eq!( + storage_class_snapshot.parities_for_sc(storage_class), + self.baseline_storage_class.parities_for_sc(storage_class), + "{storage_class} snapshot changed" + ); + } + } + } + + fn storage_class_server_config(standard: &str) -> ServerConfig { + let mut config = ServerConfig::new(); + let mut kvs = KVS::new(); + kvs.insert(storageclass::CLASS_STANDARD.to_string(), standard.to_string()); + config + .0 + .insert(STORAGE_CLASS_SUB_SYS.to_string(), HashMap::from([(DEFAULT_DELIMITER.to_string(), kvs)])); + config + } + + async fn build_isolated_heterogeneous_store(temp_dir: &Path) -> Arc { + let mut pools = Vec::new(); + for (pool_index, drives_per_set) in [4, 2].into_iter().enumerate() { + let mut endpoints = Vec::new(); + for disk_index in 0..drives_per_set { + let disk_path = temp_dir.join(format!("pool{pool_index}/disk{disk_index}")); + tokio::fs::create_dir_all(&disk_path) + .await + .expect("create test disk directory"); + let mut endpoint = Endpoint::try_from(disk_path.to_str().expect("utf-8 test disk path")).expect("local endpoint"); + endpoint.set_pool_index(pool_index); + endpoint.set_set_index(0); + endpoint.set_disk_index(disk_index); + endpoints.push(endpoint); + } + pools.push(PoolEndpoints { + legacy: false, + set_count: 1, + drives_per_set, + endpoints: Endpoints::from(endpoints), + cmd_line: format!("runtime-config-pool-{pool_index}"), + platform: "test".to_string(), + }); + } + + let endpoint_pools = EndpointServerPools::from(pools); + let instance_ctx = new_instance_ctx(); + init_local_disks_with_instance_ctx(&instance_ctx, endpoint_pools.clone()) + .await + .expect("register isolated test disks"); + ECStore::new_with_instance_ctx( + "127.0.0.1:0".parse().expect("test address"), + endpoint_pools, + CancellationToken::new(), + instance_ctx, + ) + .await + .expect("build isolated heterogeneous store") + } + + async fn runtime_config_reload_fixture() -> RuntimeConfigReloadFixture { + let temp_dir = TempDir::new().expect("runtime config reload temp dir"); + let store = build_isolated_heterogeneous_store(temp_dir.path()).await; + assert_eq!(StorageAdminApi::set_drive_counts(store.as_ref()), vec![4, 2]); + + let rejected_server = storage_class_server_config("EC:2"); + save_admin_server_config(store.clone(), &rejected_server) + .await + .expect("persist rejected storage class config"); + + let baseline_server = storage_class_server_config("EC:1"); + let baseline_kvs = baseline_server + .get_value(STORAGE_CLASS_SUB_SYS, DEFAULT_DELIMITER) + .expect("baseline storage class KVS"); + let baseline_storage_class = Arc::new(prepare_storage_class_kvs(&baseline_kvs, &[4, 2]).expect("baseline storage class")); + let server_snapshot = Arc::new(Mutex::new(Some(baseline_server.clone()))); + let storage_class_snapshot = Arc::new(Mutex::new(baseline_storage_class.clone())); + let server_set_calls = Arc::new(AtomicUsize::new(0)); + let storage_class_set_calls = Arc::new(AtomicUsize::new(0)); + let context = AppContext::new( + store, + Arc::new(TestIamInterface), + Arc::new(TestKmsInterface { + manager: Arc::new(KmsServiceManager::new()), + }), + ) + .with_test_runtime_config_interfaces( + Arc::new(TestServerConfigInterface { + snapshot: server_snapshot.clone(), + set_calls: server_set_calls.clone(), + }), + Arc::new(TestStorageClassInterface { + snapshot: storage_class_snapshot.clone(), + set_calls: storage_class_set_calls.clone(), + }), + ); + + RuntimeConfigReloadFixture { + _temp_dir: temp_dir, + context, + baseline_server, + baseline_storage_class, + server_snapshot, + storage_class_snapshot, + server_set_calls, + storage_class_set_calls, + } + } + + #[tokio::test] + #[serial_test::serial(storage_class_env)] + async fn peer_dynamic_reload_rejects_later_pool_without_publishing() { + temp_env::async_with_vars( + [ + (storageclass::STANDARD_ENV, None::<&str>), + (storageclass::RRS_ENV, None::<&str>), + (storageclass::OPTIMIZE_ENV, None::<&str>), + (storageclass::INLINE_BLOCK_ENV, None::<&str>), + ], + async { + let fixture = runtime_config_reload_fixture().await; + let err = reload_dynamic_config_runtime_state_for_context(Some(&fixture.context), STORAGE_CLASS_SUB_SYS) + .await + .expect_err("later pool parity must reject peer dynamic reload"); + + assert!( + err.message() + .is_some_and(|message| message.contains("storage class validation failed for pool 1")), + "unexpected dynamic reload error: {err:?}" + ); + fixture.assert_snapshots_unchanged(); + }, + ) + .await; + } + + #[tokio::test] + #[serial_test::serial(storage_class_env)] + async fn peer_dynamic_reload_publishes_valid_per_pool_storage_snapshot() { + temp_env::async_with_vars( + [ + (storageclass::STANDARD_ENV, None::<&str>), + (storageclass::RRS_ENV, None::<&str>), + (storageclass::OPTIMIZE_ENV, None::<&str>), + (storageclass::INLINE_BLOCK_ENV, None::<&str>), + ], + async { + let fixture = runtime_config_reload_fixture().await; + let candidate = storage_class_server_config(""); + save_admin_server_config(fixture.context.object_store(), &candidate) + .await + .expect("persist valid automatic storage class config"); + + reload_dynamic_config_runtime_state_for_context(Some(&fixture.context), STORAGE_CLASS_SUB_SYS) + .await + .expect("valid peer dynamic reload must publish its prepared storage snapshot"); + + assert_eq!(fixture.server_set_calls.load(Ordering::SeqCst), 0); + assert_eq!(fixture.storage_class_set_calls.load(Ordering::SeqCst), 1); + assert_eq!( + *fixture.server_snapshot.lock().expect("server config result lock"), + Some(fixture.baseline_server.clone()), + "dynamic reload must not replace the server-config snapshot" + ); + let storage_class = fixture.storage_class_snapshot.lock().expect("storage class result lock"); + assert_eq!(storage_class.parities_for_sc(storageclass::STANDARD), Some(vec![2, 1])); + assert_eq!(storage_class.parities_for_sc(storageclass::RRS), Some(vec![1, 1])); + }, + ) + .await; + } + + #[tokio::test] + #[serial_test::serial(storage_class_env)] + async fn peer_full_reload_rejects_later_pool_without_publishing() { + temp_env::async_with_vars( + [ + (storageclass::STANDARD_ENV, None::<&str>), + (storageclass::RRS_ENV, None::<&str>), + (storageclass::OPTIMIZE_ENV, None::<&str>), + (storageclass::INLINE_BLOCK_ENV, None::<&str>), + ], + async { + let fixture = runtime_config_reload_fixture().await; + let err = reload_runtime_config_snapshot_for_context(Some(&fixture.context)) + .await + .expect_err("later pool parity must reject peer full reload"); + + assert_eq!(err.message(), Some("failed to prepare server config")); + fixture.assert_snapshots_unchanged(); + }, + ) + .await; + } + + #[tokio::test] + async fn full_reload_publishes_snapshots_before_best_effort_worker_failure() { + let events = Arc::new(Mutex::new(Vec::new())); + let publish_events = events.clone(); + let worker_events = events.clone(); + + reload_runtime_config_snapshot_with( + async { Ok(ServerConfig::new()) }, + |config| async { Ok((config, PreparedRuntimeConfig::default())) }, + move |_config, _prepared| { + publish_events.lock().expect("reload event lock").push("publish"); + Ok(()) + }, + move |_config| async move { + let mut events = worker_events.lock().expect("reload event lock"); + events.push("worker-1-applied"); + events.push("worker-2-failed"); + Err(internal_error("injected worker reload failure")) + }, + ) + .await + .expect("worker failure must not roll back validated storage/server snapshots"); + + assert_eq!( + *events.lock().expect("reload result lock"), + ["publish", "worker-1-applied", "worker-2-failed"] + ); + } + #[test] fn dynamic_config_subsystems_match_runtime_apply_support() { assert!(is_dynamic_config_subsystem(AUDIT_WEBHOOK_SUB_SYS)); @@ -428,6 +874,11 @@ mod tests { assert!(!is_dynamic_config_subsystem("notify_webhook")); } + #[test] + fn full_config_worker_reload_uses_one_representative_per_worker_family() { + assert_eq!(FULL_CONFIG_WORKER_SUBSYSTEMS, [AUDIT_WEBHOOK_SUB_SYS, SCANNER_SUB_SYS]); + } + #[test] fn background_config_reload_plan_never_mutates_workers() { for sub_system in [ @@ -454,12 +905,112 @@ mod tests { } #[test] + #[serial_test::serial(storage_class_env)] fn validate_storage_class_kvs_rejects_invalid_parity() { let mut kvs = KVS::new(); kvs.insert("standard".to_string(), "EC:5".to_string()); - let err = validate_storage_class_kvs(&kvs, &[4]).expect_err("invalid parity should fail"); - assert_eq!(err.code(), &S3ErrorCode::InvalidRequest); + without_storage_class_env(|| { + let err = validate_storage_class_kvs(&kvs, &[4]).expect_err("invalid parity should fail"); + assert_eq!(err.code(), &S3ErrorCode::InvalidRequest); + }); + } + + #[test] + #[serial_test::serial(storage_class_env)] + fn prepare_storage_class_kvs_keeps_all_pool_geometry() { + without_storage_class_env(|| { + let prepared = prepare_storage_class_kvs(&KVS::new(), &[4, 2]).expect("prepare heterogeneous pools"); + + assert_eq!(prepared.parities_for_sc(storageclass::STANDARD), Some(vec![2, 1])); + }); + } + + #[test] + #[serial_test::serial(storage_class_env)] + fn prepare_storage_class_kvs_rejects_explicit_parity_for_later_pool() { + let mut kvs = KVS::new(); + kvs.insert(storageclass::CLASS_STANDARD.to_string(), "EC:2".to_string()); + + without_storage_class_env(|| { + let err = prepare_storage_class_kvs(&kvs, &[4, 2]).expect_err("second pool must reject parity equal to its width"); + + assert_eq!(err.code(), &S3ErrorCode::InvalidRequest); + assert!(err.message().unwrap_or_default().contains("pool 1 (2 drives)")); + }); + } + + #[test] + #[serial_test::serial(storage_class_env)] + fn prepare_storage_class_kvs_rejects_empty_topology() { + without_storage_class_env(|| { + let err = prepare_storage_class_kvs(&KVS::new(), &[]).expect_err("empty topology must fail closed"); + + assert_eq!(err.code(), &S3ErrorCode::InvalidRequest); + assert!(err.message().unwrap_or_default().contains("at least one pool")); + }); + } + + #[test] + fn missing_prepared_storage_class_fails_without_publishing() { + let publish_calls = std::cell::Cell::new(0); + + let err = PreparedRuntimeConfig::default() + .publish_storage_class_for_context(None) + .expect_err("production publisher must reject a missing candidate"); + let injected_err = PreparedRuntimeConfig::default() + .publish_storage_class_for_context_with(None, |_| publish_calls.set(publish_calls.get() + 1)) + .expect_err("missing candidate must fail closed"); + + assert_eq!(err.code(), &S3ErrorCode::InternalError); + assert_eq!(injected_err.code(), &S3ErrorCode::InternalError); + assert_eq!(publish_calls.get(), 0); + } + + #[test] + #[serial_test::serial(storage_class_env)] + fn prepared_storage_class_publishes_exact_candidate_without_reparse() { + let candidate = without_storage_class_env(|| prepare_storage_class_kvs(&KVS::new(), &[4, 2]).expect("prepare candidate")); + let mut published = None; + + temp_env::with_vars([(storageclass::STANDARD_ENV, Some("EC:2"))], || { + PreparedRuntimeConfig { + storage_class: Some(candidate), + } + .publish_storage_class_with(|storage_class| published = Some(storage_class)); + }); + + assert_eq!( + published.and_then(|storage_class| storage_class.parities_for_sc(storageclass::STANDARD)), + Some(vec![2, 1]) + ); + } + + #[test] + #[serial_test::serial(storage_class_env)] + fn rejected_storage_class_candidate_does_not_publish() { + without_storage_class_env(|| { + let mut published = prepare_storage_class_kvs(&KVS::new(), &[4, 4]).expect("prepare baseline"); + let mut invalid_kvs = KVS::new(); + invalid_kvs.insert(storageclass::CLASS_STANDARD.to_string(), "EC:2".to_string()); + + let rejected = match prepare_storage_class_kvs(&invalid_kvs, &[4, 2]) { + Ok(storage_class) => { + PreparedRuntimeConfig { + storage_class: Some(storage_class), + } + .publish_storage_class_with(|storage_class| published = storage_class); + false + } + Err(err) => { + assert_eq!(err.code(), &S3ErrorCode::InvalidRequest); + true + } + }; + + assert!(rejected); + assert_eq!(published.parities_for_sc(storageclass::STANDARD), Some(vec![2, 2])); + }); } #[test] diff --git a/rustfs/src/admin/storage_api.rs b/rustfs/src/admin/storage_api.rs index 0027cdbd5..490ee1793 100644 --- a/rustfs/src/admin/storage_api.rs +++ b/rustfs/src/admin/storage_api.rs @@ -377,15 +377,24 @@ pub(crate) mod versioning_sys { } pub(crate) mod storageclass { + #[cfg(test)] + pub(crate) const CLASS_STANDARD: &str = super::ecstore_config::storageclass::CLASS_STANDARD; pub(crate) const INLINE_BLOCK_ENV: &str = super::ecstore_config::storageclass::INLINE_BLOCK_ENV; pub(crate) const OPTIMIZE_ENV: &str = super::ecstore_config::storageclass::OPTIMIZE_ENV; + #[cfg(test)] + pub(crate) const RRS: &str = super::ecstore_config::storageclass::RRS; pub(crate) const RRS_ENV: &str = super::ecstore_config::storageclass::RRS_ENV; + #[cfg(test)] + pub(crate) const STANDARD: &str = super::ecstore_config::storageclass::STANDARD; pub(crate) const STANDARD_ENV: &str = super::ecstore_config::storageclass::STANDARD_ENV; pub(crate) type Config = super::ecstore_config::storageclass::Config; - pub(crate) fn lookup_config(kvs: &rustfs_config::server_config::KVS, set_drive_count: usize) -> super::Result { - super::ecstore_config::storageclass::lookup_config(kvs, set_drive_count) + pub(crate) fn lookup_config_for_pools( + kvs: &rustfs_config::server_config::KVS, + set_drive_counts: &[usize], + ) -> super::Result { + super::ecstore_config::storageclass::lookup_config_for_pools(kvs, set_drive_counts) } } diff --git a/rustfs/src/app/context/global.rs b/rustfs/src/app/context/global.rs index 309de0b77..3d0c87992 100644 --- a/rustfs/src/app/context/global.rs +++ b/rustfs/src/app/context/global.rs @@ -344,6 +344,16 @@ impl AppContext { object_data_cache: ObjectDataCacheAdapter::disabled_arc(), } } + + pub(crate) fn with_test_runtime_config_interfaces( + mut self, + server_config: Arc, + storage_class: Arc, + ) -> Self { + self.server_config = server_config; + self.storage_class = storage_class; + self + } } static APP_CONTEXT_SINGLETON: OnceLock> = OnceLock::new(); diff --git a/rustfs/src/runtime_sources.rs b/rustfs/src/runtime_sources.rs index 6e3a6cd8f..b1fc8b09b 100644 --- a/rustfs/src/runtime_sources.rs +++ b/rustfs/src/runtime_sources.rs @@ -58,6 +58,9 @@ pub(crate) fn set_test_outbound_tls_generation(generation: u64) { #[cfg(test)] pub(crate) use context::install_test_app_context; +#[cfg(test)] +pub(crate) use context::{IamInterface, KmsInterface, ServerConfigInterface, StorageClassInterface}; + pub(crate) fn current_app_context() -> Option> { context::get_global_app_context() } diff --git a/rustfs/src/server/readiness.rs b/rustfs/src/server/readiness.rs index 585b3b120..a4064081d 100644 --- a/rustfs/src/server/readiness.rs +++ b/rustfs/src/server/readiness.rs @@ -489,61 +489,92 @@ fn disk_is_online_for_readiness(disk: &Disk) -> bool { state_is_acceptable } -fn pool_write_quorum(info: &StorageInfo, pool_idx: usize, set_drive_count: usize) -> usize { +fn pool_erasure_layout(info: &StorageInfo, pool_idx: usize, set_drive_count: usize) -> Option<(usize, usize)> { if set_drive_count == 0 { - return 1; + return None; } - let data_drives = info - .backend - .standard_sc_data - .get(pool_idx) - .copied() - .filter(|count| *count > 0) - .unwrap_or_else(|| (set_drive_count / 2).max(1)); + if !info.backend.drives_per_set.is_empty() { + if info.backend.drives_per_set.get(pool_idx).copied() != Some(set_drive_count) { + return None; + } + if (!info.backend.standard_sc_data.is_empty() && info.backend.standard_sc_data.len() != info.backend.drives_per_set.len()) + || (!info.backend.standard_sc_parities.is_empty() + && info.backend.standard_sc_parities.len() != info.backend.drives_per_set.len()) + { + return None; + } + } - let parity_drives = if let Some(drives_per_set) = info.backend.drives_per_set.get(pool_idx).copied() { - drives_per_set.saturating_sub(data_drives) - } else if let Some(parity) = info.backend.standard_sc_parities.get(pool_idx).copied() { - parity - } else if let Some(parity) = info.backend.standard_sc_parity { - parity - } else { - set_drive_count.saturating_sub(data_drives) + let has_data = !info.backend.standard_sc_data.is_empty(); + let has_parities = !info.backend.standard_sc_parities.is_empty(); + let (data_drives, parity_drives) = match (has_data, has_parities) { + (true, true) => ( + info.backend.standard_sc_data.get(pool_idx).copied()?, + info.backend.standard_sc_parities.get(pool_idx).copied()?, + ), + (true, false) => { + let data = info.backend.standard_sc_data.get(pool_idx).copied()?; + // The per-pool data vector is exact. A legacy scalar may be stale + // for a heterogeneous topology, so derive the matching parity from + // the pool drive count instead of combining two representations. + let parity = set_drive_count.checked_sub(data)?; + (data, parity) + } + (false, true) => { + let parity = info.backend.standard_sc_parities.get(pool_idx).copied()?; + (set_drive_count.checked_sub(parity)?, parity) + } + (false, false) => { + let parity = info.backend.standard_sc_parity?; + (set_drive_count.checked_sub(parity)?, parity) + } }; - let mut write_quorum = data_drives; - if data_drives == parity_drives { - write_quorum += 1; + if data_drives == 0 || parity_drives > data_drives || data_drives.checked_add(parity_drives) != Some(set_drive_count) { + return None; } - write_quorum.max(1) + + Some((data_drives, parity_drives)) } -fn pool_read_quorum(info: &StorageInfo, pool_idx: usize, set_drive_count: usize) -> usize { - if set_drive_count == 0 { - return 1; +fn pool_write_quorum(info: &StorageInfo, pool_idx: usize, set_drive_count: usize) -> Option { + let (data_drives, parity_drives) = pool_erasure_layout(info, pool_idx, set_drive_count)?; + if data_drives == parity_drives { + data_drives.checked_add(1) + } else { + Some(data_drives) + } +} + +fn pool_read_quorum(info: &StorageInfo, pool_idx: usize, set_drive_count: usize) -> Option { + pool_erasure_layout(info, pool_idx, set_drive_count).map(|(data_drives, _)| data_drives) +} + +fn configured_readiness_topology(info: &StorageInfo) -> Option<(&[usize], &[usize])> { + if info.backend.total_sets.is_empty() + || info.backend.total_sets.len() != info.backend.drives_per_set.len() + || info.backend.total_sets.contains(&0) + || info.backend.drives_per_set.contains(&0) + { + return None; } - info.backend - .standard_sc_data - .get(pool_idx) - .copied() - .filter(|count| *count > 0) - .unwrap_or_else(|| (set_drive_count / 2).max(1)) - .max(1) + Some((&info.backend.total_sets, &info.backend.drives_per_set)) } fn storage_ready_from_runtime_state_with_quorum(info: &StorageInfo, quorum_for_set: F) -> bool where - F: Fn(&StorageInfo, usize, usize) -> usize, + F: Fn(&StorageInfo, usize, usize) -> Option, { if info.disks.is_empty() { return false; } + let configured_topology_present = !info.backend.total_sets.is_empty() || !info.backend.drives_per_set.is_empty(); let mut total_online = 0usize; let mut set_online_counts: HashMap<(usize, usize), usize> = HashMap::new(); - let mut set_drive_counts: HashMap<(usize, usize), usize> = HashMap::new(); + let mut observed_set_drive_counts: HashMap<(usize, usize), usize> = HashMap::new(); let mut seen_disks: HashSet<(String, String, i32, i32, i32)> = HashSet::new(); for disk in &info.disks { @@ -565,7 +596,9 @@ where let pool_idx = disk.pool_index as usize; let set_idx = disk.set_index as usize; let key = (pool_idx, set_idx); - *set_drive_counts.entry(key).or_default() += 1; + if !configured_topology_present { + *observed_set_drive_counts.entry(key).or_default() += 1; + } if disk_is_online_for_readiness(disk) { total_online += 1; @@ -573,15 +606,43 @@ where } } - if total_online == 0 || set_drive_counts.is_empty() { + if total_online == 0 { return false; } - set_drive_counts.into_iter().all(|((pool_idx, set_idx), set_drive_count)| { - let online = set_online_counts.get(&(pool_idx, set_idx)).copied().unwrap_or_default(); - let quorum = quorum_for_set(info, pool_idx, set_drive_count); - online >= quorum - }) + if configured_topology_present { + let Some((total_sets, drives_per_set)) = configured_readiness_topology(info) else { + return false; + }; + + return total_sets + .iter() + .zip(drives_per_set) + .enumerate() + .all(|(pool_idx, (&set_count, &configured_drive_count))| { + (0..set_count).all(|set_idx| { + let online = set_online_counts.get(&(pool_idx, set_idx)).copied().unwrap_or_default(); + quorum_for_set(info, pool_idx, configured_drive_count).is_some_and(|quorum| online >= quorum) + }) + }); + } + + if observed_set_drive_counts.is_empty() { + return false; + } + + observed_set_drive_counts + .into_iter() + .all(|((pool_idx, set_idx), observed_drive_count)| { + let online = set_online_counts.get(&(pool_idx, set_idx)).copied().unwrap_or_default(); + let layout_drive_count = info + .backend + .drives_per_set + .get(pool_idx) + .copied() + .unwrap_or(observed_drive_count); + quorum_for_set(info, pool_idx, layout_drive_count).is_some_and(|quorum| online >= quorum) + }) } fn storage_ready_from_runtime_state(info: &StorageInfo) -> bool { @@ -954,6 +1015,21 @@ mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; use temp_env::{async_with_vars, with_var}; + fn online_readiness_disks(set_idx: i32, count: i32) -> Vec { + (0..count) + .map(|disk_index| Disk { + endpoint: format!("node-{set_idx}-{disk_index}:9000"), + drive_path: format!("/set{set_idx}/data{disk_index}"), + pool_index: 0, + set_index: set_idx, + disk_index, + state: "ok".to_string(), + runtime_state: Some("online".to_string()), + ..Default::default() + }) + .collect() + } + #[test] fn startup_runtime_readiness_wait_constants_are_ordered() { assert!(STARTUP_RUNTIME_READINESS_MAX_WAIT > STARTUP_RUNTIME_READINESS_POLL_INTERVAL); @@ -1269,11 +1345,215 @@ mod tests { assert_eq!(readiness_pending_dependency(rustfs_common::SystemStage::IamReady), "startup_finalization"); } + #[test] + fn pool_quorum_uses_exact_heterogeneous_backend_layout() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_data: vec![2, 1], + standard_sc_parities: vec![2, 1], + // Deliberately stale: exact per-pool vectors must take precedence. + standard_sc_parity: Some(2), + drives_per_set: vec![4, 2], + ..Default::default() + }, + disks: Vec::new(), + }; + + assert_eq!(pool_erasure_layout(&info, 0, 4), Some((2, 2))); + assert_eq!(pool_erasure_layout(&info, 1, 2), Some((1, 1))); + assert_eq!(pool_write_quorum(&info, 0, 4), Some(3)); + assert_eq!(pool_write_quorum(&info, 1, 2), Some(2)); + assert_eq!(pool_read_quorum(&info, 0, 4), Some(2)); + assert_eq!(pool_read_quorum(&info, 1, 2), Some(1)); + } + + #[test] + fn pool_write_quorum_does_not_fall_back_to_half_when_exact_data_exists() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_data: vec![6], + standard_sc_parities: vec![2], + drives_per_set: vec![8], + ..Default::default() + }, + disks: Vec::new(), + }; + + assert_eq!(pool_write_quorum(&info, 0, 8), Some(6)); + } + + #[test] + fn exact_data_vector_ignores_stale_scalar_for_runtime_readiness() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_data: vec![6], + // Deliberately stale: the exact data vector and topology imply + // parity 2, so this legacy scalar must not make readiness fail. + standard_sc_parity: Some(4), + total_sets: vec![1], + drives_per_set: vec![8], + ..Default::default() + }, + disks: online_readiness_disks(0, 8), + }; + + assert_eq!(pool_erasure_layout(&info, 0, 8), Some((6, 2))); + assert!(storage_read_ready_from_runtime_state(&info)); + assert!(storage_ready_from_runtime_state(&info)); + } + + #[test] + fn exact_data_vector_with_invalid_geometry_fails_closed() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_data: vec![9], + standard_sc_parity: Some(1), + total_sets: vec![1], + drives_per_set: vec![8], + ..Default::default() + }, + disks: online_readiness_disks(0, 8), + }; + + assert_eq!(pool_erasure_layout(&info, 0, 8), None); + assert!(!storage_read_ready_from_runtime_state(&info)); + assert!(!storage_ready_from_runtime_state(&info)); + } + + #[test] + fn pool_quorum_accepts_valid_legacy_scalar_layout() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_parity: Some(1), + drives_per_set: vec![4], + ..Default::default() + }, + disks: Vec::new(), + }; + + assert_eq!(pool_erasure_layout(&info, 0, 4), Some((3, 1))); + assert_eq!(pool_write_quorum(&info, 0, 4), Some(3)); + assert_eq!(pool_read_quorum(&info, 0, 4), Some(3)); + } + + #[test] + fn legacy_payload_without_topology_uses_observed_sets_and_scalar_layout() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_parity: Some(1), + ..Default::default() + }, + disks: online_readiness_disks(0, 3), + }; + + assert_eq!(pool_erasure_layout(&info, 0, 3), Some((2, 1))); + assert!(storage_read_ready_from_runtime_state(&info)); + assert!(storage_ready_from_runtime_state(&info)); + } + + #[test] + fn configured_topology_uses_configured_drive_count_when_rows_are_missing() { + let backend = BackendInfo { + standard_sc_data: vec![2], + standard_sc_parities: vec![2], + total_sets: vec![1], + drives_per_set: vec![4], + ..Default::default() + }; + let three_online = StorageInfo { + backend: backend.clone(), + disks: online_readiness_disks(0, 3), + }; + let two_online = StorageInfo { + backend, + disks: online_readiness_disks(0, 2), + }; + + assert!(storage_read_ready_from_runtime_state(&three_online)); + assert!(storage_ready_from_runtime_state(&three_online)); + assert!(storage_read_ready_from_runtime_state(&two_online)); + assert!(!storage_ready_from_runtime_state(&two_online)); + } + + #[test] + fn configured_topology_requires_sets_with_no_disk_rows() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_data: vec![2], + standard_sc_parities: vec![2], + total_sets: vec![2], + drives_per_set: vec![4], + ..Default::default() + }, + disks: online_readiness_disks(0, 3), + }; + + assert!(!storage_read_ready_from_runtime_state(&info)); + assert!(!storage_ready_from_runtime_state(&info)); + } + + #[test] + fn configured_topology_fails_closed_with_only_total_sets() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_parity: Some(1), + total_sets: vec![1], + ..Default::default() + }, + disks: online_readiness_disks(0, 3), + }; + + assert!(!storage_read_ready_from_runtime_state(&info)); + assert!(!storage_ready_from_runtime_state(&info)); + } + + #[test] + fn configured_topology_fails_closed_with_only_drives_per_set() { + let info = StorageInfo { + backend: BackendInfo { + standard_sc_parity: Some(1), + drives_per_set: vec![3], + ..Default::default() + }, + disks: online_readiness_disks(0, 3), + }; + + assert!(!storage_read_ready_from_runtime_state(&info)); + assert!(!storage_ready_from_runtime_state(&info)); + } + + #[test] + fn storage_readiness_fails_closed_when_backend_layout_is_empty() { + let disks = (0..4) + .map(|disk_index| Disk { + endpoint: format!("127.0.0.1:900{disk_index}"), + drive_path: format!("/data{disk_index}"), + pool_index: 0, + set_index: 0, + disk_index, + state: "ok".to_string(), + runtime_state: Some("online".to_string()), + ..Default::default() + }) + .collect(); + let info = StorageInfo { + backend: BackendInfo { + drives_per_set: vec![4], + ..Default::default() + }, + disks, + }; + + assert!(!storage_ready_from_runtime_state(&info)); + assert!(!storage_read_ready_from_runtime_state(&info)); + } + #[test] fn storage_ready_from_runtime_state_returns_false_when_all_disks_faulty() { let info = StorageInfo { backend: BackendInfo { standard_sc_data: vec![1], + total_sets: vec![1], drives_per_set: vec![1], ..Default::default() }, @@ -1294,6 +1574,7 @@ mod tests { let info = StorageInfo { backend: BackendInfo { standard_sc_data: vec![1], + total_sets: vec![1], drives_per_set: vec![1], ..Default::default() }, @@ -1326,6 +1607,7 @@ mod tests { let info = StorageInfo { backend: BackendInfo { standard_sc_data: vec![2], + total_sets: vec![1], drives_per_set: vec![4], ..Default::default() }, @@ -1350,8 +1632,9 @@ mod tests { }; let info = StorageInfo { backend: BackendInfo { - standard_sc_data: vec![2], - drives_per_set: vec![4], + standard_sc_data: vec![1], + total_sets: vec![1], + drives_per_set: vec![2], ..Default::default() }, disks: vec![duplicate_disk.clone(), duplicate_disk], @@ -1375,6 +1658,7 @@ mod tests { let info = StorageInfo { backend: BackendInfo { standard_sc_data: vec![1], + total_sets: vec![2], drives_per_set: vec![2], ..Default::default() }, @@ -1390,7 +1674,17 @@ mod tests { ..Default::default() }, Disk { - endpoint: "127.0.0.1:9000".to_string(), + endpoint: "127.0.0.1:9001".to_string(), + drive_path: "/set0d1".to_string(), + pool_index: 0, + set_index: 0, + disk_index: 1, + state: "ok".to_string(), + runtime_state: Some("online".to_string()), + ..Default::default() + }, + Disk { + endpoint: "127.0.0.1:9002".to_string(), drive_path: "/set1d0".to_string(), pool_index: 0, set_index: 1, diff --git a/scripts/check_architecture_migration_rules.sh b/scripts/check_architecture_migration_rules.sh index ed9fa9e71..b9d676e62 100755 --- a/scripts/check_architecture_migration_rules.sh +++ b/scripts/check_architecture_migration_rules.sh @@ -141,6 +141,10 @@ ECSTORE_STORAGE_API_ROOT_CONSUMER_HITS_FILE="${TMP_DIR}/ecstore_storage_api_root ECSTORE_TEST_DIRECT_API_HITS_FILE="${TMP_DIR}/ecstore_test_direct_api_hits.txt" ECSTORE_BENCH_DIRECT_API_HITS_FILE="${TMP_DIR}/ecstore_bench_direct_api_hits.txt" ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE="${TMP_DIR}/erasure_panicking_constructor_hits.txt" +ERASURE_PANICKING_CONSTRUCTOR_ALL_HITS_FILE="${TMP_DIR}/erasure_panicking_constructor_all_hits.txt" +ERASURE_GUARD_FIXTURE_HITS_FILE="${TMP_DIR}/erasure_guard_fixture_hits.txt" +ERASURE_GUARD_FIXTURE_EXPECTED_FILE="${TMP_DIR}/erasure_guard_fixture_expected.txt" +ERASURE_GUARD_FIXTURE="scripts/fixtures/architecture_migration_rules/cfg_test_field.rs" LOCAL_STORAGE_API_RAW_CONTRACT_PATH_HITS_FILE="${TMP_DIR}/local_storage_api_raw_contract_path_hits.txt" STORE_API_DELETE_DTO_REEXPORTS_FILE="${TMP_DIR}/store_api_delete_dto_reexports.txt" STORE_API_DELETE_DTO_INTERNAL_HITS_FILE="${TMP_DIR}/store_api_delete_dto_internal_hits.txt" @@ -1154,9 +1158,12 @@ fi ( cd "$ROOT_DIR" - find rustfs/src crates -type f -name '*.rs' -print0 | + { + find rustfs/src crates -type f -name '*.rs' -print0 + printf '%s\0' "$ERASURE_GUARD_FIXTURE" + } | xargs -0 perl -ne ' - next unless $ARGV =~ m{^(?:rustfs/src/|crates/[^/]+/src/)}; + next unless $ARGV =~ m{^(?:rustfs/src/|crates/[^/]+/src/|scripts/fixtures/architecture_migration_rules/cfg_test_field\.rs$)}; if (!defined $current_file || $ARGV ne $current_file) { $current_file = $ARGV; $line = 0; @@ -1174,9 +1181,9 @@ fi my $item = $2; if ($item =~ /^\s*(?:\/\/.*)?$/) { $pending_test_item = 1; - } elsif ($item !~ /;\s*(?:\/\/.*)?$/ && $item =~ /\{/ && $item !~ /}\s*(?:\/\/.*)?$/) { + } elsif ($item !~ /[,;]\s*(?:\/\/.*)?$/ && $item =~ /\{/ && $item !~ /}\s*(?:\/\/.*)?$/) { $in_test_item = 1; - } elsif ($item !~ /;\s*(?:\/\/.*)?$/ && $item !~ /\{/) { + } elsif ($item !~ /[,;]\s*(?:\/\/.*)?$/ && $item !~ /\{/) { $pending_test_item = 1; } next; @@ -1184,6 +1191,8 @@ fi if ($pending_test_item) { if (/;\s*(?:\/\/.*)?$/) { $pending_test_item = 0; + } elsif (/^(\s*).*,\s*(?:\/\/.*)?$/ && $1 eq $test_indent) { + $pending_test_item = 0; } elsif (/\{/) { $pending_test_item = 0; $in_test_item = 1 unless /}\s*(?:\/\/.*)?$/; @@ -1193,8 +1202,22 @@ fi if (/^\s*(?!\/\/).*\bErasure::new(?:_with_options)?\s*\(/) { print "$ARGV:$line:$_"; } - ' || true -) >"$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE" + ' || true +) >"$ERASURE_PANICKING_CONSTRUCTOR_ALL_HITS_FILE" + +grep -Fv "$ERASURE_GUARD_FIXTURE:" "$ERASURE_PANICKING_CONSTRUCTOR_ALL_HITS_FILE" \ + >"$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE" || true + +grep -F "$ERASURE_GUARD_FIXTURE:" "$ERASURE_PANICKING_CONSTRUCTOR_ALL_HITS_FILE" \ + >"$ERASURE_GUARD_FIXTURE_HITS_FILE" || true +printf '%s\n' \ + "$ERASURE_GUARD_FIXTURE:19: let _ = Erasure::new(2, 1, 64);" \ + "$ERASURE_GUARD_FIXTURE:23: let _ = Erasure::new_with_options(2, 1, 64, false);" \ + >"$ERASURE_GUARD_FIXTURE_EXPECTED_FILE" + +if ! cmp -s "$ERASURE_GUARD_FIXTURE_EXPECTED_FILE" "$ERASURE_GUARD_FIXTURE_HITS_FILE"; then + report_failure "Erasure constructor guard must resume production scanning after a cfg(test) field" +fi if [[ -s "$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE" ]]; then report_failure "production code must use fallible Erasure constructors: $(paste -sd '; ' "$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE")" diff --git a/scripts/fixtures/architecture_migration_rules/cfg_test_field.rs b/scripts/fixtures/architecture_migration_rules/cfg_test_field.rs new file mode 100644 index 000000000..bda78ce71 --- /dev/null +++ b/scripts/fixtures/architecture_migration_rules/cfg_test_field.rs @@ -0,0 +1,24 @@ +struct GuardFixture { + #[cfg(test)] + test_only: (), + #[cfg(test)] + test_generic: Option< + usize, + >, +} + +#[cfg(test)] +fn test_only_constructor( + data_shards: usize, + parity_shards: usize, +) { + let _ = Erasure::new_with_options(data_shards, parity_shards, 64, false); +} + +fn production_after_test_fields() { + let _ = Erasure::new(2, 1, 64); +} + +fn second_production_path() { + let _ = Erasure::new_with_options(2, 1, 64, false); +}