-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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.
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
Use a Deployment for kube-dns #32018
Conversation
f155b4b
to
24dd81b
Compare
24dd81b
to
b5c17fa
Compare
cc @thockin |
Have we tested upgrades from a version where kube-dns is running as an RC (e.g. 1.3.6) to a cluster where it's running as a deployment (with this PR) to ensure that it works as expected? And then tested upgrading to a version with a newer deployment to ensure that it works properly? The advantage of using a deployment is the update semantics (you could tell the deployment to update instead of deleting & recreating it), but I don't believe that the addon manager will actually do that during an update, which negates the advantages of switching. While it would be nice if all cluster addons ran in deployments, this seems to introduce churn unnecessarily. @kubernetes/sig-cluster-lifecycle is working on replacing the current addon management system with something nicer (as part of the kubeadm work) so it might be better to just defer trying to clean this up in the mean time (adding extra review & testing time) and wait for the overhaul to the addon management system. |
It is correct that the current addon manager would not take the advantage of deployment during upgrade. As below codes indicate:
And switching kube-dns to using deployment has not been completely tested. Should hold this off with regard to testing and addon manager's upgrade. |
Can someone speak to what the lifecycle work is, if not a deployment? On Wed, Sep 7, 2016 at 8:40 AM, Robert Bailey notifications@github.com
|
I'm guessing a rewrite of this shell script: https://github.com/kubernetes/kubernetes/blob/master/cluster/addons/addon-manager/kube-addon-update.sh Recreating a deployment is useless, by |
Eitherways I guess we'd be looking at 1.5 rollout, so planning for all addons in deployments and using apply instead of create/delet in the new addon controller SGTM (if that's the proposed plan of action) |
I think On Wed, Sep 7, 2016 at 10:11 AM, Prashanth B notifications@github.com
|
#29551 is the last feature I was hoping for in kubectl apply before using it for addons. Eventual goal for addon management is |
do we have a plan for how to delete an addon that used to be an RC and On Wed, Sep 7, 2016 at 10:32 AM, Mike Danese notifications@github.com
|
we can orphan the pods (update rc selectos and scale to 0 in one update) then create the deployment so the rs adopts them |
We just need a way to describe the transition - dropping files into a dir On Wed, Sep 7, 2016 at 10:47 AM, Prashanth B notifications@github.com
|
Isn't that what #29551 does? |
From a quick scan, I don't want to delete the dns pods i just want to delete the rc with --cascade=false, then deployment upgrade the dns pods to the new version |
I think that can be covered with the use of a label selector with https://github.com/kubernetes/kubernetes/blob/master/cluster/addons/dns/skydns-rc.yaml.in#L40 Regardless, the primary motivation of #29551 is exactly this so if you see something that is broken about the proposal it would be helpful if you pointed it out. |
GCE e2e build/test passed for commit b5c17fa. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit b5c17fa. |
Automatic merge from submit-queue |
Wait, we didn't actually want this to merge correct? |
Oh, I do think it should not be merged too. Sorry |
@thockin added an lgtm label 8 days ago and i guess we never removed it... |
I think it's fine since were in cherrypick mode. Do not cherrypick this. Fair? On Sat, Sep 10, 2016 at 11:48 PM, Robert Bailey notifications@github.com
|
I merged the revert of this PR - it completely broke Rescheduler tests. |
Once re-reverting please ensure that you update also Rescheduler e2e. |
@Mrhohh we should talk about this one. I am WFH this morning, but let's On Mon, Sep 12, 2016 at 1:01 AM, Piotr Szczesniak notifications@github.com
|
@MrHohn no worries. It's not obvious. |
Got it. Thanks!
…On Thu, 1 Dec 2016 at 11:15 PM, Zihong Zheng ***@***.***> wrote:
@ApsOps <https://github.com/ApsOps> Sorry for the confusion. This is not
in v1.4.x. This PR got reverted by #32452
<#32452> a day after it was
merged.
kube-dns are now using a Deployment (introduced by #36008
<#36008>). And it is part of
v1.5.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#32018 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACQOwpJ_sVS8i5Y6XD9EVmpRaJ3qSnJHks5rDwepgaJpZM4J0KFt>
.
|
Attempt to fix #31554
Switching kube-dns from using Replication Controller to Deployment.
The outdated kube-dns YAML file in coreos and juju dir is also updated. Most of the specific memory limit in the files remain unchanged because it seems like people were modifying it explicitly(c8d82fc). Only the memory limit for healthz is increased due to this pending investigation(#29688).
YAML files stay in *-rc.yaml format considering there are a lots of scripts in cluster and hack dirs are using this format. But it may be fine to changed them all.
@bprashanth @girishkalele
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)