stdout/stderr around
A lot of functions in the cmd and ctl packages were passing these
around and barely using them. Replaced them with a logger for most
functions. Some functions get an io.Writer instead so that their
tests can find the output they're looking for.
More cleanup on fb-1766: reworked the tests that were using io.Pipe
or os.Pipe to check their results so they now use a bytes.Buffer.
Unexported some variables that didn't need to be exported.
Fixed NewConfigCommand to use the provided stderr, not os.Stderr.
Added tests for rbf_dump, rbf_page, and keygen, since those weren't
being tested at all.
Added chksum_test, final cleanup.
Cobra automatically displays usage messages, and also a gratuitous
"Error: [...]" line in some cases, when any error at all occurs
running a command. To suppress the usage message, you have to set
cmd.SilenceUsage to true. But the code that would do this doesn't
have access to it. To address this, we introduce a category of
"usage error", implemented with stdlib error wrapping (%w) and
use errors.Is to check for it. There's also utility functions
to do this checking automatically, or indeed, to handle wrapping
of the ctl.SomethingCommand and handle running it with a suitable
context and everything.
In fact, several of the places we're checking for usage errors,
we can never actually report one, but we're checking consistently
so that if we want to report usage errors, we can.
For instance, server.Start and (dax)server.Start don't ever
return usage errors, right now, but we're checking their responses
anyway.
This fixes the OriginalIP and RequestUserID in the main featurebase
package, and the Access and Refresh tokens, the UserInfo, and the
[]string of Indexes passed with context.Context(s) in the authn package.
An empty struct was used for all of these keys (and relevant helper
functions we added) to avoid allocations where possible while still
using the context functionality.
Some of the logic in the server.GetIndexes function was fixed.
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.
* Add refresh token header/cookie
As part of work on automatic refreshing of access tokens in the grafana plugin
(FB-1377), we will now accept a refresh token in the "X-Molecula-Refresh-Token"
header or the "refresh-molecula-chip" cookie.
This refresh token will be used if the access token is expired. To achieve this,
there was a lot of plumbing that had to be done. Here is a list of some of it:
* Added lots of constants for the new values.
* Removed token cache, since we will be keeping state on the clients.
* We now only refresh tokens when they are expired, which is more inline with the
OAuth spec.
* Refactored SetGRPCMetadata to be simpler to read.
* Refactored AddAuthToken.
* Update failing tests.
* We now don't split GRPC cookies on ";". Not sure why we did that before tbh.
I also added TODOs to add the refresh token to other subcommands. This is out of
scope for my current ticket, but it would be nice to have in the future.
* remove unnecessary context from Authenticate
* Add comments on why we check both cases for headers
It's because some GRPC clients lowercase metadata names. I've run into issues with
this enough that I think it's worth the extra checks. We prefer lowercase though,
because that's "standard".
* Fix test that broke during rebase
- Add auth-token for featurebase import, backup and restore
- Add auth-token to http request
- Create a cluster tests with auth enabled
- Add test for import with auth enabled
A few things were going wrong here.
First, we take a "RetryPeriod" option on backup and restore which is
meant to be roughly the total amount of time we spend retrying any
given request before failing. However we were incorrectly passing that
as the RetryMaxWait which is the maximum amount of time to sleep
between any two attempts. We now do some fuzzy math to figure out
approximately how many attempts we should make given a minimum sleep
of 100ms and the fact that we double the sleep time every attempt.
Second, during the backup test, if a host was totally stopped when we
started the request, it would fail immediately and then retry, but if
the host was stopped during the request (after DNS had resolved), then
the request would wait for the DialTimeout which we default to 30s, so
turning off the cluster for 5 seconds and turning it back on resulted
in the backup completing rather than failing. Because of this, we
change the commandClient to have a default dial timeout of 1 second.
I was tempted to change the global default to 1s which I think would
be fine, but didn't want to break anything too badly.
instead of awkwardly reading an entire file into a buffer, we use
retryablehttp's reader func to open the file fresh if we need to
retry, so a small fixed-size buffer can be used internally for copying
the contents onto the network.
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
```
Previously, multiple frames with different prefixes were used to separate
different data layouts. This included separating standard row/column
layouts from inverted column/row layouts as well as storing aggregate
information for timestamp data.
Unfortunately, this caused frame meta data to be copied between multiple
frames and it made it difficult to keep these frames in sync.
This commit separates these different physical layouts into `Views`.
A `Frame` now has one or more views which represent each layout.
Fragments have been moved from under the `Frame` to be contained
within the `View`.
There are two primary views:
- `standard`
- `inverse`
If a frame has a time quantum, then views are generated for these
each of the standard/inverse views. For example a time quantum
of `YMDH` for the date `2000-01-02T00:00:00Z` would create the
following views:
- `standard_2000`
- `inverse_2000`
- `standard_200001`
- `inverse_200001`
- `standard_20000102`
- `inverse_20000102`
From the user's perspective, nothing should change in PQL. Different
PQL statements will handle the appropriate view automatically. For
example, `Bitmap()` and `Profile()` will fetch using the `standard`
view or the `inverse` view, respectively. The `Range()` statement
will lookup the appropriate time-based views automatically.
