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

This is a follow up of #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.

Author: sgaist

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: |

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 }}

helm-chart/binderhub/templates/container-builder/daemonset.yaml

@@ -1,7 +1,8 @@
-{{- if ne .Values.imageBuilderType "local" -}}
+{{- if ne .Values.imageBuilderType "host" -}}
 {{- $builderName := .Values.imageBuilderType -}}
-{{- $builder := index .Values $builderName -}}
+{{- $builder := (index .Values $builderName) -}}
 {{- $daemonset := $builder.daemonset -}}
+
 apiVersion: apps/v1
 kind: DaemonSet
 metadata:

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 }}

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
 

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