A nil Qcx is a crime against existence and makes baby pandas cry.
Having taken out the hack that tried to accommodate this when tests did it,
we now have to fix the tests. Oh no.
After continuing to see weird test failures, did some more careful testing,
discovered that CI can pause a machine entirely for up to 29 seconds
or so very rarely, and 5-10 seconds quite frequently, which causes
cascading heartbeat failures and so on. Remove those.
It turns out that it's desireable to be able to configure the bootstrap
timeout for etcd, because during startup, we end up delaying that long
(N-1) times in series during each cluster creation, which is pointless
when we're starting the whole cluster. Reduces test runtime by several
minutes.
Attributes are unmaintained and unused.
They have become more of a liability than a benefit.
This change eliminates them from the codebase.
The only user-visible change (assuming that attrs are not used) is that the attrs field will no longer appear in row JSON.
CI systems sometimes hiccup for five seconds, which causes heartbeat leases
to fail and breaks all sorts of things. As a workaround, update heartbeat
TTL for tests only. This might in turn cause different failures to do
with leader elections, but in theory those should be handled now?
A while back we started just polling the reported cluster state of one node
when starting a cluster for tests. This works fine if we're doing fresh
new etcd queries for every single operation -- but that's insanely
expensive, it turns out.
When we use the watcher, some nodes will report stale data for "a
while", where "a while" appears to be easily a couple dozen milliseconds.
This is probably irrelevant in most real-world cases, because the common
case (detecting a node going down) means that we have at least five
seconds after a node goes down before etcd notices the lease expiring,
and a few milliseconds more or less won't matter.
But we have tests that assume either that node 0 is always the
coordinator (wrong) or that waiting for node 0 to think the cluster
is up means that every node in the cluster thinks the cluster is up,
or at least that it means that the coordinator thinks the cluster is
up. We retried later operations but not the initial ones against
the coordinator.
In fact, we probably want to wait for the entire cluster to think
it's up before we start trying things on clusters.
We also replace the "CheckClusterState" function with the existing
AwaitState call, or a new AssertState which errors out since that's
the way we usually use AwaitState anyway.
In the AwaitPrimaryState function, which used to be
AwaitCoordinatorState in a different long-lost revision, we have
to delay until a primary node is available, or fail if one does
not become available, to avoid a panic. This probably shouldn't
happen anymore, because of the last change:
Also, rovide dummy topology.Node entries before metadata is read.
During initial startup, we want to be able to do things like determine
which node is the primary, even before we've read metadata from them.
To do this, we populate the node list with dummy entries that just have
the ID (the only part we need to sort our list), and a node state of
UNKNOWN.
This breaks the fancy logic for determining whether or not to update
the node data, because the initial status of UNKNOWN matches what we
get from SetMetadata giving us new data so we end up not realizing
that this was actually a meaningful change. But actually, that's
a pretty niche optimization; we usually only get state changes when
there's an actual change in state. The updates here are cheap
and only happen after a write (or on the first query) so it's not
worth making the logic a lot fancier to make it work, when we can
just do the simple thing and update any time the dirty flag is set.
We also standardize on a 50ms delay, because 1ms delays were
really expensive when each check was hitting etcd multiple times,
and 50ms is Usually Long Enough.
In nearly all cases, we can just switch ioutil.TempDir->testhook.TempDir
and similarly for TempFile. There's one case where we can't because we
need files to be removed before tests are over.
Also in the process give identifiable names to a lot of temporary files
and make sure they're being cleaned up, and don't use "/tmp/foo" as a
file name in a test that could be running in more than one test process
at once. :)
etcd runs a LOT more goroutines during server startup. Fix a
goroutine/for loop bug causing us to run four 7-node clusters
instead of 1/3/4/7-node clusters, also have the test/cluster
code reduce import workers. We can't do much about the spamminess
of the Raft stuff, but this should tone it down some.
Long story short: Once we create a server and start it, we can't start
it again. We can't close it and restart it, and we can't just start
it without closing it.
Unfortunately, if the server's config needs to change, we have a Problem
here.
This ultimately means that the retry logic for GetListeners can't actually
retry successfully; if we fail on the first attempt, we necessarily fail
on any later attempts also, and if we try to fix that, we get panics.
But!
We don't actually NEED to retry. We just need to ensure that we can
open a :0 port, extract the actual port number, and use that in places
where the port number mattered, without having to rebind it.
The only actual place we needed to rebind things was opening gRPC
servers, so we introduce a gRPC Listener that can be used instead of
trying to bind to a specified port.
In a bunch of other cases where we had similar logic to try to allocate
and then use a port, we can switch to just using a provided listener.
For instance, net/http has `Serve(net.Listener, handler)`, not just
ListenAndServe(addr, handler).
This should eliminate the weird CI failures from eaddrinuse.
NOT fixed: server/cluster_test.go/TestClusterResize_AddNode isn't working
right now. The new node isn't actually being added to the existing cluster.
I attempted this but was outsmarted by it, and I think fixing the
rest of this is worth it as a separate thing.
