Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history 
This changes behavior so that both PRS & ERS prefer, but do not exclude, replicas that are taking backups.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
  • Loading branch information
@ejortegau
ejortegau committed Oct 30, 2024
1 parent 2d22cf4 commit 54bdcc0
Show file tree
Hide file tree
Showing 4 changed files with 139 additions and 18 deletions.
15 changes: 7 additions & 8 deletions go/vt/vtctl/reparentutil/emergency_reparenter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,7 @@ func (erp *EmergencyReparenter) identifyPrimaryCandidate(
// filterValidCandidates filters valid tablets, keeping only the ones which can successfully be promoted without any constraint failures and can make forward progress on being promoted
func (erp *EmergencyReparenter) filterValidCandidates(validTablets []*topodatapb.Tablet, tabletsReachable []*topodatapb.Tablet, tabletsBackingUp map[string]bool, prevPrimary *topodatapb.Tablet, opts EmergencyReparentOptions) ([]*topodatapb.Tablet, error) {
var restrictedValidTablets []*topodatapb.Tablet
var notPreferredValidTablets []*topodatapb.Tablet
for _, tablet := range validTablets {
tabletAliasStr := topoproto.TabletAliasString(tablet.Alias)
// Remove tablets which have MustNot promote rule since they must never be promoted
Expand All @@ -757,16 +758,14 @@ func (erp *EmergencyReparenter) filterValidCandidates(validTablets []*topodatapb
}
continue
}
// Remove candidates that are running a backup.
// Put candidates that are running a backup in a separate list
backingUp, ok := tabletsBackingUp[tabletAliasStr]
if ok && backingUp {
erp.logger.Infof("Removing %s from list of valid candidates for promotion because it is running a backup", tabletAliasStr)
if opts.NewPrimaryAlias != nil && topoproto.TabletAliasEqual(opts.NewPrimaryAlias, tablet.Alias) {
return nil, vterrors.Errorf(vtrpc.Code_ABORTED, "proposed primary %s is taking backup, refusing to promote it", topoproto.TabletAliasString(opts.NewPrimaryAlias))
}
continue
erp.logger.Infof("Setting %s in list of valid candidates taking a backup", tabletAliasStr)
notPreferredValidTablets = append(notPreferredValidTablets, tablet)
} else {
restrictedValidTablets = append(restrictedValidTablets, tablet)
}
restrictedValidTablets = append(restrictedValidTablets, tablet)
}
return restrictedValidTablets, nil
return append(restrictedValidTablets, notPreferredValidTablets...), nil
}
4 changes: 2 additions & 2 deletions go/vt/vtctl/reparentutil/emergency_reparenter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4489,12 +4489,12 @@ func TestEmergencyReparenter_filterValidCandidates(t *testing.T) {
tabletsBackingUp: noTabletsBackingUp,
filteredTablets: []*topodatapb.Tablet{primaryTablet, replicaTablet, replicaCrossCellTablet},
}, {
name: "filter backing up",
name: "host backing up must be last in the list",
durability: "none",
validTablets: allTablets,
tabletsReachable: allTablets,
tabletsBackingUp: replicaBackingUp,
filteredTablets: []*topodatapb.Tablet{primaryTablet, replicaCrossCellTablet},
filteredTablets: []*topodatapb.Tablet{primaryTablet, replicaCrossCellTablet, replicaTablet},
}, {
name: "filter cross cell",
durability: "none",
Expand Down
27 changes: 20 additions & 7 deletions go/vt/vtctl/reparentutil/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,13 @@ func ElectNewPrimary(
// mutex to secure the next two fields from concurrent access
mu sync.Mutex
// tablets that are possible candidates to be the new primary and their positions
validTablets []*topodatapb.Tablet
tabletPositions []replication.Position
innodbBufferPool []int
errorGroup, groupCtx = errgroup.WithContext(ctx)
validTablets []*topodatapb.Tablet
tabletPositions []replication.Position
innodbBufferPool []int
preferNotTablets []*topodatapb.Tablet
preferNotTabletPositions []replication.Position
preferNotInnodbBufferPool []int
errorGroup, groupCtx = errgroup.WithContext(ctx)
)

// candidates are the list of tablets that can be potentially promoted after filtering out based on preliminary checks.
Expand Down Expand Up @@ -135,7 +138,9 @@ func ElectNewPrimary(
tabletPositions = append(tabletPositions, pos)
innodbBufferPool = append(innodbBufferPool, innodbBufferPoolData[topoproto.TabletAliasString(tb.Alias)])
} else {
reasonsToInvalidate.WriteString(fmt.Sprintf("\n%v is running a backup, so not using it as promotion candidate", topoproto.TabletAliasString(tablet.Alias)))
preferNotTablets = append(preferNotTablets, tb)
preferNotTabletPositions = append(preferNotTabletPositions, pos)
preferNotInnodbBufferPool = append(preferNotInnodbBufferPool, innodbBufferPoolData[topoproto.TabletAliasString(tb.Alias)])
}
} else {
reasonsToInvalidate.WriteString(fmt.Sprintf("\n%v has %v replication lag which is more than the tolerable amount", topoproto.TabletAliasString(tablet.Alias), replLag))
Expand All @@ -150,7 +155,7 @@ func ElectNewPrimary(
}

// return an error if there are no valid tablets available
if len(validTablets) == 0 {
if len(validTablets) == 0 && len(preferNotTablets) == 0 {
return nil, vterrors.Errorf(vtrpc.Code_INTERNAL, "cannot find a tablet to reparent to%v", reasonsToInvalidate.String())
}

Expand All @@ -159,8 +164,16 @@ func ElectNewPrimary(
if err != nil {
return nil, err
}
err = sortTabletsForReparent(preferNotTablets, preferNotTabletPositions, innodbBufferPool, opts.durability)
if err != nil {
return nil, err
}

if len(validTablets) > 0 {
return validTablets[0].Alias, nil
}

return validTablets[0].Alias, nil
return preferNotTablets[0].Alias, nil
}

// findPositionLagBackingUpForTablet processes the replication position and lag for a single tablet and
Expand Down
111 changes: 110 additions & 1 deletion go/vt/vtctl/reparentutil/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func TestElectNewPrimary(t *testing.T) {
errContains: nil,
},
{
name: "Two good replicas, but one of them backing up",
name: "Two good replicas, but one of them backing up so we pick the other one",
tmc: &chooseNewPrimaryTestTMClient{
// both zone1-101 and zone1-102 are equivalent from a replicaiton PoV, but zone1-102 is taking a backup
replicationStatuses: map[string]*replicationdatapb.Status{
Expand Down Expand Up @@ -200,6 +200,54 @@ func TestElectNewPrimary(t *testing.T) {
},
errContains: nil,
},
{
name: "Only one replica, but it's backing up. We still use it.",
tmc: &chooseNewPrimaryTestTMClient{
// both zone1-101 and zone1-102 are equivalent from a replicaiton PoV, but zone1-102 is taking a backup
replicationStatuses: map[string]*replicationdatapb.Status{
"zone1-0000000101": {
Position: "MySQL56/3E11FA47-71CA-11E1-9E33-C80AA9429562:1-5",
BackingUp: true,
},
},
},
tolerableReplLag: 50 * time.Second,
shardInfo: topo.NewShardInfo("testkeyspace", "-", &topodatapb.Shard{
PrimaryAlias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 100,
},
}, nil),
tabletMap: map[string]*topo.TabletInfo{
"primary": {
Tablet: &topodatapb.Tablet{
Alias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 100,
},
Type: topodatapb.TabletType_PRIMARY,
},
},
"replica1": {
Tablet: &topodatapb.Tablet{
Alias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 101,
},
Type: topodatapb.TabletType_REPLICA,
},
},
},
avoidPrimaryAlias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 0,
},
expected: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 101,
},
errContains: nil,
},
{
name: "new primary alias provided - no tolerable replication lag",
tolerableReplLag: 0,
Expand Down Expand Up @@ -475,6 +523,67 @@ func TestElectNewPrimary(t *testing.T) {
},
errContains: nil,
},
{
name: "Two replicas, first one with too much lag, another one backing up - elect the one taking backup",
tmc: &chooseNewPrimaryTestTMClient{
// zone1-101 is behind zone1-102
replicationStatuses: map[string]*replicationdatapb.Status{
"zone1-0000000101": {
Position: "MySQL56/3E11FA47-71CA-11E1-9E33-C80AA9429562:1",
ReplicationLagSeconds: 55,
},
"zone1-0000000102": {
Position: "MySQL56/3E11FA47-71CA-11E1-9E33-C80AA9429562:1-5",
BackingUp: true,
},
},
},
tolerableReplLag: 50 * time.Second,
shardInfo: topo.NewShardInfo("testkeyspace", "-", &topodatapb.Shard{
PrimaryAlias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 100,
},
}, nil),
tabletMap: map[string]*topo.TabletInfo{
"primary": {
Tablet: &topodatapb.Tablet{
Alias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 100,
},
Type: topodatapb.TabletType_PRIMARY,
},
},
"replica1": {
Tablet: &topodatapb.Tablet{
Alias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 101,
},
Type: topodatapb.TabletType_REPLICA,
},
},
"replica2": {
Tablet: &topodatapb.Tablet{
Alias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 102,
},
Type: topodatapb.TabletType_REPLICA,
},
},
},
avoidPrimaryAlias: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 0,
},
expected: &topodatapb.TabletAlias{
Cell: "zone1",
Uid: 102,
},
errContains: nil,
},
{
name: "found a replica - more advanced relay log position",
tmc: &chooseNewPrimaryTestTMClient{
Expand Down

0 comments on commit 54bdcc0

Please sign in to comment.