diff --git a/crates/ecstore/src/disk/disk_store.rs b/crates/ecstore/src/disk/disk_store.rs index 73a4da288..c4650eb04 100644 --- a/crates/ecstore/src/disk/disk_store.rs +++ b/crates/ecstore/src/disk/disk_store.rs @@ -44,6 +44,12 @@ use uuid::Uuid; const DISK_HEALTH_OK: u32 = 0; const DISK_HEALTH_FAULTY: u32 = 1; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum TimeoutHealthAction { + MarkFailure, + IgnoreFailure, +} + pub const ENV_RUSTFS_DRIVE_ACTIVE_MONITORING: &str = "RUSTFS_DRIVE_ACTIVE_MONITORING"; pub const DEFAULT_RUSTFS_DRIVE_ACTIVE_MONITORING: bool = true; pub const SKIP_IF_SUCCESS_BEFORE: Duration = Duration::from_secs(5); @@ -804,6 +810,21 @@ impl LocalDiskWrapper { operation: F, timeout_duration: Duration, ) -> Result + where + F: FnOnce() -> Fut, + Fut: std::future::Future>, + { + self.track_disk_health_with_op_and_timeout_action(op, operation, timeout_duration, TimeoutHealthAction::MarkFailure) + .await + } + + async fn track_disk_health_with_op_and_timeout_action( + &self, + op: &'static str, + operation: F, + timeout_duration: Duration, + timeout_health_action: TimeoutHealthAction, + ) -> Result where F: FnOnce() -> Fut, Fut: std::future::Future>, @@ -848,7 +869,9 @@ impl LocalDiskWrapper { Err(_) => { // Timeout occurred, mark disk as potentially faulty and decrement waiting counter self.health.decrement_waiting(); - if self.health.mark_failure(&self.endpoint(), "operation_timeout") { + if timeout_health_action == TimeoutHealthAction::MarkFailure + && self.health.mark_failure(&self.endpoint(), "operation_timeout") + { self.spawn_recovery_monitor_if_needed(); } counter!( @@ -996,8 +1019,13 @@ impl DiskAPI for LocalDiskWrapper { } async fn walk_dir(&self, opts: WalkDirOptions, wr: &mut W) -> Result<()> { - self.track_disk_health_with_op("walk_dir", || async { self.disk.walk_dir(opts, wr).await }, get_drive_walkdir_timeout()) - .await + self.track_disk_health_with_op_and_timeout_action( + "walk_dir", + || async { self.disk.walk_dir(opts, wr).await }, + get_drive_walkdir_timeout(), + TimeoutHealthAction::IgnoreFailure, + ) + .await } async fn delete_version( @@ -1203,6 +1231,28 @@ mod tests { use super::*; use crate::disk::endpoint::Endpoint; use crate::disk::health_state::RuntimeDriveHealthState; + use std::{ + io, + pin::Pin, + task::{Context, Poll}, + }; + use tokio::io::AsyncWrite; + + struct PendingWriter; + + impl AsyncWrite for PendingWriter { + fn poll_write(self: Pin<&mut Self>, _cx: &mut Context<'_>, _buf: &[u8]) -> Poll> { + Poll::Pending + } + + fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + } #[test] fn drive_metadata_timeout_uses_default_when_unset() { @@ -1323,6 +1373,97 @@ mod tests { assert!(health.offline_duration().is_none()); } + #[tokio::test] + async fn ignored_timeout_does_not_mark_drive_failure() { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let endpoint = + Endpoint::try_from(dir.path().to_str().expect("temp dir should be valid UTF-8")).expect("endpoint should parse"); + let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created")); + let wrapper = LocalDiskWrapper::new(disk, false); + + let result = wrapper + .track_disk_health_with_op_and_timeout_action( + "walk_dir", + || async { + tokio::time::sleep(Duration::from_millis(20)).await; + Ok(()) + }, + Duration::from_millis(1), + TimeoutHealthAction::IgnoreFailure, + ) + .await; + + assert_eq!(result.expect_err("operation should time out"), DiskError::Timeout); + assert_eq!(wrapper.runtime_state(), RuntimeDriveHealthState::Online); + assert!(!wrapper.health.is_faulty()); + } + + #[tokio::test] + async fn walk_dir_writer_backpressure_timeout_does_not_mark_drive_failure() { + temp_env::async_with_vars([(rustfs_config::ENV_DRIVE_WALKDIR_TIMEOUT_SECS, Some("1"))], async { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let endpoint = + Endpoint::try_from(dir.path().to_str().expect("temp dir should be valid UTF-8")).expect("endpoint should parse"); + let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created")); + let wrapper = LocalDiskWrapper::new(disk, false); + let bucket = "test-bucket"; + let object = "test-object"; + + wrapper.make_volume(bucket).await.expect("bucket should be created"); + + let mut file_info = FileInfo::new(&format!("{bucket}/{object}"), 1, 0); + file_info.volume = bucket.to_string(); + file_info.name = object.to_string(); + file_info.mod_time = Some(::time::OffsetDateTime::now_utc()); + file_info.erasure.index = 1; + + wrapper + .write_metadata("", bucket, object, file_info) + .await + .expect("object metadata should be written"); + + let mut writer = PendingWriter; + let result = wrapper + .walk_dir( + WalkDirOptions { + bucket: bucket.to_string(), + recursive: true, + ..Default::default() + }, + &mut writer, + ) + .await; + + assert_eq!(result.expect_err("walk_dir should time out"), DiskError::Timeout); + assert_eq!(wrapper.runtime_state(), RuntimeDriveHealthState::Online); + assert!(!wrapper.health.is_faulty()); + }) + .await; + } + + #[tokio::test] + async fn default_timeout_marks_drive_failure() { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let endpoint = + Endpoint::try_from(dir.path().to_str().expect("temp dir should be valid UTF-8")).expect("endpoint should parse"); + let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created")); + let wrapper = LocalDiskWrapper::new(disk, false); + + let result = wrapper + .track_disk_health_with_op( + "read_metadata", + || async { + tokio::time::sleep(Duration::from_millis(20)).await; + Ok(()) + }, + Duration::from_millis(1), + ) + .await; + + assert_eq!(result.expect_err("operation should time out"), DiskError::Timeout); + assert_eq!(wrapper.runtime_state(), RuntimeDriveHealthState::Suspect); + } + #[test] fn reset_for_store_init_retry_clears_faulty_and_back_online() { let endpoint = Endpoint::try_from("/tmp/reset-store-init-retry").expect("endpoint should parse"); diff --git a/crates/ecstore/src/rpc/remote_disk.rs b/crates/ecstore/src/rpc/remote_disk.rs index 6859999ee..3591e6353 100644 --- a/crates/ecstore/src/rpc/remote_disk.rs +++ b/crates/ecstore/src/rpc/remote_disk.rs @@ -68,6 +68,12 @@ use tonic::{Request, service::interceptor::InterceptedService, transport::Channe use tracing::{debug, info, warn}; use uuid::Uuid; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum FailureHealthAction { + MarkFailure, + IgnoreFailure, +} + async fn copy_stream_with_buffer(reader: &mut R, writer: &mut W, buffer_size: usize) -> io::Result where R: AsyncRead + Unpin, @@ -320,6 +326,21 @@ impl RemoteDisk { operation: F, timeout_duration: Duration, ) -> Result + where + F: FnOnce() -> Fut, + Fut: std::future::Future>, + { + self.execute_with_timeout_for_op_and_health_action(op, operation, timeout_duration, FailureHealthAction::MarkFailure) + .await + } + + async fn execute_with_timeout_for_op_and_health_action( + &self, + op: &'static str, + operation: F, + timeout_duration: Duration, + failure_health_action: FailureHealthAction, + ) -> Result where F: FnOnce() -> Fut, Fut: std::future::Future>, @@ -338,6 +359,17 @@ impl RemoteDisk { self.health.last_started.store(now, std::sync::atomic::Ordering::Relaxed); self.health.increment_waiting(); + if timeout_duration == Duration::ZERO { + let operation_result = operation().await; + if operation_result.is_ok() { + self.health.log_success(); + } + self.health.decrement_waiting(); + self.handle_network_like_error(op, timeout_duration, &operation_result, failure_health_action) + .await; + return operation_result; + } + // Execute operation with timeout let result = time::timeout(timeout_duration, operation()).await; @@ -348,24 +380,8 @@ impl RemoteDisk { self.health.log_success(); } self.health.decrement_waiting(); - if let Err(err) = &operation_result - && is_network_like_disk_error(err) - { - counter!( - "rustfs_drive_op_network_error_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 network-like error" - ); - self.mark_faulty_and_evict("operation_network_error").await; - } + self.handle_network_like_error(op, timeout_duration, &operation_result, failure_health_action) + .await; operation_result } Err(_) => { @@ -377,7 +393,9 @@ impl RemoteDisk { "op" => op.to_string() ) .increment(1); - self.mark_faulty_and_evict("operation_timeout").await; + if failure_health_action == FailureHealthAction::MarkFailure { + self.mark_faulty_and_evict("operation_timeout").await; + } warn!( endpoint = %self.endpoint, addr = %self.addr, @@ -390,6 +408,35 @@ impl RemoteDisk { } } + async fn handle_network_like_error( + &self, + op: &'static str, + timeout_duration: Duration, + operation_result: &Result, + failure_health_action: FailureHealthAction, + ) { + if let Err(err) = operation_result + && is_network_like_disk_error(err) + { + counter!( + "rustfs_drive_op_network_error_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 network-like error" + ); + if failure_health_action == FailureHealthAction::MarkFailure { + self.mark_faulty_and_evict("operation_network_error").await; + } + } + } + async fn mark_faulty_and_evict(&self, reason: &'static str) { if self.health.mark_offline(&self.endpoint, reason) { self.spawn_recovery_monitor_if_needed(); @@ -1146,7 +1193,8 @@ impl DiskAPI for RemoteDisk { async fn walk_dir(&self, opts: WalkDirOptions, wr: &mut W) -> Result<()> { info!("walk_dir {}", self.endpoint.to_string()); - self.execute_with_timeout( + self.execute_with_timeout_for_op_and_health_action( + "walk_dir", || async { let disk = self.disk_ref().await; @@ -1172,6 +1220,7 @@ impl DiskAPI for RemoteDisk { Ok(()) }, get_drive_walkdir_timeout(), + FailureHealthAction::IgnoreFailure, ) .await } @@ -1994,6 +2043,82 @@ mod tests { assert!(!remote_disk.is_online().await, "remote disk should be marked faulty after timeout"); } + #[tokio::test] + async fn test_execute_with_timeout_can_ignore_remote_timeout_failure() { + let url = url::Url::parse("http://remote-timeout-ignored: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_for_op_and_health_action( + "walk_dir", + || async { + tokio::time::sleep(Duration::from_millis(50)).await; + Ok::<(), Error>(()) + }, + Duration::from_millis(10), + FailureHealthAction::IgnoreFailure, + ) + .await + .expect_err("timeout should fail"); + + assert!(err.to_string().contains("timeout")); + assert!(remote_disk.is_online().await, "ignored timeout should not mark remote disk faulty"); + } + + #[tokio::test] + async fn test_execute_with_timeout_zero_duration_waits_for_operation() { + let url = url::Url::parse("http://remote-no-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(); + + remote_disk + .execute_with_timeout( + || async { + tokio::time::sleep(Duration::from_millis(10)).await; + Ok::<(), Error>(()) + }, + Duration::ZERO, + ) + .await + .expect("zero duration should disable the operation timeout"); + + assert!( + remote_disk.is_online().await, + "successful no-timeout operation should keep remote disk online" + ); + } + #[tokio::test] async fn test_execute_with_timeout_evicts_cached_connection() { let addr = "http://127.0.0.1:59991".to_string(); @@ -2136,6 +2261,58 @@ mod tests { ); } + #[tokio::test] + async fn test_execute_with_timeout_can_ignore_network_like_error() { + let addr = "http://127.0.0.1:59995".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_for_op_and_health_action( + "walk_dir", + || async { Err::<(), Error>(DiskError::Io(std::io::Error::new(std::io::ErrorKind::TimedOut, "stall timeout"))) }, + Duration::from_secs(1), + FailureHealthAction::IgnoreFailure, + ) + .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, + "ignored network-like error should not mark remote disk faulty" + ); + assert!( + GLOBAL_CONN_MAP.read().await.contains_key(&addr), + "ignored network-like error should not evict cached connection" + ); + } + #[tokio::test] async fn test_execute_with_timeout_keeps_remote_disk_online_for_business_error() { let addr = "http://127.0.0.1:59994".to_string();