If the engine was shutting down from a previous session
(e.closing=true), it would return an error code when trying to get
status. In that case, ipnlocal would never unblock any callers that
were waiting on the status.
Not sure if this ever happened in real life, but I accidentally
triggered it while writing a test.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
magicsock.Conn.ParseEndpoint requires a peer's public key,
disco key, and legacy ip/ports in order to do its job.
We currently accomplish that by:
* adding the public key in our wireguard-go fork
* encoding the disco key as magic hostname
* using a bespoke comma-separated encoding
It's a bit messy.
Instead, switch to something simpler: use a json-encoded struct
containing exactly the information we need, in the form we use it.
Our wireguard-go fork still adds the public key to the
address when it passes it to ParseEndpoint, but now the code
compensating for that is just a couple of simple, well-commented lines.
Once this commit is in, we can remove that part of the fork
and remove the compensating code.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
The new code is ugly, but much faster and leaner.
name old time/op new time/op delta
SetPeers-8 7.81µs ± 1% 3.59µs ± 1% -54.04% (p=0.000 n=9+10)
name old alloc/op new alloc/op delta
SetPeers-8 7.68kB ± 0% 2.53kB ± 0% -67.08% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
SetPeers-8 237 ± 0% 99 ± 0% -58.23% (p=0.000 n=10+10)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Because it showed up on hello profiles.
Cycle through some moderate-sized sets of peers.
This should cover the "small tweaks to netmap"
and the "up/down cycle" cases.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Yes, it printed, but that was an implementation detail for hashing.
And coming optimization will make it print even less.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Not that it matters, but we were missing a close parens.
It's cheap, so add it.
name old time/op new time/op delta
Hash-8 6.64µs ± 0% 6.67µs ± 1% +0.42% (p=0.008 n=9+10)
name old alloc/op new alloc/op delta
Hash-8 1.54kB ± 0% 1.54kB ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
Hash-8 37.0 ± 0% 37.0 ± 0% ~ (all equal)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The struct field names don't change within a single run,
so they are irrelevant. Use the field index instead.
name old time/op new time/op delta
Hash-8 6.52µs ± 0% 6.64µs ± 0% +1.91% (p=0.000 n=6+9)
name old alloc/op new alloc/op delta
Hash-8 1.67kB ± 0% 1.54kB ± 0% -7.66% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
Hash-8 53.0 ± 0% 37.0 ± 0% -30.19% (p=0.000 n=10+10)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
These show up a lot in our data structures.
name old time/op new time/op delta
Hash-8 11.5µs ± 1% 7.8µs ± 1% -32.17% (p=0.000 n=10+10)
name old alloc/op new alloc/op delta
Hash-8 1.98kB ± 0% 1.67kB ± 0% -15.73% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
Hash-8 82.0 ± 0% 53.0 ± 0% -35.37% (p=0.000 n=10+10)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The sha256 hash writer doesn't implement WriteString.
(See https://github.com/golang/go/issues/38776.)
As a consequence, we end up converting many strings to []byte.
Wrapping a bufio.Writer around the hash writer lets us
avoid these conversions by using WriteString.
Using a bufio.Writer is, perhaps surprisingly, almost as cheap as using unsafe.
The reason is that the sha256 writer does internal buffering,
but doesn't do any when handed larger writers.
Using a bufio.Writer merely shifts the data copying from one buffer
to a different one.
Using a concrete type for Print and print cuts 10% off of the execution time.
name old time/op new time/op delta
Hash-8 15.3µs ± 0% 11.5µs ± 0% -24.84% (p=0.000 n=10+10)
name old alloc/op new alloc/op delta
Hash-8 2.82kB ± 0% 1.98kB ± 0% -29.57% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
Hash-8 140 ± 0% 82 ± 0% -41.43% (p=0.000 n=10+10)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
deepprint currently accounts for 15% of allocs in tailscaled.
This is a useful benchmark to have.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
On benchmark completion, we shut down the wgengine.
If we happen to poll for status during shutdown,
we get an "engine closing" error.
It doesn't hurt anything; ignore it.
Fixestailscale/corp#1776
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
interfaces.Tailscale only returns an interface if it has at least one Tailscale
IP assigned to it. In the resolved DNS manager, when we're called upon to tear
down DNS config, the interface no longer has IPs.
Instead, look up the interface index on construction and reuse it throughout
the daemon lifecycle.
Fixes#1892.
Signed-off-by: David Anderson <dave@natulte.net>
If nobody is connected to the IPN bus, don't burn CPU & waste
allocations (causing more GC) by encoding netmaps for nobody.
This will notably help hello.ipn.dev.
Updates tailscale/corp#1773
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Without any synchronization here, the "first packet" callback can
be delayed indefinitely, while other work continues.
Since the callback starts the benchmark timer, this could skew results.
Worse, if the benchmark manages to complete before the benchmark timer begins,
it'll cause a data race with the benchmark shutdown performed by package testing.
That is what is reported in #1881.
This is a bit unfortunate, in that it means that users of TrafficGen have
to be careful to keep this callback speedy and lightweight and to avoid deadlocks.
Fixes#1881
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
It is possible to get multiple status callbacks from an Engine.
We need to wait for at least one from each Engine.
Without limiting to one per Engine,
wait.Wait can exit early or can panic due to a negative counter.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This reduces the speed with which these benchmarks exhaust their supply fds.
Not to zero unfortunately, but it's still helpful when doing long runs.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This function accounted for ~1% of all allocs by tailscaled.
It is trivial to improve, so may as well.
name old time/op new time/op delta
KeyMarshalText-8 197ns ± 0% 47ns ± 0% -76.12% (p=0.016 n=4+5)
name old alloc/op new alloc/op delta
KeyMarshalText-8 200B ± 0% 80B ± 0% -60.00% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
KeyMarshalText-8 5.00 ± 0% 1.00 ± 0% -80.00% (p=0.008 n=5+5)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The old way was way too fragile and had felt like it had more special
cases than normal cases. (see #1874, #1860, #1834, etc) It became very
obvious the old algorithm didn't work when we made the output be
pretty and try to show the user the command they need to run in
5ecc7c7200 for #1746)
The new algorithm is to map the prefs (current and new) back to flags
and then compare flags. This nicely handles the OS-specific flags and
the n:1 and 1:n flag:pref cases.
No change in the existing already-massive test suite, except some ordering
differences (the missing items are now sorted), but some new tests are
added for behavior that was broken before. In particular, it now:
* preserves non-pref boolean flags set to false, and preserves exit
node IPs (mapping them back from the ExitNodeID pref, as well as
ExitNodeIP),
* doesn't ignore --advertise-exit-node when doing an EditPrefs call
(#1880)
* doesn't lose the --operator on the non-EditPrefs paths (e.g. with
--force-reauth, or when the backend was not in state Running).
Fixes#1880
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Needed for the "up checker" to map back from exit node stable IDs (the
ipn.Prefs.ExitNodeID) back to an IP address in error messages.
But also previously requested so people can use it to then make API
calls. The upcoming "tailscale admin" subcommand will probably need it
too.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This reverts commit 7d16c8228b.
I have no idea how I ended up here. The bug I was fixing with this change
fails to reproduce on Ubuntu 18.04 now, and this change definitely does
break 20.04, 20.10, and Debian Buster. So, until we can reliably reproduce
the problem this was meant to fix, reverting.
Part of #1875
Signed-off-by: David Anderson <dave@natulte.net>
The --advertise-routes and --advertise-exit-node flags both mutating
one pref is the gift that keeps on giving.
I need to rewrite the this up warning code to first map prefs back to
flag values and then just compare flags instead of comparing prefs,
but this is the minimal fix for now.
This also includes work on the tests, to make them easier to write
(and more accurate), by letting you write the flag args directly and
have that parse into the upArgs/MaskedPrefs directly, the same as the
code, rather than them being possibly out of sync being written by
hand.
Fixes https://twitter.com/EXPbits/status/1390418145047887877
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Fields rename only.
Part of the general effort to make our code agnostic about endpoint formatting.
It's just a name, but it will soon be a misleading one; be more generic.
Do this as a separate commit because it generates a lot of whitespace changes.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Upstream wireguard-go renamed the interface method
from CreateEndpoint to ParseEndpoint.
I missed some comments. Fix them.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
By using conn.NewDefaultBind, this test requires that our endpoints
be comprehensible to wireguard-go. Instead, use a no-op bind that
treats endpoints as opaque strings.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Instead of calling ParseHex, do the hex.Decode directly.
name old time/op new time/op delta
UnmarshalJSON-8 86.9ns ± 0% 42.6ns ± 0% -50.94% (p=0.000 n=15+14)
name old alloc/op new alloc/op delta
UnmarshalJSON-8 128B ± 0% 0B -100.00% (p=0.000 n=15+15)
name old allocs/op new allocs/op delta
UnmarshalJSON-8 2.00 ± 0% 0.00 -100.00% (p=0.000 n=15+15)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Legacy endpoints (addrSet) currently reconstruct their dst string when requested.
Instead, store the dst string we were given to begin with.
In addition to being simpler and cheaper, this makes less code
aware of how to interpret endpoint strings.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
