Handle ProxySQL cleanup during sandbox deletion#48
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves sandbox deletion (dbdeployer delete) by attempting to gracefully stop an embedded ProxySQL “sub-sandbox” (via proxysql/stop) before removing the sandbox directory, aligning deletion behavior with setups that include ProxySQL.
Changes:
- Detect
proxysql/stopunder the sandbox directory and run it before deleting files. - Apply the same ProxySQL stop behavior to both
RemoveSandbox(deprecated) andRemoveCustomSandbox.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| proxysqlStop := path.Join(fullPath, "proxysql", "stop") | ||
| if _, statErr := os.Stat(proxysqlStop); statErr == nil { | ||
| common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql")) | ||
| _, _ = common.RunCmd(proxysqlStop) |
There was a problem hiding this comment.
When runConcurrently is true, this block executes proxysql/stop immediately instead of returning it as part of execList like the main sandbox stop/removal steps. That breaks the function’s concurrency contract (side effects during planning) and can also run ProxySQL stop before the scheduled DB stop. Consider using common.ExecExists and, if runConcurrently, append a concurrent.ExecCommand for proxysqlStop with the same priority as the main stop; if not concurrent, run it and handle/propagate errors (instead of ignoring them) so a failed stop doesn’t silently proceed to deletion.
| proxysqlStop := path.Join(fullPath, "proxysql", "stop") | |
| if _, statErr := os.Stat(proxysqlStop); statErr == nil { | |
| common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql")) | |
| _, _ = common.RunCmd(proxysqlStop) | |
| proxysqlSandbox := path.Join(fullPath, "proxysql") | |
| proxysqlStop := path.Join(proxysqlSandbox, "stop") | |
| if common.ExecExists(proxysqlStop) { | |
| if runConcurrently { | |
| var proxysqlCommand = concurrent.ExecCommand{ | |
| Cmd: proxysqlStop, | |
| Args: []string{}, | |
| } | |
| // Use the same priority as the main stop command | |
| execList = append(execList, concurrent.ExecutionList{Logger: nil, Priority: 0, Command: proxysqlCommand}) | |
| } else { | |
| common.CondPrintf("Stopping ProxySQL in %s\n", proxysqlSandbox) | |
| _, err = common.RunCmd(proxysqlStop) | |
| if err != nil { | |
| return emptyExecutionList, fmt.Errorf("error while stopping ProxySQL in %s: %s", proxysqlSandbox, err) | |
| } | |
| } |
| if _, statErr := os.Stat(proxysqlStop); statErr == nil { | ||
| common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql")) | ||
| _, _ = common.RunCmd(proxysqlStop) |
There was a problem hiding this comment.
Same issue here as in RemoveSandbox: this runs proxysql/stop synchronously even when runConcurrently is true, and it ignores execution errors. Please make ProxySQL stop follow the same execution model as the rest of the deletion workflow (enqueue as a priority-0 task when concurrent; otherwise run and handle/propagate errors). Also prefer common.ExecExists over os.Stat so we only attempt to execute an actually-executable stop script.
| if _, statErr := os.Stat(proxysqlStop); statErr == nil { | |
| common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql")) | |
| _, _ = common.RunCmd(proxysqlStop) | |
| if common.ExecExists(proxysqlStop) { | |
| common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql")) | |
| if runConcurrently { | |
| var proxysqlCommand = concurrent.ExecCommand{ | |
| Cmd: proxysqlStop, | |
| } | |
| execList = append(execList, concurrent.ExecutionList{Logger: nil, Priority: 0, Command: proxysqlCommand}) | |
| } else { | |
| if _, err = common.RunCmd(proxysqlStop); err != nil { | |
| return emptyExecutionList, fmt.Errorf(globals.ErrWhileStoppingSandbox, path.Join(fullPath, "proxysql")) | |
| } | |
| } |
| // If a proxysql sub-sandbox exists, stop it before removing the directory | ||
| proxysqlStop := path.Join(fullPath, "proxysql", "stop") | ||
| if _, statErr := os.Stat(proxysqlStop); statErr == nil { | ||
| common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql")) | ||
| _, _ = common.RunCmd(proxysqlStop) | ||
| } |
There was a problem hiding this comment.
New behavior (detecting and stopping a proxysql/stop script) isn’t covered by tests. Since sandbox/sandbox_test.go already exercises sandbox removal paths, please add a unit/integration-style test that creates a proxysql/stop script (e.g., one that writes a sentinel file) and asserts it is invoked during deletion, both for the concurrent and non-concurrent paths (as applicable).
Closes #42
Summary
dbdeployer delete, check if aproxysql/subdirectory exists with astopscriptproxysql/stopto gracefully shut down the ProxySQL process before the directory is deletedRemoveSandbox(deprecated) andRemoveCustomSandboxfunctionsTest plan
go build -o dbdeployer .dbdeployer deleteworks normally for sandboxes without ProxySQLdbdeployer deletestops ProxySQL before cleanup for sandboxes with aproxysql/subdirectory