mirror of
https://github.com/rustfs/rustfs.git
synced 2026-07-28 17:18:58 +00:00
fix(admin): make server_info retry re-dial and match peer disks by resolved host (#4618)
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 <heihutu@gmail.com>
This commit is contained in:
@@ -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();
|
||||
|
||||
@@ -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<PeerDiskHealth> {
|
||||
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<String>) -> 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());
|
||||
|
||||
Reference in New Issue
Block a user