WIP: reset admission check after deactivation #3350

KPostOffice · 2024-10-28T19:44:30Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #3346

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2024-10-28T19:44:32Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

k8s-ci-robot · 2024-10-28T19:44:38Z

Welcome @KPostOffice!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-28T19:44:39Z

Hi @KPostOffice. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

netlify · 2024-10-28T19:44:46Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`74a0fb3`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/672a9e6a9d67fd0008a8451c

tenzen-y · 2024-10-28T20:28:58Z

/ok-to-test

mimowo · 2024-10-29T07:31:45Z

cc @PBundyra

mimowo · 2024-10-31T10:35:29Z

@KPostOffice PTAL at the failures, maybe we could include as bugfix in 0.9.

k8s-ci-robot · 2024-11-04T19:24:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KPostOffice
Once this PR has been reviewed and has the lgtm label, please assign mimowo for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/workload/admissionchecks.go

pkg/controller/core/workload_controller.go

mimowo · 2024-11-05T06:45:59Z

cc @PBundyra for a review pass. It seems the failing integration tests are for BookingExpired. PTAL.

pkg/controller/core/workload_controller.go

PBundyra · 2024-11-05T11:09:13Z

pkg/controller/core/workload_controller_test.go

Could you add a scenario for deactivated test, to observe transition from DeactivationTarget -> EvictedByDeactivation and resetting Workload's AdmissionChecks and RequeueState

PBundyra · 2024-11-05T11:20:40Z

pkg/workload/admissionchecks.go

+		checks[i] = kueue.AdmissionCheckState{
+			Name:               checks[i].Name,
+			State:              kueue.CheckStatePending,
+			LastTransitionTime: metav1.NewTime(time.Now()),


Could we use a clock used in WorkloadController?

mimowo · 2024-11-05T21:21:34Z

pkg/controller/core/workload_controller.go

@@ -204,6 +204,7 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			wl.Status.RequeueState = nil
 			updated = true
 		}
+		updated = updated || workload.ResetChecksOnDeactivation(&wl)


swap the order: workload.ResetChecksOnDeactivation(&wl) || updated
otherwise ResetChecksOnDeactivation may not be called if updated is already true.

Ah, true. Thanks for catching 👍🏻

Signed-off-by: Kevin <[email protected]>

k8s-ci-robot · 2024-11-05T22:52:19Z

@KPostOffice: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kueue-test-unit-main	`74a0fb3`	link	true	`/test pull-kueue-test-unit-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

mbobrovskyi · 2024-11-06T06:40:33Z

pkg/controller/core/workload_controller.go

+		updated = workload.ResetChecksOnDeactivation(&wl) || updated
 		if updated {


Suggested change

updated = workload.ResetChecksOnDeactivation(&wl) || updated

if updated {

if workload.ResetChecksOnDeactivation(&wl, r.clock.Now()) || updated {

mbobrovskyi · 2024-11-06T06:50:10Z

pkg/workload/admissionchecks.go

+var (
+	realClock = clock.RealClock{}
+)
+


Suggested change

var (

realClock = clock.RealClock{}

)

I don’t think this will work well. We need to use the same clock as WorkloadReconciler, so we can replace it with FakeClock in tests.

mbobrovskyi · 2024-11-06T06:50:50Z

pkg/workload/admissionchecks.go

 			Message:            "Reset to Pending after eviction. Previously: " + string(checks[i].State),
 		}
 	}
 }

+func ResetChecksOnDeactivation(w *kueue.Workload) bool {


Suggested change

func ResetChecksOnDeactivation(w *kueue.Workload) bool {

func ResetChecksOnDeactivation(w *kueue.Workload, now time.Time) bool {

mbobrovskyi · 2024-11-06T06:51:11Z

pkg/workload/admissionchecks.go

+		checks[i] = kueue.AdmissionCheckState{
+			Name:               checks[i].Name,
+			State:              kueue.CheckStatePending,
+			LastTransitionTime: metav1.NewTime(realClock.Now()),


Suggested change

LastTransitionTime: metav1.NewTime(realClock.Now()),

LastTransitionTime: metav1.NewTime(now),

mimowo · 2024-11-06T09:11:39Z

pkg/workload/admissionchecks.go

 			Message:            "Reset to Pending after eviction. Previously: " + string(checks[i].State),
 		}
 	}
 }

+func ResetChecksOnDeactivation(w *kueue.Workload) bool {


I don't think we need a new function for this, it is virtually the same as ResetChecksOnEviction, the only differences being time as a new arg and skipping of updating for Pending.

So, I would suggest to just update the existing function and reuse in this context. Deactivation is a special case of Eviction, so I think the name is also fine.

PBundyra · 2024-11-06T09:20:40Z

test/integration/controller/admissionchecks/provisioning/provisioning_test.go

Please adjust tests description according to your change

PBundyra · 2024-11-06T09:23:47Z

test/integration/controller/admissionchecks/provisioning/provisioning_test.go

@@ -443,7 +435,7 @@ var _ = ginkgo.Describe("Provisioning", ginkgo.Ordered, ginkgo.ContinueOnFailure
 				gomega.Eventually(func(g gomega.Gomega) {
 					g.Expect(k8sClient.Get(ctx, wlKey, &updatedWl)).To(gomega.Succeed())
 					g.Expect(workload.IsEvictedByDeactivation(&updatedWl)).To(gomega.BeTrue())
-					util.ExpectEvictedWorkloadsTotalMetric(cq.Name, kueue.WorkloadEvictedByDeactivation, 1)
+					util.ExpectEvictedWorkloadsTotalMetric(cq.Name, kueue.WorkloadEvictedByDeactivation+kueue.WorkloadEvictedByAdmissionCheck, 1)


Is the metric kueue.WorkloadEvictedByAdmissionCheck increased in this scenario?

mimowo · 2024-11-06T09:26:33Z

cool to see all integration and e2e tests passing already 👍

k8s-ci-robot requested review from mbobrovskyi and mimowo October 28, 2024 19:44

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 28, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 28, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 28, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2024

KPostOffice force-pushed the resetACOnWLDeactivation branch 2 times, most recently from 4db1187 to 6f7c974 Compare October 28, 2024 23:31

KPostOffice force-pushed the resetACOnWLDeactivation branch from 6f7c974 to f279f18 Compare November 4, 2024 19:24

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 4, 2024

KPostOffice force-pushed the resetACOnWLDeactivation branch 2 times, most recently from dc8bc5a to a559886 Compare November 5, 2024 00:06

mimowo reviewed Nov 5, 2024

View reviewed changes

pkg/workload/admissionchecks.go Show resolved Hide resolved

mimowo reviewed Nov 5, 2024

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

mimowo mentioned this pull request Nov 5, 2024

Use DeactivationTarget to improve message when deactivating due to Rejected AdmissionChecks #3331

Open

PBundyra reviewed Nov 5, 2024

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

PBundyra reviewed Nov 5, 2024

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2024

KPostOffice force-pushed the resetACOnWLDeactivation branch from a9d5dd0 to 37a4619 Compare November 5, 2024 21:20

mimowo reviewed Nov 5, 2024

View reviewed changes

KPostOffice added 5 commits November 5, 2024 16:44

reset admission check after deactivation

6faa53b

Signed-off-by: Kevin <[email protected]>

adjust unit tests

adef1be

Signed-off-by: Kevin <[email protected]>

use deactivation as prefix

4fe28ad

Signed-off-by: Kevin <[email protected]>

use realClock in admissionCheck package

efd114c

Signed-off-by: Kevin <[email protected]>

remove rejected check because it's transitory

ad6f582

Signed-off-by: Kevin <[email protected]>

KPostOffice force-pushed the resetACOnWLDeactivation branch from 37a4619 to ad6f582 Compare November 5, 2024 22:14

swap order to avoid short circuit

74a0fb3

Signed-off-by: Kevin <[email protected]>

mbobrovskyi reviewed Nov 6, 2024

View reviewed changes

mimowo reviewed Nov 6, 2024

View reviewed changes

PBundyra reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: reset admission check after deactivation #3350

WIP: reset admission check after deactivation #3350

KPostOffice commented Oct 28, 2024

k8s-ci-robot commented Oct 28, 2024

k8s-ci-robot commented Oct 28, 2024

k8s-ci-robot commented Oct 28, 2024

netlify bot commented Oct 28, 2024 •

edited

Loading

tenzen-y commented Oct 28, 2024

mimowo commented Oct 29, 2024

mimowo commented Oct 31, 2024

k8s-ci-robot commented Nov 4, 2024

mimowo commented Nov 5, 2024

PBundyra Nov 5, 2024

PBundyra Nov 5, 2024

mimowo Nov 5, 2024

KPostOffice Nov 5, 2024

k8s-ci-robot commented Nov 5, 2024

mbobrovskyi Nov 6, 2024

mbobrovskyi Nov 6, 2024

mbobrovskyi Nov 6, 2024

mbobrovskyi Nov 6, 2024

mimowo Nov 6, 2024

PBundyra Nov 6, 2024 •

edited

Loading

PBundyra Nov 6, 2024

mimowo commented Nov 6, 2024

		updated = workload.ResetChecksOnDeactivation(&wl) \|\| updated
		if updated {

	updated = workload.ResetChecksOnDeactivation(&wl) \|\| updated
	if updated {
	if workload.ResetChecksOnDeactivation(&wl, r.clock.Now()) \|\| updated {

	func ResetChecksOnDeactivation(w *kueue.Workload) bool {
	func ResetChecksOnDeactivation(w *kueue.Workload, now time.Time) bool {

	LastTransitionTime: metav1.NewTime(realClock.Now()),
	LastTransitionTime: metav1.NewTime(now),

WIP: reset admission check after deactivation #3350

Are you sure you want to change the base?

WIP: reset admission check after deactivation #3350

Conversation

KPostOffice commented Oct 28, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 28, 2024

k8s-ci-robot commented Oct 28, 2024

k8s-ci-robot commented Oct 28, 2024

netlify bot commented Oct 28, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

tenzen-y commented Oct 28, 2024

mimowo commented Oct 29, 2024

mimowo commented Oct 31, 2024

k8s-ci-robot commented Nov 4, 2024

mimowo commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PBundyra Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo commented Nov 6, 2024

netlify bot commented Oct 28, 2024 •

edited

Loading

PBundyra Nov 6, 2024 •

edited

Loading