fix(netcheck): Make ICMP ping optional#1137
Conversation
On Linux you don't have permission to send ICMP pings by default, so creating the pinger will fail. This should not make all of netcheck fail.
| match Pinger::new().await { | ||
| Ok(pinger) => Some(pinger), | ||
| Err(err) => { | ||
| debug!("failed to create pinger: {err:#}"); |
There was a problem hiding this comment.
Maybe remove the https probes from the plan in this case?
There was a problem hiding this comment.
hum, no i think they should stay in since they will keep working. I'm not really sure why these pingers are triggered by the https probes in the plan, the tailscale code only uses the https and pinger probes if the udp stun fails. but i decided that changing (or even investigating this) should not be mixed into this PR.
but good call, should create an issue not to forget: #1138
There was a problem hiding this comment.
Maybe the pinger should be created lazily? The reason is that I went and finished a todo that they had, where the https probes are actually integrated into the plans, instead of being added manually after the fact.
There was a problem hiding this comment.
Yes, can we do this as part of #1138? In either case the fix of making failure to create the pinger non-fatal is needed.
|
Can we ignore this failure? it's a flaky one that I'd like to address separately: #1138 |
2 participants
On Linux you don't have permission to send ICMP pings by default, so
creating the pinger will fail. This should not make all of netcheck
fail.