Skip to content

ci: Handle concurrent deletions in CI - #696

Merged
ystros merged 2 commits into
ubuntu-jammyfrom
update-cleanup-scripts
Jul 30, 2026
Merged

ci: Handle concurrent deletions in CI#696
ystros merged 2 commits into
ubuntu-jammyfrom
update-cleanup-scripts

Conversation

@ystros

@ystros ystros commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

This pipeline exists in the ubuntu-jammy, ubuntu-noble, and ubuntu-resolute
pipelines. They all run at the same time. The cleanup of 3 year old images is
looking at ALL distributions in each pipeline. This caused failures where one
pipeline would delete an AMI and then the other 2 pipelines would try to delete
that AMI again and fail.

We cannot limit the cleanup to the current pipeline's OS, because we want to
clean up old AMIs even after a stemcell line is EOL and its pipeline has been
deleted. So this commit makes the cleanup script tolerant to the AMI having
been deleted.

This also removes an unused snapshot_id cleanup mechanism.

This was used in a `cleanup-light-aws-builder-test-amis` job that has since
been removed from the pipeline in c2e1e3c
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ceb6ab35-66aa-4e44-b3cc-1c874ef58d3f

📥 Commits

Reviewing files that changed from the base of the PR and between 2d79b18 and 148df08.

📒 Files selected for processing (1)
  • ci/tasks/light-aws/cleanup-ami.sh

Walkthrough

The cleanup task removes the snapshot_id parameter and no longer filters AMIs using it. The shell script adds a helper that treats selected missing AWS EC2 resources as successful deletions while propagating other errors. Image deregistration and snapshot deletion now use this helper, including snapshots that previously could be skipped.

Suggested reviewers: aramprice

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the change, but it omits the required Merge Forward notes and AI Review Feedback section. Add the Merge Forward branch instructions and an AI Review Feedback section that resolves or explains every review comment.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: tolerating concurrent CI deletions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch update-cleanup-scripts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 27, 2026
Alphasite
Alphasite previously approved these changes Jul 27, 2026

@Alphasite Alphasite 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.

i think its functionally fine, but have a few minor nits. My main concern is around snapshot_id, but i figure you did your due diligence

Comment thread ci/tasks/light-aws/cleanup-ami.sh Outdated
Comment thread ci/tasks/light-aws/cleanup-ami.sh Outdated
Comment thread ci/tasks/light-aws/cleanup-ami.sh
Comment thread ci/tasks/light-aws/cleanup-ami.sh Outdated

Copilot AI 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.

Pull request overview

This PR updates the Light AWS AMI cleanup task to tolerate expected “already deleted” errors caused by concurrent pipeline runs cleaning up the same shared AWS account, preventing CI failures when multiple pipelines attempt to delete the same AMI/snapshot.

Changes:

  • Add a delete_tolerating_missing wrapper to treat “not found/unavailable” AWS CLI delete errors as non-fatal.
  • Use the wrapper for aws ec2 deregister-image and aws ec2 delete-snapshot operations.
  • Remove the unused snapshot_id parameter and its associated cleanup/query logic.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
ci/tasks/light-aws/cleanup-ami.yml Removes the unused snapshot_id task param to match current script behavior.
ci/tasks/light-aws/cleanup-ami.sh Adds collision-tolerant deletion wrapper and applies it to AMI/snapshot deletion; removes obsolete snapshot-id-based logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@beyhan

beyhan commented Jul 30, 2026

Copy link
Copy Markdown
Member

During the FI WG meeting we wasn't sure whether this is ready for merge. @Alphasite you have some small improvement suggestions but also approved it. Is ready?

This pipeline exists in the ubuntu-jammy, ubuntu-noble, and ubuntu-resolute
pipelines. They all run at the same time. The cleanup of 3 year old images is
looking at ALL distributions in each pipeline. This caused failures where one
pipeline would delete an AMI and then the other 2 pipelines would try to delete
that AMI again and fail.

We cannot limit the cleanup to the current pipeline's OS, because we want to
clean up old AMIs even after a stemcell line is EOL and its pipeline has been
deleted. So this commit makes the cleanup script tolerant to the AMI having
been deleted.
@ystros
ystros dismissed stale reviews from Alphasite and coderabbitai[bot] via 148df08 July 30, 2026 16:10
@ystros
ystros force-pushed the update-cleanup-scripts branch from 2d79b18 to 148df08 Compare July 30, 2026 16:10
@ystros
ystros requested a review from Alphasite July 30, 2026 16:14
@ystros

ystros commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Just addressed the feedback, could you take another look @Alphasite ?

@Alphasite

Copy link
Copy Markdown
Contributor

Ah yes sorry. Approved LGTM. My concerns were relatively minor

@ystros
ystros merged commit 3b50641 into ubuntu-jammy Jul 30, 2026
10 checks passed
@github-project-automation github-project-automation Bot moved this from Pending Merge | Prioritized to Done in Foundational Infrastructure Working Group Jul 30, 2026
@ystros
ystros deleted the update-cleanup-scripts branch July 30, 2026 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

4 participants