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.

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

Add a way to customize coredns version. #78851

Open
wants to merge 1 commit into
base: master
from

Conversation

@mborsz
Copy link
Member

commented Jun 10, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Discussion in #78691 says that we should be able to customize coredns version. This PR allows to do so.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mborsz
To complete the pull request process, please assign bowei
You can assign the PR to them by writing /assign @bowei in a comment when ready.

The full list of commands accepted by this bot can be found 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

@mborsz

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

/test pull-kubernetes-e2e-gke

@mborsz

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

/milestone v1.15

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@mborsz: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.15

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

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@mborsz: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke cfe9d86 link /test pull-kubernetes-e2e-gke

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.

@mborsz

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

The -e2e-gke test is failing for unrelated reason. https://prow.k8s.io/job-history/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-e2e-gke shows that few last runs are failing.

@@ -259,6 +259,7 @@ fi
# Optional: Install cluster DNS.
# Set CLUSTER_DNS_CORE_DNS to 'false' to install kube-dns instead of CoreDNS.
CLUSTER_DNS_CORE_DNS="${CLUSTER_DNS_CORE_DNS:-true}"
CORE_DNS_VERSION="${KUBE_CORE_DNS_VERSION:-}"

This comment has been minimized.

Copy link
@liggitt

liggitt Jun 10, 2019

Member

if KUBE_CORE_DNS_VERSION is unset, will this produce an invalid manifest? If so, we should either set a valid default here or remove the :- to make this produce an error if KUBE_CORE_DNS_VERSION is not set

This comment has been minimized.

Copy link
@liggitt

liggitt Jun 10, 2019

Member

I don't see KUBE_CORE_DNS_VERSION specified in the repo. Won't that make kube-up for GCE break?

This comment has been minimized.

Copy link
@mborsz

mborsz Jun 10, 2019

Author Member

It's not required -- the default value is used in configure-helper.sh if this is not specified.

In #77918 I was adding similar config variable using slightly different approach:

This time I'm using slightly different approach to avoid breaking gke.

@chrisohaver

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Customization of the CoreDNS version should also be associated with changes to the Deployment and ConfigMap for the appropriate version.

ConfigMaps for example are not always backward/forward compatible in CoreDNS between versions (specifically between 1.3.1 and 1.5.0). I think the default 1.3.1 ConfigMap happens to be compatible with 1.5.0, but not vice versa.

The 1.3.1 Deployment used the health plugin to report readiness. In CoreDNS 1.5.0, the health plugin no longer accurately reports readiness, and thus the new ready plugin (added in 1.5.0) is used to report readiness. Using the 1.3.1 Deployment with the 1.5.0 image probably won't pose a problem in the scale test (because the scale tests don't test DNS function), but would present problems in the field, especially at scale, since it would result in Pods reporting "ready" before they have fully synced to the API.

@@ -116,7 +116,7 @@ spec:
beta.kubernetes.io/os: linux
containers:
- name: coredns
image: k8s.gcr.io/coredns:1.5.0
image: k8s.gcr.io/coredns:$CORE_DNS_VERSION

This comment has been minimized.

Copy link
@spiffxp

spiffxp Jun 10, 2019

Member

Based on changes in #78691 I think the readinessProbe.httpGet fields should also be modified

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Jun 10, 2019

Contributor

I agree, and the ConfigMap Corefile needs to include the ready plugin when being deployed in 1.5.0.

@tpepper

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Should we explicitly milestone this to 1.16 so it's clear in the short term that this is not required for 1.15?

@chrisohaver

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Should we explicitly milestone this to 1.16 so it's clear in the short term that this is not required for 1.15?

I believe @mborsz's intent is to get this merged into 1.15... #78691 (comment)

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@mborsz: PR needs rebase.

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.

@spiffxp

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

/milestone v1.15
I'll put this in the current milestone for now but it's not clear to me whether we want to block code thaw on this or not

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone Jun 10, 2019

@mborsz

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

I was thinking about merging this as a part of 1.15, but given that:

  • the logic here is quite complex (I would need to rewrite config for 1.15)
  • the memory increase isn't that big: #78562 (comment)

I think we can close this PR and focus on mitigating the issue by increasing memory limit (until we really understand what was causing the issue in coredns and fix this).

There is no need to block 1.15 on this PR.

@jberkus

This comment has been minimized.

Copy link

commented Jun 11, 2019

/remove-milestone

@soggiest

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.15, v1.16 Jun 11, 2019

@spiffxp

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

needs rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.