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

Upgrade k3s version to v1.27.9+k3s1 #108

Closed
wants to merge 1 commit into from

Conversation

RobbieBuxton
Copy link
Collaborator

Added an overlay to upgrade k3s to the version on unstable so that we get the bug fix mentioned in #102 . Also removed the containerd overlay as the 23.11 contains the fix we required.

closes #102

@RobbieBuxton
Copy link
Collaborator Author

@antoinerg do you mind testing this locally? For some reason I can't seem to replicate the bug. Thanks!

Comment on lines +7 to +24
# Fixes https://github.com/pdtpartners/nix-snapshotter/issues/102 due to 23.11 only supporting v1.27.6 while we need v1.27.7.
# Remove once we upgrade to the next stable release.
k3s = super.k3s.overrideAttrs (oldAttrs: {
k3sRepo = super.fetchgit {
url = "https://github.com/k3s-io/k3s";
rev = "v1.27.9+k3s1";
sha256 = "sha256-Zr9Zp9pi7S3PCTveiuSb0RebiGZrxxKC+feTAWO47Js=";
Copy link
Contributor

Choose a reason for hiding this comment

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

@RobbieBuxton I just wanted to point out that nixpkgs-unstable has a recent enough version (it's currently 1.27.9+k3s1). Should we depend on this one instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is the same version as what unstable has and I don't think the implementation of k3s has changed since the introduction of 23.11 so this is a nice way to get an updated package without having to subscribe to two version of nixpkgs.

@antoinerg
Copy link
Contributor

antoinerg commented Jan 20, 2024

@antoinerg do you mind testing this locally? For some reason I can't seem to replicate the bug. Thanks!

Please see the extra flag needed at https://github.com/pdtpartners/nix-snapshotter/pull/109/files#diff-d6431f0c929f50d0ee7cc009dafb4a0f0b45b124e8da1a2643622bfee77204a2. It won't work without this!

@antoinerg
Copy link
Contributor

For some reason I can't seem to replicate the bug.

That's surprising to me. Are you testing via the VM? So doing nix run ".#vm", logging in, and then run kubectl apply -Rf /etc/kubernetes/redis?

@RobbieBuxton
Copy link
Collaborator Author

For some reason I can't seem to replicate the bug.

That's surprising to me. Are you testing via the VM? So doing nix run ".#vm", logging in, and then run kubectl apply -Rf /etc/kubernetes/redis?

Yeah, not entirely sure why, might be something to do with cache like I think you mentioned a few days ago explaining why the CI isn't picking it up either. Haven't had time to thoroughly test though.

@RobbieBuxton
Copy link
Collaborator Author

RobbieBuxton commented Jan 20, 2024

Added @antoinerg's fix from #109

@RobbieBuxton
Copy link
Collaborator Author

Would appreciate the review as well @elpdt852 when you have a moment, thanks!

Comment on lines 7 to 13
# Depends on PR merged into main, but not yet in a release tag.
# See: https://github.com/containerd/containerd/pull/9028
containerd = super.containerd.overrideAttrs(o: {
src = self.fetchFromGitHub {
inherit (o.src) owner repo;
rev = "779875a057ff98e9b754371c193fe3b0c23ae7a2";
hash = "sha256-sXMDMX0QPbnFvRYrAP+sVFjTI9IqzOmLnmqAo8lE9pg=";
Copy link
Contributor

Choose a reason for hiding this comment

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

I smoked test the latest changes but k3s wouldn't come up healthy. Also, I am wondering why containerd has been removed here? cc @RobbieBuxton

Copy link
Collaborator Author

@RobbieBuxton RobbieBuxton Jan 22, 2024

Choose a reason for hiding this comment

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

Sorry, I was being silly there I shouldn't have removed that. I thought I had forgotten to remove that when upgrading to "23.11" in my last PR but the required fix isn't actually included in the current version on stable containerd 1.7.11. I'll revert that now, thanks for picking that up!

Signed-off-by: Robbie Buxton <robbiesbuxton@gmail.com>

Co-authored-by: Antoine Roy-Gobeil <antoine@plot.ly>
Co-authored-by: Robbie Buxton <robbiesbuxton@gmail.com>
@antoinerg
Copy link
Contributor

antoinerg commented Jan 22, 2024

@RobbieBuxton the way I test is described in #102 (comment). When following the steps from that comment and starting from your branch, I end up with a VM with:

[root@nixos:~]# k3s --version
k3s version v1.27.6+k3s1 (bd04941a)
go version go1.20.11

Somehow the newer k3s version does not find its way into the VM!

@RobbieBuxton
Copy link
Collaborator Author

RobbieBuxton commented Jan 22, 2024

@RobbieBuxton the way I test is described in #102 (comment). When following the steps from that comment and starting from your branch, I end up with a VM with:

[root@nixos:~]# k3s --version
k3s version v1.27.6+k3s1 (bd04941a)
go version go1.20.11

Somehow the newer k3s version does not find its way into the VM!

@RobbieBuxton the way I test is described in #102 (comment). When following the steps from that comment and starting from your branch, I end up with a VM with:

[root@nixos:~]# k3s --version
k3s version v1.27.6+k3s1 (bd04941a)
go version go1.20.11

Somehow the newer k3s version does not find its way into the VM!

So yeah, turns out overriding k3s is a lot more of a pain than I previously thought and it wasn't being overridden properly at all. I think your original approach of just taking unstable is might better after all. Sorry about all that! We should probably just use your PR.
EDIT: I am also able to reproduce the bug now as well which is nice.
EDIT2: maybe a patch of the new commit would work, I'll have a look into it tonight

@elpdt852
Copy link
Collaborator

elpdt852 commented Jan 22, 2024

turns out overriding k3s is a lot more of a pain than I previously thought

Yeah I recall looking into this and didn't add my patch in an overlay due to how the upstream derivation was structured. If nixpkgs-unstable already has k3s with my PR merged, then we should just switch back to nixpkgs-unstable. I will take a look at this PR tonight.

Excited to have rootless k3s + nix-snapshotter working as a home-manager module.

@elpdt852 elpdt852 added the ok-to-test Runs NixOS tests label Jan 22, 2024
@antoinerg
Copy link
Contributor

antoinerg commented Jan 22, 2024

I think your original approach of just taking unstable is might better after all. Sorry about all that! We should probably just use your PR.

No worries! Feel free to review #109 then!

cc @elpdt852

@antoinerg
Copy link
Contributor

Please consider #110 which is even simpler!

@RobbieBuxton
Copy link
Collaborator Author

#110 is a better PR so closing this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Runs NixOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

k3s example: error pulling image
3 participants