[slice] Remove Slice finalizer, once all Pods in the Workload have gracefully terminated. #517

mbobrovskyi · 2025-07-02T16:34:31Z

Fixes / Features

Remove Slice finalizer, once all Pods in the Workload have gracefully terminated.

mbobrovskyi · 2025-07-02T16:37:09Z

cc: @IrvingMg @PBundyra

IrvingMg

Overall LGTM, just a couple of nits.

slice/internal/controller/workload_controller.go

IrvingMg · 2025-07-04T10:59:09Z

LGTM

cc: @PBundyra

slice/internal/controller/indexer.go

slice/internal/util/slices/slices.go

slice/internal/controller/workload_controller.go

mbobrovskyi · 2025-07-16T08:56:57Z

@gabesaba, I've addressed all your comments. Could you please take a look?

slice/internal/controller/workload_controller_test.go

pajakd · 2025-07-29T12:59:24Z

Overall LGTM, left a few more small comments.

slice/internal/controller/indexer.go

slice/internal/controller/workload_controller.go

slice/test/utils/utils.go

slice/test/e2e/jobset_test.go

slice/internal/controller/workload_controller.go

slice/internal/controller/workload_controller_test.go

slice/test/e2e/jobset_test.go

slice/internal/controller/workload_controller.go

gabesaba · 2025-07-31T09:25:30Z

slice/internal/controller/workload_controller.go

+	finalize, _ := shouldFinalize(wl)
+	return !finalize && validateRelevantWorkload(wl) == nil


What is the intention of this logic? Could you add a comment please?

But maybe the Predicate is no longer needed — we check this in the Reconcile() function. WDYT?

What if we just simplify the filter, and return true always - for each event type (besides generic)? It looks like the handler itself will handle any workload just fine. This will make it easier to debug too.

But it’s the same whether we use a predicate or just return true, right?

I’ve reverted the changes related to the Predicate. But if you think we still need to return true, I can revert it again.

slice/test/e2e/jobset_test.go

… terminated.

gabesaba

lgtm, this looks great. Thank you for all the revisions :)

mbobrovskyi requested a review from IrvingMg July 2, 2025 16:34

mbobrovskyi requested review from mwysokin, mimowo, gabesaba, PBundyra, mwielgus and pajakd as code owners July 2, 2025 16:34

mbobrovskyi force-pushed the slice/finalize branch from de1e59a to 0a23e95 Compare July 2, 2025 16:43

mbobrovskyi changed the title ~~Remove Slice finalizer, once all Pods in the Workload have gracefully terminated.~~ [slice] Remove Slice finalizer, once all Pods in the Workload have gracefully terminated. Jul 3, 2025

mbobrovskyi force-pushed the slice/finalize branch 3 times, most recently from 992a005 to 04c024c Compare July 3, 2025 08:57

IrvingMg reviewed Jul 3, 2025

View reviewed changes

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

mbobrovskyi force-pushed the slice/finalize branch from fe2e251 to c4469a5 Compare July 3, 2025 16:03

mbobrovskyi requested a review from IrvingMg July 3, 2025 16:07

mbobrovskyi force-pushed the slice/finalize branch 3 times, most recently from e215bda to bedab10 Compare July 4, 2025 10:44

gabesaba reviewed Jul 14, 2025

View reviewed changes

mbobrovskyi force-pushed the slice/finalize branch 2 times, most recently from e038b46 to d3c3bd0 Compare July 15, 2025 08:50

mbobrovskyi requested a review from gabesaba July 15, 2025 08:51

mbobrovskyi force-pushed the slice/finalize branch 2 times, most recently from f42798a to f8fd426 Compare July 16, 2025 08:55

gabesaba mentioned this pull request Jul 16, 2025

[slice] Create Slice for each PodSet in Workload. #520

Merged

gabesaba mentioned this pull request Jul 28, 2025

[slice] Update AdmissionCheck depending on success/failure. #521

Merged

mbobrovskyi force-pushed the slice/finalize branch from f8fd426 to 511ad08 Compare July 28, 2025 16:36

pajakd reviewed Jul 29, 2025

View reviewed changes

slice/internal/controller/workload_controller_test.go Outdated Show resolved Hide resolved

gabesaba reviewed Jul 29, 2025

View reviewed changes

slice/internal/controller/indexer.go Outdated Show resolved Hide resolved

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

gabesaba reviewed Jul 29, 2025

View reviewed changes

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

slice/internal/controller/workload_controller.go Show resolved Hide resolved

gabesaba reviewed Jul 29, 2025

View reviewed changes

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

mbobrovskyi force-pushed the slice/finalize branch 2 times, most recently from 4c60461 to 6535835 Compare July 30, 2025 08:47

mbobrovskyi requested review from pajakd and gabesaba July 30, 2025 08:47

mbobrovskyi force-pushed the slice/finalize branch from 6535835 to d8d4db5 Compare July 30, 2025 09:01

gabesaba reviewed Jul 30, 2025

View reviewed changes

mbobrovskyi force-pushed the slice/finalize branch from d8d4db5 to f2765c8 Compare July 30, 2025 11:13

mbobrovskyi requested a review from gabesaba July 30, 2025 11:15

gabesaba reviewed Jul 30, 2025

View reviewed changes

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

mbobrovskyi force-pushed the slice/finalize branch 2 times, most recently from 556b5b2 to 81e8c64 Compare July 30, 2025 15:20

mbobrovskyi requested a review from gabesaba July 30, 2025 15:21

gabesaba reviewed Jul 31, 2025

View reviewed changes

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

mbobrovskyi force-pushed the slice/finalize branch 2 times, most recently from 4377f2c to 24c88ba Compare July 31, 2025 07:20

gabesaba reviewed Jul 31, 2025

View reviewed changes

mbobrovskyi force-pushed the slice/finalize branch from 24c88ba to 4cc0760 Compare July 31, 2025 09:53

Remove Slice finalizer, once all Pods in the Workload have gracefully…

0f97a57

… terminated.

mbobrovskyi force-pushed the slice/finalize branch from 4cc0760 to 0f97a57 Compare July 31, 2025 11:16

mbobrovskyi added 2 commits July 31, 2025 14:23

Revert Predicate implementations.

48c85aa

Fix lint.

664ce92

gabesaba reviewed Jul 31, 2025

View reviewed changes

gabesaba merged commit 5ff57e1 into slice-main Jul 31, 2025
16 of 17 checks passed

gabesaba deleted the slice/finalize branch July 31, 2025 12:19

		finalize, _ := shouldFinalize(wl)
		return !finalize && validateRelevantWorkload(wl) == nil

[slice] Remove Slice finalizer, once all Pods in the Workload have gracefully terminated. #517

[slice] Remove Slice finalizer, once all Pods in the Workload have gracefully terminated. #517

Uh oh!

Conversation

mbobrovskyi commented Jul 2, 2025

Fixes / Features

Uh oh!

mbobrovskyi commented Jul 2, 2025

Uh oh!

IrvingMg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IrvingMg commented Jul 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbobrovskyi commented Jul 16, 2025

Uh oh!

Uh oh!

pajakd commented Jul 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabesaba Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabesaba Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabesaba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mbobrovskyi Jul 31, 2025 •

edited

Loading

gabesaba Jul 31, 2025 •

edited

Loading

mbobrovskyi Jul 31, 2025 •

edited

Loading