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.

Sign up for GitHub

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

Jump to bottom

nixos/k3s: add initial k3s service #84783

Merged
merged 8 commits into from Apr 17, 2020
Merged

nixos/k3s: add initial k3s service #84783

merged 8 commits into from Apr 17, 2020

Conversation

euank
Copy link
Member

@euank euank commented Apr 9, 2020 •
edited

Motivation for this change

I thought that others might find this service definition handy! I'll use it whether we land it in the tree or not.

Things done

I added a k3s service with a few options that I personally find handy.

Note that this is my first nixos module, so it's very likely I'm missing something I should do. I don't know if I need to do more for docs, or if there's more testing I need to do.

I would definitely appreciate any pointers for stuff I might have missed or should do for a new module. If there's an exemplary pull request I could mimic, I'd be happy to take inspiration from that.

Testing done

I tested this on my nixos machine with the following config:

  services.k3s.enable = true;
  services.k3s.role = "server";
  services.k3s.docker = true;

With that configuration, running sudo k3s kubectl get pod and such worked as I'd expect.

@ngerstle
Copy link
Contributor

ngerstle commented Apr 9, 2020

Looks very similar to what I have, nice work!

ngerstle
ngerstle approved these changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@ngerstle ngerstle left a comment

Choose a reason for hiding this comment

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

Would like to see a package option exposed, but LGTM!

@misuzu
Copy link
Contributor

misuzu commented Apr 9, 2020 •
edited

It might make sense to separate server and agent to corresponding modules (and probably systemd units).

{
  services.k3s = {
    agent = {
      enable = true;
      docker = true;
      serverAddr = "https://10.0.0.10:6443";
      token = "sometoken";
    };
    server = {
      enable = true;
    };
  };
}

@euank euank force-pushed the nixos-k3s branch 6 times, most recently from e27a5b5 to 45f9f80 Compare April 10, 2020 07:01
@euank
Copy link
Member Author

euank commented Apr 10, 2020

Thanks for all the help cleaning this up @misuzu! I think I made all the improvements you suggested, and it looks quite a bit better for it.

On whether we should split agent and server, I'm not totally sure the right thing to do there.

I do think it makes more sense to keep it as one systemd service at least. The upstream documentation / installer assumes it's "k3s.service", so it makes it less surprising if "systemctl status k3s.service" etc works on nixos too.
There's also very little reason to want to run both services independently (i.e. the server role by default already runs the agent, so if you want both, you've already got both).

Because of the above, I ended up with the single config structure and single module.

nlewo
nlewo requested changes Apr 16, 2020
View reviewed changes
Copy link
Member

@nlewo nlewo 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 you contribution! This is a nice first module;)

Could you add a NixOS test for this module? You could for instance look at the docker test implementation: https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/docker.nix

nixos/modules/services/cluster/k3s/default.nix Show resolved Hide resolved
nixos/modules/services/cluster/k3s/default.nix Outdated
Type = "notify";
KillMode = "process";
Delegate = "yes";
LimitNOFILE = "infinity";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we want infinity everywhere. Is it required to make k3s working? A user really wanting this could still override these values.
So, if this is not strictly required, could you remove all these infinity values?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not strictly required, but it's often what you want I think.

If you're using the builtin containerd runtime in k3s, you'll hit nofile limits pretty quickly if you run a lot of pods. I removed it for now since I'm not totally confident in this.

nixos/modules/services/cluster/k3s/default.nix Outdated Show resolved Hide resolved
@euank euank requested a review from infinisil as a code owner April 17, 2020 06:49
@euank euank force-pushed the nixos-k3s branch 2 times, most recently from 245731b to 17ce3ec Compare April 17, 2020 06:55
offlinehacker
offlinehacker requested changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@offlinehacker offlinehacker left a comment

Choose a reason for hiding this comment

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

Just two small changes needed, but everything else seems ok. Thanks!

nixos/modules/services/cluster/k3s/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/k3s/default.nix Outdated Show resolved Hide resolved
@euank
nixos/k3s: add a test that k3s works; minor module improvements
c12c69e 
This is a single-node test. Eventually we should also have a multi-node
test to verify the agent bit works, but that one's more involved.
@euank
nixos/k3s: add option description
b1e1ae0
@euank
nixos/k3s: add defaults for token/serveraddr
47c75b6 
Now that the assertion enforces their presence, we dont' need to use the typesystem for it.
nlewo
nlewo requested changes Apr 17, 2020
View reviewed changes
nixos/tests/k3s.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/k3s/default.nix Outdated Show resolved Hide resolved
@euank
Copy link
Member Author

euank commented Apr 17, 2020

I think I made the changes you requested @offlinehacker, and I also added a test (though I only ran it a couple times, so beware it may have some inconsistent flake or race).

@nlewo
Copy link
Member

nlewo commented Apr 17, 2020

@euank You also need to reference the test in nixos/tests/all_tests.nix.

@nlewo
Copy link
Member

nlewo commented Apr 17, 2020

@GrahamcOfBorg test k3s

nlewo
nlewo approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@nlewo nlewo 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 your contribution!

@nlewo nlewo merged commit bc138f4 into NixOS:master Apr 17, 2020
@euank euank mentioned this pull request Nov 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misuzu misuzu misuzu left review comments

@offlinehacker offlinehacker offlinehacker approved these changes

@ngerstle ngerstle ngerstle approved these changes

@nlewo nlewo nlewo approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@euank @ngerstle @misuzu @nlewo @offlinehacker