OCPBUGS-66263: Include index image sub-digests in dry run mapping.txt#1355
OCPBUGS-66263: Include index image sub-digests in dry run mapping.txt#1355dorzel wants to merge 1 commit 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. |
|
/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
|
/retest |
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration 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)
WalkthroughDry-run now detects OCI manifest lists, expands them into per-instance digest source->destination mappings (including digest-pinned destinations), and records sub-digests for missing-image tracking. Tests add an OCI layout builder and verify sub-digest mapping and destination generation. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as Dry-Run Executor
participant Parser as alltransports.ParseImageName
participant Transport as containers/image Source
participant Manifest as Manifest Parser
participant Mapper as Mapping Writer
Executor->>Parser: Parse image name (source)
Parser-->>Executor: reference (or fallback)
Executor->>Transport: Open image source (system context)
Transport->>Manifest: Fetch manifest bytes
Manifest-->>Executor: manifest bytes
alt manifest is a manifest list
Executor->>Manifest: Parse index -> instance digests
Manifest-->>Executor: [digest1, digest2, ...]
loop for each digest
Executor->>Mapper: write "source@digest = sub-destination"
Mapper-->>Executor: mapping written
end
else single image
Executor->>Mapper: write single mapping
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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
| // Try to get manifest list sub-digests from source | ||
| manifestDigests, err := o.getManifestListDigests(ctx, img.Source) | ||
| if err != nil { | ||
| o.Log.Debug("unable to get manifest list info for %s: %v", img.Source, err) | ||
| } else if len(manifestDigests) > 0 { | ||
| // This is a manifest list, write each sub-digest | ||
| // Remove the digest suffix (if any) to get the base reference | ||
| sourceBase, _, _ := strings.Cut(img.Source, "@") | ||
| for _, digest := range manifestDigests { | ||
| subSource := sourceBase + "@" + digest | ||
| buff.WriteString(subSource + "=" + img.Destination + "\n") | ||
| } | ||
| } |
There was a problem hiding this comment.
O(n) network calls per dry run degrades performance and may hit rate limits
getManifestListDigests opens a full network connection (TLS handshake, auth, manifest fetch) for every entry in allImages, unconditionally, on every dry run. GetManifest is noted in the library as a call that "may use a remote (= slow) service." For image sets with dozens or hundreds of entries this is a significant regression in dry run responsiveness, and sequential manifest pulls against rate-limited registries (e.g. registry.access.redhat.com, registry.redhat.io) can exhaust quotas mid-run.
Consider whether this lookup can be gated — e.g. only invoked when a tag-based source reference is present (a digest-pinned source is already expanded; no lookup needed), or whether the CopyImageSchema structs could carry a pre-computed IsManifestList flag set during the collection phase where the manifest is already fetched.
Additionally, when getManifestListDigests returns an error the failure is logged only at Debug level (line 48). For an actual connectivity or auth problem against a real manifest-list image, users running without debug logging will see a silently truncated mapping.txt with no indication that sub-digests are missing. Elevating to Warn would be more appropriate.
🤖 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 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.
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. |
|
@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. |
4 participants
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