Skip to content

🌱 Set Progressing to Succeeded on ClusterExtensionRevision only after availability probes pass #2524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pedjak wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −16
Open

🌱 Set Progressing to Succeeded on ClusterExtensionRevision only after availability probes pass #2524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 10 additions & 12 deletions internal/operator-controller/controllers/clusterextensionrevision_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"errors"
"fmt"
"strings"
"sync"
"time"

appsv1 "k8s.io/api/apps/v1"
Expand All @@ -20,6 +19,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/clock"
"pkg.package-operator.run/boxcutter"
"pkg.package-operator.run/boxcutter/machinery"
machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
Expand Down Expand Up @@ -48,9 +48,7 @@ type ClusterExtensionRevisionReconciler struct {
Client client.Client
RevisionEngineFactory RevisionEngineFactory
TrackingCache trackingCache
// track if we have queued up the reconciliation that detects eventual progress deadline issues
// keys is revision UUID, value is boolean
progressDeadlineCheckInFlight sync.Map
Clock clock.Clock
}

type trackingCache interface {
Expand Down Expand Up @@ -86,15 +84,15 @@ func (c *ClusterExtensionRevisionReconciler) Reconcile(ctx context.Context, req
// check if we reached the progress deadline only if the revision is still progressing and has not succeeded yet
if isStillProgressing && !succeeded {
timeout := time.Duration(pd) * time.Minute
if time.Since(existingRev.CreationTimestamp.Time) > timeout {
if c.Clock.Since(existingRev.CreationTimestamp.Time) > timeout {
// progress deadline reached, reset any errors and stop reconciling this revision
markAsNotProgressing(reconciledRev, ocv1.ReasonProgressDeadlineExceeded, fmt.Sprintf("Revision has not rolled out for %d minutes.", pd))
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message uses "minutes" which is always plural, but when the progress deadline is 1 minute, it should say "minute" (singular). Consider using a conditional or pluralization helper to make the message grammatically correct for all values. For example: fmt.Sprintf("Revision has not rolled out for %d minute(s).", pd) or a more sophisticated approach that checks if pd == 1.

Suggested change
markAsNotProgressing(reconciledRev, ocv1.ReasonProgressDeadlineExceeded, fmt.Sprintf("Revision has not rolled out for %d minutes.", pd))
unit := "minutes"
if pd == 1 {
unit = "minute"
}
markAsNotProgressing(reconciledRev, ocv1.ReasonProgressDeadlineExceeded, fmt.Sprintf("Revision has not rolled out for %d %s.", pd, unit))

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@perdasilva perdasilva Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'minute(s)' might also be acceptable here

reconcileErr = nil
res = ctrl.Result{}
} else if _, found := c.progressDeadlineCheckInFlight.Load(existingRev.GetUID()); !found && reconcileErr == nil {
// if we haven't already queued up a reconcile to check for progress deadline, queue one up, but only once
c.progressDeadlineCheckInFlight.Store(existingRev.GetUID(), true)
res = ctrl.Result{RequeueAfter: timeout}
} else if reconcileErr == nil {
requeueAfter := existingRev.CreationTimestamp.Time.Add(timeout).Add(2 * time.Second).Sub(c.Clock.Now()).Round(time.Second)
Copy link
Contributor

@perdasilva perdasilva Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the reason for the +2 seconds?

Copy link
Contributor Author

@pedjak pedjak Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the reason for the +2 seconds?

Requeing is not exact time-wise, so we can land here for the second time if we do not add a bit of drift. With drift we ensure that the next reconciliation is going to detect the deadline has exceeded. We could experiment with adding just 1 sec, if that would your preference, but I think it is safer to have larger drift. An alternative would be to drop Add(2 * time.Second), but then we need to add a check:

if requeueAfter == 0 {
  requeueafter = 1 * time.Second
}

wdyt?

Copy link
Contributor

@perdasilva perdasilva Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with keeping it as is - maybe we could just add a comment to call this out as it might trip up the next person? Maybe also move the drift to a const to add additional context with the constant's name?

l.Info(fmt.Sprintf("ProgressDeadline not exceeded, requeue after ~%v to check again.", requeueAfter))
res = ctrl.Result{RequeueAfter: requeueAfter}
}
}
}
Expand Down Expand Up @@ -208,9 +206,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
}

revVersion := cer.GetAnnotations()[labels.BundleVersionKey]
if !rres.InTransition() {
markAsProgressing(cer, ocv1.ReasonSucceeded, fmt.Sprintf("Revision %s has rolled out.", revVersion))
} else {
if rres.InTransition() {
markAsProgressing(cer, ocv1.ReasonRollingOut, fmt.Sprintf("Revision %s is rolling out.", revVersion))
}

Expand All @@ -231,6 +227,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
}
}

markAsProgressing(cer, ocv1.ReasonSucceeded, fmt.Sprintf("Revision %s has rolled out.", revVersion))
markAsAvailable(cer, ocv1.ClusterExtensionRevisionReasonProbesSucceeded, "Objects are available and pass all probes.")

// We'll probably only want to remove this once we are done updating the ClusterExtension conditions
Expand Down Expand Up @@ -333,6 +330,7 @@ func (c *ClusterExtensionRevisionReconciler) SetupWithManager(mgr ctrl.Manager)
return true
},
}
c.Clock = clock.RealClock{}
return ctrl.NewControllerManagedBy(mgr).
For(
&ocv1.ClusterExtensionRevision{},
Expand Down
16 changes: 12 additions & 4 deletions internal/operator-controller/controllers/clusterextensionrevision_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/clock"
clocktesting "k8s.io/utils/clock/testing"
"pkg.package-operator.run/boxcutter"
"pkg.package-operator.run/boxcutter/machinery"
machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
Expand Down Expand Up @@ -372,6 +374,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionReconciliation(t
name: "set Progressing:True:Succeeded once transition rollout is finished",
revisionResult: mockRevisionResult{
inTransition: false,
isComplete: true,
},
reconcilingRevisionName: clusterExtensionRevisionName,
existingObjs: func() []client.Object {
Expand All @@ -380,7 +383,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionReconciliation(t
meta.SetStatusCondition(&rev1.Status.Conditions, metav1.Condition{
Type: ocv1.TypeProgressing,
Status: metav1.ConditionTrue,
Reason: ocv1.ReasonSucceeded,
Reason: ocv1.ReasonRollingOut,
Message: "Revision 1.0.0 is rolling out.",
ObservedGeneration: 1,
})
Expand Down Expand Up @@ -945,16 +948,19 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ProgressDeadline(t *testi
validate func(*testing.T, client.Client)
reconcileErr error
reconcileResult ctrl.Result
clock clock.Clock
}{
{
name: "progressing set to false when progress deadline is exceeded",
existingObjs: func() []client.Object {
ext := newTestClusterExtension()
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
rev1.Spec.ProgressDeadlineMinutes = 1
rev1.CreationTimestamp = metav1.NewTime(time.Now().Add(-61 * time.Second))
rev1.CreationTimestamp = metav1.NewTime(time.Date(2022, 1, 1, 0, 0, 0, 0, time.UTC))
return []client.Object{rev1, ext}
},
// 61sec elapsed since the creation of the revision
clock: clocktesting.NewFakeClock(time.Date(2022, 1, 1, 0, 1, 1, 0, time.UTC)),
revisionResult: &mockRevisionResult{
inTransition: true,
},
Expand All @@ -975,13 +981,14 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ProgressDeadline(t *testi
ext := newTestClusterExtension()
rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
rev1.Spec.ProgressDeadlineMinutes = 1
rev1.CreationTimestamp = metav1.NewTime(time.Now())
rev1.CreationTimestamp = metav1.NewTime(time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC))
return []client.Object{rev1, ext}
},
clock: clocktesting.NewFakeClock(time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC)),
revisionResult: &mockRevisionResult{
inTransition: true,
},
reconcileResult: ctrl.Result{RequeueAfter: 1 * time.Minute},
reconcileResult: ctrl.Result{RequeueAfter: 62 * time.Second},
validate: func(t *testing.T, c client.Client) {
rev := &ocv1.ClusterExtensionRevision{}
err := c.Get(t.Context(), client.ObjectKey{
Expand Down Expand Up @@ -1049,6 +1056,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ProgressDeadline(t *testi
TrackingCache: &mockTrackingCache{
client: testClient,
},
Clock: tc.clock,
}).Reconcile(t.Context(), ctrl.Request{
NamespacedName: types.NamespacedName{
Name: clusterExtensionRevisionName,
Expand Down
33 changes: 33 additions & 0 deletions test/e2e/features/install.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,39 @@ Feature: Install ClusterExtension
invalid ClusterExtension configuration: invalid configuration: unknown field "watchNamespace"
"""

@BoxcutterRuntime
@ProgressDeadline
Scenario: Report ClusterExtension as not progressing if the rollout does not become available within given timeout
Given min value for ClusterExtension .spec.progressDeadlineMinutes is set to 1
And min value for ClusterExtensionRevision .spec.progressDeadlineMinutes is set to 1
When ClusterExtension is applied
"""
apiVersion: olm.operatorframework.io/v1
kind: ClusterExtension
metadata:
name: ${NAME}
spec:
namespace: ${TEST_NAMESPACE}
progressDeadlineMinutes: 1
serviceAccount:
name: olm-sa
source:
sourceType: Catalog
catalog:
packageName: test
# bundle refers bad image references, so that the deployment never becomes available
version: 1.0.2
selector:
matchLabels:
"olm.operatorframework.io/metadata.name": test-catalog
"""
Then ClusterExtensionRevision "${NAME}-1" reports Progressing as False with Reason ProgressDeadlineExceeded
And ClusterExtension reports Progressing as False with Reason ProgressDeadlineExceeded and Message:
Copy link
Contributor

@perdasilva perdasilva Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not for this PR, but I wonder if we should call it availabilityDeadline or revisionRolloutDeadline or something like that. If the rollout has moved from one phase to another, it has made progress.

"""
Revision has not rolled out for 1 minutes.
"""
And ClusterExtension reports Progressing transition between 1 and 2 minutes since its creation

@BoxcutterRuntime
@ProgressDeadline
Scenario: Report ClusterExtension as not progressing if the rollout does not complete within given timeout
Expand Down
Loading