search

jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
549 stars 305 forks source link

storage_extra_labels doesn't override common_labels #289

Open zkatona opened 5 years ago

zkatona commented 5 years ago

Summary by @consideRatio 2020-10-25

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:

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

Cheers,

Zoltán

consideRatio commented 5 years ago

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 =/

https://github.com/jupyterhub/kubespawner/blob/44d638b7c63576b7f88e82e6073de3f2ee2c4116/kubespawner/spawner.py#L1431-L1437

https://github.com/jupyterhub/kubespawner/blob/44d638b7c63576b7f88e82e6073de3f2ee2c4116/kubespawner/spawner.py#L1533-L1540

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

zkatona commented 5 years ago

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 _commonlabels. 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_extralabels.

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 _commonlabels, as it applies to more types of objects (more generic), then _extralabels (less generic). What do you think?

consideRatio commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Any progress here?

consideRatio commented 4 years ago

@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.