Eliminate replicaPath from CRD (#109)

Author: tardieu

Makefile

@@ -83,7 +83,7 @@ generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and
 	$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..."
 
 .PHONY: generate-apiref
-generate-apiref: genref
+generate-apiref: genref ## Generate API Reference for project website.
 	cd  site/genref && $(GENREF)  -o ../_pages
 
 .PHONY: fmt

api/v1beta2/appwrapper_types.go

@@ -59,10 +59,6 @@ type AppWrapperPodSet struct {
 	//+optional
 	Replicas *int32 `json:"replicas,omitempty"`
 
-	// ReplicaPath is the path within Component.Template to the replica count for this PodSet
-	//+optional
-	ReplicaPath string `json:"replicaPath,omitempty"`
-
 	// PodPath is the path Component.Template to the PodTemplateSpec for this PodSet
 	PodPath string `json:"podPath"`
 }

config/crd/bases/workload.codeflare.dev_appwrappers.yaml

@@ -138,10 +138,6 @@ spec:
                             description: PodPath is the path Component.Template to
                               the PodTemplateSpec for this PodSet
                             type: string
-                          replicaPath:
-                            description: ReplicaPath is the path within Component.Template
-                              to the replica count for this PodSet
-                            type: string
                           replicas:
                             description: Replicas is the number of pods in this PodSet
                             format: int32

internal/webhook/appwrapper_fixtures_test.go

@@ -174,7 +174,7 @@ func deployment(replicaCount int, milliCPU int64) workloadv1beta2.AppWrapperComp
 	jsonBytes, err := yaml.YAMLToJSON([]byte(yamlString))
 	Expect(err).NotTo(HaveOccurred())
 	return workloadv1beta2.AppWrapperComponent{
-		PodSets:  []workloadv1beta2.AppWrapperPodSet{{ReplicaPath: "template.spec.replicas", PodPath: "template.spec.template"}},
+		PodSets:  []workloadv1beta2.AppWrapperPodSet{{Replicas: ptr.To(int32(replicaCount)), PodPath: "template.spec.template"}},
 		Template: runtime.RawExtension{Raw: jsonBytes},
 	}
 }
@@ -365,7 +365,7 @@ func rayCluster(workerCount int, milliCPU int64) workloadv1beta2.AppWrapperCompo
 	return workloadv1beta2.AppWrapperComponent{
 		PodSets: []workloadv1beta2.AppWrapperPodSet{
 			{Replicas: ptr.To(int32(1)), PodPath: "template.spec.headGroupSpec.template"},
-			{ReplicaPath: "template.spec.workerGroupSpecs[0].maxReplicas", PodPath: "template.spec.workerGroupSpecs[0].template"},
+			{Replicas: ptr.To(int32(workerCount)), PodPath: "template.spec.workerGroupSpecs[0].template"},
 		},
 		Template: runtime.RawExtension{Raw: jsonBytes},
 	}
@@ -421,7 +421,7 @@ func jobSet(replicasWorker int, milliCPUWorker int64) workloadv1beta2.AppWrapper
 	return workloadv1beta2.AppWrapperComponent{
 		PodSets: []workloadv1beta2.AppWrapperPodSet{
 			{PodPath: "template.spec.replicatedJobs[0].template.spec.template"},
-			{ReplicaPath: "template.spec.replicatedJobs[1].template.spec.parallelism", PodPath: "template.spec.replicatedJobs[1].template.spec.template"},
+			{Replicas: ptr.To(int32(replicasWorker)), PodPath: "template.spec.replicatedJobs[1].template.spec.template"},
 		},
 		Template: runtime.RawExtension{Raw: jsonBytes},
 	}

internal/webhook/appwrapper_webhook.go

@@ -62,10 +62,6 @@ func (w *AppWrapperWebhook) Default(ctx context.Context, obj runtime.Object) err
 	if w.Config.EnableKueueIntegrations {
 		jobframework.ApplyDefaultForSuspend((*wlc.AppWrapper)(aw), w.Config.ManageJobsWithoutQueueName)
 	}
-	if err := expandPodSets(ctx, aw); err != nil {
-		log.FromContext(ctx).Info("Error raised during podSet expansion", "job", aw)
-		return err
-	}
 	return nil
 }
 
@@ -102,44 +98,6 @@ func (w *AppWrapperWebhook) ValidateDelete(context.Context, runtime.Object) (adm
 	return nil, nil
 }
 
-// expandPodSets expands and simplifies the AppWrapper's PodSets
-func expandPodSets(_ 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)
-		podSetsPath := compPath.Child("podSets")
-
-		// TODO: Here is where we would automatically create elided PodSets for known GVKs
-		//       See https://github.com/project-codeflare/appwrapper/issues/65
-
-		// Evaluate any ReplicaPaths and expand them to Replicas to simplify later processing
-		for psIdx, ps := range component.PodSets {
-			podSetPath := podSetsPath.Index(psIdx)
-			if ps.ReplicaPath != "" {
-				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")
-				}
-
-				if rc, err := utils.GetReplicas(unstruct, ps.ReplicaPath); err != nil {
-					return field.Invalid(podSetPath.Child("replicaPath"), ps.ReplicaPath,
-						fmt.Sprintf("replicaPath does not refer to an int: %v", err))
-				} else {
-					if ps.Replicas != nil && *ps.Replicas != rc {
-						return field.Invalid(podSetPath.Child("replicas"), ps.Replicas,
-							fmt.Sprintf("does not match value %v at path %v", rc, ps.ReplicaPath))
-					}
-					component.PodSets[psIdx].Replicas = &rc
-				}
-			}
-		}
-
-	}
-	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
@@ -262,9 +220,6 @@ func (w *AppWrapperWebhook) validateAppWrapperUpdate(old *workloadv1beta2.AppWra
 				if oldComponent.PodSets[psIdx].PodPath != newComponent.PodSets[psIdx].PodPath {
 					allErrors = append(allErrors, field.Forbidden(compPath.Child("podsets").Index(psIdx).Child("podPath"), msg))
 				}
-				if oldComponent.PodSets[psIdx].ReplicaPath != newComponent.PodSets[psIdx].ReplicaPath {
-					allErrors = append(allErrors, field.Forbidden(compPath.Child("podsets").Index(psIdx).Child("replciaPath"), msg))
-				}
 			}
 		}
 	}

samples/wrapped-deployment.yaml

@@ -7,8 +7,8 @@ metadata:
 spec:
   components:
   - podSets:
-    - podPath: template.spec.template
-      replicaPath: template.spec.replicas
+    - replicas: 1
+      podPath: template.spec.template
     template:
       apiVersion: apps/v1
       kind: Deployment

samples/wrapped-pytorch-job.yaml

@@ -7,10 +7,10 @@ metadata:
 spec:
   components:
   - podSets:
-    - podPath: template.spec.pytorchReplicaSpecs.Master.template
-      replicaPath: template.spec.pytorchReplicaSpecs.Master.replicas
-    - podPath: template.spec.pytorchReplicaSpecs.Worker.template
-      replicaPath: template.spec.pytorchReplicaSpecs.Worker.replicas
+    - replicas: 1
+      podPath: template.spec.pytorchReplicaSpecs.Master.template
+    - replicas: 1
+      podPath: template.spec.pytorchReplicaSpecs.Worker.template
     template:
       apiVersion: "kubeflow.org/v1"
       kind: PyTorchJob

site/_pages/appwrapper.v1beta2.md

@@ -126,13 +126,6 @@ arbitrary metadata about the Component to customize its treatment by the AppWrap
    <p>Replicas is the number of pods in this PodSet</p>
 </td>
 </tr>
-<tr><td><code>replicaPath</code><br/>
-<code>string</code>
-</td>
-<td>
-   <p>ReplicaPath is the path within Component.Template to the replica count for this PodSet</p>
-</td>
-</tr>
 <tr><td><code>podPath</code> <B>[Required]</B><br/>
 <code>string</code>
 </td>

site/_pages/sample-pytorch.md

@@ -15,10 +15,10 @@ metadata:
 spec:
   components:
   - podSets:
-    - podPath: template.spec.pytorchReplicaSpecs.Master.template
-      replicaPath: template.spec.pytorchReplicaSpecs.Master.replicas
-    - podPath: template.spec.pytorchReplicaSpecs.Worker.template
-      replicaPath: template.spec.pytorchReplicaSpecs.Worker.replicas
+    - replicas: 1
+      podPath: template.spec.pytorchReplicaSpecs.Master.template
+    - replicas: 1
+      podPath: template.spec.pytorchReplicaSpecs.Worker.template
     template:
       apiVersion: "kubeflow.org/v1"
       kind: PyTorchJob

test/e2e/appwrapper_test.go

@@ -196,10 +196,6 @@ var _ = Describe("AppWrapper E2E Test", func() {
 			Expect(updateAppWrapper(ctx, awName, func(aw *workloadv1beta2.AppWrapper) {
 				aw.Spec.Components[0].PodSets[0].Replicas = ptr.To(int32(12))
 			})).ShouldNot(Succeed())
-
-			Expect(updateAppWrapper(ctx, awName, func(aw *workloadv1beta2.AppWrapper) {
-				aw.Spec.Components[0].PodSets[0].ReplicaPath = "bad"
-			})).ShouldNot(Succeed())
 		})
 	})