WaitForPodsReady: Reset .status.requeueState.count once the workload is deactivated #2219
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -163,13 +163,6 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c | |||||
|
|
||||||
| if ptr.Deref(wl.Spec.Active, true) { | ||||||
| var updated bool | ||||||
| if cond := apimeta.FindStatusCondition(wl.Status.Conditions, kueue.WorkloadRequeued); cond != nil && cond.Status == metav1.ConditionFalse { | ||||||
| switch cond.Reason { | ||||||
| case kueue.WorkloadEvictedByDeactivation: | ||||||
|
|
@@ -194,16 +187,39 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c | |||||
| if updated { | ||||||
| return ctrl.Result{}, workload.ApplyAdmissionStatus(ctx, r.client, &wl, true) | ||||||
| } | ||||||
| } else { | ||||||
| var updated, evicted bool | ||||||
| if !apimeta.IsStatusConditionTrue(wl.Status.Conditions, kueue.WorkloadEvicted) { | ||||||
| message := "The workload is deactivated" | ||||||
|
|
||||||
| // The deactivation reason could be deduced as the maximum number of re-queuing retries if the workload met all criteria below: | ||||||
| // 1. The waitForPodsReady feature is enabled, which means that it has "PodsReady" condition. | ||||||
| // 2. The workload has already exceeded the PodsReadyTimeout. | ||||||
| // 3. The workload already has been re-queued previously, which means it doesn't have the requeueAt field. | ||||||
| // 4. The number of re-queued has already reached the waitForPodsReady.requeuingBackoffLimitCount. | ||||||
| if apimeta.IsStatusConditionFalse(wl.Status.Conditions, kueue.WorkloadPodsReady) && | ||||||
| ((!workload.HasRequeueState(&wl) && ptr.Equal(r.waitForPodsReady.requeuingBackoffLimitCount, ptr.To[int32](0))) || | ||||||
| (workload.HasRequeueState(&wl) && wl.Status.RequeueState.RequeueAt == nil && | ||||||
| ptr.Equal(wl.Status.RequeueState.Count, r.waitForPodsReady.requeuingBackoffLimitCount))) { | ||||||
| message = fmt.Sprintf("%s by exceeded the maximum number of re-queuing retries", message) | ||||||
|
There was a problem hiding this comment. @mimowo @alculquicondor I added the dedicated message like this based on your recommendation. HDYT? There was a problem hiding this comment. I think the dedicated message is useful There was a problem hiding this comment.
Suggested change
nit, I think it will read better There was a problem hiding this comment. SGTM, thank you for this recommendation! |
||||||
| } | ||||||
| workload.SetEvictedCondition(&wl, kueue.WorkloadEvictedByDeactivation, message) | ||||||
| updated = true | ||||||
| evicted = true | ||||||
| } | ||||||
| if wl.Status.RequeueState != nil { | ||||||
| wl.Status.RequeueState = nil | ||||||
| updated = true | ||||||
| } | ||||||
| if updated { | ||||||
| if err := workload.ApplyAdmissionStatus(ctx, r.client, &wl, true); err != nil { | ||||||
| return ctrl.Result{}, fmt.Errorf("setting eviction: %w", err) | ||||||
| } | ||||||
| if evicted && wl.Status.Admission != nil { | ||||||
| metrics.ReportEvictedWorkloads(string(wl.Status.Admission.ClusterQueue), kueue.WorkloadEvictedByDeactivation) | ||||||
| } | ||||||
| return ctrl.Result{}, nil | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| cqName, cqOk := r.queues.ClusterQueueForWorkload(&wl) | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1795,9 +1795,7 @@ var _ = ginkgo.Describe("Pod controller interacting with Workload controller whe | |
| gomega.Eventually(func(g gomega.Gomega) { | ||
| g.Expect(k8sClient.Get(ctx, wlKey, wl)).Should(gomega.Succeed()) | ||
| g.Expect(ptr.Deref(wl.Spec.Active, true)).Should(gomega.BeFalse()) | ||
| g.Expect(wl.Status.RequeueState).Should(gomega.BeNil()) | ||
| g.Expect(wl.Status.Conditions).To(gomega.ContainElements( | ||
| gomega.BeComparableTo(metav1.Condition{ | ||
| Type: kueue.WorkloadPodsReady, | ||
|
|
@@ -1809,13 +1807,13 @@ var _ = ginkgo.Describe("Pod controller interacting with Workload controller whe | |
| Type: kueue.WorkloadQuotaReserved, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "Pending", | ||
| Message: "The workload is deactivated by exceeded the maximum number of re-queuing retries", | ||
| }, util.IgnoreConditionTimestampsAndObservedGeneration), | ||
| gomega.BeComparableTo(metav1.Condition{ | ||
| Type: kueue.WorkloadEvicted, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: kueue.WorkloadEvictedByDeactivation, | ||
| Message: "The workload is deactivated by exceeded the maximum number of re-queuing retries", | ||
| }, util.IgnoreConditionTimestampsAndObservedGeneration), | ||
| gomega.BeComparableTo(metav1.Condition{ | ||
| Type: kueue.WorkloadAdmitted, | ||
|
|
@@ -1827,7 +1825,7 @@ var _ = ginkgo.Describe("Pod controller interacting with Workload controller whe | |
| Type: podcontroller.WorkloadWaitingForReplacementPods, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: kueue.WorkloadEvictedByDeactivation, | ||
| Message: "The workload is deactivated by exceeded the maximum number of re-queuing retries", | ||
| }, util.IgnoreConditionTimestampsAndObservedGeneration), | ||
| )) | ||
| }, util.Timeout, util.Interval).Should(gomega.Succeed()) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Maybe it is better to declare
evictedas*stringas it is used to bump the metric.I worry that if we have a
bool, then someone can forget updating line 219 when adding a new scenario for deactivation. WDYT?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.
@mimowo Uhm, do you assume that the
evictedhas the reason for eviction? (here isInactiveWorkload)?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, never mind, I thought for another scenario that Pods ready, but in that case it would still be WorkloadInactive
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.
Yeah, after we introduce a dedicated reason, we may refactor here somehow, though.