bjw-s / helm-charts

A collection of Helm charts
https://bjw-s.github.io/helm-charts/
Apache License 2.0
570 stars 106 forks source link

feat(common): omit replicas field in Deployment if null is given #134

Closed maurits-funda closed 1 year ago

maurits-funda commented 1 year ago

Description of the change

When using a HorizontalPodAutoscaler, the "replicas" field must be omitted. As per the Kubernetes documentation.

If a HorizontalPodAutoscaler (or any similar API for horizontal scaling) is managing scaling for a Deployment, don't set .spec.replicas.

This ensures the Deployment will not scale down when doing a (rolling) update of the Pods.

Changed

Omit replicas field in Deployment if controller.replicas = null.

Benefits

This allows the use of a HorizontalPodAutoscaler in combination with the Deployment that this chart creates.

Possible drawbacks

When specifying replicas: null or replicas: in the Kubernetes manifest, the default number of 1 Pod will be used. In case anyone is using this, the amount of replicas stays the same, but will no longer be updated by the Kubernetes manifest on subsequent applies.

Applicable issues

N/A

Additional information

None

Checklist

bjw-s commented 1 year ago

Thanks for this PR!! Just a quick question: how are you using this with HPA? I remember removing built-in support for HPA quite some time ago because it was a bit outside of the scope for (at the time) the k8s-at-home project

bjw-s commented 1 year ago

Also, I'm not sure what is going on with the CI there... I'll try and dig in to that a bit further

maurits-funda commented 1 year ago

Our HPA template looks like this:

{{- define "[OMITTED].class.native.horizontalPodAutoscaler" -}}
  {{- $resourceName := include "[OMITTED].lib.names.resourceName" $ }}
  {{- $values := hasKey . "ResourceValues" | ternary .ResourceValues .Values.horizontalPodAutoscaler -}}

  {{- /* Same as Deployment's metadata.name, see https://github.com/bjw-s/helm-charts/blob/main/charts/library/common/templates/classes/_deployment.tpl#L14 */}}
  {{- $deploymentName := include "bjw-s.common.lib.chart.names.fullname" $ -}}
---
apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: {{ $resourceName }}
  {{- with (merge ($values.labels | default dict) (include "bjw-s.common.lib.metadata.allLabels" $ | fromYaml)) }}
  labels: {{- toYaml . | nindent 4 }}
  {{- end }}
  {{- with (merge ($values.annotations | default dict) (include "bjw-s.common.lib.metadata.globalAnnotations" $ | fromYaml)) }}
  annotations: {{- toYaml . | nindent 4 }}
  {{- end }}
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: "{{ $deploymentName }}"
  minReplicas: {{ $values.minReplicas | default 1 }}
  maxReplicas: {{ $values.maxReplicas | default 10 }}
  metrics: {{ toYaml $values.metrics | nindent 4 }}
  behavior: {{ toYaml $values.behavior | nindent 4 }}
{{- end }}

We then set controller.replicas = null.