From 3eaf229a0d0365d1d023866e6e02f8fcf1fc80f4 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Thu, 15 Dec 2022 14:15:47 +0100 Subject: [PATCH 1/4] refactor(helm-chart): Simplify image-cleaner handling 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. --- helm-chart/binderhub/schema.yaml | 8 ++-- helm-chart/binderhub/templates/NOTES.txt | 9 +++++ .../container-builder/daemonset.yaml | 5 ++- .../binderhub/templates/image-cleaner.yaml | 39 +++++++++---------- helm-chart/binderhub/values.yaml | 3 +- .../binderhub-chart-config.yaml | 4 +- tools/templates/lint-and-validate-values.yaml | 3 +- 7 files changed, 38 insertions(+), 33 deletions(-) diff --git a/helm-chart/binderhub/schema.yaml b/helm-chart/binderhub/schema.yaml index 5c38156ab..dc844cffa 100644 --- 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 index c468c0b56..73da374a0 100644 --- 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/container-builder/daemonset.yaml b/helm-chart/binderhub/templates/container-builder/daemonset.yaml index 7a0471089..ad33f8718 100644 --- a/helm-chart/binderhub/templates/container-builder/daemonset.yaml +++ b/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: diff --git a/helm-chart/binderhub/templates/image-cleaner.yaml b/helm-chart/binderhub/templates/image-cleaner.yaml index 4e8152764..f12935cf4 100644 --- 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,17 +38,15 @@ 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 @@ -54,29 +54,27 @@ spec: 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 index a912a1458..ca5c2badc 100644 --- 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/testing/k8s-binder-k8s-hub/binderhub-chart-config.yaml b/testing/k8s-binder-k8s-hub/binderhub-chart-config.yaml index 19027d868..17f398182 100644 --- a/testing/k8s-binder-k8s-hub/binderhub-chart-config.yaml +++ b/testing/k8s-binder-k8s-hub/binderhub-chart-config.yaml @@ -33,12 +33,12 @@ ingress: # used actively in our CI system. enabled: true -# No in cluster builder to test the creation/update of the image-cleaner DaemonSet +# No "in cluster" builder to test the creation/update of the image-cleaner DaemonSet # resource because it also requires us to setup a container registry to test # against which we haven't. We currently only test this through the use of # lint-and-validate-values.yaml and setting this value explicitly to make sure # our rendered templates are valid against a k8s api-server. -imageBuilderType: "local" +imageBuilderType: "host" # NOTE: This is a mirror of the jupyterhub section in # jupyterhub-chart-config.yaml in testing/local-binder-k8s-hub, keep these diff --git a/tools/templates/lint-and-validate-values.yaml b/tools/templates/lint-and-validate-values.yaml index 8e569accb..6f6319991 100644 --- 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 From 4ccecaabb3b9a0a2a54b916942214264555b52fa Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Thu, 15 Dec 2022 15:08:03 +0100 Subject: [PATCH 2/4] fix(container-builder): apply resources properly They are part of the builder object, not the daemonset. --- .../binderhub/templates/container-builder/daemonset.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helm-chart/binderhub/templates/container-builder/daemonset.yaml b/helm-chart/binderhub/templates/container-builder/daemonset.yaml index ad33f8718..5d6b990ba 100644 --- a/helm-chart/binderhub/templates/container-builder/daemonset.yaml +++ b/helm-chart/binderhub/templates/container-builder/daemonset.yaml @@ -47,9 +47,9 @@ spec: {{- with $daemonset.image.pullPolicy }} imagePullPolicy: {{ . }} {{- end }} - {{- with $daemonset.resources }} + {{- with $builder.resources }} resources: - {{- $daemonset.resources | toYaml | nindent 12 }} + {{- $builder.resources | toYaml | nindent 12 }} {{- end }} {{- if eq $builderName "dind" }} args: From 53493afe9a530b0614100463c9efd19a3aa1b93a Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Thu, 15 Dec 2022 16:49:19 +0100 Subject: [PATCH 3/4] refactor(image-cleaner): remove local variable use --- .../binderhub/templates/image-cleaner.yaml | 35 +++++++++---------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/helm-chart/binderhub/templates/image-cleaner.yaml b/helm-chart/binderhub/templates/image-cleaner.yaml index f12935cf4..5326ffce8 100644 --- a/helm-chart/binderhub/templates/image-cleaner.yaml +++ b/helm-chart/binderhub/templates/image-cleaner.yaml @@ -1,7 +1,4 @@ {{- if .Values.imageCleaner.enabled -}} -{{- $Values := .Values -}} -{{- $imageBuilderType := $Values.imageBuilderType -}} -{{- $imageCleaner := $Values.imageCleaner -}} apiVersion: apps/v1 kind: DaemonSet metadata: @@ -38,15 +35,15 @@ spec: serviceAccountName: {{ .Release.Name }}-image-cleaner {{- end }} containers: - - name: image-cleaner-{{ $imageBuilderType }} - image: {{ $imageCleaner.image.name }}:{{ $imageCleaner.image.tag }} - {{- with $imageCleaner.image.pullPolicy }} + - name: image-cleaner-{{ .Values.imageBuilderType }} + image: {{ .Values.imageCleaner.image.name }}:{{ .Values.imageCleaner.image.tag }} + {{- with .Values.imageCleaner.image.pullPolicy }} imagePullPolicy: {{ . }} {{- end }} volumeMounts: - - name: storage-{{ $imageBuilderType }} - mountPath: /var/lib/{{ $imageBuilderType }} - - name: socket-{{ $imageBuilderType }} + - name: storage-{{ .Values.imageBuilderType }} + mountPath: /var/lib/{{ .Values.imageBuilderType }} + - name: socket-{{ .Values.imageBuilderType }} mountPath: /var/run/docker.sock env: - name: DOCKER_IMAGE_CLEANER_NODE_NAME @@ -54,27 +51,27 @@ spec: fieldRef: fieldPath: spec.nodeName - name: DOCKER_IMAGE_CLEANER_PATH_TO_CHECK - value: /var/lib/{{ $imageBuilderType }} + value: /var/lib/{{ .Values.imageBuilderType }} - name: DOCKER_IMAGE_CLEANER_DELAY_SECONDS - value: {{ $imageCleaner.delay | quote }} + value: {{ .Values.imageCleaner.delay | quote }} - name: DOCKER_IMAGE_CLEANER_THRESHOLD_TYPE - value: {{ $imageCleaner.imageGCThresholdType | quote }} + value: {{ .Values.imageCleaner.imageGCThresholdType | quote }} - name: DOCKER_IMAGE_CLEANER_THRESHOLD_HIGH - value: {{ $imageCleaner.imageGCThresholdHigh | quote }} + value: {{ .Values.imageCleaner.imageGCThresholdHigh | quote }} - name: DOCKER_IMAGE_CLEANER_THRESHOLD_LOW - value: {{ $imageCleaner.imageGCThresholdLow | quote }} + value: {{ .Values.imageCleaner.imageGCThresholdLow | quote }} terminationGracePeriodSeconds: 0 volumes: - {{- if eq $imageBuilderType "host" }} + {{- if eq .Values.imageBuilderType "host" }} - name: storage-host hostPath: - path: {{ $imageCleaner.host.dockerLibDir }} + path: {{ .Values.imageCleaner.host.dockerLibDir }} - name: socket-host hostPath: - path: {{ $imageCleaner.host.dockerSocket }} + path: {{ .Values.imageCleaner.host.dockerSocket }} type: Socket {{- end }} - {{- if eq $imageBuilderType "dind" }} + {{- if eq .Values.imageBuilderType "dind" }} - name: storage-dind hostPath: path: {{ .Values.dind.hostLibDir }} @@ -84,7 +81,7 @@ spec: path: {{ .Values.dind.hostSocketDir }}/docker.sock type: Socket {{- end }} - {{- if eq $imageBuilderType "pink" }} + {{- if eq .Values.imageBuilderType "pink" }} - name: storage-pink hostPath: path: {{ .Values.pink.hostStorageDir }} From a93f8b5f0e3f33a95acd9b9d582b51ccd67d53d0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 16 Dec 2022 21:41:50 +0100 Subject: [PATCH 4/4] Update inline comment and a syntax detail --- helm-chart/binderhub/templates/container-builder/daemonset.yaml | 2 +- helm-chart/binderhub/values.yaml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/helm-chart/binderhub/templates/container-builder/daemonset.yaml b/helm-chart/binderhub/templates/container-builder/daemonset.yaml index 5d6b990ba..a40906b7a 100644 --- a/helm-chart/binderhub/templates/container-builder/daemonset.yaml +++ b/helm-chart/binderhub/templates/container-builder/daemonset.yaml @@ -1,6 +1,6 @@ {{- if ne .Values.imageBuilderType "host" -}} {{- $builderName := .Values.imageBuilderType -}} -{{- $builder := (index .Values $builderName) -}} +{{- $builder := index .Values $builderName -}} {{- $daemonset := $builder.daemonset -}} apiVersion: apps/v1 diff --git a/helm-chart/binderhub/values.yaml b/helm-chart/binderhub/values.yaml index ca5c2badc..3b2daf3c1 100644 --- a/helm-chart/binderhub/values.yaml +++ b/helm-chart/binderhub/values.yaml @@ -299,6 +299,7 @@ imageCleaner: imageGCThresholdHigh: 80 imageGCThresholdLow: 60 # cull images on the host docker as well as dind + # configuration to use if `imageBuilderType: host` is configured host: dockerSocket: /var/run/docker.sock dockerLibDir: /var/lib/docker