Skip to content
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

Added DNS Reverse Record logic for service IPs #26226

Merged
merged 1 commit into from
Jun 3, 2016
Merged

Added DNS Reverse Record logic for service IPs #26226

merged 1 commit into from
Jun 3, 2016
+95 −26

Conversation

ArtfulCoder
Copy link
Contributor

No description provided.

@ArtfulCoder ArtfulCoder added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 24, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2016
thockin
thockin reviewed May 25, 2016
View reviewed changes
pkg/dns/dns.go
kd.cacheLock.Lock()
defer kd.cacheLock.Unlock()
kd.cache.setSubCache(service.Name, subCache, subCachePath...)
kd.reverseRecordMap[service.Spec.ClusterIP] = reverseRecord
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add reverse records for petset pods (with hostname and subdomain set AND a governing headless service? Maybe that's yet another followup. Can you just file an issue on that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed issue #26752 to track this.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016
@thockin thockin changed the title Added DNS Reverse Record logic Added DNS Reverse Record logic for service IPs May 25, 2016
@thockin
Copy link
Member

thockin commented May 25, 2016

LGTM

please build and push the image and get green on tests.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@ArtfulCoder ArtfulCoder added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 25, 2016
@googlebot
Copy link

CLAs look good, thanks!

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016
@thockin thockin added this to the v1.3 milestone May 25, 2016
@thockin
Copy link
Member

thockin commented May 25, 2016

status on this? It's marked "do not merge"...

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016
@ArtfulCoder
Copy link
Contributor Author

will rebase today and track the merge.

@ArtfulCoder
Copy link
Contributor Author

I marked it as do-not-merge since I wanted the dnsmasq PR to go in first.
I will take care of this today.

@thockin
Copy link
Member

thockin commented May 27, 2016

dnsmasq is in

@ArtfulCoder ArtfulCoder added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 27, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2016
@ArtfulCoder ArtfulCoder added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 27, 2016
@ArtfulCoder ArtfulCoder added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 27, 2016
@ArtfulCoder
Copy link
Contributor Author

@k8s-bot test this issue: #IGNORE

@ArtfulCoder
Copy link
Contributor Author

ArtfulCoder commented May 27, 2016 •
edited
Loading

blocked by #26335 @girishkalele

@ArtfulCoder
Copy link
Contributor Author

the test for the PR fails because #26335 is not yet merged.

@wojtek-t
Copy link
Member

I think test failure may be related...

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2016
@ArtfulCoder
Copy link
Contributor Author

rebased. waiting for test run.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 1, 2016
@ArtfulCoder ArtfulCoder added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2016
@ArtfulCoder
Copy link
Contributor Author

@girishkalele can you look into this.
The failure is related to the PR change.
I rebaesd yest. in the hope that the test would pass but it didnt :(

@ArtfulCoder
Copy link
Contributor Author

@girishkalele you might want to do a new kube-dns image with the code of the PR and then try.. hopefully, it is a simple case of pushing a new kube-dns image.

@girishkalele
Copy link

@ArtfulCoder

If I understand correctly, I checkout your PR, build a new kube-dns from your code and push to Google Containers. Will do.

@girishkalele
Copy link

@k8s-bot test this issue: #IGNORE

@girishkalele girishkalele mentioned this pull request Jun 2, 2016
Closed
@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Jun 3, 2016

GCE e2e build/test passed for commit 4224dbd.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 693dae7 into kubernetes:master Jun 3, 2016
@bprashanth bprashanth mentioned this pull request Jun 4, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thockin thockin

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
7 participants
@ArtfulCoder @thockin @googlebot @wojtek-t @girishkalele @k8s-github-robot @k8s-bot