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

enable prePuller.hook.pullOnlyOnChanges by default #2179

Conversation

consideRatio
Copy link
Member

By enabling this feature by default, those with prePuller.hook.enabled=true will not need to wait ~10 seconds on hook-image-awaiter and hook-image-puller pods to startup and shut down if no new image has been configured.

In practice, this feature works by inspecting the rendered Helm template of the hook-image-puller DaemonSet for a change to anything besides labels since the last helm upgrade was made.

This feature also relies on the ability for us to lookup a value in a ConfigMap using the Helm 3 feature called lookup.

@yuvipanda
Copy link
Collaborator

Yay!

What's the minimum helm version for this? I had to update from 3.2 to get this to work - but not sure what's the minimum version

@consideRatio
Copy link
Member Author

Oh helm 3.1 introduced the lookup function.

@yuvipanda
Copy link
Collaborator

oh, hmm - not sure why I needed at least 3.3... Either way, do we mention that somewhere? Happy to merge this once we have that...

@consideRatio
Copy link
Member Author

consideRatio commented May 6, 2021

Hmmm i have not mentioned 3.3, i have no clear signal on whats required besides at least 3.1. we only test against different k8s versions, not helm versions.

If you could conclude 3.3 was required that is good for us to know. Note that you will always need to make one upgrade with the feature bugfix, and only after that this flag will make a difference. (A checksum must be updated correctly and stored in the hub configmap, which later can be found to be the same)

When you said "i needed" does it mean you got an error before, or you still found the image-puller to show up?

@yuvipanda
Copy link
Collaborator

@consideRatio yeah, with 3.2 I got an error in rendering the daemonset helper. I migraetd to latest (3.5) and it went away. Unfortunately I don't have those logs anymore

@consideRatio
Copy link
Member Author

consideRatio commented May 6, 2021

@yuvipanda I've concluded that 3.4.X doesn't work but 3.5.0 works. As upgrading to a recent helm 3 binary is easy, I suggest we accept a requirement on helm 3.5 and above. I'm making sure this is documented and we test against the minimally known supported helm version going onwards.

See #2181


It turns out 3.5.0 is now required because helm have given the built in context .Chart special treatment that caused issues after #2174 where my solution was to manipulate .Chart.Version to avoid modifications of a checksum.

I think it may be possible to retain support for a lower version by bashing my head against this a while but I suggest we simply require 3.5+ instead of supporting 3.x (where x < 5).

@yuvipanda
Copy link
Collaborator

+1 on a minimum helm 3.5 requirement.

@consideRatio consideRatio force-pushed the pr/enable-prePuller-pullOnlyOnChanges-by-default branch from effa963 to 5237e72 Compare May 12, 2021 15:15
@yuvipanda yuvipanda merged commit 28677d2 into jupyterhub:master May 12, 2021
@yuvipanda
Copy link
Collaborator

yay! :)

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request May 12, 2021
jupyterhub/zero-to-jupyterhub-k8s#2179 Merge pull request #2179 from consideRatio/pr/enable-prePuller-pullOnlyOnChanges-by-default
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants