fix(ecs): filter out non-active ECS services #5125

iamhopaul123 · 2023-07-25T17:35:44Z

Applies similar fix as #5062 to ecs service.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

github-actions · 2023-07-25T17:46:13Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	51488	51400	+0.17
macOS (arm)	51684	51604	+0.16
linux (amd)	45324	45244	+0.18
linux (arm)	43588	43524	+0.15
windows (amd)	42140	42072	+0.16

codecov-commenter · 2023-07-25T17:49:27Z

Codecov Report

Merging #5125 (3b24f16) into mainline (5109d75) will decrease coverage by 0.01%.
Report is 3 commits behind head on mainline.
The diff coverage is 81.42%.

@@             Coverage Diff              @@
##           mainline    #5125      +/-   ##
============================================
- Coverage     69.65%   69.65%   -0.01%     
============================================
  Files           295      295              
  Lines         43243    43331      +88     
  Branches        285      285              
============================================
+ Hits          30122    30183      +61     
- Misses        11646    11675      +29     
+ Partials       1475     1473       -2

Files Changed	Coverage Δ
internal/pkg/term/progress/ecs.go	`76.92% <0.00%> (+1.16%)`	⬆️
internal/pkg/cli/task_run.go	`53.92% <66.66%> (+0.07%)`	⬆️
internal/pkg/aws/ecs/ecs.go	`91.08% <82.14%> (-0.94%)`	⬇️
internal/pkg/aws/ecs/service.go	`91.76% <84.00%> (+24.23%)`	⬆️
internal/pkg/ecs/ecs.go	`77.37% <91.66%> (-6.84%)`	⬇️

... and 3 files with indirect coverage changes

internal/pkg/aws/ecs/service.go

internal/pkg/ecs/ecs.go

internal/pkg/aws/ecs/ecs.go

internal/pkg/ecs/ecs.go

iamhopaul123 · 2023-07-26T19:19:00Z

I think having to specify an app+env provides enough security, right? I don't think Copilot will let you make two envs of the same name in the same app. Though what happens if someone creates an app in tools account A, then an env in account B, then an app in tools account C, and an env in account B? Maybe this is out of scope

Good call i think i added some extra logic to catch this.

internal/pkg/aws/ecs/ecs.go

bvtujo

lgtm just one tiny nit

internal/pkg/aws/ecs/ecs.go

internal/pkg/aws/ecs/service.go

joanniclaborde · 2023-08-16T22:15:42Z

internal/pkg/aws/ecs/ecs.go

+			return nil, err
+		}
+		if prevSvcArn != nil && prevSvcArn.clusterName != svcArn.clusterName {
+			return nil, fmt.Errorf("service %q and service %q should be in the same cluster", prevSvcArn.String(), svcArn.String())


Hi! I keep getting this error when doing copilot service show, and it's referencing old (deleted) clusters and services. Is it possible that this cluster check should be done after inactive services are filtered out?

KollaAdithya · 2023-08-16T23:51:59Z

Hey @joanniclaborde !

Which version of Copilot are you using. Fix for this is released in Copilot version v1.29.1
https://github.com/aws/copilot-cli/releases/tag/v1.29.1

joanniclaborde · 2023-08-17T00:01:59Z

Hey @joanniclaborde !

Which version of Copilot are you using. Fix for this is released in Copilot version v1.29.1 https://github.com/aws/copilot-cli/releases/tag/v1.29.1

I'm using the latest version, v1.29.1.

KollaAdithya · 2023-08-17T00:20:54Z

Can you please paste the error message to help us debug the issue?

joanniclaborde · 2023-08-17T00:50:14Z

% copilot svc show                                                                                                                                                            
Service name: my-svc
✘ describe service my-svc: retrieve rollback alarm names: get service my-svc: check if services are active: service "arn:aws:ecs:us-east-1:1234567890:service/my-app-staging-Cluster-s63uAzrXXXXX/my-app-staging-my-svc-Service-c0iGyjmXXXXX" and service "arn:aws:ecs:us-east-1:1234567890:service/my-app-staging-Cluster-7tGl5VSXXXXX/my-app-staging-my-svc-Service-2DLOI9QXXXXX" should be in the same cluster

my-app-staging-Cluster-s63uAzrXXXXX is the current active cluster, while my-app-staging-Cluster-7tGl5VSXXXXX is an old deleted one.

I still get that same error message when I delete the whole app and recreate it (with the same names, of course).

Thanks!

KollaAdithya · 2023-08-17T15:53:02Z

Thanks for reporting the bug. I am able to reproduce it locally. We will work on the fix for this!

address #5125 (comment) This PR fixes below **Scenario**  App: demo Env: test Svc: frontend 1. And the svc is successfully deployed , `copilot svc show` and `copilot task run —--generate-cmd demo/test/frontend` works ✅ ``` Cluster - demo-test-Cluster-ABC(Active  Service - demo-test-Cluster-ABC/demo-test-frontend-Service-ABC(Active)  ``` 2. After this if i delete the test environment and frontend service.  ``` Cluster - demo-test-Cluster-ABC (InActive)  Service - demo-test-Cluster-ABC/demo-test-frontend-Service-ABC(InActive)  ``` 3. Deploying service and Environment with the same names. ``` Cluster - demo-test-Cluster-ABC(InActive)   Service - demo-test-Cluster-ABC/demo-test-frontend-Service-ABC(InActive)   Cluster - demo-test-Cluster-DEF(Active)   Service - demo-test-Cluster-DEF/demo-test-frontend-Service-DEF(Active) ``` 4. `copilot svc show` and `copilot task run —--generate-cmd demo/test/frontend` will generate the below errors.    ``` copilot task run --generate-cmd demo/test/frontend ✘ generate task run command from service frontend of application demo deployed in environment test: retrieve network configuration for service frontend: check if services are active: service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-Bus8o0uclnAW/demo-test-frontend-Service-E0rKy3e2S78t" and service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-TJyweayOGepM/demo-test-frontend-Service-4Se9IVA2878D" should be in the same cluster   copilot svc show Application: demo Only found one service, defaulting to: frontend ✘ describe service frontend: retrieve rollback alarm names: get service frontend: check if services are active: service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-Bus8o0uclnAW/demo-test-frontend-Service-E0rKy3e2S78t" and service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-TJyweayOGepM/demo-test-frontend-Service-4Se9IVA2878D" should be in the same cluster ```    By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

dannyrandall · 2023-08-30T21:32:55Z

Hey @joanniclaborde! We just released this fix v1.30.0!🚀🎉 Thanks for reporting the issue!

fix(ecs): filter out non-active ECS services

6fceb5f

iamhopaul123 requested a review from a team as a code owner July 25, 2023 17:35

iamhopaul123 requested review from dannyrandall and removed request for a team July 25, 2023 17:35

amazon-ecs-cli-v2-pr-manager requested a review from KollaAdithya July 25, 2023 17:35

dannyrandall reviewed Jul 25, 2023

View reviewed changes

internal/pkg/aws/ecs/service.go Outdated Show resolved Hide resolved

internal/pkg/aws/ecs/service.go Outdated Show resolved Hide resolved

internal/pkg/ecs/ecs.go Outdated Show resolved Hide resolved

Merge branch 'mainline' into service-fix

81c89d0

bvtujo suggested changes Jul 26, 2023

View reviewed changes

internal/pkg/aws/ecs/ecs.go Show resolved Hide resolved

internal/pkg/ecs/ecs.go Outdated Show resolved Hide resolved

internal/pkg/ecs/ecs.go Outdated Show resolved Hide resolved

Addr fb

57be3dd

bvtujo reviewed Jul 26, 2023

View reviewed changes

internal/pkg/aws/ecs/ecs.go Show resolved Hide resolved

bvtujo approved these changes Jul 26, 2023

View reviewed changes

Add more comment

044e868

dannyrandall reviewed Jul 27, 2023

View reviewed changes

internal/pkg/aws/ecs/ecs.go Outdated Show resolved Hide resolved

internal/pkg/aws/ecs/service.go Show resolved Hide resolved

Let ECS handle no arn for api call

3b24f16

dannyrandall approved these changes Jul 28, 2023

View reviewed changes

Merge branch 'mainline' into service-fix

0230cae

mergify bot merged commit fc2dc09 into aws:mainline Jul 28, 2023
12 checks passed

This was referenced Jul 31, 2023

During service deployment: "more than one ECS service with tags" #5144

Closed

copilot status/show errors #5147

Closed

joanniclaborde reviewed Aug 16, 2023

View reviewed changes

KollaAdithya mentioned this pull request Aug 18, 2023

fix: filter active cluster before calling active services #5204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecs): filter out non-active ECS services #5125

fix(ecs): filter out non-active ECS services #5125

iamhopaul123 commented Jul 25, 2023

github-actions bot commented Jul 25, 2023 •

edited

Loading

codecov-commenter commented Jul 25, 2023 •

edited

Loading

iamhopaul123 commented Jul 26, 2023

bvtujo left a comment

joanniclaborde Aug 16, 2023

KollaAdithya commented Aug 16, 2023

joanniclaborde commented Aug 17, 2023

KollaAdithya commented Aug 17, 2023

joanniclaborde commented Aug 17, 2023

KollaAdithya commented Aug 17, 2023 •

edited

Loading

dannyrandall commented Aug 30, 2023

fix(ecs): filter out non-active ECS services #5125

fix(ecs): filter out non-active ECS services #5125

Conversation

iamhopaul123 commented Jul 25, 2023

github-actions bot commented Jul 25, 2023 • edited Loading

codecov-commenter commented Jul 25, 2023 • edited Loading

Codecov Report

iamhopaul123 commented Jul 26, 2023

bvtujo left a comment

Choose a reason for hiding this comment

joanniclaborde Aug 16, 2023

Choose a reason for hiding this comment

KollaAdithya commented Aug 16, 2023

joanniclaborde commented Aug 17, 2023

KollaAdithya commented Aug 17, 2023

joanniclaborde commented Aug 17, 2023

KollaAdithya commented Aug 17, 2023 • edited Loading

dannyrandall commented Aug 30, 2023

github-actions bot commented Jul 25, 2023 •

edited

Loading

codecov-commenter commented Jul 25, 2023 •

edited

Loading

KollaAdithya commented Aug 17, 2023 •

edited

Loading