K8SPS-852 errantTransactionsPolicy: report quarantined members NotReady - #1412
Open
hors wants to merge 10 commits into
Open
K8SPS-852 errantTransactionsPolicy: report quarantined members NotReady#1412hors wants to merge 10 commits into
hors wants to merge 10 commits into
Conversation
Readiness: an async member that the bootstrap quarantines for errant transactions now reports NotReady. checkReadinessAsync stats the existing quarantine marker (<datadir>/quarantine) and fails the probe when present, so the divergence surfaces in the cluster status and halts rollouts instead of a diverged member silently counting as ready. rebuild deletes the pod and inject-empty clears the marker on rejoin, so only the manual policy leaves a member persistently NotReady. (Reverses the prior "stay Ready" behavior; the OrderedReady scale/rollout stall while quarantined is the intended signal.) E2e test (async-errant-transactions): exercises all three policies end to end plus the quarantine path: - manual: ErrantGTIDsDetected emitted, member untouched, errant DB survives - rebuild: ErrantMemberRebuild, cluster reconverges 3/3, errant DB gone - inject-empty: ErrantGTIDsInjectEmpty, member rejoins, errant DB kept locally - quarantine: forge errant txn + restart -> member returns NotReady with the quarantine marker (the same path a failed-over old primary hits)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes async members that are quarantined due to errant transactions report NotReady (via a quarantine marker file), and adds operator-side reconciliation to resolve/quarantine/join such members according to spec.mysql.errantTransactionsPolicy, so divergence is surfaced in cluster status and rollouts are halted instead of silently proceeding.
Changes:
- Add a quarantine marker (
/var/lib/mysql/quarantine) and update async readiness to fail when it exists. - Extend async replication reconciliation to (a) repair broken replicas after takeover, and (b) resolve “quarantined / unjoined” members via rebuild/inject-empty/manual policy flows.
- Add an end-to-end KUTTL test (
async-errant-transactions) covering manual/rebuild/inject-empty and quarantine behavior; register it in the PR test suite.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/orchestrator/client.go | Add Orchestrator instance fields for errant GTIDs/thread states and an API wrapper for gtid-errant-inject-empty. |
| pkg/mysql/mysql.go | Introduce a shared constant for the quarantine marker file path. |
| pkg/db/replication.go | Add helpers for read-only detection, GTID set operations, and empty-GTID injection (pod-exec based). |
| pkg/controller/ps/controller.go | Enhance async replication reconciliation: repair broken replicas and manage quarantined/unjoined members per policy. |
| pkg/controller/ps/upgrade.go | Avoid rollout deadlock by recreating unready outdated pods during SmartUpdate. |
| cmd/healthcheck/main.go | Make async readiness fail when quarantine marker exists. |
| cmd/bootstrap/async/async_replication.go | Quarantine members with errant GTIDs instead of cloning over local transactions; clear marker on successful join. |
| cmd/internal/db/db.go | Ensure CHANGE REPLICATION SOURCE sets GET_SOURCE_PUBLIC_KEY=1; return source host even when replication is stopped; add GTID_SUBTRACT helper. |
| api/v1/perconaservermysql_types.go | Add errantTransactionsPolicy API field with enum + default. |
| api/v1/perconaservermysql_types_test.go | Add/adjust a defaults/validation test case (backup disabled with image). |
| deploy/crd.yaml, deploy/bundle.yaml, deploy/cw-bundle.yaml, config/crd/bases/... | CRD schema updates for errantTransactionsPolicy (enum + default). |
| deploy/cr.yaml | Document errantTransactionsPolicy in the example CR. |
| cmd/example-gen/scripts/lib/ps.sh | Exclude errantTransactionsPolicy from commented example fields. |
| cmd/example-gen/pkg/defaults/manual.go | Set manual default for generated example manifests. |
| e2e-tests/functions | Add helper to find an async replica by @@super_read_only. |
| e2e-tests/tests/async-errant-transactions/* | New KUTTL E2E test steps for errant transaction policies and quarantine NotReady behavior. |
| e2e-tests/run-pr.csv | Register the new E2E test for PR runs (8.4). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+150
| q := fmt.Sprintf("SELECT GTID_SUBTRACT('%s', '%s') AS diff", a, b) | ||
| err := m.query(ctx, q, &rows) |
Comment on lines
+192
to
+194
| // gtidSetRe matches one uuid:intervals element of a GTID set, | ||
| // e.g. 3e11fa47-71ca-11e1-9e33-c80aa9429562:1-5:11. | ||
| var gtidSetRe = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) |
Comment on lines
+104
to
+106
| if _, err := os.Stat(mysql.QuarantineFile); err == nil { | ||
| return errors.New("member is quarantined due to errant transactions; see the ErrantGTIDsDetected event") | ||
| } |
egegunes
reviewed
Jun 19, 2026
Contributor
There was a problem hiding this comment.
i think it'd better to move all new functions to new file called async.go
hors
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
cmd/healthcheck/main.go:106
os.Staterrors other than "file does not exist" are currently ignored, which can incorrectly mark the pod Ready if the quarantine marker can’t be accessed (e.g. permission/IO errors). Treat unexpected stat failures as readiness failures so the condition is visible.
if _, err := os.Stat(mysql.QuarantineFile); err == nil {
return errors.New("member is quarantined due to errant transactions; see the ErrantGTIDsDetected event")
}
pkg/controller/ps/async.go:365
- Same PVC-deletion safety concern here: deleting by name without UID preconditions can delete a freshly reprovisioned PVC if this reconcile path repeats. Fetch the PVC and delete it with UID preconditions to avoid rebuild loops and accidental data loss.
pvc := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%s-%s", mysql.DataVolumeName, mysqlPod.Name),
Namespace: cr.Namespace,
},
}
if err := r.Delete(ctx, pvc); client.IgnoreNotFound(err) != nil {
return errors.Wrapf(err, "delete PVC %s", pvc.Name)
}
if err := r.Delete(ctx, mysqlPod); client.IgnoreNotFound(err) != nil {
return errors.Wrapf(err, "delete pod %s", mysqlPod.Name)
}
pkg/db/replication.go:150
- Building the GTID_SUBTRACT query via string interpolation can break on unexpected characters (and is harder to reason about from a security perspective). At minimum, escape single quotes before embedding the GTID sets.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (4)
pkg/db/replication.go:150
GTIDSubtractbuilds SQL viafmt.Sprintfwith unescaped string literals. Even if inputs are usually server-generated, a malformed/trimmed GTID string containing a quote would break the query. Escape quotes before interpolation (or switch to a parameterized query path if available).
pkg/db/replication.go:194- The
gtidSetRecomment says it matches a full "uuid:intervals" element, but the regex only matches the UUID prefix. Adjust the comment to match actual behavior to avoid confusion for future edits.
cmd/healthcheck/main.go:106 os.Stat()errors other than nil (e.g. permission/IO errors) are currently ignored, which could let a quarantined member report Ready even though the marker exists but can't be stat'ed. Treat non-IsNotExisterrors as probe failures (or at least surface them).
if _, err := os.Stat(mysql.QuarantineFile); err == nil {
return errors.New("member is quarantined due to errant transactions; see the ErrantGTIDsDetected event")
}
pkg/controller/ps/async.go:67
- In the initial pod scan,
primaryPodis only set afterIsReadonly()succeeds. IfIsReadonly()fails for the primary pod, the loopcontinues andprimaryPodstays nil, causing this function to return early and skip primary confirmation/quarantine reconciliation. SetprimaryPodbefore the read-only check, and log the read-only check error instead of silently skipping.
um := database.NewReplicationManager(pod, r.ClientCmd, apiv1.UserOperator, operatorPass, "127.0.0.1")
readOnly, err := um.IsReadonly(ctx)
if err != nil {
continue
}
if !readOnly {
writableExists = true
}
if mysql.PodFQDN(cr, pod) == primary.Key.Hostname {
primaryPod = pod
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cmd/healthcheck/main.go:106
- checkReadinessAsync treats the quarantine marker as present only when os.Stat returns nil, but it silently ignores other Stat errors (e.g. permission/IO). In those cases the readiness probe would incorrectly report Ready even though it can’t reliably determine quarantine state.
// A quarantined member (errant transactions, not joined to the cluster) is
// reported NotReady so the divergence surfaces in the cluster status and
// halts rollouts. rebuild/inject-empty clear the marker quickly; only the
// manual policy leaves it in place.
if _, err := os.Stat(mysql.QuarantineFile); err == nil {
return errors.New("member is quarantined due to errant transactions; see the ErrantGTIDsDetected event")
}
pkg/db/replication.go:151
- GTIDSubtract builds the SQL string by interpolating raw GTID set strings into single-quoted literals. If the GTID strings ever contain quotes or backslashes (unexpected formatting, corrupted output, etc.), this can break the query or be interpreted as additional SQL. It’s safer to escape before embedding into a SQL literal.
pkg/db/replication.go:194 - The gtidSetRe comment says it matches a full "uuid:intervals" element, but the regexp only matches the UUID portion. This is misleading when maintaining/using expandGTIDSet.
hors
marked this pull request as ready for review
August 5, 2026 19:57
hors
requested review from
eleo007,
gkech,
jvpasinatto,
nmarukovich,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
August 5, 2026 19:57
hors
marked this pull request as draft
August 5, 2026 22:41
Collaborator
commit: 33fbcb6 |
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
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.
Readiness: an async member that the bootstrap quarantines for errant transactions now reports NotReady. checkReadinessAsync stats the existing quarantine marker (/quarantine) and fails the probe when present, so the divergence surfaces in the cluster status and halts rollouts instead of a diverged member silently counting as ready. rebuild deletes the pod and inject-empty clears the marker on rejoin, so only the manual policy leaves a member persistently NotReady. (Reverses the prior "stay Ready" behavior; the OrderedReady scale/rollout stall while quarantined is the intended signal.)
E2e test (async-errant-transactions): exercises all three policies end to end plus the quarantine path:
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability