Support async reconciliation

[turkenf]

What problem are you facing?

For some time, upjet-based providers have been reconciling resources asynchronously. In order to do so, it "lies" to managed reconciler. Doing so causes lots of issues. So far, we have discussed the topic under those issues. It would be better if we could track them here, in a single place.

• ManagementPolicy Late Initialize W/Provider Pod Restart Triggers Unexpected Behaviors crossplane#5918 see this comment
• Continual reconciliation on Azure provider on error upjet#439 see this comment
• [Bug]: No 'LateInitialize' management policy combined with an ungraceful provider pod termination can lead to attempted resource recreation crossplane-contrib/provider-upjet-azure#802
• [Bug]: KMS Key does not get external-name set unless LateInitialize is set - recreated onevery restart crossplane-contrib/provider-upjet-aws#1715

How could Crossplane help solve your problem?

Support async reconciliation in managed reconciler.

[turkenh]

Provider helm has a similar issue: crossplane-contrib/provider-helm#63

We could live without it to date, but, if there were support for such a mode in crossplane runtime, we could have provided a better UX for certain charts (i.e. the ones with pre-install hooks) and with wait: true.

[chlunde]

Is there a design document explaining why the C/U/D operations are running in a new goroutine?
I understand they need a longer timeout, so the context.Context would either need a longer timeout or run in a new one based on .Background().

But I don't know the reason for a new goroutine. I would suspect you could achieve the same effect with a high MaxConcurrentReconciles.

This would be more in line with what controller-runtime and crossplane-runtime expects, but I'm sure there's a good reason I don't know of :)

[negz]

I've spent the day exploring the code and working through this. What I keep coming back to is:

• A single reconcile loop isn't supposed to be a long running, polling operation. It's considered a bad practice.
• We have long running, polling operations. When we call one of of Terraform's functions, it doesn't return until it's finished polling to confirm the operation succeeded. There's no easy way to avoid this.

These two things are fundamentally at odds with each other.

It's interesting to think about why a single reconcile loop isn't supposed to sit around polling for an operation to finish. I can't find any official guidance or documentation on the topic. The closest I can find is kubernetes-sigs/controller-runtime#1763, but AFAIK this is the opinion of an experienced community member, not a maintainer.

That said, I do agree it's definitely best for reconciles to be short-lived, working iteratively. This approach:

• Avoids starving the Reconcile worker pool by filling it up with long-running goroutines that're just polling
• Avoids state loss if the controller crashes or shuts down in the middle of a long-polling reconcile

I don't see how the async implementation helps with either of these concerns:

• It avoids Reconcile worker pool starvation by shifting the work into another, unbounded async operation worker pool
• It still has potential data/state loss if the controller crashes

If the controller crashes while we're inside a Terraform create method for a resource with a non-deterministic identifier that would've been returned as part of the create call, there's nothing we can do. We don't know whether we crashed before making the create call, or after the create call while polling. We've potentially leaked a resource. This problem is unavoidable in any implementation that wraps Terraform CRUD functions.

I feel that shifting the work into a background pool of async operations is working around, not solving, the worker pool starvation issue. It's doing it in a way that I believe has some downsides ¹:

• We're only avoiding Reconcile worker starvation by moving the real work into another, unbounded worker pool.
• We need additional metrics like upbound_resource_ext_api_duration to monitor the duration of async operations (or at least act as a proxy for the duration of those operations), and we're missing metrics for async operation pool size.
• Concurrency knobs like MaxConcurrentReconciles are misleading - yes it technically caps the Reconciles but it doesn't cap how much Reconcile-triggered work the controller will try to do concurrently. I imagine you could OOM a provider by triggering many many reconciles that in turn spawn many long-running operations under the current architecture.

I feel it'd be a simpler, more predictable system if we let the Reconcile worker pool invoke Terraform CRUD methods synchronously (via the ExternalClient). This would still need some updates to the MR reconciler:

• Set Creating, Deleting etc status condition immediately before calling create or delete, so users know it's working.
• Avoid multiple Reconciles concurrently operating on the same resource - e.g. return early if already Creating ²

The main case against doing this is that it changes the meaning of the --max-reconcile-rate flag. This flag currently defaults to 100. It's overloaded. It sets the controller's MaxConcurrentReconciles, but also its client-side and reconcile rate limits. If we shift the Terraform work from the unbounded async operation pool into the bounded (by --max-reconcile-rate) reconcile pool, many folks will need to increase --max-reconcile-rate. What today is 20 concurrent Reconciles with potentially 500 async operations would instead become 500 concurrent reconciles. Users with large Crossplane deployments would need to re-tune their --max-reconcile-rate flag for the new behavior.

Footnotes

1. This is all based on my code exploration. I could be wrong - let me know if I missed things. ↩

2. I imagine we'd need some timeout where we assume the create failed if it's still creating after N minutes. ↩

[negz]

The main case against doing this is that it changes the meaning of the --max-reconcile-rate flag.

We recently removed this flag and the global rate limiter from c/c per crossplane/crossplane#6790. I think there's a case for doing this for providers as well:

• Providers don't need client-side rate limiting anymore (as explained in the issue)
• The global reconcile rate limiter causes misleading metrics (also explained in the issue)
• In upjet, --max-reconcile-rate is a particularly bad proxy for cloud API calls (e.g. to avoid rate limiting)

If we do decide to change the flag's meaning we might also consider going a step further like we did in c/c:

• Stop client-side rate-limiting, let AP&F do its job
• Stop trying to rate limit reconciles (what I think folks really want is to control the rate of cloud API calls)
• Expose a --max-concurrent-reconciles flag that tunes exactly that

[negz]

Avoid multiple Reconciles concurrently operating on the same resource

Nevermind, the workqueue automatically handles this for us. It tracks items being processed, and doesn't allow them to be processed multiple times simultaneously. However, it remembers "dirty" items and queues them for processing once the currently processing item is done.

https://github.com/kubernetes/client-go/blob/a296bd7/util/workqueue/queue.go#L245-L247

[ulucinar]

I believe the main reason the async mode has been implemented in terrajet/upjet was around the responsiveness issues:

1. When a resource is being provisioned, we would like to give feedback to the user via the status conditions of the MR. An example issue about this is here: Asynchronous deployment of helm releases crossplane-contrib/provider-helm#63. Similar to that issue, if we don't introduce some form of async external client support in the managed reconciler and instead decide to switch to the synchronous client implementations in upjet, then for certain cases, the MRs will not acquire any status conditions for a prolonged time (until the long running reconcile completes). This would be a breaking behavior change.
2. Another type of responsiveness issue would happen when the desired state changes during a long running reconcile ¹. Assume a cluster resource initially with just 3 replicas. The user updates the replica count erroneously to 100 and then figures out her mistake and updates the replica count to 10. If the external client is making blocking update calls, then she will first observe the cluster being scaled to 100 replicas and then down to 10.
3. In providers we have configurable polling with regular intervals to be able to detect drifts when the actual state changes. If the reconciler worker is blocked in a long running operation, then the controller-runtime will miss the configured periodic reconcile.

Another problem I see with abandoning the asynchronous external clients in upjet in favor of their synchronous counterparts is that it will result in a breaking configuration change: Consider there exists 100 MRs in a cluster. With an async external client in the current situation, you can reconcile them with a pool of 10 workers as the workers are not blocked doing long running operations. But if we switch to synchronous clients, then you can end up in a situation where these 10 workers are blocked creating/updating 10 MRs and the rest of the 90 MRs have no available reconciler workers to reconcile them. The users of the upjet-based providers will then need to adjust the MaxConcurrentReconciles parameters for their provider deployments. And this is another breaking change that our users will need to deal with, and it is related to resource utilization and performance metrics that they are monitoring. Another complication here is that for the upjet-based providers (and others in general), the MaxConcurrentReconciles is a provider-level (global) parameter that's shared across all the resources in the provider. So whether a resource's creates/updated/deletes are long running or not, when the user tries to adjust this parameter, all resource kinds are affected.

Here's a document where we discuss some issues caused by the mismatch between the crossplane-runtime managed reconciler's synchronicity assumptions and upjet's asynchronous external clients: https://docs.google.com/document/d/1UCRJhdsIfF4hZ_haw1TXg7RMQZkHKO8J0lTDHAV4rlU/edit?usp=sharing

The document also discusses some possible alternatives we can implement including adding support for async external clients in the managed reconciler, switching to the synchronous external clients in upjet and implementing workarounds for the issues. There are also some statistics for resource CRUD operation timeout overrides in the Terraform AWS provider.

I feel like adding async support for the managed reconciler in a backwards-compatible way is a superior option because:

• It implements the principle for keeping the reconciles short. For asynchronous external APIs, this is already the case and the issue is with the synchronous APIs. If the managed reconciler allows the proper implementation of the asynchronous external clients (running against synchronous APIs), we are in a better situation in terms of coverage in my opinion.
• Avoid unresponsive APIs and have the possibility to further improve responsiveness.
• Avoid the breaking behavior changes if we give up upjet's async external clients.

Footnotes

1. For the sake of clarity and correctness, upjet currently does not handle the second type of responsiveness issue mentioned above. The synchronous external API would need to provide a canceling mechanism for an async external client to implement it and Terraform APIs do not provide a high level API to cancel long running creates/updates. One way we may investigate for upjet is canceling the context we pass down to the Terraform layer, however, we have not yet looked into this. What's more important for me though is that we lose our ability to implement a cancel and update strategy for a synchronous API when the reconciliation is done in a synchronous manner, as the controller runtime will not run another reconciler worker to reconcile the desired state if there already exists another worker reconciling that object (namespaced name) blocked in a synchronous call. ↩

[github-actions]

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue 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. Leaving a comment starting with /fresh will mark this issue as not stale.

[bobh66]

We need a definitive answer to this issue one way or another, or we need to merge #850 to fix the problems in the existing implementation.