Namespace controller should always get latest state prior to deletion

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

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,
 		},
 	}
-
+	testNamespaceFinalizeComplete := &api.Namespace{
-	if containsVersion(versions, "extensions/v1beta1") {
+		ObjectMeta: api.ObjectMeta{
-		resources := []unversioned.APIResource{}
+			Name:              "test",
-		for _, resource := range []string{"daemonsets", "deployments", "jobs", "horizontalpodautoscalers", "ingresses"} {
+			ResourceVersion:   "1",
-			resources = append(resources, unversioned.APIResource{Name: resource})
+			DeletionTimestamp: &now,
-		}
+		},
-		mockClient.Resources = map[string]*unversioned.APIResourceList{
+		Spec: api.NamespaceSpec{},
-			"extensions/v1beta1": {
+		Status: api.NamespaceStatus{
-				GroupVersion: "extensions/v1beta1",
+			Phase: api.NamespaceTerminating,
-				APIResources: resources,
+		},
-			},
-		}
 	}
 
-	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()
+	scenarios := map[string]struct {
-	for _, action := range mockClient.Actions() {
+		testNamespace     *api.Namespace
-		actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource(), action.GetSubresource()}, "-"))
+		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))
+	for scenario, testInput := range scenarios {
-	}
+		mockClient := testclient.NewSimpleFake(testInput.testNamespace)
-	if !expectedActionSet.HasAll(actionSet.List()...) {
+		if containsVersion(versions, "extensions/v1beta1") {
-		t.Errorf("Expected actions:\n%v\n but got:\n%v\nDifference:\n%v", expectedActionSet, actionSet, actionSet.Difference(expectedActionSet))
+			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))
+		}
 	}
 }