Timeout error retries for SSM resources #8992
Conversation
ghost
added
size/XS
| @@ -164,21 +164,26 @@ func resourceAwsSsmDocumentCreate(d *schema.ResourceData, meta interface{}) erro | |||
| } | |||
|
|
|||
| log.Printf("[DEBUG] Waiting for SSM Document %q to be created", d.Get("name").(string)) | |||
| var resp *ssm.CreateDocumentOutput | |||
| @@ -369,6 +374,11 @@ func resourceAwsSsmDocumentDelete(d *schema.ResourceData, meta interface{}) erro | |||
|
|
|||
| return resource.RetryableError(fmt.Errorf("SSM Document (%s) still exists", d.Id())) | |||
| }) | |||
| if isResourceTimeoutError(err) { | |||
There was a problem hiding this comment.
Woo! Okay this one is strange currently, let's fix it!
The resource.Retry() function above is using DescribeDocument calls to wait explicitly for the InvalidDocument error from the API to say "hey! this is actually gone after you deleted it". So in our case, we likely will need some additional logic here, not only to retry the last call, but also say "great! you deleted it!" after this new DescribeDocument call.
So maybe this logic can look something like:
input := &ssm.DescribeDocumentInput{
Name: aws.String(d.Get("name").(string)),
}
log.Printf("[DEBUG] Waiting for SSM Document %q to be deleted", d.Get("name").(string))
err = resource.Retry(10*time.Minute, func() *resource.RetryError {
_, err := ssmconn.DescribeDocument(input)
if isAWSErr(err, ssm.ErrCodeInvalidDocument, "") {
return nil
}
if err != nil {
return resource.NonRetryableError(err)
}
return resource.RetryableError(fmt.Errorf("SSM Document (%s) still exists", d.Id()))
})
if isResourceTimeoutError(err) {
_, err = ssmconn.DescribeDocument(input)
}
if isAWSErr(err, ssm.ErrCodeInvalidDocument, "") {
return nil
}
if err != nil {
return fmt.Errorf("error waiting for SSM Document (%s) deletion: %s", d.Id(), err)
}Please reach out if you have any questions 👍
There was a problem hiding this comment.
@bflad gotcha! Pushed an update to this, but I am left with one question. After the retry, we return nil if it was an ErrCodeInvalidDocument (indicating it was deleted), and return an error if there was one. But, if there wasn't an error, wouldn't that imply that it still described the document and so it wasn't deleted?
There was a problem hiding this comment.
Aye, that's the rabbit hole with this type of deletion logic. You are correct that it should probably return an error indicating if the document was still found, but up to you if you'd like to implement that now this logic is considered best effort. 😅
bflad
added
the
waiting-response
ghost
added
size/S
ryndaniels
removed
the
waiting-response
There was a problem hiding this comment.
LGTM! 🚀
--- PASS: TestAccAWSSsmResourceDataSync_basic (13.51s)
--- PASS: TestAccAWSSsmResourceDataSync_import (14.01s)
--- PASS: TestAccAWSSSMDocument_basic (17.97s)
--- PASS: TestAccAWSSSMDocument_permission_batching (19.40s)
--- PASS: TestAccAWSSsmResourceDataSync_update (21.13s)
--- PASS: TestAccAWSSSMDocument_session (21.67s)
--- PASS: TestAccAWSSSMDocument_permission_public (27.22s)
--- PASS: TestAccAWSSSMDocument_update (29.41s)
--- PASS: TestAccAWSSSMDocument_DocumentFormat_YAML (32.20s)
--- PASS: TestAccAWSSSMDocument_params (32.64s)
--- PASS: TestAccAWSSSMDocument_Tags (36.51s)
--- PASS: TestAccAWSSSMDocument_permission_private (38.84s)
--- PASS: TestAccAWSSSMDocument_automation (47.26s)
--- PASS: TestAccAWSSSMDocument_permission_change (87.00s)
| @@ -369,6 +374,11 @@ func resourceAwsSsmDocumentDelete(d *schema.ResourceData, meta interface{}) erro | |||
|
|
|||
| return resource.RetryableError(fmt.Errorf("SSM Document (%s) still exists", d.Id())) | |||
| }) | |||
| if isResourceTimeoutError(err) { | |||
There was a problem hiding this comment.
Aye, that's the rabbit hole with this type of deletion logic. You are correct that it should probably return an error indicating if the document was still found, but up to you if you'd like to implement that now this logic is considered best effort. 😅
|
This has been released in version 2.16.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Community Note
Related #7873
Release note for CHANGELOG:
Output from acceptance testing: