-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix fungibility: Try next flavor if can't preempt on first #1366
Fix fungibility: Try next flavor if can't preempt on first #1366
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/hold |
72e6f77
to
e7115a3
Compare
/label tide/merge-method-merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I left just a question.
if lastAssignment != nil { | ||
assignment.LastState = *lastAssignment | ||
} else { | ||
assignment.LastState = workload.AssigmentClusterQueueState{ | ||
LastState: workload.AssigmentClusterQueueState{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we set the lastAssignment
to the assignment.LastState
when the lastAssignment
isn't nil?
IIUC, we can use the lastAssignment
since the lastAssignment
is the latest, not outdated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed we are using it, but directly from the variable lastAssigment
, not assignment.LastState
.
The problem with not clearing it is that assignment.append
(line 324) keeps adding indexes to the slice of indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That makes sense.
This is just a confirmation, with this change, the scheduler will try all flavors every scheduling and the scheduler's performance will be slightly lower, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still using the values of lastAssigment
to skip flavors that were already tried. So basically the logic here was not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. It makes sense. Thanks
@@ -142,7 +142,7 @@ func (c *clusterQueueBase) requeueIfNotPresent(wInfo *workload.Info, immediate b | |||
c.rwm.Lock() | |||
defer c.rwm.Unlock() | |||
key := workload.Key(wInfo.Obj) | |||
if immediate || c.queueInadmissibleCycle >= c.popCycle { | |||
if immediate || c.queueInadmissibleCycle >= c.popCycle || wInfo.LastAssignment.PendingFlavors() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this fix is great to see. I couldn't find this hidden bug.
LGTM label has been added. Git tree hash: 748c53c3b37b67962ea8a6c173a31e0538c2f05e
|
/cherry-pick release-0.5 |
@tenzen-y: once the present PR merges, I will cherry-pick it on top of release-0.5 in a new PR and assign it to you. In 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/test-infra repository. |
/hold |
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
if lastAssignment != nil { | ||
assignment.LastState = *lastAssignment | ||
} else { | ||
assignment.LastState = workload.AssigmentClusterQueueState{ | ||
LastState: workload.AssigmentClusterQueueState{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That makes sense.
This is just a confirmation, with this change, the scheduler will try all flavors every scheduling and the scheduler's performance will be slightly lower, right?
eeecf77
to
7dbbe8d
Compare
/lgtm |
LGTM label has been added. Git tree hash: 66c521fe6e32036d6e8c549e0eb563b2e891731f
|
@tenzen-y: new pull request created: #1370 In 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/test-infra repository. |
Thanks for your help 👍 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes 3 problems:
Which issue(s) this PR fixes:
Fixes #1344
Special notes for your reviewer:
This supersedes #1356.
I want to add more tests before merging.
Does this PR introduce a user-facing change?