diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index fcca05d62db..3d6761504ca 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -1280,6 +1280,27 @@ func (s *VtctldServer) DeleteTablets(ctx context.Context, req *vtctldatapb.Delet return &vtctldatapb.DeleteTabletsResponse{}, nil } +// fillReparentResponseFromEvent copies keyspace, shard, and promoted primary from a reparent +// event into the caller-supplied fields. When ev.ShardInfo is zero-valued (i.e. +// reparentShardLocked returned before populating it), the caller's default values +// are preserved unchanged. +func fillReparentResponseFromEvent(keyspace, shard *string, promotedPrimary **topodatapb.TabletAlias, ev *events.Reparent) { + if ev == nil || ev.ShardInfo.Shard == nil { + return + } + if k, s := ev.ShardInfo.Keyspace(), ev.ShardInfo.ShardName(); k != "" && s != "" { + if keyspace != nil { + *keyspace = k + } + if shard != nil { + *shard = s + } + } + if promotedPrimary != nil && ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) { + *promotedPrimary = ev.NewPrimary.Alias + } +} + // EmergencyReparentShard is part of the vtctldservicepb.VtctldServer interface. func (s *VtctldServer) EmergencyReparentShard(ctx context.Context, req *vtctldatapb.EmergencyReparentShardRequest) (resp *vtctldatapb.EmergencyReparentShardResponse, err error) { span, ctx := trace.NewSpan(ctx, "VtctldServer.EmergencyReparentShard") @@ -1331,15 +1352,7 @@ func (s *VtctldServer) EmergencyReparentShard(ctx context.Context, req *vtctldat Keyspace: req.Keyspace, Shard: req.Shard, } - - if ev != nil { - resp.Keyspace = ev.ShardInfo.Keyspace() - resp.Shard = ev.ShardInfo.ShardName() - - if ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) { - resp.PromotedPrimary = ev.NewPrimary.Alias - } - } + fillReparentResponseFromEvent(&resp.Keyspace, &resp.Shard, &resp.PromotedPrimary, ev) m.RLock() defer m.RUnlock() @@ -3332,15 +3345,7 @@ func (s *VtctldServer) PlannedReparentShard(ctx context.Context, req *vtctldatap Keyspace: req.Keyspace, Shard: req.Shard, } - - if ev != nil { - resp.Keyspace = ev.ShardInfo.Keyspace() - resp.Shard = ev.ShardInfo.ShardName() - - if ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) { - resp.PromotedPrimary = ev.NewPrimary.Alias - } - } + fillReparentResponseFromEvent(&resp.Keyspace, &resp.Shard, &resp.PromotedPrimary, ev) m.RLock() defer m.RUnlock() diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go index 3386de0d2d4..5e314f9460b 100644 --- a/go/vt/vtctl/grpcvtctldserver/server_test.go +++ b/go/vt/vtctl/grpcvtctldserver/server_test.go @@ -52,6 +52,7 @@ import ( "vitess.io/vitess/go/vt/topo" "vitess.io/vitess/go/vt/topo/memorytopo" "vitess.io/vitess/go/vt/topo/topoproto" + "vitess.io/vitess/go/vt/topotools/events" "vitess.io/vitess/go/vt/vtctl/grpcvtctldserver/testutil" "vitess.io/vitess/go/vt/vtctl/localvtctldclient" "vitess.io/vitess/go/vt/vtctl/schematools" @@ -4670,6 +4671,93 @@ func TestDeleteTablets(t *testing.T) { } } +// TestFillReparentResponseFromEvent exercises fillReparentResponseFromEvent, +// including the nil-pointer guards on each output parameter. +func TestFillReparentResponseFromEvent(t *testing.T) { + t.Parallel() + + makeEv := func(keyspace, shard string, newPrimary *topodatapb.Tablet) *events.Reparent { + ev := &events.Reparent{} + ev.ShardInfo = *topo.NewShardInfo(keyspace, shard, &topodatapb.Shard{}, nil) + ev.NewPrimary = newPrimary + return ev + } + + t.Run("nil ev is a no-op", func(t *testing.T) { + t.Parallel() + k, s := "ks", "-" + var pp *topodatapb.TabletAlias + fillReparentResponseFromEvent(&k, &s, &pp, nil) + assert.Equal(t, "ks", k) + assert.Equal(t, "-", s) + assert.Nil(t, pp) + }) + + t.Run("nil keyspace pointer does not panic", func(t *testing.T) { + t.Parallel() + s := "-" + var pp *topodatapb.TabletAlias + assert.NotPanics(t, func() { + fillReparentResponseFromEvent(nil, &s, &pp, makeEv("ks", "-", nil)) + }) + assert.Equal(t, "-", s) // shard still written + }) + + t.Run("nil shard pointer does not panic", func(t *testing.T) { + t.Parallel() + k := "ks" + var pp *topodatapb.TabletAlias + assert.NotPanics(t, func() { + fillReparentResponseFromEvent(&k, nil, &pp, makeEv("ks", "-", nil)) + }) + assert.Equal(t, "ks", k) // keyspace still written + }) + + t.Run("nil promotedPrimary pointer does not panic", func(t *testing.T) { + t.Parallel() + k, s := "ks", "-" + tablet := &topodatapb.Tablet{Alias: &topodatapb.TabletAlias{Cell: "zone1", Uid: 100}} + assert.NotPanics(t, func() { + fillReparentResponseFromEvent(&k, &s, nil, makeEv("ks", "-", tablet)) + }) + assert.Equal(t, "ks", k) + assert.Equal(t, "-", s) + }) + + t.Run("populates all fields when ev is fully populated", func(t *testing.T) { + t.Parallel() + k, s := "default", "0" + var pp *topodatapb.TabletAlias + tablet := &topodatapb.Tablet{Alias: &topodatapb.TabletAlias{Cell: "zone1", Uid: 100}} + fillReparentResponseFromEvent(&k, &s, &pp, makeEv("ks", "-", tablet)) + assert.Equal(t, "ks", k) + assert.Equal(t, "-", s) + assert.Equal(t, tablet.Alias, pp) + }) + + t.Run("preserves defaults when ShardInfo is zero-valued", func(t *testing.T) { + t.Parallel() + k, s := "ks", "-" + var pp *topodatapb.TabletAlias + fillReparentResponseFromEvent(&k, &s, &pp, &events.Reparent{}) // zero ShardInfo + assert.Equal(t, "ks", k) + assert.Equal(t, "-", s) + assert.Nil(t, pp) + }) + + t.Run("zero ShardInfo early return skips promotedPrimary even if NewPrimary is set", func(t *testing.T) { + t.Parallel() + k, s := "ks", "-" + var pp *topodatapb.TabletAlias + ev := &events.Reparent{} + ev.NewPrimary = &topodatapb.Tablet{Alias: &topodatapb.TabletAlias{Cell: "zone1", Uid: 100}} + fillReparentResponseFromEvent(&k, &s, &pp, ev) + assert.Equal(t, "ks", k) + assert.Equal(t, "-", s) + assert.Nil(t, pp) // early return on nil Shard; NewPrimary not copied + }) +} + func TestEmergencyReparentShard(t *testing.T) { t.Parallel() @@ -4868,6 +4956,57 @@ func TestEmergencyReparentShard(t *testing.T) { } } +// TestEmergencyReparentShardResponsePreservesReqOnEarlyFailure is a regression +// test for the case where reparentShardLocked returns an error before populating +// ev.ShardInfo. In that situation the response must still carry the keyspace/shard +// from the request rather than empty strings. +// +// The test injects a topo Get error on the shard data path (after the shard lock +// is acquired) so that GetShard fails before ev.ShardInfo is assigned. With the +// unfixed code this produces resp.Keyspace == "" and resp.Shard == "". +func TestEmergencyReparentShardResponsePreservesReqOnEarlyFailure(t *testing.T) { + t.Parallel() + + ctx := t.Context() + ts, factory := memorytopo.NewServerAndFactory(ctx, "zone1") + + testutil.AddTablets(ctx, t, ts, &testutil.AddTabletOptions{ + AlsoSetShardPrimary: true, + ForceSetShardPrimary: true, + }, &topodatapb.Tablet{ + Alias: &topodatapb.TabletAlias{ + Cell: "zone1", + Uid: 100, + }, + Type: topodatapb.TabletType_PRIMARY, + PrimaryTermStartTime: &vttime.Time{ + Seconds: 100, + }, + Keyspace: "testkeyspace", + Shard: "-", + }) + + // Inject a failure on shard data reads after the shard is created. This causes + // reparentShardLocked to fail at GetShard before ev.ShardInfo = *shardInfo is + // reached, leaving ev.ShardInfo zero-valued while ev itself is non-nil. + factory.AddOperationError(memorytopo.Get, `.*shards/-/Shard$`, topo.NewError(topo.NoNode, "injected")) + + vtctld := testutil.NewVtctldServerWithTabletManagerClient(t, ts, &testutil.TabletManagerClient{}, func(ts *topo.Server) vtctlservicepb.VtctldServer { + return NewVtctldServer(vtenv.NewTestEnv(), ts) + }) + + resp, err := vtctld.EmergencyReparentShard(ctx, &vtctldatapb.EmergencyReparentShardRequest{ + Keyspace: "testkeyspace", + Shard: "-", + }) + require.Error(t, err) + require.NotNil(t, resp) + testutil.AssertEmergencyReparentShardResponsesEqual(t, &vtctldatapb.EmergencyReparentShardResponse{ + Keyspace: "testkeyspace", + Shard: "-", + }, resp) +} + func TestExecuteFetchAsApp(t *testing.T) { t.Parallel() @@ -9011,6 +9150,46 @@ func TestPlannedReparentShard(t *testing.T) { expectEventsToOccur: true, expectedErr: "global status vars failed", }, + { + // Regression test: when reparentShardLocked returns an error before populating + // ev.ShardInfo (e.g. the ExpectedPrimary check fires before ev.ShardInfo is + // assigned), the response must still carry the keyspace/shard from the request + // rather than empty strings. + name: "response preserves keyspace/shard when reparent fails before ShardInfo is populated", + 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: "-", + }, + }, + tmc: &testutil.TabletManagerClient{}, + req: &vtctldatapb.PlannedReparentShardRequest{ + Keyspace: "testkeyspace", + Shard: "-", + // ExpectedPrimary deliberately points to a non-existent tablet so that + // reparentShardLocked fails the expected-primary check before assigning + // ev.ShardInfo, leaving ev.ShardInfo zero-valued. + ExpectedPrimary: &topodatapb.TabletAlias{ + Cell: "zone1", + Uid: 999, + }, + }, + expected: &vtctldatapb.PlannedReparentShardResponse{ + Keyspace: "testkeyspace", + Shard: "-", + }, + expectEventsToOccur: false, + expectedErr: "primary zone1-0000000100 is not equal to expected alias zone1-0000000999", + }, } for _, tt := range tests { @@ -9041,6 +9220,10 @@ func TestPlannedReparentShard(t *testing.T) { if tt.expectedErr != "" { assert.EqualError(t, err, tt.expectedErr) + if tt.expected != nil { + require.NotNil(t, resp) + testutil.AssertPlannedReparentShardResponsesEqual(t, tt.expected, resp) + } return }