[release-17.0] Do not drain tablet in incremental backup (#13773)

go/vt/mysqlctl/backupengine.go

@@ -32,6 +32,7 @@ import (
 	"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"
@@ -46,7 +47,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

@@ -42,6 +42,7 @@ import (
 	"vitess.io/vitess/go/vt/logutil"
 	stats "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"
@@ -1080,7 +1081,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/builtinbackupengine2_test.go

@@ -0,0 +1,35 @@
+/*
+Copyright 2023 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// Package mysqlctl_test is the blackbox tests for package mysqlctl.
+package mysqlctl
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
+)
+
+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

@@ -35,6 +35,7 @@ import (
 	"vitess.io/vitess/go/mysql"
 	"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"
@@ -944,7 +945,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
 		}