diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d12a8b68..78848a027 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -854,6 +854,12 @@ jobs: run: | sudo apt-get install -y iptables sudo -n iptables --version + # The endpoint-blackhole heal scenario needs CAP_NET_ADMIN. Containerised + # runners can run iptables but not touch the rule set; the test then logs + # a skip instead of failing, so surface that here where it is visible. + if ! sudo -n iptables -w 5 -S OUTPUT >/dev/null 2>&1; then + echo "::warning::iptables cannot read the OUTPUT chain on this runner (no CAP_NET_ADMIN); the endpoint-blackhole heal scenario will be skipped" + fi - name: Set up Python uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 diff --git a/crates/e2e_test/README.md b/crates/e2e_test/README.md index 9c6d90c0a..9ba6ae346 100644 --- a/crates/e2e_test/README.md +++ b/crates/e2e_test/README.md @@ -184,6 +184,8 @@ the wiring source of truth. Committed test-ID digests under ## Troubleshooting +**Endpoint blackhole scenario skipped** — `heal_erasure_disk_rebuild_test::tests::test_cluster_root_heal_recovers_after_target_endpoint_blackhole` installs a loopback `iptables` DROP rule and therefore needs `CAP_NET_ADMIN` (root or passwordless `sudo -n iptables`). A host where `iptables` is missing or cannot read the OUTPUT chain (typical inside an unprivileged container, where the nf_tables backend reports "Permission denied" even under `sudo`) logs a `heal_interruption_skipped` warning and returns without exercising heal. Set `RUSTFS_E2E_REQUIRE_NET_FAULT_INJECTION=1` on lanes that do provision the capability so a broken runner fails instead of skipping. + **Reproduce a CI failure locally** — run the exact profile/lane: ```bash diff --git a/crates/e2e_test/src/heal_erasure_disk_rebuild_test.rs b/crates/e2e_test/src/heal_erasure_disk_rebuild_test.rs index 68b0006b1..a3ec8f159 100644 --- a/crates/e2e_test/src/heal_erasure_disk_rebuild_test.rs +++ b/crates/e2e_test/src/heal_erasure_disk_rebuild_test.rs @@ -34,6 +34,8 @@ mod tests { use tokio::net::TcpStream; use tokio::time::{Duration, Instant, sleep, timeout}; use tracing::info; + #[cfg(target_os = "linux")] + use tracing::warn; const POOL_METADATA_OBJECT: &str = "pool.bin"; @@ -115,6 +117,49 @@ mod tests { } impl TcpPortBlackhole { + /// Environment flag that turns an unusable fault-injection host into a + /// hard failure instead of a logged skip. Lanes that provision + /// `CAP_NET_ADMIN` set it so a broken runner cannot pass silently. + #[cfg(target_os = "linux")] + const REQUIRE_ENV: &str = "RUSTFS_E2E_REQUIRE_NET_FAULT_INJECTION"; + + /// Probe whether this host can manipulate the OUTPUT chain at all. + /// + /// Returns `Ok(Some(reason))` when `iptables` is missing or lacks + /// `CAP_NET_ADMIN` (the nf_tables backend reports "Permission denied" + /// even under `sudo` inside an unprivileged container) and the lane did + /// not demand fault injection; returns an error when the lane demands + /// it; returns `Ok(None)` when the blackhole can be installed. + #[cfg(target_os = "linux")] + fn unavailable_reason() -> Result, Box> { + let id = Command::new("id").arg("-u").output()?; + if !id.status.success() { + return Err(format!("failed to determine the test process uid: {}", String::from_utf8_lossy(&id.stderr)).into()); + } + let use_sudo = String::from_utf8_lossy(&id.stdout).trim() != "0"; + let mut command = if use_sudo { + let mut command = Command::new("sudo"); + command.args(["-n", "iptables"]); + command + } else { + Command::new("iptables") + }; + let probe = command.args(["-w", "5", "-S", "OUTPUT"]).output(); + let reason = match probe { + Ok(output) if output.status.success() => return Ok(None), + Ok(output) => format!( + "iptables cannot read the OUTPUT chain (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ), + Err(err) => format!("iptables is not runnable: {err}"), + }; + if std::env::var_os(Self::REQUIRE_ENV).is_some() { + return Err(format!("{} is set but network fault injection is unavailable: {reason}", Self::REQUIRE_ENV).into()); + } + Ok(Some(reason)) + } + fn install(address: &str) -> Result> { let address = address.parse::()?; if !address.ip().is_loopback() { @@ -199,6 +244,27 @@ mod tests { } } + /// Remove a disk directory underneath a running server. Background writers + /// (scanner, usage cache, heal markers) can recreate entries between the + /// recursive listing and the final `rmdir`, which surfaces as + /// `DirectoryNotEmpty` on macOS; retry briefly so the wipe reflects the + /// operator action rather than a listing race. + fn wipe_directory_while_server_runs(disk: &Path) -> std::io::Result<()> { + let mut last_err = None; + for _ in 0..20 { + match std::fs::remove_dir_all(disk) { + Ok(()) => return Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::DirectoryNotEmpty => { + last_err = Some(err); + std::thread::sleep(std::time::Duration::from_millis(100)); + } + Err(err) => return Err(err), + } + } + Err(last_err.expect("retry loop only exits without success after recording an error")) + } + fn has_file_under(path: &Path) -> bool { let Ok(entries) = std::fs::read_dir(path) else { return false; @@ -481,7 +547,7 @@ mod tests { ); } - std::fs::remove_dir_all(&disk0).expect("disk0 wipe should succeed while server is running"); + wipe_directory_while_server_runs(&disk0).expect("disk0 wipe should succeed while server is running"); std::fs::create_dir_all(&disk0).expect("disk0 should be recreated empty while server is running"); assert!(!has_file_under(&disk0), "disk0 must be empty immediately after runtime wipe"); @@ -868,6 +934,18 @@ mod tests { #[cfg(target_os = "linux")] #[tokio::test(flavor = "multi_thread")] async fn test_cluster_root_heal_recovers_after_target_endpoint_blackhole() -> Result<(), Box> { + if let Some(reason) = TcpPortBlackhole::unavailable_reason()? { + init_logging(); + warn!( + event = "heal_interruption_skipped", + component = "e2e_test", + subsystem = "heal", + interruption_kind = "target_endpoint_blackhole", + reason, + "Skipping endpoint blackhole scenario: network fault injection is unavailable on this host" + ); + return Ok(()); + } timeout( Duration::from_secs(420), run_cluster_root_heal_interruption(InterruptionScenario::TargetEndpointBlackhole), diff --git a/crates/heal/src/heal/manager/auto_scan.rs b/crates/heal/src/heal/manager/auto_scan.rs index d457d3ee9..82097bf02 100644 --- a/crates/heal/src/heal/manager/auto_scan.rs +++ b/crates/heal/src/heal/manager/auto_scan.rs @@ -150,6 +150,7 @@ impl HealManager { Err(DiskError::UnformattedDisk) => { if !super::super::replacement_readiness::auto_replacement_target_ready(disk, &local_disks) .await + && !super::super::replacement_readiness::directory_backed_replacement_fallback_enabled() { deferred_replacement_endpoints.insert(endpoint.to_string()); skipped_invalid_count += 1; diff --git a/crates/heal/src/heal/replacement_readiness.rs b/crates/heal/src/heal/replacement_readiness.rs index 6cd9d1cc3..4ddc94340 100644 --- a/crates/heal/src/heal/replacement_readiness.rs +++ b/crates/heal/src/heal/replacement_readiness.rs @@ -18,6 +18,25 @@ use super::{ DiskOption, DiskStore, Endpoint, HealDiskExt as _, local_disk_map_read, new_disk, resume::ReplacementTargetIdentity, }; +/// Whether automatic replacement may fall back to the set-wide format heal when +/// a target cannot pass the independent-mount admission. +/// +/// Directory-backed deployments already declare, through +/// `RUSTFS_UNSAFE_BYPASS_DISK_CHECK`, that their endpoints are plain +/// directories sharing a device with the host root. Those endpoints can never +/// satisfy [`auto_replacement_target_identity`], so without this fallback a +/// runtime-wiped or replaced directory disk would stay deferred forever. The +/// admission check itself is never bypassed; the fallback only routes the heal +/// through the ordinary format path that formats every unformatted disk in the +/// set, which is exactly what the pre-admission `heal_disk` path did. +pub(crate) fn directory_backed_replacement_fallback_enabled() -> bool { + rustfs_utils::get_env_bool_with_aliases( + rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, + &[rustfs_config::ENV_MINIO_CI], + rustfs_config::DEFAULT_UNSAFE_BYPASS_DISK_CHECK, + ) +} + pub(crate) async fn auto_replacement_target_ready(disk: &DiskStore, local_disks: &[DiskStore]) -> bool { auto_replacement_target_identity(disk, local_disks).await.is_some() } @@ -184,12 +203,47 @@ mod tests { assert!(endpoint.is_local); } + #[test] + fn directory_backed_fallback_is_off_by_default() { + temp_env::with_vars( + [ + (rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, None::<&str>), + (rustfs_config::ENV_MINIO_CI, None::<&str>), + ], + || assert!(!directory_backed_replacement_fallback_enabled()), + ); + } + + #[test] + fn directory_backed_fallback_follows_the_disk_check_bypass() { + temp_env::with_vars( + [ + (rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, Some("true")), + (rustfs_config::ENV_MINIO_CI, None::<&str>), + ], + || assert!(directory_backed_replacement_fallback_enabled()), + ); + temp_env::with_vars( + [ + (rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, Some("false")), + (rustfs_config::ENV_MINIO_CI, Some("true")), + ], + || { + assert!( + !directory_backed_replacement_fallback_enabled(), + "the canonical key must win over the alias" + ) + }, + ); + } + #[tokio::test] async fn runtime_environment_cannot_bypass_mount_admission() { temp_env::async_with_vars( [ ("RUSTFS_TEST_AUTO_REPLACEMENT_READINESS_BYPASS", Some("1")), ("RUSTFS_E2E_AUTO_REPLACEMENT_READINESS_BYPASS", Some("1")), + (rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, Some("true")), ], async { let temp = TempDir::new().expect("temporary replacement root should be created"); diff --git a/crates/heal/src/heal/task/heal_erasure_set.rs b/crates/heal/src/heal/task/heal_erasure_set.rs index 446cb07d7..76b65a41a 100644 --- a/crates/heal/src/heal/task/heal_erasure_set.rs +++ b/crates/heal/src/heal/task/heal_erasure_set.rs @@ -42,7 +42,36 @@ impl HealTask { progress.update_stage(0, 4); } - let is_auto_replacement = matches!(self.source, HealRequestSource::AutoHeal) && !self.heal_endpoints.is_empty(); + let mut is_auto_replacement = matches!(self.source, HealRequestSource::AutoHeal) && !self.heal_endpoints.is_empty(); + if is_auto_replacement + && crate::heal::replacement_readiness::directory_backed_replacement_fallback_enabled() + && self + .await_with_control(self.storage.replacement_target_identities(&self.heal_endpoints)) + .await + .is_err() + { + // Directory-backed endpoints cannot pass replacement admission; the + // operator opted out of disk checks, so heal the set the way the + // pre-admission `heal_disk` path did instead of deferring forever. + warn!( + target: "rustfs::heal::task", + event = EVENT_HEAL_ERASURE_SET_STAGE, + component = LOG_COMPONENT_HEAL, + subsystem = LOG_SUBSYSTEM_TASK, + task_id = %self.id, + set_disk_id, + stage = "replacement_admission", + result = "directory_backed_fallback", + target_count = self.heal_endpoints.len(), + "Heal erasure set falls back to set-wide format heal for a replacement target that is not an independently mounted disk" + ); + is_auto_replacement = false; + } + let replacement_targets = if is_auto_replacement { + self.heal_endpoints.clone() + } else { + Vec::new() + }; let replacement_resume_disk = if is_auto_replacement { let mut requested_targets = self.heal_endpoints.clone(); requested_targets.sort_unstable(); @@ -421,7 +450,7 @@ impl HealTask { heal_opts, self.source, ) - .with_replacement_targets(self.heal_endpoints.clone(), is_auto_replacement.then(|| self.id.clone())) + .with_replacement_targets(replacement_targets, is_auto_replacement.then(|| self.id.clone())) .with_replacement_identity_fence(replacement_target_identities.clone()) .with_mainline_pacer(self.mainline_pacer.clone()); diff --git a/crates/heal/src/heal/task/tests.rs b/crates/heal/src/heal/task/tests.rs index 788b356ac..23118c92b 100644 --- a/crates/heal/src/heal/task/tests.rs +++ b/crates/heal/src/heal/task/tests.rs @@ -480,6 +480,95 @@ async fn automatic_replacement_uses_target_scoped_format() { ); } +fn directory_backed_replacement_request() -> HealRequest { + let mut request = HealRequest::new( + HealType::ErasureSet { + buckets: Vec::new(), + set_disk_id: "pool_0_set_0".to_string(), + }, + HealOptions { + pool_index: Some(0), + set_index: Some(0), + ..Default::default() + }, + HealPriority::Low, + ); + request.source = HealRequestSource::AutoHeal; + request.heal_endpoints = vec!["/data/disk0".to_string()]; + request +} + +#[tokio::test] +async fn directory_backed_replacement_falls_back_to_set_format_when_disk_checks_are_bypassed() { + temp_env::async_with_vars( + [ + (rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, Some("true")), + (rustfs_config::ENV_MINIO_CI, None::<&str>), + ], + async { + let storage = Arc::new(MockStorage { + replacement_target_identities_ready: Mutex::new(false), + global_format_ok_endpoints: Mutex::new(vec!["/data/disk0".to_string()]), + ..Default::default() + }); + let task = HealTask::from_request(directory_backed_replacement_request(), storage.clone()); + + // The mock has no local disk behind "/data/disk0", so the run stops at + // the healing-marker step that follows the format stage, exactly like + // `automatic_replacement_uses_target_scoped_format`. The assertions + // below pin which format path ran before that point. + let err = task.execute().await.expect_err("the mock has no local healing marker target"); + assert!( + err.to_string().contains("healing marker target is unavailable"), + "the fallback must reach the post-format marker step, got: {err}" + ); + + assert_eq!( + *storage.global_format_calls.lock().unwrap(), + 1, + "the fallback must run exactly one set-wide format heal" + ); + assert!( + storage.replacement_format_calls.lock().unwrap().is_empty(), + "the fallback must not run the target-scoped replacement format" + ); + }, + ) + .await; +} + +#[tokio::test] +async fn directory_backed_replacement_stays_fail_closed_without_disk_check_bypass() { + temp_env::async_with_vars( + [ + (rustfs_config::ENV_UNSAFE_BYPASS_DISK_CHECK, None::<&str>), + (rustfs_config::ENV_MINIO_CI, None::<&str>), + ], + async { + let storage = Arc::new(MockStorage { + replacement_target_identities_ready: Mutex::new(false), + ..Default::default() + }); + let task = HealTask::from_request(directory_backed_replacement_request(), storage.clone()); + + task.execute() + .await + .expect_err("an inadmissible replacement target must keep failing closed"); + + assert_eq!( + *storage.global_format_calls.lock().unwrap(), + 0, + "fail-closed admission must not format the set" + ); + assert!( + storage.replacement_format_calls.lock().unwrap().is_empty(), + "fail-closed admission must not format the target" + ); + }, + ) + .await; +} + #[tokio::test] async fn automatic_replacement_persists_intent_before_format() { let storage = Arc::new(MockStorage { @@ -962,6 +1051,8 @@ struct MockStorage { format_no_heal_required: Mutex, format_error: Mutex>, global_format_calls: Mutex, + /// Endpoints the set-wide format mock reports as freshly formatted (`state == "ok"`). + global_format_ok_endpoints: Mutex>, replacement_format_calls: Mutex)>>, replacement_target_identities_ready: Mutex, replacement_target_identity_sequences: Mutex>>, @@ -1338,10 +1429,26 @@ impl HealStorageAPI for MockStorage { return Err(error); } let no_heal_required = *self.format_no_heal_required.lock().unwrap(); + let result = HealResultItem { + after: Infos { + drives: self + .global_format_ok_endpoints + .lock() + .unwrap() + .iter() + .map(|endpoint| HealDriveInfo { + endpoint: endpoint.clone(), + state: "ok".to_string(), + ..Default::default() + }) + .collect(), + }, + ..Default::default() + }; if no_heal_required { - Ok((HealResultItem::default(), Some(Error::Storage(EcstoreError::NoHealRequired)))) + Ok((result, Some(Error::Storage(EcstoreError::NoHealRequired)))) } else { - Ok((HealResultItem::default(), None)) + Ok((result, None)) } }