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

kubeadm: Drop arch suffixes #66960

Merged
merged 2 commits into from
Aug 28, 2018
Merged

kubeadm: Drop arch suffixes #66960

merged 2 commits into from
Aug 28, 2018

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Aug 3, 2018 •
edited
Loading

What this PR does / why we need it:

This change removes arch suffixes from control plane images (etcd, kube-apiserver, kube-scheduler, kube-proxy, etc.). These are not needed, as almost all control plane images have a fat manifest now. It also adds a missing unit test for GetGenericImage

We have arch suffixes only for kube-dns images now.

Sample output of kubeadm config images pull with this change:

$ ./_output/bin/kubeadm config images pull
[config/images] Pulled k8s.gcr.io/kube-apiserver:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-controller-manager:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-scheduler:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-proxy:v1.11.1
[config/images] Pulled k8s.gcr.io/pause:3.1
[config/images] Pulled k8s.gcr.io/etcd:3.2.18
[config/images] Pulled k8s.gcr.io/coredns:1.1.3
$ ./_output/bin/kubeadm config images pull --feature-gates=CoreDNS=false
[config/images] Pulled k8s.gcr.io/kube-apiserver:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-controller-manager:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-scheduler:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-proxy:v1.11.1
[config/images] Pulled k8s.gcr.io/pause:3.1
[config/images] Pulled k8s.gcr.io/etcd:3.2.18
[config/images] Pulled k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.10
[config/images] Pulled k8s.gcr.io/k8s-dns-sidecar-amd64:1.14.10
[config/images] Pulled k8s.gcr.io/k8s-dns-dnsmasq-nanny-amd64:1.14.10

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Refs kubernetes/kubeadm#1030 kubernetes/kubeadm#51

Special notes for your reviewer:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/kind feature
/assign @luxas
/assign @timothysc
/assign @dims
/assign @neolit123

Release note:

kubeadm: Control plane images (etcd, kube-apiserver, kube-proxy, etc.) don't use arch suffixes. Arch suffixes are kept for kube-dns only.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. area/kubeadm labels Aug 3, 2018
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 3, 2018
@k8s-ci-robot
Copy link
Contributor

@rosti: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

What this PR does / why we need it:

This change removes arch suffixes from control plane images (etcd, kube-apiserver, kube-scheduler, kube-proxy, etc.). These are not needed, as almost all control plane images have a fat manifest now. It also adds a missing unit test for GetGenericImage

We have arch suffixes only for kube-dns images now.

Sample output kubeadm config images pull with this change:

$ ./_output/bin/kubeadm config images pull
[config/images] Pulled k8s.gcr.io/kube-apiserver:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-controller-manager:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-scheduler:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-proxy:v1.11.1
[config/images] Pulled k8s.gcr.io/pause:3.1
[config/images] Pulled k8s.gcr.io/etcd:3.2.18
[config/images] Pulled k8s.gcr.io/coredns:1.1.3
$ ./_output/bin/kubeadm config images pull --feature-gates=CoreDNS=false
[config/images] Pulled k8s.gcr.io/kube-apiserver:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-controller-manager:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-scheduler:v1.11.1
[config/images] Pulled k8s.gcr.io/kube-proxy:v1.11.1
[config/images] Pulled k8s.gcr.io/pause:3.1
[config/images] Pulled k8s.gcr.io/etcd:3.2.18
[config/images] Pulled k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.10
[config/images] Pulled k8s.gcr.io/k8s-dns-sidecar-amd64:1.14.10
[config/images] Pulled k8s.gcr.io/k8s-dns-dnsmasq-nanny-amd64:1.14.10

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#1030

Special notes for your reviewer:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/kind feature
/assign @luxas
/assign @timothysc
/assign @dims
/assign @neolit123

Release note:

kubeadm: Control plane images (etcd, kube-apiserver, kube-proxy, etc.) don't use arch suffixes. Arch suffixes are kept for kube-dns only.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 3, 2018
@kad
Copy link
Member

kad commented Aug 3, 2018

/ok-to-test

However, I'm worried by the word almost. What is the actual status for all images that kubeadm referencing ?

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 3, 2018
@dims
Copy link
Member

dims commented Aug 3, 2018

@kad since kube-dns is now deprecated we are not going to touch that. so other than that we should be good. i am in the process of updating all the images we use in e2e-testing and only one left there is nginx. (#66055)

@rosti
Copy link
Contributor Author

rosti commented Aug 3, 2018

@kad almost means (as @dims said) all images, except kube-dns.

@neolit123
Copy link
Member

seems odd that kubedns doesn't have a manifest list even if deprecated.
all the important arch images are there for it.

@neolit123
Copy link
Member

thanks @rosti 🎉
/lgtm
/test pull-kubernetes-integration

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

dims commented Aug 3, 2018

@neolit123 there's an old PR we can resurrect kubernetes/dns#30

@neolit123
Copy link
Member

@dims it's not a big issue.
we are going to have to maintain this blob in kubeadm until kube-dns is deprecated, anyhow:
https://github.com/rosti/kubernetes/blob/e10a8f49858f194ef14d2373b3e1356f75d1869f/cmd/kubeadm/app/images/images.go#L84
(i.e. there will be another PR at some point).

@rosti
Copy link
Contributor Author

rosti commented Aug 3, 2018

I think @neolit123 is right here. It's not a big deal to have the arch suffixes only for kube-dns.

@timothysc
Copy link
Member

I'm ok with this, but we need documentation somewhere to cross ref with the build or test folks. This is too much tribal-fu.

/cc @kubernetes/sig-release-feature-requests @kubernetes/sig-testing-misc
/approve
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 3, 2018
@rdodev
Copy link
Contributor

rdodev commented Aug 3, 2018

Pardon my ignorance, but will this apply to 1.12+ or will it be backported to other kubeadm versions?

@dims
Copy link
Member

dims commented Aug 3, 2018

@rdodev 1.12+ only

@neolit123
Copy link
Member

this page needs a bump for sure on the kubeadm side:
https://kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-init/

from there i'm not sure which page outlines the "global" proposed k8s model of using manifest lists.

@timothysc
Copy link
Member

@rosti - can you rebase here?

@rosti
kubeadm: remove arch suffix from control plane images
14dbfdc 
This change removes arch suffixes from control plane images (etcd,
kube-apiserver, kube-scheduler, kube-proxy, etc.). These are not needed, as
almost all control plane images have a fat manifest now.

We have arch suffixes only for kube-dns images now.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@rosti
kubeadm: Add missing unit test for GetGenericImage
8c59c6d 
Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2018
@rosti
Copy link
Contributor Author

rosti commented Aug 28, 2018

Rebased

@timothysc
Copy link
Member

/hold
/assign @BenTheElder

We need confirmation that this is all ready.

/cc @dims @neolit123

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2018
@timothysc timothysc removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2018
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2018
timothysc
timothysc approved these changes Aug 28, 2018
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, rosti, timothysc

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-integration

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 5774bf1 into kubernetes:master Aug 28, 2018
@neolit123
Copy link
Member

note kubernetes/release#516 was reverted and 1.12 is not ready for manifest lists yet.
kubernetes/release#620

@dims
Copy link
Member

dims commented Sep 6, 2018

@neolit123 i have kubernetes/release#622 in progress

@neolit123
Copy link
Member

saw it yesterday, thanks @dims

@mkumatag
Copy link
Member

mkumatag commented Sep 7, 2018

I see latest kubeadm is failing at the moment, can someone revert this patch to make it work on other archs like power, arm?

@dims

mkumatag added a commit to mkumatag/kubernetes that referenced this pull request Sep 7, 2018
@mkumatag
Revert "Merge pull request kubernetes#66960 from rosti/drop-arch-suff…
8ea44b2 
…ixes"

This reverts commit 5774bf1, reversing
changes made to d46d671.
@mkumatag mkumatag mentioned this pull request Sep 7, 2018
Closed
@mkumatag
Copy link
Member

mkumatag commented Sep 7, 2018

I submitted a PR - #68394 revert these changes meanwhile.

@neolit123
Copy link
Member

^ responded in #68394

@rosti rosti mentioned this pull request Sep 19, 2018
Merged
@rosti rosti deleted the drop-arch-suffixes branch November 22, 2018 15:27
@hrak hrak mentioned this pull request Feb 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas left review comments

@timothysc timothysc timothysc approved these changes

@dims dims Awaiting requested review from dims

@neolit123 neolit123 Awaiting requested review from neolit123

Assignees

@dims dims

@timothysc timothysc

@neolit123 neolit123

@BenTheElder BenTheElder

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm 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. 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. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

None yet