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

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
+		}
+	}
+	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 dh.addFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters)
+	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/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,124 @@ var _ = framework.KubeDescribe("Federation namespace [Feature:Federation]", func
 		It("should be created and deleted successfully", func() {
 			framework.SkipUnlessFederated(f.ClientSet)
 
-			ns := api_v1.Namespace{
+			nsName := createNamespace(f.FederationClientset_1_5.Core().Namespaces())
+
+			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("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(namespacePrefix),
+					Name:      api.SimpleNameGenerator.GenerateName(eventNamePrefix),
+					Namespace: nsName,
+				},
+				InvolvedObject: api_v1.ObjectReference{
+					Kind:      "Pod",
+					Namespace: nsName,
+					Name:      "sample-pod",
 				},
 			}
-			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)
-
-			// 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("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("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("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))
+	fail := false
+	for clusterName, clusterClientset := range clusters {
+		_, err := clusterClientset.Core().Namespaces().Get(nsName)
+		if orphanDependents && errors.IsNotFound(err) {
+			fail = true
+			//			framework.Failf("unexpected NotFound error for namespace %s in cluster %s, expected namespace to exist", nsName, clusterName)
+			By(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)) {
+			fail = true
+			By(fmt.Sprintf("expected NotFound error for namespace %s in cluster %s, got error: %v", nsName, clusterName, err))
+		} else {
+			By(fmt.Sprintf("Woohoo!! Succeeded for cluster %s", clusterName))
+		}
+	}
+	if fail == true {
+		framework.Failf("Failed")
+	}
+}
+
+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 +198,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)