service/servicediscovery: Refactor waiter logic into separate package, add test sweepers #12765
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
…, add test sweepers Reference: #12660 As with many resources that have asynchronous deletion processes, the addition of test sweepers needed to reuse that waiter logic to prevent future errors with an upcoming Route 53 Zone sweeper. Rather than copy the waiter logic in yet more places, this consolidates it into a separate package. This new waiter package setup is a proposal for future refactorings to all services. This also contains a fix for the failing acceptance test: ``` TestAccAWSServiceDiscoveryPrivateDnsNamespace_error_Overlap: testing.go:662: Step 0, expected error: errors during apply: error waiting for Service Discovery Private DNS Namespace (gvrwv.example.com) creation: CANNOT_CREATE_HOSTED_ZONE: The VPC vpc-083c996e0d85ad42e in region us-west-2 has already been associated with the hosted zone Z0432288GS3ZP21ZJMDI with the same domain name. (Service: AmazonRoute53; Status Code: 400; Error Code: ConflictingDomainExists; Request ID: 9b9cbfcb-0c54-4ea7-81ea-b7d47f694dc3) To match: overlapping name space ``` Output from acceptance testing: ``` --- PASS: TestAccAWSServiceDiscoveryHttpNamespace_basic (88.95s) --- PASS: TestAccAWSServiceDiscoveryHttpNamespace_Description (89.11s) --- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_basic (114.07s) --- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_longname (121.32s) --- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_error_Overlap (195.29s) --- PASS: TestAccAWSServiceDiscoveryPublicDnsNamespace_basic (100.89s) --- PASS: TestAccAWSServiceDiscoveryPublicDnsNamespace_longname (109.14s) --- PASS: TestAccAWSServiceDiscoveryService_http (92.09s) --- PASS: TestAccAWSServiceDiscoveryService_public (124.04s) --- PASS: TestAccAWSServiceDiscoveryService_private (146.52s) ``` Output from sweepers in AWS Commercial: ```console $ aws servicediscovery list-services | jq '.Services | length' 3 $ aws servicediscovery list-namespaces | jq '.Namespaces | length' 23 $ go test ./aws -v -timeout=10h -sweep-allow-failures -sweep=us-west-2 -sweep-run=aws_service_discovery_http_namespace,aws_service_discovery_public_dns_namespace,aws_service_discovery_private_dns_namespace 2020/04/10 13:07:20 [DEBUG] Running Sweepers for region (us-west-2): 2020/04/10 13:07:20 [DEBUG] Sweeper (aws_service_discovery_private_dns_namespace) has dependency (aws_service_discovery_service), running.. 2020/04/10 13:07:20 [DEBUG] Running Sweeper (aws_service_discovery_service) in region (us-west-2) ... 2020/04/10 13:07:24 [INFO] Deleting Service Discovery Service: srv-6u6zvcurfafyydch 2020/04/10 13:07:24 [INFO] Deleting Service Discovery Service: srv-eatg6appbkntd3hg 2020/04/10 13:07:25 [INFO] Deleting Service Discovery Service: srv-vbwvlfj72ljmgugf ... 2020/04/10 13:07:25 [DEBUG] Running Sweeper (aws_service_discovery_private_dns_namespace) in region (us-west-2) ... 2020/04/10 13:07:27 [INFO] Deleting Service Discovery Private DNS Namespace: ns-litixb75ds3f2uf5 2020/04/10 13:07:28 [DEBUG] Waiting for state to become: [SUCCESS] ... 2020/04/10 13:08:14 [INFO] Deleting Service Discovery Private DNS Namespace: ns-r5wjzm375fvgdwh4 ... 2020/04/10 13:08:51 [INFO] Deleting Service Discovery Private DNS Namespace: ns-4ytxqn3lisykz7no ... 2020/04/10 13:09:38 [INFO] Deleting Service Discovery Private DNS Namespace: ns-qtpimmlhqxvlpsn6 ... 2020/04/10 13:10:25 [INFO] Deleting Service Discovery Private DNS Namespace: ns-g56wgnwsupzygoqe ... 2020/04/10 13:11:12 [INFO] Deleting Service Discovery Private DNS Namespace: ns-7bbfqneobn4rwqpf ... 2020/04/10 13:11:59 [INFO] Deleting Service Discovery Private DNS Namespace: ns-udoy6ezpgshurvoe ... 2020/04/10 13:12:46 [DEBUG] Sweeper (aws_service_discovery_service) already ran in region (us-west-2) 2020/04/10 13:12:46 [DEBUG] Sweeper (aws_service_discovery_public_dns_namespace) has dependency (aws_service_discovery_service), running.. 2020/04/10 13:12:46 [DEBUG] Sweeper (aws_service_discovery_service) already ran in region (us-west-2) 2020/04/10 13:12:46 [DEBUG] Running Sweeper (aws_service_discovery_public_dns_namespace) in region (us-west-2) ... 2020/04/10 13:12:49 [INFO] Deleting Service Discovery Public DNS Namespace: ns-nblkmh5ji2m2n5th ... 2020/04/10 13:13:36 [INFO] Deleting Service Discovery Public DNS Namespace: ns-i5byd6xt2r4segki ... 2020/04/10 13:14:23 [INFO] Deleting Service Discovery Public DNS Namespace: ns-wwqgutsv3sjltcvd ... 2020/04/10 13:15:01 [INFO] Deleting Service Discovery Public DNS Namespace: ns-enyejqaeiqmgo3o5 ... 2020/04/10 13:15:47 [INFO] Deleting Service Discovery Public DNS Namespace: ns-qq4cew6zzsxalhox ... 2020/04/10 13:16:25 [INFO] Deleting Service Discovery Public DNS Namespace: ns-clbx2ontloe5fkkv ... 2020/04/10 13:17:12 [INFO] Deleting Service Discovery Public DNS Namespace: ns-bsttigkgmqolz74y ... 2020/04/10 13:17:59 [INFO] Deleting Service Discovery Public DNS Namespace: ns-itmaxb5ig6fr4f3f ... 2020/04/10 13:18:45 [INFO] Deleting Service Discovery Public DNS Namespace: ns-6mbdoqjjtpidsqms ... 2020/04/10 13:19:32 [DEBUG] Sweeper (aws_service_discovery_http_namespace) has dependency (aws_service_discovery_service), running.. 2020/04/10 13:19:32 [DEBUG] Sweeper (aws_service_discovery_service) already ran in region (us-west-2) 2020/04/10 13:19:32 [DEBUG] Running Sweeper (aws_service_discovery_http_namespace) in region (us-west-2) ... 2020/04/10 13:19:35 [INFO] Deleting Service Discovery HTTP Namespace: ns-ewqs7bukqucp6pdi ... 2020/04/10 13:20:11 [INFO] Deleting Service Discovery HTTP Namespace: ns-akmhftxycyw3mbrv ... 2020/04/10 13:20:48 [INFO] Deleting Service Discovery HTTP Namespace: ns-dqhnlww7lj5soebl ... 2020/04/10 13:21:24 [INFO] Deleting Service Discovery HTTP Namespace: ns-ojutb4eigjvmh4vs ... 2020/04/10 13:22:01 [INFO] Deleting Service Discovery HTTP Namespace: ns-7pngubm2arpkzuas ... 2020/04/10 13:22:37 [INFO] Deleting Service Discovery HTTP Namespace: ns-wf7yw7gdfz4nluk2 ... 2020/04/10 13:23:14 [INFO] Deleting Service Discovery HTTP Namespace: ns-m6onhy62jrhepxza ... 2020/04/10 13:23:50 Sweeper Tests ran successfully: - aws_service_discovery_private_dns_namespace - aws_service_discovery_public_dns_namespace - aws_service_discovery_http_namespace - aws_service_discovery_service $ aws servicediscovery list-services | jq '.Services | length' 0 $ aws servicediscovery list-namespaces | jq '.Namespaces | length' 0 ``` Output from sweepers in AWS GovCloud (US): ```console $ go test ./aws -v -timeout=10h -sweep-allow-failures -sweep=us-gov-west-1 -sweep-run=aws_service_discovery_http_namespace,aws_service_discovery_public_dns_namespace,aws_service_discovery_private_dns_namespace 2020/04/10 12:51:43 [DEBUG] Running Sweepers for region (us-gov-west-1): 2020/04/10 12:51:43 [DEBUG] Sweeper (aws_service_discovery_http_namespace) has dependency (aws_service_discovery_service), running.. 2020/04/10 12:51:43 [DEBUG] Running Sweeper (aws_service_discovery_service) in region (us-gov-west-1) ... 2020/04/10 12:51:46 [WARN] Skipping Service Discovery Services sweep for us-gov-west-1: RequestError: send request failed caused by: Post "https://servicediscovery.us-gov-west-1.amazonaws.com/": dial tcp: lookup servicediscovery.us-gov-west-1.amazonaws.com: no such host 2020/04/10 12:51:46 [DEBUG] Running Sweeper (aws_service_discovery_http_namespace) in region (us-gov-west-1) ... 2020/04/10 12:51:49 [WARN] Skipping Service Discovery HTTP Namespaces sweep for us-gov-west-1: RequestError: send request failed caused by: Post "https://servicediscovery.us-gov-west-1.amazonaws.com/": dial tcp: lookup servicediscovery.us-gov-west-1.amazonaws.com: no such host 2020/04/10 12:51:49 [DEBUG] Sweeper (aws_service_discovery_service) already ran in region (us-gov-west-1) 2020/04/10 12:51:49 [DEBUG] Sweeper (aws_service_discovery_public_dns_namespace) has dependency (aws_service_discovery_service), running.. 2020/04/10 12:51:49 [DEBUG] Sweeper (aws_service_discovery_service) already ran in region (us-gov-west-1) 2020/04/10 12:51:49 [DEBUG] Running Sweeper (aws_service_discovery_public_dns_namespace) in region (us-gov-west-1) ... 2020/04/10 12:51:52 [WARN] Skipping Service Discovery Public DNS Namespaces sweep for us-gov-west-1: RequestError: send request failed caused by: Post "https://servicediscovery.us-gov-west-1.amazonaws.com/": dial tcp: lookup servicediscovery.us-gov-west-1.amazonaws.com: no such host 2020/04/10 12:51:52 [DEBUG] Sweeper (aws_service_discovery_private_dns_namespace) has dependency (aws_service_discovery_service), running.. 2020/04/10 12:51:52 [DEBUG] Sweeper (aws_service_discovery_service) already ran in region (us-gov-west-1) 2020/04/10 12:51:52 [DEBUG] Running Sweeper (aws_service_discovery_private_dns_namespace) in region (us-gov-west-1) ... 2020/04/10 12:51:57 [WARN] Skipping Service Discovery Private DNS Namespaces sweep for us-gov-west-1: RequestError: send request failed caused by: Post "https://servicediscovery.us-gov-west-1.amazonaws.com/": dial tcp: lookup servicediscovery.us-gov-west-1.amazonaws.com: no such host 2020/04/10 12:51:57 Sweeper Tests ran successfully: - aws_service_discovery_service - aws_service_discovery_http_namespace - aws_service_discovery_public_dns_namespace - aws_service_discovery_private_dns_namespace ```
ghost
added
needs-triage
bflad
removed
the
needs-triage
gdavison
approved these changes
Apr 15, 2020
There was a problem hiding this comment.
A few suggestions, but LGTM 🚀
--- PASS: TestAccAWSServiceDiscoveryService_http (94.65s)
--- PASS: TestAccAWSServiceDiscoveryHttpNamespace_Description (94.71s)
--- PASS: TestAccAWSServiceDiscoveryHttpNamespace_basic (94.82s)
--- PASS: TestAccAWSServiceDiscoveryPublicDnsNamespace_basic (95.07s)
--- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_basic (95.43s)
--- PASS: TestAccAWSServiceDiscoveryPublicDnsNamespace_longname (95.47s)
--- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_longname (96.12s)
--- PASS: TestAccAWSServiceDiscoveryService_public (101.61s)
--- PASS: TestAccAWSServiceDiscoveryService_private (102.85s)
--- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_error_Overlap (166.31s)
Comment on lines
26
to
31
| switch output := outputRaw.(type) { | ||
| case *servicediscovery.GetOperationOutput: | ||
| return output, err | ||
| default: | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Since there are only two possibilities, would if v, ok := outputRaw.(*servicediscovery.GetOperationOutput); ok { ... } work better here?
There was a problem hiding this comment.
They are functionally equivalent, yes. Will update.
| } | ||
|
|
||
| opresp, err := stateConf.WaitForState() | ||
| operationOutput, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)) |
Comment on lines
+56
to
+77
| input := &servicediscovery.DeleteNamespaceInput{ | ||
| Id: namespace.Id, | ||
| } | ||
|
|
||
| log.Printf("[INFO] Deleting Service Discovery HTTP Namespace: %s", id) | ||
| output, err := conn.DeleteNamespace(input) | ||
|
|
||
| if err != nil { | ||
| sweeperErr := fmt.Errorf("error deleting Service Discovery HTTP Namespace (%s): %w", id, err) | ||
| log.Printf("[ERROR] %s", sweeperErr) | ||
| sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) | ||
| continue | ||
| } | ||
|
|
||
| if output != nil && output.OperationId != nil { | ||
| if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil { | ||
| sweeperErr := fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) deletion: %w", id, err) | ||
| log.Printf("[ERROR] %s", sweeperErr) | ||
| sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
The deletion logic is repeated here and in the resource itself. Would it make sense to have it in one place and call it for the resource and the sweeper?
There was a problem hiding this comment.
Most likely, yes. Ideally we can setup the sweepers to call the existing resource deletion logic rather than re-inventing the wheel each sweeper. Probably worth a followup design proposal though. 👍
Comment on lines
+57
to
+78
| input := &servicediscovery.DeleteNamespaceInput{ | ||
| Id: namespace.Id, | ||
| } | ||
|
|
||
| log.Printf("[INFO] Deleting Service Discovery Private DNS Namespace: %s", id) | ||
| output, err := conn.DeleteNamespace(input) | ||
|
|
||
| if err != nil { | ||
| sweeperErr := fmt.Errorf("error deleting Service Discovery Private DNS Namespace (%s): %w", id, err) | ||
| log.Printf("[ERROR] %s", sweeperErr) | ||
| sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) | ||
| continue | ||
| } | ||
|
|
||
| if output != nil && output.OperationId != nil { | ||
| if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil { | ||
| sweeperErr := fmt.Errorf("error waiting for Service Discovery Private DNS Namespace (%s) deletion: %w", id, err) | ||
| log.Printf("[ERROR] %s", sweeperErr) | ||
| sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Same deletion logic question
Comment on lines
+56
to
+77
| input := &servicediscovery.DeleteNamespaceInput{ | ||
| Id: namespace.Id, | ||
| } | ||
|
|
||
| log.Printf("[INFO] Deleting Service Discovery Public DNS Namespace: %s", id) | ||
| output, err := conn.DeleteNamespace(input) | ||
|
|
||
| if err != nil { | ||
| sweeperErr := fmt.Errorf("error deleting Service Discovery Public DNS Namespace (%s): %w", id, err) | ||
| log.Printf("[ERROR] %s", sweeperErr) | ||
| sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) | ||
| continue | ||
| } | ||
|
|
||
| if output != nil && output.OperationId != nil { | ||
| if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil { | ||
| sweeperErr := fmt.Errorf("error waiting for Service Discovery Public DNS Namespace (%s) deletion: %w", id, err) | ||
| log.Printf("[ERROR] %s", sweeperErr) | ||
| sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Same deletion logic question
Output from acceptance testing: ``` --- PASS: TestAccAWSServiceDiscoveryHttpNamespace_Description (89.96s) --- PASS: TestAccAWSServiceDiscoveryHttpNamespace_basic (90.19s) --- PASS: TestAccAWSServiceDiscoveryService_http (91.10s) --- PASS: TestAccAWSServiceDiscoveryPublicDnsNamespace_basic (108.49s) --- PASS: TestAccAWSServiceDiscoveryPublicDnsNamespace_longname (108.72s) --- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_longname (120.06s) --- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_basic (120.21s) --- PASS: TestAccAWSServiceDiscoveryService_public (121.87s) --- PASS: TestAccAWSServiceDiscoveryService_private (140.99s) --- PASS: TestAccAWSServiceDiscoveryPrivateDnsNamespace_error_Overlap (191.75s) ```
|
Adjusted the conditional logic and acceptance tested again: Will ensure CI is okay, write up a quick tracking issue for future refactoring, then merge this in. |
Closed
7 tasks
|
Refactoring design proposal issue 👍 #12840 |
|
This has been released in version 2.58.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
|
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Community Note
Closes #12660
Release note for CHANGELOG:
As with many resources that have asynchronous deletion processes, the addition of test sweepers needed to reuse that waiter logic to prevent future errors with an upcoming Route 53 Zone sweeper. Rather than copy the waiter logic in yet more places, this consolidates it into a separate package.
This new waiter package setup is a proposal for future refactorings to all services.
This also contains a fix for the failing acceptance test:
Output from acceptance testing:
Output from sweepers in AWS Commercial:
Output from sweepers in AWS GovCloud (US):