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

Support fullnameOverride / nameOverride and reference resources by named templates #1923

Merged
merged 28 commits into from
Jan 18, 2021

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Nov 25, 2020

Summary

Most helm charts provide a fullnameOverride configuration and a nameOverride configuration, something that also ships with the helm create boilerplate. This PR introduce such configuration options, but the default values are set in a way that retain the current naming convention.

With the new configuration options, there are five kinds of naming schemes that can be used depending on if fullnameOverride and nameOverride are null, blank, or non-blank strings.

Name format Resource types fullnameOverride nameOverride Note
component namespaced "" * Default
release-component cluster wide "" * Default
fullname-component * str * -
release-component * null "" -
release-(name-)component * null str omitted if contained in release
release-(chart-)component * null null omitted if contained in release

Changes

  • All references to resource names are now replaced with statements like {{ include "jupyterhub.hub.fullname" . }}.
  • Named templates are made usable with the . scope from a chart with JupyterHub as a dependency chart.
    This will be useful for binderhub for example.
  • The named templates rendered values are also exposed in the hub configmap.
    • They are directly available as keys such as hub, proxy-public, and user-scheduler-ref which correspond to their named template name. This is useful if you want to mount one name to an environment variable for example.
    • They are also available as a name-templates.yaml blob which is can be easier other times.
  • fullnameOverride and nameOverride config options added
    • With documentation in schema.yaml

Periphery changes

  • A script to help diff changes locally is added
  • The configmaps traefik-proxy-config and hub-config are renamed to autohttps and hub.
    By doing so we become more systematic in how we name things and avoid introducing two additional named templates to reference specifically for these configmaps.
  • We no longer try to load /etc/jupyterhub/config/config.yaml which isn't mounted anyhow
    It's not possible for someone to put a file there either because we mount the configmap in a way that would replace that folder.

Some benefits ?

  • A large step towards supporting multiple installations in the same namespace
  • Aligns with a common practice of providing fullnameOverride and nameOverride chart config.
  • Not backward incompatible (due to our default values of fullnameOverride and nameOverride)
  • No more duplication of logic for names ("hub-secret" or .Values.hub.existingSecret etc.)
  • Provides a future proof way to reference resources for parent charts

Some downsides ?

  • The chart complexity rose a bit, but I think and we have lines of code now, but I think the readability improved overall.

Review notes

In #1978 I added the ability to see a diff on the rendered Helm templates between the latest dev release of the chart and the PR. It isn't helpful for changes to the hub configmap or the autohttps configmap as those were renamed. For those specifically, it's better to not use this diff but the unrendered diff with GitHub's normal UI.

Here is an example helm diff for 1329fa9 which currently is the latest change.

The documentation is updated in the configuration reference, see the PR docs preview.

Implementation deliberations

New default naming convention?

Do we want to transition to a new default naming convention, namely where fullnameOverride and nameOverride is null and our resources are named with a release-(jupyterhub-)component pattern.

  • Decision
    Perhaps, but not in this PR.

Disallow changes to fullnameOverride / nameOverride?

Should we allow a change fullnameOverride / nameOverride where it probably leads to issues, or should we make template rendering fail if attempting to do so?

  • Decision
    Perhaps, but not in this PR.

Should this PR solve #1791?

It currently doesn't. I think the following pieces are missing.

  • KubeSpawner's created resources must not have conflicts, so pod_name_template, pvc_name_template and more needs to be configured to include the helm release name or similar.

  • KubeSpawner's resource inspections must filter out only the pod it manages.
    Currently, only one label is used as a selector, the configurable component_label which is component: singleuser-server by default. While this could be modified to include more information, KubeSpawner should instead support the ability to include more selector labels.

  • Decision
    No, let's work towards that in future PRs. This PR is already a sizable chunk that is quite cohesive on its own.

@consideRatio consideRatio changed the title Support fullnameOverride configuration in a backward compatible way [WIP] Support fullnameOverride configuration in a backward compatible way Nov 25, 2020
@consideRatio consideRatio marked this pull request as draft November 25, 2020 18:06
@consideRatio consideRatio changed the title [WIP] Support fullnameOverride configuration in a backward compatible way Refactor resource references to not use hardcoded strings Nov 26, 2020
@consideRatio consideRatio marked this pull request as ready for review November 26, 2020 17:50
@consideRatio consideRatio changed the title Refactor resource references to not use hardcoded strings Refactor to reference resources' names using named templates instead of a hardcoded strings Nov 28, 2020
@consideRatio consideRatio force-pushed the pr/fullname-override branch 9 times, most recently from ef778d9 to 6001377 Compare January 9, 2021 00:10
@consideRatio consideRatio changed the title Refactor to reference resources' names using named templates instead of a hardcoded strings Support fullnameOverride / nameOverride and reference resources by named templates Jan 9, 2021
@consideRatio consideRatio marked this pull request as draft January 9, 2021 01:17
@consideRatio consideRatio force-pushed the pr/fullname-override branch 2 times, most recently from 5a0fab5 to f8dbdf4 Compare January 9, 2021 03:49
@consideRatio consideRatio marked this pull request as ready for review January 9, 2021 04:45
@consideRatio

This comment has been minimized.

@consideRatio consideRatio added this to the 0.11.0 milestone Jan 10, 2021
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

hub-secret is actually a resource used by other Helm charts

I found only one reference to this in mybinder.org-deploy and none elsewhere, so I don't think that's enough of a reason to leave it out of this consistency update. We will need to be aware of this and make that one line update to mybinder.org-deploy when we next bump this chart, but it is an okay requirement, I think. Plus, it would probably be more prudent for consumers of that info to use .Values.jupyterhub.proxy.secretToken in its own secret rather than accessing the secret itself. I'd call that an 'unprotected private api access' like accessing a _private attribute in Python. Not really a problem to do, but it's okay to expect more churn if you do use them. The error will also be pretty informative ("failed to mount nonexistent hub-secret volume: no such secret" or similar).

jupyterhub/files/hub/z2jh.py Outdated Show resolved Hide resolved
@consideRatio consideRatio force-pushed the pr/fullname-override branch 4 times, most recently from 0fb0678 to 2b61e10 Compare January 17, 2021 14:48
@minrk minrk merged commit aca9181 into jupyterhub:master Jan 18, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 18, 2021
jupyterhub/zero-to-jupyterhub-k8s#1923 Merge pull request #1923 from consideRatio/pr/fullname-override
@@ -47,9 +47,9 @@ spec:
{{- .Values.hub.extraVolumes | toYaml | trimSuffix "\n" | nindent 8 }}
{{- end }}
{{- if eq .Values.hub.db.type "sqlite-pvc" }}
- name: hub-db-dir
- name: pvc
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a possibly breaking change, btw. If you are using initContainers that also mount /srv/jupyterhub, you'll end up with this error:

Error: UPGRADE FAILED: cannot patch "hub" with kind Deployment: Deployment.apps "hub" is invalid: spec.template.spec.containers[1].volumeMounts[0].name: Not found: "hub-db-dir"

We should at least note this somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants