fix: controller panics due to missing defaults (#340)

* fix controller panics due to missing defaults Signed-off-by: Dejan Zele Pejchev <[email protected]> * add unit test for scheduler webhook default Signed-off-by: Dejan Zele Pejchev <[email protected]> --------- Signed-off-by: Dejan Zele Pejchev <[email protected]>
armadaproject · Nov 26, 2024 · 122dba3 · 122dba3
1 parent 100a0e7
commit 122dba3
Show file tree

Hide file tree

Showing 8 changed files with 131 additions and 33 deletions.
diff --git 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)
 	}
@@ -83,6 +79,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/api/install/v1alpha1/scheduler_webhook_test.go 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)
+		})
+	}
+}
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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{
 										{
-Original file line number
+Diff line change
@@ Expand Up @@
     		},
     	}
     }
+    func defaultAlpineImage() string {
+    	return "alpine:3.20"
+    }