GA graduation criteria for NodeLocal DNSCache #1351

prameshj · 2019-11-02T21:41:49Z

Modified the existing KEP, as discussed in #1307

sftim · 2019-12-04T18:21:57Z

keps/sig-network/0030-nodelocal-dns-cache.md

+- Upgrade to a newer CoreDNS version(1.16.x) in [node-cache](https://github.com/kubernetes/dns/pull/328).
+- Ensure that Kubernetes [e2e tests with NodeLocal DNSCache](https://k8s-testgrid.appspot.com/sig-network-gce#gci-gce-kube-dns-nodecache) are passing.
+- Scalability tests with NodeLocal DNSCache enabled, verifying the [HA modes](https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190424-NodeLocalDNS-beta-proposal.md#design-details) as well as the regular mode.
+- Have N clusters(number TBD) running in production with NodeLocal DNSCache enabled.



I'd like to get the documentation updated and improved, so that there is a good-quality Task page, before GA.

I'd like to see docs clear enough that someone who has just qualified as CKA can follow that Task page without help, and without needing in to do research / fill in missing details.

At the moment I don't think https://kubernetes.io/docs/tasks/administer-cluster/nodelocaldns/ meets that criterion.

As this is an add-on, does it also make sense to document how to remove it? (documenting that would help encourage production use)

Thanks for the feedback! I submitted the change to docs on how to remove the addon - kubernetes/website@master...prameshj:patch-4

Can you help improve the tasks page with suggestions on what to add? Would adding cluster create commands/kubectl commands to check that node-local-dns pods are running and healthy... make it more useful?

I can try to help with documenting this add-on (I've already opened kubernetes/website#17944). I'd want to be clear on intent here. A few things:

What does it mean for this add-on to be GA? I think it means that its optional and that if you enable it, the Kubernetes project believes the add-on is finished.

Is https://github.com/kubernetes/kubernetes/blob/master/cluster/addons/dns/nodelocaldns/nodelocaldns.yaml a manifest or a template that can become a manifest? What kind of template?

How can the reader verify that their changes worked as intended?

From the proposed changes:

KUBE_ENABLE_NODELOCAL_DNS=true kubetest --up

What are the prerequisites for that to work? (and are those explained to the reader?)

@prameshj, are you planning to make a pull request based on kubernetes/website@master...prameshj:patch-4 ?

For this KEP, I think it's enough to add an extra line about providing clear / comprehensible documentation aimed at cluster operators.

kubernetes/[email protected]:patch-4

I just did.. I thought I already had, thanks for the reminder!
kubernetes/website#18190

I can try to help with documenting this add-on (I've already opened kubernetes/website#17944). I'd want to be clear on intent here. A few things:

What does it mean for this add-on to be GA? I think it means that its optional and that if you enable it, the Kubernetes project believes the add-on is finished.

Is https://github.com/kubernetes/kubernetes/blob/master/cluster/addons/dns/nodelocaldns/nodelocaldns.yaml a manifest or a template that can become a manifest? What kind of template?

How can the reader verify that their changes worked as intended?

IMO, Addon being GA means it is suitable to run in production.

It is a template, where most of the PILLAR_ variables need to be substituted. Not sure what you are referring to, by kind of template.

Changes can be verified by a) checking that node-local-dns pods are running b) checking the node-local-dns pods metrics to make sure request count is going up.

From the proposed changes:

KUBE_ENABLE_NODELOCAL_DNS=true kubetest --up

What are the prerequisites for that to work? (and are those explained to the reader?)
Good point. That command assumes you have a working kubernetes repo setup to build and run tests. I can include it in the other PR.

Added documentation to the criteria.

johnbelamaric · 2019-12-13T18:05:16Z

/assign

sftim · 2019-12-18T09:32:40Z

keps/sig-network/0030-nodelocal-dns-cache.md

@@ -192,7 +192,13 @@ Having the pods query the nodelocal cache introduces a single point of failure.



I've just had a thought: 169.254.20.10 is link-local. If Node-local DNS cache relies, by default, on Pods being able to route to 169.254.20.10 then that is a dependency on undefined / nonstandard behavior.
According to (my reading of) internet standards, 169.254.0.0/16 is only reachable for a client on the same link.

I recommend addressing that, somehow, before GA. If the decision is to go against the standard, let's record that explicitly.

The link-local IP is attached to an interface on the host. You are right, it does not exist on each pod. Since all pod traffic does go through the host network and subject to iptables rules configured, this works. I am not aware of any CNI plugins where this would not work. From what i understand, only 127.0.00/8 traffic does not leave the pod.

A CNI plugin will need to route traffic to 169.254.20.10, but there's no standardized mechanism to make this happen (because 169.254.20.10 is link-local, and link-local addresses aren't routeable).

The packets just need to hit the hostNetwork since the link-local IP address exists on an interface in the node. This creates a route in the "local" routing table of the form:
"local 169.254.20.10 dev nodelocaldns proto kernel scope host src 169.254.20.10"

This is sufficient to deliver the packet to anything listening locally on that address. Isn't that true?

Also, if the k8s cluster is using kube-proxy in iptables mode, the pods can use either the link-local IP or the kube-dns service IP to access node-local-dns.

local 169.254.20.10 dev nodelocaldns proto kernel scope host src 169.254.20.10

I'm really not keen on setting that in stone. Allocating from the link-local range like this and expecting routing to work, even within only one host, just doesn't seem like an approach that can scale.

It's different when a cloud provider does it at the network layer, because there's no concept of those packets being routed. There's a (virtual) link and it turns out that when the instance boots something else is defending 169.254.169.254 or whatever. That works fine.

Ideally, I can set up a Kubernetes cluster that puts all Pods on the same link-local network, and that's a valid supported configuration (provided I find a CNI that makes it happen). I realize that 169.254.0.0/16 looks like a convenient range to borrow an address from, and I can see why it's often going to be a pragmatic choice.

To be clear: my concern is about making & documenting 169.254.20.10 as a default value. If you have to provide an IPv4 address at deploy time and the documentation suggests picking an address inside 169.254.0.0/16 then that isn't a problem and doesn't set any problematic precedent.

ok, if I understand correctly, we want to tweak the documentation to say:
"Selecting a listen ip for NodeLocal DNSCache from 169.254.20.10/16 is recommended, in our examples we will use 169.254.20.10". Is that correct? I have added this to the website doc PR - kubernetes/website#18716

We can continue the discussion there. From the GA graduation perspective, are the changes here ok to implement?

Selecting a listen ip for NodeLocal DNSCache from 169.254.20.10/16 is recommended, in our examples we will use 169.254.20.10

(Retrospectively) I think you've taken an appropriate and pragmatic tack. Thanks.

prameshj · 2020-01-17T18:59:35Z

/assign @bowei @johnbelamaric @thockin

thockin

Good criteria, thanks for updating.

Do we have/need a long-term plan to keep the CoreDNS version updated?

Do we have an easy-to-run benchmark test that shows the value of cache over no-cache?

prameshj · 2020-01-17T20:52:02Z

Good criteria, thanks for updating.

Do we have/need a long-term plan to keep the CoreDNS version updated?

Currently, the update is on a need basis. It would be a good idea to update when there is a major bugfix/additional functionality.

Do we have an easy-to-run benchmark test that shows the value of cache over no-cache?
We have https://github.com/kubernetes/perf-tests/tree/master/dns
these are not long-running by default and lookup only a few hostname. Enhancing this is part of the scale test improvements listed in the GA criteria.

prameshj · 2020-01-21T20:30:46Z

I have updated the criteria based on the review comments. PTAl @thockin @johnbelamaric @bowei @sftim

sftim · 2020-01-22T00:57:09Z

I'm still dubious about taking 169.254.20.10 as a default to GA, because it seems to me like a misuse of the link-local IPv4 address space.

prameshj · 2020-01-22T05:54:21Z

I'm still dubious about taking 169.254.20.10 as a default to GA, because it seems to me like a misuse of the link-local IPv4 address space.

We are not mentioning 169.254.20.10 as a default in this KEP and in the docs, we will be mentioning the 169.254.0.0/16 as a potential space to pick the listen ip from. Do you have suggestions for what else to do here? Also, just to be clear, this feature is not going to be enabled by default, in GA.

johnbelamaric

/lgtm

keps/sig-network/0030-nodelocal-dns-cache.md

johnbelamaric · 2020-01-22T21:44:03Z

@sftim I don't think that concern should block this KEP change. It can be taken up again in the actual docs PR if you're still concerned about the language there.

/lgtm
/approve

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2020-01-23T00:35:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, prameshj, 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:

~~keps/sig-network/OWNERS~~ [thockin]

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

sftim · 2020-01-23T21:00:57Z

@prameshj thanks for clarifying - #1351 (comment) makes sense to me.
If the sample manifests and documentation only suggest using a link-local address and leave that up to the cluster administrator to implement, I've no concern.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 2, 2019

k8s-ci-robot requested review from caseydavenport and thockin November 2, 2019 21:42

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. labels Nov 2, 2019

Added graduation criteria for nodelocaldns GA

bf37839

prameshj force-pushed the nodelocal-ga branch from be3f020 to bf37839 Compare November 2, 2019 21:44

sftim reviewed Dec 4, 2019

View reviewed changes

k8s-ci-robot assigned johnbelamaric Dec 13, 2019

sftim reviewed Dec 18, 2019

View reviewed changes

jeremyrickard mentioned this pull request Jan 7, 2020

Graduate NodeLocal DNSCache to GA #1024

Closed

Added documentation to GA criteria

acda4a7

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 8, 2020

k8s-ci-robot assigned bowei and thockin Jan 17, 2020

thockin reviewed Jan 17, 2020

View reviewed changes

Update the criteria.

8e3cdaf

johnbelamaric reviewed Jan 22, 2020

View reviewed changes

keps/sig-network/0030-nodelocal-dns-cache.md Outdated Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2020

Update coreDNS version in criteria.

623d9a8

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2020

thockin reviewed Jan 23, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2020

k8s-ci-robot merged commit 74e610b into kubernetes:master Jan 23, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GA graduation criteria for NodeLocal DNSCache #1351

GA graduation criteria for NodeLocal DNSCache #1351

prameshj commented Nov 2, 2019

sftim Dec 4, 2019 •

edited

Loading

sftim Dec 4, 2019

prameshj Dec 5, 2019

sftim Dec 9, 2019

sftim Dec 17, 2019

sftim Dec 17, 2019

prameshj Dec 18, 2019

prameshj Dec 18, 2019

prameshj Jan 8, 2020

johnbelamaric commented Dec 13, 2019

sftim Dec 18, 2019

prameshj Jan 8, 2020

sftim Jan 8, 2020

prameshj Jan 8, 2020

sftim Jan 9, 2020

prameshj Jan 15, 2020

sftim Feb 4, 2020

prameshj commented Jan 17, 2020

thockin left a comment

prameshj commented Jan 17, 2020

prameshj commented Jan 21, 2020

sftim commented Jan 22, 2020

prameshj commented Jan 22, 2020

johnbelamaric left a comment

johnbelamaric commented Jan 22, 2020

thockin left a comment

k8s-ci-robot commented Jan 23, 2020

sftim commented Jan 23, 2020

		@@ -192,7 +192,13 @@ Having the pods query the nodelocal cache introduces a single point of failure.

GA graduation criteria for NodeLocal DNSCache #1351

GA graduation criteria for NodeLocal DNSCache #1351

Conversation

prameshj commented Nov 2, 2019

sftim Dec 4, 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

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

johnbelamaric commented Dec 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prameshj commented Jan 17, 2020

thockin left a comment

Choose a reason for hiding this comment

prameshj commented Jan 17, 2020

prameshj commented Jan 21, 2020

sftim commented Jan 22, 2020

prameshj commented Jan 22, 2020

johnbelamaric left a comment

Choose a reason for hiding this comment

johnbelamaric commented Jan 22, 2020

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 23, 2020

sftim commented Jan 23, 2020

sftim Dec 4, 2019 •

edited

Loading