The default etcd config means that if two of this test run around the
same time, we end up with one of them failing because it can't bind.
Elsewhere, we resolve this by binding to ephemeral ports and fixing
up the config to use them, so we duplicate that here.
This includes duplicating the existing listenerWithURL from test/,
because that package has to import us, so we can't import it, and
I don't really want to make a separate package for one trivial
function.
This should always be etcdserver.ErrLeaderChanged, but actually
apparently it's not always:
non-retryable error: etcdserver: leader changed
The "non-retryable" comes from our code. The "leader changed"
message appears to come from etcdserver, but there appear to be
circumstances where it has a suffix, or it could get wrapped,
so we check for the string being contained in an error. This is
not pretty.
we had a CI job fail in an interesting way, but can't tell if the
etcd retrying stuff is working, so adding in this wrapping so we can
better differentiate the errors if we see it again.
Job is here: https://gitlab.com/molecula/featurebase/-/jobs/1977060827
Failure is:
```
=== RUN TestClusterStuff
cluster_test.go:36: creating index: against http://pilosa2:10101/index/testidx 404 Not Found: 'creating index: sending CreateIndex message: executing request: against http://pilosa3:10101/internal/cluster/message 500 Internal Server Error: 'processing message: getting index: testidx: etcdserver: request timed out
''
--- FAIL: TestClusterStuff (8.85s)
```
This tries to be more correct/careful about retries (checking against
the actual exported errors from etcdserver, not just the string
representations), and also supports retrying on timeouts, not just
on client changes. It can also retry more than once, mostly in case
we hit one of each of those.
For timeout errors, we mostly use the fact that it's a timeout to
give us a reasonable backoff, but then delay a fraction of a second
longer just to give it a moment to recover if the ErrTimeout is
masking something else that took longer.
i used this script, a little clunky but it got the job done
```bash
for file in `find . -type f -print | grep '\.go'`; do
sed '1,/^\/\/ limitations under the License.$/d' $file > $file.tmp;
result=`cat $file.tmp`
if [[ result != "" ]]; then
gofmt $file.tmp &> /dev/null;
if [[ $? == 0 ]]; then
mv $file.tmp $file && gofmt -w $file;
else
rm $file.tmp;
fi
else
rm $file.tmp;
fi
done
```
We disable fsync more consistently in testing, including using
etcd's already-existing UnsafeNoFsync option to disable fsyncs
in the backing store boltdb used by etcd, to reduce runtime of
our tests on MacOS significantly.
Corresponding to this, we update etcd by one patch to pick
up a locally-invented patch which turns out to be nearly-identical
to the upstream fix for "disabling fsync makes boltdb not
even bother to write some data sometimes", which caused crashes
galore.
If we explicitly shut a node down, we don't want everyone else
thinking it's up for the next 5 seconds. Worse, in CI, we have random
long delays (10+ seconds) with no CPU activity at all, so we have to
set the TTL longer there. Which makes any test checking for responsive
detection of a node going down take even longer. So! We revoke
leases on our way down, and this makes the tests not take so
long, and allows us to have a reasonable timeout on the test, while
having a completely unreasonable HeartbeatTTL to make CI stop
breaking randomly.
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.
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.
This is a significant overhaul! Quite a lot of things changed here.
Basically: Prior to this, every request for data from etcd implies
requesting the current live data from etcd, and then unpacking it or
extracting it in some way. This is expensive, which is why we have
a cache in front of it.
We don't need to do that! We can use a Watch, which notifies us
of changes as changes happen. However, there's some challenges and
difficulties along the way, and there's a couple of other changes
which are included here because it's a pain to try to separate them
out.
1. We require a logger to be provided to create our internal Etcd
wrapper. We then use that logger, instead of `fmt.Printf`. This makes
debugging messages work better, and also diagnostics, and so on.
2. The internal client that we are reusing can enter a failed state
after a leader election, in which case we have to recreate the client
to have a working client. We add a new internal-use method,
`retryClient`, which wraps a function which takes an etcd client
and returns an error, and checks for leader-election type errors
and retries creating the client when they happen. That last bit
has not been successfully tested because it's actually really hard
to trigger this now. (Because it was related in part to the
amount of etcd traffic we were producing, which is reduced.)
3. The general swap over from looking things up to unpacking things
as they come in, then returning those already-unpacked things when
we get requests.
With this change, *many tests will fail*. That is addressed by
a separate commit which addresses the secondary problem, which is
that some of our test harness code was relying on the assumption
that if any node in a cluster thinks the cluster is up, every node
will. That was usually true when we were doing everything as
expensive fully-synchronized cluster checks, but becomes significantly
less reliably true in real-world cases where nodes are also
going down sometimes, or nodes are going up and down unexpectedly.
The new etcd implementation has internal caching-like behavior which is
much more reliable (it doesn't use a TTL, it just updates when there's updates
to process) so we don't need this cache.
There's some loose ends here because really we probably want to be
using the top-level server logger, and we should fix that, but in the
mean time, let's not swallow the errors as much, because the last
line printed doesn't actually show what the error was, but it could.
To do this, we distinguish between the current error (which might
be a wrapper around DeadlineExceeded) and a previous error which
we might prefer to return, if one exists, since it's more likely
the "real" cause.
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. :)
