Allow late-initialization before a Terraform apply #263
Conversation
ulucinar
force-pushed
the
lateinit-before-plan
branch
from
e1cdd88 to
37cbd30
Compare
| return managed.ExternalObservation{}, errors.Wrap(err, "cannot late initialize parameters") | ||
| } | ||
| lateInitialized := annotationsUpdated || lateInitedParams | ||
| resourceReady := tr.GetCondition(xpv1.TypeReady).Equal(xpv1.Available()) | ||
| // We try to mark the resource ready before the spec late initialization |
There was a problem hiding this comment.
nit: looks like this comment needs to be updated, i.e. we no longer mark as ready before late init
There was a problem hiding this comment.
If I'm not mistaken, we still prioritize marking as ready over late-initialization, as ExternalObservation.ResourceLateInitialized will not be set to true if the resource does not have the Ready condition. And we prioritize a status update for initially marking the resource as ready over a spec update for actually late-initializing the resource. One of the goals of this change was to preserve that optimization as it affects the time-to-readiness metric we defined for managed resources.
| // We try to mark the resource ready before the spec late initialization | ||
| // and a call for "Plan" because those operations are costly, i.e. late-init | ||
| // causes a spec update which defers status update to the next reconcile and | ||
| // "Plan" takes a few seconds. | ||
| if !tr.GetCondition(xpv1.TypeReady).Equal(xpv1.Available()) { | ||
| if !resourceReady || lateInitialized { | ||
| tr.SetConditions(xpv1.Available()) |
There was a problem hiding this comment.
nit: I noticed that it was already the case before, but, confused to mark the resource as ready when the resource is not ready :) it would be great if you would mind clarifying this as well while updating the above comment.
There was a problem hiding this comment.
Yes, that also confused me and caught my interest while preparing this PR. But as you mentioned this is already the case in main branch. I added a comment making this assumption explicit and maybe we can follow the implications of this assumption up with a separate issue.
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
ulucinar
force-pushed
the
lateinit-before-plan
branch
from
37cbd30 to
d7301ad
Compare
ulucinar
changed the title
| return managed.ExternalObservation{}, errors.Wrap(err, "cannot late initialize parameters") | ||
| } | ||
| lateInitialized := annotationsUpdated || lateInitedParams | ||
| resourceReady := tr.GetCondition(xpv1.TypeReady).Equal(xpv1.Available()) |
There was a problem hiding this comment.
This is to make sure that if unavailable -> available transition is necessary, we will update the status hence no need to do any spec-related operations since they will be lost anyway. So alreadyAvailable or markedAvailable could be a less confusing than resourceReady because it doesn't represent the status in the cloud, it's only about whether the resource is marked as ready in etcd.
| tr.SetConditions(xpv1.Available()) | ||
| return managed.ExternalObservation{ | ||
| ResourceExists: true, | ||
| ResourceUpToDate: true, | ||
| ConnectionDetails: conn, | ||
| ResourceLateInitialized: annotationsUpdated, | ||
| ResourceLateInitialized: lateInitialized && resourceReady, |
There was a problem hiding this comment.
Even if late-initialization is done, we should issue status update, i.e. ResourceLateInitialized: false, because readiness is more important than updating the spec. The only exception is the critical annotations we can't loose like external name, that is why it was conditioned on annotationsUpdated though I can see how that's confusing without code comments.
Description of your changes
Due to an optimization related with the readiness of
Terraformedresources, in certain circumstances, we never have a chance to do a late initialization before we run a Terraform apply. However, as revealed in crossplane-contrib/provider-jet-azure#139, for certain resources and their configurations, it helps to do an early late-initialize before a Terraform plan. If certain optional fields are not set, a Terraform plan fails because we use theprevent_destroylifecycle meta-argument.Categorically, we can also claim that these are upstream Terraform issues (e.g., one is here) but with this PR, we can both solve such cases and also preserve the readiness optimization we already have. For some context, please refer to crossplane-contrib/provider-jet-azure#139 and please also see crossplane-contrib/provider-jet-azure#139 (comment).
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Tested via consuming this PR in
provider-jet-azurein the context of crossplane-contrib/provider-jet-azure#139.