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

Make CoreDNS default in kubeup and update CoreDNS version/manifest in kubeup and kubeadm #69883

Merged
merged 1 commit into from Oct 19, 2018
Merged

Make CoreDNS default in kubeup and update CoreDNS version/manifest in kubeup and kubeadm #69883

merged 1 commit into from Oct 19, 2018

Conversation

chrisohaver
Copy link
Contributor

@chrisohaver chrisohaver commented Oct 16, 2018 •
edited

What this PR does / why we need it:

Makes CoreDNS default for kube-up
Updates manifest and CoreDNS version for kube-up and kubeadm.

KEP: https://github.com/kubernetes/community/blob/master/keps/sig-network/0012-20180518-coredns-default-proposal.md
Feature: kubernetes/enhancements#566

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 #

Special notes for your reviewer:

This version of CoreDNS (1.2.4) contains improvements that reduce memory usage at scale. Based on the 500 node and 2000 node e2e scale test results, we have linearly projected that memory use in the 5000 node scale test should be less than the prescribed resource limit. But since the 5000 node scale test is not available as a pre-submit, we cannot confirm until this PR is merged.

Release note:

CoreDNS is now the default DNS server in kube-up deployments.

@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/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. 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. labels Oct 16, 2018
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Oct 16, 2018
@chrisohaver
Copy link
Contributor Author

chrisohaver commented Oct 16, 2018 •
edited

/hold until 1.2.3 is pushed to gcr.io (#69880)

@k8s-ci-robot k8s-ci-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Oct 16, 2018
@rajansandeep rajansandeep mentioned this pull request Oct 16, 2018
Closed
@chrisohaver
Copy link
Contributor Author

/hold

@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 Oct 16, 2018
@rajansandeep
Copy link
Contributor

/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 Oct 16, 2018
neolit123
neolit123 reviewed Oct 16, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

@chrisohaver thanks for the update to coredns 1.2.3.
added one minor comment.

cmd/kubeadm/app/phases/addons/dns/manifests.go
resources:
- nodes
verbs:
- get
Copy link
Member

Choose a reason for hiding this comment

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

i'm pretty sure these should be under rules:.
http://people.redhat.com/jrivera/openshift-docs_preview/openshift-online/glusterfs-review/rest_api/apis-rbac.authorization.k8s.io/v1.ClusterRole.html#object-schema

i guess this applies to the rest of the files that are changed from the diff.

Copy link

Choose a reason for hiding this comment

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

@neolit123, they are under rules: as one more list item. Am i missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

never mind, i got confused by the indentation.

@chrisohaver
Copy link
Contributor Author

We've identified a bug in CoreDNS 1.2.3 that makes it invalid for kubernetes deployment.
Closing this issue. Will re-open, or re-sibmit a new PR with new version when the CoreDNS issue is resolved and new version cut.

@dims dims mentioned this pull request Oct 17, 2018
Closed
@chrisohaver chrisohaver reopened this Oct 18, 2018
@chrisohaver
Copy link
Contributor Author

chrisohaver commented Oct 18, 2018 •
edited

Latest commit is intended to run the latest coredns release (1.2.4) through e2e presubmit tests prior to pushing the coredns image to gcr.io. If all is OK, we will kick off the push to gcr.io, and I'll update the image locations in the manifests here (from coredns/coredns to k8s.gcr.io/coredns).

@rajansandeep rajansandeep mentioned this pull request Oct 18, 2018
Closed
@chrisohaver
Copy link
Contributor Author

I suspect the e2e test nodes don't have access staging-k8s.gcr.io, hence failure to pull images, and subsequent test failure. Will test now on k8s.gcr.io, since latest version appears to be already promoted.

@chrisohaver
Copy link
Contributor Author

I've squashed the commits. Commit 0cfb4bb is now pointing to the k8s.gcr.io repo.

@neolit123
Copy link
Member

Will test now on k8s.gcr.io, since latest version appears to be already promoted.

yes, the image up.

Make CoreDNS default and update version

there have been quite a bit of confusion about this part over the last couple of cycles - "default in what?"
i think the first sentence in the description summarizes it better:

Makes CoreDNS default for kube-up

@chrisohaver chrisohaver changed the title Make CoreDNS default and update version Make CoreDNS default in kubeup and update coredns version/manifest in kubeup and kubeadm Oct 18, 2018
@chrisohaver chrisohaver changed the title Make CoreDNS default in kubeup and update coredns version/manifest in kubeup and kubeadm Make CoreDNS default in kubeup and update CoreDNS version/manifest in kubeup and kubeadm Oct 18, 2018
@chrisohaver
Copy link
Contributor Author

/test pull-kubernetes-integration

@chrisohaver
Copy link
Contributor Author

/hold cancel

@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 Oct 18, 2018
@chrisohaver
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@chrisohaver
Copy link
Contributor Author

/assign @bowei @timothysc

@chrisohaver
Copy link
Contributor Author

/assign @thockin

@fturib
Copy link

fturib commented Oct 18, 2018

/assign @thockin

@thockin
Copy link
Member

thockin commented Oct 18, 2018

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisohaver, thockin

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2018
@AishSundar
Copy link
Contributor

@jberkus and @mortent as FYI

@k8s-ci-robot k8s-ci-robot merged commit 52de5c5 into kubernetes:master Oct 19, 2018
@shashidharatd
Copy link

How about merging this pr to other release branches? (v1.11 and v1.12). Does this pr qualify to be back ported to previous releases?

@chrisohaver
Copy link
Contributor Author

How about merging this pr to other release branches? (v1.11 and v1.12). Does this pr qualify to be back ported to previous releases?

I don't believe we can port the entire change back, i.e. Retroactively making CoreDNS the default cluster DNS for kubeup installations.

But we could push newer default versions/manifests of coredns into kubedns/kubeadm of older versions. I'd think this would be reserved for security fixes and major bug fixes, although I'm not personally familiar with the policy for support on past versions of k8s. Question for the sig-lifecycle team I think?

@neolit123
Copy link
Member

How about merging this pr to other release branches? (v1.11 and v1.12). Does this pr qualify to be back ported to previous releases?

nope, not really.

@prodanlabs
Copy link

good

@rajansandeep rajansandeep mentioned this pull request Oct 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@shashidharatd shashidharatd shashidharatd left review comments

@bowei bowei Awaiting requested review from bowei

@mwielgus mwielgus Awaiting requested review from mwielgus

Assignees

@timothysc timothysc

@bowei bowei

@thockin thockin

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet