DRA: Prioritized Alternatives in Device Requests, II#130622
DRA: Prioritized Alternatives in Device Requests, II#130622k8s-ci-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
…and used This was previously caught during Filter by the allocator check. Doing it sooner avoids wasting resources on a pod which ultimately cannot get scheduled. While at it, be a bit more clear about which feature is disabled. The user might not know that.
5b511b9 to
225d8bb
Compare
| return nil, status | ||
| } | ||
| } else { | ||
| if !pl.enablePrioritizedList { |
There was a problem hiding this comment.
Should we reject pods with FirstAvailable at PreEnqueue additionally when PrioritizedList is disabled? which would reduce unnecessary scheduling cycles consumed for those pods.
There was a problem hiding this comment.
Ah, that being said, I just remembered the discussion at #129698.
If we reject pods at PreEnqueue, no error message would be shown up for users. Hmm, so looks like this is not a great idea, at least for now, until we've got a solution for that problem.
There was a problem hiding this comment.
That would be even better. Let me check that.
There was a problem hiding this comment.
Right, there was a reason (hadn't seen your second message when writing my initial reply).
There was a problem hiding this comment.
Yeah, it would be nice to be in PreEnqueue performance-wise though. Anyways, we can revisit all those PreEnqueue stuff later.
There was a problem hiding this comment.
This situation is going to be extremely rare (enable feature, create claim, disable feature) and temporary (will disappear as the feature matures, hopefully in a few releases). It's good to catch it when it's just one if check in some existing code, but I am not sure whether it would be worth more complex changes in PreEnqueue.
I was about to write that when I saw that you had already retracted the suggestion. 😄
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, sanposhiho 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 |
|
/remove-kind api-change |
225d8bb to
ae4543f
Compare
Yes. That was a leftover from having included the full implementation earlier before rebasing. @dom4ha LGTM? |
| claim := claimPrioritizedList.DeepCopy() | ||
| claim.Namespace = namespace | ||
| claim, err = tCtx.Client().ResourceV1beta1().ResourceClaims(namespace).Create(tCtx, claim, metav1.CreateOptions{}) | ||
| if enabled { |
There was a problem hiding this comment.
Maybe reduce the indentation by flipping the logic:
| if enabled { | |
| if !enabled { | |
| require.Error(tCtx, err, "claim should have become invalid after dropping FirstAvailable") | |
| return | |
| } |
There was a problem hiding this comment.
Okay. Fixed, and also the unit test failure in the allocation that was caused by the modified error text.
This adds dedicated integration tests for the feature to the general test/integration/dra for the API and some minimal testing with the scheduler. It also adds non-performance test cases for scheduler_perf because that is a better place for running through the complete flow (for example, can reuse infrastructure for setting up nodes).
ae4543f to
89440b1
Compare
|
LGTM label has been added. DetailsGit tree hash: e0070d417ffb22b670c09ae3109164e3248c7e74 |
|
/lgtm |
|
/test pull-kubernetes-e2e-kind Pod scheduling timeout. |
|
/triage accepted |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This is a follow-up to address some review comments in #128586:
Which issue(s) this PR fixes:
Related-to: kubernetes/enhancements#4816
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE because this is not a functional changed compared to 1.32. Listing the feature once is enough.