Repository navigation
TAS: infer flavor for grouped PodSets with no managed resource requests - #12562
kubernetes-prow[bot] merged 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
kubernetes-prow
Bot
added
do-not-merge/work-in-progress
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds TAS flavor resolution for request-less pod sets, switches TAS error handling to sentinel values, and expands regression coverage for leader/worker assignment and elastic workload topology rejection. ChangesTAS flavor resolution updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
kubernetes-prow
Bot
added
the
cncf-cla: yes
kubernetes-prow
Bot
added
the
size/L
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
![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
🤖 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/scheduler/flavorassigner/tas_flavorassigner.go`:
- Around line 160-175: The TAS flavor inference in inferTASFlavorFromPodSetGroup
is order-dependent because it returns the first same-group peer with a single
TAS flavor. Update the peer scan in tas_flavorassigner.go to validate all
eligible peers in the PodSetGroup before choosing a flavor, and reject the group
when peers resolve to different TAS flavors instead of inheriting whichever peer
appears first in wl.Obj.Spec.PodSets. Use the existing podSetAssignmentByName
and onlyTASFlavor flow to detect conflicting peer flavors and surface the
ambiguity as an error.
🪄 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: Pro
Run ID: 16f66805-b5d3-4864-bbb5-19b50fe841e6
📒 Files selected for processing (2)
pkg/scheduler/flavorassigner/flavorassigner_test.gopkg/scheduler/flavorassigner/tas_flavorassigner.go
a09781c to
86e8c55
Compare
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 `@pkg/scheduler/flavorassigner/tas_flavorassigner.go`:
- Around line 176-183: Exclude Count=0 peers from TAS flavor inference in
tas_flavorassigner.go. Update the peer iteration in the TAS flavor assigner
logic so only active PodSets are considered, matching WorkloadsTopologyRequests
behavior and avoiding inference from inactive peers. Use the existing peer
assignment flow around podSetAssignmentByName, onlyTASFlavor, and the
peerPodSet/peerAssignment checks to filter out peers whose PodSet Count is zero
before attempting flavor inference or conflict detection.
🪄 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: Pro
Run ID: adedf234-ecda-45cf-add8-b46094e70dbe
📒 Files selected for processing (2)
pkg/scheduler/flavorassigner/flavorassigner_test.gopkg/scheduler/flavorassigner/tas_flavorassigner.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/flavorassigner/flavorassigner_test.go
fd3e433 to
9f8673b
Compare
j-skiba
left a comment
j-skiba
left a comment
There was a problem hiding this comment.
I think this fix exposes another issue here
kueue/pkg/resources/requests.go
Line 226 in 571fa9b
as
r will be empty then the returned result will be equal to 0.
Maybe let's add an integration test for that
|
/hold |
kubernetes-prow
Bot
added
the
do-not-merge/hold
dkaluza
left a comment
dkaluza
left a comment
There was a problem hiding this comment.
For now I have reviewed the tests, I will try to review the rest tomorrow.
dkaluza
left a comment
dkaluza
left a comment
There was a problem hiding this comment.
Went through the rest of code. I'm generally a little worried about the complexity of this code and the fact that by it we are probably only covering a single edge case - when peer podset has an assignment. As mentioned in the original issue, there might be also other edge cases that we should probably think about (e.g. what if there are no peers at all?)
In my opinion we should investigate the root cause, why none of the flavors was assigned to this podset and try to improve there. For example if this is cause because we always require a resource in this assignment maybe it would be a good idea to create an artificial resource entry in the assignment to just handle those in more systematic manner (remark I do not know if this is viable, one should investigate).
9f8673b to
02b93ee
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
081373e to
9223e5f
Compare
| // For PodSets with requests, keep only flavors for resources this PodSet requests. | ||
| var reqKeys []corev1.ResourceName | ||
| idxPodSet.podSet.Requests.ForEach(func(name corev1.ResourceName, _ int64) { | ||
| reqKeys = append(reqKeys, name) | ||
| }) | ||
| podSetFlavors := utilmaps.FilterKeys(groupFlavors, reqKeys) | ||
| if idxPodSet.podSet.Requests.Len() != 0 { | ||
| log.V(5).Info("Resolved PodSet flavors from group flavors", | ||
| "podSet", idxPodSet.podSet.Name, | ||
| "requestedResources", idxPodSet.podSet.Requests.Len(), | ||
| "resolvedFlavors", len(podSetFlavors)) | ||
| return podSetFlavors | ||
| } |
There was a problem hiding this comment.
| // For PodSets with requests, keep only flavors for resources this PodSet requests. | |
| var reqKeys []corev1.ResourceName | |
| idxPodSet.podSet.Requests.ForEach(func(name corev1.ResourceName, _ int64) { | |
| reqKeys = append(reqKeys, name) | |
| }) | |
| podSetFlavors := utilmaps.FilterKeys(groupFlavors, reqKeys) | |
| if idxPodSet.podSet.Requests.Len() != 0 { | |
| log.V(5).Info("Resolved PodSet flavors from group flavors", | |
| "podSet", idxPodSet.podSet.Name, | |
| "requestedResources", idxPodSet.podSet.Requests.Len(), | |
| "resolvedFlavors", len(podSetFlavors)) | |
| return podSetFlavors | |
| } | |
| if idxPodSet.podSet.Requests.Len() != 0 { | |
| // For PodSets with requests, keep only flavors for resources this PodSet requests. | |
| var reqKeys []corev1.ResourceName | |
| idxPodSet.podSet.Requests.ForEach(func(name corev1.ResourceName, _ int64) { | |
| reqKeys = append(reqKeys, name) | |
| }) | |
| podSetFlavors := utilmaps.FilterKeys(groupFlavors, reqKeys) | |
| log.V(5).Info("Resolved PodSet flavors from group flavors", | |
| "podSet", idxPodSet.podSet.Name, | |
| "requestedResources", idxPodSet.podSet.Requests.Len(), | |
| "resolvedFlavors", len(podSetFlavors)) | |
| return podSetFlavors | |
| } |
I might be missing something, but why not just like this, scoping reqKeys to where it is needed?
There was a problem hiding this comment.
I have followed your suggestion while slightly changing it
9223e5f to
d0c2735
Compare
dkaluza
left a comment
dkaluza
left a comment
There was a problem hiding this comment.
Please update the PR description as we no longer assign the single flavor from the CQ
dkaluza
left a comment
dkaluza
left a comment
There was a problem hiding this comment.
Code LGTM, just nitpick to fix a comment
d0c2735 to
cd4cd15
Compare
mimowo
left a comment
mimowo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
/cherrypick release-0.19
/cherrypick release-0.18
Thank you for the fix 👍
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: 8b69141861e469db3493157fbefbd252dad22576 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkaluza, mimowo, mszadkow 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 |
kubernetes-prow
Bot
added
the
approved
|
/cherrypick release-0.19 |
|
@mszadkow please prepare the CPs if the robot fails |
|
@mimowo: new pull request created: #13849 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. |
|
@mimowo: new pull request created: #13850 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. |
What type of PR is this?
/kind bug
/kind regression
/area tas
/area integrations
What this PR does / why we need it:
When a TAS-enabled Workload has a PodSet that requests none of its ClusterQueue's
managed resources, Kueue's flavor assigner previously left that PodSet's flavor
unresolved and rejected it with
no TAS flavor assigned.This PR resolves the flavor for such a PodSet by inheriting the TAS flavor already resolved for its PodSet group, instead of being filtered down to an empty flavor set.
If this can't be applied the PodSet still correctly fails, since there is no information to determine which flavor it should use.
Which issue(s) this PR fixes:
Fixes #12417
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Summary
Bug Fixes
Tests