Extend dns configmap tests to include CoreDNS #63265

rajansandeep · 2018-04-27T21:07:24Z

What this PR does / why we need it:
This PR extends the DNS configmap e2e tests to include testing the CoreDNS ConfigMap.
The tests now test the equivalent stubdomain, federation and upstreamnameserver configuration of kube-dns for CoreDNS, when CoreDNS is the installed DNS server.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62865

Special notes for your reviewer:

Release note:

NONE

rajansandeep · 2018-05-02T17:22:35Z

/sig network
/area dns

fturib · 2018-05-08T16:39:03Z

/cc @MrHohn

MrHohn

Thanks for the works, this looks good overall.

MrHohn · 2018-05-08T17:47:38Z

test/e2e/network/dns_common.go

@@ -235,6 +242,21 @@ func (t *dnsTestCommon) deleteUtilPod() {
 	}
 }

+// deleteCoreDNSPods manually deletes the CoreDNS pods to apply the changes to the ConfigMap.


Interesting, so CoreDNS pod doesn't reload configmap upon update? Will this behavior change in the future?

Would reload work? https://github.com/coredns/coredns/tree/master/plugin/reload

Yes, reload plugin would work. However it is slow : it can take up to 2mn30 to have the reload effective.
Also there is a dependency with the image used for CoreDNS.
Right now CoreDNS used in kube-up does not support the reload plugin.

(that manifest is expected to be updated next week, before code freeze, with last version of CoreDNS and the support of reload).

Ack, we can live with this for now.

it can take up to 2mn30 to have the reload effective

Curious why 2m30, would this be changed to a pull model in the future?

Oops I meant push..

@MrHohn : it is not expected to be changed.
The current behavior to update the config of CoreDNS is to associate a persitent file named "Corefile" with the configmap "coredns".
The delay to have the persistent file updated after edition of the Configmap can go up to 2mn.
The delay to "reload" the configuration file in CoreDNS is tuned to 30sec (but could be decrease down to 2 sec).

Overall the most important delay is coming from Kubernetes' mechanism to propagate the update to the persistent file.

MrHohn · 2018-05-08T18:23:58Z

test/e2e/network/dns_configmap.go

+           pods insecure
+           upstream
+           fallthrough in-addr.arpa ip6.arpa
+        }


I see the default corefile contains more options than this, should we keep them consistent (for the unchanged part), or at least recover to what it was after test?

kubernetes/cluster/addons/dns/coredns.yaml.base

Lines 57 to 69 in 8d5b9d3

Corefile: |

.:53 {

errors

health

kubernetes __PILLAR__DNS__DOMAIN__ in-addr.arpa ip6.arpa {

pods insecure

upstream

fallthrough in-addr.arpa ip6.arpa

}

prometheus :9153

proxy . /etc/resolv.conf

cache 30

}

I kept the corefile minimal for the tests as the additional options which I removed from the default won't affect the tests.

Fair enough. Can we store the configmap before test, and recover that after test?

MrHohn · 2018-05-08T18:24:48Z

/assign
cc @grayluck

MrHohn · 2018-05-21T22:15:10Z

test/e2e/network/dns_configmap.go

        proxy . /etc/resolv.conf
+        cache 30


Humm...Instead of hardcoding the config value here, can we fetch the configmap directly before test? In that way we are off the burden of keeping these consistent with whatever is in the CoreDNS deployment configuration.

Similar to:

kubernetes/test/e2e/autoscaling/dns_autoscaling.go

Lines 65 to 67 in 7076eed

pcm, err := fetchDNSScalingConfigMap(c)

Expect(err).NotTo(HaveOccurred())

previousParams = pcm.Data

MrHohn

Thanks, this looks good. Could you squash the fix-up commits?
/approve

fturib

I agree with logic.
But, unless I am wrong, we need to ensure the configmap is restored even if execution of the test is interrupted because of failing part.

fturib · 2018-05-22T18:27:29Z

test/e2e/network/dns_configmap.go

@@ -160,6 +162,7 @@ func (t *dnsNameserverTest) run() {
 	defer t.deleteDNSServerPod()

 	if t.name == "coredns" {
+		originalConfigMapData = t.fetchCoreDNSConfigMapData()
 		t.setConfigMap(&v1.ConfigMap{Data: map[string]string{


As we expect that the Configmap returned to the original one, WHATEVER the result of the test. We need to ensure that it is setback to the "originalConfigMapData" with a "defer function" (like the one of line 162).

fturib · 2018-05-22T18:30:17Z

test/e2e/network/dns_configmap.go

+	if t.name == "coredns" {
+		t.setConfigMap(&v1.ConfigMap{Data: originalConfigMapData})
+		t.deleteCoreDNSPods()
+	} else {


we should keep this return to original, as there is a dependent test after it.
But we still need the "defer .." restore to ensure it whatever the result of all tests.

I would think that is also true for the kube-dns clear of ConfigMap.
Maybe be worth to create a function or restoring the original configmap configuration and defer that function on each test.

Good point, defer seems like a better option.

Regarding kube-dns, the default kube-dns configmap is empty. So a reset would probably be enough. But having such function sounds good as well.

fturib · 2018-05-22T18:33:14Z

test/e2e/network/dns_common.go

+		err = podClient.Delete(pod.Name, metav1.NewDeleteOptions(0))
+		Expect(err).NotTo(HaveOccurred())
+	}
+}


Should you wait the Pod is restored before going ahead ? or that is part of a delete option ?

fturib

Thanks for the update

rajansandeep · 2018-05-23T14:35:02Z

/retest

MrHohn · 2018-05-23T18:36:00Z

/lgtm

/test pull-kubernetes-e2e-kops-aws

k8s-ci-robot · 2018-05-23T18:36:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, rajansandeep

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:

~~test/e2e/network/OWNERS~~ [MrHohn]

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

rajansandeep · 2018-05-24T10:59:55Z

/retest

rajansandeep · 2018-05-24T12:09:05Z

/retest

k8s-github-robot · 2018-05-24T16:41:19Z

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 27, 2018

k8s-ci-robot requested review from rramkumar1 and thockin April 27, 2018 21:07

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. area/dns labels May 2, 2018

k8s-ci-robot requested a review from MrHohn May 8, 2018 16:39

MrHohn reviewed May 8, 2018

View reviewed changes

k8s-ci-robot assigned MrHohn May 8, 2018

MrHohn reviewed May 21, 2018

View reviewed changes

MrHohn approved these changes May 22, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2018

fturib suggested changes May 22, 2018

View reviewed changes

extend configmap tests to include CoreDNS

d4ff4ca

fturib approved these changes May 23, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2018

fturib mentioned this pull request May 24, 2018

Enable CoreDNS as a DNS plugin for Kubernetes kubernetes/enhancements#427

Closed

k8s-github-robot merged commit 52f44cd into kubernetes:master May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend dns configmap tests to include CoreDNS #63265

Extend dns configmap tests to include CoreDNS #63265

rajansandeep commented Apr 27, 2018 •

edited

Loading

rajansandeep commented May 2, 2018

fturib commented May 8, 2018

MrHohn left a comment

MrHohn May 8, 2018

MrHohn May 8, 2018

fturib May 18, 2018 •

edited

Loading

MrHohn May 18, 2018

MrHohn May 18, 2018

fturib May 21, 2018

MrHohn May 8, 2018

rajansandeep May 18, 2018

MrHohn May 18, 2018

MrHohn commented May 8, 2018

MrHohn May 21, 2018

rajansandeep May 22, 2018

MrHohn left a comment

fturib left a comment

fturib May 22, 2018

fturib May 22, 2018

fturib May 22, 2018

MrHohn May 22, 2018

fturib May 22, 2018

fturib left a comment

rajansandeep commented May 23, 2018

MrHohn commented May 23, 2018

k8s-ci-robot commented May 23, 2018

rajansandeep commented May 24, 2018

rajansandeep commented May 24, 2018

k8s-github-robot commented May 24, 2018

	Corefile: \|
	.:53 {
	errors
	health
	kubernetes __PILLAR__DNS__DOMAIN__ in-addr.arpa ip6.arpa {
	pods insecure
	upstream
	fallthrough in-addr.arpa ip6.arpa
	}
	prometheus :9153
	proxy . /etc/resolv.conf
	cache 30
	}

	pcm, err := fetchDNSScalingConfigMap(c)
	Expect(err).NotTo(HaveOccurred())
	previousParams = pcm.Data

Extend dns configmap tests to include CoreDNS #63265

Extend dns configmap tests to include CoreDNS #63265

Conversation

rajansandeep commented Apr 27, 2018 • edited Loading

rajansandeep commented May 2, 2018

fturib commented May 8, 2018

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fturib May 18, 2018 • 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

MrHohn commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn left a comment

Choose a reason for hiding this comment

fturib left a 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

fturib left a comment

Choose a reason for hiding this comment

rajansandeep commented May 23, 2018

MrHohn commented May 23, 2018

k8s-ci-robot commented May 23, 2018

rajansandeep commented May 24, 2018

rajansandeep commented May 24, 2018

k8s-github-robot commented May 24, 2018

rajansandeep commented Apr 27, 2018 •

edited

Loading

fturib May 18, 2018 •

edited

Loading