Skip to content

Draft: Add UpToDate condition for managed resources #942

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
bobh66 wants to merge 1 commit into
base: main
Choose a base branch
from
+119 −32
Open

Draft: Add UpToDate condition for managed resources #942

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
69 changes: 68 additions & 1 deletion apis/common/condition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ const (
// as a system condition. See the tracking issue for more details
// https://github.com/crossplane/crossplane/issues/5643.
TypeHealthy ConditionType = "Healthy"

// TypeUpToDate resources are believed to accurately represent the remote resource state.
// The condition is controlled by the ResourceUpToDate return value from the Observe method.
TypeUpToDate ConditionType = "UpToDate"
)

// A ConditionReason represents the reason a resource is in a condition.
Expand All @@ -65,6 +69,14 @@ const (
ReasonReconcilePaused ConditionReason = "ReconcilePaused"
)

// Reasons a resource is or is not up to date.
const (
ReasonUpdateRestricted ConditionReason = "UpdateRestricted"
ReasonObserveMatch ConditionReason = "ObserveMatch"
ReasonUpdateFailed ConditionReason = "UpdateFailed"
ReasonUpdateRequested ConditionReason = "UpdateRequested"
)

// See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

// A Condition that may apply to a resource.
Expand Down Expand Up @@ -130,7 +142,7 @@ func (c Condition) WithObservedGeneration(gen int64) Condition {
// Crossplane system (e.g, Ready, Synced, Healthy).
func IsSystemConditionType(t ConditionType) bool {
switch t {
case TypeReady, TypeSynced, TypeHealthy:
case TypeReady, TypeSynced, TypeHealthy, TypeUpToDate:
return true
}

Expand Down Expand Up @@ -312,3 +324,58 @@ func ReconcilePaused() Condition {
Reason: ReasonReconcilePaused,
}
}

// ObserveMatch returns a condition that indicates the Observe
// method returned true.
func ObserveMatch() Condition {
return Condition{
Type: TypeUpToDate,
Status: corev1.ConditionTrue,
LastTransitionTime: metav1.Now(),
Reason: ReasonObserveMatch,
}
}

// UpdateRequested returns a condition that indicates the Observe
// method returned false and the Update method was called successfully.
func UpdateRequested() Condition {
return Condition{
Type: TypeUpToDate,
Status: corev1.ConditionFalse,
LastTransitionTime: metav1.Now(),
Reason: ReasonUpdateRequested,
}
}

// UpdateRestricted returns a condition that indicates the Observe
// method returned false and the Update method is not allowed.
func UpdateRestricted() Condition {
return Condition{
Type: TypeUpToDate,
Status: corev1.ConditionFalse,
LastTransitionTime: metav1.Now(),
Reason: ReasonUpdateRestricted,
}
}

// UpdateFailed returns a condition that indicates the Observe
// method returned false and the Update method was unsuccessful in synchronizing the resource state.
func UpdateFailed() Condition {
return Condition{
Type: TypeUpToDate,
Status: corev1.ConditionFalse,
LastTransitionTime: metav1.Now(),
Reason: ReasonUpdateFailed,
}
}

// PausedUnknown returns a condition that indicates the Observe
// method was not called because reconciliation is paused so the status is unknown.
func PausedUnknown() Condition {
return Condition{
Type: TypeUpToDate,
Status: corev1.ConditionUnknown,
LastTransitionTime: metav1.Now(),
Reason: ReasonReconcilePaused,
}
}
20 changes: 20 additions & 0 deletions apis/common/v1/condition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,23 @@ func ReconcileError(err error) Condition {
func ReconcilePaused() Condition {
return common.ReconcilePaused()
}

// ObserveMatch returns a condition indicating that the managed resource desired state matches
// the external resource state.
func ObserveMatch() Condition { return common.ObserveMatch() }

// PausedUnknown returns a condition that indicates reconciliation is paused and it is unknown if the desired
// state of the managed resource matches the external resource state.
func PausedUnknown() Condition { return common.PausedUnknown() }

// UpdateRequested returns a condition that indicates a mismatch between the managed resource and the
// external resource was detected and an update of the external resource has been requested.
func UpdateRequested() Condition { return common.UpdateRequested() }

// UpdateFailed returns a condition that indicates a mismatch between the managed resource and the
// external resource was detected and an updated of the external resource was attempted and failed.
func UpdateFailed() Condition { return common.UpdateFailed() }

// UpdateRestricted returns a condition that indicates a mismatch between the managed resource and the
// external resource was detected but an Update was not attempted due to managementPolicy restrictions.
func UpdateRestricted() Condition { return common.UpdateRestricted() }
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 @@ -939,7 +939,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(xpv1.ReconcilePaused())
status.MarkConditions(xpv1.ReconcilePaused(), xpv1.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)
Expand Down Expand Up @@ -1436,7 +1436,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
// https://github.com/crossplane/crossplane/issues/289
reconcileAfter := r.pollIntervalHook(managed, r.pollInterval)
log.Debug("External resource is up to date", "requeue-after", time.Now().Add(reconcileAfter))
status.MarkConditions(xpv1.ReconcileSuccess())
status.MarkConditions(xpv1.ReconcileSuccess(), xpv1.ObserveMatch())
r.metricRecorder.recordFirstTimeReady(managed)

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

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

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

return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}
Expand Down Expand Up @@ -1506,7 +1506,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
reconcileAfter := r.pollIntervalHook(managed, r.pollInterval)
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(xpv1.ReconcileSuccess())
status.MarkConditions(xpv1.ReconcileSuccess(), xpv1.UpdateRequested().WithMessage(observation.Diff))

return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}
26 changes: 13 additions & 13 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(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.ObserveMatch().WithObservedGeneration(42))
Copy link

@jcogilvie jcogilvie Mar 16, 2026

Choose a reason for hiding this comment

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

It doesn't look like we ever assert on the diff message in any of these tests -- could/should we?

Copy link
Contributor Author

@bobh66 bobh66 Mar 17, 2026

Choose a reason for hiding this comment

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

Probably should for the three cases where it can happen. I'll have to look into how we can inject specific diffs, since it isn't finding any now.


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(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.ObserveMatch().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(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate)).WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate)).WithObservedGeneration(42), xpv1.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(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.UpdateRequested().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
Expand Down Expand Up @@ -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(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.UpdateRequested().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
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(xpv1.ReconcilePaused().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcilePaused().WithObservedGeneration(42), xpv1.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(xpv1.ManagementPolicies{})
want.SetConditions(xpv1.ReconcilePaused().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcilePaused().WithObservedGeneration(42), xpv1.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(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.ObserveMatch().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(xpv1.ManagementPolicies{xpv1.ManagementActionObserve})
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42), xpv1.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(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete})
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.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(xpv1.ManagementPolicies{xpv1.ManagementActionAll})
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42), xpv1.UpdateRequested().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
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(xpv1.ManagementPolicies{xpv1.ManagementActionAll})
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.UpdateRequested().WithObservedGeneration(42))

if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
reason := "A successful managed resource update should be reported as a conditioned status."
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(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionUpdate, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete})
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42), xpv1.ObserveMatch().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