Bump CoreDNS version to 1.6.5 and update manifest #85108

rajansandeep · 2019-11-11T21:31:06Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Bumps the CoreDNS version to 1.6.5
Updates the corefile-migration library to v1.0.4 which includes migration support up to CoreDNS v1.6.5

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This PR is dependent on the CoreDNS 1.6.5 image be pushed to gcr.io for which #84993 has been opened.

/hold

Does this PR introduce a user-facing change?:

Kubeadm now includes CoreDNS version 1.6.5
 - `kubernetes` plugin adds metrics to measure kubernetes control plane latency.
 -  the `health` plugin now includes the `lameduck` option by default, which waits for a duration before shutting down.

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

rajansandeep · 2019-11-11T21:38:40Z

/priority important-soon
/assign @neolit123 @BenTheElder
/cc @chrisohaver

neolit123 · 2019-11-11T21:46:44Z

@rajansandeep this is very close the release, but i'm going to try to review it later.

pull-kubernetes-e2e-kind — Job failed.
ERROR: error building node image: command "docker save -o /tmp/kind-node-image755345787/bits/images/6.tar k8s.gcr.io/coredns:1.6.5" failed with error: exit status 1

we should push the image fist.
the kind job is now PR blocking.

BenTheElder · 2019-11-11T21:52:18Z

image is pushing #84993 (comment)

BenTheElder · 2019-11-11T21:53:43Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

@@ -223,7 +223,10 @@ metadata:
  labels:
    k8s-app: kube-dns
 spec:
-  replicas: 2
+  # replicas: not specified here:
+  # 1. In order to make Addon Manager do not reconcile this replicas parameter.


does kubeadm have an "addon manager" ? @neolit123

no, it does not have one, per se.
it has "phases" that manage addons.

the comment can be:
# Default replica count is 1

right, that's what I thought re: phases.
I think the rest of the details in this comment make more sense for kube-up and less sense for kubeadm (presuming this is referring to the "Addon manager" in cluster/)

BenTheElder · 2019-11-11T22:43:17Z

/test pull-kubernetes-e2e-kind

neolit123 · 2019-11-11T23:00:28Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

@@ -313,7 +325,9 @@ data:
  Corefile: |
    .:53 {
        errors
-        health
+        health {
+           lameduck 12s


is 12s the timeout for the health check in this case?
the timeout for CP components is 15s, so we may want to match that.

Actually - as I was describing the reasoning behind this, I realized that a timeout of 5 seconds should be all that is necessary. When picking 12s, I was conflating the issue with the readiness/health check periods, which I don't think actually come into play. The function of lameduck is to finish processing in flight queries before shutting down. A lameduck of longer than 5 would typically be pointless, since most clients have a default timeout of 5 seconds (and thus would have stopped listening for a response after then).

yep, 5 seems good if that is sufficient.

@rajansandeep do you agree with the change to 5 seconds?

Yes, I agree. I'll push a commit to reflect those changes.

BenTheElder · 2019-11-11T23:06:34Z

/retest
kind passed [now that the coreDNS image is live]

rajansandeep · 2019-11-11T23:13:11Z

/hold cancel
Since the image seems to have been pushed to gcr

neolit123 · 2019-11-11T23:26:25Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+              - key: k8s-app
+                operator: In
+                values: ["kube-dns"]
+            topologyKey: kubernetes.io/hostname


@rajansandeep could you please explain the motivation?

my understanding is the following:

we reduce the replica count to 1.

coredns will deploy on the primary CP node (where kubeadm init is called).

the anti-affinity rule makes sure that the Pod will not schedule on a Node that already has it.

if i'm not mistaken, this will not improve much what we have right now.
a problem we have currently, is that both replicas land on the same primary CP Node.

ideally what we want is a coredns instance to be deployed on all CP Nodes.
one way of doing that is with static-pods, but given we treat coredns as an addon we should use a DaemonSet with a NodeSelector that matches the kubeadm "master" node-role.

i'm going to experiment with that in a bit.

sadly, by changing the coredns object type we are going to break a lot of users that have automation around kubectl patch deployment coredns..., so such a change is not a great idea without a grace period.

@neolit123
With pod anti-affinity enabled and 2 coredns replicas:

If a user has only a master node installed via kubeadm init, there will be one coredns pod in running state and one in pending state.

The other coredns pod will remain in pending state and waits for scheduling until another worker node is created via kubeadm join.

a problem we have currently, is that both replicas land on the same primary CP Node.

Pod anti-affinity solves this problem.

With pod anti-affinity enabled and 2 coredns replicas:

If a user has only a master node installed via kubeadm init, there will be one coredns pod in running state and one in pending state.

The other coredns pod will remain in pending state and waits for scheduling until another worker node is created via kubeadm join.

this is true for 2 replicas and antit-affinity, we don't want Pending pods because it will break e2e tests using our test suite, where all pods are expected to be Ready.

a problem we have currently, is that both replicas land on the same primary CP Node.

Pod anti-affinity solves this problem.

yes. but we reduce the replicas to 1, so if the primary CP node becomes NotReady (e.g. shutdown) the coredns service will still go down and the pod will not reschedule on a Ready node. (same happens for 2 replicas, without anti-affinity).

i guess i'm trying to see how 1 replica with anti-affinity is an improvement over 2 replicas without it.

like i've mentioned earlier, ideally we want a coredns DS for all CP nodes.

if continuing to use a Deployment we might want to add these tolerations: #55713 (comment)

^ this issue BTW is one where users are being quite confused by some scheduling aspects of k8s.

@chrisohaver PTAL too.

so basically i'm proposing that we keep the replica count to 2.
and introduce the following:

spec: ... tolerations: - key: "node.kubernetes.io/unreachable" operator: "Exists" effect: "NoExecute" tolerationSeconds: 15 - key: "node.kubernetes.io/not-ready" operator: "Exists" effect: "NoExecute" tolerationSeconds: 15

this will improve the current deployment by rescheduling the coredns Pods if a Node becomes NotReady after 15 seconds.

i don't think the anti-affinity rule is needed here:

affinity: podAntiAffinity: requiredDuringSchedulingIgnoredDuringExecution: - labelSelector: matchExpressions: - key: k8s-app operator: In values: ["kube-dns"] topologyKey: kubernetes.io/hostname

because with the current setup the deployment already does that.

Let's start with the more trivial questions here. Is this required for the CoreDNS version bump?
If so, why is this a patch release and not a minor version bump? If it's not required, can we split it and move it into a separate PR?

Yes, I've removed the pod anti-affinity changes from this PR and move it to another PR.

aojea · 2019-11-12T09:07:54Z

/test pull-kubernetes-e2e-kind-ipv6

rosti

Thanks @rajansandeep !

rosti · 2019-11-12T14:40:15Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+              - key: k8s-app
+                operator: In
+                values: ["kube-dns"]
+            topologyKey: kubernetes.io/hostname


Let's start with the more trivial questions here. Is this required for the CoreDNS version bump?
If so, why is this a patch release and not a minor version bump? If it's not required, can we split it and move it into a separate PR?

chrisohaver · 2019-11-12T16:12:36Z

Let's start with the more trivial questions here. Is this required for the CoreDNS version bump?

No it's not.

If it's not required, can we split it and move it into a separate PR?

Yes - makes sense.

neolit123 · 2019-11-12T17:07:02Z

/approve
i think that there are a lot of flakes in CI right now.

…n of coredns up to version 1.6.5

neolit123 · 2019-11-12T18:13:23Z

/lgtm
thanks @rajansandeep

aojea · 2019-11-12T18:32:16Z

/test pull-kubernetes-e2e-kind-ipv6

rajansandeep · 2019-11-12T22:17:08Z

@neolit123 Does this need the milestone tag?

neolit123 · 2019-11-12T22:43:24Z

@neolit123 Does this need the milestone tag?

i don't think it does yet.

/retest

soltysh

/approve
dep updates

neolit123 · 2019-11-13T13:21:13Z

/assign @BenTheElder
PTAL for approval.

rajansandeep · 2019-11-13T17:42:08Z

/assign @liggitt
For root approval for changes in vendor dep.

liggitt · 2019-11-13T17:58:10Z

cmd/kubeadm/app/phases/addons/dns/dns_test.go

@@ -632,7 +632,9 @@ func TestCreateCoreDNSConfigMap(t *testing.T) {
    }`,
 			expectedCorefileData: `.:53 {
    errors
-    health
+    health {
+        lameduck 5s


is this a required change? will users with a custom dns config be broken if they don't make this change as well?

It's not required. It's just an improvement that reduces query failures during rolling upgrades. The setting allows CoreDNS to complete in flight dns queries before exiting.

Without the setting, CoreDNS will not be broken.

liggitt · 2019-11-13T18:04:29Z

/approve
vendor update looks good

/hold on the config compatibility question
kubeadm maintainers can unhold at will

k8s-ci-robot · 2019-11-13T18:05:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, neolit123, rajansandeep, soltysh

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:

~~OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2019-11-13T18:42:39Z

looks like I got scooped @neolit123 ... :prow_fire: 😞

neolit123 · 2019-11-13T18:45:46Z

looks like I got scooped @neolit123 ... :prow_fire:

np

neolit123 · 2019-11-13T18:46:52Z

canceling the hold as per @chrisohaver 's explanation here:
#85108 (comment)

thanks
/hold cancel

k8s-ci-robot requested review from dchen1107, detiber and a team November 11, 2019 21:32

k8s-ci-robot added area/dependency Issues or PRs related to dependency changes area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 11, 2019

k8s-ci-robot assigned BenTheElder and neolit123 Nov 11, 2019

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 Nov 11, 2019

k8s-ci-robot requested a review from chrisohaver November 11, 2019 21:38

BenTheElder reviewed Nov 11, 2019

View reviewed changes

neolit123 reviewed Nov 11, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2019

neolit123 reviewed Nov 11, 2019

View reviewed changes

rosti reviewed Nov 12, 2019

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 12, 2019

rajansandeep added 2 commits November 12, 2019 13:05

bump coredns version and update manifest

f931dad

bump vendor of corefile-migration lib to 1.0.4 which support migratio…

2544a76

…n of coredns up to version 1.6.5

rajansandeep force-pushed the prepcorednsfor1.17-kubeadm branch from 460dd60 to 2544a76 Compare November 12, 2019 18:11

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019

chrisohaver approved these changes Nov 12, 2019

View reviewed changes

soltysh approved these changes Nov 13, 2019

View reviewed changes

k8s-ci-robot assigned liggitt Nov 13, 2019

liggitt reviewed Nov 13, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019

k8s-ci-robot merged commit c33af5b into kubernetes:master Nov 13, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 13, 2019

neolit123 mentioned this pull request Nov 27, 2019

redesign the CoreDNS Pod Deployment in kubeadm kubernetes/kubeadm#1931

Closed

Bump CoreDNS version to 1.6.5 and update manifest #85108

Bump CoreDNS version to 1.6.5 and update manifest #85108

Conversation

rajansandeep commented Nov 11, 2019 • edited Loading

rajansandeep commented Nov 11, 2019

neolit123 commented Nov 11, 2019

BenTheElder commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Nov 11, 2019 • edited Loading

rajansandeep commented Nov 11, 2019

Choose a reason for hiding this comment

neolit123 Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

neolit123 Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Nov 12, 2019

rosti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisohaver commented Nov 12, 2019

neolit123 commented Nov 12, 2019

neolit123 commented Nov 12, 2019

aojea commented Nov 12, 2019

rajansandeep commented Nov 12, 2019

neolit123 commented Nov 12, 2019

soltysh left a comment

Choose a reason for hiding this comment

neolit123 commented Nov 13, 2019 • edited Loading

rajansandeep commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 13, 2019

k8s-ci-robot commented Nov 13, 2019

BenTheElder commented Nov 13, 2019

neolit123 commented Nov 13, 2019

neolit123 commented Nov 13, 2019

rajansandeep commented Nov 11, 2019 •

edited

Loading

BenTheElder commented Nov 11, 2019 •

edited

Loading

neolit123 Nov 11, 2019 •

edited

Loading

neolit123 Nov 12, 2019 •

edited

Loading

neolit123 Nov 12, 2019 •

edited

Loading

neolit123 commented Nov 13, 2019 •

edited

Loading