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

Enable CoreDNS as default - when k8s is installed with kube-up #62147

Closed
wants to merge 2 commits into from
Closed

Enable CoreDNS as default - when k8s is installed with kube-up #62147

wants to merge 2 commits into from

Conversation

fturib
Copy link

@fturib fturib commented Apr 4, 2018 •
edited

DO NOT MERGE

What this PR does / why we need it:
Enable CoreDNS as default (for kube-up installations)
It will allow to run CI tests to prepare graduation criteria for CoreDNS as Default

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

NOTE for release : I guess that CoreDNS as default server for k8s needs a longer description. This specific PR is to ensure we validate all e2e.
If the

By default, when the cluster is setup using kube-up tool, 
the DNS Service of the cluster will be ensure by CoreDNS, instead of the usual kube-dns. 
However kube-dns can still be installed if the corresponding environment variable 
is set before launching the installation:
CLUSTER_DNS_CORE_DNS=false

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XS Denotes a PR that changes 0-9 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 Apr 4, 2018
@fturib
Copy link
Author

fturib commented Apr 4, 2018

/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 Apr 4, 2018
@fturib
Copy link
Author

fturib commented Apr 4, 2018

/uncc vishh
/uncc jingax10

@fturib
Copy link
Author

fturib commented Apr 4, 2018 •
edited

@rajansandeep : finally most easy way is to do the change to DEFAULT, and run the /test. see conversation here : kubernetes/test-infra#7557

@rajansandeep
Copy link
Contributor

@MrHohn @bowei can this get an ok to test to be able to run the tests on the PR. Thanks!

@jingax10
Copy link
Contributor

jingax10 commented Apr 5, 2018

/ok-to-test

@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 Apr 5, 2018
@fturib
Copy link
Author

fturib commented Apr 5, 2018

/test pull-kubernetes-e2e-gce-100-performance-coredns

@fturib
Copy link
Author

fturib commented Apr 5, 2018

/test pull-kubernetes-e2e-gce-100-performance

@fturib
Copy link
Author

fturib commented Apr 13, 2018

Re-rerun the test now that it seems be ok more often (I mean basic issue fixed)
/test pull-kubernetes-e2e-gce-100-performance

@fturib
Copy link
Author

fturib commented Apr 16, 2018

/test pull-kubernetes-e2e-gce-100-performance-coredns

@rajansandeep
Copy link
Contributor

/test pull-kubernetes-e2e-gce-100-performance

@fturib
Copy link
Author

fturib commented Apr 23, 2018

/test pull-kubernetes-e2e-gce

@fturib fturib changed the title [do-not-merge] enable CoreDNS as default - for test purpose only Enable CoreDNS as default - when k8s is installed with kube-up Apr 23, 2018
@rajansandeep
Copy link
Contributor

/hold cancel

@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 do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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 Apr 23, 2018
@fturib
Copy link
Author

fturib commented Apr 23, 2018

/cc @MrHohn @bowei - thanks for review !

@fturib
Copy link
Author

fturib commented Apr 23, 2018

/assign @vishh

@fturib
Copy link
Author

fturib commented Apr 23, 2018

/assign @jingax10 - thanks for review ...

@MrHohn MrHohn mentioned this pull request Apr 27, 2018
Closed
@bowei
Copy link
Member

bowei commented May 3, 2018

This should be held until the next release.

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@fturib
Copy link
Author

fturib commented May 3, 2018

I will remove that PR, and re-push when needed for later release.

@fturib fturib closed this May 3, 2018
@fturib fturib reopened this Aug 13, 2018
@fturib
Copy link
Author

fturib commented Aug 13, 2018

I Reopened this PR to validate that CoreDNS as "default" would pass the e2e tests.
We will keep it open until we get a GO from Sig-network to merge this PR.

@fturib
Copy link
Author

fturib commented Aug 14, 2018

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-100-performance

@fturib
Copy link
Author

fturib commented Aug 15, 2018

/test pull-kubernetes-e2e-gce

@fturib
Copy link
Author

fturib commented Aug 16, 2018

/test pull-kubernetes-e2e-containerd-gce

@fturib
Copy link
Author

fturib commented Aug 18, 2018

/test pull-kubernetes-e2e-gce

@fturib
Copy link
Author

fturib commented Aug 18, 2018

/test pull-kubernetes-e2e-containerd-gce

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 18, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fturib, jingax10, MrHohn
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: timothysc

If they are not already assigned, you can assign the PR to them by writing /assign @timothysc 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

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

@fturib: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-containerd-gce 1860c294630d124f8ede4f02320d6a0044f81353 link /test pull-kubernetes-e2e-containerd-gce
pull-kubernetes-integration eb6c318 link /test pull-kubernetes-integration
pull-kubernetes-e2e-gce eb6c318 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

This was referenced Aug 19, 2018
Merged
Closed
johnbelamaric
johnbelamaric reviewed Aug 20, 2018
View reviewed changes
cmd/kubeadm/app/phases/addons/dns/manifests.go
@@ -244,8 +244,6 @@ spec:
image: {{ .ImageRepository }}/coredns:{{ .Version }}
imagePullPolicy: IfNotPresent
resources:
limits:
Copy link
Member

Choose a reason for hiding this comment

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

why are you removing limits?

@fturib
Copy link
Author

fturib commented Aug 21, 2018

@johnbelamaric : removing the limit was just a test ...

@fturib
Copy link
Author

fturib commented Aug 21, 2018

I close this PR in favor of this one : #67569

It has the same target, and fix issue kubernetes/dns#254 found.

@fturib fturib closed this Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric johnbelamaric left review comments

Assignees

@vishh vishh

@MrHohn MrHohn

@jingax10 jingax10

Labels
area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@fturib @rajansandeep @jingax10 @MrHohn @fejta-bot @bowei @k8s-github-robot @k8s-ci-robot @johnbelamaric @vishh