feat: cleanup ebs snapshots from testinfra ami's#2098
feat: cleanup ebs snapshots from testinfra ami's#2098
Conversation
There was a problem hiding this comment.
Pull request overview
Adds cleanup logic to the Testinfra AMI build workflow to remove AMI artifacts created during the run, aiming to prevent lingering AWS resources (and associated costs) after the workflow completes.
Changes:
- Fetch EBS snapshot IDs associated with the “stage 2” AMIs created by the workflow.
- Deregister stage 2 AMIs and attempt to delete their backing EBS snapshots.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SNAPSHOT_IDS=$(aws ec2 describe-images \ | ||
| --region ap-southeast-1 \ | ||
| --image-ids "$ami_id" \ | ||
| --query 'Images[*].BlockDeviceMappings[*].Ebs.SnapshotId' \ | ||
| --output text) |
There was a problem hiding this comment.
The PR description/title mention cleaning up EBS volumes, but this change deletes EBS snapshots associated with the AMI. If the intent is snapshot cleanup (likely the actual cost driver), please update the PR description/title for accuracy; if volumes are also being left behind, add a corresponding describe-volumes/delete-volume cleanup based on the same execution-id tags.
| SNAPSHOT_IDS=$(aws ec2 describe-images \ | ||
| --region ap-southeast-1 \ | ||
| --image-ids "$ami_id" \ | ||
| --query 'Images[*].BlockDeviceMappings[*].Ebs.SnapshotId' \ | ||
| --output text) | ||
| echo "Deregistering stage 2 AMI: $ami_id" | ||
| aws ec2 deregister-image --region ap-southeast-1 --image-id "$ami_id" || true | ||
| for snap_id in $SNAPSHOT_IDS; do | ||
| echo "Deleting snapshot: $snap_id" | ||
| aws ec2 delete-snapshot --region ap-southeast-1 --snapshot-id "$snap_id" || true |
There was a problem hiding this comment.
--query 'Images[*].BlockDeviceMappings[*].Ebs.SnapshotId' --output text can yield None/empty values for non-EBS mappings; the for snap_id in $SNAPSHOT_IDS loop will then attempt to delete an invalid snapshot id. Filter out nulls in the query (or guard in bash) so only real snapshot IDs are passed to delete-snapshot.
| aws ec2 deregister-image --region ap-southeast-1 --image-id "$ami_id" || true | ||
| for snap_id in $SNAPSHOT_IDS; do | ||
| echo "Deleting snapshot: $snap_id" | ||
| aws ec2 delete-snapshot --region ap-southeast-1 --snapshot-id "$snap_id" || true | ||
| done |
There was a problem hiding this comment.
aws ec2 delete-snapshot ... || true will silently leave snapshots behind when deletion fails (e.g., eventual-consistency right after deregistration, snapshots still referenced, or IAM permission issues), which undermines the goal of this cleanup. Consider adding a short retry/backoff (and/or collecting failures and failing the step at the end) so leaks are surfaced and cleanup is more reliable.
jfreeland
changed the title
WIP: I need to go validate this before we merge.
What kind of change does this PR introduce?
Delete the EBS snapshots backing testing AMI's that are built from the testinfra workflow. I think we're not deleting the snapshots (EBS volumes) when we're done. This has to be done separately in AWS.