Repository: kubernetes-sigs/kueue

Issue:
Cleanup deleteWorkloads from the TestLastSchedulingContext unit test
<!-- Please only use this template for submitting clean up requests -->

**What would you like to be cleaned**:

I would like the field `deleteWorkloads` to be just a slice of workload references.

**Why is this needed**:

Specifying the entire workload is not necessary, the test code could just "Get" the latest workload version based on the reference and then delete.

Discussion:
@Singularity23x0: /assign

Solution patch:
diff --git a/pkg/webhooks/workload_webhook.go b/pkg/webhooks/workload_webhook.go
index e72170077..af6184ca1 100644
--- a/pkg/webhooks/workload_webhook.go
+++ b/pkg/webhooks/workload_webhook.go
@@ -301,10 +301,8 @@ func validateAdmissionUpdate(new, old *kueue.Admission, path *field.Path) field.
 		if len(new.PodSetAssignments) != len(old.PodSetAssignments) {
 			return apivalidation.ValidateImmutableField(new, old, path)
 		}
-		// Allow to update (set) TopologyAssignment when DelayedTopologyRequest
-		// transitions from Pending to Ready.
+		// Allow to update (set) TopologyAssignments
 		for i := range new.PodSetAssignments {
-			// allow to update TopologyAssignments
 			old.PodSetAssignments[i].TopologyAssignment = new.PodSetAssignments[i].TopologyAssignment
 			old.PodSetAssignments[i].DelayedTopologyRequest = new.PodSetAssignments[i].DelayedTopologyRequest
 		}



Test patch:
diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go
index 0ff12c4c6..703eb9136 100644
--- a/pkg/scheduler/scheduler_test.go
+++ b/pkg/scheduler/scheduler_test.go
@@ -3786,7 +3786,7 @@ func TestLastSchedulingContext(t *testing.T) {
 		cqs                            []kueue.ClusterQueue
 		admittedWorkloads              []kueue.Workload
 		workloads                      []kueue.Workload
-		deleteWorkloads                []kueue.Workload
+		deleteWorkloads                []client.ObjectKey
 		wantPreempted                  sets.Set[workload.Reference]
 		wantAdmissionsOnFirstSchedule  map[workload.Reference]kueue.Admission
 		wantAdmissionsOnSecondSchedule map[workload.Reference]kueue.Admission
@@ -3826,14 +3826,10 @@ func TestLastSchedulingContext(t *testing.T) {
 					Request(corev1.ResourceCPU, "20").
 					Obj(),
 			},
-			deleteWorkloads: []kueue.Workload{
-				*utiltesting.MakeWorkload("low-1", "default").
-					Queue("main").
-					Request(corev1.ResourceCPU, "50").
-					ReserveQuota(utiltesting.MakeAdmission("eng-alpha").Assignment(corev1.ResourceCPU, "on-demand", "50").Obj()).
-					Admitted(true).
-					Obj(),
-			},
+			deleteWorkloads: []client.ObjectKey{{
+				Namespace: metav1.NamespaceDefault,
+				Name:      "low-1",
+			}},
 			wantPreempted:                 sets.Set[workload.Reference]{},
 			wantAdmissionsOnFirstSchedule: map[workload.Reference]kueue.Admission{},
 			wantAdmissionsOnSecondSchedule: map[workload.Reference]kueue.Admission{
@@ -3860,8 +3856,7 @@ func TestLastSchedulingContext(t *testing.T) {
 					Request(corev1.ResourceCPU, "20").
 					Obj(),
 			},
-			deleteWorkloads: []kueue.Workload{},
-			wantPreempted:   sets.Set[workload.Reference]{},
+			wantPreempted: sets.Set[workload.Reference]{},
 			wantAdmissionsOnFirstSchedule: map[workload.Reference]kueue.Admission{
 				"default/workload1": *utiltesting.MakeAdmission("eng-cohort-beta").Assignment(corev1.ResourceCPU, "on-demand", "20").Obj(),
 				"default/borrower":  *utiltesting.MakeAdmission("eng-cohort-alpha").Assignment(corev1.ResourceCPU, "on-demand", "20").Obj(),
@@ -3893,8 +3888,7 @@ func TestLastSchedulingContext(t *testing.T) {
 					Request(corev1.ResourceCPU, "20").
 					Obj(),
 			},
-			deleteWorkloads: []kueue.Workload{},
-			wantPreempted:   sets.Set[workload.Reference]{},
+			wantPreempted: sets.Set[workload.Reference]{},
 			wantAdmissionsOnFirstSchedule: map[workload.Reference]kueue.Admission{
 				"default/workload": *utiltesting.MakeAdmission("eng-cohort-theta").Assignment(corev1.ResourceCPU, "spot", "20").Obj(),
 			},
@@ -3930,8 +3924,7 @@ func TestLastSchedulingContext(t *testing.T) {
 					Request(corev1.ResourceCPU, "20").
 					Obj(),
 			},
-			deleteWorkloads: []kueue.Workload{},
-			wantPreempted:   sets.Set[workload.Reference]{},
+			wantPreempted: sets.Set[workload.Reference]{},
 			wantAdmissionsOnFirstSchedule: map[workload.Reference]kueue.Admission{
 				"default/workload": *utiltesting.MakeAdmission("eng-cohort-theta").Assignment(corev1.ResourceCPU, "on-demand", "20").Obj(),
 			},
@@ -3964,7 +3957,10 @@ func TestLastSchedulingContext(t *testing.T) {
 					Request(corev1.ResourceCPU, "20").
 					Obj(),
 			},
-			deleteWorkloads:               []kueue.Workload{*utiltesting.MakeWorkload("placeholder-alpha", "default").Obj()},
+			deleteWorkloads: []client.ObjectKey{{
+				Namespace: metav1.NamespaceDefault,
+				Name:      "placeholder-alpha",
+			}},
 			wantPreempted:                 sets.New[workload.Reference]("default/placeholder-alpha"),
 			wantAdmissionsOnFirstSchedule: map[workload.Reference]kueue.Admission{},
 			wantAdmissionsOnSecondSchedule: map[workload.Reference]kueue.Admission{
@@ -4047,8 +4043,11 @@ func TestLastSchedulingContext(t *testing.T) {
 					Request(corev1.ResourceCPU, "22").
 					Obj(),
 			},
-			deleteWorkloads: []kueue.Workload{*utiltesting.MakeWorkload("alpha2", "default").Obj()},
-			wantPreempted:   sets.New[workload.Reference]("default/alpha2"),
+			deleteWorkloads: []client.ObjectKey{{
+				Namespace: metav1.NamespaceDefault,
+				Name:      "alpha2",
+			}},
+			wantPreempted: sets.New[workload.Reference]("default/alpha2"),
 			wantAdmissionsOnSecondSchedule: map[workload.Reference]kueue.Admission{
 				"default/alpha1": *utiltesting.MakeAdmission("eng-cohort-alpha").Assignment(corev1.ResourceCPU, "on-demand", "22").Obj(),
 				"default/alpha3": *utiltesting.MakeAdmission("eng-cohort-alpha").Assignment(corev1.ResourceCPU, "spot", "22").Obj(),
@@ -4130,16 +4129,21 @@ func TestLastSchedulingContext(t *testing.T) {
 				t.Errorf("Unexpected scheduled workloads (-want,+got):\n%s", diff)
 			}
 
-			for _, wl := range tc.deleteWorkloads {
-				err := cl.Delete(ctx, &wl)
+			for _, workloadReference := range tc.deleteWorkloads {
+				var workload kueue.Workload
+				err := cl.Get(ctx, workloadReference, &workload)
+				if err != nil {
+					t.Errorf("Unable to get workload: %v", err)
+				}
+				err = cl.Delete(ctx, &workload)
 				if err != nil {
 					t.Errorf("Delete workload failed: %v", err)
 				}
-				err = cqCache.DeleteWorkload(log, &wl)
+				err = cqCache.DeleteWorkload(log, &workload)
 				if err != nil {
 					t.Errorf("Delete workload failed: %v", err)
 				}
-				qManager.QueueAssociatedInadmissibleWorkloadsAfter(ctx, &wl, nil)
+				qManager.QueueAssociatedInadmissibleWorkloadsAfter(ctx, &workload, nil)
 			}
 
 			scheduler.schedule(ctx)
diff --git a/pkg/webhooks/workload_webhook_test.go b/pkg/webhooks/workload_webhook_test.go
index 27a795ce9..b9cbf45ac 100644
--- a/pkg/webhooks/workload_webhook_test.go
+++ b/pkg/webhooks/workload_webhook_test.go
@@ -376,7 +376,7 @@ func TestValidateWorkloadUpdate(t *testing.T) {
 				State:              kueue.CheckStateReady,
 			}).Obj(),
 		},
-		"TopologyAssignment cannot be mutated": {
+		"TopologyAssignment can be mutated": {
 			enableTopologyAwareScheduling: true,
 			before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				PodSets(
@@ -420,60 +420,22 @@ func TestValidateWorkloadUpdate(t *testing.T) {
 						Obj(),
 				).
 				Obj(),
-			wantErr: field.ErrorList{
-				field.Invalid(field.NewPath("status", "admission"), nil, ""),
-			},
+			wantErr: nil,
 		},
-		"TopologyAssignment cannot be set without delayedTopologyRequest": {
+		"PodSets cannot be removed from admission": {
 			enableTopologyAwareScheduling: true,
 			before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				PodSets(
 					*testingutil.MakePodSet("ps1", 3).Obj(),
+					*testingutil.MakePodSet("ps2", 3).Obj(),
 				).
 				ReserveQuota(
 					testingutil.MakeAdmission("cluster-queue").
 						PodSets(kueue.PodSetAssignment{
 							Name: "ps1",
 						}).
-						Obj(),
-				).
-				Obj(),
-			after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
-				PodSets(
-					*testingutil.MakePodSet("ps1", 3).Obj(),
-				).
-				ReserveQuota(
-					testingutil.MakeAdmission("cluster-queue").
 						PodSets(kueue.PodSetAssignment{
-							Name: "ps1",
-							TopologyAssignment: &kueue.TopologyAssignment{
-								Levels: []string{"level"},
-								Domains: []kueue.TopologyDomainAssignment{
-									{
-										Values: []string{"abc"},
-										Count:  3,
-									},
-								},
-							},
-						}).
-						Obj(),
-				).
-				Obj(),
-			wantErr: field.ErrorList{
-				field.Invalid(field.NewPath("status", "admission"), nil, ""),
-			},
-		},
-		"TopologyAssignment can be set if delayedTopologyRequest is Pending": {
-			enableTopologyAwareScheduling: true,
-			before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
-				PodSets(
-					*testingutil.MakePodSet("ps1", 3).Obj(),
-				).
-				ReserveQuota(
-					testingutil.MakeAdmission("cluster-queue").
-						PodSets(kueue.PodSetAssignment{
-							Name:                   "ps1",
-							DelayedTopologyRequest: ptr.To(kueue.DelayedTopologyRequestStatePending),
+							Name: "ps2",
 						}).
 						Obj(),
 				).
@@ -481,20 +443,12 @@ func TestValidateWorkloadUpdate(t *testing.T) {
 			after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				PodSets(
 					*testingutil.MakePodSet("ps1", 3).Obj(),
+					*testingutil.MakePodSet("ps2", 3).Obj(),
 				).
 				ReserveQuota(
 					testingutil.MakeAdmission("cluster-queue").
 						PodSets(kueue.PodSetAssignment{
 							Name: "ps1",
-							TopologyAssignment: &kueue.TopologyAssignment{
-								Levels: []string{"level"},
-								Domains: []kueue.TopologyDomainAssignment{
-									{
-										Values: []string{"abc"},
-										Count:  3,
-									},
-								},
-							},
 						}).
 						Obj(),
 				).
@@ -503,49 +457,22 @@ func TestValidateWorkloadUpdate(t *testing.T) {
 				field.Invalid(field.NewPath("status", "admission"), nil, ""),
 			},
 		},
-		"TopologyAssignment cannot be set if delayedTopologyRequest is Ready": {
+		"PodSets cannot be added to admission": {
 			enableTopologyAwareScheduling: true,
 			before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				PodSets(
 					*testingutil.MakePodSet("ps1", 3).Obj(),
-				).
-				ReserveQuota(
-					testingutil.MakeAdmission("cluster-queue").
-						PodSets(kueue.PodSetAssignment{
-							Name:                   "ps1",
-							DelayedTopologyRequest: ptr.To(kueue.DelayedTopologyRequestStateReady),
-						}).
-						Obj(),
-				).
-				Obj(),
-			after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
-				PodSets(
-					*testingutil.MakePodSet("ps1", 3).Obj(),
+					*testingutil.MakePodSet("ps2", 3).Obj(),
 				).
 				ReserveQuota(
 					testingutil.MakeAdmission("cluster-queue").
 						PodSets(kueue.PodSetAssignment{
 							Name: "ps1",
-							TopologyAssignment: &kueue.TopologyAssignment{
-								Levels: []string{"level"},
-								Domains: []kueue.TopologyDomainAssignment{
-									{
-										Values: []string{"abc"},
-										Count:  3,
-									},
-								},
-							},
 						}).
 						Obj(),
 				).
 				Obj(),
-			wantErr: field.ErrorList{
-				field.Invalid(field.NewPath("status", "admission"), nil, ""),
-			},
-		},
-		"PodSets cannot be removed from admission": {
-			enableTopologyAwareScheduling: true,
-			before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
+			after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				PodSets(
 					*testingutil.MakePodSet("ps1", 3).Obj(),
 					*testingutil.MakePodSet("ps2", 3).Obj(),
@@ -561,19 +488,6 @@ func TestValidateWorkloadUpdate(t *testing.T) {
 						Obj(),
 				).
 				Obj(),
-			after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
-				PodSets(
-					*testingutil.MakePodSet("ps1", 3).Obj(),
-					*testingutil.MakePodSet("ps2", 3).Obj(),
-				).
-				ReserveQuota(
-					testingutil.MakeAdmission("cluster-queue").
-						PodSets(kueue.PodSetAssignment{
-							Name: "ps1",
-						}).
-						Obj(),
-				).
-				Obj(),
 			wantErr: field.ErrorList{
 				field.Invalid(field.NewPath("status", "admission"), nil, ""),
 			},
@@ -619,6 +533,7 @@ func TestValidateWorkloadUpdate(t *testing.T) {
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
 			features.SetFeatureGateDuringTest(t, features.ElasticJobsViaWorkloadSlices, tc.enableElasticJobsFeature)
+			features.SetFeatureGateDuringTest(t, features.TopologyAwareScheduling, tc.enableTopologyAwareScheduling)
 			errList := ValidateWorkloadUpdate(tc.after, tc.before, configapi.MultiKueueDispatcherModeAllAtOnce)
 			if diff := cmp.Diff(tc.wantErr, errList, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); diff != "" {
 				t.Errorf("ValidateWorkloadUpdate() mismatch (-want +got):\n%s", diff)