dask-gateway: conditional install with dask-gateway.enabled

[consideRatio]

Perhaps one doesn't want to install dask-gateway, then the dask-gateway.enabled Helm chart configuration can be used to disable it.

Technical reference on the Helm mechanism used, see: https://helm.sh/docs/chart_best_practices/dependencies/#conditions-and-tags

[TomAugspurger]

Thanks @consideRatio, and thanks for the link to the docs. I wasn't aware of conditions.

[jhamman]

Thanks for this @consideRatio! Do you think it is possible for us to add a similar conditional for the dask-kubernetes integration. Having the ability to control both of these in the same way would be great.

[consideRatio]

@scottyhq I'm not able to answer this because I'm not overviewing what the dask-kubernetes integration technically.

But, I think yes, you can make a lot of things conditional based on this Helm chart configuration. You can for example influence Helm template could do a conditional like {{- if .Values.dask-kubernetes.enabled }}. You can influence the python configuration running under jupyterhub.hub.extraConfig if you read the mounted helm chart configuration as well.

Ah now I saw the rbac stuff associated with dask. Are they part of dask-gateway setup? Then you should have...

-{{- if .Values.rbac.enabled -}}
+{{- if and .Values.rbac.enabled .Values.dask-gateway.enabled -}}
 kind: ServiceAccount
 apiVersion: v1
 metadata:
   name: daskkubernetes
   namespace: {{ .Release.Namespace }}

[jhamman]

IIRC, the code block you have above is just for the dask-kubernetes integration. So I guess we already have the flag in place, its just not consistently named.

Ideally, we would just condition this as:

-{{- if .Values.rbac.enabled -}}
+{{- if .Values.dask-kubernetes.enabled -}}
 kind: ServiceAccount
 apiVersion: v1
 metadata:
   name: daskkubernetes
   namespace: {{ .Release.Namespace }}

This would require deprecating the rbac flag but I think that is okay.

[consideRatio]

@jhamman the rbac.enabled value is also passed to jupyterhub as declared in Chart.yaml under the dependencies section.

I think it could make sense to add dask-kubernetes.enabled alongside the suggested patch here: https://github.com/pangeo-data/helm-chart/pull/133#issuecomment-653073165

I'm not sure who would want to disable RBAC at this point in time though, at one point it was new, but now it is the default and strongly recommended.

[jhamman]

@consideRatio - let's do as suggested. That is to keep rbac enabled (as is now default in z2jh) but disable dask-kubernetes via its own flag. Do you want to do that here?

[consideRatio]

@jhamman I added a commit to enable disabling dask-kubernetes. I suggest a release of this helm chart makes a hard deprecation of dask-kubernetes if its no longer used.

[jhamman]

Thanks @consideRatio - lgtm.

Would love to hear from @jacobtomlinson, @rabernat, and @scottyhq on the explicit deprecation of the dask-kubernetes integration. From my perspective, setting the default value to false would be sufficient but I'm more than willing to be convinced otherwise.

[TomAugspurger]

+1 to setting dask-kubernetes.enabled to False by default. That makes it clear that you (probably) only need one of dask-gateway or dask-kubernetes enabled.

[scottyhq]

Thanks @consideRatio! Happy to have this merged and set dask-kubernetes to False by default.

Ultimately though, I think it makes the most sense to just remove dask-kubernetes from this helm chart going forward. Old versions exist if folks want to go back in time, but per discussion here https://github.com/dask/helm-chart/issues/68 I think it makes most sense for two modes of deployment:

1. Single-user (dask helm chart or dask-kubernetes)
2. multi-user (this pangeo helm chart with dask-gateway)