Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

packaging: Add post-installation fixup for systemd-resolved for kubelet #63632

Conversation

randomvariable
Copy link
Member

@randomvariable randomvariable commented May 9, 2018

What this PR does / why we need it:

On systemd-resolved based systems, /etc/resolv.conf gets rewritten such that the listed nameserver is not compatible with containers run by kubelet.

This PR adds a post-installation script to Debian packaging to check if /run/systemd/resolve/resolv.conf is available, and if so place a systemd override for kubelet to set up the DNS resolution correctly.

This is needed for at least all versions of Ubuntu 17.10 onwards.

Additionally, a small fix up to kubernetes-cni to ensure /opt/cni/bin exists, otherwise package doesn't install on minimal ubuntu installs.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to kubernetes/kubeadm#787
Not marking as fixed, as other tasks need to be considered:

  • Creating a preflight check in kubeadm for this scenario
  • Taking back control over the kubelet config
  • Switching to CoreDNS by default
  • Trying to mitigate this issue by specifying {some extra parameter} in the CoreDNS configmap

Special notes for your reviewer:
This is still using the deprecated command line flags, but is a minimal change which should get these
distros working out of the box. It may also be at odds to the dynamic kubelet configuration.

Release note:

kubelet package for debian systems now supports systemd-resolved (Ubuntu 17.10+ / Debian Stretch+)

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 9, 2018
@randomvariable
Copy link
Member Author

/sig cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label May 9, 2018
@randomvariable
Copy link
Member Author

randomvariable commented May 9, 2018

/assign luxas as the main discussant from kubeadm office hours on 2018-06-09

@@ -0,0 +1,10 @@
#!/bin/sh

if [ -f "/run/systemd/resolve/resolv.conf" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the systemd-resolved service is stopped, this file exists and contains old data? We should probably change this check to:
if systemctl is-active systemd-resolved; then

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Will update when I've got a moment

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved in newer commit. Will squash after approval.

@detiber
Copy link
Member

detiber commented May 14, 2018

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 14, 2018
@randomvariable randomvariable force-pushed the packaging/debian/systemd-resolved branch from f1ce8f0 to d67dc06 Compare May 14, 2018 16:59
@detiber
Copy link
Member

detiber commented May 14, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2018
@detiber
Copy link
Member

detiber commented May 14, 2018

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-kubemark-e2e-gce

@mikedanese
Copy link
Member

/assign

@detiber
Copy link
Member

detiber commented May 15, 2018

Related to kubernetes/kubeadm#273

@mikedanese
Copy link
Member

A couple comments/questions:

  • Should we make the same changes in the rpm build in build/rpms? It would be nice to keep deb and rpm as similar as possible.
  • Do we need to make these changes in k8s.io/release? Those are the builds that produce the packages we deploy to apt and yum repos. These are used primarily for CI.

@randomvariable
Copy link
Member Author

randomvariable commented May 15, 2018

Fedora isn't currently using systemd-resolved, and are sticking with NetworkManager for now, and SUSE appears to be in the same boat. If someone's gone to the effort of explicitly configuring systemd-resolved on those systems, they should be to manually configure overrides.
I might do a separate documentation update about it though.

As far as k/release goes, I can't see package builds happening there, just the rule definition of k8s_deb.
I thought CI would be taking outputs from here? Can't seem to find the debian packages in the artifacts though.

@mikedanese
Copy link
Member

We use dpkg-buildpackage in k8s.io/release. See here

@randomvariable randomvariable force-pushed the packaging/debian/systemd-resolved branch from ad4d130 to c870d57 Compare May 18, 2018 08:41
Check if systemd-resolved is active and if so place a systemd override
to set up the DNS resolution correctly.

Signed-off-by: Naadir Jeewa <naadir@randomvariable.co.uk>
@randomvariable randomvariable force-pushed the packaging/debian/systemd-resolved branch from c870d57 to 08af346 Compare May 18, 2018 08:45
@randomvariable
Copy link
Member Author

@mikedanese ah, thanks. as we have per distro builds there i would do it differently. will open issue

@randomvariable randomvariable changed the title debian: Add post-installation fixup for systemd-resolved for kubelet packaging: Add post-installation fixup for systemd-resolved for kubelet May 18, 2018
@randomvariable
Copy link
Member Author

/test pull-kubernetes-integration

@randomvariable
Copy link
Member Author

/test pull-kubernetes-e2e-gce

@randomvariable randomvariable force-pushed the packaging/debian/systemd-resolved branch from 08af346 to 55de637 Compare May 18, 2018 10:09
@randomvariable
Copy link
Member Author

/test pull-kubernetes-e2e-kops-aws

@randomvariable
Copy link
Member Author

/test pull-kubernetes-e2e-gce

@neolit123
Copy link
Member

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce

1 similar comment
@neolit123
Copy link
Member

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce

@neolit123
Copy link
Member

/test pull-kubernetes-e2e-gce

@detiber
Copy link
Member

detiber commented May 18, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: detiber, randomvariable
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mikedanese

Assign the PR to them by writing /assign @mikedanese in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@neolit123
Copy link
Member

/test pull-kubernetes-e2e-gce

3 similar comments
@randomvariable
Copy link
Member Author

/test pull-kubernetes-e2e-gce

@neolit123
Copy link
Member

/test pull-kubernetes-e2e-gce

@neolit123
Copy link
Member

/test pull-kubernetes-e2e-gce

@neolit123
Copy link
Member

/joke

@k8s-ci-robot
Copy link
Contributor

@neolit123: I had a dream that I was a muffler last night. I woke up exhausted!

In response to this:

/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@luxas
Copy link
Member

luxas commented May 22, 2018

See kubernetes/kubeadm#845 for an alternative approach.
Do you think we should do this for kubelets generally or only for kubelets that kubeadm manage?

@praseodym
Copy link
Contributor

I think it makes a lot of sense to have all kubelet flags managed by the Kubernetes installer (kubeadm in this case) and keep any post-install logic out of the kubelet package.

@randomvariable
Copy link
Member Author

I agree. Was initially worried about getting too close to release and not having something working out of the box on Ubuntu 18.04.

@luxas
Copy link
Member

luxas commented May 22, 2018

@randomvariable Please do work on implementing kubernetes/kubeadm#845 ;)
The PR it depends on should get merged tonight or so.
Yeah thanks for working on this (!), but I think we'll have time to do "the real thing" 😄

I do agree with you both though that minimizing what the debs/rpms do, and letting the installer (in this case kubeadm) have full control over the env makes a lot of sense. We don't have any other post-install logic or preferred args for the kubelet as-is either. With default args the kubelet is kinda useless (no authn/authz for example), it really depends on getting all the bits in place.

So I'm gonna close this for now, and say we're letting kubeadm control all the bits. If we can't get that happen, we can always reopen this as a last resort to getting it fixed for the v1.11 release.

@luxas luxas closed this May 22, 2018
@randomvariable
Copy link
Member Author

Yeah, defo treat this as last resort. Can start on kubernetes/kubeadm#845 later in the week.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants