Do not configure staging resources by default

Instead, only set resource requests when the configuration is present. We plan to handle resource limits separately and will not set those directly on the pods. Co-authored-by: Dave Walter <[email protected]>
wanddynosios · Jul 13, 2023 · 1c87dd5 · 1c87dd5
1 parent 1c6a207
commit 1c87dd5
Show file tree

Hide file tree

Showing 8 changed files with 88 additions and 39 deletions.
diff --git a/controllers/config/config.go b/controllers/config/config.go
@@ -17,21 +17,21 @@ type ControllerConfig struct {
 	IncludeContourRouter     bool `yaml:"includeContourRouter"`
 
 	// core controllers
-	CFProcessDefaults                CFProcessDefaults       `yaml:"cfProcessDefaults"`
-	CFStagingResourceLimits          CFStagingResourceLimits `yaml:"cfStagingResourceLimits"`
-	CFRootNamespace                  string                  `yaml:"cfRootNamespace"`
-	ContainerRegistrySecretNames     []string                `yaml:"containerRegistrySecretNames"`
-	TaskTTL                          string                  `yaml:"taskTTL"`
-	WorkloadsTLSSecretName           string                  `yaml:"workloads_tls_secret_name"`
-	WorkloadsTLSSecretNamespace      string                  `yaml:"workloads_tls_secret_namespace"`
-	BuilderName                      string                  `yaml:"builderName"`
-	RunnerName                       string                  `yaml:"runnerName"`
-	NamespaceLabels                  map[string]string       `yaml:"namespaceLabels"`
-	ExtraVCAPApplicationValues       map[string]any          `yaml:"extraVCAPApplicationValues"`
-	MaxRetainedPackagesPerApp        int                     `yaml:"maxRetainedPackagesPerApp"`
-	MaxRetainedBuildsPerApp          int                     `yaml:"maxRetainedBuildsPerApp"`
-	LogLevel                         zapcore.Level           `yaml:"logLevel"`
-	SpaceFinalizerAppDeletionTimeout *int64                  `yaml:"spaceFinalizerAppDeletionTimeout"`
+	CFProcessDefaults                CFProcessDefaults  `yaml:"cfProcessDefaults"`
+	CFStagingResources               CFStagingResources `yaml:"cfStagingResources"`
+	CFRootNamespace                  string             `yaml:"cfRootNamespace"`
+	ContainerRegistrySecretNames     []string           `yaml:"containerRegistrySecretNames"`
+	TaskTTL                          string             `yaml:"taskTTL"`
+	WorkloadsTLSSecretName           string             `yaml:"workloads_tls_secret_name"`
+	WorkloadsTLSSecretNamespace      string             `yaml:"workloads_tls_secret_namespace"`
+	BuilderName                      string             `yaml:"builderName"`
+	RunnerName                       string             `yaml:"runnerName"`
+	NamespaceLabels                  map[string]string  `yaml:"namespaceLabels"`
+	ExtraVCAPApplicationValues       map[string]any     `yaml:"extraVCAPApplicationValues"`
+	MaxRetainedPackagesPerApp        int                `yaml:"maxRetainedPackagesPerApp"`
+	MaxRetainedBuildsPerApp          int                `yaml:"maxRetainedBuildsPerApp"`
+	LogLevel                         zapcore.Level      `yaml:"logLevel"`
+	SpaceFinalizerAppDeletionTimeout *int64             `yaml:"spaceFinalizerAppDeletionTimeout"`
 
 	// job-task-runner
 	JobTTL string `yaml:"jobTTL"`
@@ -50,7 +50,7 @@ type CFProcessDefaults struct {
 	Timeout     *int64 `yaml:"timeout"`
 }
 
-type CFStagingResourceLimits struct {
+type CFStagingResources struct {
 	BuildCacheMB int64 `yaml:"buildCacheMB"`
 	DiskMB       int64 `yaml:"diskMB"`
 	MemoryMB     int64 `yaml:"memoryMB"`
@@ -78,8 +78,8 @@ func LoadFromPath(path string) (*ControllerConfig, error) {
 		config.SpaceFinalizerAppDeletionTimeout = tools.PtrTo(defaultTimeout)
 	}
 
-	if config.CFStagingResourceLimits.BuildCacheMB == 0 {
-		config.CFStagingResourceLimits.BuildCacheMB = defaultBuildCacheMB
+	if config.CFStagingResources.BuildCacheMB == 0 {
+		config.CFStagingResources.BuildCacheMB = defaultBuildCacheMB
 	}
 
 	return &config, nil

diff --git a/controllers/config/config_test.go b/controllers/config/config_test.go
@@ -35,7 +35,7 @@ var _ = Describe("LoadFromPath", func() {
 				DiskQuotaMB: 512,
 				Timeout:     tools.PtrTo(int64(30)),
 			},
-			CFStagingResourceLimits: config.CFStagingResourceLimits{
+			CFStagingResources: config.CFStagingResources{
 				BuildCacheMB: 1024,
 				DiskMB:       512,
 				MemoryMB:     2048,
@@ -73,7 +73,7 @@ var _ = Describe("LoadFromPath", func() {
 				DiskQuotaMB: 512,
 				Timeout:     tools.PtrTo(int64(30)),
 			},
-			CFStagingResourceLimits: config.CFStagingResourceLimits{
+			CFStagingResources: config.CFStagingResources{
 				BuildCacheMB: 1024,
 				DiskMB:       512,
 				MemoryMB:     2048,
@@ -125,11 +125,11 @@ var _ = Describe("LoadFromPath", func() {
 
 	When("the staging build cache size is not set", func() {
 		BeforeEach(func() {
-			cfg.CFStagingResourceLimits.BuildCacheMB = 0
+			cfg.CFStagingResources.BuildCacheMB = 0
 		})
 
 		It("uses the default", func() {
-			Expect(retConfig.CFStagingResourceLimits.BuildCacheMB).To(Equal(int64(2048)))
+			Expect(retConfig.CFStagingResources.BuildCacheMB).To(Equal(int64(2048)))
 		})
 	})
 })

diff --git a/helm/korifi/controllers/configmap.yaml b/helm/korifi/controllers/configmap.yaml
@@ -46,7 +46,7 @@ data:
     builderReadinessTimeout: {{ required "builderReadinessTimeout is required" .Values.kpackImageBuilder.builderReadinessTimeout }}
     containerRepositoryPrefix: {{ .Values.global.containerRepositoryPrefix | quote }}
     builderServiceAccount: kpack-service-account
-    cfStagingResourceLimits:
+    cfStagingResources:
       buildCacheMB: {{ .Values.api.lifecycle.stagingRequirements.buildCacheMB }}
       diskMB: {{ .Values.api.lifecycle.stagingRequirements.diskMB }}
       memoryMB: {{ .Values.api.lifecycle.stagingRequirements.memoryMB }}

diff --git a/helm/korifi/values.schema.json b/helm/korifi/values.schema.json
@@ -184,11 +184,11 @@
               "type": "object",
               "properties": {
                 "memoryMB": {
-                  "description": "Memory in MB for staging.",
+                  "description": "Memory request in MB for staging.",
                   "type": "integer"
                 },
                 "diskMB": {
-                  "description": "Ephemeral Disk limit in MB for staging apps.",
+                  "description": "Ephemeral Disk request in MB for staging apps.",
                   "type": "integer"
                 },
                 "buildCacheMB": {

diff --git a/helm/korifi/values.yaml b/helm/korifi/values.yaml
@@ -45,8 +45,8 @@ api:
     type: buildpack
     stack: cflinuxfs3
     stagingRequirements:
-      memoryMB: 1024
-      diskMB: 1024
+      memoryMB: 0
+      diskMB: 0
       buildCacheMB: 2048
 
   builderName: kpack-image-builder

diff --git a/kpack-image-builder/controllers/buildworkload_controller.go b/kpack-image-builder/controllers/buildworkload_controller.go
@@ -652,7 +652,7 @@ func (r *BuildWorkloadReconciler) reconcileKpackImage(
 		return err
 	}
 
-	cacheSize, err := resource.ParseQuantity(fmt.Sprintf("%dMi", r.controllerConfig.CFStagingResourceLimits.BuildCacheMB))
+	cacheSize, err := resource.ParseQuantity(fmt.Sprintf("%dMi", r.controllerConfig.CFStagingResources.BuildCacheMB))
 	if err != nil {
 		log.Info("failed to parse image cache size", "reason", err)
 		return err
@@ -700,14 +700,9 @@ func (r *BuildWorkloadReconciler) reconcileKpackImage(
 				},
 			},
 			Build: &buildv1alpha2.ImageBuild{
-				Services: buildWorkload.Spec.Services,
-				Env:      buildWorkload.Spec.Env,
-				Resources: corev1.ResourceRequirements{
-					Limits: corev1.ResourceList{
-						corev1.ResourceEphemeralStorage: *resource.NewScaledQuantity(int64(r.controllerConfig.CFStagingResourceLimits.DiskMB), resource.Mega),
-						corev1.ResourceMemory:           *resource.NewScaledQuantity(int64(r.controllerConfig.CFStagingResourceLimits.MemoryMB), resource.Mega),
-					},
-				},
+				Services:  buildWorkload.Spec.Services,
+				Env:       buildWorkload.Spec.Env,
+				Resources: GetBuildResources(r.controllerConfig.CFStagingResources.DiskMB, r.controllerConfig.CFStagingResources.MemoryMB),
 			},
 			Cache: &buildv1alpha2.ImageCacheConfig{
 				Volume: &buildv1alpha2.ImagePersistentVolumeCache{
@@ -759,6 +754,22 @@ func (r *BuildWorkloadReconciler) reconcileKpackImage(
 	return nil
 }
 
+func GetBuildResources(diskMB, memoryMB int64) corev1.ResourceRequirements {
+	resourceRequirements := corev1.ResourceRequirements{
+		Requests: map[corev1.ResourceName]resource.Quantity{},
+	}
+
+	if diskMB != 0 {
+		resourceRequirements.Requests[corev1.ResourceEphemeralStorage] = *resource.NewScaledQuantity(diskMB, resource.Mega)
+	}
+
+	if memoryMB != 0 {
+		resourceRequirements.Requests[corev1.ResourceMemory] = *resource.NewScaledQuantity(memoryMB, resource.Mega)
+	}
+
+	return resourceRequirements
+}
+
 func (r *BuildWorkloadReconciler) isResizable(ctx context.Context, log logr.Logger, scName string) (bool, error) {
 	scList := storagev1.StorageClassList{}
 	if listErr := r.k8sClient.List(ctx, &scList); listErr != nil {

diff --git a/kpack-image-builder/controllers/buildworkload_controller_test.go b/kpack-image-builder/controllers/buildworkload_controller_test.go
@@ -144,6 +144,44 @@ var _ = Describe("BuildWorkloadReconciler", func() {
 		}
 	})
 
+	Describe("GetBuildResources", func() {
+		var (
+			diskMB, memoryMB     int64
+			resourceRequirements corev1.ResourceRequirements
+		)
+
+		JustBeforeEach(func() {
+			resourceRequirements = controllers.GetBuildResources(diskMB, memoryMB)
+		})
+
+		It("does not set the resource requests by default", func() {
+			Expect(resourceRequirements.Limits).To(BeEmpty())
+			Expect(resourceRequirements.Requests).To(BeEmpty())
+		})
+
+		When("staging diskMB is configured", func() {
+			BeforeEach(func() {
+				diskMB = 1234
+			})
+
+			It("sets the ephemeralStorage resource request", func() {
+				Expect(resourceRequirements.Limits).To(BeEmpty())
+				Expect(resourceRequirements.Requests).To(HaveKeyWithValue(corev1.ResourceEphemeralStorage, *resource.NewScaledQuantity(diskMB, resource.Mega)))
+			})
+		})
+
+		When("staging memoryMB is configured", func() {
+			BeforeEach(func() {
+				memoryMB = 4321
+			})
+
+			It("sets the memory resource request", func() {
+				Expect(resourceRequirements.Limits).To(BeEmpty())
+				Expect(resourceRequirements.Requests).To(HaveKeyWithValue(corev1.ResourceMemory, *resource.NewScaledQuantity(memoryMB, resource.Mega)))
+			})
+		})
+	})
+
 	Describe("BuildWorkload initialization phase", func() {
 		JustBeforeEach(func() {
 			buildWorkload = buildWorkloadObject(buildWorkloadGUID, namespaceGUID, source, env, services, reconcilerName, buildpacks)
@@ -162,8 +200,8 @@ var _ = Describe("BuildWorkloadReconciler", func() {
 					g.Expect(kpackImage.Spec.Source.Registry.ImagePullSecrets).To(BeEquivalentTo(source.Registry.ImagePullSecrets))
 					g.Expect(kpackImage.Spec.Build.Env).To(Equal(env))
 					g.Expect(kpackImage.Spec.Build.Services).To(BeEquivalentTo(services))
-					g.Expect(kpackImage.Spec.Build.Resources.Limits.StorageEphemeral().String()).To(Equal(fmt.Sprintf("%dM", 2048)))
-					g.Expect(kpackImage.Spec.Build.Resources.Limits.Memory().String()).To(Equal(fmt.Sprintf("%dM", 1234)))
+					g.Expect(kpackImage.Spec.Build.Resources.Requests.StorageEphemeral().String()).To(Equal(fmt.Sprintf("%dM", 2048)))
+					g.Expect(kpackImage.Spec.Build.Resources.Requests.Memory().String()).To(Equal(fmt.Sprintf("%dM", 1234)))
 
 					g.Expect(kpackImage.Spec.Builder.Kind).To(Equal("ClusterBuilder"))
 					g.Expect(kpackImage.Spec.Builder.Name).To(Equal("cf-kpack-builder"))

diff --git a/kpack-image-builder/controllers/suite_test.go b/kpack-image-builder/controllers/suite_test.go
@@ -110,7 +110,7 @@ var _ = BeforeSuite(func() {
 		ClusterBuilderName:        "cf-kpack-builder",
 		ContainerRepositoryPrefix: "image/registry/tag",
 		BuilderServiceAccount:     "builder-service-account",
-		CFStagingResourceLimits: config.CFStagingResourceLimits{
+		CFStagingResources: config.CFStagingResources{
 			BuildCacheMB: 1024,
 			DiskMB:       2048,
 			MemoryMB:     1234,