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

Pin helm version to v2.6.2 #486

Merged
merged 2 commits into from
Feb 8, 2018
Merged

Pin helm version to v2.6.2 #486

merged 2 commits into from
Feb 8, 2018

Conversation

yuvipanda
Copy link
Collaborator

helm/helm#3275 has caused
serious amounts of downtime in the last week or so for
various deployments at Berkeley, so let's recommend people use
the last known version that did not have these issues.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
helm/helm#3275 has caused
serious amounts of downtime in the last week or so for
various deployments at Berkeley, so let's recommend people use
the last known version that did not have these issues.
@yuvipanda
Copy link
Collaborator Author

/cc @ryanlovett who was bitten by this again today.

Would be great if someone other than me could validate the commands!

CHANGELOG.md Outdated

```bash
curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get > install-helm.bash
bash install-helm.bash --version 2.6.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

The version needs a 'v' in front, v2.6.2.

@@ -15,11 +15,16 @@ terminal:

.. code:: bash

curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get | bash
curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get > install-helm.bash
bash install-helm.bash --version 2.6.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too, "v2.6.2"

CHANGELOG.md Outdated
```bash
curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get > install-helm.bash
bash install-helm.bash --version 2.6.2
helm install --upgrade
Copy link
Collaborator

@ryanlovett ryanlovett Feb 6, 2018

Choose a reason for hiding this comment

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

There is no '--upgrade' flag to helm install in 2.6.2. I think you meant helm init --upgrade ?

@yuvipanda
Copy link
Collaborator Author

Fixed, @ryanlovett!

Copy link
Collaborator

@ryanlovett ryanlovett left a comment

Choose a reason for hiding this comment

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

+1!

@choldgraf choldgraf merged commit a9fa124 into jupyterhub:master Feb 8, 2018
@choldgraf
Copy link
Member

looks great!

@consideRatio
Copy link
Member

consideRatio commented Feb 8, 2018

@yuvipanda @choldgraf @ryanlovett

About helm version v2.6.2

The Helm annotation allowing deletion of jobs through annotations like "helm.sh/hook-delete-policy": hook-succeeded that was introduced in Helm v2.7.0. This is relevant for the pre-pullers hooks, for every new upgrade, helm will duplicate:

One clusterrole, clusterrolebinding, role, rolebinding, serviceaccount, daemonset, and one job.

@yuvipanda
Copy link
Collaborator Author

yuvipanda commented Feb 8, 2018 via email

@manics
Copy link
Member

manics commented Feb 8, 2018

Helm has a --wait flag (though I haven't tried it) helm/helm#1820

Would one option be to make the pre-puller a separate chart, and get people to run two commands?

helm install jupyterhub-prepuller -f config.yaml --wait
helm install jupyterhub -f config.yaml

where the pre-puller could pull the image as an initContainer, then either switch to a pause process or run the continuous pre-puller? https://codefresh.io/kubernetes/single-use-daemonset-pattern-pre-pulling-images-kubernetes/

@consideRatio
Copy link
Member

@manics i have considered that also! If that is done, I think one might resolve some issues with helm upgrades where the hook resources are already existing and errors like that. Depends, I dont grasp this fully:

If you use helm to upgrade a DaemonSet as part of a hook, or not, will that behave differently when it comes the the " already exist" error? One does not get this error when we upgrade the hub deployment or a serviceaccount object etc... But we get it if the resource is a hook? Or do we? Hmmm...

All in all, I'm thinking that the use of Hooks can cause some issues, and we do it in order to order things before others. By having two charts, one updated after another, one could circumvent that. One might also introduce other issues, I know I don't know all the consequences.

@manics manics mentioned this pull request Aug 15, 2018
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants