Skip to content

Commit 86d0049

Browse files
josvazgjosvazg
authored
CLOUDP-340286: Reconciler on skip removal (#2839)
* CLOUDP-340286: Reconciler on skip removal Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com> * Reuse predicate logic --------- Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com>
1 parent 84ec394 commit 86d0049

File tree

4 files changed

+296
-21
lines changed

4 files changed

+296
-21
lines changed

‎internal/controller/customresource/customresource.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/Masterminds/semver"
2222
"go.uber.org/zap"
2323
apierrors "k8s.io/apimachinery/pkg/api/errors"
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2425
"sigs.k8s.io/controller-runtime/pkg/client"
2526
"sigs.k8s.io/controller-runtime/pkg/reconcile"
2627

@@ -136,7 +137,7 @@ func ResourceVersionIsValid(resource akov2.AtlasCustomResource) (bool, error) {
136137
}
137138

138139
// ReconciliationShouldBeSkipped returns 'true' if reconciliation should be skipped for this resource.
139-
func ReconciliationShouldBeSkipped(resource akov2.AtlasCustomResource) bool {
140+
func ReconciliationShouldBeSkipped(resource metav1.Object) bool {
140141
if v, ok := resource.GetAnnotations()[ReconciliationPolicyAnnotation]; ok {
141142
return v == ReconciliationPolicySkip
142143
}

‎internal/controller/registry.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func (r *Registry) registerControllers(c cluster.Cluster, ap atlas.Provider) {
144144

145145
// deprecatedPredicates are to be phased out in favor of defaultPredicates
146146
func (r *Registry) deprecatedPredicates() []predicate.Predicate {
147-
return append(r.sharedPredicates, watch.DeprecatedCommonPredicates())
147+
return append(r.sharedPredicates, watch.DeprecatedCommonPredicates[client.Object]())
148148
}
149149

150150
// defaultPredicates minimize the reconciliations controllers actually do, avoiding

‎internal/controller/watch/predicates.go‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,25 +21,29 @@ import (
2121
"sigs.k8s.io/controller-runtime/pkg/client"
2222
"sigs.k8s.io/controller-runtime/pkg/event"
2323
"sigs.k8s.io/controller-runtime/pkg/predicate"
24+
25+
"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/controller/customresource"
2426
)
2527

2628
// DeprecatedCommonPredicates returns the predicate which filter out the changes done to any field except for spec (e.g. status)
2729
// Also we should reconcile if finalizers have changed (see https://blog.openshift.com/kubernetes-operators-best-practices/)
2830
// This will be phased out gradually to be replaced by DefaultPredicates
29-
func DeprecatedCommonPredicates() predicate.Funcs {
30-
return predicate.Funcs{
31-
UpdateFunc: func(e event.UpdateEvent) bool {
32-
if e.ObjectOld.GetResourceVersion() == e.ObjectNew.GetResourceVersion() {
33-
// resource version didn't change, so this is a resync, allow reconciliation.
34-
return true
35-
}
31+
func DeprecatedCommonPredicates[T metav1.Object]() predicate.TypedPredicate[T] {
32+
return predicate.Or(
33+
SkipAnnotationRemovedPredicate[T](),
34+
predicate.TypedFuncs[T]{
35+
UpdateFunc: func(e event.TypedUpdateEvent[T]) bool {
36+
if e.ObjectOld.GetResourceVersion() == e.ObjectNew.GetResourceVersion() {
37+
// resource version didn't change, so this is a resync, allow reconciliation.
38+
return true
39+
}
3640

37-
if e.ObjectOld.GetGeneration() == e.ObjectNew.GetGeneration() && reflect.DeepEqual(e.ObjectNew.GetFinalizers(), e.ObjectOld.GetFinalizers()) {
38-
return false
39-
}
40-
return true
41-
},
42-
}
41+
if e.ObjectOld.GetGeneration() == e.ObjectNew.GetGeneration() && reflect.DeepEqual(e.ObjectNew.GetFinalizers(), e.ObjectOld.GetFinalizers()) {
42+
return false
43+
}
44+
return true
45+
},
46+
})
4347
}
4448

4549
func SelectNamespacesPredicate(namespaces []string) predicate.Funcs {
@@ -58,11 +62,25 @@ func SelectNamespacesPredicate(namespaces []string) predicate.Funcs {
5862
})
5963
}
6064

65+
// SkipAnnotationRemovedPredicate reconciles on updates when the skip reconcile
66+
// annotation is removed or changed
67+
func SkipAnnotationRemovedPredicate[T metav1.Object]() predicate.TypedPredicate[T] {
68+
return predicate.TypedFuncs[T]{
69+
UpdateFunc: func(e event.TypedUpdateEvent[T]) bool {
70+
if customresource.ReconciliationShouldBeSkipped(e.ObjectOld) &&
71+
!customresource.ReconciliationShouldBeSkipped(e.ObjectNew) {
72+
return true
73+
}
74+
return false
75+
},
76+
}
77+
}
78+
6179
// GlobalResyncAwareGenerationChangePredicate reconcile on unfrequent global
6280
// resyncs or on spec generation changes, but ignore finalizer changes
6381
func GlobalResyncAwareGenerationChangePredicate[T metav1.Object]() predicate.TypedPredicate[T] {
64-
return predicate.Or[T](
65-
predicate.Not[T](predicate.TypedResourceVersionChangedPredicate[T]{}), // for the global resync
82+
return predicate.Or(
83+
predicate.Not(predicate.TypedResourceVersionChangedPredicate[T]{}), // for the global resync
6684
predicate.TypedGenerationChangedPredicate[T]{},
6785
)
6886
}
@@ -79,8 +97,11 @@ func IgnoreDeletedPredicate[T metav1.Object]() predicate.TypedPredicate[T] {
7997

8098
// DefaultPredicates avoid spurious after deletion or finalizer changes handling
8199
func DefaultPredicates[T metav1.Object]() predicate.TypedPredicate[T] {
82-
return predicate.And[T](
83-
GlobalResyncAwareGenerationChangePredicate[T](),
100+
return predicate.And( // or the generation changed or timed out (except for deletions)
101+
predicate.Or(
102+
GlobalResyncAwareGenerationChangePredicate[T](),
103+
SkipAnnotationRemovedPredicate[T](),
104+
),
84105
IgnoreDeletedPredicate[T](),
85106
)
86107
}

‎internal/controller/watch/predicates_test.go‎

Lines changed: 255 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,21 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
package watch
15+
package watch_test
1616

1717
import (
1818
"testing"
1919

2020
"github.com/stretchr/testify/assert"
21+
corev1 "k8s.io/api/core/v1"
2122
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"sigs.k8s.io/controller-runtime/pkg/client"
2224
"sigs.k8s.io/controller-runtime/pkg/event"
2325

26+
"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
2427
akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
28+
"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/controller/customresource"
29+
"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/controller/watch"
2530
)
2631

2732
func TestSelectNamespacesPredicate(t *testing.T) {
@@ -61,11 +66,259 @@ func TestSelectNamespacesPredicate(t *testing.T) {
6166

6267
for name, tt := range tests {
6368
t.Run(name, func(t *testing.T) {
64-
f := SelectNamespacesPredicate(tt.namespaces)
69+
f := watch.SelectNamespacesPredicate(tt.namespaces)
6570
assert.Equal(t, tt.expect, f.CreateFunc(tt.createEvent))
6671
assert.Equal(t, tt.expect, f.UpdateFunc(tt.updateEvent))
6772
assert.Equal(t, tt.expect, f.DeleteFunc(tt.deleteEvent))
6873
assert.Equal(t, tt.expect, f.GenericFunc(tt.genericEvent))
6974
})
7075
}
7176
}
77+
78+
func TestDeprecatedCommonPredicates(t *testing.T) {
79+
for _, tc := range []struct {
80+
title string
81+
old *akov2.AtlasProject
82+
new *akov2.AtlasProject
83+
want bool
84+
}{
85+
{
86+
title: "no changes - resync",
87+
old: sampleObj(),
88+
new: sampleObj(),
89+
want: true,
90+
},
91+
{
92+
title: "no gen change",
93+
old: sampleObj(resourceVersion("0")),
94+
new: sampleObj(resourceVersion("1")),
95+
want: false,
96+
},
97+
{
98+
title: "finalizers changed",
99+
old: sampleObj(resourceVersion("0")),
100+
new: sampleObj(resourceVersion("1"), finalizers([]string{"finalize"})),
101+
want: true,
102+
},
103+
{
104+
title: "skipped",
105+
old: sampleObj(resourceVersion("0")),
106+
new: sampleObj(resourceVersion("1"), skipAnnotation()),
107+
want: false,
108+
},
109+
{
110+
title: "no longer skipped",
111+
old: sampleObj(resourceVersion("0"), skipAnnotation()),
112+
new: sampleObj(
113+
resourceVersion("1"),
114+
),
115+
want: true,
116+
},
117+
} {
118+
t.Run(tc.title, func(t *testing.T) {
119+
f := watch.DeprecatedCommonPredicates[client.Object]()
120+
assert.Equal(t, tc.want, f.Update(
121+
event.UpdateEvent{ObjectOld: tc.old, ObjectNew: tc.new}))
122+
})
123+
}
124+
}
125+
126+
func TestDefaultPredicates(t *testing.T) {
127+
for _, tc := range []struct {
128+
title string
129+
old *akov2.AtlasProject
130+
new *akov2.AtlasProject
131+
wantCreate bool
132+
wantUpdate bool
133+
wantDelete bool
134+
wantGeneric bool
135+
}{
136+
{
137+
title: "no changes",
138+
old: sampleObj(),
139+
new: sampleObj(),
140+
wantCreate: true,
141+
wantUpdate: true,
142+
wantDelete: false,
143+
wantGeneric: true,
144+
},
145+
{
146+
title: "no gen change",
147+
old: sampleObj(resourceVersion("0")),
148+
new: sampleObj(resourceVersion("1")),
149+
wantCreate: true,
150+
wantUpdate: false,
151+
wantDelete: false,
152+
wantGeneric: true,
153+
},
154+
{
155+
title: "finalizers set",
156+
old: sampleObj(resourceVersion("0")),
157+
new: sampleObj(resourceVersion("1"), finalizers([]string{"finalize"})),
158+
wantCreate: true,
159+
wantUpdate: false, // finalizer changes do not trigger updates unlike with deprecated
160+
wantDelete: false,
161+
wantGeneric: true,
162+
},
163+
{
164+
title: "skipped",
165+
old: sampleObj(
166+
resourceVersion("0"),
167+
),
168+
new: sampleObj(
169+
resourceVersion("1"),
170+
skipAnnotation(),
171+
),
172+
wantCreate: true,
173+
wantUpdate: false,
174+
wantDelete: false,
175+
wantGeneric: true,
176+
},
177+
{
178+
title: "no longer skipped",
179+
old: sampleObj(resourceVersion("0"), skipAnnotation()),
180+
new: sampleObj(resourceVersion("1")),
181+
wantCreate: true,
182+
wantUpdate: true,
183+
wantDelete: false,
184+
wantGeneric: true,
185+
},
186+
{
187+
title: "finalizers removed",
188+
old: sampleObj(resourceVersion("0"), finalizers([]string{"finalize"})),
189+
new: sampleObj(resourceVersion("1")),
190+
wantCreate: true,
191+
wantUpdate: false,
192+
wantDelete: false,
193+
wantGeneric: true,
194+
},
195+
} {
196+
t.Run(tc.title, func(t *testing.T) {
197+
f := watch.DefaultPredicates[*akov2.AtlasProject]()
198+
assert.Equal(t, tc.wantCreate,
199+
f.Create(event.TypedCreateEvent[*akov2.AtlasProject]{Object: tc.new}), "on create")
200+
assert.Equal(t, tc.wantUpdate,
201+
f.Update(event.TypedUpdateEvent[*akov2.AtlasProject]{
202+
ObjectOld: tc.old, ObjectNew: tc.new,
203+
}), "on update")
204+
assert.Equal(t, tc.wantDelete,
205+
f.Delete(event.TypedDeleteEvent[*akov2.AtlasProject]{Object: tc.new}), "on delete")
206+
assert.Equal(t, tc.wantGeneric,
207+
f.Generic(event.TypedGenericEvent[*akov2.AtlasProject]{Object: tc.new}), "on generic event")
208+
})
209+
}
210+
}
211+
212+
func TestReadyTransitionPredicate(t *testing.T) {
213+
for _, tc := range []struct {
214+
title string
215+
old *akov2.AtlasProject
216+
new *akov2.AtlasProject
217+
wantCreate bool
218+
wantUpdate bool
219+
wantDelete bool
220+
wantGeneric bool
221+
}{
222+
{
223+
title: "no changes",
224+
old: sampleObj(),
225+
new: sampleObj(),
226+
wantCreate: false,
227+
wantUpdate: false,
228+
wantDelete: true,
229+
wantGeneric: false,
230+
},
231+
{
232+
title: "ready now",
233+
old: sampleObj(ready(corev1.ConditionFalse)),
234+
new: sampleObj(ready(corev1.ConditionTrue)),
235+
wantCreate: false,
236+
wantUpdate: true,
237+
wantDelete: true,
238+
wantGeneric: false,
239+
},
240+
{
241+
title: "no longer ready",
242+
old: sampleObj(ready(corev1.ConditionTrue)),
243+
new: sampleObj(ready(corev1.ConditionFalse)),
244+
wantCreate: false,
245+
wantUpdate: false,
246+
wantDelete: true,
247+
wantGeneric: false,
248+
},
249+
} {
250+
t.Run(tc.title, func(t *testing.T) {
251+
f := watch.ReadyTransitionPredicate(projectReady)
252+
assert.Equal(t, tc.wantCreate,
253+
f.Create(event.CreateEvent{Object: tc.new}), "on create")
254+
assert.Equal(t, tc.wantUpdate,
255+
f.Update(event.UpdateEvent{ObjectOld: tc.old, ObjectNew: tc.new}), "on update")
256+
assert.Equal(t, tc.wantDelete,
257+
f.Delete(event.DeleteEvent{Object: tc.new}), "on delete")
258+
assert.Equal(t, tc.wantGeneric,
259+
f.Generic(event.GenericEvent{Object: tc.new}), "on generic event")
260+
})
261+
}
262+
}
263+
264+
func projectReady(p *akov2.AtlasProject) bool {
265+
for _, c := range p.Status.Conditions {
266+
if c.Type == api.ReadyType && c.Status == corev1.ConditionTrue {
267+
return true
268+
}
269+
}
270+
return false
271+
}
272+
273+
type optionFunc func(*akov2.AtlasProject) *akov2.AtlasProject
274+
275+
func sampleObj(opts ...optionFunc) *akov2.AtlasProject {
276+
p := &akov2.AtlasProject{
277+
ObjectMeta: metav1.ObjectMeta{
278+
Name: "project",
279+
Namespace: "ns",
280+
Annotations: map[string]string{},
281+
},
282+
Spec: akov2.AtlasProjectSpec{
283+
Name: "atlas-project",
284+
},
285+
}
286+
for _, opt := range opts {
287+
p = opt(p)
288+
}
289+
return p
290+
}
291+
292+
func resourceVersion(rs string) optionFunc {
293+
return func(p *akov2.AtlasProject) *akov2.AtlasProject {
294+
p.ResourceVersion = rs
295+
return p
296+
}
297+
}
298+
299+
func skipAnnotation() optionFunc {
300+
return func(p *akov2.AtlasProject) *akov2.AtlasProject {
301+
p.Annotations[customresource.ReconciliationPolicyAnnotation] =
302+
customresource.ReconciliationPolicySkip
303+
return p
304+
}
305+
}
306+
307+
func finalizers(f []string) optionFunc {
308+
return func(p *akov2.AtlasProject) *akov2.AtlasProject {
309+
p.Finalizers = f
310+
return p
311+
}
312+
}
313+
314+
func ready(status corev1.ConditionStatus) optionFunc {
315+
return func(p *akov2.AtlasProject) *akov2.AtlasProject {
316+
p.Status.Conditions = append(p.Status.Conditions,
317+
api.Condition{
318+
Type: api.ReadyType,
319+
Status: status,
320+
},
321+
)
322+
return p
323+
}
324+
}

0 commit comments

Comments
 (0)