- 
                Notifications
    
You must be signed in to change notification settings  - Fork 401
 
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.
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
base: main
Are you sure you want to change the base?
Conversation
| # cull images until it drops below 60% | ||
| imageGCThresholdHigh: 80 | ||
| imageGCThresholdLow: 60 | ||
| # cull images on the host docker as well as dind | 
There was a problem hiding this comment.
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
| - name: storage-{{ $builderName }} | ||
| hostPath: | ||
| path: {{ eq $builderName "dind" | ternary $builder.hostLibDir $builder.hostStorageDir }} | ||
| type: DirectoryOrCreate | 
There was a problem hiding this comment.
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.
BinderHub/K8s has a long standing bug where sometimes the docker socket is created as a host directory.
2a5fefc    to
    9f57d20      
    Compare
  
    | type: boolean | ||
| description: | | ||
| DEPRECATED: use imageCleaner.enabled if the cleaner shall not used. | ||
| dockerSocket: | 
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes nice!
BinderHub/K8s has a long standing bug where sometimes the docker socket is created as a host directory.
Closes #1230
This is a breaking change since
imageCleaner.host.dockerSocketis replaced byimageCleaner.host.dockerSocket{Dir,Name}.However given that dind has been the recommended way to run BinderHub for a quite a while I don't think it's worth the complexity of adding the Helm template logic to handle the old case.