Controllers doesn't take any actions when being deleted. #27438

gmarek · 2016-06-15T13:38:46Z

I started doing it for other controllers but it's not always clear to me how it should work. I'll be adding other ones as separate commits to this PR.

cc @caesarxuchao @lavalamp

caesarxuchao · 2016-06-15T17:52:49Z

pkg/controller/daemon/controller.go

@@ -671,7 +671,7 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error {
 		return err
 	}
 	dsNeedsSync := dsc.expectations.SatisfiedExpectations(dsKey)
-	if dsNeedsSync {
+	if dsNeedsSync && ds.DeletionTimestamp == nil {


I'm doing this in a similar way for RC, so SGTM.

For RC, I also need to check DeletionTimestamp!=nil before adopting orphaned pods. daemon controller may need to do the same.

The amount of boilerplate is insane...

Am I mistaken, or adoption is not implemented for Daemons yet?

It's where you want to set the pod's Controller ref. I mean you need to stop setting controller ref if the controller is about to be deleted. In daemon set's case, we might do it in addPod().

I'll push my changes to RC later today so I can get your comments, though it's not completed :)

gmarek · 2016-06-17T12:49:44Z

CC @janetkuo for deployment @soltysh for job

caesarxuchao · 2016-06-17T16:31:05Z

#27600 is a WIP implementation for RC.

soltysh · 2016-06-20T09:55:46Z

cc @Kargakis @pwittrock @mfojtik for deployments

soltysh · 2016-06-20T09:57:11Z

jobs LGTM

0xmichalis · 2016-06-20T09:58:09Z

Shouldn't we just not resync the object in case it is deleted? It's much simpler than handling DeletionTimestamp in the controller loop.

0xmichalis · 2016-06-20T10:04:18Z

pkg/controller/deployment/deployment_controller.go

 	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 selector is required.")
 		return nil
 	}

-	if d.Spec.Paused {
+	if d.Spec.Paused || d.DeletionTimestamp != nil {


A deleted deployment is different from a paused deployment; these two should be separate. I think the best fix would be to stop requeueing the deployment when it is deleted.

Please update this and check for the timestamp before checking for paused. #20273 changes paused deployments to do more things than simple staus updates.

gmarek · 2016-06-20T10:14:14Z

@Kargakis - I'm not certain if I understand correctly what you're saying - do you mean that we shouldn't run syncPausedDeploymentStatus on DeploymentController if deletionTimestamp is set? That was my original idea, but if I correctly recall my discussion with @bgrant0607 we decided that we want to prevent controllers from doing any meaningful changes while we still want to update Statuses.

0xmichalis · 2016-06-20T10:18:48Z

I mean that we shouldn't requeue at all on deletion:

kubernetes/pkg/controller/deployment/deployment_controller.go

Line 214 in d244a34

dc.enqueueDeployment(d)

Deletions ought to be fast so I am not sure if there is any value in updating the status of an object but I may be wrong.

gmarek · 2016-06-20T10:21:34Z

@Kargakis - they won't be fast soon enough, when cascading deletion will be turned on. Before deleting object X we'll wait until all dependees are deleted, which can take some time - especially that Pods do have 30s grace period by default.

gmarek · 2016-06-20T10:22:12Z

Thanks for the line - I'll look at that part of code.

0xmichalis · 2016-06-20T10:22:43Z

@Kargakis - they won't be fast soon enough, when cascading deletion will be turned on. Before deleting object X we'll wait until all dependees are deleted, which can take some time - especially that Pods do have 30s grace period by default.

Hm, i see. Agreed

gmarek · 2016-06-23T13:17:24Z

I didn't manage to finish this before vacation:/ Let's proceed with reviews.

pwittrock · 2016-06-23T22:11:36Z

Added myself as a reviewer for Deployments.

caesarxuchao · 2016-06-23T22:38:21Z

@janetkuo @pwittrock do you know if the deployment controller adopts existing replication controllers? Non-empty DeleteTimestamp should prevent the adoption as well.

0xmichalis · 2016-06-24T08:32:42Z

@janetkuo @pwittrock do you know if the deployment controller adopts existing replication controllers? Non-empty DeleteTimestamp should prevent the adoption as well.

Yes, Deployments can adopt replica sets.

0xmichalis · 2016-07-04T09:55:30Z

Deployment controller changes LGTM

gmarek · 2016-07-04T09:56:48Z

@mikedanese - can you take a look at DaemonController changes?

gmarek · 2016-07-07T08:12:40Z

Ping @mikedanese

mikedanese · 2016-07-11T14:34:29Z

DaemonsetController change LGTM

gmarek · 2016-07-11T14:49:01Z

Squashed commit. @caesarxuchao - can you apply the final LGTM?

caesarxuchao · 2016-07-11T17:17:16Z

pkg/controller/daemon/daemoncontroller_test.go

@@ -289,6 +289,24 @@ func TestSufficentCapacityNodeDaemonLaunchesPod(t *testing.T) {
 	syncAndValidateDaemonSets(t, manager, ds, podControl, 1, 0)
 }

+// DaemonSets should place onto nodes with sufficient free resource


Could you update the comment? Or just remove it, the test name has enough information.

Yikes. Thanks for noticing.

caesarxuchao · 2016-07-11T17:22:02Z

Just a nit, otherwise LGTM.

gmarek · 2016-07-12T07:43:27Z

Done. @caesarxuchao PTAL

k8s-bot · 2016-07-12T08:28:33Z

GCE e2e build/test passed for commit 95de5a3.

caesarxuchao · 2016-07-12T17:03:01Z

LGTM. Thanks!!

k8s-github-robot · 2016-07-12T17:52:27Z

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

k8s-bot · 2016-07-12T19:36:40Z

GCE e2e build/test passed for commit 95de5a3.

k8s-github-robot · 2016-07-12T19:37:09Z

Automatic merge from submit-queue

gmarek added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 15, 2016

gmarek self-assigned this Jun 15, 2016

googlebot added the cla: yes label Jun 15, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jun 15, 2016

caesarxuchao reviewed Jun 15, 2016
View reviewed changes

gmarek force-pushed the controllerDeletion branch from a30a731 to 0b4310b Compare June 17, 2016 11:47

gmarek changed the title ~~DaemonController doesn't take any actions when being deleted.~~ Controllers doesn't take any actions when being deleted. Jun 17, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 17, 2016

ss7pro mentioned this pull request Jun 17, 2016

Petset pods are not terminated where petset is removed. #27621

Closed

0xmichalis reviewed Jun 20, 2016
View reviewed changes

gmarek removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 23, 2016

gmarek assigned caesarxuchao and unassigned gmarek Jun 23, 2016

pwittrock self-assigned this Jun 23, 2016

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2016

gmarek force-pushed the controllerDeletion branch from 66fec1d to a382c43 Compare July 4, 2016 09:16

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2016

gmarek force-pushed the controllerDeletion branch from a382c43 to e1f6b08 Compare July 7, 2016 08:03

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2016

gmarek assigned mikedanese Jul 7, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2016

gmarek force-pushed the controllerDeletion branch from e1f6b08 to 96f1827 Compare July 11, 2016 12:44

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2016

gmarek force-pushed the controllerDeletion branch from 96f1827 to 0ccba0f Compare July 11, 2016 14:48

caesarxuchao reviewed Jul 11, 2016
View reviewed changes

gmarek added 3 commits July 12, 2016 09:43

DaemonController doesn't take any actions when being deleted.

d895825

DeploymentController doesn't take any actions when being deleted

7815a3e

JobController doesn't take any actions when being deleted.

95de5a3

gmarek force-pushed the controllerDeletion branch from 0ccba0f to 95de5a3 Compare July 12, 2016 07:43

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2016

k8s-github-robot merged commit d90cc90 into kubernetes:master Jul 12, 2016

gmarek deleted the controllerDeletion branch August 30, 2016 09:48

Controllers doesn't take any actions when being deleted. #27438

Controllers doesn't take any actions when being deleted. #27438

Conversation

gmarek commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Jun 17, 2016 • edited Loading

caesarxuchao commented Jun 17, 2016

soltysh commented Jun 20, 2016

soltysh commented Jun 20, 2016

0xmichalis commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Jun 20, 2016

0xmichalis commented Jun 20, 2016

gmarek commented Jun 20, 2016

gmarek commented Jun 20, 2016

0xmichalis commented Jun 20, 2016 • edited Loading

gmarek commented Jun 23, 2016

pwittrock commented Jun 23, 2016

caesarxuchao commented Jun 23, 2016

0xmichalis commented Jun 24, 2016

0xmichalis commented Jul 4, 2016

gmarek commented Jul 4, 2016

gmarek commented Jul 7, 2016

mikedanese commented Jul 11, 2016

gmarek commented Jul 11, 2016

caesarxuchao Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Jul 11, 2016

gmarek commented Jul 12, 2016

k8s-bot commented Jul 12, 2016

caesarxuchao commented Jul 12, 2016

k8s-github-robot commented Jul 12, 2016

k8s-bot commented Jul 12, 2016

k8s-github-robot commented Jul 12, 2016

gmarek commented Jun 17, 2016 •

edited

Loading

0xmichalis commented Jun 20, 2016 •

edited

Loading

caesarxuchao Jul 11, 2016 •

edited

Loading