fix(replication): prevent excessive wait times after deploy (fixes #131) - #137
Conversation
Two issues caused multi-minute delays on 'deploy replication' in v2.4.0: 1. wait_until_wsrep_ready spun 120s per node on non-Galera MySQL (commit a2caf94 added wait_wsrep_after_start but didn't guard against vanilla MySQL where wsrep_ready does not exist) 2. wait_until_replica_ready defaulted to max_attempts=60 (60s per replica), while the original reporter found 5s was sufficient Fixes: - wait_until_wsrep_ready: check if the wsrep_ready status variable exists before polling; return immediately on non-Galera nodes - wait_until_replica_ready: reduce default max_attempts from 60 to 20 (≤20s total wait per replica) - Update call sites in init_slaves.gotxt and init_slaves_84.gotxt Tests: - TestInitSlavesTemplates_IncludeReplicaReadyWait: also check timeout warning message; fix pre-existing missing template fields - TestWaitWsrepAfterStart_DoesNotBlockNonGalera: verifies the wait_wsrep_after_start template renders and uses || true - TestSbInclude_WaitUntilWsrepReady_DetectsNonGalera: verifies the wsrep existence guard exists in sb_include - TestSbInclude_WaitUntilReplicaReady_BoundedWait: verifies max_attempts default is 20 (≤30s budget)
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 32 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplica readiness polling is shortened from 60 to 20 attempts across replication templates. Single-instance wsrep readiness now exits early when the status variable is unavailable. New integration coverage measures replication deployment time and verifies immediate master and replica readiness. ChangesReplica readiness and template validation
Deployment time regression coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReplicaStartup
participant ReadinessHelpers
participant MySQLSocket
ReplicaStartup->>ReadinessHelpers: start replica and request readiness
ReadinessHelpers->>MySQLSocket: connect as root and query readiness
MySQLSocket-->>ReadinessHelpers: return wsrep or replica status
ReadinessHelpers-->>ReplicaStartup: continue or report bounded timeout
sequenceDiagram
participant IntegrationWorkflow
participant DeployTimeBudget
participant Dbdeployer
participant ReplicationSandboxes
IntegrationWorkflow->>DeployTimeBudget: run with MAX_DEPLOY_SECONDS=45
DeployTimeBudget->>Dbdeployer: deploy two-node replication
Dbdeployer-->>DeployTimeBudget: return deployment result and elapsed time
DeployTimeBudget->>ReplicationSandboxes: query replica and master
ReplicationSandboxes-->>DeployTimeBudget: return query responses and logs
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🧹 Nitpick comments (1)
sandbox/templates_flavor_test.go (1)
236-247: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the template assertions beyond substring presence.
The replica test verifies that each wait command exists, but not that it appears after the corresponding
START REPLICAcommand. The wsrep test similarly checks only thegreptoken and default value; it would pass even if the guard returned failure—or incorrectly returned success when the probe command failed.Assert command ordering for each replica and execute the rendered wsrep helper with stubs covering both “no variable” and “probe failed” cases.
Also applies to: 282-287
🤖 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 `@sandbox/templates_flavor_test.go` around lines 236 - 247, Strengthen the rendered-template tests by asserting each wait_until_replica_ready invocation occurs after its matching START REPLICA command, rather than only checking presence. Extend the wsrep helper assertions to execute the rendered helper with stubs for both an unset variable and a failed probe, verifying the guard returns the expected failure status in each case and does not report success when probing fails.
🤖 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 `@sandbox/templates/single/sb_include.gotxt`:
- Around line 80-82: Update the wsrep status probe around $use_cmd so a failed
SHOW STATUS command is not interpreted as a non-Galera node. Capture and
distinguish the command’s exit status from grep’s no-match result: short-circuit
only when SHOW STATUS succeeds without a wsrep_ready row, and otherwise preserve
the probe failure for continued polling or return it.
---
Nitpick comments:
In `@sandbox/templates_flavor_test.go`:
- Around line 236-247: Strengthen the rendered-template tests by asserting each
wait_until_replica_ready invocation occurs after its matching START REPLICA
command, rather than only checking presence. Extend the wsrep helper assertions
to execute the rendered helper with stubs for both an unset variable and a
failed probe, verifying the guard returns the expected failure status in each
case and does not report success when probing fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c344a9a-45c5-4caa-b885-101b9a887a54
📒 Files selected for processing (4)
sandbox/templates/replication/init_slaves.gotxtsandbox/templates/replication/init_slaves_84.gotxtsandbox/templates/single/sb_include.gotxtsandbox/templates_flavor_test.go
Adds test/deploy-time-budget.sh, a standalone bash test that: - Deploys replication (1 slave) with real MySQL binaries - Measures wall-clock time against MAX_DEPLOY_SECONDS (45s) - Verifies the replica serves queries immediately after deploy (catches the #131 race condition where deploy returned before replica was ready) - Checks for server errors in the error log Adds a deploy-time-budget CI job in integration_tests.yml that: - Downloads and unpacks MySQL 8.4.4 - Runs the time-budget test with MAX_DEPLOY_SECONDS=45 - Fails if deploy exceeds the budget (catches regressions like the 120s wsrep spin or 60s replica wait from v2.4.0)
… -n flag - sb_include.gotxt: connect as root (no password) via socket for wsrep detection guard, so it works before msandbox user exists - deploy-time-budget.sh: use -n 2 (total nodes, not slaves) for replication with 1 slave
wait_until_wsrep_ready now uses root via socket for both detection and polling, since msandbox user does not exist before load_grants. This eliminates the 120s wasted polling loop per node on fresh Galera/PXC nodes while still correctly waiting for wsrep_ready=ON before grants/DML.
The start/stop/use templates use shell if blocks (not Go template
{{if}}) so both branches' literal strings appear in rendered output.
Tests now verify the correct default AND the presence of the flavor
conditional, rather than asserting the absence of the other flavor's
binary string.
34601c0 to
df96a7f
Compare
- Retry root socket connect before Galera detection so a not-yet-ready server is not mistaken for non-Galera (empty query output). - Resolve client as mariadb/mysql by FLAVOR (CLIENT_BASEDIR then BASEDIR). - Match wsrep_ready ON specifically; keep a modest connect budget (30s).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/integration_tests.yml (1)
134-142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHonor the restored tarball cache.
This step always downloads and overwrites the cached tarball, so the cache provides no time or reliability benefit. Match the existing matrix job’s file-existence guard.
Proposed fix
mkdir -p /tmp/mysql-tarball TARBALL="mysql-8.4.4-linux-glibc2.17-x86_64.tar.xz" - curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ - "https://dev.mysql.com/get/Downloads/MySQL-8.4/$TARBALL" \ - || curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ - "https://downloads.mysql.com/archives/get/p/23/file/$TARBALL" + if [ ! -f "/tmp/mysql-tarball/$TARBALL" ]; then + curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ + "https://dev.mysql.com/get/Downloads/MySQL-8.4/$TARBALL" \ + || curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ + "https://downloads.mysql.com/archives/get/p/23/file/$TARBALL" + fi🤖 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 @.github/workflows/integration_tests.yml around lines 134 - 142, Update the “Download MySQL 8.4.4” step to guard the curl download with the existing tarball file-existence check used by the matrix job, so an already cached /tmp/mysql-tarball/$TARBALL is preserved; continue listing the tarball afterward.
🤖 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 @.github/workflows/integration_tests.yml:
- Line 114: Update the actions/checkout@v4 step in the executable-download job
to disable persisted credentials by configuring persist-credentials as false.
Keep the checkout behavior otherwise unchanged.
In `@test/deploy-time-budget.sh`:
- Around line 93-111: Invoke the script’s existing cleanup function immediately
before the `dbdeployer deploy replication` command, ensuring stale `rsandbox_*`
fixtures are removed before deployment while preserving the current
cleanup-on-exit behavior.
- Around line 50-68: Update find_test_version so its missing-directory and
no-versions diagnostics are written to stderr, keeping failed discovery output
out of VERSION. Require the preferred-version branch to select only 8.4+ or any
9.x version, and update the VERSION assignment/call site to branch on
find_test_version’s exit status so failures trigger the existing skip path
instead of continuing with diagnostic text.
---
Nitpick comments:
In @.github/workflows/integration_tests.yml:
- Around line 134-142: Update the “Download MySQL 8.4.4” step to guard the curl
download with the existing tarball file-existence check used by the matrix job,
so an already cached /tmp/mysql-tarball/$TARBALL is preserved; continue listing
the tarball afterward.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c68ab1b-c48b-452b-8c0a-1138302ea084
📒 Files selected for processing (5)
.github/workflows/integration_tests.ymlsandbox/templates/single/sb_include.gotxtsandbox/templates/single/wait_wsrep_after_start.gotxtsandbox/templates_flavor_test.gotest/deploy-time-budget.sh
- workflow: persist-credentials: false on the executable-download job so a compromised MySQL binary cannot read the job token (artipacked). - deploy-time-budget.sh: send version-discovery diagnostics to stderr and branch on exit status so a failed discovery hits the skip path instead of leaking text into VERSION. - Require MySQL >= 8.4 (was >= 8.0) to match the stated #131 target. - Clean stale rsandbox_* fixtures before deploying so a leftover ready topology cannot make the test pass spuriously.
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 `@sandbox/templates/single/sb_include.gotxt`:
- Around line 125-130: Update the wsrep_check query handling near the local
wsrep_check assignment to capture and inspect $mysql_cmd’s exit status before
interpreting empty output. Return 0 only when the query succeeds with no result;
propagate or handle a failed query as an error so transient connection failures
do not bypass the wsrep_ready wait.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b4191f0-dcf9-45cd-97b4-f9538fd72dac
📒 Files selected for processing (4)
.github/workflows/integration_tests.ymlsandbox/templates/single/sb_include.gotxtsandbox/templates_flavor_test.gotest/deploy-time-budget.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration_tests.yml
Only treat an empty wsrep_ready probe as "non-Galera" when the SHOW STATUS query itself succeeds. A failed probe now falls through to the polling loop instead of bypassing the wsrep_ready wait (CodeRabbit).
wait_until_replica_ready connects as the sandbox user, but init_slaves exports NOPASSWORD=1 for the root steps and that leaked into the probe: 'use' then drops the password, so SELECT 1 fails with Access denied even once the user exists, and the poll burned its full ~20s budget on every replication deploy. Clear NOPASSWORD for the probe so it authenticates and returns as soon as the replica is actually ready (~1-2s).
|
Thanks for working on this @renecannao. I pulled Are you planning to release a v2.4.1 soon ? |
|
Hi @jfg956 . Yes, I will fire the release of v2.4.1 shortly . |
2 participants
After
dbdeployer deploy replicationin v2.4.0, the deployment takes ~5 minutes instead of seconds. This PR fixes two independent sources of excessive delay:Root causes
1.
wait_until_wsrep_readyspins 120s per node on non-Galera MySQLCommit
a2caf94addedwait_wsrep_after_start(to fix PXC/Galera flakes) but it runs unconditionally for every sandbox node. On vanilla MySQL,SHOW STATUS LIKE 'wsrep_ready'returns empty,grep -qi 'ON'fails, and the function waits the full 60 attempts × 2s = 120s per node. With master + 1 slave that is 4 minutes of unnecessary delay.2.
wait_until_replica_readydefault max_attempts=60The #131 fix (
4adea12) polled replicas withmax_attempts=60, sleep_sec=1. The original reporter found 5s was sufficient. Combined with the wsrep delay, this pushed the total to ~5min.Fixes
sb_include.gotxtwait_until_wsrep_ready: check ifwsrep_readystatus variable exists before polling; return immediately on non-Galerasb_include.gotxtwait_until_replica_ready: defaultmax_attempts60→20init_slaves.gotxt20 1instead of60 1init_slaves_84.gotxtTests added
TestWaitWsrepAfterStart_DoesNotBlockNonGalera— verifieswait_wsrep_after_starttemplate renders and uses|| trueTestSbInclude_WaitUntilWsrepReady_DetectsNonGalera— verifies the wsrep-existence guard exists insb_includeTestSbInclude_WaitUntilReplicaReady_BoundedWait— verifiesmax_attemptsdefault is 20 (≤30s budget)TestInitSlavesTemplates_IncludeReplicaReadyWait— enhanced to check timeout warning message; fixed pre-existing missing template fieldsFixes #131
Summary by CodeRabbit
Bug Fixes
Tests / CI