Skip to content

Implement UpToDate condition in managed reconciler #945

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

Draft
bobh66 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Implement UpToDate condition in managed reconciler #945

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 pkg/reconciler/managed/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,7 +972,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
log.Debug("Reconciliation is paused either through the `spec.managementPolicies` or the pause annotation", "annotation", meta.AnnotationKeyReconciliationPaused)
record.Event(managed, event.Normal(reasonReconciliationPaused, "Reconciliation is paused either through the `spec.managementPolicies` or the pause annotation",
"annotation", meta.AnnotationKeyReconciliationPaused))
status.MarkConditions(xpv2.ReconcilePaused())
status.MarkConditions(xpv2.ReconcilePaused(), xpv2.PausedUnknown())
// if the pause annotation is removed or the management policies changed, we will have a chance to reconcile
// again and resume and if status update fails, we will reconcile again to retry to update the status
return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
Comment on lines +975 to 978

@coderabbitai coderabbitai Bot Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Could we prevent stale UpToDate conditions on the other error paths too?

Thanks for wiring this in. Right now the new condition is only updated in these branches, so any pre-observe error or a later failure after a fresh Observe() can leave the previous UpToDate condition on the object. That can make users see an old ObserveMatched/Update* result even though the latest reconcile never reached that conclusion. Would it make sense to compute a fresh UpToDate condition once per reconcile and include it in every subsequent status update, or explicitly mark it Unknown when observe never completes?

As per coding guidelines, **/pkg/reconciler/**: "Conditions must be actionable for users (not developers), stable/deterministic, with proper Type/Reason/Message format."

Also applies to: 1439-1440, 1459-1459, 1478-1478, 1509-1509

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/reconciler/managed/reconciler.go` around lines 942 - 945, The new
UpToDate condition is only set in some branches (via
status.MarkConditions(xpv2.ReconcilePaused(), xpv2.PausedUnknown()) then
r.client.Status().Update) leaving stale UpToDate state on other error paths;
update the reconcile flow so a fresh UpToDate condition is computed and applied
on every status update path (including pre-Observe errors and post-Observe
failures) — e.g., derive a deterministic UpToDate condition before returning
from Reconcile functions (where Observe() is called) and ensure every call that
uses r.client.Status().Update (including the branches around Observe(),
Create/Update/Delete error returns referenced near status.MarkConditions and the
other noted locations) includes that computed condition (or sets UpToDate to
Unknown with an appropriate reason/message when Observe did not complete) so
status is always consistent for users.

Expand Down Expand Up @@ -1496,7 +1496,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
// https://github.com/crossplane/crossplane/issues/289
reconcileAfter := r.pollIntervalHook(managed, r.effectivePollInterval(managed))
log.Debug("External resource is up to date", "requeue-after", time.Now().Add(reconcileAfter))
status.MarkConditions(xpv2.ReconcileSuccess())
status.MarkConditions(xpv2.ReconcileSuccess(), xpv2.ObserveMatched())
r.metricRecorder.recordFirstTimeReady(managed)

// record that we intentionally did not update the managed resource
Expand All @@ -1516,7 +1516,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
if !policy.ShouldUpdate() {
reconcileAfter := r.pollIntervalHook(managed, r.effectivePollInterval(managed))
log.Debug("Skipping update due to managementPolicies. Reconciliation succeeded", "requeue-after", time.Now().Add(reconcileAfter))
status.MarkConditions(xpv2.ReconcileSuccess())
status.MarkConditions(xpv2.ReconcileSuccess(), xpv2.UpdateRestricted().WithMessage(observation.Diff))
Comment thread
bobh66 marked this conversation as resolved.

@jbw976 jbw976 Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what are we thinking for the size of these diffs and limits on k8s conditions fields? i think metav1.Condition.Message has a limit of 32KB: https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/types.go#L1664

should we consider truncating this diff to be safe?

@bobh66 bobh66 Apr 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably a good idea.


return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
}
Expand All @@ -1535,7 +1535,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
}

record.Event(managed, event.Warning(reasonCannotUpdate, err))
status.MarkConditions(xpv2.ReconcileError(errors.Wrap(err, errReconcileUpdate)))
status.MarkConditions(xpv2.ReconcileError(errors.Wrap(err, errReconcileUpdate)), xpv2.UpdateFailed().WithMessage(observation.Diff))

return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
}
Expand Down Expand Up @@ -1566,7 +1566,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
reconcileAfter := r.pollIntervalHook(managed, r.effectivePollInterval(managed))
log.Debug("Successfully requested update of external resource", "requeue-after", time.Now().Add(reconcileAfter))
record.Event(managed, event.Normal(reasonUpdated, "Successfully requested update of external resource"))
status.MarkConditions(xpv2.ReconcileSuccess())
status.MarkConditions(xpv2.ReconcileSuccess(), xpv2.UpdateRequested().WithMessage(observation.Diff))

return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
}
34 changes: 17 additions & 17 deletions pkg/reconciler/managed/reconciler_legacy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ func TestReconciler(t *testing.T) {
MockGet: legacyManagedMockGetFn(nil, 42),
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.ObserveMatched().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful no-op reconcile should be reported as a conditioned status."
Expand Down Expand Up @@ -1088,7 +1088,7 @@ func TestReconciler(t *testing.T) {
MockGet: legacyManagedMockGetFn(nil, 42),
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.ObserveMatched().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful no-op reconcile should be reported as a conditioned status."
Expand Down Expand Up @@ -1253,7 +1253,7 @@ func TestReconciler(t *testing.T) {
MockGet: legacyManagedMockGetFn(nil, 42),
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetConditions(xpv2.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate)).WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate)).WithObservedGeneration(42), xpv2.UpdateFailed().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "Errors while updating an external resource should be reported as a conditioned status."
Expand Down Expand Up @@ -1354,7 +1354,7 @@ func TestReconciler(t *testing.T) {
MockGet: legacyManagedMockGetFn(nil, 42),
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.UpdateRequested().WithObservedGeneration(42).WithMessage("this is a diff"))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
Expand All @@ -1373,7 +1373,7 @@ func TestReconciler(t *testing.T) {
WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "this is a diff"}, nil
},
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
Expand All @@ -1398,7 +1398,7 @@ func TestReconciler(t *testing.T) {
MockGet: legacyManagedMockGetFn(nil, 42),
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.UpdateRequested().WithObservedGeneration(42).WithMessage("this is a diff"))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
Expand All @@ -1417,7 +1417,7 @@ func TestReconciler(t *testing.T) {
WithTypedExternalConnector(TypedExternalConnectorFn[*fake.LegacyManaged](func(_ context.Context, _ *fake.LegacyManaged) (TypedExternalClient[*fake.LegacyManaged], error) {
c := &TypedExternalClientFns[*fake.LegacyManaged]{
ObserveFn: func(_ context.Context, _ *fake.LegacyManaged) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "this is a diff"}, nil
},
UpdateFn: func(_ context.Context, _ *fake.LegacyManaged) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
Expand Down Expand Up @@ -1450,7 +1450,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetAnnotations(map[string]string{meta.AnnotationKeyReconciliationPaused: "true"})
want.SetConditions(xpv2.ReconcilePaused().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcilePaused().WithObservedGeneration(42), xpv2.PausedUnknown().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := `If managed resource has the pause annotation with value "true", it should acquire "Synced" status condition with the status "False" and the reason "ReconcilePaused".`
Expand Down Expand Up @@ -1480,7 +1480,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetManagementPolicies(xpv2.ManagementPolicies{})
want.SetConditions(xpv2.ReconcilePaused().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcilePaused().WithObservedGeneration(42), xpv2.PausedUnknown().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := `If managed resource has the pause annotation with value "true", it should acquire "Synced" status condition with the status "False" and the reason "ReconcilePaused".`
Expand Down Expand Up @@ -1516,7 +1516,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetAnnotations(map[string]string{meta.AnnotationKeyReconciliationPaused: "false"})
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.ObserveMatched().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := `Managed resource should acquire Synced=False/ReconcileSuccess status condition after a resume.`
Expand Down Expand Up @@ -1778,7 +1778,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve})
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42), xpv2.UpdateRestricted().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "With ObserveOnly, a successful managed resource observation should be reported as a conditioned status."
Expand Down Expand Up @@ -1915,7 +1915,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve, xpv2.ManagementActionLateInitialize, xpv2.ManagementActionCreate, xpv2.ManagementActionDelete})
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.UpdateRestricted().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := `Managed resource should acquire Synced=False/ReconcileSuccess status condition.`
Expand Down Expand Up @@ -1966,7 +1966,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll})
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42), xpv2.UpdateRequested().WithObservedGeneration(42).WithMessage("this is a diff"))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
Expand All @@ -1986,7 +1986,7 @@ func TestReconciler(t *testing.T) {
WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "this is a diff"}, nil
},
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
Expand Down Expand Up @@ -2017,7 +2017,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionAll})
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.UpdateRequested().WithObservedGeneration(42).WithMessage("this is a diff"))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
Expand All @@ -2037,7 +2037,7 @@ func TestReconciler(t *testing.T) {
WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil
return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "this is a diff"}, nil
},
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
Expand Down Expand Up @@ -2069,7 +2069,7 @@ func TestReconciler(t *testing.T) {
MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
want := newLegacyManaged(42)
want.SetManagementPolicies(xpv2.ManagementPolicies{xpv2.ManagementActionObserve, xpv2.ManagementActionUpdate, xpv2.ManagementActionCreate, xpv2.ManagementActionDelete})
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.ObserveMatched().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "Errors updating a managed resource should be reported as a conditioned status."
Expand Down
Loading
Loading