Skip to content

Commit 426beb1

Browse files
fix: Allow spec changes after ProgressDeadlineExceeded
Once ProgressDeadlineExceeded is set, the controller predicate was blocking ALL updates including spec changes like lifecycleState=Archived. This prevented timed-out ClusterObjectSets from being archived. Now allows generation/spec changes while still suppressing status-only churn.
1 parent 43351b2 commit 426beb1

2 files changed

Lines changed: 209 additions & 7 deletions

File tree

internal/operator-controller/controllers/clusterobjectset_controller.go

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -344,21 +344,44 @@ type Sourcoser interface {
344344
Source(handler handler.EventHandler, predicates ...predicate.Predicate) source.Source
345345
}
346346

347+
// shouldAllowProgressDeadlineExceededUpdate determines if an update should be allowed
348+
// when a ClusterObjectSet has reached ProgressDeadlineExceeded status.
349+
// It allows spec/generation changes (including lifecycleState transitions) but suppresses
350+
// status-only updates to prevent reconcile churn.
351+
func shouldAllowProgressDeadlineExceededUpdate(oldRev, newRev *ocv1.ClusterObjectSet) bool {
352+
// Allow deletions to happen
353+
if !newRev.DeletionTimestamp.IsZero() {
354+
return true
355+
}
356+
357+
// Check if the revision has ProgressDeadlineExceeded condition
358+
cnd := meta.FindStatusCondition(newRev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
359+
if cnd == nil || cnd.Status != metav1.ConditionFalse || cnd.Reason != ocv1.ReasonProgressDeadlineExceeded {
360+
return true
361+
}
362+
363+
// Progress deadline exceeded - allow generation/spec changes but suppress status-only churn
364+
// Allow if generation changed (indicates spec change, including lifecycleState transitions)
365+
if oldRev.Generation != newRev.Generation {
366+
return true
367+
}
368+
369+
// Suppress status-only updates
370+
return false
371+
}
372+
347373
func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
348374
skipProgressDeadlineExceededPredicate := predicate.Funcs{
349375
UpdateFunc: func(e event.UpdateEvent) bool {
350-
rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet)
376+
newRev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet)
351377
if !ok {
352378
return true
353379
}
354-
// allow deletions to happen
355-
if !rev.DeletionTimestamp.IsZero() {
380+
oldRev, ok := e.ObjectOld.(*ocv1.ClusterObjectSet)
381+
if !ok {
356382
return true
357383
}
358-
if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded {
359-
return false
360-
}
361-
return true
384+
return shouldAllowProgressDeadlineExceededUpdate(oldRev, newRev)
362385
},
363386
}
364387
c.Clock = clock.RealClock{}

internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,3 +235,182 @@ func (m *mockTrackingCacheInternal) Watch(ctx context.Context, user client.Objec
235235
func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates ...predicate.Predicate) source.Source {
236236
return nil
237237
}
238+
239+
func Test_shouldAllowProgressDeadlineExceededUpdate(t *testing.T) {
240+
for _, tc := range []struct {
241+
name string
242+
oldRev *ocv1.ClusterObjectSet
243+
newRev *ocv1.ClusterObjectSet
244+
expected bool
245+
}{
246+
{
247+
name: "allow deletion",
248+
oldRev: &ocv1.ClusterObjectSet{
249+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
250+
Status: ocv1.ClusterObjectSetStatus{
251+
Conditions: []metav1.Condition{
252+
{
253+
Type: ocv1.ClusterObjectSetTypeProgressing,
254+
Status: metav1.ConditionFalse,
255+
Reason: ocv1.ReasonProgressDeadlineExceeded,
256+
},
257+
},
258+
},
259+
},
260+
newRev: &ocv1.ClusterObjectSet{
261+
ObjectMeta: metav1.ObjectMeta{
262+
Generation: 1,
263+
DeletionTimestamp: &metav1.Time{Time: time.Now()},
264+
},
265+
Status: ocv1.ClusterObjectSetStatus{
266+
Conditions: []metav1.Condition{
267+
{
268+
Type: ocv1.ClusterObjectSetTypeProgressing,
269+
Status: metav1.ConditionFalse,
270+
Reason: ocv1.ReasonProgressDeadlineExceeded,
271+
},
272+
},
273+
},
274+
},
275+
expected: true,
276+
},
277+
{
278+
name: "allow when ProgressDeadlineExceeded condition not present",
279+
oldRev: &ocv1.ClusterObjectSet{
280+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
281+
},
282+
newRev: &ocv1.ClusterObjectSet{
283+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
284+
},
285+
expected: true,
286+
},
287+
{
288+
name: "allow when Progressing condition is true",
289+
oldRev: &ocv1.ClusterObjectSet{
290+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
291+
Status: ocv1.ClusterObjectSetStatus{
292+
Conditions: []metav1.Condition{
293+
{
294+
Type: ocv1.ClusterObjectSetTypeProgressing,
295+
Status: metav1.ConditionTrue,
296+
Reason: ocv1.ReasonRollingOut,
297+
},
298+
},
299+
},
300+
},
301+
newRev: &ocv1.ClusterObjectSet{
302+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
303+
Status: ocv1.ClusterObjectSetStatus{
304+
Conditions: []metav1.Condition{
305+
{
306+
Type: ocv1.ClusterObjectSetTypeProgressing,
307+
Status: metav1.ConditionTrue,
308+
Reason: ocv1.ReasonRollingOut,
309+
},
310+
},
311+
},
312+
},
313+
expected: true,
314+
},
315+
{
316+
name: "allow generation change when ProgressDeadlineExceeded",
317+
oldRev: &ocv1.ClusterObjectSet{
318+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
319+
Status: ocv1.ClusterObjectSetStatus{
320+
Conditions: []metav1.Condition{
321+
{
322+
Type: ocv1.ClusterObjectSetTypeProgressing,
323+
Status: metav1.ConditionFalse,
324+
Reason: ocv1.ReasonProgressDeadlineExceeded,
325+
},
326+
},
327+
},
328+
},
329+
newRev: &ocv1.ClusterObjectSet{
330+
ObjectMeta: metav1.ObjectMeta{Generation: 2}, // Generation changed
331+
Status: ocv1.ClusterObjectSetStatus{
332+
Conditions: []metav1.Condition{
333+
{
334+
Type: ocv1.ClusterObjectSetTypeProgressing,
335+
Status: metav1.ConditionFalse,
336+
Reason: ocv1.ReasonProgressDeadlineExceeded,
337+
},
338+
},
339+
},
340+
},
341+
expected: true,
342+
},
343+
{
344+
name: "suppress status-only update when ProgressDeadlineExceeded",
345+
oldRev: &ocv1.ClusterObjectSet{
346+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
347+
Status: ocv1.ClusterObjectSetStatus{
348+
Conditions: []metav1.Condition{
349+
{
350+
Type: ocv1.ClusterObjectSetTypeProgressing,
351+
Status: metav1.ConditionFalse,
352+
Reason: ocv1.ReasonProgressDeadlineExceeded,
353+
},
354+
},
355+
},
356+
},
357+
newRev: &ocv1.ClusterObjectSet{
358+
ObjectMeta: metav1.ObjectMeta{Generation: 1}, // Same generation
359+
Status: ocv1.ClusterObjectSetStatus{
360+
Conditions: []metav1.Condition{
361+
{
362+
Type: ocv1.ClusterObjectSetTypeProgressing,
363+
Status: metav1.ConditionFalse,
364+
Reason: ocv1.ReasonProgressDeadlineExceeded,
365+
},
366+
{
367+
Type: ocv1.ClusterObjectSetTypeAvailable,
368+
Status: metav1.ConditionFalse,
369+
Reason: "SomeReason",
370+
},
371+
},
372+
},
373+
},
374+
expected: false,
375+
},
376+
{
377+
name: "allow spec change via generation bump when ProgressDeadlineExceeded (lifecycleState transition)",
378+
oldRev: &ocv1.ClusterObjectSet{
379+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
380+
Spec: ocv1.ClusterObjectSetSpec{
381+
LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive,
382+
},
383+
Status: ocv1.ClusterObjectSetStatus{
384+
Conditions: []metav1.Condition{
385+
{
386+
Type: ocv1.ClusterObjectSetTypeProgressing,
387+
Status: metav1.ConditionFalse,
388+
Reason: ocv1.ReasonProgressDeadlineExceeded,
389+
},
390+
},
391+
},
392+
},
393+
newRev: &ocv1.ClusterObjectSet{
394+
ObjectMeta: metav1.ObjectMeta{Generation: 2}, // Generation bumped due to spec change
395+
Spec: ocv1.ClusterObjectSetSpec{
396+
LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived,
397+
},
398+
Status: ocv1.ClusterObjectSetStatus{
399+
Conditions: []metav1.Condition{
400+
{
401+
Type: ocv1.ClusterObjectSetTypeProgressing,
402+
Status: metav1.ConditionFalse,
403+
Reason: ocv1.ReasonProgressDeadlineExceeded,
404+
},
405+
},
406+
},
407+
},
408+
expected: true,
409+
},
410+
} {
411+
t.Run(tc.name, func(t *testing.T) {
412+
result := shouldAllowProgressDeadlineExceededUpdate(tc.oldRev, tc.newRev)
413+
require.Equal(t, tc.expected, result)
414+
})
415+
}
416+
}

0 commit comments

Comments
 (0)