Implement UpToDate condition in managed reconciler#945
Implement UpToDate condition in managed reconciler#945bobh66 wants to merge 4 commits intocrossplane:mainfrom
Conversation
📝 WalkthroughWalkthroughThe reconciler now marks enriched status conditions alongside base reconcile states. Each reconciliation outcome (paused, success, failure, update requested/restricted) is accompanied by a specific condition reflecting its details. Test expectations are updated to validate these enhanced conditions across scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/reconciler/managed/reconciler_modern_test.go (2)
1787-1787: Please remove duplicated.WithObservedGeneration(42)chaining.Tiny cleanup, but this looks accidental and adds noise during future maintenance.
Proposed cleanup
- want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42), xpv2.UpdateRestricted().WithObservedGeneration(42)) + want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.UpdateRestricted().WithObservedGeneration(42)) - want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42), xpv2.UpdateRequested().WithObservedGeneration(42).WithMessage("this is a diff")) + want.SetConditions(xpv2.ReconcileSuccess().WithObservedGeneration(42), xpv2.UpdateRequested().WithObservedGeneration(42).WithMessage("this is a diff"))Also applies to: 1976-1976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reconciler/managed/reconciler_modern_test.go` at line 1787, Remove the accidental duplicate chaining of .WithObservedGeneration(42) when setting conditions: in the want.SetConditions call that uses xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42) and the analogous call around line 1976, keep only a single .WithObservedGeneration(42) on each condition (e.g., xpv2.ReconcileSuccess().WithObservedGeneration(42) and xpv2.UpdateRestricted().WithObservedGeneration(42)).
1262-1262: Could we add one failure-path case that asserts diff-backedUpdateFailedmessaging?Quick clarification: was it intentional not to validate message propagation on
UpdateFailedwhenExternalObservation.Diffis present? Adding that one case would keep failure conditions as actionable as the success-path coverage you added.As per coding guidelines,
**/pkg/reconciler/**: Conditions must be actionable for users (not developers), stable/deterministic, with proper Type/Reason/Message format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reconciler/managed/reconciler_modern_test.go` at line 1262, Add a failing-path table test in reconciler_modern_test.go that mirrors the success-path diff-backed case but asserts UpdateFailed includes the diff text: create a test entry that sets the ExternalObservation with Diff (e.g., ExternalObservation{Diff: "some diff"}), simulate the update error (use errBoom and wrap with errReconcileUpdate), and set want.SetConditions to expect xpv2.ReconcileError(...).WithObservedGeneration(42) and xpv2.UpdateFailed().WithObservedGeneration(42) whose Message contains both the diff and the wrapped error text; place this entry alongside the existing cases so the test asserts message propagation for UpdateFailed when ExternalObservation.Diff is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reconciler/managed/reconciler.go`:
- Around line 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.
- Line 1459: Replace the raw cmp.Diff from ExternalObservation.Diff being
embedded in the user-facing condition (via status.MarkConditions(...
xpv2.UpdateRestricted().WithMessage(observation.Diff) ...)) with a short,
stable, actionable message (e.g., "update restricted: external changes detected"
or similar) so conditions remain deterministic; instead write observation.Diff
to debug/change logs using the reconciler logger (e.g., r.log.Debug/Info) or a
dedicated change log entry. Update all occurrences where observation.Diff is
used as a condition message (the status.MarkConditions calls around
ReconcileSuccess() / UpdateRestricted()) to use the stable message and emit the
raw diff to logs only.
---
Nitpick comments:
In `@pkg/reconciler/managed/reconciler_modern_test.go`:
- Line 1787: Remove the accidental duplicate chaining of
.WithObservedGeneration(42) when setting conditions: in the want.SetConditions
call that uses
xpv2.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)
and the analogous call around line 1976, keep only a single
.WithObservedGeneration(42) on each condition (e.g.,
xpv2.ReconcileSuccess().WithObservedGeneration(42) and
xpv2.UpdateRestricted().WithObservedGeneration(42)).
- Line 1262: Add a failing-path table test in reconciler_modern_test.go that
mirrors the success-path diff-backed case but asserts UpdateFailed includes the
diff text: create a test entry that sets the ExternalObservation with Diff
(e.g., ExternalObservation{Diff: "some diff"}), simulate the update error (use
errBoom and wrap with errReconcileUpdate), and set want.SetConditions to expect
xpv2.ReconcileError(...).WithObservedGeneration(42) and
xpv2.UpdateFailed().WithObservedGeneration(42) whose Message contains both the
diff and the wrapped error text; place this entry alongside the existing cases
so the test asserts message propagation for UpdateFailed when
ExternalObservation.Diff is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b380076-1f09-448e-b595-6809a6d09675
📒 Files selected for processing (3)
pkg/reconciler/managed/reconciler.gopkg/reconciler/managed/reconciler_legacy_test.gopkg/reconciler/managed/reconciler_modern_test.go
| 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) |
There was a problem hiding this comment.
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.
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
jbw976
left a comment
jbw976
left a comment
There was a problem hiding this comment.
Thanks for driving this @bobh66. A couple meta questions in addition to the review comments:
- what does this look like in practice? e.g. do you have an example of a
kubectl describewith these conditions added, especially with a diff? - have we verified that providers are setting the diff field in practice and we'll typically have good data to show? e.g. a couple code paths in upjet that may be relevant that are not setting the ExternalObservation.Diff field:
| record.Event(managed, event.Warning(reasonCannotPublish, err)) | ||
| status.MarkConditions(xpv2.ReconcileError(err)) | ||
|
|
||
| return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) |
There was a problem hiding this comment.
this path will not get the status condition UpdateRequested(), even though the update was successfully requested.
what do you think about always calling status.MarkConditions(xpv2.UpdateRequested().WithMessage(observation.Diff)) immediately after the update request is successful, e.g. around line 1482-1488? that way, we have the condition set no matter what path gets taken later on.
that would also mean to remove the setting of it on line 1509, because we've already done it.
it's my understanding that calling status.MarkCondtions() multiple times is safe as it only replaces matching condition types, it won't remove any conditions: https://github.com/crossplane/crossplane/blob/main/apis/core/v2/condition.go#L190-L193
so this:
status.MarkConditions(xpv2.ReconcileSuccess(), xpv2.UpdateRequested().WithMessage(observation.Diff))
should be the same as:
status.MarkConditions(xpv2.UpdateRequested().WithMessage(observation.Diff))
# then later on...
status.MarkConditions(xpv2.ReconcileSuccess())
There was a problem hiding this comment.
That works - I'll make the change.
| reconcileAfter := r.pollIntervalHook(managed, r.pollInterval) | ||
| 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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Probably a good idea.
I have not been able to test this with a real provider yet. The API refactoring that moved the common APIs to c/c caused a lot of downstream fallout that makes pulling these changes into a provider for testing a lot more work. I'm hoping to get it working in provider-kubernetes but it's a lot more effort than I had expected.
I have not validated this, and indeed even provider-kubernetes does not supply the diff info. We could leave it out but I think it's the direction we want/need to go, so I'd prefer to support it and let the providers catch up. |
3 participants
Description of your changes
Adds a new UpToDate condition (name is subject to better suggestions) that exposes the ResourceUpToDate status from the Observe() call. This allows users to know whether the ManagedResource desired state matches the external resource, and whether an update has been requested, failed, or restricted by management policies.
There are five basic (non-error condition) scenarios:
The three False condition scenarios will include the contents of the observation.Diff if it is available.
This is important when importing existing resources to be able to be sure that Crossplane will not make any enexpected changes to the external resource.
I'm open to renaming the condition if there is a better option.
This build will fail until crossplane/crossplane#7294 is merged, at which point go.mod needs to be updated to the new version.
Fixes #939
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.