Skip to content

Image-cleaner: mount docker dir instead of socket #2030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 4, 2025
+35 −14
Merged

Image-cleaner: mount docker dir instead of socket #2030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9f3ef0
Image-cleaner: mount docker dir instead of socket
manics Oct 24, 2025
9f57d20
Add note about breaking change
manics Oct 24, 2025
a7e36d9
Add breaking changes to NOTES.txt
manics Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ make it easier to track what has changed over time_

## Breaking changes

### `imageCleaner.host.dockerSocket` is replaced by `dockerSocketDir` and `dockerSocketName`

`imageCleaner.host.dockerSocket` is replaced by `imageCleaner.host.dockerSocketDir` and `imageCleaner.host.dockerSocketHost` [#2030](https://github.com/jupyterhub/binderhub/pull/2030).
This gives us more control over how the socket is mounted to work around some bugs.

### `binderhub_config.py` is mounted at runtime

The `binderhub_config.py` file is now mounted at runtime instead of being built into the BinderHub image
Expand Down
12 changes: 10 additions & 2 deletions helm-chart/binderhub/schema.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ properties:
host:
type: object
additionalProperties: false
required: [dockerSocket, dockerLibDir]
required: [dockerSocketDir, dockerSocketName, dockerLibDir]
properties:
enabled:
type: boolean
Expand All @@ -504,7 +504,15 @@ properties:
dockerSocket:
Copy link
Member

@consideRatio consideRatio Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to help people make a transition, this entry could remain allowed, and then if its ovserved in NOTES.txt, we would use the fail template command to error with a message.

We are already erroring if its specified, so this would just be a help to transition

Copy link
Member

@consideRatio consideRatio Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type would be allowed to be null as well then, and then we would look for it to be set.

I recall z2jh have some deprecation and breaking change message template ready if you want to go for it

Copy link
Member Author

@manics manics Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done!

Shouldn't the hasKey check mean we don't need to make it nullable?`

Copy link
Member

@consideRatio consideRatio Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes nice!

type: string
description: |
TODO
REMOVED: use imageCleaner.host.dockerSocketDir and imageCleaner.host.dockerSocketHost instead.
dockerSocketDir:
type: string
description: |
Host directory containing the docker socket.
dockerSocketName:
type: string
description: |
Filename of the docker socket.
dockerLibDir:
type: string
description: |
Expand Down
4 changes: 4 additions & 0 deletions helm-chart/binderhub/templates/NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ config:
{{- $breaking = print $breaking "\n\nThe image cleaner is either disabled or adapted to the value of imageBuilderType." }}
{{- end }}

{{- if hasKey .Values.imageCleaner.host "dockerSocket" }}
{{- $breaking = print $breaking "\n\nCHANGED: imageCleaner.host.dockerSocket has been removed, use imageCleaner.host.dockerSocketDir and imageCleaner.host.dockerSocketHost instead" }}
{{- end }}

{{- if $breaking }}
{{- fail (print $breaking_title $breaking) }}
{{- end }}
20 changes: 11 additions & 9 deletions helm-chart/binderhub/templates/image-cleaner.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ spec:
volumeMounts:
- name: storage-{{ $builderName }}
mountPath: /var/lib/{{ $builderName }}
- name: socket-{{ $builderName }}
mountPath: /var/run/docker.sock
- name: socketdir-{{ $builderName }}
mountPath: /var/run
env:
{{- if .Values.imageCleaner.cordon }}
- name: DOCKER_IMAGE_CLEANER_NODE_NAME
Expand All @@ -68,6 +68,8 @@ spec:
value: {{ .Values.imageCleaner.imageGCThresholdHigh | quote }}
- name: DOCKER_IMAGE_CLEANER_THRESHOLD_LOW
value: {{ .Values.imageCleaner.imageGCThresholdLow | quote }}
- name: DOCKER_HOST
value: unix:///var/run/{{ eq $builderName "host" | ternary .Values.imageCleaner.host.dockerSocketName $builder.hostSocketName }}
{{- with .Values.imageCleaner.extraEnv }}
{{- include "jupyterhub.extraEnv" . | nindent 8 }}
{{- end }}
Expand All @@ -77,19 +79,19 @@ spec:
- name: storage-host
hostPath:
path: {{ .Values.imageCleaner.host.dockerLibDir }}
- name: socket-host
- name: socketdir-host
hostPath:
path: {{ .Values.imageCleaner.host.dockerSocket }}
type: Socket
path: {{ .Values.imageCleaner.host.dockerSocketDir }}
type: Directory
{{- end }}
{{- if or (eq $builderName "dind") (eq $builderName "pink") }}
- name: storage-{{ $builderName }}
hostPath:
path: {{ eq $builderName "dind" | ternary $builder.hostLibDir $builder.hostStorageDir }}
type: DirectoryOrCreate
Copy link
Member Author

@manics manics Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image-cleaner should never create Docker directories, that should be handled by Docker.

- name: socket-{{ $builderName }}
type: Directory
- name: socketdir-{{ $builderName }}
hostPath:
path: {{ $builder.hostSocketDir }}/{{ $builder.hostSocketName }}
type: Socket
path: {{ $builder.hostSocketDir }}
type: Directory
{{- end }}
{{- end }}
5 changes: 3 additions & 2 deletions helm-chart/binderhub/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,11 @@ imageCleaner:
# cull images until it drops below 60%
imageGCThresholdHigh: 80
imageGCThresholdLow: 60
# cull images on the host docker as well as dind
Copy link
Member Author

@manics manics Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's ever done both

# cull images on the host docker
# configuration to use if `imageBuilderType: host` is configured
host:
dockerSocket: /var/run/docker.sock
dockerSocketDir: /var/run
dockerSocketName: docker.sock
dockerLibDir: /var/lib/docker

ingress:
Expand Down
3 changes: 2 additions & 1 deletion tools/templates/lint-and-validate-values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ imageCleaner:
imageGCThresholdHigh: 2
imageGCThresholdLow: 1
host:
dockerSocket: /var/run/docker.sock
dockerSocketDir: /var/run
dockerSocketName: docker.sock
dockerLibDir: /var/lib/docker

ingress:
Expand Down
Loading