Repository: kubernetes-sigs/kueue

Issue:
Add validation for PodSet Grouping
<!-- Please only use this template for submitting enhancement requests -->

**What would you like to be added**:
In https://github.com/kubernetes-sigs/kueue/pull/5845 we have added a possibility to schedule leader with workers. A new annotation "podset-group-name" has been introduced, but without explicit user-friendly validation. The goal of this issue is to have proper validation in place:
- Make sure that a specific PodSet Group name is used in 2 PodSets. If it is used in only 1 PodSet it does not make sense from the user perspective and suggests misconfiguration. At the same time algorithm is considering only 2 PodSets when grouping so it does not work for more than 2 PodSets.
- Make sure value PodSet Group name annotations adheres to regexp: [a-z][a-z0-9]*.
- The annotations "podset-required-topology"/"podset-preferred-topology" and value of them have to match in both grouped PodSets. In other words - both have to use the same required or preferred annotation and the requested topology in both has to match.
- Make sure that at least one of the PodSets has only 1 Pod
- PodSet Slices cannot be requested with PodSet Grouping

**Why is this needed**:
User might misconfigure the Leader and Workers co-location and it would not be obvious for them to debug what is wrong. Validation will surface any misconfigurations.

**Completion requirements**:

This enhancement requires the following artifacts:

- [ ] Design doc
- [ ] API change
- [ ] Docs update

The artifacts should be linked in subsequent comments.


Discussion:
@kshalot: /assign@kannon92: If we want validation, should we make it a proper API rather than an annotation/label?@mbobrovskyi: This is a Pod annotation. We cannot add it to the API.@kannon92: > This is a Pod annotation. We cannot add it to the API.

Ah I see.@mimowo: @kshalot @PBundyra where are we with the status, it this accurate?

- [ ] Make sure that a specific PodSet Group name is used in 2 PodSets. If it is used in only 1 PodSet it does not make sense from the user perspective and suggests misconfiguration. At the same time algorithm is considering only 2 PodSets when grouping so it does not work for more than 2 PodSets.
- [x] Make sure value PodSet Group name annotations adheres to regexp: [a-z][a-z0-9]*.
- [ ] The annotations "podset-required-topology"/"podset-preferred-topology" and value of them have to match in both grouped PodSets. In other words - both have to use the same required or preferred annotation and the requested topology in both has to match.
- [ ] Make sure that at least one of the PodSets has only 1 Pod
- [x] PodSet Slices cannot be requested with PodSet Grouping@kshalot: What you posted is accurate.

1. Name validation was added in #6708.
1. Slicing validation was added in #7051.

The rest is WIP, likely covered in a single PR, hopefully by EOD/tomorrow morning.@mimowo: After the picture looks complete: https://github.com/kubernetes-sigs/kueue/pull/7061

- [x] Make sure that a specific PodSet Group name is used in 2 PodSets. If it is used in only 1 PodSet it does not make sense from the user perspective and suggests misconfiguration. At the same time algorithm is considering only 2 PodSets when grouping so it does not work for more than 2 PodSets.
- [x] Make sure value PodSet Group name annotations adheres to regexp: [a-z][a-z0-9]*.
- [x] The annotations "podset-required-topology"/"podset-preferred-topology" and value of them have to match in both grouped PodSets. In other words - both have to use the same required or preferred annotation and the requested topology in both has to match.
- [x] Make sure that at least one of the PodSets has only 1 Pod
- [x] PodSet Slices cannot be requested with PodSet Grouping

Solution patch:
diff --git a/pkg/controller/jobframework/tas_validation.go b/pkg/controller/jobframework/tas_validation.go
index 6cd0f2176..733598976 100644
--- a/pkg/controller/jobframework/tas_validation.go
+++ b/pkg/controller/jobframework/tas_validation.go
@@ -34,6 +34,7 @@ func ValidateTASPodSetRequest(replicaPath *field.Path, replicaMetadata *metav1.O
 	_, unconstrainedFound := replicaMetadata.Annotations[kueuebeta.PodSetUnconstrainedTopologyAnnotation]
 	sliceRequiredValue, sliceRequiredFound := replicaMetadata.Annotations[kueuebeta.PodSetSliceRequiredTopologyAnnotation]
 	_, sliceSizeFound := replicaMetadata.Annotations[kueuebeta.PodSetSliceSizeAnnotation]
+	podSetGroupNameValue, podSetGroupNameFound := replicaMetadata.Annotations[kueuebeta.PodSetGroupName]
 
 	// validate no more than 1 annotation
 	asInt := func(b bool) int {
@@ -65,7 +66,6 @@ func ValidateTASPodSetRequest(replicaPath *field.Path, replicaMetadata *metav1.O
 	}
 
 	// validate PodSetGroupName annotation
-	podSetGroupNameValue, podSetGroupNameFound := replicaMetadata.Annotations[kueuebeta.PodSetGroupName]
 	if podSetGroupNameFound {
 		allErrs = append(allErrs, validatePodSetGroupNameAnnotation(podSetGroupNameValue, annotationsPath.Key(kueuebeta.PodSetGroupName))...)
 	}
@@ -82,10 +82,18 @@ func ValidateTASPodSetRequest(replicaPath *field.Path, replicaMetadata *metav1.O
 			allErrs = append(allErrs, field.Required(annotationsPath.Key(kueuebeta.PodSetSliceSizeAnnotation), "slice size is required if slice topology is requested"))
 		}
 	}
-
 	if !sliceRequiredFound && sliceSizeFound {
 		allErrs = append(allErrs, field.Forbidden(annotationsPath.Key(kueuebeta.PodSetSliceSizeAnnotation), fmt.Sprintf("cannot be set when '%s' is not present", kueuebeta.PodSetSliceRequiredTopologyAnnotation)))
 	}
+	if podSetGroupNameFound {
+		if sliceSizeFound {
+			allErrs = append(allErrs, field.Forbidden(annotationsPath.Key(kueuebeta.PodSetSliceSizeAnnotation), fmt.Sprintf("cannot be set when '%s' is present", kueuebeta.PodSetGroupName)))
+		}
+
+		if sliceRequiredFound {
+			allErrs = append(allErrs, field.Forbidden(annotationsPath.Key(kueuebeta.PodSetSliceRequiredTopologyAnnotation), fmt.Sprintf("cannot be set when '%s' is present", kueuebeta.PodSetGroupName)))
+		}
+	}
 
 	return allErrs
 }



Test patch:
diff --git a/pkg/controller/jobs/leaderworkerset/leaderworkerset_webhook_test.go b/pkg/controller/jobs/leaderworkerset/leaderworkerset_webhook_test.go
index 56065e563..d10a3a911 100644
--- a/pkg/controller/jobs/leaderworkerset/leaderworkerset_webhook_test.go
+++ b/pkg/controller/jobs/leaderworkerset/leaderworkerset_webhook_test.go
@@ -259,6 +259,41 @@ func TestValidateCreate(t *testing.T) {
 			}.ToAggregate(),
 			topologyAwareScheduling: true,
 		},
+		"invalid slice topology request - slicing requested together with grouping": {
+			lws: testingleaderworkerset.MakeLeaderWorkerSet("test-lws", "").
+				Queue("test-queue").
+				LeaderTemplate(corev1.PodTemplateSpec{
+					ObjectMeta: metav1.ObjectMeta{
+						Annotations: map[string]string{
+							kueuealpha.PodSetGroupName:                       "leadername1",
+							kueuealpha.PodSetSliceRequiredTopologyAnnotation: "cloud.com/block",
+							kueuealpha.PodSetSliceSizeAnnotation:             "1",
+						},
+					},
+				}).
+				Size(4).
+				WorkerTemplate(corev1.PodTemplateSpec{
+					ObjectMeta: metav1.ObjectMeta{
+						Annotations: map[string]string{
+							kueuealpha.PodSetGroupName:                       "workername1",
+							kueuealpha.PodSetSliceRequiredTopologyAnnotation: "cloud.com/block",
+							kueuealpha.PodSetSliceSizeAnnotation:             "1",
+						},
+					},
+				}).
+				Obj(),
+			wantErr: field.ErrorList{
+				field.Forbidden(field.NewPath("spec.leaderWorkerTemplate.leaderTemplate.metadata.annotations").
+					Key("kueue.x-k8s.io/podset-slice-size"), "cannot be set when 'kueue.x-k8s.io/podset-group-name' is present"),
+				field.Forbidden(field.NewPath("spec.leaderWorkerTemplate.leaderTemplate.metadata.annotations").
+					Key("kueue.x-k8s.io/podset-slice-required-topology"), "cannot be set when 'kueue.x-k8s.io/podset-group-name' is present"),
+				field.Forbidden(field.NewPath("spec.leaderWorkerTemplate.workerTemplate.metadata.annotations").
+					Key("kueue.x-k8s.io/podset-slice-size"), "cannot be set when 'kueue.x-k8s.io/podset-group-name' is present"),
+				field.Forbidden(field.NewPath("spec.leaderWorkerTemplate.workerTemplate.metadata.annotations").
+					Key("kueue.x-k8s.io/podset-slice-required-topology"), "cannot be set when 'kueue.x-k8s.io/podset-group-name' is present"),
+			}.ToAggregate(),
+			topologyAwareScheduling: true,
+		},
 		"valid PodSet group name request": {
 			lws: testingleaderworkerset.MakeLeaderWorkerSet("test-lws", "").
 				Queue("test-queue").
diff --git a/test/integration/singlecluster/controller/jobs/trainjob/trainjob_controller_test.go b/test/integration/singlecluster/controller/jobs/trainjob/trainjob_controller_test.go
index f287f3087..58c7bcfaf 100644
--- a/test/integration/singlecluster/controller/jobs/trainjob/trainjob_controller_test.go
+++ b/test/integration/singlecluster/controller/jobs/trainjob/trainjob_controller_test.go
@@ -312,16 +312,16 @@ var _ = ginkgo.Describe("TrainJob controller for workloads when only jobs with q
 				Replicas: 1,
 			}).
 			Obj()
-		testCtr := testingtrainjob.MakeClusterTrainingRuntime("test", testJobSet.Spec)
+		testTr := testingtrainjob.MakeTrainingRuntime("test", ns.Name, testJobSet.Spec)
 		trainJob := testingtrainjob.MakeTrainJob("trainjob-test", ns.Name).RuntimeRef(kftrainerapi.RuntimeRef{
 			APIGroup: ptr.To("trainer.kubeflow.org"),
 			Name:     "test",
-			Kind:     ptr.To("ClusterTrainingRuntime"),
+			Kind:     ptr.To(kftrainerapi.TrainingRuntimeKind),
 		}).
 			Suspend(false).
 			Obj()
 
-		util.MustCreate(ctx, k8sClient, testCtr)
+		util.MustCreate(ctx, k8sClient, testTr)
 		util.MustCreate(ctx, k8sClient, trainJob)
 		createdTrainJob := &kftrainerapi.TrainJob{}
 		gomega.Eventually(func(g gomega.Gomega) {