From 1e4c4486a8862d6421da433883ecb1a554862810 Mon Sep 17 00:00:00 2001 From: Andrew Costa Date: Thu, 13 Jul 2023 09:31:10 -0700 Subject: [PATCH] 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 --- controllers/config/config.go | 36 ++++++++-------- controllers/config/config_test.go | 8 ++-- helm/korifi/controllers/configmap.yaml | 2 +- .../controllers/buildworkload_controller.go | 29 +++++++++---- .../buildworkload_controller_test.go | 42 ++++++++++++++++++- kpack-image-builder/controllers/suite_test.go | 2 +- 6 files changed, 84 insertions(+), 35 deletions(-) diff --git a/controllers/config/config.go b/controllers/config/config.go index fcd234e4d..3af4b67d8 100644 --- 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 index 887daa34c..7068d5f5e 100644 --- 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 index 351678249..c8fe21947 100644 --- 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/kpack-image-builder/controllers/buildworkload_controller.go b/kpack-image-builder/controllers/buildworkload_controller.go index 5d7042a3c..369af49ce 100644 --- 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 index 46adaaf79..5d49df820 100644 --- 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 index 0601dc5e1..e42c7ddc5 100644 --- 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,