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

Use SkyDNS as a library for a more integrated kube DNS #23930

Merged
merged 6 commits into from
May 22, 2016
Merged

Use SkyDNS as a library for a more integrated kube DNS #23930

merged 6 commits into from
May 22, 2016

Conversation

ArtfulCoder
Copy link
Contributor

No description provided.

@ArtfulCoder
Copy link
Contributor Author

@thockin
just fyi, if you want to see all the changes done to make a one-container-dns-pod

@ArtfulCoder ArtfulCoder changed the title (DO NOT MERGE EVER) Vendor skydns (DO NOT MERGE EVER) one-container-kubedns-pod Apr 6, 2016
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 6, 2016
@davidopp davidopp assigned thockin and unassigned davidopp Apr 6, 2016
@luxas
Copy link
Member

luxas commented Apr 6, 2016

I understand if you don't want to merge this, but I really like the idea.
This DNS setup would be SO much easier to deploy, update and maintain.
Are you sure you don't want this in?

@ArtfulCoder ArtfulCoder force-pushed the vendor-skydns branch 2 times, most recently from 0bc0091 to d16abf8 Compare April 8, 2016 23:14
@ArtfulCoder ArtfulCoder force-pushed the vendor-skydns branch 2 times, most recently from ce30157 to fcc03a9 Compare May 2, 2016 22:24
@ArtfulCoder ArtfulCoder changed the title (DO NOT MERGE EVER) one-container-kubedns-pod one-container-kubedns-pod May 2, 2016
@ArtfulCoder
Copy link
Contributor Author

@thockin PTAL

@ArtfulCoder
Copy link
Contributor Author

Once the PR is approved, I will push the container image to gcr.

@luxas
Copy link
Member

luxas commented May 3, 2016

Please add the -amd64 suffix to the image name. I can create a followup that cross-builds for the other arches, but the naming should be right from the beginning

@luxas luxas added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 3, 2016
@ArtfulCoder ArtfulCoder force-pushed the vendor-skydns branch 2 times, most recently from 4d70135 to e3f3776 Compare May 3, 2016 20:49
@luxas luxas mentioned this pull request May 3, 2016
Closed
@ArtfulCoder
Copy link
Contributor Author

@luxas added amd64 suffix

luxas
luxas reviewed May 4, 2016
View reviewed changes
cluster/saltbase/salt/kube-dns/skydns-rc.yaml.in
{% endif %}
containers:
- name: kubedns
image: artfulcoder/kubedns-amd64:1.0
Copy link
Member

Choose a reason for hiding this comment

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

todo before lgtm

@luxas
Copy link
Member

luxas commented May 4, 2016

One thing that hit me is that I think the build/kube-dns Dockerfile and Makefile aren't really necessary, even though you've done them very well (you fixed cross-compilation from the start 👍)

So you might consider to use kube-build's inbuilt way of wrapping binaries in containers.
The binary will be placed in /usr/local/bin/kube-dns (code), and the image will be released at the same time as the rest of the binaries/images, so there would be no manual pushing (which I think is great)

What you would have to change is these lines:
https://github.com/ArtfulCoder/kubernetes/blob/vendor-skydns/build/common.sh#L103
https://github.com/ArtfulCoder/kubernetes/blob/vendor-skydns/build/common.sh#L1484

Just add kube-dns to the list along with the arch-specific base image, and add it to the push list, and that's it 😄
WDYT?

thockin
thockin reviewed May 21, 2016
View reviewed changes
pkg/dns/dns.go
// caller is responsible for using the cacheLock before invoking methods on cache
// the cache is not thread-safe, and the caller can guarantee thread safety by using
// the cacheLock
cacheLock *sync.RWMutex
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't need to be a pointer any more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@thockin
Copy link
Member

thockin commented May 21, 2016

I have a few nits, mostly requests for comments. LGTM! self-LGTM after you repush, or just do a followup if by some grace this merges.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2016
@ArtfulCoder
Copy link
Contributor Author

tagged image and pushed.
waiting for green run.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2016
@k8s-bot
Copy link

k8s-bot commented May 21, 2016

GCE e2e build/test passed for commit 3ada217.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2016
@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 May 22, 2016

GCE e2e build/test passed for commit 3ada217.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 82cb4c1 into kubernetes:master May 22, 2016
k8s-github-robot pushed a commit that referenced this pull request May 23, 2016
Merge pull request #25727 from madhusudancs/fed-dns
e958c0c 
Automatic merge from submit-queue

Handle federated service name lookups in kube-dns.

For the domain name queries that fail to match any records in the local
kube-dns cache, we check if the queried name matches the federation
pattern. If it does, we send a CNAME response to the federated name.

For more details look at the comments in the code.

Tests are coming ...

Also, this PR is based on @ArtfulCoder's PR #23930. So please review only the last commit here.

PTAL @ArtfulCoder @thockin @quinton-hoole @nikhiljindal 

[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
@girishkalele girishkalele mentioned this pull request May 24, 2016
Closed
@ravilr
Copy link
Contributor

ravilr commented May 24, 2016

@ArtfulCoder is there an issue with dnsperf load test results after this change. Thanks.

@thockin
Copy link
Member

thockin commented May 24, 2016

Are you reporting a problem or asking for a dnsperf run?

On Tue, May 24, 2016 at 10:59 AM, ravilr notifications@github.com wrote:

@ArtfulCoder https://github.com/ArtfulCoder is there an issue with
dnsperf load test results after this change. Thanks.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#23930 (comment)

@ravilr
Copy link
Contributor

ravilr commented May 24, 2016

@thockin, was looking for perf results, if already run.

@bboreham bboreham mentioned this pull request Jun 29, 2016
Open
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.