fix(replication): prevent excessive wait times after deploy (fixes #131) - #137
Open
renecannao wants to merge 7 commits into
Open
fix(replication): prevent excessive wait times after deploy (fixes #131)#137renecannao wants to merge 7 commits into
renecannao wants to merge 7 commits into
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. |
📝 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 Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
Comment on lines
+80
to
+82
| if ! $use_cmd -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null | grep -q 'wsrep_ready'; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat a failed status probe as “non-Galera”.
When $use_cmd fails, stderr is suppressed and the pipeline produces no matching output, so ! ... | grep enters this branch and returns success. A Galera node that is temporarily unavailable can therefore skip all readiness polling and proceed before wsrep_ready=ON.
Only short-circuit when SHOW STATUS succeeds and returns no row; otherwise continue polling or return the probe failure.
Suggested fix
+ local wsrep_status
- if ! $use_cmd -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null | grep -q 'wsrep_ready'; then
- return 0
+ if wsrep_status=$($use_cmd -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null); then
+ if ! grep -q 'wsrep_ready' <<<"$wsrep_status"; then
+ return 0
+ fi
fi📝 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
| if ! $use_cmd -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null | grep -q 'wsrep_ready'; then | |
| return 0 | |
| fi | |
| local wsrep_status | |
| if wsrep_status=$($use_cmd -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null); then | |
| if ! grep -q 'wsrep_ready' <<<"$wsrep_status"; then | |
| return 0 | |
| fi | |
| 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 `@sandbox/templates/single/sb_include.gotxt` around lines 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.
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.
renecannao
force-pushed
the
fix/131-replica-ready-racing-fix
branch
from
July 30, 2026 00:53
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
Comment on lines
+125
to
+130
| # Connected: empty SHOW STATUS means wsrep_ready does not exist (non-Galera). | ||
| local wsrep_check | ||
| wsrep_check=$($mysql_cmd --no-defaults -S "$SOCKET_FILE" -u root -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null) | ||
| if [ -z "$wsrep_check" ]; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check the query's exit status before treating empty output as "non-Galera".
Line 127 discards the exit status of $mysql_cmd. If the query fails after a successful connect (transient socket drop, server restart), stdout is empty and line 128 wrongly returns 0, treating a real Galera node as non-Galera and skipping the wsrep_ready wait. This is the same failure pattern already flagged on this file for the previous grep-pipeline version: a command failure is indistinguishable from "no matching row" and is misclassified as success.
Distinguish command failure from an empty-but-successful result.
🐛 Proposed fix to distinguish query failure from a genuinely missing variable
# Connected: empty SHOW STATUS means wsrep_ready does not exist (non-Galera).
local wsrep_check
- wsrep_check=$($mysql_cmd --no-defaults -S "$SOCKET_FILE" -u root -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null)
- if [ -z "$wsrep_check" ]; then
- return 0
- fi
+ if wsrep_check=$($mysql_cmd --no-defaults -S "$SOCKET_FILE" -u root -BN -e "SHOW STATUS LIKE 'wsrep_ready';" 2>/dev/null); then
+ if [ -z "$wsrep_check" ]; then
+ return 0
+ fi
+ else
+ echo "WARNING: wsrep_ready query failed after connect; retrying" >&2
+ 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 `@sandbox/templates/single/sb_include.gotxt` around lines 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.
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.
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