Skip to content

Commit 208cfaa

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#82288 from deads2k/ns-conditions
fix namespace termination conditions to be consistent and correct
2 parents 9614a85 + 076bf94 commit 208cfaa

File tree

9 files changed

+453
-79
lines changed

9 files changed

+453
-79
lines changed

‎pkg/controller/namespace/deletion/BUILD‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ go_library(
3030

3131
go_test(
3232
name = "go_default_test",
33-
srcs = ["namespaced_resources_deleter_test.go"],
33+
srcs = [
34+
"namespaced_resources_deleter_test.go",
35+
"status_condition_utils_test.go",
36+
],
3437
embed = [":go_default_library"],
3538
deps = [
3639
"//pkg/apis/core:go_default_library",

‎pkg/controller/namespace/deletion/namespaced_resources_deleter.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -515,16 +515,18 @@ func (d *namespacedResourcesDeleter) deleteAllContent(ns *v1.Namespace) (int64,
515515
}
516516
}
517517

518-
if len(errs) > 0 {
519-
if hasChanged := conditionUpdater.Update(ns); hasChanged {
520-
if _, err = d.nsClient.UpdateStatus(ns); err != nil {
521-
utilruntime.HandleError(fmt.Errorf("couldn't update status condition for namespace %q: %v", namespace, err))
522-
}
518+
// 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",
519+
// we need to reflect that information. Recall that additional finalizers can be set on namespaces, so this finalizer may clear itself and
520+
// NOT remove the resource instance.
521+
if hasChanged := conditionUpdater.Update(ns); hasChanged {
522+
if _, err = d.nsClient.UpdateStatus(ns); err != nil {
523+
utilruntime.HandleError(fmt.Errorf("couldn't update status condition for namespace %q: %v", namespace, err))
523524
}
524-
return estimate, utilerrors.NewAggregate(errs)
525525
}
526-
klog.V(4).Infof("namespace controller - deleteAllContent - namespace: %s, estimate: %v", namespace, estimate)
527-
return estimate, nil
526+
527+
// if len(errs)==0, NewAggregate returns nil.
528+
klog.V(4).Infof("namespace controller - deleteAllContent - namespace: %s, estimate: %v, errors: %v", namespace, estimate, utilerrors.NewAggregate(errs))
529+
return estimate, utilerrors.NewAggregate(errs)
528530
}
529531

530532
// estimateGrracefulTermination will estimate the graceful termination required for the specific entity in the namespace

‎pkg/controller/namespace/deletion/namespaced_resources_deleter_test.go‎

Lines changed: 53 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio
146146
strings.Join([]string{"get", "namespaces", ""}, "-"),
147147
strings.Join([]string{"create", "namespaces", "finalize"}, "-"),
148148
strings.Join([]string{"list", "pods", ""}, "-"),
149+
strings.Join([]string{"update", "namespaces", "status"}, "-"),
149150
strings.Join([]string{"delete", "namespaces", ""}, "-"),
150151
),
151152
metadataClientActionSet: metadataClientActionSet,
@@ -187,68 +188,66 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio
187188
}
188189

189190
for scenario, testInput := range scenarios {
190-
testHandler := &fakeActionHandler{statusCode: 200}
191-
srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP)
192-
defer srv.Close()
193-
194-
mockClient := fake.NewSimpleClientset(testInput.testNamespace)
195-
metadataClient, err := metadata.NewForConfig(clientConfig)
196-
if err != nil {
197-
t.Fatal(err)
198-
}
199-
200-
fn := func() ([]*metav1.APIResourceList, error) {
201-
return resources, testInput.gvrError
202-
}
203-
d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), metadataClient, mockClient.CoreV1(), fn, v1.FinalizerKubernetes, true)
204-
if err := d.Delete(testInput.testNamespace.Name); !matchErrors(err, testInput.expectErrorOnDelete) {
205-
t.Errorf("scenario %s - expected error %q when syncing namespace, got %q, %v", scenario, testInput.expectErrorOnDelete, err, testInput.expectErrorOnDelete == err)
206-
}
191+
t.Run(scenario, func(t *testing.T) {
192+
testHandler := &fakeActionHandler{statusCode: 200}
193+
srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP)
194+
defer srv.Close()
207195

208-
// validate traffic from kube client
209-
actionSet := sets.NewString()
210-
for _, action := range mockClient.Actions() {
211-
actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource().Resource, action.GetSubresource()}, "-"))
212-
}
213-
if !actionSet.Equal(testInput.kubeClientActionSet) {
214-
t.Errorf("scenario %s - mock client expected actions:\n%v\n but got:\n%v\nDifference:\n%v", scenario,
215-
testInput.kubeClientActionSet, actionSet, testInput.kubeClientActionSet.Difference(actionSet))
216-
}
196+
mockClient := fake.NewSimpleClientset(testInput.testNamespace)
197+
metadataClient, err := metadata.NewForConfig(clientConfig)
198+
if err != nil {
199+
t.Fatal(err)
200+
}
217201

218-
// validate traffic from metadata client
219-
actionSet = sets.NewString()
220-
for _, action := range testHandler.actions {
221-
actionSet.Insert(action.String())
222-
}
223-
if !actionSet.Equal(testInput.metadataClientActionSet) {
224-
t.Errorf("scenario %s - metadata client expected actions:\n%v\n but got:\n%v\nDifference:\n%v", scenario,
225-
testInput.metadataClientActionSet, actionSet, testInput.metadataClientActionSet.Difference(actionSet))
226-
}
202+
fn := func() ([]*metav1.APIResourceList, error) {
203+
return resources, testInput.gvrError
204+
}
205+
d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), metadataClient, mockClient.CoreV1(), fn, v1.FinalizerKubernetes, true)
206+
if err := d.Delete(testInput.testNamespace.Name); !matchErrors(err, testInput.expectErrorOnDelete) {
207+
t.Errorf("expected error %q when syncing namespace, got %q, %v", testInput.expectErrorOnDelete, err, testInput.expectErrorOnDelete == err)
208+
}
227209

228-
// validate status conditions
229-
if testInput.expectStatus != nil {
230-
obj, err := mockClient.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "namespaces"}, testInput.testNamespace.Namespace, testInput.testNamespace.Name)
231-
if err != nil {
232-
t.Errorf("Unexpected error in getting the namespace: %v", err)
233-
continue
210+
// validate traffic from kube client
211+
actionSet := sets.NewString()
212+
for _, action := range mockClient.Actions() {
213+
actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource().Resource, action.GetSubresource()}, "-"))
214+
}
215+
if !actionSet.Equal(testInput.kubeClientActionSet) {
216+
t.Errorf("mock client expected actions:\n%v\n but got:\n%v\nDifference:\n%v",
217+
testInput.kubeClientActionSet, actionSet, testInput.kubeClientActionSet.Difference(actionSet))
234218
}
235-
ns, ok := obj.(*v1.Namespace)
236-
if !ok {
237-
t.Errorf("Expected a namespace but received %v", obj)
238-
continue
219+
220+
// validate traffic from metadata client
221+
actionSet = sets.NewString()
222+
for _, action := range testHandler.actions {
223+
actionSet.Insert(action.String())
239224
}
240-
if ns.Status.Phase != testInput.expectStatus.Phase {
241-
t.Errorf("Expected namespace status phase %v but received %v", testInput.expectStatus.Phase, ns.Status.Phase)
242-
continue
225+
if !actionSet.Equal(testInput.metadataClientActionSet) {
226+
t.Errorf(" metadata client expected actions:\n%v\n but got:\n%v\nDifference:\n%v",
227+
testInput.metadataClientActionSet, actionSet, testInput.metadataClientActionSet.Difference(actionSet))
243228
}
244-
for _, expCondition := range testInput.expectStatus.Conditions {
245-
nsCondition := getCondition(ns.Status.Conditions, expCondition.Type)
246-
if nsCondition == nil {
247-
t.Errorf("Missing namespace status condition %v", expCondition.Type)
248-
continue
229+
230+
// validate status conditions
231+
if testInput.expectStatus != nil {
232+
obj, err := mockClient.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "namespaces"}, testInput.testNamespace.Namespace, testInput.testNamespace.Name)
233+
if err != nil {
234+
t.Fatalf("Unexpected error in getting the namespace: %v", err)
235+
}
236+
ns, ok := obj.(*v1.Namespace)
237+
if !ok {
238+
t.Fatalf("Expected a namespace but received %v", obj)
239+
}
240+
if ns.Status.Phase != testInput.expectStatus.Phase {
241+
t.Fatalf("Expected namespace status phase %v but received %v", testInput.expectStatus.Phase, ns.Status.Phase)
242+
}
243+
for _, expCondition := range testInput.expectStatus.Conditions {
244+
nsCondition := getCondition(ns.Status.Conditions, expCondition.Type)
245+
if nsCondition == nil {
246+
t.Fatalf("Missing namespace status condition %v", expCondition.Type)
247+
}
249248
}
250249
}
251-
}
250+
})
252251
}
253252
}
254253

‎pkg/controller/namespace/deletion/status_condition_utils.go‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import (
2121
"sort"
2222
"strings"
2323

24-
"k8s.io/api/core/v1"
24+
v1 "k8s.io/api/core/v1"
2525
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2626
"k8s.io/client-go/discovery"
2727
)
@@ -128,25 +128,18 @@ func makeDeleteContentCondition(err []error) *v1.NamespaceCondition {
128128
func updateConditions(status *v1.NamespaceStatus, newConditions []v1.NamespaceCondition) (hasChanged bool) {
129129
for _, conditionType := range conditionTypes {
130130
newCondition := getCondition(newConditions, conditionType)
131-
oldCondition := getCondition(status.Conditions, conditionType)
132-
if newCondition == nil && oldCondition == nil {
133-
// both are nil, no update necessary
134-
continue
131+
// if we weren't failing, then this returned nil. We should set the "ok" variant of the condition
132+
if newCondition == nil {
133+
newCondition = newSuccessfulCondition(conditionType)
135134
}
135+
oldCondition := getCondition(status.Conditions, conditionType)
136+
137+
// only new condition of this type exists, add to the list
136138
if oldCondition == nil {
137-
// only new condition of this type exists, add to the list
138139
status.Conditions = append(status.Conditions, *newCondition)
139140
hasChanged = true
140-
} else if newCondition == nil {
141-
// only old condition of this type exists, set status to false
142-
if oldCondition.Status != v1.ConditionFalse {
143-
oldCondition.Status = v1.ConditionFalse
144-
oldCondition.Message = okMessages[conditionType]
145-
oldCondition.Reason = okReasons[conditionType]
146-
oldCondition.LastTransitionTime = metav1.Now()
147-
hasChanged = true
148-
}
149-
} else if oldCondition.Message != newCondition.Message {
141+
142+
} else if oldCondition.Status != newCondition.Status || oldCondition.Message != newCondition.Message || oldCondition.Reason != newCondition.Reason {
150143
// old condition needs to be updated
151144
if oldCondition.Status != newCondition.Status {
152145
oldCondition.LastTransitionTime = metav1.Now()
@@ -161,6 +154,16 @@ func updateConditions(status *v1.NamespaceStatus, newConditions []v1.NamespaceCo
161154
return
162155
}
163156

157+
func newSuccessfulCondition(conditionType v1.NamespaceConditionType) *v1.NamespaceCondition {
158+
return &v1.NamespaceCondition{
159+
Type: conditionType,
160+
Status: v1.ConditionFalse,
161+
LastTransitionTime: metav1.Now(),
162+
Reason: okReasons[conditionType],
163+
Message: okMessages[conditionType],
164+
}
165+
}
166+
164167
func getCondition(conditions []v1.NamespaceCondition, conditionType v1.NamespaceConditionType) *v1.NamespaceCondition {
165168
for i := range conditions {
166169
if conditions[i].Type == conditionType {

‎pkg/controller/namespace/deletion/status_condition_utils_test.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/*
2+
Copyright 2019 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package deletion
18+
19+
import (
20+
"reflect"
21+
"testing"
22+
23+
v1 "k8s.io/api/core/v1"
24+
)
25+
26+
func TestUpdateConditions(t *testing.T) {
27+
tests := []struct {
28+
name string
29+
30+
newConditions []v1.NamespaceCondition
31+
startingStatus *v1.NamespaceStatus
32+
33+
expecteds []v1.NamespaceCondition
34+
}{
35+
{
36+
name: "leave unknown",
37+
38+
newConditions: []v1.NamespaceCondition{},
39+
startingStatus: &v1.NamespaceStatus{
40+
Conditions: []v1.NamespaceCondition{
41+
{Type: "unknown", Status: v1.ConditionTrue},
42+
},
43+
},
44+
expecteds: []v1.NamespaceCondition{
45+
{Type: "unknown", Status: v1.ConditionTrue},
46+
*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
47+
*newSuccessfulCondition(v1.NamespaceDeletionGVParsingFailure),
48+
*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
49+
},
50+
},
51+
{
52+
name: "replace with success",
53+
54+
newConditions: []v1.NamespaceCondition{},
55+
startingStatus: &v1.NamespaceStatus{
56+
Conditions: []v1.NamespaceCondition{
57+
{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
58+
},
59+
},
60+
expecteds: []v1.NamespaceCondition{
61+
*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
62+
*newSuccessfulCondition(v1.NamespaceDeletionGVParsingFailure),
63+
*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
64+
},
65+
},
66+
{
67+
name: "leave different order",
68+
69+
newConditions: []v1.NamespaceCondition{},
70+
startingStatus: &v1.NamespaceStatus{
71+
Conditions: []v1.NamespaceCondition{
72+
{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue},
73+
{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
74+
},
75+
},
76+
expecteds: []v1.NamespaceCondition{
77+
*newSuccessfulCondition(v1.NamespaceDeletionGVParsingFailure),
78+
*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
79+
*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
80+
},
81+
},
82+
{
83+
name: "overwrite with failure",
84+
85+
newConditions: []v1.NamespaceCondition{
86+
{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
87+
},
88+
startingStatus: &v1.NamespaceStatus{
89+
Conditions: []v1.NamespaceCondition{
90+
{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionFalse},
91+
{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
92+
},
93+
},
94+
expecteds: []v1.NamespaceCondition{
95+
{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
96+
*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
97+
*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
98+
},
99+
},
100+
{
101+
name: "write new failure",
102+
103+
newConditions: []v1.NamespaceCondition{
104+
{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
105+
},
106+
startingStatus: &v1.NamespaceStatus{
107+
Conditions: []v1.NamespaceCondition{
108+
{Type: v1.NamespaceDeletionDiscoveryFailure, Status: v1.ConditionTrue},
109+
},
110+
},
111+
expecteds: []v1.NamespaceCondition{
112+
*newSuccessfulCondition(v1.NamespaceDeletionDiscoveryFailure),
113+
{Type: v1.NamespaceDeletionGVParsingFailure, Status: v1.ConditionTrue, Reason: "foo", Message: "bar"},
114+
*newSuccessfulCondition(v1.NamespaceDeletionContentFailure),
115+
},
116+
},
117+
}
118+
119+
for _, test := range tests {
120+
t.Run(test.name, func(t *testing.T) {
121+
updateConditions(test.startingStatus, test.newConditions)
122+
123+
actuals := test.startingStatus.Conditions
124+
if len(actuals) != len(test.expecteds) {
125+
t.Fatal(actuals)
126+
}
127+
for i := range actuals {
128+
actual := actuals[i]
129+
expected := test.expecteds[i]
130+
expected.LastTransitionTime = actual.LastTransitionTime
131+
if !reflect.DeepEqual(expected, actual) {
132+
t.Error(actual)
133+
}
134+
}
135+
})
136+
}
137+
}

‎test/integration/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ filegroup(
5858
"//test/integration/kubelet:all-srcs",
5959
"//test/integration/master:all-srcs",
6060
"//test/integration/metrics:all-srcs",
61+
"//test/integration/namespace:all-srcs",
6162
"//test/integration/objectmeta:all-srcs",
6263
"//test/integration/openshift:all-srcs",
6364
"//test/integration/pods:all-srcs",

0 commit comments

Comments
 (0)