fix: refuse switchover when target replica has fatal replication error - #1
Merged
Merged
Conversation
Field-incident pattern reproduced on moodle-education-stg-db and
moodle-education-prod-db within the same week: spec.replication.primary.podIndex
was pointed at a replica whose SQL thread had aborted (errno 1062, Duplicate
entry on moodle.mdl_task_log), and the switchover state machine then proceeded
into lockPrimaryWithReadLock and setPrimaryReadOnly anyway — degrading the
still-healthy current primary to read_only=ON — before reaching
waitForReplicaSync, where MASTER_GTID_WAIT timed out every iteration because a
replica whose SQL thread has aborted cannot advance its gtid_current_pos no
matter how long we wait. Each reconcile retried the same sequence
indefinitely; both clusters stayed wedged for 5+ days until manual rollback of
spec.replication.primary.podIndex.
The retry loop is structurally bad even ignoring the wedge: locking the
healthy primary read-only at the start of every reconcile, then unlocking it
via the stale-switchover reset on the next reconcile when the spec is rolled
back, creates a churn of FLUSH TABLES WITH READ LOCK / UNLOCK TABLES against
the live workload for as long as the wedge persists.
Add an early guard at the top of reconcileSwitchover, after the stale-switchover
reset and after shouldReconcileSwitchover. targetReplicaError inspects the
target replica's last-observed status (MariaDB.Status.Replication.Replicas,
populated by the status reconciler from previous loops) and refuses to proceed
if any of the following is true:
- LastSQLErrno != 0 (fatal SQL thread error, the 1062 wedge mode)
- LastIOErrno != 0 (fatal IO thread error, e.g. errno 1236 binlog-missing)
- Slave_SQL_Running=No with errno=0 (manually stopped or slave_skip_errors
aftermath)
- Slave_IO_Running=No with errno=0 (network drop or manual stop)
Each path surfaces an actionable error message and emits a Warning event:
"switchover target replica '<pod>' has unrecoverable SQL thread error
(errno 1062): Duplicate entry '71306391' for key 'PRIMARY'; recover the
replica before retrying the switchover"
Reading from cached status (not a live SQL query) is deliberate — in the
moodle-education-prod-db incident the target's mariadb container was in
CrashLoopBackOff with 2376 restarts, and a live ReplicaStatus call would
itself have failed with "invalid connection" and masked the real cause. The
status reconciler had recorded the last good observation before the container
started bouncing.
The Slave_*_Running checks default-to-true via ptr.Deref(_, true) so an
uninitialized replica (status never observed yet) does not block the very
first switchover; the guard fires only when the status reconciler has
populated an explicit false.
Note: this does not unblock auto-recovery on the wedged target — that path
remains gated by shouldReconcileReplicaRecovery's IsSwitchingPrimary /
IsReplicationSwitchoverRequired exclusion. Plumbing recovery to run
concurrently with a wedged switchover is a larger behavioural change tracked
as a follow-up; this commit narrowly addresses the "infinite retry that
keeps degrading the healthy primary" half of the problem and surfaces a
clear, actionable error to operators.
Tests:
- targetReplicaError is a pure status-reader → covered by a table-driven
unit test with ten cases: nil status, target == current primary, healthy
target, SQL errno 1062, IO errno 1236, SQL thread stopped without errno,
IO thread stopped without errno, nil thread-running fields default to
true, target absent from map, unset PodIndex. The end-to-end
reconcileSwitchover behaviour follows the existing convention in this
file (TestWaitForReplicaSync_* / TestWaitForNewPrimarySync_*) of
integration-test-only coverage with a documented design record here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an early guard in
reconcileSwitchoverthat refuses to start a switchover when the target replica's last-observedStatus.Replication.Replicas[pod]indicates a fatal replication state (non-zeroLastSQLErrno/LastIOErrno, orSlave_SQL_Running=No/Slave_IO_Running=Nowith errno zero).Without this guard the switchover state machine repeatedly locked the still-healthy current primary read-only and then timed out in
MASTER_GTID_WAITfor as long as the wedge persisted — observed on moodle-education-stg-db and moodle-education-prod-db within a week of each other, both stuck for 5+ days witherrno 1062(Duplicate entry onmoodle.mdl_task_log) until manual rollback ofspec.replication.primary.podIndex.Why read cached status, not live SQL
In the prod incident the target's
mariadbcontainer was inCrashLoopBackOffwith 2376 restarts. A liveReplicaStatuscall would itself have failed withinvalid connectionand masked the actual cause.MariaDB.Status.Replication.Replicasis populated by the status reconciler in earlier loops and carries the last good observation across the bouncing container.Failure modes the guard catches
LastSQLErrno != 01062Duplicate entry onmdl_task_log(this PR's seed)LastIOErrno != 01236master is missing the GTIDSlave_SQL_Running=No(errno=0)slave_skip_errorsaftermath, manualSTOP SLAVE SQL_THREADSlave_IO_Running=No(errno=0)SlaveSQLRunning/SlaveIORunningdefault totrueviaptr.Deref(_, true)so an uninitialized replica (status never observed yet) does not block the very first switchover; the guard fires only after the status reconciler has populated an explicitfalse.Explicit non-goal
This does not unblock auto-recovery on the wedged target — that path remains gated by
shouldReconcileReplicaRecovery'sIsSwitchingPrimary/IsReplicationSwitchoverRequiredexclusion. Plumbing recovery to run concurrently with a wedged switchover is a larger behavioural change and tracked as a follow-up. This PR narrowly addresses the infinite-retry-that-keeps-degrading-the-healthy-primary half.Test plan
targetReplicaError(TestTargetReplicaError_GuardsAgainstWedgedTarget)Slave_SQL_Running=Nowith errno=0Slave_IO_Running=Nowith errno=0go test ./pkg/controller/replication/... -racecleangofmt,go vet, fullgo build ./...cleanreconcileSwitchovercall site follows existing convention in this file (integration-test-only with a documented design record)