OCPBUGS-66263: Include index image sub-digests in dry run mapping.txt#1355
Open
dorzel wants to merge 3 commits intoopenshift:mainfrom
Open
OCPBUGS-66263: Include index image sub-digests in dry run mapping.txt#1355dorzel wants to merge 3 commits intoopenshift:mainfrom
dorzel wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Member
Author
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
dorzel
force-pushed
the
OCPBUGS-66263
branch
2 times, most recently
from
ed935b0 to
054890d
Compare
Member
Author
|
/retest |
adolfo-ab
approved these changes
Feb 19, 2026
Contributor
adolfo-ab
left a comment
adolfo-ab
left a comment
There was a problem hiding this comment.
Just a small suggestion related to the test code, otherwise lgtm.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adolfo-ab, dorzel The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
dorzel
force-pushed
the
OCPBUGS-66263
branch
from
054890d to
06bc475
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDry-run now discovers OCI manifest lists at runtime and via pre-collected data, expands them into per-instance digest mappings (source@sha256 → destination), appends sub-digest entries to missing-image lists, and exposes helpers to produce digest-pinned destinations. Tests exercise OCI layout creation and unreachable-image behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as DryRun Executor
participant Parser as alltransports.ParseImageName
participant ImageSrc as containers/image Source (Opts.SrcImage)
participant Manifest as Manifest Parser (imgmanifest)
participant Mapper as mapping writer / missing-image tracker
CLI->>Parser: Parse image reference (source or docker:// fallback)
Parser-->>CLI: parsed reference
CLI->>ImageSrc: Open image reference (system context)
ImageSrc->>Manifest: Fetch manifest bytes & media type
Manifest-->>CLI: manifest bytes, media type
alt manifest is multi-image (manifest list)
CLI->>Manifest: Parse index -> instance digests
Manifest-->>CLI: [digest1, digest2, ...]
loop per instance digest
CLI->>Mapper: write "source@digest = subDest" (subDigestDestination)
Mapper-->>CLI: mapping recorded
CLI->>Mapper: add sub-digest to missing-image entries if needed
end
else single-image
CLI->>Mapper: write base "source = destination"
Mapper-->>CLI: mapping recorded
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/cli/dryrun_test.go (1)
189-190:⚠️ Potential issue | 🟡 MinorPre-existing:
assert.FileExistson line 190 checksmappingPathinstead ofmissingImgsPath.This appears to be a copy-paste bug in the existing test — it asserts existence of
mappingPath(already checked on line 177) instead of the newly constructedmissingImgsPath. Not introduced by this PR, but worth fixing while you're here.Proposed fix
- assert.FileExists(t, mappingPath) + assert.FileExists(t, missingImgsPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun_test.go` around lines 189 - 190, The test currently constructs missingImgsPath but then mistakenly calls assert.FileExists(t, mappingPath) again; update the assertion to check the correct variable by replacing the second assert.FileExists invocation to assert.FileExists(t, missingImgsPath) so the test verifies the presence of the missing images file (variable missingImgsPath) rather than re-checking mappingPath.
🧹 Nitpick comments (2)
internal/pkg/cli/dryrun_test.go (1)
295-296: Nit:os.RemoveAllis redundant fort.TempDir().
t.TempDir()already registers automatic cleanup when the test ends. The explicitdefer os.RemoveAll(testFolder)is unnecessary. The same pattern also appears on line 48 and line 120 in the pre-existing tests.Proposed fix
testFolder := t.TempDir() - defer os.RemoveAll(testFolder)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun_test.go` around lines 295 - 296, The test creates testFolder via t.TempDir() and then redundantly calls defer os.RemoveAll(testFolder); remove the unnecessary explicit cleanup calls (the defer os.RemoveAll(...) lines) so the tests rely on t.TempDir()'s automatic cleanup—search for occurrences around testFolder/t.TempDir() and delete the os.RemoveAll usages.internal/pkg/cli/dryrun.go (1)
44-57: Performance:getManifestListDigestsis called for every image, adding network I/O to the dry-run path.For
docker://sources this opens a connection and fetches the manifest from a remote registry. In large image sets (hundreds/thousands of images), this could significantly degrade dry-run speed. Consider:
- Parallelizing the manifest lookups (e.g., bounded goroutine pool), or
- Adding a flag/option to opt into sub-digest expansion, or
- Caching the system context and reusing connections where possible.
Not a blocker since errors are gracefully handled and logged at debug level, but worth considering for large-scale usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun.go` around lines 44 - 57, The dry-run loop currently calls getManifestListDigests for every image (in internal/pkg/cli/dryrun.go), causing remote registry network I/O per image; change this by adding an opt-in flag (e.g., --expand-subdigests) checked in the dry-run path and only call getManifestListDigests when enabled, and/or replace the sequential calls with a bounded goroutine worker pool that performs getManifestListDigests concurrently for images (use a shared cached system context/reused HTTP client across workers to avoid repeated connection setup), then collect results and produce the same subSource -> destination writes to buff as before; ensure getManifestListDigests is called from the worker pool and errors remain debug-logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/pkg/cli/dryrun_test.go`:
- Around line 189-190: The test currently constructs missingImgsPath but then
mistakenly calls assert.FileExists(t, mappingPath) again; update the assertion
to check the correct variable by replacing the second assert.FileExists
invocation to assert.FileExists(t, missingImgsPath) so the test verifies the
presence of the missing images file (variable missingImgsPath) rather than
re-checking mappingPath.
---
Nitpick comments:
In `@internal/pkg/cli/dryrun_test.go`:
- Around line 295-296: The test creates testFolder via t.TempDir() and then
redundantly calls defer os.RemoveAll(testFolder); remove the unnecessary
explicit cleanup calls (the defer os.RemoveAll(...) lines) so the tests rely on
t.TempDir()'s automatic cleanup—search for occurrences around
testFolder/t.TempDir() and delete the os.RemoveAll usages.
In `@internal/pkg/cli/dryrun.go`:
- Around line 44-57: The dry-run loop currently calls getManifestListDigests for
every image (in internal/pkg/cli/dryrun.go), causing remote registry network I/O
per image; change this by adding an opt-in flag (e.g., --expand-subdigests)
checked in the dry-run path and only call getManifestListDigests when enabled,
and/or replace the sequential calls with a bounded goroutine worker pool that
performs getManifestListDigests concurrently for images (use a shared cached
system context/reused HTTP client across workers to avoid repeated connection
setup), then collect results and produce the same subSource -> destination
writes to buff as before; ensure getManifestListDigests is called from the
worker pool and errors remain debug-logged.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
internal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.go
dorzel
force-pushed
the
OCPBUGS-66263
branch
from
06bc475 to
1a213a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/cli/dryrun.go (1)
59-68:⚠️ Potential issue | 🟡 MinorSub-digest entries are not checked against the local cache
Sub-digest lines are now written to mapping.txt but are never checked via
o.Mirror.Checkand are never added tomissingImgsBuff. In a partial-cache scenario (e.g. the manifest list index is present but one or more architecture blobs are absent), these missing entries will go unreported, and the warning counter on line 87 will undercount.Consider extending the cache-check loop to cover the expanded sub-digest entries, or at a minimum add a comment explaining that sub-digest cache validation is intentionally deferred to the full mirror run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun.go` around lines 59 - 68, The current loop only checks img.Destination against the local cache (o.Mirror.Check) and therefore ignores any expanded sub-digest entries written into mapping.txt; update the logic in the dry-run path so that after expanding an image into sub-digest destinations you iterate each sub-digest destination and call o.Mirror.Check(ctx, subDest, o.Opts, false), and for any err or !exists write the mapping (use img.Source or the appropriate sub-source mapping) into missingImgsBuff and increment nbMissingImgs; if intentional deferral is required instead, add a clear comment near o.Opts.IsMirrorToDisk() explaining that sub-digest cache validation is deferred to the full mirror run and why.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pkg/cli/dryrun.go`:
- Around line 45-57: Currently o.getManifestListDigests is called for every
img.Source during dry-run which triggers a full remote manifest fetch per image;
update the logic to avoid unnecessary network calls by only invoking
o.getManifestListDigests when the source is not already digest-pinned (i.e.,
img.Source does not contain a digest "@...") or when a new boolean flag on the
CopyImageSchema (e.g., IsManifestList) is unset — alternatively compute and set
IsManifestList during the collection phase where manifests are already fetched
and consult that flag here; also change the logger call from o.Log.Debug(...) to
o.Log.Warn(...) when getManifestListDigests returns an error so users see
missing sub-digests without debug logging.
- Around line 52-56: The current loop writing sub-digest mappings uses
sourceBase (from img.Source) but leaves img.Destination with its tag, causing
each arch-specific line to map to the same dest tag and overwrite; update the
loop that iterates manifestDigests (the block building subSource and calling
buff.WriteString) to compute a destBase that has any tag removed before
appending the sub-digest. Implement tag-stripping for img.Destination by
removing the last ":tag" occurrence (i.e., split at the last ':' after the final
'/') or, better, reuse the same image-reference parsing used for img.Source to
produce a destination reference without tag, then append "@"+digest so lines
become destBase+"@"+digest; use that destBase when writing buff.WriteString
instead of the original img.Destination.
- Around line 98-106: getManifestListDigests currently calls
alltransports.ParseImageName and fails for Cincinnati-style image names that
lack a transport prefix; update getManifestListDigests to detect this case and
retry parsing by prepending "docker://" when alltransports.ParseImageName
returns a transport-not-found/parse error (or when source does not contain
"://"), so that images like
"quay.io/openshift-release-dev/ocp-release:4.13.10-x86_64" are parsed and
manifest-list expansion proceeds; keep the original error handling for other
parse failures and ensure the retry only happens when the input appears to lack
a transport prefix.
---
Outside diff comments:
In `@internal/pkg/cli/dryrun.go`:
- Around line 59-68: The current loop only checks img.Destination against the
local cache (o.Mirror.Check) and therefore ignores any expanded sub-digest
entries written into mapping.txt; update the logic in the dry-run path so that
after expanding an image into sub-digest destinations you iterate each
sub-digest destination and call o.Mirror.Check(ctx, subDest, o.Opts, false), and
for any err or !exists write the mapping (use img.Source or the appropriate
sub-source mapping) into missingImgsBuff and increment nbMissingImgs; if
intentional deferral is required instead, add a clear comment near
o.Opts.IsMirrorToDisk() explaining that sub-digest cache validation is deferred
to the full mirror run and why.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
internal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/pkg/cli/dryrun_test.go
aguidirh
reviewed
Feb 24, 2026
Contributor
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks @dorzel, I've added some comments, I'm wondering if we should include the child digests of the manifest list only when the customer asks for it (maybe a flag where they can select the archs or all) ?
I would like to discuss this option with the team (@r4f4 and @adolfo-ab) to avoid breaking changes. I know some customers currently are depending on the current mappings.txt to copy the images with skopeo.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/cli/dryrun.go (1)
73-100:⚠️ Potential issue | 🟡 MinorUse a consistent total when reporting missing images.
nbMissingImgsis incremented for sub-digest entries (Line 77), but the denominator remainslen(allImages)(Line 99). This can produce misleading ratios (e.g.,8/3).Proposed fix
- nbMissingImgs := 0 + nbMissingImgs := 0 + totalMappings := 0 @@ for _, img := range allImages { buff.WriteString(img.Source + "=" + img.Destination + "\n") + totalMappings++ @@ for _, digest := range manifestDigests { subSource := sourceBase + "@" + digest subDest := subDigestDestination(img.Destination, digest) buff.WriteString(subSource + "=" + subDest + "\n") + totalMappings++ subDigestEntries = append(subDigestEntries, subDigestEntry{source: subSource, dest: subDest}) } } @@ - o.Log.Warn(emoji.Warning+" %d/%d images necessary for mirroring are not available in the cache.", nbMissingImgs, len(allImages)) + o.Log.Warn(emoji.Warning+" %d/%d images necessary for mirroring are not available in the cache.", nbMissingImgs, totalMappings)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun.go` around lines 73 - 100, The warning uses nbMissingImgs for the numerator but len(allImages) for the denominator which is inconsistent because nbMissingImgs is incremented for sub-digest entries; fix by computing a consistent total (e.g., totalExpectedImgs) before writing warnings: initialize totalExpectedImgs := len(allImages), when iterating subDigestEntries add len(subDigestEntries) (or increment totalExpectedImgs in the same loop where nbMissingImgs is incremented), then use totalExpectedImgs as the denominator in o.Log.Warn instead of len(allImages); reference nbMissingImgs, subDigestEntries, allImages and the two o.Log.Warn calls to update the message.
♻️ Duplicate comments (1)
internal/pkg/cli/dryrun.go (1)
51-55:⚠️ Potential issue | 🟠 MajorAvoid unconditional manifest lookups in the dry-run image loop.
ExecutorSchema.DryRunstill callsExecutorSchema.getManifestListDigestsfor every image entry. That introduces one remote manifest fetch per image and can significantly slow dry-runs or hit registry limits at scale.#!/bin/bash # Verify unconditional invocation of getManifestListDigests inside the per-image loop. cat -n internal/pkg/cli/dryrun.go | sed -n '44,70p' # Confirm call site(s) and surrounding guards. rg -nP --type=go -C3 'getManifestListDigests\(ctx,\s*img\.Source\)' internal/pkg/cli/dryrun.go
🧹 Nitpick comments (1)
internal/pkg/cli/dryrun_test.go (1)
314-321: Fail fast on registry setup errors in this test path.Using
t.Errorfduring test bootstrap lets execution continue after setup failure, which can obscure the real failure cause.Proposed fix
regCfg, err := setupRegForTest(testFolder) if err != nil { - t.Errorf("storage cache error: %v ", err) + t.Fatalf("storage cache error: %v", err) } reg, err := registry.NewRegistry(context.Background(), regCfg) if err != nil { - t.Errorf("storage cache error: %v ", err) + t.Fatalf("storage cache error: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun_test.go` around lines 314 - 321, The test currently uses t.Errorf when setupRegForTest(testFolder) or registry.NewRegistry(...) fails, which allows the test to continue; change both t.Errorf calls to t.Fatalf (or call t.FailNow) so the test aborts immediately on setup failure, and include context in the message referencing setupRegForTest/regCfg and registry.NewRegistry/reg to make the failure point clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/pkg/cli/dryrun.go`:
- Around line 73-100: The warning uses nbMissingImgs for the numerator but
len(allImages) for the denominator which is inconsistent because nbMissingImgs
is incremented for sub-digest entries; fix by computing a consistent total
(e.g., totalExpectedImgs) before writing warnings: initialize totalExpectedImgs
:= len(allImages), when iterating subDigestEntries add len(subDigestEntries) (or
increment totalExpectedImgs in the same loop where nbMissingImgs is
incremented), then use totalExpectedImgs as the denominator in o.Log.Warn
instead of len(allImages); reference nbMissingImgs, subDigestEntries, allImages
and the two o.Log.Warn calls to update the message.
---
Nitpick comments:
In `@internal/pkg/cli/dryrun_test.go`:
- Around line 314-321: The test currently uses t.Errorf when
setupRegForTest(testFolder) or registry.NewRegistry(...) fails, which allows the
test to continue; change both t.Errorf calls to t.Fatalf (or call t.FailNow) so
the test aborts immediately on setup failure, and include context in the message
referencing setupRegForTest/regCfg and registry.NewRegistry/reg to make the
failure point clear.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
internal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.go
|
@dorzel: An error was encountered querying GitHub for users with public email (ngavali@redhat.com) for bug OCPBUGS-66263 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: connect: connection refused
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by @MayXuQQ |
openshift-ci-robot
added
the
verified
|
@MayXuQQ: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
aguidirh
reviewed
Mar 6, 2026
Contributor
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
With the following ImageSetConfig:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v1alpha2
mirror:
platform:
channels:
- name: stable-4.17
minVersion: 4.17.0
maxVersion: 4.17.0
- name: stable-4.16
minVersion: 4.16.0
maxVersion: 4.16.0
graph: true
...ommited operators/additionalImages/Helm
I got the following error on the graph image:
2026/03/06 17:21:27 [WARN] : unable to get manifest list info for docker://localhost:55000/openshift/graph-image:latest: error creating image source for docker://localhost:55000/openshift/graph-image:latest: pinging container registry localhost:55000: Get "https://localhost:55000/v2/": http: server gave HTTP response to HTTPS client
aguidi@fedora-redhat:~/go/src/github.com/aguidirh/oc-mirror$ ./bin/oc-mirror -c ./alex-tests/alex-isc/isc2.yaml file://alex-tests/ocpbugs66263 --v2 --dry-run
2026/03/06 16:58:45 [INFO] : 👋 Hello, welcome to oc-mirror
2026/03/06 16:58:45 [INFO] : ⚙️ setting up the environment for you...
2026/03/06 16:58:45 [INFO] : ⚙️ environment version: v0.2.0-alpha.1-541-ga51c524
2026/03/06 16:58:45 [INFO] : 🔀 workflow mode: mirrorToDisk
2026/03/06 16:58:46 [INFO] : 🕵 going to discover the necessary images...
2026/03/06 16:58:46 [INFO] : 🔍 collecting release images...
✓ (22s) Collecting release quay.io/openshift-release-dev/ocp-release:4.17.0-x86_64
✓ (36s) Collecting release quay.io/openshift-release-dev/ocp-release:4.16.0-x86_64
✓ (28s) Collecting release quay.io/openshift-release-dev/ocp-release:4.16.55-x86_64
2026/03/06 17:02:35 [INFO] : 🔍 collecting operator images...
✓ (1m54s) Collecting catalog registry.redhat.io/redhat/redhat-operator-index:v4.20
✓ (2m57s) Collecting catalog registry.redhat.io/redhat/redhat-operator-index:v4.18
✓ (41s) Collecting catalog oci:///home/aguidi/go/src/github.com/aguidirh/oc-mirror/alex-tests/redhat-operator-index-oci
✓ (1s) Collecting catalog oci:///home/aguidi/go/src/github.com/aguidirh/oc-mirror/alex-tests/ibm-catalog
2026/03/06 17:08:10 [INFO] : 🔍 collecting additional images...
2026/03/06 17:08:10 [WARN] : [AdditionalImagesCollector] registry.redhat.io/ubi8/ubi:latest@sha256:44d75007b39e0e1bbf1bcfd0721245add54c54c3f83903f8926fb4bef6827aa2 has both tag and digest : using digest to pull, but tag only for mirroring
2026/03/06 17:08:10 [INFO] : 🔍 collecting helm images...
2026/03/06 17:21:27 [WARN] : unable to get manifest list info for docker://localhost:55000/openshift/graph-image:latest: error creating image source for docker://localhost:55000/openshift/graph-image:latest: pinging container registry localhost:55000: Get "https://localhost:55000/v2/": http: server gave HTTP response to HTTPS client
2026/03/06 17:22:40 [WARN] : ⚠️ 627/628 images necessary for mirroring are not available in the cache.
2026/03/06 17:22:40 [WARN] : List of missing images in : alex-tests/ocpbugs66263/working-dir/dry-run/missing.txt.
please re-run the mirror to disk process
2026/03/06 17:22:40 [INFO] : 📄 list of all images for mirroring in : alex-tests/ocpbugs66263/working-dir/dry-run/mapping.txt
2026/03/06 17:22:40 [INFO] : mirror time : 23m54.144253148s
2026/03/06 17:22:40 [INFO] : 👋 Goodbye, thank you for using oc-mirror
Also I realized that the graph image was included in the mapping.txt but not in the missing.txt
The WARN comes from L54
Could you please take a look?
Contributor
|
Another thing I would like you to check is about performance, I ran the same image set config in main branch in the PR. Main branch above took 9 minutes, see below: While in the PR it took 23 minutes, see below: This is happening because we are calling the getManifestListDigests for all the images. One way to improve the performance is calling In order to do that, during the collection you could create a slice in the struct If during the collection the children of the manifest list are already available, it could be also an idea to add them to the slice instead of the manifest list itself. Feel free to contact me if you have questions about it. This is related to this code rabbit potential issue. |
openshift-ci-robot
added
jira/invalid-bug
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is invalid:
Comment DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Member
Author
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: MayXuQQ. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/operator/filtered_collector.go (1)
349-385:⚠️ Potential issue | 🟡 MinorDon't silently swallow
ListFromBloberrors; digest-only catalogs can also short-circuit after list detection.Two small points on
getCatalogDigest:
- Lines 374–382: the
ListFromBloberror is silently discarded. If it fails for a legitimate multi-image manifest, the pre-collection optimization that motivated this collector-side change is lost and dry-run will fall back to re-inspecting the same catalog downstream — without any signal that pre-collection failed. A single debug/warn log would make this diagnosable without changing behavior.- Line 349 vs lines 367–371: for digest-only catalogs during dry-run you already have
imgSpec.Digestin hand; re-computing it viaimgmanifest.Digest(manifestBytes).Encoded()is redundant (and relies on the registry returning byte-for-byte canonical manifest bytes to match). You can still fetch/list-detect, but returnimgSpec.Digestdirectly as the catalog digest.Suggested diff
- if o.Opts.IsDryRun && imgmanifest.MIMETypeIsMultiImage(manifestType) { - list, err := imgmanifest.ListFromBlob(manifestBytes, manifestType) - if err == nil { - instances := list.Instances() - digests := make([]string, 0, len(instances)) - for _, inst := range instances { - digests = append(digests, inst.String()) - } - copyImageSchemaMap.ManifestListDigests[imgSpec.ReferenceWithTransport] = digests - } - } - - return catalogDigest, nil + if o.Opts.IsDryRun && imgmanifest.MIMETypeIsMultiImage(manifestType) { + list, err := imgmanifest.ListFromBlob(manifestBytes, manifestType) + if err != nil { + o.Log.Debug("failed to parse manifest list for %s, dry-run will re-inspect: %v", imgSpec.ReferenceWithTransport, err) + } else { + instances := list.Instances() + digests := make([]string, 0, len(instances)) + for _, inst := range instances { + digests = append(digests, inst.String()) + } + copyImageSchemaMap.ManifestListDigests[imgSpec.ReferenceWithTransport] = digests + } + } + + if imgSpec.IsImageByDigestOnly() { + return imgSpec.Digest, nil + } + return catalogDigest, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/operator/filtered_collector.go` around lines 349 - 385, In getCatalogDigest (in filtered_collector.go) avoid recomputing the catalog digest for digest-only images and surface ListFromBlob failures: when imgSpec.IsImageByDigestOnly() && o.Opts.IsDryRun return imgSpec.Digest immediately instead of calling imgmanifest.Digest(manifestBytes).Encoded(); and when calling imgmanifest.ListFromBlob(manifestBytes, manifestType) do not silently ignore errors — log the error (debug or warn) with context (e.g., the reference imgSpec.ReferenceWithTransport) before proceeding, and only populate copyImageSchemaMap.ManifestListDigests on success as you already do.
🧹 Nitpick comments (2)
internal/pkg/operator/filtered_collector_test.go (1)
1033-1036: Consider adding a failure toggle forImageManifest(minor test-coverage gap).The updated mock always returns a valid single-arch manifest and
nilerror. The existingFailImageManifestflag only gatesGetImageManifest/GetOCIImageManifest, not this method. As a result, the new error paths added togetCatalogDigestinfiltered_collector.go(ImageManifestfailure,imgmanifest.Digestfailure, multi-image branch) aren't reachable from any test that uses this mock. Adding something like aFailImageManifestFetchtoggle (and/or a configurable payload) would let you cover the new error/multi-image paths.Not blocking — flagging for future coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/operator/filtered_collector_test.go` around lines 1033 - 1036, The mock MockManifest.ImageManifest currently always returns a valid manifest and nil error, leaving the new error/multi-image branches in getCatalogDigest (in filtered_collector.go) untested; add a boolean flag (e.g., FailImageManifestFetch) to the MockManifest struct and have ImageManifest check it and return a non-nil error when set, and optionally allow injecting a configurable manifest payload (or a list of manifests) so tests can exercise imgmanifest.Digest failures and the multi-image branch; update tests to toggle FailImageManifestFetch (and payload) to cover those paths.internal/pkg/cli/dryrun_test.go (1)
406-410: Nit:.invalidis reserved but a DNS lookup still happens.Using the RFC 2606
.invalidTLD is the right choice for an unresolvable hostname, but on hosts with captive DNS or certain corporate resolvers the lookup can still take a few seconds before failing. If this test is observed to be slow in CI, consider pairing withopts.CommandTimeoutor pointingSourceat a127.0.0.1:<unused-port>instead. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun_test.go` around lines 406 - 410, The test uses hostnames under the .invalid TLD in the imgs slice (v2alpha1.CopyImageSchema entries with Source built via consts.DockerProtocol) which can still incur DNS resolution delays on some CI environments; update the test to avoid DNS lookup latency by either (A) setting the invocation to use opts.CommandTimeout (or equivalent command timeout in the test harness) around the manifest inspection calls that use imgs so the test fails fast, or (B) change the Source fields in the imgs array to point to 127.0.0.1:<unused-port> (loopback with a port guaranteed unused) instead of fake-registry.invalid to eliminate external DNS resolution; adjust references to CopyImageSchema.Source and the imgs variable accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/pkg/operator/filtered_collector.go`:
- Around line 349-385: In getCatalogDigest (in filtered_collector.go) avoid
recomputing the catalog digest for digest-only images and surface ListFromBlob
failures: when imgSpec.IsImageByDigestOnly() && o.Opts.IsDryRun return
imgSpec.Digest immediately instead of calling
imgmanifest.Digest(manifestBytes).Encoded(); and when calling
imgmanifest.ListFromBlob(manifestBytes, manifestType) do not silently ignore
errors — log the error (debug or warn) with context (e.g., the reference
imgSpec.ReferenceWithTransport) before proceeding, and only populate
copyImageSchemaMap.ManifestListDigests on success as you already do.
---
Nitpick comments:
In `@internal/pkg/cli/dryrun_test.go`:
- Around line 406-410: The test uses hostnames under the .invalid TLD in the
imgs slice (v2alpha1.CopyImageSchema entries with Source built via
consts.DockerProtocol) which can still incur DNS resolution delays on some CI
environments; update the test to avoid DNS lookup latency by either (A) setting
the invocation to use opts.CommandTimeout (or equivalent command timeout in the
test harness) around the manifest inspection calls that use imgs so the test
fails fast, or (B) change the Source fields in the imgs array to point to
127.0.0.1:<unused-port> (loopback with a port guaranteed unused) instead of
fake-registry.invalid to eliminate external DNS resolution; adjust references to
CopyImageSchema.Source and the imgs variable accordingly.
In `@internal/pkg/operator/filtered_collector_test.go`:
- Around line 1033-1036: The mock MockManifest.ImageManifest currently always
returns a valid manifest and nil error, leaving the new error/multi-image
branches in getCatalogDigest (in filtered_collector.go) untested; add a boolean
flag (e.g., FailImageManifestFetch) to the MockManifest struct and have
ImageManifest check it and return a non-nil error when set, and optionally allow
injecting a configurable manifest payload (or a list of manifests) so tests can
exercise imgmanifest.Digest failures and the multi-image branch; update tests to
toggle FailImageManifestFetch (and payload) to cover those paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf8bc58f-f8cd-4681-bde2-a248da9f22e0
📒 Files selected for processing (6)
internal/pkg/api/v2alpha1/type_internal.gointernal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.gointernal/pkg/cli/executor.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/pkg/cli/dryrun.go
Contributor
|
/retest |
aguidirh
reviewed
Apr 23, 2026
…st list digests during collection
|
@dorzel: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds functionality to include index image sub-digests in mapping.txt, as part of the linked bug. This facilitates users who manually mirror based on mapping.txt or use automated security scanning which requires all digests to be listed.
Github / Jira issue: https://issues.redhat.com/browse/OCPBUGS-66263
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Corresponding
mapping.txt:Expected Outcome
All sub-digests of any index image in the mapping.txt are listed directly below it.
Summary by CodeRabbit
New Features
Tests