Skip to content

Conversation

@consideRatio
Copy link
Collaborator

@consideRatio consideRatio commented Feb 16, 2022

The Helm chart relies on Traefik, and Traefik relies on some CRDs be pre-installed, so we bundle those with the dask-gateway helm chart. This installation updates those bundled CRDs and the referenced Traefik version along with them.

@martindurant
Copy link
Member

+1, I'll test later this week.
No idea why PBS is failing, but the main test is only failing on formatting ( #477 #476 )

@dhirschfeld
Copy link

Just confirming that the bundled CRDs won't interfere if I have an AKS cluster with traefik already deployed?

> kubectl get crd -A | rg traefik
ingressroutes.traefik.containo.us                                  2022-02-14T02:36:48Z
ingressroutetcps.traefik.containo.us                               2022-02-14T02:36:48Z
ingressrouteudps.traefik.containo.us                               2022-02-14T02:36:48Z
middlewares.traefik.containo.us                                    2022-02-14T02:36:48Z
middlewaretcps.traefik.containo.us                                 2022-02-14T02:36:48Z
serverstransports.traefik.containo.us                              2022-02-14T02:36:48Z
tlsoptions.traefik.containo.us                                     2022-02-14T02:36:48Z
tlsstores.traefik.containo.us                                      2022-02-14T02:36:48Z
traefikservices.traefik.containo.us                                2022-02-14T02:36:48Z

@consideRatio consideRatio force-pushed the pr/update-traefik-crds branch from e2d7945 to e08c46e Compare February 16, 2022 15:37
@consideRatio
Copy link
Collaborator Author

@martindurant I'm not sure about the PBS failure's root cause - but I'm quite confident it's unrelated. I opened this issue about it: #422

@dhirschfeld the dask-gateway helm chart does indeed install some CRDs, and I'm not sure how helm handles it if there are already CRDs installed or similar. I'm quite confident though that this PRs change wouldn't break anything by keeping them updated.

For details on how helm handles a helm chart with a crds folder, there is this documentation

These CRDs are not templated, but will be installed by default when running a helm install for the chart. If the CRD already exists, it will be skipped with a warning.

There is no support at this time for upgrading or deleting CRDs using Helm.

In practice, this means that a fresh install of dask-gateway will provide fresh CRDs, and if you upgrade after this has been made - I believe nothing will change in practice: the old CRDs will be used. This could be a problem that needs to be managed by communicating to the end users that the dask-gateway helm chart relies on modern traefik CRDs for its modern traefik version - and because of that, will need to manually delete the traefik CRDs and re-run a helm upgrade for example.

A discussion about this in depth is probably not relevant for the scope of this PR though - I suggest opening an issue if you want to deliberate on it further. So far, we haven't had problems with this.


I've rebased this on the main branch that includes some CI fixes, hopefully the PBS test doesn't intermittently crash on us again.

@dhirschfeld
Copy link

Thanks for the info @consideRatio! I imagine if you were trying to run two very different versions there might be some issues but I'm using close-to-the-latest version and I don't think the CRDs change too much so it should be fine.

Something to keep in mind and if I ever do run into problems I can open an issue with a concrete example.

@martindurant
Copy link
Member

I can confirm that the updated traefik and CRDs work fine (and with latest dask too)

@martindurant martindurant merged commit fd77685 into dask:main Feb 18, 2022
@martindurant
Copy link
Member

As a follow up, we might consider documentation about which versions of what we support; for example, the new CRD versions backport for a while, but how long? I suspect finding exact version bounds on things may be quite an involved process.

@consideRatio
Copy link
Collaborator Author

@martindurant yeah it's tricky, especially since the versions of the CRDs are not really changed - they modify an existing version of the k8s resource they call v1alpha1.

I think what we should do, is to realize via helm chart upgrade test, if a change will break an existing installation when upgrading. Then, if we conclude that it did - then we can make a decision on if we can/should do something to be backward compatible.

I'm quite lost in general on the issues until they are demonstrated practically - so having an upgrade test of a helm chart installation seems like step 1. I opened #480 to reflect that need.

Thanks @martindurant and @dhirschfeld for your review efforts!

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.

3 participants