featurebase/etcd/leasedkv.go
Seebs b3a4e52a13 simplify, streamline, and possibly debug embedded etcd
The root problem this is attempting to address is sporadic
weird cases in which etcd mistakenly thinks it's down even when
it's up. I am not confident that this is addressed, but there's
a reasonable chance that it is, and I can't trigger it at the
moment, but it was always sporadic, so that doesn't prove much.

There's a lot going on here, and it comes into roughly three
categories.

First: Dropping unused/unneeded code. There's a lot of leftover
bits from the initial development and refactoring of this.

Second: Unifying and shuffling some of the design. We had
multiple interfaces which are functionally impossible to
usefully implement separately, so they're combined together,
and in some cases, moved.

Third: Streamlining logic and simplifying design choices.

This is combined into one commit because the changes are
thoroughly entertwined with each other and you can't usefully
break most of them out.

Also, a bunch of test coverage for most of these changes.

Big changes:

We merge the topology and disco packages.  The topology and disco
packages being separate creates a complicated tangle of problems
and dependencies.  The fundamental problem, approximately, is that
topology.Node has to track disco.NodeState.

There's three core interfaces interacting here:
	topology.Noder (maintains list of nodes)
	disco.Stator (maintains the state of a node)
	disco.Metadator (stores, possibly retrieves, node metadata)
But the node state mantained by the Noder *is* the set of node
metadata, plus state updates produced by Stators. The only actual
non-trivial and usable implementation of these interfaces is a single
thing which implements all three, and in which the implementations
share a single backend data source which they are all modifying.

But you can't move Noder into disco, because Noder has to refer
to topology.Node, but topology.Node refers to disco.

Solution: First, merge these two packages. Second, merge these
three interfaces, to provide a single interface which is more
clear about the fact that (metadator.)SetMetadata() and
(stator.)Started() are both changing the output we'll get from
(noder.)Nodes().

We rework the node state tracking.

We have this nodeStates map which is almost unused. Really, we
don't need it at all. Every node's state is either its last heartbeat
state or "Unknown", so we simplify this a bit. Also, we ensure that
the populateNodeStates function itself is yielding the sorted nodes
list, so we don't have to be as worried about possible later lookups
of sortedNodes happening outside a lock. We also add diagnostics
for deleting nodes from the metadata list (this should never happen),
and try to track heartbeat state more closely.

This is *probably* what fixes the underlying reported problem,
if anything did.

Still an open issue: Make heartbeat state changes aware of when
they're talking about *this* node and possibly not try to
mark it down? Except this may have a flaw: That would result in
each node disagreeing with other nodes in etcd about the state
of that node in the failure cases, and undermine the point of
using etcd to keep these states consistent.

We reduce the number of contexts and cancelfuncs in the etcd wrapper.

We create a shared context for the non-etcd.embed children of our
etcd wrapper, the heartbeat/keepalive and the node watcher, so we
can cancel that one context and cancel all of those at once, so
we don't need to separately track a function to call to cancel
the watch, AND be closing another channel. Also, our shutdown
now propagates automatically to the various etcd API calls we've
made for things like the node watcher and keepalive calls.

We still need to watch that channel in watchNodesOnce, though,
because apparently the watch doesn't yield an error even if the
context calling it is canceled. Whee.

This should reduce the risk of ending up in an inconsistent state,
and also the Close() function is probably idempotent now.

Smaller changes:

* Remove config-generators that existed to generate etcd
  configs but were used only for tests that no longer exist
  or make sense.
* Move the logic to generate etcd configs into the etcd
  package, instead of the "testing" subpackage. This allows
  us to write a self-contained config generator for
  clusters where the nodes know about each other, but do
  this just with etcd, not with full featurebase servers.
* Move the thing generating `fake:%d` socket names into
  the etcd package, which is the only place we use it.
  Also simplify it slightly.
* Don't panic on invalid URLs, report errors from them.
* At least try to use etcd's config.Validate functionality.
  It's underdocumented, so we're not sure what it will report,
  but at least if it does we'll get reports from it and
  know what they are?
* Try to handle CompactRevision errors from watches more
  correctly -- after a CompactRevision, any future attempt
  to watch from a lower revision will necessarily fail, so
  we adjust our target revision up. We don't have good
  testing for this.
* Drop the Metadata() method (that used to be in Metadator)
  because nothing ever used it and it didn't make much sense
  to try.
* Convert SetMetadata from taking an arbitrary json blob
  to taking the only data that would ever be valid since
  we always use it to extract node information anyway.
* Drop several unused functions, unexport things only used
  internally.
* Replace Started() with SetState("STARTED"), allowing us
  to write tests that mess with states. We weren't really thinking
  carefully about state transitions sometimes and now it's much
  easier to do that thinking.
* Stop leaving stray localhost:2380 and localhost:2379 in
  our embed config. We still sometimes see peer requests from
  those and I honestly don't know why, but at least it should
  be rarer.
2022-07-21 11:42:35 -05:00

246 lines
6.5 KiB
Go

// Copyright 2021 Molecula Corp. All rights reserved.
package etcd
import (
"context"
"log"
"sync"
"time"
"github.com/molecula/featurebase/v3/disco"
"github.com/pkg/errors"
clientv3 "go.etcd.io/etcd/client/v3"
"go.etcd.io/etcd/client/v3/clientv3util"
)
// leasedKV is an etcd key and value attached to a lease. It can be used to detect if a node went down.
// It will try to renew the lease at any cost after losing it.
// It will recreate the previous existing value for the key again.
type leasedKV struct {
e *Etcd
parentContext context.Context
cancel context.CancelFunc
done <-chan struct{}
leaseID clientv3.LeaseID
key string
ttlSeconds int64
mu sync.Mutex
value string // protected by mu
stopped bool // protected by mu
}
func newLeasedKV(e *Etcd, ctx context.Context, key string, ttlSeconds int64) *leasedKV {
return &leasedKV{
e: e,
parentContext: ctx,
key: key,
ttlSeconds: ttlSeconds,
}
}
// Start creates the key and attaches it to a lease.
// If the lease cannot be renewed in time, it will try to renew it ad finitum.
func (l *leasedKV) Start(initValue string) error {
l.mu.Lock()
defer l.mu.Unlock()
kaChann, err := l.create(initValue)
if err != nil {
return err
}
go l.consumeLease(kaChann)
return nil
}
// create creates the lease and yields a KeepAlive channel. It also stashes a cancel
// function for our local context that we use in case of internal issues, and the
// done channel for the internal context, which will be readable as soon as either
// our context or the parent context is done.
func (l *leasedKV) create(initValue string) (<-chan *clientv3.LeaseKeepAliveResponse, error) {
ctx, cancel := context.WithCancel(l.parentContext)
if l.cancel != nil {
l.cancel()
}
l.cancel = cancel
l.done = ctx.Done()
var leaseResp *clientv3.LeaseGrantResponse
err := l.e.retryClient(func(cli *clientv3.Client) (err error) {
leaseResp, err = cli.Grant(ctx, l.ttlSeconds)
return err
})
if err != nil {
return nil, errors.Wrap(err, "creating a lease")
}
l.leaseID = leaseResp.ID
err = l.e.retryClient(func(cli *clientv3.Client) (err error) {
_, err = cli.Txn(ctx).
Then(clientv3.OpPut(l.key, initValue, clientv3.WithLease(l.leaseID))).
Commit()
return err
})
if err != nil {
return nil, errors.Wrapf(err, "creating key %s with value [%s]", l.key, initValue)
}
var kaChan <-chan *clientv3.LeaseKeepAliveResponse
err = l.e.retryClient(func(cli *clientv3.Client) (err error) {
kaChan, err = cli.KeepAlive(ctx, l.leaseID)
return err
})
if err != nil {
return nil, errors.Wrapf(err, "keeping alive the lease for the key %s with value %s", l.key, l.value)
}
l.value = initValue
return kaChan, nil
}
func (l *leasedKV) consumeLease(ch <-chan *clientv3.LeaseKeepAliveResponse) {
for {
select {
case _, ok := <-ch:
if ok {
continue
}
l.mu.Lock()
if l.stopped {
l.mu.Unlock()
return
}
if e := retry("consumeLease", 1*time.Second, func() error {
kaChann, err := l.create(l.value)
if err != nil {
return err
}
go l.consumeLease(kaChann)
return nil
}); e != nil {
log.Printf("lease %q cannot be recreated: %v", l.key, e)
l.mu.Unlock()
return
}
log.Printf("lease %q recreated after a problem", l.key)
l.mu.Unlock()
return
case <-l.done:
// don't recreate lease.
return
}
}
}
// Stop will cancel the lease renewal.
// After calling Stop, this object should be discarded and not used anymore.
func (l *leasedKV) Stop() {
l.mu.Lock()
defer l.mu.Unlock()
l.stopped = true
if l.cancel != nil {
l.cancel()
}
// low-effort attempt to cancel existing lease. if the cluster is
// shutting down, we don't want this to take long. Note that we don't
// use the parent context for this -- if we got cancelled, we still
// want this attempt to run.
ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
err := l.e.retryClient(func(cli *clientv3.Client) (err error) {
_, err = cli.Revoke(ctx, l.leaseID)
return err
})
// if retryClient succeeds, just to be thorough, we'll cancel that
// context.
cancel()
if err != nil {
// It turns out that this will almost always report a
// failure because since we're shutting things down,
// the cluster as a whole may not be able to process responses.
// So this is a low-interest message usually.
l.e.logger.Debugf("revoking lease during shutdown: %v", err)
}
}
// Set will change the specific value for this key.
func (l *leasedKV) Set(ctx context.Context, value string) error {
l.mu.Lock()
defer l.mu.Unlock()
err := l.e.retryClient(func(cli *clientv3.Client) (err error) {
_, err = cli.Txn(ctx).
Then(clientv3.OpPut(l.key, value, clientv3.WithIgnoreLease())).
Commit()
return err
})
// l.e.logger.Printf("set key %q on %q value %q: err %v", l.key, l.e.options.Name, value, err)
if err != nil {
return errors.Wrapf(err, "creating key %s with value [%s]", l.key, l.value)
}
l.value = value
return nil
}
// Get will obtain the actual value for the key.
func (l *leasedKV) Get(ctx context.Context) (string, error) {
l.mu.Lock()
defer l.mu.Unlock()
var getResp *clientv3.TxnResponse
err := l.e.retryClient(func(cli *clientv3.Client) (err error) {
getResp, err = cli.Txn(ctx).
If(clientv3util.KeyExists(l.key)).
Then(clientv3.OpGet(l.key, clientv3.WithIgnoreLease())).
Commit()
return err
})
if err != nil {
return "", errors.Wrapf(err, "getting key %s", l.key)
}
if !getResp.Succeeded || len(getResp.Responses) == 0 {
return "", disco.ErrNoResults
}
l.value = string(getResp.Responses[0].GetResponseRange().Kvs[0].Value)
return l.value, nil
}
// retry retries a function at a given interval until it succeeds, or until it
// returns context.DeadlineExceeded, at which point we return the last other
// error it returned, or DeadlineExceeded if we didn't have another previous
// error. So other errors (connection failures, etcetera) get retried, but
// DeadlineExceeded means we're done trying. But, if we failed due to a
// connection error, then got a DeadlineExceeded on a retry, we want to report
// the connection error, which is a lot more informative.
func retry(desc string, sleep time.Duration, f func() error) (err error) {
for {
lastErr := f()
if lastErr == nil {
return lastErr
}
if errors.Is(lastErr, context.DeadlineExceeded) {
if err != nil {
return err
} else {
return lastErr
}
}
log.Printf("%s: got error %v, retrying", desc, lastErr)
err = lastErr
time.Sleep(sleep)
}
}