feat(charts): support maxReplicationLag on mysql backends - #88
Open
viicslen wants to merge 2 commits into
Open
Conversation
The ProxySQLConfig CRD has carried maxReplicationLag on MySQLBackend since the API was introduced, but the two standalone charts drop it: their mysql_servers row renders address/port/hostgroup/max_connections/comment only. Operator-less users therefore cannot shun a lagging reader, and a replica that falls behind keeps serving stale reads instead of failing over to another host in the hostgroup. Renders only when set, so an unchanged values file produces byte-identical output and no existing release restarts. mysql only — pgsql_servers has no equivalent column. Both ci/special-chars-values.yaml files gain a reader carrying the field, so `ct install` exercises it against a real ProxySQL rather than only asserting that Helm renders something. Verified separately by feeding the rendered cnf to a proxysql binary, which parses it and starts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughBoth ProxySQL Helm charts now support optional ChangesReplication-lag backend support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@charts/proxysql-cluster/values.yaml`:
- Around line 53-55: The comments in charts/proxysql-cluster/values.yaml lines
53-55 and charts/proxysql/values.yaml lines 55-57 incorrectly describe the
privilege as belonging to a generic backend user. Update both descriptions to
identify ProxySQL’s monitor account and state that REPLICATION CLIENT or SLAVE
MONITOR must be granted to that account on each monitored backend.
🪄 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 Plus
Run ID: 16a51aac-d506-4d6a-8f3c-8996d1b79c13
📒 Files selected for processing (8)
charts/proxysql-cluster/Chart.yamlcharts/proxysql-cluster/ci/special-chars-values.yamlcharts/proxysql-cluster/files/proxysql.cnfcharts/proxysql-cluster/values.yamlcharts/proxysql/Chart.yamlcharts/proxysql/ci/special-chars-values.yamlcharts/proxysql/files/proxysql.cnfcharts/proxysql/values.yaml
The maxReplicationLag notes said "the backend user" needs REPLICATION CLIENT / SLAVE MONITOR. It's ProxySQL's monitor account (monitor_username, default "monitor") that reads replica status, and the grant is needed on every monitored backend. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Context
ProxySQLConfig'sMySQLBackendhas carriedmaxReplicationLagsince the API was introduced (proxysqlconfig_types.go:120, and it's in the shipped CRD). The two standalone charts drop it — theirmysql_serversrow rendersaddress/port/hostgroup/max_connections/commentonly.So operator-less users cannot shun a lagging reader at all. A replica that falls behind keeps serving stale reads instead of failing over to another host in the hostgroup, and there is no way to express otherwise through chart values.
This also leaves a gap around #16 / #24. #16 asked for drift, shunned backends and replication lag in status; #24 shipped the first two and deferred lag surfacing ("Replication-lag surfacing from the monitor schema (deferred from #16, see spec note)"). For chart users the deferred half can never show anything useful, because the configuration that produces a lag-shun does not exist in the first place. This PR is the prerequisite for that follow-up being observable on the operator-less path.
Same class of gap as #68 (standalone charts missing
--reloadfor operator parity).Change
One template line per chart, in
files/proxysql.cnf:pgsql_servershas no equivalent column.maxReplicationLag, so the two paths stay consistent.Both charts'
values.yamldocument it, and bothci/special-chars-values.yamlgain a reader carrying the field soct installexercises it against a real ProxySQL rather than only asserting Helm renders something.Chart versions bumped:
proxysql0.1.0 → 0.2.0,proxysql-cluster0.2.0 → 0.3.0. Happy to drop those if you batch chart releases.Verification
helm lintgreen on both charts{ address="...", port=3306, hostgroup=1, max_connections=1000, max_replication_lag=5 }proxysql.cnfto a realproxysqlbinary: parses cleanly and proceeds to normal startupPossibly worth a separate issue
While checking how lag-shunning surfaces,
RuntimeState.ShunnedCount()(runtime.go:126-135) matches the status string exactly:ProxySQL treats lag-shunning as a distinct status. From the strings in a running
proxysqlbinary:If that value reaches
runtime_mysql_servers.statusas written, a backend shunned for replication lag would not be counted, andshunnedBackendswould read0during exactly the scenario #16 wanted surfaced — including for operator users who already havemaxReplicationLag.I verified the enum in the binary but could not query
runtime_mysql_serversdirectly to observe the literal column value, so please treat this as a lead rather than a confirmed reproduction. Happy to open a separate issue or PR if it holds up.Not included
users.mysql[]has the same shape of gap — the CRD'sMySQLUserhasdefaultSchema(:142), the charts do not. Left out to keep this reviewable; glad to add it here or in a follow-up, whichever you prefer.AI disclosure: this change was written with AI assistance (Claude Code). I reviewed the diff and the reasoning before opening it, and I'm accountable for the content.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores