[vtctld] Migrate topo management RPCs#7395
Conversation
Signed-off-by: Andrew Mason <amason@slack-corp.com>
… add tests Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…rser from vtctl Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…veKeyspaceCell` Signed-off-by: Andrew Mason <amason@slack-corp.com>
Also some changes to `RemoveShardCell` tests Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…om wrangler Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…d home for it Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
doeg
left a comment
doeg
left a comment
There was a problem hiding this comment.
I'm still reviewing this! Had a couple of preliminary questions to make sure I'm interpreting this vs. the docs correctly. :)
| bool was_dry_run = 3; | ||
| } | ||
|
|
||
| message CreateKeyspaceRequest { |
There was a problem hiding this comment.
Just curious -- CreateKeyspace docs don't mention a few of these params. Do you think this is usual doc drift or are there $reasons they aren't included? (Or am I looking at the wrong thing entirely?)
There was a problem hiding this comment.
Yeah, my broad answer on this is that these are similar to but distinctly not the vtctl commands, and that includes the CLI definitions (and, those docs don't actually list all the flags you can actually use in the current version). So exact matching is not a requirement, and I figured this was a good opportunity to improve on the existing CLI options rather than just carry them over.
|
|
||
| // ServedFroms specifies a set of db_type:keyspace pairs used to serve | ||
| // traffic for the keyspace. | ||
| repeated topodata.Keyspace.ServedFrom served_froms = 6; |
There was a problem hiding this comment.
On served_froms specifically, the docs list served_from 🤔 are they supposed to be the same?
There was a problem hiding this comment.
I think I can take this one -- the existing docs list the arg as -served_from=tablettype1:ks1,tablettype2:ks2,...
but served_from takes a list and gets mapped into ServedFroms for the actual call if more than one is provided (code ref):
if len(servedFrom) > 0 {
for name, value := range servedFrom {
tt, err := parseServingTabletType3(name)
if err != nil {
return err
}
ki.ServedFroms = append(ki.ServedFroms, &topodatapb.Keyspace_ServedFrom{
TabletType: tt,
Keyspace: value,
})
}
}This is formalizing that CLI behavior into a request object.
Signed-off-by: Andrew Mason <amason@slack-corp.com>
doeg
left a comment
doeg
left a comment
There was a problem hiding this comment.
Ok I read this whole thing and, first of all: this is amazing. An immense amount of work + as usual you are so very thorough.
Standard caveat I am but a golang/Vitess novice, so definitely get a review from a Vitess-knower! (I'm going to comment instead of approve for that reason.) :)
From my perspective though, this is all very approachable and easy to follow. Your comments + tests are 🏆 so good. 🏆 It is a pleasure to learn from you. 😎
| func commandCreateKeyspace(cmd *cobra.Command, args []string) error { | ||
| name := cmd.Flags().Arg(0) | ||
|
|
||
| switch topodatapb.KeyspaceType(createKeyspaceOptions.KeyspaceType) { |
There was a problem hiding this comment.
Babby golang question -- is this switch construction equivalent to:
kst := topodatapb.KeyspaceType(createKeyspaceOptions.KeyspaceType)
if (kst != topodatapb.KeyspaceType_NORMAL || kst != topodatapb.KeyspaceType_SNAPSHOT) {
return errors.New("invalid keyspace type passed to --type")
}I like it! It's nice that it avoids the need for a kst variable (as above). If there are other advantages over an if, I'm curious to know them!
There was a problem hiding this comment.
Yep, those are exactly equivalent! I think also you could remove the kst variable and call the function both times, and the compiler should (🤞) be smart enough to simplify the expression and store the result of the first call.
If there are other advantages over an if, I'm curious to know them!
I think it's mainly a style convention! I've come to find it more readable, and I think it's generally preferred in Go to write switch statements over long if-else-if-else chains, which is hinted at, but not explicitly stated in effective go.
| return nil | ||
| } | ||
|
|
||
| // IsPrimaryTablet is a helper function to determine whether the current tablet |
There was a problem hiding this comment.
These comments are so good ❤️
| } | ||
|
|
||
| // KeyRangeStartEqual returns true if right's keyrange start is _after_ left's start | ||
| // KeyRangeStartSmaller returns true if right's keyrange start is _after_ left's start |
There was a problem hiding this comment.
githook linter caught it for me 😀
…tation Signed-off-by: Andrew Mason <amason@slack-corp.com>
setassociative
left a comment
setassociative
left a comment
There was a problem hiding this comment.
I want to sit down with a real monitor and A/B the new implementations from the ones they're based on but I don't expect anything surprising. I had a few nit picks but nothing that is actually concerning or suggests major changes, I think.
|
|
||
| t, err := time.Parse(time.RFC3339, createKeyspaceOptions.SnapshotTimestamp) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
TIOLI: indicate which flag failed to parse time here
| func commandDeleteKeyspace(cmd *cobra.Command, args []string) error { | ||
| ks := cmd.Flags().Arg(0) | ||
|
|
||
| _, err := client.DeleteKeyspace(commandCtx, &vtctldatapb.DeleteKeyspaceRequest{ |
There was a problem hiding this comment.
general pedantry: for these cases where an empty result is returned I can't decide if it makes sense to go ahead and print the empty result so that we don't have to remember to update the client (which we will forget) if they have some payload in the future
There was a problem hiding this comment.
That's a great point. I started down the following compromise:
resp, err := client.DeleteKeyspace(...)
if err != nil {
return fmt.Errorf("...", ks, err)
}
defer func() { fmt.Fprintf(os.Stderr, "Successfully deleted keyspace %v.\n", ks }()
data, err := cli.MarshalJSON(resp)
if err != nil {
return err
}
fmt.Printf("%s\n", data)
return nilWhich produces:
❯ vtctldclient --server "localhost:15999" DeleteKeyspace -r commerce
{
}
Successfully deleted keyspace commerce.
And I kinda wish I got back bytes that let me do something like if len(data) > 0 { fmt.Printf("%s\n", data) } but I'm also not sure how to do that without losing the pretty printing (and also the curly braces for the empty-but-non-null object).
What we could do now that I'm thinking this through is expand cli.MarshalJSON's proto.Message case to protoreflect on the message and return an empty slice if there are no real fields in the message. However. That only works if we're using google.golang.org/protobuf/proto, which provides a protoreflect package that works with the proto.Message type defined in that module, but does not work with the types in github.com/golang/protobuf/proto, because the former is technically a v2 of the latter and they didn't keep backwards compatibility on this one.
So I think either of these has to be Good Enough, but I'm also curious if you have other ideas after reading all this:
- The "Really Hope We Don't Forget" approach. We could also add something to the PR template, but that feels a bit heavy-handed since it will not apply to something like 99% of PRs.
- Accept that these commands will print an empty object over 3 lines (
[]byte("{\n\n}"), plus the newline fromfmt.Printf("%s\n", data))
| // the true primary. This can occur if someone issues a DeleteTablet while | ||
| // the system is in transition (a reparenting event is in progress and parts of | ||
| // the topo have not yet been updated). | ||
| func IsPrimaryTablet(ctx context.Context, ts *topo.Server, ti *topo.TabletInfo) (bool, error) { |
There was a problem hiding this comment.
We should confirm that the method here (walking MasterTermStartTime) is a universally correct way to make this check. How is MTST set / how is it coordinated with the MTST of the shard. When doing PRS/ERS/TER topo changes how are those values updated?
As written i don't see this being problematic since it's only called from deleteTablet but I could imagine it eventually being a private method here for deleteTablet if there are concerns about exposing it more widely. @deepthi is likel the authoritative wourd here.
But, tbh, I think it's probably fine as is and a useful function to have around!
There was a problem hiding this comment.
This is an extraction of isMasterTablet, which now calls this
There was a problem hiding this comment.
@setassociative we went through those scenarios while writing the original version, so it should be fine.
I think the concern about public vs private function is valid, but that can be addressed in the PR that replaces the legacy command implementations with calls to VtctldServer().Foo().
| return nil, err | ||
| } | ||
|
|
||
| changedTablet, _ := s.ts.GetTablet(ctx, req.TabletAlias) |
There was a problem hiding this comment.
I'd at least catch the error and log ... something? getting a Nil back for after tablet on success is a bit weird and we'll want some kind trail if we hit it
| } | ||
|
|
||
| // CreateKeyspace is part of the vtctlservicepb.VtctldServer interface. | ||
| func (s *VtctldServer) CreateKeyspace(ctx context.Context, req *vtctldatapb.CreateKeyspaceRequest) (*vtctldatapb.CreateKeyspaceResponse, error) { |
There was a problem hiding this comment.
I understand these implementations are intended as a leg over from having the implementation tied up in vtctl/vtctl.go::commandCreateKeyspace etc but it feels odd that many of these implementations are so heavily duplicated and could result in drift. How do we plan on preventing that (rapid deprecation+removal of legacy impl is a totally valid answer).
Even so one thing I'd like us to consider is how we might want to detach the command functionality from the transport? E.g. what does it look like if we also wanted a REST API or something that kicked off create keyspace etc. (Viable answer here is : we only ever care about grpc but if that changes the req/resp objects provide a clear way to introduce a layer of indirection so we don't have to duplicate code again).
There was a problem hiding this comment.
but it feels odd that many of these implementations are so heavily duplicated and could result in drift. How do we plan on preventing that (rapid deprecation+removal of legacy impl is a totally valid answer).
Totally. I've stuck a VtctldServer inside of the wrangler, which powers the legacy implementation. My plan here is to make another pass after this PR and replace all the logic with a call to wr.VtctldServer().CreateKeyspace(...) and so on, and then munge those responses back into the legacy output format.
I didn't do this in this PR mainly because this branch was already gigantic, and it'll let me just focus on making sure I don't accidentally change the legacy output, which I've already done once.
one thing I'd like us to consider is how we might want to detach the command functionality from the transport?
This one is really interesting. My answer for the legacy CLI is still to deprecate+remove. But, more abstractly about the REST case:
I'm not sure how we can guarantee client/server type safety without adding something like jsonschema to the mix, but ignoring that, I would probably use the grpc api as the de facto standard, and add adapters for REST/whatever, so we would:
- Add a global flag to
vtctldclientthat says which transport to use. Add any extra flags needed to set up those transports (like thegrpc_static_auth_credsflag for the grpc transport) - Create
package httpvtctldclientin the same style aspackage grpcvtctldclient. This is where we'll translate the proto message objects into an HTTP request, and then the HTTP response back into a proto message. Similar for any other transports we want to support. - In
Root.PersistentPreRunE, instantiate the correct client (grpcvtctldclient.New(...),httpvtctldclient.New(...), etc), everything about the individual command implementations is unchanged.
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
rohit-nayak-ps
left a comment
rohit-nayak-ps
left a comment
There was a problem hiding this comment.
lgtm
a couple of trivial nits. will merge once addressed
go/cmd/vtctldclient/cli/pflag.go
Outdated
| topodatapb "vitess.io/vitess/go/vt/proto/topodata" | ||
| ) | ||
|
|
||
| // StringMapValue augements flagutil.StringMapValue so it can be used as a |
|
|
||
| // KeyspaceIDTypeFlag adds the pflag.Value interface to a | ||
| // topodatapb.KeyspaceIdType. | ||
| type KeyspaceIDTypeFlag topodatapb.KeyspaceIdType |
There was a problem hiding this comment.
where is this used? didn't see a reference in this PR
go/cmd/vtctldclient/cli/shards.go
Outdated
| @@ -0,0 +1,43 @@ | |||
| /* | |||
| Copyright 20201 The Vitess Authors. | |||
Signed-off-by: Andrew Mason <amason@slack-corp.com>
[vtctld] Migrate topo management RPCs
[vtctld] Migrate topo management RPCs Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
[vtctld] Migrate topo management RPCs Signed-off-by: Andrew Mason <amason@slack-corp.com>
6 participants
Description
Preface
Okay this is a large PR because I couldn't figure out a good way to add RPCs to the proto file in parallel branches without conflicting with those same other branches; in the future (or I can take an afternoon to restructure commits on this one) I could just add a bunch of RPC definitions that panic when called in a single upfront PR and then implement each in a parallel branch. But anyway, onward!
The branch is structured in a series of paired commits, mostly. It goes essentially like:
Occasionally there's an extra commit or two when I needed to touch code outside of
grpcvtctldserverandcmd/vtctldclient, but you can mostly view the changes one command at a time by looking at those pairs of commits.This PR adds a slew of topo-management RPCs to the vtctld server, namely:
ChangeTabletTypeCreateKeyspaceCreateShardDeleteKeyspaceDeleteShards(in the old vtctl server, this was calledDeleteShardbut it could delete multiple shards)DeleteTablets(same renaming as above)GetShardRemoveKeyspaceCellRemoveShardCellIn addition, I made some refactors to other parts of vitess to facilitate this change, in particular:
KeyspaceIDTypeStringtopackage key, similar totopoproto.TabletAliasStringbut for thetopodatapb.KeyspaceIdTypeenum.KeyspaceTypeStringtopackage topoproto, same as above but for thetopodatapb.KeyspaceTypeenum.package keyhad helpers for some of the stuff intopodatapb, instead of it all being intopoproto, but all theKeyspaceIdTypehelpers were there, so that's why I added it topackage keyinstead ofpackage topoproto. Happy to just move it if folks feel otherwise.isMasterTabletfromwranglertotopotools.IsPrimaryTabletso I could reuse the code in both.vtctltopackage topo. I wanted to put this intopoproto, but becausetopoimportstopoproto, and this function needs to importtopo, this would have created a circular import.See it in action
I exercised all the new commands against the local examples, and will include their output here:
ChangeTabletTypeCreateKeyspaceGetShardRemoveShardCellRemoveKeyspaceCellCreateShardDeleteTabletsDeleteShardsDeleteKeyspaceRelated Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: