SKIP-1072 Allow 0 replicas for deployment (#246)

* Allow scaling down to 0 * Make skiperator hpa-compatible * Fix hashing for spec and labels * Fix labels and annotations * Remove replicas from assert * Use util function * Add tests for scaling and hpa * Add pdb test for 0 replicas * Fix test assert * Use non-cryptographic hashlib * Update controllers/application/deployment.go Co-authored-by: Even Holthe <[email protected]> * Remove unneeded stuff --------- Co-authored-by: Even Holthe <[email protected]>
kartverket · Jun 23, 2023 · 54c470d · 54c470d
1 parent 51e3acc
commit 54c470d
Show file tree

Hide file tree

Showing 24 changed files with 386 additions and 103 deletions.
diff --git a/api/v1alpha1/application.go b/api/v1alpha1/application.go
@@ -44,7 +44,7 @@ type ApplicationSpec struct {
 	//+kubebuilder:validation:Optional
 	Resources ResourceRequirements `json:"resources,omitempty"`
 	//+kubebuilder:validation:Optional
-	Replicas Replicas `json:"replicas,omitempty"`
+	Replicas *Replicas `json:"replicas,omitempty"`
 	//+kubebuilder:validation:Optional
 	Strategy Strategy `json:"strategy,omitempty"`
 
@@ -284,8 +284,16 @@ const (
 )
 
 func (a *Application) FillDefaultsSpec() {
-	a.Spec.Replicas.Min = max(1, a.Spec.Replicas.Min)
-	a.Spec.Replicas.Max = max(a.Spec.Replicas.Min, a.Spec.Replicas.Max)
+	if a.Spec.Replicas == nil {
+		a.Spec.Replicas = &Replicas{
+			Min: 2,
+			Max: 5,
+		}
+	} else if a.Spec.Replicas.Min == 0 && a.Spec.Replicas.Max == 0 {
+	} else {
+		a.Spec.Replicas.Min = max(1, a.Spec.Replicas.Min)
+		a.Spec.Replicas.Max = max(a.Spec.Replicas.Min, a.Spec.Replicas.Max)
+	}
 
 	if a.Spec.Replicas.TargetCpuUtilization == 0 {
 		a.Spec.Replicas.TargetCpuUtilization = 80

diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/controllers/application/deployment.go b/controllers/application/deployment.go
@@ -3,14 +3,15 @@ package applicationcontroller
 import (
 	"context"
 	"fmt"
-
 	skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
 	"github.com/kartverket/skiperator/pkg/util"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -20,10 +21,7 @@ const (
 	AnnotationKeyLinkPrefix = "link.argocd.argoproj.io/external-link"
 )
 
-func (r *ApplicationReconciler) reconcileDeployment(ctx context.Context, application *skiperatorv1alpha1.Application) (reconcile.Result, error) {
-	controllerName := "Deployment"
-	r.SetControllerProgressing(ctx, application, controllerName)
-
+func (r *ApplicationReconciler) defineDeployment(ctx context.Context, application *skiperatorv1alpha1.Application) (appsv1.Deployment, error) {
 	deployment := appsv1.Deployment{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Deployment",
@@ -35,96 +33,163 @@ func (r *ApplicationReconciler) reconcileDeployment(ctx context.Context, applica
 		},
 	}
 
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &deployment, func() error {
-		// Set application as owner of the deployment
-		err := ctrlutil.SetControllerReference(application, &deployment, r.GetScheme())
-		if err != nil {
-			r.SetControllerError(ctx, application, controllerName, err)
-			return err
-		}
+	skiperatorContainer := corev1.Container{
+		Name:            application.Name,
+		Image:           application.Spec.Image,
+		ImagePullPolicy: corev1.PullAlways,
+		Command:         application.Spec.Command,
+		SecurityContext: &corev1.SecurityContext{
+			Privileged:               util.PointTo(false),
+			AllowPrivilegeEscalation: util.PointTo(false),
+			ReadOnlyRootFilesystem:   util.PointTo(true),
+			RunAsUser:                util.PointTo(util.SkiperatorUser),
+			RunAsGroup:               util.PointTo(util.SkiperatorUser),
+		},
+		Ports:   getContainerPorts(application),
+		EnvFrom: getEnvFrom(application.Spec.EnvFrom),
+		Resources: corev1.ResourceRequirements{
+			Limits:   application.Spec.Resources.Limits,
+			Requests: application.Spec.Resources.Requests,
+		},
+		Env:                      application.Spec.Env,
+		ReadinessProbe:           getProbe(application.Spec.Readiness),
+		LivenessProbe:            getProbe(application.Spec.Liveness),
+		StartupProbe:             getProbe(application.Spec.Startup),
+		TerminationMessagePath:   corev1.TerminationMessagePathDefault,
+		TerminationMessagePolicy: corev1.TerminationMessagePolicy("File"),
+	}
 
-		r.SetLabelsFromApplication(ctx, &deployment, *application)
-		util.SetCommonAnnotations(&deployment)
-
-		skiperatorContainer := corev1.Container{
-			Name:            application.Name,
-			Image:           application.Spec.Image,
-			ImagePullPolicy: corev1.PullAlways,
-			Command:         application.Spec.Command,
-			SecurityContext: &corev1.SecurityContext{
-				Privileged:               util.PointTo(false),
-				AllowPrivilegeEscalation: util.PointTo(false),
-				ReadOnlyRootFilesystem:   util.PointTo(true),
-				RunAsUser:                util.PointTo(util.SkiperatorUser),
-				RunAsGroup:               util.PointTo(util.SkiperatorUser),
-			},
-			Ports:   getContainerPorts(application),
-			EnvFrom: getEnvFrom(application.Spec.EnvFrom),
-			Resources: corev1.ResourceRequirements{
-				Limits:   application.Spec.Resources.Limits,
-				Requests: application.Spec.Resources.Requests,
-			},
-			Env:            application.Spec.Env,
-			ReadinessProbe: getProbe(application.Spec.Readiness),
-			LivenessProbe:  getProbe(application.Spec.Liveness),
-			StartupProbe:   getProbe(application.Spec.Startup),
-		}
+	var err error
 
-		podVolumes, containerVolumeMounts := getContainerVolumeMountsAndPodVolumes(application)
-		podVolumes, containerVolumeMounts, err = r.appendGCPVolumeMount(application, ctx, &skiperatorContainer, containerVolumeMounts, podVolumes)
-		if err != nil {
-			r.SetControllerError(ctx, application, controllerName, err)
-			return err
-		}
-		skiperatorContainer.VolumeMounts = containerVolumeMounts
+	podVolumes, containerVolumeMounts := getContainerVolumeMountsAndPodVolumes(application)
+	podVolumes, containerVolumeMounts, err = r.appendGCPVolumeMount(application, ctx, &skiperatorContainer, containerVolumeMounts, podVolumes)
+	if err != nil {
+		r.SetControllerError(ctx, application, controllerName, err)
+		return deployment, err
+	}
+	skiperatorContainer.VolumeMounts = containerVolumeMounts
 
-		labels := util.GetApplicationSelector(application.Name)
+	labels := util.GetApplicationSelector(application.Name)
 
-		deployment.Spec = appsv1.DeploymentSpec{
-			Selector: &metav1.LabelSelector{MatchLabels: labels},
-			Replicas: getReplicasFromAppSpec(application.Spec.Replicas.Min),
-			Strategy: appsv1.DeploymentStrategy{
-				Type:          appsv1.DeploymentStrategyType(application.Spec.Strategy.Type),
-				RollingUpdate: getRollingUpdateStrategy(application.Spec.Strategy.Type),
+	deployment.Spec = appsv1.DeploymentSpec{
+		Selector: &metav1.LabelSelector{MatchLabels: labels},
+		Strategy: appsv1.DeploymentStrategy{
+			Type:          appsv1.DeploymentStrategyType(application.Spec.Strategy.Type),
+			RollingUpdate: getRollingUpdateStrategy(application.Spec.Strategy.Type),
+		},
+		Template: corev1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: labels,
+				Annotations: map[string]string{
+					"argocd.argoproj.io/sync-options": "Prune=false",
+					"prometheus.io/scrape":            "true",
+				},
 			},
-			Template: corev1.PodTemplateSpec{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels: labels,
-					Annotations: map[string]string{
-						"argocd.argoproj.io/sync-options": "Prune=false",
-						"prometheus.io/scrape":            "true",
-					},
+			Spec: corev1.PodSpec{
+				Containers: []corev1.Container{
+					skiperatorContainer,
 				},
-				Spec: corev1.PodSpec{
-					Containers: []corev1.Container{
-						skiperatorContainer,
-					},
 
-					// TODO: Make this as part of operator in a safe way
-					ImagePullSecrets: []corev1.LocalObjectReference{{Name: "github-auth"}},
-					SecurityContext: &corev1.PodSecurityContext{
-						SupplementalGroups: []int64{util.SkiperatorUser},
-						FSGroup:            util.PointTo(util.SkiperatorUser),
-						SeccompProfile: &corev1.SeccompProfile{
-							Type: corev1.SeccompProfileTypeRuntimeDefault,
-						},
+				// TODO: Make this as part of operator in a safe way
+				ImagePullSecrets: []corev1.LocalObjectReference{{Name: "github-auth"}},
+				SecurityContext: &corev1.PodSecurityContext{
+					SupplementalGroups: []int64{util.SkiperatorUser},
+					FSGroup:            util.PointTo(util.SkiperatorUser),
+					SeccompProfile: &corev1.SeccompProfile{
+						Type: corev1.SeccompProfileTypeRuntimeDefault,
 					},
-					ServiceAccountName: application.Name,
-					Volumes:            podVolumes,
-					PriorityClassName:  fmt.Sprintf("skip-%s", application.Spec.Priority),
 				},
+				ServiceAccountName: application.Name,
+				// The resulting kubernetes object includes the ServiceAccount field, and thus it's required in order
+				// to not create a diff for the hash of existing and wanted spec
+				DeprecatedServiceAccount:      application.Name,
+				Volumes:                       podVolumes,
+				PriorityClassName:             fmt.Sprintf("skip-%s", application.Spec.Priority),
+				RestartPolicy:                 corev1.RestartPolicyAlways,
+				TerminationGracePeriodSeconds: util.PointTo(int64(corev1.DefaultTerminationGracePeriodSeconds)),
+				DNSPolicy:                     corev1.DNSClusterFirst,
+				SchedulerName:                 corev1.DefaultSchedulerName,
 			},
-			RevisionHistoryLimit: util.PointTo(int32(2)),
-		}
+		},
+		RevisionHistoryLimit:    util.PointTo(int32(2)),
+		ProgressDeadlineSeconds: util.PointTo(int32(600)),
+	}
+
+	// Setting replicas to 0 when skiperator manifest specifies min/max to 0
+	if shouldScaleToZero(application.Spec.Replicas.Min, application.Spec.Replicas.Max) {
+		deployment.Spec.Replicas = util.PointTo(int32(0))
+	}
 
-		// add an external link to argocd
-		ingresses := application.Spec.Ingresses
-		if len(ingresses) > 0 {
-			deployment.ObjectMeta.Annotations[AnnotationKeyLinkPrefix] = fmt.Sprintf("https://%s", ingresses[0])
+	r.SetLabelsFromApplication(ctx, &deployment, *application)
+	util.SetCommonAnnotations(&deployment)
+
+	// add an external link to argocd
+	ingresses := application.Spec.Ingresses
+	if len(ingresses) > 0 {
+		deployment.ObjectMeta.Annotations[AnnotationKeyLinkPrefix] = fmt.Sprintf("https://%s", ingresses[0])
+	}
+
+	// Set application as owner of the deployment
+	err = ctrlutil.SetControllerReference(application, &deployment, r.GetScheme())
+	if err != nil {
+		r.SetControllerError(ctx, application, controllerName, err)
+		return deployment, err
+	}
+
+	return deployment, nil
+}
+
+func (r *ApplicationReconciler) reconcileDeployment(ctx context.Context, application *skiperatorv1alpha1.Application) (reconcile.Result, error) {
+	controllerName := "Deployment"
+	r.SetControllerProgressing(ctx, application, controllerName)
+
+	deployment := appsv1.Deployment{}
+	deploymentDefinition, err := r.defineDeployment(ctx, application)
+
+	err = r.GetClient().Get(ctx, types.NamespacedName{Name: application.Name, Namespace: application.Namespace}, &deployment)
+	if err != nil {
+		if errors.IsNotFound(err) {
+			r.GetRecorder().Eventf(
+				application,
+				corev1.EventTypeNormal, "NotFound",
+				"Deployment resource for application %s not found. Creating deployment",
+				application.Name,
+			)
+			err = r.GetClient().Create(ctx, &deploymentDefinition)
+			if err != nil {
+				r.SetControllerError(ctx, application, controllerName, err)
+				return reconcile.Result{}, err
+			}
+		} else {
+			r.SetControllerError(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+	} else {
+		if !shouldScaleToZero(application.Spec.Replicas.Min, application.Spec.Replicas.Max) {
+			// Ignore replicas set by HPA when checking diff
+			if int32(*deployment.Spec.Replicas) > 0 {
+				deployment.Spec.Replicas = nil
+			}
 		}
 
-		return nil
-	})
+		deploymentHash := util.GetHashForStructs([]interface{}{
+			&deployment.Spec,
+			&deployment.Labels,
+		})
+		deploymentDefinitionHash := util.GetHashForStructs([]interface{}{
+			&deploymentDefinition.Spec,
+			&deploymentDefinition.Labels,
+		})
+
+		if deploymentHash != deploymentDefinitionHash {
+			patch := client.MergeFrom(deployment.DeepCopy())
+			err = r.GetClient().Patch(ctx, &deploymentDefinition, patch)
+			if err != nil {
+				r.SetControllerError(ctx, application, controllerName, err)
+				return reconcile.Result{}, err
+			}
+		}
+	}
 
 	r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
 
@@ -321,6 +386,7 @@ func getContainerPorts(application *skiperatorv1alpha1.Application) []corev1.Con
 		{
 			Name:          "main",
 			ContainerPort: int32(application.Spec.Port),
+			Protocol:      corev1.ProtocolTCP,
 		},
 	}
 
@@ -340,15 +406,16 @@ func getRollingUpdateStrategy(updateStrategy string) *appsv1.RollingUpdateDeploy
 		return nil
 	}
 
-	return &appsv1.RollingUpdateDeployment{}
+	return &appsv1.RollingUpdateDeployment{
+		// Fill with defaults
+		MaxUnavailable: &intstr.IntOrString{Type: intstr.String, StrVal: "25%"},
+		MaxSurge:       &intstr.IntOrString{Type: intstr.String, StrVal: "25%"},
+	}
 }
 
-func getReplicasFromAppSpec(appReplicas uint) *int32 {
-	var replicas = int32(appReplicas)
-	if replicas == 0 {
-		minReplicas := int32(1)
-		return &minReplicas
+func shouldScaleToZero(minReplicas uint, maxReplicas uint) bool {
+	if minReplicas == 0 && maxReplicas == 0 {
+		return true
 	}
-
-	return &replicas
+	return false
 }
diff --git a/controllers/application/horizontal_pod_autoscaler.go b/controllers/application/horizontal_pod_autoscaler.go
@@ -2,11 +2,11 @@ package applicationcontroller
 
 import (
 	"context"
-
 	skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
 	"github.com/kartverket/skiperator/pkg/util"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -16,6 +16,17 @@ func (r *ApplicationReconciler) reconcileHorizontalPodAutoscaler(ctx context.Con
 	r.SetControllerProgressing(ctx, application, controllerName)
 
 	horizontalPodAutoscaler := autoscalingv2.HorizontalPodAutoscaler{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}}
+	if shouldScaleToZero(application.Spec.Replicas.Min, application.Spec.Replicas.Max) {
+		err := r.GetClient().Delete(ctx, &horizontalPodAutoscaler)
+		err = client.IgnoreNotFound(err)
+		if err != nil {
+			r.SetControllerError(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, nil)
+		return reconcile.Result{}, nil
+	}
+
 	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &horizontalPodAutoscaler, func() error {
 		// Set application as owner of the horizontal pod autoscaler
 		err := ctrlutil.SetControllerReference(application, &horizontalPodAutoscaler, r.GetScheme())

diff --git a/controllers/application/service.go b/controllers/application/service.go
@@ -30,6 +30,9 @@ func (r *ApplicationReconciler) reconcileService(ctx context.Context, applicatio
 
 		// ServiceMonitor requires labels to be set on service to select it
 		labels := service.GetLabels()
+		if len(labels) == 0 {
+			labels = make(map[string]string)
+		}
 		labels["app"] = application.Name
 		service.SetLabels(labels)
 

diff --git a/generate.go b/generate.go
@@ -1,4 +1,5 @@
 package root
 
-//go:generate controller-gen crd paths=./api/...
+//go:generate controller-gen crd paths=./...
+//go:generate controller-gen object paths=./...
 //go:generate controller-gen rbac:roleName=skiperator paths=./...