Skip to content
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.

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

Update status readiness #654

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TerjeLafton
Copy link

@TerjeLafton TerjeLafton commented Jan 29, 2024

Description of your changes

This PR aims to align the behavior of the reconciler for Update operations with that of Create operations.

It achieves this by rewriting some of the Reconcile function to not use the call to external.Update as a last resort, but instead evaluate if it is needed based on the returned observation.ResourceUpToDate and policy.ShouldUpdate.

Fixes #583

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

We have updated all tests related to the Update functionality, so they now test if the xpv1.Updating() condition is there and that it returns the updated reconcile.Result{Requeue: true} struct.
No tests have been added, but we are not sure if it is needed.

Signed-off-by: Terje Lafton / Intility AS <[email protected]>
…uccessful update

Signed-off-by: Terje Lafton / Intility AS <[email protected]>
@TerjeLafton TerjeLafton changed the title Update status readiness #583 Update status readiness Jan 29, 2024
Copy link
Author

@TerjeLafton TerjeLafton left a comment

Choose a reason for hiding this comment

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

We just did a quick self review to explain some of our decisions.

// accordingly.
// https://github.com/crossplane/crossplane/issues/289
// skip the update if the management policy is set to ignore updates
if !policy.ShouldUpdate() {
Copy link
Author

Choose a reason for hiding this comment

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

This was originally lower in the reconciler, but we moved it here to catch cases where the management policy does not allow updates early.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this is an improvement on the previous behavior.

At this point in the reconcile loop we know no update is needed, regardless of whether the policy would or wouldn't allow an update. I feel it's more useful to log that no update is needed than to log that no update will be performed due to policy. Logging that no update will be performed due to policy leaves it ambiguous as to whether an update is needed.

Copy link
Author

Choose a reason for hiding this comment

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

That is a good point! But I think I would rather rewrite the if on line 1157:

From this: if !observation.ResourceUpToDate && policy.ShouldUpdate() {
To this: if !observation.ResourceUpToDate {

And then I can immediately on the line below check if !policy.ShouldUpdate() and tell the user that the update will not be performed. This way it will only log this if an update is actually needed.

Would this maybe create spam for the users, though? I am not very familiar with how the ShouldUpdate policy is set, but when it is set to false, would the user not be spammed with Skipping update... on every reconcile then?

managed.SetConditions(xpv1.ReconcileSuccess())
return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}
if !observation.ResourceUpToDate && policy.ShouldUpdate() {
Copy link
Author

Choose a reason for hiding this comment

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

This if condition is what allows us to treat the Update case the same as the Create case.

This is as close to the implementation for Create as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure observation.ResourceUpToDate is the correct signal from which to derive an Updating condition.

To me:

  • ReasonCreating means a resource is unavailable because it's still being created by the cloud provider.
  • ReasonUpdating would therefore mean a resource is currently being updated by the cloud provider.

It would potentially make sense to set ReasonUpdating if the cloud provider API let us know that the resource was processing an update. For example I think a GKE cluster would go into state REPAIRING at the API level and be partially degraded while updating. On the other hand, a lot of cloud APIs won't give any indication that an update is being processed.

I believe observation.ResourceUpToDate indicates that there's a diff between the MR's desired state, and the observed state of the external resource. It's not indicating whether the external resource is updating but rather whether the external resource appears to need an update.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this reconciler sets the Creating condition, but it never unsets it. It never sets the Available condition.

The reconciler can know that it triggered the creation of of an external resource, but it has no way to know whether creation is done. The ExternalClient is responsible for setting the resource to Available when it notices that creation has completed. For some resources the ExternalClient knows that creation has completed due to some API level state transition. Other resources are ready as soon as they're created.

Will ExternalClient implementations need to factor in the existence of a new Updating condition by handling the case where a resource has transitioned from Updating back to Available?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure observation.ResourceUpToDate is the correct signal from which to derive an Updating condition.

To me:

* `ReasonCreating` means a resource is unavailable because it's still being created by the cloud provider.

* `ReasonUpdating` would therefore mean a resource is currently being updated by the cloud provider.

It would potentially make sense to set ReasonUpdating if the cloud provider API let us know that the resource was processing an update. For example I think a GKE cluster would go into state REPAIRING at the API level and be partially degraded while updating. On the other hand, a lot of cloud APIs won't give any indication that an update is being processed.

I believe observation.ResourceUpToDate indicates that there's a diff between the MR's desired state, and the observed state of the external resource. It's not indicating whether the external resource is updating but rather whether the external resource appears to need an update.

I don't really agree with this.

You seem to focus a lot on what the cloud provider reports for the resource, so if they say it is ready, we set it to ready, but is this really what we want?
When I see that a resource is ready, I would think the cloud provider says it is good to go AND it matches my desired state.
If I update some firewall resource to add a rule to block something happening, is this really ready until my observe function has verified that the rule is actually there?

When I first read this in the reconcile function, it read it as "The API returned a 2xx code indicating that we have successfully created the resource, now let's requeue immediately to make sure it is actually created".

Copy link
Author

Choose a reason for hiding this comment

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

Note that this reconciler sets the Creating condition, but it never unsets it. It never sets the Available condition.

The reconciler can know that it triggered the creation of of an external resource, but it has no way to know whether creation is done. The ExternalClient is responsible for setting the resource to Available when it notices that creation has completed. For some resources the ExternalClient knows that creation has completed due to some API level state transition. Other resources are ready as soon as they're created.

Will ExternalClient implementations need to factor in the existence of a new Updating condition by handling the case where a resource has transitioned from Updating back to Available?

That is a good point.
I am not very familiar with how for example the Upjet providers set the ready field, but I assume they do it at the end of the observe function just before they return the response saying that the resource both exists and is up to date.

My desire was always to just make Update behave the same as Create, to always give the most up to date status on if the resource is as I desire.
So in the implementation I proposed, I set the status to not ready and Updating with an instant requeue to have the observe function have to verify that things are OK. This way it should be covered by the same logic that already sets the resource as Ready by existing providers.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't actually able to find these definitions in the docs, only this from the troubleshooting part:

Most Crossplane resources set the Ready condition. Ready represents the availability of the resource - whether it is creating, deleting, available, unavailable, binding, etc.

You acknowledge that there is a gap here, and I feel like the issues you linked confirm the gap and that is a big enough problem that workarounds have been attempted to fix it.

I understand that definitions and such can be scary to change, as they often introduce some level of a breaking change, but I don't really see the negative side of changing this. All providers already implement a way of checking if the resource is actually compliant with the desired state, I only suggest that we actually use it for Update too.

What is the argument for not changing this?
The only negative I see is that the resource will be shown as Updating for a couple of seconds before it shows Ready.

Another issue with the current implementation:
If I were to create my own in-house provider, and I wanted the Ready field to better indicate if the resource is updating, I could manually set the condition in my Update function. The problem is that there is no reconcile requeue after successfully requesting the update, so if I have 10 minute poll interval, it means my resource is Updating for 10 minutes, even though it only took a second.

Copy link
Member

@negz negz Feb 22, 2024

Choose a reason for hiding this comment

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

What is the argument for not changing this?

I'm not convinced the way you're addressing the problem in this PR is the best way to address the problem. If we're going to change the semantics of Ready, we should take a step back and:

  • Consider the impact.
  • Consider whether changing the semantics of Ready is the right thing to do (vs alternatives like adopting kstatus.

I think a change like this warrants at least a one-pager to get us all aligned on what the problem is, and on which solution is best.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good! Would you want me to write it, or is this something you would do internally?
I am not very familiar with the one-pager concept, but it seems like most of the existing ones are written by maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

I'm definitely happy for you to write it. 😄 I think it's a great thing to have more community members leading designs. We can pair you up with either myself or a maintainer closer to your timezone (e.g. @phisco or @turkenh) to help bounce ideas from.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I can give it a try 😄
I'll work on a first draft and open a PR in the Crossplane repo. I will do some research into Kstatus too.

Comment on lines +1167 to +1168
managed.SetConditions(xpv1.Updating(), xpv1.ReconcileError(errors.Wrap(err, errReconcileUpdate)))
return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
Copy link
Author

Choose a reason for hiding this comment

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

This is mostly the point of the PR. This is what gives us more consistent behaviour for Update.

Copy link
Author

Choose a reason for hiding this comment

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

Also similar additions on line 1177, for example.

Comment on lines +1181 to +1184
// We've successfully updated our external resource. In many cases the
// updating process takes a little time to finish. We requeue explicitly
// order to observe the external resource to determine whether it's
// ready for use.
Copy link
Author

Choose a reason for hiding this comment

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

I am not sure about the "We requeue explicitly order to observe..." sentence. It seems a bit odd to me.

This comment was copied from the Create case above and changed to fit with the Update case.

Comment on lines +1191 to +1195
// We did not need to create, update, or delete our external resource.
// Per the below issue nothing will notify us if and when the external
// resource we manage changes, so we requeue a speculative reconcile
// after the specified poll interval in order to observe it and react
// accordingly.
Copy link
Author

Choose a reason for hiding this comment

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

This is the comment that was used for the if observation.ResourceUpToDate case.

We moved it to the bottom as it seems to work well for the last resort case, which is when there is nothing we need to do.

// https://github.com/crossplane/crossplane/issues/289
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"))
Copy link
Author

Choose a reason for hiding this comment

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

We chose to not emit any events here, but we are not sure.

Emitting here would emit every reconcile that just observed and everything is okay, so it doesn't seem right.

@TerjeLafton TerjeLafton marked this pull request as ready for review January 29, 2024 14:59
@TerjeLafton TerjeLafton requested review from a team as code owners January 29, 2024 14:59
@TerjeLafton
Copy link
Author

Ready for review 😄 @phisco @pedjak

@turkenh, long since I made the issue and promised this PR. It's been busy 😅

@negz negz self-assigned this Jan 30, 2024
@negz
Copy link
Member

negz commented Jan 30, 2024

Assigning myself - I'd like to make some time to review before we proceed here. This reconciler is very subtle and very widely used.

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review, this fell out of my GitHub notifications somehow.

// accordingly.
// https://github.com/crossplane/crossplane/issues/289
// skip the update if the management policy is set to ignore updates
if !policy.ShouldUpdate() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this is an improvement on the previous behavior.

At this point in the reconcile loop we know no update is needed, regardless of whether the policy would or wouldn't allow an update. I feel it's more useful to log that no update is needed than to log that no update will be performed due to policy. Logging that no update will be performed due to policy leaves it ambiguous as to whether an update is needed.

func Updating() Condition {
return Condition{
Type: TypeReady,
Status: corev1.ConditionFalse,
Copy link
Member

Choose a reason for hiding this comment

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

Per #583 (comment), I don't think an updating resource is necessarily an unready resource.

managed.SetConditions(xpv1.ReconcileSuccess())
return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}
if !observation.ResourceUpToDate && policy.ShouldUpdate() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure observation.ResourceUpToDate is the correct signal from which to derive an Updating condition.

To me:

  • ReasonCreating means a resource is unavailable because it's still being created by the cloud provider.
  • ReasonUpdating would therefore mean a resource is currently being updated by the cloud provider.

It would potentially make sense to set ReasonUpdating if the cloud provider API let us know that the resource was processing an update. For example I think a GKE cluster would go into state REPAIRING at the API level and be partially degraded while updating. On the other hand, a lot of cloud APIs won't give any indication that an update is being processed.

I believe observation.ResourceUpToDate indicates that there's a diff between the MR's desired state, and the observed state of the external resource. It's not indicating whether the external resource is updating but rather whether the external resource appears to need an update.

managed.SetConditions(xpv1.ReconcileSuccess())
return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}
if !observation.ResourceUpToDate && policy.ShouldUpdate() {
Copy link
Member

Choose a reason for hiding this comment

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

Note that this reconciler sets the Creating condition, but it never unsets it. It never sets the Available condition.

The reconciler can know that it triggered the creation of of an external resource, but it has no way to know whether creation is done. The ExternalClient is responsible for setting the resource to Available when it notices that creation has completed. For some resources the ExternalClient knows that creation has completed due to some API level state transition. Other resources are ready as soon as they're created.

Will ExternalClient implementations need to factor in the existence of a new Updating condition by handling the case where a resource has transitioned from Updating back to Available?

@TerjeLafton
Copy link
Author

Apologies for the delayed review, this fell out of my GitHub notifications somehow.

No problem! Thanks for the review 😄
It couldn't be merged before the next version anyway, so no rush.

@plumbis
Copy link
Contributor

plumbis commented Apr 12, 2024

@TerjeLafton , when this is merged can you open a Docs Issue referencing this PR so we document your updates?

Thanks!

Copy link

github-actions bot commented Sep 3, 2024

Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Adding a comment starting with /fresh will mark this PR as not stale.

@github-actions github-actions bot added stale and removed stale labels Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent readiness status updates
3 participants