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

Switch core master base images (kube-apiserver, kube-scheduler) from debian to distroless #90674

Merged
merged 1 commit into from May 9, 2020
Merged

Switch core master base images (kube-apiserver, kube-scheduler) from debian to distroless #90674

merged 1 commit into from May 9, 2020

Conversation

dims
Copy link
Member

@dims dims commented May 1, 2020 •
edited by justaugustus

Kudos to @yuwenma for all the earlier work on this that made this PR possible!

Premise : Since the PR for switching on --log-file in the manifests ran into trouble and got reverted a few times, trying another approach here. The key idea here is that we need to

  • Keep the same behavior we have now with redirection of stdout/stderr 1>>/var/log/kube-apiserver.log 2>&1
  • While avoiding a dependency on bash or any other shell

So we basically need a go based runner which redirects stdout/stderr. See go-runner.go. Then we need to wrap this go-runner in a distroless image which we can then use in both the bazel and make based builds. So that's what we ended up in this PR.

Signed-off-by: Davanum Srinivas davanum@gmail.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
Please see the image repo https://github.com/dims/go-runner, happy to add that repo to k/k or elsewhere once we validate this approach works

Which issue(s) this PR fixes:

Related to KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-release/1729-rebase-images-to-distroless

Special notes for your reviewer:
Borrowed code from previous experiments by @yuwenma from #75306 and #83390

Does this PR introduce a user-facing change?:

Switch core master base images (kube-apiserver, kube-scheduler) from debian to distroless

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/provider/gcp Issues or PRs related to gcp provider labels May 1, 2020
@k8s-ci-robot k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 1, 2020
@dims dims force-pushed the move-to-distroless-image branch 2 times, most recently from cab5dc8 to c84d6a3 Compare May 1, 2020 17:15
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 1, 2020
@dims dims force-pushed the move-to-distroless-image branch 2 times, most recently from 9555b54 to 984e089 Compare May 1, 2020 21:16
@dims dims marked this pull request as draft May 2, 2020 01:17
@dims
Copy link
Member Author

dims commented May 2, 2020

/test pull-kubernetes-conformance-kind-ga-only-parallel

@dims
Copy link
Member Author

dims commented May 2, 2020

/test pull-kubernetes-e2e-kind-ipv6

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 2, 2020
@dims
Copy link
Member Author

dims commented May 2, 2020

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 2, 2020
@dims
Copy link
Member Author

dims commented May 2, 2020

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 2, 2020
@dims
Copy link
Member Author

dims commented May 2, 2020

/assign @tallclair @yuwenma @wojtek-t @mm4tt

@BenTheElder
Copy link
Member

I remember this now. The controller manager runs flexvolume plugins, and flexvolume plugins include our e2e plugins may be shell scripts. You can't rip out the userspace from controller manager without breaking flexvolume.
/lgtm cancel

@BenTheElder
Copy link
Member

/approve cancel

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, fejta, justaugustus

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

@BenTheElder
Copy link
Member

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-storage/flexvolume.md

init, attach, detach, wait for attach, checking if attached are all called from the controller manager, which is done via an exec model. While the binary called comes from a path on the host, it is invoked in the controller-manager container filesystem (or kubelet).

Currently they can reasonably expect a generic linux userspace ... debian specifically for controller manager.

justaugustus
justaugustus requested changes May 9, 2020
View reviewed changes
Copy link
Member

@justaugustus justaugustus left a comment •
edited

Choose a reason for hiding this comment

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

@BenTheElder -- would I be correct in the assumption that removing the controller-manager changes should get us back in a good state?

If so, @dims, I've made a few code suggestions on what to revert.

EDIT: @dims -- In places where we can't/shouldn't use go-runner e.g., kube-controller-manager, kube-proxy, can you add a note referencing the base image exception list?

build/BUILD Outdated Show resolved Hide resolved
build/common.sh Outdated Show resolved Hide resolved
cluster/gce/manifests/kube-controller-manager.manifest Outdated Show resolved Hide resolved
@BenTheElder
Copy link
Member

As far as I know controller-manager / flexvolume is the only thing like this, reverting it should be sufficient but unfortunate, we'll still be stuck shipping these images ...

I had some conversations with @yuwenma and the GKE node team about this in the past when we were trying to figure out @yuwenma's attempt at this.

IIRC per @msau42 the idea was that eventually flexvolume could be deprecated out, but probably not until CSI was further along, or something along those lines ...

@justaugustus
Copy link
Member

As far as I know controller-manager / flexvolume is the only thing like this, reverting it should be sufficient but unfortunate, we'll still be stuck shipping these images ...

Okay, that was what I was getting from skimming through some of the docs/issues.
FWIW, we'll be still be required to ship debian-base (to support debian-iptables and others on the exceptions list).

@dims
Copy link
Member Author

dims commented May 9, 2020

/retest

@yuwenma @dims
Switch core master base images from debian to distroless
1aa67fc 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims dims force-pushed the move-to-distroless-image branch from c459a39 to 1aa67fc Compare May 9, 2020 10:55
@dims
Copy link
Member Author

dims commented May 9, 2020

/hold cancel

@justaugustus @BenTheElder removed controller-manager from this PR. Let's deal with that later.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2020
@justaugustus
Copy link
Member

EDIT: @dims -- In places where we can't/shouldn't use go-runner e.g., kube-controller-manager, kube-proxy, can you add a note referencing the base image exception list?

@dims -- Can you address this as well? I want to make sure we deter changes to the images and provide context about why.

@dims
Copy link
Member Author

dims commented May 9, 2020

@justaugustus not yet. i want this to merge and i am looking at options for controller-manager, looking at logs now.

@justaugustus
Copy link
Member

/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 9, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9dce6a6 into kubernetes:master May 9, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone May 9, 2020
@justaugustus justaugustus mentioned this pull request May 10, 2020
Merged
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 10, 2020
@justaugustus justaugustus changed the title Switch core master base images from debian to distroless Switch core master base images (kube-apiserver, kube-scheduler) from debian to distroless May 10, 2020
@justaugustus justaugustus mentioned this pull request May 11, 2020
Closed
@neolit123 neolit123 mentioned this pull request May 12, 2020
Open
@justaugustus justaugustus mentioned this pull request May 13, 2020
Closed
19 tasks
@tosi3k tosi3k mentioned this pull request May 15, 2020
Closed
6 tasks
This was referenced May 17, 2020
Merged
Merged
@vpnachev vpnachev mentioned this pull request Dec 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fejta fejta fejta left review comments

@justaugustus justaugustus justaugustus requested changes

@lavalamp lavalamp Awaiting requested review from lavalamp

@MrHohn MrHohn Awaiting requested review from MrHohn

@immutableT immutableT Awaiting requested review from immutableT

Assignees

@justaugustus justaugustus

@BenTheElder BenTheElder

@fejta fejta

@dekkagaijin dekkagaijin

@cblecker cblecker

@mm4tt mm4tt

@wojtek-t wojtek-t

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes area/provider/gcp Issues or PRs related to gcp provider area/release-eng Issues or PRs related to the Release Engineering subproject area/security cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet