Skip to content

Fix Bugs in ECS Cluster Cleanup Script - #1798

Merged
agarakan merged 1 commit into
mainfrom
fix_ecs_cluster_cleanup
Jul 30, 2025
Merged

Fix Bugs in ECS Cluster Cleanup Script#1798
agarakan merged 1 commit into
mainfrom
fix_ecs_cluster_cleanup

Conversation

@agarakan

Copy link
Copy Markdown
Contributor

Description of the issue

This script has been failing to clean up old ECS clusters during the daily resource cleanup due to various bugs.

See old output of buggy ECS Resource Cleanup run (clean-ecs-clusters): https://github.com/aws/amazon-cloudwatch-agent/actions/runs/16610452973/job/46992332357

Description of changes

** What **

  1. Fixed pagination bug
  2. Removed premature deletion loop return
  3. Added check for running services
  4. Fixed Error messages for readability
  5. Documented cluster deletion criteria inline

See improved ECS Resource Cleanup (clean-ecs-clusters) after changes: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/16628166215/job/47050284629

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Validated locally against developer account

cd tool/clean && go run ./clean_ecs/clean_ecs.go --tags clean

Requirements

Before commiting your code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

Integration Tests

To run integration tests against this PR, add the ready for testing label.

** What **

1. Fixed pagination bug
2. Removed premature deletion loop return
3. Added check for running services
4. Fixed Error messages for readability
5. Documented cluster deletion criteria inline

** Why **
This script has been failing to clean up old ECS clusters during the
daily resource cleanup due to various bugs.

Validated by running locally against personal aws account
@agarakan
agarakan requested a review from a team as a code owner July 30, 2025 16:28
Comment thread tool/clean/clean_ecs/clean_ecs.go
Comment thread tool/clean/clean_ecs/clean_ecs.go
_, err := client.DeleteService(ctx, &deleteServiceInput)
if err != nil {
log.Printf("Error %v deleteing service %s cluster %s", err, serviceInput, *clusterId)
log.Printf("Error deleting service %s in cluster %s: %v", serviceInput, *clusterId, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How did we fix this error log?

The service cannot be stopped while it is scaled above 0. deleteing service {%!s(*string=0xc0001cc9f0)  %!s(*int32=<nil>) %!s(*string=<nil>)  {}} cluster arn:aws:ecs:us-west-2:506463145083:cluster/cwagent-integ-test-cluster-9c9e8807f261bf1d

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fix is on Line 69. Checks if cluster has running services, as you cannot delete a cluster with running services.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We'll add logic to safely clean up clusters with active services as a follow up. This CR already modified a lot of core logic, so didnt want to over-clutter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, yeah I was just wondering because I'd imagine a lot of our stale clusters have services running on them which we will need to clean up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see, okay ill track that. Basically we'll need to do scaledowns before we can clean up active services on clusters. I'll track adding that logic in the follow up

@Paramadon Paramadon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@agarakan
agarakan merged commit 7215c51 into main Jul 30, 2025
80 checks passed
@agarakan
agarakan deleted the fix_ecs_cluster_cleanup branch July 30, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants