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

teach evaluator to conditionally retry provider invocation #438

Closed
wants to merge 5 commits into from

Conversation

nyobe
Copy link
Contributor

@nyobe nyobe commented Jan 28, 2025

Adds a RetryableError error that providers can return to opt into being retried by the evaluator when they encounter a temporary error (such as a timeout).

Closes https://github.com/pulumi/pulumi-service/issues/25444

@nyobe nyobe marked this pull request as ready for review February 13, 2025 02:14
@nyobe nyobe requested review from pgavlin and glena February 13, 2025 02:14
providers can wrap an error with this type to indicate to the evaluator that they encountered a temporary error, allowing the evaluator to retry invoking the provider.
@nyobe nyobe force-pushed the claire/retry-rotation branch from 518e695 to e36fe70 Compare February 13, 2025 02:16
@nyobe nyobe requested a review from seanyeh February 13, 2025 02:52
@nyobe nyobe added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Feb 13, 2025
Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

so at a high level I'm wondering if this is kind of a footgun.

if a rotator gets halfway through a rotation, it'll need to have some way to understand the state it is in when it is next retried.

I don't think that we have a solution for that at the moment.

I wonder if instead of handling this in the evaluator we should just add a helper function that provider authors can use for retries.

provider.go Outdated
// RetryableError indicates a temporary error was encountered by a provider.
// Providers can wrap an error with this type to indicate to the evaluator that it may retry calling the provider.
type RetryableError struct {
Err error
Copy link
Member

Choose a reason for hiding this comment

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

nit: since we have Unwrap, I'd make this field unexported

Copy link
Member

Choose a reason for hiding this comment

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

also--let's add a factory for these

func Retryable(err error) RetryableError { return RetryableError{err} }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nyobe
Copy link
Contributor Author

nyobe commented Feb 13, 2025

if a rotator gets halfway through a rotation, it'll need to have some way to understand the state it is in when it is next retried.

When providers have access to managing creds they'll usually have the authority to overwrite whatever they find, so keeping intermediate state isn't as necessary like it would be if we tried to use the rotated credential to manage itself.

I wonder if instead of handling this in the evaluator we should just add a helper function that provider authors can use for retries.

We sort of do have a helper available with the retry.Until utility. Also, many sdks (like aws's) do their own builtin retries. Since the provider itself has the most context about the errors it encounters, maybe we should just let the provider itself make the decision on how it wants to retry itself, rather than trying to put that in the evaluator? (So just close this for now?)

@nyobe nyobe requested a review from pgavlin February 13, 2025 20:05
@pgavlin
Copy link
Member

pgavlin commented Feb 13, 2025

Since the provider itself has the most context about the errors it encounters, maybe we should just let the provider itself make the decision on how it wants to retry itself, rather than trying to put that in the evaluator? (So just close this for now?)

Yeah, this is what I'm thinking tbh. That allows us to sidestep questions of state, etc.

@nyobe nyobe closed this Feb 13, 2025
@nyobe nyobe deleted the claire/retry-rotation branch February 13, 2025 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants