[v2] Retry failed updates with exponential backoff

[blampe]

Currently, if our automation APIs call fail they return non-nil errors to the operator. In #676 I modified Update to translate these errors into a "failed" status on the Update/Stack, but other operations (preview etc.) still surface these errors and automatically re-queue.

We'd like to retry these failed updates much less aggressively than we retry transient network errors, for example. To accomplish this we do a few things:

• We consolidate the update controller's streaming logic for consistent error handling across all operations.
• We return errors with known gRPC status codes as-is, but unknown status codes are translated into failed results for all operations.
• We start tracking the number of times a stack has attempted an update. This is used to determine how much exponential backoff to apply.
• A failed update is considered synced for a cooldown period before we retry it. The cooldown period starts at 5 minutes and doubles for every failed attempt, eventually maxing out at 24 hours.

Fixes #677

[blampe]

This is really hairy! I'll probably break this out and put a table test around it.

[EronWright]

To your point, I don't understand why !isStackMarkedToBeDeleted is there, because I feel we should backoff in the destroy operation as we would for the update operation.

Maybe some inline funcs to give names to the sub-expressions? Like:

isUpToDate = func() bool {
	return instance.Status.LastUpdate.State == shared.SucceededStackStateMessage &&
		(isStackMarkedToBeDeleted || (instance.Status.LastUpdate.LastSuccessfulCommit == currentCommit && (!sess.stack.ContinueResyncOnCommitMatch || time.Since(instance.Status.LastUpdate.LastResyncTime.Time) < resyncFreq))
}
isCoolingDown = func() bool {
	return instance.Status.LastUpdate.State == shared.FailedStackStateMessage &&
		time.Since(instance.Status.LastUpdate.LastResyncTime.Time) < cooldown)
}

synced := instance.Status.LastUpdate != nil &&
		instance.Status.LastUpdate.Generation == instance.Generation &&
                 (isUpToDate() || isCoolingDown())

[blampe]

!isStackMarkedToBeDeleted was in here because we had a test that expected finalize to immediately retry a failed deletion, but we should do the same backoff in that case so I fixed the test.

I broke out isSynced into its own function to make it easier to unit test.

[codecov]

Codecov Report

Attention: Patch coverage is 78.17259% with 43 lines in your changes missing coverage. Please review.

Project coverage is 53.68%. Comparing base (83f8438) to head (f15ea11).
Report is 1 commits behind head on v2.

Files with missing lines	Patch %	Lines
...ator/internal/controller/auto/update_controller.go	71.11%	36 Missing and 3 partials ⚠️
...tor/internal/controller/pulumi/stack_controller.go	93.22%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               v2     #709      +/-   ##
==========================================
+ Coverage   50.22%   53.68%   +3.46%     
==========================================
  Files          27       27              
  Lines        2919     2902      -17     
==========================================
+ Hits         1466     1558      +92     
+ Misses       1272     1164     -108     
+ Partials      181      180       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EronWright]

If synced is true due to backoff, we shouldn't mark ourselves as ready, right? Maybe you'd want to check the lastUpdate's status.

[blampe]

Good point especially re: the dependencies feature. I changed this to mark as stalled if the update failed.

[EronWright]

I would advocate for resetting the failure count to zero here. Once the update is successful, retaining the failure count seems debatable. A couple of weird cases:

1. In the case of periodic resync, the failure count stays at non-zero in perpetuity.
2. In the case of sporatic errors (fail, success, fail), one might use backoff prematurely. Or is that intentional?

When a container recovers from a crashloop, does the status still reflect the situation or are "events" the only historical record at that point?

[blampe]

In the case of sporatic errors (fail, success, fail), one might use backoff prematurely. Or is that intentional?

Very good point, this isn't intentional. We should definitely reset here.

[EronWright]

This seems not aggressive enough to cover short-term transient errors like a network connection error.

I suppose the stack could have spec elements for this

[blampe]

Lowered the initial backoff to 1 minute.

[EronWright]

Why would the stack be ready when it is in backoff? Consider, for example, the Stack dependencies feature, where one stack waits for another to be ready. You wouldn't want to prematurely unblock a dependent.

[blampe]

Changed to reconciling.