tm revamp: remove calls to refreshTablet

go/vt/servenv/servenv.go

@@ -158,6 +158,7 @@ func OnTermSync(f func()) {
 
 // fireOnTermSyncHooks returns true iff all the hooks finish before the timeout.
 func fireOnTermSyncHooks(timeout time.Duration) bool {
+	defer log.Flush()
 	log.Infof("Firing synchronous OnTermSync hooks and waiting up to %v for them", timeout)
 
 	timer := time.NewTimer(timeout)

go/vt/vtctl/vtctl.go

@@ -164,12 +164,12 @@ var commands = []commandGroup{
 	{
 		"Tablets", []command{
 			{"InitTablet", commandInitTablet,
-				"[-allow_update] [-allow_different_shard] [-allow_master_override] [-parent] [-db_name_override=<db name>] [-hostname=<hostname>] [-mysql_port=<port>] [-port=<port>] [-grpc_port=<port>] [-tags=tag1:value1,tag2:value2] -keyspace=<keyspace> -shard=<shard> <tablet alias> <tablet type>",
+				"DEPRECATED [-allow_update] [-allow_different_shard] [-allow_master_override] [-parent] [-db_name_override=<db name>] [-hostname=<hostname>] [-mysql_port=<port>] [-port=<port>] [-grpc_port=<port>] [-tags=tag1:value1,tag2:value2] -keyspace=<keyspace> -shard=<shard> <tablet alias> <tablet type>",
 				"Initializes a tablet in the topology.\n"},
 			{"GetTablet", commandGetTablet,
 				"<tablet alias>",
 				"Outputs a JSON structure that contains information about the Tablet."},
-			{"UpdateTabletAddrs", commandUpdateTabletAddrs,
+			{"DEPRECATED UpdateTabletAddrs", commandUpdateTabletAddrs,
 				"[-hostname <hostname>] [-ip-addr <ip addr>] [-mysql-port <mysql port>] [-vt-port <vt port>] [-grpc-port <grpc port>] <tablet alias> ",
 				"Updates the IP address and port numbers of a tablet."},
 			{"DeleteTablet", commandDeleteTablet,

go/vt/vttablet/tabletmanager/action_agent.go

@@ -184,9 +184,6 @@ type ActionAgent struct {
 	// _shardSyncCancel is the function to stop the background shard sync goroutine.
 	_shardSyncCancel context.CancelFunc
 
-	// _tablet has the Tablet record we last read from the topology server.
-	_tablet *topodatapb.Tablet
-
 	// _disallowQueryService is set to the reason we should be
 	// disallowing queries from being served. It is set from changeCallback,
 	// and used by healthcheck. If empty, we should allow queries.
@@ -230,6 +227,11 @@ type ActionAgent struct {
 	_lockTablesTimer      *time.Timer
 	// _isBackupRunning tells us whether there is a backup that is currently running
 	_isBackupRunning bool
+
+	pubMu sync.Mutex
+	// _tablet has the Tablet record we last read from the topology server.
+	tablet       *topodatapb.Tablet
+	isPublishing bool
 }
 
 // NewActionAgent creates a new ActionAgent and registers all the
@@ -326,6 +328,33 @@ func NewActionAgent(
 		agent.registerQueryService()
 	})
 
+	// optionally populate metadata records
+	if !*restoreFromBackup && *initPopulateMetadata {
+		// we use initialTablet here because it has the intended tabletType.
+		// the tablet returned by agent.Tablet() will have type UNKNOWN until
+		// we call updateState.
+		localMetadata := agent.getLocalMetadataValues(agent.initialTablet.Type)
+		if agent.Cnf != nil { // we are managing mysqld
+			// we'll use batchCtx here because we are still initializing and can't proceed unless this succeeds
+			if err := agent.MysqlDaemon.Wait(batchCtx, agent.Cnf); err != nil {
+				return nil, err
+			}
+		}
+		err := mysqlctl.PopulateMetadataTables(agent.MysqlDaemon, localMetadata, topoproto.TabletDbName(agent.initialTablet))
+		if err != nil {
+			return nil, vterrors.Wrap(err, "failed to -init_populate_metadata")
+		}
+	}
+
+	// Update our state (need the action lock).
+	// We do this upfront to prevent this from racing with restoreFromBackup
+	// in case it gets launched.
+	if err := agent.lock(batchCtx); err != nil {
+		return nil, err
+	}
+	agent.updateState(batchCtx, agent.initialTablet, "Start")
+	agent.unlock()
+
 	// two cases then:
 	// - restoreFromBackup is set: we restore, then initHealthCheck, all
 	//   in the background
@@ -342,34 +371,6 @@ func NewActionAgent(
 			agent.initHealthCheck()
 		}()
 	} else {
-		// optionally populate metadata records
-		if *initPopulateMetadata {
-			// we use initialTablet here because it has the intended tabletType.
-			// the tablet returned by agent.Tablet() will have type UNKNOWN until we call
-			// refreshTablet
-			localMetadata := agent.getLocalMetadataValues(agent.initialTablet.Type)
-			if agent.Cnf != nil { // we are managing mysqld
-				// we'll use batchCtx here because we are still initializing and can't proceed unless this succeeds
-				if err := agent.MysqlDaemon.Wait(batchCtx, agent.Cnf); err != nil {
-					return nil, err
-				}
-			}
-			err := mysqlctl.PopulateMetadataTables(agent.MysqlDaemon, localMetadata, topoproto.TabletDbName(agent.initialTablet))
-			if err != nil {
-				return nil, vterrors.Wrap(err, "failed to -init_populate_metadata")
-			}
-		}
-
-		// Update our state (need the action lock).
-		if err := agent.lock(batchCtx); err != nil {
-			return nil, err
-		}
-		if err := agent.refreshTablet(batchCtx, "Start"); err != nil {
-			agent.unlock()
-			return nil, err
-		}
-		agent.unlock()
-
 		// synchronously start health check if needed
 		agent.initHealthCheck()
 	}
@@ -423,9 +424,7 @@ func NewTestActionAgent(batchCtx context.Context, ts *topo.Server, tabletAlias *
 		panic(vterrors.Wrap(err, "agent.lock() failed"))
 	}
 	defer agent.unlock()
-	if err := agent.refreshTablet(batchCtx, "Start"); err != nil {
-		panic(vterrors.Wrapf(err, "agent.refreshTablet(%v) failed", tabletAlias))
-	}
+	agent.updateState(batchCtx, agent.initialTablet, "Start")
 
 	return agent
 }
@@ -475,9 +474,7 @@ func NewComboActionAgent(batchCtx context.Context, ts *topo.Server, tabletAlias
 		panic(vterrors.Wrap(err, "agent.lock() failed"))
 	}
 	defer agent.unlock()
-	if err := agent.refreshTablet(batchCtx, "Start"); err != nil {
-		panic(vterrors.Wrapf(err, "agent.refreshTablet(%v) failed", tabletAlias))
-	}
+	agent.updateState(batchCtx, agent.initialTablet, "Start")
 
 	return agent
 }
@@ -488,19 +485,19 @@ func (agent *ActionAgent) registerQueryRuleSources() {
 }
 
 func (agent *ActionAgent) setTablet(tablet *topodatapb.Tablet) {
-	agent.mutex.Lock()
-	agent._tablet = proto.Clone(tablet).(*topodatapb.Tablet)
-	agent.mutex.Unlock()
+	agent.pubMu.Lock()
+	agent.tablet = proto.Clone(tablet).(*topodatapb.Tablet)
+	agent.pubMu.Unlock()
 
 	// Notify the shard sync loop that the tablet state changed.
 	agent.notifyShardSync()
 }
 
-// Tablet reads the stored Tablet from the agent, protected by mutex.
+// Tablet reads the stored Tablet from the agent.
 func (agent *ActionAgent) Tablet() *topodatapb.Tablet {
-	agent.mutex.Lock()
-	tablet := proto.Clone(agent._tablet).(*topodatapb.Tablet)
-	agent.mutex.Unlock()
+	agent.pubMu.Lock()
+	tablet := proto.Clone(agent.tablet).(*topodatapb.Tablet)
+	agent.pubMu.Unlock()
 	return tablet
 }
 
@@ -666,6 +663,9 @@ func (agent *ActionAgent) Start(ctx context.Context, dbcfgs *dbconfigs.DBConfigs
 		return err
 	}
 
+	// Initialize masterTermStartTime
+	agent.setMasterTermStartTime(logutil.ProtoToTime(agent.initialTablet.MasterTermStartTime))
+
 	// Verify the topology is correct.
 	agent.verifyTopology(ctx)
 
@@ -719,8 +719,7 @@ func (agent *ActionAgent) Start(ctx context.Context, dbcfgs *dbconfigs.DBConfigs
 
 	// Initialize the current tablet to match our current running
 	// state: Has most field filled in, but type is UNKNOWN.
-	// Subsequents calls to updateState or refreshTablet
-	// will then work as expected.
+	// Subsequents calls to updateState will then work as expected.
 	startingTablet := proto.Clone(agent.initialTablet).(*topodatapb.Tablet)
 	startingTablet.Type = topodatapb.TabletType_UNKNOWN
 	agent.setTablet(startingTablet)

go/vt/vttablet/tabletmanager/healthcheck.go

@@ -33,7 +33,6 @@ import (
 	"html/template"
 	"time"
 
-	"github.com/golang/protobuf/proto"
 	"vitess.io/vitess/go/timer"
 	"vitess.io/vitess/go/vt/health"
 	"vitess.io/vitess/go/vt/log"
@@ -182,8 +181,8 @@ func (agent *ActionAgent) runHealthCheck() {
 func (agent *ActionAgent) runHealthCheckLocked() {
 	agent.checkLock()
 	// read the current tablet record and tablet control
+	tablet := agent.Tablet()
 	agent.mutex.Lock()
-	tablet := proto.Clone(agent._tablet).(*topodatapb.Tablet)
 	shouldBeServing := agent._disallowQueryService == ""
 	runUpdateStream := agent._enableUpdateStream
 	ignoreErrorExpr := agent._ignoreHealthErrorExpr

go/vt/vttablet/tabletmanager/healthcheck_test.go

@@ -670,7 +670,7 @@ func TestTabletControl(t *testing.T) {
 	// now refresh the tablet state, as the resharding process would do
 	agent.RefreshState(ctx)
 
-	// QueryService changed back from SERVING to NOT_SERVING since refreshTablet()
+	// QueryService changed back from SERVING to NOT_SERVING since RefreshState()
 	// re-read the topology and saw that REPLICA is still not allowed to serve.
 	if _, err := expectBroadcastData(agent.QueryServiceControl, true, "", 19); err != nil {
 		t.Fatal(err)

go/vt/vttablet/tabletmanager/init_tablet.go

@@ -96,6 +96,7 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
 	}); err != nil {
 		return vterrors.Wrap(err, "InitTablet cannot GetOrCreateShard shard")
 	}
+	var masterTermStartTime time.Time
 	if si.MasterAlias != nil && topoproto.TabletAliasEqual(si.MasterAlias, agent.TabletAlias) {
 		// We're marked as master in the shard record, which could mean the master
 		// tablet process was just restarted. However, we need to check if a new
@@ -110,7 +111,7 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
 			// Update the master term start time (current value is 0) because we
 			// assume that we are actually the MASTER and in case of a tiebreak,
 			// vtgate should prefer us.
-			agent.setMasterTermStartTime(time.Now())
+			masterTermStartTime = time.Now()
 		case err == nil:
 			if oldTablet.Type == topodatapb.TabletType_MASTER {
 				// We're marked as master in the shard record,
@@ -119,9 +120,9 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
 				// Read the master term start time from tablet.
 				// If it is nil, it might mean that we are upgrading, so use current time instead
 				if oldTablet.MasterTermStartTime != nil {
-					agent.setMasterTermStartTime(oldTablet.GetMasterTermStartTime())
+					masterTermStartTime = oldTablet.GetMasterTermStartTime()
 				} else {
-					agent.setMasterTermStartTime(time.Now())
+					masterTermStartTime = time.Now()
 				}
 			}
 		default:
@@ -141,7 +142,7 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
 				if oldMasterTermStartTime.After(currentShardTime) {
 					tabletType = topodatapb.TabletType_MASTER
 					// read the master term start time from tablet
-					agent.setMasterTermStartTime(oldMasterTermStartTime)
+					masterTermStartTime = oldTablet.GetMasterTermStartTime()
 				}
 			}
 		default:
@@ -220,8 +221,8 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
 		DbNameOverride: *initDbNameOverride,
 		Tags:           initTags,
 	}
-	if !agent.masterTermStartTime().IsZero() {
-		tablet.MasterTermStartTime = logutil.TimeToProto(agent.masterTermStartTime())
+	if !masterTermStartTime.IsZero() {
+		tablet.MasterTermStartTime = logutil.TimeToProto(masterTermStartTime)
 	}
 	if port != 0 {
 		tablet.PortMap["vt"] = port
@@ -265,7 +266,5 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
 	default:
 		return vterrors.Wrap(err, "CreateTablet failed")
 	}
-
-	agent.setTablet(tablet)
 	return nil
 }

go/vt/vttablet/tabletmanager/init_tablet_test.go

@@ -27,6 +27,7 @@ import (
 	"vitess.io/vitess/go/history"
 	"vitess.io/vitess/go/mysql/fakesqldb"
 	"vitess.io/vitess/go/vt/dbconfigs"
+	"vitess.io/vitess/go/vt/logutil"
 	"vitess.io/vitess/go/vt/mysqlctl/fakemysqldaemon"
 	"vitess.io/vitess/go/vt/topo"
 	"vitess.io/vitess/go/vt/topo/memorytopo"
@@ -295,7 +296,7 @@ func TestInitTablet(t *testing.T) {
 	if ti.Type != topodatapb.TabletType_MASTER {
 		t.Errorf("wrong tablet type: %v", ti.Type)
 	}
-	ter1 := agent._masterTermStartTime
+	ter1 := logutil.ProtoToTime(ti.Tablet.MasterTermStartTime)
 	if ter1.IsZero() {
 		t.Fatalf("MASTER tablet should have a masterTermStartTime set")
 	}
@@ -317,7 +318,7 @@ func TestInitTablet(t *testing.T) {
 	if ti.Type != topodatapb.TabletType_MASTER {
 		t.Errorf("wrong tablet type: %v", ti.Type)
 	}
-	ter2 := agent._masterTermStartTime
+	ter2 := logutil.ProtoToTime(ti.Tablet.MasterTermStartTime)
 	if ter2.IsZero() || !ter2.Equal(ter1) {
 		t.Fatalf("After a restart, masterTermStartTime must be equal to the previous time saved in the tablet record. Previous timestamp: %v current timestamp: %v", ter1, ter2)
 	}
@@ -342,7 +343,7 @@ func TestInitTablet(t *testing.T) {
 	if len(ti.Tags) != 1 || ti.Tags["aaa"] != "bbb" {
 		t.Errorf("wrong tablet tags: %v", ti.Tags)
 	}
-	ter3 := agent._masterTermStartTime
+	ter3 := logutil.ProtoToTime(ti.Tablet.MasterTermStartTime)
 	if ter3.IsZero() || !ter3.Equal(ter2) {
 		t.Fatalf("After a restart, masterTermStartTime must be set to the previous time saved in the tablet record. Previous timestamp: %v current timestamp: %v", ter2, ter3)
 	}

go/vt/vttablet/tabletmanager/restore.go

@@ -62,28 +62,16 @@ func (agent *ActionAgent) RestoreData(ctx context.Context, logger logutil.Logger
 }
 
 func (agent *ActionAgent) restoreDataLocked(ctx context.Context, logger logutil.Logger, waitForBackupInterval time.Duration, deleteBeforeRestore bool) error {
-	// change type to RESTORE (using UpdateTabletFields so it's
-	// always authorized)
 	var originalType topodatapb.TabletType
-	if _, err := agent.TopoServer.UpdateTabletFields(ctx, agent.TabletAlias, func(tablet *topodatapb.Tablet) error {
-		originalType = tablet.Type
-		tablet.Type = topodatapb.TabletType_RESTORE
-		return nil
-	}); err != nil {
-		return vterrors.Wrap(err, "Cannot change type to RESTORE")
-	}
-
-	// let's update our internal state (stop query service and other things)
-	if err := agent.refreshTablet(ctx, "restore from backup"); err != nil {
-		return vterrors.Wrap(err, "failed to update state before restore")
-	}
+	tablet := agent.Tablet()
+	originalType, tablet.Type = tablet.Type, topodatapb.TabletType_RESTORE
+	agent.updateState(ctx, tablet, "restore from backup")
 
 	// Try to restore. Depending on the reason for failure, we may be ok.
 	// If we're not ok, return an error and the agent will log.Fatalf,
 	// causing the process to be restarted and the restore retried.
 	// Record local metadata values based on the original type.
 	localMetadata := agent.getLocalMetadataValues(originalType)
-	tablet := agent.Tablet()
 
 	keyspace := tablet.Keyspace
 	keyspaceInfo, err := agent.TopoServer.GetKeyspace(ctx, keyspace)
@@ -157,11 +145,8 @@ func (agent *ActionAgent) restoreDataLocked(ctx context.Context, logger logutil.
 		// alter replication here.
 	default:
 		// If anything failed, we should reset the original tablet type
-		agent.TopoServer.UpdateTabletFields(context.Background(), tablet.Alias, func(tablet *topodatapb.Tablet) error {
-			tablet.Type = originalType
-			return nil
-		})
-		agent.refreshTablet(ctx, "failed for restore from backup")
+		tablet.Type = originalType
+		agent.updateState(ctx, tablet, "failed for restore from backup")
 		return vterrors.Wrap(err, "Can't restore backup")
 	}
 
@@ -175,17 +160,8 @@ func (agent *ActionAgent) restoreDataLocked(ctx context.Context, logger logutil.
 	}
 
 	// Change type back to original type if we're ok to serve.
-	if _, err := agent.TopoServer.UpdateTabletFields(context.Background(), tablet.Alias, func(tablet *topodatapb.Tablet) error {
-		tablet.Type = originalType
-		return nil
-	}); err != nil {
-		return vterrors.Wrapf(err, "Cannot change type back to %v", originalType)
-	}
-
-	// let's update our internal state (start query service and other things)
-	if err := agent.refreshTablet(context.Background(), "after restore from backup"); err != nil {
-		return vterrors.Wrap(err, "failed to update state after backup")
-	}
+	tablet.Type = originalType
+	agent.updateState(ctx, tablet, "after restore from backup")
 
 	return nil
 }