Add read-only reader sites#116
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThis change adds first-party read-only replica sites with role-aware validation, per-site Service and MySQL configuration, direct replication-source convergence, safer ordered updates, reader-specific observability, expanded playground topology, and supporting tests and documentation. ChangesRead-only API and schema
Controller convergence and reconciliation
Operational validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class non-promotable read-only reader sites to Bloodraven (API/CRD + controller behavior), including direct-primary replication source convergence, reader-isolated readiness semantics, per-site Service/MySQL config overrides, and updated playground/e2e coverage to validate N-site rollouts.
Changes:
- Introduce
read-onlySiteRole with conditional placement validation, per-site Service templates, per-site MySQL config overrides, and reader-specific lag thresholds. - Implement and test direct-primary replication source convergence (GTID-gated repoint/restart) plus new source-convergence metrics/status fields.
- Update playground tooling, scenarios, and documentation for a 3-site default topology (2 candidates + 1 reader) and release-profile coverage.
Reviewed changes
Copilot reviewed 79 out of 80 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/envtest/reconciler_test.go | Envtest validation/transition coverage for read-only, serviceTemplate rules, and ordered-update deferral. |
| test/component/source_convergence_test.go | Component test for source convergence after restart (no failover history). |
| test/component/helpers_test.go | Mock MySQL updates to reflect replication source/thread state mutations. |
| playground/setup.sh | Require 3 workers; label nodes for {iad,pdx,reader}; adapt waits and user creation to N sites. |
| playground/rebuild.sh | Derive MySQL deployments from CR sites for sidecar rebuild restarts. |
| playground/manifests/failovergroup.yaml | Add reader site with site-only Service/MySQL overrides and reader lag threshold. |
| playground/chaos.sh | Generalize commands to arbitrary site names; NetworkPolicy partition; node lookup via site label. |
| internal/state/matrix.go | Add read-only role; fence writable non-promotables; exclude readers from core readiness evaluation. |
| internal/state/matrix_test.go | Tests for reader isolation and fencing semantics. |
| internal/playground/scenarios/s40_observer_test.go | Unit tests for scenario-40 observer error handling and transitions. |
| internal/playground/scenarios/s34_operator_kill_during_wait_replica.go | Adjust invariants/messages to “N-1 followers read-only”. |
| internal/playground/scenarios/s22_replication_status_after_recovery.go | Adjust invariants/messages for N-site convergence. |
| internal/playground/scenarios/s19_reclone_interlock.go | Adjust convergence predicate to N-site topology. |
| internal/playground/scenarios/s18_rapid_cr_spec_changes.go | Update scenario assertions to N-site topology. |
| internal/playground/scenarios/s16_mysql_process_kill.go | Update convergence expectations for N-site topology. |
| internal/playground/scenarios/s13_operator_kill_during_bootstrap.go | Update convergence expectations for N-site topology. |
| internal/playground/scenarios/s12_rolling_update_healthy_state.go | Update ordered-update scenario wording/assertions for N sites. |
| internal/playground/scenarios/s12_old_primary_recovery.go | Update reconvergence predicate for N-site topology. |
| internal/playground/scenarios/s11_total_loss_recovery.go | Update total-loss scenario for scaling every site to 0 and N-site reconvergence. |
| internal/playground/scenarios/s10_full_bootstrap_after_data_wipe.go | Update bootstrap reconvergence predicate for N-site topology. |
| internal/playground/scenarios/s08_gtid_divergence_detection.go | Update divergence-cleanup predicate for N-site topology. |
| internal/playground/scenarios/s06_self_fence_isolated_primary.go | Scale all peers (not just one) to isolate primary in N-site topology. |
| internal/playground/scenarios/s05_operator_kill_during_failover.go | Update convergence predicate for N-site topology. |
| internal/playground/scenarios/s02_planned_switchover.go | Add verification that every follower directly replicates from new primary after switchover. |
| internal/playground/scenarios/inventory_40_test.go | Ensures scenario 40 is registered + release-profiled; adds host canonicalization/reader assertions tests. |
| internal/playground/runner/profile.go | Extend release profile with scenario 40. |
| internal/playground/runner/profile_registry_test.go | Update release profile expected count and membership. |
| internal/playground/runner/executor.go | Update cleanup/reconverge wording for N-site topology. |
| internal/playground/logs/matcher.go | Add structured log matcher supporting JSON + slog-text formats. |
| internal/playground/logs/matcher_test.go | Tests structured matcher against JSON and text log lines. |
| internal/playground/kube/endpointslices.go | Add EndpointSlice flattening helpers for service endpoint observation. |
| internal/playground/kube/endpointslices_test.go | Tests EndpointSlice flattening + serving/ready selection behavior. |
| internal/metrics/metrics.go | Add bloodraven_replication_source_state gauge and bounded state list. |
| internal/metrics/metrics_test.go | Verify new replication source state metric is registered. |
| internal/controller/updater.go | Add N-site ordered rollout support via UpdateTarget + sequential follower updates + safe handoff rules. |
| internal/controller/updater_test.go | Tests multi-follower updates, reader exclusion from handoff, unhealthy follower drift retention, and readiness gating. |
| internal/controller/source_convergence.go | Implement GTID-gated direct-primary convergence (repoint/restart), status state, and metrics emission. |
| internal/controller/source_convergence_test.go | Tests multi-follower convergence, divergence gates, failure handling, and suppression conditions. |
| internal/controller/runner.go | Integrate source convergence into status; tighten Ready/Degraded semantics (exclude readers); switch internal service hosts for DSNs and clients; drift inspection behavior tweaks. |
| internal/controller/runner_test.go | Update/extend runner tests for new status semantics and writable non-promotable degradation. |
| internal/controller/reclone_test.go | Allow read-only sites as reclone recipients (with confirmation). |
| internal/controller/mysql_tls.go | Add internal per-site Service name/host helpers. |
| internal/controller/mysql_tls_test.go | Tests internal site service host helper. |
| internal/controller/credentials.go | Dial active site’s internal Service for credential reconciliation; TLS ServerName remains client-facing hostname. |
| internal/controller/backup_reconciler.go | Exclude read-only sites from backup source selection and from explicit overrides. |
| internal/controller/backup_reconciler_test.go | Tests backup source exclusion for read-only and updated host expectations. |
| internal/controller/backup_job.go | Switch backup job MySQL host to internal site Service. |
| internal/controller/archiver_poller.go | Switch sidecar client URLs to internal site Service. |
| examples/minimal-failovergroup.yaml | Example updated to include reader role, templates, and replication thresholds. |
| docs/docs/playground.mdx | Document 3-site playground topology and scenario 40 usage. |
| docs/docs/planned-failover.mdx | Document follower source convergence and reader endpoint gating after planned switchover. |
| docs/docs/operations.mdx | Add reader reclone guidance and blocked convergence troubleshooting. |
| docs/docs/multi-site.mdx | Document roles/terminology, topology rules, and N-site behaviors including readers. |
| docs/docs/monitoring.mdx | Add new replication source state metric reference. |
| docs/docs/monitoring-prometheus.mdx | Add PromQL guidance for reader/source-convergence monitoring. |
| docs/docs/log-schema.mdx | Extend stable log schema for source convergence + GTID fields. |
| docs/docs/kubectl-plugin.mdx | Update reclone docs to cover readers and cold reclone semantics. |
| docs/docs/known-limitations.mdx | Document reader limitations and backup-source exclusions. |
| docs/docs/failover.mdx | Document reader isolation, direct-source convergence, and N-site ordered updates. |
| docs/docs/examples.mdx | Update minimal example description to reflect reader. |
| docs/docs/crd-reference.mdx | Update CRD reference for new fields/validations, internal Services, and status fields. |
| docs/docs/configuration.mdx | Document MySQL config precedence, service inheritance, and internal Services behavior. |
| docs/docs/backup-overview.mdx | Document backup source eligibility exclusions for readers. |
| docs/docs/app-integration.mdx | Document client/internal Services and reader endpoint gating semantics. |
| config/crd/bases/shipstream.io_mysqlstandbyclusters.yaml | Regenerated CRD: add reader role, per-site serviceTemplate/mysqlConf, externalTrafficPolicy, and validations. |
| config/crd/bases/shipstream.io_mysqlfailovergroups.yaml | Regenerated CRD: add reader role, per-site serviceTemplate/mysqlConf, externalTrafficPolicy, and new status fields. |
| charts/bloodraven/crds/shipstream.io_mysqlstandbyclusters.yaml | Chart CRD sync with regenerated bases. |
| charts/bloodraven/crds/shipstream.io_mysqlfailovergroups.yaml | Chart CRD sync with regenerated bases. |
| api/v1alpha1/zz_generated.deepcopy.go | Regenerated deepcopy updates for new fields and pointer copying. |
| api/v1alpha1/types.go | API additions: read-only role, per-site mysqlConf/serviceTemplate, readOnlyMaxLagSeconds, new status fields and validations. |
| api/v1alpha1/site_helpers.go | Helpers for role checks and effective lag thresholds. |
| api/v1alpha1/site_helpers_test.go | Unit tests for new helpers and effective lag behavior. |
| .github/kind/e2e-calico.yaml | Increase kind cluster workers to support 3-site E2E topology. |
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/envtest/reconciler_test.go (1)
485-515: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify every site's named MySQL container remains untouched.
The safe-rollout contract covers all existing Deployments, but this checks only
dc1and assumes MySQL isContainers[0]. Capture and compare the named MySQL container image for bothdc1anddc2; otherwise concurrent patching ofdc2can pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/envtest/reconciler_test.go` around lines 485 - 515, Update the existing-deployment verification in the reconciliation test to cover both dc1 and dc2. Capture each Deployment’s MySQL container image by container name rather than assuming Containers[0], then assert after reconciliation that both named-container images remain unchanged.internal/state/matrix.go (1)
71-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCount configured core sites, including
StateUnknown.
coreCountcurrently counts only writable/read-only/unreachable buckets. When all primary candidates are stillUnknown, it becomes zero and reportsHealthy. Increment the core count by role before state classification so startup/failed observations remainNoPrimary, not healthy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/state/matrix.go` around lines 71 - 102, Update the observation loop around coreCount so every configured primary-candidate site increments the core-site count before state classification, including sites with StateUnknown. Keep fencing and bucket classification behavior unchanged, and ensure an all-unknown primary-candidate set reaches the existing non-healthy/NoPrimary handling instead of the coreCount == 0 Healthy path.
🧹 Nitpick comments (3)
internal/metrics/metrics_test.go (1)
39-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold this into a table-driven metric-registration test.
This adds another one-off registration test; use a case table so future metrics only require another entry.
As per coding guidelines, “Add table-driven unit tests beside the code they cover, using the existing
*_test.golayout underinternal/.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/metrics/metrics_test.go` around lines 39 - 54, Replace the one-off TestRegisterIncludesReplicationSourceState with a table-driven registration test covering replication source state and structured for additional metric cases. Keep each case responsible for setting up any required labels, gathering the registry, and asserting the expected metric family, while preserving cleanup through ReplicationSourceState.DeleteLabelValues.Source: Coding guidelines
test/envtest/reconciler_test.go (1)
402-413: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that administrative Services remain internal-only.
Existence alone would allow an
-internalService to regress toNodePortorLoadBalancer. Assert the expected cluster-internal type and absence of external exposure fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/envtest/reconciler_test.go` around lines 402 - 413, The Service checks in the reconciler test should validate that each administrative `-internal` Service remains cluster-internal, not just that it exists and has an owner reference. For those services, assert the expected internal `corev1.ServiceType` and verify external exposure fields such as node ports and load balancer ingress are absent; leave the existing checks for other services unchanged.internal/controller/reclone_test.go (1)
93-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold this role case into a table-driven reclone validation test.
Add the reader variant to the adjacent cold-reclone cases instead of duplicating setup and assertions.
As per coding guidelines, “Add table-driven unit tests beside the code they cover, using the existing
*_test.golayout underinternal/.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/reclone_test.go` around lines 93 - 99, Fold TestValidateRecloneRequest_ReadOnlyRecipient_OK into the adjacent table-driven cold-reclone validation test, adding a case that configures the recipient with SiteRoleReadOnly and uses the correct confirmation token. Remove the standalone test and reuse the existing table setup and assertion flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/reconciler.go`:
- Around line 705-721: Update the settings assembly around
normalizedMySQLSettings and the operator-owned invariant map to remove both
skip-log-bin and disable-log-bin aliases, including underscore forms such as
skip_log_bin, before rendering the final configuration. Ensure group and site
overrides cannot reintroduce these keys, while preserving the enforced log-bin
setting, and add coverage for both underscore and hyphen inputs.
- Around line 1305-1314: Update reconcileSiteService to assign
svc.Spec.LoadBalancerIP from site.LBIP when the effective service type is
LoadBalancer, and clear it for other service types or when the configuration
changes away from LoadBalancer. Add tests covering the reader LoadBalancer path
and clearing the field after a service-type transition.
In `@internal/controller/runner.go`:
- Around line 893-900: Move the source-convergence evaluation in the runner’s
replication-status handling ahead of the nil Replication guard, specifically for
read-only follower states. Ensure probe failures with Replication=nil and
Pending/ProbeFailed still set the Degraded condition when SourceConvergenceState
is not sourceConvergenceConverged, while preserving the existing nil-status skip
behavior for unrelated cases.
In `@internal/controller/source_convergence.go`:
- Around line 145-158: Update the rollback restart calls in the post-stop probe
and source-change failure paths within the surrounding convergence method to use
a fresh bounded context derived from the parent context, rather than the
potentially expired ctx. Create or obtain that rollback context before stopping
replication and pass it to follower.mysql.StartReplica, preserving the existing
divergence check and error returns.
In `@internal/controller/topology.go`:
- Line 1296: Bound each MySQL fencing and safety-probe phase, including the
SetSuperReadOnly call and the regions around the referenced probe blocks, with a
short child context deadline derived from the manager Poll context. Use the
existing normal-polling timeout pattern, pass the bounded context to every probe
in each phase, and ensure the child context is released after the phase
completes so stalled drivers cannot block fencing, failover, or status updates
indefinitely.
In `@internal/controller/updater.go`:
- Around line 229-240: Update the failover flow around failover.Execute so the
authority/status change and onPromoted callback occur immediately after a
successful failover, before CheckReadOnly is called. Preserve the existing error
return for CheckReadOnly failures while ensuring the promoted handoff target is
recorded even when that validation fails.
- Around line 212-220: Update the follower selection loop in the updater handoff
logic to track which followers were successfully processed by the earlier
update/probe phase, and only assign handoff to a non-drifted follower or one
whose update completed successfully; do not reselect a drifted follower solely
because a second requireDirectReplica call succeeds. Add a regression test
covering a candidate whose first probe fails and second probe succeeds,
verifying it is not handed off while stale.
In `@internal/metrics/metrics.go`:
- Around line 97-100: Add stable failover-group identity to
ReplicationSourceState by including namespace/group labels alongside site and
state, then update every metric emitter, test, and documentation reference to
supply and describe those labels consistently. Preserve the existing state-set
semantics while ensuring sites with the same name in different groups produce
distinct series.
In `@internal/playground/logs/matcher.go`:
- Around line 39-71: Update textFieldMatches to match complete
whitespace-delimited key/value tokens, requiring a valid key boundary and
ensuring the value ends before whitespace or the end of the line. Preserve
support for quoted values while rejecting key prefixes and value prefixes such
as oldsource=... and source=auto-clone-old; keep Structured’s JSON matching
unchanged.
In `@internal/state/matrix.go`:
- Around line 73-75: Update the matrix action-building flow around the
writable-site handling and PromotionCandidates population: when any writable
site with a non-promotable role is added to FenceSites, return a
fencing-specific degraded action immediately instead of allowing promotion
candidates to be populated. Preserve promotion only after a subsequent poll
confirms the site is no longer writable or has been successfully fenced.
In `@playground/chaos-scenarios.md`:
- Around line 270-277: Update Scenario 11’s adjacent Verify text to state that
recovery results in one writable site and two read-only follower sites, matching
the three deployments scaled by the injection.
- Line 1073: Update the continuous observer description to avoid claiming
immediate failure: state that s40ContinuousObservation.fail records invariant
violations during scale-down, empty-datadir detection, clone, or catch-up, with
errors reported later by stopAndCheck. Do not imply the active operation is
aborted unless the runner is changed to cancel on failure.
In `@test/envtest/reconciler_test.go`:
- Around line 369-377: Update the owner-reference assertion in the per-site
ConfigMap verification loop to validate that each ConfigMap is controlled by the
expected object using metav1.IsControlledBy(&cm, fg), rather than checking only
OwnerReferences[0].Name. Preserve the existing failure reporting and ConfigMap
lookup behavior.
- Around line 161-173: Strengthen the negative cases in the test loop around
tt.wantOK and k8sClient.Create by asserting that the error is a Kubernetes
schema-validation Invalid error, ideally also verifying the expected field path.
Keep the valid-object assertion unchanged, and ensure unrelated failures such as
namespace setup errors cannot satisfy the invalid-object branch.
---
Outside diff comments:
In `@internal/state/matrix.go`:
- Around line 71-102: Update the observation loop around coreCount so every
configured primary-candidate site increments the core-site count before state
classification, including sites with StateUnknown. Keep fencing and bucket
classification behavior unchanged, and ensure an all-unknown primary-candidate
set reaches the existing non-healthy/NoPrimary handling instead of the coreCount
== 0 Healthy path.
In `@test/envtest/reconciler_test.go`:
- Around line 485-515: Update the existing-deployment verification in the
reconciliation test to cover both dc1 and dc2. Capture each Deployment’s MySQL
container image by container name rather than assuming Containers[0], then
assert after reconciliation that both named-container images remain unchanged.
---
Nitpick comments:
In `@internal/controller/reclone_test.go`:
- Around line 93-99: Fold TestValidateRecloneRequest_ReadOnlyRecipient_OK into
the adjacent table-driven cold-reclone validation test, adding a case that
configures the recipient with SiteRoleReadOnly and uses the correct confirmation
token. Remove the standalone test and reuse the existing table setup and
assertion flow.
In `@internal/metrics/metrics_test.go`:
- Around line 39-54: Replace the one-off
TestRegisterIncludesReplicationSourceState with a table-driven registration test
covering replication source state and structured for additional metric cases.
Keep each case responsible for setting up any required labels, gathering the
registry, and asserting the expected metric family, while preserving cleanup
through ReplicationSourceState.DeleteLabelValues.
In `@test/envtest/reconciler_test.go`:
- Around line 402-413: The Service checks in the reconciler test should validate
that each administrative `-internal` Service remains cluster-internal, not just
that it exists and has an owner reference. For those services, assert the
expected internal `corev1.ServiceType` and verify external exposure fields such
as node ports and load balancer ingress are absent; leave the existing checks
for other services unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b69e124e-1447-4e2e-9cc4-4562345036db
📒 Files selected for processing (80)
.github/kind/e2e-calico.yamlapi/v1alpha1/site_helpers.goapi/v1alpha1/site_helpers_test.goapi/v1alpha1/types.goapi/v1alpha1/zz_generated.deepcopy.gocharts/bloodraven/crds/shipstream.io_mysqlfailovergroups.yamlcharts/bloodraven/crds/shipstream.io_mysqlstandbyclusters.yamlconfig/crd/bases/shipstream.io_mysqlfailovergroups.yamlconfig/crd/bases/shipstream.io_mysqlstandbyclusters.yamldocs/docs/app-integration.mdxdocs/docs/backup-overview.mdxdocs/docs/configuration.mdxdocs/docs/crd-reference.mdxdocs/docs/examples.mdxdocs/docs/failover.mdxdocs/docs/known-limitations.mdxdocs/docs/kubectl-plugin.mdxdocs/docs/log-schema.mdxdocs/docs/monitoring-prometheus.mdxdocs/docs/monitoring.mdxdocs/docs/multi-site.mdxdocs/docs/operations.mdxdocs/docs/planned-failover.mdxdocs/docs/playground.mdxexamples/minimal-failovergroup.yamlinternal/controller/archiver_poller.gointernal/controller/backup_job.gointernal/controller/backup_reconciler.gointernal/controller/backup_reconciler_test.gointernal/controller/credentials.gointernal/controller/mysql_tls.gointernal/controller/mysql_tls_test.gointernal/controller/reclone_test.gointernal/controller/reconciler.gointernal/controller/reconciler_test.gointernal/controller/runner.gointernal/controller/runner_test.gointernal/controller/source_convergence.gointernal/controller/source_convergence_test.gointernal/controller/topology.gointernal/controller/topology_test.gointernal/controller/updater.gointernal/controller/updater_test.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gointernal/playground/kube/endpointslices.gointernal/playground/kube/endpointslices_test.gointernal/playground/logs/matcher.gointernal/playground/logs/matcher_test.gointernal/playground/runner/executor.gointernal/playground/runner/profile.gointernal/playground/runner/profile_registry_test.gointernal/playground/scenarios/common.gointernal/playground/scenarios/inventory_40_test.gointernal/playground/scenarios/s02_planned_switchover.gointernal/playground/scenarios/s05_operator_kill_during_failover.gointernal/playground/scenarios/s06_self_fence_isolated_primary.gointernal/playground/scenarios/s08_gtid_divergence_detection.gointernal/playground/scenarios/s10_full_bootstrap_after_data_wipe.gointernal/playground/scenarios/s11_total_loss_recovery.gointernal/playground/scenarios/s12_old_primary_recovery.gointernal/playground/scenarios/s12_rolling_update_healthy_state.gointernal/playground/scenarios/s13_operator_kill_during_bootstrap.gointernal/playground/scenarios/s16_mysql_process_kill.gointernal/playground/scenarios/s18_rapid_cr_spec_changes.gointernal/playground/scenarios/s19_reclone_interlock.gointernal/playground/scenarios/s22_replication_status_after_recovery.gointernal/playground/scenarios/s34_operator_kill_during_wait_replica.gointernal/playground/scenarios/s40_observer_test.gointernal/playground/scenarios/s40_reader_data_loss.gointernal/state/matrix.gointernal/state/matrix_test.goplayground/chaos-scenarios.mdplayground/chaos.shplayground/manifests/failovergroup.yamlplayground/rebuild.shplayground/setup.shtest/component/helpers_test.gotest/component/source_convergence_test.gotest/envtest/reconciler_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/playground-chaos/reset.go (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new normalization phase.
Please verify that
normalizeFreshBaselineruns after the first healthy baseline, preserves the scale-down → status-clear → restart → wait ordering, and stops on the first error. This path performs several API-server mutations and should be covered before merge.As per coding guidelines, run
make test-envtestwhen you touch anything that interacts with the API server.Also applies to: 129-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/playground-chaos/reset.go` at line 117, Add regression tests for the reset sequence around normalizeFreshBaseline, verifying it runs after the first healthy baseline, preserves scale-down → status-clear → restart → wait ordering, and stops immediately on the first error. Cover the API-server mutation interactions using the existing test patterns and run make test-envtest.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/playground-chaos/reset.go`:
- Line 117: Add regression tests for the reset sequence around
normalizeFreshBaseline, verifying it runs after the first healthy baseline,
preserves scale-down → status-clear → restart → wait ordering, and stops
immediately on the first error. Cover the API-server mutation interactions using
the existing test patterns and run make test-envtest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 150c0194-d8d6-4372-bf2d-8d36af43c7c3
📒 Files selected for processing (6)
cmd/playground-chaos/reset.gointernal/controller/updater.gointernal/controller/updater_test.gointernal/playground/scenarios/s31_pitr_verification_rustfs.gointernal/playground/scenarios/s40_observer_test.gointernal/playground/scenarios/s40_reader_data_loss.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/updater_test.go
- internal/playground/scenarios/s40_observer_test.go
- internal/playground/scenarios/s40_reader_data_loss.go
- internal/controller/updater.go
Automates the R3/R4/R5/R9 chaos proposals from the issue #115 review comment as playground scenarios with the standard Precheck/forensics contract: - 41-reader-availability-during-failover (R3, release): the reader answers SELECTs continuously through an unplanned failover and then repoints directly at the new primary; source-host history proves no chained or blocked intermediate state. - 42-reader-stall-no-group-degradation (R4, smoke+release): SOURCE_DELAY grows reader lag past readOnlyMaxLagSeconds for a 3x-maxLagSeconds soak with zero group-level effect; only endpoint shedding plus lag and source-state metrics react. - 43-writable-reader-fence (R5, release): a writable reader is fenced without debounce, a planned failover targeting it is rejected with the role error, and its errant GTID trips the convergence containment gate into Blocked/GTIDDiverged; cleanup reconciles the errant transactions as empty transactions on the primary. - 44-reader-source-convergence-invariant (R9, release): a manually repointed (chained) reader heals back to the direct primary as a poll-loop invariant, emitting the documented convergence log events with no failover consumed. Shared reader helpers (topology resolution, serving-status contract, marker seeding/replication, endpoint waits) move out of scenario 40 into reader_helpers.go for reuse. R6-R8 remain documented as manual follow-ups in chaos-scenarios.md.
All four new reader scenarios (41-44) plus the smoke profile were run against a fresh 3-site k3d playground. Two fixes from that run: - s02 follower-convergence verify: tolerate a transiently empty status.activeSite after PlannedFailoverPhaseSucceeded (the snapshot can publish one cycle before writable confirmation lands); only a different non-empty site is a terminal failure. This flaked the first smoke batch and passed on rerun with the fix. - Progress messages printed *int64 SecondsBehindSource as a pointer address (lag=0x...); add formatLag and use it in scenarios 40-44. Live results: 44 PASS 3s, 42 PASS 1m33s, 43 PASS 13s, 41 PASS 39s, then smoke profile (01, 02x2, 42) all PASS.
Collaborator
Author
Addressed all 21 review comments (20 unique findings; the Copilot and CodeRabbit log-matcher comments were duplicates) in Fixes
Review disposition
Verification
The PR is open with zero unresolved review threads. CI is running for the latest branch head; Generate Check has passed, E2E is skipped, and the remaining jobs are in progress. |
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.
Closes #115.
Summary
read-onlysites with conditional placement validation, per-site MySQL/Service controls, reader lag thresholds, generated CRDs, and chart synchronizationOperational impact
Existing
primary-candidateanddr-onlymanifests retain their requiredlbIP/taintNodeSelectorcontract. Reader failures remain visible per site but do not degrade the failover group's writer readiness. Client site Services are now MySQL-only; operator and sidecar traffic uses an always-ClusterIP-internalService. Existing Deployments migrate to per-site ConfigMaps through the ordered updater.Test plan
make generate && make manifests— passeddiff -r config/crd/bases charts/bloodraven/crds— passedmake vet/make lint— passedmake test/make test-envtest— passedhelm lint charts/bloodraven— passedgit diff --check— passedE2Eworkflow dispatched withprofile=release; local context lacks Bloodraven CRDs/MFGMegamind artifacts
.tmp/megamind-read-only-readers/plans/final.md.tmp/megamind-read-only-readers/reviews/validated-findings.md.tmp/megamind-read-only-readers/reviews/fixed-review-2.md.tmp/megamind-read-only-readers/final/local-gates.mdThe
.tmprun artifacts remain local/ignored; paths are included for maintainers with workspace access.Megamind Educational Brief
Journey
Issue #115 began with useful primitives—centralized promotion eligibility, clone recovery, per-site status, and replica Services—but the planning critique showed that a healthy replication thread could still be chained through the demoted primary. Three models refined the task, and a three-round architecture debate converged unanimously on eight bundled decisions. Implementation was split into core operator, playground E2E, and docs scopes.
The first ultra-review validated 13 behavioral gaps, including destructive recovery after a replica-status probe error, recovery trusting a writable non-promotable site, stale serving labels under ambiguous authority, unsafe startup Deployment patching, and incomplete active-last verification. Targeted fixes plus two fixed-review passes closed all 13 before delivery.
Design decisions
RESET REPLICA ALL.dr-onlycompatibility and topology-relay evidence.Architecture
primary-candidate; any second writable site invalidates authority.healthy=yes-selected Service. Operator, replication, clone, backup, archiver, and sidecar traffic use the always-ClusterIP internal Service.Lessons
Evidence
Grounded in issue #115, commit
922745ed73c1f52b001d5f1448b82f3f32fcba24, the changed API/controller/playground/docs files, all required local gate outcomes above, and the review/fix artifacts listed under Megamind artifacts. Hosted release-profile E2E was dispatched; this description intentionally does not claim it passed until the workflow completes.Summary by CodeRabbit