Keep chunk churn alive when a range deleter wedges - #1290
Conversation
The MongoDB Cluster Summary nightly asserts 9 empty panels and found 11: Chunks Move Events and Chunks Split Events were both blank. chunk-churn.js ran its repeat loop in-process and issued moveChunk with _waitForDelete: true and no time bound. A range deleter that never finishes blocks that command forever, and on the same collection it then blocks the next cycle's splitChunk too -- reproduced on a fresh sharded setup with the same 3-dev-latest digest as the failing run, where _shardsvrMoveRange sat at 1212s and splitChunk at 1115s with one pending config.rangeDeletions entry. mongosh stayed alive throughout, so restart: unless-stopped never fired. config.changelog recorded its last split and moveChunk at 01:43:29 and nothing after; by 02:03 an instant query for mongodb_mongos_sharding_changelog_10min_total returned no series at all, which is exactly what blanks both panels. Move the repeat loop into the compose entrypoint under `timeout` and make the script a single cycle, so a wedged command can only ever cost one cycle instead of the whole run. Bound each command with maxTimeMS as a soft stop so abandoned work does not pile up server-side, and drop _waitForDelete, which was the wedge trigger: it forced synchronous orphan cleanup while ranges bounced between the two shards. Pick the least-loaded shard as the move target rather than any other shard, so a range only goes back once the counts have genuinely swung. Both panels match every *split* / *moveChunk* event, moveChunk.start and moveChunk.error included, so a cycle that loses the race to orphan cleanup still keeps the series alive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QznHEPiKdzGoR7TvpeeLKZ Signed-off-by: travagliad <davi.travaglia@percona.com>
The server reads the first key of the command document as the command name, so leading with maxTimeMS made every split and moveChunk fail with "no such cmd: maxTimeMS". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QznHEPiKdzGoR7TvpeeLKZ Signed-off-by: travagliad <davi.travaglia@percona.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
WalkthroughThe chunk churn script now performs one cycle and applies Sequence Diagram(s)sequenceDiagram
participant ComposeEntrypoint
participant ChunkChurnScript
participant MongoDB
ComposeEntrypoint->>ComposeEntrypoint: validate timeout settings
ComposeEntrypoint->>ChunkChurnScript: invoke one churn cycle under timeout
ChunkChurnScript->>MongoDB: run bounded split, move, and merge commands
MongoDB-->>ChunkChurnScript: return command results
ChunkChurnScript-->>ComposeEntrypoint: exit after one cycle
ComposeEntrypoint->>ComposeEntrypoint: sleep between cycles
Merge Risk: 🟡 Moderate · up to The PR moves churn repetition into a timed command and adds configurable timeouts, but some accepted edge-case values can disable the cycle timeout or produce an invalid or inconsistent command timeout. That could allow a wedged cycle to stop producing data again, so the validation issues should be fixed or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: c3af9012-9096-4972-98ea-9fc466e5c47c
📒 Files selected for processing (2)
qa-integration/pmm_psmdb-pbm_setup/conf/chunk-churn/chunk-churn.jsqa-integration/pmm_psmdb-pbm_setup/docker-compose-sharded.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
No gate covers the behaviour this PR changes. Lint checks |
- leastLoadedShard only ever returned the other shard: start-sharded.sh adds exactly two, so excluding the current owner leaves one candidate and the per-shard counting and sort could not change the outcome. Go back to picking the other shard directly. Dropping _waitForDelete and the entrypoint timeout are what keep the cycle from wedging. - Reject a cycle or command timeout that is not a positive integer, and keep the command bound below the cycle bound. `timeout 0` runs without a limit and `maxTimeMS: 0` means unlimited, so either would silently disable the safeguard this change exists to add. CHURN_INTERVAL_SECONDS gets the same treatment since the loop passes it to sleep. - A cycle that exits non-zero has not necessarily timed out: mongosh also exits non-zero on a script error, and the in-process try/catch went away with the loop. Stop naming a cause the loop never checked. - Trim the comments that narrated the previous design in both files. That history belongs in the commit and the PR, not aged into the sources. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QznHEPiKdzGoR7TvpeeLKZ Signed-off-by: travagliad <davi.travaglia@percona.com>
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: a14e2283-5055-420f-a15a-66a5e46395d6
📒 Files selected for processing (2)
qa-integration/pmm_psmdb-pbm_setup/conf/chunk-churn/chunk-churn.jsqa-integration/pmm_psmdb-pbm_setup/docker-compose-sharded.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Three ways an operator could still end up with no limit at all, all confirmed by running the shipped validation directly: - `00` and `000` passed the digit-only check and reached GNU timeout as zero, which disables it -- `timeout 00 sh -c 'sleep 2'` lets the child run to completion. Reject any leading zero instead of just literal 0. That also removes an arithmetic hazard the check had introduced: `090` passed as digits, but the shell reads a leading zero as octal, so $(( 090 / 2 )) aborted the entrypoint with "value too great for base" before the loop ever started. - A cycle timeout of 1 made the derived command timeout 1/2 = 0, and the script then substituted its own 45s fallback -- a 45s maxTimeMS under a 1s kill. Require at least 2s for the cycle bound so the derived value cannot round down to zero. - maxTimeMS wants a positive integer, but `1.2345` and `Infinity` both satisfied `> 0`. Require Number.isInteger as well. An interval of 0 now falls back too: it was harmless for correctness but would have spun chunk moves in a tight loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QznHEPiKdzGoR7TvpeeLKZ Signed-off-by: travagliad <davi.travaglia@percona.com>
|
Green nightly test: https://pmm.cd.percona.com/job/pmm3-ui-tests-nightly-gha/375/ |
|
@coderabbitai review For reviewers, in case the summary comment above is read before it refreshes: its Merge Risk: Moderate · up to Current state of the head: E2E tests Matrix and lint both green, no merge conflict, all nine review threads resolved, and Generated by Claude Code |
|
|
1 participant
Failures fixed (investigator)
Nightly E2E tests Matrix (remote PMM Server), jobtest execution / @nightly)codeceptjs-e2e/tests/verifyMongodbDashboards_test.js:24/@nightly @dashboards— Open the MongoDB Cluster Summary Dashboard and verify Metrics are present and graphs are displayedWhat failed
Both Chunks panels read
mongodb_mongos_sharding_changelog_10min_total, a rolling10-minute window over
config.changelog. #1161 added thechunk-churnservice tokeep that window fed, because MongoDB 7.0+ no longer auto-splits and the balancer has
nothing to migrate in a 3-chunk hashed collection.
Root cause — the QA setup, not the product
The churn generator stops permanently the first time a range deletion wedges.
chunk-churn.jsran its repeat loop in-process and issuedmoveChunkwith_waitForDelete: trueand no time bound. That blocks until the donor's range deleterfinishes; when the deleter never finishes, the command never returns, and on the same
collection it then blocks the next cycle's
splitChunktoo. mongosh stays alivethroughout, so
restart: unless-stoppednever fires — the container looks healthy whileproducing nothing.
Reproduced on a throwaway Linode VM against
perconalab/pmm-server:3-dev-latestatsha256:ccdb9b80…, the same digest the failing run recorded to Launchable, using theunmodified
--database psmdb,SETUP_TYPE=shardingsetup:config.changelogrecorded its lastsplitandmoveChunkat 01:43:29 and nothing after.db.currentOpshowed_shardsvrMoveRangerunning 1212s andsplitChunk1115s on thers1primary, with one pendingconfig.rangeDeletionsentry.chunk-churnwasrunning=true restarts=0, last log line an orphans-cleanupExceededTimeLimit.mongodb_mongos_sharding_changelog_10min_total{cluster="sharded"}returned no series at all — which is exactly what renders both panels as "No data".The assertion is right and PMM is fine: the panels were populated 01:38–01:43 while the
churn was working. The bound of 9 is untouched.
The fix
chunk-churn.jsand into the compose entrypoint undertimeout, so a wedged command can only ever cost one cycle instead of the whole run. The script is now a single cycle._waitForDelete, the wedge trigger — it forced synchronous orphan cleanup while ranges bounced between the two shards.maxTimeMS(CHURN_COMMAND_TIMEOUT_SECONDS, 45s) as a soft stop so abandoned work does not pile up server-side;CHURN_CYCLE_TIMEOUT_SECONDS(90s) is the hard backstop. Worst case is one cycle per ~210s, comfortably inside the 10-minute window.timeout 0andmaxTimeMS: 0both mean "no limit",timeout 00is that same zero by another spelling, and a leading zero makes the shell read the value as octal ($(( 090 / 2 ))aborts the entrypoint outright). Anything that is not a positive integer without a leading zero falls back to the default, the cycle bound must be at least 2s so the derived command bound cannot round down to zero, and the script independently requiresNumber.isIntegerfor its ownmaxTimeMS.Both panels match every
*split*/*moveChunk*event —moveChunk.startandmoveChunk.errorincluded — so a cycle that loses the race to orphan cleanup still keepsthe series alive.
Verification
Confirmed by a real nightly on this branch
pmm3-ui-tests-nightly-gha#375 — SUCCESS,PMM_QA_GIT_BRANCH=claude/jolly-curie-72zswc,DOCKER_VERSION=perconalab/pmm-server:3-dev-latest, 81 minutes. This is the same pipelinewhose failure opened this investigation, run against the fix on a freshly provisioned CI
environment, so it settles the one thing the repro VM could not (see the caveat at the end
of this section, now closed). The
@qanscenario below passed in that run too.On the repro VM
The cluster that produced the wedge is poisoned by it (the orphaned server-side ops
outlive the client), so the fix was verified on a freshly provisioned sharded cluster,
over a window comparable to the gap between setup and the nightly's dashboard tests:
CHURN_INTERVAL_SECONDS…{event=~".*split.*"}series…{event=~".*moveChunk.*"}seriesEvaluating the panels' own expressions (
avg by (event) (irate(…[5m]))) returns series forboth, so they render lines rather than "No data".
The scenario itself passes on the fixed setup (
Number of no data and N/A elements is = 9,against the unchanged bound of 9).
Timeout validation
Exercised by extracting the shipped entrypoint from the compose file and running it
verbatim, so the table below is the real code's behaviour rather than a paraphrase of it:
CYCLE/COMMAND/INTERVALin90/45/120(defaults)0,00,000,090/45/1201/1/1202/5/1090/00/12090/200/12090/45/0abc/-5/1.5300/30/60Every derived command bound is positive and below the cycle bound. The script's own
maxTimeMSguard resolves unset,0,00,abc,-5,1.2345andInfinityto45000ms, and
30to 30000ms.What only a real nightly can confirm— now confirmedThe empty-panel total is environment-sensitive: the repro box's other panels are not the
same set that is empty on a CI runner (pre-fix, the same scenario passed there at ≤9 even
with both Chunks panels blank, because two panels that are empty in CI have data on that
box). The repro run established causally that the Chunks panels have data and that the
churn no longer wedges; the aggregate count against CI's own panel mix is what nightly #375
above confirms.
Also investigated — not fixed, did not reproduce
The same run's other red job,
test execution / @qan|@valkey-nightly|…, failed on:This did not reproduce: on the same VM the scenario passes,
CREATE TABLE classes (is present in QAN for the pdpgsql service, and it passed again in nightly #375. Nightly
could not be re-run at investigation time to settle it — the workflow takes an external
pmm_server_addressand that server is gone by the time this runs — so it is left unfixedand untracked here rather than guessed at.
Worth noting for whoever picks it up:
CREATE TABLE classesis a one-shot DDL frompgsql_load.sqlat setup time, andpg_stat_monitoron this setup retains onlypgsm_bucket_time=60×pgsm_max_buckets=10= 10 minutes. Registration does happenbefore the load, so there is no ordering bug, but a one-shot query that is missed by the
agent's first collection is unrecoverable for the rest of the run — a plausible source of
intermittency that a durable fix would address by not depending on a single DDL.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QznHEPiKdzGoR7TvpeeLKZ