PodSet inference only updates the Status, not the Spec

Author: dgrove-oss

internal/controller/appwrapper/suite_test.go

@@ -39,6 +39,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	workloadv1beta2 "github.com/project-codeflare/appwrapper/api/v1beta2"
+	"github.com/project-codeflare/appwrapper/internal/controller/awstatus"
 	kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1"
 )
 
@@ -100,6 +101,7 @@ var _ = BeforeSuite(func() {
 	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme})
 	Expect(err).NotTo(HaveOccurred())
 	Expect(k8sClient).NotTo(BeNil())
+	awstatus.CacheClient(k8sClient)
 })
 
 var _ = AfterSuite(func() {

internal/webhook/appwrapper_webhook.go

@@ -62,17 +62,21 @@ type AppWrapperWebhook struct {
 
 var _ webhook.CustomDefaulter = &AppWrapperWebhook{}
 
-// Default ensures that Suspend is set appropriately when an AppWrapper is created
+// Default fills in default values when an AppWrapper is created:
+//  1. Inject default queue name
+//  2. Ensure Suspend is set appropriately
+//  3. Add labels with the user name and id
 func (w *AppWrapperWebhook) Default(ctx context.Context, obj runtime.Object) error {
 	aw := obj.(*workloadv1beta2.AppWrapper)
 	log.FromContext(ctx).Info("Applying defaults", "job", aw)
+
+	// Queue name and Suspend
 	if w.Config.EnableKueueIntegrations {
+		if w.Config.QueueName != "" && aw.Annotations[QueueNameLabel] == "" && aw.Labels[QueueNameLabel] == "" {
+			aw.Labels[QueueNameLabel] = w.Config.QueueName
+		}
 		jobframework.ApplyDefaultForSuspend((*wlc.AppWrapper)(aw), w.Config.ManageJobsWithoutQueueName)
 	}
-	if err := inferPodSets(ctx, aw); err != nil {
-		log.FromContext(ctx).Info("Error raised during podSet inference", "job", aw)
-		return err
-	}
 
 	// inject labels with user name and id
 	request, err := admission.RequestFromContext(ctx)
@@ -83,11 +87,6 @@ func (w *AppWrapperWebhook) Default(ctx context.Context, obj runtime.Object) err
 	username := utils.SanitizeLabel(userInfo.Username)
 	aw.Labels = utilmaps.MergeKeepFirst(map[string]string{AppWrapperUsernameLabel: username, AppWrapperUserIDLabel: userInfo.UID}, aw.Labels)
 
-	// inject default queue name if missing from appwrapper and configured on controller
-	if w.Config.QueueName != "" && aw.Annotations[QueueNameLabel] == "" && aw.Labels[QueueNameLabel] == "" {
-		aw.Labels[QueueNameLabel] = w.Config.QueueName
-	}
-
 	return nil
 }
 
@@ -124,30 +123,6 @@ func (w *AppWrapperWebhook) ValidateDelete(context.Context, runtime.Object) (adm
 	return nil, nil
 }
 
-// inferPodSets infers the AppWrapper's PodSets
-func inferPodSets(_ context.Context, aw *workloadv1beta2.AppWrapper) error {
-	components := aw.Spec.Components
-	componentsPath := field.NewPath("spec").Child("components")
-	for idx, component := range components {
-		compPath := componentsPath.Index(idx)
-
-		// Automatically create elided PodSets for known GVKs
-		if len(component.DeclaredPodSets) == 0 {
-			unstruct := &unstructured.Unstructured{}
-			_, _, err := unstructured.UnstructuredJSONScheme.Decode(component.Template.Raw, nil, unstruct)
-			if err != nil {
-				return field.Invalid(compPath.Child("template"), component.Template, "failed to decode as JSON")
-			}
-			podSets, err := utils.InferPodSets(unstruct)
-			if err != nil {
-				return err
-			}
-			components[idx].DeclaredPodSets = podSets
-		}
-	}
-	return nil
-}
-
 // rbacs required to enable SubjectAccessReview
 //+kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create
 //+kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=list
@@ -221,25 +196,31 @@ func (w *AppWrapperWebhook) validateAppWrapperCreate(ctx context.Context, aw *wo
 			}
 		}
 
-		// 4. Each PodSet.Path must specify a path within Template to a v1.PodSpecTemplate
+		// 4. Every DeclaredPodSet must specify a path within Template to a v1.PodSpecTemplate
 		podSetsPath := compPath.Child("podSets")
 		for psIdx, ps := range component.DeclaredPodSets {
 			podSetPath := podSetsPath.Index(psIdx)
 			if ps.Path == "" {
 				allErrors = append(allErrors, field.Required(podSetPath.Child("path"), "podspec must specify path"))
 			}
 			if _, err := utils.GetPodTemplateSpec(unstruct, ps.Path); err != nil {
-				allErrors = append(allErrors, field.Invalid(podSetPath.Child("path"), ps.Path,
-					fmt.Sprintf("path does not refer to a v1.PodSpecTemplate: %v", err)))
+				allErrors = append(allErrors, field.Invalid(podSetPath.Child("path"), ps.Path, fmt.Sprintf("path does not refer to a v1.PodSpecTemplate: %v", err)))
 			}
-			podSpecCount += 1
 		}
 
 		// 5. Validate PodSets for known GVKs
-		if err := utils.ValidatePodSets(unstruct, component.DeclaredPodSets); err != nil {
-			allErrors = append(allErrors, field.Invalid(podSetsPath, component.DeclaredPodSets, err.Error()))
+		if inferred, err := utils.InferPodSets(unstruct); err != nil {
+			allErrors = append(allErrors, field.Invalid(compPath.Child("template"), component.Template, fmt.Sprintf("error inferring PodSets: %v", err)))
+		} else {
+			if len(component.DeclaredPodSets) > len(inferred) {
+				podSpecCount += len(component.DeclaredPodSets)
+			} else {
+				podSpecCount += len(inferred)
+			}
+			if err := utils.ValidatePodSets(component.DeclaredPodSets, inferred); err != nil {
+				allErrors = append(allErrors, field.Invalid(podSetsPath, component.DeclaredPodSets, err.Error()))
+			}
 		}
-
 	}
 
 	// 6. Enforce Kueue limitation that 0 < podSpecCount <= 8

pkg/utils/utils.go

@@ -302,48 +302,48 @@ func InferPodSets(obj *unstructured.Unstructured) ([]workloadv1beta2.AppWrapperP
 		}
 	}
 
-	return podSets, nil
-}
-
-// ValidatePodSets compares declared and inferred PodSets for known GVKs
-func ValidatePodSets(obj *unstructured.Unstructured, podSets []workloadv1beta2.AppWrapperPodSet) error {
-	declared := map[string]workloadv1beta2.AppWrapperPodSet{}
-
-	// construct a map with declared PodSets and find duplicates
-	for _, p := range podSets {
-		if _, ok := declared[p.Path]; ok {
-			return fmt.Errorf("duplicate PodSets with path '%v'", p.Path)
+	for _, ps := range podSets {
+		if _, err := GetPodTemplateSpec(obj, ps.Path); err != nil {
+			return nil, fmt.Errorf("%v does not refer to a v1.PodSpecTemplate: %v", ps.Path, err)
 		}
-		declared[p.Path] = p
 	}
 
-	// infer PodSets
-	inferred, err := InferPodSets(obj)
-	if err != nil {
-		return err
-	}
+	return podSets, nil
+}
 
-	// nothing inferred, nothing to validate
-	if len(inferred) == 0 {
+// ValidatePodSets valdiates the declared and inferred PodSets
+func ValidatePodSets(declared []workloadv1beta2.AppWrapperPodSet, inferred []workloadv1beta2.AppWrapperPodSet) error {
+	if len(declared) == 0 {
 		return nil
 	}
 
-	// compare PodSet counts
-	if len(inferred) != len(declared) {
-		return fmt.Errorf("PodSet count %v differs from expected count %v", len(declared), len(inferred))
+	// Validate that there are no duplicate paths in declared
+	declaredPaths := map[string]workloadv1beta2.AppWrapperPodSet{}
+	for _, p := range declared {
+		if _, ok := declaredPaths[p.Path]; ok {
+			return fmt.Errorf("multiple DeclaredPodSets with path '%v'", p.Path)
+		}
+		declaredPaths[p.Path] = p
 	}
 
-	// match inferred PodSets to declared PodSets
-	for _, ips := range inferred {
-		dps, ok := declared[ips.Path]
-		if !ok {
-			return fmt.Errorf("PodSet with path '%v' is missing", ips.Path)
+	// Validate that the declared PodSets match what inference computed
+	if len(inferred) > 0 {
+		if len(inferred) != len(declared) {
+			return fmt.Errorf("DeclaredPodSet count %v differs from inferred count %v", len(declared), len(inferred))
 		}
 
-		ipr := ptr.Deref(ips.Replicas, 1)
-		dpr := ptr.Deref(dps.Replicas, 1)
-		if ipr != dpr {
-			return fmt.Errorf("replica count %v differs from expected count %v for PodSet at path position '%v'", dpr, ipr, ips.Path)
+		// match inferred PodSets to declared PodSets
+		for _, ips := range inferred {
+			dps, ok := declaredPaths[ips.Path]
+			if !ok {
+				return fmt.Errorf("PodSet with path '%v' is missing", ips.Path)
+			}
+
+			ipr := ptr.Deref(ips.Replicas, 1)
+			dpr := ptr.Deref(dps.Replicas, 1)
+			if ipr != dpr {
+				return fmt.Errorf("replica count %v differs from inferred count %v for PodSet at path position '%v'", dpr, ipr, ips.Path)
+			}
 		}
 	}