mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-29 16:37:07 +00:00
fix(storage): resolve erasure parity per pool (#4977)
* fix(filemeta): add state-aware file info validation
* fix(filemeta): validate shard arithmetic and delete paths
* fix(ecstore): add fallible erasure construction
* fix(ecstore): resolve storage parity per pool
* fix(storage): report heterogeneous erasure layouts
* fix(admin): publish prepared storage config atomically
* fix(storage): harden per-pool parity boundaries
* fix(storage): address pre-PR validation findings
* test(ci): fix strict-topology validation fixtures
* fix(heal): preserve delete markers during repair
* refactor(filemeta): drop unused ValidatedFileInfo witness
ValidatedFileInfo wrapped an unread `_file_info` reference alongside an `Option<ValidatedErasureLayout>`, but only the layout was ever consumed. Return the layout directly from `FileInfo::validate` so the sole production consumer (`LocalDisk::check_parts`) and the two unit tests read it without the extra witness type and lifetime.
No behavior change.
* fix(filemeta): keep compressed and MinIO-migrated tiered objects readable
The new decode-path validation rejected several legitimate on-disk shapes that older RustFS and MinIO-migrated data carry, turning readable objects into FileCorrupt:
- Compressed objects written with an unknown upload size persist a negative per-part actual_size (the documented "unknown size" sentinel that ObjectInfo::get_actual_size already tolerates). validate_collection_contents rejected it via usize::try_from; now a negative actual_size skips shard validation and only real, non-negative sizes are checked.
- MinIO-migrated objects transitioned to a versioned remote tier store the tier version id as a UUID string, not 16 raw bytes. MetaObject::into_fileinfo returned FileCorrupt (main tolerated it as None), making all versions of the object unreadable; MetaDeleteMarker free-version records took a Some(nil) sentinel path with the same effect, which also breaks free-version expiry (remote-tier leak). Both now decode through a shared transitioned_version_id_from_meta_sys helper: 16 raw bytes or a UUID string are accepted, anything else is tolerated as None instead of failing the read.
Regression tests updated to assert the readable/compat behavior, with new tests covering MinIO string-form recovery.
* fix(scanner): build the delete-marker test fixture without erasure geometry
get_size_counts_delete_markers_separately_from_versions built its delete marker with `FileInfo::new(object, 1, 1)`, which attaches erasure geometry (data=1/parity=1/distribution). This PR classifies versions by shape via `is_storage_delete_marker()` (no geometry) rather than the raw `deleted` flag, so a geometry-bearing "delete marker" is correctly serialized as a purge-pending payload Object and counted as a version — CI saw summary.versions=3, expected 2.
Real delete markers carry no erasure geometry (delete paths build them as `FileInfo { deleted: true, ..Default::default() }`), so construct the fixture the same way. It then classifies as a storage delete marker and the counts (versions=2, delete_markers=1) hold. This keeps the PR's more-correct classification, which prevents a purge-pending object's geometry from being dropped when serialized as a bare delete marker.
* docs(changelog): note per-pool parity fix and storage-class startup upgrade caveat
Records the #4801 per-pool erasure parity fix under Fixed, and documents the upgrade behavior where a persisted storage class that a small or heterogeneous pool cannot satisfy now fails startup — with the RUSTFS_STORAGE_CLASS_STANDARD recovery steps. Docs-only; covers R4 from the on-disk compatibility audit.
* fix(heal): report parity from erasure geometry, not is_valid()
heal_object set HealResultItem.parity_blocks via `if lfi.is_valid()`, which was missed by the migration of the other quorum/metadata predicates. With the new `is_valid()` semantics (full payload validation; delete markers now return false), a delete marker or a geometry-bearing version with a benign collection quirk would misreport parity as the pool default instead of its own. Use `has_valid_erasure_geometry()` — the narrow "does this carry erasure geometry" predicate the rest of the migration uses — so reporting matches the object's actual layout. Reporting-only; no data-path change.
* fix(filemeta): do not silently serialize a non-canonical deleted FileInfo as an Object
`From<FileInfo> for FileMetaVersion` classifies by `is_storage_delete_marker()` (shape), which correctly routes canonical delete markers to Delete and purge-pending payloads (deleted=true with real erasure geometry) to Object. But a `deleted` FileInfo that is neither a canonical marker nor a valid erasure payload 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; `From` is infallible, so surface it with a structured `warn!` on the malformed branch instead of writing corrupt metadata silently. Legitimate purge-pending objects (valid geometry) are unaffected — the guard only fires for `deleted && !has_valid_erasure_geometry()`.
* test(filemeta): assert real historical xl.meta versions pass metadata-read validation
Empirical companion to the code-reasoned decode-tolerance invariants (docs/architecture/erasure-coding.md §11) and the rolling-upgrade / MinIO-migration compatibility concern: the tightened `validate_for_metadata_read` runs on every local disk read and peer-RPC-decoded FileInfo, so it must accept every version of real historically-written xl.meta, never reject it as FileCorrupt.
Loads five real fixtures — MinIO small-inline, MinIO versioned (two object versions + a delete marker), MinIO large multipart, a legacy V1 (xl.json-derived) object, and a legacy meta_ver 2 object — decodes every version with parts materialized, and asserts validate_for_metadata_read() is Ok for each. Reverting the tolerant handling (delete-marker shape, legacy per-part checksums, string/short transitioned-versionID, negative actual_size) turns this red.
* fix(ci): remove duplicate storage test re-exports
---------
Co-authored-by: overtrue <anzhengchao@gmail.com>
This commit is contained in:
cxymds
@@ -702,6 +702,7 @@ impl SetDisks {
|
||||
opts: &ObjectOptions,
|
||||
) -> Result<(ObjectInfo, Option<OldCurrentSize>)> {
|
||||
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<TempDir>, Vec<DiskStore>, Arc<SetDisks>) {
|
||||
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<TempDir>, Vec<DiskStore>, Arc<SetDisks>) {
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user