Adding cascading deletion support to federated namespaces #34648

nikhiljindal · 2016-10-12T20:57:04Z

With this change, whenever a federated namespace is deleted with DeleteOptions.OrphanDependents = false, then federation namespace controller first deletes the corresponding namespaces from all underlying clusters before deleting the federated namespace.

cc @kubernetes/sig-cluster-federation @caesarxuchao

Adding support for DeleteOptions.OrphanDependents for federated namespaces. Setting it to false while deleting a federated namespace also deletes the corresponding namespace from all registered clusters.

This change is

nikhiljindal · 2016-10-12T20:58:01Z

I am working on updating our e2e test

nikhiljindal · 2016-10-13T01:43:43Z

Updated the PR to include e2e changes. Verified that test/e2e/federated-namespace passes

caesarxuchao

Sorry, I'm not familiar with the federation code base. I asked two question, they might be silly. I'll take another pass tomorrow.

caesarxuchao · 2016-10-13T03:50:45Z

federation/pkg/federation-controller/namespace/namespace_controller.go

+		// removeFinalizerFunc
+		func(obj runtime.Object, finalizer string) (runtime.Object, error) {
+			namespace := obj.(*api_v1.Namespace)
+			finalizerSet := sets.NewString()


I don't get why you need to make the Set. Why not just make []api_v1.FinalizerName directly?

caesarxuchao · 2016-10-13T04:15:24Z

federation/pkg/federation-controller/namespace/namespace_controller.go

@@ -371,6 +416,13 @@ func (nc *NamespaceController) delete(namespace *api_v1.Namespace) error {
 		return fmt.Errorf("failed to delete events list from namespace: %v", err)
 	}

+	// Delete the namespace from all underlying clusters.
+	updatedNamespaceObj, err := nc.deletionHelper.ProcessDeletion(updatedNamespace)


I thought ProcessDeletion should take a fed_v1.Namespace, why is it taking a regular namespace?

There is no special fed_v1.Namespace. We use the same v1.Namespace object in federation

nikhiljindal · 2016-10-13T18:10:43Z

Updated the code as per comments. Ready for another look

caesarxuchao · 2016-10-13T20:53:53Z

federation/pkg/federation-controller/namespace/namespace_controller.go

+		return err
+	}
+
+	// Remove kube_api.FinalizerKubernetes


The federated namespace is deleted before the underlying cluster namespaces are deleted from etcd, right? This is consistent with the mode of the garbage collector, i.e., asynchronous, but different from deletion of a cluster namespace, which is synchronous. I'm not sure if this is what you want.

No. federated namespace will not be deleted unless the namespaces have been deleted in all underlying clusters.

Where is it blocked? It seems the UpdateOnError is not blocking.

Yes it was blocked until the Delete request succeeded for all clusters but it wasnt waiting for all resources to go away. Updated the code to wait.

nikhiljindal · 2016-10-14T21:04:07Z

Updated the controller to add a DeleteFederatedDependents finalizer before creating any dependent resources in sub clusters. This now ensures that the federation resource is not deleted unless all dependents have been removed.
PTAL.

nikhiljindal · 2016-10-19T22:07:12Z

Updated the code as discussed. I am now creating the finalizer in ObjectMeta.Finalizers instead of putting it in NamespaceSpec.Finalizers.
Verified that federated-namespace e2e test passes :)

nikhiljindal · 2016-10-20T06:40:24Z

Retrying unit test
@k8s-bot unit test this

nikhiljindal · 2016-10-20T09:14:29Z

Retrying unit test
@k8s-bot unit test this

caesarxuchao · 2016-10-21T03:42:29Z

federation/pkg/federation-controller/namespace/namespace_controller.go

@@ -255,6 +350,21 @@ func (nc *NamespaceController) reconcileNamespace(namespace string) {
 		return
 	}

+	glog.V(3).Infof("Ensuring delete dependents finalizer for namespace: %s", baseNamespace.Name)
+	// Add the DeleteFederatedDependents finalizer before creating a namespace in


Just ideas: DeleteUnderlyingClusterObjects, DeleteDependentsInUnderlyingClusters. Not sure if DeleteFederatedDependents is intuitive. Your call.

Renamed to DeleteFromUnderlyingClusters

caesarxuchao · 2016-10-21T03:49:38Z

federation/pkg/federation-controller/namespace/namespace_controller.go

+	}
+
+	updatedNamespace = updatedNamespaceObj.(*api_v1.Namespace)
+	err = nc.federatedApiClient.Core().Namespaces().Delete(updatedNamespace.Name, &api_v1.DeleteOptions{})


Do you need to pass an empty deleteOptions? Can it be nil?

Yes nil should work as well. Will try :)

caesarxuchao · 2016-10-21T03:55:58Z

federation/pkg/federation-controller/namespace/namespace_controller.go

-	// Right now there is just 5 types of objects: ReplicaSet, Secret, Ingress, Events and Service.
+	if nc.hasFinalizerFuncInSpec(updatedNamespace, api_v1.FinalizerKubernetes) {
+		// Delete resources in this namespace.
+		updatedNamespace, err = nc.removeKubernetesFinalizer(updatedNamespace)


This function is deleteNamespaceResourcesAndRemoveNamespaceSpecFinalizer :) Maybe split it to two functions? "KubernetesFinalizer" is not clear.

Yes, I can rename it to deleteNamespaceResourcesAndRemoveNamespaceSpecFinalizer but thats a bit long ;)
I think the current name is fine (its a private func).
I dont want to change that code much.
There is a TODO to make it consistent with the kubernetes code. We should be able to reuse the same code in kubernetes and federation to delete namespace resources.

Ok. I still think "NamespaceSpecFinalizer" is more clear than "KubernetesFinalizer". Your call, as it's a private func.

There is a TODO to make it consistent with the kubernetes code. We should be able to reuse the same code in kubernetes and federation to delete namespace resources.

Not sure I follow this part. What kubernetes code are you referring to?

To clarify, I named it removeKubernetesFinalizer because it removes the finalizer called FinalizerKubernetes.

Not sure I follow this part. What kubernetes code are you referring to?

kubernetes namespace controller also has the same code that deletes all resources in a namespace before removing the finalizer - same as what this function does.

To clarify, I named it removeKubernetesFinalizer because it removes the finalizer called FinalizerKubernetes.

I think FinalizerKubernetes is a bad name :)

Yes now it is. It is clear now that we should have been more specific. I guess the thinking at that time was that this is the finalizer that kubernetes is adding to do its work. Now we have so many so its better to be specific.

caesarxuchao · 2016-10-21T03:57:28Z

federation/pkg/federation-controller/namespace/namespace_controller_test.go

@@ -108,11 +114,19 @@ func TestNamespaceController(t *testing.T) {

 	// Test add federated namespace.
 	namespaceWatch.Add(&ns1)
+	// Verify that the DeleteFederatedDependents finalizer is added to the namespace.
+	// Note: finalize invokes the create action in Fake client.


That does sound like a bug? Could you open an issue?

Fixing in #35478

caesarxuchao · 2016-10-21T04:03:52Z

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

+	}
+	if len(operations) > 0 {
+		// We have deleted a bunch of resources.
+		// Wait for the store to observe all the deletions.


Where is the "wait" is implemented?

Wait is implemented by not returning success here.
Since len(operations) > 0 we return here without removing the finalizer. Next time this function is called, we will again first compute if we want to do any operations in underlying clusters. If at any time, len(operation) = 0, then we proceed. Else we wait till len(operations) == 0.
Caller is expected to keep calling HandleObjectInUnderlyingClusters until it returns success (err == nil).

I see. I didn't notice you called deliverNamespace to process the namespace again in the future. So it looks good.

caesarxuchao · 2016-10-21T04:06:22Z

test/e2e/federated-namespace.go

@@ -36,12 +36,13 @@ const (
 )

 // Create/delete ingress api objects
-var _ = framework.KubeDescribe("Federation namespace [Feature:Federation]", func() {
+var _ = framework.KubeDescribe("Federation namespace [Feature:Federation12]", func() {


What is Federation12?

Will fix before committing.

caesarxuchao · 2016-10-21T04:07:33Z

test/e2e/framework/util.go

@@ -2210,6 +2210,7 @@ func DumpEventsInNamespace(eventsLister EventsLister, namespace string) {
 	events, err := eventsLister(v1.ListOptions{}, namespace)
 	Expect(err).NotTo(HaveOccurred())

+	By(fmt.Sprintf("Found %d events.", len(events.Items)))


Do you want to check in this code?

Yes helps in debugging.

caesarxuchao · 2016-10-21T04:10:28Z

federation/pkg/federation-controller/namespace/namespace_controller.go

+		return obj, nil
+	}
+	namespace.ObjectMeta.Finalizers = newFinalizers
+	namespace, err := nc.federatedApiClient.Core().Namespaces().Finalize(namespace)


Why not just call Update? Calling Finalize looks like it's delete the namespace.spec.finalizer

Why?
My expectation is that if only the list of finalizers are being modified, then we should use Finalize(). Else use update().
Is that not correct?

No. Finalize is a special method that only exists in the namespace client. So I think we should call Finalize() only when we update the namespace.spec.finalizer.

oh ok. Hadnt realized the spec.Finalizer and ObjectMeta.Finalizer difference. You are right. Will update

caesarxuchao · 2016-10-21T04:12:56Z

federation/pkg/federation-controller/namespace/namespace_controller.go

-		}
+	// Remove kube_api.FinalizerKubernetes
+	if len(namespace.Spec.Finalizers) != 0 {
+		return nc.removeFinalizerFromSpec(namespace, api_v1.FinalizerKubernetes)


I think you should check if all resources are actually deleted to make the deletion synchronous. Cluster namespace deletion will wait for all resources are deleted. Perhaps this will be easier to implement after you apply the deleteHelper pattern to other resources?

I think you should check if all resources are actually deleted to make the deletion synchronous.

Yes this is not required right now since all deletions are immediate (there is no cascading deletion). Will need to update this before enabling cascading deletion for any of these resources.
The plan is to refactor kubernetes namespace controller code to make the namespace deletion code reusable and then use it here.
Leaving as is in this PR.

Ok. Thanks for the explanation.

caesarxuchao · 2016-10-21T04:14:53Z

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

+)
+
+const (
+	FinalizerDeleteFederatedDependents string = "federation.kubernetes.io/delete-federated-dependents"


Add a comment? Also mention the OrphanFinalizer will take precedence.

nikhiljindal · 2016-10-25T20:20:44Z

Verified that federation namespace e2e test is passing

nikhiljindal · 2016-10-25T22:11:16Z

Replaced Finalize() by Update() as per the comment above and squashed commits.
PTAL.

caesarxuchao · 2016-10-25T22:23:18Z

LGTM. Thanks @nikhiljindal !

k8s-ci-robot · 2016-10-25T22:35:14Z

Jenkins verification failed for commit b95fac161531a5df07b034d52047b2351b0bf5f9. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

nikhiljindal · 2016-10-26T00:32:37Z

Trying to satisfy bazel and fix unit tests

k8s-ci-robot · 2016-10-26T23:53:00Z

Jenkins GCE e2e failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-26T23:53:01Z

Jenkins Kubemark GCE e2e failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-26T23:53:01Z

Jenkins GKE smoke e2e failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-26T23:53:01Z

Jenkins GCI GKE smoke e2e failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-26T23:53:01Z

Jenkins GCI GCE e2e failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-26T23:53:12Z

Jenkins GCE etcd3 e2e failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-26T23:54:01Z

Jenkins unit/integration failed for commit ebbbd02abc6ef6b4da535e39fc1493444771cb7d. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

nikhiljindal · 2016-10-27T02:29:17Z

All tests passed. Adding back LGTM

k8s-github-robot · 2016-10-27T03:18:44Z

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

k8s-github-robot · 2016-10-27T04:04:03Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 12, 2016

nikhiljindal added the area/cluster-federation label Oct 12, 2016

nikhiljindal assigned caesarxuchao Oct 12, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 12, 2016

nikhiljindal force-pushed the NSCasDel branch from 88c9e29 to 71397d2 Compare October 13, 2016 01:42

nikhiljindal added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 13, 2016

caesarxuchao reviewed Oct 13, 2016

View reviewed changes

nikhiljindal force-pushed the NSCasDel branch from fcbc2c6 to 46c082d Compare October 19, 2016 21:59

nikhiljindal added this to the v1.5 milestone Oct 19, 2016

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 20, 2016

nikhiljindal force-pushed the NSCasDel branch from 84dba9d to 3ee605d Compare October 20, 2016 18:23

caesarxuchao suggested changes Oct 21, 2016

View reviewed changes

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 25, 2016

nikhiljindal mentioned this pull request Oct 25, 2016

Fixing finalize and status in fake_namespace testclient #35478

Closed

nikhiljindal force-pushed the NSCasDel branch 2 times, most recently from c286004 to b95fac1 Compare October 25, 2016 22:10

caesarxuchao approved these changes Oct 25, 2016

View reviewed changes

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

nikhiljindal force-pushed the NSCasDel branch from b95fac1 to ae376a3 Compare October 26, 2016 00:14

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2016

nikhiljindal force-pushed the NSCasDel branch from ae376a3 to ebbbd02 Compare October 26, 2016 23:48

nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2016

nikhiljindal removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2016

Adding cascading deletion support to federated namespaces

f955d55

nikhiljindal force-pushed the NSCasDel branch from ebbbd02 to f955d55 Compare October 26, 2016 23:54

nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2016

k8s-github-robot merged commit dcdbf27 into kubernetes:master Oct 27, 2016

Adding cascading deletion support to federated namespaces #34648

Adding cascading deletion support to federated namespaces #34648

Conversation

nikhiljindal commented Oct 12, 2016 • edited Loading

nikhiljindal commented Oct 12, 2016

nikhiljindal commented Oct 13, 2016

caesarxuchao left a 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

nikhiljindal commented Oct 13, 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

nikhiljindal commented Oct 14, 2016

nikhiljindal commented Oct 19, 2016

nikhiljindal commented Oct 20, 2016

nikhiljindal commented Oct 20, 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

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

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

nikhiljindal commented Oct 25, 2016

nikhiljindal commented Oct 25, 2016

caesarxuchao commented Oct 25, 2016

k8s-ci-robot commented Oct 25, 2016

nikhiljindal commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

k8s-ci-robot commented Oct 26, 2016

nikhiljindal commented Oct 27, 2016

k8s-github-robot commented Oct 27, 2016

k8s-github-robot commented Oct 27, 2016

nikhiljindal commented Oct 12, 2016 •

edited

Loading