[slice] Cluster scoped

[pajakd]

Description

The Slice CR is changed from namespaced to cluster-scoped. This PR is to align with this change.

Issue

Testing

[mbobrovskyi]

What if we have workloads with the same names and PodSet names in different namespaces? Should we include the namespace in the name?

[mbobrovskyi]

Please also add test case for this scenario.

[pajakd]

I added a test "should handle two JobSets with the same name in different namespaces", which passes without any additional changes. The thing is that each workload gets a different name (bc it is build using has of UID of the JobSet https://github.com/kubernetes-sigs/kueue/blob/36e330ef5aa07ec20157cd523805cd173dac488b/pkg/controller/jobframework/workload_names.go#L52).

Do you think its possible to construct a test in which two workloads have the same names?

[mbobrovskyi]

Yes, what if the user creates a prebuilt workload?

[pajakd]

Oh, I see. I think you are right. So I prepended the namespace to the slice name for fix the potential name collision. Added a unit test "should create a slice for another workload with the same name but in a different namespace" that should cover this scenario. Please take a look.

[mbobrovskyi]

Previously, we didn’t handle removing slices when a workload was deleted due to ownerReference. I think we should add this logic to the reconciler to handle workload deletion properly.

[pajakd]

Previously, we didn’t handle removing slices when a workload was deleted due to ownerReference. I think we should add this logic to the reconciler to handle workload deletion properly.

If workload is deleted then shouldFinalize(wl) is true and this logic removes the slices:

xpk/slice/internal/controller/workload_controller.go

Lines 109 to 112 in cf114f1

	if finalize, reason := shouldFinalize(wl); finalize {
	if controllerutil.ContainsFinalizer(wl, SliceControllerName) {
	log.V(3).Info(fmt.Sprintf("Cleaning up the Slices and finalizing the Workload because %s", reason))
	cleanedUp, err := r.cleanupSlices(ctx, wl)

I can't think of a scenario that requires additional cleanup (also, none of our e2e tests is failing with this change). What am I missing?

[mbobrovskyi]

I can't think of a scenario that requires additional cleanup (also, none of our e2e tests is failing with this change). What am I missing?

Before this, we try to get the workload, and if it doesn’t exist, we skip the reconciliation. But probably it shouldn't be a problem because we have finalizer, right?

[pajakd]

I can't think of a scenario that requires additional cleanup (also, none of our e2e tests is failing with this change). What am I missing?

Before this, we try to get the workload, and if it doesn’t exist, we skip the reconciliation. But probably it shouldn't be a problem because we have finalizer, right?

I think so too. We delete the finalizer only after performing the cleanup so the workload cannot be deleted if the slices still exist.

[PBundyra]

Let's use const vals here

[pajakd]

Done.

[PBundyra]

Overall LGTM