From 9eb0e2090281d357d666d3036ac202746c9d653a Mon Sep 17 00:00:00 2001 From: Dejan Zele Pejchev Date: Tue, 26 Nov 2024 13:43:59 +0100 Subject: [PATCH 1/2] fix controller panics due to missing defaults Signed-off-by: Dejan Zele Pejchev --- api/install/v1alpha1/scheduler_webhook.go | 18 ++++++++++ .../install/armadaserver_controller.go | 2 +- internal/controller/install/common_helpers.go | 4 +++ .../controller/install/lookout_controller.go | 10 +++--- .../install/lookout_controller_test.go | 6 ++-- .../install/scheduler_controller.go | 35 ++++++++++--------- .../install/scheduler_controller_test.go | 6 ++-- 7 files changed, 52 insertions(+), 29 deletions(-) diff --git a/api/install/v1alpha1/scheduler_webhook.go b/api/install/v1alpha1/scheduler_webhook.go index b60d6799..c853a33e 100644 --- a/api/install/v1alpha1/scheduler_webhook.go +++ b/api/install/v1alpha1/scheduler_webhook.go @@ -83,6 +83,24 @@ func (r *Scheduler) Default() { } } + if r.Spec.Pruner == nil { + r.Spec.Pruner = &PrunerConfig{ + Schedule: "@hourly", + } + if r.Spec.Pruner.Resources == nil { + r.Spec.Pruner.Resources = &corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "cpu": resource.MustParse("300m"), + "memory": resource.MustParse("1Gi"), + }, + Requests: corev1.ResourceList{ + "cpu": resource.MustParse("200m"), + "memory": resource.MustParse("512Mi"), + }, + } + } + } + // prometheus if r.Spec.Prometheus != nil && r.Spec.Prometheus.Enabled { if r.Spec.Prometheus.ScrapeInterval == nil { diff --git a/internal/controller/install/armadaserver_controller.go b/internal/controller/install/armadaserver_controller.go index 3dd642bf..a94b0b6e 100644 --- a/internal/controller/install/armadaserver_controller.go +++ b/internal/controller/install/armadaserver_controller.go @@ -316,7 +316,7 @@ func createArmadaServerMigrationJobs(as *installv1alpha1.ArmadaServer, commonCon Containers: []corev1.Container{{ Name: "wait-for-pulsar", ImagePullPolicy: corev1.PullIfNotPresent, - Image: "alpine:3.16", + Image: defaultAlpineImage(), Args: []string{ "/bin/sh", "-c", diff --git a/internal/controller/install/common_helpers.go b/internal/controller/install/common_helpers.go index 0067c349..e7e6c7f8 100644 --- a/internal/controller/install/common_helpers.go +++ b/internal/controller/install/common_helpers.go @@ -866,3 +866,7 @@ func defaultDeploymentStrategy(maxUnavailable int32) appsv1.DeploymentStrategy { }, } } + +func defaultAlpineImage() string { + return "alpine:3.20" +} diff --git a/internal/controller/install/lookout_controller.go b/internal/controller/install/lookout_controller.go index 3dd6ebf1..7e20bb00 100644 --- a/internal/controller/install/lookout_controller.go +++ b/internal/controller/install/lookout_controller.go @@ -390,11 +390,11 @@ func createLookoutMigrationJob(lookout *installv1alpha1.Lookout, serviceAccountN Command: []string{ "/bin/sh", "-c", - `echo "Waiting for Postres..." + `echo "Waiting for Postgres..." while ! nc -z $PGHOST $PGPORT; do sleep 1 done - echo "Postres started!" + echo "Postgres started!" echo "Creating DB $PGDB if needed..." psql -v ON_ERROR_STOP=1 --username "$PGUSER" -c "CREATE DATABASE $PGDB" psql -v ON_ERROR_STOP=1 --username "$PGUSER" -c "GRANT ALL PRIVILEGES ON DATABASE $PGDB TO $PGUSER" @@ -503,15 +503,15 @@ func createLookoutCronJob(lookout *installv1alpha1.Lookout, serviceAccountName s SecurityContext: lookout.Spec.PodSecurityContext, InitContainers: []corev1.Container{{ Name: "lookout-db-pruner-db-wait", - Image: "alpine:3.10", + Image: defaultAlpineImage(), Command: []string{ "/bin/sh", "-c", - `echo "Waiting for Postres..." + `echo "Waiting for Postgres..." while ! nc -z $PGHOST $PGPORT; do sleep 1 done - echo "Postres started!"`, + echo "Postgres started!"`, }, Env: []corev1.EnvVar{ { diff --git a/internal/controller/install/lookout_controller_test.go b/internal/controller/install/lookout_controller_test.go index 113b9a24..96fcfbc5 100644 --- a/internal/controller/install/lookout_controller_test.go +++ b/internal/controller/install/lookout_controller_test.go @@ -829,15 +829,15 @@ func TestLookoutReconciler_CreateCronJob(t *testing.T) { InitContainers: []corev1.Container{ { Name: "lookout-db-pruner-db-wait", - Image: "alpine:3.10", + Image: defaultAlpineImage(), Command: []string{ "/bin/sh", "-c", - `echo "Waiting for Postres..." + `echo "Waiting for Postgres..." while ! nc -z $PGHOST $PGPORT; do sleep 1 done - echo "Postres started!"`, + echo "Postgres started!"`, }, Env: []corev1.EnvVar{ { diff --git a/internal/controller/install/scheduler_controller.go b/internal/controller/install/scheduler_controller.go index 810efc58..d79bc4d7 100644 --- a/internal/controller/install/scheduler_controller.go +++ b/internal/controller/install/scheduler_controller.go @@ -410,7 +410,7 @@ func newSchedulerMigrationJob(scheduler *installv1alpha1.Scheduler, serviceAccou Command: []string{ "/bin/sh", "-c", - `echo "Waiting for Postres..." + `echo "Waiting for Postgres..." while ! nc -z $PGHOST $PGPORT; do sleep 1 done @@ -494,18 +494,20 @@ func newSchedulerCronJob(scheduler *installv1alpha1.Scheduler, serviceAccountNam appConfigFlag, appConfigFilepath, } - if scheduler.Spec.Pruner.Args.Timeout != "" { - prunerArgs = append(prunerArgs, "--timeout", scheduler.Spec.Pruner.Args.Timeout) - } - if scheduler.Spec.Pruner.Args.Batchsize > 0 { - prunerArgs = append(prunerArgs, "--batchsize", fmt.Sprintf("%v", scheduler.Spec.Pruner.Args.Batchsize)) - } - if scheduler.Spec.Pruner.Args.ExpireAfter != "" { - prunerArgs = append(prunerArgs, "--expireAfter", scheduler.Spec.Pruner.Args.ExpireAfter) - } - prunerResources := corev1.ResourceRequirements{} - if scheduler.Spec.Pruner.Resources != nil { - prunerResources = *scheduler.Spec.Pruner.Resources + var prunerResources corev1.ResourceRequirements + if pruner := scheduler.Spec.Pruner; pruner != nil { + if pruner.Args.Timeout != "" { + prunerArgs = append(prunerArgs, "--timeout", scheduler.Spec.Pruner.Args.Timeout) + } + if pruner.Args.Batchsize > 0 { + prunerArgs = append(prunerArgs, "--batchsize", fmt.Sprintf("%v", scheduler.Spec.Pruner.Args.Batchsize)) + } + if pruner.Args.ExpireAfter != "" { + prunerArgs = append(prunerArgs, "--expireAfter", scheduler.Spec.Pruner.Args.ExpireAfter) + } + if pruner.Resources != nil { + prunerResources = *scheduler.Spec.Pruner.Resources + } } name := scheduler.Name + "-db-pruner" @@ -518,7 +520,6 @@ func newSchedulerCronJob(scheduler *installv1alpha1.Scheduler, serviceAccountNam Annotations: map[string]string{"checksum/config": GenerateChecksumConfig(scheduler.Spec.ApplicationConfig.Raw)}, }, Spec: batchv1.CronJobSpec{ - Schedule: scheduler.Spec.Pruner.Schedule, JobTemplate: batchv1.JobTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -543,15 +544,15 @@ func newSchedulerCronJob(scheduler *installv1alpha1.Scheduler, serviceAccountNam SecurityContext: scheduler.Spec.PodSecurityContext, InitContainers: []corev1.Container{{ Name: "scheduler-db-pruner-db-wait", - Image: "alpine:3.10", + Image: defaultAlpineImage(), Command: []string{ "/bin/sh", "-c", - `echo "Waiting for Postres..." + `echo "Waiting for Postgres..." while ! nc -z $PGHOST $PGPORT; do sleep 1 done - echo "Postres started!"`, + echo "Postgres started!"`, }, Env: []corev1.EnvVar{ { diff --git a/internal/controller/install/scheduler_controller_test.go b/internal/controller/install/scheduler_controller_test.go index d716173d..58f77fcf 100644 --- a/internal/controller/install/scheduler_controller_test.go +++ b/internal/controller/install/scheduler_controller_test.go @@ -1026,15 +1026,15 @@ func TestSchedulerReconciler_createSchedulerCronJob(t *testing.T) { InitContainers: []corev1.Container{ { Name: "scheduler-db-pruner-db-wait", - Image: "alpine:3.10", + Image: defaultAlpineImage(), Command: []string{ "/bin/sh", "-c", - `echo "Waiting for Postres..." + `echo "Waiting for Postgres..." while ! nc -z $PGHOST $PGPORT; do sleep 1 done - echo "Postres started!"`, + echo "Postgres started!"`, }, Env: []corev1.EnvVar{ { From 0bf9ad87124965cd15fa49d4c4ad1a39222a47e5 Mon Sep 17 00:00:00 2001 From: Dejan Zele Pejchev Date: Tue, 26 Nov 2024 16:37:19 +0100 Subject: [PATCH 2/2] add unit test for scheduler webhook default Signed-off-by: Dejan Zele Pejchev --- api/install/v1alpha1/scheduler_webhook.go | 4 - .../v1alpha1/scheduler_webhook_test.go | 79 +++++++++++++++++++ 2 files changed, 79 insertions(+), 4 deletions(-) create mode 100644 api/install/v1alpha1/scheduler_webhook_test.go diff --git a/api/install/v1alpha1/scheduler_webhook.go b/api/install/v1alpha1/scheduler_webhook.go index c853a33e..71e8ceb3 100644 --- a/api/install/v1alpha1/scheduler_webhook.go +++ b/api/install/v1alpha1/scheduler_webhook.go @@ -53,10 +53,6 @@ func (r *Scheduler) Default() { r.Spec.Image.Repository = "gresearch/armada-scheduler" } - if r.Spec.Replicas == nil { - r.Spec.Replicas = ptr.To[int32](1) - } - if r.Spec.Migrate == nil { r.Spec.Migrate = ptr.To[bool](true) } diff --git a/api/install/v1alpha1/scheduler_webhook_test.go b/api/install/v1alpha1/scheduler_webhook_test.go new file mode 100644 index 00000000..47784657 --- /dev/null +++ b/api/install/v1alpha1/scheduler_webhook_test.go @@ -0,0 +1,79 @@ +/* +Copyright 2023. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/utils/ptr" +) + +func TestScheduler_Default(t *testing.T) { + tests := []struct { + name string + input *Scheduler + assert func(*testing.T, *Scheduler) + }{ + { + name: "field is set and not overridden", + input: &Scheduler{ + Spec: SchedulerSpec{ + CommonSpecBase: CommonSpecBase{ + Image: Image{ + Repository: "test/armada-scheduler", + Tag: "latest", + }, + }, + }, + }, + assert: func(t *testing.T, s *Scheduler) { + assert.Equal(t, "test/armada-scheduler", s.Spec.Image.Repository) + }, + }, + { + name: "field is not set and gets defaulted", + input: &Scheduler{ + Spec: SchedulerSpec{ + Migrate: nil, + }, + }, + assert: func(t *testing.T, s *Scheduler) { + assert.Equal(t, ptr.To(true), s.Spec.Migrate) + }, + }, + { + name: "nothing is set and everything gets defaulted", + input: &Scheduler{ + Spec: SchedulerSpec{}, + }, + assert: func(t *testing.T, scheduler *Scheduler) { + assert.Equal(t, "gresearch/armada-scheduler", scheduler.Spec.Image.Repository) + assert.Equal(t, ptr.To[bool](true), scheduler.Spec.Migrate) + assert.Equal(t, GetDefaultSecurityContext(), scheduler.Spec.SecurityContext) + assert.Equal(t, GetDefaultPodSecurityContext(), scheduler.Spec.PodSecurityContext) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.input.Default() + tt.assert(t, tt.input) + }) + } +}