ci(buildkit): probe pod ordinals up to the KEDA replica ceiling (OPS-7960) - #12377
Merged
Merged
Conversation
get_active_indices() discovers builder pods by resolving buildkit-<arch>-<i> over DNS for i in [0, MAX_POD_CHECK). The bound was 10, matching the KEDA maxReplicaCount at the time. Raising the builder pool above that leaves the extra pods undiscovered: they run, hold a node, and never receive a build, while the pods below the bound absorb the whole queue. Bump the default to 16 and read it from the environment so it can be tuned from a workflow without a code change. It has to stay >= the KEDA maxReplicaCount for the buildkit StatefulSet. Raising this ahead of any pool change is safe on its own: ordinals with no pod behind them simply fail to resolve and are skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Contributor
WalkthroughThe BuildKit routing script increases the default DNS pod probing limit from 10 to 16 and documents that the value is an exclusive upper bound for pod ordinals. ChangesBuildKit DNS probing
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
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 @.github/scripts/route_buildkit.sh:
- Around line 149-152: Validate MAX_POD_CHECK immediately after its default
assignment and before the probe loop, rejecting malformed or non-positive values
with a clear error and nonzero exit. Ensure the accepted upper bound remains
aligned with the buildkit StatefulSet’s KEDA maxReplicaCount so discovery covers
every permitted pod ordinal.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a619722-a467-41c3-8abd-566303896be2
📒 Files selected for processing (1)
.github/scripts/route_buildkit.sh
|
nv-tusharma
approved these changes
Jul 29, 2026
dillon-cullinan
approved these changes
Jul 29, 2026
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
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.
Summary
get_active_indices()in.github/scripts/route_buildkit.shdiscovers builder pods by resolvingbuildkit-<arch>-<i>over DNS foriin[0, MAX_POD_CHECK). That bound was10, matching the KEDAmaxReplicaCountat the time it was written.Raising the builder pool above 10 without raising this leaves the extra pods undiscovered: they start, hold a Karpenter node, and never receive a build, while the pods below the bound absorb the entire queue. This bumps the default to
16and makes it environment-overridable.Prerequisite for ai-dynamo/velonix#545, which takes the pool from 9 to 15 pods per arch. Raising this first is safe on its own — ordinals with no pod behind them fail to resolve and are skipped, so on today's 9-pod fleet the behavior is unchanged apart from a few extra NXDOMAIN lookups.
Why it matters beyond the pod count
compute_group_pools()splits discovered pods into three cache groups —vllm,sglang, andgeneral+trtllm— viapool_size=$(( (count + 2) / 3 )). The split is driven bycount, so a staleMAX_POD_CHECKsilently pins the group sizes too.Running the real algorithm over contiguous fleets (it reproduces the pools seen in production logs at n=9 —
{6,7,8}amd64,{0,2,8}arm64):Note 16 is not a better 15:
pool_sizerounds up, so three pools of 6 over 16 pods forces two pods into two cache domains at once. Multiples of 3 stay disjoint while the group count is 3.Measured motivation
First-attempt build outcomes, stratified sample of 14 PR runs/day over 2026-07-24..07-29 (670 build jobs; runs cancelled by a superseding push excluded, since those cancel long builds before they can time out):
Timeouts are confined to the shared
general+trtllmgroup. Successful builds in that group have a median of 9–17 min against 30/45/60 min caps, so the failures are contention, not work that outgrew its budget.Validation
bash -n .github/scripts/route_buildkit.shpasses.MAX_POD_CHECKunset resolves to16; exported value takes precedence.compute_group_poolsverbatim and ran it over contiguous fleets; at n=9 it reproduces the exact pools observed in production build logs, confirming the harness matches the deployed algorithm.nslookupguard.Linear: https://linear.app/nvidia/issue/OPS-7960/increase-dynamo-builder-buildkit-warm-pool-from-9-to-15-pods-per-arch
🤖 Generated with Claude Code
Summary by CodeRabbit