From 071a4600bc469b5c887ac684cb86973b627306c3 Mon Sep 17 00:00:00 2001 From: houseme Date: Thu, 9 Jul 2026 18:50:00 +0800 Subject: [PATCH] fix(admin): make server_info retry re-dial and match peer disks by resolved host (#4618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up fixes to #4607 (both verified real bugs, each with a regression test): 1. The in-call server_info retry did not re-dial on network errors. A network-like first failure runs through `finalize_result`, which sets the client's `offline` gate; the retry then called `evict_connection` and re-invoked `server_info`, but `get_client` short-circuits on that gate and returns "temporarily offline" without dialing. Only the async recovery monitor would clear it. So the retry re-dialed only in the timeout branch and was a no-op in the transport/half-open branch it was meant to cover. Add `PeerRestClient::prepare_retry` (evict + clear the offline gate) and use it before the retry. 2. Synthesized/degraded drive lists went empty on hostname deployments. `PeerRestClient::host` is an `XHost` that `hosts_sorted` builds via `XHost::try_from` -> `to_socket_addrs`, so it is the resolved `IP:port`; but `synthesized_disks`/`peer_disk_health` compared it against `Endpoint::host_port()`, which is the raw `hostname:port`. On hostname clusters the compare missed, the drive list came back empty, and `unknownDisks` stayed 0 — reproducing the "drives vanish from the summary" regression #4607 fixed (also affected the pre-existing offline path). Compare through a shared `endpoint_host_matches` helper that canonicalizes the endpoint side through the same `XHost` resolution. Tests: `peer_rest_client_prepare_retry_clears_offline_gate`, `endpoint_host_matches_direct_and_canonicalized` (uses localhost, no external DNS). Follow-up to rustfs/rustfs#4607; tracked in rustfs/backlog#1049. Co-authored-by: heihutu --- .../src/cluster/rpc/peer_rest_client.rs | 30 ++++++++++ .../ecstore/src/services/notification_sys.rs | 59 +++++++++++++++++-- 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/crates/ecstore/src/cluster/rpc/peer_rest_client.rs b/crates/ecstore/src/cluster/rpc/peer_rest_client.rs index 81a306194..05897f49f 100644 --- a/crates/ecstore/src/cluster/rpc/peer_rest_client.rs +++ b/crates/ecstore/src/cluster/rpc/peer_rest_client.rs @@ -147,6 +147,20 @@ impl PeerRestClient { evict_failed_connection(&self.grid_host).await; } + /// Prepare this client for an immediate fresh-connection retry. + /// + /// On a network-like failure `finalize_result` both evicts the channel and + /// sets the offline gate, after which `get_client` fast-fails with + /// "temporarily offline" and only the async background recovery monitor + /// would clear the gate (not within this call). So a plain `evict_connection` + /// is not enough to make an in-call retry actually re-dial: the gate still + /// short-circuits it. This drops the cached channel AND clears the gate so + /// the very next `get_client` re-dials. See rustfs/backlog#1049 (P1-B). + pub async fn prepare_retry(&self) { + self.evict_connection().await; + self.offline.store(false, Ordering::Release); + } + fn is_network_like_error(err: &Error) -> bool { let message = err.to_string().to_ascii_lowercase(); [ @@ -1163,6 +1177,22 @@ mod tests { assert!(err.to_string().contains("temporarily offline")); } + #[tokio::test] + async fn peer_rest_client_prepare_retry_clears_offline_gate() { + // finalize_result sets the offline gate on a network error; without + // clearing it, an in-call retry would fast-fail on the gate instead of + // re-dialing (rustfs/backlog#1049 P1-B). prepare_retry must clear it. + let client = test_peer_client(); + client.offline.store(true, Ordering::Release); + + client.prepare_retry().await; + + assert!( + !client.offline.load(Ordering::Acquire), + "prepare_retry must clear the offline gate so the next get_client re-dials" + ); + } + #[tokio::test] async fn peer_rest_client_finalize_result_marks_offline_for_network_errors() { let client = test_peer_client(); diff --git a/crates/ecstore/src/services/notification_sys.rs b/crates/ecstore/src/services/notification_sys.rs index 72578d73e..6d2ece230 100644 --- a/crates/ecstore/src/services/notification_sys.rs +++ b/crates/ecstore/src/services/notification_sys.rs @@ -27,6 +27,7 @@ use rustfs_madmin::health::{Cpus, MemInfo, OsInfo, Partitions, ProcInfo, SysConf use rustfs_madmin::metrics::RealtimeMetrics; use rustfs_madmin::net::NetInfo; use rustfs_madmin::{ItemState, ServerProperties, StorageInfo}; +use rustfs_utils::XHost; use std::collections::hash_map::DefaultHasher; use std::future::Future; use std::hash::{Hash, Hasher}; @@ -385,9 +386,13 @@ impl NotificationSys { Err(_) => debug!("peer {host} server_info timed out (attempt 1/2) after {peer_timeout:?}"), } - // Drop the suspect channel so the retry re-dials on a fresh - // connection instead of reusing the bad one. - client.evict_connection().await; + // Drop the suspect channel AND clear the offline gate so the + // retry actually re-dials. A network-like first failure runs + // through `finalize_result`, which sets the offline gate; a bare + // `evict_connection` would leave that gate up and the retry would + // fast-fail with "temporarily offline" instead of reconnecting + // (rustfs/backlog#1049 P1-B). + client.prepare_retry().await; // Second and final attempt on the fresh channel. match timeout(peer_timeout, client.server_info()).await { @@ -1150,7 +1155,7 @@ async fn peer_disk_health(host: &str) -> Option { let Some(ep) = set.set_endpoints.get(idx) else { continue; }; - if host != ep.host_port() { + if !endpoint_host_matches(host, &ep.host_port()) { continue; } let online = match slot { @@ -1376,7 +1381,7 @@ fn synthesized_disks(host: &str, endpoints: &EndpointServerPools, state: ItemSta for pool in endpoints.as_ref() { for ep in pool.endpoints.as_ref() { - if (host.is_empty() && ep.is_local) || host == ep.host_port() { + if (host.is_empty() && ep.is_local) || endpoint_host_matches(host, &ep.host_port()) { disks.push(rustfs_madmin::Disk { endpoint: ep.to_string(), state: state.to_string().to_owned(), @@ -1392,6 +1397,27 @@ fn synthesized_disks(host: &str, endpoints: &EndpointServerPools, state: ItemSta disks } +/// Whether `peer_host` refers to the same node as an endpoint whose +/// `host_port()` is `ep_host_port`. +/// +/// `PeerRestClient::host` is an `XHost`, which resolves names to an address on +/// construction (`hosts_sorted` -> `XHost::try_from` -> `to_socket_addrs`), so +/// `peer_host` is the resolved `IP:port`. An endpoint's `host_port()`, however, +/// is `url.host():port` — still the raw `hostname:port` on hostname-based +/// deployments. A plain string compare therefore misses on hostname clusters, +/// leaving the synthesized/degraded drive list empty and `unknownDisks` at 0 +/// (rustfs/rustfs#4607 follow-up). Compare directly first (fast path / IP +/// deployments), then canonicalize the endpoint side through the same `XHost` +/// resolution and compare again. +fn endpoint_host_matches(peer_host: &str, ep_host_port: &str) -> bool { + if peer_host == ep_host_port { + return true; + } + XHost::try_from(ep_host_port.to_string()) + .map(|resolved| resolved.to_string() == peer_host) + .unwrap_or(false) +} + fn aggregate_notification_failures(operation: &str, failures: Vec) -> Result<()> { if failures.is_empty() { return Ok(()); @@ -1701,6 +1727,29 @@ mod tests { assert!(!cached_snapshot_is_fresh(Some(stale)), "an old success is stale"); } + #[test] + fn endpoint_host_matches_direct_and_canonicalized() { + // Direct match (IP deployment): peer host already equals host_port. + assert!(endpoint_host_matches("10.0.0.12:9000", "10.0.0.12:9000")); + // Different IPs must not match. + assert!(!endpoint_host_matches("10.0.0.12:9000", "10.0.0.99:9000")); + + // Hostname deployment: `PeerRestClient::host` is the resolved `IP:port`, + // the endpoint keeps the raw `hostname:port`. Resolve "localhost" the + // same way `XHost` does (avoids depending on external DNS) and confirm + // the canonical compare matches — the regression this fixes is the + // synthesized/degraded drive list going empty on hostname clusters. + let resolved = XHost::try_from("localhost:9000".to_string()) + .expect("localhost should resolve") + .to_string(); + assert!( + endpoint_host_matches(&resolved, "localhost:9000"), + "resolved localhost ({resolved}) must match the hostname endpoint" + ); + // A resolved address that is not localhost must not match. + assert!(!endpoint_host_matches("203.0.113.1:9000", "localhost:9000")); + } + #[test] fn handle_server_info_failure_returns_unknown_before_threshold_without_cache() { let cache = Mutex::new(PeerAdminCache::new());