fix: remove finalizer if workload finished #1454

woehrl01 · 2023-12-14T07:47:35Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

It removes the finalizer of workloads after it's completed, if the finalizer is not deleted by the job deletion (e.g. because of controller restart)

Which issue(s) this PR fixes:

Fixes #1450

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot · 2023-12-14T07:47:37Z

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/test-infra repository.

netlify · 2023-12-14T07:47:41Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`81b8ab1`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/657c723a5a40fe0008805ad5

k8s-ci-robot · 2023-12-14T07:47:43Z

Welcome @woehrl01!

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 · 2023-12-14T07:47:44Z

Hi @woehrl01. 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/test-infra repository.

linux-foundation-easycla · 2023-12-14T07:47:48Z

The committers listed above are authorized under a signed CLA.

✅ login: woehrl01 / name: Lukas Wöhrl (e07c19d, 1431cf5, 14b6854, 81b8ab1)

alculquicondor · 2023-12-14T16:10:50Z

cc @achernevskii
/ok-to-test

alculquicondor · 2023-12-14T16:11:50Z

Please add unit tests

alculquicondor · 2023-12-14T16:15:34Z

Actually
/hold

Can you debug why this logic didn't remove the finalizer in this case?

kueue/pkg/controller/jobframework/reconciler.go

Line 249 in 615dc8e

return ctrl.Result{}, r.removeFinalizer(ctx, wl)

UPDATE: #1450 (comment)

achernevskii · 2023-12-14T16:42:53Z

pkg/controller/core/workload_controller.go

+func (r *WorkloadReconciler) removeFinalizer(ctx context.Context, wl *kueue.Workload) error {
+	if controllerutil.RemoveFinalizer(wl, kueue.ResourceInUseFinalizerName) {
+		return r.client.Update(ctx, wl)
+	}
+	return nil
+}


There's a similar method in pkg/controller/jobframework/reconciler.go.

kueue/pkg/controller/jobframework/reconciler.go

Lines 661 to 666 in e12a871

func (r *JobReconciler) removeFinalizer(ctx context.Context, wl *kueue.Workload) error {

if controllerutil.RemoveFinalizer(wl, kueue.ResourceInUseFinalizerName) {

return r.client.Update(ctx, wl)

}

return nil

}

We could convert that method into an util function and use it in both places.

I would hold from this, as I believe that the fix should be in the reconciler only #1450 (comment)

Alright, I just pushed a change which deletes the finalizer in the case that the stopJob results in the error that the object is not found. In any other case (and failure of removing the finalizer) results in returning an error on reconcile.

Uhmmm is the not found the case that you faced in production?
If somehow removing the finalizer fails, I think the next reconcile wouldn't be able to reach to this piece of the code, would it?
Maybe a better solution is closer what you had initially. But we shouldn't try to remove finalizers from two places. So perhaps just doing it in the workload controller only is better.
Whatever you do, add a unit test.

Sorry for the back and forth, but I think I have a better understanding of the problem now

Yes, the problem I faced is that the Pod is already deleted, but the finalizer is still on the workload resource.

Alright, let me think of an alternative implementation. If meanwhile you have some ideas, please let me know.

To summarize, I think the solution should be:

Do finalizer removal from Workload controller

Do NOT do any workload finalizer removal from the job reconciler, for better separation of concerns and to avoid the controllers from stumbling into each other.

k8s-ci-robot · 2023-12-15T15:31:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: woehrl01
Once this PR has been reviewed and has the lgtm label, please assign alculquicondor 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

wip fmt

k8s-ci-robot · 2023-12-15T15:43:46Z

@woehrl01: The following tests 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-verify-main	`81b8ab1`	link	true	`/test pull-kueue-verify-main`
pull-kueue-test-unit-main	`81b8ab1`	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/test-infra repository. I understand the commands that are listed here.

woehrl01 · 2023-12-18T15:41:47Z

@alculquicondor I'm encountering issues with the final unit tests. Although the logic in the changes seems correct, the jobs need an additional reconciliation loop to be marked as completed, setting the 'JobCompleted' condition on the workload to true. Do you have suggestions for an elegant solution to implement this?

alculquicondor

In general, in unit tests you should just test the effect of one reconcile loop.

If something requires more than one reconcile loop, you can consider them 2 different test cases. And if the interaction could be complex enough, it's worth considering an integration test.

alculquicondor · 2023-12-18T16:17:54Z

test/util/mock.go

@@ -0,0 +1,80 @@
+package util


Why do we need an additional mockClient? Prefer to use the fake client provided by controller-runtime

alculquicondor · 2023-12-18T16:18:43Z

go.mod

 	github.com/prometheus/common v0.44.0 // indirect
 	github.com/prometheus/procfs v0.11.1 // indirect
 	github.com/sirupsen/logrus v1.9.3 // indirect
 	github.com/spf13/cobra v1.7.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	github.com/stoewer/go-strcase v1.3.0 // indirect
+	github.com/stretchr/objx v0.5.0 // indirect
+	github.com/stretchr/testify v1.8.4 // indirect


Remove this dependency:

https://github.com/kubernetes/community/blob/master/sig-scheduling/CONTRIBUTING.md#technical-and-style-guidelines

woehrl01 · 2023-12-21T05:06:41Z

@alculquicondor because of the upcoming season I have to abandon this PR for a few weeks. Feel free to pick it up in the meantime, I see that there are already some PR depending on this and I don't want to introduce a blocker. Thanks!

woehrl01 · 2023-12-28T02:41:54Z

Superseded by #1523

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 14, 2023

k8s-ci-robot requested review from denkensk and trasc December 14, 2023 07:47

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 Dec 14, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 14, 2023

woehrl01 force-pushed the cleanup_finalizer_workload branch from 309db57 to 816ec06 Compare December 14, 2023 07:54

woehrl01 changed the title ~~cleanup finalizer workload~~ fix: remove finalizer if workload finished Dec 14, 2023

woehrl01 marked this pull request as ready for review December 14, 2023 07:58

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2023

k8s-ci-robot requested a review from mimowo December 14, 2023 07:58

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 Dec 14, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2023

achernevskii reviewed Dec 14, 2023

View reviewed changes

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 14, 2023

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 14, 2023

woehrl01 force-pushed the cleanup_finalizer_workload branch 2 times, most recently from 4063476 to ee2a5f7 Compare December 14, 2023 21:23

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 15, 2023

woehrl01 added 4 commits December 15, 2023 16:32

fix: remove finalizer if workload finished

e07c19d

fix: execute finalizer if deleted workload

1431cf5

only ignore stopJob error if not found

14b6854

wip fmt

fix: remove finalizer from workload controller and added test

81b8ab1

woehrl01 force-pushed the cleanup_finalizer_workload branch from 88556d8 to 81b8ab1 Compare December 15, 2023 15:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023

alculquicondor mentioned this pull request Dec 15, 2023

Prebuilt workload suport #1358

Merged

alculquicondor reviewed Dec 18, 2023

View reviewed changes

achernevskii mentioned this pull request Dec 18, 2023

Add workload finished event #1483

Merged

woehrl01 marked this pull request as draft December 21, 2023 05:08

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2023

alculquicondor mentioned this pull request Dec 27, 2023

Update pod controller to reconcile not found pods #1512

Merged

woehrl01 closed this Dec 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove finalizer if workload finished #1454

fix: remove finalizer if workload finished #1454

woehrl01 commented Dec 14, 2023 •

edited

Loading

k8s-ci-robot commented Dec 14, 2023

netlify bot commented Dec 14, 2023 •

edited

Loading

k8s-ci-robot commented Dec 14, 2023

k8s-ci-robot commented Dec 14, 2023

linux-foundation-easycla bot commented Dec 14, 2023 •

edited

Loading

alculquicondor commented Dec 14, 2023

alculquicondor commented Dec 14, 2023

alculquicondor commented Dec 14, 2023 •

edited

Loading

achernevskii Dec 14, 2023

alculquicondor Dec 14, 2023

woehrl01 Dec 14, 2023

alculquicondor Dec 14, 2023

alculquicondor Dec 14, 2023

woehrl01 Dec 14, 2023

alculquicondor Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023 •

edited

Loading

woehrl01 commented Dec 18, 2023

alculquicondor left a comment

alculquicondor Dec 18, 2023

alculquicondor Dec 18, 2023

woehrl01 commented Dec 21, 2023

woehrl01 commented Dec 28, 2023

	func (r JobReconciler) removeFinalizer(ctx context.Context, wl kueue.Workload) error {
	if controllerutil.RemoveFinalizer(wl, kueue.ResourceInUseFinalizerName) {
	return r.client.Update(ctx, wl)
	}
	return nil
	}

fix: remove finalizer if workload finished #1454

fix: remove finalizer if workload finished #1454

Conversation

woehrl01 commented Dec 14, 2023 • edited Loading

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 Dec 14, 2023

netlify bot commented Dec 14, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Dec 14, 2023

k8s-ci-robot commented Dec 14, 2023

linux-foundation-easycla bot commented Dec 14, 2023 • edited Loading

alculquicondor commented Dec 14, 2023

alculquicondor commented Dec 14, 2023

alculquicondor commented Dec 14, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023 • edited Loading

woehrl01 commented Dec 18, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woehrl01 commented Dec 21, 2023

woehrl01 commented Dec 28, 2023

woehrl01 commented Dec 14, 2023 •

edited

Loading

netlify bot commented Dec 14, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 14, 2023 •

edited

Loading

alculquicondor commented Dec 14, 2023 •

edited

Loading

k8s-ci-robot commented Dec 15, 2023 •

edited

Loading