From b34587fdd5f3649437be1f4f82edb0757a7898fb Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Wed, 3 Jul 2024 10:58:13 +0200 Subject: [PATCH] fix(iroh-net): delays of non-stun probes for subsequent relays (#2445) ## Description The idea of the probe plan is that first the STUN probes happen, if those don't work we add the other probes. However when there are multiple relay servers we accidentally started the subsequent probes for all but the first relay server too late. This makes sure to globally record when the last STUN probe was sent and re-uses this value for all the relay servers which are probed. ## Breaking Changes ## Notes & open questions Fixes #2444 See #2221 for flaky tests. ## Change checklist - [x] Self-review. - ~~[ ] Documentation updates if relevant.~~ - [x] Tests if relevant. - ~~[ ] All breaking changes documented.~~ --- iroh-net/src/netcheck/reportgen/probes.rs | 450 +++++++--------------- 1 file changed, 131 insertions(+), 319 deletions(-) diff --git a/iroh-net/src/netcheck/reportgen/probes.rs b/iroh-net/src/netcheck/reportgen/probes.rs index 874125523f..61c3a2140c 100644 --- a/iroh-net/src/netcheck/reportgen/probes.rs +++ b/iroh-net/src/netcheck/reportgen/probes.rs @@ -202,6 +202,10 @@ impl ProbePlan { pub(super) fn initial(relay_map: &RelayMap, if_state: &interfaces::State) -> Self { let mut plan = Self(BTreeSet::new()); + // The first time we need add probes after the STUN we record this delay, so that + // further relay server can re-use this delay. + let mut max_stun_delay: Option = None; + for relay_node in relay_map.nodes() { let mut stun_ipv4_probes = ProbeSet::new(ProbeProto::StunIpv4); let mut stun_ipv6_probes = ProbeSet::new(ProbeProto::StunIpv6); @@ -234,7 +238,8 @@ impl ProbePlan { let mut icmp_probes_ipv4 = ProbeSet::new(ProbeProto::IcmpV4); let mut icmp_probes_ipv6 = ProbeSet::new(ProbeProto::IcmpV6); for attempt in 0..3 { - let start = plan.max_delay() + DEFAULT_INITIAL_RETRANSMIT; + let start = *max_stun_delay.get_or_insert_with(|| plan.max_delay()) + + DEFAULT_INITIAL_RETRANSMIT; let delay = start + DEFAULT_INITIAL_RETRANSMIT * attempt as u32; https_probes @@ -278,6 +283,10 @@ impl ProbePlan { } let mut plan = Self(Default::default()); + // The first time we need add probes after the STUN we record this delay, so that + // further relay servers can re-use this delay. + let mut max_stun_delay: Option = None; + let had_stun_ipv4 = !last_report.relay_v4_latency.is_empty(); let had_stun_ipv6 = !last_report.relay_v6_latency.is_empty(); let had_both = if_state.have_v6 && had_stun_ipv4 && had_stun_ipv6; @@ -349,7 +358,7 @@ impl ProbePlan { let mut https_probes = ProbeSet::new(ProbeProto::Https); let mut icmp_v4_probes = ProbeSet::new(ProbeProto::IcmpV4); let mut icmp_v6_probes = ProbeSet::new(ProbeProto::IcmpV6); - let start = plan.max_delay(); + let start = *max_stun_delay.get_or_insert_with(|| plan.max_delay()); for attempt in 0..attempts { let delay = start + (retransmit_delay * attempt as u32) @@ -471,6 +480,26 @@ mod tests { use super::*; + /// Shorthand which declares a new ProbeSet. + /// + /// `$kind`: The `ProbeProto`. + /// `$node`: Expression which will be an `Arc`. + /// `$delays`: A `Vec` of the delays for this probe. + macro_rules! probeset { + (proto: ProbeProto::$kind:ident, relay: $node:expr, delays: $delays:expr,) => { + ProbeSet { + proto: ProbeProto::$kind, + probes: $delays + .iter() + .map(|delay| Probe::$kind { + delay: *delay, + node: $node, + }) + .collect(), + } + }; + } + #[tokio::test] async fn test_initial_probeplan() { let relay_map = default_relay_map(); @@ -480,192 +509,75 @@ mod tests { let plan = ProbePlan::initial(&relay_map, &if_state); let expected_plan: ProbePlan = [ - ProbeSet { + probeset! { proto: ProbeProto::StunIpv4, - probes: vec![ - Probe::StunIpv4 { - delay: Duration::ZERO, - node: relay_node_1.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_millis(100), - node: relay_node_1.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_millis(200), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::ZERO, + Duration::from_millis(100), + Duration::from_millis(200)], }, - ProbeSet { + probeset! { proto: ProbeProto::StunIpv6, - probes: vec![ - Probe::StunIpv6 { - delay: Duration::ZERO, - node: relay_node_1.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_millis(100), - node: relay_node_1.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_millis(200), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::ZERO, + Duration::from_millis(100), + Duration::from_millis(200)], }, - ProbeSet { + probeset! { proto: ProbeProto::Https, - probes: vec![ - Probe::Https { - delay: Duration::from_millis(300), - node: relay_node_1.clone(), - }, - Probe::Https { - delay: Duration::from_millis(400), - node: relay_node_1.clone(), - }, - Probe::Https { - delay: Duration::from_millis(500), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::from_millis(300), + Duration::from_millis(400), + Duration::from_millis(500)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV4, - probes: vec![ - Probe::IcmpV4 { - delay: Duration::from_millis(300), - node: relay_node_1.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_millis(400), - node: relay_node_1.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_millis(500), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::from_millis(300), + Duration::from_millis(400), + Duration::from_millis(500)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV6, - probes: vec![ - Probe::IcmpV6 { - delay: Duration::from_millis(300), - node: relay_node_1.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_millis(400), - node: relay_node_1.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_millis(500), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::from_millis(300), + Duration::from_millis(400), + Duration::from_millis(500)], }, - ProbeSet { + probeset! { proto: ProbeProto::StunIpv4, - probes: vec![ - Probe::StunIpv4 { - delay: Duration::ZERO, - node: relay_node_2.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_millis(100), - node: relay_node_2.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_millis(200), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::ZERO, + Duration::from_millis(100), + Duration::from_millis(200)], }, - ProbeSet { + probeset! { proto: ProbeProto::StunIpv6, - probes: vec![ - Probe::StunIpv6 { - delay: Duration::ZERO, - node: relay_node_2.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_millis(100), - node: relay_node_2.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_millis(200), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::ZERO, + Duration::from_millis(100), + Duration::from_millis(200)], }, - ProbeSet { + probeset! { proto: ProbeProto::Https, - probes: vec![ - Probe::Https { - delay: Duration::from_millis(600), - node: relay_node_2.clone(), - }, - Probe::Https { - delay: Duration::from_millis(700), - node: relay_node_2.clone(), - }, - Probe::Https { - delay: Duration::from_millis(800), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::from_millis(300), + Duration::from_millis(400), + Duration::from_millis(500)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV4, - probes: vec![ - Probe::IcmpV4 { - delay: Duration::from_millis(600), - node: relay_node_2.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_millis(700), - node: relay_node_2.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_millis(800), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::from_millis(300), + Duration::from_millis(400), + Duration::from_millis(500)], }, - ProbeSet { - proto: ProbeProto::StunIpv4, - probes: vec![ - Probe::StunIpv4 { - delay: Duration::ZERO, - node: relay_node_2.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_millis(100), - node: relay_node_2.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_millis(200), - node: relay_node_2.clone(), - }, - ], - }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV6, - probes: vec![ - Probe::IcmpV6 { - delay: Duration::from_millis(600), - node: relay_node_2.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_millis(700), - node: relay_node_2.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_millis(800), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::from_millis(300), + Duration::from_millis(400), + Duration::from_millis(500)], }, ] .into_iter() @@ -715,175 +627,75 @@ mod tests { }; let plan = ProbePlan::with_last_report(&relay_map, &if_state, &last_report); let expected_plan: ProbePlan = [ - ProbeSet { + probeset! { proto: ProbeProto::StunIpv4, - probes: vec![ - Probe::StunIpv4 { - delay: Duration::ZERO, - node: relay_node_1.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_micros(52_400), - node: relay_node_1.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_micros(104_800), - node: relay_node_1.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_micros(157_200), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::ZERO, + Duration::from_micros(52_400), + Duration::from_micros(104_800), + Duration::from_micros(157_200)], }, - ProbeSet { + probeset! { proto: ProbeProto::StunIpv6, - probes: vec![ - Probe::StunIpv6 { - delay: Duration::ZERO, - node: relay_node_1.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_micros(52_400), - node: relay_node_1.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_micros(104_800), - node: relay_node_1.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_micros(157_200), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::ZERO, + Duration::from_micros(52_400), + Duration::from_micros(104_800), + Duration::from_micros(157_200)], }, - ProbeSet { + probeset! { proto: ProbeProto::Https, - probes: vec![ - Probe::Https { - delay: Duration::from_micros(207_200), - node: relay_node_1.clone(), - }, - Probe::Https { - delay: Duration::from_micros(259_600), - node: relay_node_1.clone(), - }, - Probe::Https { - delay: Duration::from_micros(312_000), - node: relay_node_1.clone(), - }, - Probe::Https { - delay: Duration::from_micros(364_400), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::from_micros(207_200), + Duration::from_micros(259_600), + Duration::from_micros(312_000), + Duration::from_micros(364_400)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV4, - probes: vec![ - Probe::IcmpV4 { - delay: Duration::from_micros(207_200), - node: relay_node_1.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_micros(259_600), - node: relay_node_1.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_micros(312_000), - node: relay_node_1.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_micros(364_400), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::from_micros(207_200), + Duration::from_micros(259_600), + Duration::from_micros(312_000), + Duration::from_micros(364_400)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV6, - probes: vec![ - Probe::IcmpV6 { - delay: Duration::from_micros(207_200), - node: relay_node_1.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_micros(259_600), - node: relay_node_1.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_micros(312_000), - node: relay_node_1.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_micros(364_400), - node: relay_node_1.clone(), - }, - ], + relay: relay_node_1.clone(), + delays: [Duration::from_micros(207_200), + Duration::from_micros(259_600), + Duration::from_micros(312_000), + Duration::from_micros(364_400)], }, - ProbeSet { + probeset! { proto: ProbeProto::StunIpv4, - probes: vec![ - Probe::StunIpv4 { - delay: Duration::ZERO, - node: relay_node_2.clone(), - }, - Probe::StunIpv4 { - delay: Duration::from_micros(52_400), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::ZERO, + Duration::from_micros(52_400)], }, - ProbeSet { + probeset! { proto: ProbeProto::StunIpv6, - probes: vec![ - Probe::StunIpv6 { - delay: Duration::ZERO, - node: relay_node_2.clone(), - }, - Probe::StunIpv6 { - delay: Duration::from_micros(52_400), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::ZERO, + Duration::from_micros(52_400)], }, - ProbeSet { + probeset! { proto: ProbeProto::Https, - probes: vec![ - Probe::Https { - delay: Duration::from_micros(414_400), - node: relay_node_2.clone(), - }, - Probe::Https { - delay: Duration::from_micros(466_800), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::from_micros(207_200), + Duration::from_micros(259_600)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV4, - probes: vec![ - Probe::IcmpV4 { - delay: Duration::from_micros(414_400), - node: relay_node_2.clone(), - }, - Probe::IcmpV4 { - delay: Duration::from_micros(466_800), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::from_micros(207_200), + Duration::from_micros(259_600)], }, - ProbeSet { + probeset! { proto: ProbeProto::IcmpV6, - probes: vec![ - Probe::IcmpV6 { - delay: Duration::from_micros(414_400), - node: relay_node_2.clone(), - }, - Probe::IcmpV6 { - delay: Duration::from_micros(466_800), - node: relay_node_2.clone(), - }, - ], + relay: relay_node_2.clone(), + delays: [Duration::from_micros(207_200), + Duration::from_micros(259_600)], }, ] .into_iter()