Add built-in ProxySQL failover hooks#36
Conversation
Adds ProxySQLAdminAddress, ProxySQLAdminPort, ProxySQLAdminUser, ProxySQLAdminPassword, ProxySQLAdminUseTLS, ProxySQLWriterHostgroup, ProxySQLReaderHostgroup, and ProxySQLPreFailoverAction fields to the Configuration struct with defaults (port 6032, user "admin", pre-failover action "offline_soft").
Wire proxysql.InitHook() into both the continuous-discovery startup path (go/logic/orchestrator.go) and the CLI path (go/app/cli.go), and add the singleton/init logic in go/proxysql/init.go.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds built-in ProxySQL pre/post-failover hooks: new go/proxysql package (client, hook, init), config fields, CLI commands, documentation, startup initialization, and calls into topology recovery to run PreFailover/PostFailover (errors logged/audited, non-blocking). Changes
Sequence Diagram(s)sequenceDiagram
participant Orch as Orchestrator
participant Pre as PreFailoverProcesses
participant Hook as ProxySQL Hook
participant Admin as ProxySQL Admin (MySQL)
participant KV as KVStore
Orch->>Pre: Start failover (recoverDeadMaster)
Pre-->>Orch: pre-failover scripts complete
Orch->>Hook: PreFailover(oldMasterHost, oldMasterPort)
Hook->>Admin: Execute drain SQL (UPDATE/LOAD MYSQL SERVERS TO RUNTIME)
Admin-->>Hook: OK / error (logged)
Hook-->>Orch: return
Orch->>KV: DistributePairs (promote successor)
KV-->>Orch: distribution complete
Orch->>Hook: PostFailover(newMasterHost,...)
Hook->>Admin: DELETE/INSERT runtime_mysql_servers, LOAD, SAVE
Admin-->>Hook: OK / error (logged)
Hook-->>Orch: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Orchestrator's capabilities by integrating direct support for ProxySQL failover management. It provides a robust, built-in mechanism to automatically update ProxySQL hostgroups during master recovery and graceful takeovers, thereby streamlining operations and reducing reliance on external scripting. The changes introduce a dedicated Go package for ProxySQL interaction, new configuration parameters, and command-line tools for easier management and testing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces built-in ProxySQL failover hooks, which is a significant enhancement for orchestrator's integration capabilities. The new go/proxysql package provides a robust client for the ProxySQL Admin API and implements the pre/post failover logic. The configuration options are well-documented, and the non-blocking nature of the hooks ensures that ProxySQL issues do not impede MySQL failover, which is a critical design choice. Unit tests for the new client and hook logic are also included, demonstrating good test coverage. Overall, this is a well-implemented feature that greatly simplifies ProxySQL integration.
| sqls = append(sqls, "REPLACE INTO mysql_servers (hostgroup_id, hostname, port, status) VALUES (?, ?, ?, 'OFFLINE_SOFT')") | ||
| args = append(args, []interface{}{readerHostgroup, oldHost, oldPort}) |
There was a problem hiding this comment.
This line adds the old master to the reader hostgroup with OFFLINE_SOFT status after a new master is promoted. This behavior is unusual. Typically, a failed master is completely removed from active service or moved to a dedicated 'maintenance' hostgroup, not a 'reader' hostgroup, even if offline. This could lead to unexpected behavior or confusion for users. Please clarify the intent or consider removing the old master from all active hostgroups.
| "ProxySQLAdminAddress": "127.0.0.1", | ||
| "ProxySQLAdminPort": 6032, | ||
| "ProxySQLAdminUser": "admin", | ||
| "ProxySQLAdminPassword": "admin", |
There was a problem hiding this comment.
The sample configuration uses "admin" as the ProxySQLAdminPassword. While this is a sample, it's generally a good practice to avoid suggesting default, insecure passwords even in examples, as users might copy-paste without changing. Consider using an empty string or a placeholder like "your_password" to encourage secure practices.
| "ProxySQLAdminPassword": "admin", | |
| "ProxySQLAdminPassword": "", | |
go/app/cli.go
Outdated
| } | ||
| case registerCliCommand("proxysql-test", "ProxySQL", `Test connectivity to ProxySQL Admin interface`): | ||
| { | ||
| proxysql.InitHook() |
There was a problem hiding this comment.
The proxysql.InitHook() function is called here, but it's already called globally in Cli function (line 217) and uses sync.Once. While safe, this call is redundant and can be removed for clarity.
go/app/cli.go
Outdated
| } | ||
| case registerCliCommand("proxysql-servers", "ProxySQL", `Show mysql_servers from ProxySQL`): | ||
| { | ||
| proxysql.InitHook() |
There was a problem hiding this comment.
Similar to the proxysql-test command, proxysql.InitHook() is called here again. This call is redundant as InitHook is already executed globally and is designed to run only once.
There was a problem hiding this comment.
Pull request overview
Adds first-class ProxySQL integration to orchestrator by introducing built-in pre/post failover hooks that update ProxySQL hostgroups via the Admin API, plus CLI helpers and configuration/doc updates to make the feature usable out of the box.
Changes:
- Introduce new
go/proxysqlpackage (Admin client + hook logic + singleton init + unit tests). - Wire ProxySQL hooks into dead-master recovery and graceful master takeover flows.
- Add new config fields, sample config entries, CLI commands, and documentation for ProxySQL hooks.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go/proxysql/init.go | Singleton hook initialization and global accessor |
| go/proxysql/client.go | ProxySQL Admin SQL client (DSN + exec/query/ping) |
| go/proxysql/hook.go | Pre/post failover SQL generation and hook execution |
| go/proxysql/client_test.go | Unit tests for client initialization/DSN |
| go/proxysql/hook_test.go | Unit tests for hook no-op behavior and SQL generation |
| go/logic/topology_recovery.go | Invoke pre/post failover hooks during recovery/takeover |
| go/logic/orchestrator.go | Initialize ProxySQL hooks during continuous discovery startup |
| go/app/cli.go | Add proxysql-test and proxysql-servers CLI commands |
| go/config/config.go | Add new ProxySQL configuration fields + defaults |
| go/config/config_test.go | Test default values for new ProxySQL config fields |
| docs/proxysql-hooks.md | Document ProxySQL hook configuration and behavior |
| conf/orchestrator-sample.conf.json | Add sample ProxySQL config keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var ( | ||
| hookOnce sync.Once | ||
| defaultHook = NewHook(nil, 0, 0, "") | ||
| ) | ||
|
|
||
| func InitHook() { | ||
| hookOnce.Do(func() { | ||
| client := NewClient( | ||
| config.Config.ProxySQLAdminAddress, | ||
| config.Config.ProxySQLAdminPort, | ||
| config.Config.ProxySQLAdminUser, | ||
| config.Config.ProxySQLAdminPassword, | ||
| config.Config.ProxySQLAdminUseTLS, | ||
| ) | ||
| hook := NewHook( | ||
| client, | ||
| config.Config.ProxySQLWriterHostgroup, | ||
| config.Config.ProxySQLReaderHostgroup, | ||
| config.Config.ProxySQLPreFailoverAction, | ||
| ) | ||
| if hook.IsConfigured() { | ||
| log.Infof("ProxySQL hooks enabled: admin=%s:%d writer_hg=%d reader_hg=%d", | ||
| config.Config.ProxySQLAdminAddress, | ||
| config.Config.ProxySQLAdminPort, | ||
| config.Config.ProxySQLWriterHostgroup, | ||
| config.Config.ProxySQLReaderHostgroup, | ||
| ) | ||
| } else if config.Config.ProxySQLAdminAddress != "" && config.Config.ProxySQLWriterHostgroup == 0 { | ||
| log.Warningf("ProxySQL: ProxySQLAdminAddress is set but ProxySQLWriterHostgroup is 0 (unconfigured). ProxySQL hooks will be inactive.") | ||
| } | ||
| defaultHook = hook | ||
| }) |
There was a problem hiding this comment.
defaultHook is reassigned inside hookOnce.Do(...) but read via GetHook() without any synchronization. Since InitHook() is started in a goroutine (see go/logic/orchestrator.go), this creates a real data race (read/write of defaultHook) under the race detector, even though it’s “just” a pointer write. Consider storing the hook in an atomic.Value (or guarding with a mutex), or make initialization synchronous and ensure defaultHook is never mutated after other goroutines can call GetHook().
| go ometrics.InitGraphiteMetrics() | ||
| go acceptSignals() | ||
| go kv.InitKVStores() | ||
| go proxysql.InitHook() |
There was a problem hiding this comment.
Starting proxysql.InitHook() in its own goroutine means failover/recovery code can call proxysql.GetHook() concurrently while initialization is still updating the global hook, which can cause a data race and also makes hook availability timing-dependent. It should be safe to call proxysql.InitHook() synchronously during startup (before recovery routines can run), or switch the hook storage to an atomic/mutex-protected mechanism.
| go proxysql.InitHook() | |
| proxysql.InitHook() |
go/proxysql/client.go
Outdated
| dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/", user, password, address, port) | ||
| if useTLS { | ||
| dsn += "?tls=true" |
There was a problem hiding this comment.
The ProxySQL Admin DSN here does not set any driver-side timeouts (e.g. timeout, readTimeout, writeTimeout). In failure scenarios (ProxySQL down / network blackhole), sql.Open/db.Exec can block for a long time, which would delay failover even though ProxySQL errors are intended to be non-blocking. Suggest adding explicit DSN timeouts (similar to go/db/db.go’s MySQL DSNs) or reusing existing config timeout settings for the ProxySQL client.
| dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/", user, password, address, port) | |
| if useTLS { | |
| dsn += "?tls=true" | |
| base := fmt.Sprintf("%s:%s@tcp(%s:%d)/", user, password, address, port) | |
| // Add driver-side timeouts so ProxySQL Admin operations fail fast on network issues. | |
| params := "timeout=1s&readTimeout=1s&writeTimeout=1s" | |
| dsn := base + "?" + params | |
| if useTLS { | |
| dsn = base + "?tls=true&" + params |
go/proxysql/hook.go
Outdated
| log.Infof("proxysql: pre-failover: draining old master %s:%d (action=%s)", oldMasterHost, oldMasterPort, h.preFailoverAction) | ||
| query, args := buildPreFailoverSQL(h.preFailoverAction, oldMasterHost, oldMasterPort, h.writerHostgroup) | ||
| if query == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
PreFailover logs "draining old master" before determining whether the configured action actually produces any SQL. With ProxySQLPreFailoverAction="none", buildPreFailoverSQL returns an empty query and the hook becomes a no-op, but the log still claims it is draining. Consider moving the log after the query == "" check or logging a distinct "skipped" message for none.
| log.Infof("proxysql: pre-failover: draining old master %s:%d (action=%s)", oldMasterHost, oldMasterPort, h.preFailoverAction) | |
| query, args := buildPreFailoverSQL(h.preFailoverAction, oldMasterHost, oldMasterPort, h.writerHostgroup) | |
| if query == "" { | |
| return nil | |
| } | |
| query, args := buildPreFailoverSQL(h.preFailoverAction, oldMasterHost, oldMasterPort, h.writerHostgroup) | |
| if query == "" { | |
| log.Infof("proxysql: pre-failover: skipping drain of old master %s:%d (action=%s)", oldMasterHost, oldMasterPort, h.preFailoverAction) | |
| return nil | |
| } | |
| log.Infof("proxysql: pre-failover: draining old master %s:%d (action=%s)", oldMasterHost, oldMasterPort, h.preFailoverAction) |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
go/config/config_test.go (1)
40-54: Consider consistent assertion style and add missing default check.The test uses
t.Errorfwhile other tests in this file use thetest.S(t)helper. Also,ProxySQLPreFailoverActiondefault ("offline_soft") is not tested.🧪 Align with existing test style
func TestProxySQLConfigDefaults(t *testing.T) { cfg := newConfiguration() - if cfg.ProxySQLAdminPort != 6032 { - t.Errorf("expected default ProxySQLAdminPort=6032, got %d", cfg.ProxySQLAdminPort) - } - if cfg.ProxySQLAdminUser != "admin" { - t.Errorf("expected default ProxySQLAdminUser=admin, got %s", cfg.ProxySQLAdminUser) - } - if cfg.ProxySQLWriterHostgroup != 0 { - t.Errorf("expected default ProxySQLWriterHostgroup=0, got %d", cfg.ProxySQLWriterHostgroup) - } - if cfg.ProxySQLReaderHostgroup != 0 { - t.Errorf("expected default ProxySQLReaderHostgroup=0, got %d", cfg.ProxySQLReaderHostgroup) - } + test.S(t).ExpectEquals(cfg.ProxySQLAdminPort, 6032) + test.S(t).ExpectEquals(cfg.ProxySQLAdminUser, "admin") + test.S(t).ExpectEquals(cfg.ProxySQLWriterHostgroup, 0) + test.S(t).ExpectEquals(cfg.ProxySQLReaderHostgroup, 0) + test.S(t).ExpectEquals(cfg.ProxySQLPreFailoverAction, "offline_soft") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/config/config_test.go` around lines 40 - 54, Update TestProxySQLConfigDefaults to use the project's test helper (call test.S(t) and use the returned assertion object) instead of raw t.Errorf checks, and add an assertion that cfg.ProxySQLPreFailoverAction equals "offline_soft"; locate the test function TestProxySQLConfigDefaults and the configuration constructor newConfiguration() to make these changes and replace the four t.Errorf checks with equivalent assertions (e.g., s.Require().Equal or s.Equal) and add one more assertion for ProxySQLPreFailoverAction.go/proxysql/client.go (2)
62-74: Unusual API:Queryreturns*sql.DBrequiring caller to close both.This design requires callers to manage two resources (
rowsanddb). While the pattern works (as shown in cli.go context snippet), it's error-prone. Consider either:
- Adding a helper method that handles iteration internally
- Documenting this clearly in the function comment
The current comment says "Caller must close both rows and db" which is good, but the order matters -
rows.Close()should be called beforedb.Close().📝 Suggested documentation improvement
-// Query executes a query and returns rows. Caller must close both rows and db. +// Query executes a query and returns rows. Caller must close both rows and db, +// with rows.Close() called before db.Close(). Example: +// rows, db, err := client.Query(...) +// if err != nil { return err } +// defer db.Close() +// defer rows.Close() // defers execute LIFO, so rows closes first🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/proxysql/client.go` around lines 62 - 74, The Query method on Client returns both *sql.Rows and *sql.DB which is error-prone; update the function by either adding a convenience helper (e.g., Client.QueryFunc or Client.QueryAll) that accepts a callback to iterate rows and closes rows then db internally, or improve the doc comment on Client.Query to explicitly state the required close order (call rows.Close() before db.Close()) and the rationale; reference Client.Query, rows.Close(), db.Close(), and the new helper name if added so callers can find and use the safe alternative.
38-44: Consider adding connection timeout.The
openDB()function doesn't configure any connection timeout. If ProxySQL is unreachable, operations could hang indefinitely. The MySQL driver supports timeout parameters in the DSN.⏱️ Add connection timeout to DSN
func NewClient(address string, port int, user, password string, useTLS bool) *Client { if address == "" { return nil } - dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/", user, password, address, port) + dsn := fmt.Sprintf("%s:%s@tcp(%s:%d)/?timeout=10s&readTimeout=30s&writeTimeout=30s", user, password, address, port) if useTLS { - dsn += "?tls=true" + dsn += "&tls=true" }go/app/cli.go (1)
217-217: Note:proxysql.InitHook()called redundantly.
InitHook()is called both at line 217 (for all CLI commands) and again at lines 1791/1812 within the ProxySQL-specific commands. This is safe due to the singleton pattern but slightly redundant. The calls within the specific commands can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/app/cli.go` at line 217, proxysql.InitHook() is being invoked globally for all CLI commands and again inside the ProxySQL-specific command handlers; remove the redundant calls inside the ProxySQL command implementations so InitHook() is only called once globally (keep the single top-level proxysql.InitHook() call and delete the additional proxysql.InitHook() invocations found inside the ProxySQL-specific command functions).go/proxysql/client_test.go (1)
21-27: Consider adding test for TLS DSN construction.The test covers the non-TLS case but doesn't verify the DSN when
useTLS=true. This would ensure the TLS parameter is correctly appended.🧪 Add TLS DSN test
+func TestClientDSNWithTLS(t *testing.T) { + client := NewClient("127.0.0.1", 6032, "admin", "secret", true) + expected := "admin:secret@tcp(127.0.0.1:6032)/?tls=true" + if client.dsn != expected { + t.Errorf("expected DSN %q, got %q", expected, client.dsn) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/proxysql/client_test.go` around lines 21 - 27, Add a new unit test that covers the TLS case: call NewClient with useTLS=true (e.g., in a new TestClientDSN_TLS or by extending TestClientDSN), then assert client.dsn equals the expected TLS DSN string (same base "admin:secret@tcp(127.0.0.1:6032)/" with the TLS query param appended, e.g. including "?tls=true" or the specific TLS query your NewClient generates); use the same t.Errorf style as TestClientDSN to fail when client.dsn does not match the expected value.go/proxysql/hook_test.go (1)
69-77: Consider adding content validation for post-failover SQL.The test verifies the count of SQL statements and args but doesn't validate the actual SQL content. Consider adding assertions for expected SQL patterns similar to
TestPreFailoverSQLGeneration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/proxysql/hook_test.go` around lines 69 - 77, TestPostFailoverSQLGeneration currently only checks counts; extend it to validate the actual SQL content returned by buildPostFailoverSQL so regressions are caught. Inside TestPostFailoverSQLGeneration, iterate over the sqls and assert each statement matches expected patterns (use strings.Contains or regexp) for the post-failover operations (e.g., statements that set new-master host/port, remove or reweight old-master) and assert corresponding args in args slice include the expected values ("new-master", "old-master", 3306, and weight values like 10/20). Reference the existing TestPreFailoverSQLGeneration for pattern/assertion style and validate that sqls[i] content and args[i] align with the intended effect of buildPostFailoverSQL.go/proxysql/init.go (1)
10-13: Data race ondefaultHookpointer.
defaultHookis written inInitHook()(line 40) and read inGetHook()(line 45) without synchronization. SinceInitHook()is launched as a goroutine (pergo/logic/orchestrator.go:590), concurrent calls toGetHook()during startup can race.The current design mitigates impact because the initial
NewHook(nil, 0, 0, "")returns a no-op hook (IsConfigured()returns false). However, this is still undefined behavior under the Go memory model.♻️ Proposed fix using atomic.Pointer (Go 1.19+)
import ( "sync" + "sync/atomic" "github.com/proxysql/golib/log" "github.com/proxysql/orchestrator/go/config" ) var ( hookOnce sync.Once - defaultHook = NewHook(nil, 0, 0, "") + defaultHook atomic.Pointer[Hook] ) + +func init() { + defaultHook.Store(NewHook(nil, 0, 0, "")) +}Then update the assignment and getter:
- defaultHook = hook + defaultHook.Store(hook) }) } func GetHook() *Hook { - return defaultHook + return defaultHook.Load() }Also applies to: 44-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/proxysql/init.go` around lines 10 - 13, defaultHook is a plain pointer causing a data race between InitHook() (which assigns it in a goroutine) and GetHook() (which reads it); change defaultHook to an atomic.Pointer[Hook] (from sync/atomic) and initialize it with Store(NewHook(...)) instead of direct assignment, then update GetHook() to use Load() and handle a nil case by returning a safe no-op hook (e.g., NewHook(nil,0,0,"")); ensure InitHook() uses defaultHook.Store(...) and any reads use defaultHook.Load() so accesses are atomic and race-free while preserving hookOnce and NewHook semantics.go/proxysql/hook.go (1)
72-84: Consider logging a warning for unknownpreFailoverActionvalues.The default case silently falls back to
offline_soft. While this is safe, logging a warning would help operators identify configuration typos.💡 Optional improvement
default: + log.Warningf("proxysql: unknown preFailoverAction '%s', defaulting to 'offline_soft'", action) return "UPDATE mysql_servers SET status='OFFLINE_SOFT' WHERE hostname=? AND port=? AND hostgroup_id=?", args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/proxysql/hook.go` around lines 72 - 84, The default branch in buildPreFailoverSQL silently treats unknown action values as "offline_soft"; update buildPreFailoverSQL to log a warning when the action is unrecognized (e.g., log.Printf or the package's logger) indicating the unexpected preFailoverAction and that it's defaulting to offline_soft, so operators can spot configuration typos; reference the function name buildPreFailoverSQL and the action parameter when adding the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proxysql-hooks.md`:
- Around line 61-71: The fenced code block containing the ASCII diagram (the
block that begins with triple backticks before the "Dead master detected"
diagram) is missing a language specifier; update that opening fence to include a
language label such as text or plaintext (e.g., change ``` to ```text) so the
static analysis error is resolved and syntax/highlighting tools treat the
diagram as plain text.
- Around line 53-57: The docs miss describing that when readerHostgroup > 0 the
old master is also added back to the reader hostgroup with OFFLINE_SOFT status;
update the prose to include a step (after removing old master from writer
hostgroup) stating that the old master is added to the reader hostgroup with
OFFLINE_SOFT so it doesn't immediately accept traffic (mirror the behavior
implemented in hook.go around the readerHostgroup/ OFFLINE_SOFT logic).
In `@go/app/cli.go`:
- Around line 1789-1809: The proxysql-test command should defensively verify the
client returned by proxysql.NewClient is not nil before calling client.Ping to
avoid a potential nil pointer panic; update the proxysql-test case to mirror the
proxysql-servers check: after creating client with proxysql.NewClient(...) check
if client == nil and call log.Fatal with a clear message (e.g., "Unable to
create ProxySQL client") if nil, otherwise proceed to call client.Ping() and
handle its error as already done.
In `@go/logic/orchestrator.go`:
- Line 39: Move the "github.com/proxysql/orchestrator/go/proxysql" import into
the same block as the other internal imports from
"github.com/proxysql/orchestrator/go/..." in orchestrator.go and ensure the
internal imports are sorted alphabetically (so proxysql is ordered correctly
among peers); run gofmt/goimports to verify the import grouping and ordering is
fixed.
In `@go/logic/topology_recovery.go`:
- Around line 39-42: Reorder the import block in topology_recovery.go to satisfy
Go conventions: place standard library imports first, a blank line, then
third-party/external packages (e.g., "github.com/patrickmn/go-cache",
"github.com/rcrowley/go-metrics"), another blank line, and finally internal
project imports (e.g., "github.com/proxysql/orchestrator/go/proxysql",
"github.com/proxysql/orchestrator/go/util"); you can auto-fix by running gofmt
-w go/logic/topology_recovery.go or goimports -w go/logic/topology_recovery.go
to update the import ordering.
---
Nitpick comments:
In `@go/app/cli.go`:
- Line 217: proxysql.InitHook() is being invoked globally for all CLI commands
and again inside the ProxySQL-specific command handlers; remove the redundant
calls inside the ProxySQL command implementations so InitHook() is only called
once globally (keep the single top-level proxysql.InitHook() call and delete the
additional proxysql.InitHook() invocations found inside the ProxySQL-specific
command functions).
In `@go/config/config_test.go`:
- Around line 40-54: Update TestProxySQLConfigDefaults to use the project's test
helper (call test.S(t) and use the returned assertion object) instead of raw
t.Errorf checks, and add an assertion that cfg.ProxySQLPreFailoverAction equals
"offline_soft"; locate the test function TestProxySQLConfigDefaults and the
configuration constructor newConfiguration() to make these changes and replace
the four t.Errorf checks with equivalent assertions (e.g., s.Require().Equal or
s.Equal) and add one more assertion for ProxySQLPreFailoverAction.
In `@go/proxysql/client_test.go`:
- Around line 21-27: Add a new unit test that covers the TLS case: call
NewClient with useTLS=true (e.g., in a new TestClientDSN_TLS or by extending
TestClientDSN), then assert client.dsn equals the expected TLS DSN string (same
base "admin:secret@tcp(127.0.0.1:6032)/" with the TLS query param appended, e.g.
including "?tls=true" or the specific TLS query your NewClient generates); use
the same t.Errorf style as TestClientDSN to fail when client.dsn does not match
the expected value.
In `@go/proxysql/client.go`:
- Around line 62-74: The Query method on Client returns both *sql.Rows and
*sql.DB which is error-prone; update the function by either adding a convenience
helper (e.g., Client.QueryFunc or Client.QueryAll) that accepts a callback to
iterate rows and closes rows then db internally, or improve the doc comment on
Client.Query to explicitly state the required close order (call rows.Close()
before db.Close()) and the rationale; reference Client.Query, rows.Close(),
db.Close(), and the new helper name if added so callers can find and use the
safe alternative.
In `@go/proxysql/hook_test.go`:
- Around line 69-77: TestPostFailoverSQLGeneration currently only checks counts;
extend it to validate the actual SQL content returned by buildPostFailoverSQL so
regressions are caught. Inside TestPostFailoverSQLGeneration, iterate over the
sqls and assert each statement matches expected patterns (use strings.Contains
or regexp) for the post-failover operations (e.g., statements that set
new-master host/port, remove or reweight old-master) and assert corresponding
args in args slice include the expected values ("new-master", "old-master",
3306, and weight values like 10/20). Reference the existing
TestPreFailoverSQLGeneration for pattern/assertion style and validate that
sqls[i] content and args[i] align with the intended effect of
buildPostFailoverSQL.
In `@go/proxysql/hook.go`:
- Around line 72-84: The default branch in buildPreFailoverSQL silently treats
unknown action values as "offline_soft"; update buildPreFailoverSQL to log a
warning when the action is unrecognized (e.g., log.Printf or the package's
logger) indicating the unexpected preFailoverAction and that it's defaulting to
offline_soft, so operators can spot configuration typos; reference the function
name buildPreFailoverSQL and the action parameter when adding the warning.
In `@go/proxysql/init.go`:
- Around line 10-13: defaultHook is a plain pointer causing a data race between
InitHook() (which assigns it in a goroutine) and GetHook() (which reads it);
change defaultHook to an atomic.Pointer[Hook] (from sync/atomic) and initialize
it with Store(NewHook(...)) instead of direct assignment, then update GetHook()
to use Load() and handle a nil case by returning a safe no-op hook (e.g.,
NewHook(nil,0,0,"")); ensure InitHook() uses defaultHook.Store(...) and any
reads use defaultHook.Load() so accesses are atomic and race-free while
preserving hookOnce and NewHook semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 283df9ab-3bb5-453b-b4c4-29080bf5ab78
📒 Files selected for processing (12)
conf/orchestrator-sample.conf.jsondocs/proxysql-hooks.mdgo/app/cli.gogo/config/config.gogo/config/config_test.gogo/logic/orchestrator.gogo/logic/topology_recovery.gogo/proxysql/client.gogo/proxysql/client_test.gogo/proxysql/hook.gogo/proxysql/hook_test.gogo/proxysql/init.go
…prove logging - Use atomic.Pointer[Hook] for defaultHook to fix data race - Add DSN timeouts (1s connect/read/write) to ProxySQL client - Fix import ordering in orchestrator.go and topology_recovery.go - Move pre-failover log after query=="" check to avoid misleading message - Add log.Warningf for unknown preFailoverAction in buildPreFailoverSQL - Remove redundant InitHook() calls in CLI proxysql-test/proxysql-servers - Add nil check for client in proxysql-test command - Fix docs: add missing reader hostgroup step, use ```text fence
Closes #30
Summary
Built-in pre/post failover hooks that update ProxySQL hostgroups via its Admin API — no custom scripts needed. "orchestrator + ProxySQL works out of the box."
New
go/proxysql/packageclient.go— ProxySQL Admin connection management (uses MySQL protocol driver)hook.go— Pre/post failover hook logic with parameterized SQL queriesinit.go— Singleton initialization, nil-safe (no-op when unconfigured)Configuration
8 new fields in
Configstruct:ProxySQLAdminAddress,ProxySQLAdminPort(default: 6032),ProxySQLAdminUser(default: admin),ProxySQLAdminPassword,ProxySQLAdminUseTLSProxySQLWriterHostgroup,ProxySQLReaderHostgroupProxySQLPreFailoverAction(default: offline_soft)Failover integration
CLI commands
proxysql-test— Test connectivity to ProxySQL Adminproxysql-servers— Show runtime_mysql_servers tableSafety
defaultHookinitialized at package level to no-op, avoiding data racesTest plan
go test ./go/proxysql/...)go test ./go/... -vet=off)go build -o bin/orchestrator ./go/cmd/orchestrator)Summary by CodeRabbit
New Features
Documentation
Tests