-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Conversation
Labelling this PR as size/M |
Can you add a test? |
GCE e2e build/test passed for commit 7ee41e24b3b41091024326fcab8f58b4bcc4bd8c. |
Yup I'll add a test then reassign to you. |
GCE e2e build/test passed for commit 1d668195a49bf90ed3a57e399acb64546f55e6fa. |
fe950c7
to
f3bd288
Compare
@davidopp I've added a test. |
GCE e2e build/test passed for commit fe950c77c5dedd653c63fecd32b01b56a31388f4. |
GCE e2e build/test passed for commit f3bd2883ef63277f6a6dcb8f96e843fd0b39fd0f. |
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). |
@davidopp The node selector is ok. The pod selector is the one that is getting emptied out. As this test is currently written:
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? |
@davidopp PTAL, I added the scenario you were suggesting |
GCE e2e build/test passed for commit 45f59dc855cff2b9d3dbd0d48ed5ca9710807027. |
LGTM |
Removing label |
GCE e2e build/test passed for commit 625ce91. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Deployments have the same issue. I don't think it makes sense to fix one but not the other. cc @janetkuo |
@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? |
GCE e2e build/test passed for commit 625ce91. |
@mikedanese I responded on that issue. Yes, I think we need an analogous fix for Deployment. |
Ok, I will readd |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 625ce91. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 625ce91. |
Automatic merge from submit-queue |
@@ -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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…3223-upstream-release-1.2 Auto commit by PR queue bot
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. |
…pick-of-#23223-upstream-release-1.2 Auto commit by PR queue bot
…pick-of-#23223-upstream-release-1.2 Auto commit by PR queue bot
Fixes #23218