diff --git a/go/vt/vttablet/tabletmanager/restore.go b/go/vt/vttablet/tabletmanager/restore.go index 078b49674dc..20bc4524956 100644 --- a/go/vt/vttablet/tabletmanager/restore.go +++ b/go/vt/vttablet/tabletmanager/restore.go @@ -129,7 +129,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. @@ -178,7 +178,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") @@ -194,7 +194,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 1db7e7dccde..40e7a499995 100644 --- a/go/vt/vttablet/tabletmanager/rpc_replication.go +++ b/go/vt/vttablet/tabletmanager/rpc_replication.go @@ -249,11 +249,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.MysqlDaemon.SetReadOnly(false); err != nil { - 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 } return mysql.EncodePosition(pos), nil @@ -283,7 +279,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 } } @@ -522,7 +518,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 } } @@ -624,7 +620,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 @@ -735,13 +731,7 @@ func (tm *TabletManager) PromoteReplica(ctx context.Context) (string, error) { return "", err } - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_MASTER); 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 := tm.MysqlDaemon.SetReadOnly(false); 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 fad4a8b9048..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,6 +163,13 @@ func (ts *tmState) ChangeTabletType(ctx context.Context, tabletType topodatapb.T if 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 ts.tablet.MasterTermStartTime = masterTermStartTime 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)