WaitForPodsReady: Reset the requeueState while reconciling #1838
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,7 +248,7 @@ var _ = ginkgo.Describe("SchedulerWithWaitForPodsReady", func() { | |
| // To avoid flakiness, we don't verify if the workload has a QuotaReserved=false with pending reason here. | ||
| }) | ||
|
|
||
| ginkgo.It("Should re-admit a timed out workload and deactivate a workload exceeded the re-queue count limit. After that re-activating a workload", func() { | ||
| ginkgo.By("create the 'prod' workload") | ||
| prodWl := testing.MakeWorkload("prod", ns.Name).Queue(prodQueue.Name).Request(corev1.ResourceCPU, "2").Obj() | ||
| gomega.Expect(k8sClient.Create(ctx, prodWl)).Should(gomega.Succeed()) | ||
|
|
@@ -276,6 +276,29 @@ var _ = ginkgo.Describe("SchedulerWithWaitForPodsReady", func() { | |
| g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(prodWl), prodWl)).Should(gomega.Succeed()) | ||
| g.Expect(ptr.Deref(prodWl.Spec.Active, true)).Should(gomega.BeFalse()) | ||
| }, util.Timeout, util.Interval).Should(gomega.Succeed()) | ||
|
|
||
| ginkgo.By("verify the re-activated inactive 'prod' workload re-queue state is reset") | ||
| // TODO: Once we move a logic to issue the Eviction with InactiveWorkload reason, we need to remove the below updates. | ||
| // REF: https://github.com/kubernetes-sigs/kueue/issues/1841 | ||
| gomega.Eventually(func(g gomega.Gomega) { | ||
| g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(prodWl), prodWl)).Should(gomega.Succeed()) | ||
| apimeta.SetStatusCondition(&prodWl.Status.Conditions, metav1.Condition{ | ||
| Type: kueue.WorkloadEvicted, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: kueue.WorkloadEvictedByDeactivation, | ||
| Message: "evicted by Test", | ||
| }) | ||
| g.Expect(k8sClient.Status().Update(ctx, prodWl)).Should(gomega.Succeed()) | ||
|
Comment on lines
+285
to
+291
There was a problem hiding this comment. wouldn't the controller add this already? There was a problem hiding this comment. Although the jobs controller adds this condition here: , we don't register the jobs controller to the manager.kueue/test/integration/scheduler/podsready/suite_test.go Lines 68 to 104 in 975b5f2 There was a problem hiding this comment. We just register the indexers: kueue/test/integration/scheduler/podsready/suite_test.go Lines 96 to 97 in 975b5f2 There was a problem hiding this comment. Interesting... There was a problem hiding this comment. It makes sense, but we need to remember why we put this here. There was a problem hiding this comment. Can you add a TODO to remove this status update and link to #1841? There was a problem hiding this comment. Sure. Thank you for raising it! |
||
| }, util.Timeout, util.Interval).Should(gomega.Succeed(), "Job reconciler should add an Evicted condition with InactiveWorkload to the Workload") | ||
| gomega.Eventually(func(g gomega.Gomega) { | ||
| g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(prodWl), prodWl)).Should(gomega.Succeed()) | ||
| prodWl.Spec.Active = ptr.To(true) | ||
| g.Expect(k8sClient.Update(ctx, prodWl)).Should(gomega.Succeed()) | ||
| }, util.Timeout, util.Interval).Should(gomega.Succeed(), "Reactivate inactive Workload") | ||
| gomega.Eventually(func(g gomega.Gomega) { | ||
| g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(prodWl), prodWl)).Should(gomega.Succeed()) | ||
| g.Expect(prodWl.Status.RequeueState).Should(gomega.BeNil()) | ||
|
There was a problem hiding this comment. should we verify at the start of this sequence that the Requeue state is in a certain configuration (non-nil, or something more specific), in case the previous part of test changes? Not relevant if this test is split out and this state is set explicitly. There was a problem hiding this comment. We already verify that here: Does it make sense? |
||
| }, util.Timeout, util.Interval).Should(gomega.Succeed()) | ||
| }) | ||
|
|
||
| ginkgo.It("Should unblock admission of new workloads in other ClusterQueues once the admitted workload exceeds timeout", func() { | ||
|
|
||
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.
nit: s/activating/activate
may we consider a separate test for this, assuming that setting up the state isn't too difficult?
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.
It is possible to divide this test into two tests, but we need to start another manager to configure the
backoffLimitCount=1 or 0like this:kueue/test/integration/scheduler/podsready/scheduler_test.go
Line 201 in 975b5f2
It could increase integration test time. So, to avoid increasing time, I implement this test like the current one.