Add common labels for Pods & PVCs #161

consideRatio · 2018-04-18T19:32:32Z

Summary

Changes are fully backwards compatible.
The kubespawner is hardcoded with the label selector component=singleuser-server and still does not support multiple deployments in the same namespace.
Spawned Pods and PVCs get additional common_labels, this allows z2jh to set the chart and release labels on spawned Pods and PVCs.
PR utilized by jupyterhub/zero-to-jupyterhub-k8s#625

Background

In jupyterhub/zero-to-jupyterhub-k8s#625, I wanted to make all objects created within the z2jh helm chart follow Helms Best Practices for labels. This PR helps ensure that and thereby fixes #159.

Details

These are the labels of relevance in this PR. Note that the PodReflector is hardcoded to find only the pods with a singleuser-server label.

Object	Label	Traitlet	Default value
PVC / Pod	`component`	hardcoded	`'singleuser-storage'` / `'singleuser-server'`
PVC / Pod	`app`	`common_labels`	`'jupyterhub'`
PVC / Pod	`heritage`	`common_labels`	`'jupyterhub'`

History

I thought of using environment variables to configure certain labels. But configuring specific variables would couple kubespawner unnecessarily (to z2jh and Helm for example).

Implementation notes

I made sure common_labels couldn't be overridden by extra labels provided to the kubespawner

PR TODO

Configure using traitlets
Exclude all helm chart presumptions
Test this in conjunction with jupyterhub/zero-to-jupyterhub-k8s#625
Consider if singleuser-server's need a restart while introducing this code or changes to common_labels and how to avoid it. Update: I decided to keep a hardcoded pod-selection based on the namespace, heritage=jupyter,component=singleuser-server for now.
Figure out all interactions KubeSpawner might have with pods after spawn. Update: start, poll and stop, using the PodReflector to find the pods.
Test this better together with jupyterhub/zero-to-jupyterhub-k8s#625.

Excluded from this PR

Figure out how / when / why KubeIngressProxy is used
Figure out all interactions KubeIngressProxy might have with pods after spawn
Consider sharing common_labels trait with KubeIngressProxy

consideRatio · 2018-04-18T19:34:51Z

kubespawner/spawner.py

-        'heritage': 'jupyterhub',
-        'component': 'singleuser-server',
-    }
-


Heritage were already defined in _build_common_labels and component was added specifically anyhow in get_pvc_manifest and _build_pod_labels.

Oh... But perhaps they were utilized by something outside the kubespawner project that I didn't know of? I did not find an internal reference to them though.

Nah... This should be fine I think.

The labels field in the PodReflector overrides the NamespacedResourceReflector field and I didn't realize that fully until recently. As they fill the purpose of selecting the pods within the namespace with those labels, they must remain.

consideRatio · 2018-04-18T19:36:42Z

kubespawner/spawner.py

+            'release': os.getenv('LABEL_RELEASE', 'unknown')
+            'chart': os.getenv('LABEL_CHART', 'unknown')
+            'heritage': os.getenv('LABEL_HERITAGE', 'jupyterhub')
+        })


I make sure to add these after the extra labels are added.

consideRatio · 2018-04-18T19:39:20Z

kubespawner/spawner.py

-        labels.update(self.pod_reflector.labels)
-        return self._build_common_labels(labels)
+        })
+        return labels


This part contained redundant labels before, first component was added, then the extra labels, then heritage and component again from PodReflector, and finally heritage again and app from _build_common_labels.

consideRatio · 2018-04-18T19:41:23Z

kubespawner/spawner.py

@@ -1008,6 +1003,9 @@ def get_pvc_manifest(self):
        Make a pvc manifest that will spawn current user's pvc.
        """
        labels = self._build_common_labels(self._expand_all(self.user_storage_extra_labels))
+        labels.update({
+            'component': 'singleuser-storage'
+        })


The call to _build_common_labels here is the only other place _build_common_labels is used. So the PVCs will get all the recommended labels as well.

yuvipanda · 2018-04-19T22:19:06Z

kubespawner/spawner.py

        labels.update(extra_labels)
+        labels.update({
+            'app': os.getenv('LABEL_APP', 'jupyterhub'),


I don't think we should be using enviornment variables directly. We should use traitlets here. KubeSpawner can also be used without helm, so chart and release shouldn't be here (as they are helm specific).

I think a generic way to do this would be to have the concept of 'identifying labels', which will be added to every pod / PVC. This can be set as a traitlet, and we can add appropriate values for it in the helm chart.

We must also be extremely careful here about backwards compatibility. We must test the following scenarios:

Pod started with older version of KubeSpawner (without this patch). Then KubeSpawner is updated to include this patch. KubeSpawner can still see the old pod, and is able to start / poll / stop it appropriately.

It might also be useful to introduce a 'kubespawner labeling scheme version' label, which will allow us to switch labeling schemes in an easier fashion in the future.

Thank you @yuvipanda for the feedback on this! I'll put in effort to think this through this, make sure we are backwards compatible without requiring a restart of all singleuser-server pods and stop using environment variables!

Should the pod culler also support traitlets ? It was using the environment variable LABEL_SELECTOR in order to know what pods were singleuser-server pods.

I have some learning to do regarding traitlets, but If you talk to me as if i get it all, I'll eventually do :)

Was made as preparation for [z2jh#625](jupyterhub/zero-to-jupyterhub-k8s#625).

Label selection on all labels may end up a bit much, so in the zero-to-jupyterhub-k8s repo I reduced it to `component`, `app`, and `release`. Kubespawner is not helm dependent and should not be linked to `release` though, and having custom configuration of label selection could be troublesome, as all singleuser-servers could require a restart or get their labels updated if the kubespawner setting was updated. In the future, perhaps we will tackle this?

consideRatio · 2018-05-13T13:51:20Z

@yuvipanda since your last review:

Now using traitlets instead of environment variables!
No longer coupling kubespawner with Helm!
I decided not to introduce the complexity of letting the PodReflector select based on labels configured through a traitlet, but instead finds all pods using the hardcoded selection component=singleuser-server. The main drawback of this is that we still won't support multiple deployment within the same namespace as we could by selecting on app=... and release=... labels as well. The benefit of this choice though is that we are fully backwards compatible and avoid adding complex logic and procedures on changes to the label selection.

This PR is a prerequisite for z2jh#625. I want to to additional testing before I consider this merge ready.

consideRatio · 2018-05-26T22:24:47Z

Tested this PR on my cluster for a while without issues, also verified labeling works as it should in conjunction with z2jh#625. If this kubespawner prerequisite PR is merged, I'll bump to utilize it in z2jh#625 and remove WIP for that also.

minrk · 2018-05-29T09:17:29Z

Thanks, @consideRatio!

consideRatio changed the title ~~Add recommended labels~~ [WIP] Add recommended labels Apr 18, 2018

consideRatio commented Apr 18, 2018

View reviewed changes

consideRatio requested a review from yuvipanda April 18, 2018 19:41

consideRatio mentioned this pull request Apr 18, 2018

.yaml refresher jupyterhub/zero-to-jupyterhub-k8s#625

Merged

21 tasks

consideRatio force-pushed the labels-pr branch from 309d7ef to d1bcfb5 Compare April 18, 2018 21:47

consideRatio changed the title ~~[WIP] Add recommended labels~~ Add recommended labels Apr 18, 2018

consideRatio changed the title ~~Add recommended labels~~ Add common labels for dynamically created Pods & PVCs Apr 19, 2018

consideRatio changed the title ~~Add common labels for dynamically created Pods & PVCs~~ Add common labels for Pods & PVCs Apr 19, 2018

yuvipanda requested changes Apr 19, 2018

View reviewed changes

consideRatio changed the title ~~Add common labels for Pods & PVCs~~ [WIP] Add common labels for Pods & PVCs Apr 20, 2018

consideRatio mentioned this pull request Apr 22, 2018

KubeIngressProxy docs #163

Closed

4 tasks

consideRatio added 4 commits May 13, 2018 15:14

added labels setable through env variables

9a54e5e

Was made as preparation for [z2jh#625](jupyterhub/zero-to-jupyterhub-k8s#625).

syntax error bugfix

60beeae

common_labels traitlets introduced

627ba48

consideRatio force-pushed the labels-pr branch from 8789a6a to 0f87c23 Compare May 13, 2018 13:39

consideRatio mentioned this pull request May 13, 2018

Multiple deployments in the same namespace #173

Closed

consideRatio changed the title ~~[WIP] Add common labels for Pods & PVCs~~ Add common labels for Pods & PVCs May 26, 2018

minrk merged commit cff7f01 into jupyterhub:master May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common labels for Pods & PVCs #161

Add common labels for Pods & PVCs #161

consideRatio commented Apr 18, 2018 •

edited

Loading

consideRatio Apr 18, 2018

consideRatio Apr 18, 2018

consideRatio Apr 19, 2018

consideRatio May 13, 2018

consideRatio Apr 18, 2018

consideRatio Apr 18, 2018

consideRatio Apr 18, 2018

yuvipanda Apr 19, 2018

consideRatio Apr 20, 2018

consideRatio commented May 13, 2018 •

edited

Loading

consideRatio commented May 26, 2018

minrk commented May 29, 2018

Add common labels for Pods & PVCs #161

Add common labels for Pods & PVCs #161

Conversation

consideRatio commented Apr 18, 2018 • edited Loading

Summary

Background

Details

History

Implementation notes

PR TODO

Excluded from this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio commented May 13, 2018 • edited Loading

consideRatio commented May 26, 2018

minrk commented May 29, 2018

consideRatio commented Apr 18, 2018 •

edited

Loading

consideRatio commented May 13, 2018 •

edited

Loading