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

Retry ECS create/update when target group isn't yet attached #3535

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

tomelliff
Copy link
Contributor

The ECS service can be attached to a load balancer's target group but at that point it may not yet be attached to a load balancer as the dependency graph Terraform generates normally will create the ECS service at the same time as the load balancer listener/listener rule.
This could be avoided by setting the ECS service 'depends_on' to include the load balancer listener/listener rule but this is not usable if the load balancer listener/listener rule is created in another module.

The target group in the acceptance test now gets the name from the load balancer, forcing a dependency between the load balancer and the target group.
Unfortunately it's not possible to use something from the listener to force the dependency as the listener already has a dependency on the target group.

Fixes #3495

@ghost ghost added the size/XS Managed by automation to categorize the size of a PR. label Feb 26, 2018
@tomelliff
Copy link
Contributor Author

Relevant acceptance tests:

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSEcsService_withAlb"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withAlb -timeout 120m
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (278.82s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	278.837s
$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSEcsService_healthCheckGracePeriodSeconds"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_healthCheckGracePeriodSeconds -timeout 120m
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (297.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	297.977s

@tomelliff
Copy link
Contributor Author

tomelliff commented Feb 26, 2018

pinging @bflad for reference following discussion in #3495

I'm not sure it's really great but the only way I can think of forcing a dependency chain that works otherwise since the 1.9.0 change is to put the listener port/protocol in the service name which is hacky as all hell and makes me very sad especially as that service name ends up being propagated all over the place into the AWS console, dashboards and log groups/streams. If the ECS service could be tagged then I would have slightly less objection about forcing an interpolation into a tag value there just because I can kinda sweep that under a rug but unfortunately that's not an option.

Otherwise I'm going to have to either remove the DRYness of my ecs-web-service module setting up a load balancer and calling the ecs-service module (which defaults to a worker based load balancer-less setup) so I repeat the ECS service stuff in both modules or collapse both modules together and litter every LB related resource with count = "${var.load_balanced_service ? 1 : 0}" which is much uglier and more confusing than I currently have.

@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Feb 26, 2018
@tomelliff
Copy link
Contributor Author

Derp, helps if I actually stage the test changes before committing and pushing. Force pushed that back over with the test changes.

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/ecs Issues and PRs that pertain to the ecs service. labels Feb 27, 2018
…balancer

The ECS service can be attached to a load balancer's target group but at that point it may not yet be attached to a load balancer as the dependency graph Terraform generates normally will create the ECS service at the same time as the load balancer listener/listener rule.
This could be avoided by setting the ECS service 'depends_on' to include the load balancer listener/listener rule but this is not usable if the load balancer listener/listener rule is created in another module.

The target group in the acceptance test now gets the name from the load balancer, forcing a dependency between the load balancer and the target group.
Unfortunately it's not possible to use something from the listener to force the dependency as the listener already has a dependency on the target group.
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Jul 11, 2018
@tomelliff
Copy link
Contributor Author

@bflad Just rebased it back on to master now, apologies for not spotting your message on Gitter sooner.

I've not re-ran the tests after the rebase but it looks fine from a glance. If you don't beat me to it I'll re-run the tests tomorrow morning when I get a chance.

@mikemac42
Copy link

Thanks for working on this fix. Been looking forward to this getting released, but the PR has been open for a while. Any chance this can get merged in soon?

@bflad bflad added this to the v1.33.0 milestone Aug 17, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Sorry this PR got lost in the shuffle. 😅 Let's get it in, thanks @tomelliff! 🚀

21 tests passed (all tests)
=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (9.58s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (10.04s)
=== RUN   TestAccAWSEcsService_basicImport
--- PASS: TestAccAWSEcsService_basicImport (10.04s)
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (13.41s)
=== RUN   TestAccAWSEcsService_withReplicaSchedulingStrategy
--- PASS: TestAccAWSEcsService_withReplicaSchedulingStrategy (13.56s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (13.66s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (13.60s)
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (13.69s)
=== RUN   TestAccAWSEcsServiceDataSource_basic
--- PASS: TestAccAWSEcsServiceDataSource_basic (14.15s)
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (20.42s)
=== RUN   TestAccAWSEcsService_withDaemonSchedulingStrategy
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (21.53s)
=== RUN   TestAccAWSEcsService_withServiceRegistries_container
--- PASS: TestAccAWSEcsService_withServiceRegistries_container (25.11s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (37.54s)
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (44.34s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (49.30s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (80.93s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (121.56s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (132.80s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (198.55s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (236.36s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (250.58s)

@bflad bflad merged commit 329dae9 into hashicorp:master Aug 17, 2018
bflad added a commit that referenced this pull request Aug 17, 2018
@mikemac42
Copy link

Thank you both!

@bflad
Copy link
Contributor

bflad commented Aug 22, 2018

This has been released in version 1.33.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 3, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ecs Issues and PRs that pertain to the ecs service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_ecs_service no longer retries when target group is not attached to load balancer
3 participants