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

Switch DNS addons from skydns to kubedns #26335

Merged
merged 3 commits into from
May 31, 2016
Merged

Switch DNS addons from skydns to kubedns #26335

merged 3 commits into from
May 31, 2016

Conversation

girishkalele
Copy link

Change GCI and trusty cluster-helper scripts to use kubedns instead of skydns.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 26, 2016
luxas
luxas reviewed May 26, 2016
View reviewed changes
cluster/addons/dns/kubedns-rc.yaml.in
name: dns-tcp
protocol: TCP
- name: healthz
image: gcr.io/google_containers/exechealthz: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.

exechealthz-amd64:1.0

@bprashanth
Copy link
Contributor

Can you update the RELEASE guide with how to build a new image (there's one in the kube2sky subdir, not sure how we want to documenting releasing this though, maybe add it to build/kube-dns?), and add a comment in skydns.rc that it's deprecated. I'd also prefer just having one kubedns.rs, so maybe remove the deployment in cluster/saltbase for now?

@roberthbailey
Copy link
Contributor

lgtm once you've addressed comments from @luxas

@girishkalele
Copy link
Author

Noticed a new merge to master - some more changes to the kubedns-rc saltbase files.

@ArtfulCoder - after the switch to GCI images, the new kubedns pod is running anymore when I do kube-up. Its running the old skydns pod. I am trying to fix the files in the addons directory and cluster-helper to switch to kube-dns.

@roberthbailey roberthbailey mentioned this pull request May 27, 2016
Closed
@ArtfulCoder
Copy link
Contributor

  1. lets pick 1.2 kubedns image
  2. lets have the dnsmasq container too.
    https://github.com/ArtfulCoder/kubernetes/blob/a7bf2455782695924cdee50f6818e18da788b5f5/cluster/saltbase/salt/kube-dns/kubedns-rc.yaml.in

@ArtfulCoder ArtfulCoder mentioned this pull request May 27, 2016
Merged
thockin
thockin reviewed May 27, 2016
View reviewed changes
cluster/addons/dns/kubedns-rc.yaml.in
@@ -0,0 +1,80 @@
apiVersion: v1
kind: ReplicationController
Copy link
Member

Choose a reason for hiding this comment

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

isn't this basically the same as cluster/saltbase/salt/kube-dns/kubedns-rc.yaml.in ? Why do we need both? Can we just kill off cluster/addons/dns and leave a pointer to the new location?

Copy link
Member

Choose a reason for hiding this comment

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

also see #25020

@thockin
Copy link
Member

thockin commented May 27, 2016

I'm all for expedient solutions, but this seems redundant, doesn't it? can we discuss quickly today?

@girishkalele
Copy link
Author

@thockin
Yes, this was getting more confusing with all the duplication.
Yesterday I took a step back to take a look at all the duplication of the skydns-rc.yaml.in and skydns-rc.yaml and understand how these are getting packaged into the manifests tarball for GCE/GKE vs other platforms.
I believe the the old directories cluster/addons/dns/(kube2sky|skydns) also need to go away since they are no longer in use.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2016
@girishkalele
Copy link
Author

Was labelled XXL because of the large deletions of deprecated kube2sky and skydns directories.

bprashanth
bprashanth reviewed May 27, 2016
View reviewed changes
cluster/addons/dns/skydns-rc.yaml.in
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you're doing this anyway why not just get rid of addons and move to cluster/saltbase instead? (IMO addons/ for DNS is confusing, and I thought previously you wanted to keep the change small)

Copy link
Author

Choose a reason for hiding this comment

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

Tim suggested the same (remove from 'addons')

I am deleting the cluster/addons/dns directory completely and moving templates to saltbase and build code to build/kube-dns.

@girishkalele girishkalele mentioned this pull request May 27, 2016
Closed
bprashanth
bprashanth reviewed May 28, 2016
View reviewed changes
docs/devel/running-locally.md
@@ -189,7 +189,7 @@ KUBE_DNS_DOMAIN="cluster.local"
KUBE_DNS_REPLICAS=1
```

To know more on DNS service you can look [here](http://issue.k8s.io/6667). Related documents can be found [here](../../cluster/addons/dns/#how-do-i-configure-it)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also update local-up-cluster?

@bprashanth
Copy link
Contributor

Changing the filename from skydns-rc/svc to kubedns-rc/svc rename will be a much noisier change, I held off on renaming it for that reason.

If it's not just a simple rename, I'm fine with a follow up, as long as you add a todo somewhere. I don't want people to see skydns as a pod name when they do kubectl get pods because that's misleading.

I think we should get this in asap so we can start testing it asap, hopefully before we cut the branch.

bprashanth
bprashanth reviewed May 31, 2016
View reviewed changes
cluster/saltbase/salt/kube-dns/skydns-rc.yaml.in
@@ -53,7 +71,7 @@ spec:
timeoutSeconds: 5
args:
# command = "/kube-dns"
- --domain={{ pillar['dns_domain'] }}.
- --domain={{ pillar['dns_domain'] }}
Copy link
Contributor

Choose a reason for hiding this comment

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

are you removing the "." ?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch...the . needs to be there.

@bprashanth
Copy link
Contributor

This is your issue if you need a unittest rerun and are not going to upload a new pr (#26578)

@girishkalele
Copy link
Author

Oh thanks...I didn't see the test failure - it seems like it was a flake, I will submit the review changes and rerun the tests.

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

We should fix the v13 x image: 1.2 offset, but not necessarily in this pr since there will be more pushes. I also guess this doesn't need a release note since we're adding one for the inital kube-dns.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2016
@bprashanth bprashanth added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 31, 2016
@bprashanth
Copy link
Contributor

Marking p2 since DNS is currently getting no test coverage

@bprashanth bprashanth added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 31, 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 31, 2016

GCE e2e build/test passed for commit 555b900.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ee412ef into kubernetes:master May 31, 2016
@bprashanth bprashanth mentioned this pull request Jun 1, 2016
Closed
mikedanese added a commit to mikedanese/kubernetes that referenced this pull request Jun 1, 2016
@mikedanese
fix godeps on master broken in kubernetes#26335
cadfaee
@mikedanese mikedanese mentioned this pull request Jun 1, 2016
Merged
mikedanese added a commit that referenced this pull request Jun 1, 2016
@mikedanese
Merge pull request #26607 from mikedanese/fix-godeps
f575c81 
fix godeps on master broken in #26335
@girishkalele girishkalele deleted the kubedns-transition branch June 1, 2016 17:01
This was referenced Jun 1, 2016
Merged
Closed
@bprashanth bprashanth mentioned this pull request Jun 4, 2016
Closed
@zmerlynn zmerlynn mentioned this pull request Jun 5, 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/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. 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.
10 participants
@girishkalele @bprashanth @roberthbailey @ArtfulCoder @thockin @k8s-github-robot @k8s-bot @colemickens @luxas @googlebot