Skip to content

fix: clean up main branch container images - #1339

Open
valdo766hi wants to merge 4 commits into
NVIDIA:mainfrom
valdo766hi:fix-cleanup-main-container-images-171
Open

fix: clean up main branch container images#1339
valdo766hi wants to merge 4 commits into
NVIDIA:mainfrom
valdo766hi:fix-cleanup-main-container-images-171

Conversation

@valdo766hi

@valdo766hi valdo766hi commented May 27, 2026

Copy link
Copy Markdown

Closes #171.

Summary

This completes the cleanup work started in #598.

The existing workflow only removes untagged GHCR images. Images published from main are tagged as main-<shortsha>, so they were not covered by the untagged cleanup policy.

Changes

  • keep the existing untagged image cleanup
  • add cleanup for old main-* tagged images
  • keep the latest 10 matching main images
  • preserve release tags by only matching main-*
  • keep manual runs dry-run safe by default
  • allow scheduled runs to perform real cleanup
  • add currently published packages missing from the cleanup matrix

Validation

ruby -e 'require "yaml"; YAML.load_file(".github/workflows/cleanup-untagged-images.yml"); puts "valid yaml"'
grep -nE 'delete-tags: main-\*|keep-n-tagged: 10|older-than:|validate: true' .github/workflows/cleanup-untagged-images.yml
grep -c "dry-run: \${{ github.event_name == 'workflow_dispatch' && github.event.inputs.dry_run || 'false' }}" .github/workflows/cleanup-untagged-images.yml
git diff --check

Summary by CodeRabbit

  • Chores
    • Renamed and clarified the container image cleanup workflow and its manual-run inputs.
    • Reformatted cutoff input to a human-readable "30 days" and standardized retention defaults.
    • Expanded the list of image packages covered and improved ordering.
    • Adjusted behavior for dry-run between scheduled and manual runs.
    • Added automated removal of older build tags, preserving the 10 most recent and enabling validation.

Signed-off-by: valdo766hi <rivaldosilalahi13@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented May 27, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented May 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9bfd6aa1-8fa1-4eaf-b4cc-230fbb488098

📥 Commits

Reviewing files that changed from the base of the PR and between ebb9dfc and 595eb3d.

📒 Files selected for processing (1)
  • .github/workflows/cleanup-untagged-images.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/cleanup-untagged-images.yml

📝 Walkthrough

Walkthrough

GitHub Actions workflow now named "Cleanup Container Images"; reformats cut_off to "30 days"; expands the container image matrix with additional packages; updates the retention step to default dry-run: false for scheduled runs; and adds ghcr cleanup step to remove old main-* tags, keeping 10 recent.

Changes

Container Image Cleanup Workflow

Layer / File(s) Summary
Workflow metadata and input parameters
.github/workflows/cleanup-untagged-images.yml
Workflow display name changed from "Cleanup Untagged Container Images" to "Cleanup Container Images"; job renamed from cleanup-untagged-images to cleanup-container-images; cut_off input default reformatted from '30d' to '30 days'.
Container image matrix expansion
.github/workflows/cleanup-untagged-images.yml
Image package matrix expanded with new packages including nvsentinel/plugins-slinky-drainer, nvsentinel/gpu-reset, nvsentinel/janitor-provider, and nvsentinel/preflight-nccl-loopback, with existing entries reordered.
Image cleanup steps and retention policy
.github/workflows/cleanup-untagged-images.yml
snok/container-retention-policy updated to fall back to '30 days' and dry-run: false for scheduled runs; new dataaxiom/ghcr-cleanup-action step added to delete main-* tags older than cut_off, with keep-n-tagged: 10, dry-run wired to manual input, and validate: true.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 From main the tags sprout, many bright,
I nip the weeds beneath the night.
Ten I keep, the rest I prune,
Registry tidy by the moon,
Hops and carrots—cleanup done!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding cleanup for main branch container images. It directly relates to the primary objective in the changeset.
Linked Issues check ✅ Passed The PR implements all core requirements from issue #171: adding cleanup for main-* tagged images, retaining 10 recent images, defaulting manual runs to dry-run, and extending the cleanup matrix.
Out of Scope Changes check ✅ Passed All changes are scoped to the cleanup workflow objectives. The matrix additions (nvsentinel/plugins-slinky-drainer, gpu-reset, janitor-provider, preflight-nccl-loopback) represent published packages missing from cleanup, directly supporting the requirement to extend coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/cleanup-untagged-images.yml:
- Line 117: Replace the mutable tag usage "dataaxiom/ghcr-cleanup-action@v1.2.1"
with an immutable 40-character commit SHA for the same action (e.g.,
"dataaxiom/ghcr-cleanup-action@<full-commit-sha>"); find the correct commit SHA
in the dataaxiom/ghcr-cleanup-action repository (the commit corresponding to
v1.2.1 or the desired release) and update the uses line so the workflow
references that exact SHA instead of the version tag.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 66fc5d07-a25f-447a-9701-ec209549213d

📥 Commits

Reviewing files that changed from the base of the PR and between c28bf0c and 15feb10.

📒 Files selected for processing (1)
  • .github/workflows/cleanup-untagged-images.yml

Comment thread .github/workflows/cleanup-untagged-images.yml Outdated
Signed-off-by: valdo766hi <rivaldosilalahi13@gmail.com>
Signed-off-by: valdo766hi <rivaldosilalahi13@gmail.com>
@valdo766hi

Copy link
Copy Markdown
Author

please take a look sir @KaivalyaMDabhadkar

@KaivalyaMDabhadkar

Copy link
Copy Markdown
Contributor

Thanks for the PR @valdo766hi !
Requesting some changes before we enable scheduled real deletion here:

This PR changes scheduled runs from dry-run to real deletion, because schedule events now resolve dry-run to false. That is fine for the new main-* cleanup goal, but it also makes the existing untagged cleanup step delete for real.

This step is still using snok/container-retention-policy@v3.0.1. NVSentinel publishes multi-arch images (linux/amd64,linux/arm64), and snok’s docs state that GHCR represents multi-arch child manifests as untagged package versions; deleting those children can break the tagged parent image. snok documents automatic multi-arch child protection starting in v3.1.0, while this workflow is pinned to v3.0.1.

Can you please:

  1. Run this workflow manually with dry_run: true and share the output before we enable scheduled deletion.
  2. Since this PR also changes scheduled runs from dry-run to real deletion, can we make the untagged cleanup multi-arch safe before merge, either by upgrading to a pinned snok v3.1.0 SHA or by moving untagged cleanup to dataaxiom/ghcr-cleanup-action as well?

Signed-off-by: valdo766hi <rivaldosilalahi13@gmail.com>
@valdo766hi

valdo766hi commented Jun 2, 2026

Copy link
Copy Markdown
Author

Thanks for the review @KaivalyaMDabhadkar.

I updated the existing untagged cleanup to use snok/container-retention-policy v3.1.0 pinned by full commit SHA.

For the dry-run output, running this from my fork would not be useful because the job is guarded with github.repository == 'NVIDIA/NVSentinel', so it would skip outside the upstream repo.

Could a maintainer/vetter trigger or approve a manual workflow_dispatch run in NVIDIA/NVSentinel with dry_run=true?

@github-actions

Copy link
Copy Markdown
Contributor

@valdo766hi this PR has been inactive for 14 days. Do you need help finishing it, or should we close it for now? Feel free to reopen anytime.

@lalitadithya

Copy link
Copy Markdown
Collaborator

@KaivalyaMDabhadkar , can you please help here?

@KaivalyaMDabhadkar

Copy link
Copy Markdown
Contributor

Hi @valdo766hi, apologies for the long silence here, this fell off my radar.

I re-reviewed the latest changes and the snok/container-retention-policy v3.1.0 bump (SHA-pinned) resolves the multi-arch concern from my earlier review, v3.1.0 automatically detects and retains child manifests of tagged multi-arch images, so the untagged cleanup is now safe to run. The dataaxiom/ghcr-cleanup-action usage also checks out: keep-n-tagged combined with delete-tags: main-* restricts retention to the main-* subset, so release tags are untouched, and the pinned SHA matches v1.2.1.

On the dry run: you're right, I'll push this branch to a temporary branch in NVIDIA/NVSentinel and trigger a workflow_dispatch with dry_run=true myself, then share the run output here. If the logs look clean (only untagged versions and old main-* tags staged for deletion, no release tags), I'll approve and merge.

No further action currently needed from your side, thanks for the contribution and for your patience!

@KaivalyaMDabhadkar

KaivalyaMDabhadkar commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Hi @valdo766hi,

About the snok v3.1.0 bump: every nightly run of this workflow on main is currently failing because v3.0.1 rejects GitHub’s new-format GITHUB_TOKEN ([example failing run](https://github.com/NVIDIA/NVSentinel/actions/runs/30317091734)). Your SHA-pinned v3.1.0 bump fixes that and correctly protects multi-arch child manifests.

The dataaxiom action needs to be allowlisted. The validation run failed at startup because dataaxiom/ghcr-cleanup-action is not currently allowed in the NVIDIA organization ([run](https://github.com/NVIDIA/NVSentinel/actions/runs/30339605627)).

I’m going to request that this exact SHA be added to the Actions allowlist:

dataaxiom/ghcr-cleanup-action@f092b48ba3b604b2a83690dc4b2bbb3392e1045f

After reviewing the alternatives, I prefer keeping the purpose-built action rather than replacing it with an inline actions/github-script implementation. It handles multi-arch images and referrers explicitly and includes throttling and retries for GitHub API rate limits—which is important because the dry run identified over 10,000 stale main-* versions.

No change is currently needed from your side for the dataaxiom step. Once it is allowlisted, I’ll rerun the workflow with dry_run=true and verify that only eligible main-* tags are selected.

One change is still needed: nvsentinel/plugins-slinky-drainer does not exist in GHCR; the published package is nvsentinel/slinky-drainer. Please update the matrix entry to:

          - nvsentinel/slinky-drainer

Everything else reviewed clean. Thanks for the contribution and your patience!

@KaivalyaMDabhadkar

Copy link
Copy Markdown
Contributor

Hi @valdo766hi, closing the loop on the dataaxiom question, sorry for the churn on this one. I know my last comment said I would rather keep the purpose-built action, but after allowlisting it and testing it properly, I have to walk that back beacause testing changed my mind, so let me share exactly what I found.

Round 1, allowlisted, ran as-is: the policy gate cleared, but the run failed on all 23 of the current-generation nvsentinel/* packages with Request failed with status code 429 (run 30426162779). The earlier claim about the action retries on rate limits turned out to be wrong. It fails the step on the first 429, and our matrix (16 concurrent jobs sharing the repo GITHUB_TOKEN, with packages holding 4-6k versions each) trips GitHub's secondary limits pretty reliably.

Round 2, throttled with max-parallel: 3: that did fix the rate limiting (run 30431628191, 41/42 green in about 26 minutes). But when I did the log verification, I found something more concerning: the action staged four nvsentinel/nic-health-monitor versions that carry release tags (v0.8.0, v0.9.0, v1.2.0, v1.3.0 which are the releases we promoted by re-tagging main builds, now older than the cutoff). I read through the action's source at the pinned SHA to understand what would happen in a real run: when keep-n-tagged is set, deletions go through a phase that removes whole versions by digest, without checking for other tags. There is an untag -> reload -> recompute sequence designed to protect these versions, but it relies on read-after-write consistency between the registry and the Packages API and dry-run mode can't exercise that path at all, so we'd have no way to validate it before real deletions start running nightly right next to our release images. Since promoting releases by re-tagging main builds is normal practice in this repo (slinky-drainer's v1.16.0 is another example), I'm not comfortable taking that risk.

So two asks, and apologies for moving the goalposts after saying no change was needed on this step:

1. Could you replace the dataaxiom step with an actions/github-script step that does the same cleanup via the Packages API? actions/github-script is GitHub-created (so no allowlist concerns) and its octokit client retries on rate limits out of the box. Please pin it to a full commit SHA like the other actions in the workflow. The step should:

  • List all versions of the package (${{ matrix.package }}) using the org package-versions API, paginated, with the workflow's GITHUB_TOKEN.
  • Select only versions where every tag starts with main- as that way a version carrying a release tag (or any other tag) alongside a main-* alias can never be touched, which is the safety property the dataaxiom step couldn't give us.
  • From that set, keep anything newer than the cut_off input (same shared input as the snok step; treating the leading number as days is fine), sort the expired ones newest-first, keep the 10 most recent as a safety buffer, and delete the rest via the delete-package-version-for-org endpoint.
  • Honor the same dry-run expression as the snok step: when dry-run is on, only log what would be deleted (id, updated date, and tags per version) instead of calling the delete endpoint.

I've already validated exactly this logic across the full matrix at full parallelism (run 30342041763): 41/42 green, 10,548 stale main-* versions staged, zero release tags anywhere, and no max-parallel needed since octokit absorbed the same rate-limit conditions. Happy to share the exact step I tested if that saves you time.

2. The slinky fix from my last comment still applies: nvsentinel/plugins-slinky-drainer -> nvsentinel/slinky-drainer (the package doesn't exist under the current name, which is the one red job in every validation run).

One optional extra if you're up for it: the same step can also clean up orphaned attestation referrers (versions whose only tags are sha256-<digest> pointing at images that no longer exist) but that's a nice-to-have we can also handle in a follow-up, so don't let it hold up the two changes above.

Once you've pushed both changes, I'll re-run the dry run against your branch, post the log here, and approve. Thanks so much for your patience, this PR ended up being even more valuable than it first looked, since the snok bump also fixes our nightly cleanup, which has been silently failing.

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.

[Feature]: Clean up old images published form main in container registry

3 participants