OKD-194: Fix manifest list detection for single-arch release images#1062
OKD-194: Fix manifest list detection for single-arch release images#1062jatinsu wants to merge 3 commits intoopenshift:masterfrom
Conversation
Detect manifest lists by checking the manifest type directly (Manifest::ML variant) instead of counting the number of architectures. This fixes scraping of release images that are manifest lists with a single architecture (e.g. OKD SCOS releases), which previously failed with 404 errors when the graph builder mistakenly tried to fetch manifest-reference digests as blobs.
openshift-ci-robot
added
the
jira/valid-reference
|
@jatinsu: This pull request references OKD-194 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
Walkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jatinsu 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs`:
- Around line 268-280: The code currently calls arch.as_ref().unwrap() which can
panic because arch is Option<String> (set to None when manifest.architectures()
fails); replace the unsafe unwrap by matching on arch or using arch.as_deref() /
map to safely handle the None case before using the value (e.g., if let Some(a)
= arch.as_deref() { ... } or arch.as_deref().unwrap_or("unknown") as
appropriate) in the function that builds the image metadata (referencing the
arch variable and the surrounding manifest handling logic).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c26a3298-8bb5-41a7-81ea-a56566bfc5e9
📒 Files selected for processing (2)
cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rsdocs/design/openshift.md
cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs
Outdated
Show resolved
Hide resolved
|
/retest-required |
1 similar comment
|
/retest-required |
|
@jatinsu: This pull request references OKD-194 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
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)
cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs (1)
339-375:⚠️ Potential issue | 🟠 MajorKeep child-manifest failures scoped to the current tag.
The child-manifest lookup bypasses the graceful per-tag failure handling used for the outer manifest lookup: an empty child list still panics via
.expect(), and a nested child-manifest lookup failure escapes the callback via.await?instead of being handled and counted like outer failures. Since this path runs for manifest lists, one malformed or transiently unavailable child manifest will interrupt the entire release scrape instead of being skipped.Suggested fix
if is_manifest_list { - let digest = layers_digests - .first() - .map(std::string::ToString::to_string) - .expect( - format!("no images referenced in ManifestList ref:{}", manifestref) - .as_str(), - ); + let digest = match layers_digests.first() { + Some(digest) => digest.to_string(), + None => { + error!( + "no images referenced in ManifestList ref:{} for {}:{}", + manifestref, &repo, &tag + ); + skip_releases.fetch_add(1, Ordering::SeqCst); + return Ok(()); + } + }; // TODO: destructured assignments are unstable in current rust, after updating rust // change this to (_,_,layers_digests) and remove separate assignment from below. - let (_ml_arch, _ml_manifestref, ml_layers_digests, _ml_is_manifest_list) = - get_manifest_layers(digest, &repo, ®istry_client).await?; + let (_ml_arch, _ml_manifestref, ml_layers_digests, _ml_is_manifest_list) = + match get_manifest_layers(digest, &repo, ®istry_client).await { + Ok(result) => result, + Err(e) => { + error!( + "fetching child manifest for manifest list {}:{}: {}", + &repo, &tag, e + ); + skip_releases.fetch_add(1, Ordering::SeqCst); + return Ok(()); + } + }; layers_digests = ml_layers_digests; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs` around lines 339 - 375, The child-manifest lookup currently panics with .expect() when layers_digests.first() is empty and propagates errors using .await? instead of handling them per-tag; update the is_manifest_list branch to first check layers_digests.first() and if None log an error for repo:tag, increment skip_releases.fetch_add(1, Ordering::SeqCst) and return Ok(()), and replace the get_manifest_layers(digest, &repo, ®istry_client).await? call with a match (or if let) that on Err(e) logs the failure for the child manifest (including repo and tag), increments skip_releases.fetch_add(1, Ordering::SeqCst) (or sig_releases if it’s a signature by checking tag.contains(".sig")), and returns Ok(()); on Ok(...) assign ml_layers_digests to layers_digests as before. Ensure you reference get_manifest_layers, layers_digests, manifestref, is_manifest_list, sig_releases and skip_releases to implement the scoped per-tag error handling.
🤖 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
`@cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs`:
- Around line 339-375: The child-manifest lookup currently panics with .expect()
when layers_digests.first() is empty and propagates errors using .await? instead
of handling them per-tag; update the is_manifest_list branch to first check
layers_digests.first() and if None log an error for repo:tag, increment
skip_releases.fetch_add(1, Ordering::SeqCst) and return Ok(()), and replace the
get_manifest_layers(digest, &repo, ®istry_client).await? call with a match
(or if let) that on Err(e) logs the failure for the child manifest (including
repo and tag), increments skip_releases.fetch_add(1, Ordering::SeqCst) (or
sig_releases if it’s a signature by checking tag.contains(".sig")), and returns
Ok(()); on Ok(...) assign ml_layers_digests to layers_digests as before. Ensure
you reference get_manifest_layers, layers_digests, manifestref,
is_manifest_list, sig_releases and skip_releases to implement the scoped per-tag
error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d90bf9a-568a-4a38-9fdc-dd6393623b51
📒 Files selected for processing (1)
cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs (1)
257-257: Consider replacing the 4-tuple return with a named struct
get_manifest_layersnow returns a positional 4-tuple that is already being partially ignored at call sites. A small struct would make call sites safer and easier to evolve.♻️ Suggested refactor sketch
+struct ManifestLayers { + arch: Option<String>, + manifestref: String, + layers_digests: Vec<String>, + is_manifest_list: bool, +} - -async fn get_manifest_layers(...) -> Result<(Option<String>, String, Vec<String>, bool), Error> { +async fn get_manifest_layers(...) -> Result<ManifestLayers, Error> { ... - Ok((arch, manifestref, layers_digests, is_manifest_list)) + Ok(ManifestLayers { arch, manifestref, layers_digests, is_manifest_list }) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 295-295, 339-340, 377-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs` at line 257, The function get_manifest_layers returns a positional 4-tuple (Option<String>, String, Vec<String>, bool) which is hard to read and error-prone at call sites; replace that tuple with a small named struct (e.g., ManifestLayers { config: Option<String>, digest: String, layers: Vec<String>, is_manifest_list: bool }) and update get_manifest_layers signature and all call sites to return and destructure that struct (including callers indicated around get_manifest_layers lines and locations where the tuple is partially ignored) so callers use field names instead of positional indexes, improving clarity and easing future evolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs`:
- Line 257: The function get_manifest_layers returns a positional 4-tuple
(Option<String>, String, Vec<String>, bool) which is hard to read and
error-prone at call sites; replace that tuple with a small named struct (e.g.,
ManifestLayers { config: Option<String>, digest: String, layers: Vec<String>,
is_manifest_list: bool }) and update get_manifest_layers signature and all call
sites to return and destructure that struct (including callers indicated around
get_manifest_layers lines and locations where the tuple is partially ignored) so
callers use field names instead of positional indexes, improving clarity and
easing future evolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49614b3c-6ce8-4525-bf8e-1fc1d49a6b42
📒 Files selected for processing (1)
cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs
|
@jatinsu: 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. |
2 participants
This PR fixes scraping of release images that are manifest lists with a single architecture (for example OKD releases), which would previously fail with a 404 error when the graph builder mistakenly tried to fetch manifest-reference digests as blobs.