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

k3s: add multiple versions #214026

Merged
merged 1 commit into from
Feb 8, 2023
Merged

k3s: add multiple versions #214026

merged 1 commit into from
Feb 8, 2023

Conversation

superherointj
Copy link
Contributor

@superherointj superherointj commented Feb 1, 2023

Fixes #213943

This PR is a start offering some structure to having multiple versions.
Listing all versions we have available right now. Newer versions require further work.
I'm trying to keep things simple here. (so no fancy abstraction, DRY, etc)

  • k3s (k3s_1_26)
    • k3s_1_24 (1.24.4+k3s1) (1.24.10+k3s1 is available at upstream.)
    • k3s_1_25 (1.25.3+k3s1) (1.25.6+k3s1 is available at upstream.)
    • k3s_1_26 (1.26.1+k3s1) (in sync with upstream)

NixOS tests and update script are disabled in legacy versions.

k3s_1_26 is now rolling. Update script and tests are working just as were before. (No changes for latest!)

Patches are duplicated on purpose for when time is due directory can be deleted and nothing will be affected.

Due to priorities, I do not plan doing upgrades, back-ports, tests, update script for previous releases. But PRs are welcomed. And would be better someone with genuine interest in this to step up. Hopefully, as we move forward, we can get these things to a better state.

I might improve latest update script to automatically add new major versions in a new folder and pin previous version.

So basically what we have here, is exactly what we had before (latest rolling versions), plus previous releases we had. No changes. (I did copy and paste from history. Changed just a few minor issues that were patched, like cross compilation patch.)


This PR is awaiting review from maintainers: @euank @Mic92

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Feb 1, 2023
@ofborg ofborg bot requested review from euank and Mic92 February 1, 2023 19:19
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 1, 2023
Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Absolutely awesome! ❤️

@yajo
Copy link
Contributor

yajo commented Feb 2, 2023

Do you think this could be backported to 22.11? To ease migration from 22.05... Otherwise I can just overlay them, and the problem will be fixed for at least 23.05 onward.

@superherointj
Copy link
Contributor Author

Do you think this could be backported to 22.11? To ease migration from 22.05...

Back-porting is always possible, with fixes - if necessary.
Feel free to make a PR back-porting to 22.11 and 22.05.

Otherwise I can just overlay them, and the problem will be fixed for at least 23.05 onward.

Up to you.

--

Personally, I'd rather spend time moving things forward than backwards.

@yajo
Copy link
Contributor

yajo commented Feb 2, 2023

Personally, I'd rather spend time moving things forward than backwards.

Makes sense indeed. I'll just overlay what I need. Thanks for the help! 😊

@mweinelt
Copy link
Member

mweinelt commented Feb 2, 2023

Do we really need all four versions? Especially, 1.23 with less than a month of support looks pretty pointless to me. How about reducing the set to the actively maintained ones?

Personally, I'd rather spend time moving things forward than backwards.

That does resonate with me quite a bit.

[1] https://endoflife.date/kubernetes

@superherointj
Copy link
Contributor Author

Do we really need all four versions?

I have no strong opinion on this.

Especially, 1.23 with less than a month of support looks pretty pointless to me.

Release 22.05 is pinned to 1.23.

How about reducing the set to the actively maintained ones?

I wouldn't mind reducing the set.

Will remove 1.23 then.

[1] https://endoflife.date/kubernetes

Can follow this.

@yajo
Copy link
Contributor

yajo commented Feb 3, 2023

I think that yes, we need 1.23. Quoting from #213943:

to have a successful HA cluster upgrade, you need to:

Upgrade your server nodes to the latest patch version available. One node at a time.
[...]
Latest k3s in NixOS 22.05:

k3sVersion = "1.23.10+k3s1"; # k3s git tag

However, upstream's latest for that minor version is v1.23.16+k3s1.

I can add myself as maintainer to help you after this is merged, and propose the upgrades.

@euank
Copy link
Member

euank commented Feb 3, 2023

I'm okay keeping 1.23 since it's in "maintenance mode" for a couple weeks yet. If, hypothetically, there were a CVE that were patched for 1.23, and someone were using an older nixpkgs with an insecure 1.23, it would be safest for them to update to the latest patch version of 1.23 to be secure.

I think that's a reasonable enough argument for keeping it around, and then dropping it once it won't see security releases (admittedly, in 3 weeks).

I, personally, expect that going forward it won't be much of a burden to maintain a few extra older versions. Typically, maintenance work comes with upstream packaging changes, and those shouldn't happen for release branches, except in unusual cases.
Once the update script is updated appropriately, we'll be able to just let the update script do it's job for historical supported versions.

I'll leave a separate review comment regarding the actual PR, but figured this discussion about what to keep would be easier to respond to separately.

Copy link
Member

@euank euank left a comment

Choose a reason for hiding this comment

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

Thanks for opening this @superherointj! This definitely seems helpful to have, and like a good first step. As you said, this is very much not fancy (no DRY, no automatic-update-script-ing for old releases, etc), but this is still an improvement to the current state of things I think.

I think there are a few things that we can weigh here as for this approach. If you don't mind, I'll just kinda dump my thoughts here.
Basically, there's two motivating reasons we should keep around supported versions:

  1. To allow people to correctly update their clusters (i.e. step forward the api server, then the kubelet, without having to use different nixpkgs versions across the cluster)
  2. To allow people to consume patches for bugs and security issues without having to do more difficult upgrades (i.e. to consume a CVE fix, without also having to deal with k8s deprecations, by letting them stick on the same minor version)

This PR, if you follow k8s's recommendations exactly, actually does neither of those things. 2 obviously requires also spending time updating the patch version for old releases.
1 would work in practice, but K8s docs tell you to update to the latest patch version before introducing minor version number skew... again, in reality, it rarely matters.

The argument against this, I think, is that introducing old patch versions is kinda providing an "attractive nuisance", in that it lets people think they might be getting 1 & 2, but in reality they get sorta-1 and not 2 at all.

That all said, the other argument for this is that it doesn't do 1 or 2, but it's an incremental step towards 1 & 2. The first step of having up-to-date old versions is having old versions in the first place.

This last bit is why I'm in favor of this, even without the DRY etc, because I think this lays the foundation to make those next steps (patch backporting, automating old patch updates, etc) happen more quickly.

... Lotta words, I know, sorry! The tl;dr is just thanks, the PR is appreciated, let's try to followup with patch updates and update script automation before too long!

I'm hoping to find some time soon to tackle a new update script, if you're not already planning to!

@mweinelt mweinelt added 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Feb 5, 2023
@superherointj
Copy link
Contributor Author

Hi @euank,

First, sorry for the delay in answering you.

My approach was simplistic indeed. I wouldn't mind if you have a better idea for implementing this. Either I could merge this as it is now (without 1.23 even?) and then you could change anything you would like (you have my green light for sure). Or another option would be to just start a new PR closing this.

Regarding the update script, if I were to do it I'd likely do it much later, so please, feel free to do it.

Just let me know which path forward you prefer.

@euank
Copy link
Member

euank commented Feb 5, 2023

Just let me know which path forward you prefer.

I was trying to say in that long-winded review comment that I'm in favor of merging this, and then iterating on it!

I think this is an improvement over the current state of things, and I'm not confident I (or someone else) will do a better iteration super soon!

@yajo
Copy link
Contributor

yajo commented Feb 6, 2023

I would appreciate it if you could restore the 1.23 derivation because that's the one I'd need right now and, after all, the job was already done, so why not?

I noticed that most packages are a bit outdated, but I think it would be better to just merge this foundational PR, so we can do further updates later in smaller PRs, one by one. If we wait too much, 1.23 will disappear before I can fix it! 😢

1 would work in practice, but K8s docs tell you to update to the latest patch version before introducing minor version number skew... again, in reality, it rarely matters.

I also think that the latest patch upgrade probably is not necessary... but TBH k8s is so complex and important that I prefer to be extra cautious here. 😅

@superherointj
Copy link
Contributor Author

superherointj commented Feb 7, 2023

I would appreciate it if you could restore the 1.23 derivation because that's the one I'd need right now
If we wait too much, 1.23 will disappear before I can fix it! cry

Don't cry, just learn Nix. No package really disappears.

You can always use any past versions already, you just need to add a new input to your flake referencing the commit, and inherit package from that. Nix magic.

and, after all, the job was already done, so why not?

I deleted it. -.-
Just added back. :)

I noticed that most packages are a bit outdated, but I think it would be better to just merge this foundational PR, so we can do further updates later in smaller PRs, one by one.

Yes...

1 would work in practice, but K8s docs tell you to update to the latest patch version before introducing minor version number skew... again, in reality, it rarely matters.

I've been meaning to get to this PR for a week already. K3s is a priority for me because I use it in my Pi cluster. But time is limited. I or euank may bump it on and off, but it is not guaranteed.
For a perfect legacy support in K3s, consider participating and bumping it yourself. It is not that hard.

I also think that the latest patch upgrade probably is not necessary...

Questions really is to whom...

but TBH k8s is so complex and important that I prefer to be extra cautious here. sweat_smile

No doubt.

@superherointj
Copy link
Contributor Author

Anyone cares to review/approve again? So I can merge this...

@ofborg ofborg bot requested a review from euank February 7, 2023 15:42
@mweinelt mweinelt removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Feb 8, 2023
@superherointj superherointj merged commit 2122d22 into NixOS:master Feb 8, 2023
@superherointj superherointj deleted the refactor-k3s-multiple-versions branch February 8, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

k3s: package all supported versions
4 participants