diff --git a/go/test/endtoend/tabletmanager/tablet_test.go b/go/test/endtoend/tabletmanager/tablet_test.go index 8e6a1ad5a5e..0ce8cd94276 100644 --- a/go/test/endtoend/tabletmanager/tablet_test.go +++ b/go/test/endtoend/tabletmanager/tablet_test.go @@ -46,11 +46,15 @@ func TestEnsureDB(t *testing.T) { err = clusterInstance.VtctlclientProcess.ExecuteCommand("TabletExternallyReparented", tablet.Alias) require.NoError(t, err) - // It goes SERVING because TER calls ChangeTabletType which will also set the database to read-write - assert.Equal(t, "SERVING", tablet.VttabletProcess.GetTabletStatus()) + // It is still NOT_SERVING because the db is read-only. + assert.Equal(t, "NOT_SERVING", tablet.VttabletProcess.GetTabletStatus()) status := tablet.VttabletProcess.GetStatusDetails() - assert.Contains(t, status, "Serving") + assert.Contains(t, status, "read-only") + // Switch to read-write and verify that that we go serving. + _ = clusterInstance.VtctlclientProcess.ExecuteCommand("SetReadWrite", tablet.Alias) + err = tablet.VttabletProcess.WaitForTabletType("SERVING") + require.NoError(t, err) killTablets(t, tablet) } diff --git a/go/vt/vttablet/tabletmanager/restore.go b/go/vt/vttablet/tabletmanager/restore.go index 4b0cbf1c9df..5e56cb143c3 100644 --- a/go/vt/vttablet/tabletmanager/restore.go +++ b/go/vt/vttablet/tabletmanager/restore.go @@ -130,7 +130,7 @@ func (tm *TabletManager) restoreDataLocked(ctx context.Context, logger logutil.L if originalType == topodatapb.TabletType_MASTER { originalType = tm.baseTabletType } - if err := tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_RESTORE); err != nil { + if err := tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_RESTORE, DBActionNone); err != nil { return err } // Loop until a backup exists, unless we were told to give up immediately. @@ -179,7 +179,7 @@ func (tm *TabletManager) restoreDataLocked(ctx context.Context, logger logutil.L // No-op, starting with empty database. default: // If anything failed, we should reset the original tablet type - if err := tm.tmState.ChangeTabletType(ctx, originalType); err != nil { + if err := tm.tmState.ChangeTabletType(ctx, originalType, DBActionNone); err != nil { log.Errorf("Could not change back to original tablet type %v: %v", originalType, err) } return vterrors.Wrap(err, "Can't restore backup") @@ -195,7 +195,7 @@ func (tm *TabletManager) restoreDataLocked(ctx context.Context, logger logutil.L } // Change type back to original type if we're ok to serve. - return tm.tmState.ChangeTabletType(ctx, originalType) + return tm.tmState.ChangeTabletType(ctx, originalType, DBActionNone) } // restoreToTimeFromBinlog restores to the snapshot time of the keyspace diff --git a/go/vt/vttablet/tabletmanager/rpc_actions.go b/go/vt/vttablet/tabletmanager/rpc_actions.go index 0992d63b4cf..f6eba93f651 100644 --- a/go/vt/vttablet/tabletmanager/rpc_actions.go +++ b/go/vt/vttablet/tabletmanager/rpc_actions.go @@ -33,6 +33,16 @@ import ( vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" ) +// DBAction is used to tell ChangeTabletType whether to call SetReadOnly on change to +// MASTER tablet type +type DBAction int + +// Allowed values for DBAction +const ( + DBActionNone = DBAction(iota) + DBActionSetReadWrite +) + // This file contains the implementations of RPCTM methods. // Major groups of methods are broken out into files named "rpc_*.go". @@ -62,11 +72,11 @@ func (tm *TabletManager) ChangeType(ctx context.Context, tabletType topodatapb.T return err } defer tm.unlock() - return tm.changeTypeLocked(ctx, tabletType) + return tm.changeTypeLocked(ctx, tabletType, DBActionNone) } // ChangeType changes the tablet type -func (tm *TabletManager) changeTypeLocked(ctx context.Context, tabletType topodatapb.TabletType) error { +func (tm *TabletManager) changeTypeLocked(ctx context.Context, tabletType topodatapb.TabletType, action DBAction) error { // We don't want to allow multiple callers to claim a tablet as drained. There is a race that could happen during // horizontal resharding where two vtworkers will try to DRAIN the same tablet. This check prevents that race from // causing errors. @@ -74,7 +84,7 @@ func (tm *TabletManager) changeTypeLocked(ctx context.Context, tabletType topoda return fmt.Errorf("Tablet: %v, is already drained", tm.tabletAlias) } - if err := tm.tmState.ChangeTabletType(ctx, tabletType); err != nil { + if err := tm.tmState.ChangeTabletType(ctx, tabletType, action); err != nil { return err } diff --git a/go/vt/vttablet/tabletmanager/rpc_backup.go b/go/vt/vttablet/tabletmanager/rpc_backup.go index cbb5f179f61..b5666672402 100644 --- a/go/vt/vttablet/tabletmanager/rpc_backup.go +++ b/go/vt/vttablet/tabletmanager/rpc_backup.go @@ -84,7 +84,7 @@ func (tm *TabletManager) Backup(ctx context.Context, concurrency int, logger log } originalType = tablet.Type // update our type to BACKUP - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_BACKUP); err != nil { + if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_BACKUP, DBActionNone); err != nil { return err } } @@ -115,7 +115,7 @@ func (tm *TabletManager) Backup(ctx context.Context, concurrency int, logger log // Change our type back to the original value. // Original type could be master so pass in a real value for masterTermStartTime - if err := tm.changeTypeLocked(bgCtx, originalType); err != nil { + if err := tm.changeTypeLocked(bgCtx, originalType, DBActionNone); err != nil { // failure in changing the topology type is probably worse, // so returning that (we logged the snapshot error anyway) if returnErr != nil { diff --git a/go/vt/vttablet/tabletmanager/rpc_replication.go b/go/vt/vttablet/tabletmanager/rpc_replication.go index 9ca7d535088..8116592a2b2 100644 --- a/go/vt/vttablet/tabletmanager/rpc_replication.go +++ b/go/vt/vttablet/tabletmanager/rpc_replication.go @@ -244,7 +244,7 @@ func (tm *TabletManager) InitMaster(ctx context.Context) (string, error) { // Set the server read-write, from now on we can accept real // client writes. Note that if semi-sync replication is enabled, // we'll still need some replicas to be able to commit transactions. - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_MASTER); err != nil { + if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_MASTER, DBActionSetReadWrite); err != nil { return "", err } @@ -281,7 +281,7 @@ func (tm *TabletManager) InitReplica(ctx context.Context, parent *topodatapb.Tab // is used on the old master when using InitShardMaster with // -force, and the new master is different from the old master. if tm.Tablet().Type == topodatapb.TabletType_MASTER { - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_REPLICA); err != nil { + if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_REPLICA, DBActionNone); err != nil { return err } } @@ -520,7 +520,7 @@ func (tm *TabletManager) setMasterLocked(ctx context.Context, parentAlias *topod // unintentionally change the type of RDONLY tablets tablet := tm.Tablet() if tablet.Type == topodatapb.TabletType_MASTER { - if err := tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_REPLICA); err != nil { + if err := tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_REPLICA, DBActionNone); err != nil { return err } } @@ -622,7 +622,7 @@ func (tm *TabletManager) ReplicaWasRestarted(ctx context.Context, parent *topoda if tablet.Type != topodatapb.TabletType_MASTER { return nil } - return tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_REPLICA) + return tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_REPLICA, DBActionNone) } // StopReplicationAndGetStatus stops MySQL replication, and returns the @@ -733,7 +733,7 @@ func (tm *TabletManager) PromoteReplica(ctx context.Context) (string, error) { return "", err } - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_MASTER); err != nil { + if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_MASTER, DBActionSetReadWrite); err != nil { return "", err } diff --git a/go/vt/vttablet/tabletmanager/tm_state.go b/go/vt/vttablet/tabletmanager/tm_state.go index 80a4d5e05b5..68ba9235c49 100644 --- a/go/vt/vttablet/tabletmanager/tm_state.go +++ b/go/vt/vttablet/tabletmanager/tm_state.go @@ -150,7 +150,7 @@ func (ts *tmState) RefreshFromTopoInfo(ctx context.Context, shardInfo *topo.Shar ts.updateLocked(ctx) } -func (ts *tmState) ChangeTabletType(ctx context.Context, tabletType topodatapb.TabletType) error { +func (ts *tmState) ChangeTabletType(ctx context.Context, tabletType topodatapb.TabletType, action DBAction) error { ts.mu.Lock() defer ts.mu.Unlock() log.Infof("Changing Tablet Type: %v", tabletType) @@ -163,10 +163,12 @@ func (ts *tmState) ChangeTabletType(ctx context.Context, tabletType topodatapb.T if err != nil { return err } - // We call SetReadOnly only after the topo has been updated to avoid - // situations where two tablets are master at the DB level but not at the vitess level - if err := ts.tm.MysqlDaemon.SetReadOnly(false); err != nil { - return err + if action == DBActionSetReadWrite { + // We call SetReadOnly only after the topo has been updated to avoid + // situations where two tablets are master at the DB level but not at the vitess level + if err := ts.tm.MysqlDaemon.SetReadOnly(false); err != nil { + return err + } } ts.tablet.Type = tabletType diff --git a/go/vt/vttablet/tabletmanager/tm_state_test.go b/go/vt/vttablet/tabletmanager/tm_state_test.go index 680fa41e08d..4de8c87d557 100644 --- a/go/vt/vttablet/tabletmanager/tm_state_test.go +++ b/go/vt/vttablet/tabletmanager/tm_state_test.go @@ -176,14 +176,14 @@ func TestStateChangeTabletType(t *testing.T) { Uid: 2, } - err := tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_MASTER) + err := tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_MASTER, DBActionSetReadWrite) require.NoError(t, err) ti, err := ts.GetTablet(ctx, alias) require.NoError(t, err) assert.Equal(t, topodatapb.TabletType_MASTER, ti.Type) assert.NotNil(t, ti.MasterTermStartTime) - err = tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_REPLICA) + err = tm.tmState.ChangeTabletType(ctx, topodatapb.TabletType_REPLICA, DBActionNone) require.NoError(t, err) ti, err = ts.GetTablet(ctx, alias) require.NoError(t, err)