[vtctld] Add remaining reparent commands to VtctldServer by ajm188 · Pull Request #7536 · vitessio/vitess

ajm188 · 2021-02-22T20:23:42Z

Description

This PR adds the remaining reparent-related RPCs to VtctldServer, namely:

EmergencyReparentShard
PlannedReparentShard
ReparentTablet
TabletExternallyReparented

~~It includes a cherry-picked version of #7523, which is required to get some ERS smoke tests working. After merging that PR, I'll rebase on master to pull out those changes.~~ ✅

Example usage

EmergencyReparentShard

❯ vtctldclient --server "localhost:15999" GetTablets
zone1-0000000100 commerce 0 master SFO-M-AMASON02:15100 SFO-M-AMASON02:17100 [] 2021-02-22T19:02:23Z
zone1-0000000101 commerce 0 replica SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] <null>
zone1-0000000102 commerce 0 rdonly SFO-M-AMASON02:15102 SFO-M-AMASON02:17102 [] <null>
❯ vtctldclient --server "localhost:15999" EmergencyReparentShard commerce/0
I0222 19:10:24.577677 replication.go:181] getting replication position from zone1-0000000100
I0222 19:10:24.577679 replication.go:181] getting replication position from zone1-0000000102
I0222 19:10:24.577685 replication.go:181] getting replication position from zone1-0000000101
W0222 19:10:24.582681 replication.go:205] failed to get replication status from zone1-0000000100: rpc error: code = Unknown desc = TabletManager.StopReplicationAndGetStatus on zone1-0000000100 error: before status failed: no replication status: before status failed: no replication status
I0222 19:10:24.612392 emergency_reparenter.go:132] promoting tablet zone1-0000000102 to master
I0222 19:10:24.991490 emergency_reparenter.go:172] populating reparent journal on new master zone1-0000000102
I0222 19:10:24.991507 emergency_reparenter.go:178] setting new master on replica zone1-0000000100
I0222 19:10:24.991525 emergency_reparenter.go:178] setting new master on replica zone1-0000000101
❯ vtctldclient --server "localhost:15999" GetTablets
zone1-0000000100 commerce 0 replica SFO-M-AMASON02:15100 SFO-M-AMASON02:17100 [] <null>
zone1-0000000101 commerce 0 replica SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] <null>
zone1-0000000102 commerce 0 master SFO-M-AMASON02:15102 SFO-M-AMASON02:17102 [] 2021-02-22T19:10:24Z

PlannedReparentShard

❯ vtctldclient --server "localhost:15999" GetTablets
zone1-0000000100 commerce 0 master SFO-M-AMASON02:15100 SFO-M-AMASON02:17100 [] 2021-02-22T18:37:43Z
zone1-0000000101 commerce 0 replica SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] <null>
zone1-0000000102 commerce 0 rdonly SFO-M-AMASON02:15102 SFO-M-AMASON02:17102 [] <null>
❯ vtctldclient --server "localhost:15999" PlannedReparentShard commerce/0 --new-primary zone1-0000000101
I0222 18:38:00.244309 planned_reparenter.go:202] checking replication on primary-elect zone1-0000000101
I0222 18:38:00.299035 planned_reparenter.go:238] demoting current primary: zone1-0000000100
I0222 18:38:01.360709 planned_reparenter.go:607] populating reparent journal on new primary zone1-0000000101
I0222 18:38:01.360728 planned_reparenter.go:586] setting new primary on replica zone1-0000000100
I0222 18:38:01.360742 planned_reparenter.go:586] setting new primary on replica zone1-0000000102
❯ vtctldclient --server "localhost:15999" GetTablets
zone1-0000000100 commerce 0 replica SFO-M-AMASON02:15100 SFO-M-AMASON02:17100 [] <null>
zone1-0000000101 commerce 0 master SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] 2021-02-22T18:38:01Z
zone1-0000000102 commerce 0 rdonly SFO-M-AMASON02:15102 SFO-M-AMASON02:17102 [] <null>

Related Issue(s)

Closes Migrate reparent commands to VtctldServer #7428

Checklist

Should this PR be backported? No
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

ajm188 · 2021-02-22T21:00:54Z

Looking into the datarace failure, which appears related to the PRS implementation in VtctldServer.

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

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

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

…ize VtctldServer with tmclient Signed-off-by: Andrew Mason <amason@slack-corp.com>

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

deepthi

Nice work. I have some comments on the UX, the rest lgtm.

deepthi · 2021-02-24T23:46:37Z

go/cmd/vtctldclient/internal/command/reparents.go

 func init() {
+	EmergencyReparentShard.Flags().DurationVar(&emergencyReparentShardOptions.WaitReplicasTimeout, "wait-replicas-timeout", *topo.RemoteOperationTimeout, "Time to wait for replicas to catch up in reparenting.")
+	EmergencyReparentShard.Flags().StringVar(&emergencyReparentShardOptions.NewPrimaryAliasStr, "new-primary", "", "Alias of a tablet that should be the new primary. If not specified, the vtctld will select the best candidate to promote.")
+	EmergencyReparentShard.Flags().StringSliceVarP(&emergencyReparentShardOptions.IgnoreReplicaAliasStrList, "ignore-replicas", "i", nil, "Comma-separated, repeated list of replica tablet aliases to ignore during the emergency reparent.")


Suggested change

EmergencyReparentShard.Flags().StringSliceVarP(&emergencyReparentShardOptions.IgnoreReplicaAliasStrList, "ignore-replicas", "i", nil, "Comma-separated, repeated list of replica tablet aliases to ignore during the emergency reparent.")

EmergencyReparentShard.Flags().StringSliceVarP(&emergencyReparentShardOptions.IgnoreReplicaAliasStrList, "ignore-replicas", "", nil, "Comma-separated, repeated list of replica tablet aliases to ignore during the emergency reparent.")

I assume this is a typo.

Why "Comma-separated, repeated list"? Why not just "Comma-separated list"?

It's repeated in the sense that you can specify the flag multiple times, so both -i "${alias1},${alias2}" and -i ${alias1} -i ${alias2} will work (as well as -i "${alias1},${alias2}" -i ${alias3}). I'm not sure of a concise way to word that, and what I have here is definitely not straightforward, so I'm very open to suggestions!

I get it. Fine for right now, maybe we can revisit later.

deepthi · 2021-02-24T23:48:26Z

go/cmd/vtctldclient/internal/command/reparents.go

+}
+
 func init() {
+	EmergencyReparentShard.Flags().DurationVar(&emergencyReparentShardOptions.WaitReplicasTimeout, "wait-replicas-timeout", *topo.RemoteOperationTimeout, "Time to wait for replicas to catch up in reparenting.")


All the old flags use underscore (_). Is it a cobra convention to use dash (-)?

It's a GNU convention [1] (and therefore more common among most CLIs), so I've been doing it for all the vtctldclient options (except the global action_timeout, which I copied from vtctlclient and for some reason didn't change at the time). Happy to back this out in a separate change (since I'll have to update a bunch of flags) if Vitess prefers to enforce underscores everywhere.

[1]: Stack Overflow, which provides some additional reasoning as well as linking to the GNU style guide: https://stackoverflow.com/a/50537232

I see.. let us follow the convention.

deepthi · 2021-02-24T23:49:51Z

proto/vtctldata.proto

+  // primary. If NewPrimary was set in the request, then this will be the same
+  // alias. Otherwise, it will be the alias of the tablet found to be most
+  // up-to-date.
+  topodata.TabletAlias promoted_primary = 3;


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

ajm188 requested review from deepthi, doeg and rohit-nayak-ps February 22, 2021 20:23

ajm188 requested a review from sougou as a code owner February 22, 2021 20:23

ajm188 force-pushed the am_vtctld_reparent_commands branch from 35cb340 to 2d7c891 Compare February 24, 2021 20:18

Andrew Mason added 15 commits February 24, 2021 15:47

Add reparent RPCs, run generators, add server stubs

8b1f317

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

Implement TabletExternallyReparented, add tests

4e20e14

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

Add CLI entrypoint for TabletExternallyReparented

8fd1a36

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

Implement ReparentTablet with tests

124bdee

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

Add CLI entrypoint for ReparentTablet

a88301e

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

Add more protobuf-related assertion helpers to testutil

5ef0a56

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

Add missing set of NewMaster on the Reparent event after successful…

0f3539c

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

Implement ERS rpc, with tests

f999833

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

Add CLI entrypoint for EmergencyReparentShard

d305e68

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

Run even more tests in parallel, use the safer test helper to initial…

f7408dc

…ize VtctldServer with tmclient Signed-off-by: Andrew Mason <amason@slack-corp.com>

Implement PRS rpc with tests

af7e8ca

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

Add CLI entrypoint for PlannedReparentShard

ac10ae9

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

Fix data races caused by callback logger appends

f21c590

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

Fully fix data race flakes

a1ba1f8

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

Fix types after merge

24e393a

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

ajm188 force-pushed the am_vtctld_reparent_commands branch from 2d7c891 to 24e393a Compare February 24, 2021 20:48

deepthi reviewed Feb 24, 2021

View reviewed changes

Update Use strings for reparent commands

a0b69ef

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

deepthi approved these changes Feb 25, 2021

View reviewed changes

deepthi merged commit d34d951 into vitessio:master Feb 25, 2021

askdba added the Component: Cluster management label Feb 26, 2021

askdba added this to the v10.0 milestone Feb 26, 2021

ajm188 deleted the am_vtctld_reparent_commands branch March 4, 2021 16:32

ajm188 added the Type: Feature label May 23, 2021

ajm188 mentioned this pull request Jul 14, 2021

slack vitess v10.pre tinyspeck/vitess#228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vtctld] Add remaining reparent commands to VtctldServer#7536

[vtctld] Add remaining reparent commands to VtctldServer#7536
deepthi merged 16 commits intovitessio:masterfrom
tinyspeck:am_vtctld_reparent_commands

ajm188 commented Feb 22, 2021 •

edited

Loading

Uh oh!

ajm188 commented Feb 22, 2021

Uh oh!

deepthi left a comment

Uh oh!

deepthi Feb 24, 2021

Uh oh!

deepthi Feb 24, 2021

Uh oh!

ajm188 Feb 25, 2021

Uh oh!

deepthi Feb 25, 2021

Uh oh!

deepthi Feb 24, 2021

Uh oh!

ajm188 Feb 25, 2021

Uh oh!

deepthi Feb 25, 2021

Uh oh!

deepthi Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	EmergencyReparentShard.Flags().StringSliceVarP(&emergencyReparentShardOptions.IgnoreReplicaAliasStrList, "ignore-replicas", "i", nil, "Comma-separated, repeated list of replica tablet aliases to ignore during the emergency reparent.")
	EmergencyReparentShard.Flags().StringSliceVarP(&emergencyReparentShardOptions.IgnoreReplicaAliasStrList, "ignore-replicas", "", nil, "Comma-separated, repeated list of replica tablet aliases to ignore during the emergency reparent.")

Conversation

ajm188 commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example usage

Related Issue(s)

Checklist

Deployment Notes

Impacted Areas in Vitess

Uh oh!

ajm188 commented Feb 22, 2021

Uh oh!

deepthi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajm188 commented Feb 22, 2021 •

edited

Loading