From 745c58e788453b41b49ba125b0ec61c26e4e71a0 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Wed, 4 Nov 2015 17:47:28 -0500 Subject: [PATCH] Namespace controller should always get latest state prior to deletion --- .../namespace/namespace_controller.go | 23 ++++- .../namespace/namespace_controller_test.go | 87 ++++++++++++------- 2 files changed, 79 insertions(+), 31 deletions(-) diff --git a/pkg/controller/namespace/namespace_controller.go b/pkg/controller/namespace/namespace_controller.go index 2b6545209af..860c8e4dbca 100644 --- a/pkg/controller/namespace/namespace_controller.go +++ b/pkg/controller/namespace/namespace_controller.go @@ -141,7 +141,14 @@ func finalizeNamespaceFunc(kubeClient client.Interface, namespace *api.Namespace for _, value := range finalizerSet.List() { namespaceFinalize.Spec.Finalizers = append(namespaceFinalize.Spec.Finalizers, api.FinalizerName(value)) } - return kubeClient.Namespaces().Finalize(&namespaceFinalize) + namespace, err := kubeClient.Namespaces().Finalize(&namespaceFinalize) + if err != nil { + // it was removed already, so life is good + if errors.IsNotFound(err) { + return namespace, nil + } + } + return namespace, err } type contentRemainingError struct { @@ -268,10 +275,22 @@ func updateNamespaceStatusFunc(kubeClient client.Interface, namespace *api.Names } // syncNamespace orchestrates deletion of a Namespace and its associated content. -func syncNamespace(kubeClient client.Interface, versions *unversioned.APIVersions, namespace *api.Namespace) (err error) { +func syncNamespace(kubeClient client.Interface, versions *unversioned.APIVersions, namespace *api.Namespace) error { if namespace.DeletionTimestamp == nil { return nil } + + // multiple controllers may edit a namespace during termination + // first get the latest state of the namespace before proceeding + // if the namespace was deleted already, don't do anything + namespace, err := kubeClient.Namespaces().Get(namespace.Name) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + return err + } + glog.V(4).Infof("Syncing namespace %s", namespace.Name) // ensure that the status is up to date on the namespace diff --git a/pkg/controller/namespace/namespace_controller_test.go b/pkg/controller/namespace/namespace_controller_test.go index db92594f6cf..e3ba5ad5d71 100644 --- a/pkg/controller/namespace/namespace_controller_test.go +++ b/pkg/controller/namespace/namespace_controller_test.go @@ -74,9 +74,8 @@ func TestFinalizeNamespaceFunc(t *testing.T) { } func testSyncNamespaceThatIsTerminating(t *testing.T, versions *unversioned.APIVersions) { - mockClient := &testclient.Fake{} now := unversioned.Now() - testNamespace := &api.Namespace{ + testNamespacePendingFinalize := &api.Namespace{ ObjectMeta: api.ObjectMeta{ Name: "test", ResourceVersion: "1", @@ -89,26 +88,21 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *unversioned.APIV Phase: api.NamespaceTerminating, }, } - - if containsVersion(versions, "extensions/v1beta1") { - resources := []unversioned.APIResource{} - for _, resource := range []string{"daemonsets", "deployments", "jobs", "horizontalpodautoscalers", "ingresses"} { - resources = append(resources, unversioned.APIResource{Name: resource}) - } - mockClient.Resources = map[string]*unversioned.APIResourceList{ - "extensions/v1beta1": { - GroupVersion: "extensions/v1beta1", - APIResources: resources, - }, - } + testNamespaceFinalizeComplete := &api.Namespace{ + ObjectMeta: api.ObjectMeta{ + Name: "test", + ResourceVersion: "1", + DeletionTimestamp: &now, + }, + Spec: api.NamespaceSpec{}, + Status: api.NamespaceStatus{ + Phase: api.NamespaceTerminating, + }, } - err := syncNamespace(mockClient, versions, testNamespace) - if err != nil { - t.Errorf("Unexpected error when synching namespace %v", err) - } // TODO: Reuse the constants for all these strings from testclient - expectedActionSet := sets.NewString( + pendingActionSet := sets.NewString( + strings.Join([]string{"get", "namespaces", ""}, "-"), strings.Join([]string{"list", "replicationcontrollers", ""}, "-"), strings.Join([]string{"list", "services", ""}, "-"), strings.Join([]string{"list", "pods", ""}, "-"), @@ -119,11 +113,10 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *unversioned.APIV strings.Join([]string{"list", "serviceaccounts", ""}, "-"), strings.Join([]string{"list", "persistentvolumeclaims", ""}, "-"), strings.Join([]string{"create", "namespaces", "finalize"}, "-"), - strings.Join([]string{"delete", "namespaces", ""}, "-"), ) if containsVersion(versions, "extensions/v1beta1") { - expectedActionSet.Insert( + pendingActionSet.Insert( strings.Join([]string{"list", "daemonsets", ""}, "-"), strings.Join([]string{"list", "deployments", ""}, "-"), strings.Join([]string{"list", "jobs", ""}, "-"), @@ -133,15 +126,51 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *unversioned.APIV ) } - actionSet := sets.NewString() - for _, action := range mockClient.Actions() { - actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource(), action.GetSubresource()}, "-")) + scenarios := map[string]struct { + testNamespace *api.Namespace + expectedActionSet sets.String + }{ + "pending-finalize": { + testNamespace: testNamespacePendingFinalize, + expectedActionSet: pendingActionSet, + }, + "complete-finalize": { + testNamespace: testNamespaceFinalizeComplete, + expectedActionSet: sets.NewString( + strings.Join([]string{"get", "namespaces", ""}, "-"), + strings.Join([]string{"delete", "namespaces", ""}, "-"), + ), + }, } - if !actionSet.HasAll(expectedActionSet.List()...) { - t.Errorf("Expected actions:\n%v\n but got:\n%v\nDifference:\n%v", expectedActionSet, actionSet, expectedActionSet.Difference(actionSet)) - } - if !expectedActionSet.HasAll(actionSet.List()...) { - t.Errorf("Expected actions:\n%v\n but got:\n%v\nDifference:\n%v", expectedActionSet, actionSet, actionSet.Difference(expectedActionSet)) + + for scenario, testInput := range scenarios { + mockClient := testclient.NewSimpleFake(testInput.testNamespace) + if containsVersion(versions, "extensions/v1beta1") { + resources := []unversioned.APIResource{} + for _, resource := range []string{"daemonsets", "deployments", "jobs", "horizontalpodautoscalers", "ingresses"} { + resources = append(resources, unversioned.APIResource{Name: resource}) + } + mockClient.Resources = map[string]*unversioned.APIResourceList{ + "extensions/v1beta1": { + GroupVersion: "extensions/v1beta1", + APIResources: resources, + }, + } + } + err := syncNamespace(mockClient, versions, testInput.testNamespace) + if err != nil { + t.Errorf("scenario %s - Unexpected error when synching namespace %v", scenario, err) + } + actionSet := sets.NewString() + for _, action := range mockClient.Actions() { + actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource(), action.GetSubresource()}, "-")) + } + if !actionSet.HasAll(testInput.expectedActionSet.List()...) { + t.Errorf("scenario %s - Expected actions:\n%v\n but got:\n%v\nDifference:\n%v", scenario, testInput.expectedActionSet, actionSet, testInput.expectedActionSet.Difference(actionSet)) + } + if !testInput.expectedActionSet.HasAll(actionSet.List()...) { + t.Errorf("scenario %s - Expected actions:\n%v\n but got:\n%v\nDifference:\n%v", scenario, testInput.expectedActionSet, actionSet, actionSet.Difference(testInput.expectedActionSet)) + } } }