From f1b92af4a37267e01b944129ab1f41e2154073bf Mon Sep 17 00:00:00 2001 From: houseme Date: Sun, 23 Aug 2026 01:15:38 +0800 Subject: [PATCH 01/12] feat(ecstore): coalesce GET ReadVersion RPCs (#6395) --- crates/ecstore/src/cluster/rpc/remote_disk.rs | 97 ++- .../src/cluster/rpc/runtime_sources.rs | 60 +- crates/ecstore/src/disk/mod.rs | 94 ++- crates/ecstore/src/lib.rs | 8 + crates/ecstore/src/runtime/global.rs | 15 +- .../src/set_disk/core/io_primitives.rs | 572 +++++++++++++++++- crates/ecstore/src/set_disk/ops/object.rs | 2 +- crates/ecstore/src/set_disk/read.rs | 80 ++- crates/io-metrics/src/internode_metrics.rs | 7 + rustfs/src/server/readiness.rs | 5 + rustfs/src/storage/rpc/node_service/disk.rs | 114 +++- rustfs/src/storage/storage_api.rs | 4 + rustfs/src/storage_api.rs | 3 +- 13 files changed, 987 insertions(+), 74 deletions(-) diff --git a/crates/ecstore/src/cluster/rpc/remote_disk.rs b/crates/ecstore/src/cluster/rpc/remote_disk.rs index 32bf3ae57..a47533a81 100644 --- a/crates/ecstore/src/cluster/rpc/remote_disk.rs +++ b/crates/ecstore/src/cluster/rpc/remote_disk.rs @@ -42,8 +42,9 @@ use futures::lock::Mutex; use metrics::counter; use rustfs_filemeta::{FileInfo, ObjectPartInfo, RawFileInfo}; use rustfs_io_metrics::internode_metrics::{ - INTERNODE_STAGE_READ_VERSION_REQUEST_ENCODE, INTERNODE_STAGE_READ_VERSION_RESPONSE_DECODE, - INTERNODE_STAGE_READ_VERSION_RPC_ROUNDTRIP, + INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_ENCODE, INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_DECODE, + INTERNODE_STAGE_BATCH_READ_VERSION_RPC_ROUNDTRIP, INTERNODE_STAGE_READ_VERSION_REQUEST_ENCODE, + INTERNODE_STAGE_READ_VERSION_RESPONSE_DECODE, INTERNODE_STAGE_READ_VERSION_RPC_ROUNDTRIP, }; use rustfs_protos::ChannelClass; use rustfs_protos::evict_failed_connection; @@ -98,6 +99,7 @@ const NS_SCANNER_CAPABILITY_PROBE_TIMEOUT: Duration = Duration::from_secs(5); const REMOTE_DISK_READ_RETRY_BASE_BACKOFF: Duration = Duration::from_millis(50); const ENV_RUSTFS_METADATA_BATCH_READ: &str = "RUSTFS_METADATA_BATCH_READ"; const LEGACY_ENV_RUSTFS_BATCH_METADATA_RPC: &str = "RUSTFS_BATCH_METADATA_RPC"; +const ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE: &str = "RUSTFS_GET_METADATA_READ_VERSION_COALESCE"; const BATCH_METADATA_RPC_OFF: &str = "off"; const BATCH_METADATA_RPC_AUTO: &str = "auto"; const BATCH_METADATA_RPC_ON: &str = "on"; @@ -202,7 +204,8 @@ fn parse_batch_metadata_rpc_mode(raw: &str) -> BatchMetadataRpcMode { } fn batch_metadata_rpc_mode_from_env() -> BatchMetadataRpcMode { - rustfs_utils::get_env_opt_str(ENV_RUSTFS_METADATA_BATCH_READ) + rustfs_utils::get_env_opt_str(ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE) + .or_else(|| rustfs_utils::get_env_opt_str(ENV_RUSTFS_METADATA_BATCH_READ)) .or_else(|| rustfs_utils::get_env_opt_str(LEGACY_ENV_RUSTFS_BATCH_METADATA_RPC)) .as_deref() .map(parse_batch_metadata_rpc_mode) @@ -1826,6 +1829,12 @@ fn record_read_version_stage(stage: &'static str, started_at: Option) { } } +fn record_batch_read_version_stage(stage: &'static str, started_at: Option) { + if let Some(started_at) = started_at { + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_stage(stage, started_at.elapsed()); + } +} + /// Aggregate encoded size (bytes) of a `ReadMultiple` response, preferring the msgpack payloads /// and falling back to the JSON compatibility strings. Used to size the RPC for the payload /// histogram / large-payload alerting (grpc-optimization P0 instrumentation). @@ -1936,6 +1945,27 @@ fn decode_batch_read_version_response_items( Ok(batch_read_version_resps) } +fn batch_read_version_request_payload_len(req: &BatchReadVersionReq, req_json: &str, req_bin: &[u8]) -> usize { + req.items + .iter() + .fold(req_json.len().saturating_add(req_bin.len()), |total, item| { + total + .saturating_add(item.org_volume.len()) + .saturating_add(item.volume.len()) + .saturating_add(item.path.len()) + .saturating_add(item.version_id.len()) + }) +} + +fn batch_read_version_response_payload_len(response: &BatchReadVersionResponse) -> usize { + response + .batch_read_version_resps + .iter() + .map(String::len) + .sum::() + .saturating_add(response.batch_read_version_resps_bin.iter().map(Bytes::len).sum::()) +} + fn validate_decoded_file_info(file_info: &FileInfo) -> Result<()> { file_info.validate_for_metadata_read().map_err(Into::into) } @@ -2837,14 +2867,19 @@ impl DiskAPI for RemoteDisk { state = "started", "Remote disk RPC started" ); + let batch_read_version_attribution_enabled = rustfs_io_metrics::get_stage_metrics_enabled(); + let encode_started = read_version_stage_timer(batch_read_version_attribution_enabled); let batch_read_version_req = compat_json(&req)?; let batch_read_version_req_bin = encode_msgpack(&req)?; - + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_ENCODE, encode_started); + let request_payload_bytes = batch_read_version_attribution_enabled + .then(|| batch_read_version_request_payload_len(&req, &batch_read_version_req, &batch_read_version_req_bin)); let batch_result = self .execute_with_timeout_for_op( "batch_read_version", move || async move { let disk = self.disk_ref().await; + let disk_len = disk.len(); let mut client = self .get_bulk_client() .await @@ -2855,9 +2890,20 @@ impl DiskAPI for RemoteDisk { batch_read_version_req_bin: batch_read_version_req_bin.into(), }); + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_request(); + if let Some(request_payload_bytes) = request_payload_bytes { + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_sent_bytes( + request_payload_bytes.saturating_add(disk_len), + ); + } + let rpc_started = read_version_stage_timer(batch_read_version_attribution_enabled); let response = match client.batch_read_version(request).await { - Ok(response) => response.into_inner(), + Ok(response) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RPC_ROUNDTRIP, rpc_started); + response.into_inner() + } Err(status) if status.code() == Code::Unimplemented => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RPC_ROUNDTRIP, rpc_started); if mode.should_fallback_on_unimplemented() { record_batch_read_version_gate_decision(mode, BATCH_READ_VERSION_GATE_FALLBACK_UNIMPLEMENTED); warn!( @@ -2874,6 +2920,7 @@ impl DiskAPI for RemoteDisk { } record_batch_read_version_gate_decision(mode, BATCH_READ_VERSION_GATE_UNSUPPORTED_NO_FALLBACK); + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_error(); warn!( event = EVENT_REMOTE_DISK_RPC, component = LOG_COMPONENT_ECSTORE, @@ -2886,14 +2933,33 @@ impl DiskAPI for RemoteDisk { ); return Err(Error::from(status)); } - Err(status) => return Err(Error::from(status)), + Err(status) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RPC_ROUNDTRIP, rpc_started); + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_error(); + return Err(Error::from(status)); + } }; if !response.success { + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_error(); return Err(response.error.unwrap_or_default().into()); } - decode_batch_read_version_response_items(response, &self.endpoint).map(Some) + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_recv_bytes( + batch_read_version_response_payload_len(&response), + ); + let decode_started = read_version_stage_timer(batch_read_version_attribution_enabled); + match decode_batch_read_version_response_items(response, &self.endpoint) { + Ok(batch_read_version_resps) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_DECODE, decode_started); + Ok(Some(batch_read_version_resps)) + } + Err(err) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_DECODE, decode_started); + crate::cluster::rpc::runtime_sources::record_remote_disk_grpc_batch_read_version_error(); + Err(err) + } + } }, get_max_timeout_duration(), ) @@ -4621,6 +4687,7 @@ mod tests { } else { "file version not found".to_string() }, + error_code: if success { 0 } else { DiskError::FileVersionNotFound.to_u32() }, } } @@ -4740,6 +4807,7 @@ mod tests { fn batch_metadata_rpc_mode_uses_documented_env_before_legacy_alias() { temp_env::with_vars( [ + (ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE, None::<&str>), (ENV_RUSTFS_METADATA_BATCH_READ, Some("auto")), (LEGACY_ENV_RUSTFS_BATCH_METADATA_RPC, Some("on")), ], @@ -4749,10 +4817,25 @@ mod tests { ); } + #[test] + fn batch_metadata_rpc_mode_uses_get_coalescer_env_before_batch_env() { + temp_env::with_vars( + [ + (ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE, Some("on")), + (ENV_RUSTFS_METADATA_BATCH_READ, Some("off")), + (LEGACY_ENV_RUSTFS_BATCH_METADATA_RPC, Some("off")), + ], + || { + assert_eq!(batch_metadata_rpc_mode_from_env(), BatchMetadataRpcMode::On); + }, + ); + } + #[test] fn batch_metadata_rpc_mode_falls_back_to_legacy_env_alias() { temp_env::with_vars( [ + (ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE, None::<&str>), (ENV_RUSTFS_METADATA_BATCH_READ, None::<&str>), (LEGACY_ENV_RUSTFS_BATCH_METADATA_RPC, Some("on")), ], diff --git a/crates/ecstore/src/cluster/rpc/runtime_sources.rs b/crates/ecstore/src/cluster/rpc/runtime_sources.rs index 0c8393a2e..ab9a34fe9 100644 --- a/crates/ecstore/src/cluster/rpc/runtime_sources.rs +++ b/crates/ecstore/src/cluster/rpc/runtime_sources.rs @@ -14,9 +14,10 @@ use rustfs_io_metrics::internode_metrics::{ INTERNODE_MSGPACK_CODEC_JSON, INTERNODE_MSGPACK_CODEC_MSGPACK, INTERNODE_MSGPACK_DIRECTION_RESPONSE, - INTERNODE_OPERATION_GRPC_READ_ALL, INTERNODE_OPERATION_GRPC_READ_MULTIPLE, INTERNODE_OPERATION_GRPC_READ_VERSION, - INTERNODE_OPERATION_GRPC_WRITE_ALL, INTERNODE_OPERATION_PUT_FILE_STREAM, INTERNODE_OPERATION_READ_FILE_STREAM, - INTERNODE_TRANSPORT_BACKEND_GRPC, INTERNODE_TRANSPORT_BACKEND_TCP_HTTP, global_internode_metrics, + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, INTERNODE_OPERATION_GRPC_READ_ALL, INTERNODE_OPERATION_GRPC_READ_MULTIPLE, + INTERNODE_OPERATION_GRPC_READ_VERSION, INTERNODE_OPERATION_GRPC_WRITE_ALL, INTERNODE_OPERATION_PUT_FILE_STREAM, + INTERNODE_OPERATION_READ_FILE_STREAM, INTERNODE_TRANSPORT_BACKEND_GRPC, INTERNODE_TRANSPORT_BACKEND_TCP_HTTP, + global_internode_metrics, }; use std::time::Duration; @@ -93,6 +94,59 @@ pub(crate) fn record_remote_disk_grpc_read_version_request() { ); } +pub(crate) fn record_remote_disk_grpc_batch_read_version_request() { + if !rustfs_io_metrics::get_stage_metrics_enabled() { + return; + } + global_internode_metrics().record_outgoing_request_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + ); +} + +pub(crate) fn record_remote_disk_grpc_batch_read_version_stage(stage: &'static str, duration: Duration) { + if !rustfs_io_metrics::get_stage_metrics_enabled() { + return; + } + global_internode_metrics().record_stage_duration_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + stage, + duration, + ); +} + +pub(crate) fn record_remote_disk_grpc_batch_read_version_error() { + if !rustfs_io_metrics::get_stage_metrics_enabled() { + return; + } + global_internode_metrics() + .record_error_for_operation_and_backend(INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, INTERNODE_TRANSPORT_BACKEND_GRPC); +} + +pub(crate) fn record_remote_disk_grpc_batch_read_version_sent_bytes(bytes: usize) { + if !rustfs_io_metrics::get_stage_metrics_enabled() { + return; + } + global_internode_metrics().record_sent_bytes_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + bytes, + ); +} + +pub(crate) fn record_remote_disk_grpc_batch_read_version_recv_bytes(bytes: usize) { + if !rustfs_io_metrics::get_stage_metrics_enabled() { + return; + } + global_internode_metrics().record_recv_bytes_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + bytes, + ); + record_grpc_payload_size(INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, bytes); +} + pub(crate) fn record_remote_disk_grpc_read_version_error() { if !rustfs_io_metrics::get_stage_metrics_enabled() { return; diff --git a/crates/ecstore/src/disk/mod.rs b/crates/ecstore/src/disk/mod.rs index ecd0179e0..fb8c8de5a 100644 --- a/crates/ecstore/src/disk/mod.rs +++ b/crates/ecstore/src/disk/mod.rs @@ -44,6 +44,8 @@ pub const PART_TRANSACTION_ROLLBACK: &str = "rollback"; const LOG_COMPONENT_ECSTORE: &str = "ecstore"; const LOG_SUBSYSTEM_DISK: &str = "disk"; const EVENT_DISK_PART_ERR_UNCLASSIFIED: &str = "disk_part_err_unclassified"; +const ENV_BATCH_READ_VERSION_SERVER_PARALLELISM: &str = "RUSTFS_BATCH_READ_VERSION_SERVER_PARALLELISM"; +const BATCH_READ_VERSION_SERVER_PARALLELISM: usize = 4; pub fn part_transaction_path(part_path: &str) -> String { match part_path.rsplit_once('/') { @@ -62,6 +64,7 @@ use bytes::Bytes; use endpoint::Endpoint; use error::DiskError; use error::{Error, Result}; +use futures::stream::{self, StreamExt}; use local::LocalDisk; use rustfs_filemeta::{FileInfo, ObjectPartInfo, RawFileInfo}; use rustfs_madmin::info_commands::DiskMetrics; @@ -417,6 +420,14 @@ impl DiskAPI for Disk { } } + #[tracing::instrument(level = "trace", skip_all)] + async fn batch_read_version(&self, req: BatchReadVersionReq) -> Result> { + match self { + Disk::Local(local_disk) => local_disk.batch_read_version(req).await, + Disk::Remote(remote_disk) => remote_disk.batch_read_version(req).await, + } + } + #[tracing::instrument(level = "trace", skip_all)] async fn read_xl(&self, volume: &str, path: &str, read_data: bool) -> Result { match self { @@ -1028,36 +1039,47 @@ where D: DiskAPI + ?Sized, { validate_batch_read_version_item_count(req.items.len())?; + let parallelism = batch_read_version_server_parallelism(); - let mut responses = Vec::with_capacity(req.items.len()); - for (index, item) in req.items.iter().enumerate() { - let response = match disk - .read_version(&item.org_volume, &item.volume, &item.path, &item.version_id, &req.opts) - .await - { - Ok(file_info) => BatchReadVersionResp { - index, - path: item.path.clone(), - version_id: item.version_id.clone(), - success: true, - file_info, - error: String::new(), - }, - Err(err) => BatchReadVersionResp { - index, - path: item.path.clone(), - version_id: item.version_id.clone(), - success: false, - file_info: FileInfo::default(), - error: err.to_string(), - }, - }; - responses.push(response); - } + let mut responses = stream::iter(req.items.into_iter().enumerate()) + .map(|(index, item)| async move { + match disk + .read_version(&item.org_volume, &item.volume, &item.path, &item.version_id, &req.opts) + .await + { + Ok(file_info) => BatchReadVersionResp { + index, + path: item.path, + version_id: item.version_id, + success: true, + file_info, + error: String::new(), + error_code: 0, + }, + Err(err) => BatchReadVersionResp { + index, + path: item.path, + version_id: item.version_id, + success: false, + file_info: FileInfo::default(), + error: err.to_string(), + error_code: err.to_u32(), + }, + } + }) + .buffer_unordered(parallelism) + .collect::>() + .await; + responses.sort_unstable_by_key(|response| response.index); Ok(responses) } +fn batch_read_version_server_parallelism() -> usize { + rustfs_utils::get_env_usize(ENV_BATCH_READ_VERSION_SERVER_PARALLELISM, BATCH_READ_VERSION_SERVER_PARALLELISM) + .clamp(1, BATCH_READ_VERSION_MAX_ITEMS) +} + #[derive(Debug, Default, Serialize, Deserialize)] pub struct CheckPartsResp { pub results: Vec, @@ -1322,6 +1344,8 @@ pub struct BatchReadVersionResp { pub success: bool, pub file_info: FileInfo, pub error: String, + #[serde(default)] + pub error_code: u32, } pub fn validate_batch_read_version_item_count(item_count: usize) -> Result<()> { @@ -1417,6 +1441,26 @@ mod tests { assert!(!partial_valid_location.valid()); } + #[test] + fn batch_read_version_server_parallelism_defaults_to_conservative_four() { + temp_env::with_var(ENV_BATCH_READ_VERSION_SERVER_PARALLELISM, None::<&str>, || { + assert_eq!(batch_read_version_server_parallelism(), 4); + }); + } + + #[test] + fn batch_read_version_server_parallelism_honors_env_with_bounds() { + temp_env::with_var(ENV_BATCH_READ_VERSION_SERVER_PARALLELISM, Some("8"), || { + assert_eq!(batch_read_version_server_parallelism(), 8); + }); + temp_env::with_var(ENV_BATCH_READ_VERSION_SERVER_PARALLELISM, Some("0"), || { + assert_eq!(batch_read_version_server_parallelism(), 1); + }); + temp_env::with_var(ENV_BATCH_READ_VERSION_SERVER_PARALLELISM, Some("9999"), || { + assert_eq!(batch_read_version_server_parallelism(), BATCH_READ_VERSION_MAX_ITEMS); + }); + } + /// Test FileInfoVersions find_version_index #[test] fn test_file_info_versions_find_version_index() { diff --git a/crates/ecstore/src/lib.rs b/crates/ecstore/src/lib.rs index 2ed643ead..da4abbd39 100644 --- a/crates/ecstore/src/lib.rs +++ b/crates/ecstore/src/lib.rs @@ -81,6 +81,14 @@ pub fn shutdown_background_monitors() { cluster::rpc::shutdown_background_monitors(); } +/// Publish that the process is ready to serve user-object GET traffic. +/// +/// Experimental metadata coalescing is allowed to run only after this point so +/// startup and internal metadata reads keep the original per-disk path. +pub fn mark_get_metadata_read_version_coalescing_service_ready() { + runtime::global::mark_get_metadata_read_version_coalescing_service_ready(); +} + #[cfg(test)] mod rio_tests { #[test] diff --git a/crates/ecstore/src/runtime/global.rs b/crates/ecstore/src/runtime/global.rs index fcc4411f0..56ba22a0a 100644 --- a/crates/ecstore/src/runtime/global.rs +++ b/crates/ecstore/src/runtime/global.rs @@ -25,7 +25,10 @@ use lazy_static::lazy_static; use rustfs_lock::client::LockClient; use std::{ collections::HashMap, - sync::{Arc, OnceLock}, + sync::{ + Arc, OnceLock, + atomic::{AtomicBool, Ordering}, + }, time::SystemTime, }; use tokio::sync::{OnceCell, RwLock}; @@ -37,6 +40,16 @@ pub const DISK_MIN_INODES: u64 = 1000; pub const DISK_FILL_FRACTION: f64 = 0.99; pub const DISK_RESERVE_FRACTION: f64 = 0.15; +static GET_METADATA_READ_VERSION_COALESCING_SERVICE_READY: AtomicBool = AtomicBool::new(false); + +pub(crate) fn mark_get_metadata_read_version_coalescing_service_ready() { + GET_METADATA_READ_VERSION_COALESCING_SERVICE_READY.store(true, Ordering::Release); +} + +pub(crate) fn get_metadata_read_version_coalescing_service_ready() -> bool { + GET_METADATA_READ_VERSION_COALESCING_SERVICE_READY.load(Ordering::Acquire) +} + // Global singletons for backward compatibility with MinIO port. // These should be migrated to AppContext over time. // See issue #730 for migration plan. diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index d17c1c751..1a77ba267 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -53,11 +53,12 @@ use crate::diagnostics::get::{ GetObjectFailureReason, classify_disk_error, get_stage_timer_if_enabled, record_get_object_pipeline_failure, record_get_object_pipeline_failure_for_path, record_get_stage_duration_if_enabled, }; -use crate::disk::disk_store::DiskStoreRenameDataExt; +use crate::disk::disk_store::{DiskStoreRenameDataExt, get_drive_metadata_timeout}; use crate::disk::local::DELETE_DATA_DIR_MARKER_PREFIX; use crate::disk::{ - DataDirDeleteStatus, OldCurrentSize, PART_TRANSACTION_NEW_META, PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, - PartTransactionAction, STORAGE_FORMAT_FILE_BACKUP, part_transaction_path, + BATCH_READ_VERSION_MAX_ITEMS, BatchReadVersionItem, BatchReadVersionReq, BatchReadVersionResp, DataDirDeleteStatus, Disk, + OldCurrentSize, PART_TRANSACTION_NEW_META, PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, PartTransactionAction, + STORAGE_FORMAT_FILE_BACKUP, part_transaction_path, }; use crate::erasure::coding::BitrotReader; use crate::io_support::bitrot::ShardReader; @@ -75,7 +76,7 @@ use std::{ future::Future, pin::Pin, sync::{ - OnceLock, + Arc, OnceLock, atomic::{AtomicUsize, Ordering}, }, task::{Context, Poll}, @@ -94,6 +95,242 @@ fn metadata_distribution_key(bucket: &str, object: &str) -> String { [bucket, object].join("/") } +fn read_version_coalescing_enabled() -> bool { + let enabled = || { + rustfs_utils::get_env_opt_str(ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE) + .is_some_and(|value| value.eq_ignore_ascii_case("auto") || value.eq_ignore_ascii_case("on")) + }; + + #[cfg(test)] + { + enabled() + } + + #[cfg(not(test))] + { + static ENABLED: OnceLock = OnceLock::new(); + *ENABLED.get_or_init(enabled) + } +} + +fn read_version_coalescing_delay() -> Duration { + #[cfg(test)] + { + let micros = rustfs_utils::get_env_u64( + ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS, + DEFAULT_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS, + ); + Duration::from_micros(micros) + } + + #[cfg(not(test))] + { + static DELAY: OnceLock = OnceLock::new(); + *DELAY.get_or_init(|| { + Duration::from_micros(rustfs_utils::get_env_u64( + ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS, + DEFAULT_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS, + )) + }) + } +} + +struct CoalescedReadVersionRequest { + item: BatchReadVersionItem, + tx: oneshot::Sender>, +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +struct ReadVersionCoalescerKey { + disk: usize, + incl_free_versions: bool, + read_data: bool, + healing: bool, +} + +impl ReadVersionCoalescerKey { + fn new(disk: &DiskStore, opts: &ReadOptions) -> Self { + Self { + disk: Arc::as_ptr(disk) as usize, + incl_free_versions: opts.incl_free_versions, + read_data: opts.read_data, + healing: opts.healing, + } + } +} + +#[derive(Default)] +struct ReadVersionCoalescer { + lanes: HashMap>, +} + +fn read_version_coalescer() -> &'static Mutex { + static COALESCER: OnceLock> = OnceLock::new(); + COALESCER.get_or_init(|| Mutex::new(ReadVersionCoalescer::default())) +} + +fn record_read_version_coalescer_event(event: &'static str, item_count: usize) { + counter!( + METRIC_GET_METADATA_READ_VERSION_COALESCER_TOTAL, + "event" => event, + "item_count" => item_count.to_string() + ) + .increment(1); +} + +async fn read_version_via_coalescer( + disk: DiskStore, + org_bucket: &str, + bucket: &str, + object: &str, + version_id: &str, + opts: &ReadOptions, + allow_coalescing: bool, +) -> disk::error::Result { + if !allow_coalescing || !read_version_coalescing_enabled() { + return disk.read_version(org_bucket, bucket, object, version_id, opts).await; + } + if !matches!(disk.as_ref(), Disk::Remote(_)) { + record_read_version_coalescer_event("bypass_non_remote", 1); + return disk.read_version(org_bucket, bucket, object, version_id, opts).await; + } + + let (tx, rx) = oneshot::channel(); + let item = BatchReadVersionItem { + org_volume: org_bucket.to_string(), + volume: bucket.to_string(), + path: object.to_string(), + version_id: version_id.to_string(), + }; + let lane_key = ReadVersionCoalescerKey::new(&disk, opts); + let pending = { + let mut coalescer = read_version_coalescer().lock().await; + let lane = coalescer.lanes.entry(lane_key).or_default(); + let schedule_delayed_flush = lane.is_empty(); + lane.push(CoalescedReadVersionRequest { item, tx }); + if lane.len() >= BATCH_READ_VERSION_MAX_ITEMS { + coalescer.lanes.remove(&lane_key) + } else if schedule_delayed_flush { + let disk = disk.clone(); + let task_opts = *opts; + tokio::spawn(async move { + tokio::time::sleep(read_version_coalescing_delay()).await; + flush_read_version_coalescer_lane(lane_key, disk, task_opts).await; + }); + None + } else { + None + } + }; + + if let Some(pending) = pending { + flush_read_version_coalescer_pending(lane_key, disk, *opts, pending).await; + } + + rx.await + .unwrap_or_else(|_| Err(DiskError::other("coalesced read_version response channel closed"))) +} + +async fn flush_read_version_coalescer_lane(lane_key: ReadVersionCoalescerKey, disk: DiskStore, opts: ReadOptions) { + let pending = { + let mut coalescer = read_version_coalescer().lock().await; + coalescer.lanes.remove(&lane_key).unwrap_or_default() + }; + flush_read_version_coalescer_pending(lane_key, disk, opts, pending).await; +} + +async fn flush_read_version_coalescer_pending( + lane_key: ReadVersionCoalescerKey, + disk: DiskStore, + opts: ReadOptions, + pending: Vec, +) { + if pending.is_empty() { + return; + } + + #[cfg(test)] + { + let mut observed_paths = HashSet::new(); + for request in &pending { + if observed_paths.insert(request.item.path.as_str()) { + disk_call_counters::record(&request.item.path, disk_call_counters::KIND_BATCH_READ_VERSION, lane_key.disk); + } + } + } + + let mut senders = Vec::with_capacity(pending.len()); + let mut items = Vec::with_capacity(pending.len()); + for request in pending { + senders.push(request.tx); + items.push(request.item); + } + + let expected_items = items.clone(); + record_read_version_coalescer_event("attempted_batch", items.len()); + let result = + match tokio::time::timeout(get_drive_metadata_timeout(), disk.batch_read_version(BatchReadVersionReq { items, opts })) + .await + { + Ok(result) => result, + Err(_) => Err(DiskError::Timeout), + }; + match result { + Ok(responses) => { + let results = map_batch_read_version_responses(&expected_items, responses); + for (tx, result) in senders.into_iter().zip(results) { + let _ = tx.send(result); + } + } + Err(err) => { + let message = err.to_string(); + for tx in senders { + let _ = tx.send(Err(DiskError::other(message.clone()))); + } + } + } +} + +fn map_batch_read_version_responses( + expected_items: &[BatchReadVersionItem], + responses: Vec, +) -> Vec> { + let mut results = (0..expected_items.len()) + .map(|_| Err(DiskError::other("coalesced read_version response missing"))) + .collect::>(); + let mut seen = vec![false; expected_items.len()]; + for response in responses { + let Some(expected) = expected_items.get(response.index) else { + continue; + }; + let Some(slot) = results.get_mut(response.index) else { + continue; + }; + if seen[response.index] { + *slot = Err(DiskError::other("coalesced read_version response duplicate index")); + continue; + } + seen[response.index] = true; + if response.path != expected.path || response.version_id != expected.version_id { + *slot = Err(DiskError::other("coalesced read_version response identity mismatch")); + } else { + *slot = if response.success { + Ok(response.file_info) + } else { + Err(batch_read_version_response_error(response.error_code, response.error)) + }; + } + } + results +} + +fn batch_read_version_response_error(error_code: u32, error: String) -> DiskError { + match DiskError::from_u32(error_code) { + Some(DiskError::Io(_)) | None => DiskError::other(error), + Some(error) => error, + } +} + pub(in crate::set_disk) fn bounded_metadata_fanout_order( bucket: &str, object: &str, @@ -133,11 +370,15 @@ pub(in crate::set_disk) fn bounded_metadata_fanout_order( order } use tokio::io::{AsyncRead, ReadBuf}; -use tokio::sync::RwLock; +use tokio::sync::{Mutex, RwLock, oneshot}; use tokio::task::JoinSet; pub(in crate::set_disk) const EVENT_SET_DISK_READ: &str = "set_disk_read"; pub(in crate::set_disk) const ENV_RUSTFS_GET_DATA_BLOCKS_FIRST_READER_SETUP: &str = "RUSTFS_GET_DATA_BLOCKS_FIRST_READER_SETUP"; +const ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE: &str = "RUSTFS_GET_METADATA_READ_VERSION_COALESCE"; +const ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS: &str = "RUSTFS_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS"; +const DEFAULT_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS: u64 = 200; +const METRIC_GET_METADATA_READ_VERSION_COALESCER_TOTAL: &str = "rustfs_get_metadata_read_version_coalescer_total"; pub(in crate::set_disk) const ENV_RUSTFS_PUT_RENAME_EARLY_ACK_ENABLE: &str = "RUSTFS_PUT_RENAME_EARLY_ACK_ENABLE"; /// Default reader-setup strategy for the GET read path (rustfs/backlog#1215, /// #1159, #923). @@ -2356,6 +2597,7 @@ impl SetDisks { false, true, 0, + false, ) .await?; Ok((ress, errors)) @@ -2386,6 +2628,36 @@ impl SetDisks { true, caller_allows_early_stop, default_parity_count, + false, + ) + .await + } + + #[allow(clippy::too_many_arguments)] + pub(in crate::set_disk) async fn read_all_fileinfo_observed_for_get_object( + disks: &[Option], + org_bucket: &str, + bucket: &str, + object: &str, + version_id: &str, + read_data: bool, + incl_free_versions: bool, + caller_allows_early_stop: bool, + default_parity_count: usize, + ) -> disk::error::Result<(Vec, Vec>, MetadataFanoutDiagnostics)> { + Self::read_all_fileinfo_inner( + disks, + org_bucket, + bucket, + object, + version_id, + read_data, + false, + incl_free_versions, + true, + caller_allows_early_stop, + default_parity_count, + true, ) .await } @@ -2408,6 +2680,7 @@ impl SetDisks { // subset would fail write quorum (backlog#872 regression). caller_allows_early_stop: bool, default_parity_count: usize, + allow_coalescing: bool, ) -> disk::error::Result<(Vec, Vec>, MetadataFanoutDiagnostics)> { let early_stop_enabled = caller_allows_early_stop && observe && (is_get_metadata_early_stop_enabled() || is_version_early_stop_enabled()); @@ -2424,6 +2697,7 @@ impl SetDisks { healing, incl_free_versions, default_parity_count, + allow_coalescing, ) .await; } @@ -2446,6 +2720,7 @@ impl SetDisks { healing, incl_free_versions, observe, + allow_coalescing, ) .await } @@ -2461,6 +2736,7 @@ impl SetDisks { healing: bool, incl_free_versions: bool, observe: bool, + allow_coalescing: bool, ) -> disk::error::Result<(Vec, Vec>, MetadataFanoutDiagnostics)> { let fanout_start = observe.then(Instant::now); let mut ress = Vec::with_capacity(disks.len()); @@ -2492,7 +2768,7 @@ impl SetDisks { if let Some(delay) = slowtail_fault.as_ref().and_then(|fault| fault.delay_for_disk(disk_index)) { tokio::time::sleep(delay).await; } - disk.read_version(&org_bucket, &bucket, &object, &version_id, &task_opts) + read_version_via_coalescer(disk, &org_bucket, &bucket, &object, &version_id, &task_opts, allow_coalescing) .await } else { Err(DiskError::DiskNotFound) @@ -2559,6 +2835,7 @@ impl SetDisks { healing: bool, incl_free_versions: bool, default_parity_count: usize, + allow_coalescing: bool, ) -> disk::error::Result<(Vec, Vec>, MetadataFanoutDiagnostics)> { let fanout_start = Instant::now(); let mut ress = vec![FileInfo::default(); disks.len()]; @@ -2607,7 +2884,7 @@ impl SetDisks { if let Some(delay) = slowtail_fault.as_ref().and_then(|fault| fault.delay_for_disk(index)) { tokio::time::sleep(delay).await; } - disk.read_version(&org_bucket, &bucket, &object, &version_id, &task_opts) + read_version_via_coalescer(disk, &org_bucket, &bucket, &object, &version_id, &task_opts, allow_coalescing) .await } else { Err(DiskError::DiskNotFound) @@ -5737,6 +6014,7 @@ pub(crate) mod disk_call_counters { /// Kind label for the per-disk `read_version` metadata RPC. pub const KIND_READ_VERSION: &str = "read_version"; + pub const KIND_BATCH_READ_VERSION: &str = "batch_read_version"; /// Registry key: (object, kind, disk_index). type CountKey = (String, String, usize); @@ -6460,6 +6738,286 @@ mod tests { drop(dirs); } + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn metadata_read_version_coalescer_bypasses_local_disks() { + const DISKS: usize = 4; + let bucket = "coalesced-read-version-local-bypass-bucket"; + let object_a = "coalesced-local-object-a"; + let object_b = "coalesced-local-object-b"; + let (dirs, disks) = call_counter_local_disks(bucket, DISKS).await; + install_metadata_fanout_fileinfo(&disks, bucket, object_a, None).await; + install_metadata_fanout_fileinfo(&disks, bucket, object_b, None).await; + + temp_env::async_with_vars( + [ + (ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE, Some("auto")), + (ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE_DELAY_MICROS, Some("5000")), + ], + async { + let calls = disk_call_counters::observe(object_a); + let disks_a = disks.clone(); + let disks_b = disks.clone(); + let read_a = tokio::spawn(async move { + SetDisks::read_all_fileinfo_observed_for_get_object( + &disks_a, "", bucket, object_a, "", false, false, false, 2, + ) + .await + .map(|(file_infos, errors, _)| (file_infos, errors)) + }); + tokio::task::yield_now().await; + let read_b = tokio::spawn(async move { + SetDisks::read_all_fileinfo_observed_for_get_object( + &disks_b, "", bucket, object_b, "", false, false, false, 2, + ) + .await + .map(|(file_infos, errors, _)| (file_infos, errors)) + }); + + let (metadata_a, errs_a) = read_a + .await + .expect("first read task should not panic") + .expect("first coalesced read should resolve"); + let (metadata_b, errs_b) = read_b + .await + .expect("second read task should not panic") + .expect("second coalesced read should resolve"); + + assert_eq!(metadata_a.iter().filter(|fi| fi.name == object_a).count(), DISKS); + assert_eq!(metadata_b.iter().filter(|fi| fi.name == object_b).count(), DISKS); + assert!(errs_a.iter().all(Option::is_none)); + assert!(errs_b.iter().all(Option::is_none)); + assert_eq!( + calls.total(disk_call_counters::KIND_READ_VERSION), + DISKS as u64, + "local disks still execute the ordinary per-disk read_version path" + ); + assert_eq!( + calls.total(disk_call_counters::KIND_BATCH_READ_VERSION), + 0, + "GET coalescing targets internode RPC count only and must not batch local disk reads" + ); + }, + ) + .await; + + drop(dirs); + } + + #[tokio::test] + async fn metadata_read_version_coalescer_requires_get_object_intent() { + const DISKS: usize = 4; + let bucket = "coalesced-read-version-default-bypass-bucket"; + let object = "default-bypass-object"; + let (dirs, disks) = call_counter_local_disks(bucket, DISKS).await; + install_metadata_fanout_fileinfo(&disks, bucket, object, None).await; + + temp_env::async_with_vars([(ENV_RUSTFS_GET_METADATA_READ_VERSION_COALESCE, Some("auto"))], async { + let calls = disk_call_counters::observe(object); + let (metadata, errs) = SetDisks::read_all_fileinfo(&disks, "", bucket, object, "", false, false, false) + .await + .expect("default metadata read should resolve"); + + assert_eq!(metadata.iter().filter(|fi| fi.name == object).count(), DISKS); + assert!(errs.iter().all(Option::is_none)); + assert_eq!(calls.total(disk_call_counters::KIND_READ_VERSION), DISKS as u64); + assert_eq!( + calls.total(disk_call_counters::KIND_BATCH_READ_VERSION), + 0, + "non-GET metadata paths must bypass coalescer even when the env gate is enabled" + ); + }) + .await; + + drop(dirs); + } + + #[test] + fn batch_read_version_response_mapping_preserves_index_and_errors() { + let expected_items = vec![ + BatchReadVersionItem { + org_volume: String::new(), + volume: "bucket".to_string(), + path: "object-a".to_string(), + version_id: "v-a".to_string(), + }, + BatchReadVersionItem { + org_volume: String::new(), + volume: "bucket".to_string(), + path: "object-b".to_string(), + version_id: "v-b".to_string(), + }, + BatchReadVersionItem { + org_volume: String::new(), + volume: "bucket".to_string(), + path: "object-c".to_string(), + version_id: "v-c".to_string(), + }, + ]; + let ok_file_info = FileInfo { + name: "object-a".to_string(), + ..Default::default() + }; + let responses = vec![ + BatchReadVersionResp { + index: 2, + path: "object-c".to_string(), + version_id: "v-c".to_string(), + success: false, + file_info: FileInfo::default(), + error: "disk read failed".to_string(), + error_code: 0, + }, + BatchReadVersionResp { + index: 0, + path: "object-a".to_string(), + version_id: "v-a".to_string(), + success: true, + file_info: ok_file_info, + error: String::new(), + error_code: 0, + }, + ]; + + let mut results = map_batch_read_version_responses(&expected_items, responses).into_iter(); + let first = results + .next() + .expect("slot 0 should exist") + .expect("slot 0 should map the success response by index"); + assert_eq!(first.name, "object-a"); + + let missing = results + .next() + .expect("slot 1 should exist") + .expect_err("slot 1 should stay missing"); + assert!( + missing.to_string().contains("response missing"), + "unexpected missing response error: {missing}" + ); + + let failed = results + .next() + .expect("slot 2 should exist") + .expect_err("slot 2 should map the response error"); + assert!(failed.to_string().contains("disk read failed"), "unexpected per-item error: {failed}"); + assert!(results.next().is_none()); + } + + #[test] + fn batch_read_version_response_mapping_preserves_typed_not_found_errors() { + let expected_items = vec![ + BatchReadVersionItem { + org_volume: String::new(), + volume: "bucket".to_string(), + path: "object-a".to_string(), + version_id: "v-a".to_string(), + }, + BatchReadVersionItem { + org_volume: String::new(), + volume: "bucket".to_string(), + path: "object-b".to_string(), + version_id: "v-b".to_string(), + }, + ]; + let results = map_batch_read_version_responses( + &expected_items, + vec![ + BatchReadVersionResp { + index: 0, + path: "object-a".to_string(), + version_id: "v-a".to_string(), + success: false, + file_info: FileInfo::default(), + error: DiskError::FileNotFound.to_string(), + error_code: DiskError::FileNotFound.to_u32(), + }, + BatchReadVersionResp { + index: 1, + path: "object-b".to_string(), + version_id: "v-b".to_string(), + success: false, + file_info: FileInfo::default(), + error: DiskError::FileVersionNotFound.to_string(), + error_code: DiskError::FileVersionNotFound.to_u32(), + }, + ], + ); + + assert!(matches!(results.first().expect("slot 0 should exist"), Err(DiskError::FileNotFound))); + assert!(matches!( + results.get(1).expect("slot 1 should exist"), + Err(DiskError::FileVersionNotFound) + )); + } + + #[test] + fn batch_read_version_response_mapping_rejects_identity_mismatch_and_duplicate_index() { + let expected_items = vec![BatchReadVersionItem { + org_volume: String::new(), + volume: "bucket".to_string(), + path: "object-a".to_string(), + version_id: "v-a".to_string(), + }]; + let mismatched = map_batch_read_version_responses( + &expected_items, + vec![BatchReadVersionResp { + index: 0, + path: "object-b".to_string(), + version_id: "v-a".to_string(), + success: true, + file_info: FileInfo { + name: "object-b".to_string(), + ..Default::default() + }, + error: String::new(), + error_code: 0, + }], + ) + .pop() + .expect("slot 0 should exist") + .expect_err("identity mismatch should fail closed"); + assert!( + mismatched.to_string().contains("identity mismatch"), + "unexpected mismatch error: {mismatched}" + ); + + let duplicate = map_batch_read_version_responses( + &expected_items, + vec![ + BatchReadVersionResp { + index: 0, + path: "object-a".to_string(), + version_id: "v-a".to_string(), + success: true, + file_info: FileInfo { + name: "object-a".to_string(), + ..Default::default() + }, + error: String::new(), + error_code: 0, + }, + BatchReadVersionResp { + index: 0, + path: "object-a".to_string(), + version_id: "v-a".to_string(), + success: true, + file_info: FileInfo { + name: "object-a".to_string(), + ..Default::default() + }, + error: String::new(), + error_code: 0, + }, + ], + ) + .pop() + .expect("slot 0 should exist") + .expect_err("duplicate response index should fail closed"); + assert!( + duplicate.to_string().contains("duplicate index"), + "unexpected duplicate error: {duplicate}" + ); + } + /// Isolation guard: unobserved objects record nothing (so parallel tests do /// not inflate one another), and a scope clears its own counts on drop. #[tokio::test] diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 0d429e3b3..8f07a7ff7 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -1294,7 +1294,7 @@ impl crate::storage_api_contracts::object::ObjectIO for SetDisks { (prepared.snapshot, prepared.object_info) } else { match self - .get_object_fileinfo( + .get_object_fileinfo_for_get_object_reader( bucket, object, opts, diff --git a/crates/ecstore/src/set_disk/read.rs b/crates/ecstore/src/set_disk/read.rs index 2b556b143..15e7e4e2a 100644 --- a/crates/ecstore/src/set_disk/read.rs +++ b/crates/ecstore/src/set_disk/read.rs @@ -259,10 +259,33 @@ impl SetDisks { read_data: bool, caller_allows_early_stop: bool, ) -> Result { - self.get_object_fileinfo_gated(bucket, object, opts, read_data, caller_allows_early_stop) + self.get_object_fileinfo_gated_inner(bucket, object, opts, read_data, caller_allows_early_stop, false) .await } + #[tracing::instrument(level = "debug", skip(self))] + #[hotpath::measure(impl_type = "SetDisks")] + pub(super) async fn get_object_fileinfo_for_get_object_reader( + &self, + bucket: &str, + object: &str, + opts: &ObjectOptions, + read_data: bool, + caller_allows_early_stop: bool, + ) -> Result { + let allow_read_version_coalescing = !crate::bucket::utils::is_meta_bucketname(bucket) + && crate::runtime::global::get_metadata_read_version_coalescing_service_ready(); + self.get_object_fileinfo_gated_inner( + bucket, + object, + opts, + read_data, + caller_allows_early_stop, + allow_read_version_coalescing, + ) + .await + } + /// Like `get_object_fileinfo`, but `allow_early_stop=false` forces the full /// quorum fanout. Read-before-write callers (object tagging) must use this: /// the returned online-disk set is the write target, and the early-stop @@ -275,6 +298,20 @@ impl SetDisks { opts: &ObjectOptions, read_data: bool, allow_early_stop: bool, + ) -> Result { + self.get_object_fileinfo_gated_inner(bucket, object, opts, read_data, allow_early_stop, false) + .await + } + + #[allow(clippy::too_many_arguments)] + async fn get_object_fileinfo_gated_inner( + &self, + bucket: &str, + object: &str, + opts: &ObjectOptions, + read_data: bool, + allow_early_stop: bool, + allow_read_version_coalescing: bool, ) -> Result { let vid = opts.version_id.clone().unwrap_or_default(); let stage_metrics_enabled = rustfs_io_metrics::get_stage_metrics_enabled(); @@ -337,19 +374,34 @@ impl SetDisks { // read_all_fileinfo_observed (see read_all_fileinfo_early_stop in // core/io_primitives.rs); unsafe requests and callers that opt out // (allow_early_stop=false) fall back to full-wait. - let (mut parts_metadata, errs, metadata_fanout_diagnostics) = Self::read_all_fileinfo_observed( - &disks, - "", - bucket, - object, - vid.as_str(), - read_data, - false, - opts.incl_free_versions, - allow_early_stop, - self.default_parity_count, - ) - .await?; + let (mut parts_metadata, errs, metadata_fanout_diagnostics) = if allow_read_version_coalescing { + Self::read_all_fileinfo_observed_for_get_object( + &disks, + "", + bucket, + object, + vid.as_str(), + read_data, + opts.incl_free_versions, + allow_early_stop, + self.default_parity_count, + ) + .await? + } else { + Self::read_all_fileinfo_observed( + &disks, + "", + bucket, + object, + vid.as_str(), + read_data, + false, + opts.incl_free_versions, + allow_early_stop, + self.default_parity_count, + ) + .await? + }; let metadata_metrics_path = if crate::bucket::utils::is_meta_bucketname(bucket) { GET_OBJECT_PATH_INTERNAL_META } else { diff --git a/crates/io-metrics/src/internode_metrics.rs b/crates/io-metrics/src/internode_metrics.rs index 65f93c2a1..6b1ea382c 100644 --- a/crates/io-metrics/src/internode_metrics.rs +++ b/crates/io-metrics/src/internode_metrics.rs @@ -54,6 +54,13 @@ pub const INTERNODE_STAGE_READ_VERSION_RESPONSE_JSON_ENCODE: &str = "read_versio pub const INTERNODE_STAGE_READ_VERSION_RESPONSE_MSGPACK_ENCODE: &str = "read_version_response_msgpack_encode"; pub const INTERNODE_STAGE_READ_VERSION_RPC_ROUNDTRIP: &str = "read_version_rpc_roundtrip"; pub const INTERNODE_STAGE_READ_VERSION_RESPONSE_DECODE: &str = "read_version_response_decode"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_ENCODE: &str = "batch_read_version_request_encode"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_DECODE: &str = "batch_read_version_request_decode"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_DISK_READ: &str = "batch_read_version_disk_read"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_JSON_ENCODE: &str = "batch_read_version_response_json_encode"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_MSGPACK_ENCODE: &str = "batch_read_version_response_msgpack_encode"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_RPC_ROUNDTRIP: &str = "batch_read_version_rpc_roundtrip"; +pub const INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_DECODE: &str = "batch_read_version_response_decode"; const OPERATION_LABEL: &str = "operation"; const BACKEND_LABEL: &str = "backend"; diff --git a/rustfs/src/server/readiness.rs b/rustfs/src/server/readiness.rs index d3954b7a7..8a934e06e 100644 --- a/rustfs/src/server/readiness.rs +++ b/rustfs/src/server/readiness.rs @@ -20,6 +20,7 @@ use crate::storage_api::server::readiness::contract::admin::StorageAdminApi; use crate::storage_api::server::readiness::{Endpoint, EndpointServerPools, is_dist_erasure}; #[cfg(test)] use crate::storage_api::server::readiness::{Endpoints, PoolEndpoints}; +use crate::storage_api::startup::shutdown::mark_get_metadata_read_version_coalescing_service_ready; use bytes::Bytes; use http::HeaderValue; use http::{Request as HttpRequest, Response, StatusCode}; @@ -212,6 +213,9 @@ where if readiness_gate_blocks_path(path, &readiness) { return Ok(service_not_ready_response(readiness.current_stage())); } + if !is_probe_path(path) && readiness.is_ready() { + mark_get_metadata_read_version_coalescing_service_ready(); + } let resp = inner.call(req).await?; // System is ready, forward to the actual S3/RPC handlers // Transparently converts any response body into a BoxBody, and then Trace/Cors/Compression continues to work @@ -232,6 +236,7 @@ pub async fn publish_ready_when_runtime_ready( collect_node_readiness, |dependency_readiness| { readiness.mark_stage(rustfs_common::SystemStage::FullReady); + mark_get_metadata_read_version_coalescing_service_ready(); if let Some(state_manager) = state_manager { state_manager.update(ServiceState::Ready); } diff --git a/rustfs/src/storage/rpc/node_service/disk.rs b/rustfs/src/storage/rpc/node_service/disk.rs index de80a10f0..617d0b80d 100644 --- a/rustfs/src/storage/rpc/node_service/disk.rs +++ b/rustfs/src/storage/rpc/node_service/disk.rs @@ -23,10 +23,12 @@ use bytes::Bytes; use rustfs_filemeta::FileInfo; use rustfs_io_metrics::internode_metrics::{ INTERNODE_MSGPACK_CODEC_JSON, INTERNODE_MSGPACK_CODEC_MSGPACK, INTERNODE_MSGPACK_DIRECTION_REQUEST, - INTERNODE_OPERATION_GRPC_READ_ALL, INTERNODE_OPERATION_GRPC_READ_VERSION, INTERNODE_OPERATION_GRPC_WRITE_ALL, - INTERNODE_STAGE_READ_VERSION_DISK_READ, INTERNODE_STAGE_READ_VERSION_REQUEST_DECODE, - INTERNODE_STAGE_READ_VERSION_RESPONSE_JSON_ENCODE, INTERNODE_STAGE_READ_VERSION_RESPONSE_MSGPACK_ENCODE, - INTERNODE_TRANSPORT_BACKEND_GRPC, global_internode_metrics, + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, INTERNODE_OPERATION_GRPC_READ_ALL, INTERNODE_OPERATION_GRPC_READ_VERSION, + INTERNODE_OPERATION_GRPC_WRITE_ALL, INTERNODE_STAGE_BATCH_READ_VERSION_DISK_READ, + INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_DECODE, INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_JSON_ENCODE, + INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_MSGPACK_ENCODE, INTERNODE_STAGE_READ_VERSION_DISK_READ, + INTERNODE_STAGE_READ_VERSION_REQUEST_DECODE, INTERNODE_STAGE_READ_VERSION_RESPONSE_JSON_ENCODE, + INTERNODE_STAGE_READ_VERSION_RESPONSE_MSGPACK_ENCODE, INTERNODE_TRANSPORT_BACKEND_GRPC, global_internode_metrics, }; use rustfs_protos::proto_gen::node_service::*; use serde::de::DeserializeOwned; @@ -242,24 +244,42 @@ fn record_read_version_stage(stage: &'static str, started_at: Option) { } } +fn record_batch_read_version_stage(stage: &'static str, started_at: Option) { + if let Some(started_at) = started_at { + global_internode_metrics().record_stage_duration_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + stage, + started_at.elapsed(), + ); + } +} + fn encode_batch_read_version_response_payloads( batch_read_version_resps: &[BatchReadVersionResp], request_decoded_from_msgpack: bool, ) -> std::result::Result<(Vec, Vec), DiskError> { + let attribution_enabled = rustfs_io_metrics::get_stage_metrics_enabled(); let mut batch_read_version_resps_json = Vec::with_capacity(batch_read_version_resps.len()); - let mut batch_read_version_resps_bin = Vec::with_capacity(batch_read_version_resps.len()); - + let json_encode_started = internode_stage_timer(attribution_enabled); for batch_read_version_resp in batch_read_version_resps { batch_read_version_resps_json.push( compat_response_json(batch_read_version_resp, request_decoded_from_msgpack) .map_err(|err| DiskError::other(format!("encode BatchReadVersionResp json failed: {err}")))?, ); + } + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_JSON_ENCODE, json_encode_started); + + let mut batch_read_version_resps_bin = Vec::with_capacity(batch_read_version_resps.len()); + let msgpack_encode_started = internode_stage_timer(attribution_enabled); + for batch_read_version_resp in batch_read_version_resps { batch_read_version_resps_bin.push(Bytes::from(encode_msgpack_with_capacity( batch_read_version_resp, "BatchReadVersionResp", FILE_INFO_MSGPACK_ENCODE_CAPACITY_HINT, )?)); } + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_RESPONSE_MSGPACK_ENCODE, msgpack_encode_started); Ok((batch_read_version_resps_json, batch_read_version_resps_bin)) } @@ -485,15 +505,37 @@ impl NodeService { &self, request: Request, ) -> Result, Status> { + let attribution_enabled = rustfs_io_metrics::get_stage_metrics_enabled(); let request = request.into_inner(); + if attribution_enabled { + let metrics = global_internode_metrics(); + metrics.record_incoming_request_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + ); + metrics.record_recv_bytes_for_operation_and_backend( + INTERNODE_OPERATION_GRPC_BATCH_READ_VERSION, + INTERNODE_TRANSPORT_BACKEND_GRPC, + request + .disk + .len() + .saturating_add(request.batch_read_version_req.len()) + .saturating_add(request.batch_read_version_req_bin.len()), + ); + } if let Some(disk) = self.find_disk(&request.disk).await { + let decode_started = internode_stage_timer(attribution_enabled); let decoded_batch_read_version_req: DecodedRpcPayload = match decode_msgpack_or_json_with_source( &request.batch_read_version_req_bin, &request.batch_read_version_req, "BatchReadVersionReq", ) { - Ok(batch_read_version_req) => batch_read_version_req, + Ok(batch_read_version_req) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_DECODE, decode_started); + batch_read_version_req + } Err(err) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_REQUEST_DECODE, decode_started); return Ok(Response::new(BatchReadVersionResponse { success: false, batch_read_version_resps: Vec::new(), @@ -514,8 +556,10 @@ impl NodeService { })); } + let disk_read_started = internode_stage_timer(attribution_enabled); match disk.batch_read_version(batch_read_version_req).await { Ok(batch_read_version_resps) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_DISK_READ, disk_read_started); let (batch_read_version_resps, batch_read_version_resps_bin) = match encode_batch_read_version_response_payloads(&batch_read_version_resps, request_decoded_from_msgpack) { @@ -537,12 +581,15 @@ impl NodeService { error: None, })) } - Err(err) => Ok(Response::new(BatchReadVersionResponse { - success: false, - batch_read_version_resps: Vec::new(), - batch_read_version_resps_bin: Vec::new(), - error: Some(err.into()), - })), + Err(err) => { + record_batch_read_version_stage(INTERNODE_STAGE_BATCH_READ_VERSION_DISK_READ, disk_read_started); + Ok(Response::new(BatchReadVersionResponse { + success: false, + batch_read_version_resps: Vec::new(), + batch_read_version_resps_bin: Vec::new(), + error: Some(err.into()), + })) + } } } else { Ok(Response::new(BatchReadVersionResponse { @@ -722,9 +769,9 @@ impl NodeService { &self, request: Request, ) -> Result, Status> { - let request = request.into_inner(); let metrics = global_internode_metrics(); let read_version_attribution_enabled = rustfs_io_metrics::get_stage_metrics_enabled(); + let request = request.into_inner(); if read_version_attribution_enabled { metrics.record_incoming_request_for_operation_and_backend( INTERNODE_OPERATION_GRPC_READ_VERSION, @@ -1635,6 +1682,7 @@ mod tests { encode_batch_read_version_response_payloads, encode_delete_versions_errors, encode_file_info_msgpack, encode_msgpack, encode_msgpack_named, encode_read_multiple_response_payloads, encode_rename_data_response_payloads, }; + use crate::storage::DiskError; use crate::storage::rpc::node_service::make_server; use crate::storage::storage_api::ReadMultipleResp; use crate::storage::storage_api::RenameDataResp; @@ -2028,7 +2076,8 @@ mod tests { path: "object-a".to_string(), version_id: "version-a".to_string(), success: false, - error: "file version not found".to_string(), + error: DiskError::FileVersionNotFound.to_string(), + error_code: DiskError::FileVersionNotFound.to_u32(), ..Default::default() }]; @@ -2044,8 +2093,43 @@ mod tests { .expect("msgpack batch read version response should decode"); assert_eq!(json_decoded.index, responses[0].index); + assert_eq!(json_decoded.error_code, responses[0].error_code); assert_eq!(msgpack_decoded.path, responses[0].path); assert_eq!(msgpack_decoded.error, responses[0].error); + assert_eq!(msgpack_decoded.error_code, responses[0].error_code); + } + + #[test] + fn batch_read_version_response_decode_accepts_legacy_payload_without_error_code() { + #[derive(Serialize)] + struct LegacyBatchReadVersionResp { + index: usize, + path: String, + version_id: String, + success: bool, + file_info: FileInfo, + error: String, + } + + let legacy = LegacyBatchReadVersionResp { + index: 2, + path: "object-legacy".to_string(), + version_id: "version-legacy".to_string(), + success: false, + file_info: FileInfo::default(), + error: "legacy error".to_string(), + }; + let legacy_json = serde_json::to_string(&legacy).expect("legacy json should encode"); + let legacy_msgpack = encode_msgpack(&legacy, "LegacyBatchReadVersionResp").expect("legacy msgpack should encode"); + + let json_decoded: BatchReadVersionResp = + decode_msgpack_or_json(&[], &legacy_json, "BatchReadVersionResp").expect("legacy json should decode"); + let msgpack_decoded: BatchReadVersionResp = + decode_msgpack_or_json(&legacy_msgpack, "", "BatchReadVersionResp").expect("legacy msgpack should decode"); + + assert_eq!(json_decoded.error_code, 0); + assert_eq!(msgpack_decoded.error_code, 0); + assert_eq!(msgpack_decoded.error, legacy.error); } #[test] diff --git a/rustfs/src/storage/storage_api.rs b/rustfs/src/storage/storage_api.rs index e5277c0dd..512e255ee 100644 --- a/rustfs/src/storage/storage_api.rs +++ b/rustfs/src/storage/storage_api.rs @@ -1100,6 +1100,10 @@ pub(crate) fn shutdown_background_monitors() { rustfs_ecstore::shutdown_background_monitors(); } +pub(crate) fn mark_get_metadata_read_version_coalescing_service_ready() { + rustfs_ecstore::mark_get_metadata_read_version_coalescing_service_ready(); +} + pub(crate) fn set_global_rustfs_port(value: u16) { ecstore_global::set_global_rustfs_port(value); } diff --git a/rustfs/src/storage_api.rs b/rustfs/src/storage_api.rs index e90c1f528..dea2923ac 100644 --- a/rustfs/src/storage_api.rs +++ b/rustfs/src/storage_api.rs @@ -284,7 +284,8 @@ pub(crate) mod startup { pub(crate) mod shutdown { pub(crate) use crate::storage::storage_api::{ - shutdown_background_monitors, shutdown_background_services, store_compression_total_in_backend, + mark_get_metadata_read_version_coalescing_service_ready, shutdown_background_monitors, shutdown_background_services, + store_compression_total_in_backend, }; } From 62c465eceff14a83988d2998558275f77595b4c6 Mon Sep 17 00:00:00 2001 From: cxymds Date: Sun, 23 Aug 2026 01:39:06 +0800 Subject: [PATCH 02/12] fix(heal): supervise scheduler task panics (#6351) From 87235ffd285d859cf7403d6d94d34998e1cac7c0 Mon Sep 17 00:00:00 2001 From: cxymds Date: Sun, 23 Aug 2026 01:40:03 +0800 Subject: [PATCH 03/12] perf(ecstore): bound decommission entry workers (#6360) --- crates/ecstore/src/core/pools.rs | 1270 +++++++++++++++++++----- crates/ecstore/src/runtime/instance.rs | 9 + 2 files changed, 1019 insertions(+), 260 deletions(-) diff --git a/crates/ecstore/src/core/pools.rs b/crates/ecstore/src/core/pools.rs index 90ad40464..9d8e0d01a 100644 --- a/crates/ecstore/src/core/pools.rs +++ b/crates/ecstore/src/core/pools.rs @@ -75,7 +75,7 @@ use std::sync::{ atomic::{AtomicBool, AtomicUsize, Ordering}, }; use time::{Duration, OffsetDateTime}; -use tokio::sync::{OwnedSemaphorePermit, Semaphore}; +use tokio::sync::{OwnedSemaphorePermit, Semaphore, mpsc}; use tokio_util::sync::CancellationToken; use tracing::{debug, error, info, warn}; @@ -93,6 +93,10 @@ const DECOMMISSION_PROGRESS_SAVE_ITEM_THRESHOLD: usize = 1000; const DECOMMISSION_PROGRESS_SAVE_RETRY_BACKOFF: Duration = Duration::seconds(1); const DECOMMISSION_BUCKET_CONCURRENCY_ENV: &str = "RUSTFS_DECOMMISSION_BUCKET_CONCURRENCY"; const DECOMMISSION_BUCKET_CONCURRENCY_DEFAULT_CAP: usize = 4; +const DECOMMISSION_ENTRY_CONCURRENCY_ENV: &str = "RUSTFS_DECOMMISSION_ENTRY_CONCURRENCY"; +const DECOMMISSION_ENTRY_CONCURRENCY_DEFAULT_CAP: usize = 8; +const DECOMMISSION_ENTRY_CONCURRENCY_HARD_CAP: usize = 64; +const DECOMMISSION_ENTRY_WORKERS_PER_SET: usize = 2; const DECOMMISSION_TARGET_CAPACITY_OVERHEAD_PERCENT: usize = 30; const DECOMMISSION_LISTING_MAX_ATTEMPTS: usize = 3; const DECOMMISSION_LISTING_RETRY_DELAY: std::time::Duration = std::time::Duration::from_secs(5); @@ -356,6 +360,19 @@ fn decommission_bucket_concurrency_limit() -> usize { rustfs_utils::get_env_usize(DECOMMISSION_BUCKET_CONCURRENCY_ENV, default_limit).max(1) } +fn default_decommission_entry_concurrency(cpu_count: usize) -> usize { + cpu_count.clamp(1, DECOMMISSION_ENTRY_CONCURRENCY_DEFAULT_CAP) +} + +fn clamp_decommission_entry_concurrency(limit: usize) -> usize { + limit.clamp(1, DECOMMISSION_ENTRY_CONCURRENCY_HARD_CAP) +} + +fn decommission_entry_concurrency_limit() -> usize { + let default_limit = default_decommission_entry_concurrency(num_cpus::get()); + clamp_decommission_entry_concurrency(rustfs_utils::get_env_usize(DECOMMISSION_ENTRY_CONCURRENCY_ENV, default_limit)) +} + fn is_decommission_meta_bucket(bucket: &DecomBucketInfo) -> bool { bucket.name == RUSTFS_META_BUCKET } @@ -521,6 +538,7 @@ fn spawn_decommission_index_cancelers( store: Arc, rx: CancellationToken, index_cancelers: Vec<(usize, DecommissionCancelerGuard)>, + entry_budget: Arc, ) -> tokio::task::JoinHandle<()> { tokio::spawn(async move { let mut stop_queue = false; @@ -536,7 +554,8 @@ fn spawn_decommission_index_cancelers( let worker = tokio::spawn({ let store = store.clone(); let canceler = canceler.clone(); - async move { store.do_decommission_in_routine(canceler, idx).await } + let entry_budget = entry_budget.clone(); + async move { store.do_decommission_in_routine(canceler, idx, entry_budget).await } }); if let Err(err) = await_decommission_worker(idx, worker).await { error!( @@ -770,6 +789,47 @@ fn count_decommission_item(meta: &mut PoolMeta, idx: usize, size: usize, failed: Ok(()) } +fn ensure_decommission_generation(meta: &PoolMeta, idx: usize, generation: OffsetDateTime) -> Result<()> { + let Some(pool) = meta.pools.get(idx) else { + return Err(invalid_decommission_pool_index_error(meta.pools.len(), idx)); + }; + let Some(info) = pool.decommission.as_ref() else { + return Err(decommission_metadata_not_initialized_error("check decommission generation")); + }; + + if info.start_time == Some(generation) && !info.queued && is_decommission_active(info.complete, info.failed, info.canceled) { + Ok(()) + } else { + Err(Error::OperationCanceled) + } +} + +async fn run_decommission_side_effect( + rx: &CancellationToken, + operation_gate: &Arc>, + operation: F, +) -> Result +where + F: FnOnce() -> Fut, + Fut: std::future::Future>, +{ + let _operation_guard = tokio::select! { + biased; + _ = rx.cancelled() => return Err(Error::OperationCanceled), + guard = operation_gate.read() => guard, + }; + + if rx.is_cancelled() { + return Err(Error::OperationCanceled); + } + + let result = operation().await; + if rx.is_cancelled() { + return Err(Error::OperationCanceled); + } + result +} + fn track_decommission_current_object_stage( meta: &mut PoolMeta, idx: usize, @@ -962,22 +1022,6 @@ fn resolve_decommission_partial_listing_entry( )) } -async fn record_decommission_entry_error( - entry_error: &Arc>>, - rx: &CancellationToken, - err: Error, -) { - if rx.is_cancelled() { - return; - } - - let mut first_err = entry_error.lock().await; - if first_err.is_none() && !rx.is_cancelled() { - *first_err = Some(err); - rx.cancel(); - } -} - fn resolve_decommission_pool_meta_reload_result(result: Result<()>, stage: &str) -> Result<()> { result.map_err(|err| Error::other(format!("decommission pool meta reload failed during {stage}: {err}"))) } @@ -1110,6 +1154,7 @@ async fn wait_decommission_listing_retry(rx: &CancellationToken, delay: std::tim } } +#[cfg(test)] async fn run_decommission_listing_with_retry( rx: CancellationToken, bucket: String, @@ -1117,11 +1162,31 @@ async fn run_decommission_listing_with_retry( pool_idx: usize, set_idx: usize, max_attempts: usize, - mut list: List, + list: List, ) -> Result<()> where List: FnMut(ListCallback) -> ListFuture, ListFuture: std::future::Future>, +{ + run_decommission_listing_with_retry_and_drain(rx, bucket, cb, pool_idx, set_idx, max_attempts, list, || async { false }).await +} + +#[allow(clippy::too_many_arguments)] +async fn run_decommission_listing_with_retry_and_drain( + rx: CancellationToken, + bucket: String, + cb: ListCallback, + pool_idx: usize, + set_idx: usize, + max_attempts: usize, + mut list: List, + mut drain: Drain, +) -> Result<()> +where + List: FnMut(ListCallback) -> ListFuture, + ListFuture: std::future::Future>, + Drain: FnMut() -> DrainFuture, + DrainFuture: std::future::Future, { let max_attempts = max_attempts.max(1); @@ -1153,7 +1218,12 @@ where "Decommission listing started" ); - match list(cb.clone()).await { + let list_result = list(cb.clone()).await; + if drain().await { + return Ok(()); + } + + match list_result { Ok(()) => { debug!( event = EVENT_DECOMMISSION_BUCKET, @@ -1385,6 +1455,7 @@ where Ok(()) } +#[cfg(test)] async fn wait_decommission_worker_drain(workers: &Semaphore, limit: usize) -> Result<()> { let permits = u32::try_from(limit) .map_err(|_| Error::other(format!("decommission worker limit {limit} exceeds semaphore drain capacity")))?; @@ -2058,7 +2129,7 @@ impl PoolMeta { pub fn decommission_failed(&mut self, idx: usize) -> bool { if let Some(stats) = self.pools.get_mut(idx) { if let Some(d) = &stats.decommission { - if !d.failed { + if is_decommission_active(d.complete, d.failed, d.canceled) { stats.last_update = OffsetDateTime::now_utc(); let mut pd = d.clone(); @@ -2106,7 +2177,7 @@ impl PoolMeta { pub fn decommission_complete(&mut self, idx: usize) -> bool { if let Some(stats) = self.pools.get_mut(idx) { if let Some(d) = &stats.decommission { - if !d.complete { + if is_decommission_active(d.complete, d.failed, d.canceled) { stats.last_update = OffsetDateTime::now_utc(); let mut pd = d.clone(); @@ -2759,12 +2830,13 @@ impl ECStore { snapshot.save(self.pools.clone()).await } - async fn save_decommission_progress_checkpoint(&self, idx: usize) -> Result { + async fn save_decommission_progress_checkpoint(&self, idx: usize, generation: OffsetDateTime) -> Result { // Lock order: save gate, then the short pool metadata read/write sections. Peer // reloads are intentionally performed by the caller after both locks are released. let _save_guard = self.pool_meta_save_gate.lock().await; let (snapshot, checkpoint) = { let pool_meta = self.pool_meta.read().await; + ensure_decommission_generation(&pool_meta, idx, generation)?; let Some(checkpoint) = pool_meta.decommission_progress_checkpoint( idx, DECOMMISSION_PROGRESS_SAVE_INTERVAL, @@ -3078,6 +3150,8 @@ impl ECStore { ); } + self.wait_for_decommission_side_effects().await; + if should_save_pool_meta && let Err(err) = self.save_current_pool_meta().await { if let Some(previous_pool_meta) = previous_pool_meta { let mut pool_meta = self.pool_meta.write().await; @@ -3103,6 +3177,22 @@ impl ECStore { ensure_decommission_terminal_operation_supported(self.single_pool(), "clear decommission")?; let _start_guard = self.start_gate.lock().await; + { + let pool_meta = self.pool_meta.read().await; + let pool_count = pool_meta.pools.len(); + ensure_valid_decommission_pool_index(pool_count, idx)?; + let Some(pool) = pool_meta.pools.get(idx) else { + return Err(invalid_decommission_pool_index_error(pool_count, idx)); + }; + let (decommission_present, complete, failed, canceled) = pool + .decommission + .as_ref() + .map(|info| (info.has_decommission_state(), info.complete, info.failed, info.canceled)) + .unwrap_or((false, false, false, false)); + ensure_decommission_clear_allowed(true, decommission_present, complete, failed, canceled)?; + } + self.cancel_decommission_routines_and_wait(&[idx]).await; + let (should_reload_pool_meta, previous_pool_meta) = { let mut pool_meta = self.pool_meta.write().await; let previous_pool_meta = pool_meta.clone(); @@ -3118,11 +3208,6 @@ impl ECStore { return Err(err); } - { - let mut cancelers = self.decommission_cancelers.write().await; - take_and_cancel_decommission_canceler(cancelers.as_mut_slice(), idx); - } - if should_reload_pool_meta && let Some(notification_sys) = runtime_sources::notification_sys() { let stage = format!("clear_decommission for pool {idx}"); resolve_decommission_pool_meta_reload_result(notification_sys.reload_pool_meta().await, stage.as_str())?; @@ -3131,21 +3216,51 @@ impl ECStore { Ok(()) } - async fn promote_queued_decommission(&self, idx: usize) -> Result<()> { - let promoted = { + async fn promote_queued_decommission(&self, idx: usize, owner: &DecommissionCanceler) -> Result { + // Serialize promotion and generation capture with clear/restart transitions. + let (promoted, generation, save_error) = { + let _start_guard = self.start_gate.lock().await; let mut pool_meta = self.pool_meta.write().await; - pool_meta.promote_queued_decommission(idx) + if pool_meta.pools.get(idx).is_none() { + return Err(Error::other("failed to start decommission: target pool was not found")); + } + let promoted = pool_meta.promote_queued_decommission(idx); + drop(pool_meta); + + let save_error = if promoted { + self.save_current_pool_meta().await.err() + } else { + None + }; + + let generation = self.active_decommission_generation(idx).await?; + (promoted, generation, save_error) }; - if promoted { - self.save_current_pool_meta().await?; - if let Some(notification_sys) = runtime_sources::notification_sys() { - let stage = format!("promote_queued_decommission for pool {idx}"); - resolve_decommission_pool_meta_reload_result(notification_sys.reload_pool_meta().await, stage.as_str())?; + if let Some(err) = save_error { + resolve_decommission_terminal_mark_after_error_result( + self.decommission_failed_for_operation(idx, owner).await, + idx, + &err, + )?; + return Err(err); + } + + if promoted && let Some(notification_sys) = runtime_sources::notification_sys() { + let stage = format!("promote_queued_decommission for pool {idx}"); + if let Err(err) = + resolve_decommission_pool_meta_reload_result(notification_sys.reload_pool_meta().await, stage.as_str()) + { + resolve_decommission_terminal_mark_after_error_result( + self.decommission_failed_for_operation(idx, owner).await, + idx, + &err, + )?; + return Err(err); } } - Ok(()) + Ok(generation) } async fn record_decommission_terminal_reload_failure(&self, idx: usize, stage: &str, err: Error) -> Result<()> { @@ -3189,6 +3304,21 @@ impl ECStore { is_decommission_cancel_requested(rx.is_cancelled(), pool_meta.pools.get(idx)) } + async fn cancel_decommission_routines_and_wait(&self, indices: &[usize]) { + { + let mut cancelers = self.decommission_cancelers.write().await; + for idx in indices { + take_and_cancel_decommission_canceler(cancelers.as_mut_slice(), *idx); + } + } + self.wait_for_decommission_side_effects().await; + } + + async fn wait_for_decommission_side_effects(&self) { + let operation_gate = self.ctx.decommission_operation_gate(); + let _operation_guard = operation_gate.write().await; + } + async fn reserve_decommission_routines( &self, rx: &CancellationToken, @@ -3233,7 +3363,12 @@ impl ECStore { ) -> Result<()> { let index_cancelers = self.reserve_decommission_routines(&rx, indices.as_slice()).await?; if !index_cancelers.is_empty() { - std::mem::drop(spawn_decommission_index_cancelers(store, rx, index_cancelers)); + std::mem::drop(spawn_decommission_index_cancelers( + store, + rx, + index_cancelers, + Arc::new(Semaphore::new(decommission_entry_concurrency_limit())), + )); } Ok(()) @@ -3255,7 +3390,12 @@ impl ECStore { return Ok(()); } - std::mem::drop(spawn_decommission_index_cancelers(self.clone(), rx, index_cancelers)); + std::mem::drop(spawn_decommission_index_cancelers( + self.clone(), + rx, + index_cancelers, + Arc::new(Semaphore::new(decommission_entry_concurrency_limit())), + )); Ok(()) } @@ -3280,20 +3420,344 @@ impl ECStore { let index_cancelers = self .start_decommission_with_routines(indices, &rx, local_indices.as_slice()) .await?; - std::mem::drop(spawn_decommission_index_cancelers(store, rx, index_cancelers)); + std::mem::drop(spawn_decommission_index_cancelers( + store, + rx, + index_cancelers, + Arc::new(Semaphore::new(decommission_entry_concurrency_limit())), + )); Ok(()) } + async fn active_decommission_generation(&self, idx: usize) -> Result { + let pool_meta = self.pool_meta.read().await; + let Some(pool) = pool_meta.pools.get(idx) else { + return Err(invalid_decommission_pool_index_error(pool_meta.pools.len(), idx)); + }; + let Some(info) = pool.decommission.as_ref() else { + return Err(decommission_metadata_not_initialized_error("load decommission generation")); + }; + let Some(generation) = info.start_time else { + return Err(Error::OperationCanceled); + }; + ensure_decommission_generation(&pool_meta, idx, generation)?; + Ok(generation) + } + + async fn ensure_decommission_generation_current(&self, idx: usize, generation: OffsetDateTime) -> Result<()> { + let pool_meta = self.pool_meta.read().await; + ensure_decommission_generation(&pool_meta, idx, generation) + } + + #[allow(clippy::too_many_arguments)] + async fn decommission_entry_worker( + self: Arc, + rx: CancellationToken, + idx: usize, + set_idx: usize, + generation: OffsetDateTime, + bucket: String, + set: Arc, + lifecycle_config: Option, + object_lock_config: Option, + replication_config: Option<(ReplicationConfiguration, OffsetDateTime)>, + expected_bucket_incarnation_id: Option, + entry_budget: Arc, + queue: Arc>>, + entry_error: Arc>>, + ) { + loop { + let queued = tokio::select! { + biased; + _ = rx.cancelled() => return, + item = async { + let mut queue = queue.lock().await; + queue.recv().await + } => item, + }; + let Some(QueuedDecommissionEntry { entry, queue_permit }) = queued else { + return; + }; + let object_name = entry.name.clone(); + + if entry_error.lock().await.is_some() { + drop(queue_permit); + continue; + } + + if let Err(err) = self.ensure_decommission_generation_current(idx, generation).await { + if matches!(err, Error::OperationCanceled) { + rx.cancel(); + } else { + record_decommission_entry_error(&entry_error, &rx, err).await; + } + return; + } + + if let Err(err) = backpressure::wait_for_data_movement_admission(DataMovementOperation::Decommission, idx, &rx).await + { + if matches!(err, Error::OperationCanceled) { + return; + } + error!( + event = EVENT_DECOMMISSION_ENTRY, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_POOLS, + pool_index = idx, + set_index = set_idx, + bucket = %bucket, + object = %object_name, + state = "entry_admission_failed", + error = %err, + "Decommission entry admission failed" + ); + record_decommission_entry_error(&entry_error, &rx, err).await; + return; + } + + let entry_budget_permit = match tokio::select! { + biased; + _ = rx.cancelled() => return, + permit = entry_budget.clone().acquire_owned() => permit, + } { + Ok(permit) => permit, + Err(err) => { + let err = Error::other(format!("decommission entry budget permit acquire failed: {err}")); + error!( + event = EVENT_DECOMMISSION_ENTRY, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_POOLS, + pool_index = idx, + set_index = set_idx, + bucket = %bucket, + object = %object_name, + state = "entry_budget_acquire_failed", + error = %err, + "Decommission entry budget permit acquire failed" + ); + record_decommission_entry_error(&entry_error, &rx, err).await; + return; + } + }; + + let result = self + .decommission_entry( + rx.clone(), + idx, + generation, + entry, + bucket.clone(), + set.clone(), + lifecycle_config.clone(), + object_lock_config.clone(), + replication_config.clone(), + expected_bucket_incarnation_id, + ) + .await; + drop(entry_budget_permit); + drop(queue_permit); + + if let Err(err) = result { + error!( + event = EVENT_DECOMMISSION_ENTRY, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_POOLS, + pool_index = idx, + set_index = set_idx, + bucket = %bucket, + object = %object_name, + state = "entry_failed", + error = %err, + "Decommission entry failed" + ); + record_decommission_entry_error(&entry_error, &rx, err).await; + return; + } + } + } + + #[allow(clippy::too_many_arguments)] + async fn decommission_set( + self: Arc, + rx: CancellationToken, + idx: usize, + set_idx: usize, + generation: OffsetDateTime, + set: Arc, + bi: DecomBucketInfo, + lifecycle_config: Option, + object_lock_config: Option, + replication_config: Option<(ReplicationConfiguration, OffsetDateTime)>, + expected_bucket_incarnation_id: Option, + entry_budget: Arc, + entry_error: Arc>>, + ) -> Result<()> { + let worker_count = DECOMMISSION_ENTRY_WORKERS_PER_SET; + let queue_capacity = decommission_entry_queue_capacity(worker_count); + let outstanding_capacity = queue_capacity.saturating_add(worker_count); + let outstanding = Arc::new(Semaphore::new(outstanding_capacity)); + let (tx, rx_queue) = mpsc::channel(queue_capacity); + let queue = Arc::new(tokio::sync::Mutex::new(rx_queue)); + + let mut entry_workers = tokio::task::JoinSet::new(); + for _ in 0..worker_count { + let this = self.clone(); + let rx = rx.clone(); + let bucket = bi.name.clone(); + let set = set.clone(); + let lifecycle_config = lifecycle_config.clone(); + let object_lock_config = object_lock_config.clone(); + let replication_config = replication_config.clone(); + let queue = queue.clone(); + let entry_budget = entry_budget.clone(); + let entry_error = entry_error.clone(); + entry_workers.spawn(async move { + this.decommission_entry_worker( + rx, + idx, + set_idx, + generation, + bucket, + set, + lifecycle_config, + object_lock_config, + replication_config, + expected_bucket_incarnation_id, + entry_budget, + queue, + entry_error, + ) + .await; + }); + } + + let callback: ListCallback = Arc::new({ + let tx = tx.clone(); + let outstanding = outstanding.clone(); + let callback_rx = rx.clone(); + let entry_error = entry_error.clone(); + let bucket = bi.name.clone(); + move |entry: MetaCacheEntry| { + let tx = tx.clone(); + let outstanding = outstanding.clone(); + let callback_rx = callback_rx.clone(); + let entry_error = entry_error.clone(); + let bucket = bucket.clone(); + Box::pin(async move { + if callback_rx.is_cancelled() || entry_error.lock().await.is_some() { + return; + } + + if matches!( + enqueue_decommission_entry(&callback_rx, &outstanding, &tx, entry).await, + DecommissionEntryEnqueueResult::Closed + ) { + let err = Error::other("decommission entry queue closed"); + error!( + event = EVENT_DECOMMISSION_ENTRY, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_POOLS, + pool_index = idx, + set_index = set_idx, + bucket = %bucket, + state = "entry_queue_closed", + error = %err, + "Decommission entry queue closed" + ); + record_decommission_entry_error(&entry_error, &callback_rx, err).await; + } + }) + } + }); + + let list_set = set.clone(); + let list_rx = rx.clone(); + let list_rx_for_list = list_rx.clone(); + let list_rx_for_drain = list_rx.clone(); + let list_bi = bi.clone(); + let list_outstanding = outstanding.clone(); + let list_entry_error = entry_error.clone(); + let mut listing = tokio::spawn(async move { + run_decommission_listing_with_retry_and_drain( + list_rx.clone(), + list_bi.name.clone(), + callback, + idx, + set_idx, + DECOMMISSION_LISTING_MAX_ATTEMPTS, + move |callback| { + let set = list_set.clone(); + let rx = list_rx_for_list.clone(); + let bucket = list_bi.clone(); + let entry_error = list_entry_error.clone(); + async move { + set.list_objects_to_decommission(rx, bucket, callback, entry_error, idx, set_idx) + .await + } + }, + move || { + let rx = list_rx_for_drain.clone(); + let outstanding = list_outstanding.clone(); + async move { drain_decommission_entry_queue(&rx, &outstanding, outstanding_capacity).await } + }, + ) + .await + }); + + let mut listing_result = None; + let mut workers_left = worker_count; + let mut sender = Some(tx); + while listing_result.is_none() || workers_left > 0 { + tokio::select! { + biased; + result = &mut listing, if listing_result.is_none() => { + let result = resolve_decommission_listing_worker_result(set_idx, result); + if result.is_err() { + rx.cancel(); + } + listing_result = Some(result); + drop(sender.take()); + } + worker_result = entry_workers.join_next(), if workers_left > 0 => { + workers_left -= 1; + if let Some(Err(err)) = worker_result { + let err = Error::other(format!("decommission entry worker {set_idx} task join error: {err}")); + error!( + event = EVENT_DECOMMISSION_ENTRY, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_POOLS, + pool_index = idx, + set_index = set_idx, + bucket = %bi.name, + state = "entry_worker_join_failed", + error = %err, + "Decommission entry worker task failed" + ); + record_decommission_entry_error(&entry_error, &rx, err).await; + } + } + } + } + + let listing_result = listing_result.unwrap_or_else(|| Err(Error::other("decommission listing task did not complete"))); + if let Some(err) = entry_error.lock().await.clone() { + return Err(err); + } + listing_result + } + async fn track_decommission_entry_progress_stage( &self, idx: usize, + generation: OffsetDateTime, bucket: &str, object: &str, stage: &'static str, ) -> Result<()> { { let mut pool_meta = self.pool_meta.write().await; + ensure_decommission_generation(&pool_meta, idx, generation)?; track_decommission_current_object_stage(&mut pool_meta, idx, bucket, object, stage) .map_err(|err| with_decommission_entry_context(stage, bucket, object, err))?; } @@ -3302,15 +3766,15 @@ impl ECStore { } #[allow(unused_assignments, clippy::too_many_arguments)] - #[tracing::instrument(skip(self, set, _worker_permit, lifecycle_config, object_lock_config, replication_config))] + #[tracing::instrument(skip(self, set, lifecycle_config, object_lock_config, replication_config))] async fn decommission_entry( self: &Arc, rx: CancellationToken, idx: usize, + generation: OffsetDateTime, entry: MetaCacheEntry, bucket: String, set: Arc, - _worker_permit: OwnedSemaphorePermit, lifecycle_config: Option, object_lock_config: Option, replication_config: Option<(ReplicationConfiguration, OffsetDateTime)>, @@ -3343,6 +3807,8 @@ impl ECStore { rx.cancel(); } decommission_cancel_signal_result(rx.is_cancelled())?; + self.ensure_decommission_generation_current(idx, generation).await?; + let operation_gate = self.ctx.decommission_operation_gate(); let bucket_incarnation_fence = match expected_bucket_incarnation_id { Some(expected) => Some(self.acquire_bucket_incarnation_fence(&bucket, expected).await?), @@ -3364,15 +3830,18 @@ impl ECStore { } decommission_cancel_signal_result(rx.is_cancelled())?; - if should_skip_lifecycle_for_data_movement( - self.clone(), - &bucket, - version, - lifecycle_config.as_ref(), - object_lock_config.as_ref(), - true, - &LcEventSrc::Decom, - ) + if run_decommission_side_effect(&rx, &operation_gate, || async { + should_skip_lifecycle_for_data_movement( + self.clone(), + &bucket, + version, + lifecycle_config.as_ref(), + object_lock_config.as_ref(), + true, + &LcEventSrc::Decom, + ) + .await + }) .await .map_err(|err| with_decommission_entry_context("lifecycle_expiry", bucket.as_str(), version.name.as_str(), err))? { @@ -3405,13 +3874,15 @@ impl ECStore { let mut failure = false; let mut error = None; if version.deleted { - if let Err(err) = self - .delete_object( + if let Err(err) = run_decommission_side_effect(&rx, &operation_gate, || async { + self.delete_object( bucket.as_str(), &version.name, decommission_delete_marker_opts(version, version_id.clone(), idx, expected_bucket_incarnation_id), ) .await + }) + .await { if is_decommission_copy_cleanup_safe_error(&err) { warn!( @@ -3463,6 +3934,7 @@ impl ECStore { { let mut pool_meta = self.pool_meta.write().await; + ensure_decommission_generation(&pool_meta, idx, generation)?; if let Err(err) = count_decommission_item(&mut pool_meta, idx, 0, failure) { return Err(with_decommission_entry_context( "count_decommission_item", @@ -3494,14 +3966,16 @@ impl ECStore { for _i in 0..3 { if version.is_remote() { - if let Err(err) = self - .decommission_tiered_object( + if let Err(err) = run_decommission_side_effect(&rx, &operation_gate, || async { + self.decommission_tiered_object( bucket.as_str(), &version.name, version, &decommission_remote_tiered_opts(version, version_id.clone(), idx, expected_bucket_incarnation_id), ) .await + }) + .await { if is_decommission_copy_cleanup_safe_error(&err) { ignore = true; @@ -3565,16 +4039,19 @@ impl ECStore { self.track_decommission_entry_progress_stage( idx, + generation, bucket_name.as_str(), object_name.as_str(), DECOMMISSION_STAGE_MIGRATE_OBJECT, ) .await?; - if let Err(err) = self - .clone() - .decommission_object(idx, bucket, rd, expected_bucket_incarnation_id) - .await + if let Err(err) = run_decommission_side_effect(&rx, &operation_gate, || async { + self.clone() + .decommission_object(idx, bucket, rd, expected_bucket_incarnation_id) + .await + }) + .await { if is_decommission_copy_cleanup_safe_error(&err) { ignore = true; @@ -3632,6 +4109,7 @@ impl ECStore { { let mut pool_meta = self.pool_meta.write().await; + ensure_decommission_generation(&pool_meta, idx, generation)?; if let Err(err) = count_decommission_item(&mut pool_meta, idx, decommission_item_size(version.size), failure) { return Err(with_decommission_entry_context( "count_decommission_item", @@ -3656,9 +4134,11 @@ impl ECStore { return Err(Error::other("decommission bucket incarnation fence was lost before source cleanup")); } decommission_cancel_signal_result(rx.is_cancelled())?; + self.ensure_decommission_generation_current(idx, generation).await?; self.track_decommission_entry_progress_stage( idx, + generation, bucket.as_str(), entry.name.as_str(), DECOMMISSION_STAGE_CLEANUP_PREFLIGHT, @@ -3667,6 +4147,7 @@ impl ECStore { self.track_decommission_entry_progress_stage( idx, + generation, bucket.as_str(), entry.name.as_str(), DECOMMISSION_STAGE_SOURCE_CLEANUP, @@ -3676,29 +4157,32 @@ impl ECStore { let source_cleanup_mutation_fence = self .acquire_decommission_source_cleanup_fence(bucket.as_str(), entry.name.as_str(), set.as_ref()) .await?; - let cleanup_result = data_movement::cleanup_source_entry_if_unchanged( - set.clone(), - bucket.as_str(), - entry.name.as_str(), - &fivs, - &cleanup_preflight_allowed_missing, - data_movement::SourceCleanupBucketFence { - expected_incarnation_id: expected_bucket_incarnation_id, - lifecycle_guard: bucket_incarnation_fence - .as_ref() - .and_then(|guard| guard.namespace_lock_guard()), - object_mutation_fence: Some(&source_cleanup_mutation_fence), - }, - "decommission", - ) - .await - .map_err(|err| match err { - data_movement::SourceCleanupError::SourceChanged => Error::other(format!( - "decommission: source cleanup preflight failed for {}/{}: source versions changed after migration started", - bucket, entry.name - )), - data_movement::SourceCleanupError::Storage(err) => err, - }); + let cleanup_result = run_decommission_side_effect(&rx, &operation_gate, || async { + data_movement::cleanup_source_entry_if_unchanged( + set.clone(), + bucket.as_str(), + entry.name.as_str(), + &fivs, + &cleanup_preflight_allowed_missing, + data_movement::SourceCleanupBucketFence { + expected_incarnation_id: expected_bucket_incarnation_id, + lifecycle_guard: bucket_incarnation_fence + .as_ref() + .and_then(|guard| guard.namespace_lock_guard()), + object_mutation_fence: Some(&source_cleanup_mutation_fence), + }, + "decommission", + ) + .await + .map_err(|err| match err { + data_movement::SourceCleanupError::SourceChanged => Error::other(format!( + "decommission: source cleanup preflight failed for {}/{}: source versions changed after migration started", + bucket, entry.name + )), + data_movement::SourceCleanupError::Storage(err) => err, + }) + }) + .await; resolve_decommission_entry_cleanup_delete_result(cleanup_result, bucket.as_str(), entry.name.as_str())? } else if decommissioned != fivs.versions.len() || expired > 0 { warn!( @@ -3718,6 +4202,7 @@ impl ECStore { let should_save_progress = { let mut pool_meta = self.pool_meta.write().await; + ensure_decommission_generation(&pool_meta, idx, generation)?; if let Err(err) = track_decommission_current_object(&mut pool_meta, idx, bucket.as_str(), entry.name.as_str()) { return Err(with_decommission_entry_context( @@ -3738,6 +4223,7 @@ impl ECStore { self.track_decommission_entry_progress_stage( idx, + generation, bucket.as_str(), entry.name.as_str(), DECOMMISSION_STAGE_ENTRY_FINISHED, @@ -3745,7 +4231,7 @@ impl ECStore { .await?; if should_save_progress { - match self.save_decommission_progress_checkpoint(idx).await { + match self.save_decommission_progress_checkpoint(idx, generation).await { Ok(true) => { if let Some(notification_sys) = runtime_sources::notification_sys() && let Err(err) = resolve_decommission_entry_reload_result( @@ -3797,12 +4283,19 @@ impl ECStore { bucket: String, set: Arc, ) -> Result<()> { - let worker_permit = Arc::new(Semaphore::new(1)) - .acquire_owned() - .await - .map_err(|err| Error::other(format!("decommission test worker permit acquire failed: {err}")))?; - self.decommission_entry(CancellationToken::new(), idx, entry, bucket, set, worker_permit, None, None, None, None) - .await + self.decommission_entry( + CancellationToken::new(), + idx, + OffsetDateTime::now_utc(), + entry, + bucket, + set, + None, + None, + None, + None, + ) + .await } #[tracing::instrument(skip(self, rx))] @@ -3812,13 +4305,10 @@ impl ECStore { idx: usize, pool: Arc, bi: DecomBucketInfo, + entry_budget: Arc, ) -> Result<()> { - let worker_limit = pool.disk_set.len() * 2; - if worker_limit == 0 { - return Err(Error::other("decommission worker limit must be greater than zero")); - } - let workers = Arc::new(Semaphore::new(worker_limit)); let entry_error = Arc::new(tokio::sync::Mutex::new(None::)); + let generation = self.active_decommission_generation(idx).await?; let mut listing_workers = Vec::with_capacity(pool.disk_set.len()); let mut lifecycle_config = None; @@ -3847,12 +4337,6 @@ impl ECStore { } for (set_idx, set) in pool.disk_set.iter().enumerate() { - let listing_permit = workers - .clone() - .acquire_owned() - .await - .map_err(|err| Error::other(format!("decommission listing worker permit acquire failed: {err}")))?; - debug!( event = EVENT_DECOMMISSION_BUCKET, component = LOG_COMPONENT_ECSTORE, @@ -3864,130 +4348,34 @@ impl ECStore { "Decommission listing worker started" ); - let decommission_entry: ListCallback = Arc::new({ - let this = Arc::clone(self); - let bucket = bi.name.clone(); - let workers = workers.clone(); - let set = set.clone(); - let lifecycle_config = lifecycle_config.clone(); - let object_lock_config = object_lock_config.clone(); - let replication_config = replication_config.clone(); - let entry_error = entry_error.clone(); - let callback_rx = rx.clone(); - move |entry: MetaCacheEntry| { - let this = this.clone(); - let bucket = bucket.clone(); - let workers = workers.clone(); - let set = set.clone(); - let lifecycle_config = lifecycle_config.clone(); - let object_lock_config = object_lock_config.clone(); - let replication_config = replication_config.clone(); - let expected_bucket_incarnation_id = expected_bucket_incarnation_id; - let entry_error = entry_error.clone(); - let callback_rx = callback_rx.clone(); - - Box::pin(async move { - if callback_rx.is_cancelled() { - return; - } - if entry_error.lock().await.is_some() { - return; - } - - if let Err(err) = - backpressure::wait_for_data_movement_admission(DataMovementOperation::Decommission, idx, &callback_rx) - .await - { - if matches!(err, Error::OperationCanceled) { - return; - } - error!("decommission_pool: data movement admission failed: {err}"); - let mut first_err = entry_error.lock().await; - if first_err.is_none() { - *first_err = Some(err); - callback_rx.cancel(); - } - return; - } - - if entry_error.lock().await.is_some() { - return; - } - - let worker_permit = match tokio::select! { - _ = callback_rx.cancelled() => return, - permit = workers.clone().acquire_owned() => permit, - } { - Ok(permit) => permit, - Err(err) => { - let err = Error::other(format!("decommission entry worker permit acquire failed: {err}")); - error!("decommission_pool: decommission_entry failed: {err}"); - let mut first_err = entry_error.lock().await; - if first_err.is_none() { - *first_err = Some(err); - callback_rx.cancel(); - } - return; - } - }; - if entry_error.lock().await.is_some() { - return; - } - let entry_rx = callback_rx.clone(); - if let Err(err) = this - .decommission_entry( - entry_rx, - idx, - entry, - bucket, - set, - worker_permit, - lifecycle_config, - object_lock_config, - replication_config, - expected_bucket_incarnation_id, - ) - .await - { - error!("decommission_pool: decommission_entry failed: {err}"); - let mut first_err = entry_error.lock().await; - if first_err.is_none() { - *first_err = Some(err); - callback_rx.cancel(); - } - } - }) - } - }); - let set = set.clone(); + let store = Arc::clone(self); let rx_clone = rx.clone(); - let bi = bi.clone(); - let set_id = set_idx; - let listing_entry_error = entry_error.clone(); + let bi_clone = bi.clone(); + let lifecycle_config = lifecycle_config.clone(); + let object_lock_config = object_lock_config.clone(); + let replication_config = replication_config.clone(); + let entry_budget = entry_budget.clone(); + let entry_error = entry_error.clone(); let worker = tokio::spawn(async move { - let _listing_permit = listing_permit; - run_decommission_listing_with_retry( - rx_clone.clone(), - bi.name.clone(), - decommission_entry.clone(), - idx, - set_id, - DECOMMISSION_LISTING_MAX_ATTEMPTS, - |callback| { - let set = set.clone(); - let rx = rx_clone.clone(); - let bucket = bi.clone(); - let entry_error = listing_entry_error.clone(); - async move { - set.list_objects_to_decommission(rx, bucket, callback, entry_error.clone(), idx, set_id) - .await - } - }, - ) - .await + store + .decommission_set( + rx_clone, + idx, + set_idx, + generation, + set, + bi_clone, + lifecycle_config, + object_lock_config, + replication_config, + expected_bucket_incarnation_id, + entry_budget, + entry_error, + ) + .await }); - listing_workers.push((set_id, worker)); + listing_workers.push((set_idx, worker)); } debug!( @@ -4010,9 +4398,11 @@ impl ECStore { } } - wait_decommission_worker_drain(&workers, worker_limit).await?; + if let Some(err) = listing_worker_error { + return Err(err); + } - if let Some(err) = resolve_decommission_listing_error(listing_worker_error, entry_error.lock().await.clone()) { + if let Some(err) = entry_error.lock().await.clone() { return Err(err); } @@ -4044,9 +4434,14 @@ impl ECStore { } #[tracing::instrument(skip(self, canceler))] - pub async fn do_decommission_in_routine(self: &Arc, canceler: DecommissionCanceler, idx: usize) -> Result<()> { + pub async fn do_decommission_in_routine( + self: &Arc, + canceler: DecommissionCanceler, + idx: usize, + entry_budget: Arc, + ) -> Result<()> { let rx = canceler.token().clone(); - self.run_decommission_in_routine(rx, idx, &canceler).await + self.run_decommission_in_routine(rx, idx, &canceler, entry_budget).await } async fn run_decommission_in_routine( @@ -4054,15 +4449,20 @@ impl ECStore { rx: CancellationToken, idx: usize, canceler: &DecommissionCanceler, + entry_budget: Arc, ) -> Result<()> { - if let Err(err) = self.promote_queued_decommission(idx).await { - resolve_decommission_terminal_mark_after_error_result( - self.decommission_failed_for_operation(idx, canceler).await, - idx, - &err, - )?; - return Err(err); - } + let generation = match self.promote_queued_decommission(idx, canceler).await { + Ok(generation) => generation, + Err(Error::OperationCanceled) => return Ok(()), + Err(err) => { + resolve_decommission_terminal_mark_after_error_result( + self.decommission_failed_for_operation(idx, canceler).await, + idx, + &err, + )?; + return Err(err); + } + }; if rx.is_cancelled() { let already_canceled = { let pool_meta = self.pool_meta.read().await; @@ -4089,7 +4489,7 @@ impl ECStore { } return Ok(()); } - let result = self.decommission_in_background(rx.clone(), idx).await; + let result = self.decommission_in_background(rx.clone(), idx, entry_budget).await; let (final_state, canceled, cmd_line) = { let pool_meta = self.pool_meta.read().await; @@ -4204,6 +4604,12 @@ impl ECStore { ))); } + if self.decommission_cancel_requested(idx, &rx).await { + rx.cancel(); + } + decommission_cancel_signal_result(rx.is_cancelled())?; + self.ensure_decommission_generation_current(idx, generation).await?; + info!( event = EVENT_DECOMMISSION_STATE, component = LOG_COMPONENT_ECSTORE, @@ -4440,6 +4846,7 @@ impl ECStore { idx: usize, pool: Arc, bucket: DecomBucketInfo, + entry_budget: Arc, ) -> Result<()> { let is_decommissioned = { let pool_meta = self.pool_meta.read().await; @@ -4465,7 +4872,10 @@ impl ECStore { warn!("decommission: currently on bucket {}", &bucket.name); - if let Err(err) = self.decommission_pool(rx.clone(), idx, pool, bucket.clone()).await { + if let Err(err) = self + .decommission_pool(rx.clone(), idx, pool, bucket.clone(), entry_budget) + .await + { error!("decommission: decommission_pool err {:?}", &err); return Err(err); } else { @@ -4499,40 +4909,53 @@ impl ECStore { pool: Arc, buckets: Vec, limit: usize, + entry_budget: Arc, ) -> Result<()> { let store = Arc::clone(self); run_decommission_buckets_bounded(rx, buckets, limit, move |bucket, rx| { let store = Arc::clone(&store); let pool = pool.clone(); - Box::pin(async move { store.decommission_pending_bucket(rx, idx, pool, bucket).await }) + let entry_budget = entry_budget.clone(); + Box::pin(async move { store.decommission_pending_bucket(rx, idx, pool, bucket, entry_budget).await }) }) .await } #[tracing::instrument(skip(self, rx))] - async fn decommission_in_background(self: &Arc, rx: CancellationToken, idx: usize) -> Result<()> { + async fn decommission_in_background( + self: &Arc, + rx: CancellationToken, + idx: usize, + entry_budget: Arc, + ) -> Result<()> { let pool = get_by_index(self.pools.as_slice(), idx, "load decommission background pool")?.clone(); let pending = { let pool_meta = self.pool_meta.read().await; pool_meta.pending_buckets(idx) }; - let bucket_concurrency = decommission_bucket_concurrency_limit(); if bucket_concurrency <= 1 { for bucket in pending { - self.decommission_pending_bucket(rx.clone(), idx, pool.clone(), bucket) + self.decommission_pending_bucket(rx.clone(), idx, pool.clone(), bucket, entry_budget.clone()) .await?; } return Ok(()); } let (regular_buckets, meta_buckets) = split_decommission_buckets(pending); - self.decommission_buckets_concurrently(rx.clone(), idx, pool.clone(), regular_buckets, bucket_concurrency) - .await?; + self.decommission_buckets_concurrently( + rx.clone(), + idx, + pool.clone(), + regular_buckets, + bucket_concurrency, + entry_budget.clone(), + ) + .await?; for bucket in meta_buckets { - self.decommission_pending_bucket(rx.clone(), idx, pool.clone(), bucket) + self.decommission_pending_bucket(rx.clone(), idx, pool.clone(), bucket, entry_budget.clone()) .await?; } @@ -4600,6 +5023,8 @@ impl ECStore { self.ensure_decommission_rebalance_idle_after_refresh().await?; let all_space_infos = self.get_decommission_all_pool_space_infos().await?; + self.cancel_decommission_routines_and_wait(&indices).await; + let index_cancelers = if let Some((rx, local_indices)) = reservation { // Lock order matches terminal transitions: decommission_cancelers // before pool_meta while start_gate excludes another start. @@ -5169,6 +5594,14 @@ mod tests { let mut pool_meta = build_pool_meta(); assert!(pool_meta.decommission_cancel(0)); assert_eq!(pool_meta.pools[0].decommission.as_ref().and_then(|info| info.start_time), None); + + let mut pool_meta = build_pool_meta(); + assert!(pool_meta.decommission_cancel(0)); + assert!(!pool_meta.decommission_complete(0)); + + let mut pool_meta = build_pool_meta(); + assert!(pool_meta.decommission_failed(0)); + assert!(!pool_meta.decommission_complete(0)); } #[test] @@ -5552,6 +5985,79 @@ mod tests { pub type ListCallback = Arc BoxFuture<'static, ()> + Send + Sync + 'static>; +const DECOMMISSION_ENTRY_QUEUE_HARD_CAP: usize = 256; + +struct QueuedDecommissionEntry { + entry: MetaCacheEntry, + queue_permit: OwnedSemaphorePermit, +} + +enum DecommissionEntryEnqueueResult { + Enqueued, + Canceled, + Closed, +} + +fn decommission_entry_queue_capacity(worker_limit: usize) -> usize { + worker_limit.saturating_mul(2).clamp(1, DECOMMISSION_ENTRY_QUEUE_HARD_CAP) +} + +async fn enqueue_decommission_entry( + rx: &CancellationToken, + outstanding: &Arc, + tx: &mpsc::Sender, + entry: MetaCacheEntry, +) -> DecommissionEntryEnqueueResult { + let queue_permit = match tokio::select! { + biased; + _ = rx.cancelled() => return DecommissionEntryEnqueueResult::Canceled, + permit = outstanding.clone().acquire_owned() => permit, + } { + Ok(permit) => permit, + Err(_) => return DecommissionEntryEnqueueResult::Closed, + }; + + let queued = QueuedDecommissionEntry { entry, queue_permit }; + tokio::select! { + biased; + _ = rx.cancelled() => DecommissionEntryEnqueueResult::Canceled, + result = tx.send(queued) => { + if result.is_ok() { + DecommissionEntryEnqueueResult::Enqueued + } else { + DecommissionEntryEnqueueResult::Closed + } + } + } +} + +async fn drain_decommission_entry_queue(rx: &CancellationToken, outstanding: &Arc, capacity: usize) -> bool { + let Ok(permits) = u32::try_from(capacity) else { + return true; + }; + + tokio::select! { + _ = rx.cancelled() => true, + result = outstanding.acquire_many(permits) => result.is_err(), + } +} + +async fn record_decommission_entry_error( + entry_error: &Arc>>, + rx: &CancellationToken, + err: Error, +) { + if rx.is_cancelled() { + return; + } + + let mut first_err = entry_error.lock().await; + if first_err.is_none() && !rx.is_cancelled() { + *first_err = Some(err); + rx.cancel(); + } +} + impl SetDisks { #[tracing::instrument(skip(self, rx, cb_func, entry_error))] async fn list_objects_to_decommission( @@ -5843,23 +6349,26 @@ mod pools_tests { use super::record_decommission_entry_error; use super::resolve_decommission_listing_error; use super::{ + DECOMMISSION_ENTRY_CONCURRENCY_DEFAULT_CAP, DECOMMISSION_ENTRY_CONCURRENCY_HARD_CAP, DECOMMISSION_ENTRY_QUEUE_HARD_CAP, DECOMMISSION_PROGRESS_SAVE_INTERVAL, DECOMMISSION_PROGRESS_SAVE_ITEM_THRESHOLD, DecomBucketInfo, DecommissionCanceler, - DecommissionStartPoolState, DecommissionTerminalState, ListCallback, PoolDecommissionInfo, PoolMeta, PoolSpaceInfo, - PoolStatus, apply_decommission_status_space_info, await_decommission_worker, bind_decommission_cancelers, - bind_missing_decommission_cancelers, cancel_decommission_canceler, classify_decommission_terminal_state, - count_decommission_item, decommission_cancel_signal_result, decommission_item_size, decommission_meta_bucket_options, - decommission_start_pool_state, dedup_indices, default_decommission_bucket_concurrency, - ensure_decommission_cancel_allowed, ensure_decommission_clear_allowed, ensure_decommission_listing_disks_available, - ensure_decommission_not_rebalancing, ensure_decommission_start_allowed, ensure_decommission_start_keeps_active_pool, - ensure_decommission_start_local_leader, ensure_decommission_start_pool_states, - ensure_decommission_start_rebalance_meta_allowed, ensure_decommission_start_target_capacity, - ensure_decommission_terminal_operation_supported, ensure_local_decommission_pool_leaders, - ensure_valid_decommission_pool_index, first_resumable_decommission_queue_indices, get_by_index, - guard_decommission_cancelers, has_active_decommission_canceler, is_decommission_active, is_decommission_cancel_requested, - load_decommission_entry_versions, local_decommission_queue_prefix, mark_decommission_bucket_done, - merge_pool_status_refresh, missing_decommission_worker_prefix, observe_decommission_terminal_reload_result, - pool_meta_has_active_decommission, require_decommission_store, reserve_decommission_start_cancelers, - resolve_decommission_bucket_done_save_result, resolve_decommission_bucket_state, + DecommissionEntryEnqueueResult, DecommissionStartPoolState, DecommissionTerminalState, ListCallback, + PoolDecommissionInfo, PoolMeta, PoolSpaceInfo, PoolStatus, QueuedDecommissionEntry, apply_decommission_status_space_info, + await_decommission_worker, bind_decommission_cancelers, bind_missing_decommission_cancelers, + cancel_decommission_canceler, clamp_decommission_entry_concurrency, classify_decommission_terminal_state, + count_decommission_item, decommission_cancel_signal_result, decommission_entry_queue_capacity, decommission_item_size, + decommission_meta_bucket_options, decommission_start_pool_state, dedup_indices, default_decommission_bucket_concurrency, + default_decommission_entry_concurrency, drain_decommission_entry_queue, enqueue_decommission_entry, + ensure_decommission_cancel_allowed, ensure_decommission_clear_allowed, ensure_decommission_generation, + ensure_decommission_listing_disks_available, ensure_decommission_not_rebalancing, ensure_decommission_start_allowed, + ensure_decommission_start_keeps_active_pool, ensure_decommission_start_local_leader, + ensure_decommission_start_pool_states, ensure_decommission_start_rebalance_meta_allowed, + ensure_decommission_start_target_capacity, ensure_decommission_terminal_operation_supported, + ensure_local_decommission_pool_leaders, ensure_valid_decommission_pool_index, first_resumable_decommission_queue_indices, + get_by_index, guard_decommission_cancelers, has_active_decommission_canceler, is_decommission_active, + is_decommission_cancel_requested, load_decommission_entry_versions, local_decommission_queue_prefix, + mark_decommission_bucket_done, merge_pool_status_refresh, missing_decommission_worker_prefix, + observe_decommission_terminal_reload_result, pool_meta_has_active_decommission, require_decommission_store, + reserve_decommission_start_cancelers, resolve_decommission_bucket_done_save_result, resolve_decommission_bucket_state, resolve_decommission_check_after_list_result, resolve_decommission_entry_cleanup_delete_result, resolve_decommission_entry_exact_versions, resolve_decommission_entry_reload_result, resolve_decommission_listing_worker_result, resolve_decommission_optional_bucket_config_result, @@ -5868,7 +6377,8 @@ mod pools_tests { resolve_decommission_terminal_mark_after_error_result, resolve_decommission_terminal_mark_result, resolve_decommission_update_after_result, resolve_start_decommission_pool_meta_reload_result, rollback_start_decommission_pool_meta, run_decommission_buckets_bounded, run_decommission_listing_with_retry, - should_cleanup_decommission_source_entry, should_continue_decommission_queue, should_count_decommission_version_complete, + run_decommission_listing_with_retry_and_drain, run_decommission_side_effect, should_cleanup_decommission_source_entry, + should_continue_decommission_queue, should_count_decommission_version_complete, should_preserve_decommission_canceled_state, should_reject_decommission_cancel_as_terminal, should_retry_decommission_cancel_reload, should_retry_decommission_listing, should_skip_canceled_decommission_routine, spawn_decommission_index_cancelers, split_decommission_buckets, take_and_cancel_decommission_canceler, @@ -6130,6 +6640,25 @@ mod pools_tests { assert_eq!(default_decommission_bucket_concurrency(8), 4); } + #[test] + fn test_default_decommission_entry_concurrency_is_conservative() { + assert_eq!(default_decommission_entry_concurrency(0), 1); + assert_eq!(default_decommission_entry_concurrency(1), 1); + assert_eq!(default_decommission_entry_concurrency(4), 4); + assert_eq!(default_decommission_entry_concurrency(16), DECOMMISSION_ENTRY_CONCURRENCY_DEFAULT_CAP); + } + + #[test] + fn test_decommission_entry_concurrency_clamps_operator_configuration() { + assert_eq!(clamp_decommission_entry_concurrency(0), 1); + assert_eq!(clamp_decommission_entry_concurrency(1), 1); + assert_eq!( + clamp_decommission_entry_concurrency(DECOMMISSION_ENTRY_CONCURRENCY_HARD_CAP), + DECOMMISSION_ENTRY_CONCURRENCY_HARD_CAP + ); + assert_eq!(clamp_decommission_entry_concurrency(usize::MAX), DECOMMISSION_ENTRY_CONCURRENCY_HARD_CAP); + } + #[test] fn test_split_decommission_buckets_keeps_meta_buckets_last() { let (regular, meta) = split_decommission_buckets(vec![ @@ -6301,6 +6830,190 @@ mod pools_tests { assert!(result.is_ok()); } + #[test] + fn test_decommission_entry_queue_capacity_is_bounded() { + assert_eq!(decommission_entry_queue_capacity(0), 1); + assert_eq!(decommission_entry_queue_capacity(1), 2); + assert_eq!( + decommission_entry_queue_capacity(DECOMMISSION_ENTRY_QUEUE_HARD_CAP), + DECOMMISSION_ENTRY_QUEUE_HARD_CAP + ); + assert_eq!(decommission_entry_queue_capacity(usize::MAX), DECOMMISSION_ENTRY_QUEUE_HARD_CAP); + } + + #[tokio::test] + async fn test_drain_decommission_entry_queue_waits_for_all_outstanding_entries() { + let outstanding = Arc::new(Semaphore::new(1)); + let held = outstanding + .clone() + .acquire_owned() + .await + .expect("test outstanding permit should acquire"); + let rx = CancellationToken::new(); + let drain = tokio::spawn({ + let outstanding = outstanding.clone(); + let rx = rx.clone(); + async move { drain_decommission_entry_queue(&rx, &outstanding, 1).await } + }); + + tokio::task::yield_now().await; + assert!(!drain.is_finished(), "queue drain must wait for active entry work"); + drop(held); + + let drained = tokio::time::timeout(StdDuration::from_secs(1), drain) + .await + .expect("queue drain should finish after entry completion") + .expect("queue drain task should not panic"); + assert!(!drained); + } + + #[tokio::test] + async fn test_enqueue_decommission_entry_observes_cancellation_when_queue_is_full() { + let outstanding = Arc::new(Semaphore::new(2)); + let (tx, mut queue) = tokio::sync::mpsc::channel(1); + let held = outstanding + .clone() + .acquire_owned() + .await + .expect("first queue permit should acquire"); + tx.send(QueuedDecommissionEntry { + entry: MetaCacheEntry::default(), + queue_permit: held, + }) + .await + .expect("first entry should fill the queue"); + + let rx = CancellationToken::new(); + let enqueue = tokio::spawn({ + let rx = rx.clone(); + let outstanding = outstanding.clone(); + let tx = tx.clone(); + async move { enqueue_decommission_entry(&rx, &outstanding, &tx, MetaCacheEntry::default()).await } + }); + + tokio::task::yield_now().await; + rx.cancel(); + let result = tokio::time::timeout(StdDuration::from_secs(1), enqueue) + .await + .expect("full queue enqueue should observe cancellation") + .expect("enqueue task should not panic"); + assert!(matches!(result, DecommissionEntryEnqueueResult::Canceled)); + drop(queue.recv().await); + } + + #[tokio::test] + async fn test_decommission_side_effect_gate_quiesces_before_transition() { + let operation_gate = Arc::new(tokio::sync::RwLock::new(())); + let rx = CancellationToken::new(); + let started = Arc::new(tokio::sync::Notify::new()); + let release = Arc::new(tokio::sync::Notify::new()); + let operation = tokio::spawn({ + let operation_gate = operation_gate.clone(); + let rx = rx.clone(); + let started = started.clone(); + let release = release.clone(); + async move { + run_decommission_side_effect(&rx, &operation_gate, || async { + started.notify_one(); + release.notified().await; + Ok::<_, Error>(()) + }) + .await + } + }); + + started.notified().await; + rx.cancel(); + let transition = tokio::spawn({ + let operation_gate = operation_gate.clone(); + async move { + let _guard = operation_gate.write().await; + } + }); + tokio::task::yield_now().await; + assert!(!transition.is_finished(), "transition must wait for the in-flight side effect"); + + release.notify_one(); + let operation_result = operation.await.expect("operation task should not panic"); + assert!(matches!(operation_result, Err(Error::OperationCanceled))); + transition.await.expect("transition task should not panic"); + + let called = Arc::new(AtomicBool::new(false)); + let result = run_decommission_side_effect(&rx, &operation_gate, { + let called = called.clone(); + move || async move { + called.store(true, Ordering::SeqCst); + Ok::<_, Error>(()) + } + }) + .await; + assert!(matches!(result, Err(Error::OperationCanceled))); + assert!(!called.load(Ordering::SeqCst)); + } + + #[tokio::test] + async fn test_decommission_transition_waits_without_registered_canceler() { + let store = decommission_worker_test_store(PoolMeta::default(), vec![None]); + let operation_gate = store.ctx.decommission_operation_gate(); + let operation_guard = operation_gate.read().await; + let transition = tokio::spawn({ + let store = store.clone(); + async move { store.cancel_decommission_routines_and_wait(&[0]).await } + }); + + tokio::task::yield_now().await; + assert!( + !transition.is_finished(), + "a transition must wait for an in-flight side effect even after its canceler slot is gone" + ); + + drop(operation_guard); + tokio::time::timeout(StdDuration::from_secs(1), transition) + .await + .expect("transition should finish after the side effect") + .expect("transition task should not panic"); + } + + #[tokio::test(start_paused = true)] + async fn test_run_decommission_listing_with_retry_drains_before_each_retry() { + let attempts = Arc::new(AtomicUsize::new(0)); + let drains = Arc::new(AtomicUsize::new(0)); + let err = run_decommission_listing_with_retry_and_drain( + CancellationToken::new(), + "bucket-a".to_string(), + noop_decommission_list_callback(), + 1, + 2, + 2, + { + let attempts = attempts.clone(); + move |_| { + let attempts = attempts.clone(); + async move { + attempts.fetch_add(1, Ordering::SeqCst); + Err(Error::SlowDown) + } + } + }, + { + let drains = drains.clone(); + move || { + let drains = drains.clone(); + async move { + drains.fetch_add(1, Ordering::SeqCst); + false + } + } + }, + ) + .await + .expect_err("permanent listing failure must be returned"); + + assert!(err.to_string().contains("attempt 2/2")); + assert_eq!(attempts.load(Ordering::SeqCst), 2); + assert_eq!(drains.load(Ordering::SeqCst), 2); + } + #[test] fn test_get_by_index_returns_value_when_in_range() { let values = vec!["a", "b", "c"]; @@ -7528,6 +8241,43 @@ mod pools_tests { assert!(!is_decommission_active(false, false, true)); } + #[test] + fn test_ensure_decommission_generation_rejects_stale_or_queued_workers() { + let generation = OffsetDateTime::UNIX_EPOCH; + let mut meta = PoolMeta { + pools: vec![PoolStatus { + id: 0, + cmd_line: "pool-0".to_string(), + last_update: generation, + decommission: Some(PoolDecommissionInfo { + start_time: Some(generation), + ..Default::default() + }), + }], + ..Default::default() + }; + + assert!(ensure_decommission_generation(&meta, 0, generation).is_ok()); + assert!(ensure_decommission_generation(&meta, 0, generation + Duration::seconds(1)).is_err()); + + meta.pools[0] + .decommission + .as_mut() + .expect("decommission metadata should exist") + .queued = true; + assert!(ensure_decommission_generation(&meta, 0, generation).is_err()); + + let replacement_generation = generation + Duration::seconds(2); + let info = meta.pools[0] + .decommission + .as_mut() + .expect("decommission metadata should exist"); + info.queued = false; + info.start_time = Some(replacement_generation); + assert!(ensure_decommission_generation(&meta, 0, generation).is_err()); + assert!(ensure_decommission_generation(&meta, 0, replacement_generation).is_ok()); + } + #[test] fn test_pool_meta_has_active_decommission_counts_running_and_queued_states() { let active_meta = PoolMeta { @@ -8869,7 +9619,7 @@ mod pools_tests { canceler.cancel(); let err = store - .do_decommission_in_routine(canceler.clone(), 0) + .do_decommission_in_routine(canceler.clone(), 0, Arc::new(Semaphore::new(1))) .await .expect_err("missing worker metadata should fail the routine"); @@ -8885,7 +9635,7 @@ mod pools_tests { let store = decommission_worker_test_store(PoolMeta::default(), vec![Some(first.clone()), Some(queued.clone())]); let guards = guard_decommission_cancelers(vec![(0, first.clone()), (1, queued.clone())]); - spawn_decommission_index_cancelers(store.clone(), CancellationToken::new(), guards) + spawn_decommission_index_cancelers(store.clone(), CancellationToken::new(), guards, Arc::new(Semaphore::new(1))) .await .expect("decommission supervisor should finish after queued cleanup"); diff --git a/crates/ecstore/src/runtime/instance.rs b/crates/ecstore/src/runtime/instance.rs index 71a1898cf..9453ed02b 100644 --- a/crates/ecstore/src/runtime/instance.rs +++ b/crates/ecstore/src/runtime/instance.rs @@ -160,6 +160,10 @@ pub struct InstanceContext { /// workers (scanner/heal/tier/lifecycle) without touching another instance. /// Replaces the process-global cancel-token static. background_cancel_token: OnceLock, + /// Serializes decommission data-movement operations with cancellation and + /// a subsequent restart. Readers are held across one object side effect; + /// the transition path takes the writer after cancelling the routine. + decommission_operation_gate: Arc>, /// Resolves object-encryption material at the application boundary. object_encryption_resolver: OnceLock>, tier_delete_journal_recovery_stores: std::sync::Mutex>, @@ -200,6 +204,7 @@ impl InstanceContext { local_disk_set_drives: Arc::new(RwLock::new(Vec::new())), bucket_metadata_sys: std::sync::Mutex::new(None), background_cancel_token: OnceLock::new(), + decommission_operation_gate: Arc::new(RwLock::new(())), object_encryption_resolver: OnceLock::new(), tier_delete_journal_recovery_stores: std::sync::Mutex::new(HashSet::new()), transition_transaction_recovery_stores: std::sync::Mutex::new(HashSet::new()), @@ -218,6 +223,10 @@ impl InstanceContext { self.lock_manager.clone() } + pub(crate) fn decommission_operation_gate(&self) -> Arc> { + Arc::clone(&self.decommission_operation_gate) + } + /// Install the application-owned object-encryption resolver once. pub fn set_object_encryption_resolver( &self, From ddc4120c82bf82627e711deee1f1ce302e141635 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:40:28 +0800 Subject: [PATCH 04/12] ci: detect incomplete and stale scheduled validations (#6357) --- .config/make/tests.mak | 1 + .../actions/schedule-failure-issue/action.yml | 65 +- .github/scheduled-validations.json | 14 + .github/workflows/audit.yml | 2 +- .github/workflows/build.yml | 22 +- .github/workflows/ci-docs-only.yml | 5 +- .github/workflows/ci.yml | 41 +- .github/workflows/coverage.yml | 2 +- .github/workflows/e2e-replication-nightly.yml | 2 +- .github/workflows/e2e-s3tests.yml | 2 +- .github/workflows/fuzz.yml | 2 +- .github/workflows/minio-interop.yml | 18 + .github/workflows/mint.yml | 2 +- .github/workflows/nightly-gnu.yml | 22 +- .github/workflows/performance-ab.yml | 2 +- .github/workflows/runner-hygiene.yml | 2 +- .../scheduled-validation-freshness.yml | 57 ++ .../scheduled-validation-watchdog.yml | 63 ++ .../check_scheduled_validation_freshness.py | 263 ++++++++ scripts/check_test_wiring.py | 562 +++++++++++++++++- 20 files changed, 1123 insertions(+), 26 deletions(-) create mode 100644 .github/scheduled-validations.json create mode 100644 .github/workflows/scheduled-validation-freshness.yml create mode 100644 .github/workflows/scheduled-validation-watchdog.yml create mode 100644 scripts/check_scheduled_validation_freshness.py diff --git a/.config/make/tests.mak b/.config/make/tests.mak index 1dec7db3e..626df9b84 100644 --- a/.config/make/tests.mak +++ b/.config/make/tests.mak @@ -36,6 +36,7 @@ script-tests: ## Run shell script tests ./scripts/test_manual_transition_runbooks.sh ./scripts/check_embedded_secrets.sh --self-test python3 ./scripts/check_test_wiring.py --self-test + python3 ./scripts/check_scheduled_validation_freshness.py --self-test python3 ./scripts/s3-tests/test_report_compat.py bash -n ./scripts/validate_object_data_cache_cold_stampede.sh python3 ./scripts/check_object_data_cache_follower_samples.py --self-test diff --git a/.github/actions/schedule-failure-issue/action.yml b/.github/actions/schedule-failure-issue/action.yml index 60e938690..d505387e4 100644 --- a/.github/actions/schedule-failure-issue/action.yml +++ b/.github/actions/schedule-failure-issue/action.yml @@ -14,9 +14,10 @@ name: "Schedule Failure Issue" description: >- - Open (or update) a tracking issue when a scheduled workflow run fails. + Open (or update) a tracking issue when a scheduled workflow run fails or + does not complete normally. Dedupes by workflow name: if an open issue titled - "[scheduled-failure] " already exists, the failure is + "[scheduled-failure] " already exists, the result is appended as a comment; otherwise a new issue is created. This is the single alerting mechanism for all scheduled pipelines (backlog#1149 ci-8). @@ -38,6 +39,30 @@ inputs: Set to an empty string to skip labeling. required: false default: "infrastructure" + source-run-id: + description: "Run ID to report. Defaults to the current workflow run." + required: false + default: ${{ github.run_id }} + source-run-attempt: + description: "Run attempt to report. Defaults to the current attempt." + required: false + default: ${{ github.run_attempt }} + source-event: + description: "Trigger event of the run being reported." + required: false + default: ${{ github.event_name }} + source-ref-name: + description: "Ref name of the run being reported." + required: false + default: ${{ github.ref_name }} + source-sha: + description: "Commit SHA of the run being reported." + required: false + default: ${{ github.sha }} + details-file: + description: "Optional Markdown file appended to the issue body." + required: false + default: "" runs: using: "composite" @@ -48,17 +73,22 @@ runs: GH_TOKEN: ${{ inputs.github-token }} WORKFLOW_NAME: ${{ inputs.workflow-name }} ISSUE_LABEL: ${{ inputs.label }} + SOURCE_RUN_ID: ${{ inputs.source-run-id }} + SOURCE_RUN_ATTEMPT: ${{ inputs.source-run-attempt }} + SOURCE_EVENT: ${{ inputs.source-event }} + SOURCE_REF_NAME: ${{ inputs.source-ref-name }} + SOURCE_SHA: ${{ inputs.source-sha }} + DETAILS_FILE: ${{ inputs.details-file }} run: | set -euo pipefail title="[scheduled-failure] ${WORKFLOW_NAME}" - run_url="${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" + run_url="${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${SOURCE_RUN_ID}" - # Failed job names for this run attempt. The alert job runs while the - # run as a whole is still in progress, so inspect the jobs that have - # already completed with a non-success conclusion. + # Inspect the reported run attempt. It can be the current in-workflow + # failure or a completed run observed by the external watchdog. failed_jobs="$(gh api \ - "repos/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}/attempts/${GITHUB_RUN_ATTEMPT}/jobs" \ + "repos/${GITHUB_REPOSITORY}/actions/runs/${SOURCE_RUN_ID}/attempts/${SOURCE_RUN_ATTEMPT}/jobs" \ --paginate \ --jq '.jobs[] | select(.conclusion == "failure" or .conclusion == "timed_out" or .conclusion == "cancelled") @@ -67,15 +97,26 @@ runs: failed_jobs="- (failed job not recorded yet — see the run page)" fi + details="" + if [ -n "${DETAILS_FILE}" ]; then + if [ -f "${DETAILS_FILE}" ]; then + details="$(cat "${DETAILS_FILE}")" + else + details="Details file was not available: \`${DETAILS_FILE}\`" + fi + fi + body="$(cat <- + always() && github.event_name == 'schedule' && + (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')) + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Open or update failure-tracking issue + uses: ./.github/actions/schedule-failure-issue + with: + github-token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/ci-docs-only.yml b/.github/workflows/ci-docs-only.yml index 85ce9b32f..de78e7f7f 100644 --- a/.github/workflows/ci-docs-only.yml +++ b/.github/workflows/ci-docs-only.yml @@ -126,7 +126,10 @@ jobs: run: ./scripts/check_embedded_secrets.sh - name: Check test wiring - run: python3 ./scripts/check_test_wiring.py + run: | + python3 ./scripts/check_test_wiring.py --self-test + python3 ./scripts/check_scheduled_validation_freshness.py --self-test + python3 ./scripts/check_test_wiring.py - name: Check no planning docs committed run: ./scripts/check_no_planning_docs.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a06e4667..ae74ec40a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,7 +59,7 @@ on: merge_group: types: [ checks_requested ] schedule: - - cron: "0 0 * * 0" # Weekly on Sunday at midnight UTC + - cron: "11 0 * * 0" # Weekly on Sunday 00:11 UTC workflow_dispatch: permissions: @@ -161,7 +161,10 @@ jobs: run: ./scripts/check_embedded_secrets.sh - name: Check test wiring - run: python3 ./scripts/check_test_wiring.py + run: | + python3 ./scripts/check_test_wiring.py --self-test + python3 ./scripts/check_scheduled_validation_freshness.py --self-test + python3 ./scripts/check_test_wiring.py - name: Check no planning docs committed run: ./scripts/check_no_planning_docs.sh @@ -1032,3 +1035,37 @@ jobs: path: artifacts/s3tests-single/** if-no-files-found: ignore retention-days: 3 + + alert-on-failure: + name: Alert on scheduled failure + needs: + - typos + - quick-checks + - test-and-lint + - test-ilm-integration-serial + - test-and-lint-rio-v2 + - test-and-lint-protocols + - build-rustfs-debug-binary + - build-rustfs-debug-binary-rio-v2 + - uring-integration + - e2e-tests + - e2e-full + - e2e-tests-rio-v2 + - s3-implemented-tests + - s3-lifecycle-behavior-tests + if: >- + always() && github.event_name == 'schedule' && + (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')) + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Open or update failure-tracking issue + uses: ./.github/actions/schedule-failure-issue + with: + github-token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index baa3c87a7..ece00c2eb 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -37,7 +37,7 @@ on: # build (01:00), e2e-s3tests (02:00), audit (03:00), nix-flake-update # (05:00), mint (06:00), and the daily fuzz (02:00), minio-interop (03:17), # e2e-replication-nightly (04:00) and performance-ab (06:00) lanes. - - cron: "0 7 * * 0" + - cron: "43 7 * * 0" # Only alert-on-failure needs more than read access; it declares its own # job-level `issues: write`. diff --git a/.github/workflows/e2e-replication-nightly.yml b/.github/workflows/e2e-replication-nightly.yml index 837d9f79f..ef312d180 100644 --- a/.github/workflows/e2e-replication-nightly.yml +++ b/.github/workflows/e2e-replication-nightly.yml @@ -40,7 +40,7 @@ on: schedule: # 04:00 UTC nightly — staggered clear of fuzz/e2e-s3tests (02:00), # stale (01:30) and performance-ab (06:00). - - cron: "0 4 * * *" + - cron: "29 4 * * *" # Only alert-on-failure needs more than read access; it declares its own # job-level `issues: write`. diff --git a/.github/workflows/e2e-s3tests.yml b/.github/workflows/e2e-s3tests.yml index 1e3ad90b6..693fa8450 100644 --- a/.github/workflows/e2e-s3tests.yml +++ b/.github/workflows/e2e-s3tests.yml @@ -93,7 +93,7 @@ on: schedule: # Weekly full sweep (Sunday 02:00 UTC): full suite, run against BOTH the # single-node and the 4-node distributed topologies (matrix below). - - cron: "0 2 * * 0" + - cron: "19 2 * * 0" env: # main user diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index d41a1107f..6aa780fc6 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -30,7 +30,7 @@ on: - "Cargo.lock" - ".github/workflows/fuzz.yml" schedule: - - cron: "0 2 * * *" + - cron: "17 2 * * *" workflow_dispatch: inputs: profile: diff --git a/.github/workflows/minio-interop.yml b/.github/workflows/minio-interop.yml index 3ee33e9bf..5f105ac41 100644 --- a/.github/workflows/minio-interop.yml +++ b/.github/workflows/minio-interop.yml @@ -121,3 +121,21 @@ jobs: cargo nextest run --run-ignored ignored-only --no-tests=fail \ -p "$INTEROP_PACKAGE" --features "$INTEROP_FEATURES" \ -E "$INTEROP_FILTER" + + alert-on-failure: + name: Alert on scheduled failure + needs: [minio-interop] + if: always() && github.event_name == 'schedule' && contains(needs.*.result, 'failure') + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Open or update failure-tracking issue + uses: ./.github/actions/schedule-failure-issue + with: + github-token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/mint.yml b/.github/workflows/mint.yml index dd9acc6f8..b9a7e354a 100644 --- a/.github/workflows/mint.yml +++ b/.github/workflows/mint.yml @@ -76,7 +76,7 @@ on: schedule: # Weekly, after the Sunday s3-tests full sweep (starts 02:00 UTC, up to # 3h) has finished, so the two never contend for the same runner pool. - - cron: "0 6 * * 0" + - cron: "41 6 * * 0" env: S3_ACCESS_KEY: rustfsadmin-ci diff --git a/.github/workflows/nightly-gnu.yml b/.github/workflows/nightly-gnu.yml index 1f7c2d488..946761e54 100644 --- a/.github/workflows/nightly-gnu.yml +++ b/.github/workflows/nightly-gnu.yml @@ -16,7 +16,7 @@ name: Nightly GNU Build on: schedule: - - cron: "0 0 * * *" + - cron: "7 0 * * *" timezone: "Asia/Shanghai" workflow_dispatch: @@ -194,3 +194,23 @@ jobs: - name: Run HA leader failover live checks (three-node Raft cluster in Docker) run: bash scripts/test/vault_ha_kms_live.sh + + alert-on-failure: + name: Alert on scheduled failure + needs: [build, kms-vault-lane, kms-vault-ha-failover] + if: >- + always() && github.event_name == 'schedule' && + (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')) + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Open or update failure-tracking issue + uses: ./.github/actions/schedule-failure-issue + with: + github-token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/performance-ab.yml b/.github/workflows/performance-ab.yml index ac8e8d7c5..a7ecf1d4b 100644 --- a/.github/workflows/performance-ab.yml +++ b/.github/workflows/performance-ab.yml @@ -33,7 +33,7 @@ name: Performance A/B on: schedule: - - cron: "0 6 * * *" # 06:00 UTC nightly, against main + - cron: "31 6 * * *" # 06:31 UTC nightly, against main workflow_dispatch: inputs: duration: diff --git a/.github/workflows/runner-hygiene.yml b/.github/workflows/runner-hygiene.yml index c231b5706..bdebf2c77 100644 --- a/.github/workflows/runner-hygiene.yml +++ b/.github/workflows/runner-hygiene.yml @@ -30,7 +30,7 @@ name: Runner Hygiene on: schedule: - - cron: "0 6 1 * *" # Monthly, 1st at 06:00 UTC (after the daily audit cron) + - cron: "37 6 1 * *" # Monthly, 1st at 06:37 UTC workflow_dispatch: permissions: diff --git a/.github/workflows/scheduled-validation-freshness.yml b/.github/workflows/scheduled-validation-freshness.yml new file mode 100644 index 000000000..eef340869 --- /dev/null +++ b/.github/workflows/scheduled-validation-freshness.yml @@ -0,0 +1,57 @@ +# Copyright 2024 RustFS Team +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: Scheduled Validation Freshness + +on: + schedule: + - cron: "47 23 * * *" + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: scheduled-validation-freshness + cancel-in-progress: false + +jobs: + check-freshness: + name: Check scheduled validation freshness + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + actions: read + contents: read + issues: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Check latest scheduled runs + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set +e + python3 scripts/check_scheduled_validation_freshness.py \ + --report "${RUNNER_TEMP}/scheduled-validation-freshness.md" + status=$? + cat "${RUNNER_TEMP}/scheduled-validation-freshness.md" >> "${GITHUB_STEP_SUMMARY}" + exit "${status}" + - name: Open or update freshness issue + if: failure() + uses: ./.github/actions/schedule-failure-issue + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + details-file: ${{ runner.temp }}/scheduled-validation-freshness.md diff --git a/.github/workflows/scheduled-validation-watchdog.yml b/.github/workflows/scheduled-validation-watchdog.yml new file mode 100644 index 000000000..e778ec640 --- /dev/null +++ b/.github/workflows/scheduled-validation-watchdog.yml @@ -0,0 +1,63 @@ +# Copyright 2024 RustFS Team +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: Scheduled Validation Watchdog + +on: + workflow_run: + workflows: + - "Security Audit" + - "Build and Release" + - "Continuous Integration" + - "coverage" + - "e2e-nightly" + - "e2e-s3tests" + - "Fuzz" + - "mint" + - "minio-interop" + - "Nightly GNU Build" + - "Performance A/B" + - "Runner Hygiene" + types: [completed] + +permissions: + contents: read + +jobs: + alert-on-incomplete-run: + name: Alert on incomplete scheduled run + if: >- + github.event.workflow_run.event == 'schedule' && + github.event.workflow_run.conclusion != 'success' && + github.event.workflow_run.conclusion != 'failure' + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + actions: read + contents: read + issues: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Open or update incomplete-run issue + uses: ./.github/actions/schedule-failure-issue + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + workflow-name: ${{ github.event.workflow_run.name }} + source-run-id: ${{ github.event.workflow_run.id }} + source-run-attempt: ${{ github.event.workflow_run.run_attempt }} + source-event: ${{ github.event.workflow_run.event }} + source-ref-name: ${{ github.event.workflow_run.head_branch }} + source-sha: ${{ github.event.workflow_run.head_sha }} diff --git a/scripts/check_scheduled_validation_freshness.py b/scripts/check_scheduled_validation_freshness.py new file mode 100644 index 000000000..d9bdf0597 --- /dev/null +++ b/scripts/check_scheduled_validation_freshness.py @@ -0,0 +1,263 @@ +#!/usr/bin/env python3 +"""Fail when a critical scheduled validation has not started recently.""" + +from __future__ import annotations + +import argparse +from datetime import datetime, timedelta, timezone +import json +import os +from pathlib import Path +import re +import sys +import tempfile +import unittest +from unittest import mock +from urllib.parse import quote, urlencode +from urllib.request import Request, urlopen + + +ROOT = Path(__file__).resolve().parents[1] + + +def load_validations(path: Path) -> list[tuple[str, int]]: + data = json.loads(path.read_text()) + if not isinstance(data, list) or not data: + raise ValueError("scheduled validation config must be a non-empty list") + + validations: list[tuple[str, int]] = [] + seen: set[str] = set() + for item in data: + if not isinstance(item, dict): + raise ValueError("scheduled validation entries must be objects") + workflow = item.get("workflow") + max_age_hours = item.get("max_age_hours") + if not isinstance(workflow, str) or not re.fullmatch( + r"\.github/workflows/[a-z0-9-]+\.yml", workflow + ): + raise ValueError(f"invalid scheduled validation workflow: {workflow!r}") + if workflow in seen: + raise ValueError(f"duplicate scheduled validation workflow: {workflow}") + if ( + not isinstance(max_age_hours, int) + or isinstance(max_age_hours, bool) + or max_age_hours <= 0 + ): + raise ValueError(f"invalid max_age_hours for {workflow}: {max_age_hours!r}") + seen.add(workflow) + validations.append((workflow, max_age_hours)) + return validations + + +def parse_timestamp(value: object) -> datetime: + if not isinstance(value, str): + raise ValueError(f"invalid run timestamp: {value!r}") + parsed = datetime.fromisoformat(value.replace("Z", "+00:00")) + if parsed.tzinfo is None: + raise ValueError(f"run timestamp has no timezone: {value!r}") + return parsed.astimezone(timezone.utc) + + +def stale_reason( + run: dict[str, object] | None, now: datetime, max_age_hours: int +) -> str | None: + if run is None: + return "no scheduled run has been recorded" + created_at = parse_timestamp(run.get("created_at")) + age = now - created_at + if age > timedelta(hours=max_age_hours): + return f"last scheduled run is {age.total_seconds() / 3600:.1f}h old" + return None + + +def fetch_latest_scheduled_run( + repository: str, workflow: str, token: str, api_url: str +) -> dict[str, object] | None: + owner, repo = repository.split("/", 1) + workflow_name = Path(workflow).name + endpoint = ( + f"{api_url.rstrip('/')}/repos/{quote(owner, safe='')}/{quote(repo, safe='')}" + f"/actions/workflows/{quote(workflow_name, safe='')}/runs?" + + urlencode({"event": "schedule", "per_page": 1}) + ) + request = Request( + endpoint, + headers={ + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {token}", + "X-GitHub-Api-Version": "2022-11-28", + }, + ) + with urlopen(request, timeout=30) as response: + payload = json.load(response) + runs = payload.get("workflow_runs") + if not isinstance(runs, list): + raise ValueError(f"GitHub returned no workflow_runs list for {workflow}") + if not runs: + return None + if not isinstance(runs[0], dict): + raise ValueError(f"GitHub returned an invalid workflow run for {workflow}") + return runs[0] + + +def write_report(path: Path, failures: list[tuple[str, int, str, str]]) -> None: + lines = ["## Scheduled validation freshness"] + if not failures: + lines.append("") + lines.append("All critical scheduled validations have a recent scheduled run.") + else: + lines.extend( + [ + "", + "The following critical validations are stale or could not be inspected:", + "", + "| Workflow | Limit | Result | Last run |", + "| --- | ---: | --- | --- |", + ] + ) + for workflow, max_age_hours, reason, run_url in failures: + link = f"[open]({run_url})" if run_url else "—" + lines.append(f"| `{workflow}` | {max_age_hours}h | {reason} | {link} |") + path.write_text("\n".join(lines) + "\n") + + +def check_freshness( + config: Path, report: Path, repository: str, token: str, api_url: str +) -> int: + now = datetime.now(timezone.utc) + failures: list[tuple[str, int, str, str]] = [] + for workflow, max_age_hours in load_validations(config): + try: + run = fetch_latest_scheduled_run(repository, workflow, token, api_url) + reason = stale_reason(run, now, max_age_hours) + if reason is not None: + run_url = str(run.get("html_url", "")) if run else "" + failures.append((workflow, max_age_hours, reason, run_url)) + except Exception as error: + failures.append( + (workflow, max_age_hours, f"inspection failed: {error}", "") + ) + write_report(report, failures) + return 1 if failures else 0 + + +class SelfTests(unittest.TestCase): + NOW = datetime(2026, 8, 22, 12, tzinfo=timezone.utc) + + def test_freshness_boundaries(self) -> None: + at_limit = {"created_at": "2026-08-21T00:00:00Z"} + past_limit = {"created_at": "2026-08-20T23:59:59Z"} + self.assertIsNone(stale_reason(at_limit, self.NOW, 36)) + self.assertIsNotNone(stale_reason(past_limit, self.NOW, 36)) + self.assertIsNotNone(stale_reason(None, self.NOW, 36)) + + def test_config_rejects_duplicate_and_invalid_entries(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "validations.json" + path.write_text( + json.dumps( + [ + {"workflow": ".github/workflows/ci.yml", "max_age_hours": 36}, + {"workflow": ".github/workflows/ci.yml", "max_age_hours": 0}, + ] + ) + ) + with self.assertRaises(ValueError): + load_validations(path) + path.write_text( + json.dumps( + [{"workflow": ".github/workflows/ci.yml", "max_age_hours": 0}] + ) + ) + with self.assertRaises(ValueError): + load_validations(path) + + def test_check_reports_missing_runs(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + config = root / "validations.json" + report = root / "report.md" + config.write_text( + json.dumps( + [ + {"workflow": ".github/workflows/ci.yml", "max_age_hours": 36}, + {"workflow": ".github/workflows/fuzz.yml", "max_age_hours": 36}, + {"workflow": ".github/workflows/mint.yml", "max_age_hours": 36}, + ] + ) + ) + with mock.patch( + __name__ + ".fetch_latest_scheduled_run", + side_effect=[ + {"created_at": "2999-01-01T00:00:00Z"}, + None, + RuntimeError("API unavailable"), + ], + ): + self.assertEqual( + check_freshness( + config, + report, + "rustfs/rustfs", + "token", + "https://api.github.test", + ), + 1, + ) + contents = report.read_text() + self.assertIn(".github/workflows/fuzz.yml", contents) + self.assertIn("inspection failed: API unavailable", contents) + self.assertNotIn(".github/workflows/ci.yml`", contents) + + config.write_text( + json.dumps( + [{"workflow": ".github/workflows/ci.yml", "max_age_hours": 36}] + ) + ) + with mock.patch( + __name__ + ".fetch_latest_scheduled_run", + return_value={"created_at": "2999-01-01T00:00:00Z"}, + ): + self.assertEqual( + check_freshness( + config, + report, + "rustfs/rustfs", + "token", + "https://api.github.test", + ), + 0, + ) + self.assertIn("All critical scheduled validations", report.read_text()) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--config", type=Path, default=ROOT / ".github/scheduled-validations.json" + ) + parser.add_argument("--report", type=Path) + parser.add_argument("--self-test", action="store_true") + args = parser.parse_args() + + if args.self_test: + load_validations(args.config) + suite = unittest.defaultTestLoader.loadTestsFromTestCase(SelfTests) + return ( + 0 if unittest.TextTestRunner(verbosity=2).run(suite).wasSuccessful() else 1 + ) + if args.report is None: + parser.error("--report is required unless --self-test is used") + + repository = os.environ.get("GITHUB_REPOSITORY", "") + token = os.environ.get("GH_TOKEN", "") + api_url = os.environ.get("GITHUB_API_URL", "https://api.github.com") + if not re.fullmatch(r"[^/\s]+/[^/\s]+", repository): + parser.error("GITHUB_REPOSITORY must be owner/repository") + if not token: + parser.error("GH_TOKEN is required") + return check_freshness(args.config, args.report, repository, token, api_url) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/check_test_wiring.py b/scripts/check_test_wiring.py index 4edd41f0d..aa97dc944 100755 --- a/scripts/check_test_wiring.py +++ b/scripts/check_test_wiring.py @@ -10,11 +10,17 @@ import sys import tempfile import tomllib import unittest +from datetime import datetime, timezone from unittest import mock from pathlib import Path +from zoneinfo import ZoneInfo, ZoneInfoNotFoundError ROOT = Path(__file__).resolve().parents[1] +SCHEDULED_ALERT_WORKFLOWS = tuple( + item["workflow"] + for item in json.loads((ROOT / ".github/scheduled-validations.json").read_text()) +) def words(value: str) -> set[str]: @@ -252,6 +258,292 @@ def check_profile_definitions(root: Path) -> list[str]: return errors +def yaml_block(lines: list[str], key: str, indent: int) -> list[str] | None: + try: + start = lines.index(f"{' ' * indent}{key}:") + 1 + except ValueError: + return None + end = next( + ( + index + for index in range(start, len(lines)) + if lines[index].strip() + and not lines[index].lstrip().startswith("#") + and len(lines[index]) - len(lines[index].lstrip()) <= indent + ), + len(lines), + ) + return lines[start:end] + + +def workflow_step_block(job_lines: list[str], action: str) -> tuple[int, list[str]] | None: + uses_index = next( + ( + index + for index, line in enumerate(job_lines) + if ( + line.split("#", 1)[0].strip() == f"- uses: {action}" + and len(line) - len(line.lstrip()) == 6 + ) + or ( + line.split("#", 1)[0].strip() == f"uses: {action}" + and len(line) - len(line.lstrip()) == 8 + ) + ), + None, + ) + if uses_index is None: + return None + start = next( + ( + index + for index in range(uses_index, -1, -1) + if job_lines[index].lstrip().startswith("- ") + ), + uses_index, + ) + indent = len(job_lines[start]) - len(job_lines[start].lstrip()) + end = next( + ( + index + for index in range(start + 1, len(job_lines)) + if len(job_lines[index]) - len(job_lines[index].lstrip()) == indent + and job_lines[index].lstrip().startswith("- ") + ), + len(job_lines), + ) + return start, job_lines[start:end] + + +def alert_step_errors( + job_lines: list[str], + expected_action_if: str | None, + required_permissions: tuple[str, ...], + required_action_tokens: tuple[str, ...], +) -> list[str]: + checkout = workflow_step_block(job_lines, "actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0") + action = workflow_step_block(job_lines, "./.github/actions/schedule-failure-issue") + errors: list[str] = [] + permissions = yaml_block(job_lines, "permissions", 4) + permission_text = "\n".join(line.split("#", 1)[0] for line in permissions or []) + missing_permissions = [token for token in required_permissions if token not in permission_text] + if missing_permissions: + errors.append("alert job permissions missing " + ", ".join(missing_permissions)) + if checkout is None: + errors.append("checkout step is missing") + if action is None: + errors.append("local alert action step is missing") + if checkout is None or action is None: + return errors + + if checkout[0] >= action[0]: + errors.append("checkout must run before the local alert action") + checkout_ifs = [line.strip() for line in checkout[1] if line.strip().startswith("if:")] + if checkout_ifs: + errors.append("checkout step must not be conditional") + action_ifs = [line.strip() for line in action[1] if line.strip().startswith("if:")] + expected_ifs = [] if expected_action_if is None else [expected_action_if] + if action_ifs != expected_ifs: + errors.append("alert action has an invalid step condition") + action_text = "\n".join(line.split("#", 1)[0] for line in action[1]) + missing_action_tokens = [token for token in required_action_tokens if token not in action_text] + if missing_action_tokens: + errors.append("alert action inputs missing " + ", ".join(missing_action_tokens)) + return errors + + +def schedule_utc_slots(hour: int, minute: int, timezone_name: str | None) -> set[tuple[int, int]]: + if timezone_name is None: + return {(hour, minute)} + zone = ZoneInfo(timezone_name) + return { + (utc.hour, utc.minute) + for year in (2025, 2026) + for month in range(1, 13) + for utc in [datetime(year, month, 1, hour, minute, tzinfo=zone).astimezone(timezone.utc)] + } + + +def check_scheduled_alerts(root: Path) -> list[str]: + errors: list[str] = [] + schedule_slots: dict[tuple[int, int], list[str]] = {} + for relative in SCHEDULED_ALERT_WORKFLOWS: + path = root / relative + try: + lines = path.read_text().splitlines() + except FileNotFoundError: + errors.append(f"{relative}: missing scheduled validation workflow") + continue + + on_block = yaml_block(lines, "on", 0) + schedule_block = yaml_block(on_block or [], "schedule", 2) + schedule_lines = schedule_block or [] + cron_indices = [index for index, line in enumerate(schedule_lines) if re.match(r"^\s*-\s+cron:", line)] + if not cron_indices: + errors.append(f"{relative}: missing simple numeric schedule") + else: + for position, cron_index in enumerate(cron_indices): + cron_line = schedule_lines[cron_index] + schedule = re.match(r"^\s*-\s+cron:\s*[\"']?(\d+)\s+(\d+)\s+", cron_line) + if not schedule: + errors.append(f"{relative}: missing simple numeric schedule") + continue + minute, hour = map(int, schedule.groups()) + if minute == 0: + errors.append(f"{relative}: scheduled validation must avoid minute zero") + entry_end = cron_indices[position + 1] if position + 1 < len(cron_indices) else len(schedule_lines) + entry = "\n".join(schedule_lines[cron_index + 1 : entry_end]) + timezone_match = re.search(r"^\s*timezone:\s*[\"']?([^\"'\s]+)", entry, re.MULTILINE) + timezone_name = timezone_match.group(1) if timezone_match else None + try: + utc_slots = schedule_utc_slots(hour, minute, timezone_name) + except ZoneInfoNotFoundError: + errors.append(f"{relative}: unknown schedule timezone {timezone_name}") + continue + for slot in utc_slots: + schedule_slots.setdefault(slot, []).append(relative) + + job_lines = yaml_block(lines, "alert-on-failure", 2) + if job_lines is None: + errors.append(f"{relative}: missing alert-on-failure job") + continue + job = "\n".join(line.split("#", 1)[0] for line in job_lines) + required = ( + "always()", + "github.event_name == 'schedule'", + "contains(needs.*.result, 'failure')", + "issues: write", + "uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + "uses: ./.github/actions/schedule-failure-issue", + "github-token: ${{ secrets.GITHUB_TOKEN }}", + ) + missing = [token for token in required if token not in job] + if missing: + errors.append(f"{relative}: alert-on-failure missing {', '.join(missing)}") + else: + errors.extend( + f"{relative}: {error}" + for error in alert_step_errors(job_lines, None, ("issues: write",), ("github-token: ${{ secrets.GITHUB_TOKEN }}",)) + ) + + for (hour, minute), workflows in schedule_slots.items(): + if len(workflows) > 1: + errors.append( + f"scheduled validations share {hour:02d}:{minute:02d} UTC: {', '.join(workflows)}" + ) + + watchdog_path = root / ".github/workflows/scheduled-validation-watchdog.yml" + try: + watchdog_lines = watchdog_path.read_text().splitlines() + except FileNotFoundError: + errors.append(".github/workflows/scheduled-validation-watchdog.yml: missing completion watchdog") + return errors + watchdog_on = yaml_block(watchdog_lines, "on", 0) + watchdog_run = yaml_block(watchdog_on or [], "workflow_run", 2) + watchdog_workflows = yaml_block(watchdog_run or [], "workflows", 4) + if watchdog_workflows is None: + errors.append(".github/workflows/scheduled-validation-watchdog.yml: missing workflow_run workflows") + return errors + watchdog_sources = "\n".join(line.split("#", 1)[0] for line in watchdog_workflows) + for relative in SCHEDULED_ALERT_WORKFLOWS: + path = root / relative + if not path.is_file(): + continue + source = path.read_text() + match = re.search(r"^name:\s*[\"']?([^\"'\n]+)", source, re.MULTILINE) + if not match: + errors.append(f"{relative}: missing workflow name") + elif f'- "{match.group(1).strip()}"' not in watchdog_sources: + errors.append(f"{relative}: missing from scheduled completion watchdog") + watchdog_job_lines = yaml_block(watchdog_lines, "alert-on-incomplete-run", 2) + if watchdog_job_lines is None: + errors.append(".github/workflows/scheduled-validation-watchdog.yml: missing alert-on-incomplete-run job") + return errors + watchdog_job = "\n".join(line.split("#", 1)[0] for line in watchdog_job_lines) + required = ( + "github.event.workflow_run.event == 'schedule'", + "github.event.workflow_run.conclusion != 'success'", + "github.event.workflow_run.conclusion != 'failure'", + "actions: read", + "issues: write", + "uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + "uses: ./.github/actions/schedule-failure-issue", + "github-token: ${{ secrets.GITHUB_TOKEN }}", + "workflow-name: ${{ github.event.workflow_run.name }}", + "source-run-id: ${{ github.event.workflow_run.id }}", + "source-run-attempt: ${{ github.event.workflow_run.run_attempt }}", + "source-event: ${{ github.event.workflow_run.event }}", + "source-ref-name: ${{ github.event.workflow_run.head_branch }}", + "source-sha: ${{ github.event.workflow_run.head_sha }}", + ) + missing = [token for token in required if token not in watchdog_job] + if missing: + errors.append( + ".github/workflows/scheduled-validation-watchdog.yml: missing " + ", ".join(missing) + ) + else: + errors.extend( + ".github/workflows/scheduled-validation-watchdog.yml: " + error + for error in alert_step_errors( + watchdog_job_lines, + None, + ("actions: read", "issues: write"), + ( + "github-token: ${{ secrets.GITHUB_TOKEN }}", + "workflow-name: ${{ github.event.workflow_run.name }}", + "source-run-id: ${{ github.event.workflow_run.id }}", + "source-run-attempt: ${{ github.event.workflow_run.run_attempt }}", + "source-event: ${{ github.event.workflow_run.event }}", + "source-ref-name: ${{ github.event.workflow_run.head_branch }}", + "source-sha: ${{ github.event.workflow_run.head_sha }}", + ), + ) + ) + + freshness_path = root / ".github/workflows/scheduled-validation-freshness.yml" + try: + freshness_lines = freshness_path.read_text().splitlines() + except FileNotFoundError: + errors.append(".github/workflows/scheduled-validation-freshness.yml: missing freshness check") + return errors + freshness_job_lines = yaml_block(freshness_lines, "check-freshness", 2) + if freshness_job_lines is None: + errors.append(".github/workflows/scheduled-validation-freshness.yml: missing check-freshness job") + return errors + freshness_job = "\n".join(line.split("#", 1)[0] for line in freshness_job_lines) + required = ( + "python3 scripts/check_scheduled_validation_freshness.py", + "actions: read", + "issues: write", + "if: failure()", + "uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + "uses: ./.github/actions/schedule-failure-issue", + "github-token: ${{ secrets.GITHUB_TOKEN }}", + "details-file: ${{ runner.temp }}/scheduled-validation-freshness.md", + ) + missing = [token for token in required if token not in freshness_job] + if missing: + errors.append( + ".github/workflows/scheduled-validation-freshness.yml: missing " + ", ".join(missing) + ) + else: + errors.extend( + ".github/workflows/scheduled-validation-freshness.yml: " + error + for error in alert_step_errors( + freshness_job_lines, + "if: failure()", + ("actions: read", "issues: write"), + ( + "github-token: ${{ secrets.GITHUB_TOKEN }}", + "details-file: ${{ runner.temp }}/scheduled-validation-freshness.md", + ), + ) + ) + if not (root / "scripts/check_scheduled_validation_freshness.py").is_file(): + errors.append("scripts/check_scheduled_validation_freshness.py: missing freshness checker") + return errors + + def check_profile_listing(root: Path, profile: str, listing: Path) -> list[str]: try: expected_digest = profile_selection(root, profile) @@ -281,6 +573,7 @@ def validate(root: Path) -> list[str]: errors.extend(check_runner_selection(root)) errors.extend(check_s3_tests_runner(root)) errors.extend(check_profile_definitions(root)) + errors.extend(check_scheduled_alerts(root)) return errors @@ -363,6 +656,7 @@ class SelfTests(unittest.TestCase): mock.patch(__name__ + ".check_fuzz_targets", return_value=[]), mock.patch(__name__ + ".check_runner_selection", return_value=[]), mock.patch(__name__ + ".check_profile_definitions", return_value=[]), + mock.patch(__name__ + ".check_scheduled_alerts", return_value=[]), ): self.assertEqual(len(validate(root)), 1) @@ -413,6 +707,272 @@ class SelfTests(unittest.TestCase): with mock.patch.object(sys, "platform", "linux"): self.assertEqual(len(check_profile_listing(root, "e2e-full", listing)), 1) + def test_scheduled_alerts_require_completion_watchdog(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + alert = ( + " alert-on-failure:\n" + " if: always() && github.event_name == 'schedule' && " + "contains(needs.*.result, 'failure')\n" + " permissions:\n" + " issues: write\n" + " steps:\n" + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " - uses: ./.github/actions/schedule-failure-issue\n" + " with:\n" + " github-token: ${{ secrets.GITHUB_TOKEN }}\n" + ) + names: list[str] = [] + for index, relative in enumerate(SCHEDULED_ALERT_WORKFLOWS, start=1): + path = root / relative + path.parent.mkdir(parents=True, exist_ok=True) + names.append(path.stem) + path.write_text( + f'name: "{path.stem}"\n' + f'on:\n schedule:\n - cron: "{index} {index} * * *"\n' + f'jobs:\n{alert}' + ) + watchdog = root / ".github/workflows/scheduled-validation-watchdog.yml" + watchdog.write_text( + "on:\n workflow_run:\n workflows:\n" + + "\n".join(f' - "{name}"' for name in names) + + "\njobs:\n" + + " alert-on-incomplete-run:\n" + + " github.event.workflow_run.event == 'schedule'\n" + + " github.event.workflow_run.conclusion != 'success'\n" + + " github.event.workflow_run.conclusion != 'failure'\n" + + " permissions:\n" + + " actions: read\n" + + " issues: write\n" + + " steps:\n" + + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + + " - uses: ./.github/actions/schedule-failure-issue\n" + + " with:\n" + + " github-token: ${{ secrets.GITHUB_TOKEN }}\n" + + " workflow-name: ${{ github.event.workflow_run.name }}\n" + + " source-run-id: ${{ github.event.workflow_run.id }}\n" + + " source-run-attempt: ${{ github.event.workflow_run.run_attempt }}\n" + + " source-event: ${{ github.event.workflow_run.event }}\n" + + " source-ref-name: ${{ github.event.workflow_run.head_branch }}\n" + + " source-sha: ${{ github.event.workflow_run.head_sha }}\n" + ) + freshness = root / ".github/workflows/scheduled-validation-freshness.yml" + freshness.write_text( + "jobs:\n" + " check-freshness:\n" + " permissions:\n" + " actions: read\n" + " issues: write\n" + " steps:\n" + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " - run: python3 scripts/check_scheduled_validation_freshness.py\n" + " - uses: ./.github/actions/schedule-failure-issue\n" + " if: failure()\n" + " with:\n" + " github-token: ${{ secrets.GITHUB_TOKEN }}\n" + " details-file: ${{ runner.temp }}/scheduled-validation-freshness.md\n" + ) + checker = root / "scripts/check_scheduled_validation_freshness.py" + checker.parent.mkdir() + checker.write_text("") + self.assertEqual(check_scheduled_alerts(root), []) + + first = root / SCHEDULED_ALERT_WORKFLOWS[0] + mutations = ( + ("contains(needs.*.result, 'failure')", "false"), + ("issues: write", "issues: read"), + ( + "uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + "uses: actions/checkout@missing", + ), + ( + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n", + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " if: github.event_name == 'workflow_dispatch'\n", + ), + ( + " - uses: ./.github/actions/schedule-failure-issue\n", + " - uses: ./.github/actions/schedule-failure-issue\n" + " if: github.event_name == 'workflow_dispatch'\n", + ), + ("uses: ./.github/actions/schedule-failure-issue", "uses: actions/checkout@v7"), + ("github-token: ${{ secrets.GITHUB_TOKEN }}", "github-token: missing"), + ) + for required, replacement in mutations: + original = first.read_text() + first.write_text(original.replace(required, replacement)) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text(original) + + first_original = first.read_text() + real_steps = ( + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " - uses: ./.github/actions/schedule-failure-issue\n" + " with:\n" + " github-token: ${{ secrets.GITHUB_TOKEN }}\n" + ) + first.write_text( + first_original.replace( + real_steps, + " - run: |\n" + " : <<'MARKER'\n" + " uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " MARKER\n" + " - run: |\n" + " : <<'MARKER'\n" + " uses: ./.github/actions/schedule-failure-issue\n" + " github-token: ${{ secrets.GITHUB_TOKEN }}\n" + " MARKER\n", + ) + ) + self.assertTrue(check_scheduled_alerts(root)) + first.write_text( + first_original.replace( + real_steps, + " - uses: ./.github/actions/schedule-failure-issue\n" + " with:\n" + " github-token: ${{ secrets.GITHUB_TOKEN }}\n" + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n", + ) + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text(first_original) + + watchdog_mutations = ( + ("actions: read", "actions: none"), + ("issues: write", "issues: read"), + ( + "uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + "uses: actions/checkout@missing", + ), + ( + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n", + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " if: github.event_name == 'workflow_dispatch'\n", + ), + ( + " - uses: ./.github/actions/schedule-failure-issue\n", + " - uses: ./.github/actions/schedule-failure-issue\n" + " if: github.event_name == 'workflow_dispatch'\n", + ), + ("uses: ./.github/actions/schedule-failure-issue", "uses: actions/checkout@v7"), + ("github-token: ${{ secrets.GITHUB_TOKEN }}", "github-token: missing"), + ("source-event: ${{ github.event.workflow_run.event }}", "source-event: watchdog"), + ( + "source-ref-name: ${{ github.event.workflow_run.head_branch }}", + "source-ref-name: main", + ), + ("source-sha: ${{ github.event.workflow_run.head_sha }}", "source-sha: missing"), + ) + for required, replacement in watchdog_mutations: + original = watchdog.read_text() + watchdog.write_text(original.replace(required, replacement)) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + watchdog.write_text(original) + + watchdog_original = watchdog.read_text() + watchdog.write_text( + watchdog_original.replace("issues: write", "issues: read") + + " decoy:\n permissions:\n issues: write\n" + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + watchdog.write_text(watchdog_original) + + first_original = first.read_text() + first.write_text( + first_original.replace(' schedule:\n - cron: "1 1 * * *"\n', "") + + ' decoy:\n strategy:\n matrix:\n cron:\n - "1 1 * * *"\n' + + ' runs-on: ubuntu-latest\n steps:\n - run: true\n' + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text(first_original) + + first.write_text( + first_original.replace( + ' - cron: "1 1 * * *"\n', + ' - cron: "1 1 * * *"\n - cron: "0 5 * * *"\n', + ) + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text( + first_original.replace( + ' - cron: "1 1 * * *"\n', + ' - cron: "1 1 * * *"\n - cron: "2 2 * * *"\n', + ) + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text(first_original) + + watchdog.write_text( + watchdog_original.replace(f' - "{names[0]}"\n', "") + + f' decoy:\n strategy:\n matrix:\n workflow:\n - "{names[0]}"\n' + + ' runs-on: ubuntu-latest\n steps:\n - run: true\n' + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + watchdog.write_text(watchdog_original) + + watchdog.write_text(watchdog_original.replace(f' - "{names[0]}"\n', "")) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + watchdog.write_text(watchdog_original) + original = first.read_text() + first.write_text(re.sub(r'- cron: "\d+ \d+', '- cron: "0 0', original, count=1)) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text(original) + + second = root / SCHEDULED_ALERT_WORKFLOWS[1] + second_original = second.read_text() + second.write_text(re.sub(r'- cron: "\d+ \d+', '- cron: "1 1', second_original, count=1)) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + second.write_text(second_original) + + first.write_text( + first_original.replace( + ' - cron: "1 1 * * *"\n', + ' - cron: "7 0 * * *"\n timezone: "Asia/Shanghai"\n', + ) + ) + second.write_text( + second_original.replace( + ' - cron: "2 2 * * *"\n', + ' - cron: "2 2 * * *"\n - cron: "7 16 * * *"\n', + ) + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + first.write_text(first_original) + second.write_text(second_original) + + freshness_original = freshness.read_text() + freshness.write_text(freshness_original.replace("details-file:", "report-file:")) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + freshness.write_text( + freshness_original.replace("github-token: ${{ secrets.GITHUB_TOKEN }}", "github-token: missing") + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + freshness.write_text( + freshness_original.replace( + "uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + "uses: actions/checkout@missing", + ) + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + freshness.write_text( + freshness_original.replace( + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n", + " - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0\n" + " if: github.event_name == 'workflow_dispatch'\n", + ) + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + freshness.write_text( + freshness_original.replace("if: failure()", "if: github.event_name == 'workflow_dispatch'") + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + freshness.write_text( + freshness_original.replace("issues: write", "issues: read") + + " decoy:\n permissions:\n issues: write\n" + ) + self.assertEqual(len(check_scheduled_alerts(root)), 1) + def main() -> int: if sys.argv[1:] == ["--self-test"]: suite = unittest.defaultTestLoader.loadTestsFromTestCase(SelfTests) @@ -436,7 +996,7 @@ def main() -> int: for error in errors: print(f"ERROR: {error}", file=sys.stderr) return 1 - print("OK: e2e modules, runner selection, fuzz matrices, profiles, and bounded diagnostics are wired") + print("OK: e2e modules, runner selection, fuzz matrices, profiles, and scheduled alerts are wired") return 0 From 51e369be6c13a48d87569b9d6fa3eddfeff019cd Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Sun, 23 Aug 2026 01:40:44 +0800 Subject: [PATCH 05/12] fix(policy): accept legacy bucket policy ID field (#6362) --- crates/policy/src/policy/policy.rs | 42 +++++++++++++++++++- docs/architecture/compat-cleanup-register.md | 1 + 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index d412fa789..946f930a8 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -195,7 +195,8 @@ pub struct BucketPolicyArgs<'a> { #[derive(Serialize, Deserialize, Clone, Default, Debug)] #[serde(deny_unknown_fields)] pub struct BucketPolicy { - #[serde(default, rename = "Id", skip_serializing_if = "ID::is_empty")] + // RUSTFS_COMPAT_TODO(rustfs-6339): accept bucket policies persisted with the legacy "ID" key. Remove after migration tooling rewrites every retained legacy bucket policy. + #[serde(default, rename = "Id", alias = "ID", skip_serializing_if = "ID::is_empty")] pub id: ID, #[serde(rename = "Version")] pub version: String, @@ -2786,7 +2787,7 @@ mod test { let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should parse"); // Verify empty fields are omitted - assert!(!parsed.as_object().unwrap().contains_key("ID"), "Empty ID should be omitted"); + assert!(parsed.get("Id").is_none(), "Empty ID should be omitted"); let statement = &parsed["Statement"][0]; assert!(!statement.as_object().unwrap().contains_key("Sid"), "Empty Sid should be omitted"); @@ -2809,6 +2810,43 @@ mod test { assert_eq!(statement["Principal"]["AWS"], "*"); } + #[test] + fn test_bucket_policy_deserializes_legacy_id() { + let legacy_policy = br#"{"ID":"","Version":"2012-10-17","Statement":[{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"NotAction":[],"Resource":["arn:aws:s3:::bucket/*"],"NotResource":[],"Condition":{}}]}"#; + + let policy: BucketPolicy = + serde_json::from_slice(legacy_policy).expect("bucket policy with legacy ID should deserialize"); + assert!(policy.id.is_empty()); + policy.is_valid().expect("legacy bucket policy should remain valid"); + + let policy: BucketPolicy = serde_json::from_str(r#"{"ID":"legacy-policy","Version":"2012-10-17","Statement":[]}"#) + .expect("non-empty legacy ID should deserialize"); + assert_eq!(policy.id.0, "legacy-policy"); + + let serialized = serde_json::to_value(&policy).expect("bucket policy should serialize"); + assert_eq!(serialized["Id"], "legacy-policy"); + assert!(serialized.get("ID").is_none(), "legacy ID spelling should not be serialized"); + } + + #[test] + fn test_bucket_policy_legacy_id_alias_remains_strict() { + let unknown_field = r#"{"Version":"2012-10-17","Statement":[],"Unexpected":true}"#; + let error = + serde_json::from_str::(unknown_field).expect_err("unrelated unknown fields should remain rejected"); + assert!( + error.to_string().contains("unknown field `Unexpected`"), + "unexpected deserialization error: {error}" + ); + + let duplicate_id = r#"{"Id":"current-policy","ID":"legacy-policy","Version":"2012-10-17","Statement":[]}"#; + let error = serde_json::from_str::(duplicate_id) + .expect_err("canonical and legacy ID fields should not be accepted together"); + assert!( + error.to_string().contains("duplicate field `Id`"), + "unexpected deserialization error: {error}" + ); + } + #[test] fn test_existing_object_tag_condition_helpers() { let identity_policy = Policy::parse_config( diff --git a/docs/architecture/compat-cleanup-register.md b/docs/architecture/compat-cleanup-register.md index 9904b41c9..d7ecec7f5 100644 --- a/docs/architecture/compat-cleanup-register.md +++ b/docs/architecture/compat-cleanup-register.md @@ -12,6 +12,7 @@ for later deletion. ## Open Items +- `rustfs-6339` legacy bucket policy ID casing: earlier RustFS releases persisted the top-level policy identifier as "ID", while current writes use the S3-compatible "Id" spelling. Readers accept both spellings so retained bucket metadata remains usable after upgrade. Remove the legacy alias after migration tooling has rewritten every retained bucket policy using "ID". - `table-publication-fence-v1` table publication fencing: nodes that predate table and table-bucket publication fences can mutate live files while a new node is publishing a catalog pointer. New nodes retain exact object guards until the operator confirms that every serving node uses the new fences. Fleet confirmation also requires non-overlapping active warehouse prefixes and lifecycle workers that exclude table buckets. Remove the exact live-file fallback and the fleet-confirmation gate after the minimum supported RustFS release acquires table fences for registered-table mutations and table-bucket fences for unresolved-prefix mutations. - `table-catalog-strong-snapshot-v1` durable strong catalog snapshot compatibility: version 1 writes continue during mixed-version rollout until operators confirm that every serving node reads version 2, and version 1 table/view identifier collisions remain available only for cleanup. Remove version 1 writes and collision cleanup after the minimum supported RustFS release reads version 2 and every retained durable strong snapshot is collision-free and has been upgraded to version 2. - `table-catalog-migration-fence-v1` durable strong migration fence compatibility: version 1 "PREPARING" fences did not distinguish a known-absent global strong snapshot from an unknown baseline, so retries read them but fail closed if the global snapshot is missing. Version 2 preserves the same JSON shape and records the pre-migration global snapshot ETag in the existing target_snapshot_etag field while the fence is "PREPARING". Remove version 1 reads after every supported direct-upgrade source writes version 2 fences and operators have completed or cancelled every older in-progress backing migration. From 26e6508b64401131127a35aebede0d56ffa34507 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:42:11 +0800 Subject: [PATCH 06/12] fix(ecstore): reject equal-time latest identity conflicts before index fallback (#6374) --- crates/ecstore/src/store/rebalance.rs | 387 +++++++++++++++++- crates/ecstore/src/store/rebalance/support.rs | 167 +++++++- 2 files changed, 532 insertions(+), 22 deletions(-) diff --git a/crates/ecstore/src/store/rebalance.rs b/crates/ecstore/src/store/rebalance.rs index 7d7c1d91e..dc2fa2ee5 100644 --- a/crates/ecstore/src/store/rebalance.rs +++ b/crates/ecstore/src/store/rebalance.rs @@ -859,6 +859,7 @@ fn lifecycle_delete_all_test_failure(phase: crate::object_api::LifecycleDeleteAl #[cfg(test)] mod tests { use super::*; + use crate::bucket::replication::{ReplicationStatusType, VersionPurgeStatusType}; use crate::config::storageclass::{CLASS_RRS, CLASS_STANDARD, lookup_config_for_pools_without_env}; use crate::disk::error::DiskError; use crate::layout::endpoint::Endpoint; @@ -1423,6 +1424,14 @@ mod tests { } } + fn object_info_with_identity(unix_ts: i64, delete_marker: bool, version_id: Uuid, etag: Option) -> ObjectInfo { + ObjectInfo { + version_id: Some(version_id), + etag, + ..object_info_with_mod_time(unix_ts, delete_marker) + } + } + #[test] fn resolve_latest_object_info_candidates_returns_latest_delete_marker() { let candidates = vec![ @@ -1446,7 +1455,7 @@ mod tests { } #[test] - fn resolve_latest_object_info_candidates_prefers_higher_pool_idx_on_equal_mod_time() { + fn resolve_latest_object_info_candidates_prefers_higher_pool_idx_on_equal_mod_time_for_equivalent_candidates() { let candidates = vec![ LatestObjectInfoCandidate { info: Some(object_info_with_mod_time(10, false)), @@ -1466,6 +1475,382 @@ mod tests { assert_eq!(idx, 1); } + #[test] + fn resolve_latest_object_info_candidates_keeps_index_fallback_for_fully_equivalent_identities() { + let candidates = vec![ + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string()))), + idx: 2, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string()))), + idx: 7, + err: None, + }, + ]; + + let (info, idx) = resolve_latest_object_info_candidates(candidates, "bucket", "object", &ObjectOptions::default()) + .expect("equivalent replicas must resolve deterministically"); + + assert_eq!(idx, 7); + assert_eq!(info.version_id, Some(Uuid::from_u128(1))); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_equal_time_version_id_conflict() { + let candidates = vec![ + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string()))), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(2), Some("etag-a".to_string()))), + idx: 1, + err: None, + }, + ]; + + let err = resolve_latest_object_info_candidates(candidates, "bucket", "object", &ObjectOptions::default()) + .expect_err("divergent version ids must not silently resolve to the higher pool index"); + + assert_eq!(err, Error::ErasureReadQuorum); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_equal_time_etag_conflict() { + let candidates = vec![ + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-old".to_string()))), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-new".to_string()))), + idx: 1, + err: None, + }, + ]; + + let err = resolve_latest_object_info_candidates(candidates, "bucket", "object", &ObjectOptions::default()) + .expect_err("divergent etags must not silently resolve to the higher pool index"); + + assert_eq!(err, Error::ErasureReadQuorum); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_equal_time_delete_marker_conflict() { + let candidates = vec![ + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), None)), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, true, Uuid::from_u128(1), Some("etag-a".to_string()))), + idx: 1, + err: None, + }, + ]; + + let err = resolve_latest_object_info_candidates(candidates, "bucket", "object", &ObjectOptions::default()) + .expect_err("a delete marker tied with a live version must not be masked by the pool index"); + + assert_eq!(err, Error::ErasureReadQuorum); + } + + fn assert_equal_time_identity_conflict(left: ObjectInfo, right: ObjectInfo) { + let err = resolve_latest_object_info_candidates( + vec![ + LatestObjectInfoCandidate { + info: Some(left), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(right), + idx: 1, + err: None, + }, + ], + "bucket", + "object", + &ObjectOptions::default(), + ) + .expect_err("equal-time identity divergence must fail closed"); + + assert_eq!(err, Error::ErasureReadQuorum); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_equal_time_payload_identity_conflicts() { + let base = object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())); + + let mut data_dir = base.clone(); + data_dir.data_dir = Some(Uuid::from_u128(2)); + assert_equal_time_identity_conflict(base.clone(), data_dir); + + let mut size = base.clone(); + size.size = 1; + assert_equal_time_identity_conflict(base.clone(), size); + + let mut actual_size = base.clone(); + actual_size.actual_size = 1; + assert_equal_time_identity_conflict(base.clone(), actual_size); + + let mut checksum = base.clone(); + checksum.checksum = Some(bytes::Bytes::from_static(b"checksum")); + assert_equal_time_identity_conflict(base.clone(), checksum); + + let mut parts = base.clone(); + parts.parts = std::sync::Arc::new(vec![rustfs_filemeta::ObjectPartInfo { + etag: "part-etag".to_string(), + number: 1, + size: 1, + ..Default::default() + }]); + assert_equal_time_identity_conflict(base.clone(), parts); + + let mut transition = base; + transition.transitioned_object.tier = "tier-a".to_string(); + assert_equal_time_identity_conflict( + object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())), + transition, + ); + } + + #[test] + fn resolve_latest_object_info_candidates_accepts_internal_metadata_aliases() { + let base = object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())); + let mut rustfs_alias = base.clone(); + rustfs_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + "x-rustfs-internal-compression".to_string(), + "zstd".to_string(), + )])); + let mut minio_alias = base.clone(); + minio_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + "X-MINIO-INTERNAL-COMPRESSION".to_string(), + "zstd".to_string(), + )])); + + let (_, idx) = resolve_latest_object_info_candidates( + vec![ + LatestObjectInfoCandidate { + info: Some(rustfs_alias), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(minio_alias), + idx: 1, + err: None, + }, + ], + "bucket", + "object", + &ObjectOptions::default(), + ) + .expect("same-value internal aliases should resolve"); + assert_eq!(idx, 1); + + let mut dual_alias = base.clone(); + dual_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([ + ("x-rustfs-internal-compression".to_string(), "zstd".to_string()), + ("x-minio-internal-compression".to_string(), "zstd".to_string()), + ])); + let mut single_alias = base; + single_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + "x-rustfs-internal-compression".to_string(), + "zstd".to_string(), + )])); + + let (_, idx) = resolve_latest_object_info_candidates( + vec![ + LatestObjectInfoCandidate { + info: Some(dual_alias), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(single_alias), + idx: 1, + err: None, + }, + ], + "bucket", + "object", + &ObjectOptions::default(), + ) + .expect("dual-key and single-key internal metadata should resolve"); + assert_eq!(idx, 1); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_different_internal_metadata_alias_values() { + let base = object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())); + let mut rustfs_alias = base.clone(); + rustfs_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + "x-rustfs-internal-compression".to_string(), + "zstd".to_string(), + )])); + let mut minio_alias = base; + minio_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + "x-minio-internal-compression".to_string(), + "snappy".to_string(), + )])); + + assert_equal_time_identity_conflict(rustfs_alias, minio_alias); + } + + #[test] + fn resolve_latest_object_info_candidates_preserves_dynamic_internal_metadata_identity_case() { + for suffix_prefix in ["replication-reset-", "replication-delete-marker-version-"] { + let base = object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())); + let mut rustfs_alias = base.clone(); + rustfs_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + format!( + "X-RUSTFS-INTERNAL-{}{suffix}", + suffix_prefix.to_uppercase(), + suffix = "arn:aws:s3:::Bucket" + ), + "value".to_string(), + )])); + let mut minio_alias = base.clone(); + minio_alias.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + format!("x-minio-internal-{suffix_prefix}arn:aws:s3:::Bucket"), + "value".to_string(), + )])); + + let (_, idx) = resolve_latest_object_info_candidates( + vec![ + LatestObjectInfoCandidate { + info: Some(rustfs_alias.clone()), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(minio_alias), + idx: 1, + err: None, + }, + ], + "bucket", + "object", + &ObjectOptions::default(), + ) + .expect("dynamic internal aliases with the same target should resolve"); + assert_eq!(idx, 1); + + let mut different_target_case = base; + different_target_case.user_defined = std::sync::Arc::new(std::collections::HashMap::from([( + format!("x-minio-internal-{suffix_prefix}arn:aws:s3:::bucket"), + "value".to_string(), + )])); + + assert_equal_time_identity_conflict(rustfs_alias, different_target_case); + } + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_conflicting_internal_metadata_aliases_in_one_candidate() { + let base = object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())); + let mut first = base.clone(); + first.user_defined = std::sync::Arc::new(std::collections::HashMap::from([ + ("x-rustfs-internal-compression".to_string(), "zstd".to_string()), + ("x-minio-internal-compression".to_string(), "snappy".to_string()), + ])); + let mut second = base; + second.user_defined = first.user_defined.clone(); + + assert_equal_time_identity_conflict(first, second); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_replication_identity_conflict() { + let base = object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())); + + let mut replication = base.clone(); + replication.replication_status_internal = Some("PENDING".to_string()); + replication.replication_status = ReplicationStatusType::Pending; + assert_equal_time_identity_conflict(base.clone(), replication); + + let mut purge = base.clone(); + purge.version_purge_status_internal = Some("PENDING".to_string()); + purge.version_purge_status = VersionPurgeStatusType::Pending; + assert_equal_time_identity_conflict(base.clone(), purge); + + let mut decision = base; + decision.replication_decision = "replicate".to_string(); + assert_equal_time_identity_conflict( + object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string())), + decision, + ); + } + + #[test] + fn resolve_latest_object_info_candidates_rejects_none_vs_unix_epoch_mod_time() { + let mut without_mod_time = object_info_with_identity(0, false, Uuid::from_u128(1), Some("etag-a".to_string())); + without_mod_time.mod_time = None; + let with_unix_epoch = object_info_with_identity(0, false, Uuid::from_u128(1), Some("etag-a".to_string())); + + assert_equal_time_identity_conflict(without_mod_time, with_unix_epoch); + } + + #[test] + fn resolve_latest_object_info_candidates_ignores_older_identity_conflicts() { + let latest = object_info_with_identity(20, false, Uuid::from_u128(1), Some("etag-latest".to_string())); + let mut older = object_info_with_identity(10, true, Uuid::from_u128(2), Some("etag-old".to_string())); + older.data_dir = Some(Uuid::from_u128(2)); + + let (info, idx) = resolve_latest_object_info_candidates( + vec![ + LatestObjectInfoCandidate { + info: Some(latest), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: Some(older), + idx: 9, + err: None, + }, + ], + "bucket", + "object", + &ObjectOptions::default(), + ) + .expect("older identity divergence must not affect the latest candidate"); + + assert_eq!(idx, 0); + assert_eq!( + info.mod_time, + Some(OffsetDateTime::from_unix_timestamp(20).expect("operation should succeed")) + ); + } + + #[test] + fn resolve_latest_object_info_candidates_ignores_not_found_pools_when_resolving() { + let candidates = vec![ + LatestObjectInfoCandidate { + info: Some(object_info_with_identity(10, false, Uuid::from_u128(1), Some("etag-a".to_string()))), + idx: 0, + err: None, + }, + LatestObjectInfoCandidate { + info: None, + idx: 1, + err: Some(Error::ObjectNotFound("bucket".to_string(), "object".to_string())), + }, + ]; + + let (info, idx) = resolve_latest_object_info_candidates(candidates, "bucket", "object", &ObjectOptions::default()) + .expect("not-found pools must not block resolution of found candidates"); + + assert_eq!(idx, 0); + assert_eq!(info.version_id, Some(Uuid::from_u128(1))); + } + #[test] fn resolve_latest_object_info_candidates_returns_non_not_found_error() { let err = resolve_latest_object_info_candidates( diff --git a/crates/ecstore/src/store/rebalance/support.rs b/crates/ecstore/src/store/rebalance/support.rs index e37fc97bc..6e4db41b8 100644 --- a/crates/ecstore/src/store/rebalance/support.rs +++ b/crates/ecstore/src/store/rebalance/support.rs @@ -12,10 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::cmp::Ordering; +use std::collections::HashMap; use crate::error::{Error, Result, StorageError, is_err_object_not_found, is_err_version_not_found}; use crate::object_api::{ObjectInfo, ObjectOptions}; +use rustfs_utils::http::metadata_compat::{ + SUFFIX_REPLICATION_DELETE_MARKER_VERSION_ARN_PREFIX, SUFFIX_REPLICATION_RESET_ARN_PREFIX, + strip_internal_prefix_preserving_case, +}; use rustfs_utils::path::decode_dir_object; use time::OffsetDateTime; @@ -137,37 +141,158 @@ pub(super) fn rebalance_disk_set_lookup_error(pool_idx: usize, set_idx: usize, p )) } +fn latest_candidate_mod_time(candidate: &LatestObjectInfoCandidate) -> Option { + candidate + .info + .as_ref() + .map(|info| info.mod_time.unwrap_or(OffsetDateTime::UNIX_EPOCH)) +} + +fn same_transition_identity(left: &ObjectInfo, right: &ObjectInfo) -> bool { + left.transition_version_state == right.transition_version_state + && left.transitioned_object.name == right.transitioned_object.name + && left.transitioned_object.version_id == right.transitioned_object.version_id + && left.transitioned_object.tier == right.transitioned_object.tier + && left.transitioned_object.free_version == right.transitioned_object.free_version + && left.transitioned_object.status == right.transitioned_object.status +} + +#[derive(PartialEq, Eq)] +struct LatestUserDefinedIdentity { + internal: HashMap, + other: HashMap, +} + +fn normalize_internal_identity_suffix(key: &str) -> Option { + let suffix = strip_internal_prefix_preserving_case(key)?; + + for dynamic_prefix in [ + SUFFIX_REPLICATION_RESET_ARN_PREFIX, + SUFFIX_REPLICATION_DELETE_MARKER_VERSION_ARN_PREFIX, + ] { + let prefix_len = dynamic_prefix.len(); + if let (Some(prefix), Some(remainder)) = (suffix.get(..prefix_len), suffix.get(prefix_len..)) + && prefix.eq_ignore_ascii_case(dynamic_prefix) + { + return Some(format!("{dynamic_prefix}{remainder}")); + } + } + + Some(suffix.to_lowercase()) +} + +fn normalize_user_defined_identity(user_defined: &HashMap) -> Option { + let mut identity = LatestUserDefinedIdentity { + internal: HashMap::with_capacity(user_defined.len()), + other: HashMap::with_capacity(user_defined.len()), + }; + + for (key, value) in user_defined { + if let Some(suffix) = normalize_internal_identity_suffix(key) { + if identity + .internal + .insert(suffix, value.clone()) + .is_some_and(|previous| previous != *value) + { + return None; + } + } else { + identity.other.insert(key.clone(), value.clone()); + } + } + + Some(identity) +} + +fn same_user_defined_identity(left: &ObjectInfo, right: &ObjectInfo) -> bool { + match ( + normalize_user_defined_identity(&left.user_defined), + normalize_user_defined_identity(&right.user_defined), + ) { + (Some(left), Some(right)) => left == right, + _ => false, + } +} + +/// Pool-specific erasure geometry is intentionally excluded: `get_object_info` +/// returns each pool's own `data_blocks`/`parity_blocks`, so those values can +/// differ for the same object version while the selected winner still carries +/// the chosen pool's layout. `put_object_reader` is also intentionally +/// excluded because it is a transient request handle that `ObjectInfo::clone` +/// drops. Every other ObjectInfo field is part of the production-visible +/// identity and must agree before the pool index can provide a deterministic +/// tie-break. +fn same_latest_object_info_identity(left: &ObjectInfo, right: &ObjectInfo) -> bool { + left.bucket == right.bucket + && left.name == right.name + && left.storage_class == right.storage_class + && left.mod_time == right.mod_time + && left.size == right.size + && left.actual_size == right.actual_size + && left.is_dir == right.is_dir + && same_user_defined_identity(left, right) + && left.user_tags == right.user_tags + && left.version_id == right.version_id + && left.data_dir == right.data_dir + && left.delete_marker == right.delete_marker + && same_transition_identity(left, right) + && left.restore_ongoing == right.restore_ongoing + && left.restore_expires == right.restore_expires + && left.parts == right.parts + && left.is_latest == right.is_latest + && left.content_type == right.content_type + && left.content_encoding == right.content_encoding + && left.expires == right.expires + && left.num_versions == right.num_versions + && left.successor_mod_time == right.successor_mod_time + && left.etag == right.etag + && left.inlined == right.inlined + && left.metadata_only == right.metadata_only + && left.version_only == right.version_only + && left.replication_status_internal == right.replication_status_internal + && left.replication_status == right.replication_status + && left.version_purge_status_internal == right.version_purge_status_internal + && left.version_purge_status == right.version_purge_status + && left.replication_decision == right.replication_decision + && left.checksum == right.checksum +} + pub(super) fn resolve_latest_object_info_candidates( - mut candidates: Vec, + candidates: Vec, bucket: &str, object: &str, opts: &ObjectOptions, ) -> Result<(ObjectInfo, usize)> { - candidates.sort_by(|a, b| { - let a_mod = if let Some(info) = &a.info { - info.mod_time.unwrap_or(OffsetDateTime::UNIX_EPOCH) - } else { - OffsetDateTime::UNIX_EPOCH + let latest_mod_time = candidates.iter().filter_map(latest_candidate_mod_time).max(); + + if let Some(latest_mod_time) = latest_mod_time { + let mut latest_candidates = candidates + .into_iter() + .filter(|candidate| latest_candidate_mod_time(candidate) == Some(latest_mod_time)) + .collect::>(); + + latest_candidates.sort_by(|left, right| right.idx.cmp(&left.idx)); + + let Some(winner) = latest_candidates.first() else { + return Err(Error::ErasureReadQuorum); + }; + let Some(winner_info) = winner.info.as_ref() else { + return Err(Error::ErasureReadQuorum); }; - let b_mod = if let Some(info) = &b.info { - info.mod_time.unwrap_or(OffsetDateTime::UNIX_EPOCH) - } else { - OffsetDateTime::UNIX_EPOCH - }; - - if a_mod == b_mod { - return if a.idx < b.idx { Ordering::Greater } else { Ordering::Less }; + if latest_candidates.iter().skip(1).any(|candidate| { + candidate + .info + .as_ref() + .is_none_or(|info| !same_latest_object_info_identity(winner_info, info)) + }) { + return Err(Error::ErasureReadQuorum); } - b_mod.cmp(&a_mod) - }); + return Ok((winner_info.clone(), winner.idx)); + } for candidate in candidates { - if let Some(info) = candidate.info { - return Ok((info, candidate.idx)); - } - if let Some(err) = candidate.err && !is_err_object_not_found(&err) && !is_err_version_not_found(&err) From 6d85a9c6a87f456624e1819e72ac9ba327f0d9bb Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:42:58 +0800 Subject: [PATCH 07/12] ci(s3tests): stabilize HAProxy request handling (#6386) --- .github/workflows/e2e-s3tests.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/e2e-s3tests.yml b/.github/workflows/e2e-s3tests.yml index 693fa8450..1be61d73e 100644 --- a/.github/workflows/e2e-s3tests.yml +++ b/.github/workflows/e2e-s3tests.yml @@ -90,6 +90,10 @@ on: description: "Optional pytest -m expression" required: false default: "" + testexpr: + description: "Optional pytest -k expression" + required: false + default: "" schedule: # Weekly full sweep (Sunday 02:00 UTC): full suite, run against BOTH the # single-node and the 4-node distributed topologies (matrix below). @@ -116,6 +120,7 @@ env: XDIST: ${{ github.event.inputs.xdist || '4' }} MAXFAIL: ${{ github.event.inputs.maxfail || '0' }} MARKEXPR: ${{ github.event.inputs.markexpr || '' }} + TESTEXPR: ${{ github.event.inputs.testexpr || '' }} S3_SHARD_COUNT: ${{ github.event_name == 'schedule' && '4' || github.event.inputs.shard-count || '1' }} TEST_TIMEOUT: "300" @@ -269,14 +274,20 @@ jobs: EOF cat > haproxy.cfg <<'EOF' + global + log stdout format raw local0 info + defaults mode http + log global + log-format '%ci:%cp [%tr] %ft %b/%s %TR/%Tw/%Tc/%Tr/%Ta %ST %B %tsc %HM %HP' timeout connect 5s timeout client 30s timeout server 30s frontend fe_s3 bind *:9000 + option http-buffer-request default_backend be_s3 backend be_s3 @@ -314,6 +325,7 @@ jobs: XDIST="${XDIST}" \ MAXFAIL="${MAXFAIL}" \ MARKEXPR="${MARKEXPR}" \ + TESTEXPR="${TESTEXPR}" \ ./scripts/s3-tests/run.sh - name: Publish compatibility report From c6590182eda61183c5e41ac53bc23938e84426e5 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:43:27 +0800 Subject: [PATCH 08/12] ci(mint): pin manual image default (#6387) --- .github/workflows/mint.yml | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/.github/workflows/mint.yml b/.github/workflows/mint.yml index b9a7e354a..25ae02916 100644 --- a/.github/workflows/mint.yml +++ b/.github/workflows/mint.yml @@ -45,13 +45,6 @@ # docker-capable self-hosted `dind-sm-standard-2` label was the alternative but # has fewer cores and reintroduces fleet-state risk for no reliability gain. -# DISABLED. This workflow is switched off in the repository's Actions settings -# (state: disabled_manually) and does not run on any trigger, including its cron -# and workflow_dispatch. That state lives in GitHub's UI and is invisible when -# reading this file, which has already misled at least one audit — hence this -# banner. Re-enabling is a UI action; anyone doing so should first check that the -# workflow still matches the current CI layout. See rustfs/backlog#1603. -# name: mint on: @@ -70,9 +63,9 @@ on: - core - full mint-image: - description: "Mint image reference" + description: "Mint image reference (empty = pinned default)" required: false - default: "minio/mint:edge" + default: "" schedule: # Weekly, after the Sunday s3-tests full sweep (starts 02:00 UTC, up to # 3h) has finished, so the two never contend for the same runner pool. From f44b30c61a1d6b0ae41a45c782892eaa5301c3ac Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:43:52 +0800 Subject: [PATCH 09/12] ci(perf): fix nightly regression baseline (#6389) --- .github/workflows/performance-ab.yml | 159 ++++++++---------- .../security/check_performance_ab_workflow.sh | 23 ++- 2 files changed, 88 insertions(+), 94 deletions(-) diff --git a/.github/workflows/performance-ab.yml b/.github/workflows/performance-ab.yml index a7ecf1d4b..de4986388 100644 --- a/.github/workflows/performance-ab.yml +++ b/.github/workflows/performance-ab.yml @@ -22,13 +22,6 @@ # correctness cost (e.g. the #4221 fsync durability fix) is recorded, not # blocked (rustfs/backlog#935 correction 1). -# DISABLED. This workflow is switched off in the repository's Actions settings -# (state: disabled_manually) and does not run on any trigger, including its cron -# and workflow_dispatch. That state lives in GitHub's UI and is invisible when -# reading this file, which has already misled at least one audit — hence this -# banner. Re-enabling is a UI action; anyone doing so should first check that the -# workflow still matches the current CI layout. See rustfs/backlog#1603. -# name: Performance A/B on: @@ -37,7 +30,7 @@ on: workflow_dispatch: inputs: duration: - description: "warp duration per round (short by default to fit the double-build budget)" + description: "warp duration per round" required: false default: "12s" type: string @@ -46,12 +39,8 @@ on: required: false default: false type: boolean - push: - # Every main commit pre-builds and caches its release binary (perf-3) so the - # nightly A/B restores a ready baseline instead of paying the double build. - branches: [main] - permissions: + actions: read contents: read env: @@ -59,83 +48,19 @@ env: RUST_BACKTRACE: 1 jobs: - # perf-3: on every push to main, build the release binary once and cache it - # keyed by commit SHA (rustfs-baseline-). The warp-ab measurements - # restore this instead of paying the ~32min-per-side source - # build. That double build is what pushed the expanded 24-cell nightly past its - # ceiling — 2026-07-11..07-14 all cancelled on the 120min timeout. Incremental - # builds off the shared cargo cache keep each push cheap, and building on the - # same sm-standard-2 runner the A/B measures on guarantees the cached binary is - # ABI-identical. Do NOT source this from build.yml's per-merge artifact: those - # are cancelled ~7/8 of the time and are not a reliable baseline. - build-baseline-cache: - name: Build + cache baseline binary - if: github.event_name == 'push' - runs-on: sm-standard-2 - # Latest-wins: consumers only ever restore the binary for the *current* - # origin/main tip, so when pushes land faster than the ~65min build, a - # superseded build's output is dead weight — cancel it instead of stacking - # hour-long jobs on the shared runner pool. A skipped intermediate SHA at - # most costs one same-commit self-heal in the A/B job. - concurrency: - group: perf-baseline-build-main - cancel-in-progress: true - # #4806 put thin LTO + codegen-units=1 on [profile.release], pushing a - # single release build past 60min on this runner — every cache build on - # 2026-07-15 died on the old 60min ceiling ("exceeded the maximum execution - # time of 1h0m0s") and the cache never populated. The measured binary must - # keep the production profile, so the budget absorbs the build instead. - timeout-minutes: 100 - steps: - - name: Checkout repository - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - with: - persist-credentials: false - - - name: Setup Rust environment - uses: ./.github/actions/setup - with: - rust-version: stable - cache-shared-key: warp-ab-${{ hashFiles('**/Cargo.lock') }} - cache-save-if: ${{ github.ref == 'refs/heads/main' }} - - - name: Build release rustfs - run: cargo build --release --bin rustfs - - - name: Stage binary for cache - run: | - set -euo pipefail - mkdir -p baseline-bin - cp target/release/rustfs baseline-bin/rustfs - - - name: Cache baseline binary by SHA - uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 - with: - path: baseline-bin/rustfs - key: rustfs-baseline-${{ github.sha }} - warp-ab: name: Warp A/B budget gate - # Always run on schedule / manual dispatch. Never on push — that event only - # feeds build-baseline-cache above. - if: >- - github.event_name == 'schedule' || - github.event_name == 'workflow_dispatch' runs-on: sm-standard-2 - # With perf-3's cached baseline binary the common (cache-hit) nightly is - # measurement-only and finishes well under 50min. This ceiling stays - # generous only to absorb the same-commit cache-miss self-heal (~65min - # single build with the post-#4806 LTO profile + measurement). A timeout - # surfaces via the alert-on-failure job (it fires on cancelled/timed-out, - # not just failure). perf-6 recalibrates the budget once the noise study - # lands. - timeout-minutes: 120 + # A normal nightly restores the last successful binary and builds only the + # candidate; daily access keeps that cache warm. A cache miss may build both + # and needs room for the A/B run plus artifact and cache publication. + timeout-minutes: 180 steps: - name: Checkout repository uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: persist-credentials: false - fetch-depth: 0 # baseline is built from origin/main + fetch-depth: 0 # baseline may be an earlier successful scheduled head - name: Setup Rust environment uses: ./.github/actions/setup @@ -163,24 +88,55 @@ jobs: fi echo "allow_regression=$allow" >> "$GITHUB_OUTPUT" - # perf-3: resolve the commits so the cache can be keyed by SHA. The - # baseline is origin/main; the candidate is the checked-out ref. On the - # nightly (checkout == main) they are the same commit, so one cached binary - # serves both phases and the run does zero source builds. + # A failed regression run must keep comparing against the last known-good + # scheduled head. Otherwise the next nightly would absorb the regression + # into its baseline and turn green without a fix. + - name: Find last successful scheduled baseline + id: scheduled_baseline + if: github.event_name == 'schedule' + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 + with: + result-encoding: string + script: | + const { data } = await github.rest.actions.listWorkflowRuns({ + owner: context.repo.owner, + repo: context.repo.repo, + workflow_id: "performance-ab.yml", + event: "schedule", + status: "success", + per_page: 1, + }); + return data.workflow_runs[0]?.head_sha ?? ""; + + # Manual runs compare a selected ref with current main. Scheduled runs + # compare current main with the last successful scheduled head. With no + # history, the first run measures the candidate against itself and seeds + # that head only if the complete rig succeeds. - name: Resolve baseline / candidate commits id: commits + env: + SCHEDULED_BASELINE_SHA: ${{ steps.scheduled_baseline.outputs.result }} run: | set -euo pipefail - baseline_sha="$(git rev-parse origin/main)" candidate_sha="$(git rev-parse HEAD)" + if [[ "${{ github.event_name }}" == "schedule" ]]; then + baseline_sha="${SCHEDULED_BASELINE_SHA:-$candidate_sha}" + if ! git merge-base --is-ancestor "$baseline_sha" "$candidate_sha"; then + echo "::error::scheduled baseline $baseline_sha is not an ancestor of candidate $candidate_sha" >&2 + exit 1 + fi + else + baseline_sha="$(git rev-parse origin/main)" + fi + git cat-file -e "${baseline_sha}^{commit}" echo "baseline_sha=$baseline_sha" >> "$GITHUB_OUTPUT" echo "candidate_sha=$candidate_sha" >> "$GITHUB_OUTPUT" echo "baseline commit: $baseline_sha" echo "candidate commit: $candidate_sha" - # Exact-key restore of the baseline binary built by build-baseline-cache - # when origin/main last landed. A miss (binary evicted or not built yet) - # leaves cache-hit unset and the rig falls back to a source build. + # Exact-key restore of the candidate binary saved by its successful + # scheduled run. A miss leaves cache-hit unset and falls back to a source + # build of that known-good head. - name: Restore cached baseline binary id: baseline_cache uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 @@ -270,11 +226,11 @@ jobs: elif [[ "$selfheal_built" == "true" ]]; then base_src="source build (cache self-heal, saved as rustfs-baseline-$baseline_sha)" else - base_src="isolated origin/main source build (saved as rustfs-baseline-$baseline_sha)" + base_src="isolated baseline source build (saved as rustfs-baseline-$baseline_sha)" fi if [[ "$candidate_sha" == "$baseline_sha" ]]; then - # Nightly on main: the candidate is the same commit as the baseline, - # so reuse the one binary for both phases and skip all builds. + # No commits landed since the last successful baseline, so reuse + # the one binary for both phases and measure only rig drift. args+=(--candidate-bin "$base_bin") cand_src="same binary as baseline (same commit)" elif [[ "$candidate_built" == "true" ]]; then @@ -362,6 +318,23 @@ jobs: fi } >> "$GITHUB_STEP_SUMMARY" + - name: Stage successful candidate baseline + if: >- + steps.ab.outputs.status == '0' && + steps.commits.outputs.baseline_sha != steps.commits.outputs.candidate_sha + run: | + set -euo pipefail + cp candidate-bin/rustfs baseline-bin/rustfs + + - name: Cache successful candidate baseline + if: >- + steps.ab.outputs.status == '0' && + steps.commits.outputs.baseline_sha != steps.commits.outputs.candidate_sha + uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 + with: + path: baseline-bin/rustfs + key: rustfs-baseline-${{ steps.commits.outputs.candidate_sha }} + # Scheduled failure alerting is handled by the alert-on-failure job below # (perf-2 consuming ci-8's schedule-failure-issue composite action). diff --git a/scripts/security/check_performance_ab_workflow.sh b/scripts/security/check_performance_ab_workflow.sh index 5465e4fdf..09b97cc0d 100755 --- a/scripts/security/check_performance_ab_workflow.sh +++ b/scripts/security/check_performance_ab_workflow.sh @@ -13,8 +13,29 @@ require_absent_pattern() { fi } +require_present_pattern() { + local pattern="$1" + local description="$2" + + if ! grep -Eq -- "$pattern" "$workflow"; then + echo "invalid performance A/B workflow contract: $description" >&2 + exit 1 + fi +} + require_absent_pattern '(^|[^[:alnum:]_])pull_request(_target)?([^[:alnum:]_]|$)' "the workflow must not contain PR event handling" require_absent_pattern 'pull-requests[[:space:]]*:[[:space:]]*write' "the workflow must not receive PR write permission" require_absent_pattern 'permissions[[:space:]]*:[[:space:]]*write-all' "the workflow must not receive broad write permission" +require_absent_pattern '^[[:space:]]*push:' "the workflow must not spend a release build on every main push" +require_present_pattern 'listWorkflowRuns' "the scheduled baseline must come from workflow history" +require_present_pattern 'status:[[:space:]]*"success"' "the scheduled baseline must be a successful run" +require_present_pattern 'SCHEDULED_BASELINE_SHA' "the resolved scheduled baseline must reach the comparison" +require_present_pattern "SCHEDULED_BASELINE_SHA:-\\\$candidate_sha" "the first scheduled run must seed from its verified candidate" +require_present_pattern 'git merge-base --is-ancestor' "the scheduled baseline must stay on candidate history" +require_present_pattern 'Cache successful candidate baseline' "a successful candidate must become the next cached baseline" +if ! sed -n '/^ warp-ab:/,/^ alert-on-failure:/p' "$workflow" | grep -Eq '^ timeout-minutes:[[:space:]]*180([[:space:]]|$)'; then + echo "invalid performance A/B workflow contract: the cold-cache path must fit both builds, the A/B run, and evidence publication" >&2 + exit 1 +fi -echo "Performance A/B workflow trust boundary ok." +echo "Performance A/B workflow contract ok." From 7ba6f8cb33a3eeb875321ea08d245e751d10c292 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:44:35 +0800 Subject: [PATCH 10/12] test(e2e): fix cluster nightly oracles (#6397) --- .github/workflows/e2e-replication-nightly.yml | 3 ++ crates/e2e_test/src/object_lambda_test.rs | 37 +++++++++++++------ .../stale_multipart_cleanup_cluster_test.rs | 37 +++++++------------ 3 files changed, 41 insertions(+), 36 deletions(-) diff --git a/.github/workflows/e2e-replication-nightly.yml b/.github/workflows/e2e-replication-nightly.yml index ef312d180..145ad317e 100644 --- a/.github/workflows/e2e-replication-nightly.yml +++ b/.github/workflows/e2e-replication-nightly.yml @@ -196,6 +196,9 @@ jobs: cache-save-if: 'false' install-build-packaging-tools: 'false' + - name: Verify protocol socket oracle + run: ss -tn state CLOSE-WAIT >/dev/null + # The suite owns fixed protocol ports and serializes its internal cases. - name: Verify protocol e2e membership env: diff --git a/crates/e2e_test/src/object_lambda_test.rs b/crates/e2e_test/src/object_lambda_test.rs index aa6f7d4a1..d2d5f0036 100644 --- a/crates/e2e_test/src/object_lambda_test.rs +++ b/crates/e2e_test/src/object_lambda_test.rs @@ -16,6 +16,7 @@ use crate::common::{RustFSTestClusterEnvironment, RustFSTestEnvironment, init_lo use aws_sdk_s3::primitives::ByteStream; use http::header::{CONTENT_TYPE, HOST}; use reqwest::StatusCode; +use rustfs_config::{ENV_DRIVE_ACTIVE_CHECK_INTERVAL_SECS, ENV_NOTIFY_ENABLE}; use rustfs_signer::pre_sign_v4; use rustfs_utils::egress::ENV_OUTBOUND_ALLOW_ORIGINS; use s3s::Body; @@ -976,7 +977,8 @@ async fn test_get_object_lambda_rejects_disabled_target() -> Result<(), Box Result<(), Box Resul init_logging(); let mut env = RustFSTestEnvironment::new().await?; - env.start_rustfs_server(vec![]).await?; + env.start_rustfs_server_with_env(vec![], &[(ENV_NOTIFY_ENABLE, "true")]) + .await?; let bucket = "object-lambda-e2e-invalid-endpoint"; @@ -1064,7 +1074,8 @@ async fn test_configure_object_lambda_notify_webhook_rejects_response_header_tim init_logging(); let mut env = RustFSTestEnvironment::new().await?; - env.start_rustfs_server(vec![]).await?; + env.start_rustfs_server_with_env(vec![], &[(ENV_NOTIFY_ENABLE, "true")]) + .await?; let response = send_configure_webhook_target_request( &env, @@ -1173,6 +1184,8 @@ async fn test_listen_notification_fans_in_remote_node_events() -> Result<(), Box init_logging(); let mut cluster = RustFSTestClusterEnvironment::new(2).await?; + cluster.set_env(ENV_NOTIFY_ENABLE, "true"); + cluster.set_env(ENV_DRIVE_ACTIVE_CHECK_INTERVAL_SECS, "1"); cluster.start().await?; let bucket = "listen-notification-cluster"; diff --git a/crates/e2e_test/src/stale_multipart_cleanup_cluster_test.rs b/crates/e2e_test/src/stale_multipart_cleanup_cluster_test.rs index c5fc45e17..82d0924bc 100644 --- a/crates/e2e_test/src/stale_multipart_cleanup_cluster_test.rs +++ b/crates/e2e_test/src/stale_multipart_cleanup_cluster_test.rs @@ -15,7 +15,6 @@ use crate::common::{RustFSTestClusterEnvironment, init_logging}; use aws_sdk_s3::error::SdkError; use aws_sdk_s3::primitives::ByteStream; -use aws_sdk_s3::types::CompletedMultipartUpload; use tokio::time::{Duration, sleep}; use tracing::info; use uuid::Uuid; @@ -43,32 +42,18 @@ async fn list_parts_reports_missing_upload( } } -async fn complete_reports_missing_upload( +async fn multipart_listing_reports_missing_upload( client: &aws_sdk_s3::Client, bucket: &str, key: &str, upload_id: &str, ) -> Result> { - let result = client - .complete_multipart_upload() - .bucket(bucket) - .key(key) - .upload_id(upload_id) - .multipart_upload(CompletedMultipartUpload::builder().build()) - .send() - .await; - match result { - Ok(_) => Ok(false), - Err(SdkError::ServiceError(err)) => { - let code = err.err().meta().code().unwrap_or(""); - if code == "NoSuchUpload" { - Ok(true) - } else { - Err(format!("unexpected complete_multipart_upload service error: code={code}, err={err:?}").into()) - } - } - Err(err) => Err(format!("unexpected complete_multipart_upload error: {err:?}").into()), - } + let result = client.list_multipart_uploads().bucket(bucket).prefix(key).send().await?; + + Ok(!result + .uploads() + .iter() + .any(|upload| upload.key() == Some(key) && upload.upload_id() == Some(upload_id))) } async fn wait_for_cleanup_on_all_nodes( @@ -81,8 +66,8 @@ async fn wait_for_cleanup_on_all_nodes( let mut all_cleaned = true; for (idx, client) in clients.iter().enumerate() { let list_parts_missing = list_parts_reports_missing_upload(client, bucket, key, upload_id).await?; - let complete_missing = complete_reports_missing_upload(client, bucket, key, upload_id).await?; - if !(list_parts_missing && complete_missing) { + let listing_missing = multipart_listing_reports_missing_upload(client, bucket, key, upload_id).await?; + if !(list_parts_missing && listing_missing) { info!("stale multipart still visible on node {} at attempt {}", idx, attempt + 1); all_cleaned = false; break; @@ -146,6 +131,10 @@ async fn test_stale_multipart_cleanup_removes_incomplete_upload_across_cluster() 1, "multipart upload should be visible before background cleanup" ); + assert!( + !multipart_listing_reports_missing_upload(&clients[2], CLEANUP_BUCKET, &key, &upload_id).await?, + "multipart upload listing should contain the upload before background cleanup" + ); wait_for_cleanup_on_all_nodes(&clients, CLEANUP_BUCKET, &key, &upload_id).await?; From b91845c98cdf55e17170e8338bba772f81480e75 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:44:50 +0800 Subject: [PATCH 11/12] ci: align cache writer and reader keys (#6398) --- .github/workflows/cache-warm.yml | 9 ++++++--- scripts/security/check_cache_save_if.sh | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/.github/workflows/cache-warm.yml b/.github/workflows/cache-warm.yml index a9902fc4d..660d96b09 100644 --- a/.github/workflows/cache-warm.yml +++ b/.github/workflows/cache-warm.yml @@ -94,6 +94,9 @@ concurrency: env: CARGO_TERM_COLOR: always + # Swatinem/rust-cache hashes every RUST* variable. Keep this aligned with + # ci.yml or the writer and readers use disjoint cache keys. + RUST_BACKTRACE: 1 jobs: # Readers: test-and-lint, test-ilm-integration-serial, build-rustfs-debug-binary, @@ -101,7 +104,7 @@ jobs: warm-ci-dev: name: Warm ci-dev runs-on: sm-standard-4 - timeout-minutes: 90 + timeout-minutes: 120 env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" steps: @@ -191,7 +194,7 @@ jobs: warm-ci-feat-rio: name: Warm ci-feat-rio runs-on: sm-standard-4 - timeout-minutes: 90 + timeout-minutes: 120 env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" steps: @@ -219,7 +222,7 @@ jobs: warm-ci-feat-proto: name: Warm ci-feat-proto runs-on: sm-standard-4 - timeout-minutes: 90 + timeout-minutes: 120 env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" steps: diff --git a/scripts/security/check_cache_save_if.sh b/scripts/security/check_cache_save_if.sh index 3e63bdc8a..27abc6545 100755 --- a/scripts/security/check_cache_save_if.sh +++ b/scripts/security/check_cache_save_if.sh @@ -64,3 +64,25 @@ if [ "$status" -ne 0 ]; then fi echo "OK: every ./.github/actions/setup call states cache-save-if explicitly" + +# rust-cache hashes every CARGO*, CC*, CFLAGS*, CXX*, CMAKE*, and RUST* +# variable that is present when the setup action runs. The dedicated writer +# and the CI readers therefore need identical workflow-level compiler env. +compiler_env() { + awk ' + /^env:[[:space:]]*$/ { in_env = 1; next } + in_env && /^[^[:space:]]/ { exit } + in_env && /^ (CARGO|CC|CFLAGS|CXX|CMAKE|RUST)[A-Z0-9_]*:/ { print } + ' "$1" | sort +} + +ci_env="$(compiler_env .github/workflows/ci.yml)" +warm_env="$(compiler_env .github/workflows/cache-warm.yml)" + +if [ "$ci_env" != "$warm_env" ]; then + echo "CI and cache-warm compiler environments differ; rust-cache keys will not match:" >&2 + diff -u <(printf '%s\n' "$ci_env") <(printf '%s\n' "$warm_env") >&2 || true + exit 1 +fi + +echo "OK: cache-warm and CI compiler environments match" From 2d7120460b327fc2bb9bbd701e2fb1cb5cfacfba Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 23 Aug 2026 01:45:15 +0800 Subject: [PATCH 12/12] test(e2e): remove fake KMS suite results (#6401) --- .config/e2e-full-selection.txt | 4 +- crates/e2e_test/src/kms/mod.rs | 3 - crates/e2e_test/src/kms/test_runner.rs | 499 ------------------------- docs/testing/e2e-suite-inventory.md | 4 +- 4 files changed, 4 insertions(+), 506 deletions(-) delete mode 100644 crates/e2e_test/src/kms/test_runner.rs diff --git a/.config/e2e-full-selection.txt b/.config/e2e-full-selection.txt index bb9022327..dfad0f0bd 100644 --- a/.config/e2e-full-selection.txt +++ b/.config/e2e-full-selection.txt @@ -1,2 +1,2 @@ -sha256-darwin=b4ae71aa894e5c7795ae3eb8116f1777a7601d0f5db3898be2e48faf3329bd9b -sha256-linux=433debd9d9defa832986269abdf0f1d131597b2d7a417ce930e17c1fd47d85ba +sha256-darwin=9f767b37ed8b1c82da62ea441462d75487785c8086e56f08fb6f6cd89c6e2e52 +sha256-linux=fbdaf42b220958d4b1e8880e0f8b5a7992d38e21051bb60596dd4538424757d6 diff --git a/crates/e2e_test/src/kms/mod.rs b/crates/e2e_test/src/kms/mod.rs index 5e6b9fe19..3b849fa1b 100644 --- a/crates/e2e_test/src/kms/mod.rs +++ b/crates/e2e_test/src/kms/mod.rs @@ -39,9 +39,6 @@ mod kms_edge_cases_test; #[cfg(test)] mod kms_fault_recovery_test; -#[cfg(test)] -mod test_runner; - #[cfg(test)] mod bucket_default_encryption_test; diff --git a/crates/e2e_test/src/kms/test_runner.rs b/crates/e2e_test/src/kms/test_runner.rs deleted file mode 100644 index 558c14631..000000000 --- a/crates/e2e_test/src/kms/test_runner.rs +++ /dev/null @@ -1,499 +0,0 @@ -// Copyright 2024 RustFS Team -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -#![allow(dead_code)] -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Unified KMS test suite runner -//! -//! This module provides a unified interface for running KMS tests with categorization, -//! filtering, and comprehensive reporting capabilities. - -use crate::common::init_logging; -use std::time::Instant; -use tokio::time::{Duration, sleep}; -use tracing::{debug, error, info, warn}; - -/// Test category for organization and filtering -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub enum TestCategory { - CoreFunctionality, - MultipartEncryption, - EdgeCases, - FaultRecovery, - Comprehensive, - Performance, -} - -impl TestCategory { - pub fn as_str(&self) -> &'static str { - match self { - TestCategory::CoreFunctionality => "core-functionality", - TestCategory::MultipartEncryption => "multipart-encryption", - TestCategory::EdgeCases => "edge-cases", - TestCategory::FaultRecovery => "fault-recovery", - TestCategory::Comprehensive => "comprehensive", - TestCategory::Performance => "performance", - } - } -} - -/// Test definition with metadata -#[derive(Debug, Clone)] -pub struct TestDefinition { - pub name: String, - pub description: String, - pub category: TestCategory, - pub estimated_duration: Duration, - pub is_critical: bool, -} - -impl TestDefinition { - pub fn new( - name: impl Into, - description: impl Into, - category: TestCategory, - estimated_duration: Duration, - is_critical: bool, - ) -> Self { - Self { - name: name.into(), - description: description.into(), - category, - estimated_duration, - is_critical, - } - } -} - -/// Test execution result -#[derive(Debug, Clone)] -pub struct TestResult { - pub test_name: String, - pub category: TestCategory, - pub success: bool, - pub duration: Duration, - pub error_message: Option, -} - -impl TestResult { - pub fn success(test_name: String, category: TestCategory, duration: Duration) -> Self { - Self { - test_name, - category, - success: true, - duration, - error_message: None, - } - } - - pub fn failure(test_name: String, category: TestCategory, duration: Duration, error: String) -> Self { - Self { - test_name, - category, - success: false, - duration, - error_message: Some(error), - } - } -} - -/// Comprehensive test suite configuration -#[derive(Debug, Clone)] -pub struct TestSuiteConfig { - pub categories: Vec, - pub include_critical_only: bool, - pub max_duration: Option, - pub parallel_execution: bool, -} - -impl Default for TestSuiteConfig { - fn default() -> Self { - Self { - categories: vec![ - TestCategory::CoreFunctionality, - TestCategory::MultipartEncryption, - TestCategory::EdgeCases, - TestCategory::FaultRecovery, - TestCategory::Comprehensive, - ], - include_critical_only: false, - max_duration: None, - parallel_execution: false, - } - } -} - -/// Unified KMS test suite runner -pub struct KMSTestSuite { - tests: Vec, - config: TestSuiteConfig, -} - -impl KMSTestSuite { - /// Create a new test suite with default configuration - pub fn new() -> Self { - let tests = vec![ - // Core Functionality Tests - TestDefinition::new( - "test_local_kms_end_to_end", - "End-to-end KMS test with all encryption types", - TestCategory::CoreFunctionality, - Duration::from_secs(60), - true, - ), - TestDefinition::new( - "test_local_kms_key_isolation", - "Test KMS key isolation and security", - TestCategory::CoreFunctionality, - Duration::from_secs(45), - true, - ), - // Multipart Encryption Tests - TestDefinition::new( - "test_local_kms_multipart_upload", - "Test large file multipart upload with encryption", - TestCategory::MultipartEncryption, - Duration::from_secs(120), - true, - ), - TestDefinition::new( - "test_step1_basic_single_file_encryption", - "Basic single file encryption test", - TestCategory::MultipartEncryption, - Duration::from_secs(30), - false, - ), - TestDefinition::new( - "test_step2_basic_multipart_upload_without_encryption", - "Basic multipart upload without encryption", - TestCategory::MultipartEncryption, - Duration::from_secs(45), - false, - ), - TestDefinition::new( - "test_step3_multipart_upload_with_sse_s3", - "Multipart upload with SSE-S3 encryption", - TestCategory::MultipartEncryption, - Duration::from_secs(60), - true, - ), - TestDefinition::new( - "test_step4_large_multipart_upload_with_encryption", - "Large file multipart upload with encryption", - TestCategory::MultipartEncryption, - Duration::from_secs(90), - false, - ), - TestDefinition::new( - "test_step5_all_encryption_types_multipart", - "All encryption types multipart test", - TestCategory::MultipartEncryption, - Duration::from_secs(120), - true, - ), - // Edge Cases Tests - TestDefinition::new( - "test_kms_zero_byte_file_encryption", - "Test encryption of zero-byte files", - TestCategory::EdgeCases, - Duration::from_secs(20), - false, - ), - TestDefinition::new( - "test_kms_single_byte_file_encryption", - "Test encryption of single-byte files", - TestCategory::EdgeCases, - Duration::from_secs(20), - false, - ), - TestDefinition::new( - "test_kms_multipart_boundary_conditions", - "Test multipart upload boundary conditions", - TestCategory::EdgeCases, - Duration::from_secs(45), - false, - ), - TestDefinition::new( - "test_kms_invalid_key_scenarios", - "Test invalid key scenarios", - TestCategory::EdgeCases, - Duration::from_secs(30), - false, - ), - TestDefinition::new( - "test_kms_concurrent_encryption", - "Test concurrent encryption operations", - TestCategory::EdgeCases, - Duration::from_secs(60), - false, - ), - TestDefinition::new( - "test_kms_key_validation_security", - "Test key validation security", - TestCategory::EdgeCases, - Duration::from_secs(30), - false, - ), - // Fault Recovery Tests - TestDefinition::new( - "test_kms_key_directory_unavailable", - "Test KMS when key directory is unavailable", - TestCategory::FaultRecovery, - Duration::from_secs(45), - false, - ), - TestDefinition::new( - "test_kms_corrupted_key_files", - "Test KMS with corrupted key files", - TestCategory::FaultRecovery, - Duration::from_secs(30), - false, - ), - TestDefinition::new( - "test_kms_multipart_upload_interruption", - "Test multipart upload interruption recovery", - TestCategory::FaultRecovery, - Duration::from_secs(60), - false, - ), - TestDefinition::new( - "test_kms_resource_constraints", - "Test KMS under resource constraints", - TestCategory::FaultRecovery, - Duration::from_secs(90), - false, - ), - // Comprehensive Tests - TestDefinition::new( - "test_comprehensive_kms_full_workflow", - "Full KMS workflow comprehensive test", - TestCategory::Comprehensive, - Duration::from_secs(300), - true, - ), - TestDefinition::new( - "test_comprehensive_stress_test", - "KMS stress test with large datasets", - TestCategory::Comprehensive, - Duration::from_secs(400), - false, - ), - TestDefinition::new( - "test_comprehensive_key_isolation", - "Comprehensive key isolation test", - TestCategory::Comprehensive, - Duration::from_secs(180), - false, - ), - TestDefinition::new( - "test_comprehensive_concurrent_operations", - "Comprehensive concurrent operations test", - TestCategory::Comprehensive, - Duration::from_secs(240), - false, - ), - TestDefinition::new( - "test_comprehensive_performance_benchmark", - "KMS performance benchmark test", - TestCategory::Comprehensive, - Duration::from_secs(360), - false, - ), - ]; - - Self { - tests, - config: TestSuiteConfig::default(), - } - } - - /// Configure the test suite - pub fn with_config(mut self, config: TestSuiteConfig) -> Self { - self.config = config; - self - } - - /// Filter tests based on category - pub fn filter_by_category(&self, category: &TestCategory) -> Vec<&TestDefinition> { - self.tests.iter().filter(|test| &test.category == category).collect() - } - - /// Filter tests based on criticality - pub fn filter_critical_tests(&self) -> Vec<&TestDefinition> { - self.tests.iter().filter(|test| test.is_critical).collect() - } - - /// Get test summary by category - pub fn get_category_summary(&self) -> std::collections::HashMap> { - let mut summary = std::collections::HashMap::new(); - for test in &self.tests { - summary.entry(test.category.clone()).or_insert_with(Vec::new).push(test); - } - summary - } - - /// Run the complete test suite - pub async fn run_test_suite(&self) -> Vec { - init_logging(); - info!("🚀 Starting unified KMS test suite"); - - let start_time = Instant::now(); - let mut results = Vec::new(); - - // Filter tests based on configuration - let tests_to_run: Vec<&TestDefinition> = self - .tests - .iter() - .filter(|test| self.config.categories.contains(&test.category)) - .filter(|test| !self.config.include_critical_only || test.is_critical) - .collect(); - - info!("📊 Test plan: {} test(s) scheduled", tests_to_run.len()); - for (i, test) in tests_to_run.iter().enumerate() { - info!(" {}. {} ({})", i + 1, test.name, test.category.as_str()); - } - - // Execute tests - for (i, test_def) in tests_to_run.iter().enumerate() { - info!("🧪 Running test {}/{}: {}", i + 1, tests_to_run.len(), test_def.name); - info!(" 📝 Description: {}", test_def.description); - info!(" 🏷️ Category: {}", test_def.category.as_str()); - info!(" ⏱️ Estimated duration: {:?}", test_def.estimated_duration); - - let test_start = Instant::now(); - let result = self.run_single_test(test_def).await; - let test_duration = test_start.elapsed(); - - match result { - Ok(_) => { - info!("✅ Test passed: {} ({:.2}s)", test_def.name, test_duration.as_secs_f64()); - results.push(TestResult::success(test_def.name.clone(), test_def.category.clone(), test_duration)); - } - Err(e) => { - error!("❌ Test failed: {} ({:.2}s): {}", test_def.name, test_duration.as_secs_f64(), e); - results.push(TestResult::failure( - test_def.name.clone(), - test_def.category.clone(), - test_duration, - e.to_string(), - )); - } - } - - // Add delay between tests to avoid resource conflicts - if i < tests_to_run.len() - 1 { - debug!("⏸️ Waiting two seconds before the next test..."); - sleep(Duration::from_secs(2)).await; - } - } - - let total_duration = start_time.elapsed(); - self.print_test_summary(&results, total_duration); - - results - } - - /// Run a single test by dispatching to the appropriate test function - async fn run_single_test(&self, test_def: &TestDefinition) -> Result<(), Box> { - // This is a placeholder for test dispatch logic - // In a real implementation, this would dispatch to actual test functions - warn!("⚠️ Test '{}' is not implemented in the unified runner; skipping", test_def.name); - Ok(()) - } - - /// Print comprehensive test summary - fn print_test_summary(&self, results: &[TestResult], total_duration: Duration) { - info!("📊 KMS test suite summary"); - info!("⏱️ Total duration: {:.2} seconds", total_duration.as_secs_f64()); - info!("📈 Total tests: {}", results.len()); - - let passed = results.iter().filter(|r| r.success).count(); - let failed = results.iter().filter(|r| !r.success).count(); - - info!("✅ Passed: {}", passed); - info!("❌ Failed: {}", failed); - info!("📊 Success rate: {:.1}%", (passed as f64 / results.len() as f64) * 100.0); - - // Summary by category - let mut category_summary: std::collections::HashMap = std::collections::HashMap::new(); - for result in results { - let (total, passed_count) = category_summary.entry(result.category.clone()).or_insert((0, 0)); - *total += 1; - if result.success { - *passed_count += 1; - } - } - - info!("📊 Category summary:"); - for (category, (total, passed_count)) in category_summary { - info!( - " 🏷️ {}: {}/{} ({:.1}%)", - category.as_str(), - passed_count, - total, - (passed_count as f64 / total as f64) * 100.0 - ); - } - - // List failed tests - if failed > 0 { - warn!("❌ Failing tests:"); - for result in results.iter().filter(|r| !r.success) { - warn!(" - {}: {}", result.test_name, result.error_message.as_deref().unwrap_or("Unknown error")); - } - } - } -} - -/// Quick test suite for critical tests only -#[tokio::test] -async fn test_kms_critical_suite() -> Result<(), Box> { - let config = TestSuiteConfig { - categories: vec![TestCategory::CoreFunctionality, TestCategory::MultipartEncryption], - include_critical_only: true, - max_duration: Some(Duration::from_secs(600)), // 10 minutes max - parallel_execution: false, - }; - - let suite = KMSTestSuite::new().with_config(config); - let results = suite.run_test_suite().await; - - let failed_count = results.iter().filter(|r| !r.success).count(); - if failed_count > 0 { - return Err(format!("Critical test suite failed: {failed_count} tests failed").into()); - } - - info!("✅ All critical tests passed"); - Ok(()) -} - -/// Full comprehensive test suite -#[tokio::test] -async fn test_kms_full_suite() -> Result<(), Box> { - let suite = KMSTestSuite::new(); - let results = suite.run_test_suite().await; - - let total_tests = results.len(); - let failed_count = results.iter().filter(|r| !r.success).count(); - let success_rate = ((total_tests - failed_count) as f64 / total_tests as f64) * 100.0; - - info!("📊 Full suite success rate: {:.1}%", success_rate); - - // Allow up to 10% failure rate for non-critical tests - if success_rate < 90.0 { - return Err(format!("Test suite success rate too low: {success_rate:.1}%").into()); - } - - info!("✅ Full test suite succeeded"); - Ok(()) -} diff --git a/docs/testing/e2e-suite-inventory.md b/docs/testing/e2e-suite-inventory.md index 7d80a802d..df2fc0847 100644 --- a/docs/testing/e2e-suite-inventory.md +++ b/docs/testing/e2e-suite-inventory.md @@ -58,7 +58,7 @@ | heal_erasure_disk_rebuild_test | 4 | 🌙 | | inline_fast_path_cluster_test | 16 | | | internode_rpc_signature_e2e_test | 5 | | -| kms | 48 | | +| kms | 46 | | | leading_slash_key_test | 2 | ✅ | | lifecycle_regression_test | 4 | | | list_buckets_auth_test | 1 | ✅ | @@ -99,4 +99,4 @@ | tls_hot_reload_test | 1 | ✅ | | version_id_regression_test | 10 | ✅ | -**Total listed: 577 tests across 82 modules · PR smoke: 163 tests / 36 modules · merge/main full: 455 tests / 73 modules · nightly replication: 55 tests · nightly cluster faults: 28 tests / 7 modules · nightly protocols: 16 tests** · updated 2026-08-21. +**Total listed: 575 tests across 82 modules · PR smoke: 163 tests / 36 modules · merge/main full: 453 tests / 73 modules · nightly replication: 55 tests · nightly cluster faults: 28 tests / 7 modules · nightly protocols: 16 tests** · updated 2026-08-23.