Fix cascading delete #48138

FengyunPan · 2017-06-27T14:06:48Z

Kubernetes 1.6 adds a PropagationPolicy to the delete options,
obsoleting the previous OrphanDependents boolean. This PR sets
PropagationPolicy=foreground instead of OrphanDependents=false
when federation controller manager delete cluster resource.

@caesarxuchao ptal

Release note:

NONE

k8s-ci-robot · 2017-06-27T14:06:55Z

Hi @FengyunPan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-06-27T14:06:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: FengyunPan
We suggest the following additional approver: madhusudancs

Assign the PR to them by writing /assign @madhusudancs in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

federation/OWNERS
test/integration/federation/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

madhusudancs · 2017-06-27T19:21:35Z

/unassign
/assign @nikhiljindal

caesarxuchao · 2017-06-27T22:07:12Z

/assign @nikhiljindal @caesarxuchao

caesarxuchao · 2017-06-28T17:55:36Z

Sorry, i'll take a look after release 1.7 ships.

colhom · 2017-06-28T23:52:04Z

federation/pkg/federatedtypes/crudtester/crudtester.go

 	if err != nil {
 		c.tl.Fatalf("Error deleting federated %s %q: %v", c.kind, namespacedName, err)
 	}

-	deletingInCluster := (orphanDependents != nil && *orphanDependents == false)
+	deletingInCluster := defaultPropagationPolicy != nil && *defaultPropagationPolicy == "Foreground"


.. && *defaultPropagationPolicy == metav1.DeletePropagationForeground

Done, thank you.

spiffxp · 2017-06-30T18:20:58Z

/ok-to-test

caesarxuchao · 2017-07-05T20:40:03Z

federation/pkg/federation-controller/deployment/deploymentcontroller.go

@@ -186,8 +186,10 @@ func NewDeploymentController(federationClient fedclientset.Interface) *Deploymen
 		},
 		func(client kubeclientset.Interface, obj runtime.Object) error {
 			rs := obj.(*extensionsv1.Deployment)
-			orphanDependents := false
-			err := client.Extensions().Deployments(rs.Namespace).Delete(rs.Name, &metav1.DeleteOptions{OrphanDependents: &orphanDependents})
+			defaultPropagationPolicy := metav1.DeletePropagationForeground


Maybe s/defaultPropagationPolicy/policy? I don't see why it's called "default".

@nikhiljindal is the federation control plane going to support both "foreground" and "background" gc? ~~If so, an option should be passed into this function.~~

Actually if the federation control plane is going to support "background" GC, it could still deletes underlying resources with "ForegroundPropagtionPolicy", it only needs to remove the federation resource immediately. That's a separate issue anyway.

orphanDependents := false
client.Extensions().Deployments(rs.Namespace).Delete(rs.Name, metav1.DeleteOptions{OrphanDependents: &orphanDependents})
Actually, the code can't cascade the deletion of deployment's pods. I will check it later.

caesarxuchao · 2017-07-05T20:50:38Z

Federation controllerManager just removes the federation resource
and orphans the corresponding resources when orphanDependents is
false and PropagationPolicy is nil,

@FengyunPan i don't think this claim is true. The underlying objects are deleted with orphan=false, so they and their dependents will be garbage collected, in the background.

That said, i think using DeletePropagationForeground when deleting the underlying resources is correct, because the federation control plane intends to support foreground GC. @nikhiljindal could you confirm?

FengyunPan · 2017-07-06T14:05:45Z

The 'kubectl delete deployment xxx' work fine, it called "m.RESTClient.Delete().
NamespaceIfScoped(namespace,m.NamespaceScoped).Resource(m.Resource).Name(name).Body(options).Do().Error()",
but "c.client.Delete().Namespace(c.ns).Resource("deployments"). VersionedParams(&listOptions, heme.ParameterCodec). Body(options). Do().Error()" can't work.
I don't know the relationship between OrphanDependents and PropagationPolicy, is there any doc about it?

caesarxuchao · 2017-07-06T18:27:18Z

There should be comment on the types.

The failed cascading deletion was fixed by #44058.

FengyunPan · 2017-07-12T12:18:29Z

@caesarxuchao Thank you, I have update the comment and PR.
The fedaration deployment has been merged into 'sync/controller.go'. When user delete fed-deployment, adapter.ClusterDelete() will be called.
@nikhiljindal PTAL.

caesarxuchao · 2017-07-12T18:48:50Z

test/integration/federation/crud_test.go

-	orphanedDependents := true
-	testCases := map[string]*bool{
-		"Resource should not be deleted from underlying clusters when OrphanDependents is true": &orphanedDependents,
-		"Resource should not be deleted from underlying clusters when OrphanDependents is nil":  nil,


Why do you remove this case?

Oops, sorry, after reading the federation code again, I find that the federation apiserver use DeletionOptions.OrphanDependents to delete federation-resource and cluster-resource, so update the case until federation-apiserver don't use OrphanDependents.

caesarxuchao · 2017-07-12T18:49:23Z

One nit, otherwise lgtm.

nikhiljindal · 2017-07-18T21:04:09Z

federation/pkg/federatedtypes/crudtester/crudtester.go

-		// May need extra time to delete both federation and cluster resources
-		waitTimeout = c.clusterWaitTimeout
-	}
+	waitTimeout := ForeverTimeout


Pass the right clusterWaitTimeout in

kubernetes/test/integration/federation/framework/crudtester.go

Line 46 in 50ec438

return crudtester.NewFederatedTypeCRUDTester(logger, adapter, clusterClients, DefaultWaitInterval, wait.ForeverTestTimeout)

rather than overwriting it here.

Oops, I will update it.

nikhiljindal · 2017-07-18T21:04:31Z

federation/pkg/federatedtypes/crudtester/crudtester.go

-	}
+	waitTimeout := ForeverTimeout
+	//if deletingInCluster {
+	//	// May need extra time to delete both federation and cluster resources


Delete unused code

nikhiljindal · 2017-07-18T21:05:03Z

federation/pkg/federatedtypes/crudtester/crudtester.go

@@ -155,7 +160,7 @@ func (c *FederatedTypeCRUDTester) CheckDelete(obj pkgruntime.Object, orphanDepen
 		return false, err
 	})
 	if err != nil {
-		c.tl.Fatalf("Error deleting federated %s %q: %v", c.kind, qualifiedName, err)
+		c.tl.Fatalf("Error deleting federated %s %q in %s: %v", c.kind, qualifiedName, waitTimeout, err)


%s seconds?

The waitTimeout contains unit.

nikhiljindal · 2017-07-18T21:16:35Z

federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go

-// and deletion helper does a cascading deletion.
+// If user deletes the resource with nil DeleteOptions or DeletionOptions.OrphanDependents = true,
+// then the federation controllerManager just delete fed-resource.
+// if user deletes the resource with DeletionOptions.OrphanDependents = true,


It should be OrphanDependents = false here instead of true?

nikhiljindal · 2017-07-18T21:17:21Z

federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go

-// DeletionOptions.OrphanDependents = true then the apiserver removes the orphan finalizer
-// and deletion helper does a cascading deletion.
+// If user deletes the resource with nil DeleteOptions or DeletionOptions.OrphanDependents = true,
+// then the federation controllerManager just delete fed-resource.


controllerManager -> controller manager.
delete -> deletes
fed-resource -> federation resource

Thank you for fixing it

nikhiljindal · 2017-07-18T21:17:32Z

federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go

+// then the federation controllerManager just delete fed-resource.
+// if user deletes the resource with DeletionOptions.OrphanDependents = true,
+// then the federation apiserver removes the orphan finalizer and deletion helper
+// does a cascading deletion(delete fed-resource and delete cluster-resource).


does a cascading deletion (deletes federation resource and cluster resources).

FengyunPan · 2017-07-19T05:29:06Z

@nikhiljindal Deleting federation deployment and cluster deployment(contains rs and pod) is my PR's purpose. Actrually, after deleting federation deployment with OrphanDependents = false, the pods of federation deployment was left behind. Is it need to add Foreground PropagationPolicy into DeleteOptions?

nikhiljindal · 2017-07-19T05:51:53Z

aah thanks for explaining that.
Please update the PR description to say that you are fixing that bug. Please update the PR description as well.

I looked at the code again and we are only changing the DELETE request that is sent to clusters, we are now setting PropagationPolicy to Foreground. This is not changing the DELETE request that is sent to federation apiserver by federation controller, hence my comment above is not valid.

This looks good to me. Will add lgtm label, once the open comments have been fixed.
Thanks!

FengyunPan · 2017-07-19T09:03:38Z

/test pull-kubernetes-unit

FengyunPan · 2017-07-20T05:28:07Z

/retest

FengyunPan · 2017-07-20T12:59:53Z

@nikhiljindal Hi, can you help me? I just use PropagationPolicy for deleting cluster resource and didn't update the deletion of federation resource. Why the test case times out?

nikhiljindal · 2017-08-21T19:45:51Z

Which test case is timing out?

Before this change, deletion of dependent resources in underlying clusters was happening in background. For ex: while deleting federated deployment, federated controller was deleting deployment from underlying cluster such that it does not wait for replicasets and pods to be deleted (they were being deleted later in the background, without blocking deployment deletion). With this change, deployment deletion in underlying cluster will wait for replicasets and pods deletion. So it is expected to take some more time than before.

FengyunPan · 2017-08-22T02:10:40Z

@nikhiljindal This case is timing out: k8s.io/kubernetes/test/integration/federation -run TestFederationCRUD

I change more time for this case later.

After deleting federation deployment with OrphanDependents=false, the pods of federation deployment was left behind. Kubernetes 1.6 adds a PropagationPolicy to the delete options, obsoleting the previous OrphanDependents boolean. This PR sets PropagationPolicy=foreground instead of OrphanDependents=false when federation controller manager delete cluster resource.

FengyunPan · 2017-08-22T04:47:05Z

/test pull-kubernetes-bazel
/test pull-kubernetes-unit
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce-etcd3

FengyunPan · 2017-08-24T13:05:56Z

/retest

k8s-ci-robot · 2017-08-24T13:52:38Z

@FengyunPan: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`3c3d638`	link	`/test pull-kubernetes-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-09-28T08:09:41Z

@FengyunPan PR needs rebase

k8s-github-robot · 2017-11-22T23:29:25Z

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @FengyunPan @caesarxuchao @colhom @nikhiljindal

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

k8s-ci-robot · 2017-11-22T23:29:26Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 27, 2017

k8s-github-robot assigned colhom and madhusudancs Jun 27, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 27, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 27, 2017

k8s-ci-robot assigned nikhiljindal and unassigned madhusudancs Jun 27, 2017

k8s-ci-robot assigned caesarxuchao Jun 27, 2017

colhom suggested changes Jun 28, 2017

View reviewed changes

FengyunPan force-pushed the cascade-delete branch from cee5f16 to 78ed5d5 Compare June 29, 2017 01:23

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 30, 2017

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

caesarxuchao reviewed Jul 5, 2017

View reviewed changes

FengyunPan force-pushed the cascade-delete branch from 78ed5d5 to eed5b96 Compare July 12, 2017 12:12

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

caesarxuchao reviewed Jul 12, 2017

View reviewed changes

FengyunPan force-pushed the cascade-delete branch 2 times, most recently from 0a7176f to 3260752 Compare July 13, 2017 11:47

nikhiljindal reviewed Jul 18, 2017

View reviewed changes

FengyunPan force-pushed the cascade-delete branch from 8e958ef to 2f8bd9e Compare July 19, 2017 06:29

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

FengyunPan force-pushed the cascade-delete branch from 2f8bd9e to b763678 Compare July 19, 2017 06:44

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

FengyunPan force-pushed the cascade-delete branch 3 times, most recently from 3913acb to 5a98fbc Compare July 19, 2017 08:24

FengyunPan force-pushed the cascade-delete branch from 5a98fbc to 1fffdee Compare July 20, 2017 11:50

FengyunPan force-pushed the cascade-delete branch 2 times, most recently from 10c4242 to c537d32 Compare August 22, 2017 02:45

FengyunPan force-pushed the cascade-delete branch from c537d32 to 3c3d638 Compare August 22, 2017 03:45

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

k8s-github-robot closed this Nov 22, 2017

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 22, 2017

Fix cascading delete #48138

Fix cascading delete #48138

Conversation

FengyunPan commented Jun 27, 2017 • edited by k8s-github-robot Loading

k8s-ci-robot commented Jun 27, 2017

k8s-github-robot commented Jun 27, 2017

madhusudancs commented Jun 27, 2017

caesarxuchao commented Jun 27, 2017

caesarxuchao commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffxp commented Jun 30, 2017

Choose a reason for hiding this comment

caesarxuchao Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Jul 5, 2017

FengyunPan commented Jul 6, 2017 • edited Loading

caesarxuchao commented Jul 6, 2017

FengyunPan commented Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

FengyunPan Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

caesarxuchao commented Jul 12, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FengyunPan commented Jul 19, 2017

nikhiljindal commented Jul 19, 2017

FengyunPan commented Jul 19, 2017

FengyunPan commented Jul 20, 2017

FengyunPan commented Jul 20, 2017

nikhiljindal commented Aug 21, 2017

FengyunPan commented Aug 22, 2017

FengyunPan commented Aug 22, 2017

FengyunPan commented Aug 24, 2017

k8s-ci-robot commented Aug 24, 2017 • edited Loading

k8s-github-robot commented Sep 28, 2017

k8s-github-robot commented Nov 22, 2017

k8s-ci-robot commented Nov 22, 2017

FengyunPan commented Jun 27, 2017 •

edited by k8s-github-robot

Loading

caesarxuchao Jul 5, 2017 •

edited

Loading

FengyunPan commented Jul 6, 2017 •

edited

Loading

FengyunPan commented Jul 12, 2017 •

edited

Loading

FengyunPan Jul 13, 2017 •

edited

Loading

k8s-ci-robot commented Aug 24, 2017 •

edited

Loading