diff --git a/federation/cmd/federation-apiserver/app/server.go b/federation/cmd/federation-apiserver/app/server.go index a1bd1e3bf6..aff83f4ed8 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 6c1754ef4e..0fada4c105 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 b0dedaef65..c80b6ddca6 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 9728bf1d1f..877a8c58a1 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 f601f21a0e..f1eea75a9f 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/BUILD b/test/e2e/BUILD index 10c75c6247..1f2f2c9f86 100644 --- a/test/e2e/BUILD +++ b/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", diff --git a/test/e2e/federated-namespace.go b/test/e2e/federated-namespace.go index f92d1ae189..30f1669903 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,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) diff --git a/test/test_owners.csv b/test/test_owners.csv index fbdfa9037f..e2846b5659 100644 --- a/test/test_owners.csv +++ b/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