fix(windows,linux): ensure set_routes is idempotent#6051
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
| Report | Fri, July 26, 2024 at 05:16:37 UTC |
| Project | Firezone |
| Branch | chore/windows/keep-all-routes-updated |
| Testbed | github-actions |
Click to view all benchmark results
| Benchmark | Throughput | Throughput Results bits/s | (Δ%) | Throughput Lower Boundary bits/s | (%) |
|---|---|---|---|
| direct-tcp-client2server | ✅ (view plot) | 235,883,956.45 (-1.89%) | 235,803,869.26 (99.97%) |
| direct-tcp-server2client | ✅ (view plot) | 251,320,646.23 (+2.04%) | 240,276,504.72 (95.61%) |
| direct-udp-client2server | ✅ (view plot) | 283,181,083.38 (-1.72%) | 271,471,083.14 (95.86%) |
| direct-udp-server2client | ✅ (view plot) | 392,148,501.11 (-0.35%) | 382,020,759.39 (97.42%) |
| relayed-tcp-client2server | ✅ (view plot) | 239,786,479.55 (-2.45%) | 239,689,551.24 (99.96%) |
| relayed-tcp-server2client | ✅ (view plot) | 253,204,720.07 (-1.41%) | 245,107,228.46 (96.80%) |
| relayed-udp-client2server | ✅ (view plot) | 234,317,477.87 (+2.42%) | 218,145,271.48 (93.10%) |
| relayed-udp-server2client | ✅ (view plot) | 338,186,212.48 (-0.57%) | 319,854,147.78 (94.58%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
I am surprised this makes a difference? If you use the caches |
The important difference here is that we call |
|
Manually tested on Windows, seems to work alright! |
thomaseizinger
changed the title
set_routes is idempotent
Co-authored-by: Thomas Eizinger <thomas@eizinger.io> Signed-off-by: Gabi <gabrielalejandro7@gmail.com>
| return; | ||
| } | ||
|
|
||
| tracing::warn!(%route, "Failed to remove route: {e}") |
There was a problem hiding this comment.
The caller doesn't decide how to handle the error?
There was a problem hiding this comment.
What are we meant to do? This is something we should report via Sentry to know about it before users report bugs and we request logs TBH.
There was a problem hiding this comment.
I was thinking if a route fails to add or remove, it will hard to debug where the symptoms appear, if we ignore the error here. But yeah I guess either way we have to use Sentry.
3 participants
Windows may delete the default route during roaming. To prevent this from causing problems, we make
set_routesadd all routes regardless of the previously stored ones. The known routes are only used to compute, what routes are to be removed.For Linux we do the same to make it consistent across platforms.
This also give us the chance to not clear the cache when ips are set, since now all routes are always added, meaning they will be always re-added when roaming.
Overall, this more closely aligns Linux and Windows with how Firezone works on Apple and Android. There, we always remove all routes and set new ones. Removing routes happens very rarely (only when CIDR resources are deactivated), thus, not removing all and re-adding the routes is still deemed to be worth it.
With the new implementation, this is guaranteed to always make the new routes take effect and at the same time be idempotent.