Correct Flavor Assignment in Pod Importer - #12924
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:
📝 WalkthroughWalkthroughThe pod importer now resolves and validates ChangesPod flavor assignment
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant PodImporter
participant ImportCache
participant Workload
participant Admission
PodImporter->>ImportCache: resolve queue and validate flavors
ImportCache-->>PodImporter: return flavor assignments
PodImporter->>Workload: construct validated workload
Workload-->>PodImporter: return checked workload
PodImporter->>Admission: submit workload with assignments
Possibly related PRs
Suggested labels: 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
becaf7f to
93e6229
Compare
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
|
/test pull-kueue-test-e2e-baseline-main-1-36 |
|
/cc @ivnovakov @vladikkuzn ptal |
|
@mszadkow: GitHub didn't allow me to request PR reviews from the following users: ptal. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Read through this. The core fix is right: each requested resource is mapped to the flavor of the resource group that actually covers it, instead of assigning ResourceGroups[0].Flavors[0] to every resource. The two new table cases pin the exact per-resource flavors (cpu-flavor for cpu, gpu-flavor for gpu) and the uncovered-resource error, so they fail against the old code. That is the right discriminator.
Two questions before I approve, neither blocking:
- Within a covered group you always take Flavors[0]. For a group with several flavors, say on-demand and spot, a pod actually running on spot gets imported as on-demand. Is first-flavor a deliberate simplification, or should the importer match the flavor to the pod's node labels? This is pre-existing behavior, I just want the choice recorded.
- Re-import: if a pod was already imported and its resource is no longer covered by the target CQ, admitWorkload now returns ResourceNotCoveredError and the earlier admission is left in place. That is fine for a one-shot run, but worth confirming it is the intended behavior.
Scope and tests look good otherwise. Nits inline.
Following the repo AI policy: I used an AI assistant while preparing this review and verified the findings myself.
93e6229 to
530666d
Compare
|
@ArangoGutierrez ptal |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/importer/pod/import_test.go (1)
260-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider migrating this new test to Ginkgo/Gomega.
While the use of table-driven testing perfectly aligns with the project preferences, this test relies on standard Go
testing(t.Run,t.Fatalf). As per path instructions, ensure tests follow the existing Ginkgo/Gomega patterns in the project (e.g. usingDescribeTableandEntry), unless this specific package is intentionally exempted from the project-wide Ginkgo usage.🤖 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/importer/pod/import_test.go` around lines 260 - 316, Migrate TestResourceFlavorForResource to the package’s existing Ginkgo/Gomega style, replacing t.Run and t.Fatalf with a DescribeTable containing Entry cases and an assertion for the expected flavor. Preserve all current test scenarios and inputs, unless this package is explicitly exempt from the project-wide Ginkgo usage.Source: Path instructions
🤖 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.
Nitpick comments:
In `@cmd/importer/pod/import_test.go`:
- Around line 260-316: Migrate TestResourceFlavorForResource to the package’s
existing Ginkgo/Gomega style, replacing t.Run and t.Fatalf with a DescribeTable
containing Entry cases and an assertion for the expected flavor. Preserve all
current test scenarios and inputs, unless this package is explicitly exempt from
the project-wide Ginkgo usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a78c8ad9-75d4-4d8f-b603-3557be77e362
📒 Files selected for processing (2)
cmd/importer/pod/import.gocmd/importer/pod/import_test.go
|
The plan is the follow up with exact flavor match. |
Nothing from my side |
kubernetes-prow
Bot
added
the
needs-rebase
530666d to
877d050
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Second pass, now on 877d050.
The first-flavor question from the last round is resolved well: the behavior is documented on resourceFlavorForResource and pinned by a unit test, so the decision lives in the code rather than in a review thread. The new ErrCQNotFound guard and the nil-labels guard are both good additions. ResourceNotCoveredError is still exported.
One new issue is worth looking at before merge. Turning the uncovered-resource case into a hard failure is the right call, but the failure is not mirrored into the dry-run check, so --dry-run passes for exactly the pods the real import will reject, and it rejects them after it has already labeled the running pod. Details inline.
Following the repo AI policy: I used an AI assistant while preparing this review and verified the findings myself against the tree.
|
@mszadkow ptal |
|
/lgtm cancel |
kubernetes-prow
Bot
removed
the
lgtm
9b692e0 to
08fe5cd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
kubernetes-prow
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/importer/pod/import.go (1)
112-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn
ctx.Err()instead of a new error.
waitForRetryreports every context termination as"context canceled". A deadline expiry is then indistinguishable from a cancellation, and callers cannot useerrors.Is(err, context.Canceled).♻️ Proposed refactor
select { case <-ctx.Done(): - return errors.New("context canceled") + return ctx.Err() case <-t.C: return nil }AI-assisted review. As per coding guidelines, "Disclose AI usage in pull request descriptions, comments, and issue filings."
🤖 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/importer/pod/import.go` around lines 112 - 126, Update waitForRetry to return ctx.Err() directly when ctx.Done() is selected instead of constructing a new error, preserving the existing timeout and negative-timeout behavior.cmd/importer/cache/cache_test.go (1)
30-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
ValidateClusterQueueFlavors.The new cache exposes two memoized APIs. This file tests only
FlavorsByResource.ValidateClusterQueueFlavorscarries behavior that is easy to regress: it memoizesnilresults, it memoizes errors, and it sorts flavor references so the error message is deterministic when several flavors are missing. None of that is pinned here.check_test.goreachesErrCQInvalidonly throughCheck, so it cannot detect a change in which flavor name the message reports.Add a table-driven case set with: all flavors known, one missing flavor, two missing flavors asserting the lexicographically first name in the message, and a repeat call after mutating
cq.Spec.ResourceGroupsto confirm memoization.Prefer table-driven tests for multiple scenarios, as per path instructions.
AI-assisted review. As per coding guidelines, "Disclose AI usage in pull request descriptions, comments, and issue filings."
🤖 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/importer/cache/cache_test.go` around lines 30 - 99, Add a table-driven TestValidateClusterQueueFlavors covering all-known flavors, one missing flavor, and two missing flavors; assert successful validation, the expected error, and the lexicographically first missing flavor in the multi-missing message. In each case, call ValidateClusterQueueFlavors again after clearing clusterQueue.Spec.ResourceGroups to verify both nil results and errors are memoized.Source: Path instructions
🤖 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/importer/pod/import.go`:
- Around line 229-238: Update the admission patch retry loop around
PatchAdmissionStatus to honor checkError’s reload result by re-reading the
current Workload and refreshing wl before retrying, then reapply the admission
update. Add bounded retry attempts with backoff for zero-timeout conflicts,
returning the final error when the limit is reached; follow the existing
addLabels retry pattern and preserve cancellation handling in waitForRetry.
---
Nitpick comments:
In `@cmd/importer/cache/cache_test.go`:
- Around line 30-99: Add a table-driven TestValidateClusterQueueFlavors covering
all-known flavors, one missing flavor, and two missing flavors; assert
successful validation, the expected error, and the lexicographically first
missing flavor in the multi-missing message. In each case, call
ValidateClusterQueueFlavors again after clearing
clusterQueue.Spec.ResourceGroups to verify both nil results and errors are
memoized.
In `@cmd/importer/pod/import.go`:
- Around line 112-126: Update waitForRetry to return ctx.Err() directly when
ctx.Done() is selected instead of constructing a new error, preserving the
existing timeout and negative-timeout behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ac4d82f-fad2-4a3c-9921-b9f40ba72f09
📒 Files selected for processing (8)
cmd/importer/README.mdcmd/importer/cache/cache.gocmd/importer/cache/cache_test.gocmd/importer/pod/check.gocmd/importer/pod/check_test.gocmd/importer/pod/errors.gocmd/importer/pod/import.gocmd/importer/pod/import_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/importer/README.md
08fe5cd to
6cc6e53
Compare
|
@dkaluza all addressed, I have changed the approach and now all that can be is calculated at |
dkaluza
left a comment
dkaluza
left a comment
There was a problem hiding this comment.
Sorry didn't manage to go fully through the implementation today, will try to follow up tomorrow with the remaining files.
|
/approve |
|
@mimowo: once the present PR merges, I will cherry-pick it on top of 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
6cc6e53 to
60395ca
Compare
60395ca to
b8a4956
Compare
in order to match with configurable Kueue behavior
|
@dkaluza ptal :) |
| if len(info.TotalRequests) == 0 { | ||
| return nil, fmt.Errorf("workload has no total requests: %w", cache.ErrPodInvalid) | ||
| } | ||
| flavors, err := flavorAssignmentsForRequests(importCache.FlavorsByResourceForClusterQueue(kueue.ClusterQueueReference(cq.Name)), cq.Name, info.TotalRequests[0].Requests) |
There was a problem hiding this comment.
I'm assuming total requests from the workload info does not include excluded resources, is this right?
There was a problem hiding this comment.
yes, the "filtering" happens inside workload.NewInfo()
| wantError: &resourceNotCoveredError{Resource: corev1.ResourceName("nvidia.com/gpu"), ClusterQueue: "cq1"}, | ||
| }, | ||
| "excluded resource request is ignored": { | ||
| pods: []corev1.Pod{*basePodWrapper.Clone().Request(corev1.ResourceName("vendor.com/special"), "1").Obj()}, |
There was a problem hiding this comment.
how does this work?
Shouldn't this lead to ErrPodInvalid since there are no requests then?
Or there is an entry with empty requests and this is why this passes but flavors will be empty?
Is workload with empty flavors permitted? I'm guessing it is not. Is the current behavior intended?
Btw. can we have a test with ErrPodInvalid - I couldn't find it right now...
There was a problem hiding this comment.
Shouldn't this lead to ErrPodInvalid since there are no requests then?
No. ErrPodInvalid is only returned when len(info.TotalRequests) == 0, i.e. when the workload has zero PodSets. It is not returned when a PodSet exists but its Requests map is empty.
Or there is an entry with empty requests and this is why this passes but flavors will be empty?
Yes — this is exactly what happens. The pod becomes one PodSet, so TotalRequests has length 1 (len(info.TotalRequests) == 1, not 0). That one entry's Requests map is empty because the only requested resource (vendor.com/special) was dropped by the exclusion filter. So the ErrPodInvalid check is skipped, flavorAssignmentsForRequests iterates over zero requests, and it returns an empty flavors map with no error.
Is workload with empty flavors permitted?
It's a hard question, it's not directly forbidden - a Workload can be admitted with an empty Flavors map for a PodSet, but it varies with outcome.
Btw. can we have a test with ErrPodInvalid - I couldn't find it right now...
Yes, but it needs a case where wl.Spec.PodSets is empty, which a single plain pod cannot produce (a pod always yields exactly one PodSet, regardless of what it requests). To hit ErrPodInvalid you'd need to go through ConstructComposableWorkload with an input that results in zero constructed PodSets
However, it seems to be a dead code condition anyways.
I will drop it
AI suggests:
Importer is a pod-by-pod tool, entirely blind to Kueue's pod-group feature.
Each Pod is treated in isolation via pod.FromObject (which never sets isGroup), which is precisely why ConstructComposableWorkload always takes the single-pod branch and always yields exactly one PodSet.
The ErrPodInvalid (zero-PodSets) guard in check.go:138-140 isn't guarding against some importer-specific edge case — it's dead code for a more general path that the importer's current single-pod design never exercises.
| // waitForRetry blocks for timeout, or returns early with an error if ctx is | ||
| // done first. A negative timeout returns immediately. | ||
| func waitForRetry(ctx context.Context, timeout time.Duration) error { | ||
| if timeout < 0 { |
There was a problem hiding this comment.
| if timeout < 0 { | |
| if timeout <= 0 { |
Maybe it is worth it to return immediately for non-positive timeout?
IIUC 0 still means retry immediately
| } | ||
|
|
||
| // waitForRetry blocks for timeout, or returns early with an error if ctx is | ||
| // done first. A negative timeout returns immediately. |
There was a problem hiding this comment.
| // done first. A negative timeout returns immediately. | |
| // done first. A non-positive timeout returns immediately. |
| for range maxAttempts { | ||
| err := workloadpatching.PatchAdmissionStatus(ctx, c, wl, realClock, update, workloadpatching.WithForceApply()) | ||
| retry, _, timeout := checkError(err) | ||
| retry, reload, timeout := checkError(err) |
There was a problem hiding this comment.
What happens if PatchAdmissionStatus returned nil? Is it handled gracefully by checkError?
This is not trivial right now for me that this code returns nil if there was no error in status patching at all.
Can we add an explicit return nil when error is nil?
There was a problem hiding this comment.
right, I will add it
| } | ||
| } | ||
|
|
||
| func TestFlavorAssignmentsForRequests(t *testing.T) { |
There was a problem hiding this comment.
Why this test is in import_test when tested function is not in import.go?
There was a problem hiding this comment.
good point, thanks
|
@mszadkow reviewed, generally looks good, but I'm still concerned about this ErrPodInvalid, see #12924 (comment) |
* remove dead condition - due to nature of importer * minor fixes
8 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR improves behavior of Flavor Assignment in Pod Importer and test coverage for additional resources.
It introduces structured error handling for uncovered resources, keeps resource-to-flavor selection deterministic, and consolidates importer tests into a cleaner table-driven structure.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Bug Fixes
Documentation
Tests