From 28edfd6190d7c3d310ec9acce5827d22ec515bb7 Mon Sep 17 00:00:00 2001 From: houseme Date: Thu, 16 Apr 2026 17:21:45 +0800 Subject: [PATCH] fix(storage): harden offline drive fail-fast paths (#2564) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: houseme <4829346+houseme@users.noreply.github.com> --- Cargo.lock | 3 + crates/common/src/heal_channel.rs | 82 +- crates/config/README.md | 12 + crates/config/src/constants/drive.rs | 60 ++ crates/config/src/constants/heal.rs | 52 ++ crates/config/src/constants/mod.rs | 1 + crates/config/src/constants/scanner.rs | 9 + crates/config/src/lib.rs | 2 + .../ecstore/src/cache_value/metacache_set.rs | 92 ++- crates/ecstore/src/disk/disk_store.rs | 392 +++++++++- crates/ecstore/src/disk/health_state.rs | 237 ++++++ crates/ecstore/src/disk/mod.rs | 26 + crates/ecstore/src/rpc/remote_disk.rs | 442 +++++++++-- crates/ecstore/src/rpc/remote_locker.rs | 192 ++++- crates/ecstore/src/set_disk.rs | 547 ++++++++++--- crates/ecstore/src/set_disk/lock.rs | 241 +++++- crates/heal/Cargo.toml | 2 + crates/heal/src/error.rs | 8 + crates/heal/src/heal/channel.rs | 150 +++- crates/heal/src/heal/erasure_healer.rs | 259 ++++-- crates/heal/src/heal/manager.rs | 738 ++++++++++++++++-- crates/heal/src/heal/storage.rs | 83 +- crates/heal/src/heal/task.rs | 83 +- crates/heal/tests/heal_bug_fixes_test.rs | 149 ++++ crates/heal/tests/heal_integration_test.rs | 4 +- crates/iam/src/lib.rs | 2 +- crates/iam/src/oidc.rs | 142 +++- crates/lock/src/client/local.rs | 2 +- crates/lock/src/distributed_lock.rs | 261 +++++-- crates/lock/src/namespace/tests.rs | 146 ++++ crates/madmin/src/info_commands.rs | 8 + crates/rio/src/http_reader.rs | 95 ++- crates/scanner/Cargo.toml | 1 + crates/scanner/src/scanner_folder.rs | 432 ++++++++-- crates/scanner/src/scanner_io.rs | 89 ++- 35 files changed, 4426 insertions(+), 618 deletions(-) create mode 100644 crates/config/src/constants/drive.rs create mode 100644 crates/ecstore/src/disk/health_state.rs diff --git a/Cargo.lock b/Cargo.lock index b9ca1da76..e36849dbe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8029,6 +8029,7 @@ dependencies = [ "async-trait", "futures", "http 1.4.0", + "metrics", "rustfs-common", "rustfs-config", "rustfs-ecstore", @@ -8037,6 +8038,7 @@ dependencies = [ "serde", "serde_json", "serial_test", + "temp-env", "tempfile", "thiserror 2.0.18", "tokio", @@ -8492,6 +8494,7 @@ dependencies = [ "chrono", "futures", "http 1.4.0", + "metrics", "path-clean", "rand 0.10.1", "rmp-serde", diff --git a/crates/common/src/heal_channel.rs b/crates/common/src/heal_channel.rs index 348bdc9f5..7b7d585ef 100644 --- a/crates/common/src/heal_channel.rs +++ b/crates/common/src/heal_channel.rs @@ -18,7 +18,7 @@ use std::{ fmt::{self, Display}, sync::OnceLock, }; -use tokio::sync::{broadcast, mpsc}; +use tokio::sync::{broadcast, mpsc, oneshot}; use uuid::Uuid; pub const HEAL_DELETE_DANGLING: bool = true; @@ -206,11 +206,59 @@ pub struct HealOpts { pub set: Option, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum HealAdmissionDropReason { + QueueFull, + PolicyDropped, +} + +impl HealAdmissionDropReason { + pub fn as_str(self) -> &'static str { + match self { + Self::QueueFull => "queue_full", + Self::PolicyDropped => "policy_dropped", + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum HealAdmissionResult { + Accepted, + Merged, + Full, + Dropped(HealAdmissionDropReason), +} + +impl HealAdmissionResult { + pub fn result_label(self) -> &'static str { + match self { + Self::Accepted => "accepted", + Self::Merged => "merged", + Self::Full => "full", + Self::Dropped(_) => "dropped", + } + } + + pub fn reason_label(self) -> &'static str { + match self { + Self::Dropped(reason) => reason.as_str(), + _ => "none", + } + } + + pub fn is_admitted(self) -> bool { + matches!(self, Self::Accepted | Self::Merged) + } +} + /// Heal channel command type -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum HealChannelCommand { /// Start a new heal task - Start(HealChannelRequest), + Start { + request: HealChannelRequest, + response_tx: oneshot::Sender>, + }, /// Query heal task status Query { heal_path: String, client_token: String }, /// Cancel heal task @@ -339,9 +387,22 @@ pub fn subscribe_heal_responses() -> broadcast::Receiver { heal_response_sender().subscribe() } +/// Send heal start request and wait for structured admission feedback. +pub async fn send_heal_request_with_admission(request: HealChannelRequest) -> Result { + let (response_tx, response_rx) = oneshot::channel(); + send_heal_command(HealChannelCommand::Start { request, response_tx }).await?; + response_rx + .await + .map_err(|e| format!("Failed to receive heal admission response: {e}"))? +} + /// Send heal start request pub async fn send_heal_request(request: HealChannelRequest) -> Result<(), String> { - send_heal_command(HealChannelCommand::Start(request)).await + match send_heal_request_with_admission(request).await? { + HealAdmissionResult::Accepted | HealAdmissionResult::Merged => Ok(()), + HealAdmissionResult::Full => Err("Heal request queue is full".to_string()), + HealAdmissionResult::Dropped(reason) => Err(format!("Heal request dropped: {}", reason.as_str())), + } } /// Send heal query request @@ -542,6 +603,19 @@ pub async fn send_heal_disk(set_disk_id: String, priority: Option]) -> Pin + Send>> + Send + 'static>; type FinishedFn = Box]) -> Pin + Send>> + Send + 'static>; +#[derive(Debug)] +enum PeekOutcome { + Ready(Option), + Error(rustfs_filemeta::Error), + TimedOut, +} + +async fn peek_with_timeout(reader: &mut MetacacheReader, timeout_duration: Duration) -> PeekOutcome { + match timeout(timeout_duration, reader.peek()).await { + Ok(Ok(entry)) => PeekOutcome::Ready(entry), + Ok(Err(err)) => PeekOutcome::Error(err), + Err(_) => PeekOutcome::TimedOut, + } +} + #[derive(Default)] pub struct ListPathRawOptions { pub disks: Vec>, @@ -160,6 +179,7 @@ pub async fn list_path_raw(rx: CancellationToken, opts: ListPathRawOptions) -> d } let revjob = spawn(async move { + let peek_timeout = get_drive_walkdir_stall_timeout(); let mut errs: Vec> = Vec::with_capacity(readers.len()); for _ in 0..readers.len() { errs.push(None); @@ -191,8 +211,8 @@ pub async fn list_path_raw(rx: CancellationToken, opts: ListPathRawOptions) -> d continue; } - let entry = match r.peek().await { - Ok(res) => { + let entry = match peek_with_timeout(r, peek_timeout).await { + PeekOutcome::Ready(res) => { if let Some(entry) = res { // info!("read entry disk: {}, name: {}", i, entry.name); entry @@ -203,7 +223,7 @@ pub async fn list_path_raw(rx: CancellationToken, opts: ListPathRawOptions) -> d continue; } } - Err(err) => { + PeekOutcome::Error(err) => { if err == rustfs_filemeta::Error::Unexpected { at_eof += 1; // warn!("list_path_raw: peek err eof, disk: {}", i); @@ -236,6 +256,31 @@ pub async fn list_path_raw(rx: CancellationToken, opts: ListPathRawOptions) -> d continue; } } + PeekOutcome::TimedOut => { + has_err += 1; + errs[i] = Some(DiskError::Timeout); + let endpoint = opts + .disks + .get(i) + .and_then(|disk| disk.as_ref().map(|disk| disk.endpoint().to_string())) + .unwrap_or_else(|| "missing".to_string()); + counter!( + "rustfs_list_path_raw_stall_total", + "drive" => endpoint.clone() + ) + .increment(1); + warn!( + drive = %endpoint, + bucket = %opts.bucket, + path = %opts.path, + timeout_ms = peek_timeout.as_millis(), + "list_path_raw reader peek timed out; excluding drive from current merge" + ); + let (detached_rd, write_half) = tokio::io::duplex(1); + drop(write_half); + *r = MetacacheReader::new(detached_rd); + continue; + } }; // warn!("list_path_raw: loop entry: {:?}, disk: {}", &entry.name, i); @@ -364,3 +409,42 @@ pub async fn list_path_raw(rx: CancellationToken, opts: ListPathRawOptions) -> d // warn!("list_path_raw: done"); Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use rustfs_filemeta::MetacacheWriter; + + #[tokio::test] + async fn peek_with_timeout_times_out_on_silent_reader() { + let (_writer, reader) = tokio::io::duplex(64); + let mut reader = MetacacheReader::new(reader); + + let outcome = peek_with_timeout(&mut reader, Duration::from_millis(20)).await; + assert!(matches!(outcome, PeekOutcome::TimedOut)); + } + + #[tokio::test] + async fn peek_with_timeout_reads_entry_before_deadline() { + let (reader, writer) = tokio::io::duplex(256); + let mut metacache_reader = MetacacheReader::new(reader); + + tokio::spawn(async move { + let mut writer = MetacacheWriter::new(writer); + let entry = MetaCacheEntry { + name: "bucket/object".to_string(), + metadata: vec![1, 2, 3], + cached: None, + reusable: false, + }; + writer.write(&[entry]).await.expect("entry should be written"); + writer.close().await.expect("writer should close"); + }); + + let outcome = peek_with_timeout(&mut metacache_reader, Duration::from_secs(1)).await; + match outcome { + PeekOutcome::Ready(Some(entry)) => assert_eq!(entry.name, "bucket/object"), + other => panic!("expected ready entry, got {other:?}"), + } + } +} diff --git a/crates/ecstore/src/disk/disk_store.rs b/crates/ecstore/src/disk/disk_store.rs index 958d2c07a..9d28c05f4 100644 --- a/crates/ecstore/src/disk/disk_store.rs +++ b/crates/ecstore/src/disk/disk_store.rs @@ -16,10 +16,16 @@ use crate::disk::{ CheckPartsResp, DeleteOptions, DiskAPI, DiskError, DiskInfo, DiskInfoOptions, DiskLocation, Endpoint, Error, FileInfoVersions, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, Result, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, + health_state::{ + RuntimeDriveHealthState, classify_drive_recovery, get_drive_returning_probe_interval, + get_drive_returning_success_threshold, get_drive_suspect_failure_threshold, record_drive_offline_duration, + record_drive_recovery_class, record_drive_runtime_state, record_drive_state_transition, + }, local::{LocalDisk, ScanGuard}, }; use crate::global::GLOBAL_LOCAL_DISK_ID_MAP; use bytes::Bytes; +use metrics::counter; use rustfs_filemeta::{FileInfo, ObjectPartInfo, RawFileInfo}; use std::{ path::PathBuf, @@ -40,7 +46,6 @@ const DISK_HEALTH_FAULTY: u32 = 1; pub const ENV_RUSTFS_DRIVE_ACTIVE_MONITORING: &str = "RUSTFS_DRIVE_ACTIVE_MONITORING"; pub const DEFAULT_RUSTFS_DRIVE_ACTIVE_MONITORING: bool = true; -pub const ENV_RUSTFS_DRIVE_MAX_TIMEOUT_DURATION: &str = "RUSTFS_DRIVE_MAX_TIMEOUT_DURATION"; pub const CHECK_EVERY: Duration = Duration::from_secs(15); pub const SKIP_IF_SUCCESS_BEFORE: Duration = Duration::from_secs(5); pub const CHECK_TIMEOUT_DURATION: Duration = Duration::from_secs(5); @@ -51,9 +56,52 @@ lazy_static::lazy_static! { } pub fn get_max_timeout_duration() -> Duration { - std::env::var(ENV_RUSTFS_DRIVE_MAX_TIMEOUT_DURATION) - .map(|v| Duration::from_secs(v.parse::().unwrap_or(30))) - .unwrap_or(Duration::from_secs(30)) + Duration::from_secs(rustfs_utils::get_env_u64( + rustfs_config::ENV_DRIVE_MAX_TIMEOUT_DURATION, + rustfs_config::DEFAULT_DRIVE_MAX_TIMEOUT_DURATION_SECS, + )) +} + +fn get_drive_timeout_duration(env_key: &str, default_secs: u64) -> Duration { + Duration::from_secs( + rustfs_utils::get_env_opt_u64_with_aliases(env_key, &[rustfs_config::ENV_DRIVE_MAX_TIMEOUT_DURATION]) + .unwrap_or(default_secs), + ) +} + +pub fn get_drive_metadata_timeout() -> Duration { + get_drive_timeout_duration( + rustfs_config::ENV_DRIVE_METADATA_TIMEOUT_SECS, + rustfs_config::DEFAULT_DRIVE_METADATA_TIMEOUT_SECS, + ) +} + +pub fn get_drive_disk_info_timeout() -> Duration { + get_drive_timeout_duration( + rustfs_config::ENV_DRIVE_DISK_INFO_TIMEOUT_SECS, + rustfs_config::DEFAULT_DRIVE_DISK_INFO_TIMEOUT_SECS, + ) +} + +pub fn get_drive_list_dir_timeout() -> Duration { + get_drive_timeout_duration( + rustfs_config::ENV_DRIVE_LIST_DIR_TIMEOUT_SECS, + rustfs_config::DEFAULT_DRIVE_LIST_DIR_TIMEOUT_SECS, + ) +} + +pub fn get_drive_walkdir_timeout() -> Duration { + get_drive_timeout_duration( + rustfs_config::ENV_DRIVE_WALKDIR_TIMEOUT_SECS, + rustfs_config::DEFAULT_DRIVE_WALKDIR_TIMEOUT_SECS, + ) +} + +pub fn get_drive_walkdir_stall_timeout() -> Duration { + get_drive_timeout_duration( + rustfs_config::ENV_DRIVE_WALKDIR_STALL_TIMEOUT_SECS, + rustfs_config::DEFAULT_DRIVE_WALKDIR_STALL_TIMEOUT_SECS, + ) } /// DiskHealthTracker tracks the health status of a disk. @@ -68,6 +116,16 @@ pub struct DiskHealthTracker { pub status: AtomicU32, /// Atomic number of waiting operations pub waiting: AtomicU32, + /// Runtime drive health state + pub runtime_state: AtomicU32, + /// Consecutive failures while transitioning away from online + pub consecutive_failures: AtomicU32, + /// Consecutive successes while returning online + pub consecutive_successes: AtomicU32, + /// When the drive first left the online state + pub offline_since_unix_secs: AtomicI64, + /// Last runtime state transition timestamp + pub last_transition_unix_secs: AtomicI64, } impl DiskHealthTracker { @@ -83,6 +141,11 @@ impl DiskHealthTracker { last_started: AtomicI64::new(now), status: AtomicU32::new(DISK_HEALTH_OK), waiting: AtomicU32::new(0), + runtime_state: AtomicU32::new(RuntimeDriveHealthState::Online as u32), + consecutive_failures: AtomicU32::new(0), + consecutive_successes: AtomicU32::new(0), + offline_since_unix_secs: AtomicI64::new(0), + last_transition_unix_secs: AtomicI64::new(now / 1_000_000_000), } } @@ -110,12 +173,139 @@ impl DiskHealthTracker { self.status.store(DISK_HEALTH_OK, Ordering::Release); } + #[cfg(test)] + pub fn force_runtime_state_for_test(&self, state: RuntimeDriveHealthState) { + self.runtime_state.store(state as u32, Ordering::Release); + match state { + RuntimeDriveHealthState::Offline => self.set_faulty(), + RuntimeDriveHealthState::Online | RuntimeDriveHealthState::Suspect | RuntimeDriveHealthState::Returning => { + self.set_ok(); + } + } + } + pub fn swap_ok_to_faulty(&self) -> bool { self.status .compare_exchange(DISK_HEALTH_OK, DISK_HEALTH_FAULTY, Ordering::AcqRel, Ordering::Relaxed) .is_ok() } + pub fn runtime_state(&self) -> RuntimeDriveHealthState { + RuntimeDriveHealthState::from_u32(self.runtime_state.load(Ordering::Acquire)) + } + + pub fn offline_duration(&self) -> Option { + let offline_since = self.offline_since_unix_secs.load(Ordering::Acquire); + if offline_since <= 0 { + return None; + } + let now = current_unix_secs(); + Some(Duration::from_secs(now.saturating_sub(offline_since as u64))) + } + + pub fn mark_failure(&self, endpoint: &Endpoint, reason: &'static str) -> bool { + let current = self.runtime_state(); + let now = current_unix_secs(); + let next = match current { + RuntimeDriveHealthState::Online => { + self.consecutive_failures.store(1, Ordering::Release); + self.consecutive_successes.store(0, Ordering::Release); + self.offline_since_unix_secs + .compare_exchange(0, now as i64, Ordering::AcqRel, Ordering::Relaxed) + .ok(); + RuntimeDriveHealthState::Suspect + } + RuntimeDriveHealthState::Suspect => { + let failures = self.consecutive_failures.fetch_add(1, Ordering::AcqRel) + 1; + if failures >= get_drive_suspect_failure_threshold() { + RuntimeDriveHealthState::Offline + } else { + RuntimeDriveHealthState::Suspect + } + } + RuntimeDriveHealthState::Returning => { + self.consecutive_failures.store(0, Ordering::Release); + self.consecutive_successes.store(0, Ordering::Release); + RuntimeDriveHealthState::Offline + } + RuntimeDriveHealthState::Offline => RuntimeDriveHealthState::Offline, + }; + + self.status.store(DISK_HEALTH_FAULTY, Ordering::Release); + self.transition_state(endpoint, current, next, reason); + current == RuntimeDriveHealthState::Online + } + + pub fn mark_recovery_success(&self, endpoint: &Endpoint, reason: &'static str) -> bool { + let current = self.runtime_state(); + let next = match current { + RuntimeDriveHealthState::Online => RuntimeDriveHealthState::Online, + RuntimeDriveHealthState::Suspect => RuntimeDriveHealthState::Online, + RuntimeDriveHealthState::Offline => { + self.consecutive_successes.store(1, Ordering::Release); + RuntimeDriveHealthState::Returning + } + RuntimeDriveHealthState::Returning => { + let successes = self.consecutive_successes.fetch_add(1, Ordering::AcqRel) + 1; + if successes >= get_drive_returning_success_threshold() { + RuntimeDriveHealthState::Online + } else { + RuntimeDriveHealthState::Returning + } + } + }; + + let became_online = next == RuntimeDriveHealthState::Online; + if became_online { + self.status.store(DISK_HEALTH_OK, Ordering::Release); + self.consecutive_failures.store(0, Ordering::Release); + self.consecutive_successes.store(0, Ordering::Release); + } + self.transition_state(endpoint, current, next, reason); + if became_online { + self.log_success(); + } + became_online + } + + fn transition_state( + &self, + endpoint: &Endpoint, + current: RuntimeDriveHealthState, + next: RuntimeDriveHealthState, + reason: &'static str, + ) { + if current == next { + return; + } + + self.runtime_state.store(next as u32, Ordering::Release); + self.last_transition_unix_secs + .store(current_unix_secs() as i64, Ordering::Release); + + if matches!( + next, + RuntimeDriveHealthState::Suspect | RuntimeDriveHealthState::Offline | RuntimeDriveHealthState::Returning + ) && self.offline_since_unix_secs.load(Ordering::Acquire) == 0 + { + self.offline_since_unix_secs + .store(current_unix_secs() as i64, Ordering::Release); + } + + if next == RuntimeDriveHealthState::Online { + if let Some(duration) = self.offline_duration() { + record_drive_offline_duration(endpoint, duration); + record_drive_recovery_class(classify_drive_recovery(duration)); + } + self.offline_since_unix_secs.store(0, Ordering::Release); + } else if let Some(duration) = self.offline_duration() { + record_drive_offline_duration(endpoint, duration); + } + + record_drive_state_transition(endpoint, current, next, reason); + record_drive_runtime_state(endpoint, next); + } + /// Increment waiting operations counter pub fn increment_waiting(&self) { self.waiting.fetch_add(1, Ordering::Relaxed); @@ -137,6 +327,13 @@ impl DiskHealthTracker { } } +fn current_unix_secs() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() +} + impl Default for DiskHealthTracker { fn default() -> Self { Self::new() @@ -186,19 +383,34 @@ impl LocalDiskWrapper { let env_health_check = rustfs_utils::get_env_bool(ENV_RUSTFS_DRIVE_ACTIVE_MONITORING, DEFAULT_RUSTFS_DRIVE_ACTIVE_MONITORING); - Self { + let wrapper = Self { disk, health: Arc::new(DiskHealthTracker::new()), health_check: health_check && env_health_check, cancel_token: CancellationToken::new(), disk_id: Arc::new(RwLock::new(None)), - } + }; + record_drive_runtime_state(&wrapper.disk.endpoint(), RuntimeDriveHealthState::Online); + wrapper } pub fn get_disk(&self) -> Arc { self.disk.clone() } + pub fn runtime_state(&self) -> RuntimeDriveHealthState { + self.health.runtime_state() + } + + pub fn offline_duration_secs(&self) -> Option { + self.health.offline_duration().map(|duration| duration.as_secs()) + } + + #[cfg(test)] + pub fn force_runtime_state_for_test(&self, state: RuntimeDriveHealthState) { + self.health.force_runtime_state_for_test(state); + } + /// Enable health monitoring after disk creation. /// Used to defer health checks until after startup format loading completes. pub fn enable_health_check(&self) { @@ -219,6 +431,20 @@ impl LocalDiskWrapper { self.cancel_token.cancel(); } + fn spawn_recovery_monitor_if_needed(&self) { + if !self.health_check { + return; + } + + self.health.increment_waiting(); + let health = Arc::clone(&self.health); + let disk = Arc::clone(&self.disk); + let cancel_token = self.cancel_token.clone(); + tokio::spawn(async move { + Self::monitor_disk_status(disk, health, cancel_token).await; + }); + } + /// Monitor disk writability periodically async fn monitor_disk_writable(disk: Arc, health: Arc, cancel_token: CancellationToken) { // TODO: config interval @@ -256,7 +482,9 @@ impl LocalDiskWrapper { let test_obj = format!("health-check-{}", Uuid::new_v4()); - if Self::perform_health_check(disk.clone(), &TEST_BUCKET, &test_obj, &TEST_DATA, true, CHECK_TIMEOUT_DURATION).await.is_err() && health.swap_ok_to_faulty() { + if Self::perform_health_check(disk.clone(), &TEST_BUCKET, &test_obj, &TEST_DATA, true, CHECK_TIMEOUT_DURATION).await.is_err() + && health.mark_failure(&disk.endpoint(), "active_health_check_failed") + { // Health check failed, disk is considered faulty warn!("health check: failed, disk is considered faulty"); @@ -345,9 +573,9 @@ impl LocalDiskWrapper { /// Monitor disk status and try to bring it back online async fn monitor_disk_status(disk: Arc, health: Arc, cancel_token: CancellationToken) { - const CHECK_EVERY: Duration = Duration::from_secs(5); + let check_every = get_drive_returning_probe_interval(); - let mut interval = time::interval(CHECK_EVERY); + let mut interval = time::interval(check_every); loop { tokio::select! { @@ -362,12 +590,18 @@ impl LocalDiskWrapper { let test_obj = format!("health-check-{}", Uuid::new_v4()); match Self::perform_health_check(disk.clone(), &TEST_BUCKET, &test_obj, &TEST_DATA, false, CHECK_TIMEOUT_DURATION).await { Ok(_) => { + let state_before = health.runtime_state(); + let is_online = health.mark_recovery_success(&disk.endpoint(), "recovery_probe_success"); + info!("Disk {} recovery probe succeeded; state={:?}", disk.to_string(), state_before); + if !is_online { + continue; + } info!("Disk {} is back online", disk.to_string()); - health.set_ok(); health.decrement_waiting(); return; } Err(e) => { + health.mark_failure(&disk.endpoint(), "recovery_probe_failed"); warn!("Disk {} still faulty: {:?}", disk.to_string(), e); } } @@ -435,6 +669,19 @@ impl LocalDiskWrapper { /// Track disk health for an operation. /// This method should wrap disk operations to ensure health checking. pub async fn track_disk_health(&self, operation: F, timeout_duration: Duration) -> Result + where + F: FnOnce() -> Fut, + Fut: std::future::Future>, + { + self.track_disk_health_with_op("unknown", operation, timeout_duration).await + } + + pub async fn track_disk_health_with_op( + &self, + op: &'static str, + operation: F, + timeout_duration: Duration, + ) -> Result where F: FnOnce() -> Fut, Fut: std::future::Future>, @@ -479,7 +726,21 @@ impl LocalDiskWrapper { Err(_) => { // Timeout occurred, mark disk as potentially faulty and decrement waiting counter self.health.decrement_waiting(); - warn!("disk operation timeout after {:?}", timeout_duration); + if self.health.mark_failure(&self.endpoint(), "operation_timeout") { + self.spawn_recovery_monitor_if_needed(); + } + counter!( + "rustfs_drive_op_timeout_total", + "endpoint" => self.endpoint().to_string(), + "op" => op.to_string() + ) + .increment(1); + warn!( + endpoint = %self.endpoint(), + op, + timeout_ms = timeout_duration.as_millis(), + "Local disk operation timed out" + ); Err(DiskError::other(format!("disk operation timeout after {timeout_duration:?}"))) } } @@ -489,8 +750,12 @@ impl LocalDiskWrapper { #[async_trait::async_trait] impl DiskAPI for LocalDiskWrapper { async fn read_metadata(&self, volume: &str, path: &str) -> Result { - self.track_disk_health(|| async { self.disk.read_metadata(volume, path).await }, Duration::ZERO) - .await + self.track_disk_health_with_op( + "read_metadata", + || async { self.disk.read_metadata(volume, path).await }, + get_drive_metadata_timeout(), + ) + .await } fn start_scan(&self) -> ScanGuard { @@ -565,15 +830,22 @@ impl DiskAPI for LocalDiskWrapper { return Err(DiskError::FaultyDisk); } - let result = self.disk.disk_info(opts).await?; + self.track_disk_health_with_op( + "disk_info", + || async { + let result = self.disk.disk_info(opts).await?; - if let Some(current_disk_id) = *self.disk_id.read().await - && Some(current_disk_id) != result.id - { - return Err(DiskError::DiskNotFound); - }; + if let Some(current_disk_id) = *self.disk_id.read().await + && Some(current_disk_id) != result.id + { + return Err(DiskError::DiskNotFound); + }; - Ok(result) + Ok(result) + }, + get_drive_disk_info_timeout(), + ) + .await } async fn make_volume(&self, volume: &str) -> Result<()> { @@ -587,7 +859,7 @@ impl DiskAPI for LocalDiskWrapper { } async fn list_volumes(&self) -> Result> { - self.track_disk_health(|| async { self.disk.list_volumes().await }, Duration::ZERO) + self.track_disk_health_with_op("list_volumes", || async { self.disk.list_volumes().await }, Duration::ZERO) .await } @@ -602,7 +874,7 @@ impl DiskAPI for LocalDiskWrapper { } async fn walk_dir(&self, opts: WalkDirOptions, wr: &mut W) -> Result<()> { - self.track_disk_health(|| async { self.disk.walk_dir(opts, wr).await }, Duration::ZERO) + self.track_disk_health_with_op("walk_dir", || async { self.disk.walk_dir(opts, wr).await }, get_drive_walkdir_timeout()) .await } @@ -710,9 +982,10 @@ impl DiskAPI for LocalDiskWrapper { } async fn list_dir(&self, origvolume: &str, volume: &str, dir_path: &str, count: i32) -> Result> { - self.track_disk_health( + self.track_disk_health_with_op( + "list_dir", || async { self.disk.list_dir(origvolume, volume, dir_path, count).await }, - get_max_timeout_duration(), + get_drive_list_dir_timeout(), ) .await } @@ -802,3 +1075,74 @@ impl DiskAPI for LocalDiskWrapper { .await } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::disk::endpoint::Endpoint; + use crate::disk::health_state::RuntimeDriveHealthState; + + #[test] + fn drive_metadata_timeout_uses_default_when_unset() { + temp_env::with_var_unset(rustfs_config::ENV_DRIVE_METADATA_TIMEOUT_SECS, || { + temp_env::with_var_unset(rustfs_config::ENV_DRIVE_MAX_TIMEOUT_DURATION, || { + assert_eq!( + get_drive_metadata_timeout(), + Duration::from_secs(rustfs_config::DEFAULT_DRIVE_METADATA_TIMEOUT_SECS) + ); + }); + }); + } + + #[test] + fn drive_metadata_timeout_uses_legacy_fallback_when_canonical_unset() { + temp_env::with_var_unset(rustfs_config::ENV_DRIVE_METADATA_TIMEOUT_SECS, || { + temp_env::with_var(rustfs_config::ENV_DRIVE_MAX_TIMEOUT_DURATION, Some("17"), || { + assert_eq!(get_drive_metadata_timeout(), Duration::from_secs(17)); + }); + }); + } + + #[test] + fn drive_metadata_timeout_prefers_canonical_over_legacy() { + temp_env::with_var(rustfs_config::ENV_DRIVE_METADATA_TIMEOUT_SECS, Some("7"), || { + temp_env::with_var(rustfs_config::ENV_DRIVE_MAX_TIMEOUT_DURATION, Some("17"), || { + assert_eq!(get_drive_metadata_timeout(), Duration::from_secs(7)); + }); + }); + } + + #[test] + fn runtime_state_transitions_from_online_to_suspect_then_offline() { + let endpoint = Endpoint::try_from("/tmp/runtime-state-disk").expect("endpoint should parse"); + let health = DiskHealthTracker::new(); + + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Online); + assert!(health.mark_failure(&endpoint, "timeout")); + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Suspect); + + assert!(!health.mark_failure(&endpoint, "timeout")); + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Offline); + assert!(health.offline_duration().is_some()); + } + + #[test] + fn runtime_state_transitions_back_online_after_recovery_threshold() { + let endpoint = Endpoint::try_from("/tmp/runtime-state-recovery").expect("endpoint should parse"); + let health = DiskHealthTracker::new(); + + health.mark_failure(&endpoint, "timeout"); + health.mark_failure(&endpoint, "timeout"); + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Offline); + + assert!(!health.mark_recovery_success(&endpoint, "probe")); + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Returning); + + assert!(!health.mark_recovery_success(&endpoint, "probe")); + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Returning); + + assert!(health.mark_recovery_success(&endpoint, "probe")); + assert_eq!(health.runtime_state(), RuntimeDriveHealthState::Online); + assert!(health.offline_duration().is_none()); + } +} diff --git a/crates/ecstore/src/disk/health_state.rs b/crates/ecstore/src/disk/health_state.rs new file mode 100644 index 000000000..a2536000d --- /dev/null +++ b/crates/ecstore/src/disk/health_state.rs @@ -0,0 +1,237 @@ +// 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. + +use super::{DiskAPI, DiskStore}; +use crate::disk::endpoint::Endpoint; +use metrics::{counter, gauge}; +use std::time::Duration; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[repr(u32)] +pub enum RuntimeDriveHealthState { + Online = 0, + Suspect = 1, + Offline = 2, + Returning = 3, +} + +impl RuntimeDriveHealthState { + pub fn as_str(self) -> &'static str { + match self { + Self::Online => "online", + Self::Suspect => "suspect", + Self::Offline => "offline", + Self::Returning => "returning", + } + } + + pub fn from_u32(value: u32) -> Self { + match value { + 1 => Self::Suspect, + 2 => Self::Offline, + 3 => Self::Returning, + _ => Self::Online, + } + } + + pub fn is_snapshot_eligible(self) -> bool { + matches!(self, Self::Online | Self::Suspect | Self::Returning) + } + + pub fn is_strictly_online(self) -> bool { + matches!(self, Self::Online) + } + + pub fn should_probe_for_admin(self) -> bool { + matches!(self, Self::Online | Self::Returning) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DriveRecoveryClass { + ShortOffline, + MediumOffline, + LongOffline, +} + +impl DriveRecoveryClass { + pub fn as_str(self) -> &'static str { + match self { + Self::ShortOffline => "short_offline", + Self::MediumOffline => "medium_offline", + Self::LongOffline => "long_offline", + } + } +} + +pub fn get_drive_suspect_failure_threshold() -> u32 { + rustfs_utils::get_env_u64( + rustfs_config::ENV_DRIVE_SUSPECT_FAILURE_THRESHOLD, + rustfs_config::DEFAULT_DRIVE_SUSPECT_FAILURE_THRESHOLD, + ) as u32 +} + +pub fn get_drive_returning_success_threshold() -> u32 { + rustfs_utils::get_env_u64( + rustfs_config::ENV_DRIVE_RETURNING_SUCCESS_THRESHOLD, + rustfs_config::DEFAULT_DRIVE_RETURNING_SUCCESS_THRESHOLD, + ) as u32 +} + +pub fn get_drive_returning_probe_interval() -> Duration { + Duration::from_secs(rustfs_utils::get_env_u64( + rustfs_config::ENV_DRIVE_RETURNING_PROBE_INTERVAL_SECS, + rustfs_config::DEFAULT_DRIVE_RETURNING_PROBE_INTERVAL_SECS, + )) +} + +pub fn get_drive_offline_grace_period() -> Duration { + Duration::from_secs(rustfs_utils::get_env_u64( + rustfs_config::ENV_DRIVE_OFFLINE_GRACE_PERIOD_SECS, + rustfs_config::DEFAULT_DRIVE_OFFLINE_GRACE_PERIOD_SECS, + )) +} + +pub fn get_drive_long_offline_threshold() -> Duration { + Duration::from_secs(rustfs_utils::get_env_u64( + rustfs_config::ENV_DRIVE_LONG_OFFLINE_THRESHOLD_SECS, + rustfs_config::DEFAULT_DRIVE_LONG_OFFLINE_THRESHOLD_SECS, + )) +} + +pub fn classify_drive_recovery(duration: Duration) -> DriveRecoveryClass { + if duration <= get_drive_offline_grace_period() { + DriveRecoveryClass::ShortOffline + } else if duration >= get_drive_long_offline_threshold() { + DriveRecoveryClass::LongOffline + } else { + DriveRecoveryClass::MediumOffline + } +} + +pub fn record_drive_runtime_state(endpoint: &Endpoint, state: RuntimeDriveHealthState) { + let endpoint_label = endpoint.to_string(); + let pool_label = endpoint.pool_idx.to_string(); + let set_label = endpoint.set_idx.to_string(); + let disk_label = endpoint.disk_idx.to_string(); + + for candidate in [ + RuntimeDriveHealthState::Online, + RuntimeDriveHealthState::Suspect, + RuntimeDriveHealthState::Offline, + RuntimeDriveHealthState::Returning, + ] { + gauge!( + "rustfs_drive_runtime_state", + "endpoint" => endpoint_label.clone(), + "pool" => pool_label.clone(), + "set" => set_label.clone(), + "disk" => disk_label.clone(), + "state" => candidate.as_str().to_string() + ) + .set(if candidate == state { 1.0 } else { 0.0 }); + } +} + +pub fn record_drive_state_transition( + endpoint: &Endpoint, + from: RuntimeDriveHealthState, + to: RuntimeDriveHealthState, + reason: &'static str, +) { + counter!( + "rustfs_drive_state_transition_total", + "endpoint" => endpoint.to_string(), + "pool" => endpoint.pool_idx.to_string(), + "set" => endpoint.set_idx.to_string(), + "disk" => endpoint.disk_idx.to_string(), + "from" => from.as_str().to_string(), + "to" => to.as_str().to_string(), + "reason" => reason.to_string() + ) + .increment(1); +} + +pub fn record_drive_recovery_class(class: DriveRecoveryClass) { + counter!( + "rustfs_drive_recovery_class_total", + "class" => class.as_str().to_string() + ) + .increment(1); +} + +pub fn record_drive_offline_duration(endpoint: &Endpoint, duration: Duration) { + gauge!( + "rustfs_drive_offline_duration_seconds", + "endpoint" => endpoint.to_string(), + "pool" => endpoint.pool_idx.to_string(), + "set" => endpoint.set_idx.to_string(), + "disk" => endpoint.disk_idx.to_string() + ) + .set(duration.as_secs_f64()); +} + +#[derive(Debug, Clone, Default)] +pub struct DriveMembershipSnapshot { + pub online: Vec, + pub suspect: Vec, + pub returning: Vec, + pub offline: Vec, +} + +impl DriveMembershipSnapshot { + pub fn from_optional_disks(disks: &[Option]) -> Self { + let mut snapshot = Self::default(); + + for disk in disks.iter().flatten() { + match disk.runtime_state() { + RuntimeDriveHealthState::Online => snapshot.online.push(disk.clone()), + RuntimeDriveHealthState::Suspect => snapshot.suspect.push(disk.clone()), + RuntimeDriveHealthState::Returning => snapshot.returning.push(disk.clone()), + RuntimeDriveHealthState::Offline => snapshot.offline.push(disk.clone()), + } + } + + snapshot + } + + pub fn scanner_heal_candidates(&self) -> Vec { + let mut disks = Vec::with_capacity(self.online.len() + self.suspect.len() + self.returning.len()); + disks.extend(self.online.iter().cloned()); + disks.extend(self.suspect.iter().cloned()); + disks.extend(self.returning.iter().cloned()); + disks + } + + pub fn strict_online_candidates(&self) -> Vec { + self.online.clone() + } + + pub fn strict_online_local_candidates(&self) -> Vec { + self.online.iter().filter(|disk| disk.is_local()).cloned().collect() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn runtime_drive_health_state_snapshot_eligibility_matches_membership_policy() { + assert!(RuntimeDriveHealthState::Online.is_snapshot_eligible()); + assert!(RuntimeDriveHealthState::Suspect.is_snapshot_eligible()); + assert!(RuntimeDriveHealthState::Returning.is_snapshot_eligible()); + assert!(!RuntimeDriveHealthState::Offline.is_snapshot_eligible()); + } +} diff --git a/crates/ecstore/src/disk/mod.rs b/crates/ecstore/src/disk/mod.rs index b00a036ce..4ae60e08e 100644 --- a/crates/ecstore/src/disk/mod.rs +++ b/crates/ecstore/src/disk/mod.rs @@ -19,6 +19,7 @@ pub mod error_conv; pub mod error_reduce; pub mod format; pub mod fs; +pub mod health_state; pub mod local; pub mod os; @@ -33,6 +34,7 @@ pub const STORAGE_FORMAT_FILE: &str = "xl.meta"; pub const STORAGE_FORMAT_FILE_BACKUP: &str = "xl.meta.bkp"; use crate::disk::disk_store::LocalDiskWrapper; +use crate::disk::health_state::RuntimeDriveHealthState; use crate::disk::local::ScanGuard; use crate::rpc::RemoteDisk; use bytes::Bytes; @@ -410,6 +412,30 @@ impl DiskAPI for Disk { } } +impl Disk { + pub fn runtime_state(&self) -> RuntimeDriveHealthState { + match self { + Disk::Local(local_disk) => local_disk.runtime_state(), + Disk::Remote(remote_disk) => remote_disk.runtime_state(), + } + } + + pub fn offline_duration_secs(&self) -> Option { + match self { + Disk::Local(local_disk) => local_disk.offline_duration_secs(), + Disk::Remote(remote_disk) => remote_disk.offline_duration_secs(), + } + } + + #[cfg(test)] + pub fn force_runtime_state_for_test(&self, state: RuntimeDriveHealthState) { + match self { + Disk::Local(local_disk) => local_disk.force_runtime_state_for_test(state), + Disk::Remote(remote_disk) => remote_disk.force_runtime_state_for_test(state), + } + } +} + impl Disk { /// Enable health monitoring on this disk. /// Called after startup format loading completes so that remote peers diff --git a/crates/ecstore/src/rpc/remote_disk.rs b/crates/ecstore/src/rpc/remote_disk.rs index 5872ca032..8c1990cad 100644 --- a/crates/ecstore/src/rpc/remote_disk.rs +++ b/crates/ecstore/src/rpc/remote_disk.rs @@ -17,9 +17,11 @@ use crate::disk::{ FileWriter, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, disk_store::{ CHECK_EVERY, CHECK_TIMEOUT_DURATION, DEFAULT_RUSTFS_DRIVE_ACTIVE_MONITORING, ENV_RUSTFS_DRIVE_ACTIVE_MONITORING, - SKIP_IF_SUCCESS_BEFORE, get_max_timeout_duration, + SKIP_IF_SUCCESS_BEFORE, get_drive_disk_info_timeout, get_drive_list_dir_timeout, get_drive_metadata_timeout, + get_drive_walkdir_stall_timeout, get_drive_walkdir_timeout, get_max_timeout_duration, }, endpoint::Endpoint, + health_state::{RuntimeDriveHealthState, get_drive_returning_probe_interval, record_drive_runtime_state}, }; use crate::disk::{disk_store::DiskHealthTracker, error::DiskError, local::ScanGuard}; use crate::rpc::client::{TonicInterceptor, gen_tonic_signature_interceptor, node_service_time_out_client}; @@ -31,7 +33,9 @@ use crate::{ use bytes::Bytes; use futures::lock::Mutex; use http::{HeaderMap, HeaderValue, Method, header::CONTENT_TYPE}; +use metrics::counter; use rustfs_filemeta::{FileInfo, ObjectPartInfo, RawFileInfo}; +use rustfs_protos::evict_failed_connection; use rustfs_protos::proto_gen::node_service::RenamePartRequest; use rustfs_protos::proto_gen::node_service::{ CheckPartsRequest, DeletePathsRequest, DeleteRequest, DeleteVersionRequest, DeleteVersionsRequest, DeleteVolumeRequest, @@ -43,7 +47,7 @@ use rustfs_protos::proto_gen::node_service::{ use rustfs_rio::{HttpReader, HttpWriter}; use serde::{Serialize, de::DeserializeOwned}; use std::{ - io::Cursor, + io::{Cursor, ErrorKind}, path::PathBuf, sync::{ Arc, @@ -116,10 +120,38 @@ impl RemoteDisk { health: Arc::new(DiskHealthTracker::new()), cancel_token: CancellationToken::new(), }; + record_drive_runtime_state(ep, RuntimeDriveHealthState::Online); Ok(disk) } + pub fn runtime_state(&self) -> RuntimeDriveHealthState { + self.health.runtime_state() + } + + pub fn offline_duration_secs(&self) -> Option { + self.health.offline_duration().map(|duration| duration.as_secs()) + } + + #[cfg(test)] + pub fn force_runtime_state_for_test(&self, state: RuntimeDriveHealthState) { + self.health.force_runtime_state_for_test(state); + } + + fn spawn_recovery_monitor_if_needed(&self) { + if !self.health_check { + return; + } + + let addr = self.addr.clone(); + let endpoint = self.endpoint.clone(); + let health = Arc::clone(&self.health); + let cancel_token = self.cancel_token.clone(); + tokio::spawn(async move { + Self::monitor_remote_disk_recovery(addr, endpoint, health, cancel_token).await; + }); + } + /// Enable health monitoring after disk creation. /// Used to defer health checks until after startup format loading completes, /// so that remote peers have time to come online. @@ -130,27 +162,34 @@ impl RemoteDisk { let health = Arc::clone(&self.health); let cancel_token = self.cancel_token.clone(); let addr = self.addr.clone(); + let endpoint = self.endpoint.clone(); tokio::spawn(async move { - Self::monitor_remote_disk_health(addr, health, cancel_token).await; + Self::monitor_remote_disk_health(addr, endpoint, health, cancel_token).await; }); } /// Monitor remote disk health periodically - async fn monitor_remote_disk_health(addr: String, health: Arc, cancel_token: CancellationToken) { + async fn monitor_remote_disk_health( + addr: String, + endpoint: Endpoint, + health: Arc, + cancel_token: CancellationToken, + ) { let mut interval = time::interval(CHECK_EVERY); // Perform basic connectivity check - if Self::perform_connectivity_check(&addr).await.is_err() && health.swap_ok_to_faulty() { + if Self::perform_connectivity_check(&addr).await.is_err() && health.mark_failure(&endpoint, "connectivity_probe_failed") { warn!("Remote disk health check failed for {}: marking as faulty", addr); // Start recovery monitoring let health_clone = Arc::clone(&health); let addr_clone = addr.clone(); + let endpoint_clone = endpoint.clone(); let cancel_clone = cancel_token.clone(); tokio::spawn(async move { - Self::monitor_remote_disk_recovery(addr_clone, health_clone, cancel_clone).await; + Self::monitor_remote_disk_recovery(addr_clone, endpoint_clone, health_clone, cancel_clone).await; }); } @@ -183,16 +222,17 @@ impl RemoteDisk { } // Perform basic connectivity check - if Self::perform_connectivity_check(&addr).await.is_err() && health.swap_ok_to_faulty() { + if Self::perform_connectivity_check(&addr).await.is_err() && health.mark_failure(&endpoint, "connectivity_probe_failed") { warn!("Remote disk health check failed for {}: marking as faulty", addr); // Start recovery monitoring let health_clone = Arc::clone(&health); let addr_clone = addr.clone(); + let endpoint_clone = endpoint.clone(); let cancel_clone = cancel_token.clone(); tokio::spawn(async move { - Self::monitor_remote_disk_recovery(addr_clone, health_clone, cancel_clone).await; + Self::monitor_remote_disk_recovery(addr_clone, endpoint_clone, health_clone, cancel_clone).await; }); } } @@ -201,8 +241,13 @@ impl RemoteDisk { } /// Monitor remote disk recovery and mark as healthy when recovered - async fn monitor_remote_disk_recovery(addr: String, health: Arc, cancel_token: CancellationToken) { - let mut interval = time::interval(CHECK_EVERY); + async fn monitor_remote_disk_recovery( + addr: String, + endpoint: Endpoint, + health: Arc, + cancel_token: CancellationToken, + ) { + let mut interval = time::interval(get_drive_returning_probe_interval()); loop { tokio::select! { @@ -211,9 +256,14 @@ impl RemoteDisk { } _ = interval.tick() => { if Self::perform_connectivity_check(&addr).await.is_ok() { - info!("Remote disk recovered: {}", addr); - health.set_ok(); - return; + let became_online = health.mark_recovery_success(&endpoint, "connectivity_probe_success"); + info!("Remote disk recovery probe succeeded: {}", addr); + if became_online { + info!("Remote disk recovered: {}", addr); + return; + } + } else { + health.mark_failure(&endpoint, "connectivity_probe_failed"); } } } @@ -242,6 +292,19 @@ impl RemoteDisk { /// Execute operation with timeout and health tracking async fn execute_with_timeout(&self, operation: F, timeout_duration: Duration) -> Result + where + F: FnOnce() -> Fut, + Fut: std::future::Future>, + { + self.execute_with_timeout_for_op("unknown", operation, timeout_duration).await + } + + async fn execute_with_timeout_for_op( + &self, + op: &'static str, + operation: F, + timeout_duration: Duration, + ) -> Result where F: FnOnce() -> Fut, Fut: std::future::Future>, @@ -270,17 +333,81 @@ impl RemoteDisk { self.health.log_success(); } self.health.decrement_waiting(); + if let Err(err) = &operation_result + && Self::is_timeout_like_error(err) + { + counter!( + "rustfs_drive_op_timeout_total", + "endpoint" => self.endpoint.to_string(), + "op" => op.to_string() + ) + .increment(1); + warn!( + endpoint = %self.endpoint, + addr = %self.addr, + op, + timeout_ms = timeout_duration.as_millis(), + "Remote disk operation returned a timeout-like error" + ); + self.mark_faulty_and_evict("operation_timeout_error").await; + } operation_result } Err(_) => { // Timeout occurred, mark disk as potentially faulty self.health.decrement_waiting(); - warn!("Remote disk operation timeout after {:?}", timeout_duration); + counter!( + "rustfs_drive_op_timeout_total", + "endpoint" => self.endpoint.to_string(), + "op" => op.to_string() + ) + .increment(1); + self.mark_faulty_and_evict("operation_timeout").await; + warn!( + endpoint = %self.endpoint, + addr = %self.addr, + op, + timeout_ms = timeout_duration.as_millis(), + "Remote disk operation timed out" + ); Err(Error::other(format!("Remote disk operation timeout after {timeout_duration:?}"))) } } } + fn is_timeout_like_error(err: &Error) -> bool { + matches!(err, DiskError::Timeout) || matches!(err, DiskError::Io(io_err) if io_err.kind() == ErrorKind::TimedOut) + } + + async fn mark_faulty_and_evict(&self, reason: &'static str) { + if self.health.mark_failure(&self.endpoint, reason) { + self.spawn_recovery_monitor_if_needed(); + counter!( + "rustfs_drive_faulty_mark_total", + "endpoint" => self.endpoint.to_string(), + "reason" => reason.to_string() + ) + .increment(1); + warn!( + "Remote disk marked faulty after timeout: endpoint={}, addr={}, reason={}", + self.endpoint, self.addr, reason + ); + counter!( + "rustfs_drive_connection_evict_total", + "endpoint" => self.endpoint.to_string(), + "reason" => reason.to_string() + ) + .increment(1); + info!( + endpoint = %self.endpoint, + addr = %self.addr, + reason, + "Evicting cached remote disk connection after fault transition" + ); + evict_failed_connection(&self.addr).await; + } + } + async fn get_client(&self) -> Result>> { node_service_time_out_client(&self.addr, TonicInterceptor::Signature(gen_tonic_signature_interceptor())) .await @@ -757,7 +884,8 @@ impl DiskAPI for RemoteDisk { let file_info = serde_json::to_string(&fi)?; let file_info_bin = encode_msgpack(&fi)?; - self.execute_with_timeout( + self.execute_with_timeout_for_op( + "write_metadata", || async { let disk = self.disk_ref().await; let mut client = self @@ -786,24 +914,31 @@ impl DiskAPI for RemoteDisk { } async fn read_metadata(&self, volume: &str, path: &str) -> Result { - let disk = self.disk_ref().await; - let mut client = self - .get_client() - .await - .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; - let request = Request::new(ReadMetadataRequest { - volume: volume.to_string(), - path: path.to_string(), - disk, - }); + self.execute_with_timeout_for_op( + "read_metadata", + || async { + let disk = self.disk_ref().await; + let mut client = self + .get_client() + .await + .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; + let request = Request::new(ReadMetadataRequest { + volume: volume.to_string(), + path: path.to_string(), + disk, + }); - let response = client.read_metadata(request).await?.into_inner(); + let response = client.read_metadata(request).await?.into_inner(); - if !response.success { - return Err(response.error.unwrap_or_default().into()); - } + if !response.success { + return Err(response.error.unwrap_or_default().into()); + } - Ok(response.data) + Ok(response.data) + }, + get_drive_metadata_timeout(), + ) + .await } #[tracing::instrument(skip(self))] @@ -814,7 +949,8 @@ impl DiskAPI for RemoteDisk { let file_info_bin = encode_msgpack(&fi)?; let opts_bin = encode_msgpack(opts)?; - self.execute_with_timeout( + self.execute_with_timeout_for_op( + "update_metadata", || async { let disk = self.disk_ref().await; let mut client = self @@ -967,53 +1103,66 @@ impl DiskAPI for RemoteDisk { async fn list_dir(&self, _origvolume: &str, volume: &str, dir_path: &str, count: i32) -> Result> { debug!("list_dir {}/{}", volume, dir_path); - if self.health.is_faulty() { - return Err(DiskError::FaultyDisk); - } - let disk = self.disk_ref().await; + self.execute_with_timeout( + || async { + let disk = self.disk_ref().await; - let mut client = self - .get_client() - .await - .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; - let request = Request::new(ListDirRequest { - disk, - volume: volume.to_string(), - dir_path: dir_path.to_string(), - count, - }); + let mut client = self + .get_client() + .await + .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; + let request = Request::new(ListDirRequest { + disk, + volume: volume.to_string(), + dir_path: dir_path.to_string(), + count, + }); - let response = client.list_dir(request).await?.into_inner(); + let response = client.list_dir(request).await?.into_inner(); - if !response.success { - return Err(response.error.unwrap_or_default().into()); - } + if !response.success { + return Err(response.error.unwrap_or_default().into()); + } - Ok(response.volumes) + Ok(response.volumes) + }, + get_drive_list_dir_timeout(), + ) + .await } #[tracing::instrument(skip(self, wr))] async fn walk_dir(&self, opts: WalkDirOptions, wr: &mut W) -> Result<()> { info!("walk_dir {}", self.endpoint.to_string()); - if self.health.is_faulty() { - return Err(DiskError::FaultyDisk); - } - let disk = self.disk_ref().await; + self.execute_with_timeout( + || async { + let disk = self.disk_ref().await; - let url = format!("{}/rustfs/rpc/walk_dir?disk={}", self.endpoint.grid_host(), urlencoding::encode(&disk),); + let url = format!("{}/rustfs/rpc/walk_dir?disk={}", self.endpoint.grid_host(), urlencoding::encode(&disk),); - let opts = serde_json::to_vec(&opts)?; + let opts = serde_json::to_vec(&opts)?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - build_auth_headers(&url, &Method::GET, &mut headers); + let mut headers = HeaderMap::new(); + headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); + build_auth_headers(&url, &Method::GET, &mut headers); - let mut reader = HttpReader::new(url, Method::GET, headers, Some(opts)).await?; + let mut reader = HttpReader::new_with_stall_timeout( + url, + Method::GET, + headers, + Some(opts), + Some(get_drive_walkdir_stall_timeout()), + ) + .await?; - copy_stream_with_buffer(&mut reader, wr, DEFAULT_READ_BUFFER_SIZE).await?; + copy_stream_with_buffer(&mut reader, wr, DEFAULT_READ_BUFFER_SIZE).await?; - Ok(()) + Ok(()) + }, + get_drive_walkdir_timeout(), + ) + .await } #[tracing::instrument(level = "debug", skip(self))] @@ -1424,29 +1573,32 @@ impl DiskAPI for RemoteDisk { #[tracing::instrument(skip(self))] async fn disk_info(&self, opts: &DiskInfoOptions) -> Result { - if self.health.is_faulty() { - return Err(DiskError::FaultyDisk); - } + self.execute_with_timeout_for_op( + "disk_info", + || async { + let opts = serde_json::to_string(&opts)?; + let mut client = self + .get_client() + .await + .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; + let request = Request::new(DiskInfoRequest { + disk: self.endpoint.to_string(), + opts, + }); - let opts = serde_json::to_string(&opts)?; - let mut client = self - .get_client() - .await - .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; - let request = Request::new(DiskInfoRequest { - disk: self.endpoint.to_string(), - opts, - }); + let response = client.disk_info(request).await?.into_inner(); - let response = client.disk_info(request).await?.into_inner(); + if !response.success { + return Err(response.error.unwrap_or_default().into()); + } - if !response.success { - return Err(response.error.unwrap_or_default().into()); - } + let disk_info = serde_json::from_str::(&response.disk_info)?; - let disk_info = serde_json::from_str::(&response.disk_info)?; - - Ok(disk_info) + Ok(disk_info) + }, + get_drive_disk_info_timeout(), + ) + .await } #[tracing::instrument(skip(self))] @@ -1459,9 +1611,11 @@ impl DiskAPI for RemoteDisk { #[cfg(test)] mod tests { use super::*; + use rustfs_common::GLOBAL_CONN_MAP; use std::sync::Once; use tokio::io::duplex; use tokio::net::TcpListener; + use tonic::transport::Endpoint as TonicEndpoint; use tracing::Level; use uuid::Uuid; @@ -1793,6 +1947,132 @@ mod tests { assert!(result.is_ok()); } + #[tokio::test] + async fn test_execute_with_timeout_marks_remote_disk_faulty() { + let url = url::Url::parse("http://remote-timeout:9000").unwrap(); + let endpoint = Endpoint { + url, + is_local: false, + pool_idx: 0, + set_idx: 0, + disk_idx: 0, + }; + + let remote_disk = RemoteDisk::new( + &endpoint, + &DiskOption { + cleanup: false, + health_check: false, + }, + ) + .await + .unwrap(); + + let err = remote_disk + .execute_with_timeout( + || async { + tokio::time::sleep(Duration::from_millis(50)).await; + Ok::<(), Error>(()) + }, + Duration::from_millis(10), + ) + .await + .expect_err("timeout should fail"); + + assert!(err.to_string().contains("timeout")); + assert!(!remote_disk.is_online().await, "remote disk should be marked faulty after timeout"); + } + + #[tokio::test] + async fn test_execute_with_timeout_evicts_cached_connection() { + let addr = "http://127.0.0.1:59991".to_string(); + let url = url::Url::parse(&format!("{addr}/data")).unwrap(); + let endpoint = Endpoint { + url, + is_local: false, + pool_idx: 0, + set_idx: 0, + disk_idx: 0, + }; + + let remote_disk = RemoteDisk::new( + &endpoint, + &DiskOption { + cleanup: false, + health_check: false, + }, + ) + .await + .unwrap(); + + let channel = TonicEndpoint::from_shared(addr.clone()).unwrap().connect_lazy(); + GLOBAL_CONN_MAP.write().await.insert(addr.clone(), channel); + assert!(GLOBAL_CONN_MAP.read().await.contains_key(&addr)); + + let _ = remote_disk + .execute_with_timeout( + || async { + tokio::time::sleep(Duration::from_millis(50)).await; + Ok::<(), Error>(()) + }, + Duration::from_millis(10), + ) + .await + .expect_err("timeout should fail"); + + assert!( + !GLOBAL_CONN_MAP.read().await.contains_key(&addr), + "timeout should evict cached connection" + ); + } + + #[tokio::test] + async fn test_execute_with_timeout_marks_faulty_on_timeout_like_error() { + let addr = "http://127.0.0.1:59992".to_string(); + let url = url::Url::parse(&format!("{addr}/data")).unwrap(); + let endpoint = Endpoint { + url, + is_local: false, + pool_idx: 0, + set_idx: 0, + disk_idx: 0, + }; + + let remote_disk = RemoteDisk::new( + &endpoint, + &DiskOption { + cleanup: false, + health_check: false, + }, + ) + .await + .unwrap(); + + let channel = TonicEndpoint::from_shared(addr.clone()).unwrap().connect_lazy(); + GLOBAL_CONN_MAP.write().await.insert(addr.clone(), channel); + + let err = remote_disk + .execute_with_timeout( + || async { Err::<(), Error>(DiskError::Io(std::io::Error::new(std::io::ErrorKind::TimedOut, "stall timeout"))) }, + Duration::from_secs(1), + ) + .await + .expect_err("timeout-like operation error should fail"); + + assert_eq!( + match &err { + DiskError::Io(io_err) => io_err.kind(), + other => panic!("expected io timeout error, got {other:?}"), + }, + std::io::ErrorKind::TimedOut + ); + assert!(!remote_disk.is_online().await, "timeout-like errors should mark remote disk faulty"); + assert!( + !GLOBAL_CONN_MAP.read().await.contains_key(&addr), + "timeout-like errors should evict cached connection" + ); + } + #[test] fn test_remote_disk_sync_properties() { let url = url::Url::parse("https://secure-remote:9000/data").unwrap(); diff --git a/crates/ecstore/src/rpc/remote_locker.rs b/crates/ecstore/src/rpc/remote_locker.rs index 4e786a4d2..9d020a655 100644 --- a/crates/ecstore/src/rpc/remote_locker.rs +++ b/crates/ecstore/src/rpc/remote_locker.rs @@ -18,8 +18,10 @@ use rustfs_lock::{ LockClient, LockError, LockInfo, LockRequest, LockResponse, LockStats, LockStatus, LockType, Result, types::{LockId, LockMetadata, LockPriority}, }; -use rustfs_protos::proto_gen::node_service::node_service_client::NodeServiceClient; use rustfs_protos::proto_gen::node_service::{BatchGenerallyLockRequest, GenerallyLockRequest, PingRequest}; +use rustfs_protos::{evict_failed_connection, proto_gen::node_service::node_service_client::NodeServiceClient}; +use std::time::Duration; +use tokio::time::timeout; use tonic::Request; use tonic::service::interceptor::InterceptedService; use tonic::transport::Channel; @@ -62,6 +64,77 @@ impl RemoteClient { .map_err(|err| LockError::internal(format!("can not get client, err: {err}"))) } + async fn evict_connection(&self, op: &'static str, reason: &str) { + warn!( + addr = %self.addr, + op, + reason, + "Evicting cached remote lock connection after RPC failure" + ); + evict_failed_connection(&self.addr).await; + } + + fn rpc_timeout(timeout_duration: Duration) -> Duration { + if timeout_duration.is_zero() { + Duration::from_millis(1) + } else { + timeout_duration + } + } + + async fn execute_rpc( + &self, + op: &'static str, + timeout_duration: Duration, + future: F, + ) -> std::result::Result + where + F: std::future::Future>, + { + let timeout_duration = Self::rpc_timeout(timeout_duration); + match timeout(timeout_duration, future).await { + Ok(Ok(response)) => Ok(response), + Ok(Err(err)) => { + let reason = err.to_string(); + self.evict_connection(op, &reason).await; + Err(LockError::internal(format!("{op} RPC failed: {reason}"))) + } + Err(_) => { + let reason = format!("RPC timed out after {:?}", timeout_duration); + self.evict_connection(op, &reason).await; + Err(LockError::timeout(format!("remote lock RPC {op} on {}", self.addr), timeout_duration)) + } + } + } + + fn timeout_failure_response(request: &LockRequest) -> LockResponse { + LockResponse::failure("Lock acquisition timeout", request.acquire_timeout) + } + + fn rpc_failure_response(_request: &LockRequest, err: &LockError) -> LockResponse { + LockResponse::failure(format!("Remote lock RPC failed: {err}"), Duration::ZERO) + } + + fn timeout_failure_batch(requests: &[LockRequest]) -> Vec { + requests.iter().map(Self::timeout_failure_response).collect() + } + + fn rpc_failure_batch(requests: &[LockRequest], err: &LockError) -> Vec { + requests + .iter() + .map(|request| Self::rpc_failure_response(request, err)) + .collect() + } + + fn batch_rpc_timeout(requests: &[LockRequest]) -> Duration { + requests + .iter() + .map(|request| request.acquire_timeout) + .max() + .map(Self::rpc_timeout) + .unwrap_or_else(|| Duration::from_millis(1)) + } + fn build_lock_info(request: &LockRequest, lock_info_json: Option) -> LockInfo { if let Some(lock_info_json) = lock_info_json { match serde_json::from_str::(&lock_info_json) { @@ -111,11 +184,11 @@ impl LockClient for RemoteClient { .map_err(|e| LockError::internal(format!("Failed to serialize request: {e}")))?, }); - let resp = client - .lock(req) - .await - .map_err(|e| LockError::internal(e.to_string()))? - .into_inner(); + let resp = match self.execute_rpc("lock", request.acquire_timeout, client.lock(req)).await { + Ok(resp) => resp.into_inner(), + Err(LockError::Timeout { .. }) => return Ok(Self::timeout_failure_response(request)), + Err(err) => return Ok(Self::rpc_failure_response(request, &err)), + }; // Check for explicit error first if let Some(error_info) = resp.error_info { @@ -138,6 +211,10 @@ impl LockClient for RemoteClient { } async fn acquire_locks_batch(&self, requests: &[LockRequest]) -> Result> { + if requests.is_empty() { + return Ok(Vec::new()); + } + let mut client = self.get_client().await?; let req = Request::new(BatchGenerallyLockRequest { args: requests @@ -148,11 +225,14 @@ impl LockClient for RemoteClient { .collect::>>()?, }); - let resp = client - .lock_batch(req) + let resp = match self + .execute_rpc("lock_batch", Self::batch_rpc_timeout(requests), client.lock_batch(req)) .await - .map_err(|e| LockError::internal(e.to_string()))? - .into_inner(); + { + Ok(resp) => resp.into_inner(), + Err(LockError::Timeout { .. }) => return Ok(Self::timeout_failure_batch(requests)), + Err(err) => return Ok(Self::rpc_failure_batch(requests, &err)), + }; Ok(requests .iter() @@ -380,3 +460,95 @@ impl LockClient for RemoteClient { false } } + +#[cfg(test)] +mod tests { + use super::*; + use rustfs_common::GLOBAL_CONN_MAP; + use rustfs_lock::{ObjectKey, types::LockPriority}; + use tokio::net::TcpListener; + use tokio::task::JoinHandle; + use tonic::transport::Endpoint as TonicEndpoint; + + async fn spawn_hanging_listener() -> (String, JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = format!("http://{}", listener.local_addr().unwrap()); + let task = tokio::spawn(async move { + if let Ok((stream, _)) = listener.accept().await { + let _stream = stream; + tokio::time::sleep(Duration::from_secs(2)).await; + } + }); + (addr, task) + } + + async fn cache_lazy_channel(addr: &str) { + let channel = TonicEndpoint::from_shared(addr.to_string()).unwrap().connect_lazy(); + GLOBAL_CONN_MAP.write().await.insert(addr.to_string(), channel); + } + + fn test_lock_request(timeout_duration: Duration) -> LockRequest { + LockRequest::new(ObjectKey::new("bucket", "object"), LockType::Exclusive, "owner-a") + .with_acquire_timeout(timeout_duration) + .with_priority(LockPriority::Normal) + } + + #[tokio::test] + async fn test_remote_client_acquire_lock_respects_request_timeout_and_evicts_connection() { + let (addr, accept_task) = spawn_hanging_listener().await; + cache_lazy_channel(&addr).await; + assert!(GLOBAL_CONN_MAP.read().await.contains_key(&addr)); + + let client = RemoteClient::new(addr.clone()); + let request = test_lock_request(Duration::from_millis(50)); + let started_at = tokio::time::Instant::now(); + + let response = client.acquire_lock(&request).await.unwrap(); + + assert!( + started_at.elapsed() < Duration::from_secs(1), + "remote lock RPC should honor request timeout" + ); + assert!(!response.success, "timed out lock acquisition should fail"); + assert_eq!(response.error.as_deref(), Some("Lock acquisition timeout")); + assert!( + !GLOBAL_CONN_MAP.read().await.contains_key(&addr), + "timeout should evict cached connection" + ); + + accept_task.abort(); + } + + #[tokio::test] + async fn test_remote_client_acquire_locks_batch_respects_request_timeout_and_evicts_connection() { + let (addr, accept_task) = spawn_hanging_listener().await; + cache_lazy_channel(&addr).await; + assert!(GLOBAL_CONN_MAP.read().await.contains_key(&addr)); + + let client = RemoteClient::new(addr.clone()); + let requests = vec![test_lock_request(Duration::from_millis(50))]; + let started_at = tokio::time::Instant::now(); + + let responses = client.acquire_locks_batch(&requests).await.unwrap(); + + assert!( + started_at.elapsed() < Duration::from_secs(1), + "remote batch lock RPC should honor request timeout" + ); + assert_eq!(responses.len(), 1); + assert!(!responses[0].success, "timed out batch lock acquisition should fail"); + assert_eq!(responses[0].error.as_deref(), Some("Lock acquisition timeout")); + assert!( + !GLOBAL_CONN_MAP.read().await.contains_key(&addr), + "batch timeout should evict cached connection" + ); + + accept_task.abort(); + } + + #[test] + fn test_remote_client_zero_timeout_is_clamped() { + assert_eq!(RemoteClient::rpc_timeout(Duration::ZERO), Duration::from_millis(1)); + assert_eq!(RemoteClient::rpc_timeout(Duration::from_millis(25)), Duration::from_millis(25)); + } +} diff --git a/crates/ecstore/src/set_disk.rs b/crates/ecstore/src/set_disk.rs index 096bd00f5..5e88ee7c8 100644 --- a/crates/ecstore/src/set_disk.rs +++ b/crates/ecstore/src/set_disk.rs @@ -1032,57 +1032,163 @@ impl SetDisks { 1 }; - let client_results = join_all(self.lockers.iter().cloned().enumerate().map(|(client_idx, client)| { - let requests = requests.clone(); - async move { (client_idx, client.acquire_locks_batch(&requests).await) } - })) - .await; - let mut lock_ids_by_object: Vec> = vec![Vec::new(); requests.len()]; let mut errors_by_object: Vec> = vec![None; requests.len()]; + #[derive(Clone, Copy, Debug, PartialEq, Eq)] + enum ObjectLockResolution { + Pending, + Succeeded, + Failed, + } - for (client_idx, result) in client_results { - match result { - Ok(responses) => { + let mut resolution_by_object = vec![ObjectLockResolution::Pending; requests.len()]; + let mut pending_clients = self.lockers.len(); + let mut unresolved_objects = requests.len(); + let mut cleanup_lock_ids_by_client = vec![Vec::new(); self.lockers.len()]; + + let mut pending = tokio::task::JoinSet::new(); + for (client_idx, client) in self.lockers.iter().cloned().enumerate() { + let requests = requests.clone(); + pending.spawn(async move { (client_idx, client.acquire_locks_batch(&requests).await) }); + } + + while unresolved_objects > 0 { + let Some(join_result) = pending.join_next().await else { + break; + }; + pending_clients = pending_clients.saturating_sub(1); + + match join_result { + Ok((client_idx, Ok(responses))) => { for (req_idx, request) in requests.iter().enumerate() { - match responses.get(req_idx) { - Some(response) if response.success => { - if let Some(lock_info) = response.lock_info.as_ref() { - lock_ids_by_object[req_idx].push((client_idx, lock_info.id.clone())); - } else if errors_by_object[req_idx].is_none() { - errors_by_object[req_idx] = Some(format!( - "missing distributed lock id for {}/{}", - request.resource.bucket, request.resource.object - )); + let response = responses.get(req_idx); + match resolution_by_object[req_idx] { + ObjectLockResolution::Pending => match response { + Some(response) if response.success => { + let lock_id = response + .lock_info + .as_ref() + .map(|lock_info| lock_info.id.clone()) + .unwrap_or_else(|| request.lock_id.clone()); + lock_ids_by_object[req_idx].push((client_idx, lock_id)); } - } - Some(response) => { - if errors_by_object[req_idx].is_none() { - errors_by_object[req_idx] = Some( - response - .error - .clone() - .unwrap_or_else(|| "distributed lock acquisition failed".to_string()), - ); + Some(response) => { + if errors_by_object[req_idx].is_none() { + errors_by_object[req_idx] = Some( + response + .error + .clone() + .unwrap_or_else(|| "distributed lock acquisition failed".to_string()), + ); + } } - } - None => { - if errors_by_object[req_idx].is_none() { - errors_by_object[req_idx] = - Some(format!("client {client_idx} returned incomplete batch lock response")); + None => { + if errors_by_object[req_idx].is_none() { + errors_by_object[req_idx] = + Some(format!("client {client_idx} returned incomplete batch lock response")); + } + } + }, + ObjectLockResolution::Succeeded | ObjectLockResolution::Failed => { + if let Some(response) = response + && response.success + { + let lock_id = response + .lock_info + .as_ref() + .map(|lock_info| lock_info.id.clone()) + .unwrap_or_else(|| request.lock_id.clone()); + cleanup_lock_ids_by_client[client_idx].push(lock_id); } } } } } - Err(err) => { - for error in errors_by_object.iter_mut().take(requests.len()) { - if error.is_none() { + Ok((client_idx, Err(err))) => { + for (req_idx, error) in errors_by_object.iter_mut().enumerate().take(requests.len()) { + if resolution_by_object[req_idx] == ObjectLockResolution::Pending && error.is_none() { *error = Some(format!("client {client_idx} batch lock request failed: {err}")); } } } + Err(err) => { + for (req_idx, error) in errors_by_object.iter_mut().enumerate().take(requests.len()) { + if resolution_by_object[req_idx] == ObjectLockResolution::Pending && error.is_none() { + *error = Some(format!("batch lock task join failed: {err}")); + } + } + } } + + for req_idx in 0..requests.len() { + if resolution_by_object[req_idx] != ObjectLockResolution::Pending { + continue; + } + + let success_count = lock_ids_by_object[req_idx].len(); + if success_count >= write_quorum { + resolution_by_object[req_idx] = ObjectLockResolution::Succeeded; + unresolved_objects -= 1; + } else if success_count + pending_clients < write_quorum { + resolution_by_object[req_idx] = ObjectLockResolution::Failed; + unresolved_objects -= 1; + } + } + } + + if !pending.is_empty() { + let cleanup_requests = requests.clone(); + let lockers = self.lockers.clone(); + let handle = tokio::spawn(async move { + let mut late_lock_ids_by_client = vec![Vec::new(); lockers.len()]; + let mut pending = pending; + while let Some(join_result) = pending.join_next().await { + match join_result { + Ok((client_idx, Ok(responses))) => { + for (req_idx, request) in cleanup_requests.iter().enumerate() { + if let Some(response) = responses.get(req_idx) + && response.success + { + let lock_id = response + .lock_info + .as_ref() + .map(|lock_info| lock_info.id.clone()) + .unwrap_or_else(|| request.lock_id.clone()); + if let Some(client_locks) = late_lock_ids_by_client.get_mut(client_idx) { + client_locks.push(lock_id); + } + } + } + } + Ok((_client_idx, Err(err))) => { + tracing::warn!("late distributed delete lock batch request failed: {}", err); + } + Err(err) => { + tracing::warn!("late distributed delete lock batch task join failed: {}", err); + } + } + } + + join_all(lockers.iter().cloned().enumerate().filter_map(|(client_idx, client)| { + let lock_ids = late_lock_ids_by_client.get(client_idx).cloned().unwrap_or_default(); + if lock_ids.is_empty() { + None + } else { + Some(async move { + if let Err(err) = client.release_locks_batch(&lock_ids).await { + tracing::warn!( + client_idx, + lock_count = lock_ids.len(), + "failed to cleanup late distributed delete locks in batch: {}", + err + ); + } + }) + } + })) + .await; + }); + drop(handle); } let mut failed_map = HashMap::new(); @@ -1092,24 +1198,31 @@ impl SetDisks { for (req_idx, req) in batch.requests.iter().enumerate() { let success_count = lock_ids_by_object[req_idx].len(); - if success_count >= write_quorum { - for (client_idx, lock_id) in lock_ids_by_object[req_idx].drain(..) { - held_lock_ids_by_client[client_idx].push(lock_id); + match resolution_by_object[req_idx] { + ObjectLockResolution::Succeeded => { + for (client_idx, lock_id) in lock_ids_by_object[req_idx].drain(..) { + held_lock_ids_by_client[client_idx].push(lock_id); + } + locked_objects.insert(req.key.object.as_ref().to_string()); } - locked_objects.insert(req.key.object.as_ref().to_string()); - } else { - for (client_idx, lock_id) in lock_ids_by_object[req_idx].drain(..) { - rollback_lock_ids_by_client[client_idx].push(lock_id); + ObjectLockResolution::Pending | ObjectLockResolution::Failed => { + for (client_idx, lock_id) in lock_ids_by_object[req_idx].drain(..) { + rollback_lock_ids_by_client[client_idx].push(lock_id); + } + failed_map.insert( + (req.key.bucket.as_ref().to_string(), req.key.object.as_ref().to_string()), + errors_by_object[req_idx].clone().unwrap_or_else(|| { + format!("failed to acquire distributed delete lock quorum: {success_count}/{write_quorum}") + }), + ); } - failed_map.insert( - (req.key.bucket.as_ref().to_string(), req.key.object.as_ref().to_string()), - errors_by_object[req_idx].clone().unwrap_or_else(|| { - format!("failed to acquire distributed delete lock quorum: {success_count}/{write_quorum}") - }), - ); } } + for (client_idx, cleanup_ids) in cleanup_lock_ids_by_client.into_iter().enumerate() { + rollback_lock_ids_by_client[client_idx].extend(cleanup_ids); + } + self.release_dist_delete_object_locks_batch(rollback_lock_ids_by_client).await; (failed_map, locked_objects, held_lock_ids_by_client) @@ -3975,47 +4088,57 @@ async fn get_disks_info(disks: &[Option], eps: &[Endpoint]) -> Vec ret.push(rustfs_madmin::Disk { - endpoint: eps[i].to_string(), - local: eps[i].is_local, - pool_index: eps[i].pool_idx, - set_index: eps[i].set_idx, - disk_index: eps[i].disk_idx, - state: "ok".to_owned(), + let runtime_state = disk.runtime_state(); + let offline_duration_seconds = disk.offline_duration_secs(); + if runtime_state.should_probe_for_admin() { + match disk.disk_info(&DiskInfoOptions::default()).await { + Ok(res) => ret.push(rustfs_madmin::Disk { + endpoint: eps[i].to_string(), + local: eps[i].is_local, + pool_index: eps[i].pool_idx, + set_index: eps[i].set_idx, + disk_index: eps[i].disk_idx, + state: "ok".to_owned(), - root_disk: res.root_disk, - drive_path: res.mount_path.clone(), - healing: res.healing, - scanning: res.scanning, + root_disk: res.root_disk, + drive_path: res.mount_path.clone(), + healing: res.healing, + scanning: res.scanning, + runtime_state: Some(runtime_state.as_str().to_string()), + offline_duration_seconds, - uuid: res.id.map_or_else(|| "".to_string(), |id| id.to_string()), - major: res.major as u32, - minor: res.minor as u32, - model: None, - total_space: res.total, - used_space: res.used, - available_space: res.free, - utilization: { - if res.total > 0 { - res.used as f64 / res.total as f64 * 100_f64 - } else { - 0_f64 - } - }, - used_inodes: res.used_inodes, - free_inodes: res.free_inodes, - ..Default::default() - }), - Err(err) => ret.push(rustfs_madmin::Disk { - state: err.to_string(), - endpoint: eps[i].to_string(), - local: eps[i].is_local, - pool_index: eps[i].pool_idx, - set_index: eps[i].set_idx, - disk_index: eps[i].disk_idx, - ..Default::default() - }), + uuid: res.id.map_or_else(|| "".to_string(), |id| id.to_string()), + major: res.major as u32, + minor: res.minor as u32, + model: None, + total_space: res.total, + used_space: res.used, + available_space: res.free, + utilization: { + if res.total > 0 { + res.used as f64 / res.total as f64 * 100_f64 + } else { + 0_f64 + } + }, + used_inodes: res.used_inodes, + free_inodes: res.free_inodes, + ..Default::default() + }), + Err(err) => ret.push(rustfs_madmin::Disk { + state: err.to_string(), + endpoint: eps[i].to_string(), + local: eps[i].is_local, + pool_index: eps[i].pool_idx, + set_index: eps[i].set_idx, + disk_index: eps[i].disk_idx, + runtime_state: Some(runtime_state.as_str().to_string()), + offline_duration_seconds, + ..Default::default() + }), + } + } else { + ret.push(build_runtime_snapshot_disk(&eps[i], runtime_state, offline_duration_seconds)); } } else { ret.push(rustfs_madmin::Disk { @@ -4024,6 +4147,8 @@ async fn get_disks_info(disks: &[Option], eps: &[Endpoint]) -> Vec], eps: &[Endpoint]) -> Vec, +) -> rustfs_madmin::Disk { + rustfs_madmin::Disk { + endpoint: endpoint.to_string(), + local: endpoint.is_local, + pool_index: endpoint.pool_idx, + set_index: endpoint.set_idx, + disk_index: endpoint.disk_idx, + state: runtime_state.as_str().to_string(), + runtime_state: Some(runtime_state.as_str().to_string()), + offline_duration_seconds, + ..Default::default() + } +} async fn get_storage_info(disks: &[Option], eps: &[Endpoint]) -> rustfs_madmin::StorageInfo { // let mut disks = get_disks_info(disks, eps).await; // disks.sort_by(|a, b| a.total_space.cmp(&b.total_space)); @@ -4188,14 +4331,17 @@ mod tests { use crate::disk::CHECK_PART_VOLUME_NOT_FOUND; use crate::disk::endpoint::Endpoint; use crate::disk::error::DiskError; + use crate::disk::health_state::RuntimeDriveHealthState; use crate::endpoints::SetupType; use crate::global::{is_dist_erasure, is_erasure, is_erasure_sd, update_erasure_type}; use crate::store_api::{CompletePart, ObjectInfo}; + use crate::store_init::save_format_file; use rustfs_filemeta::ErasureInfo; use rustfs_lock::client::local::LocalClient; use rustfs_lock::{LockError, LockInfo, LockResponse, LockStats}; use serial_test::serial; use std::collections::HashMap; + use tempfile::TempDir; use time::OffsetDateTime; #[derive(Debug, Default)] @@ -4240,6 +4386,60 @@ mod tests { } } + #[derive(Debug)] + struct DelayedBatchClient { + inner: Arc, + delay: Duration, + } + + #[async_trait::async_trait] + impl LockClient for DelayedBatchClient { + async fn acquire_lock(&self, request: &rustfs_lock::LockRequest) -> rustfs_lock::Result { + self.inner.acquire_lock(request).await + } + + async fn acquire_locks_batch(&self, requests: &[rustfs_lock::LockRequest]) -> rustfs_lock::Result> { + tokio::time::sleep(self.delay).await; + self.inner.acquire_locks_batch(requests).await + } + + async fn release(&self, lock_id: &rustfs_lock::LockId) -> rustfs_lock::Result { + self.inner.release(lock_id).await + } + + async fn release_locks_batch(&self, lock_ids: &[rustfs_lock::LockId]) -> rustfs_lock::Result> { + self.inner.release_locks_batch(lock_ids).await + } + + async fn refresh(&self, lock_id: &rustfs_lock::LockId) -> rustfs_lock::Result { + self.inner.refresh(lock_id).await + } + + async fn force_release(&self, lock_id: &rustfs_lock::LockId) -> rustfs_lock::Result { + self.inner.force_release(lock_id).await + } + + async fn check_status(&self, lock_id: &rustfs_lock::LockId) -> rustfs_lock::Result> { + self.inner.check_status(lock_id).await + } + + async fn get_stats(&self) -> rustfs_lock::Result { + self.inner.get_stats().await + } + + async fn close(&self) -> rustfs_lock::Result<()> { + self.inner.close().await + } + + async fn is_online(&self) -> bool { + self.inner.is_online().await + } + + async fn is_local(&self) -> bool { + self.inner.is_local().await + } + } + async fn make_test_set_disks(lockers: Vec>) -> Arc { let endpoints = vec![ Endpoint::try_from("http://127.0.0.1:9000/data").expect("first endpoint should parse"), @@ -4296,6 +4496,33 @@ mod tests { } } + async fn make_formatted_local_disk_for_info_test(disk_idx: usize, format: &FormatV3) -> (TempDir, Endpoint, DiskStore) { + let dir = tempfile::tempdir().expect("tempdir should be created"); + let mut endpoint = + Endpoint::try_from(dir.path().to_str().expect("tempdir path should be utf8")).expect("endpoint should parse"); + endpoint.set_pool_index(0); + endpoint.set_set_index(0); + endpoint.set_disk_index(disk_idx); + + let disk = new_disk( + &endpoint, + &DiskOption { + cleanup: false, + health_check: false, + }, + ) + .await + .expect("disk should be created"); + + let mut disk_format = format.clone(); + disk_format.erasure.this = format.erasure.sets[0][disk_idx]; + save_format_file(&Some(disk.clone()), &Some(disk_format)) + .await + .expect("format should be saved"); + + (dir, endpoint, disk) + } + #[test] fn disk_health_entry_returns_cached_value_within_ttl() { let entry = DiskHealthEntry { @@ -4542,6 +4769,113 @@ mod tests { drop(guard); } + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn test_acquire_dist_delete_object_locks_batch_returns_after_quorum_without_waiting_for_slow_lockers() { + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + + let manager_fast_1 = Arc::new(rustfs_lock::GlobalLockManager::new()); + let manager_fast_2 = Arc::new(rustfs_lock::GlobalLockManager::new()); + let manager_fast_3 = Arc::new(rustfs_lock::GlobalLockManager::new()); + let manager_slow = Arc::new(rustfs_lock::GlobalLockManager::new()); + + let client_fast_1: Arc = Arc::new(LocalClient::with_manager(manager_fast_1)); + let client_fast_2: Arc = Arc::new(LocalClient::with_manager(manager_fast_2)); + let client_fast_3: Arc = Arc::new(LocalClient::with_manager(manager_fast_3)); + let client_slow: Arc = Arc::new(DelayedBatchClient { + inner: Arc::new(LocalClient::with_manager(manager_slow.clone())), + delay: Duration::from_millis(250), + }); + + let set_disks = make_test_set_disks(vec![client_fast_1, client_fast_2, client_fast_3, client_slow]).await; + + let batch = rustfs_lock::BatchLockRequest::new(set_disks.locker_owner.as_str()) + .with_all_or_nothing(false) + .add_write_lock(ObjectKey::new("bucket", "object-a")) + .add_write_lock(ObjectKey::new("bucket", "object-b")); + + let started = Instant::now(); + let (failed_map, locked_objects, held_lock_ids_by_client) = + set_disks.acquire_dist_delete_object_locks_batch(&batch).await; + + assert!( + started.elapsed() < Duration::from_millis(150), + "batch distributed delete locks should return once quorum is satisfied" + ); + assert!(failed_map.is_empty()); + assert_eq!(locked_objects.len(), 2); + + set_disks + .release_dist_delete_object_locks_batch(held_lock_ids_by_client) + .await; + + tokio::time::sleep(Duration::from_millis(350)).await; + + let slow_lock = NamespaceLock::with_local_manager("slow-node".to_string(), manager_slow); + let guard_a = slow_lock + .get_write_lock(ObjectKey::new("bucket", "object-a"), "owner-b", Duration::from_millis(100)) + .await + .expect("late successful batch lock should be cleaned up for object-a"); + let guard_b = slow_lock + .get_write_lock(ObjectKey::new("bucket", "object-b"), "owner-b", Duration::from_millis(100)) + .await + .expect("late successful batch lock should be cleaned up for object-b"); + + drop(guard_a); + drop(guard_b); + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn test_acquire_dist_delete_object_locks_batch_fails_early_and_cleans_up_late_successes() { + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + + let manager_fast = Arc::new(rustfs_lock::GlobalLockManager::new()); + let manager_slow = Arc::new(rustfs_lock::GlobalLockManager::new()); + + let client_fast: Arc = Arc::new(LocalClient::with_manager(manager_fast)); + let client_fail_1: Arc = Arc::new(FailingClient); + let client_fail_2: Arc = Arc::new(FailingClient); + let client_slow: Arc = Arc::new(DelayedBatchClient { + inner: Arc::new(LocalClient::with_manager(manager_slow.clone())), + delay: Duration::from_millis(250), + }); + + let set_disks = make_test_set_disks(vec![client_fast, client_fail_1, client_fail_2, client_slow]).await; + let batch = rustfs_lock::BatchLockRequest::new(set_disks.locker_owner.as_str()) + .with_all_or_nothing(false) + .add_write_lock(ObjectKey::new("bucket", "object-a")) + .add_write_lock(ObjectKey::new("bucket", "object-b")); + + let started = Instant::now(); + let (failed_map, locked_objects, held_lock_ids_by_client) = + set_disks.acquire_dist_delete_object_locks_batch(&batch).await; + + assert!( + started.elapsed() < Duration::from_millis(150), + "batch distributed delete locks should fail as soon as quorum becomes impossible" + ); + assert!(locked_objects.is_empty()); + assert!(failed_map.contains_key(&("bucket".to_string(), "object-a".to_string()))); + assert!(failed_map.contains_key(&("bucket".to_string(), "object-b".to_string()))); + assert_eq!(held_lock_ids_by_client.iter().map(Vec::len).sum::(), 0); + + tokio::time::sleep(Duration::from_millis(350)).await; + + let slow_lock = NamespaceLock::with_local_manager("slow-node".to_string(), manager_slow); + let guard_a = slow_lock + .get_write_lock(ObjectKey::new("bucket", "object-a"), "owner-b", Duration::from_millis(100)) + .await + .expect("late successful batch failure cleanup should release object-a"); + let guard_b = slow_lock + .get_write_lock(ObjectKey::new("bucket", "object-b"), "owner-b", Duration::from_millis(100)) + .await + .expect("late successful batch failure cleanup should release object-b"); + + drop(guard_a); + drop(guard_b); + } + #[test] fn test_common_parity() { // Test common parity calculation @@ -4761,6 +5095,45 @@ mod tests { assert!(should_heal); } + #[tokio::test] + async fn test_get_disks_info_uses_runtime_snapshot_for_suspect_and_offline_disks() { + let format = FormatV3::new(1, 3); + let mut temp_dirs = Vec::new(); + let mut endpoints = Vec::new(); + let mut disks = Vec::new(); + + for disk_idx in 0..3 { + let (dir, endpoint, disk) = make_formatted_local_disk_for_info_test(disk_idx, &format).await; + temp_dirs.push(dir); + endpoints.push(endpoint); + disks.push(Some(disk)); + } + + disks[1] + .as_ref() + .expect("disk 1 should exist") + .force_runtime_state_for_test(RuntimeDriveHealthState::Suspect); + disks[2] + .as_ref() + .expect("disk 2 should exist") + .force_runtime_state_for_test(RuntimeDriveHealthState::Offline); + + let info = get_disks_info(&disks, &endpoints).await; + assert_eq!(info.len(), 3); + + assert_eq!(info[0].state, "ok"); + assert_eq!(info[0].runtime_state.as_deref(), Some("online")); + assert!(!info[0].drive_path.is_empty(), "online disk should keep immediate disk_info probe"); + + assert_eq!(info[1].state, "suspect"); + assert_eq!(info[1].runtime_state.as_deref(), Some("suspect")); + assert!(info[1].drive_path.is_empty(), "suspect disk should use runtime snapshot fallback"); + + assert_eq!(info[2].state, "offline"); + assert_eq!(info[2].runtime_state.as_deref(), Some("offline")); + assert!(info[2].drive_path.is_empty(), "offline disk should use runtime snapshot fallback"); + } + #[test] fn test_dangling_meta_errs_count() { // Test counting dangling metadata errors diff --git a/crates/ecstore/src/set_disk/lock.rs b/crates/ecstore/src/set_disk/lock.rs index c323ade21..b4711d2e5 100644 --- a/crates/ecstore/src/set_disk/lock.rs +++ b/crates/ecstore/src/set_disk/lock.rs @@ -13,6 +13,7 @@ // limitations under the License. use super::*; +use crate::disk::health_state::DriveMembershipSnapshot; impl SetDisks { pub(super) fn format_lock_error(&self, bucket: &str, object: &str, mode: &str, err: &LockResult) -> String { @@ -72,41 +73,28 @@ impl SetDisks { } pub(super) async fn get_online_disks(&self) -> Vec> { - let mut disks = self.get_disks_internal().await; - - // TODO: diskinfo filter online - - let mut new_disk = Vec::with_capacity(disks.len()); - - for disk in disks.iter() { - if let Some(d) = disk - && d.is_online().await - { - new_disk.push(disk.clone()); - } - } + let snapshot = self.drive_membership_snapshot().await; + let mut disks = snapshot.strict_online_candidates().into_iter().map(Some).collect::>(); let mut rng = rand::rng(); - disks.shuffle(&mut rng); - new_disk - // let disks = self.get_disks_internal().await; - // let (filtered, _) = self.filter_online_disks(disks).await; - // filtered.into_iter().filter(|disk| disk.is_some()).collect() + disks } pub(super) async fn get_online_local_disks(&self) -> Vec> { - let mut disks = self.get_online_disks().await; + let snapshot = self.drive_membership_snapshot().await; + let mut disks = snapshot + .strict_online_local_candidates() + .into_iter() + .map(Some) + .collect::>(); let mut rng = rand::rng(); disks.shuffle(&mut rng); disks - .into_iter() - .filter(|v| v.as_ref().is_some_and(|d| d.is_local())) - .collect() } pub async fn get_online_disks_with_healing(&self, incl_healing: bool) -> (Vec, bool) { @@ -114,28 +102,41 @@ impl SetDisks { (disks, healing > 0) } - pub async fn get_online_disks_with_healing_and_info(&self, incl_healing: bool) -> (Vec, Vec, usize) { - let mut disks = self.get_disks_internal().await; + pub async fn drive_membership_snapshot(&self) -> DriveMembershipSnapshot { + let disks = self.get_disks_internal().await; + DriveMembershipSnapshot::from_optional_disks(&disks) + } - let mut infos = Vec::with_capacity(disks.len()); + pub async fn get_online_disks_with_healing_and_info(&self, incl_healing: bool) -> (Vec, Vec, usize) { + let snapshot = self.drive_membership_snapshot().await; + let mut disks = snapshot.scanner_heal_candidates().into_iter().map(Some).collect::>(); + + let mut infos: Vec> = vec![None; disks.len()]; let mut futures = Vec::with_capacity(disks.len()); - let mut numbers: Vec = (0..disks.len()).collect(); { let mut rng = rand::rng(); disks.shuffle(&mut rng); - - numbers.shuffle(&mut rng); } - for &i in numbers.iter() { - let disk = disks[i].clone(); + for (i, disk) in disks.iter().cloned().enumerate() { futures.push(async move { - if let Some(disk) = disk { - disk.disk_info(&DiskInfoOptions::default()).await + let info = if let Some(disk) = disk { + match disk.disk_info(&DiskInfoOptions::default()).await { + Ok(info) => info, + Err(err) => DiskInfo { + error: err.to_string(), + ..Default::default() + }, + } } else { - Err(DiskError::DiskNotFound) - } + DiskInfo { + error: DiskError::DiskNotFound.to_string(), + ..Default::default() + } + }; + + Ok((i, info)) }); } @@ -143,13 +144,13 @@ impl SetDisks { let processor = get_global_processors().metadata_processor(); let results = processor.execute_batch(futures).await; - for result in results { + for (submitted_idx, result) in results.into_iter().enumerate() { match result { - Ok(res) => { - infos.push(res); + Ok((disk_idx, info)) => { + infos[disk_idx] = Some(info); } Err(err) => { - infos.push(DiskInfo { + infos[submitted_idx] = Some(DiskInfo { error: err.to_string(), ..Default::default() }); @@ -167,8 +168,11 @@ impl SetDisks { let mut new_disks = Vec::new(); let mut new_infos = Vec::new(); - for &i in numbers.iter() { - let (info, disk) = (infos[i].clone(), disks[i].clone()); + for (disk, info) in disks.into_iter().zip(infos.into_iter()) { + let Some(info) = info else { + continue; + }; + if !info.error.is_empty() || disk.is_none() { continue; } @@ -367,3 +371,160 @@ impl SetDisks { Ok((new_disks, new_infos, healing)) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::store_init::save_format_file; + use tempfile::TempDir; + use tokio::sync::RwLock; + + async fn make_formatted_local_disk(disk_idx: usize, format: &FormatV3) -> (TempDir, Endpoint, DiskStore) { + let dir = tempfile::tempdir().expect("tempdir should be created"); + let mut endpoint = + Endpoint::try_from(dir.path().to_str().expect("tempdir path should be utf8")).expect("endpoint should parse"); + endpoint.set_pool_index(0); + endpoint.set_set_index(0); + endpoint.set_disk_index(disk_idx); + + let disk = new_disk( + &endpoint, + &DiskOption { + cleanup: false, + health_check: false, + }, + ) + .await + .expect("local disk should be created"); + + let mut disk_format = format.clone(); + disk_format.erasure.this = format.erasure.sets[0][disk_idx]; + save_format_file(&Some(disk.clone()), &Some(disk_format)) + .await + .expect("format should be saved"); + + (dir, endpoint, disk) + } + + #[tokio::test] + async fn get_online_disks_with_healing_and_info_keeps_disk_and_info_aligned() { + let disk_count = 8; + let format = FormatV3::new(1, disk_count); + + let mut temp_dirs = Vec::with_capacity(disk_count); + let mut endpoints = Vec::with_capacity(disk_count); + let mut disks = Vec::with_capacity(disk_count); + + for disk_idx in 0..disk_count { + let (temp_dir, endpoint, disk) = make_formatted_local_disk(disk_idx, &format).await; + temp_dirs.push(temp_dir); + endpoints.push(endpoint); + disks.push(Some(disk)); + } + + let set_disks = SetDisks::new( + "test-owner".to_string(), + Arc::new(RwLock::new(disks)), + disk_count, + disk_count / 2, + 0, + 0, + endpoints, + format, + Vec::new(), + ) + .await; + + for _ in 0..32 { + let (online_disks, infos, healing) = set_disks.get_online_disks_with_healing_and_info(false).await; + assert_eq!(healing, 0); + assert_eq!(online_disks.len(), disk_count); + assert_eq!(infos.len(), disk_count); + + for (disk, info) in online_disks.iter().zip(infos.iter()) { + assert!( + info.error.is_empty(), + "unexpected disk_info error for {}: {}", + disk.endpoint(), + info.error + ); + assert_eq!(info.endpoint, disk.endpoint().to_string()); + assert_eq!( + info.id, + disk.get_disk_id().await.expect("disk id lookup should succeed"), + "disk info should stay aligned with disk {}", + disk.endpoint() + ); + } + } + + drop(temp_dirs); + } + + #[tokio::test] + async fn drive_membership_snapshot_filters_offline_disks_from_candidates() { + let disk_count = 4; + let format = FormatV3::new(1, disk_count); + + let mut temp_dirs = Vec::with_capacity(disk_count); + let mut endpoints = Vec::with_capacity(disk_count); + let mut disks = Vec::with_capacity(disk_count); + + for disk_idx in 0..disk_count { + let (temp_dir, endpoint, disk) = make_formatted_local_disk(disk_idx, &format).await; + temp_dirs.push(temp_dir); + endpoints.push(endpoint); + disks.push(Some(disk)); + } + + let set_disks = SetDisks::new( + "test-owner".to_string(), + Arc::new(RwLock::new(disks)), + disk_count, + disk_count / 2, + 0, + 0, + endpoints, + format, + Vec::new(), + ) + .await; + + let all_disks = set_disks.get_disks_internal().await; + all_disks[1] + .as_ref() + .expect("disk 1 should exist") + .force_runtime_state_for_test(crate::disk::health_state::RuntimeDriveHealthState::Suspect); + all_disks[2] + .as_ref() + .expect("disk 2 should exist") + .force_runtime_state_for_test(crate::disk::health_state::RuntimeDriveHealthState::Returning); + all_disks[3] + .as_ref() + .expect("disk 3 should exist") + .force_runtime_state_for_test(crate::disk::health_state::RuntimeDriveHealthState::Offline); + + let snapshot = set_disks.drive_membership_snapshot().await; + assert_eq!(snapshot.online.len(), 1); + assert_eq!(snapshot.suspect.len(), 1); + assert_eq!(snapshot.returning.len(), 1); + assert_eq!(snapshot.offline.len(), 1); + assert_eq!(snapshot.scanner_heal_candidates().len(), 3); + + let strict_online = set_disks.get_online_disks().await; + assert_eq!(strict_online.len(), 1, "strict online selection should exclude suspect/returning/offline"); + + let (online_disks, infos, healing) = set_disks.get_online_disks_with_healing_and_info(false).await; + assert_eq!(healing, 0); + assert_eq!(online_disks.len(), 3); + assert_eq!(infos.len(), 3); + assert!( + online_disks + .iter() + .all(|disk| { disk.runtime_state() != crate::disk::health_state::RuntimeDriveHealthState::Offline }), + "offline disks should be filtered by membership snapshot" + ); + + drop(temp_dirs); + } +} diff --git a/crates/heal/Cargo.toml b/crates/heal/Cargo.toml index 4be3271dd..d22bb9eb4 100644 --- a/crates/heal/Cargo.toml +++ b/crates/heal/Cargo.toml @@ -42,6 +42,7 @@ uuid = { workspace = true, features = ["v4", "serde"] } anyhow = { workspace = true } async-trait = { workspace = true } futures = { workspace = true } +metrics = { workspace = true } [dev-dependencies] serde_json = { workspace = true } @@ -50,6 +51,7 @@ tracing-subscriber = { workspace = true } tempfile = { workspace = true } walkdir = { workspace = true } http = { workspace = true } +temp-env = { workspace = true } [lib] doctest = false diff --git a/crates/heal/src/error.rs b/crates/heal/src/error.rs index 6db296512..21533ae75 100644 --- a/crates/heal/src/error.rs +++ b/crates/heal/src/error.rs @@ -68,6 +68,9 @@ pub enum Error { #[error("Invalid heal type: {heal_type}")] InvalidHealType { heal_type: String }, + #[error("Transient heal skip: {message}")] + TransientSkip { message: String }, + #[error("Heal task cancelled")] TaskCancelled, @@ -92,6 +95,11 @@ impl Error { { Error::Other(error.into().to_string()) } + + /// Create a transient skip error for retryable background heal checks. + pub fn transient_skip(message: impl Into) -> Self { + Error::TransientSkip { message: message.into() } + } } impl From for std::io::Error { diff --git a/crates/heal/src/heal/channel.rs b/crates/heal/src/heal/channel.rs index 4a2df7f50..3089be598 100644 --- a/crates/heal/src/heal/channel.rs +++ b/crates/heal/src/heal/channel.rs @@ -19,11 +19,11 @@ use crate::heal::{ }; use crate::{Error, Result}; use rustfs_common::heal_channel::{ - HealChannelCommand, HealChannelPriority, HealChannelReceiver, HealChannelRequest, HealChannelResponse, HealScanMode, - publish_heal_response, + HealAdmissionResult, HealChannelCommand, HealChannelPriority, HealChannelReceiver, HealChannelRequest, HealChannelResponse, + HealScanMode, publish_heal_response, }; use std::sync::Arc; -use tokio::sync::mpsc; +use tokio::sync::{mpsc, oneshot}; use tracing::{debug, error, info}; /// Heal channel processor @@ -82,14 +82,18 @@ impl HealChannelProcessor { /// Process heal command async fn process_command(&self, command: HealChannelCommand) -> Result<()> { match command { - HealChannelCommand::Start(request) => self.process_start_request(request).await, + HealChannelCommand::Start { request, response_tx } => self.process_start_request(request, response_tx).await, HealChannelCommand::Query { heal_path, client_token } => self.process_query_request(heal_path, client_token).await, HealChannelCommand::Cancel { heal_path } => self.process_cancel_request(heal_path).await, } } /// Process start request - async fn process_start_request(&self, request: HealChannelRequest) -> Result<()> { + async fn process_start_request( + &self, + request: HealChannelRequest, + response_tx: oneshot::Sender>, + ) -> Result<()> { info!( "Processing heal start request: {} for bucket: {}/{}", request.id, @@ -98,31 +102,60 @@ impl HealChannelProcessor { ); // Convert channel request to heal request - let heal_request = self.convert_to_heal_request(request.clone())?; + let heal_request = match self.convert_to_heal_request(request.clone()) { + Ok(heal_request) => heal_request, + Err(err) => { + let error_text = err.to_string(); + let _ = response_tx.send(Err(error_text.clone())); + self.publish_response(HealChannelResponse { + request_id: request.id, + success: false, + data: None, + error: Some(error_text), + }); + return Ok(()); + } + }; // Submit to heal manager match self.heal_manager.submit_heal_request(heal_request).await { - Ok(task_id) => { - info!("Successfully submitted heal request: {} as task: {}", request.id, task_id); + Ok(admission) => { + info!( + request_id = %request.id, + admission = admission.result_label(), + "Heal request admission decision completed" + ); + + let _ = response_tx.send(Ok(admission)); + + let (success, error) = match admission { + HealAdmissionResult::Accepted | HealAdmissionResult::Merged => (true, None), + HealAdmissionResult::Full => (false, Some("Heal request queue is full".to_string())), + HealAdmissionResult::Dropped(reason) => (false, Some(format!("Heal request dropped: {}", reason.as_str()))), + }; let response = HealChannelResponse { request_id: request.id, - success: true, - data: Some(format!("Task ID: {task_id}").into_bytes()), - error: None, + success, + data: Some( + format!("admission={},reason={}", admission.result_label(), admission.reason_label()).into_bytes(), + ), + error, }; self.publish_response(response); } Err(e) => { - error!("Failed to submit heal request: {} - {}", request.id, e); + let error_text = e.to_string(); + error!("Failed to submit heal request: {} - {}", request.id, error_text); + let _ = response_tx.send(Err(error_text.clone())); // Send error response let response = HealChannelResponse { request_id: request.id, success: false, data: None, - error: Some(e.to_string()), + error: Some(error_text), }; self.publish_response(response); @@ -247,8 +280,9 @@ impl HealChannelProcessor { #[cfg(test)] mod tests { use super::*; + use crate::heal::manager::HealConfig; use crate::heal::storage::HealStorageAPI; - use rustfs_common::heal_channel::{HealChannelPriority, HealChannelRequest, HealScanMode}; + use rustfs_common::heal_channel::{HealAdmissionResult, HealChannelPriority, HealChannelRequest, HealScanMode}; use std::sync::Arc; // Mock storage for testing @@ -569,4 +603,92 @@ mod tests { let heal_request = processor.convert_to_heal_request(channel_request).unwrap(); assert!(matches!(heal_request.heal_type, HealType::Bucket { .. })); } + + #[tokio::test] + async fn test_process_start_request_returns_admission_result() { + let storage: Arc = Arc::new(MockStorage); + let manager = Arc::new(HealManager::new( + storage, + Some(HealConfig { + queue_size: 1, + ..HealConfig::default() + }), + )); + let processor = HealChannelProcessor::new(manager); + + let request = HealChannelRequest { + id: "admission-id".to_string(), + bucket: "bucket".to_string(), + object_prefix: Some("object".to_string()), + object_version_id: None, + disk: None, + priority: HealChannelPriority::Low, + scan_mode: Some(HealScanMode::Normal), + remove_corrupted: None, + recreate_missing: None, + update_parity: None, + recursive: None, + dry_run: None, + timeout_seconds: None, + pool_index: None, + set_index: None, + force_start: false, + }; + + let (tx, rx) = oneshot::channel(); + processor + .process_start_request(request.clone(), tx) + .await + .expect("first admission should succeed"); + assert_eq!( + rx.await + .expect("oneshot should resolve") + .expect("admission should be returned"), + HealAdmissionResult::Accepted + ); + + let (tx, rx) = oneshot::channel(); + processor + .process_start_request(request, tx) + .await + .expect("duplicate admission should succeed"); + assert_eq!( + rx.await + .expect("oneshot should resolve") + .expect("admission should be returned"), + HealAdmissionResult::Merged + ); + } + + #[tokio::test] + async fn test_process_start_request_returns_error_on_invalid_request() { + let heal_manager = create_test_heal_manager(); + let processor = HealChannelProcessor::new(heal_manager); + + let request = HealChannelRequest { + id: "invalid-id".to_string(), + bucket: "bucket".to_string(), + object_prefix: None, + object_version_id: None, + disk: Some("invalid".to_string()), + priority: HealChannelPriority::Normal, + scan_mode: None, + remove_corrupted: None, + recreate_missing: None, + update_parity: None, + recursive: None, + dry_run: None, + timeout_seconds: None, + pool_index: None, + set_index: None, + force_start: false, + }; + + let (tx, rx) = oneshot::channel(); + processor + .process_start_request(request, tx) + .await + .expect("processor should surface invalid request through response channel"); + assert!(rx.await.expect("oneshot should resolve").is_err()); + } } diff --git a/crates/heal/src/heal/erasure_healer.rs b/crates/heal/src/heal/erasure_healer.rs index 5fe740520..7ece5cac1 100644 --- a/crates/heal/src/heal/erasure_healer.rs +++ b/crates/heal/src/heal/erasure_healer.rs @@ -18,11 +18,15 @@ use crate::heal::{ storage::HealStorageAPI, }; use crate::{Error, Result}; -use futures::future::join_all; +use futures::{StreamExt, future::join_all, stream::FuturesUnordered}; +use metrics::gauge; use rustfs_common::heal_channel::{HealOpts, HealScanMode}; use rustfs_ecstore::disk::DiskStore; -use std::sync::Arc; -use tokio::sync::RwLock; +use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, +}; +use tokio::sync::{RwLock, Semaphore}; use tracing::{error, info, warn}; /// Erasure Set Healer @@ -34,6 +38,29 @@ pub struct ErasureSetHealer { } impl ErasureSetHealer { + fn page_parallel_enabled() -> bool { + rustfs_utils::get_env_bool( + rustfs_config::ENV_HEAL_PAGE_PARALLEL_ENABLE, + rustfs_config::DEFAULT_HEAL_PAGE_PARALLEL_ENABLE, + ) + } + + fn heal_page_object_concurrency() -> usize { + rustfs_utils::get_env_usize( + rustfs_config::ENV_HEAL_PAGE_OBJECT_CONCURRENCY, + rustfs_config::DEFAULT_HEAL_PAGE_OBJECT_CONCURRENCY, + ) + .max(1) + } + + fn effective_heal_page_object_concurrency() -> usize { + if Self::page_parallel_enabled() { + Self::heal_page_object_concurrency() + } else { + 1 + } + } + pub fn new( storage: Arc, progress: Arc>, @@ -61,7 +88,7 @@ impl ErasureSetHealer { // 3. execute heal with resume let result = self - .execute_heal_with_resume(buckets, &resume_manager, &checkpoint_manager) + .execute_heal_with_resume(buckets, set_disk_id, &resume_manager, &checkpoint_manager) .await; // 4. cleanup resume state @@ -144,6 +171,7 @@ impl ErasureSetHealer { async fn execute_heal_with_resume( &self, buckets: &[String], + set_disk_id: &str, resume_manager: &ResumeManager, checkpoint_manager: &CheckpointManager, ) -> Result<()> { @@ -182,6 +210,7 @@ impl ErasureSetHealer { let bucket_result = self .heal_bucket_with_resume( bucket, + set_disk_id, bucket_idx, &mut current_object_index, &mut processed_objects, @@ -232,16 +261,17 @@ impl ErasureSetHealer { /// heal single bucket with resume #[allow(clippy::too_many_arguments)] - #[tracing::instrument(skip(self, current_object_index, processed_objects, successful_objects, failed_objects, _skipped_objects, resume_manager, checkpoint_manager), fields(bucket = %bucket, bucket_index = bucket_index))] + #[tracing::instrument(skip(self, current_object_index, processed_objects, successful_objects, failed_objects, skipped_objects, resume_manager, checkpoint_manager), fields(bucket = %bucket, bucket_index = bucket_index))] async fn heal_bucket_with_resume( &self, bucket: &str, + set_disk_id: &str, bucket_index: usize, current_object_index: &mut usize, processed_objects: &mut u64, successful_objects: &mut u64, failed_objects: &mut u64, - _skipped_objects: &mut u64, + skipped_objects: &mut u64, resume_manager: &ResumeManager, checkpoint_manager: &CheckpointManager, ) -> Result<()> { @@ -259,6 +289,8 @@ impl ErasureSetHealer { // 2. process objects with pagination to avoid loading all objects into memory let mut continuation_token: Option = None; let mut global_obj_idx = 0usize; + let page_concurrency_limit = Self::effective_heal_page_object_concurrency(); + let in_flight = Arc::new(AtomicUsize::new(0)); loop { // Get one page of objects @@ -266,69 +298,139 @@ impl ErasureSetHealer { .storage .list_objects_for_heal_page(bucket, "", continuation_token.as_deref()) .await?; + let checkpoint = checkpoint_manager.get_checkpoint().await; + let page_resume_index = *current_object_index; + let semaphore = Arc::new(Semaphore::new(page_concurrency_limit)); + let mut page_tasks = FuturesUnordered::new(); - // Process objects in this page for object in objects { - // Skip objects before the checkpoint - if global_obj_idx < *current_object_index { - global_obj_idx += 1; + let object_idx = global_obj_idx; + global_obj_idx += 1; + + if object_idx < *current_object_index { continue; } - // check if already processed - if checkpoint_manager.get_checkpoint().await.processed_objects.contains(&object) { - global_obj_idx += 1; + if checkpoint.processed_objects.contains(&object) || checkpoint.skipped_objects.contains(&object) { continue; } - // update current object resume_manager .set_current_item(Some(bucket.to_string()), Some(object.clone())) .await?; - // Check if object still exists before attempting heal - let object_exists = match self.storage.object_exists(bucket, &object).await { - Ok(exists) => exists, - Err(e) => { - warn!("Failed to check existence of {}/{}: {}, marking as failed", bucket, object, e); - *failed_objects += 1; - checkpoint_manager.add_failed_object(object.clone()).await?; - global_obj_idx += 1; - *current_object_index = global_obj_idx; - continue; - } - }; + let storage = self.storage.clone(); + let bucket_name = bucket.to_string(); + let object_name = object.clone(); + let cancel_token = self.cancel_token.clone(); + let in_flight = in_flight.clone(); + let set_label = set_disk_id.to_string(); + let permit = semaphore + .clone() + .acquire_owned() + .await + .map_err(|e| Error::other(format!("Failed to acquire page concurrency permit: {e}")))?; - if !object_exists { - info!( - target: "rustfs:heal:heal_bucket_with_resume" ,"Object {}/{} no longer exists, skipping heal (likely deleted intentionally)", - bucket, object - ); - checkpoint_manager.add_processed_object(object.clone()).await?; - *successful_objects += 1; // Treat as successful - object is gone as intended - global_obj_idx += 1; - *current_object_index = global_obj_idx; - continue; - } + let current_in_flight = in_flight.fetch_add(1, Ordering::SeqCst) + 1; + gauge!( + "rustfs_heal_page_concurrency_current", + "set" => set_label.clone() + ) + .set(current_in_flight as f64); - // heal object - let heal_opts = HealOpts { - scan_mode: HealScanMode::Normal, - remove: true, - recreate: true, // Keep recreate enabled for legitimate heal scenarios - ..Default::default() - }; + page_tasks.push(async move { + let _permit = permit; + let result = if cancel_token.is_cancelled() { + Err(Error::TaskCancelled) + } else { + let object_exists = match storage.object_exists(&bucket_name, &object_name).await { + Ok(exists) => exists, + Err(err @ Error::TransientSkip { .. }) => { + let current = in_flight.fetch_sub(1, Ordering::SeqCst) - 1; + gauge!( + "rustfs_heal_page_concurrency_current", + "set" => set_label.clone() + ) + .set(current as f64); + return (object_name, Err(err)); + } + Err(err) => { + let object_name_for_error = object_name.clone(); + let current = in_flight.fetch_sub(1, Ordering::SeqCst) - 1; + gauge!( + "rustfs_heal_page_concurrency_current", + "set" => set_label.clone() + ) + .set(current as f64); + return ( + object_name, + Err(Error::other(format!( + "Failed to check existence of {}/{}: {}", + bucket_name, object_name_for_error, err + ))), + ); + } + }; - match self.storage.heal_object(bucket, &object, None, &heal_opts).await { - Ok((_result, None)) => { + if !object_exists { + Ok(false) + } else { + let heal_opts = HealOpts { + scan_mode: HealScanMode::Normal, + remove: true, + recreate: true, + ..Default::default() + }; + match storage.heal_object(&bucket_name, &object_name, None, &heal_opts).await { + Ok((_result, None)) => Ok(true), + Ok((_, Some(err))) => Err(Error::other(err)), + Err(err) => Err(err), + } + } + }; + + let current = in_flight.fetch_sub(1, Ordering::SeqCst) - 1; + gauge!( + "rustfs_heal_page_concurrency_current", + "set" => set_label.clone() + ) + .set(current as f64); + + (object_name, result) + }); + } + + let mut completed_in_page = 0usize; + while let Some((object, result)) = page_tasks.next().await { + match result { + Ok(true) => { *successful_objects += 1; checkpoint_manager.add_processed_object(object.clone()).await?; info!("Successfully healed object {}/{}", bucket, object); } - Ok((_, Some(err))) => { - *failed_objects += 1; - checkpoint_manager.add_failed_object(object.clone()).await?; - warn!("Failed to heal object {}/{}: {}", bucket, object, err); + Ok(false) => { + checkpoint_manager.add_processed_object(object.clone()).await?; + *successful_objects += 1; + info!( + target: "rustfs:heal:heal_bucket_with_resume" ,"Object {}/{} no longer exists, skipping heal (likely deleted intentionally)", + bucket, object + ); + } + Err(Error::TaskCancelled) => { + gauge!( + "rustfs_heal_page_concurrency_current", + "set" => set_disk_id.to_string() + ) + .set(0.0); + return Err(Error::TaskCancelled); + } + Err(Error::TransientSkip { message }) => { + *skipped_objects += 1; + checkpoint_manager.add_skipped_object(object.clone()).await?; + warn!( + "Skipping heal for object {}/{} due to transient existence check error: {}", + bucket, object, message + ); } Err(err) => { *failed_objects += 1; @@ -338,23 +440,23 @@ impl ErasureSetHealer { } *processed_objects += 1; - global_obj_idx += 1; - *current_object_index = global_obj_idx; + completed_in_page += 1; - // check cancel status - if self.cancel_token.is_cancelled() { - info!("Heal task cancelled during object processing"); - return Err(Error::TaskCancelled); - } - - // save checkpoint periodically - if global_obj_idx.is_multiple_of(100) { - checkpoint_manager - .update_position(bucket_index, *current_object_index) - .await?; + if completed_in_page.is_multiple_of(100) { + checkpoint_manager.update_position(bucket_index, page_resume_index).await?; } } + *current_object_index = global_obj_idx; + checkpoint_manager + .update_position(bucket_index, *current_object_index) + .await?; + gauge!( + "rustfs_heal_page_concurrency_current", + "set" => set_disk_id.to_string() + ) + .set(0.0); + // Check if there are more pages if !is_truncated { break; @@ -572,3 +674,34 @@ impl ErasureSetHealer { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::ErasureSetHealer; + + #[test] + fn heal_page_object_concurrency_uses_default_when_env_is_unset() { + temp_env::with_var_unset(rustfs_config::ENV_HEAL_PAGE_OBJECT_CONCURRENCY, || { + assert_eq!( + ErasureSetHealer::heal_page_object_concurrency(), + rustfs_config::DEFAULT_HEAL_PAGE_OBJECT_CONCURRENCY + ); + }); + } + + #[test] + fn heal_page_object_concurrency_respects_env_override() { + temp_env::with_var(rustfs_config::ENV_HEAL_PAGE_OBJECT_CONCURRENCY, Some("11"), || { + assert_eq!(ErasureSetHealer::heal_page_object_concurrency(), 11); + }); + } + + #[test] + fn effective_heal_page_object_concurrency_disables_parallelism_when_flag_is_off() { + temp_env::with_var(rustfs_config::ENV_HEAL_PAGE_PARALLEL_ENABLE, Some("false"), || { + temp_env::with_var(rustfs_config::ENV_HEAL_PAGE_OBJECT_CONCURRENCY, Some("11"), || { + assert_eq!(ErasureSetHealer::effective_heal_page_object_concurrency(), 1); + }); + }); + } +} diff --git a/crates/heal/src/heal/manager.rs b/crates/heal/src/heal/manager.rs index 849935fc9..a36689e20 100644 --- a/crates/heal/src/heal/manager.rs +++ b/crates/heal/src/heal/manager.rs @@ -18,6 +18,8 @@ use crate::heal::{ task::{HealOptions, HealPriority, HealRequest, HealTask, HealTaskStatus, HealType}, }; use crate::{Error, Result}; +use metrics::{counter, gauge}; +use rustfs_common::heal_channel::{HealAdmissionDropReason, HealAdmissionResult}; use rustfs_ecstore::disk::DiskAPI; use rustfs_ecstore::disk::error::DiskError; use rustfs_ecstore::global::GLOBAL_LOCAL_DISK_MAP; @@ -27,7 +29,7 @@ use std::{ time::{Duration, SystemTime}, }; use tokio::{ - sync::{Mutex, RwLock}, + sync::{Mutex, Notify, RwLock}, time::interval, }; use tokio_util::sync::CancellationToken; @@ -80,6 +82,12 @@ impl PartialOrd for PriorityQueueItem { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum QueuePushOutcome { + Accepted, + Merged, +} + impl PriorityHealQueue { fn new() -> Self { Self { @@ -93,16 +101,24 @@ impl PriorityHealQueue { self.heap.len() } + fn pop_next(&mut self) -> Option { + self.heap.pop().map(|item| { + let key = Self::make_dedup_key(&item.request); + self.dedup_keys.remove(&key); + item.request + }) + } + fn is_empty(&self) -> bool { self.heap.is_empty() } - fn push(&mut self, request: HealRequest) -> bool { + fn push(&mut self, request: HealRequest) -> QueuePushOutcome { let key = Self::make_dedup_key(&request); // Check for duplicates if self.dedup_keys.contains(&key) { - return false; // Duplicate request, don't add + return QueuePushOutcome::Merged; } self.dedup_keys.insert(key); @@ -112,7 +128,7 @@ impl PriorityHealQueue { sequence: self.sequence, request, }); - true + QueuePushOutcome::Accepted } /// Get statistics about queue contents by priority @@ -124,6 +140,7 @@ impl PriorityHealQueue { stats } + #[cfg(test)] fn pop(&mut self) -> Option { self.heap.pop().map(|item| { let key = Self::make_dedup_key(&item.request); @@ -132,6 +149,48 @@ impl PriorityHealQueue { }) } + #[cfg(test)] + fn pop_runnable(&mut self, can_run: F) -> Option + where + F: Fn(&HealRequest) -> bool, + { + self.pop_runnable_with_skips(can_run, |_| None).0 + } + + fn pop_runnable_with_skips(&mut self, can_run: F, skip_label: G) -> (Option, Vec) + where + F: Fn(&HealRequest) -> bool, + G: Fn(&HealRequest) -> Option, + { + let mut deferred = Vec::new(); + let mut selected = None; + let mut skipped = Vec::new(); + + while let Some(item) = self.heap.pop() { + if can_run(&item.request) { + selected = Some(item); + break; + } + if let Some(label) = skip_label(&item.request) { + skipped.push(label); + } + deferred.push(item); + } + + for item in deferred { + self.heap.push(item); + } + + ( + selected.map(|item| { + let key = Self::make_dedup_key(&item.request); + self.dedup_keys.remove(&key); + item.request + }), + skipped, + ) + } + /// Create a deduplication key from a heal request fn make_dedup_key(request: &HealRequest) -> String { match &request.heal_type { @@ -187,10 +246,22 @@ pub struct HealConfig { pub heal_interval: Duration, /// Maximum concurrent heal tasks pub max_concurrent_heals: usize, + /// Maximum concurrent heal tasks allowed for a single erasure set + pub max_concurrent_per_set: usize, /// Task timeout pub task_timeout: Duration, /// Queue size pub queue_size: usize, + /// Whether duplicate low-priority requests should merge into an existing queued request. + pub low_priority_merge_enable: bool, + /// Whether low-priority requests may be dropped when the queue is full. + pub low_priority_drop_when_full: bool, + /// Whether notify-driven scheduler wakeups are enabled. + pub event_driven_scheduler_enable: bool, + /// Whether per-set bulkhead scheduling is enabled. + pub set_bulkhead_enable: bool, + /// Whether erasure-set page parallelism is enabled. + pub page_parallel_enable: bool, } impl Default for HealConfig { @@ -211,12 +282,42 @@ impl Default for HealConfig { rustfs_config::ENV_HEAL_MAX_CONCURRENT_HEALS, rustfs_config::DEFAULT_HEAL_MAX_CONCURRENT_HEALS, ); + let max_concurrent_per_set = rustfs_utils::get_env_usize( + rustfs_config::ENV_HEAL_MAX_CONCURRENT_PER_SET, + rustfs_config::DEFAULT_HEAL_MAX_CONCURRENT_PER_SET, + ); + let low_priority_merge_enable = rustfs_utils::get_env_bool( + rustfs_config::ENV_HEAL_LOW_PRIORITY_MERGE_ENABLE, + rustfs_config::DEFAULT_HEAL_LOW_PRIORITY_MERGE_ENABLE, + ); + let low_priority_drop_when_full = rustfs_utils::get_env_bool( + rustfs_config::ENV_HEAL_LOW_PRIORITY_DROP_WHEN_FULL, + rustfs_config::DEFAULT_HEAL_LOW_PRIORITY_DROP_WHEN_FULL, + ); + let event_driven_scheduler_enable = rustfs_utils::get_env_bool( + rustfs_config::ENV_HEAL_EVENT_DRIVEN_SCHEDULER_ENABLE, + rustfs_config::DEFAULT_HEAL_EVENT_DRIVEN_SCHEDULER_ENABLE, + ); + let set_bulkhead_enable = rustfs_utils::get_env_bool( + rustfs_config::ENV_HEAL_SET_BULKHEAD_ENABLE, + rustfs_config::DEFAULT_HEAL_SET_BULKHEAD_ENABLE, + ); + let page_parallel_enable = rustfs_utils::get_env_bool( + rustfs_config::ENV_HEAL_PAGE_PARALLEL_ENABLE, + rustfs_config::DEFAULT_HEAL_PAGE_PARALLEL_ENABLE, + ); Self { enable_auto_heal, heal_interval, // 10 seconds max_concurrent_heals, // max 4, - task_timeout, // 5 minutes + max_concurrent_per_set: std::cmp::min(max_concurrent_heals.max(1), max_concurrent_per_set.max(1)), + task_timeout, // 5 minutes queue_size, + low_priority_merge_enable, + low_priority_drop_when_full, + event_driven_scheduler_enable, + set_bulkhead_enable, + page_parallel_enable, } } } @@ -254,9 +355,19 @@ pub struct HealManager { cancel_token: CancellationToken, /// Statistics statistics: Arc>, + /// Scheduler wake-up notifier for event-driven dispatch + notify: Arc, } impl HealManager { + fn classify_full_admission(request: &HealRequest, config: &HealConfig) -> HealAdmissionResult { + if request.priority == HealPriority::Low && config.low_priority_drop_when_full { + HealAdmissionResult::Dropped(HealAdmissionDropReason::QueueFull) + } else { + HealAdmissionResult::Full + } + } + /// Create new HealManager pub fn new(storage: Arc, config: Option) -> Self { let config = config.unwrap_or_default(); @@ -268,6 +379,7 @@ impl HealManager { storage, cancel_token: CancellationToken::new(), statistics: Arc::new(RwLock::new(HealStatistics::new())), + notify: Arc::new(Notify::new()), } } @@ -318,17 +430,63 @@ impl HealManager { } /// Submit heal request - pub async fn submit_heal_request(&self, request: HealRequest) -> Result { + pub async fn submit_heal_request(&self, request: HealRequest) -> Result { let config = self.config.read().await; let mut queue = self.heal_queue.lock().await; let queue_len = queue.len(); let queue_capacity = config.queue_size; + if queue.contains_key(&request) { + let admission = if request.priority == HealPriority::Low && !config.low_priority_merge_enable { + HealAdmissionResult::Dropped(HealAdmissionDropReason::PolicyDropped) + } else { + HealAdmissionResult::Merged + }; + + match admission { + HealAdmissionResult::Merged => { + info!("Heal request already queued (duplicate merged): {}", request.id); + } + HealAdmissionResult::Dropped(reason) => { + warn!( + request_id = %request.id, + priority = ?request.priority, + reason = reason.as_str(), + "Dropping duplicate heal request due to admission policy" + ); + } + HealAdmissionResult::Accepted | HealAdmissionResult::Full => {} + } + + return Ok(admission); + } + if queue_len >= queue_capacity { - return Err(Error::ConfigurationError { - message: format!("Heal queue is full ({queue_len}/{queue_capacity})"), - }); + let admission = Self::classify_full_admission(&request, &config); + match admission { + HealAdmissionResult::Dropped(reason) => { + warn!( + request_id = %request.id, + priority = ?request.priority, + queue_len, + queue_capacity, + reason = reason.as_str(), + "Dropping heal request because the queue is full" + ); + } + HealAdmissionResult::Full => { + warn!( + request_id = %request.id, + priority = ?request.priority, + queue_len, + queue_capacity, + "Rejecting heal request because the queue is full" + ); + } + HealAdmissionResult::Accepted | HealAdmissionResult::Merged => {} + } + return Ok(admission); } // Warn when queue is getting full (>80% capacity) @@ -345,8 +503,8 @@ impl HealManager { let request_id = request.id.clone(); let priority = request.priority; - // Try to push the request; if it's a duplicate, still return the request_id - let is_new = queue.push(request); + let push_outcome = queue.push(request); + debug_assert_eq!(push_outcome, QueuePushOutcome::Accepted); // Log queue statistics periodically (when adding high/urgent priority items) if matches!(priority, HealPriority::High | HealPriority::Urgent) { @@ -364,13 +522,12 @@ impl HealManager { drop(queue); - if is_new { - info!("Submitted heal request: {} with priority: {:?}", request_id, priority); - } else { - info!("Heal request already queued (duplicate): {}", request_id); + info!("Submitted heal request: {} with priority: {:?}", request_id, priority); + if config.event_driven_scheduler_enable { + self.notify.notify_one(); } - Ok(request_id) + Ok(HealAdmissionResult::Accepted) } /// Get task status @@ -441,18 +598,23 @@ impl HealManager { let cancel_token = self.cancel_token.clone(); let statistics = self.statistics.clone(); let storage = self.storage.clone(); + let notify = self.notify.clone(); tokio::spawn(async move { let mut interval = interval(config.read().await.heal_interval); loop { + let event_driven_scheduler_enable = config.read().await.event_driven_scheduler_enable; tokio::select! { _ = cancel_token.cancelled() => { info!("Heal scheduler received shutdown signal"); break; } + _ = notify.notified(), if event_driven_scheduler_enable => { + Self::process_heal_queue(&heal_queue, &active_heals, &config, &statistics, &storage, ¬ify).await; + } _ = interval.tick() => { - Self::process_heal_queue(&heal_queue, &active_heals, &config, &statistics, &storage).await; + Self::process_heal_queue(&heal_queue, &active_heals, &config, &statistics, &storage, ¬ify).await; } } } @@ -468,6 +630,7 @@ impl HealManager { let active_heals = self.active_heals.clone(); let cancel_token = self.cancel_token.clone(); let storage = self.storage.clone(); + let notify = self.notify.clone(); let mut duration = { let config = config.read().await; config.heal_interval @@ -567,8 +730,13 @@ impl HealManager { HealPriority::Normal, ); let mut queue = heal_queue.lock().await; - queue.push(req); - info!("start_auto_disk_scanner: Enqueued auto erasure set heal for endpoint: {} (set_disk_id: {})", ep, set_disk_id); + if matches!(queue.push(req), QueuePushOutcome::Accepted) { + let config = config.read().await; + if config.event_driven_scheduler_enable { + notify.notify_one(); + } + info!("start_auto_disk_scanner: Enqueued auto erasure set heal for endpoint: {} (set_disk_id: {})", ep, set_disk_id); + } } } } @@ -585,6 +753,7 @@ impl HealManager { config: &Arc>, statistics: &Arc>, storage: &Arc, + notify: &Arc, ) { let config = config.read().await; let mut active_heals_guard = active_heals.lock().await; @@ -605,28 +774,49 @@ impl HealManager { return; } - // Process multiple tasks if: - // 1. We have available slots - // 2. Queue is not empty - // Prioritize urgent/high priority tasks by processing up to 2 tasks per cycle if available - let tasks_to_process = if queue_len > 0 { - std::cmp::min(available_slots, std::cmp::min(2, queue_len)) - } else { - 0 - }; + let mut running_per_set = running_erasure_set_counts(&active_heals_guard); + let mut tasks_started = 0usize; - for _ in 0..tasks_to_process { - if let Some(request) = queue.pop() { + for _ in 0..available_slots { + let selected_request = if config.set_bulkhead_enable { + let max_concurrent_per_set = config.max_concurrent_per_set; + let (selected_request, skipped_sets) = queue.pop_runnable_with_skips( + |request| can_schedule_request(request, &running_per_set, max_concurrent_per_set), + |request| heal_request_set_key(request).map(|_| heal_request_set_metric_label(request)), + ); + for skipped_set in skipped_sets { + record_scheduler_skip(&skipped_set); + } + selected_request + } else { + queue.pop_next() + }; + + if let Some(request) = selected_request { let task_priority = request.priority; + let task_type_label = heal_request_type_label(&request).to_string(); + let task_set_label = heal_request_set_metric_label(&request); + if config.set_bulkhead_enable + && let Some(set_key) = heal_request_set_key(&request) + { + *running_per_set.entry(set_key).or_insert(0) += 1; + } let task = Arc::new(HealTask::from_request(request, storage.clone())); let task_id = task.id.clone(); active_heals_guard.insert(task_id.clone(), task.clone()); + update_task_running_metric_for_task(&active_heals_guard, task.as_ref()); let active_heals_clone = active_heals.clone(); let statistics_clone = statistics.clone(); + let notify_clone = notify.clone(); + let task_type_label_for_spawn = task_type_label.clone(); + let task_set_label_for_spawn = task_set_label.clone(); // start heal task tokio::spawn(async move { - info!("Starting heal task: {} with priority: {:?}", task_id, task_priority); + info!( + "Starting heal task: {} with priority: {:?}, type: {}, set: {}", + task_id, task_priority, task_type_label_for_spawn, task_set_label_for_spawn + ); let result = task.execute().await; match result { Ok(_) => { @@ -638,6 +828,7 @@ impl HealManager { } let mut active_heals_guard = active_heals_clone.lock().await; if let Some(completed_task) = active_heals_guard.remove(&task_id) { + update_task_running_metric_for_task(&active_heals_guard, completed_task.as_ref()); // update statistics let mut stats = statistics_clone.write().await; match completed_task.get_status().await { @@ -650,7 +841,9 @@ impl HealManager { } stats.update_running_tasks(active_heals_guard.len() as u64); } + notify_clone.notify_one(); }); + tasks_started += 1; } else { break; } @@ -658,7 +851,8 @@ impl HealManager { // Update statistics for all started tasks let mut stats = statistics.write().await; - stats.total_tasks += tasks_to_process as u64; + stats.total_tasks += tasks_started as u64; + stats.update_running_tasks(active_heals_guard.len() as u64); // Log queue status if items remain if !queue.is_empty() { @@ -681,10 +875,180 @@ impl std::fmt::Debug for HealManager { } } +fn heal_request_set_key(request: &HealRequest) -> Option { + match &request.heal_type { + HealType::ErasureSet { set_disk_id, .. } => Some(set_disk_id.clone()), + _ => None, + } +} + +fn heal_request_type_label(request: &HealRequest) -> &'static str { + match &request.heal_type { + HealType::Object { .. } => "object", + HealType::Bucket { .. } => "bucket", + HealType::ErasureSet { .. } => "erasure_set", + HealType::Metadata { .. } => "metadata", + HealType::MRF { .. } => "mrf", + HealType::ECDecode { .. } => "ec_decode", + } +} + +fn heal_request_set_metric_label(request: &HealRequest) -> String { + heal_request_set_key(request).unwrap_or_else(|| match (request.options.pool_index, request.options.set_index) { + (Some(pool), Some(set)) => format!("pool_{pool}_set_{set}"), + _ => "global".to_string(), + }) +} + +fn record_scheduler_skip(set_label: &str) { + counter!( + "rustfs_heal_scheduler_skip_total", + "reason" => "set_limit".to_string(), + "set" => set_label.to_string() + ) + .increment(1); +} + +fn update_task_running_metric_for_task(active_heals: &HashMap>, task: &HealTask) { + let type_label = task.metric_type_label(); + let set_label = task.metric_set_label(); + let count = active_heals + .values() + .filter(|active_task| active_task.metric_type_label() == type_label && active_task.metric_set_label() == set_label) + .count(); + + gauge!( + "rustfs_heal_task_running", + "type" => type_label.to_string(), + "set" => set_label.clone() + ) + .set(count as f64); +} + +fn running_erasure_set_counts(active_heals: &HashMap>) -> HashMap { + let mut running = HashMap::new(); + for task in active_heals.values() { + if let HealType::ErasureSet { set_disk_id, .. } = &task.heal_type { + *running.entry(set_disk_id.clone()).or_insert(0) += 1; + } + } + running +} + +fn can_schedule_request(request: &HealRequest, running_per_set: &HashMap, max_concurrent_per_set: usize) -> bool { + match heal_request_set_key(request) { + Some(set_key) => running_per_set.get(&set_key).copied().unwrap_or(0) < max_concurrent_per_set, + None => true, + } +} + #[cfg(test)] mod tests { use super::*; + use crate::heal::storage::HealStorageAPI; use crate::heal::task::{HealOptions, HealPriority, HealRequest, HealType}; + use rustfs_common::heal_channel::HealOpts; + use rustfs_ecstore::{ + disk::{DiskStore, endpoint::Endpoint}, + store_api::BucketInfo, + }; + use rustfs_madmin::heal_commands::HealResultItem; + + struct MockStorage; + + #[async_trait::async_trait] + impl HealStorageAPI for MockStorage { + async fn get_object_meta(&self, _bucket: &str, _object: &str) -> Result> { + Ok(None) + } + + async fn get_object_data(&self, _bucket: &str, _object: &str) -> Result>> { + Ok(None) + } + + async fn put_object_data(&self, _bucket: &str, _object: &str, _data: &[u8]) -> Result<()> { + Ok(()) + } + + async fn delete_object(&self, _bucket: &str, _object: &str) -> Result<()> { + Ok(()) + } + + async fn verify_object_integrity(&self, _bucket: &str, _object: &str) -> Result { + Ok(true) + } + + async fn ec_decode_rebuild(&self, _bucket: &str, _object: &str) -> Result> { + Ok(Vec::new()) + } + + async fn get_disk_status(&self, _endpoint: &Endpoint) -> Result { + Ok(crate::heal::storage::DiskStatus::Ok) + } + + async fn format_disk(&self, _endpoint: &Endpoint) -> Result<()> { + Ok(()) + } + + async fn get_bucket_info(&self, _bucket: &str) -> Result> { + Ok(None) + } + + async fn heal_bucket_metadata(&self, _bucket: &str) -> Result<()> { + Ok(()) + } + + async fn list_buckets(&self) -> Result> { + Ok(Vec::new()) + } + + async fn object_exists(&self, _bucket: &str, _object: &str) -> Result { + Ok(false) + } + + async fn get_object_size(&self, _bucket: &str, _object: &str) -> Result> { + Ok(None) + } + + async fn get_object_checksum(&self, _bucket: &str, _object: &str) -> Result> { + Ok(None) + } + + async fn heal_object( + &self, + _bucket: &str, + _object: &str, + _version_id: Option<&str>, + _opts: &HealOpts, + ) -> Result<(HealResultItem, Option)> { + Ok((HealResultItem::default(), None)) + } + + async fn heal_bucket(&self, _bucket: &str, _opts: &HealOpts) -> Result { + Ok(HealResultItem::default()) + } + + async fn heal_format(&self, _dry_run: bool) -> Result<(HealResultItem, Option)> { + Ok((HealResultItem::default(), None)) + } + + async fn list_objects_for_heal(&self, _bucket: &str, _prefix: &str) -> Result> { + Ok(Vec::new()) + } + + async fn list_objects_for_heal_page( + &self, + _bucket: &str, + _prefix: &str, + _continuation_token: Option<&str>, + ) -> Result<(Vec, Option, bool)> { + Ok((Vec::new(), None, false)) + } + + async fn get_disk_for_resume(&self, _set_disk_id: &str) -> Result { + Err(Error::other("not implemented in tests")) + } + } #[test] fn test_priority_queue_ordering() { @@ -724,10 +1088,10 @@ mod tests { ); // Add in random order: low, high, normal, urgent - assert!(queue.push(low_req)); - assert!(queue.push(high_req)); - assert!(queue.push(normal_req)); - assert!(queue.push(urgent_req)); + assert_eq!(queue.push(low_req), QueuePushOutcome::Accepted); + assert_eq!(queue.push(high_req), QueuePushOutcome::Accepted); + assert_eq!(queue.push(normal_req), QueuePushOutcome::Accepted); + assert_eq!(queue.push(urgent_req), QueuePushOutcome::Accepted); assert_eq!(queue.len(), 4); @@ -780,9 +1144,9 @@ mod tests { let id2 = req2.id.clone(); let id3 = req3.id.clone(); - assert!(queue.push(req1)); - assert!(queue.push(req2)); - assert!(queue.push(req3)); + assert_eq!(queue.push(req1), QueuePushOutcome::Accepted); + assert_eq!(queue.push(req2), QueuePushOutcome::Accepted); + assert_eq!(queue.push(req3), QueuePushOutcome::Accepted); // Should maintain FIFO order for same priority let popped1 = queue.pop().unwrap(); @@ -820,11 +1184,11 @@ mod tests { ); // First request should be added - assert!(queue.push(req1)); + assert_eq!(queue.push(req1), QueuePushOutcome::Accepted); assert_eq!(queue.len(), 1); // Second request with same object should be rejected (duplicate) - assert!(!queue.push(req2)); + assert_eq!(queue.push(req2), QueuePushOutcome::Merged); assert_eq!(queue.len(), 1); } @@ -841,7 +1205,7 @@ mod tests { HealPriority::Normal, ); - assert!(queue.push(req)); + assert_eq!(queue.push(req), QueuePushOutcome::Accepted); assert!(queue.contains_erasure_set("pool_0_set_1")); assert!(!queue.contains_erasure_set("pool_0_set_2")); } @@ -929,7 +1293,8 @@ mod tests { for (heal_type, priority) in requests { let req = HealRequest::new(heal_type, HealOptions::default(), priority); - queue.push(req); + let outcome = queue.push(req); + assert_eq!(outcome, QueuePushOutcome::Accepted); } assert_eq!(queue.len(), 4); @@ -954,32 +1319,41 @@ mod tests { // Add requests with different priorities for _ in 0..3 { - queue.push(HealRequest::new( - HealType::Bucket { - bucket: format!("bucket-low-{}", queue.len()), - }, - HealOptions::default(), - HealPriority::Low, - )); + assert_eq!( + queue.push(HealRequest::new( + HealType::Bucket { + bucket: format!("bucket-low-{}", queue.len()), + }, + HealOptions::default(), + HealPriority::Low, + )), + QueuePushOutcome::Accepted + ); } for _ in 0..2 { - queue.push(HealRequest::new( - HealType::Bucket { - bucket: format!("bucket-normal-{}", queue.len()), - }, - HealOptions::default(), - HealPriority::Normal, - )); + assert_eq!( + queue.push(HealRequest::new( + HealType::Bucket { + bucket: format!("bucket-normal-{}", queue.len()), + }, + HealOptions::default(), + HealPriority::Normal, + )), + QueuePushOutcome::Accepted + ); } - queue.push(HealRequest::new( - HealType::Bucket { - bucket: "bucket-high".to_string(), - }, - HealOptions::default(), - HealPriority::High, - )); + assert_eq!( + queue.push(HealRequest::new( + HealType::Bucket { + bucket: "bucket-high".to_string(), + }, + HealOptions::default(), + HealPriority::High, + )), + QueuePushOutcome::Accepted + ); let stats = queue.get_priority_stats(); @@ -995,13 +1369,16 @@ mod tests { assert!(queue.is_empty()); - queue.push(HealRequest::new( - HealType::Bucket { - bucket: "test".to_string(), - }, - HealOptions::default(), - HealPriority::Normal, - )); + assert_eq!( + queue.push(HealRequest::new( + HealType::Bucket { + bucket: "test".to_string(), + }, + HealOptions::default(), + HealPriority::Normal, + )), + QueuePushOutcome::Accepted + ); assert!(!queue.is_empty()); @@ -1009,4 +1386,223 @@ mod tests { assert!(queue.is_empty()); } + + #[test] + fn test_priority_queue_pop_runnable_skips_blocked_erasure_set() { + let mut queue = PriorityHealQueue::new(); + + let blocked = HealRequest::new( + HealType::ErasureSet { + buckets: vec!["bucket-a".to_string()], + set_disk_id: "pool_0_set_1".to_string(), + }, + HealOptions::default(), + HealPriority::Urgent, + ); + let runnable = HealRequest::new( + HealType::ErasureSet { + buckets: vec!["bucket-b".to_string()], + set_disk_id: "pool_0_set_2".to_string(), + }, + HealOptions::default(), + HealPriority::Normal, + ); + + assert_eq!(queue.push(blocked), QueuePushOutcome::Accepted); + assert_eq!(queue.push(runnable.clone()), QueuePushOutcome::Accepted); + + let mut running = HashMap::new(); + running.insert("pool_0_set_1".to_string(), 1); + + let popped = queue + .pop_runnable(|request| can_schedule_request(request, &running, 1)) + .expect("should find runnable request"); + + match popped.heal_type { + HealType::ErasureSet { set_disk_id, .. } => assert_eq!(set_disk_id, "pool_0_set_2"), + other => panic!("expected erasure set request, got {other:?}"), + } + } + + #[test] + fn test_can_schedule_request_respects_per_set_limit() { + let request = HealRequest::new( + HealType::ErasureSet { + buckets: vec!["bucket".to_string()], + set_disk_id: "pool_0_set_1".to_string(), + }, + HealOptions::default(), + HealPriority::Normal, + ); + + let mut running = HashMap::new(); + running.insert("pool_0_set_1".to_string(), 1); + + assert!(!can_schedule_request(&request, &running, 1)); + assert!(can_schedule_request(&request, &running, 2)); + } + + #[tokio::test] + async fn test_submit_heal_request_returns_merged_for_duplicate() { + let storage: Arc = Arc::new(MockStorage); + let manager = HealManager::new(storage, None); + + let request = HealRequest::new( + HealType::Object { + bucket: "bucket".to_string(), + object: "object".to_string(), + version_id: None, + }, + HealOptions::default(), + HealPriority::Low, + ); + + assert_eq!( + manager + .submit_heal_request(request.clone()) + .await + .expect("first request should be accepted"), + HealAdmissionResult::Accepted + ); + assert_eq!( + manager + .submit_heal_request(request) + .await + .expect("duplicate request should produce admission result"), + HealAdmissionResult::Merged + ); + } + + #[tokio::test] + async fn test_submit_heal_request_returns_merged_before_full_for_duplicate() { + let storage: Arc = Arc::new(MockStorage); + let manager = HealManager::new( + storage, + Some(HealConfig { + queue_size: 1, + ..HealConfig::default() + }), + ); + + let request = HealRequest::new( + HealType::Object { + bucket: "bucket".to_string(), + object: "object".to_string(), + version_id: None, + }, + HealOptions::default(), + HealPriority::Low, + ); + + assert_eq!( + manager + .submit_heal_request(request.clone()) + .await + .expect("first request should be accepted"), + HealAdmissionResult::Accepted + ); + assert_eq!( + manager + .submit_heal_request(request) + .await + .expect("duplicate request should merge even when queue is full"), + HealAdmissionResult::Merged + ); + } + + #[tokio::test] + async fn test_submit_heal_request_returns_dropped_for_low_priority_when_full() { + let storage: Arc = Arc::new(MockStorage); + let manager = HealManager::new( + storage, + Some(HealConfig { + queue_size: 1, + low_priority_drop_when_full: true, + ..HealConfig::default() + }), + ); + + let accepted = HealRequest::new( + HealType::Bucket { + bucket: "bucket-a".to_string(), + }, + HealOptions::default(), + HealPriority::Normal, + ); + let dropped = HealRequest::new( + HealType::Bucket { + bucket: "bucket-b".to_string(), + }, + HealOptions::default(), + HealPriority::Low, + ); + + assert_eq!( + manager + .submit_heal_request(accepted) + .await + .expect("first request should be accepted"), + HealAdmissionResult::Accepted + ); + assert_eq!( + manager + .submit_heal_request(dropped) + .await + .expect("low priority request should be dropped with explicit admission result"), + HealAdmissionResult::Dropped(HealAdmissionDropReason::QueueFull) + ); + } + + #[test] + fn test_running_erasure_set_counts_groups_only_erasure_tasks() { + let storage: Arc = Arc::new(MockStorage); + let erasure_task = Arc::new(HealTask::from_request( + HealRequest::new( + HealType::ErasureSet { + buckets: vec!["bucket".to_string()], + set_disk_id: "pool_0_set_1".to_string(), + }, + HealOptions::default(), + HealPriority::Normal, + ), + storage.clone(), + )); + let object_task = Arc::new(HealTask::from_request( + HealRequest::new( + HealType::Object { + bucket: "bucket".to_string(), + object: "object".to_string(), + version_id: None, + }, + HealOptions::default(), + HealPriority::Normal, + ), + storage, + )); + + let mut active = HashMap::new(); + active.insert(erasure_task.id.clone(), erasure_task); + active.insert(object_task.id.clone(), object_task); + + let counts = running_erasure_set_counts(&active); + assert_eq!(counts.get("pool_0_set_1"), Some(&1)); + assert_eq!(counts.len(), 1); + } + + #[test] + fn test_heal_config_respects_feature_flags() { + temp_env::with_vars( + [ + (rustfs_config::ENV_HEAL_EVENT_DRIVEN_SCHEDULER_ENABLE, Some("false")), + (rustfs_config::ENV_HEAL_SET_BULKHEAD_ENABLE, Some("false")), + (rustfs_config::ENV_HEAL_PAGE_PARALLEL_ENABLE, Some("false")), + ], + || { + let config = HealConfig::default(); + assert!(!config.event_driven_scheduler_enable); + assert!(!config.set_bulkhead_enable); + assert!(!config.page_parallel_enable); + }, + ); + } } diff --git a/crates/heal/src/heal/storage.rs b/crates/heal/src/heal/storage.rs index 33f6210bc..2c27f4364 100644 --- a/crates/heal/src/heal/storage.rs +++ b/crates/heal/src/heal/storage.rs @@ -17,8 +17,11 @@ use async_trait::async_trait; use rustfs_common::heal_channel::{HealOpts, HealScanMode}; use rustfs_ecstore::{ disk::{DiskStore, endpoint::Endpoint}, + error::StorageError, store::ECStore, - store_api::{BucketInfo, BucketOperations, HealOperations, ListOperations, ObjectIO, ObjectOperations, StorageAPI}, + store_api::{ + BucketInfo, BucketOperations, HealOperations, ListOperations, ObjectIO, ObjectOperations, ObjectOptions, StorageAPI, + }, }; use rustfs_madmin::heal_commands::HealResultItem; use std::sync::Arc; @@ -137,6 +140,37 @@ impl ECStoreHealStorage { } } +fn is_transient_object_exists_message(message: &str) -> bool { + let message = message.to_ascii_lowercase(); + + [ + "failed to acquire read lock", + "lock acquisition failed", + "lock acquisition timeout", + "quorum not reached", + "deadline has elapsed", + "timed out", + "network error", + "transport error", + "connection refused", + ] + .iter() + .any(|pattern| message.contains(pattern)) +} + +fn is_transient_object_exists_error(err: &StorageError) -> bool { + if err.is_quorum_error() { + return true; + } + + match err { + StorageError::Lock(lock_err) => lock_err.is_retryable() || is_transient_object_exists_message(&lock_err.to_string()), + StorageError::Io(io_err) => is_transient_object_exists_message(&io_err.to_string()), + StorageError::SlowDown | StorageError::OperationCanceled => true, + _ => false, + } +} + #[async_trait] impl HealStorageAPI for ECStoreHealStorage { async fn get_object_meta(&self, bucket: &str, object: &str) -> Result> { @@ -408,14 +442,24 @@ impl HealStorageAPI for ECStoreHealStorage { async fn object_exists(&self, bucket: &str, object: &str) -> Result { debug!("Checking object exists: {}/{}", bucket, object); - // Use get_object_info for efficient existence check without heavy heal operations - match self.ecstore.get_object_info(bucket, object, &Default::default()).await { + // Existence checks are best-effort for background heal scheduling, so avoid + // acquiring an extra namespace read lock here. + let opts = ObjectOptions { + no_lock: true, + ..Default::default() + }; + + match self.ecstore.get_object_info(bucket, object, &opts).await { Ok(_) => Ok(true), // Object exists Err(e) => { - // Map ObjectNotFound to false, other errors must be propagated! if matches!(e, rustfs_ecstore::error::StorageError::ObjectNotFound(_, _)) { debug!("Object not found: {}/{}", bucket, object); Ok(false) + } else if is_transient_object_exists_error(&e) { + warn!("Skipping object existence check for {}/{} due to transient error: {}", bucket, object, e); + Err(Error::transient_skip(format!( + "Skipped object existence check for {bucket}/{object}: {e}" + ))) } else { error!("Error checking object existence {}/{}: {}", bucket, object, e); Err(Error::other(e)) @@ -594,3 +638,34 @@ impl HealStorageAPI for ECStoreHealStorage { }) } } + +#[cfg(test)] +mod tests { + use super::{is_transient_object_exists_error, is_transient_object_exists_message}; + use rustfs_ecstore::error::StorageError; + + #[test] + fn transient_object_exists_message_matches_lock_quorum_failures() { + assert!(is_transient_object_exists_message( + "Failed to acquire read lock: ns_loc: read lock acquisition failed on bucket/object: Quorum not reached: required 2, achieved 0" + )); + assert!(is_transient_object_exists_message("deadline has elapsed")); + } + + #[test] + fn transient_object_exists_error_matches_quorum_variants() { + assert!(is_transient_object_exists_error(&StorageError::ErasureReadQuorum)); + assert!(is_transient_object_exists_error(&StorageError::InsufficientReadQuorum( + "bucket".to_string(), + "object".to_string(), + ))); + } + + #[test] + fn transient_object_exists_error_does_not_treat_not_found_as_transient() { + assert!(!is_transient_object_exists_error(&StorageError::ObjectNotFound( + "bucket".to_string(), + "object".to_string(), + ))); + } +} diff --git a/crates/heal/src/heal/task.rs b/crates/heal/src/heal/task.rs index 15afc0001..bee286560 100644 --- a/crates/heal/src/heal/task.rs +++ b/crates/heal/src/heal/task.rs @@ -14,6 +14,7 @@ use crate::heal::{ErasureSetHealer, progress::HealProgress, storage::HealStorageAPI}; use crate::{Error, Result}; +use metrics::{counter, histogram}; use rustfs_common::heal_channel::{HealOpts, HealScanMode}; use serde::{Deserialize, Serialize}; use std::{ @@ -133,16 +134,20 @@ pub struct HealRequest { pub priority: HealPriority, /// Created time pub created_at: SystemTime, + /// Queue admission time used for scheduler delay metrics + pub enqueued_at: SystemTime, } impl HealRequest { pub fn new(heal_type: HealType, options: HealOptions, priority: HealPriority) -> Self { + let now = SystemTime::now(); Self { id: Uuid::new_v4().to_string(), heal_type, options, priority, - created_at: SystemTime::now(), + created_at: now, + enqueued_at: now, } } @@ -193,6 +198,8 @@ pub struct HealTask { pub progress: Arc>, /// Created time pub created_at: SystemTime, + /// Queue admission time + pub enqueued_at: SystemTime, /// Started time pub started_at: Arc>>, /// Completed time @@ -214,6 +221,7 @@ impl HealTask { status: Arc::new(RwLock::new(HealTaskStatus::Pending)), progress: Arc::new(RwLock::new(HealProgress::new())), created_at: request.created_at, + enqueued_at: request.enqueued_at, started_at: Arc::new(RwLock::new(None)), completed_at: Arc::new(RwLock::new(None)), task_start_instant: Arc::new(RwLock::new(None)), @@ -222,6 +230,27 @@ impl HealTask { } } + pub fn metric_type_label(&self) -> &'static str { + match &self.heal_type { + HealType::Object { .. } => "object", + HealType::Bucket { .. } => "bucket", + HealType::ErasureSet { .. } => "erasure_set", + HealType::Metadata { .. } => "metadata", + HealType::MRF { .. } => "mrf", + HealType::ECDecode { .. } => "ec_decode", + } + } + + pub fn metric_set_label(&self) -> String { + match &self.heal_type { + HealType::ErasureSet { set_disk_id, .. } => set_disk_id.clone(), + _ => match (self.options.pool_index, self.options.set_index) { + (Some(pool), Some(set)) => format!("pool_{pool}_set_{set}"), + _ => "global".to_string(), + }, + } + } + async fn remaining_timeout(&self) -> Result> { if let Some(total) = self.options.timeout { let start_instant = { *self.task_start_instant.read().await }; @@ -272,11 +301,26 @@ impl HealTask { } } + async fn skip_due_to_transient_object_exists(&self, bucket: &str, object: &str, err: &Error) -> Result<()> { + warn!( + "Skipping heal for {}/{} due to transient object existence check error: {}", + bucket, object, err + ); + + let mut progress = self.progress.write().await; + progress.set_current_object(Some(format!("skipped: {bucket}/{object}"))); + progress.update_progress(0, 1, 0, 0); + Ok(()) + } + #[tracing::instrument(skip(self), fields(task_id = %self.id, heal_type = ?self.heal_type))] pub async fn execute(&self) -> Result<()> { // update status and timestamps atomically to avoid race conditions let now = SystemTime::now(); let start_instant = Instant::now(); + let queue_delay = now.duration_since(self.enqueued_at).unwrap_or_default(); + let type_label = self.metric_type_label().to_string(); + let set_label = self.metric_set_label(); { let mut status = self.status.write().await; let mut started_at = self.started_at.write().await; @@ -286,6 +330,19 @@ impl HealTask { *task_start_instant = Some(start_instant); } + histogram!( + "rustfs_heal_queue_delay_seconds", + "type" => type_label.clone(), + "set" => set_label.clone() + ) + .record(queue_delay.as_secs_f64()); + counter!( + "rustfs_heal_task_start_total", + "type" => type_label, + "set" => set_label + ) + .increment(1); + info!("Task started"); let result = match &self.heal_type { @@ -369,7 +426,13 @@ impl HealTask { // Step 1: Check if object exists and get metadata warn!("Step 1: Checking object existence and metadata"); self.check_control_flags().await?; - let object_exists = self.await_with_control(self.storage.object_exists(bucket, object)).await?; + let object_exists = match self.await_with_control(self.storage.object_exists(bucket, object)).await { + Ok(exists) => exists, + Err(err @ Error::TransientSkip { .. }) => { + return self.skip_due_to_transient_object_exists(bucket, object, &err).await; + } + Err(err) => return Err(err), + }; if !object_exists { warn!("Object does not exist: {}/{}", bucket, object); if self.options.recreate_missing { @@ -631,7 +694,13 @@ impl HealTask { // Step 1: Check if object exists info!("Step 1: Checking object existence"); self.check_control_flags().await?; - let object_exists = self.await_with_control(self.storage.object_exists(bucket, object)).await?; + let object_exists = match self.await_with_control(self.storage.object_exists(bucket, object)).await { + Ok(exists) => exists, + Err(err @ Error::TransientSkip { .. }) => { + return self.skip_due_to_transient_object_exists(bucket, object, &err).await; + } + Err(err) => return Err(err), + }; if !object_exists { warn!("Object does not exist: {}/{}", bucket, object); return Err(Error::TaskExecutionFailed { @@ -791,7 +860,13 @@ impl HealTask { // Step 1: Check if object exists info!("Step 1: Checking object existence"); self.check_control_flags().await?; - let object_exists = self.await_with_control(self.storage.object_exists(bucket, object)).await?; + let object_exists = match self.await_with_control(self.storage.object_exists(bucket, object)).await { + Ok(exists) => exists, + Err(err @ Error::TransientSkip { .. }) => { + return self.skip_due_to_transient_object_exists(bucket, object, &err).await; + } + Err(err) => return Err(err), + }; if !object_exists { warn!("Object does not exist: {}/{}", bucket, object); return Err(Error::TaskExecutionFailed { diff --git a/crates/heal/tests/heal_bug_fixes_test.rs b/crates/heal/tests/heal_bug_fixes_test.rs index 8ad67880b..5374933f4 100644 --- a/crates/heal/tests/heal_bug_fixes_test.rs +++ b/crates/heal/tests/heal_bug_fixes_test.rs @@ -281,3 +281,152 @@ fn test_heal_task_status_atomic_update() { // Note: We can't directly access private fields, but creation without panic // confirms the fix works } + +#[tokio::test] +async fn test_heal_task_transient_object_exists_skip_avoids_recreate() { + use rustfs_heal::heal::storage::{DiskStatus, HealStorageAPI}; + use rustfs_heal::heal::task::{HealOptions, HealPriority, HealRequest, HealTask, HealTaskStatus, HealType}; + use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }; + + struct MockStorage { + object_exists_calls: Arc, + heal_object_calls: Arc, + } + + #[async_trait::async_trait] + impl HealStorageAPI for MockStorage { + async fn get_object_meta( + &self, + _bucket: &str, + _object: &str, + ) -> rustfs_heal::Result> { + Ok(None) + } + + async fn get_object_data(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result>> { + Ok(None) + } + + async fn put_object_data(&self, _bucket: &str, _object: &str, _data: &[u8]) -> rustfs_heal::Result<()> { + Ok(()) + } + + async fn delete_object(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result<()> { + Ok(()) + } + + async fn verify_object_integrity(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result { + Ok(true) + } + + async fn ec_decode_rebuild(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result> { + Ok(Vec::new()) + } + + async fn get_disk_status(&self, _endpoint: &rustfs_ecstore::disk::endpoint::Endpoint) -> rustfs_heal::Result { + Ok(DiskStatus::Ok) + } + + async fn format_disk(&self, _endpoint: &rustfs_ecstore::disk::endpoint::Endpoint) -> rustfs_heal::Result<()> { + Ok(()) + } + + async fn get_bucket_info(&self, _bucket: &str) -> rustfs_heal::Result> { + Ok(None) + } + + async fn heal_bucket_metadata(&self, _bucket: &str) -> rustfs_heal::Result<()> { + Ok(()) + } + + async fn list_buckets(&self) -> rustfs_heal::Result> { + Ok(Vec::new()) + } + + async fn object_exists(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result { + self.object_exists_calls.fetch_add(1, Ordering::SeqCst); + Err(rustfs_heal::Error::transient_skip( + "Skipped object existence check for bucket/object: simulated quorum failure", + )) + } + + async fn get_object_size(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result> { + Ok(None) + } + + async fn get_object_checksum(&self, _bucket: &str, _object: &str) -> rustfs_heal::Result> { + Ok(None) + } + + async fn heal_object( + &self, + _bucket: &str, + _object: &str, + _version_id: Option<&str>, + _opts: &rustfs_common::heal_channel::HealOpts, + ) -> rustfs_heal::Result<(rustfs_madmin::heal_commands::HealResultItem, Option)> { + self.heal_object_calls.fetch_add(1, Ordering::SeqCst); + Ok((rustfs_madmin::heal_commands::HealResultItem::default(), None)) + } + + async fn heal_bucket( + &self, + _bucket: &str, + _opts: &rustfs_common::heal_channel::HealOpts, + ) -> rustfs_heal::Result { + Ok(rustfs_madmin::heal_commands::HealResultItem::default()) + } + + async fn heal_format( + &self, + _dry_run: bool, + ) -> rustfs_heal::Result<(rustfs_madmin::heal_commands::HealResultItem, Option)> { + Ok((rustfs_madmin::heal_commands::HealResultItem::default(), None)) + } + + async fn list_objects_for_heal(&self, _bucket: &str, _prefix: &str) -> rustfs_heal::Result> { + Ok(Vec::new()) + } + + async fn list_objects_for_heal_page( + &self, + _bucket: &str, + _prefix: &str, + _continuation_token: Option<&str>, + ) -> rustfs_heal::Result<(Vec, Option, bool)> { + Ok((Vec::new(), None, false)) + } + + async fn get_disk_for_resume(&self, _set_disk_id: &str) -> rustfs_heal::Result { + Err(rustfs_heal::Error::other("not implemented")) + } + } + + let object_exists_calls = Arc::new(AtomicUsize::new(0)); + let heal_object_calls = Arc::new(AtomicUsize::new(0)); + let storage: Arc = Arc::new(MockStorage { + object_exists_calls: object_exists_calls.clone(), + heal_object_calls: heal_object_calls.clone(), + }); + + let request = HealRequest::new( + HealType::Object { + bucket: "bucket".to_string(), + object: "object".to_string(), + version_id: None, + }, + HealOptions::default(), + HealPriority::Normal, + ); + + let task = HealTask::from_request(request, storage); + task.execute().await.expect("transient existence check should be skipped"); + + assert_eq!(object_exists_calls.load(Ordering::SeqCst), 1); + assert_eq!(heal_object_calls.load(Ordering::SeqCst), 0); + assert_eq!(task.get_status().await, HealTaskStatus::Completed); + assert!(task.get_progress().await.is_completed()); +} diff --git a/crates/heal/tests/heal_integration_test.rs b/crates/heal/tests/heal_integration_test.rs index e7ba323ef..56711518a 100644 --- a/crates/heal/tests/heal_integration_test.rs +++ b/crates/heal/tests/heal_integration_test.rs @@ -277,10 +277,12 @@ mod serial_tests { HealPriority::Normal, ); - let task_id = heal_manager + let task_id = heal_request.id.clone(); + let admission = heal_manager .submit_heal_request(heal_request) .await .expect("Failed to submit bucket heal request"); + assert!(admission.is_admitted(), "bucket heal request should be admitted"); info!("Submitted bucket heal request with task ID: {}", task_id); diff --git a/crates/iam/src/lib.rs b/crates/iam/src/lib.rs index b5262357c..24461f78e 100644 --- a/crates/iam/src/lib.rs +++ b/crates/iam/src/lib.rs @@ -100,7 +100,7 @@ pub async fn init_oidc_sys() -> Result<()> { } Err(e) => { warn!("OIDC initialization failed (non-fatal): {}", e); - OidcSys::empty() + OidcSys::empty().map_err(Error::StringError)? } }; diff --git a/crates/iam/src/oidc.rs b/crates/iam/src/oidc.rs index a6bfdc7ec..ace5034e6 100644 --- a/crates/iam/src/oidc.rs +++ b/crates/iam/src/oidc.rs @@ -24,6 +24,7 @@ use openidconnect::{ AsyncHttpClient, AuthType, AuthorizationCode, ClientId, ClientSecret, CsrfToken, IssuerUrl, Nonce, PkceCodeChallenge, PkceCodeVerifier, RedirectUrl, Scope, }; +use reqwest::Client; use rustfs_config::oidc::*; use rustfs_config::{DEFAULT_DELIMITER, ENABLE_KEY, EnableState}; use rustfs_ecstore::config::{Config as ServerConfig, KVS, get_global_server_config}; @@ -32,13 +33,17 @@ use serde::{Deserialize, Serialize}; use std::borrow::Cow; use std::collections::HashMap; use std::future::Future; +use std::net::IpAddr; use std::pin::Pin; use std::sync::RwLock; use std::time::{Duration as StdDuration, Instant}; +use tokio::time::sleep; use tracing::{error, info, warn}; use url::Url; const OIDC_JWKS_REFRESH_INTERVAL: StdDuration = StdDuration::from_secs(24 * 60 * 60); +const OIDC_DISCOVERY_TRANSPORT_RETRIES: usize = 3; +const OIDC_DISCOVERY_TRANSPORT_RETRY_DELAY: StdDuration = StdDuration::from_millis(50); // ---- HTTP Client Adapter ---- @@ -68,7 +73,46 @@ impl std::error::Error for OidcHttpError { } /// HTTP client adapter bridging reqwest 0.13 to the `openidconnect` `AsyncHttpClient` trait. -pub(crate) struct ReqwestHttpClient(reqwest::Client); +pub(crate) struct ReqwestHttpClient { + default: Client, + no_proxy: Client, +} + +fn build_oidc_http_client(disable_proxy: bool) -> Result { + let mut builder = reqwest::Client::builder(); + if disable_proxy { + builder = builder.no_proxy(); + } + builder + .build() + .map_err(|err| format!("failed to build OIDC reqwest client: {err}")) +} + +fn should_bypass_proxy_for_oidc_uri(uri: &str) -> bool { + let Some(host) = Url::parse(uri).ok().and_then(|url| url.host_str().map(str::to_owned)) else { + return false; + }; + let host = host.trim_matches(['[', ']']); + + host.eq_ignore_ascii_case("localhost") || host.parse::().is_ok_and(|addr| addr.is_loopback()) +} + +impl ReqwestHttpClient { + fn new() -> Result { + Ok(Self { + default: build_oidc_http_client(false)?, + no_proxy: build_oidc_http_client(true)?, + }) + } + + fn client_for_uri(&self, uri: &str) -> &Client { + if should_bypass_proxy_for_oidc_uri(uri) { + &self.no_proxy + } else { + &self.default + } + } +} impl<'c> AsyncHttpClient<'c> for ReqwestHttpClient { type Error = OidcHttpError; @@ -77,9 +121,10 @@ impl<'c> AsyncHttpClient<'c> for ReqwestHttpClient { fn call(&'c self, request: http::Request>) -> Self::Future { Box::pin(async move { let (parts, body) = request.into_parts(); - let response = self - .0 - .request(parts.method, parts.uri.to_string()) + let uri = parts.uri.to_string(); + let client = self.client_for_uri(&uri); + let response = client + .request(parts.method, uri) .headers(parts.headers) .body(body) .send() @@ -193,7 +238,7 @@ pub struct OidcSys { impl OidcSys { /// Parse environment variables and discover all configured OIDC providers. pub async fn new() -> Result { - let http_client = ReqwestHttpClient(reqwest::Client::new()); + let http_client = ReqwestHttpClient::new()?; let parsed_configs = load_effective_oidc_provider_configs(get_global_server_config().as_ref()); let mut configs = HashMap::new(); let mut provider_states = HashMap::new(); @@ -226,13 +271,13 @@ impl OidcSys { } /// Create an OidcSys with no providers (useful for when OIDC is not configured). - pub fn empty() -> Self { - Self { + pub fn empty() -> Result { + Ok(Self { configs: HashMap::new(), provider_states: RwLock::new(HashMap::new()), state_store: OidcStateStore::new(), - http_client: ReqwestHttpClient(reqwest::Client::new()), - } + http_client: ReqwestHttpClient::new()?, + }) } /// Return true if any OIDC providers are configured and enabled. @@ -848,22 +893,40 @@ impl OidcSys { for candidate_issuer in candidates.iter() { let issuer_url = IssuerUrl::new(candidate_issuer.clone()).map_err(|e| format!("invalid issuer URL: {e}"))?; - match CoreProviderMetadata::discover_async(issuer_url, http_client) - .await - .map_err(|e| format!("discovery failed: {e}")) - { - Ok(metadata) => { - return Ok(ProviderState { - metadata, - discovered_at: Instant::now(), - }); - } - Err(error) => { - last_errors.push(format!("issuer '{candidate_issuer}': {error}")); - warn!( - "OIDC provider '{}' discovery attempt failed for issuer '{}': {}", - config.id, candidate_issuer, error - ); + for attempt in 0..OIDC_DISCOVERY_TRANSPORT_RETRIES { + match CoreProviderMetadata::discover_async(issuer_url.clone(), http_client) + .await + .map_err(|e| format!("discovery failed: {e}")) + { + Ok(metadata) => { + return Ok(ProviderState { + metadata, + discovered_at: Instant::now(), + }); + } + Err(error) => { + let is_transient_transport = error.contains("Request failed"); + let should_retry = is_transient_transport && attempt + 1 < OIDC_DISCOVERY_TRANSPORT_RETRIES; + if should_retry { + warn!( + "OIDC provider '{}' discovery transport attempt {}/{} failed for issuer '{}': {}", + config.id, + attempt + 1, + OIDC_DISCOVERY_TRANSPORT_RETRIES, + candidate_issuer, + error + ); + sleep(OIDC_DISCOVERY_TRANSPORT_RETRY_DELAY).await; + continue; + } + + last_errors.push(format!("issuer '{candidate_issuer}': {error}")); + warn!( + "OIDC provider '{}' discovery attempt failed for issuer '{}': {}", + config.id, candidate_issuer, error + ); + break; + } } } } @@ -924,7 +987,7 @@ pub fn load_effective_oidc_provider_configs(server_config: Option<&ServerConfig> } pub async fn validate_oidc_provider_config(config: &OidcProviderConfig) -> Result { - let http_client = ReqwestHttpClient(reqwest::Client::new()); + let http_client = ReqwestHttpClient::new()?; let state = OidcSys::discover_provider(config, &http_client).await?; Ok(OidcProviderValidationResult { @@ -1312,11 +1375,12 @@ mod tests { use std::io::Read; use std::io::Write; use std::net::{Shutdown, TcpListener}; + use std::sync::mpsc; use std::time::{Duration, Instant}; // After the last completed response, exit if no new connection arrives within this window. - const IDLE_SHUTDOWN: Duration = Duration::from_millis(100); - const ABSOLUTE_CAP: Duration = Duration::from_millis(500); + const IDLE_SHUTDOWN: Duration = Duration::from_millis(500); + const ABSOLUTE_CAP: Duration = Duration::from_secs(5); let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let base = format!("http://{}", listener.local_addr().unwrap()); @@ -1333,11 +1397,13 @@ mod tests { }) .to_string(); let jwks_body = r#"{"keys":[]}"#; + let (ready_tx, ready_rx) = mpsc::channel(); let handle = std::thread::spawn(move || { listener .set_nonblocking(true) .expect("failed to set discovery mock listener non-blocking"); + let _ = ready_tx.send(()); let mut seen = 0usize; let start = Instant::now(); @@ -1404,6 +1470,9 @@ mod tests { } } }); + ready_rx + .recv_timeout(Duration::from_millis(100)) + .expect("mock OIDC discovery server should become ready"); (base, handle) } @@ -1451,7 +1520,7 @@ mod tests { #[test] fn test_map_claims_to_policies_no_provider() { - let sys = OidcSys::empty(); + let sys = OidcSys::empty().expect("failed to initialize empty OIDC system"); let claims = OidcClaims { sub: "user123".to_string(), @@ -1610,11 +1679,22 @@ mod tests { #[test] fn test_oidc_sys_empty() { - let sys = OidcSys::empty(); + let sys = OidcSys::empty().expect("failed to initialize empty OIDC system"); assert!(!sys.has_providers()); assert!(sys.list_providers().is_empty()); } + #[test] + fn test_should_bypass_proxy_for_oidc_uri_loopback_only() { + assert!(should_bypass_proxy_for_oidc_uri("http://127.0.0.1:9000/.well-known/openid-configuration")); + assert!(should_bypass_proxy_for_oidc_uri("http://localhost:9000/.well-known/openid-configuration")); + assert!(should_bypass_proxy_for_oidc_uri("http://[::1]:9000/.well-known/openid-configuration")); + assert!(!should_bypass_proxy_for_oidc_uri( + "https://idp.example.com/.well-known/openid-configuration" + )); + assert!(!should_bypass_proxy_for_oidc_uri("not-a-url")); + } + /// Helper to create an OidcSys with configs only (no provider states needed). fn make_test_sys(configs: Vec) -> OidcSys { let mut config_map = HashMap::new(); @@ -1625,7 +1705,7 @@ mod tests { configs: config_map, provider_states: RwLock::new(HashMap::new()), state_store: OidcStateStore::new(), - http_client: ReqwestHttpClient(reqwest::Client::new()), + http_client: ReqwestHttpClient::new().expect("failed to initialize OIDC HTTP clients"), } } diff --git a/crates/lock/src/client/local.rs b/crates/lock/src/client/local.rs index 4503dfd3d..04aa1e011 100644 --- a/crates/lock/src/client/local.rs +++ b/crates/lock/src/client/local.rs @@ -108,7 +108,7 @@ impl LockClient for LocalClient { match lock_manager.acquire_lock(lock_request).await { Ok(guard) => { - let lock_id = LockId::new_unique(&request.resource); + let lock_id = request.lock_id.clone(); { let shard = self.get_shard(&lock_id); diff --git a/crates/lock/src/distributed_lock.rs b/crates/lock/src/distributed_lock.rs index 9928daffe..c98a172be 100644 --- a/crates/lock/src/distributed_lock.rs +++ b/crates/lock/src/distributed_lock.rs @@ -22,6 +22,7 @@ use futures::future::join_all; use std::sync::{Arc, LazyLock}; use std::time::Duration; use tokio::sync::mpsc; +use tokio::task::JoinSet; use tracing::warn; use uuid::Uuid; @@ -180,6 +181,8 @@ pub struct DistributedLock { quorum: usize, } +type LockAcquireTaskResult = (usize, Result); + impl DistributedLock { /// Create new distributed lock pub fn new(namespace: String, clients: Vec>, quorum: usize) -> Self { @@ -327,108 +330,196 @@ impl DistributedLock { self.acquire_guard(&req).await } + fn spawn_lock_requests(&self, request: &LockRequest) -> JoinSet { + let mut pending = JoinSet::new(); + for (idx, client) in self.clients.iter().cloned().enumerate() { + let request = request.clone(); + pending.spawn(async move { (idx, client.acquire_lock(&request).await) }); + } + pending + } + + async fn release_entries(entries: &[(LockId, Arc)], context: &'static str) { + let release_results = join_all( + entries + .iter() + .map(|(lock_id, client)| async move { (lock_id, client.release(lock_id).await) }), + ) + .await; + + for (lock_id, result) in release_results { + match result { + Ok(true) | Ok(false) => {} + Err(err) => { + tracing::warn!("{context}: failed to release lock {} on client: {}", lock_id, err); + } + } + } + } + + fn spawn_pending_cleanup( + mut pending: JoinSet, + clients: Vec>, + fallback_lock_id: LockId, + context: &'static str, + ) { + let handle = tokio::spawn(async move { + while let Some(join_result) = pending.join_next().await { + match join_result { + Ok((idx, Ok(resp))) if resp.success => { + let lock_id = resp + .lock_info + .as_ref() + .map(|info| info.id.clone()) + .unwrap_or_else(|| fallback_lock_id.clone()); + let Some(client) = clients.get(idx) else { + tracing::warn!("{context}: missing client for pending lock cleanup at index {}", idx); + continue; + }; + + if let Err(err) = client.release(&lock_id).await { + tracing::warn!("{context}: failed to cleanup late lock {} on client {}: {}", lock_id, idx, err); + } + } + Ok((idx, Ok(resp))) => { + tracing::debug!( + "{context}: pending lock request on client {} completed without success: {:?}", + idx, + resp.error + ); + } + Ok((idx, Err(err))) => { + tracing::warn!("{context}: pending lock request on client {} failed: {}", idx, err); + } + Err(err) => { + tracing::warn!("{context}: pending lock cleanup task join failed: {}", err); + } + } + } + }); + drop(handle); + } + + fn log_failed_lock_response(&self, request: &LockRequest, idx: usize, error: String) { + if request.suppress_contention_logs { + tracing::debug!( + resource = %request.resource, + owner = %request.owner, + "Failed to acquire lock on client from response: {}, error: {}", + idx, + error + ); + } else { + tracing::warn!( + resource = %request.resource, + owner = %request.owner, + "Failed to acquire lock on client from response: {}, error: {}", + idx, + error + ); + } + } + /// Quorum-based lock acquisition: success if at least the required quorum succeeds. /// Collects all individual lock_ids from successful clients and creates an aggregate lock_id. /// Returns the LockResponse with aggregate lock_id and individual lock mappings. async fn acquire_lock_quorum(&self, request: &LockRequest) -> Result<(LockResponse, Vec<(LockId, Arc)>)> { let required_quorum = self.required_quorum(request.lock_type); - let futs: Vec<_> = self - .clients - .iter() - .enumerate() - .map(|(idx, client)| async move { (idx, client.acquire_lock(request).await) }) - .collect(); - - let results = futures::future::join_all(futs).await; - - // Store all individual lock_ids and their corresponding clients + let mut pending = self.spawn_lock_requests(request); let mut individual_locks: Vec<(LockId, Arc)> = Vec::new(); + let fallback_lock_id = request.lock_id.clone(); - for (idx, result) in results { - match result { - Ok(resp) => { + while let Some(join_result) = pending.join_next().await { + match join_result { + Ok((idx, Ok(resp))) => { if resp.success { - // Collect individual lock_id and client for each successful acquisition - if let Some(lock_info) = &resp.lock_info - && idx < self.clients.len() - { - // Save the individual lock_id returned by each client - individual_locks.push((lock_info.id.clone(), self.clients[idx].clone())); + let lock_id = resp + .lock_info + .as_ref() + .map(|info| info.id.clone()) + .unwrap_or_else(|| fallback_lock_id.clone()); + + if let Some(client) = self.clients.get(idx) { + individual_locks.push((lock_id, client.clone())); + } else { + tracing::warn!("Missing lock client at index {} while recording success", idx); } } else { let error = resp.error.unwrap_or_else(|| "unknown error".to_string()); - if request.suppress_contention_logs { - tracing::debug!( - resource = %request.resource, - owner = %request.owner, - "Failed to acquire lock on client from response: {}, error: {}", - idx, - error - ); - } else { - tracing::warn!( - resource = %request.resource, - owner = %request.owner, - "Failed to acquire lock on client from response: {}, error: {}", - idx, - error - ); - } + self.log_failed_lock_response(request, idx, error); } } - Err(e) => { - tracing::warn!("Failed to acquire lock on client {}: {}", idx, e); + Ok((idx, Err(err))) => { + tracing::warn!("Failed to acquire lock on client {}: {}", idx, err); } - } - } - - if individual_locks.len() >= required_quorum { - // Generate a new aggregate lock_id for multiple client locks - let aggregate_lock_id = generate_aggregate_lock_id(&request.resource); - - tracing::debug!( - "Generated aggregate lock_id {} for {} individual locks on resource {}", - aggregate_lock_id, - individual_locks.len(), - request.resource - ); - - let resp = LockResponse::success( - LockInfo { - id: aggregate_lock_id, - resource: request.resource.clone(), - lock_type: request.lock_type, - status: LockStatus::Acquired, - owner: request.owner.clone(), - acquired_at: std::time::SystemTime::now(), - expires_at: std::time::SystemTime::now() + request.ttl, - last_refreshed: std::time::SystemTime::now(), - metadata: request.metadata.clone(), - priority: request.priority, - wait_start_time: None, - }, - Duration::ZERO, - ); - Ok((resp, individual_locks)) - } else { - // Rollback: release all locks that were successfully acquired - let rollback_count = individual_locks.len(); - let rollback_results = join_all(individual_locks.iter().map(|(individual_lock_id, client)| async move { - (individual_lock_id, client.release(individual_lock_id).await) - })) - .await; - - for (individual_lock_id, result) in rollback_results { - if let Err(e) = result { - tracing::warn!("Failed to rollback lock {} on client: {}", individual_lock_id, e); + Err(err) => { + tracing::warn!("Lock acquisition task join failed: {}", err); } } - let resp = LockResponse::failure( - format!("Failed to acquire quorum: {rollback_count}/{required_quorum} required"), - Duration::ZERO, - ); - Ok((resp, individual_locks)) + if individual_locks.len() >= required_quorum { + if !pending.is_empty() { + Self::spawn_pending_cleanup( + pending, + self.clients.clone(), + fallback_lock_id.clone(), + "distributed_lock_success_cleanup", + ); + } + + let aggregate_lock_id = generate_aggregate_lock_id(&request.resource); + tracing::debug!( + "Generated aggregate lock_id {} for {} individual locks on resource {}", + aggregate_lock_id, + individual_locks.len(), + request.resource + ); + + let resp = LockResponse::success( + LockInfo { + id: aggregate_lock_id, + resource: request.resource.clone(), + lock_type: request.lock_type, + status: LockStatus::Acquired, + owner: request.owner.clone(), + acquired_at: std::time::SystemTime::now(), + expires_at: std::time::SystemTime::now() + request.ttl, + last_refreshed: std::time::SystemTime::now(), + metadata: request.metadata.clone(), + priority: request.priority, + wait_start_time: None, + }, + Duration::ZERO, + ); + return Ok((resp, individual_locks)); + } + + if individual_locks.len() + pending.len() < required_quorum { + let rollback_count = individual_locks.len(); + Self::release_entries(&individual_locks, "distributed_lock_quorum_rollback").await; + if !pending.is_empty() { + Self::spawn_pending_cleanup( + pending, + self.clients.clone(), + fallback_lock_id.clone(), + "distributed_lock_failure_cleanup", + ); + } + + let resp = LockResponse::failure( + format!("Failed to acquire quorum: {rollback_count}/{required_quorum} required"), + Duration::ZERO, + ); + return Ok((resp, individual_locks)); + } } + + let rollback_count = individual_locks.len(); + Self::release_entries(&individual_locks, "distributed_lock_quorum_rollback").await; + let resp = LockResponse::failure( + format!("Failed to acquire quorum: {rollback_count}/{required_quorum} required"), + Duration::ZERO, + ); + Ok((resp, individual_locks)) } } diff --git a/crates/lock/src/namespace/tests.rs b/crates/lock/src/namespace/tests.rs index 1b11caa79..5ce5e91f8 100644 --- a/crates/lock/src/namespace/tests.rs +++ b/crates/lock/src/namespace/tests.rs @@ -61,6 +61,52 @@ impl crate::client::LockClient for FailingClient { } } +#[derive(Debug)] +struct DelayedClient { + inner: Arc, + delay: Duration, +} + +#[async_trait::async_trait] +impl crate::client::LockClient for DelayedClient { + async fn acquire_lock(&self, request: &LockRequest) -> crate::Result { + tokio::time::sleep(self.delay).await; + self.inner.acquire_lock(request).await + } + + async fn release(&self, lock_id: &LockId) -> crate::Result { + self.inner.release(lock_id).await + } + + async fn refresh(&self, lock_id: &LockId) -> crate::Result { + self.inner.refresh(lock_id).await + } + + async fn force_release(&self, lock_id: &LockId) -> crate::Result { + self.inner.force_release(lock_id).await + } + + async fn check_status(&self, lock_id: &LockId) -> crate::Result> { + self.inner.check_status(lock_id).await + } + + async fn get_stats(&self) -> crate::Result { + self.inner.get_stats().await + } + + async fn close(&self) -> crate::Result<()> { + self.inner.close().await + } + + async fn is_online(&self) -> bool { + self.inner.is_online().await + } + + async fn is_local(&self) -> bool { + self.inner.is_local().await + } +} + fn create_test_object_key(bucket: &str, object: &str) -> ObjectKey { ObjectKey { bucket: Arc::from(bucket), @@ -557,3 +603,103 @@ async fn test_namespace_lock_distributed_even_node_read_write_quorum_split() { "expected quorum error, got: {err}" ); } + +#[tokio::test] +async fn test_namespace_lock_distributed_read_lock_returns_after_quorum_without_waiting_for_slow_clients() { + let manager_fast_1 = Arc::new(GlobalLockManager::new()); + let manager_fast_2 = Arc::new(GlobalLockManager::new()); + let manager_slow_1 = Arc::new(GlobalLockManager::new()); + let manager_slow_2 = Arc::new(GlobalLockManager::new()); + + let client_fast_1: Arc = Arc::new(LocalClient::with_manager(manager_fast_1)); + let client_fast_2: Arc = Arc::new(LocalClient::with_manager(manager_fast_2)); + let client_slow_1: Arc = Arc::new(DelayedClient { + inner: Arc::new(LocalClient::with_manager(manager_slow_1.clone())), + delay: Duration::from_millis(250), + }); + let client_slow_2: Arc = Arc::new(DelayedClient { + inner: Arc::new(LocalClient::with_manager(manager_slow_2.clone())), + delay: Duration::from_millis(250), + }); + + let lock = NamespaceLock::with_clients( + "four-node-read".to_string(), + vec![client_fast_1, client_fast_2, client_slow_1, client_slow_2], + ); + let resource = create_test_object_key("bucket", "object"); + + let started = tokio::time::Instant::now(); + let mut guard = lock + .get_read_lock(resource.clone(), "owner-a", Duration::from_secs(1)) + .await + .expect("read lock should succeed after reaching quorum"); + + assert!( + started.elapsed() < Duration::from_millis(150), + "read lock should return once quorum is satisfied instead of waiting for slow clients" + ); + assert!(guard.release(), "distributed read guard should release successfully"); + + tokio::time::sleep(Duration::from_millis(350)).await; + + let slow_lock_1 = NamespaceLock::with_local_manager("slow-node-1".to_string(), manager_slow_1); + let slow_lock_2 = NamespaceLock::with_local_manager("slow-node-2".to_string(), manager_slow_2); + + let write_guard_1 = slow_lock_1 + .get_write_lock(resource.clone(), "owner-b", Duration::from_millis(100)) + .await + .expect("late successful read lock should be cleaned up on slow node 1"); + let write_guard_2 = slow_lock_2 + .get_write_lock(resource, "owner-b", Duration::from_millis(100)) + .await + .expect("late successful read lock should be cleaned up on slow node 2"); + + drop(write_guard_1); + drop(write_guard_2); +} + +#[tokio::test] +async fn test_namespace_lock_distributed_failure_returns_early_and_cleans_up_late_successes() { + let manager_fast = Arc::new(GlobalLockManager::new()); + let manager_slow = Arc::new(GlobalLockManager::new()); + + let client_fast: Arc = Arc::new(LocalClient::with_manager(manager_fast)); + let client_fail_1: Arc = Arc::new(FailingClient); + let client_fail_2: Arc = Arc::new(FailingClient); + let client_slow: Arc = Arc::new(DelayedClient { + inner: Arc::new(LocalClient::with_manager(manager_slow.clone())), + delay: Duration::from_millis(250), + }); + + let lock = NamespaceLock::with_clients( + "four-node-write".to_string(), + vec![client_fast, client_fail_1, client_fail_2, client_slow], + ); + let resource = create_test_object_key("bucket", "object"); + + let started = tokio::time::Instant::now(); + let err = lock + .get_write_lock(resource.clone(), "owner-a", Duration::from_secs(1)) + .await + .expect_err("write lock should fail when quorum becomes impossible"); + + assert!( + started.elapsed() < Duration::from_millis(150), + "write lock should fail as soon as quorum becomes impossible" + ); + let err_str = err.to_string().to_lowercase(); + assert!( + err_str.contains("quorum") || err_str.contains("not reached"), + "expected quorum failure, got: {err}" + ); + + tokio::time::sleep(Duration::from_millis(350)).await; + + let slow_lock = NamespaceLock::with_local_manager("slow-node".to_string(), manager_slow); + let write_guard = slow_lock + .get_write_lock(resource, "owner-b", Duration::from_millis(100)) + .await + .expect("late successful write lock should be cleaned up after early quorum failure"); + + drop(write_guard); +} diff --git a/crates/madmin/src/info_commands.rs b/crates/madmin/src/info_commands.rs index 76c94ecaa..1c326e01c 100644 --- a/crates/madmin/src/info_commands.rs +++ b/crates/madmin/src/info_commands.rs @@ -86,6 +86,10 @@ pub struct Disk { pub write_latency: f64, pub utilization: f64, pub metrics: Option, + #[serde(rename = "runtimeState", default, skip_serializing_if = "Option::is_none")] + pub runtime_state: Option, + #[serde(rename = "offlineDurationSeconds", default, skip_serializing_if = "Option::is_none")] + pub offline_duration_seconds: Option, pub heal_info: Option, pub used_inodes: u64, pub free_inodes: u64, @@ -465,6 +469,8 @@ mod tests { write_latency: 7.8, utilization: 50.0, metrics: Some(DiskMetrics::default()), + runtime_state: Some("online".to_string()), + offline_duration_seconds: Some(0), heal_info: None, used_inodes: 1000000, free_inodes: 9000000, @@ -485,6 +491,8 @@ mod tests { assert_eq!(disk.total_space, 1000000000000); assert_eq!(disk.utilization, 50.0); assert!(disk.metrics.is_some()); + assert_eq!(disk.runtime_state.as_deref(), Some("online")); + assert_eq!(disk.offline_duration_seconds, Some(0)); assert!(disk.local); } diff --git a/crates/rio/src/http_reader.rs b/crates/rio/src/http_reader.rs index 39ef43ecd..bd770f24b 100644 --- a/crates/rio/src/http_reader.rs +++ b/crates/rio/src/http_reader.rs @@ -27,8 +27,10 @@ use std::ops::Not as _; use std::pin::Pin; use std::sync::LazyLock; use std::task::{Context, Poll}; +use std::time::Duration; use tokio::io::{AsyncRead, AsyncWrite, ReadBuf}; use tokio::sync::mpsc; +use tokio::time::{self, Sleep}; use tokio_util::io::StreamReader; use tokio_util::sync::PollSender; use tracing::error; @@ -143,6 +145,8 @@ pin_project! { method: Method, headers: HeaderMap, track_internode_metrics: bool, + stall_timeout: Option, + stall_timer: Option>>, #[pin] inner: StreamReader>+Send+Sync>>, Bytes>, } @@ -151,8 +155,19 @@ pin_project! { impl HttpReader { pub async fn new(url: String, method: Method, headers: HeaderMap, body: Option>) -> io::Result { // http_log!("[HttpReader::new] url: {url}, method: {method:?}, headers: {headers:?}"); - Self::with_capacity(url, method, headers, body, 0).await + Self::with_capacity_and_stall_timeout(url, method, headers, body, 0, None).await } + + pub async fn new_with_stall_timeout( + url: String, + method: Method, + headers: HeaderMap, + body: Option>, + stall_timeout: Option, + ) -> io::Result { + Self::with_capacity_and_stall_timeout(url, method, headers, body, 0, stall_timeout).await + } + /// Create a new HttpReader from a URL. The request is performed immediately. pub async fn with_capacity( url: String, @@ -160,6 +175,17 @@ impl HttpReader { headers: HeaderMap, body: Option>, _read_buf_size: usize, + ) -> io::Result { + Self::with_capacity_and_stall_timeout(url, method, headers, body, _read_buf_size, None).await + } + + async fn with_capacity_and_stall_timeout( + url: String, + method: Method, + headers: HeaderMap, + body: Option>, + _read_buf_size: usize, + stall_timeout: Option, ) -> io::Result { let track_internode_metrics = is_internode_rpc_url(&url); let client = get_http_client(&url); @@ -202,6 +228,8 @@ impl HttpReader { method, headers, track_internode_metrics, + stall_timer: stall_timeout.map(|timeout| Box::pin(time::sleep(timeout))), + stall_timeout, }) } pub fn url(&self) -> &str { @@ -216,16 +244,40 @@ impl HttpReader { } impl AsyncRead for HttpReader { - fn poll_read(mut self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut ReadBuf<'_>) -> Poll> { + fn poll_read(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut ReadBuf<'_>) -> Poll> { + let mut this = self.project(); + let filled_before = buf.filled().len(); - match Pin::new(&mut self.inner).poll_read(cx, buf) { + match this.inner.as_mut().poll_read(cx, buf) { Poll::Ready(Ok(())) => { let bytes_read = buf.filled().len().saturating_sub(filled_before); - if self.track_internode_metrics && bytes_read > 0 { + if *this.track_internode_metrics && bytes_read > 0 { global_internode_metrics().record_recv_bytes(bytes_read); } + if bytes_read > 0 { + if let Some(stall_timeout) = *this.stall_timeout { + *this.stall_timer = Some(Box::pin(time::sleep(stall_timeout))); + } + } else { + *this.stall_timer = None; + } Poll::Ready(Ok(())) } + Poll::Pending => { + if let Some(timer) = this.stall_timer.as_mut() + && timer.as_mut().poll(cx).is_ready() + { + if *this.track_internode_metrics { + global_internode_metrics().record_error(); + } + Poll::Ready(Err(Error::new( + io::ErrorKind::TimedOut, + "HttpReader stall timeout: no data received before deadline", + ))) + } else { + Poll::Pending + } + } other => other, } } @@ -570,8 +622,9 @@ impl AsyncWrite for HttpWriter { mod tests { use super::*; use axum::{Router, body::Body, extract::State, http::StatusCode, response::IntoResponse, routing::get}; + use futures::stream::{self, StreamExt as _}; use http_body_util::BodyExt as _; - use std::io::IoSlice; + use std::io::{self, IoSlice}; use std::sync::{ Arc, atomic::{AtomicUsize, Ordering}, @@ -595,6 +648,12 @@ mod tests { (StatusCode::OK, Body::from("hello")) } + async fn get_stalling_stream(State(state): State) -> impl IntoResponse { + state.get_count.fetch_add(1, Ordering::SeqCst); + let body_stream = stream::once(async { Ok::(Bytes::from_static(b"hello")) }).chain(stream::pending()); + (StatusCode::OK, Body::from_stream(body_stream)) + } + async fn reject_head(State(state): State) -> impl IntoResponse { state.head_count.fetch_add(1, Ordering::SeqCst); StatusCode::METHOD_NOT_ALLOWED @@ -612,6 +671,7 @@ mod tests { let addr = listener.local_addr().unwrap(); let app = Router::new() .route("/stream", get(get_stream).head(reject_head).put(accept_put)) + .route("/stall", get(get_stalling_stream)) .with_state(state); let handle = tokio::spawn(async move { @@ -637,6 +697,31 @@ mod tests { handle.abort(); } + #[tokio::test] + async fn http_reader_stall_timeout_triggers_after_progress_stops() { + let state = TestState::default(); + let (base_url, handle) = start_test_server(state.clone()).await; + let url = base_url.replace("/stream", "/stall"); + + let mut reader = + HttpReader::new_with_stall_timeout(url, Method::GET, HeaderMap::new(), None, Some(Duration::from_millis(20))) + .await + .unwrap(); + + let mut first = [0u8; 5]; + reader.read_exact(&mut first).await.unwrap(); + assert_eq!(&first, b"hello"); + + let mut next = [0u8; 1]; + let err = tokio::time::timeout(Duration::from_secs(1), reader.read(&mut next)) + .await + .expect("stall timeout should wake reader") + .expect_err("reader should return a timeout error"); + assert_eq!(err.kind(), io::ErrorKind::TimedOut); + + handle.abort(); + } + #[tokio::test] async fn http_writer_does_not_send_empty_preflight_put() { let state = TestState::default(); diff --git a/crates/scanner/Cargo.toml b/crates/scanner/Cargo.toml index ec97bfa1b..42f5555f5 100644 --- a/crates/scanner/Cargo.toml +++ b/crates/scanner/Cargo.toml @@ -50,6 +50,7 @@ rustfs-ecstore = { workspace = true } http = { workspace = true } rand = { workspace = true } s3s = { workspace = true } +metrics = { workspace = true } [dev-dependencies] tracing-subscriber = { workspace = true } diff --git a/crates/scanner/src/scanner_folder.rs b/crates/scanner/src/scanner_folder.rs index 3b5243e18..9ea5e3214 100644 --- a/crates/scanner/src/scanner_folder.rs +++ b/crates/scanner/src/scanner_folder.rs @@ -15,7 +15,7 @@ use std::collections::HashSet; use std::fs::FileType; use std::io::ErrorKind; -use std::sync::Arc; +use std::sync::{Arc, Once}; use std::time::{Duration, SystemTime}; use crate::ReplTargetSizeSummary; @@ -23,9 +23,12 @@ use crate::data_usage_define::{DataUsageCache, DataUsageEntry, DataUsageHash, Da use crate::error::ScannerError; use crate::scanner_io::ScannerIODisk as _; use crate::sleeper::DynamicSleeper; -use rustfs_common::heal_channel::{HEAL_DELETE_DANGLING, HealChannelRequest, HealOpts, HealScanMode, send_heal_request}; +use metrics::{counter, describe_counter}; +use rustfs_common::heal_channel::{ + HEAL_DELETE_DANGLING, HealAdmissionResult, HealChannelPriority, HealChannelRequest, HealScanMode, + send_heal_request_with_admission, +}; use rustfs_common::metrics::{IlmAction, Metric, Metrics, UpdateCurrentPathFn, current_path_updater}; -use rustfs_ecstore::StorageAPI; use rustfs_ecstore::bucket::lifecycle::bucket_lifecycle_audit::LcEventSrc; use rustfs_ecstore::bucket::lifecycle::bucket_lifecycle_ops::apply_expiry_rule; use rustfs_ecstore::bucket::lifecycle::evaluator::Evaluator; @@ -65,6 +68,10 @@ const ENV_FAILED_OBJECT_TTL_SECS: &str = "RUSTFS_DATA_USAGE_FAILED_OBJECT_TTL_SE const ENV_FAILED_OBJECTS_MAX: &str = "RUSTFS_DATA_USAGE_FAILED_OBJECTS_MAX"; const DEFAULT_FAILED_OBJECT_TTL_SECS: u32 = 86_400; const DEFAULT_FAILED_OBJECTS_MAX: u32 = 10_000; +const METRIC_SCANNER_INLINE_HEAL_TOTAL: &str = "rustfs_scanner_inline_heal_total"; + +static SCANNER_INLINE_HEAL_WARN_ONCE: Once = Once::new(); +static SCANNER_INLINE_HEAL_METRICS_ONCE: Once = Once::new(); pub fn data_usage_update_dir_cycles() -> u32 { rustfs_utils::get_env_u32(ENV_DATA_USAGE_UPDATE_DIR_CYCLES, DATA_USAGE_UPDATE_DIR_CYCLES) @@ -74,6 +81,40 @@ pub fn heal_object_select_prob() -> u32 { rustfs_utils::get_env_u32(ENV_HEAL_OBJECT_SELECT_PROB, DEFAULT_HEAL_OBJECT_SELECT_PROB) } +fn scanner_inline_heal_enabled() -> bool { + scanner_inline_heal_enabled_from_value(std::env::var(rustfs_config::ENV_SCANNER_INLINE_HEAL_ENABLE).ok().as_deref()) +} + +fn scanner_inline_heal_enabled_from_value(value: Option<&str>) -> bool { + match value { + Some(value) => matches!(value.trim().to_ascii_lowercase().as_str(), "1" | "true" | "on" | "yes"), + None => rustfs_config::DEFAULT_SCANNER_INLINE_HEAL_ENABLE, + } +} + +fn ensure_scanner_inline_heal_metric_registered() { + SCANNER_INLINE_HEAL_METRICS_ONCE.call_once(|| { + describe_counter!( + METRIC_SCANNER_INLINE_HEAL_TOTAL, + "Total number of inline heal operations executed directly by scanner." + ); + counter!(METRIC_SCANNER_INLINE_HEAL_TOTAL).increment(0); + }); +} + +fn warn_inline_heal_compat_requested() { + if !scanner_inline_heal_enabled() { + return; + } + + SCANNER_INLINE_HEAL_WARN_ONCE.call_once(|| { + warn!( + env = rustfs_config::ENV_SCANNER_INLINE_HEAL_ENABLE, + "Inline scanner heal rollback is no longer supported; continuing to enqueue heal candidates asynchronously" + ); + }); +} + /// Cached folder information for scanning #[derive(Clone, Debug)] pub struct CachedFolder { @@ -85,6 +126,154 @@ pub struct CachedFolder { /// Type alias for get size function pub type GetSizeFn = Box Result + Send + Sync>; +fn build_bucket_heal_request(bucket: String, priority: HealChannelPriority) -> HealChannelRequest { + HealChannelRequest { + bucket, + priority, + ..Default::default() + } +} + +fn build_object_heal_request( + bucket: String, + object: String, + version_id: Option, + scan_mode: HealScanMode, + priority: HealChannelPriority, +) -> HealChannelRequest { + HealChannelRequest { + bucket, + object_prefix: Some(object), + object_version_id: version_id, + priority, + scan_mode: Some(scan_mode), + remove_corrupted: Some(HEAL_DELETE_DANGLING), + ..Default::default() + } +} + +fn heal_priority_label(priority: HealChannelPriority) -> &'static str { + match priority { + HealChannelPriority::Low => "low", + HealChannelPriority::Normal => "normal", + HealChannelPriority::High => "high", + HealChannelPriority::Critical => "critical", + } +} + +fn describe_heal_admission(result: HealAdmissionResult) -> String { + match result { + HealAdmissionResult::Accepted | HealAdmissionResult::Merged => result.result_label().to_string(), + HealAdmissionResult::Full => "queue_full".to_string(), + HealAdmissionResult::Dropped(reason) => format!("dropped:{}", reason.as_str()), + } +} + +fn record_high_priority_heal_escalation( + candidate_type: &'static str, + priority: HealChannelPriority, + result: HealAdmissionResult, +) { + counter!( + "rustfs_heal_candidate_priority_reject_total", + "type" => candidate_type.to_string(), + "priority" => heal_priority_label(priority).to_string(), + "result" => result.result_label().to_string(), + "reason" => result.reason_label().to_string() + ) + .increment(1); +} + +fn build_high_priority_heal_admission_error( + candidate_type: &'static str, + bucket: &str, + object: Option<&str>, + priority: HealChannelPriority, + result: HealAdmissionResult, +) -> ScannerError { + let object_text = object.map(|object| format!(", object='{object}'")).unwrap_or_default(); + ScannerError::Other(format!( + "high-priority heal request was not admitted: type={candidate_type}, bucket='{bucket}'{object_text}, priority={}, admission={}", + heal_priority_label(priority), + describe_heal_admission(result) + )) +} + +fn record_heal_candidate_admission(candidate_type: &'static str, priority: HealChannelPriority, result: HealAdmissionResult) { + counter!( + "rustfs_heal_candidate_enqueue_total", + "type" => candidate_type.to_string(), + "priority" => heal_priority_label(priority).to_string(), + "result" => result.result_label().to_string() + ) + .increment(1); + + if matches!(result, HealAdmissionResult::Merged) { + counter!( + "rustfs_heal_candidate_merge_total", + "type" => candidate_type.to_string() + ) + .increment(1); + } + + if let HealAdmissionResult::Dropped(reason) = result { + counter!( + "rustfs_heal_candidate_drop_total", + "type" => candidate_type.to_string(), + "reason" => reason.as_str().to_string() + ) + .increment(1); + } +} + +async fn send_scanner_heal_request( + candidate_type: &'static str, + request: HealChannelRequest, +) -> Result { + let priority = request.priority; + match send_heal_request_with_admission(request).await { + Ok(result) => { + record_heal_candidate_admission(candidate_type, priority, result); + Ok(result) + } + Err(err) => { + counter!( + "rustfs_heal_candidate_enqueue_total", + "type" => candidate_type.to_string(), + "priority" => heal_priority_label(priority).to_string(), + "result" => "channel_error".to_string() + ) + .increment(1); + Err(ScannerError::Other(err)) + } + } +} + +async fn send_required_scanner_heal_request( + candidate_type: &'static str, + bucket: &str, + object: Option<&str>, + request: HealChannelRequest, +) -> Result<(), ScannerError> { + let priority = request.priority; + let result = send_scanner_heal_request(candidate_type, request).await?; + if result.is_admitted() { + return Ok(()); + } + + record_high_priority_heal_escalation(candidate_type, priority, result); + error!( + candidate_type, + bucket, + object = object.unwrap_or(""), + priority = heal_priority_label(priority), + admission = result.result_label(), + reason = result.reason_label(), + "High-priority heal request was not admitted; escalating to scanner failure" + ); + Err(build_high_priority_heal_admission_error(candidate_type, bucket, object, priority, result)) +} + /// Scanner item representing a file during scanning #[derive(Clone, Debug)] pub struct ScannerItem { @@ -127,9 +316,8 @@ impl ScannerItem { self.object_name = split.last().unwrap_or(&"").to_string(); } - pub async fn apply_actions( + pub async fn apply_actions( &mut self, - store: Arc, object_infos: Vec, lock_retention: Option>, size_summary: &mut SizeSummary, @@ -159,7 +347,7 @@ impl ScannerItem { } }; - let size = self.heal_actions(store.clone(), oi, actual_size, size_summary).await; + let size = self.heal_actions(oi, actual_size, size_summary).await; size_summary.actions_accounting(oi, size, actual_size); @@ -246,7 +434,7 @@ impl ScannerItem { } IlmAction::NoneAction | IlmAction::ActionCount => { - size = self.heal_actions(store.clone(), oi, actual_size, size_summary).await; + size = self.heal_actions(oi, actual_size, size_summary).await; } } @@ -262,22 +450,15 @@ impl ScannerItem { self.alert_excessive_versions(remaining_versions, cumulative_size); } - async fn heal_actions( - &mut self, - store: Arc, - oi: &ObjectInfo, - actual_size: i64, - size_summary: &mut SizeSummary, - ) -> i64 { - let mut size = actual_size; - + async fn heal_actions(&mut self, oi: &ObjectInfo, actual_size: i64, size_summary: &mut SizeSummary) -> i64 { if self.heal_enabled { - size = self.apply_heal(store, oi).await; + warn_inline_heal_compat_requested(); + self.enqueue_heal(oi).await; } self.heal_replication(oi, size_summary).await; - size + actual_size } async fn heal_replication(&mut self, oi: &ObjectInfo, size_summary: &mut SizeSummary) { @@ -332,10 +513,10 @@ impl ScannerItem { } } - async fn apply_heal(&mut self, store: Arc, oi: &ObjectInfo) -> i64 { + async fn enqueue_heal(&mut self, oi: &ObjectInfo) { let done_heal = Metrics::time(Metric::HealAbandonedObject); debug!( - "apply_heal: bucket: {}, object_path: {}, version_id: {}", + "enqueue_heal: bucket: {}, object_path: {}, version_id: {}", self.bucket, self.object_path(), oi.version_id.unwrap_or_default() @@ -347,36 +528,32 @@ impl ScannerItem { HealScanMode::Normal }; - let result = match store - .clone() - .heal_object( - self.bucket.as_str(), - self.object_path().as_str(), + let result = send_scanner_heal_request( + "object", + build_object_heal_request( + self.bucket.clone(), + self.object_path(), oi.version_id - .map(|v| if v.is_nil() { "".to_string() } else { v.to_string() }) - .unwrap_or_default() - .as_str(), - &HealOpts { - remove: HEAL_DELETE_DANGLING, - scan_mode, - ..Default::default() - }, - ) - .await - { - Ok((result, err)) => { - if let Some(err) = err { - warn!("apply_heal: failed to heal object: {}", err); - } - result.object_size as i64 + .and_then(|v| if v.is_nil() { None } else { Some(v.to_string()) }), + scan_mode, + HealChannelPriority::Low, + ), + ) + .await; + + match result { + Ok(HealAdmissionResult::Accepted | HealAdmissionResult::Merged) => {} + Ok(result @ (HealAdmissionResult::Full | HealAdmissionResult::Dropped(_))) => { + warn!( + bucket = %self.bucket, + object = %self.object_path(), + admission = %describe_heal_admission(result), + "enqueue_heal: low-priority heal request was not admitted" + ); } - Err(e) => { - warn!("apply_heal: failed to heal object: {}", e); - 0 - } - }; + Err(e) => warn!("enqueue_heal: failed to submit heal request: {}", e), + } done_heal(); - result } fn alert_excessive_versions(&self, _object_infos_length: usize, _cumulative_size: i64) { @@ -964,12 +1141,13 @@ impl FolderScanner { let (bucket, prefix) = path2_bucket_object(name.as_str()); if bucket != resolver.bucket { - send_heal_request(HealChannelRequest { - bucket: bucket.clone(), - ..Default::default() - }) - .await - .map_err(ScannerError::Other)?; + send_required_scanner_heal_request( + "bucket", + &bucket, + None, + build_bucket_heal_request(bucket.clone(), HealChannelPriority::High), + ) + .await?; } resolver.bucket = bucket.clone(); @@ -1075,36 +1253,40 @@ impl FolderScanner { Ok(fivs) => fivs, Err(e) => { error!("scan_folder: list_path_raw: failed to get file info versions: {}", e); - if let Err(e) = send_heal_request(HealChannelRequest { - bucket: bucket.clone(), - object_prefix: Some(entry.name.clone()), - ..Default::default() - }).await { - error!("scan_folder: list_path_raw: failed to send heal request: {}", e); - continue; - } - - + send_required_scanner_heal_request( + "object", + &bucket, + Some(&entry.name), + build_object_heal_request( + bucket.clone(), + entry.name.clone(), + None, + self.scan_mode, + HealChannelPriority::High, + ), + ) + .await?; found_objects = true; - continue; } }; for fiv in fivs.versions { - if let Err(e) = send_heal_request(HealChannelRequest { - bucket: bucket.clone(), - object_prefix: Some(entry.name.clone()), - object_version_id: fiv.version_id.map(|v| v.to_string()), - ..Default::default() - }).await { - error!("scan_folder: list_path_raw: failed to send heal request: {}", e); - continue; - } - + send_required_scanner_heal_request( + "object", + &bucket, + Some(&entry.name), + build_object_heal_request( + bucket.clone(), + entry.name.clone(), + fiv.version_id.and_then(|v| if v.is_nil() { None } else { Some(v.to_string()) }), + self.scan_mode, + HealChannelPriority::High, + ), + ) + .await?; found_objects = true; - } @@ -1233,6 +1415,8 @@ pub async fn scan_data_folder( ) -> Result { use crate::data_usage_define::DATA_USAGE_ROOT; + ensure_scanner_inline_heal_metric_registered(); + // Check that we're not trying to scan the root if cache.info.name.is_empty() || cache.info.name == DATA_USAGE_ROOT { return Err(ScannerError::Other("internal error: root scan attempted".to_string())); @@ -1518,6 +1702,104 @@ mod tests { assert!(!scanner.new_cache.info.failed_objects.contains_key("expired")); } + #[test] + fn test_scanner_inline_heal_enabled_defaults_to_false() { + assert!(!scanner_inline_heal_enabled_from_value(None)); + } + + #[test] + fn test_scanner_inline_heal_enabled_reads_env_override() { + assert!(scanner_inline_heal_enabled_from_value(Some("true"))); + assert!(scanner_inline_heal_enabled_from_value(Some("YES"))); + assert!(scanner_inline_heal_enabled_from_value(Some("1"))); + assert!(!scanner_inline_heal_enabled_from_value(Some("false"))); + } + + #[test] + fn test_build_object_heal_request_omits_nil_version_id() { + let request = build_object_heal_request( + "bucket".to_string(), + "path/to/object".to_string(), + None, + HealScanMode::Deep, + HealChannelPriority::Low, + ); + + assert_eq!(request.bucket, "bucket"); + assert_eq!(request.object_prefix.as_deref(), Some("path/to/object")); + assert!(request.object_version_id.is_none()); + assert_eq!(request.scan_mode, Some(HealScanMode::Deep)); + assert_eq!(request.priority, HealChannelPriority::Low); + assert_eq!(request.remove_corrupted, Some(HEAL_DELETE_DANGLING)); + } + + #[test] + fn test_heal_priority_label_matches_priority_names() { + assert_eq!(heal_priority_label(HealChannelPriority::Low), "low"); + assert_eq!(heal_priority_label(HealChannelPriority::Normal), "normal"); + assert_eq!(heal_priority_label(HealChannelPriority::High), "high"); + assert_eq!(heal_priority_label(HealChannelPriority::Critical), "critical"); + } + + #[test] + fn test_describe_heal_admission_formats_unadmitted_results() { + assert_eq!(describe_heal_admission(HealAdmissionResult::Accepted), "accepted"); + assert_eq!(describe_heal_admission(HealAdmissionResult::Merged), "merged"); + assert_eq!(describe_heal_admission(HealAdmissionResult::Full), "queue_full"); + assert_eq!( + describe_heal_admission(HealAdmissionResult::Dropped( + rustfs_common::heal_channel::HealAdmissionDropReason::QueueFull + )), + "dropped:queue_full" + ); + } + + #[test] + fn test_build_high_priority_heal_admission_error_contains_context() { + let err = build_high_priority_heal_admission_error( + "object", + "bucket-a", + Some("path/to/object"), + HealChannelPriority::High, + HealAdmissionResult::Full, + ); + + let err_text = err.to_string(); + assert!(err_text.contains("type=object")); + assert!(err_text.contains("bucket='bucket-a'")); + assert!(err_text.contains("object='path/to/object'")); + assert!(err_text.contains("priority=high")); + assert!(err_text.contains("admission=queue_full")); + } + + #[tokio::test] + async fn test_heal_actions_returns_actual_size_without_inline_heal() { + let temp_dir = std::env::temp_dir(); + let file_type = std::fs::metadata(&temp_dir).unwrap().file_type(); + + let mut item = ScannerItem { + path: temp_dir.join("object").to_string_lossy().to_string(), + bucket: "bucket".to_string(), + prefix: "".to_string(), + object_name: "object".to_string(), + file_type, + lifecycle: None, + replication: None, + heal_enabled: true, + heal_bitrot: true, + debug: false, + }; + let object_info = ObjectInfo { + bucket: "bucket".to_string(), + name: "object".to_string(), + ..Default::default() + }; + let mut size_summary = SizeSummary::default(); + + let size = item.heal_actions(&object_info, 123, &mut size_summary).await; + assert_eq!(size, 123); + } + #[tokio::test] #[serial] #[cfg(unix)] diff --git a/crates/scanner/src/scanner_io.rs b/crates/scanner/src/scanner_io.rs index 5e76fd1b1..5b48ab393 100644 --- a/crates/scanner/src/scanner_io.rs +++ b/crates/scanner/src/scanner_io.rs @@ -19,6 +19,7 @@ use crate::{ DataUsageInfo, SizeSummary, TierStats, }; use futures::future::join_all; +use metrics::counter; use rand::seq::SliceRandom as _; use rustfs_common::heal_channel::HealScanMode; use rustfs_common::metrics::{Metric, Metrics, emit_scan_bucket_drive_complete}; @@ -50,6 +51,24 @@ use tokio::time::Duration; use tokio_util::sync::CancellationToken; use tracing::{debug, error, warn}; +fn record_set_scan_failure(first_err: &mut Option, err: Error) { + if first_err.is_none() { + *first_err = Some(err); + } +} + +fn finalize_nsscanner_result(results: &[DataUsageCache], first_err: Option) -> Result<()> { + if results.iter().any(|result| result.info.last_update.is_some()) { + return Ok(()); + } + + if let Some(err) = first_err { + return Err(err); + } + + Ok(()) +} + #[async_trait::async_trait] pub trait ScannerIO: Send + Sync + Debug + 'static { async fn nsscanner( @@ -125,6 +144,8 @@ impl ScannerIO for ECStore { let results_index_clone = results_index as usize; // Clone the Arc to move it into the spawned task let set_clone: Arc = Arc::clone(set); + let pool_label = set.pool_index.to_string(); + let set_label = set.set_index.to_string(); let child_token_clone = child_token.clone(); let want_cycle_clone = want_cycle; @@ -150,9 +171,21 @@ impl ScannerIO for ECStore { .nsscanner_cache(child_token_clone.clone(), all_buckets_clone, tx, want_cycle_clone, scan_mode_clone) .await { - error!("Failed to scan set: {e}"); - let _ = first_err_mutex_clone.lock().await.insert(e); - child_token_clone.cancel(); + counter!( + "rustfs_scanner_set_failure_total", + "pool" => pool_label.clone(), + "set" => set_label.clone(), + "stage" => "nsscanner_cache".to_string() + ) + .increment(1); + error!( + pool = %pool_label, + set = %set_label, + error = %e, + "Failed to scan set; continuing scanner cycle" + ); + let mut first_err = first_err_mutex_clone.lock().await; + record_set_scan_failure(&mut first_err, e); } }); wait_futs.push(scanner_fut); @@ -162,6 +195,7 @@ impl ScannerIO for ECStore { let (update_tx, mut update_rx) = tokio::sync::oneshot::channel::<()>(); let all_buckets_clone = all_buckets.iter().map(|b| b.name.clone()).collect::>(); + let results_mutex_for_updates = results_mutex.clone(); tokio::spawn(async move { let mut last_update = SystemTime::UNIX_EPOCH; let mut has_sent_once = false; @@ -177,7 +211,7 @@ impl ScannerIO for ECStore { break; } - let results = results_mutex.lock().await; + let results = results_mutex_for_updates.lock().await; let mut all_merged = DataUsageCache::default(); for result in results.iter() { if result.info.last_update.is_none() { @@ -196,7 +230,7 @@ impl ScannerIO for ECStore { break; } _ = ticker.tick() => { - let results = results_mutex.lock().await; + let results = results_mutex_for_updates.lock().await; let mut all_merged = DataUsageCache::default(); for result in results.iter() { if result.info.last_update.is_none() { @@ -223,7 +257,9 @@ impl ScannerIO for ECStore { let _ = update_tx.send(()); - Ok(()) + let first_err = first_err_mutex.lock().await.take(); + let results = results_mutex.lock().await.clone(); + finalize_nsscanner_result(&results, first_err) } } @@ -561,13 +597,7 @@ impl ScannerIODisk for Disk { Err(_) => None, }; - let Some(ecstore) = new_object_layer_fn() else { - error!("ECStore not available"); - return Err(StorageError::other("ECStore not available".to_string())); - }; - - item.apply_actions(ecstore, object_infos, lock_config, &mut size_summary) - .await; + item.apply_actions(object_infos, lock_config, &mut size_summary).await; if !free_version_infos.is_empty() { let mut expiry_state = GLOBAL_ExpiryState.write().await; @@ -668,3 +698,36 @@ impl ScannerIODisk for Disk { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn record_set_scan_failure_preserves_first_error() { + let mut first = None; + record_set_scan_failure(&mut first, Error::other("first")); + record_set_scan_failure(&mut first, Error::other("second")); + + let first = first.expect("first error should be recorded"); + assert!(first.to_string().contains("first")); + } + + #[test] + fn finalize_nsscanner_result_returns_ok_when_any_set_succeeds() { + let mut results = vec![DataUsageCache::default(), DataUsageCache::default()]; + results[1].info.last_update = Some(SystemTime::now()); + + let result = finalize_nsscanner_result(&results, Some(Error::other("set failed"))); + assert!(result.is_ok()); + } + + #[test] + fn finalize_nsscanner_result_returns_first_error_when_all_sets_fail() { + let results = vec![DataUsageCache::default(), DataUsageCache::default()]; + + let err = finalize_nsscanner_result(&results, Some(Error::other("set failed"))) + .expect_err("all failed sets should bubble first error"); + assert!(err.to_string().contains("set failed")); + } +}