Update status readiness #654
Conversation
Signed-off-by: Terje Lafton / Intility AS <terje.lafton@intility.no>
…uccessful update Signed-off-by: Terje Lafton / Intility AS <terje.lafton@intility.no>
Signed-off-by: Terje Lafton / Intility AS <terje.lafton@intility.no>
| // accordingly. | ||
| // https://github.com/crossplane/crossplane/issues/289 | ||
| // skip the update if the management policy is set to ignore updates | ||
| if !policy.ShouldUpdate() { |
There was a problem hiding this comment.
This was originally lower in the reconciler, but we moved it here to catch cases where the management policy does not allow updates early.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure observation.ResourceUpToDate is the correct signal from which to derive an Updating condition.
To me:
ReasonCreatingmeans a resource is unavailable because it's still being created by the cloud provider.ReasonUpdatingwould 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm not sure
observation.ResourceUpToDateis the correct signal from which to derive anUpdatingcondition.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
ReasonUpdatingif the cloud provider API let us know that the resource was processing an update. For example I think a GKE cluster would go into stateREPAIRINGat 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.ResourceUpToDateindicates 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".
There was a problem hiding this comment.
Note that this reconciler sets the
Creatingcondition, but it never unsets it. It never sets theAvailablecondition.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
ExternalClientis responsible for setting the resource toAvailablewhen it notices that creation has completed. For some resources theExternalClientknows that creation has completed due to some API level state transition. Other resources are ready as soon as they're created.Will
ExternalClientimplementations need to factor in the existence of a newUpdatingcondition by handling the case where a resource has transitioned fromUpdatingback toAvailable?
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Readyis the right thing to do (vs alternatives like adoptingkstatus.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| managed.SetConditions(xpv1.Updating(), xpv1.ReconcileError(errors.Wrap(err, errReconcileUpdate))) | ||
| return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) |
There was a problem hiding this comment.
This is mostly the point of the PR. This is what gives us more consistent behaviour for Update.
There was a problem hiding this comment.
Also similar additions on line 1177, for example.
| // 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. |
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
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")) |
There was a problem hiding this comment.
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.
|
Assigning myself - I'd like to make some time to review before we proceed here. This reconciler is very subtle and very widely used. |
| // accordingly. | ||
| // https://github.com/crossplane/crossplane/issues/289 | ||
| // skip the update if the management policy is set to ignore updates | ||
| if !policy.ShouldUpdate() { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
I'm not sure observation.ResourceUpToDate is the correct signal from which to derive an Updating condition.
To me:
ReasonCreatingmeans a resource is unavailable because it's still being created by the cloud provider.ReasonUpdatingwould 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() { |
There was a problem hiding this comment.
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?
No problem! Thanks for the review 😄 |
|
@TerjeLafton , when this is merged can you open a Docs Issue referencing this PR so we document your updates? Thanks! |
Description of your changes
This PR aims to align the behavior of the reconciler for
Updateoperations with that ofCreateoperations.It achieves this by rewriting some of the
Reconcilefunction to not use the call toexternal.Updateas a last resort, but instead evaluate if it is needed based on the returnedobservation.ResourceUpToDateandpolicy.ShouldUpdate.Fixes #583
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
We have updated all tests related to the
Updatefunctionality, so they now test if thexpv1.Updating()condition is there and that it returns the updatedreconcile.Result{Requeue: true}struct.No tests have been added, but we are not sure if it is needed.