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

[Part 6 of #758] Storage labels: configurable extras #924

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Sep 11, 2018

To review

TODO:

  • Change to storage.extraLabels

TODO

  • Delete storage-kind label, it is not needed with other labels in place such as component: hub on the hub-db-dir PVC and component: singleuser-storage on the user PVCs.

@@ -5,6 +5,7 @@ metadata:
name: hub-db-dir
labels:
{{- include "jupyterhub.labels" . | nindent 4 }}
hub.jupyter.org/storage-kind: "core"
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this can be applied on the PVC without trouble. You may not change the selector field in the spec of the PVC though.

Copy link
Member

Choose a reason for hiding this comment

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

What does this storage-kind label do?

The extra labels makes sense, but I don't see what is accomplished by the 'storage-kind' label.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main benefit as I see it, is that having such labels allows us to write easier instructions for various tasks in the future, or simply allow someone to maintain a jupyterhub better thanks to --selector.

What would you do if you wanted to delete all PVCs, but NOT the hub-db-dir ? Well enter regexp and scripting currently, but if you have a label on the hub-db-dir you can exclude that with a --selector.

So, it is on other words a harmless but potentially useful thing, perhaps it can be useful to us if we support someone solve something regarding this as well.

Copy link
Member

Choose a reason for hiding this comment

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

If there's a problem that user volumes aren't well labeled, we can address that by making sure they have a good label (is component not an adequate standard label that exists for this purpose?), but I think the kind=core/user distinction is being applied too broadly, and the core label isn't useful here, especially since it's being applied to exactly one already-distinguishible volume.

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, the ideas of this were introduced before some other labeling improvements, now I see we have the following labels, which i find to be enough:

  # on user PVC
  labels:
    app: jupyterhub
    chart: jupyterhub-0.7.0
    component: singleuser-storage
    heritage: jupyterhub
    release: jhub

  # on hub-db-dir PVC
  labels:
    app: jupyterhub
    chart: jupyterhub-0.7.0
    component: hub
    heritage: Tiller
    release: jhub

I'll delete the storage-kind stuff.

@consideRatio consideRatio changed the title [Part 6 of #758] Storage labels: one default + configurable extras [Part 6 of #758] Storage labels: configurable extras Sep 14, 2018
@@ -161,6 +161,7 @@ singleuser:
events: true
extraAnnotations: {}
extraLabels: {}
storageExtraLabels: {}
Copy link
Member

@minrk minrk Sep 17, 2018

Choose a reason for hiding this comment

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

Shouldn't this be below as storage.extraLabels?

Extra storage labels are now configurable through config.yaml and
`singleuser.storageExtraLabels`.
@minrk minrk merged commit 274741b into jupyterhub:master Sep 20, 2018
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.

2 participants