Skip to content
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

Simplify image-cleaner handling #1588

Merged
merged 4 commits into from
Dec 16, 2022

Conversation

sgaist
Copy link
Contributor

@sgaist sgaist commented Dec 15, 2022

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.

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.
They are part of the builder object, not the daemonset.
@sgaist sgaist force-pushed the refactor_image_cleaner_handling branch from 745465c to 4ccecaa Compare December 15, 2022 15:27
@manics
Copy link
Member

manics commented Dec 15, 2022

I've marked this as breaking since it should be considered in conjunction with #1531 so they're best treated as one larger change.

@sgaist
Copy link
Contributor Author

sgaist commented Dec 16, 2022

One practical question for NOTES.txt: I used 0.3.0 as "breaking" version number based on the current tag list from the repository but it will likely be something else. Is there a policy with regard to that kind of value and how to manage it ?

@manics
Copy link
Member

manics commented Dec 16, 2022

For this repo- not at the moment. I'm inclined to not specify the version since the current advice is to always run the latest main branch (this is what we do in https://github.com/jupyterhub/mybinder.org-deploy) so. You could perhaps link to the relevant PR(s) instead?

There's more ongoing refactoring (such as #1318 #1521) but when the repo is more stable we could consider a 1.0.0 release but that's a separate discussion!

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

LGTM, I added a commit changing a comment and updating syntax.

@consideRatio consideRatio merged commit 2ba938b into jupyterhub:main Dec 16, 2022
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Dec 16, 2022
jupyterhub/binderhub#1588 Merge pull request #1588 from sgaist/refactor_image_cleaner_handling
manics added a commit to manics/binderhub that referenced this pull request Dec 17, 2022
@sgaist sgaist deleted the refactor_image_cleaner_handling branch May 31, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants