-
Notifications
You must be signed in to change notification settings - Fork 797
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
Helm chart option to switch prePuller to Always #3327
Comments
Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗 |
I think the common behavior in pre-puller logic is to omit a imagePullPolicy, which in k8s means to adopt a Always policy if the tag is latest, and otherwise adopt a IfNotPresent policy. So for :latest tags, this isnt an issue, but it is for mutable tags that isnt latest. Based on inspecting https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/main/jupyterhub/templates/image-puller/_helpers-daemonset.tpl it seems a bit unsystematic with how we set or don't set imagePullPolicy, but its also not obvious we want to use the same in the prePuller logic as in normal pods - they could rely on the already pulled stuff. Note that a reason to not use Always for images already pulled exists besides the small time required - its that additional lookups against the container registry can build up to a rate limit. I think using Always can be fine in prePuller, at worst we lookup an image too often with the hook prePuller when upgrading... Hmmm... But we also have some logic to avoid running that too often even if the image tags are unchanged... Hmmm... But then it doesn't help with Always anyhow...
If you have the continious pre-puller activated, you can without re-deploy do a restart of the daemonset's pods with a kubectl rollout command, assuming they have pullPolicy always or using the latest tag, you will get new images. |
Note that Always as imagePullPolicy in hook prePuller (runs during helm upgrades and image tags has changed) or continuous prePuller (runs once for each node - when new nodes are added!) will only improve the situation for when helm upgrade is run, as new nodes will need pulling no matter what (unless using persistent disks for the vms, could be clusters having such nodes actually). I have an open mind about making prePuller at least able to opt-in to Always as a imagePullPolicy, but it won't ensure you not need to wait for image pulling if a new tag arrived as the system isn't actively reacting to it. |
Proposed change
Currently prePuller is active by default and can be optionally switched off, as described in the docs. The default behavior of the pre-puller, however, is to examine the tag and only pull if the tag was not previously pulled (
ifNotPresent
). This means that the pre-puller is only actually functional when images are published using a new tag for each build (like a build date or hash) and not images that use other common conventions for tags (latest
, orubuntu:jammy
, orpython:3.9
).There's not much time saved by not attempting to pull an image vs pulling an image whose layers have already been pulled, since container architecture is already content-hash based anyway.
I would like to be able to configure my
prePuller
to "Always" pull, so that I can use non-immutable docker tags and still benefit from pre-pulling. In the current setup, I can already use non-immutable docker tags, and from the documentation it is not obvious that these cannot be pre-pulled consistently.Alternative options
Alternatively, we should note in the docs that pre-puller uses
ifNotPresent
logic on the tag names, rather than trying to pull.Or maybe there's another way to trigger a pull/test-run of all images?
Stick with current status quo: The admin can still use non-immutable tags, but must manually attempt to start and stop a server with each image type in the profile list).
... Probably other solutions?
Who would use this feature?
This would benefit users/admins who want to use rolling tags.
(Optional): Suggest a solution
My preferred suggestion would be to somehow add a configuration option to the pre-puller to toggle between default logic of
ifNotPresent
toAlways
The text was updated successfully, but these errors were encountered: