Merge pull request #82288 from deads2k/ns-conditions

fix namespace termination conditions to be consistent and correct

pkg/controller/namespace/deletion/BUILD

@@ -30,7 +30,10 @@ go_library(
 
 go_test(
     name = "go_default_test",
-    srcs = ["namespaced_resources_deleter_test.go"],
+    srcs = [
+        "namespaced_resources_deleter_test.go",
+        "status_condition_utils_test.go",
+    ],
     embed = [":go_default_library"],
     deps = [
         "//pkg/apis/core:go_default_library",

pkg/controller/namespace/deletion/namespaced_resources_deleter.go

@@ -515,16 +515,18 @@ func (d *namespacedResourcesDeleter) deleteAllContent(ns *v1.Namespace) (int64,
 		}
 	}
 
-	if len(errs) > 0 {
-		if hasChanged := conditionUpdater.Update(ns); hasChanged {
-			if _, err = d.nsClient.UpdateStatus(ns); err != nil {
-				utilruntime.HandleError(fmt.Errorf("couldn't update status condition for namespace %q: %v", namespace, err))
-			}
+	// we always want to update the conditions because if we have set a condition to "it worked" after it was previously, "it didn't work",
+	// we need to reflect that information.  Recall that additional finalizers can be set on namespaces, so this finalizer may clear itself and
+	// NOT remove the resource instance.
+	if hasChanged := conditionUpdater.Update(ns); hasChanged {
+		if _, err = d.nsClient.UpdateStatus(ns); err != nil {
+			utilruntime.HandleError(fmt.Errorf("couldn't update status condition for namespace %q: %v", namespace, err))
 		}
-		return estimate, utilerrors.NewAggregate(errs)
 	}
-	klog.V(4).Infof("namespace controller - deleteAllContent - namespace: %s, estimate: %v", namespace, estimate)
-	return estimate, nil
+
+	// if len(errs)==0, NewAggregate returns nil.
+	klog.V(4).Infof("namespace controller - deleteAllContent - namespace: %s, estimate: %v, errors: %v", namespace, estimate, utilerrors.NewAggregate(errs))
+	return estimate, utilerrors.NewAggregate(errs)
 }
 
 // estimateGrracefulTermination will estimate the graceful termination required for the specific entity in the namespace

pkg/controller/namespace/deletion/namespaced_resources_deleter_test.go

@@ -146,6 +146,7 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio
 				strings.Join([]string{"get", "namespaces", ""}, "-"),
 				strings.Join([]string{"create", "namespaces", "finalize"}, "-"),
 				strings.Join([]string{"list", "pods", ""}, "-"),
+				strings.Join([]string{"update", "namespaces", "status"}, "-"),
 				strings.Join([]string{"delete", "namespaces", ""}, "-"),
 			),
 			metadataClientActionSet: metadataClientActionSet,
@@ -187,68 +188,66 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio
 	}
 
 	for scenario, testInput := range scenarios {
-		testHandler := &fakeActionHandler{statusCode: 200}
-		srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP)
-		defer srv.Close()
+		t.Run(scenario, func(t *testing.T) {
+			testHandler := &fakeActionHandler{statusCode: 200}
+			srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP)
+			defer srv.Close()
 
-		mockClient := fake.NewSimpleClientset(testInput.testNamespace)
-		metadataClient, err := metadata.NewForConfig(clientConfig)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		fn := func() ([]*metav1.APIResourceList, error) {
-			return resources, testInput.gvrError
-		}
-		d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), metadataClient, mockClient.CoreV1(), fn, v1.FinalizerKubernetes, true)
-		if err := d.Delete(testInput.testNamespace.Name); !matchErrors(err, testInput.expectErrorOnDelete) {
-			t.Errorf("scenario %s - expected error %q when syncing namespace, got %q, %v", scenario, testInput.expectErrorOnDelete, err, testInput.expectErrorOnDelete == err)
-		}
-
-		// validate traffic from kube client
-		actionSet := sets.NewString()
-		for _, action := range mockClient.Actions() {
-			actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource().Resource, action.GetSubresource()}, "-"))
-		}
-		if !actionSet.Equal(testInput.kubeClientActionSet) {
-			t.Errorf("scenario %s - mock client expected actions:\n%v\n but got:\n%v\nDifference:\n%v", scenario,
-				testInput.kubeClientActionSet, actionSet, testInput.kubeClientActionSet.Difference(actionSet))
-		}
-
-		// validate traffic from metadata client
-		actionSet = sets.NewString()
-		for _, action := range testHandler.actions {
-			actionSet.Insert(action.String())
-		}
-		if !actionSet.Equal(testInput.metadataClientActionSet) {
-			t.Errorf("scenario %s - metadata client expected actions:\n%v\n but got:\n%v\nDifference:\n%v", scenario,
-				testInput.metadataClientActionSet, actionSet, testInput.metadataClientActionSet.Difference(actionSet))
-		}
-
-		// validate status conditions
-		if testInput.expectStatus != nil {
-			obj, err := mockClient.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "namespaces"}, testInput.testNamespace.Namespace, testInput.testNamespace.Name)
+			mockClient := fake.NewSimpleClientset(testInput.testNamespace)
+			metadataClient, err := metadata.NewForConfig(clientConfig)
 			if err != nil {
-				t.Errorf("Unexpected error in getting the namespace: %v", err)
-				continue
+				t.Fatal(err)
 			}
-			ns, ok := obj.(*v1.Namespace)
-			if !ok {
-				t.Errorf("Expected a namespace but received %v", obj)
-				continue
+
+			fn := func() ([]*metav1.APIResourceList, error) {
+				return resources, testInput.gvrError
 			}
-			if ns.Status.Phase != testInput.expectStatus.Phase {
-				t.Errorf("Expected namespace status phase %v but received %v", testInput.expectStatus.Phase, ns.Status.Phase)
-				continue
+			d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), metadataClient, mockClient.CoreV1(), fn, v1.FinalizerKubernetes, true)
+			if err := d.Delete(testInput.testNamespace.Name); !matchErrors(err, testInput.expectErrorOnDelete) {
+				t.Errorf("expected error %q when syncing namespace, got %q, %v", testInput.expectErrorOnDelete, err, testInput.expectErrorOnDelete == err)
 			}
-			for _, expCondition := range testInput.expectStatus.Conditions {
-				nsCondition := getCondition(ns.Status.Conditions, expCondition.Type)
-				if nsCondition == nil {
-					t.Errorf("Missing namespace status condition %v", expCondition.Type)
-					continue
+
+			// validate traffic from kube client
+			actionSet := sets.NewString()
+			for _, action := range mockClient.Actions() {
+				actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource().Resource, action.GetSubresource()}, "-"))
+			}
+			if !actionSet.Equal(testInput.kubeClientActionSet) {
+				t.Errorf("mock client expected actions:\n%v\n but got:\n%v\nDifference:\n%v",
+					testInput.kubeClientActionSet, actionSet, testInput.kubeClientActionSet.Difference(actionSet))
+			}
+
+			// validate traffic from metadata client
+			actionSet = sets.NewString()
+			for _, action := range testHandler.actions {
+				actionSet.Insert(action.String())
+			}
+			if !actionSet.Equal(testInput.metadataClientActionSet) {
+				t.Errorf(" metadata client expected actions:\n%v\n but got:\n%v\nDifference:\n%v",
+					testInput.metadataClientActionSet, actionSet, testInput.metadataClientActionSet.Difference(actionSet))
+			}
+
+			// validate status conditions
+			if testInput.expectStatus != nil {
+				obj, err := mockClient.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "namespaces"}, testInput.testNamespace.Namespace, testInput.testNamespace.Name)
+				if err != nil {
+					t.Fatalf("Unexpected error in getting the namespace: %v", err)
+				}
+				ns, ok := obj.(*v1.Namespace)
+				if !ok {
+					t.Fatalf("Expected a namespace but received %v", obj)
+				}
+				if ns.Status.Phase != testInput.expectStatus.Phase {
+					t.Fatalf("Expected namespace status phase %v but received %v", testInput.expectStatus.Phase, ns.Status.Phase)
+				}
+				for _, expCondition := range testInput.expectStatus.Conditions {
+					nsCondition := getCondition(ns.Status.Conditions, expCondition.Type)
+					if nsCondition == nil {
+						t.Fatalf("Missing namespace status condition %v", expCondition.Type)
+					}
 				}
 			}
-		}
+		})
 	}
 }
 

pkg/controller/namespace/deletion/status_condition_utils.go

@@ -21,7 +21,7 @@ import (
 	"sort"
 	"strings"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/discovery"
 )
@@ -128,25 +128,18 @@ func makeDeleteContentCondition(err []error) *v1.NamespaceCondition {
 func updateConditions(status *v1.NamespaceStatus, newConditions []v1.NamespaceCondition) (hasChanged bool) {
 	for _, conditionType := range conditionTypes {
 		newCondition := getCondition(newConditions, conditionType)
-		oldCondition := getCondition(status.Conditions, conditionType)
-		if newCondition == nil && oldCondition == nil {
-			// both are nil, no update necessary
-			continue
+		// if we weren't failing, then this returned nil.  We should set the "ok" variant of the condition
+		if newCondition == nil {
+			newCondition = newSuccessfulCondition(conditionType)
 		}
+		oldCondition := getCondition(status.Conditions, conditionType)
+
+		// only new condition of this type exists, add to the list
 		if oldCondition == nil {
-			// only new condition of this type exists, add to the list
 			status.Conditions = append(status.Conditions, *newCondition)
 			hasChanged = true
-		} else if newCondition == nil {
-			// only old condition of this type exists, set status to false
-			if oldCondition.Status != v1.ConditionFalse {
-				oldCondition.Status = v1.ConditionFalse
-				oldCondition.Message = okMessages[conditionType]
-				oldCondition.Reason = okReasons[conditionType]
-				oldCondition.LastTransitionTime = metav1.Now()
-				hasChanged = true
-			}
-		} else if oldCondition.Message != newCondition.Message {
+
+		} else if oldCondition.Status != newCondition.Status || oldCondition.Message != newCondition.Message || oldCondition.Reason != newCondition.Reason {
 			// old condition needs to be updated
 			if oldCondition.Status != newCondition.Status {
 				oldCondition.LastTransitionTime = metav1.Now()
@@ -161,6 +154,16 @@ func updateConditions(status *v1.NamespaceStatus, newConditions []v1.NamespaceCo
 	return
 }
 
+func newSuccessfulCondition(conditionType v1.NamespaceConditionType) *v1.NamespaceCondition {
+	return &v1.NamespaceCondition{
+		Type:               conditionType,
+		Status:             v1.ConditionFalse,
+		LastTransitionTime: metav1.Now(),
+		Reason:             okReasons[conditionType],
+		Message:            okMessages[conditionType],
+	}
+}
+
 func getCondition(conditions []v1.NamespaceCondition, conditionType v1.NamespaceConditionType) *v1.NamespaceCondition {
 	for i := range conditions {
 		if conditions[i].Type == conditionType {

pkg/controller/namespace/deletion/status_condition_utils_test.go

@@ -0,0 +1,137 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package deletion
+
+import (
+	"reflect"
+	"testing"
+
+	v1 "k8s.io/api/core/v1"
+)
+
+func TestUpdateConditions(t *testing.T) {
+	tests := []struct {
+		name string
+
+		newConditions  []v1.NamespaceCondition
+		startingStatus *v1.NamespaceStatus
+
+		expecteds []v1.NamespaceCondition
+	}{
+		{
+			name: "leave unknown",
+
+			newConditions: []v1.NamespaceCondition{},
+			startingStatus: &v1.NamespaceStatus{
+				Conditions: []v1.NamespaceCondition{
+					{Type: "unknown", Status: v1.ConditionTrue},
+				},
+			},
+			expecteds: []v1.NamespaceCondition{
+				{Type: "unknown", Status: v1.ConditionTrue},
+				*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionGVParsingFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
+			},
+		},
+		{
+			name: "replace with success",
+
+			newConditions: []v1.NamespaceCondition{},
+			startingStatus: &v1.NamespaceStatus{
+				Conditions: []v1.NamespaceCondition{
+					{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
+				},
+			},
+			expecteds: []v1.NamespaceCondition{
+				*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionGVParsingFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
+			},
+		},
+		{
+			name: "leave different order",
+
+			newConditions: []v1.NamespaceCondition{},
+			startingStatus: &v1.NamespaceStatus{
+				Conditions: []v1.NamespaceCondition{
+					{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue},
+					{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
+				},
+			},
+			expecteds: []v1.NamespaceCondition{
+				*newSuccessfulCondition(v1.NamespaceDeletionGVParsingFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
+			},
+		},
+		{
+			name: "overwrite with failure",
+
+			newConditions: []v1.NamespaceCondition{
+				{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
+			},
+			startingStatus: &v1.NamespaceStatus{
+				Conditions: []v1.NamespaceCondition{
+					{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionFalse},
+					{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
+				},
+			},
+			expecteds: []v1.NamespaceCondition{
+				{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
+				*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
+				*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
+			},
+		},
+		{
+			name: "write new failure",
+
+			newConditions: []v1.NamespaceCondition{
+				{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
+			},
+			startingStatus: &v1.NamespaceStatus{
+				Conditions: []v1.NamespaceCondition{
+					{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
+				},
+			},
+			expecteds: []v1.NamespaceCondition{
+				*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
+				{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
+				*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			updateConditions(test.startingStatus, test.newConditions)
+
+			actuals := test.startingStatus.Conditions
+			if len(actuals) != len(test.expecteds) {
+				t.Fatal(actuals)
+			}
+			for i := range actuals {
+				actual := actuals[i]
+				expected := test.expecteds[i]
+				expected.LastTransitionTime = actual.LastTransitionTime
+				if !reflect.DeepEqual(expected, actual) {
+					t.Error(actual)
+				}
+			}
+		})
+	}
+}