storage_extra_labels doesn't override common_labels

[zkatona]

Summary by @consideRatio 2020-10-25

• Labels will be set on creation, there is nothing to do about this.
• storage_extra_labels are applied before common_labels, which means that common_labels will override storage_extra_labels. It is open for discussion if we want that or not. I think it would be fine to change this order. What is important is that the selector label, which is currently a single label hardcoded to component: with a value configurable with .component_label, is applied last.

---

Hi!

The problem I describe here is about assigning labels to PVCs, but before a PVC gets created: c.KubeSpawner.storage_extra_labels cannot update a label, which was created beforehand with c.KubeSpawner.common_labels. I have seen that c.KubeSpawner.storage_extra_labels can assign new labels, but it cannot overwrite labels that were already assigned by c.KubeSpawner.common_labels.

The issue was not present in KubeSpawner version 0.8.1, but it is present since at least 0.10.1.
I use:

• OpenShift Master: v3.10.83
• Kubernetes Master: v1.10.0+b81c8f8

Any ideas what could be the reason?
Thank you for your help in advance!

Cheers,

Zoltán

[consideRatio]

Ah, one could argue that we should have the common labels applied first... Or not have it be this complicated... I'm responsible for a lot of this =/

kubespawner/kubespawner/spawner.py

Lines 1431 to 1437 in 44d638b

	def _build_common_labels(self, extra_labels):
	# Default set of labels, picked up from
	# https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md
	labels = {}
	labels.update(extra_labels)
	labels.update(self.common_labels)
	return labels

kubespawner/kubespawner/spawner.py

Lines 1533 to 1540 in 44d638b

	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.storage_extra_labels))
	labels.update({
	'component': 'singleuser-storage'
	})

A workaround seem to be to configure common_labels' such as app or heritage in common_labels rather than extra_storage_labels.

[zkatona]

My scenario is the following: I want to avoid accidental deletion of PVCs, by assigning a different app label to the PVC than to other objects and pods. The default JupyterHub image I use already assigns a default app label using the common_labels. This app label is what I want to overwrite in my custom JupyterHub image, but I want to overwrite it only for PVCs. That's why I need to use storage_extra_labels.

So the workaround you offered me has the drawback that I would overwrite the app label not just for PVCs, but also for pods. This is not the intended behavior. What is your plan, regarding this bug? (I guess this behavior is not a feature, is it?)

BTW thinking a little more about it, maybe it makes more sense to apply the labels going from more generic ones to less generic ones. In the code you inserted, this would mean to apply first common_labels, as it applies to more types of objects (more generic), then extra_labels (less generic). What do you think?

[consideRatio]

Yeah kinda think alike!

But hey do you know about deletionPolicys? You can make a PV remain even though you delete its PVC.

I wanted to make sure i did not mess all my PVs up by mistake by deleting PVCs managed for example by helm etc.

If you make it "retain" instead of "delete". It matters. You can do this on your storageclass.

You may need to recreate it though because i dont think you can edit it in place but thats still fine.

You will need to update all already created PVCs though, or is it the PVs? Hmmm forgot...

[zkatona]

In my case, I want to apply those labels, before any PVCs are created. Kind of default settings to use when the PVC should be created. What happens afterwards to the PVC, is not relevant for me from this perspective.

[zkatona]

Any progress here?

[consideRatio]

@zkatona to conclude, it is my understanding that you want to have a specific label be overridden for the PVCs, so that you avoid targeting it for deletion by mistake.

It is vital that the labels that are meant to scan for pods / pvcs managed by kubespawner is never changed without KubeSpawner knowing it, but the common labels doesn't have such purpose, only the component: label has such purpose.

So, it could be acceptable to apply extra_labels after the common labels I think, it may make the most sense.