From aa518d13dea3c1f82174b0dc6cb2a1893ef624e9 Mon Sep 17 00:00:00 2001 From: nikhiljindal Date: Fri, 4 Nov 2016 20:26:52 -0700 Subject: [PATCH] Adding more e2e tests for federated namespace cascading deletion and fixing a few bugs --- .../cmd/federation-apiserver/app/server.go | 4 + .../deployment/deploymentcontroller.go | 2 +- .../namespace/namespace_controller.go | 6 +- .../util/deletionhelper/deletion_helper.go | 30 +++- pkg/registry/core/namespace/etcd/etcd.go | 13 ++ test/e2e/federated-namespace.go | 154 +++++++++++++----- 6 files changed, 159 insertions(+), 50 deletions(-) diff --git a/federation/cmd/federation-apiserver/app/server.go b/federation/cmd/federation-apiserver/app/server.go index a1bd1e3bf68..aff83f4ed84 100644 --- a/federation/cmd/federation-apiserver/app/server.go +++ b/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), } } diff --git a/federation/pkg/federation-controller/deployment/deploymentcontroller.go b/federation/pkg/federation-controller/deployment/deploymentcontroller.go index 6c1754ef4ec..0fada4c1055 100644 --- a/federation/pkg/federation-controller/deployment/deploymentcontroller.go +++ b/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++ { diff --git a/federation/pkg/federation-controller/namespace/namespace_controller.go b/federation/pkg/federation-controller/namespace/namespace_controller.go index b0dedaef656..c80b6ddca61 100644 --- a/federation/pkg/federation-controller/namespace/namespace_controller.go +++ b/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. diff --git a/federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go b/federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go index 9728bf1d1ff..877a8c58a1d 100644 --- a/federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go +++ b/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 diff --git a/pkg/registry/core/namespace/etcd/etcd.go b/pkg/registry/core/namespace/etcd/etcd.go index f601f21a0ea..f1eea75a9f2 100644 --- a/pkg/registry/core/namespace/etcd/etcd.go +++ b/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 }), ) diff --git a/test/e2e/federated-namespace.go b/test/e2e/federated-namespace.go index f92d1ae189e..a0b3339d299 100644 --- a/test/e2e/federated-namespace.go +++ b/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{ - 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)) + 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)