don't sync deployment when pod selector is empty #23467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Labelling this PR as size/XS |
k8s-github-robot
added
the
size/XS
|
GCE e2e build/test passed for commit 3755f7f10f8d509cc060326b7df7c5f5566ec5d1. |
mikedanese
force-pushed
the
dont-sync-deployment
branch
from
3755f7f
to
f145b91
Compare
| for _, action := range fake.Actions() { | ||
| t.Logf("unexpected action: %#v", action) | ||
| } | ||
| t.Errorf("expected deployment controller to not take action") |
There was a problem hiding this comment.
On master this test fails with:
=== RUN TestDeploymentController_dontSyncDeploymentsWithEmptyPodSelector
--- FAIL: TestDeploymentController_dontSyncDeploymentsWithEmptyPodSelector (0.00s)
deployment_controller_test.go:810: unexpected action: core.CreateActionImpl{ActionImpl:core.ActionImpl{Namespace:"default", Verb:"create", Resource:"replicasets", Subresource:""}, Object:(*extensions.ReplicaSet)(0xc2081238c0)}
deployment_controller_test.go:810: unexpected action: core.UpdateActionImpl{ActionImpl:core.ActionImpl{Namespace:"default", Verb:"update", Resource:"deployments", Subresource:""}, Object:(*extensions.Deployment)(0xc208081b80)}
deployment_controller_test.go:810: unexpected action: core.UpdateActionImpl{ActionImpl:core.ActionImpl{Namespace:"", Verb:"update", Resource:"replicasets", Subresource:""}, Object:(*extensions.ReplicaSet)(0xc208123b00)}
deployment_controller_test.go:810: unexpected action: core.ListActionImpl{ActionImpl:core.ActionImpl{Namespace:"", Verb:"list", Resource:"pods", Subresource:""}, ListRestrictions:core.ListRestrictions{Labels:labels.nothingSelector{}, Fields:fields.andTerm{}}}
deployment_controller_test.go:810: unexpected action: core.UpdateActionImpl{ActionImpl:core.ActionImpl{Namespace:"default", Verb:"update", Resource:"deployments", Subresource:"status"}, Object:(*extensions.Deployment)(0xc2081b6500)}
deployment_controller_test.go:812: expected deployment controller to not take action
FAIL
There was a problem hiding this comment.
Clarification: WIthout the controller change, this test fails, as expected.
k8s-github-robot
added
size/S
|
Labelling this PR as size/S |
mikedanese
force-pushed
the
dont-sync-deployment
branch
from
f145b91
to
54c32f9
Compare
k8s-github-robot
removed
the
size/S
|
Labelling this PR as size/M |
k8s-github-robot
added
the
size/M
|
GCE e2e build/test passed for commit f145b91540773de9b9de608d4862cfb75a1eddb7. |
|
GCE e2e build/test passed for commit 54c32f95d042fb2acc714f4cf80452ba6f0590c1. |
janetkuo
added
the
lgtm
| @@ -426,6 +426,11 @@ func (dc *DeploymentController) syncDeployment(key string) error { | |||
| } | |||
|
|
|||
| d := obj.(*extensions.Deployment) | |||
| everything := unversioned.LabelSelector{} | |||
| if reflect.DeepEqual(d.Spec.Selector, &everything) { | |||
| dc.eventRecorder.Eventf(d, api.EventTypeWarning, "SelectingAll", "This controller is selecting all pods. Skipping sync.") | |||
There was a problem hiding this comment.
s/controller/Deployment/
I doubt "skipping sync" is very meaningful to users. How about "A non-empty selector is required."
bgrant0607
removed
the
lgtm
mikedanese
force-pushed
the
dont-sync-deployment
branch
from
54c32f9
to
8028751
Compare
|
@bgrant0607 @janetkuo I fixed both the daemon set and the deployment event message. PTAL |
| @@ -426,6 +426,11 @@ func (dc *DeploymentController) syncDeployment(key string) error { | |||
| } | |||
|
|
|||
| d := obj.(*extensions.Deployment) | |||
| everything := unversioned.LabelSelector{} | |||
| if reflect.DeepEqual(d.Spec.Selector, &everything) { | |||
| dc.eventRecorder.Eventf(d, api.EventTypeWarning, "SelectingAll", "This deployment is selecting all pods. A non-empty sync is required.") | |||
mikedanese
force-pushed
the
dont-sync-deployment
branch
from
8028751
to
c430576
Compare
|
GCE e2e build/test passed for commit 8028751905beb2a250484826dda8cbcf2e668397. |
|
Thanks much. LGTM |
bgrant0607
added
lgtm
|
GCE e2e build/test passed for commit c430576. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit c430576. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit c430576. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit c430576. |
|
Automatic merge from submit-queue |
85 tasks
bgrant0607
added
the
cherry-pick-approved
bgrant0607
added
the
release-note
|
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
pushed a commit
to shyamjvs/kubernetes
that referenced
this pull request
Dec 1, 2016
…pick-of-#23530-kubernetes#23467-upstream-release-1.2 Automated cherry pick of kubernetes#23530 kubernetes#23467
shouhong
pushed a commit
to shouhong/kubernetes
that referenced
this pull request
Feb 14, 2017
…pick-of-#23530-kubernetes#23467-upstream-release-1.2 Automated cherry pick of kubernetes#23530 kubernetes#23467
openshift-publish-robot
pushed a commit
to openshift/kubernetes
that referenced
this pull request
Jul 26, 2019
…from-kube UPSTREAM: <carry>: remove embedded oauth server Origin-commit: 68b25f8b2004119ef841feaf4a9f2c936b432363
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #23218
cc @bgrant0607 @janetkuo