refactor(helm-chart): Simplify image-cleaner handling

This is a follow up of jupyterhub#1531. This patch refactors the creation and handling of the image cleaner daemonset. The current behaviour makes it so that if one doesn't disable the host cleaner explicitly when using dind (or pink), they would end up with two cleaners instead of one. The new behaviour avoids this issue by ensuring that only the container matching the selected imageBuilderType is created. Same goes for the volumes handling. In order to have consistent naming, the "local" imageBuilderType enumeration value is replaced with "host". It also deprecates the use of the imageCleaner.host.enabled parameter as you either want image cleanup or not.
sgaist · Dec 15, 2022 · f5601d2 · f5601d2
1 parent 3de27f2
commit f5601d2
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 29 deletions.
diff --git a/helm-chart/binderhub/schema.yaml b/helm-chart/binderhub/schema.yaml
@@ -353,8 +353,8 @@ properties:
 
   imageBuilderType:
     type: string
-    enum: ["local", "dind", "pink"]
-    default: "local"
+    enum: ["host", "dind", "pink"]
+    default: "host"
     description: |
       Selected image builder type
 
@@ -470,12 +470,12 @@ properties:
       host:
         type: object
         additionalProperties: false
-        required: [enabled, dockerSocket, dockerLibDir]
+        required: [dockerSocket, dockerLibDir]
         properties:
           enabled:
             type: boolean
             description: |
-              TODO
+              DEPRECATED: use imageCleaner.enabled if the cleaner shall not used.
           dockerSocket:
             type: string
             description: |

diff --git a/helm-chart/binderhub/templates/NOTES.txt b/helm-chart/binderhub/templates/NOTES.txt
@@ -190,6 +190,15 @@ config:
 {{- $breaking = print $breaking "\n\nimageBuilderType: dind" }}
 {{- end }}
 
+{{- if hasKey .Values.imageCleaner.host "enabled" }}
+{{- $breaking = print $breaking "\n\nCHANGED:" }}
+{{- $breaking = print $breaking "\n\nimageCleaner:" }}
+{{- $breaking = print $breaking "\n  host:" }}
+{{- $breaking = print $breaking "\n    enabled: true" }}
+{{- $breaking = print $breaking "\n\nas of version 0.3.0 is not used anymore." }}
+{{- $breaking = print $breaking "\n\nThe image cleaner is either disabled or adapted to the value of imageBuilderType." }}
+{{- end }}
+
 {{- if $breaking }}
 {{- fail (print $breaking_title $breaking) }}
 {{- end }}
diff --git a/helm-chart/binderhub/templates/image-cleaner.yaml b/helm-chart/binderhub/templates/image-cleaner.yaml
@@ -1,5 +1,7 @@
 {{- if .Values.imageCleaner.enabled -}}
 {{- $Values := .Values -}}
+{{- $imageBuilderType := $Values.imageBuilderType -}}
+{{- $imageCleaner := $Values.imageCleaner -}}
 apiVersion: apps/v1
 kind: DaemonSet
 metadata:
@@ -36,47 +38,43 @@ spec:
       serviceAccountName: {{ .Release.Name }}-image-cleaner
       {{- end }}
       containers:
-      {{- range $i, $kind := tuple "host" "dind" "pink" }}
-      {{- if or (and (eq $kind "dind") (eq $Values.imageBuilderType "dind")) (and (eq $kind "pink") (eq $Values.imageBuilderType "pink")) (and (eq $kind "host") $Values.imageCleaner.host.enabled) }}
-      - name: image-cleaner-{{ $kind }}
-        image: {{ $Values.imageCleaner.image.name }}:{{ $Values.imageCleaner.image.tag }}
-        {{- with $Values.imageCleaner.image.pullPolicy }}
+      - name: image-cleaner-{{ $imageBuilderType }}
+        image: {{ $imageCleaner.image.name }}:{{ $imageCleaner.image.tag }}
+        {{- with $imageCleaner.image.pullPolicy }}
         imagePullPolicy: {{ . }}
         {{- end }}
         volumeMounts:
-        - name: storage-{{ $kind }}
-          mountPath: /var/lib/{{ $kind }}
-        - name: socket-{{ $kind }}
+        - name: storage-{{ $imageBuilderType }}
+          mountPath: /var/lib/{{ $imageBuilderType }}
+        - name: socket-{{ $imageBuilderType }}
           mountPath: /var/run/docker.sock
         env:
         - name: DOCKER_IMAGE_CLEANER_NODE_NAME
           valueFrom:
             fieldRef:
               fieldPath: spec.nodeName
         - name: DOCKER_IMAGE_CLEANER_PATH_TO_CHECK
-          value: /var/lib/{{ $kind }}
+          value: /var/lib/{{ $imageBuilderType }}
         - name: DOCKER_IMAGE_CLEANER_DELAY_SECONDS
-          value: {{ $Values.imageCleaner.delay | quote }}
+          value: {{ $imageCleaner.delay | quote }}
         - name: DOCKER_IMAGE_CLEANER_THRESHOLD_TYPE
-          value: {{ $Values.imageCleaner.imageGCThresholdType | quote }}
+          value: {{ $imageCleaner.imageGCThresholdType | quote }}
         - name: DOCKER_IMAGE_CLEANER_THRESHOLD_HIGH
-          value: {{ $Values.imageCleaner.imageGCThresholdHigh | quote }}
+          value: {{ $imageCleaner.imageGCThresholdHigh | quote }}
         - name: DOCKER_IMAGE_CLEANER_THRESHOLD_LOW
-          value: {{ $Values.imageCleaner.imageGCThresholdLow | quote }}
-      {{- end }}
-      {{- end }}
+          value: {{ $imageCleaner.imageGCThresholdLow | quote }}
       terminationGracePeriodSeconds: 0
       volumes:
-      {{- if .Values.imageCleaner.host.enabled }}
+      {{- if eq $imageBuilderType "host" }}
       - name: storage-host
         hostPath:
-          path: {{ .Values.imageCleaner.host.dockerLibDir }}
+          path: {{ $imageCleaner.host.dockerLibDir }}
       - name: socket-host
         hostPath:
-          path: {{ .Values.imageCleaner.host.dockerSocket }}
+          path: {{ $imageCleaner.host.dockerSocket }}
           type: Socket
       {{- end }}
-      {{- if eq $Values.imageBuilderType "dind" }}
+      {{- if eq $imageBuilderType "dind" }}
       - name: storage-dind
         hostPath:
           path: {{ .Values.dind.hostLibDir }}
@@ -86,7 +84,7 @@ spec:
           path: {{ .Values.dind.hostSocketDir }}/docker.sock
           type: Socket
       {{- end }}
-      {{- if eq $Values.imageBuilderType "pink" }}
+      {{- if eq $imageBuilderType "pink" }}
       - name: storage-pink
         hostPath:
           path: {{ .Values.pink.hostStorageDir }}
@@ -96,5 +94,4 @@ spec:
           path: {{ .Values.pink.hostSocketDir }}/podman.sock
           type: Socket
       {{- end }}
-
 {{- end }}
diff --git a/helm-chart/binderhub/values.yaml b/helm-chart/binderhub/values.yaml
@@ -247,7 +247,7 @@ deployment:
     timeoutSeconds: 10
   labels: {}
 
-imageBuilderType: "local"
+imageBuilderType: "host"
 
 dind:
   initContainers: []
@@ -300,7 +300,6 @@ imageCleaner:
   imageGCThresholdLow: 60
   # cull images on the host docker as well as dind
   host:
-    enabled: true
     dockerSocket: /var/run/docker.sock
     dockerLibDir: /var/lib/docker
 

diff --git a/tools/templates/lint-and-validate-values.yaml b/tools/templates/lint-and-validate-values.yaml
@@ -86,7 +86,7 @@ deployment:
   livenessProbe: *probe
   labels: *labels
 
-imageBuilderType: local
+imageBuilderType: host
 
 dind:
   initContainers: &initContainers
@@ -128,7 +128,6 @@ imageCleaner:
   imageGCThresholdHigh: 2
   imageGCThresholdLow: 1
   host:
-    enabled: true
     dockerSocket: /var/run/docker.sock
     dockerLibDir: /var/lib/docker