Do not drain tablet in incremental backup (#13773)

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

go/vt/mysqlctl/backupengine.go

@@ -28,12 +28,12 @@ import (
 
 	"github.com/spf13/pflag"
 
-	"vitess.io/vitess/go/mysql/replication"
-
 	"vitess.io/vitess/go/mysql"
+	"vitess.io/vitess/go/mysql/replication"
 	"vitess.io/vitess/go/vt/logutil"
 	"vitess.io/vitess/go/vt/mysqlctl/backupstats"
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	"vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/topo"
@@ -48,7 +48,7 @@ var (
 // BackupEngine is the interface to take a backup with a given engine.
 type BackupEngine interface {
 	ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error)
-	ShouldDrainForBackup() bool
+	ShouldDrainForBackup(req *tabletmanagerdatapb.BackupRequest) bool
 }
 
 // BackupParams is the struct that holds all params passed to ExecuteBackup

go/vt/mysqlctl/builtinbackupengine.go

@@ -36,17 +36,17 @@ import (
 	"github.com/spf13/pflag"
 	"golang.org/x/sync/semaphore"
 
-	"vitess.io/vitess/go/mysql/replication"
-
 	"vitess.io/vitess/go/ioutil"
 	"vitess.io/vitess/go/mysql"
+	"vitess.io/vitess/go/mysql/replication"
 	"vitess.io/vitess/go/vt/concurrency"
 	"vitess.io/vitess/go/vt/log"
 	"vitess.io/vitess/go/vt/logutil"
 	stats "vitess.io/vitess/go/vt/mysqlctl/backupstats"
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
 	"vitess.io/vitess/go/vt/proto/mysqlctl"
 	mysqlctlpb "vitess.io/vitess/go/vt/proto/mysqlctl"
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	"vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/topo"
@@ -1149,7 +1149,11 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa
 
 // ShouldDrainForBackup satisfies the BackupEngine interface
 // backup requires query service to be stopped, hence true
-func (be *BuiltinBackupEngine) ShouldDrainForBackup() bool {
+func (be *BuiltinBackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb.BackupRequest) bool {
+	if req != nil && req.IncrementalFromPos != "" {
+		// Incremental backup: we do not drain the tablet.
+		return false
+	}
 	return true
 }
 

go/vt/mysqlctl/builtinbackupengine_test.go

@@ -21,6 +21,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 )
 
 func TestGetIncrementalFromPosGTIDSet(t *testing.T) {
@@ -67,3 +69,12 @@ func TestGetIncrementalFromPosGTIDSet(t *testing.T) {
 		})
 	}
 }
+
+func TestShouldDrainForBackupBuiltIn(t *testing.T) {
+	be := &BuiltinBackupEngine{}
+
+	assert.True(t, be.ShouldDrainForBackup(&tabletmanagerdatapb.BackupRequest{}))
+	assert.False(t, be.ShouldDrainForBackup(&tabletmanagerdatapb.BackupRequest{IncrementalFromPos: "auto"}))
+	assert.False(t, be.ShouldDrainForBackup(&tabletmanagerdatapb.BackupRequest{IncrementalFromPos: "99ca8ed4-399c-11ee-861b-0a43f95f28a3:1-197"}))
+	assert.False(t, be.ShouldDrainForBackup(&tabletmanagerdatapb.BackupRequest{IncrementalFromPos: "MySQL56/99ca8ed4-399c-11ee-861b-0a43f95f28a3:1-197"}))
+}

go/vt/mysqlctl/fakebackupengine.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 )
 
 type FakeBackupEngine struct {
@@ -86,7 +87,7 @@ func (be *FakeBackupEngine) ExecuteRestore(
 	return be.ExecuteRestoreReturn.Manifest, be.ExecuteRestoreReturn.Err
 }
 
-func (be *FakeBackupEngine) ShouldDrainForBackup() bool {
+func (be *FakeBackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb.BackupRequest) bool {
 	be.ShouldDrainForBackupCalls = be.ShouldDrainForBackupCalls + 1
 	return be.ShouldDrainForBackupReturn
 }

go/vt/mysqlctl/xtrabackupengine.go

@@ -36,6 +36,7 @@ import (
 	"vitess.io/vitess/go/mysql/replication"
 	"vitess.io/vitess/go/vt/logutil"
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	"vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/vterrors"
@@ -950,7 +951,7 @@ func stripeReader(readers []io.Reader, blockSize int64) io.Reader {
 
 // ShouldDrainForBackup satisfies the BackupEngine interface
 // xtrabackup can run while tablet is serving, hence false
-func (be *XtrabackupEngine) ShouldDrainForBackup() bool {
+func (be *XtrabackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb.BackupRequest) bool {
 	return false
 }
 

go/vt/mysqlctl/xtrabackupengine_test.go

@@ -22,7 +22,10 @@ import (
 	"math/rand"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"vitess.io/vitess/go/vt/logutil"
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 )
 
 func TestFindReplicationPosition(t *testing.T) {
@@ -115,3 +118,10 @@ func TestStripeRoundTrip(t *testing.T) {
 	// Test block size and stripe count that don't evenly divide data size.
 	test(6000, 7)
 }
+
+func TestShouldDrainForBackupXtrabackup(t *testing.T) {
+	be := &XtrabackupEngine{}
+
+	assert.False(t, be.ShouldDrainForBackup(nil))
+	assert.False(t, be.ShouldDrainForBackup(&tabletmanagerdatapb.BackupRequest{}))
+}

go/vt/vttablet/tabletmanager/rpc_backup.go

@@ -68,7 +68,7 @@ func (tm *TabletManager) Backup(ctx context.Context, logger logutil.Logger, req
 
 	// Prevent concurrent backups, and record stats
 	backupMode := backupModeOnline
-	if engine.ShouldDrainForBackup() {
+	if engine.ShouldDrainForBackup(req) {
 		backupMode = backupModeOffline
 	}
 	if err := tm.beginBackup(backupMode); err != nil {
@@ -80,7 +80,7 @@ func (tm *TabletManager) Backup(ctx context.Context, logger logutil.Logger, req
 	l := logutil.NewTeeLogger(logutil.NewConsoleLogger(), logger)
 
 	var originalType topodatapb.TabletType
-	if engine.ShouldDrainForBackup() {
+	if engine.ShouldDrainForBackup(req) {
 		if err := tm.lock(ctx); err != nil {
 			return err
 		}