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.

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

Revert "daemonset controller should respect taints" #31907

Merged
merged 1 commit into from Sep 2, 2016

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Sep 1, 2016

Reverts #31020

We will be unreverting with some modifications after v1.4.

cc @pwittrock @davidopp


This change is Reviewable

@mikedanese mikedanese added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 1, 2016
@mikedanese mikedanese added this to the v1.4 milestone Sep 1, 2016
@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2016
@davidopp
Copy link
Member

davidopp commented Sep 1, 2016

LGTM

@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 1, 2016
@mikedanese
Copy link
Member Author

@k8s-bot ok to test, issue: #IGNORE

@mikedanese
Copy link
Member Author

@k8s-bot node e2e test this please, issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Sep 2, 2016

GCE e2e build/test passed for commit a615c42.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 9be47a2 into master Sep 2, 2016
@ivan4th
Copy link
Contributor

ivan4th commented Sep 2, 2016

When the time comes for this to be unreverted, please take a look at #31136 too. Also you may let me know what needs to be changed there so I can have it fixed by that time. Thanks!

@lukaszo
Copy link
Contributor

lukaszo commented Sep 2, 2016

@mikedanese Why did you revert it? I don't see any bug for it.

@mikedanese
Copy link
Member Author

@lukaszo daemonset controller should not delete a daemon pod that is already running on a machine if the taint Effect on that node is NoSchedule. This is not the case with this change, we wanted to make sure this is correct when we introduce the behavior and don't want to slightly modify the behavior between v1.4 and v1.5

@lukaszo
Copy link
Contributor

lukaszo commented Sep 2, 2016

I see, thank you for the explanation.

@codablock
Copy link
Contributor

codablock commented Dec 7, 2016

@mikedanese I just realized that the current 1.5 beta still schedules daemons onto nodes with NoSchedule taints. Wasn't it planned to unrevert this after 1.4?

@mikedanese
Copy link
Member Author

@codablock yes but I didn't get around to fixing the issue. I'm going to try very hard to get this into 1.6 at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants