feat(operator): union all ProxySQLConfigs for a cluster into one desired runtime - #84
Merged
Merged
Conversation
…red runtime The operator treated each ProxySQLConfig CR as authoritative for the cluster's entire runtime: buildDesired built the desired state from ONE config, and Sync DELETEs+re-INSERTs exactly those rows on every replica. So two ProxySQLConfigs targeting one cluster oscillate — each reconcile DELETEs the other's servers/ users/rules and re-LOADs its own — and client connections landing in the DELETE+LOAD window hit 'Access denied' / 'Lost connection'. Adding any second config to a running cluster destabilised it, forcing an out-of-band one-config-per-cluster workaround. Union all ProxySQLConfigs whose clusterRef points at the cluster into a single desired runtime, so multiple configs compose instead of fighting: - new proxysqlclient.Union merges Desired states per key with last-writer-wins (servers by hostgroup:host:port, users by username, rules by ruleId, repl- hostgroups by writer hg, hostgroup-attrs by hg, proxysql-servers by host:port, variable maps by name; sqlStatements concatenated). Output is sorted by key so the sync fingerprint is stable. - buildDesired is split into a per-config translator (configToDesired) + a new buildUnionedDesired that lists all configs for the cluster, orders them by name (deterministic last-writer-wins) and merges. proxysql_servers auto-population (#39) now happens once at cluster level, only when no config set an explicit list — so a second config can't wipe it. - Reconcile builds the union: every sibling config reconciles to the SAME desired state, so they converge and stop oscillating. - Deletion re-pushes the union of the REMAINING configs (excludeName), instead of clearing the whole runtime; falls back to cleanupDesired only for the last config, and never wedges deletion on a sibling's missing Secret. Tests: Union unit tests (distinct-key compose, same-key last-writer-wins, determinism, sqlStatements order, empty-stays-nil); an envtest proving two configs on one cluster both survive + the exclude/deletion path; existing auto-population + drift tests retargeted to buildUnionedDesired. Full envtest suite green; go vet + gofmt clean; no CRD drift. Docs: 'Multiple ProxySQLConfigs per cluster' section in the configuration user guide.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe controller now combines active ChangesProxySQLConfig unioning
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProxySQLConfigResources
participant buildUnionedDesired
participant configToDesired
participant proxysqlclientUnion
participant ProxySQLCluster
ProxySQLConfigResources->>buildUnionedDesired: list and sort active cluster configs
buildUnionedDesired->>configToDesired: translate each config
configToDesired-->>buildUnionedDesired: individual Desired states
buildUnionedDesired->>proxysqlclientUnion: merge Desired states
proxysqlclientUnion-->>buildUnionedDesired: deterministic union
buildUnionedDesired->>ProxySQLCluster: apply unioned desired state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/user-guide/configuration.md (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument raw SQL statement ordering.
The merge rules describe keyed sections only. Document that unkeyed SQL statements concatenate in
ProxySQLConfigname order. Users need this rule when SQL statements in separate configs have ordering dependencies.🤖 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 `@docs/user-guide/configuration.md` around lines 50 - 54, Update the merge rules documentation in the configuration guide to state that unkeyed SQL statements are concatenated in ProxySQLConfig name order, preserving that ordering for statements with dependencies.operator/internal/controller/proxysqlconfig_controller_test.go (1)
873-899: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a same-key conflict test for name ordering.
This test only combines disjoint sections. It cannot detect a regression in
buildUnionedDesiredname sorting or last-writer-wins behavior.Create two configs with the same section key and different values. Create them in reverse lexical order. Assert that the config with the lexically last name wins.
🤖 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 `@operator/internal/controller/proxysqlconfig_controller_test.go` around lines 873 - 899, Extend the buildUnionedDesired test with two configs created in reverse lexical order that both define the same section key but different values. Assert the lexically last config’s value is retained, verifying deterministic name ordering and last-writer-wins behavior while preserving the existing disjoint-section assertions.
🤖 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 `@operator/internal/controller/proxysqlconfig_controller.go`:
- Around line 194-198: Update the error-condition handling after
buildUnionedDesired in the reconciliation flow to use DesiredBuildFailed by
default for all union-build errors, including list failures. Only use
UserSecretError when the returned error is explicitly classified as a Secret
resolution failure, preserving the existing status error message and condition
update behavior.
- Around line 677-693: Update the deletion flow around buildUnionedDesired so a
union-build error does not fall back to cleanupDesired; propagate the error
through the finalization path and requeue. Only apply full cleanupDesired after
a successful union build reports any == false, while using the remaining union
when any == true. Add a deletion test covering a remaining configuration whose
Secret lookup fails and assert the finalization error is requeued.
---
Nitpick comments:
In `@docs/user-guide/configuration.md`:
- Around line 50-54: Update the merge rules documentation in the configuration
guide to state that unkeyed SQL statements are concatenated in ProxySQLConfig
name order, preserving that ordering for statements with dependencies.
In `@operator/internal/controller/proxysqlconfig_controller_test.go`:
- Around line 873-899: Extend the buildUnionedDesired test with two configs
created in reverse lexical order that both define the same section key but
different values. Assert the lexically last config’s value is retained,
verifying deterministic name ordering and last-writer-wins behavior while
preserving the existing disjoint-section assertions.
🪄 Autofix
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 47199963-0513-42ab-a8a3-64270d2613cd
📒 Files selected for processing (6)
docs/user-guide/configuration.mdoperator/internal/controller/cleanup_desired_test.gooperator/internal/controller/proxysqlconfig_controller.gooperator/internal/controller/proxysqlconfig_controller_test.gooperator/internal/proxysqlclient/union.gooperator/internal/proxysqlclient/union_test.go
Comment on lines
+194
to
+198
| // 3) Assemble the desired runtime from the UNION of all ProxySQLConfigs targeting this | ||
| // cluster (#956) — not just this one — so multiple configs compose one runtime instead of | ||
| // each being authoritative and oscillating. Every sibling config reconciles to the same | ||
| // union, so they converge. User passwords are resolved from Secrets here. | ||
| desired, _, err := r.buildUnionedDesired(ctx, &cluster, b, "") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a condition reason that matches every union-build failure.
At Line 198, buildUnionedDesired can return a list failure from Line 413. The existing branch reports every error as UserSecretError. A list failure then has the wrong condition reason. Use DesiredBuildFailed unless the error is explicitly classified as a Secret resolution failure.
Proposed fix
- r.setCfgCondition(&cfg, cfgCondReady, metav1.ConditionFalse, "UserSecretError", err.Error())
+ r.setCfgCondition(&cfg, cfgCondReady, metav1.ConditionFalse, "DesiredBuildFailed", err.Error())As per path instructions, focus on "error/condition surfacing on .status".
🤖 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 `@operator/internal/controller/proxysqlconfig_controller.go` around lines 194 -
198, Update the error-condition handling after buildUnionedDesired in the
reconciliation flow to use DesiredBuildFailed by default for all union-build
errors, including list failures. Only use UserSecretError when the returned
error is explicitly classified as a Secret resolution failure, preserving the
existing status error message and condition update behavior.
Source: Path instructions
Comment on lines
+677
to
+693
| // Push the runtime the cluster should have WITHOUT this config: the union of the | ||
| // remaining ProxySQLConfigs (#956). If sibling configs still target the cluster, deleting | ||
| // one must NOT wipe their servers/users/rules — it re-pushes their combined state. Only | ||
| // when this was the last config do we fall back to cleanupDesired, which DELETEs every | ||
| // managed table and LOAD/SAVEs each section; there, when the config's peer list was | ||
| // operator-populated (empty spec.proxysqlServers) the auto-derived peers are re-pushed | ||
| // instead of cleared (#42), and an explicit list is cleared like every other table. | ||
| // Variables are left as-is: ProxySQL has no "unset". If the remaining union can't be built | ||
| // (e.g. a sibling's Secret is missing) we must not wedge deletion — fall back to cleanup. | ||
| cleanupState := cleanupDesired(b, len(cfg.Spec.ProxySQLServers) == 0) | ||
| if remaining, any, err := r.buildUnionedDesired(ctx, &cluster, b, cfg.Name); err != nil { | ||
| log.Info("could not build remaining-configs union for cleanup; falling back to full cleanup", | ||
| "cluster", cluster.Name, "error", err.Error()) | ||
| } else if any { | ||
| cleanupState = remaining | ||
| } | ||
| cleaned, errs := r.applyToReplicas(ctx, addrs, radminPassword, cleanupState, dialTLS) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not clean up when the remaining union is unavailable.
If configuration listing fails or a sibling Secret cannot resolve, this branch applies cleanupDesired instead of the remaining union. That clears surviving structured configuration before finalizer release. Requeue the finalization error. Run full cleanup only after a successful union build reports any == false. Add a deletion test with a remaining configuration whose Secret lookup fails.
Proposed fix
if remaining, any, err := r.buildUnionedDesired(ctx, &cluster, b, cfg.Name); err != nil {
- log.Info("could not build remaining-configs union for cleanup; falling back to full cleanup",
- "cluster", cluster.Name, "error", err.Error())
+ log.Error(err, "cleanup pending: could not build remaining-configs union",
+ "cluster", cluster.Name)
+ return ctrl.Result{RequeueAfter: requeueAfterTransient}, nil
} else if any {
cleanupState = remaining
}This conflicts with the PR objective to preserve remaining configuration during deletion. As per path instructions, use "errors via Error()".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Push the runtime the cluster should have WITHOUT this config: the union of the | |
| // remaining ProxySQLConfigs (#956). If sibling configs still target the cluster, deleting | |
| // one must NOT wipe their servers/users/rules — it re-pushes their combined state. Only | |
| // when this was the last config do we fall back to cleanupDesired, which DELETEs every | |
| // managed table and LOAD/SAVEs each section; there, when the config's peer list was | |
| // operator-populated (empty spec.proxysqlServers) the auto-derived peers are re-pushed | |
| // instead of cleared (#42), and an explicit list is cleared like every other table. | |
| // Variables are left as-is: ProxySQL has no "unset". If the remaining union can't be built | |
| // (e.g. a sibling's Secret is missing) we must not wedge deletion — fall back to cleanup. | |
| cleanupState := cleanupDesired(b, len(cfg.Spec.ProxySQLServers) == 0) | |
| if remaining, any, err := r.buildUnionedDesired(ctx, &cluster, b, cfg.Name); err != nil { | |
| log.Info("could not build remaining-configs union for cleanup; falling back to full cleanup", | |
| "cluster", cluster.Name, "error", err.Error()) | |
| } else if any { | |
| cleanupState = remaining | |
| } | |
| cleaned, errs := r.applyToReplicas(ctx, addrs, radminPassword, cleanupState, dialTLS) | |
| // Push the runtime the cluster should have WITHOUT this config: the union of the | |
| // remaining ProxySQLConfigs (`#956`). If sibling configs still target the cluster, deleting | |
| // one must NOT wipe their servers/users/rules — it re-pushes their combined state. Only | |
| // when this was the last config do we fall back to cleanupDesired, which DELETEs every | |
| // managed table and LOAD/SAVEs each section; there, when the config's peer list was | |
| // operator-populated (empty spec.proxysqlServers) the auto-derived peers are re-pushed | |
| // instead of cleared (`#42`), and an explicit list is cleared like every other table. | |
| // Variables are left as-is: ProxySQL has no "unset". If the remaining union can't be built | |
| // (e.g. a sibling's Secret is missing) we must not wedge deletion — fall back to cleanup. | |
| cleanupState := cleanupDesired(b, len(cfg.Spec.ProxySQLServers) == 0) | |
| if remaining, any, err := r.buildUnionedDesired(ctx, &cluster, b, cfg.Name); err != nil { | |
| log.Error(err, "cleanup pending: could not build remaining-configs union", | |
| "cluster", cluster.Name) | |
| return ctrl.Result{RequeueAfter: requeueAfterTransient}, nil | |
| } else if any { | |
| cleanupState = remaining | |
| } | |
| cleaned, errs := r.applyToReplicas(ctx, addrs, radminPassword, cleanupState, dialTLS) |
🤖 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 `@operator/internal/controller/proxysqlconfig_controller.go` around lines 677 -
693, Update the deletion flow around buildUnionedDesired so a union-build error
does not fall back to cleanupDesired; propagate the error through the
finalization path and requeue. Only apply full cleanupDesired after a successful
union build reports any == false, while using the remaining union when any ==
true. Add a deletion test covering a remaining configuration whose Secret lookup
fails and assert the finalization error is requeued.
Source: Path instructions
golangci-lint 'modernize' flagged the manual map-copy loop and sort.Slice in union.go; use maps.Copy and slices.Sort. No behavior change.
Trivy flags HIGH CVE-2026-56852 in golang.org/x/text v0.37.0 (norm.Iter infinite loop). go get golang.org/x/text@v0.39.0 + go mod tidy; the golang.org/x/* family moves as a compatible set (net 0.55->0.56, sys, term, tools). All indirect, no direct-dep changes. Build + full envtest suite green.
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
2 participants
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.
Problem
The operator treats each
ProxySQLConfigas authoritative for the cluster's entire runtime:buildDesiredbuilds the desired state from one CR, andproxysqlclient.SyncDELETEs + re-INSERTs exactly those rows on every replica. So two configs for one cluster oscillate — each reconcile DELETEs the other's servers/users/rules and re-LOADs its own — and client connections landing in theDELETE+LOAD ... TO RUNTIMEwindow hitAccess denied/Lost connection. Adding a secondProxySQLConfigto a running cluster destabilises it, forcing a one-config-per-cluster workaround.(Found downstream in the ProxySQL SaaS byoc live runs — a companion
<cluster>-varsconfig + an app config for backends/users/rules oscillated and dropped sysbench traffic.)Change
Union all
ProxySQLConfigs whoseclusterRefpoints at the cluster into a single desired runtime, so multiple configs compose instead of fighting:proxysqlclient.Union(new) mergesDesiredstates per key with last-writer-wins: servers byhostgroup:host:port, users byusername, query rules byruleId, replication hostgroups by writer hg, hostgroup-attrs by hg, proxysql-servers byhost:port, the three variable maps by name;sqlStatementsconcatenated in order. Output sorted by key so the sync fingerprint is stable.buildDesired→configToDesired(per-config) +buildUnionedDesired(lists the cluster's configs, orders by name for deterministic last-writer-wins, merges).proxysql_serversauto-population (Empty spec.proxysqlServers wipes the cnf-seeded peer table, disabling ProxySQL cluster sync #39) now happens once at cluster level, only when no config set an explicit list — a second config can't wipe it.Reconcilebuilds the union: every sibling config reconciles to the same desired state → they converge, oscillation stops.excludeName) instead of clearing the runtime; falls back tocleanupDesiredonly for the last config, and never wedges deletion on a sibling's missing Secret.Tests
Unionunit tests: distinct-key compose, same-key last-writer-wins, determinism,sqlStatementsorder, empty-stays-nil.buildUnionedDesired.make testenvtest suite green;go vet+gofmtclean;make manifestsshows no CRD drift (no API changes).Docs
Adds a "Multiple ProxySQLConfigs per cluster" section to
docs/user-guide/configuration.mddescribing the union + last-writer-wins semantics.Compatibility
No CRD/API changes — purely reconcile behaviour. Single-config clusters are unaffected (a one-element union == the old
buildDesired, modulo the moved auto-populate which is preserved). Suitable for a minor release (e.g.v0.5.0).Summary by CodeRabbit
New Features
Bug Fixes
Documentation