Make PRS & ERS new primary selection MySQL version aware#20211
Make PRS & ERS new primary selection MySQL version aware#20211ejortegau wants to merge 13 commits into
Conversation
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Patch version differences within the same MySQL release do not affect replication compatibility, so the version-aware candidate election now ignores them. Only major.minor boundaries trigger the preference for a lower-version primary. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
ejortegau
requested review from
frouioui,
harshit-gangal,
mattlord,
rohit-nayak-ps,
shlomi-noach and
timvaillancourt
as code owners
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR teaches the reparent (PRS/ERS) selection logic to prefer lower MySQL versions when choosing a new primary, in order to preserve replication compatibility (replicas must be at the same or higher version than the primary). The tablet's MySQL version is now propagated through the replicationdata.Status proto and incorporated as a tie-breaker after replication position and promotion rules.
Changes:
- Adds
server_versiontoreplicationdata.Statusand populates it fromMysqlDaemon.GetVersionStringinReplicationStatus/StopReplicationAndGetStatus. - Threads a per-tablet MySQL
ServerVersionmap throughsortTabletsForReparent,findMostAdvanced,findCandidate, and related helpers, applying lowest-release-first ordering after promotion rules. - Exports
AtLeast/IsSameReleaseand addsReleaseAtLeastonServerVersion, updating existing callers.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/replicationdata.proto | Adds server_version field (tag 30) to Status. |
| go/vt/vttablet/tabletmanager/rpc_replication.go | Populates ServerVersion on status responses. |
| go/vt/mysqlctl/version.go | Exports AtLeast/IsSameRelease; adds ReleaseAtLeast. |
| go/vt/mysqlctl/capabilityset.go | Updates callers to exported method names. |
| go/vt/mysqlctl/backupengine.go | Updates callers to exported method names. |
| go/vt/vtctl/reparentutil/reparent_sorter.go | Adds MySQL-version tie-breaker; new length validation; defines unknownVersion. |
| go/vt/vtctl/reparentutil/util.go | Propagates server version through electing logic and findCandidate. |
| go/vt/vtctl/reparentutil/replication.go | Collects MySQL versions into replicationSnapshot. |
| go/vt/vtctl/reparentutil/emergency_reparenter.go | Plumbs versionMap into ERS candidate selection. |
| go/vt/vtctl/reparentutil/reparent_sorter_test.go | New tests for version-aware sorting and length validation. |
| go/vt/vtctl/reparentutil/util_test.go | New tests for version-aware election and findCandidate. |
| go/vt/vtctl/reparentutil/emergency_reparenter_test.go | New tests for findMostAdvanced version preference. |
Files not reviewed (2)
- go/vt/proto/replicationdata/replicationdata.pb.go: Language not supported
- go/vt/proto/replicationdata/replicationdata_vtproto.pb.go: Language not supported
| if len(versionMap) == 0 { | ||
| // No version data — fall back to preferring the intermediate source to avoid catch-up. | ||
| for _, candidate := range possibleCandidates { | ||
| if topoproto.TabletAliasEqual(intermediateSource.Alias, candidate.Alias) { | ||
| return candidate | ||
| } | ||
| } | ||
| return possibleCandidates[0] | ||
| } |
| // If best is on the same release as the intermediate source, prefer intermediate source to avoid catch-up. | ||
| if sourceVersion.IsSameRelease(bestVersion) { | ||
| for _, candidate := range possibleCandidates { | ||
| if topoproto.TabletAliasEqual(intermediateSource.Alias, candidate.Alias) { | ||
| return candidate | ||
| } | ||
| } | ||
| } |
| after := replication.ReplicationStatusToProto(rsAfter) | ||
| after.BackupRunning = tm.IsBackupRunning() | ||
| after.ServerVersion = before.ServerVersion |
|
|
||
| // reparentSorter sorts tablets by GTID positions and Promotion rules aimed at finding the best | ||
| // candidate for intermediate promotion in emergency reparent shard, and the new primary in planned reparent shard | ||
| var unknownVersion = mysqlctl.ServerVersion{Major: math.MaxInt, Minor: math.MaxInt, Patch: math.MaxInt} |
| @@ -181,25 +193,25 @@ func findTabletPositionLagBackupStatus(ctx context.Context, tablet *topodatapb.T | |||
| sqlErr, isSQLErr := sqlerror.NewSQLErrorFromError(err).(*sqlerror.SQLError) | |||
| if isSQLErr && sqlErr != nil && sqlErr.Number() == sqlerror.ERNotReplica { | |||
| logger.Warningf("no replication statue from %v, using empty gtid set", topoproto.TabletAliasString(tablet.Alias)) | |||
|
Promptless prepared documentation updates related to this change. Triggered by PR #20211 This PR introduces MySQL version-aware primary selection in PRS and ERS. The documentation updates explain the new candidate election criteria (GTID position → promotion rules → MySQL version → InnoDB buffer pool size → tablet alias), the replication compatibility rationale, and guidance for rolling MySQL upgrades. Documentation suggestions:
|
timvaillancourt
added
Component: VTOrc
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20211 +/- ##
===========================================
- Coverage 69.67% 62.90% -6.78%
===========================================
Files 1614 260 -1354
Lines 216793 51083 -165710
===========================================
- Hits 151044 32132 -118912
+ Misses 65749 18951 -46798
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
In go/vt/vtctl/reparentutil/reparent_sorter.go:83-121 the version only wins on exact position ties, so I think that PRS can still promote a newer MySQL release during rolling upgrades because ElectNewPrimary still sorts replication position before MySQL release. For PRS, the current primary is healthy and performGracefulPromotion later waits the chosen primary-elect to catch up to both a primary snapshot and the demotion position, so a slightly-behind lower-release replica is usually safe and intended to catch up. With the current ordering, an 8.4 replica that is one transaction ahead of an 8.0 replica will still be auto-selected, which is exactly the rolling-upgrade failure mode this PR is trying to prevent for older replicas. Please add a PRS regression test where an 8.4 candidate is slightly ahead of an 8.0 candidate and make the lower release win when it is otherwise eligible.
In go/vt/vtctl/reparentutil/replication.go:321-339 ERS primary-status candidates are always treated as unknown version. When StopReplicationAndGetStatus returns ERNotReplica, the code demotes the tablet and records it in primaryStatusMap, but never records its MySQL version in mysqlVersions. Later ERS maps missing versions to unknownVersion, so reachable primary-like candidates are sorted after known-version replicas even when they are the lower compatible release. That leaves a gap in ERS version-aware election for contested-primary/split-brain cases. Please carry server version through this path too, likely by adding it to PrimaryStatus or otherwise populating res.mysqlVersions for demoted primary-status candidates.
PRS always catches the elected tablet up to the old primary's exact demotion position, so replication position head-start is irrelevant for data safety. This change introduces SortMode (SortForPRS/SortForERS) to give PRS a distinct sort order: promotion rules > MySQL version > position > buffer pool > alias. ERS retains position-first ordering to minimize data loss. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
|
Hi, @mattlord , Thanks for the feedback, I I have addressed it. |
|
👋 @ejortegau — nice work on this. The version-aware preference feels like the right trade — a louder, recoverable failure during a mixed-version window beats a silent post-PRS replication break that you only notice when older-version replicas start erroring on relay-log apply. A few small follow-ups worth folding in:
Otherwise LGTM 👍 |
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
ERS primary-status candidates still seem to lose MySQL version data in go/vt/vtctl/reparentutil/replication.go:321-338 as it handles ERNotReplica by demoting the tablet and storing only primaryStatusMap. The version map is populated only on the normal StopReplicationAndGetStatus path at replication.go:359-363. Later, findMostAdvanced maps missing versions to unknownVersion, so reachable primary-like/split-brain candidates are sorted as unknown even if they are actually the lower compatible MySQL release. This leaves a gap in the ERS protection this PR is adding. Please carry server version through the DemotePrimary/PrimaryStatus path too.
The optional version lookup seems to happen before stopping replication in go/vt/vttablet/tabletmanager/rpc_replication.go:1078-1082 as it calls GetVersionString(ctx) before stopIOThreadLocked / stopReplicationLocked. This is optional metadata, but it shares the stop RPC context and runs while holding the tabletmanager lock. If that version query or fallback path is slow, it can delay or consume the timeout before the actual stop, making ERS more brittle. Please collect this after replication is stopped, or otherwise ensure version collection cannot delay/fail the stop path.
Lastly, the Copilot note about log.Warn(..., slog.Any(...)) still looks valid.
And the PR description still says the sorter order is GTID position > promotion rules > MySQL release, while PRS now intentionally uses promotion rules > MySQL release > position.
- Remove unused `after.ServerVersion` assignment in StopReplicationAndGetStatus - Log warning when MySQL version string fails to parse - Clarify findCandidate post-loop comment explaining two-phase logic - Add v25 changelog entry with deployment note and cross-cell limitation Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 16 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- go/vt/proto/replicationdata/replicationdata.pb.go: Language not supported
- go/vt/proto/replicationdata/replicationdata_vtproto.pb.go: Language not supported
| } else { | ||
| fmt.Fprintf(&reasonsToInvalidate, "\n%v has %v replication lag which is more than the tolerable amount", topoproto.TabletAliasString(tablet.Alias), replLag) |
| v := unknownVersion | ||
| if serverVersion != "" { | ||
| _, parsed, parseErr := mysqlctl.ParseVersionString(serverVersion) | ||
| if parseErr == nil { | ||
| v = parsed | ||
| } else { | ||
| logger.Warningf("failed to parse MySQL version %q for tablet %v: %v", serverVersion, topoproto.TabletAliasString(tb.Alias), parseErr) | ||
| } | ||
| } | ||
| mysqlVersions = append(mysqlVersions, v) |
| if version, vErr := tm.MysqlDaemon.GetVersionString(ctx); vErr == nil { | ||
| protoStatus.ServerVersion = version | ||
| } else { | ||
| log.Warn("failed to get MySQL version string", slog.Any("error", vErr)) | ||
| } |
| if v.IsSameRelease(bestVersion) || v.ReleaseAtLeast(bestVersion) { | ||
| continue | ||
| } |
- Add `server_version` field to `PrimaryStatus` proto so that DemotePrimary and PrimaryStatus RPCs report the MySQL version - Populate version in PrimaryStatus, DemotePrimary, and StopReplicationAndGetStatus RPCs - Move GetVersionString call after replication stop in StopReplicationAndGetStatus to avoid delaying the critical path - Read PrimaryStatus.ServerVersion in ERS ERNotReplica path so demoted primary-status candidates no longer get unknownVersion - Extract getMySQLVersion helper to deduplicate the fetch-and-warn pattern across 4 call sites - Add test coverage for version propagation through both paths Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
ReleaseAtLeast already covers the same-release case (Minor >=), so the redundant IsSameRelease check can be removed. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
The current Check Make VTAdmin Web Proto job is failing because regenerating vtadmin web protos changes both committed files. Please rerun the vtadmin web proto generation and commit the updated artifacts.
In go/vt/vttablet/tabletmanager/rpc_replication.go:1091-1116 StopReplicationAndGetStatus only populates before.ServerVersion at the end of the full success path. The early-success paths for already-stopped IO replication or already-unhealthy replication return Before/After before the version is set. ERS builds its version map from StopReplicationStatus.Before.ServerVersion, so a reachable older-version tablet in this state becomes unknownVersion and can lose the version-aware election to a newer tablet. Please populate ServerVersion before all success returns, and add a regression test for an already-stopped replica.
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
StopReplicationAndGetStatus had several early-return paths (IO thread already stopped, replication not healthy, stop failures, after-status failure) that returned Before without populating ServerVersion. ERS builds its version map from Before.ServerVersion, so tablets hitting these paths became unknownVersion and could lose version-aware election to newer tablets. Fix: call getMySQLVersion(ctx) before every return that includes Before. Add a table-driven test covering all return paths (success and error) for both IOTHREADONLY and IOANDSQLTHREAD modes. Ref: vitessio#20211 (review) Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- go/vt/proto/replicationdata/replicationdata.pb.go: Language not supported
- go/vt/proto/replicationdata/replicationdata_vtproto.pb.go: Language not supported
Comments suppressed due to low confidence (2)
go/vt/vttablet/tabletmanager/rpc_replication.go:1
tm.getMySQLVersion(ctx)can be called multiple times in a singleStopReplicationAndGetStatusexecution, potentially causing repeatedMysqlDaemon.GetVersionStringcalls (extra round trips) along error/early-return paths. Consider retrieving the version once near the start of the method and reusing it for all branches.
/*
go/vt/vttablet/tabletmanager/rpc_replication.go:1
tm.getMySQLVersion(ctx)can be called multiple times in a singleStopReplicationAndGetStatusexecution, potentially causing repeatedMysqlDaemon.GetVersionStringcalls (extra round trips) along error/early-return paths. Consider retrieving the version once near the start of the method and reusing it for all branches.
/*
| func (tm *TabletManager) getMySQLVersion(ctx context.Context) string { | ||
| version, err := tm.MysqlDaemon.GetVersionString(ctx) | ||
| if err != nil { | ||
| log.Warn(fmt.Sprintf("failed to get MySQL version string: %v", err)) | ||
| return "" | ||
| } | ||
| return version | ||
| } |
| rs.FilePosition = rsAfter.FilePosition | ||
| rs.RelayLogSourceBinlogEquivalentPosition = rsAfter.RelayLogSourceBinlogEquivalentPosition | ||
|
|
||
| before.ServerVersion = tm.getMySQLVersion(ctx) |
| before.ServerVersion = tm.getMySQLVersion(ctx) | ||
|
|
||
| return StopReplicationAndGetStatusResponse{ | ||
| Status: &replicationdatapb.StopReplicationStatus{ | ||
| Before: before, |
| - **[VTCtld](#minor-changes-vtctld)** | ||
| - [MySQL version-aware reparent candidate election](#vtctld-version-aware-reparent) |
StopReplicationAndGetStatus had several early-return paths (IO thread already stopped, replication not healthy, stop failures, after-status failure) that returned Before without populating ServerVersion. ERS builds its version map from Before.ServerVersion, so tablets hitting these paths became unknownVersion and could lose version-aware election to newer tablets. Fix: call getMySQLVersion(ctx) before every return that includes Before. Add a table-driven test covering all return paths (success and error) for both IOTHREADONLY and IOANDSQLTHREAD modes. Ref: vitessio#20211 (review) Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
This is looking good. Just one relatively minor things... In go/vt/vttablet/tabletmanager/rpc_replication.go:1142-1158 StopReplicationAndGetStatus now populates Before.ServerVersion, but the normal successful stop path returns an After status without ServerVersion. The early no-op paths use After: before, so they include it, making the common success path inconsistent. Please set after.ServerVersion = before.ServerVersion and extend TestStopReplicationAndGetStatus_ServerVersion to assert After.ServerVersion when After is present.
Thank you @ejortegau !
The successful stop path set Before.ServerVersion but left After.ServerVersion empty, while the no-op paths return After: before and thus include it. Set after.ServerVersion = before.ServerVersion so the common success path is consistent, and extend the test to assert After.ServerVersion when present. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Eduardo Ortega <5791035+ejortegau@users.noreply.github.com>
|
Made the requested changes, @mattlord . Please let me know if there is anything else that needs adjusting. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 8 comments.
Files not reviewed (2)
- go/vt/proto/replicationdata/replicationdata.pb.go: Generated file
- go/vt/proto/replicationdata/replicationdata_vtproto.pb.go: Generated file
| sqlErr, isSQLErr := sqlerror.NewSQLErrorFromError(err).(*sqlerror.SQLError) | ||
| if isSQLErr && sqlErr != nil && sqlErr.Number() == sqlerror.ERNotReplica { | ||
| logger.Warningf("no replication statue from %v, using empty gtid set", topoproto.TabletAliasString(tablet.Alias)) |
| func (tm *TabletManager) getMySQLVersion(ctx context.Context) string { | ||
| version, err := tm.MysqlDaemon.GetVersionString(ctx) | ||
| if err != nil { | ||
| log.Warn(fmt.Sprintf("failed to get MySQL version string: %v", err)) | ||
| return "" | ||
| } | ||
| return version | ||
| } |
| if !rs.IOHealthy() { | ||
| before.ServerVersion = tm.getMySQLVersion(ctx) |
| if err := tm.stopIOThreadLocked(ctx); err != nil { | ||
| before.ServerVersion = tm.getMySQLVersion(ctx) |
| if !rs.Healthy() { | ||
| // no replication is running, just return what we got | ||
| before.ServerVersion = tm.getMySQLVersion(ctx) |
| if err := tm.stopReplicationLocked(ctx); err != nil { | ||
| before.ServerVersion = tm.getMySQLVersion(ctx) |
| if err != nil { | ||
| before.ServerVersion = tm.getMySQLVersion(ctx) |
| before.ServerVersion = tm.getMySQLVersion(ctx) | ||
| after.ServerVersion = before.ServerVersion |
Description
This PR makes PlannedReparentShard (PRS) and EmergencyReparentShard (ERS) aware of MySQL server versions when selecting a new primary.
MySQL replication requires that replicas run at the same or higher version than the primary. During rolling MySQL upgrades, a reparent that promotes a newer-version tablet can break replication for tablets still on the older version. This change adds MySQL version as a factor in candidate election so that a lower-version tablet is preferred as the new primary.
How it works:
server_versionfield is added to thereplicationdata.Statusproto, populated byReplicationStatusandStopReplicationAndGetStatusRPCs.Backport justification: This is a correctness fix for MySQL replication safety. Without version-aware election, a reparent during a rolling MySQL upgrade can promote a newer-version primary, causing older-version replicas to break replication. MySQL only guarantees forward replication compatibility (old primary → new replica), not backward (new primary → old replica). This affects any operator performing rolling MySQL upgrades on Vitess clusters.
Related Issue(s)
Fixes #20210
Checklist
Deployment Notes
No migrations or flag changes required. The version-aware election is automatic once tablets report
server_versionin their replication status. Tablets on older Vitess versions that don't populate the field are treated as "unknown version" and sorted last, preserving existing behavior.Behavior change during rolling MySQL upgrades: PRS now prefers lower-version candidates over higher-version ones, even when the lower-version tablet is slightly behind in replication position. This means PRS may elect a tablet that requires catch-up to the old primary's demotion position, which can increase reparent latency. Operators performing rolling MySQL upgrades should ensure
--wait-replicas-timeoutis generous enough to accommodate this catch-up time.It is also worth mentioning that because this is done at the candidate-sorting level, a PRS that does not specify
--allow-cross-cell-promotioncould still promote a higher-version tablet in the same cell as the current primary instead of a lower-version tablet in a different cell. The cell boundary remains a hard filter — operators who want cross-cell promotion during upgrades can opt in with--allow-cross-cell-promotion.AI Disclosure
This PR was co-authored with Claude Code, which helped with implementation, testing, and the version-comparison refinement.