-
Notifications
You must be signed in to change notification settings - Fork 243
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
Avoid requeue on ClusterQueue status update #1822
Avoid requeue on ClusterQueue status update #1822
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
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 have only reviewed some of the codes yet since CI failed.
404a3cc
to
de415e6
Compare
Change-Id: Id028414a730a238549b7fddf5d42a2eafdf959c3
de415e6
to
3e88ef3
Compare
Change-Id: Id52ef3c92abd84317c4a7a71c3a20f6f28d67d25
a042df7
to
1e66b43
Compare
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.
Generally lgtm
@@ -119,6 +119,7 @@ func (c *clusterQueueBase) PushOrUpdate(wInfo *workload.Info) { | |||
// to potentially become admissible, unless the Eviction status changed | |||
// which can affect the workloads order in the queue. | |||
if equality.Semantic.DeepEqual(oldInfo.Obj.Spec, wInfo.Obj.Spec) && | |||
equality.Semantic.DeepEqual(oldInfo.Obj.Status.ReclaimablePods, wInfo.Obj.Status.ReclaimablePods) && |
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.
Should we extend this UT:
kueue/pkg/queue/cluster_queue_impl_test.go
Line 228 in 1e66b43
func TestClusterQueueImpl(t *testing.T) { |
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.
added
/test pull-kueue-test-integration-main |
a43db01
to
1b243b3
Compare
Change-Id: I5431e741cb30541a846b65fb26aa0b7b631eb80d
1b243b3
to
40485a2
Compare
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.
Ideally, we should extend UTs, but integraiton tests should be enough now.
Thank you! |
LGTM label has been added. Git tree hash: c9cfce3c8804e92b9bac9ceabe7a048dd62131b0
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, tenzen-y 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 |
/cherry-pick release-0.6 |
@alculquicondor: new pull request created: #1832 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. |
* Avoid requeue on ClusterQueue status update Change-Id: Id028414a730a238549b7fddf5d42a2eafdf959c3 * Reclaimable pods can put pod out of inadmissible Change-Id: Id52ef3c92abd84317c4a7a71c3a20f6f28d67d25 * Delay requeueing based on RequeueState Change-Id: I5431e741cb30541a846b65fb26aa0b7b631eb80d
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Kueue updates ClusterQueues to include pending or admitted workloads, among other things. These changes shouldn't trigger requeueing.
This not only saves CPU cycles, but API calls to the apiserver for Events.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?