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

kubernetes: 1.19.5 -> 1.20.4 (dockerd -> containerd) #114737

Merged
merged 3 commits into from
Mar 7, 2021

Conversation

johanot
Copy link
Contributor

@johanot johanot commented Mar 1, 2021

Motivation for this change

This is an alternative to #109275 and #114722

Since docker 20.X has removed dockershim and Kubernetes as well has deprecated docker as container-runtime, it seemed sensible to switch to containerd as default runtime for the NixOS module.

The standard RBAC and DNS tests pass on my machine: nix-build nixos/release.nix -A tests.kubernetes.rbac.singlenode -A tests.kubernetes.rbac.multinode -A tests.kubernetes.dns.singlenode -A tests.kubernetes.dns.multinode

The PR is WIP because it still lacks documentation. Notably rel-notes, but probably also updates to sec-kubernetes in the manual.

Also, I'd like reviewers input to the question of how production ready the container runtime should be configured? Currently, I've gone with a minimal containerd.toml and minimal systemd parameters for the service. For production workloads, one likely want to configure things like ulimiits, oom-scores, cgroup params etc. Some of these settings are already set explicitly for virtualisation.docker, so users might expect this to be taken care of upstream.

Let me hear your thoughts.

cc @saschagrunert @srhb

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 1, 2021
@blaggacao
Copy link
Contributor

I believe the kube-addons manager can be provided with a deprecation message since upstream did deprecate the concept (not meant to block anything, jut a chore idea).

The CI tests fail at the same point as temhe other two PRs for good reason, the attribute is effectively not devlared in top.

Thank you a lot for pushing this!


# iptables must be disabled for kubernetes
extraOptions = "--iptables=false --ip-masq=false";
systemd.services.containerd =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think creating a containerd module would be better rather than adding it here, would also allow it to be used independent of k8s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look into separating this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First version separate module here (7fc7d420b32f8dae291990a8ef5f6670a98f43d4)

@johanot
Copy link
Contributor Author

johanot commented Mar 3, 2021

Heads up for reviewers: 964ee25fed6772b955f6d5d603bbf9a4a8385e2a adds limits and oomscore for the containerd systemd-service. Personally, I feel infinity is quite aggressive here; that is nonetheless what upstream "recommends" (https://github.com/containerd/containerd/blob/master/containerd.service#L29-L37). Feedback would be much appreciated here.

@johanot johanot force-pushed the kubernetes-1.20-with-containerd branch from e9b2d54 to 73946c9 Compare March 3, 2021 11:34
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 8.has: changelog 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 3, 2021
@johanot
Copy link
Contributor Author

johanot commented Mar 4, 2021

Added release note.

IMHO manual changes are not needed for now, since sec-kubernetes doesn't mention docker specifically. With regards to the deprecated addon-manager, I plan to create an issue on removing this, but that's likely gonna be a next-release (21.11) thing.

(kind of expected we now have rel-note merge conflicts, will rebase nixos-unstable shortly)

@johanot johanot force-pushed the kubernetes-1.20-with-containerd branch from ba0d335 to 0c54bc9 Compare March 4, 2021 12:01
@johanot johanot marked this pull request as ready for review March 4, 2021 12:02
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 4, 2021
@johanot johanot force-pushed the kubernetes-1.20-with-containerd branch from 0c54bc9 to 5e0b48e Compare March 4, 2021 13:14
@zowoq
Copy link
Contributor

zowoq commented Mar 5, 2021

LGTM, thanks @johanot.

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@zowoq zowoq force-pushed the kubernetes-1.20-with-containerd branch from 5e0b48e to 7b5c38e Compare March 7, 2021 02:52
@zowoq
Copy link
Contributor

zowoq commented Mar 7, 2021

Rebased on master to resolve a merge conflict in rl-2105.xml.

I'm running the nixos tests , I'll merge after they pass.

@ofborg ofborg bot requested a review from saschagrunert March 7, 2021 03:01
@zowoq
Copy link
Contributor

zowoq commented Mar 7, 2021

@ofborg build kubernetes

@zowoq zowoq merged commit 05f5a98 into NixOS:master Mar 7, 2021
@johanot johanot deleted the kubernetes-1.20-with-containerd branch March 7, 2021 12:17
@johanot johanot mentioned this pull request Mar 7, 2021
10 tasks
@blaggacao
Copy link
Contributor

For informative purposes only: kustomize is going to land soon as part of kubectl with kubernetes/kubernetes#98946

We should try to get this change into packages before next NixOS release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants