fix(sync-ci-images): stop assuming oc image mirror preserves manifest digests#3196
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughImage stream mirroring removes the optional source digest override and always re-queries the QCI floating tag after ChangesImage mirroring digest handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@doozer/doozerlib/cli/images_streams.py`:
- Around line 209-212: Update the QCI digest refresh around get_image_digest so
a None result triggers a retry; if the second lookup still returns None, raise
an error instead of continuing with the previous digest. Preserve the existing
successful digest flow and ensure the failure is surfaced to the caller.
- Around line 209-212: Keep the dry_run path side-effect free by preventing the
post-mirror processing around get_image_digest from executing when dry_run is
enabled. Return immediately after the dry-run print in the image mirroring flow,
or guard the digest lookup and subsequent GC mirror and imagestream patch
operations with a not-dry-run condition; preserve the existing behavior for real
runs.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9569a86a-566c-44a0-b1aa-d7260afe4dbc
📒 Files selected for processing (1)
doozer/doozerlib/cli/images_streams.py
| # Always query the actual digest at QCI after mirroring. | ||
| # oc image mirror may convert manifest formats (OCI↔Docker v2s2), | ||
| # which changes the digest, so the source digest cannot be trusted. | ||
| qci_digest = get_image_digest(floating_qci_dest, registry_config_file) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently continue when the QCI digest refresh fails.
get_image_digest can return None, after which the current warning-only path leaves the imagestream on its previous digest while reporting no failure. Retry the lookup and raise if it still cannot be confirmed.
🤖 Prompt for 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.
In `@doozer/doozerlib/cli/images_streams.py` around lines 209 - 212, Update the
QCI digest refresh around get_image_digest so a None result triggers a retry; if
the second lookup still returns None, raise an error instead of continuing with
the previous digest. Preserve the existing successful digest flow and ensure the
failure is surfaced to the caller.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep dry-run side-effect free.
The dry_run branch at Lines 191-193 only prints, so this unconditional lookup can fall through to the real GC mirror and imagestream patch. Return before querying, or guard the entire post-mirror block with if not dry_run.
Proposed fix
- qci_digest = get_image_digest(floating_qci_dest, registry_config_file)
+ if dry_run:
+ return
+ qci_digest = get_image_digest(floating_qci_dest, registry_config_file)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Always query the actual digest at QCI after mirroring. | |
| # oc image mirror may convert manifest formats (OCI↔Docker v2s2), | |
| # which changes the digest, so the source digest cannot be trusted. | |
| qci_digest = get_image_digest(floating_qci_dest, registry_config_file) | |
| # Always query the actual digest at QCI after mirroring. | |
| # oc image mirror may convert manifest formats (OCI↔Docker v2s2), | |
| # which changes the digest, so the source digest cannot be trusted. | |
| if dry_run: | |
| return | |
| qci_digest = get_image_digest(floating_qci_dest, registry_config_file) |
🤖 Prompt for 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.
In `@doozer/doozerlib/cli/images_streams.py` around lines 209 - 212, Keep the
dry_run path side-effect free by preventing the post-mirror processing around
get_image_digest from executing when dry_run is enabled. Return immediately
after the dry-run print in the image mirroring flow, or guard the digest lookup
and subsequent GC mirror and imagestream patch operations with a not-dry-run
condition; preserve the existing behavior for real runs.
… digests The source_digest optimization from PR openshift-eng#3093 assumed that oc image mirror preserves manifest bytes across registries, so the source digest could be reused as the QCI digest for GC-prevention tagging. This assumption is wrong when manifest format conversion occurs (OCI↔Docker v2s2), which changes the digest. This broke sync-ci-images for ALL OCP versions since July 14 (~71 consecutive failures). The openshift-base-nodejs Konflux image triggers the bug because its @sha256: pullspec sets source_digest, but the actual digest at QCI differs after mirroring. Revert to always querying QCI for the actual digest after mirroring, which was the working behavior before PR openshift-eng#3093. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
6b8baa5 to
1131216
Compare
|
@fgallott: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rayfordj The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
3 participants
Summary
source_digestoptimization from PR sync-ci-images: fix --stream handling, add --images filter #3093 that assumedoc image mirrorpreserves manifest digests across registriesRoot cause
PR #3093 added an optimization: extract the digest from Konflux
@sha256:source pullspecs and reuse it as the QCI digest for GC-prevention tagging, skipping theget_image_digest()call. This fails foropenshift-base-nodejsbecauseoc image mirrorconverts the manifest format, producing a different digest at QCI.Impact:
sync-ci-imagesjob has been failing for ALL OCP versions since July 14 (~71 consecutive failures, builds #629–#699). All CI base images includingbase-rhel9are stale (last updated June 8), blocking local container builds for downstream teams (e.g., ironic team).Test plan
uv run pytest doozer/tests/cli/test_images_streams.py— 28 passedsync-ci-imagesJenkins job for a single version to verify fix🤖 Generated with Claude Code
Summary by CodeRabbit