Merge pull request #36283 from nikhiljindal/nscascdelTests

Automatic merge from submit-queue

Adding more e2e tests for federated namespace cascading deletion and fixing bugs

Ref https://github.com/kubernetes/kubernetes/issues/33612

Adding more e2e tests for testing cascading deletion of federated namespace.
New tests are now verifying that cascading deletion happen when DeletionOptions.OrphanDependents=false and it does not happen when DeleteOptions.OrphanDependents=true.

Also updated deletion helper to always add OrphanFinalizer. generic registry will remove it if DeleteOptions.OrphanDependents=false. Also updated namespace registry to do the same.

We need to add the orphan finalizer to keep the orphan by default behavior. We assume that its dependents are going to be orphaned and hence add that finalizer. If user does not want the orphan behavior, he can do so using DeleteOptions and then the registry will remove that finalizer.

cc @kubernetes/sig-cluster-federation @caesarxuchao @derekwaynecarr

federation/cmd/federation-apiserver/app/server.go

@@ -210,8 +210,10 @@ func Run(s *options.ServerRunOptions) error {
 	routes.UIRedirect{}.Install(m.HandlerContainer)
 	routes.Logs{}.Install(m.HandlerContainer)
 
+	// TODO: Refactor this code to share it with kube-apiserver rather than duplicating it here.
 	restOptionsFactory := restOptionsFactory{
 		storageFactory:          storageFactory,
+		enableGarbageCollection: s.GenericServerRunOptions.EnableGarbageCollection,
 		deleteCollectionWorkers: s.GenericServerRunOptions.DeleteCollectionWorkers,
 	}
 	if s.GenericServerRunOptions.EnableWatchCache {
@@ -233,6 +235,7 @@ type restOptionsFactory struct {
 	storageFactory          genericapiserver.StorageFactory
 	storageDecorator        generic.StorageDecorator
 	deleteCollectionWorkers int
+	enableGarbageCollection bool
 }
 
 func (f restOptionsFactory) NewFor(resource unversioned.GroupResource) generic.RESTOptions {
@@ -244,6 +247,7 @@ func (f restOptionsFactory) NewFor(resource unversioned.GroupResource) generic.R
 		StorageConfig:           config,
 		Decorator:               f.storageDecorator,
 		DeleteCollectionWorkers: f.deleteCollectionWorkers,
+		EnableGarbageCollection: f.enableGarbageCollection,
 		ResourcePrefix:          f.storageFactory.ResourcePrefix(resource),
 	}
 }

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

@@ -220,7 +220,7 @@ func (fdc *DeploymentController) Run(workers int, stopCh <-chan struct{}) {
 	// Wait until the cluster is synced to prevent the update storm at the very beginning.
 	for !fdc.isSynced() {
 		time.Sleep(5 * time.Millisecond)
-		glog.Infof("Waiting for controller to sync up")
+		glog.V(3).Infof("Waiting for controller to sync up")
 	}
 
 	for i := 0; i < workers; i++ {

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

@@ -352,11 +352,11 @@ func (nc *NamespaceController) reconcileNamespace(namespace string) {
 
 	glog.V(3).Infof("Ensuring delete object from underlying clusters finalizer for namespace: %s",
 		baseNamespace.Name)
-	// Add the DeleteFromUnderlyingClusters finalizer before creating a namespace in
+	// Add the required finalizers before creating a namespace in
 	// underlying clusters.
 	// This ensures that the dependent namespaces are deleted in underlying
 	// clusters when the federated namespace is deleted.
-	updatedNamespaceObj, err := nc.deletionHelper.EnsureDeleteFromUnderlyingClustersFinalizer(baseNamespace)
+	updatedNamespaceObj, err := nc.deletionHelper.EnsureFinalizers(baseNamespace)
 	if err != nil {
 		glog.Errorf("Failed to ensure delete object from underlying clusters finalizer in namespace %s: %v",
 			baseNamespace.Name, err)
@@ -446,6 +446,7 @@ func (nc *NamespaceController) delete(namespace *api_v1.Namespace) error {
 	}
 	var err error
 	if namespace.Status.Phase != api_v1.NamespaceTerminating {
+		glog.V(2).Infof("Marking ns %s as terminating", namespace.Name)
 		nc.eventRecorder.Event(namespace, api.EventTypeNormal, "DeleteNamespace", fmt.Sprintf("Marking for deletion"))
 		_, err = nc.federatedApiClient.Core().Namespaces().Update(updatedNamespace)
 		if err != nil {
@@ -459,6 +460,7 @@ func (nc *NamespaceController) delete(namespace *api_v1.Namespace) error {
 		if err != nil {
 			return fmt.Errorf("error in deleting resources in namespace %s: %v", namespace.Name, err)
 		}
+		glog.V(2).Infof("Removed kubernetes finalizer from ns %s", namespace.Name)
 	}
 
 	// Delete the namespace from all underlying clusters.

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

@@ -78,16 +78,32 @@ func NewDeletionHelper(
 	}
 }
 
-// Ensures that the given object has the required finalizer to ensure that
-// objects are deleted in underlying clusters when this object is deleted
-// from federation control plane.
+// Ensures that the given object has both FinalizerDeleteFromUnderlyingClusters
+// and FinalizerOrphan finalizers.
+// We do this so that the controller is always notified when a federation resource is deleted.
+// If user deletes the resource with nil DeleteOptions or
+// DeletionOptions.OrphanDependents = true then the apiserver removes the orphan finalizer
+// and deletion helper does a cascading deletion.
+// Otherwise, deletion helper just removes the federation resource and orphans
+// the corresponding resources in underlying clusters.
 // This method should be called before creating objects in underlying clusters.
-func (dh *DeletionHelper) EnsureDeleteFromUnderlyingClustersFinalizer(obj runtime.Object) (
+func (dh *DeletionHelper) EnsureFinalizers(obj runtime.Object) (
 	runtime.Object, error) {
-	if dh.hasFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters) {
-		return obj, nil
+	if !dh.hasFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters) {
+		glog.V(2).Infof("Adding finalizer %s to %s", FinalizerDeleteFromUnderlyingClusters, dh.objNameFunc(obj))
+		obj, err := dh.addFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters)
+		if err != nil {
+			return obj, err
+		}
 	}
-	return dh.addFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters)
+	if !dh.hasFinalizerFunc(obj, api_v1.FinalizerOrphan) {
+		glog.V(2).Infof("Adding finalizer %s to %s", api_v1.FinalizerOrphan, dh.objNameFunc(obj))
+		obj, err := dh.addFinalizerFunc(obj, api_v1.FinalizerOrphan)
+		if err != nil {
+			return obj, err
+		}
+	}
+	return obj, nil
 }
 
 // Deletes the resources corresponding to the given federated resource from

pkg/registry/core/namespace/etcd/etcd.go

@@ -153,6 +153,19 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions)
 				if existingNamespace.Status.Phase != api.NamespaceTerminating {
 					existingNamespace.Status.Phase = api.NamespaceTerminating
 				}
+
+				// Remove orphan finalizer if options.OrphanDependents = false.
+				if options.OrphanDependents != nil && *options.OrphanDependents == false {
+					// remove Orphan finalizer.
+					newFinalizers := []string{}
+					for i := range existingNamespace.ObjectMeta.Finalizers {
+						finalizer := existingNamespace.ObjectMeta.Finalizers[i]
+						if string(finalizer) != api.FinalizerOrphan {
+							newFinalizers = append(newFinalizers, finalizer)
+						}
+					}
+					existingNamespace.ObjectMeta.Finalizers = newFinalizers
+				}
 				return existingNamespace, nil
 			}),
 		)

test/e2e/BUILD

@@ -109,6 +109,7 @@ go_library(
     deps = [
         "//federation/apis/federation/v1beta1:go_default_library",
         "//federation/client/clientset_generated/federation_release_1_5:go_default_library",
+        "//federation/client/clientset_generated/federation_release_1_5/typed/core/v1:go_default_library",
         "//federation/pkg/federation-controller/util:go_default_library",
         "//pkg/api:go_default_library",
         "//pkg/api/annotations:go_default_library",

test/e2e/federated-namespace.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 	"time"
 
+	clientset "k8s.io/kubernetes/federation/client/clientset_generated/federation_release_1_5/typed/core/v1"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/errors"
 	api_v1 "k8s.io/kubernetes/pkg/api/v1"
@@ -33,6 +34,7 @@ import (
 
 const (
 	namespacePrefix = "e2e-namespace-test-"
+	eventNamePrefix = "e2e-namespace-test-event-"
 )
 
 // Create/delete ingress api objects
@@ -42,7 +44,6 @@ var _ = framework.KubeDescribe("Federation namespace [Feature:Federation]", func
 	Describe("Namespace objects", func() {
 		var federationName string
 		var clusters map[string]*cluster // All clusters, keyed by cluster name
-		var nsName string
 
 		BeforeEach(func() {
 			framework.SkipUnlessFederated(f.ClientSet)
@@ -58,11 +59,11 @@ var _ = framework.KubeDescribe("Federation namespace [Feature:Federation]", func
 
 		AfterEach(func() {
 			framework.SkipUnlessFederated(f.ClientSet)
-			deleteAllTestNamespaces(
+			deleteAllTestNamespaces(false,
 				f.FederationClientset_1_5.Core().Namespaces().List,
 				f.FederationClientset_1_5.Core().Namespaces().Delete)
 			for _, cluster := range clusters {
-				deleteAllTestNamespaces(
+				deleteAllTestNamespaces(false,
 					cluster.Core().Namespaces().List,
 					cluster.Core().Namespaces().Delete)
 			}
@@ -72,50 +73,119 @@ var _ = framework.KubeDescribe("Federation namespace [Feature:Federation]", func
 		It("should be created and deleted successfully", func() {
 			framework.SkipUnlessFederated(f.ClientSet)
 
-			ns := api_v1.Namespace{
-				ObjectMeta: api_v1.ObjectMeta{
-					Name: api.SimpleNameGenerator.GenerateName(namespacePrefix),
-				},
-			}
-			nsName = ns.Name
-			By(fmt.Sprintf("Creating namespace %s", ns.Name))
-			_, err := f.FederationClientset_1_5.Core().Namespaces().Create(&ns)
-			framework.ExpectNoError(err, "Failed to create namespace %s", ns.Name)
+			nsName := createNamespace(f.FederationClientset_1_5.Core().Namespaces())
 
-			// Check subclusters if the namespace was created there.
-			By(fmt.Sprintf("Waiting for namespace %s to be created in all underlying clusters", ns.Name))
-			err = wait.Poll(5*time.Second, 2*time.Minute, func() (bool, error) {
-				for _, cluster := range clusters {
-					_, err := cluster.Core().Namespaces().Get(ns.Name)
-					if err != nil && !errors.IsNotFound(err) {
-						return false, err
-					}
-					if err != nil {
-						return false, nil
-					}
-				}
-				return true, nil
-			})
-			framework.ExpectNoError(err, "Not all namespaces created")
-
-			By(fmt.Sprintf("Deleting namespace %s", ns.Name))
-			deleteAllTestNamespaces(
+			By(fmt.Sprintf("Deleting namespace %s", nsName))
+			deleteAllTestNamespaces(false,
 				f.FederationClientset_1_5.Core().Namespaces().List,
 				f.FederationClientset_1_5.Core().Namespaces().Delete)
-			By(fmt.Sprintf("Verifying that namespace %s was deleted from all underlying clusters", ns.Name))
-			// Verify that the namespace was deleted from all underlying clusters as well.
-			for clusterName, clusterClientset := range clusters {
-				_, err := clusterClientset.Core().Namespaces().Get(ns.Name)
-				if err == nil || !errors.IsNotFound(err) {
-					framework.Failf("expected NotFound error for namespace %s in cluster %s, got error: %v", ns.Name, clusterName, err)
-				}
+			By(fmt.Sprintf("Verified that deletion succeeded"))
+		})
+		It("should be deleted from underlying clusters when OrphanDependents is false", func() {
+			framework.SkipUnlessFederated(f.ClientSet)
+
+			verifyNsCascadingDeletion(f.FederationClientset_1_5.Core().Namespaces(), clusters, false)
+			By(fmt.Sprintf("Verified that namespaces were deleted from underlying clusters"))
+		})
+
+		It("should not be deleted from underlying clusters when OrphanDependents is true", func() {
+			framework.SkipUnlessFederated(f.ClientSet)
+
+			verifyNsCascadingDeletion(f.FederationClientset_1_5.Core().Namespaces(), clusters, true)
+			By(fmt.Sprintf("Verified that namespaces were not deleted from underlying clusters"))
+		})
+
+		It("all resources in the namespace should be deleted when namespace is deleted", func() {
+			framework.SkipUnlessFederated(f.ClientSet)
+
+			nsName := createNamespace(f.FederationClientset_1_5.Core().Namespaces())
+
+			// Create resources in the namespace.
+			event := api_v1.Event{
+				ObjectMeta: api_v1.ObjectMeta{
+					Name:      api.SimpleNameGenerator.GenerateName(eventNamePrefix),
+					Namespace: nsName,
+				},
+				InvolvedObject: api_v1.ObjectReference{
+					Kind:      "Pod",
+					Namespace: nsName,
+					Name:      "sample-pod",
+				},
+			}
+			By(fmt.Sprintf("Creating event %s in namespace %s", event.Name, nsName))
+			_, err := f.FederationClientset_1_5.Core().Events(nsName).Create(&event)
+			if err != nil {
+				framework.Failf("Failed to create event %v in namespace %s, err: %s", event, nsName, err)
+			}
+
+			By(fmt.Sprintf("Deleting namespace %s", nsName))
+			deleteAllTestNamespaces(false,
+				f.FederationClientset_1_5.Core().Namespaces().List,
+				f.FederationClientset_1_5.Core().Namespaces().Delete)
+
+			By(fmt.Sprintf("Verify that event %s was deleted as well", event.Name))
+			latestEvent, err := f.FederationClientset_1_5.Core().Events(nsName).Get(event.Name)
+			if !errors.IsNotFound(err) {
+				framework.Failf("Event %s should have been deleted. Found: %v", event.Name, latestEvent)
 			}
 			By(fmt.Sprintf("Verified that deletion succeeded"))
 		})
 	})
 })
 
-func deleteAllTestNamespaces(lister func(api_v1.ListOptions) (*api_v1.NamespaceList, error), deleter func(string, *api_v1.DeleteOptions) error) {
+// Verifies that namespaces are deleted from underlying clusters when orphan dependents is false
+// and they are not deleted when orphan dependents is true.
+func verifyNsCascadingDeletion(nsClient clientset.NamespaceInterface,
+	clusters map[string]*cluster, orphanDependents bool) {
+	nsName := createNamespace(nsClient)
+	// Check subclusters if the namespace was created there.
+	By(fmt.Sprintf("Waiting for namespace %s to be created in all underlying clusters", nsName))
+	err := wait.Poll(5*time.Second, 2*time.Minute, func() (bool, error) {
+		for _, cluster := range clusters {
+			_, err := cluster.Core().Namespaces().Get(nsName)
+			if err != nil && !errors.IsNotFound(err) {
+				return false, err
+			}
+			if err != nil {
+				return false, nil
+			}
+		}
+		return true, nil
+	})
+	framework.ExpectNoError(err, "Not all namespaces created")
+
+	By(fmt.Sprintf("Deleting namespace %s", nsName))
+	deleteAllTestNamespaces(orphanDependents, nsClient.List, nsClient.Delete)
+
+	By(fmt.Sprintf("Verifying namespaces %s in underlying clusters", nsName))
+	errMessages := []string{}
+	for clusterName, clusterClientset := range clusters {
+		_, err := clusterClientset.Core().Namespaces().Get(nsName)
+		if orphanDependents && errors.IsNotFound(err) {
+			errMessages = append(errMessages, fmt.Sprintf("unexpected NotFound error for namespace %s in cluster %s, expected namespace to exist", nsName, clusterName))
+		} else if !orphanDependents && (err == nil || !errors.IsNotFound(err)) {
+			errMessages = append(errMessages, fmt.Sprintf("expected NotFound error for namespace %s in cluster %s, got error: %v", nsName, clusterName, err))
+		}
+	}
+	if len(errMessages) != 0 {
+		framework.Failf("%s", strings.Join(errMessages, "; "))
+	}
+}
+
+func createNamespace(nsClient clientset.NamespaceInterface) string {
+	ns := api_v1.Namespace{
+		ObjectMeta: api_v1.ObjectMeta{
+			Name: api.SimpleNameGenerator.GenerateName(namespacePrefix),
+		},
+	}
+	By(fmt.Sprintf("Creating namespace %s", ns.Name))
+	_, err := nsClient.Create(&ns)
+	framework.ExpectNoError(err, "Failed to create namespace %s", ns.Name)
+	By(fmt.Sprintf("Created namespace %s", ns.Name))
+	return ns.Name
+}
+
+func deleteAllTestNamespaces(orphanDependents bool, lister func(api_v1.ListOptions) (*api_v1.NamespaceList, error), deleter func(string, *api_v1.DeleteOptions) error) {
 	list, err := lister(api_v1.ListOptions{})
 	if err != nil {
 		framework.Failf("Failed to get all namespaes: %v", err)
@@ -123,8 +193,7 @@ func deleteAllTestNamespaces(lister func(api_v1.ListOptions) (*api_v1.NamespaceL
 	}
 	for _, namespace := range list.Items {
 		if strings.HasPrefix(namespace.Name, namespacePrefix) {
-			// Do not orphan dependents (corresponding namespaces in underlying clusters).
-			orphanDependents := false
+			By(fmt.Sprintf("Deleting ns: %s, found by listing", namespace.Name))
 			err := deleter(namespace.Name, &api_v1.DeleteOptions{OrphanDependents: &orphanDependents})
 			if err != nil {
 				framework.Failf("Failed to set %s for deletion: %v", namespace.Name, err)

test/test_owners.csv

@@ -138,7 +138,10 @@ Federation API server authentication should not accept cluster resources when th
 Federation apiserver Admission control should not be able to create resources if namespace does not exist,alex-mohr,1
 Federation apiserver Cluster objects should be created and deleted successfully,ghodss,1
 Federation events Event objects should be created and deleted successfully,karlkfi,1
+Federation namespace Namespace objects all resources in the namespace should be deleted when namespace is deleted,nikhiljindal,0
 Federation namespace Namespace objects should be created and deleted successfully,xiang90,1
+Federation namespace Namespace objects should be deleted from underlying clusters when OrphanDependents is false,nikhiljindal,0
+Federation namespace Namespace objects should not be deleted from underlying clusters when OrphanDependents is true,nikhiljindal,0
 Federation replicasets Federated ReplicaSet should create and update matching replicasets in underling clusters,childsb,1
 Federation replicasets ReplicaSet objects should be created and deleted successfully,apelisse,1
 Federation secrets Secret objects should be created and deleted successfully,pmorie,1