[e2e] vtctld init tablet and some output-based commands #15297

ajm188 · 2024-02-20T14:00:01Z

Description

This PR began as an attempt to remove the deprecated InitTablet with the underlying topo call, and then evolved to also migrate some of the output-based commands from vtctlclient to vtctldclient

Related Issue(s)

#15273

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2024-02-20T14:00:04Z

codecov · 2024-02-20T14:22:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.45%. Comparing base (696fe0e) to head (f283cf0).
Report is 69 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15297      +/-   ##
==========================================
- Coverage   67.41%   65.45%   -1.96%     
==========================================
  Files        1560     1562       +2     
  Lines      192752   193911    +1159     
==========================================
- Hits       129952   126933    -3019     
- Misses      62800    66978    +4178

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deepthi

Nice! A couple of little things need fixing, rest LGTM.

go/test/endtoend/cluster/cluster_process.go

deepthi · 2024-02-28T17:22:29Z

go/test/endtoend/cluster/cluster_process.go

@@ -246,6 +245,11 @@ func (cluster *LocalProcessCluster) StartTopo() (err error) {
 		cluster.VtctlProcess.LogDir = cluster.TmpDirectory
 	}

+	if err = cluster.TopoProcess.OpenServer(*topoFlavor, cluster.VtctlProcess.TopoGlobalRoot); err != nil {


Suggested change

if err = cluster.TopoProcess.OpenServer(*topoFlavor, cluster.VtctlProcess.TopoGlobalRoot); err != nil {

if err = cluster.TopoProcess.OpenServer(*topoFlavor, cluster.VtctldClientProcess.TopoGlobalRoot); err != nil {

Does not exist (note the original was VtctlProcess not VtctlclientProcess)

IMO we should replace the vtctl client usage as well with the new vtctldclient equivalent: https://github.com/vitessio/vitess/pull/14315/files

It offers additional testing of it and moves us away from that client as well so that we could potentially remove it as well (assuming we can use vtctldclient --server internal instead of vtctl when appropriate). Without also removing the vtctl binary as well we can't get rid of any of the vtctlclient related code so I think it's important. Unless you think I'm missing something?

go/test/endtoend/cluster/topo_process.go

mattlord

LGTM! I only had some minor comments. Let me know what you think and I'll come back to this quickly.

mattlord · 2024-02-28T17:51:26Z

go/test/endtoend/backup/vtbackup/backup_only_test.go

@@ -167,7 +167,7 @@ func firstBackupTest(t *testing.T, tabletType string) {
 	mysqlctl.CompressionEngineName = "lz4"
 	defer func() { mysqlctl.CompressionEngineName = "pgzip" }()
 	// now bring up the other replica, letting it restore from backup.
-	err = localCluster.VtctlclientProcess.InitTablet(replica2, cell, keyspaceName, hostname, shardName)
+	err = localCluster.InitTablet(replica2, keyspaceName, shardName)


AFAIK InitTablet is deprecated now for some time and thus not implemented in vtctldclient side. Can we take this opportunity to replace its usage? If not, does that mean InitTablet still has a valid use case and should be migrated to vtctldclient as well? Otherwise we're not actually getting rid of the usage, we're just moving it. It does have the nice benefit, however, of only having one place to change later on.

as i understand it, InitTablet as a user command is deprecated and (deliberately) not implemented in the new client, but ~~it's still relevant for creating tablet records without actually starting a tablet process which is useful only for tests~~ I wonder if we could use CreateTablet instead like tabletmanager does?

mattlord · 2024-02-28T17:59:18Z

go/test/endtoend/cluster/cluster_process.go

@@ -246,6 +245,11 @@ func (cluster *LocalProcessCluster) StartTopo() (err error) {
 		cluster.VtctlProcess.LogDir = cluster.TmpDirectory
 	}

+	if err = cluster.TopoProcess.OpenServer(*topoFlavor, cluster.VtctlProcess.TopoGlobalRoot); err != nil {


IMO we should replace the vtctl client usage as well with the new vtctldclient equivalent: https://github.com/vitessio/vitess/pull/14315/files

It offers additional testing of it and moves us away from that client as well so that we could potentially remove it as well (assuming we can use vtctldclient --server internal instead of vtctl when appropriate). Without also removing the vtctl binary as well we can't get rid of any of the vtctlclient related code so I think it's important. Unless you think I'm missing something?

go/test/endtoend/keyspace/keyspace_test.go

go/test/endtoend/tabletmanager/commands_test.go

ajm188 · 2024-02-28T20:13:42Z

idk why it's not letting me reply inline to #15297 (comment) but

Without also removing the vtctl binary as well we can't get rid of any of the vtctlclient related code so I think it's important. Unless you think I'm missing something?

That's absolutely correct. Let me rework the GlobalRoot to set it somewhere in addition to VtctlProcess

Signed-off-by: Andrew Mason <[email protected]>

ajm188 · 2024-03-01T21:31:31Z

@mattlord no rush at all, but this is ready for another review whenever you get some time :)

Signed-off-by: Andrew Mason <[email protected]>

mattlord

LGTM. Thank you for working on this, @ajm188 !

mattlord · 2024-03-06T13:29:23Z

go/test/endtoend/recovery/pitr/shardedpitr_test.go

 		require.NoError(t, err)
+		require.GreaterOrEqual(t, len(resp.Workflows), 1, "responce should have at least one workflow")


Not sure it's even worth a CI run, but response is misspelled.

mattlord · 2024-03-06T13:32:42Z

go/test/endtoend/reparent/utils/utils.go

 	} else {
-		assertNodeCount(t, result, int(2))
+		result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("GetShardReplication", cell1, KeyspaceShard)


We can't use vtctldclient ShardReplicationPositions instead?

mattlord · 2024-03-06T13:33:52Z

go/test/endtoend/tabletgateway/buffer/reshard/sharded_buffer_test.go

 		require.NoError(t, err)
+		require.GreaterOrEqual(t, len(resp.Workflows), 1, "responce should have at least one workflow")


responce creeping around again :-)

mattlord · 2024-03-06T13:35:39Z

go/test/endtoend/tabletgateway/vtgate_test.go

+	err = clusterInstance.VtctldClientProcess.ChangeTabletType(rdOnlyTablet, topodata.TabletType_SPARE)
 	require.NoError(t, err)
 	// Change it back to RDONLY afterward as the cluster is re-used.
 	defer func() {
-		err = clusterInstance.VtctlclientChangeTabletType(rdOnlyTablet, topodata.TabletType_RDONLY)
+		err = clusterInstance.VtctldClientProcess.ExecuteCommand("ChangeTabletType", rdOnlyTablet.Alias, "rdonly")


Curious why we're using .ChangeTabletType() in one place and .ExecuteCommand() in the other.

ajm188 added Type: Internal Cleanup Component: Build/CI Type: Testing labels Feb 20, 2024

github-actions bot added this to the v20.0.0 milestone Feb 20, 2024

ajm188 force-pushed the andrew/e2e-vtctld-init-tablet branch from e0e7152 to 551a156 Compare February 20, 2024 14:03

ajm188 force-pushed the andrew/e2e-vtctld-init-tablet branch 5 times, most recently from 7394191 to ef0aa74 Compare February 28, 2024 13:29

ajm188 marked this pull request as ready for review February 28, 2024 14:05

ajm188 requested a review from deepthi as a code owner February 28, 2024 14:05

ajm188 requested review from dbussink, mattlord and rohit-nayak-ps February 28, 2024 14:05

deepthi approved these changes Feb 28, 2024

View reviewed changes

mattlord reviewed Feb 28, 2024

View reviewed changes

ajm188 mentioned this pull request Feb 29, 2024

Add GetShardReplication to vtctldclient #15386

Closed

Andrew Mason added 12 commits February 29, 2024 10:55

more InitTablet

f88ef3f

Signed-off-by: Andrew Mason <[email protected]>

Replace old GetShardReplication commands with topo call

8923506

Signed-off-by: Andrew Mason <[email protected]>

ShardReplicationPosition e2e

803eba6

Signed-off-by: Andrew Mason <[email protected]>

ExecuteHook e2e

9431514

Signed-off-by: Andrew Mason <[email protected]>

backup output

d6847d9

Signed-off-by: Andrew Mason <[email protected]>

replace ChangeTabletType and GetTablet with new versions

3a58ce4

Signed-off-by: Andrew Mason <[email protected]>

nil out server after close

485daeb

Signed-off-by: Andrew Mason <[email protected]>

do not hardcode topo root

30ec7f7

Signed-off-by: Andrew Mason <[email protected]>

only open the server after we have established a global root

d0e11a8

Signed-off-by: Andrew Mason <[email protected]>

also last ChangeTabletType

bdccfb7

Signed-off-by: Andrew Mason <[email protected]>

re-simplify TopoProcess.Setup

5be7ffd

Signed-off-by: Andrew Mason <[email protected]>

rewrite with vtctldclient command

b1375de

Signed-off-by: Andrew Mason <[email protected]>

ajm188 force-pushed the andrew/e2e-vtctld-init-tablet branch from 80178ad to b1375de Compare February 29, 2024 16:01

Andrew Mason added 6 commits February 29, 2024 12:43

compat shim

a67e136

Signed-off-by: Andrew Mason <[email protected]>

dumb dumb

3c24c36

Signed-off-by: Andrew Mason <[email protected]>

whoops

9ac33e2

Signed-off-by: Andrew Mason <[email protected]>

tidy imports

b3bfa45

Signed-off-by: Andrew Mason <[email protected]>

thread globalroot through a little more cleanly

9728dea

Signed-off-by: Andrew Mason <[email protected]>

move this around

2f8ab0c

Signed-off-by: Andrew Mason <[email protected]>

Andrew Mason added 4 commits March 2, 2024 06:43

more GetSrvKeyspace

eb5acd0

Signed-off-by: Andrew Mason <[email protected]>

update test usage for query execs

ffaa7dc

Signed-off-by: Andrew Mason <[email protected]>

vtctldclient Workflow show output

1059658

Signed-off-by: Andrew Mason <[email protected]>

vtctldclient GetTablets

f283cf0

Signed-off-by: Andrew Mason <[email protected]>

mattlord self-requested a review March 6, 2024 00:25

mattlord approved these changes Mar 6, 2024

View reviewed changes

ajm188 merged commit c75b784 into vitessio:main Mar 6, 2024
102 checks passed

ajm188 deleted the andrew/e2e-vtctld-init-tablet branch March 6, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[e2e] vtctld init tablet and some output-based commands #15297

[e2e] vtctld init tablet and some output-based commands #15297

ajm188 commented Feb 20, 2024 •

edited

Loading

vitess-bot bot commented Feb 20, 2024 •

edited by deepthi

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

deepthi left a comment

deepthi Feb 28, 2024

ajm188 Feb 28, 2024

mattlord Feb 28, 2024

mattlord left a comment

mattlord Feb 28, 2024

ajm188 Feb 28, 2024

mattlord Feb 28, 2024

ajm188 commented Feb 28, 2024

ajm188 commented Mar 1, 2024

mattlord left a comment

mattlord Mar 6, 2024

mattlord Mar 6, 2024

mattlord Mar 6, 2024

mattlord Mar 6, 2024

	if err = cluster.TopoProcess.OpenServer(*topoFlavor, cluster.VtctlProcess.TopoGlobalRoot); err != nil {
	if err = cluster.TopoProcess.OpenServer(*topoFlavor, cluster.VtctldClientProcess.TopoGlobalRoot); err != nil {

		require.NoError(t, err)
		require.GreaterOrEqual(t, len(resp.Workflows), 1, "responce should have at least one workflow")

[e2e] vtctld init tablet and some output-based commands #15297

[e2e] vtctld init tablet and some output-based commands #15297

Conversation

ajm188 commented Feb 20, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Feb 20, 2024 • edited by deepthi Loading

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

deepthi left a 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

mattlord left a 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 Feb 28, 2024

ajm188 commented Mar 1, 2024

mattlord left a 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 Feb 20, 2024 •

edited

Loading

vitess-bot bot commented Feb 20, 2024 •

edited by deepthi

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading