Initial implementation of vtctld service #7128

ajm188 · 2020-12-07T18:32:36Z

Backport

NO

Status

READY

Description

This PR introduces the initial implementation of the vtctld API server, providing the foundation for the rest of the server implementation, and defining the CLI structure for the client binary.

Of note, this is also the first usage of cobra in vitess, which requires some extra care around vitess's flags, which is documented in a comment. Some additional refactoring globally (which I think is complex and large enough to be its own project) would make this way nicer to use (and would also let us solve issues like #4921 with viper easily).

The other thing that I want to explicitly call out is that in addition to migrating GetKeyspaces and GetKeyspace, I introduced a new command, ShowAllKeyspaces, which is the equivalent of running for ks in $(vtctl GetKeyspaces); vtctl GetKeyspace $ks; done. Instead, I changed GetKeyspaces to provide the full keyspace struct for each keyspace in the topo instead of just the keyspace names.

Related Issue(s)

#7058.

This resolves #7133.

Deployment Notes

To deploy these changes:

Server, restart the vtctld component, adding grpc-vtctld to the CSV -service_map argument
Client, deploy the new binary, run as normal (e.g. vtctldclient --server localhost:15999 --help)

Impacted Areas in Vitess

List general components of the application that this PR will affect:

Query Serving
VReplication
Cluster Management
Build (maybe?)

Testing

After spinning up ./examples/local/101_initial_cluster.sh:

bash-3.2$ vtctlclient CreateKeyspace user
bash-3.2$ vtctlclient CreateKeyspace anotherkeyspace
bash-3.2$ vtctldclient --server "localhost:15999" GetKeyspaces
[anotherkeyspace commerce user]
bash-3.2$ vtctldclient --server "localhost:15999" GetKeyspace commerce
name:"commerce" keyspace:<> 
bash-3.2$ vtctldclient --server "localhost:15999" ShowAllKeyspaces
name:"anotherkeyspace" keyspace:<> 
name:"commerce" keyspace:<> 
name:"user" keyspace:<> 
bash-3.2$ # The old commands still work
bash-3.2$ vtctlclient GetKeyspaces
anotherkeyspace
commerce
user
bash-3.2$ vtctlclient GetKeyspace user
{
  "sharding_column_name": "",
  "sharding_column_type": 0,
  "served_froms": [
  ],
  "keyspace_type": 0,
  "base_keyspace": "",
  "snapshot_time": null
}

I've only added some simple keyspace-related getters for now. No documentation or tests or anything yet, this is just a proof-of-concept / example implementation. Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

I very quickly ran into circular imports trying to keep things colocated. Signed-off-by: Andrew Mason <amason@slack-corp.com>

…typo Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

We bring in cobra to manage the CLI for us; it's the most structured one Vitess has had yet. This also forces us to do _something_ about the various project-global flags, but it's a compromise - essentially we just blindly apply all of them to the root of our command tree. Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

It turns out that some vitess packages make calls to `flag.Lookup` to find flag values, and if we swap out the original flagset, these all fail, and in some cases, panic. I encountered this while attempting to move vttablet to cobra in a separate branch, and the panic came from `logutil.PurgeLogs()` needing a `log_dir` flag ([ref][logutil_ref]). [logutil_ref]: https://github.com/vitessio/vitess/blob/20a545533bc94d6f85e1682a6539e93c32a82486/go/vt/logutil/purge.go#L106-L109 While this appears to actually not matter for the vtctldclient, since the code with the potential bug is able to run without issue, I don't want to commit a potentially-risky flag shim into the codebase that may be copied by others. Signed-off-by: Andrew Mason <amason@slack-corp.com>

derekperkins · 2020-12-08T00:15:59Z

proto/vtctlservice.proto

+  // GetKeyspaces returns the names of all keyspaces in the topo.
+  rpc GetKeyspaces(vtctldata.GetKeyspacesRequest) returns (vtctldata.GetKeyspacesResponse) {};


Is GetKeyspaces not returning the actual object because that's not how the existing rpc works? I would vote for this call to return the list of keyspace objects that you've currently created a new RPC for. If we want to be able to restrict the data that is returned, I would prefer to use a view or a fieldmask. https://google.aip.dev/157

Yeah that was originally my reason. What do you think about removing the "just the names" rpc entirely, and having clients do for _, ks := range kss { ks.Name } if they want?

I think the downside to using a fieldmask in this specific case is the "subset" of fields we want is actually a list of scalars, so our return types should either be []string or []*pb.Keyspace, which I'm not sure how to represent nicely in protos.

Agreed, my vote is to return the whole object as []*pb.Keyspace and eliminate the ShowAllKeyspaces rpc

Semi-related, it's weird that the underlying topodata.Keyspace proto doesn't have the name easily accessible, making you wrap that message here.

Yeah I agree. I wonder how controversial of a change it would be to put a name field in the Keyspace and Shard protos or if that would require a more careful migration. If the latter I'd rather stick with this thin wrapper for now and revisit later.

@deepthi do you have any thoughts here?

To the first point, all rpcs take a Request proto and return a Response proto. This allows us to change the contents of the proto between versions without breaking compatibility.
Regarding adding name to the proto objects: AFAICT the logic seems to have been that the name of the object is the filename and the object is a representation of the file contents. This makes perfect sense from the POV of the topo - why duplicate information and why not keep the data small.
As we expand the usage of these proto objects beyond topo calls and start passing them around, it will be nice to not have to wrap them with another object that has the name, so I'm not opposed to this change. We can rely on protobuf compatibility guarantees.
@sougou @enisoc please chime in if you disagree.

derekperkins · 2020-12-08T00:19:31Z

proto/vtctlservice.proto

+  // ShowAllKeyspaces returns the keyspace struct of each keyspace in the topo.
+  rpc ShowAllKeyspaces(vtctldata.ShowAllKeyspacesRequest) returns (stream vtctldata.Keyspace) {};


Is there a compelling reason to return a stream vs just a standard reply/response? Streams are much more difficult to consume and reason about. There's a pretty good discussion about the drawbacks here.
twitchtv/twirp#70 (comment)

There are obviously cases for them, especially inside of Vitess, but if we can avoid them in this server definition, I think we'll be better off.

This was a super good thread, thank you for sharing that! I think it hit the nail right on the head w.r.t. "is there a compelling reason" with

people could easily reach for it too early in order to future-proof their APIs, "just in case we need streams later," and walk themselves into an architectural hole which is very difficult to get out of.

I'll switch it to a unary RPC!

In a separate branch, I was exploring making the streaming collect the keyspaces from the topo and send them to the stream in separate goroutines, and the complexity exploded pretty quickly.

proto/vtctldata.proto

Co-authored-by: Derek Perkins <derek@derekperkins.com> Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

rohit-nayak-ps

lgtm.

I pulled the branch and it works fine. If you fix the go.sum conflict I can merge.

deepthi · 2020-12-09T18:15:08Z

proto/vtctlservice.proto

+// Service Vtctld exposes gRPC endpoints for each vt command.
+service Vtctld {
+  // GetKeyspace reads the given keyspace from the topo and returns it.
+  rpc GetKeyspace(vtctldata.GetKeyspaceRequest) returns (vtctldata.Keyspace) {};


Can you fix this before we merge? Return type should be GetKeyspaceResponse.
The rest looks good 👍

Done! Looking at that conflict now

FWIW, we wrap everything in a response internally like you are suggesting, but I was surprised to see that the current Google recommendations are to return the object itself on a Get https://google.aip.dev/131

oh! that's interesting..
maybe we should adopt that in that case for the new RPCs (they are new after all).
@ajm188 we can merge this PR as-is but revisit the return types.

Yeah I was kind of wondering about that ... I don't think the Response wrappers actually get you any additional compatibility guarantees. If I have a response like Response: {MyObject: {Field1, Field2}} vs MyObject: {Field1, Field2}, if I need to remove Field2, either way it's going to break the client, and if I want to add a field, either way it's optional to the client and goes unnoticed.

Anyway, happy to revisit this. Right now trying to figure out why after merging master and running go mod tidy I'm seeing:

❯ golangci-lint run WARN [runner] Can't run linter goanalysis_metalinter: bodyclose: failed prerequisites: [buildssa@vitess.io/vitess/go/test/endtoend/onlineddl [vitess.io/vitess/go/test/endtoend/onlineddl.test]: analysis skipped: errors in package: [/Users/amason/work/vitess/go/test/endtoend/onlineddl/onlineddl_test.go:209:9: undeclared name: testAlterTable]] ERRO Running error: bodyclose: failed prerequisites: [buildssa@vitess.io/vitess/go/test/endtoend/onlineddl [vitess.io/vitess/go/test/endtoend/onlineddl.test]: analysis skipped: errors in package: [/Users/amason/work/vitess/go/test/endtoend/onlineddl/onlineddl_test.go:209:9: undeclared name: testAlterTable]] ERRO Timeout exceeded: try increasing it by passing --timeout option

Ah, it looks like this PR missed one rename

Fixed in #7148

…sites and tests Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Review has been addressed

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Initial implementation of vtctld service

ajm188 added 10 commits December 5, 2020 14:10

First stab at a typed vtctld interface

d0547a0

I've only added some simple keyspace-related getters for now. No documentation or tests or anything yet, this is just a proof-of-concept / example implementation. Signed-off-by: Andrew Mason <amason@slack-corp.com>

Very quick example refactor of the original functionality

741f876

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Move new vtctld to a new package

9358bf9

I very quickly ran into circular imports trying to keep things colocated. Signed-off-by: Andrew Mason <amason@slack-corp.com>

Add ability to install vtctld gRPC server, update local example, fix …

4fc2514

…typo Signed-off-by: Andrew Mason <amason@slack-corp.com>

Rough WIP of the vtctld clients

a09056c

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Install vtctldclient with the rest of the client binaries

3b5f21e

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Add tests

639d2f7

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Add docs to the proto service and regenerate

3c7fdf3

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 requested review from deepthi, doeg and rohit-nayak-ps December 7, 2020 18:32

ajm188 requested a review from sougou as a code owner December 7, 2020 18:32

derekperkins reviewed Dec 8, 2020

View reviewed changes

proto/vtctldata.proto Outdated Show resolved Hide resolved

ajm188 and others added 2 commits December 8, 2020 08:06

Update proto/vtctldata.proto

37fd703

Co-authored-by: Derek Perkins <derek@derekperkins.com> Signed-off-by: Andrew Mason <amason@slack-corp.com>

Update implementation for new proto

9064b14

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 force-pushed the am_vtctld_proto branch from 0b418ef to 9064b14 Compare December 8, 2020 13:19

Merge branch 'vitessio:master' into am_vtctld_proto

ad94b67

Signed-off-by: Andrew Mason <amason@slack-corp.com>

rohit-nayak-ps approved these changes Dec 9, 2020

View reviewed changes

deepthi previously requested changes Dec 9, 2020

View reviewed changes

ajm188 added 2 commits December 9, 2020 13:58

Define GetKeyspaceResponse, put topodata.Keyspace inside, update call…

025823a

…sites and tests Signed-off-by: Andrew Mason <amason@slack-corp.com>

Merge branch 'vitessio:master' into am_vtctld_proto

b482cca

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Run go mod tidy once more

30dfa30

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 force-pushed the am_vtctld_proto branch from 323f345 to 30dfa30 Compare December 9, 2020 19:23

ajm188 added 2 commits December 9, 2020 15:09

Merge branch 'vitessio:master' into am_vtctld_proto

27237bf

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Update vtctl command to handle nested proto response

d4a8812

Signed-off-by: Andrew Mason <amason@slack-corp.com>

deepthi merged commit cb7d74c into vitessio:master Dec 10, 2020

doeg mentioned this pull request Dec 18, 2020

Add @ajm188 + @doeg to CODEOWNERS for vtctld service files #7202

Merged

6 tasks

ajm188 pushed a commit to tinyspeck/vitess that referenced this pull request Jan 10, 2021

Merge pull request vitessio#7128 from tinyspeck/am_vtctld_proto

2f2635c

Initial implementation of vtctld service

This was referenced Jan 10, 2021

Slack vitess 2020.01.11.rc0 tinyspeck/vitess#192

Closed

[vtctld] Fix accidentally-broken legacy vtctl output format #7285

Merged

Slack vitess 2021.01.11 release tinyspeck/vitess#193

Closed

ajm188 deleted the am_vtctld_proto branch January 14, 2021 15:40

setassociative pushed a commit to tinyspeck/vitess that referenced this pull request Mar 11, 2021

Merge pull request vitessio#7128 from tinyspeck/am_vtctld_proto

9000d43

Initial implementation of vtctld service

rafael mentioned this pull request Mar 17, 2021

Slack vitess 9 2021.03.16r0 tinyspeck/vitess#200

Merged

ajm188 added Component: Cluster management Type: Feature labels May 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of vtctld service #7128

Initial implementation of vtctld service #7128

ajm188 commented Dec 7, 2020 •

edited

Loading

derekperkins Dec 8, 2020

ajm188 Dec 8, 2020

derekperkins Dec 8, 2020

derekperkins Dec 8, 2020

ajm188 Dec 8, 2020

deepthi Dec 8, 2020

derekperkins Dec 8, 2020

ajm188 Dec 8, 2020

rohit-nayak-ps left a comment

deepthi Dec 9, 2020 •

edited

Loading

ajm188 Dec 9, 2020

derekperkins Dec 9, 2020

deepthi Dec 9, 2020

ajm188 Dec 9, 2020

ajm188 Dec 9, 2020

ajm188 Dec 9, 2020

		// GetKeyspaces returns the names of all keyspaces in the topo.
		rpc GetKeyspaces(vtctldata.GetKeyspacesRequest) returns (vtctldata.GetKeyspacesResponse) {};

		// ShowAllKeyspaces returns the keyspace struct of each keyspace in the topo.
		rpc ShowAllKeyspaces(vtctldata.ShowAllKeyspacesRequest) returns (stream vtctldata.Keyspace) {};

Initial implementation of vtctld service #7128

Initial implementation of vtctld service #7128

Conversation

ajm188 commented Dec 7, 2020 • edited Loading

Backport

Status

Description

Related Issue(s)

Deployment Notes

Impacted Areas in Vitess

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

deepthi Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 commented Dec 7, 2020 •

edited

Loading

deepthi Dec 9, 2020 •

edited

Loading