-
Notifications
You must be signed in to change notification settings - Fork 230
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
[Q3] Add retry logic to rancher2_cluster update #1159
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
93e6ab0
to
1f1cf71
Compare
HarrisonWAffel
approved these changes
Jun 28, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nit about not reusing a client, but otherwise lgtm
1f1cf71
to
27a6ccf
Compare
jiaqiluo
requested changes
Aug 4, 2023
thatmidwesterncoder
approved these changes
Aug 4, 2023
jiaqiluo
requested changes
Aug 4, 2023
7261652
to
e4cd06d
Compare
jiaqiluo
approved these changes
Aug 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #1040
Problem
Error when upgrading rke cluster with Terraform 1.25
which in this case, indicates an intermittent race condition on update of the
rancher2_cluster
resource.Solution
Add retry code to
rancher2_cluster
resource Update. I have chosen to useresource.Retry
instead ofcontext.WithTimeout
like in this code because we need a cluster update to retry until a non retryable error is returned or the timeout is reached. WithTimeout controls the timeout of an HTTP response, say a 500, but we don't want the request cancelled if it fails. We want to retry several times, especially on upgrade since customers tend to get pretty frustrated if they have to run aterraform apply
more than once.I've also moved
updateClusterMonitoring
into a helper function to increase readability of the Update function and updated comments.Testing
Engineering Testing
Manual Testing
The race condition in the original issue is difficult to reproduce, but team discussion has concluded that adding retry code to
rancher2_cluster
Update will fix the failure on server error.Test steps
enable_cluster_monitoring = true
(preferably while the previous update is still running to simulate the race condition)main.tf
main.tf (sections with cluster updates)
Automated Testing
QA Testing Considerations
Regressions Considerations