cli/user,zone,node: run user, zone, node commands through SQL

[tbg]

Most cli commands individually query certain endpoints and compile a result which they then pretty-print (see, for example, status node`.

This is suboptimal because it implies that certain kind of information is only exposed through the cli, and that extra development effort has to be undertaken to make the same status available through SQL when that is requested. Operators are usually able to connect to a cluster using SQL anyway, and using the cli for the same task may be cumbersome.

My proposal is to move the implementation of the user, zone, and node commands to SQL wholesale (so that the cli commands are just small wrappers that issue a SQL query).

Some of their information is already available in SQL and the rest should be, and in particular should be presented in a comparable way. For example, the various possible outputs of ./cockroach node status should be simple joins of more elementary tables in crdb_internal.

Some care has to be taken to make sure that we support returning partial information (#16489). That is, it'd be unfortunate if we joined crdb_internal.pretty_much_always_available_info_from_gossip with crdb_internal.this_only_works_when_cluster_healthy. But these joins are small, and with just a hint of tooling the cli can do these in-memory while leaving blanks. Or, we make these tables itself return partial information, though that raises questions about timeouts, etc.

[tbg]

I would say that once precedent is established, there are some good starter/external contributor projects here.

[petermattis]

I agree for the user and node commands. The zone commands are already mostly small wrappers around SQL queries. How much smaller did you imagine they would be?

[couchand]

We've talked about a similar idea in the context of the admin UI. I think this is a strategy we should be taking for all of our interfaces -- the same information (and commands?) should be available in comparable form in SQL, with the CLI, through a REST API, and in the Admin UI. That way the user can choose to consume it in whatever form is most convenient for a specific purpose, and knowledge on the use of one interface can transfer to any other as well.

[dianasaur323]

Precisely - I think it's about time for us to hash this out. A similar exercise has been proposed to basically go through all the cluster settings we expose, etc, and figure out exactly how they should be exposed and why (I might have proposed this...). Is an RFC in order? I had this on my to do list, but obviously 10,000 other things came first before it. I'm happy to take a first crack, but perhaps someone is better suited to do so?

[tbg]

The zone commands are already mostly small wrappers around SQL queries. How much smaller did you imagine they would be?

I haven't checked their size, so if they're already minimal, great 👍

[tbg]

@dianasaur323 I'd do a small RFC mostly to get to discuss the tables/commands we want to expose and the fields in each, and how we go about versioning them (if at all).

@petermattis yep, zone is already minified. user ditto! That leaves the node family as the single large offender.

The only concrete question I have is whether anyone is opposed to making ./cockroach node status only display gossiped information by default (to make it resilient).

For powering node status, I think the right is to introduce these crdb_internal tables:

• gossip_nodes: reads from Gossip, rows mirroring NodeDescriptor: create table gossip_nodes(id int primary key, address string, attributes string, locality string, server_version string);
• gossip_liveness: same but mirroring Liveness
• kv_node_statuses: calls (*statusServer).Nodes which retrieves the StoreStatuses from the KV store. (I added the kv_ prefix to make it clear that this is a replicated source, not just the executing node's view of the world).

These three should be enough to power (a new) ./cockroach node status. I'd even argue that kv_node_statuses may not be great to use in status (by default) as it's more heavyweight and breaks in unhappy clusters. Note also that we have enough to power node status --decommission: we have the liveness info plus the store status for the replica count. This means we can deprecate (*adminServer).DecommissionStatus which is a hodge-podge of gossip_liveness and kv_node_statuses (for the replica counts).

That leaves adding commands for decommission/recommission. crdb_internal.{r,d}ecommission_node(node_id)?

@couchand I whole-heartedly agree. I think #20712 is a good issue to talk about in the UI context (where most things are table-scoped; something that doesn't apply in the work scoped out here).

[tbg]

@dianasaur323 I'd do a small RFC mostly to get to discuss the tables/commands we want to expose and the fields in each, and how we go about versioning them (if at all).

I wrote this before writing out the rest of the message. Now that I have, I think we should just not commit to versioning at this point since things are likely to be in flux for a bit and nobody has pressed us on it. With that off the table (assuming there's agreement about it), I don't think an RFC is needed though we need to find some way to reduce rot (as new functionality gets introduced).

[tbg]

@dsaveliev would you be up for adding the gossip_{liveness,nodes} tables mentioned above?

Essentially, you need to

1. define a virtual table, similar to this one. You need to get access to a *Gossip (and you do, via planner.session.execCfg.Gossip) and then you can re-use some of your code from [WIP] cli: implement a command to return node's local gossip state #20403.
2. reference it in the right place
3. write basic tests that show that the table can be queried (and has the entries you'd expect).

[dsaveliev]

@tschottdorf yes, I will try to do it.

[dianasaur323]

@tschottdorf gotcha, will hold off on approaching this more holistically until we get further pressured. that being said, let's make sure @jseldess is aware from a documentation point of view?

ah, looking through this a second time, I don't think we need Jesse to do anything. Sorry about that!

[tbg]

Now that we have the crdb_internal.gossip_* tables, I think we should hook up the crdb_internal.kv_node_status table. This is largely analogous to the gossip_ tables but this time, we call into planner.session.execCfg.StatusServer.Nodes.

The only tricky thing really is that in the response, each thing that you want to make a row (NodeStatus) has a lot of nested information, for example the map Activity map[NodeID]NodeStatus_NetworkActivitiy, and there are various other slices, etc.

My idea for that would be to use JSON field types for everything that isn't scalar, but we can check the details when the initial PR is open. I think the schema for the table should be (node_id, address, attrs, ...) as in the below (I went digging through all the data we need to display):

(Desc becomes each of its fields):
	NodeID        int
	Address       string
	Attrs         []string (json)
	Locality      []string (json)
	ServerVersion String
(ditto for BuildInfo):
	GoVersion     string
	Tag           string
	Time          string
	Revision      string
	CgoCompiler   string
	Platform      string
	Distribution  string
	Type          string
	Dependencies  string
StartedAt     timestamp
UpdatedAt     timestamp
Metrics       map[string]float64 (json)
(I'll handle StoreStatus at the end)
Args          []string (json)
Env           []string (json)
Latencies     (omitted because deprecated anyway)
Activity      map[int]struct{incoming, outgoing, latency int} (json)

and finally StoreStatus: There are a few options here.

a) Emit a new row for each entry in the slice. That is, the table will have
multiple entries for each node that has more than one store, i.e. it'll look
like the left join of a hypothetical "node status" table with a "store status"
table.
b) making []StoreStatus all JSON: not my favorite option, as stores are a first-class
citizen in the things you care about, and then we may just make this whole table essentially
document-oriented.
c) splitting StoreStatus out into its own table: I think this is my favorite option. To populate
that table (say crdb_internal.kv_store_status) you would just call Nodes and gather up all of
the []StoreStatuses. A StoreStatus is essentially a StoreDescriptor and metrics:

StoreStatuses []StoreStatus:
	StoreID  int
	Attrs    []string (json); note that this isn't necessarily the same Attrs as above
	Node     use only the NodeID, and make it the first column (before storeid)
	Capacity expand inline
	Metrics map[string]float64 (json)

I suggest doing c) but I think it's fair to just omit StoreStatuses in the first iteration.

[2nishantg]

Is someone working on this issue? I wanted to work on #16489 , but seems like it would be better to start on that after this is done.

[tbg]

@Nishant9 I think this is currently waiting for takers! What's left to do here is to use the crdb_internal.gossip_* and crdb_internal.kv_* tables to populate the output of the node {status,ls} commands (instead of hitting the API endpoints that they hit now). (And a final audit to see that there's nothing else that I missed). Let me know if I can provide additional information.

[2nishantg]

@tschottdorf I think I can pick this up. Can you point me to some file in code where virtual tables are queried( I am assuming there is a better way than using lib/pq to create a postgres connection to cockroach server and then doing the query)?

[knz]

Note that cockroach user and cockroach zone now use SQL already. Only cockroach node doesn't.