Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not demote a new primary after backup completion #12856

Merged
merged 19 commits into from
May 27, 2023
Merged

do not demote a new primary after backup completion #12856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
94b9586
do not demote a new primary after backup completion
Apr 6, 2023
af9444b
move set replication source out of vtctld and into tablet
Apr 11, 2023
5b59d05
remove set replication source in vtctld
Apr 11, 2023
289fb7a
fix ctx, and wip test
Apr 11, 2023
1a119a9
add e2e test instead of unit test
May 3, 2023
60b1d0d
use built in methods
May 10, 2023
c9917cc
omit this in test
May 12, 2023
9a7ca0c
Update go/vt/vttablet/tabletmanager/rpc_backup.go
olyazavr May 17, 2023
90ea238
move wg done to top of func
May 17, 2023
ffe41c4
fix method change
May 17, 2023
d5bc717
try running this in isolation
May 18, 2023
2bb78e5
try running this in isolation, dont run with others
May 18, 2023
180f629
add debug logging
May 19, 2023
db4c441
add debug logging
May 19, 2023
a0bf584
Update go/test/endtoend/backup/vtctlbackup/backup_utils.go
olyazavr May 26, 2023
9ec0e1d
Update go/test/endtoend/backup/vtctlbackup/backup_utils.go
olyazavr May 26, 2023
45ff059
run goimports
May 26, 2023
3802300
remove dupe
May 26, 2023
2e55a26
remove generated file
May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions go/test/endtoend/backup/vtctlbackup/backup_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import (
"os/exec"
"path"
"strings"
"sync"
"syscall"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/json2"
"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/test/endtoend/cluster"
Expand Down Expand Up @@ -354,6 +356,10 @@ func TestBackup(t *testing.T, setupType int, streamMode string, stripes int, cDe
name: "TestTerminatedRestore",
method: terminatedRestore,
}, //
{
name: "DoNotDemoteNewlyPromotedPrimaryIfReparentingDuringBackup",
method: doNotDemoteNewlyPromotedPrimaryIfReparentingDuringBackup,
}, //
}

defer cluster.PanicHandler(t)
Expand All @@ -369,6 +375,10 @@ func TestBackup(t *testing.T, setupType int, streamMode string, stripes int, cDe
if len(runSpecific) > 0 && !isRegistered(test.name, runSpecific) {
continue
}
// don't run this one unless specified
if len(runSpecific) == 0 && test.name == "DoNotDemoteNewlyPromotedPrimaryIfReparentingDuringBackup" {
continue
}
if retVal := t.Run(test.name, test.method); !retVal {
return vterrors.Errorf(vtrpc.Code_UNKNOWN, "test failure: %s", test.name)
}
Expand Down Expand Up @@ -802,11 +812,11 @@ func terminatedRestore(t *testing.T) {
func checkTabletType(t *testing.T, alias string, tabletType topodata.TabletType) {
// for loop for 15 seconds to check if tablet type is correct
for i := 0; i < 15; i++ {
output, err := localCluster.VtctlclientProcess.ExecuteCommandWithOutput("GetTablet", alias)
output, err := localCluster.VtctldClientProcess.ExecuteCommandWithOutput("GetTablet", alias)
require.Nil(t, err)
var tabletPB topodata.Tablet
err = json.Unmarshal([]byte(output), &tabletPB)
require.Nil(t, err)
err = json2.Unmarshal([]byte(output), &tabletPB)
require.NoError(t, err)
if tabletType == tabletPB.Type {
return
}
Expand All @@ -815,6 +825,44 @@ func checkTabletType(t *testing.T, alias string, tabletType topodata.TabletType)
require.Failf(t, "checkTabletType failed.", "Tablet type is not correct. Expected: %v", tabletType)
}

func doNotDemoteNewlyPromotedPrimaryIfReparentingDuringBackup(t *testing.T) {
var wg sync.WaitGroup
wg.Add(2)

// Start the backup on a replica
go func() {
defer wg.Done()
// ensure this is a primary first
checkTabletType(t, primary.Alias, topodata.TabletType_PRIMARY)

// now backup
err := localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias)
require.Nil(t, err)
}()

// Perform a graceful reparent operation
go func() {
defer wg.Done()
// ensure this is a primary first
checkTabletType(t, primary.Alias, topodata.TabletType_PRIMARY)

// now reparent
_, err := localCluster.VtctlclientProcess.ExecuteCommandWithOutput(
"PlannedReparentShard", "--",
"--keyspace_shard", fmt.Sprintf("%s/%s", keyspaceName, shardName),
"--new_primary", replica1.Alias)
require.Nil(t, err)

// check that we reparented
checkTabletType(t, replica1.Alias, topodata.TabletType_PRIMARY)
}()

wg.Wait()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it there can be a race condition. The current implementation assumed both Backup and PlannedReparentShard are heavyweight enough (read: runtimes are long enough in both), that if we call them roughly concurrently, that means they'll execute concurrently to some extent.

But it does not enforce the situation where the backup has started, and then the reparent began. It could be the other way around.

I don't have a good suggestion here and I don't see a clean solution. But still its worth pointing out this test could either become flaky, or not always test what it's meant to test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought the same thing, but the test as it stands now will not be flaky, but it won't test the correct thing everytime either. When we introduce some bug, then the test would become flaky.
I thought of ways to make the test better, but other than inserting a million records to lengthen the backup process, I don't know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I spent an extraordinary amount of time trying to find ways to make this be more accurate, but nothing reasonable came to mind ):


// check that this is still a primary
checkTabletType(t, replica1.Alias, topodata.TabletType_PRIMARY)
}

// test_backup will:
// - create a shard with primary and replica1 only
// - run InitShardPrimary
Expand Down
4 changes: 4 additions & 0 deletions go/test/endtoend/backup/xtrabackup/xtrabackup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ func TestXtrabackupWithExternalZstdCompressionAndManifestedDecompressor(t *testi
backup.TestBackup(t, backup.XtraBackup, "tar", 0, cDetails, []string{"TestReplicaBackup"})
}

func TestDoNotDemoteNewlyPromotedPrimaryIfReparentingDuringBackup(t *testing.T) {
backup.TestBackup(t, backup.XtraBackup, "xbstream", 0, nil, []string{"DoNotDemoteNewlyPromotedPrimaryIfReparentingDuringBackup"})
}

func setDefaultCompressionFlag() {
mysqlctl.CompressionEngineName = "pgzip"
mysqlctl.ExternalCompressorCmd = ""
Expand Down
15 changes: 1 addition & 14 deletions go/vt/vtctl/grpcvtctldserver/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,20 +489,7 @@ func (s *VtctldServer) backupTablet(ctx context.Context, tablet *topodatapb.Tabl
logger.Errorf("failed to send stream response %+v: %v", resp, err)
}
case io.EOF:
// Do not do anything for primary tablets and when active reparenting is disabled
if mysqlctl.DisableActiveReparents || tablet.Type == topodatapb.TabletType_PRIMARY {
return nil
}

// Otherwise we find the correct primary tablet and set the replication source,
// since the primary could have changed while we executed the backup which can
// also affect whether we want to send semi sync acks or not.
tabletInfo, err := s.ts.GetTablet(ctx, tablet.Alias)
if err != nil {
return err
}

return reparentutil.SetReplicationSource(ctx, s.ts, s.tmc, tabletInfo.Tablet)
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a question here: if there is a reparent operation and some other replica is promoted to primary, will this replica still point to the correct new primary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we add a test for that as well, if it does not exist already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will, as I have ported this exact logic to rpc_backup, with the addition of checking to not demote yourself if you're the primary

default:
return err
}
Expand Down
43 changes: 43 additions & 0 deletions go/vt/vttablet/tabletmanager/rpc_backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import (
"fmt"
"time"

"vitess.io/vitess/go/vt/topotools"
"vitess.io/vitess/go/vt/vtctl/reparentutil"

"vitess.io/vitess/go/vt/logutil"
"vitess.io/vitess/go/vt/mysqlctl"
"vitess.io/vitess/go/vt/mysqlctl/backupstats"
Expand Down Expand Up @@ -100,6 +103,46 @@ func (tm *TabletManager) Backup(ctx context.Context, logger logutil.Logger, req
// Original type could be primary so pass in a real value for PrimaryTermStartTime
if err := tm.changeTypeLocked(bgCtx, originalType, DBActionNone, SemiSyncActionNone); err != nil {
l.Errorf("Failed to change tablet type from %v to %v, error: %v", topodatapb.TabletType_BACKUP, originalType, err)
return
}

// Find the correct primary tablet and set the replication source,
// since the primary could have changed while we executed the backup which can
// also affect whether we want to send semi sync acks or not.
tabletInfo, err := tm.TopoServer.GetTablet(bgCtx, tablet.Alias)
if err != nil {
l.Errorf("Failed to fetch updated tablet info, error: %v", err)
return
}

// Do not do anything for primary tablets or when active reparenting is disabled
if mysqlctl.DisableActiveReparents || tabletInfo.Type == topodatapb.TabletType_PRIMARY {
return
}

shardPrimary, err := topotools.GetShardPrimaryForTablet(bgCtx, tm.TopoServer, tablet.Tablet)
if err != nil {
return
}

durabilityName, err := tm.TopoServer.GetKeyspaceDurability(bgCtx, tablet.Keyspace)
if err != nil {
l.Errorf("Failed to get durability policy, error: %v", err)
return
}
durability, err := reparentutil.GetDurabilityPolicy(durabilityName)
if err != nil {
l.Errorf("Failed to get durability with name %v, error: %v", durabilityName, err)
}

isSemiSync := reparentutil.IsReplicaSemiSync(durability, shardPrimary.Tablet, tabletInfo.Tablet)
semiSyncAction, err := tm.convertBoolToSemiSyncAction(isSemiSync)
if err != nil {
l.Errorf("Failed to convert bool to semisync action, error: %v", err)
return
}
if err := tm.setReplicationSourceLocked(bgCtx, shardPrimary.Alias, 0, "", false, semiSyncAction); err != nil {
l.Errorf("Failed to set replication source, error: %v", err)
}
}()
}
Expand Down