Merge pull request #6804 from planetscale/ds-backport-6762-6786

tm: call SetReadOnly inside ChangeTabletType

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

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,19 +72,19 @@ 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.
  if tabletType == topodatapb.TabletType_DRAINED && tm.Tablet().Type == topodatapb.TabletType_DRAINED {
  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
  }
 

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 {

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
  }
 

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

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)