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.

Sign up for GitHub

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

Jump to bottom

SkyDNS is the only NS for Pods with DNSPolicy=ClusterFirst #15645

Merged

k8s-github-robot merged 1 commit into kubernetes:master from ArtfulCoder:oneNS

Dec 1, 2015

+19 −8

Contributor

ArtfulCoder commented Oct 14, 2015

Addresses issue : #15592
See #15592 (comment)

googlebot added the cla: yes label

k8s-github-robot commented Oct 14, 2015

Labelling this PR as size/XS

k8s-github-robot assigned vishh

k8s-github-robot added the size/XS label

ArtfulCoder unassigned vishh

Contributor

vishh commented Oct 14, 2015

@ArtfulCoder: Can you add a brief description on why this change is required?

Contributor Author

ArtfulCoder commented Oct 14, 2015

Sure . I can copy it from on the issue I have linked already.

k8s-bot commented Oct 14, 2015

GCE e2e test build/test passed for commit 681b8e3bc5ebb097fb9f8d5d9917cb08b362855f.

k8s-github-robot assigned dchen1107

thockin reviewed

View reviewed changes

pkg/kubelet/kubelet.go

    
            @@ -1190,7 +1190,7 @@ func (kl *Kubelet) getClusterDNS(pod *api.Pod) ([]string, []string, error) {
          
              	var dns, dnsSearch []string

              	if kl.clusterDNS != nil {

              		dns = append([]string{kl.clusterDNS.String()}, hostDNS...)

              		dns = []string{kl.clusterDNS.String()}

Member

thockin Oct 15, 2015

Comment on this, please. It's less obvious now. Also comment on L1163 is now wrong. Please audit this function to make sure comments match reality

ArtfulCoder assigned thockin and unassigned dchen1107

ArtfulCoder added this to the v1.2 milestone

ArtfulCoder added the sig/network label

Contributor Author

ArtfulCoder commented Oct 16, 2015

Punting this PR to 1.2 (after #15752 is addressed)

ArtfulCoder force-pushed the oneNS branch from 681b8e3 to 271bcb7 Compare

October 30, 2015 00:44

k8s-github-robot commented Oct 30, 2015

Labelling this PR as size/S

k8s-github-robot added size/S and removed size/XS labels

k8s-bot commented Oct 30, 2015

GCE e2e test build/test passed for commit 271bcb7f9e68802fbc11bcf4f4632c864118d29b.

ArtfulCoder added the release-note label

ArtfulCoder force-pushed the oneNS branch from 271bcb7 to 937fc79 Compare

October 30, 2015 18:58

k8s-bot commented Oct 30, 2015

GCE e2e test build/test passed for commit 937fc795be772bf7b2ab1c8e95153eef26742a71.

Contributor Author

ArtfulCoder commented Oct 30, 2015

Contributor Author

ArtfulCoder commented Oct 30, 2015

@dchen1107 since it is a kubelet change

thockin reviewed

View reviewed changes

pkg/kubelet/kubelet.go

    
            @@ -1330,9 +1330,17 @@ func (kl *Kubelet) getClusterDNS(pod *api.Pod) ([]string, []string, error) {
          
              	var dns, dnsSearch []string

              	if kl.clusterDNS != nil {

              		dns = append([]string{kl.clusterDNS.String()}, hostDNS...)

              		// for a pod with DnsClusterFirst policy, the cluster DNS server is the only nameserver configured for

Member

thockin Nov 2, 2015

nit: DNSClusterFirst

Contributor Author

ArtfulCoder Nov 19, 2015

done

Member

thockin commented Nov 2, 2015

nits then LGTM

Member

thockin commented Nov 16, 2015

status on this?

Contributor Author

ArtfulCoder commented Nov 19, 2015

will incorporate your feedback (was waiting for iptables proxy to be default)

Contributor Author

ArtfulCoder commented Nov 25, 2015

Incorporated Tim's feedback and re-added LGTM label that got removed because I update the PR with the feeback.

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit cf37606181d3ad97d40c7d8a8c8e39326fc8783e.

ArtfulCoder removed the lgtm label


          SkyDNS is the only NS for Pods with DNSPolicy=ClusterFirst

015df14

ArtfulCoder force-pushed the oneNS branch from cf37606 to 015df14 Compare

November 25, 2015 23:03

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit 015df14.

ArtfulCoder added the lgtm label

k8s-github-robot commented Dec 1, 2015

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 015df14.

Contributor Author

ArtfulCoder commented Dec 1, 2015

@k8s-bot test this

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 015df14.

Contributor Author

ArtfulCoder commented Dec 1, 2015

@k8s-bot test this

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 015df14.

k8s-github-robot commented Dec 1, 2015

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 015df14.

k8s-github-robot commented Dec 1, 2015

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #15645 from ArtfulCoder/oneNS

Auto commit by PR queue bot

k8s-github-robot merged commit 4602220 into kubernetes:master

janetkuo mentioned this pull request

Fix local cluster script #18046

Merged

This was referenced Dec 2, 2015

Revert "SkyDNS is the only NS for Pods with DNSPolicy=ClusterFirst" #18057

Merged

Kubemark density test failing because kubelet doesn't have a ClusterDNS #18056

Closed

Member

wojtek-t commented Dec 2, 2015

@thockin @ArtfulCoder @gmarek

Just to give more clear context why we decided to revert it.

it broke kubemark-gce, which is currently a blocking build for mergebot

A bit more context.

in kubemark, there is no skydns (we want to add it at some point, but that's not done yet)
however, with this PR merge, this caused that no pods were started (all the pods were successfully created & scheduled, but not even a single pod was started)
note, that we are using "pause" images, which aren't the most demanding ones :)

So I think the question is the following:
if this change requires DNS to start any pod, it has be clearly stated (I'm not saying it's not reasonable - it makes sense to say that).
However, if it wasn't the intention of this PR, then there was some bug in it.

Contributor

gmarek commented Dec 2, 2015

If we're going to require to DNS to start any pod, we probably should remove an option to disable it through config.

Contributor

eparis commented Dec 2, 2015

I'll throw in my objection to 'dns is required' until dns is a part of the system, instead of something on top of the system. aka I think @DirectXMan12 did some work to embed dns inside each kubelet automatically, instead of having to do something magic after the cluster is up. (not sure where it ended up). So i'm against such a new requirement as things stand today.

Real world example, the kubernetes cluster where the submit queue bot runs doesn't have dns and seems to work just fine.

Contributor Author

ArtfulCoder commented Dec 2, 2015

We don't need to have DNS in the cluster.
This also means that, we should have pods with DNSpolicy of ClusterFirst.
If we change the pod spec in the test to not use CluaterFirst DNSPolicy, the test will be green again.

I feel this is correct. We should not allow such pods to run when there is no cluster DNS server.

Sent from my iPhone

On Dec 2, 2015, at 5:59 AM, Eric Paris notifications@github.com wrote:

I'll throw in my objection to 'dns is required' until dns is a part of the system, instead of something on top of the system. aka I think @DirectXMan12 did some work to embed dns inside each kubelet automatically, instead of having to do something magic after the cluster is up. (not sure where it ended up). So i'm against such a new requirement as things stand today.

Real world example, the kubernetes cluster where the submit queue bot runs doesn't have dns and seems to work just fine.

—
Reply to this email directly or view it on GitHub.

Member

wojtek-t commented Dec 2, 2015

@ArtfulCoder - got it thanks (I think I just didn't understand it).

@gmarek - can you please fix density test (that kubemark is running) so that all pods there use different dns policy. And then we should re-revert this PR.

Contributor

gmarek commented Dec 2, 2015

The thing is that we were not explicitly setting any DNSpolicy, and this change broke us. Can we make DNSDefault a default value?

Contributor Author

ArtfulCoder commented Dec 2, 2015

Tim suggested that we just fall back to DNSDefault if dns server is not
available and log a warning.
I will make the change and it should fix these issues.

On Wed, Dec 2, 2015 at 8:31 AM, Marek Grabowski notifications@github.com
wrote:

The thing is that we were not explicitly setting any DNSpolicy, and this
change broke us. Can we make DNSDefault a default value?

—
Reply to this email directly or view it on GitHub
#15645 (comment)
.

Contributor

gmarek commented Dec 2, 2015

Perfect - thanks. In case it is needed: #18081

ArtfulCoder mentioned this pull request

SkyDNS is the only NS for Pods with DNSPolicy=ClusterFirst #18089

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm release-note sig/network size/S