generated from hashicorp/terraform-provider-scaffolding
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Store Consul cluster/snapshot state to fix failed cluster behavior #326
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
d5622e9
Fix storing of failed consul cluster/snapshot state
6394510
Add acc testing
f3b64eb
Make docs
3aec06c
Change acc testing tier to development
789ed6b
Fix scale attr
5ce199c
Fix size if acc testing of consul cluster
e7001a9
Update Consul cluster data source to also store state
4a68e2c
Fix brace in cluster def
f6787dc
go generate docs
6398b04
Use state vs cluster/snapshot_state
c26468a
Update internal/provider/resource_consul_cluster_test.go
0b6dcd1
Merge branch 'main' into jjtimmons/fix-consul-cluster-failed-state
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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.
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.
When the create fails, where would the tf have failed previously? Was it in attempting to get the client config files?
I had assumed the WaitForOperation call back on line 379 still prior to this call.
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.
you're right and yes, this was failing up above in the Wait. Where I saw this was an issue was in
resourceConsulClusterRead
(theGetConsulClusterByID
call succeeds whileGetConsulClientConfigFiles
fails). I then applied the same pattern everywhere we make these two API calls (storing results of first)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.
If it fails during the Wait then will we still clean it up?
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.
Yes, we'll still clean it up on the next
terraform delete
orterraform apply
. A PR from last year addressed this by storing the ID of the resource before polling: #59Current behavior:
tf create
fail during poll: bail, return diag.Error, we store nothing but IDtf plan
, read: GET with ID, see it's FAILED, purge from statetf apply
: create a new cluster. Don't delete prior cluster (it was purged). Probably (unless user knows to go manually delete the old resources) run into a duplicate cluster/hvn ID issueBehavior after this change:
tf create
fail during poll: bail, return diag.Error, we store nothing but IDtf plan
, read: GET with ID, set statetf apply
, old cluster is tainted, delete, create a new clusterThere 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.
Thanks for summing up the behavior change here 📝 💡
One thing I'm considering is how useful this computed output
state
might be in other circumstances, since it seems more of an internal detail. Would a user ever need to use the output ofstate
or react to it? I think that was the main reason for keeping it out of the resource schema until now.We could also resolve this issue without
state
by instead deleting any cluster that returns in a failed state, in addition to setting the ID to nil in the TF state. We could put this operation in a retry function.Which operation takes more time on the Consul service side? Deleting a failed cluster or re-attempting the creation of a failed cluster? Or any other advantages?
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.
I realized and messed up and replied out of thread: #326 (comment)