fix(metrics): servers_offline_total counts each peer once (normalize peer-health key) (#4572)

fix(metrics): normalize peer-health key so servers_offline_total counts each peer once rustfs_cluster_servers_offline_total is the count of distinct keys in the addr-keyed CLUSTER_PEER_HEALTH map. The data path keys a peer by endpoint.grid_host() (scheme://host:port) while the lock path keys it by url::Url::to_string() (scheme://host:port/, trailing slash), so one physical peer becomes two health entries and a single downed node reports 2 (2N for N). Normalize the address (trim trailing slashes) at the map boundary in record_peer_reachable/record_peer_unreachable/cluster_peer_is_offline/ cluster_peer_should_bypass so both forms collapse to one canonical key. Pure observability fix; peer selection and quorum are unchanged. Fixes rustfs/backlog#1043 Co-authored-by: heihutu <heihutu@gmail.com>
2026-07-26 08:18:18 +00:00 · 2026-07-09 04:56:08 +08:00
parent cf2da0c44d
commit 506ded59ad
2 changed files with 101 additions and 7 deletions
@@ -467,12 +467,24 @@ fn publish_offline_gauge(peers: &HashMap<String, PeerHealthState>) {
    gauge!(CLUSTER_SERVERS_OFFLINE_TOTAL).set(offline as f64);
 }

+/// Canonicalize a peer address before using it as the `CLUSTER_PEER_HEALTH` key.
+///
+/// The same physical peer is referenced by different subsystems in slightly different string forms
+/// — the data path keys by `endpoint.grid_host()` (`scheme://host:port`, no trailing slash) while
+/// the lock path keys by `url::Url::to_string()` (`scheme://host:port/`, trailing slash). Without
+/// normalization each form becomes its own health entry, so one downed node counts as 2 in the
+/// `rustfs_cluster_servers_offline_total` gauge (and 2N for N nodes). Trimming trailing slashes
+/// collapses them to a single canonical key.
+fn normalize_peer_key(addr: &str) -> &str {
+    addr.trim_end_matches('/')
+}
+
 /// Record that a cluster peer is reachable: mark it online and reset its consecutive-failure
 /// counter. Called on a successful dial to `addr`.
 pub fn record_peer_reachable(addr: &str) {
    // Recover from a poisoned lock so peer-health tracking and the offline gauge never stall permanently.
    let mut peers = CLUSTER_PEER_HEALTH.lock().unwrap_or_else(|poisoned| poisoned.into_inner());
-    let entry = peers.entry(addr.to_string()).or_default();
+    let entry = peers.entry(normalize_peer_key(addr).to_string()).or_default();
    entry.online = true;
    entry.consecutive_failures = 0;
    entry.last_reprobe = None;
@@ -484,7 +496,7 @@ pub fn record_peer_reachable(addr: &str) {
 pub fn record_peer_unreachable(addr: &str, failure_threshold: u32) {
    // Recover from a poisoned lock so peer-health tracking and the offline gauge never stall permanently.
    let mut peers = CLUSTER_PEER_HEALTH.lock().unwrap_or_else(|poisoned| poisoned.into_inner());
-    let entry = peers.entry(addr.to_string()).or_default();
+    let entry = peers.entry(normalize_peer_key(addr).to_string()).or_default();
    entry.consecutive_failures = entry.consecutive_failures.saturating_add(1);
    if entry.consecutive_failures >= failure_threshold.max(1) {
        entry.online = false;
@@ -495,7 +507,7 @@ pub fn record_peer_unreachable(addr: &str, failure_threshold: u32) {
 /// Whether a cluster peer is currently considered offline (known and marked offline).
 pub fn cluster_peer_is_offline(addr: &str) -> bool {
    let peers = CLUSTER_PEER_HEALTH.lock().unwrap_or_else(|poisoned| poisoned.into_inner());
-    peers.get(addr).map(|peer| !peer.online).unwrap_or(false)
+    peers.get(normalize_peer_key(addr)).map(|peer| !peer.online).unwrap_or(false)
 }

 /// Decide whether to fast-fail (bypass) an offline peer instead of attempting to reach it
@@ -507,7 +519,7 @@ pub fn cluster_peer_is_offline(addr: &str) -> bool {
 /// bypassed.
 pub fn cluster_peer_should_bypass(addr: &str, reprobe_interval: Duration) -> bool {
    let mut peers = CLUSTER_PEER_HEALTH.lock().unwrap_or_else(|poisoned| poisoned.into_inner());
-    let Some(entry) = peers.get_mut(addr) else {
+    let Some(entry) = peers.get_mut(normalize_peer_key(addr)) else {
        return false;
    };
    if entry.online {
@@ -529,7 +541,19 @@ pub fn cluster_peer_should_bypass(addr: &str, reprobe_interval: Duration) -> boo

 #[cfg(test)]
 fn cluster_peer_online(addr: &str) -> Option<bool> {
-    CLUSTER_PEER_HEALTH.lock().ok()?.get(addr).map(|peer| peer.online)
+    CLUSTER_PEER_HEALTH
+        .lock()
+        .ok()?
+        .get(normalize_peer_key(addr))
+        .map(|peer| peer.online)
+}
+
+#[cfg(test)]
+fn cluster_peer_health_keys() -> Vec<String> {
+    CLUSTER_PEER_HEALTH
+        .lock()
+        .map(|peers| peers.keys().cloned().collect())
+        .unwrap_or_default()
 }

 #[cfg(test)]
@@ -712,6 +736,30 @@ mod tests {
        assert_eq!(cluster_peer_online(addr), Some(true));
    }

+    #[test]
+    fn peer_health_key_is_normalized_across_address_forms() {
+        // Same physical peer, two address forms the codebase actually uses: the data path's
+        // grid_host() (no trailing slash) and the lock path's url::Url::to_string() (trailing slash).
+        // They MUST collapse to one health entry, else one downed node counts as 2 in the gauge.
+        let bare = "http://cluster-peer-health-normalize-test:9000";
+        let slashed = "http://cluster-peer-health-normalize-test:9000/";
+
+        record_peer_unreachable(bare, 1);
+        record_peer_unreachable(slashed, 1);
+
+        let keys: Vec<String> = cluster_peer_health_keys()
+            .into_iter()
+            .filter(|k| k.contains("cluster-peer-health-normalize-test"))
+            .collect();
+        assert_eq!(keys.len(), 1, "two address forms of one peer must be a single health entry, got {keys:?}");
+
+        // Either form observes the peer offline, and a reachable dial via one form clears the other.
+        assert!(cluster_peer_is_offline(bare));
+        assert!(cluster_peer_is_offline(slashed));
+        record_peer_reachable(slashed);
+        assert!(!cluster_peer_is_offline(bare), "reachable via one form must mark the shared entry online");
+    }
+
    #[test]
    fn cluster_servers_offline_total_name_is_stable() {
        assert_eq!(CLUSTER_SERVERS_OFFLINE_TOTAL, "rustfs_cluster_servers_offline_total");
@@ -36,8 +36,10 @@ already-running server. Use `--dry-run` to preview the env and command.
  compose cluster, kills `FAILOVER_NODE`, benchmarks; writes
  `target/bench/four-node-failover-<ts>/`).

-Store the paired artifacts as `target/bench/internode-transport/{baseline,after-P0,after-P1,after-P3}/`
-(the paths the design docs reference). `target/` is gitignored — attach the artifacts to the PR.
+The one-click driver writes each run to `target/bench/internode-transport/<stage>-<phase>/`
+(e.g. `p0-before/`, `p0-after/`, `p1-before/`, `p1-after/`, `p3-before/`, `p3-after/`), each
+containing the emitted `server-env.sh` plus the underlying bench artifacts. `target/` is
+gitignored — attach the paired directories to the PR / issue.

 ## Metrics to capture (Prometheus)

@@ -91,6 +93,50 @@ column, everything else at defaults. Roll a restart between runs.
  `RUSTFS_INTERNODE_OFFLINE_BYPASS=true`; expect faster failover and a correct
  `rustfs_cluster_servers_offline_total` (1 while the node is down, back to 0 after recovery).

+## Acceptance gates & artifact layout
+
+Each stage's paired run must satisfy an explicit gate before its numbers are accepted. Record
+the gate verdict (pass/fail + measured delta) in the paired directory's `summary` and attach it.
+
+| Stage | Bench | Acceptance gate | Primary metric(s) |
+|---|---|---|---|
+| **P0** | `run_internode_transport_baseline.sh` | small-RPC `duration_ms` (DiskInfo/Ping) **down**; large-metadata (ReadMultiple/BatchReadVersion) throughput **up**; a `>4 MiB` multi-version `xl.meta` no longer fails `out_of_range` (functional). | `..._operation_duration_ms{operation}`, `..._operation_payload_bytes`, object-bench throughput |
+| **P1** | `run_internode_transport_baseline.sh` (mixed: large `ReadAll` + high-frequency `Refresh`) | **lock p99 down ≥ 20%** with `RUSTFS_INTERNODE_CHANNEL_ISOLATION=true` vs baseline. | lock p99 (lock metrics), `..._operation_large_payloads_total` |
+| **P3 cold-start** | `run_internode_transport_baseline.sh` (fresh cluster, first cross-node op) | first cross-node op latency **drops ~one connect RTT** with `RUSTFS_INTERNODE_PREWARM=true`. | `..._dial_avg_time_nanos`, first-op `..._operation_duration_ms` |
+| **P3 offline** | dedicated *sustained-offline + survivor cross-node access* experiment (the standard four-node failover bench is **not** sensitive to the bypass — quorum holds, `recovery_seconds=0`) | with `RUSTFS_INTERNODE_OFFLINE_BYPASS=true`, survivor cross-node op latency to the downed peer **fast-fails** instead of hanging the dial timeout; `rustfs_cluster_servers_offline_total` = **1** while down, back to **0** after recovery. | `rustfs_cluster_servers_offline_total`, survivor cross-node `..._operation_duration_ms`, `..._dial_errors_total` |
+
+> **P2 is not a throughput gate.** Its acceptance is operational: `msgpack_json_fallback_total`
+> must read **0** across a full release window before `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=true`
+> is enabled (see the msgpack convergence runbook). Do not benchmark P2 as before/after throughput.
+
+Artifact layout per stage (attach both halves + the diff):
+
+```
+target/bench/internode-transport/
+  p0-before/  p0-after/     # server-env.sh + object-bench summaries + metric deltas
+  p1-before/  p1-after/     # + lock p99 delta (the ≥20% gate)
+  p3-before/  p3-after/     # cold-start + sustained-offline experiment + offline gauge trace
+```
+
+## Bench-host prerequisites (ansible bare-metal)
+
+Captured while running the first real A/B on a 4-node ansible cluster; needed before any live run:
+
+- **RPC secret is mandatory on current `main`.** Internode RPC fails closed: default creds
+  (`RUSTFS_SECRET_KEY=rustfsadmin`) with no `RUSTFS_RPC_SECRET` → `No valid auth token` → the cluster
+  never reaches `storage_quorum`. Set a **non-default** `RUSTFS_RPC_SECRET`, identical on every node.
+- **systemd start timeout.** The install unit is `Type=notify`; READY only fires after quorum, which on
+  freshly-purged disks exceeds a 30 s `TimeoutStartSec` → crash loop. Use a drop-in `TimeoutStartSec=infinity`.
+- **Server-side metrics need OTLP.** RustFS has no Prometheus pull endpoint (`/admin/v3/metrics` is NDJSON,
+  not exposition); it only pushes via OTLP. To capture lock/offline/internode metrics, run an
+  otel-collector (OTLP receiver → Prometheus exporter) and set `RUSTFS_OBS_ENDPOINT`,
+  `RUSTFS_OBS_METRICS_EXPORT_ENABLED=true`, `RUSTFS_OBS_METER_INTERVAL=5`. For lock p99 also set
+  `RUSTFS_OBJECT_LOCK_DIAG_ENABLE=true` (default off).
+- **P3 offline method.** Standard failover is quorum-insensitive to the bypass. Use a *sustained-offline +
+  survivor cross-node access* run instead: all nodes up → stop one node (sustained) → warm up to trip
+  offline detection → drive warp on the survivors only (`--host` excludes the dead node) → compare
+  survivor op p99 and `rustfs_cluster_servers_offline_total` for `RUSTFS_INTERNODE_OFFLINE_BYPASS` off/on.
+
 ## Rollback

 Every stage is a single-env rollback (set the env back to its baseline column and restart).