Mybinder.org-deploy: Upgrading to helm 3

[minrk]

At some point, we are going to need to migrate our deployments to helm 3. Helm 3 has some changes about how it stores state and how it runs, so we'll need to be careful about doing upgrades to avoid losing stuff.

There's some documentation on how to do this successfully, and it might be better to do the migration manually, rather than on CI, once the update of CI is ready, but before it is merged.

[minrk]

refs from https://helm.sh/docs/topics/v2_v3_migration

• helm 2to3 plugin https://github.com/helm/helm-2to3
• https://helm.sh/blog/migrate-from-helm-v2-to-helm-v3

It's possible that we can run a manual 2to3 pass on each federation member and then "just" swap helm 2 for 3 and be off. Going to need some testing first, of course.

[choldgraf]

I believe that @consideRatio recently did some work in upgrading to helm 3 for JupyterHubs in general, perhaps he can help out? (apologies if I am mis-remembering that)

[sgibson91]

After upgrading Hub23 yesterday, I'm happy to give moving staging a go tomorrow

[sgibson91]

Helm migration progress

| Federation member | 2to3 complete? | CI updated? |
| :--- | :---: | :---: |
| GKE staging | ✅ | ✅ PRs https://github.com/jupyterhub/mybinder.org-deploy/pull/1667 and https://github.com/jupyterhub/mybinder.org-deploy/pull/1670 |
| GKE prod | ✅ | ✅ PR #1738 |
| Turing | :white_check_mark: | :white_check_mark: #1690 |
| OVH | ✅ | ✅ PR #1718 |

*I assume GESIS will handle their deployment (if they haven't already done so)

[sgibson91]

In upgrading staging to helm3, we came across issues. --force in helm3 produces different behaviour to helm2 and immutable objects can no longer be updated, they have to be deleted and recreated. Hopefully this is a one-time thing that only needs to happen at the point of switching over/updating CI

Copy-pasting conversation from gitter

Sarah Gibson @sgibson91 Oct 26 14:20
Ok, seems the label selector is no having issues when trying to deploy to staging
https://github.com/jupyterhub/mybinder.org-deploy/pull/1670/checks?check_run_id=1309273607#step:14:60

Min RK @minrk 14:21
We used --force to deal with that before, did something change wrt force?

Sarah Gibson @sgibson91 Oct 26 14:21
Yes, @consideRatio advised not to use it with helm3

Min RK @minrk 14:22
But...we need it?
What's the alternative?
Looks like we need --recreate
Ah, looks like helm 3 has a separate flag to handle why we used force with helm 2
that's --recreate

Erik Sundell @consideRatio 14:24
@minrk Hmmmm, I don't find that in my helm3 as part of the helm upgrade --help list

Min RK @minrk 14:25
ok, I'm confused. Let's just put back --force, since it doesn't work without it
What has helm 3 changed about force that means we should stop using it?

Erik Sundell @consideRatio 14:25
--force force resource updates through a replacement strategy
I'm not sure what happens when using --force with Helm3, will it replace a PVC that there is no change to, or, will it replace it if there is ANY change to it even though it was only a label change that was acceptable for example?

Min RK @minrk 14:26
I assume it's the same as helm 2, which means recreate when immutable fields are changed. Where did you see that it's more destructive than helm 2?

Erik Sundell @consideRatio 14:27
I'll find my way back to the issues, but this is my takeaway after reading lots of long threads about the topic.

I believe helm3 doesn't have --recreate

Min RK @minrk 14:27
ok, I must have misread an Issue

Erik Sundell @consideRatio 14:27
helm/helm#7811

Min RK @minrk 14:31
ok, then I don't understand what to do because we absolutely require the helm2 --force behavior and I don't understand if or how the helm 3 --force behavior is different.
I need to run and pickup at barnehage, bit will check in again tomorrow

Sarah Gibson @sgibson91 Oct 26 14:32
Np, thanks for your help today

Min RK @minrk 14:33
Thank you both!

Erik Sundell @consideRatio 14:34
Cya Min! Thank you for your help earlier btw!

helm3 --force is like kubectl apply --overwrite, while the previous helm2 --force was a bit different somehow. I'll look into how problematic --overwrite is or isn't

  --overwrite=true: Automatically resolve conflicts between the modified and live configuration by using values from the modified configuration

This sounds fine!
+1 for going with --force in this CI system again. Not 100% on --force in other situations, but feel less afraid reading that
I recall replacements like delete followed by create

Sarah Gibson @sgibson91 Oct 26 14:41
Thanks @consideRatio! I will add a commit changing this and see if we can finally get staging passing

Erik Sundell @consideRatio 14:46

Sarah Gibson @sgibson91 Oct 26 14:47
Hmmm, still failing https://github.com/jupyterhub/mybinder.org-deploy/pull/1670/checks?check_run_id=1309410686#step:14:59

Erik Sundell @consideRatio 14:47
Thank you for your work on this @sgibson91
Oh wait what... hmm...
--overwrite=true: Automatically resolve conflicts between the modified and live configuration by using values from
the modified configuration
Kubectl has overwrite to true by default, so ... helm upgrade --force is kube kubectl apply by default?
https://banzaicloud.com/blog/helm3-the-good-the-bad-and-the-ugly/#changing-the-unchangeable

Sarah Gibson @sgibson91 Oct 26 14:53
Soooooo, it's impossible?

Erik Sundell @consideRatio 14:53
"If you want to modify the role binding, my suggestion would be to delete it from the cluster first before upgrading."

Sarah Gibson @sgibson91 Oct 26 14:56
Yes, but judging by the error message, I'd have to delete everything :/

Erik Sundell @consideRatio 15:01
So i've kept reading about this, for like the 3-5th time, and always left feeling not confident.

It is a complicated issue, and they are not sure how to make helm3 function after some issues in helm2. (helm/helm#7082).

I think the closest option we have is: helm delete --keep-history && helm install --replace to always work, as suggested in that PR

in that issue i mean
To conclude, Helm2 had a functionality that Helm3 doesn't - the ability to try to carefully update a resource, but when that fail, be able to delete it and create it again. Helm 3 doesn't have this, and will simply fail when it is required to delete->create a resource to update it without any ability to workaround it with helm upgrade. The fallback is to manually delete the resources and then try an upgrade again.

Sarah Gibson @sgibson91 Oct 26 15:05
Hmmmm, ok
I think I'm going to meditate on this and come back to it tomorrow

Erik Sundell @consideRatio 15:09
Error: UPGRADE FAILED:
failed to replace object: PersistentVolumeClaim "hub-db-dir" is invalid: spec: Forbidden: is immutable after creation except resources.requests for bound claims
failed to replace object: PersistentVolumeClaim "staging-grafana" is invalid: spec: Forbidden: is immutable after creation except resources.requests for bound claims
failed to replace object: PersistentVolumeClaim "staging-prometheus-server" is invalid: spec: Forbidden: is immutable after creation except resources.requests for bound claims
failed to replace object: Service "hub" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "proxy-api" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "proxy-public" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "binder" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "staging-grafana" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "staging-ingress-nginx-controller-metrics" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "staging-ingress-nginx-controller" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "staging-ingress-nginx-defaultbackend" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "staging-kube-state-metrics" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "staging-prometheus-server" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "federation-redirect" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "gcs-proxy" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "matomo" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "proxy-patches" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Service "redirector" is invalid: spec.clusterIP: Invalid value: "": field is immutable
failed to replace object: Deployment.apps "analytics-publisher" is invalid: spec.selector: Invalid value: v1.LabelSelectorMatchLabels:map[string]string"app":"analytics-publisher", "heritage":"Helm", "release":"staging", MatchExpressions:[]v1.LabelSelectorRequirement(nil): field is immutable
failed to replace object: Deployment.apps "federation-redirect" is invalid: spec.selector: Invalid value: v1.LabelSelectorMatchLabels:map[string]string"app":"federation-redirect", "heritage":"Helm", "release":"staging", MatchExpressions:[]v1.LabelSelectorRequirement(nil): field is immutable
failed to replace object: Deployment.apps "gcs-proxy" is invalid: spec.selector: Invalid value: v1.LabelSelectorMatchLabels:map[string]string"app":"gcs-proxy", "component":"nginx", "heritage":"Helm", "release":"staging", MatchExpressions:[]v1.LabelSelectorRequirement(nil): field is immutable
failed to replace object: Deployment.apps "matomo" is invalid: spec.selector: Invalid value: v1.LabelSelectorMatchLabels:map[string]string"app":"matomo", "component":"matomo", "heritage":"Helm", "release":"staging", MatchExpressions:[]v1.LabelSelectorRequirement(nil): field is immutable
failed to replace object: Deployment.apps "matomo-mysqld-exporter" is invalid: spec.selector: Invalid value: v1.LabelSelectorMatchLabels:map[string]string"app":"matomo", "component":"mysqld-exporter", "heritage":"Helm", "release":"staging", MatchExpressions:[]v1.LabelSelectorRequirement(nil): field is immutable
I split the error message apart using && as an indicator of separate errors.

So, I think if you do ...

kubectl delete pvc hub-db-dir staging-grafana staging-prometheus-server
kubectl delete svc hub proxy-api proxy-public binder staging-grafana staging-ingress-nginx-controller-metrics staging-ingress-nginx-controller staging-ingress-nginx-defaultbackend staging-kube-state-metrics staging-prometheus-server federation-redirect gcs-proxy matomo proxy-patches redirector
kubectl delete deploy analytics-publisher federation-redirect gcs-proxy matomo matomo-mysqld-exporter
Then you are ready for a helm upgrade again
:'(

Sarah Gibson @sgibson91 Oct 26 15:12
Will that have to go in the CI every time though, or just for this migration? And what information do we lose (if any) by deleting those e.g. pvcs?

Erik Sundell @consideRatio 15:14
This needs to be done whenever the Helm chart upgrade helm upgrade does leads to a change of something that k8s sais requires a recreation of the resource
If you for example do a change to a k8s deployment resource matchLabels, then you require a recreation

Sarah Gibson @sgibson91 Oct 26 15:15
So could we not put it in the CI and instead have a section in the site reliability guide saying this needs to be done?

Erik Sundell @consideRatio 15:18
Hmmmm I mean, if the CI is updating something that requires delete->create, and that was a PVC, then you ended up deleting the PVC which may be very problematic as the storage is whiped.

So, okay to always recreate? Perhaps, not sure what resources we have that could do bad when recreated. But, for a real deployment when we assume we have valuable things in requested storage disks, then doing for example helm delete && helm install in sequence is madness

Sarah Gibson @sgibson91 Oct 26 15:28
Yeah, I'm just wondering if we do actually require delete->create every time?

Erik Sundell @consideRatio 15:33
Then something is wrong
If z2jh upgrade make us require that, i make a breaking change note

Sarah Gibson @sgibson91 Oct 26 15:34
Sorry, I'm confused now, what's wrong?
Here's what I'm thinking:
1) Maybe (I don't know this for certain) not every helm upgrade we do on mybinder.org actually involves the need to delete and reinstall, so we don't put delete and reinstall in the CI
2) For the few cases that do require a delete and reinstall, we have docs in the SRE telling people how to do that, i.e. it becomes a manual step that happens infrequently

Sarah Gibson @sgibson91 Oct 26 15:39
Or I'm wrong, a delete and reinstall is required with every helm upgrade and we have to have some ridiculous CI build?

Erik Sundell @consideRatio 15:43
Assume you only had z2jh, then, helm upgrade would not need --force etc, because it doesnt change resources that are immutable
So, if the CI system running helm upgrade thinks it required, then something is wrong
1 reply
And, i saw hub resources in that list. So, what is going on?
I think it should be like step 1/2 described abobe by you, but what cause helm to think wr need tl recreate resources because it think they are changing to much? Is something modofying the resources mid way?

Sarah Gibson @sgibson91 Oct 26 15:49
This is the first upgrade since migrating to helm3 though, so I wonder if that's the reason this step is required now? I had a similar issue with Hub23 but assumed I had done something wrong

Min RK @minrk 08:02
losing PVCs on mybinder.org is dramatically less disastrous than for most JupyterHub deployments. Primarily because our only PVCs are Hub state (would lose all current sessions, which is a major but short-lived disruption) and grafana/prometheus. Our grafana charts are backed up (not constantly, but recently) and prometheus is ephemeral so not super valuable. It has happened at least once on a prometheus upgrade and nobody really noticed.

Min RK @minrk 08:17
We appear to be having these conflicts due to the change in the heritage label from Tiller to Helm, causing a one-time conflict on all resources

Erik Sundell @consideRatio 08:22
Oh heritage is not suppored to be a matchLabel btw

Min RK @minrk 08:23
it's not a matchLabel, is it?
it super bugs me that helm upgrade --dry-run does not include conflict checks

Min RK @minrk 08:31
Ah, I see we have heritage in matchLabel. I don't understand what it means to be "not supported" as a matchLabel
I'm not sure if that's a source of conflict? That wouldn't make sense

Min RK @minrk 08:38
ok, it does appear that matchLabels is the source of some of these conflicts. Removing them may help

Sarah Gibson @sgibson91 08:42
Would that be something like
kubectl delete matchLabel label1 label2... -n staging

Min RK @minrk 08:43
I think it's delete & recreate the deployment objects
It should be a one-time thing, and the --force in our helm2 deployments should mean that they won't have the same conflict
Is it okay if I try a --force upgrade on staging to see if we lose the grafana PV? There's nothing in the staging one worth keeping
I'm curious if it will survive or not

Sarah Gibson @sgibson91 08:44
I already pushed a commit that added --force yesterday and it didn't work

Min RK @minrk 08:45
ah, ok

[sgibson91]

TL;DR: helm 2to3 may not actually be totally compatible with most deployed things and the best route forward may be to delete the helm2 deployment completely and redeploy with helm3. This is ok for staging and Turing/OVH because they're either not a live service or can be removed from the federation while work is underway. A different strategy will need to be implemented for prod so I will leave this cluster til last.

Note: This "delete helm2 and redeploy with helm3" strategy is ultimately what I ended up doing for Hub23 but I thought it was because I made a mistake 😬

Some more discussion from gitter

Min RK @minrk 08:47
Ah, I see. --force results in different conflicts. Fun!

Sarah Gibson @sgibson91 08:51
I feel like if I run the below, a lot of these will recreate themselves automatically, i.e. before I can run helm upgrade. Do we think that will be problematic?
kubectl delete pvc hub-db-dir staging-grafana staging-prometheus-server
kubectl delete svc hub proxy-api proxy-public binder staging-grafana staging-ingress-nginx-controller-metrics staging-ingress-nginx-controller staging-ingress-nginx-defaultbackend staging-kube-state-metrics staging-prometheus-server federation-redirect gcs-proxy matomo proxy-patches redirector
kubectl delete deploy analytics-publisher federation-redirect gcs-proxy matomo matomo-mysqld-exporter

Min RK @minrk 08:51
It'll bring down the whole service
Seems like the service conflict will happen every time on --force

Sarah Gibson @sgibson91 08:52
Yes, aside from that ;)

Min RK @minrk 08:52
IIUC
whereas the non-force conflicts will happen just once

Sarah Gibson @sgibson91 08:52
Ok, so revert my last commit on the PR

Min RK @minrk 08:53
Maybe? I'm still trying to understand

Erik Sundell @consideRatio 08:53
@minrk i meant heritage isnt supposed to be a matchLabel according to best practices of Helm charts, so in z2jh we have avoided that, but ive seen it show up here and there in other charts

Min RK @minrk 08:53
--force is showing a conflict on unchanged resources (clusterIP) which worries me more than the conflicts on matchLabels, which we can fix
The remaining problem is that the grafana PVC uses heritage in a Label, which I guess is a bad practice, despite being a recommended best practice
~all of this would have been avoided if helm3 didn't decide to change the heritage value

Erik Sundell @consideRatio 08:55
To have a label is one thing, but to use heritage as a matchLabel / label selector, isnt

Min RK @minrk 08:55
Yes, but it's the label causing the conflict on the PVC
Most of the conflicts are on matchLabel, which we can fix. One remaining is on the grafana PVC, which is just a label
This one:
cannot patch "staging-grafana" with kind PersistentVolumeClaim: PersistentVolumeClaim "staging-grafana" is invalid: spec: Forbidden: is immutable after creation except resources.requests for bound claims

Erik Sundell @consideRatio 08:56
Oh

Min RK @minrk 08:57
and the whole diff:
staging, staging-grafana, PersistentVolumeClaim (v1) has changed:

  # Source: mybinder/charts/grafana/templates/pvc.yaml
  apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    name: staging-grafana
    namespace: staging
    labels:
      app: grafana
      chart: grafana-3.10.1
      release: staging
-     heritage: Tiller
+     heritage: Helm
    finalizers:
      - kubernetes.io/pvc-protection
  spec:
    accessModes:
      - "ReadWriteOnce"
    resources:
      requests:
        storage: "1Gi"
    storageClassName:

Erik Sundell @consideRatio 08:57
So: "kubectl label pvc" is forbidden?

Min RK @minrk 08:58
while it is bound, I guess

Erik Sundell @consideRatio 08:58
Wow, okay

Min RK @minrk 09:00
Here's what I don't understand: the prometheus-server PVC has heritage: Helm label already, but the grafana PVC has heritage: Tiller
I can't find anything different about them
maybe it was a coincidence that the prometheus server was not running when it patched the PVC? I really don't understand this.
Seems like the helm 2 to 3 upgrade process, while it's smooth to start, is a total lie that it's actually compatible with deployments deployed with helm 2, and everyone should just delete and deploy fresh with helm 3.
And this is ~100% due to the change in the heritage label.

Sarah Gibson @sgibson91 09:03
That's ultimately what I ended up doing with Hub23
But surely we can't do that with prod?!

Min RK @minrk 09:03
Yeah, I think it might be the right thing to do
I'm not sure. I have other, more time sensitive stuff today, but I'll check back in this afternoon

Sarah Gibson @sgibson91 09:04
I'm feeling I'll leave prod to be the last cluster to be updated

Min RK @minrk 09:04
I think that's a good plan

Sarah Gibson @sgibson91 09:04
At least with Turing/OVH, we can remove them from the federation

Min RK @minrk 10:41
The --force conflicts are (mostly) related to clusterIP being an empty string by default instead of omitted, which means there's a new best practice described here which is adopted by ~no charts yet as far as I can tell
I think we'll have an easier time resolving the non-force conflicts, which are mostly matchLabels and can be fixed with an upgrade while still using helm 2

Sarah Gibson @sgibson91 10:44
Ok, I'll revert my last commit to the PR then

Min RK @minrk 10:46
Make sure to keep --force on helm2, we'll need it to resolve most of the conflicts on the rest

Erik Sundell @consideRatio 10:47
This is also why removing --force works: Helm makes a PATCH operation
Helm 3 --force is crap is my take away

Min RK @minrk 10:47
yeah, it doesn't actually 'force' anything and creates loads of conflicts

Erik Sundell @consideRatio 10:47
It doesnt help like helm2 --force, but cause issues
Helm2 can help us make updates like changing matchLabels better than helm3, but

Sarah Gibson @sgibson91 10:49
The revert commit https://github.com/jupyterhub/mybinder.org-deploy/pull/1670/commits/21cd33ac0b7c710a3d8ceb9726b74f9347140218
1 reply

Erik Sundell @consideRatio 10:49
By using helm2 --force, but when helm2 and helm3 are used without force, they should be similar

Min RK @minrk 10:51
@consideRatio can you understand how the prometheus PVC was updated without being recreated when the grafana PVC can't be?
all 3 PVCs show created as 41 days ago (when we migrated to the new project), but hub-db-dir and prometheus-server both show heritage: Helm while staging-grafana has heritage: Tiller, causing the conflict

Erik Sundell @consideRatio 10:52
Gitlabs helm chart have resolved their upgrades when they do introduce breaking changes by renaming resources. They added -v1 to the resource name when they needed to do it, and the next time they will make it v2 etc.

Min RK @minrk 10:53
To me, this means that the other two PVCs were upgraded in-place, but I can't figure out why they would work and grafana would not
Yeah, that makes sense. But we aren't making any breaking changes. Only helm is making breaking changes.
Maybe those got upgraded by a --force
Interesting, though: --force did not delete & recreate PVs, it "replaced" them meaning no data loss. I guess this is what they meant to improve with helm 3's --force. It's less destructive (no data loss here), but also less forceful

Min RK @minrk 10:58
I don't suppose we can do a --force on just one resource?
helm/helm#6681 seems to be the key
Aha!
So there is a big benefit to helm 3's replacement strategy when it comes to PVCs: it doesn't lose data unlike helm 2's delete & replace!

Min RK @minrk 11:04
Downside is that due to replacement strategy, it can still conflict, as we are seeing with lots of services. This is a pain
But manual kubectl replace on the PVCs (effectively helm3 upgrade --force on just the PVCs) should let us resolve the conflict prior to upgrade

Sarah Gibson @sgibson91 11:17
Shall I try that?

Min RK @minrk 11:19
Hm, I'm not sure it will actually work
Running helm template shows that staging-grafana does indeed have an empty storageClassName which is pointed to as an issue that causes problems. Maybe that's what's happening here
I just pushed one commit to your branch removing heritage from matchLabels. It's possible that setting the grafana storageClassName to 'standard' will fix the staging-grafana conflict

Sarah Gibson @sgibson91 11:27
Ok, I'll re-run the PR
Hmmm no that didn't work
https://github.com/jupyterhub/mybinder.org-deploy/pull/1670/checks?check_run_id=1314092220#step:14:59

Erik Sundell @consideRatio 11:32
So there is a big benefit to helm 3's replacement strategy when it comes to PVCs: it doesn't lose data unlike helm 2's delete & replace!
Downside is that due to replacement strategy, it can still conflict, as we are seeing with lots of services.
Does helm3 --force / kubectl apply --overwrite allow for a label to be changed etc on a PVC?

Min RK @minrk 11:33
yes
why does this work and patch doesn't? nobody will ever know :shrug emoji:
Basically, by using replace, you can change some immutable fields
@sgibson91 I pushed one more commit to specify default storageClassName for grafana. I think it works!

Sarah Gibson @sgibson91 11:34
:fingers crossed!:
Hmmm https://github.com/jupyterhub/mybinder.org-deploy/pull/1670/checks?check_run_id=1314123443#step:14:82

Sarah Gibson @sgibson91 11:41
I think theoretically that was successful but there was a timeout issue?

Min RK @minrk 11:43
old prometheus-server rs wasn't deleted so it didn't let go of the pvc
!log kubectl delete rs staging-prometheus-server-76b4697f7c
I'm never sure what causes that
One more run and I think it will succeed

Sarah Gibson @sgibson91 11:44
Just started it
Wooo! got passed the upgrade step!

Erik Sundell @consideRatio 11:46
@minrk I could do kubectl edit pvc and add a label, i could also do kubectl apply -f tmp.yaml where i added a label to a pvc
no --overwrite was needed for this
$ kubectl version
Client Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.2", GitCommit:"52c56ce7a8272c798dbc29846288d7cd9fbae032", GitTreeState:"clean", BuildDate:"2020-04-16T11:56:40Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"17+", GitVersion:"v1.17.9-gke.1504", GitCommit:"fb367de18820fe57bac5d97ad60b7a2cef8765e9", GitTreeState:"clean", BuildDate:"2020-09-09T01:00:23Z", GoVersion:"go1.13.9b4", Compiler:"gc", Platform:"linux/amd64"}

Min RK @minrk 11:47
@consideRatio I think you're right. I was on the wrong track, and it was the empty storageClassName that was causing the problem.

Erik Sundell @consideRatio 11:47
ah okay

Min RK @minrk 11:47
Essentially the same problem as the service clusterIP when run with --force, but this time it occurred without --force
Fixed by: https://github.com/jupyterhub/mybinder.org-deploy/pull/1670/commits/f5d0b3640d1a878bc07e45e0bc0169069136ea09
but helm and kubernetes are very very bad at showing these issues. They do not show what is different. helm diff upgrade shows no changes because the template hasn't changed, and kubernetes doesn't say what field is wrong.
I was misled because the only change in the template was the label, so I assumed that was the conflict.
It was precisely this issue

Erik Sundell @consideRatio 11:50
Hmmm? But if the template had not changed except for the label, then how did something else change in the rendered output?

Min RK @minrk 11:50
The rendered output didn't change.
helm 3 compares with the existing resource not the previous template render
so where kubernetes has a default value transform (i.e. null -> "standard") helm 3 sees "standard" != null so it tries to apply the patch.
Whereas helm 2 sees null == null since it compares template->template where helm 3 compares resource->template

Erik Sundell @consideRatio 11:53
Whereas helm 2 sees null == null since it compares template->template where helm 3 compares resource->template

I'm confused, are you saying helm2 compares rendered resource with registered resource, while helm 3 considers rendered resource with past rendered resource and installed resource?

Min RK @minrk 11:53
So templates written for helm 3 need to be careful to actually omit null fields that get populated with a default non-null (service.clusterIP, for instance, pvc.storageClassName as well)
helm 3 does a 3-way merge of old template, existing resource, and new template

Erik Sundell @consideRatio 11:54

Sarah Gibson @sgibson91 11:55
I think a couple of tests (our test suite, not helm) on staging may be currently failing due to timeouts

Erik Sundell @consideRatio 11:55
So templates written for helm 3 need to be careful to actually omit null fields that get populated with a default non-null (service.clusterIP, for instance, pvc.storageClassName as well)

Does this mean that we had issues because we had templates generating resources with storageClassName: "" etc, but we needed them to be omitted entirely in Helm 3?

Min RK @minrk 11:56
yes, grafana was doing this

Erik Sundell @consideRatio 11:56
I think a couple of tests (our test suite, not helm) on staging may be currently failing due to timeouts

What kind? helm upgrade timeouts etc?

Sarah Gibson @sgibson91 11:56
No, timeouts on launching a binder

Min RK @minrk 11:56
but jupyterhub and prometheus did not have this problem, so they upgraded fine

Sarah Gibson @sgibson91 11:56
Oh, it passed eventually though
HOORAY!! GREEN CI!!
Well done everybody!

Min RK @minrk 11:57
our grafana chart is super out of date, so upgrading that to latest will likely fix it as well
But one thing at a time!
Maybe it would have been a good idea to make sure all of our dependencies are recent before migrating to helm 3?
I bet they've all faced helm 3 issues as well and fixed them.
@sgibson91 hooray! Thanks for leading the charge
dind was stuck for a bit, so my guess is that builds were slow to get started
@sgibson91 do you think it makes sense to upgrade to latest grafana and prometheus before migrating prod to helm 3?

Sarah Gibson @sgibson91 12:00
Yes, that might help

Erik Sundell @consideRatio 12:00

@sgibson91 @minrk , what are the takeaways for us to consider with regards to helm3 practices etc?

helm2 --force won't solve issues like helm2 --force could - example: matchLabel selector field changed of a deployment, and changing that is not allowed. helm2 --force would recreate the deployment resource and it would be fine, while helm3 --force won't do that.
helm3 --force can solve issues associated with templates having set storageClassName: "" or `clusterIP: "" - right?
By upgrading Helm charts in Helm2 before going to Helm3, one may avoid various issues.

Sarah Gibson @sgibson91 12:00
Though since the fixes have been made in the chart, they'll get applied to prod anyway as soon as I merge the PR?
(and Turing, and OVH)

Erik Sundell @consideRatio 12:01
Maybe it would have been a good idea to make sure all of our dependencies are recent before migrating to helm 3?
I bet they've all faced helm 3 issues as well and fixed them.

Good suggestion I think.

Min RK @minrk 12:02
yes, I believe they will
but because we've done some manual delete & recreate on staging, we don't have a guarantee that prod won't need any similar hand-holding
I believe all the manual delete & recreate done on staging will be applied by the helm 2 --force on production, but can't be 100% sure.

Erik Sundell @consideRatio 12:03
I also think helm2 --force also will manage to do whats needed

Min RK @minrk 12:03
I think so

Sarah Gibson @sgibson91 12:05
If you think upgrading grafana and prometheus before we touch prod will be the best move, then let's do it
I can merge/babysit jupyterhub/mybinder.org-deploy#1670 now

Min RK @minrk 12:06
It's more work to do while some things are up in the air. So it's probable that we've addressed most of the issues. I could go either way.

Sarah Gibson @sgibson91 12:07
A litmus test would be to see if we have the same issues on Turing/OVH first

Min RK @minrk 12:07
yup

[sgibson91]

This commit solved the issue in the end. Helm3 doesn't like empty variables.

Plan going forward:

1. Merge #1670 and babysit
2. Let things settle for a couple of days
3. Work on Turing next. See if any other issues turn up during that upgrade.
   
   
   
   • If they do, a potential fix may be to upgrade our prometheus and grafana dependencies before moving onto prod as most of the issues came from the analytics publisher, "stuff that just runs on staging/prod"
   
   

[sgibson91]

Turing upgrade now complete! 2 down, 2 to go

[sgibson91]

As of yesterday, OVH is now running Helm 3! :tada:

If anyone has some recommendations on how to smoothly do the upgrade on prod, I'd love to hear! For the other clusters, I just removed them from the federation before I started tinkering. But that's not an option with prod.

[sgibson91]

If they do, a potential fix may be to upgrade our prometheus and grafana dependencies before moving onto prod as most of the issues came from the analytics publisher, "stuff that just runs on staging/prod"

I actually think this would be a good idea as this is what @consideRatio and I ended up debugging yesterday (in the first place, at least 😂 )

[consideRatio]

Weekend migration work

• [x] prometheus made easier to upgrade by making using Recreate as an upgrade strategy since RollingUpdate isn't functional with its attached storage that can only be mounted on one pod (#1736).
• [x] Grafana and Prometheus upgraded a bit but not all the way as that requires Helm3 now, and I've made notes in requirements.yaml on future upgrades with Helm 3 (#1733 ).
• [x] GKE prod upgraded to use helm3 manually with 2to3 and a trial deployment
• [x] CI system upgraded to use helm3 for GKE prod also (#1738).
• [x] Manually cleanup Helm2 revision data already migrated to Helm3:
  
  
  
  • [x] GKE Prod
  
  
  • [x] OVH
  
  
  • [x] Staging
  
  
  • [x] Turing (Ping @sgibson91)
  
  
• [x] Manually cleanup Helm2 tiller deployment and ServiceAccount:
  
  
  
  • [x] GKE Prod
  
  
  • [x] OVH
  
  
  • [x] Staging
  
  
  • [x] Turing (Ping @sgibson91)
  
  

Is Gesis not using the mybinder.org-deploy Helm chart? If so, we can also update the Chart.yaml to apiVersion v2 and leave Helm2 stuff behind forever.

[betatim]

Nice work!

I think Gesis has been using helm 3 for quite a while already. It isn't deployed from this repository but I don't know what exactly they use. A question for @bitnik and @MridulS

[sgibson91]

• [x] Manually cleanup Helm2 revision data already migrated to Helm3:
  
  
  
  • [ ] Turing (Ping @sgibson91)
  
  
• [x] Manually cleanup Helm2 tiller deployment and ServiceAccount:
  
  
  
  • [ ] Turing (Ping @sgibson91)
  
  

Are these steps something different to running helm3 2to3 cleanup? If not, they were done back when I did the migration.

[consideRatio]

@sgibson91 i needed to manually delete the tiller SA, but tiller deploy was deleted as part of the helm 2to3 cleanup

[sgibson91]

OK, well I plan on completely destroying and redeploying the cluster tomorrow to try and fix the DNS issue - so there definitely shouldn't be any helm2 stuff after that!

[sgibson91]

Tiller service account deleted on Turing cluster

[MridulS]

Is Gesis not using the mybinder.org-deploy Helm chart?

Yeah, we don't use the mybinder.org-deploy Helm chart. We already are at helm3 for some time now :)

[sgibson91]

Reopened as I realised #1740 is still open