From 0d78a29fff67594312514e9775e9183d6d9e737c Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@users.noreply.github.com> Date: Tue, 5 Dec 2023 11:55:18 +0530 Subject: [PATCH 1/2] Fix Panic in PRS due to a missing nil check (#14656) Signed-off-by: Manan Gupta --- go/vt/vtctl/grpcvtctldserver/server.go | 2 +- go/vt/vtctl/grpcvtctldserver/server_test.go | 75 +++++++++++++++++++-- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index 834abe5bb75..3106e827adb 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -2319,7 +2319,7 @@ func (s *VtctldServer) PlannedReparentShard(ctx context.Context, req *vtctldatap resp.Keyspace = ev.ShardInfo.Keyspace() resp.Shard = ev.ShardInfo.ShardName() - if !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) { + if ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) { resp.PromotedPrimary = ev.NewPrimary.Alias } } diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go index 48b53c53c05..5882dcd9ace 100644 --- a/go/vt/vtctl/grpcvtctldserver/server_test.go +++ b/go/vt/vtctl/grpcvtctldserver/server_test.go @@ -6200,7 +6200,7 @@ func TestPlannedReparentShard(t *testing.T) { req *vtctldatapb.PlannedReparentShardRequest expected *vtctldatapb.PlannedReparentShardResponse expectEventsToOccur bool - shouldErr bool + expectedErr string }{ { name: "successful reparent", @@ -6300,7 +6300,6 @@ func TestPlannedReparentShard(t *testing.T) { }, }, expectEventsToOccur: true, - shouldErr: false, }, { // Note: this is testing the error-handling done in @@ -6316,7 +6315,7 @@ func TestPlannedReparentShard(t *testing.T) { Shard: "-", }, expectEventsToOccur: false, - shouldErr: true, + expectedErr: "node doesn't exist: keyspaces/testkeyspace/shards/-", }, { name: "invalid WaitReplicasTimeout", @@ -6326,7 +6325,71 @@ func TestPlannedReparentShard(t *testing.T) { Nanos: 1, }, }, - shouldErr: true, + expectedErr: "duration: seconds:-1 nanos:1 is out of range for time.Duration", + }, + { + name: "tablet unreachable", + ts: memorytopo.NewServer(ctx, "zone1"), + tablets: []*topodatapb.Tablet{ + { + Alias: &topodatapb.TabletAlias{ + Cell: "zone1", + Uid: 100, + }, + Type: topodatapb.TabletType_PRIMARY, + PrimaryTermStartTime: &vttime.Time{ + Seconds: 100, + }, + Keyspace: "testkeyspace", + Shard: "-", + }, + { + Alias: &topodatapb.TabletAlias{ + Cell: "zone1", + Uid: 200, + }, + Type: topodatapb.TabletType_REPLICA, + Keyspace: "testkeyspace", + Shard: "-", + }, + { + Alias: &topodatapb.TabletAlias{ + Cell: "zone1", + Uid: 101, + }, + Type: topodatapb.TabletType_RDONLY, + Keyspace: "testkeyspace", + Shard: "-", + }, + }, + tmc: &testutil.TabletManagerClient{ + // This is only needed to verify reachability, so empty results are fine. + PrimaryStatusResults: map[string]struct { + Status *replicationdatapb.PrimaryStatus + Error error + }{ + "zone1-0000000200": { + Error: fmt.Errorf("primary status failed"), + }, + "zone1-0000000101": { + Status: &replicationdatapb.PrimaryStatus{}, + }, + "zone1-0000000100": { + Status: &replicationdatapb.PrimaryStatus{}, + }, + }, + }, + req: &vtctldatapb.PlannedReparentShardRequest{ + Keyspace: "testkeyspace", + Shard: "-", + NewPrimary: &topodatapb.TabletAlias{ + Cell: "zone1", + Uid: 200, + }, + WaitReplicasTimeout: protoutil.DurationToProto(time.Millisecond * 10), + }, + expectEventsToOccur: true, + expectedErr: "primary status failed", }, } @@ -6362,8 +6425,8 @@ func TestPlannedReparentShard(t *testing.T) { testutil.AssertLogutilEventsOccurred(t, resp, "expected events to occur during ERS") }() - if tt.shouldErr { - assert.Error(t, err) + if tt.expectedErr != "" { + assert.EqualError(t, err, tt.expectedErr) return } From 9b598884f5e7c4239a322dce3372588d758f972a Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Tue, 5 Dec 2023 12:57:03 +0530 Subject: [PATCH 2/2] test: fix test such that it works in release-17.0 Signed-off-by: Manan Gupta --- go/vt/vtctl/grpcvtctldserver/server_test.go | 34 ++++++++------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go index 5882dcd9ace..ab08eee497b 100644 --- a/go/vt/vtctl/grpcvtctldserver/server_test.go +++ b/go/vt/vtctl/grpcvtctldserver/server_test.go @@ -6328,8 +6328,8 @@ func TestPlannedReparentShard(t *testing.T) { expectedErr: "duration: seconds:-1 nanos:1 is out of range for time.Duration", }, { - name: "tablet unreachable", - ts: memorytopo.NewServer(ctx, "zone1"), + name: "no promotable tablets in the same cell as the primary", + ts: memorytopo.NewServer("zone1", "zone2"), tablets: []*topodatapb.Tablet{ { Alias: &topodatapb.TabletAlias{ @@ -6345,7 +6345,7 @@ func TestPlannedReparentShard(t *testing.T) { }, { Alias: &topodatapb.TabletAlias{ - Cell: "zone1", + Cell: "zone2", Uid: 200, }, Type: topodatapb.TabletType_REPLICA, @@ -6354,7 +6354,7 @@ func TestPlannedReparentShard(t *testing.T) { }, { Alias: &topodatapb.TabletAlias{ - Cell: "zone1", + Cell: "zone2", Uid: 101, }, Type: topodatapb.TabletType_RDONLY, @@ -6363,33 +6363,23 @@ func TestPlannedReparentShard(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - // This is only needed to verify reachability, so empty results are fine. - PrimaryStatusResults: map[string]struct { - Status *replicationdatapb.PrimaryStatus - Error error + PrimaryPositionResults: map[string]struct { + Position string + Error error }{ - "zone1-0000000200": { - Error: fmt.Errorf("primary status failed"), - }, - "zone1-0000000101": { - Status: &replicationdatapb.PrimaryStatus{}, - }, "zone1-0000000100": { - Status: &replicationdatapb.PrimaryStatus{}, + Position: "doesn't matter", + Error: nil, }, }, }, req: &vtctldatapb.PlannedReparentShardRequest{ - Keyspace: "testkeyspace", - Shard: "-", - NewPrimary: &topodatapb.TabletAlias{ - Cell: "zone1", - Uid: 200, - }, + Keyspace: "testkeyspace", + Shard: "-", WaitReplicasTimeout: protoutil.DurationToProto(time.Millisecond * 10), }, expectEventsToOccur: true, - expectedErr: "primary status failed", + expectedErr: "cannot find a tablet to reparent to in the same cell as the current primary", }, }