search

jupyterhub / kubespawner

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

Add modern app.kubernetes.io labels alongside old #835

Closed consideRatio closed 1 month ago

consideRatio commented 2 months ago

This is related to https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/3404, where I'd long term like to enable us to transition to using modern k8s labelling scheme, where labels outlined in https://github.com/jupyterhub/zero-to-jupyterhub-k8s/issues/1867 are used instead of labels used in legacy helm charts.

This change is made to be entirely non-breaking.

manics commented 2 months ago

There's an ongoing discussion about how names (including pod labels) are normalised, and whether this can be changed in a backwards compatible manner:

If we want to do the above this might be a good opportunity to set the new labels to whatever the new normalisation scheme is, and leave the old labels unchanged, avoiding some backward compatibility complexity. It won't avoid all complexity since it's not just labels that are affected.

consideRatio commented 2 months ago

If we want to do the above this might be a good opportunity to set the new labels to whatever the new normalisation scheme is

Are you refering to the opportunity of getting things out in a z2jh release including a new kubespawner version, or alongisde this PR? I see this PR as practically unrelated to what we do with other labels.

I think adding new labels instead of changing things in old could make sense - very worth considering.

yuvipanda commented 2 months ago

I'm pro using this as an opportunity to fix our myriad escaping issues! Some variant of https://github.com/jupyterhub/kubespawner/pull/744 would be pretty amazing.

I don't currently have capacity to push on that though :(

consideRatio commented 2 months ago

I opened #836 to focus on the related discussion

consideRatio commented 1 month ago

After discussion in #836, do you think this is OK to review/merge @manics?

consideRatio commented 1 month ago

Thank you for reviewing @sgaist! - I'll go for a merge!

yuvipanda commented 4 weeks ago

I very much commend you for working on this and merging it, @consideRatio. To redescribe a sentiment I described in https://github.com/jupyterhub/oauthenticator/pull/735#issuecomment-2159213296, it's really important to keep momentum of review going to enable contributors to feel motivated. Without enough contributor motivation, projects slowly wither and die. And I don't want that.

However, another part of that is being able to revert PRs as well, especially before things get released. With that, I think this PR should be reverted. I've my rationale below.

I'm really confused by this PR now, as it looks like in the state it was merged, it adds new labels but doesn't listen based on them. I also wish I had expressed myself more clearly in https://github.com/jupyterhub/kubespawner/pull/835#issuecomment-2081207827 - that's on me, I'm sorry. Let me try again:

  1. We currently have a major issue with our escaping, that breaks silently under some circumstances based on what letter things start with - see https://github.com/jupyterhub/kubespawner/pull/835#issuecomment-2075996346
  2. One part of the problem fixing this is that escaping and state management is hard to figure out correctly in a backwards compatible way.
  3. However, we do have one advantage - we can change the key of the label as well, not just the escaping of the value.
  4. So I'd have liked to see the modern applabels use the newer escaping (#791, #744) from the start, so we can make a clear determination of what escaping method is used where. Old labels use old escaping, new labels new escaping.
  5. After that, we can introduce a traitlet like disable_old_labels (or similar) that will just completely stop the old style labels, and default that to False. And then a few releases later, default that to True.

So the fact that we did not have the new style labels was an advantage, and with this PR we lose that. It's a tool we should and could employ in https://github.com/jupyterhub/kubespawner/pull/835#issuecomment-2075996346, and I don't want to lose that.

So I'd say switching to new label styles must be blocked on fixing the issues in https://github.com/jupyterhub/kubespawner/pull/835#issuecomment-2075996346. That means either directing efforts towards those PRs, or waiting until someone else does. I don't think it's necessary to tie these into https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/3404, but if that is necessary, I think the path forward is for that to be reverted and for that to wait as well. I don't see what advantage simply repeating the label (without watching for it) gives us, and it makes us lose an important tool in being able to solve #737.

And finally, I understand if the moment has passed and the choice is made to just continue with it as is - because I didn't chime in before this was merged. There was enough time for that. So I'll accept a 'you should have spoken up earlier' response to this as well, and work to organize myself for making that happen better. I'm going to try to systematically spend x minutes every week reviewing and merging PRs now.

yuvipanda commented 3 weeks ago

Just helped another person with it on slack https://github.com/jupyterhub/kubespawner/issues/737#issuecomment-2161440136

minrk commented 3 weeks ago

I think I can dedicate some time later this week or next to picking #744 back up with these new labels in mind and finally think through a transition path.

yuvipanda commented 3 weeks ago

yay thank you, @minrk

yuvipanda commented 3 weeks ago

In conversation with @minrk today, he corrected me that this PR doesn't actually add any values that need escaping. Sorry for not catching that correctly on my first round! This does mean I no longer think we need to revert this PR, although I'd still love for us to get #744 done along with this, but I don't think they are particularly connected anymore.

yuvipanda commented 3 weeks ago

And to quote @minrk from our conversation,

Yeah, I really think it’s a fine PR with very little impact

So I think it was ok to self-merge this too, @consideRatio. I was just wrong, and you were right in https://github.com/jupyterhub/kubespawner/pull/835#issuecomment-2143373777. I apologize for my mistake here.

I'm now going to try to spend a minimum of 15 minutes every day reviewing and merging PRs across the JupyterHub ecosystem, with a focus on encouraging more newer contributors. I think that's the systematic solution to the systemic problem I've encountered here. Specifically, I'll also try to read the code first rather than the comments, as I think that was the particular thing that tripped me up here.

Not the first time I've had an opinion that later turned out to be wrong, and not the last either. I appreciate everyone holding space for me to make mistakes, and I promise to do the same.