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 no-negcache flag to kube-dns #53604

Merged
merged 2 commits into from Oct 13, 2017

Conversation

Projects
None yet
6 participants
@cblecker
Member

cblecker commented Oct 9, 2017

What this PR does / why we need it:
Adds the --no-negcache flag to prevent dnsmasq from caching negative (NXDOMAIN) responses. More details on why this is desirable here.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes kubernetes/dns#121

Special notes for your reviewer:
Thanks to @rsmitty (https://rsmitty.github.io/KubeDNS-Tweaks/) and @coresolve (kubernetes/dns#121 (comment)) for pointing us in the right direction.

Release note:

Add --no-negcache flag to kube-dns to prevent caching of NXDOMAIN responses.
@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker
Member

cblecker commented Oct 9, 2017

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 9, 2017

Member

/assign

Member

bowei commented Oct 9, 2017

/assign

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 9, 2017

Member

@bowei Another question.. is this worth picking back to 1.8 and/or 1.7?

Member

cblecker commented Oct 9, 2017

@bowei Another question.. is this worth picking back to 1.8 and/or 1.7?

@ahmetb

This comment has been minimized.

Show comment
Hide comment
@ahmetb

ahmetb Oct 9, 2017

Member

@cblecker the issue is not critical enough, so that pretty much I was the only one noticing it for a while kubernetes/dns#121. I'm not sure if it qualifies for backporting.

Member

ahmetb commented Oct 9, 2017

@cblecker the issue is not critical enough, so that pretty much I was the only one noticing it for a while kubernetes/dns#121. I'm not sure if it qualifies for backporting.

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 9, 2017

Member

Yeah, that makes sense to me. It's a bugfix in the sense that it's correcting unintended behaviour, but you're right, probably not enough reports of it causing problems to pick it backwards.

Member

cblecker commented Oct 9, 2017

Yeah, that makes sense to me. It's a bugfix in the sense that it's correcting unintended behaviour, but you're right, probably not enough reports of it causing problems to pick it backwards.

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 10, 2017

Member

Can you check that kubeadm also gets this update?

Member

bowei commented Oct 10, 2017

Can you check that kubeadm also gets this update?

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 10, 2017

Member

(should be findable via git grep for mentions of the kube dns image)

Member

bowei commented Oct 10, 2017

(should be findable via git grep for mentions of the kube dns image)

miekg added a commit to coredns/coredns that referenced this pull request Oct 10, 2017

plugin/cache: add minttl test
See kubernetes/kubernetes#53604, explicitaly add
test to make sure we do the right thing.

miekg added a commit to coredns/coredns that referenced this pull request Oct 10, 2017

plugin/cache: add minttl test
See kubernetes/kubernetes#53604, explicitaly add
test to make sure we do the right thing.

miekg added a commit to coredns/coredns that referenced this pull request Oct 10, 2017

plugin/cache: add minttl test (#1141)
See kubernetes/kubernetes#53604, explicitaly add
test to make sure we do the right thing.
@ahmetb

This comment has been minimized.

Show comment
Hide comment
@ahmetb

ahmetb Oct 10, 2017

Member

<off-topic> @cblecker it doesn't sound a great model that we need to update kubeadm/kops every time we update these files. they should be able to pick up these changes from upstream (this repo) </off-topic>

Member

ahmetb commented Oct 10, 2017

<off-topic> @cblecker it doesn't sound a great model that we need to update kubeadm/kops every time we update these files. they should be able to pick up these changes from upstream (this repo) </off-topic>

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 10, 2017

Member

100% agree. When I was digging into this, I found there were also some references where kube-dns wasn't bumped up to 1.14.5 (#53668 and kubernetes/minikube#2021 (comment)). Having a consistent way to define and deploy kube-dns as an addon would be great.

That being said, there is also a proposal to switch to coredns for cluster DNS services (kubernetes/community#1100), so maybe that would be the right time to fix this?

Member

cblecker commented Oct 10, 2017

100% agree. When I was digging into this, I found there were also some references where kube-dns wasn't bumped up to 1.14.5 (#53668 and kubernetes/minikube#2021 (comment)). Having a consistent way to define and deploy kube-dns as an addon would be great.

That being said, there is also a proposal to switch to coredns for cluster DNS services (kubernetes/community#1100), so maybe that would be the right time to fix this?

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 10, 2017

Member

@ahmetb -- the update strategy for kubeadm needs to be resolved

Member

bowei commented Oct 10, 2017

@ahmetb -- the update strategy for kubeadm needs to be resolved

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 10, 2017

Member

from chatter in #sig-testing it looks like the kubeadm job may be broken. if it fails (even though it's non-blocking), we should probably wait until it's fixed to validate this change.

Member

cblecker commented Oct 10, 2017

from chatter in #sig-testing it looks like the kubeadm job may be broken. if it fails (even though it's non-blocking), we should probably wait until it's fixed to validate this change.

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 11, 2017

Member

/lgtm
/approve no-issue

Member

bowei commented Oct 11, 2017

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 11, 2017

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 11, 2017

Member

/approve no-issue

Member

bowei commented Oct 11, 2017

/approve no-issue

@bowei

This comment has been minimized.

Show comment
Hide comment
@bowei

bowei Oct 11, 2017

Member

/retest

Member

bowei commented Oct 11, 2017

/retest

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 11, 2017

Member

/test pull-kubernetes-e2e-kubeadm-gce

Member

cblecker commented Oct 11, 2017

/test pull-kubernetes-e2e-kubeadm-gce

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 11, 2017

Member

/assign @krousey
for approval

Member

cblecker commented Oct 11, 2017

/assign @krousey
for approval

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 11, 2017

Member

/test pull-kubernetes-bazel-build

Member

cblecker commented Oct 11, 2017

/test pull-kubernetes-bazel-build

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 11, 2017

Member

kubeadm test issue tracked in #53570

Member

cblecker commented Oct 11, 2017

kubeadm test issue tracked in #53570

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 13, 2017

Member

/test pull-kubernetes-bazel-build

Member

cblecker commented Oct 13, 2017

/test pull-kubernetes-bazel-build

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Oct 13, 2017

Member

ping @krousey for approval

Member

cblecker commented Oct 13, 2017

ping @krousey for approval

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Oct 13, 2017

Member
Member

krousey commented Oct 13, 2017

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Oct 13, 2017

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, cblecker, krousey

Associated issue: 121

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Contributor

k8s-merge-robot commented Oct 13, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, cblecker, krousey

Associated issue: 121

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Oct 13, 2017

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Oct 13, 2017

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Oct 13, 2017

Contributor

Automatic merge from submit-queue (batch tested with PRs 53604, 53751). If you want to cherry-pick this change to another branch, please follow the instructions here.

Contributor

k8s-merge-robot commented Oct 13, 2017

Automatic merge from submit-queue (batch tested with PRs 53604, 53751). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 141aa46 into kubernetes:master Oct 13, 2017

8 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-bazel-test
Details
pull-kubernetes-bazel-test Job triggered.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job triggered.
Details
pull-kubernetes-e2e-gce-gpu Job triggered.
Details
pull-kubernetes-unit Jenkins job triggered.
Details
pull-kubernetes-verify Jenkins job triggered.
Details
cla/linuxfoundation cblecker authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-bazel Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details

@cblecker cblecker deleted the cblecker:no-negcache branch Oct 13, 2017

justinsb added a commit to justinsb/kops that referenced this pull request Feb 21, 2018

kube-dns: turn off negcache
The equivalent of kubernetes/kubernetes#53604

Not backporting to 1.5 at this point.

horaceheaven added a commit to horaceheaven/kops that referenced this pull request Feb 28, 2018

kube-dns: turn off negcache
The equivalent of kubernetes/kubernetes#53604

Not backporting to 1.5 at this point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment