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

don't sync daemonsets with selectors that match all pods #23223

Merged
merged 1 commit into from
Mar 25, 2016

Conversation

mikedanese
Copy link
Member

Fixes #23218

@k8s-github-robot
Copy link

Labelling this PR as size/M

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

Can you add a test?

@k8s-bot
Copy link

k8s-bot commented Mar 18, 2016

GCE e2e build/test passed for commit 7ee41e24b3b41091024326fcab8f58b4bcc4bd8c.

@mikedanese mikedanese assigned mikedanese and unassigned davidopp Mar 18, 2016
@mikedanese
Copy link
Member Author

Yup I'll add a test then reassign to you.

@k8s-bot
Copy link

k8s-bot commented Mar 21, 2016

GCE e2e build/test passed for commit 1d668195a49bf90ed3a57e399acb64546f55e6fa.

@mikedanese mikedanese force-pushed the dont-sync branch 2 times, most recently from fe950c7 to f3bd288 Compare March 21, 2016 21:21
@mikedanese mikedanese assigned davidopp and unassigned mikedanese Mar 21, 2016
@mikedanese
Copy link
Member Author

@davidopp I've added a test.

@mikedanese mikedanese changed the title don't sync daemonsets or controllers with selectors that match all pods don't sync daemonsets with selectors that match all pods Mar 21, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 21, 2016

GCE e2e build/test passed for commit fe950c77c5dedd653c63fecd32b01b56a31388f4.

@k8s-bot
Copy link

k8s-bot commented Mar 21, 2016

GCE e2e build/test passed for commit f3bd2883ef63277f6a6dcb8f96e843fd0b39fd0f.

@davidopp
Copy link
Member

Can you change the test so that the node has a label, and you create in the same test a second DaemonSet, which matches the node's labels, and verify that it schedules? (so that the only difference between the two daemon sets is that one has empty selector and one doesn't?) I realize there are lots of other tests in the file, but this makes the test more self-contained. Also can you change the name of the test so that it says WithEmptyLabelSelector (instead of WithoutLabelSelector) and add comment describing what the test does. Lastly, I noticed several tests in this file all have exactly the same comment describing them -- please fix (even though it's unrelated to this PR).

@mikedanese
Copy link
Member Author

@davidopp The node selector is ok. The pod selector is the one that is getting emptied out. As this test is currently written:

  1. Schedule a pod on a node with a realistic label and not controlled by a daemonset
  2. Create a daemonset that matches no nodes and matches all pods.

Before this change, this would cause the daemonset to try to delete all pods in the cluster because it would see them as miss-scheduled. After this change, we assert that the pod created in step 1. is not deleted. Do you just want me to test other scenarios?

@mikedanese
Copy link
Member Author

@davidopp PTAL, I added the scenario you were suggesting

@k8s-bot
Copy link

k8s-bot commented Mar 24, 2016

GCE e2e build/test passed for commit 45f59dc855cff2b9d3dbd0d48ed5ca9710807027.

@davidopp
Copy link
Member

LGTM

@davidopp davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate labels Mar 24, 2016
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@k8s-bot
Copy link

k8s-bot commented Mar 24, 2016

GCE e2e build/test passed for commit 625ce91.

@bgrant0607 bgrant0607 added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 24, 2016
@k8s-github-robot
Copy link

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

@bgrant0607
Copy link
Member

Deployments have the same issue. I don't think it makes sense to fix one but not the other.

cc @janetkuo

@mikedanese
Copy link
Member Author

@bgrant0607 I mentioned that I didn't think deployments had the issue #23218 (comment) or that it wasn't exactly analagous. I had added the same feature in deployment controller and removed it. Do you want me to readd it?

@k8s-bot
Copy link

k8s-bot commented Mar 24, 2016

GCE e2e build/test passed for commit 625ce91.

@bgrant0607
Copy link
Member

@mikedanese I responded on that issue. Yes, I think we need an analogous fix for Deployment.

@mikedanese
Copy link
Member Author

Ok, I will readd

@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 Mar 25, 2016

GCE e2e build/test passed for commit 625ce91.

@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 Mar 25, 2016

GCE e2e build/test passed for commit 625ce91.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b94b394 into kubernetes:master Mar 25, 2016
@mikedanese mikedanese deleted the dont-sync branch March 25, 2016 02:14
@@ -623,6 +625,12 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error {
}
ds := obj.(*extensions.DaemonSet)

everything := unversioned.LabelSelector{}
if reflect.DeepEqual(ds.Spec.Selector, &everything) {
dsc.eventRecorder.Eventf(ds, api.EventTypeWarning, "SelectingAll", "This controller is selecting all pods. Skipping sync.")
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned on the Deployment PR, I think we need to improve this message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will fix both

@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 26, 2016
k8s-github-robot referenced this pull request Mar 26, 2016
…3223-upstream-release-1.2

Auto commit by PR queue bot
@bgrant0607 bgrant0607 added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 30, 2016
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

shyamjvs referenced this pull request in shyamjvs/kubernetes Dec 1, 2016
…pick-of-#23223-upstream-release-1.2

Auto commit by PR queue bot
shouhong referenced this pull request in shouhong/kubernetes Feb 14, 2017
…pick-of-#23223-upstream-release-1.2

Auto commit by PR queue bot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

8 participants