Skip to content

🌱 CER: centralize status updates into big-R Reconcile method #2200

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

Merged
merged 1 commit into from
Sep 12, 2025
+81 −24
Merged

🌱 CER: centralize status updates into big-R Reconcile method #2200

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
10 changes: 5 additions & 5 deletions internal/operator-controller/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,14 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
l := log.FromContext(ctx).WithName("cluster-extension")
ctx = log.IntoContext(ctx, l)

l.Info("reconcile starting")
defer l.Info("reconcile ending")

existingExt := &ocv1.ClusterExtension{}
if err := r.Get(ctx, req.NamespacedName, existingExt); err != nil {
return ctrl.Result{}, client.IgnoreNotFound(err)
}

l.Info("reconcile starting")
defer l.Info("reconcile ending")

reconciledExt := existingExt.DeepCopy()
res, reconcileErr := r.reconcile(ctx, reconciledExt)

Expand All @@ -124,7 +124,7 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers)

// If any unexpected fields have changed, panic before updating the resource
unexpectedFieldsChanged := checkForUnexpectedFieldChange(*existingExt, *reconciledExt)
unexpectedFieldsChanged := checkForUnexpectedClusterExtensionFieldChange(*existingExt, *reconciledExt)
if unexpectedFieldsChanged {
panic("spec or metadata changed by reconciler")
}
Expand Down Expand Up @@ -169,7 +169,7 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C
}

// Compare resources - ignoring status & metadata.finalizers
func checkForUnexpectedFieldChange(a, b ocv1.ClusterExtension) bool {
func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) bool {
a.Status, b.Status = ocv1.ClusterExtensionStatus{}, ocv1.ClusterExtensionStatus{}
a.Finalizers, b.Finalizers = []string{}, []string{}
return !equality.Semantic.DeepEqual(a, b)
Expand Down
95 changes: 76 additions & 19 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 @@ -12,6 +12,7 @@ import (

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -67,16 +68,48 @@ func (c *ClusterExtensionRevisionReconciler) Reconcile(ctx context.Context, req
l := log.FromContext(ctx).WithName("cluster-extension-revision")
ctx = log.IntoContext(ctx, l)

rev := &ocv1.ClusterExtensionRevision{}
if err := c.Client.Get(ctx, req.NamespacedName, rev); err != nil {
existingRev := &ocv1.ClusterExtensionRevision{}
if err := c.Client.Get(ctx, req.NamespacedName, existingRev); err != nil {
return ctrl.Result{}, client.IgnoreNotFound(err)
}

l = l.WithValues("key", req.String())
l.Info("reconcile starting")
defer l.Info("reconcile ending")

return c.reconcile(ctx, rev)
reconciledRev := existingRev.DeepCopy()
res, reconcileErr := c.reconcile(ctx, reconciledRev)

// Do checks before any Update()s, as Update() may modify the resource structure!
updateStatus := !equality.Semantic.DeepEqual(existingRev.Status, reconciledRev.Status)

unexpectedFieldsChanged := checkForUnexpectedClusterExtensionRevisionFieldChange(*existingRev, *reconciledRev)
if unexpectedFieldsChanged {
panic("spec or metadata changed by reconciler")
}

// NOTE: finalizer updates are performed during c.reconcile as patches, so that reconcile can
// continue performing logic after successfully setting the finalizer. therefore we only need
// to set status here.

if updateStatus {
if err := c.Client.Status().Update(ctx, reconciledRev); err != nil {
reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err))
}
}

return res, reconcileErr
}

// Compare resources - ignoring status & metadata.finalizers
func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b ocv1.ClusterExtensionRevision) bool {
a.Status, b.Status = ocv1.ClusterExtensionRevisionStatus{}, ocv1.ClusterExtensionRevisionStatus{}

// when finalizers are updated during reconcile, we expect finalizers, managedFields, and resourceVersion
// to be updated, so we ignore changes in these fields.
a.Finalizers, b.Finalizers = []string{}, []string{}
a.ManagedFields, b.ManagedFields = nil, nil
a.ResourceVersion, b.ResourceVersion = "", ""
return !equality.Semantic.DeepEqual(a.Spec, b.Spec)
}

func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
Expand All @@ -98,11 +131,14 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
Message: err.Error(),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{}, fmt.Errorf("revision teardown: %w", errors.Join(err, c.Client.Status().Update(ctx, rev)))
return ctrl.Result{}, fmt.Errorf("revision teardown: %v", err)
}

l.Info("teardown report", "report", tres.String())
if !tres.IsComplete() {
// TODO: If it is not complete, it seems like it would be good to update
// the status in some way to tell the user that the teardown is still
// in progress.
return ctrl.Result{}, nil
}

Expand All @@ -114,9 +150,19 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
Message: err.Error(),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{}, fmt.Errorf("free cache informers: %w", errors.Join(err, c.Client.Status().Update(ctx, rev)))
return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err)
}
return ctrl.Result{}, c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer)
if err := c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
Type: "Available",
Status: metav1.ConditionFalse,
Reason: "ReconcileFailure",
Message: err.Error(),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err)
}
return ctrl.Result{}, nil
Comment on lines +155 to +165
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems semantically wrong to mark a Revision as "Available" == False for any issue with finalizers.

In general I would not report any internal error as a Status Condition.
If we want to show individual errors to users we should publish events on the resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally agree that this is likely the wrong type for this. I would expect to come back and clean this up prior to GA once there is a formal design on status handling.

In the meantime, I do think it is an important UX concern to tell the user something in a place they are likely to see it.

If they've deleted a CER (or an agent has on their behalf), but the finalizer isn't being removed for some reason, it would be a bad to UX for the CER to just... have no update. It would look like we just aren't reconciling it.

}

//
Expand All @@ -130,8 +176,9 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
Message: err.Error(),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{}, fmt.Errorf("ensure finalizer: %w", errors.Join(err, c.Client.Status().Update(ctx, rev)))
return ctrl.Result{}, fmt.Errorf("error ensuring teardown finalizer: %v", err)
}

if err := c.establishWatch(ctx, rev, revision); err != nil {
meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
Type: ocv1.ClusterExtensionRevisionTypeAvailable,
Expand All @@ -140,8 +187,9 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
Message: err.Error(),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{}, fmt.Errorf("establish watch: %w", errors.Join(err, c.Client.Status().Update(ctx, rev)))
return ctrl.Result{}, fmt.Errorf("establish watch: %v", err)
}

rres, err := c.RevisionEngine.Reconcile(ctx, *revision, opts...)
if err != nil {
meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
Expand All @@ -151,60 +199,69 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
Message: err.Error(),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{}, fmt.Errorf("revision reconcile: %w", errors.Join(err, c.Client.Status().Update(ctx, rev)))
return ctrl.Result{}, fmt.Errorf("revision reconcile: %v", err)
}
l.Info("reconcile report", "report", rres.String())

// Retry failing preflight checks with a flat 10s retry.
// TODO: report status, backoff?
if verr := rres.GetValidationError(); verr != nil {
l.Info("preflight error, retrying after 10s", "err", verr.String())

meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
Type: ocv1.ClusterExtensionRevisionTypeAvailable,
Status: metav1.ConditionFalse,
Reason: ocv1.ClusterExtensionRevisionReasonRevisionValidationFailure,
Message: fmt.Sprintf("revision validation error: %s", verr),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{RequeueAfter: 10 * time.Second}, c.Client.Status().Update(ctx, rev)
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

for i, pres := range rres.GetPhases() {
if verr := pres.GetValidationError(); verr != nil {
l.Info("preflight error, retrying after 10s", "err", verr.String())

meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
Type: ocv1.ClusterExtensionRevisionTypeAvailable,
Status: metav1.ConditionFalse,
Reason: ocv1.ClusterExtensionRevisionReasonPhaseValidationError,
Message: fmt.Sprintf("phase %d validation error: %s", i, verr),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{RequeueAfter: 10 * time.Second}, c.Client.Status().Update(ctx, rev)
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

var collidingObjs []string
for _, ores := range pres.GetObjects() {
if ores.Action() == machinery.ActionCollision {
collidingObjs = append(collidingObjs, ores.String())
}
}

if len(collidingObjs) > 0 {
l.Info("object collision error, retrying after 10s", "collisions", collidingObjs)

meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
Type: ocv1.ClusterExtensionRevisionTypeAvailable,
Status: metav1.ConditionFalse,
Reason: ocv1.ClusterExtensionRevisionReasonObjectCollisions,
Message: fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")),
ObservedGeneration: rev.Generation,
})
return ctrl.Result{RequeueAfter: 10 * time.Second}, c.Client.Status().Update(ctx, rev)
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
}

//nolint:nestif
if rres.IsComplete() {
// Archive other revisions.
for _, a := range previous {
if err := c.Client.Patch(ctx, a, client.RawPatch(
types.MergePatchType, []byte(`{"spec":{"lifecycleState":"Archived"}}`))); err != nil {
patch := []byte(`{"spec":{"lifecycleState":"Archived"}}`)
if err := c.Client.Patch(ctx, a, client.RawPatch(types.MergePatchType, patch)); err != nil {
// TODO: It feels like an error here needs to propagate to a status _somewhere_.
// Not sure the current CER makes sense? But it also feels off to set the CE
// status from outside the CE reconciler.
return ctrl.Result{}, fmt.Errorf("archive previous Revision: %w", err)
}
}
Expand Down Expand Up @@ -278,7 +335,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
meta.RemoveStatusCondition(&rev.Status.Conditions, ocv1.TypeProgressing)
}

return ctrl.Result{}, c.Client.Status().Update(ctx, rev)
return ctrl.Result{}, nil
}

type Sourcerer interface {
Expand Down Expand Up @@ -408,7 +465,7 @@ func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revisio
for _, specPhase := range rev.Spec.Phases {
phase := boxcutter.Phase{Name: specPhase.Name}
for _, specObj := range specPhase.Objects {
obj := specObj.Object
obj := specObj.Object.DeepCopy()

labels := obj.GetLabels()
if labels == nil {
Expand All @@ -420,10 +477,10 @@ func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revisio
switch specObj.CollisionProtection {
case ocv1.CollisionProtectionIfNoController, ocv1.CollisionProtectionNone:
opts = append(opts, boxcutter.WithObjectReconcileOptions(
&obj, boxcutter.WithCollisionProtection(specObj.CollisionProtection)))
obj, boxcutter.WithCollisionProtection(specObj.CollisionProtection)))
}

phase.Objects = append(phase.Objects, obj)
phase.Objects = append(phase.Objects, *obj)
}
r.Phases = append(r.Phases, phase)
}
Expand Down
Loading