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

provider/aws: Add retry logic to the aws_ecr_repository delete func #9050

Merged
merged 1 commit into from
Sep 28, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 26, 2016

Fixes #8597

There was sometimes an issue where Terraform was deleting the ECR
repository from the statefile before the reposity was actually deleted.

Added retry logic for Terraform to wait for the repository to be deleted
before proceeding with the statefile update

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEcrRepository_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/26 12:46:57 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSEcrRepository_ -timeout 120m
=== RUN   TestAccAWSEcrRepository_importBasic
--- PASS: TestAccAWSEcrRepository_importBasic (17.86s)
=== RUN   TestAccAWSEcrRepository_basic
--- PASS: TestAccAWSEcrRepository_basic (16.40s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    34.288s


return resource.RetryableError(
fmt.Errorf("%q: Timeout while waiting for the ECR Repository to be deleted", d.Id()))
})
Copy link
Contributor

@catsby catsby Sep 27, 2016

Choose a reason for hiding this comment

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

We don't check the err from resource.Retry here, so if the time limit hits we still remove from state, is that correct behavior? If you hit any of the resource.NonRetryableError paths then we'll still remove from state because we don't check the error

@catsby
Copy link
Contributor

catsby commented Sep 27, 2016

#8597 mentions a possible race condition. The description sounds as if we're attempting to delete this repo before it's actually created. Should we look into polling in the CREATE method to make sure the repo is available before moving on?

Fixes #8597

There was sometimes an issue where Terraform was deleting the ECR
repository from the statefile before the reposity was actually deleted.

Added retry logic for Terraform to wait for the repository to be deleted
before proceeding with the statefile update

```
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEcrRepository_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/26 12:46:57 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSEcrRepository_ -timeout 120m
=== RUN   TestAccAWSEcrRepository_importBasic
--- PASS: TestAccAWSEcrRepository_importBasic (17.86s)
=== RUN   TestAccAWSEcrRepository_basic
--- PASS: TestAccAWSEcrRepository_basic (16.40s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    34.288s
```
@stack72 stack72 force-pushed the b-aws-ecr-delete-timeout branch from 18cee54 to 15c8534 Compare September 28, 2016 11:01
@catsby
Copy link
Contributor

catsby commented Sep 28, 2016

The err check looks good; any thoughts on my question here?

Should we look into polling in the CREATE method to make sure the repo is available before moving on?

@stack72
Copy link
Contributor Author

stack72 commented Sep 28, 2016

After discussing with @catsby, we believe we are good with this solution and can merge it now :)

@stack72 stack72 merged commit 10eb572 into master Sep 28, 2016
@stack72 stack72 deleted the b-aws-ecr-delete-timeout branch January 31, 2017 22:12
@ghost
Copy link

ghost commented Apr 17, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_ecr_repository apply delete race condition
2 participants