Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(netcheck): Integrate https and icmp probes in probeplan #1220

Merged
merged 25 commits into from
Jul 14, 2023

Conversation

flub
Copy link
Contributor

@flub flub commented Jul 12, 2023

Description

The HTTPS and ICMP probes only need to run after the STUN probes have
failed. This is done in the probe plan by sequencing them after the
stun probes. If the STUN probes succeed the probe_would_help logic
will shortcut execution.

Because this was all constantly looking up DerpNodes in the DerpMap
they are now created once and put on the heap when the plan is
created. This means the ProbeReports now carry enough data that they
don't need looking up yet again.

Closes #1138

Notes & open questions

Because we only have one derp server currently, we never reach enough regions to abort all the probes. This means we end up waiting for the start of the icmp & http probes. Once they start the probe-would-help check however aborts the probe sets.

I'm not really happy with that, it would be better if the probesets for a region would be aborted right away once the region is done. This ties in with the other refactor of moving the probe-would-help logic upside down that I've already mentioned (see #1180 (review)).

It's probably fine for now if we create an issue for it?

The solution is enough enough: 3bd0108

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@flub flub enabled auto-merge (squash) July 14, 2023 08:13
@Arqu
Copy link
Collaborator

Arqu commented Jul 14, 2023

/netsim branch bump-ipv6-config

@github-actions
Copy link

flub/netcheck-https-icmp-probeplan.b615aa294d9082021595a5be1fe1dcac35e4bfb5
Perf report:

test case throughput_gbps throughput_transfer
iroh_latency_20ms 1_to_1 2.18 2.98
iroh_latency_20ms 1_to_3 6.73 8.04
iroh_latency_20ms 1_to_5 11.63 13.69
iroh_latency_20ms 1_to_10 20.50 23.46
iroh_latency_20ms 2_to_2 5.75 5.82
iroh_latency_20ms 2_to_4 9.79 11.46
iroh_latency_20ms 2_to_6 15.34 18.14
iroh_latency_20ms 2_to_10 21.42 27.87
iroh 1_to_1 2.87 2.92
iroh 1_to_3 6.99 8.45
iroh 1_to_5 12.37 13.71
iroh 1_to_10 21.73 25.50
iroh 2_to_2 5.70 5.80
iroh 2_to_4 9.20 11.31
iroh 2_to_6 16.37 18.22
iroh 2_to_10 25.33 28.78
iroh_latency_200ms 1_to_1 1.85 2.35
iroh_latency_200ms 1_to_3 6.24 7.17
iroh_latency_200ms 1_to_5 10.87 12.66
iroh_latency_200ms 1_to_10 22.23 25.26
iroh_latency_200ms 2_to_2 4.05 5.31
iroh_latency_200ms 2_to_4 8.90 11.05
iroh_latency_200ms 2_to_6 14.96 17.42
iroh_latency_200ms 2_to_10 23.14 27.58

@flub flub disabled auto-merge July 14, 2023 15:31
@Arqu Arqu merged commit a0ae228 into main Jul 14, 2023
12 of 14 checks passed
@Arqu Arqu deleted the flub/netcheck-https-icmp-probeplan branch July 14, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

netcheck: ping and https probes only needed when udp fails
3 participants