From 9f07d4fa99ba96b693df5b620ea8c35b0bcda1fa Mon Sep 17 00:00:00 2001 From: pratikmahajan Date: Wed, 14 May 2025 21:14:54 +0000 Subject: [PATCH 1/3] ignore single-arch shards of multi-arch images Current production Cincinnati occasionally confuses arch-specific shards of multi-arch images as single-arch releases Possibly triggered by a race or other buggy handling of Quay 502s while scraping releases. fix: check if the metadata is multi for layered and registry and ignore those single arch shards Co-authored-by: wking --- .../plugins/internal/graph_builder/release.rs | 18 ++++++++++++++++++ .../release_scrape_dockerv2/registry/mod.rs | 11 ++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/cincinnati/src/plugins/internal/graph_builder/release.rs b/cincinnati/src/plugins/internal/graph_builder/release.rs index 5800b0d8a..56cd3826f 100644 --- a/cincinnati/src/plugins/internal/graph_builder/release.rs +++ b/cincinnati/src/plugins/internal/graph_builder/release.rs @@ -52,6 +52,24 @@ pub struct Metadata { pub metadata: MapImpl, } +impl Metadata { + /// return the arch identifier set in metadata or returns `none` + pub fn metadata_arch_id(&mut self) -> (String, String) { + let layer_metadata_arch_key = "release.openshift.io/architecture"; + let registry_manifest_arch_key = "io.openshift.upgrades.graph.release.arch"; + let no_arch = "none".to_string(); + let layer_metadata_arch = self + .metadata + .get(layer_metadata_arch_key) + .map_or(no_arch.clone(), |s| s.clone()); + let registry_manifest_arch = self + .metadata + .get(registry_manifest_arch_key) + .map_or(no_arch, |s| s.clone()); + return (layer_metadata_arch, registry_manifest_arch); + } +} + impl fmt::Display for Metadata { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( diff --git a/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs b/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs index e9cca440d..4e151ec75 100644 --- a/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs +++ b/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs @@ -386,7 +386,16 @@ pub async fn fetch_releases( ) .await? { - Some(release) => release, + Some(mut release) => { + let (layer_metadata_arch, registry_manifest_arch) = + release.metadata.metadata_arch_id(); + if layer_metadata_arch == "multi" && registry_manifest_arch != "multi" { + // single arch shard of multiarch release. dont want it + return Ok(()); + } else { + release + } + } None => { // Reminder: this means the layer_digests point to layers // without any release and we've cached this before From fe23e23cfc6caf90ddbb9d96b4645afee0999e41 Mon Sep 17 00:00:00 2001 From: pratikmahajan Date: Wed, 14 May 2025 21:39:21 +0000 Subject: [PATCH 2/3] Revert "do not panic on sha-mismatch for multi-arch images" This reverts commit 97137474ec07cbb808c214e7cc7d0d14fbeb6dac. --- cincinnati/src/lib.rs | 34 +++++++--------------------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/cincinnati/src/lib.rs b/cincinnati/src/lib.rs index 905c8f987..034378034 100644 --- a/cincinnati/src/lib.rs +++ b/cincinnati/src/lib.rs @@ -87,20 +87,6 @@ impl Release { _ => bail!("could not get manifest reference"), } } - - /// return the arch identifier set in metadata or returns `none` - pub fn metadata_arch_id(&mut self) -> String { - let metadata_arch_key = "release.openshift.io/architecture"; - let no_arch = String::from("none"); - let arch = self - .get_metadata_mut() - .map(|metadata| metadata.get(metadata_arch_key)) - .unwrap_or(None); - match arch { - Some(arch) => arch.to_string(), - _ => no_arch, - } - } } /// Type to represent a Release with all its information. @@ -219,7 +205,7 @@ impl Graph { R: Into, { let missing_manifest_ref = String::from("none"); - let mut release = release.into(); + let release = release.into(); match self.find_by_version(release.version()) { Some(id) => { let node = self.dag.node_weight_mut(id.0).expect(EXPECT_NODE_WEIGHT); @@ -228,18 +214,12 @@ impl Graph { if release.manifestref().unwrap_or(&missing_manifest_ref) != node.manifestref().unwrap_or(&missing_manifest_ref) { - let release_arch = release.metadata_arch_id(); - if release_arch == node.metadata_arch_id() { - bail!( - "mismatched manifest ref for concrete release {}: {}, {}", - release.version(), - release.manifestref().unwrap_or(&missing_manifest_ref), - node.manifestref().unwrap_or(&missing_manifest_ref) - ) - } - if release_arch == "multi" { - return Ok(id); - } + bail!( + "mismatched manifest ref for concrete release {}: {}, {}", + release.version(), + release.manifestref().unwrap_or(&missing_manifest_ref), + node.manifestref().unwrap_or(&missing_manifest_ref) + ) } } *node = release; From ed9c72fa77c1d5a300c471804c5f6b5201df0990 Mon Sep 17 00:00:00 2001 From: pratikmahajan Date: Thu, 15 May 2025 21:01:13 +0000 Subject: [PATCH 3/3] remove registry-cache placeholders on release scrape failures replace the placeholders with `None` so no more 0.0.0 and remove the placeholder from the cache of we fail to get any release metadata Co-authored-by: wking --- .../release_scrape_dockerv2/registry/mod.rs | 68 ++++++++++--------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs b/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs index 4e151ec75..fdff99c34 100644 --- a/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs +++ b/cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs @@ -478,42 +478,43 @@ async fn lookup_or_fetch( } None => { cache_misses.fetch_add(1, Ordering::SeqCst); - let placeholder = Option::from(Metadata { - kind: MetadataKind::V0, - version: Version::new(0, 0, 0), - previous: vec![], - next: vec![], - metadata: Default::default(), - }); - cache.write().await.insert(manifestref.clone(), placeholder); + cache.write().await.insert(manifestref.clone(), None); - let metadata = find_first_release_metadata( + let metadata = match find_first_release_metadata( layer_digests, registry_client, repo.clone(), tag.clone(), ) .await - .context("failed to find first release")? - .map(|mut metadata| { - // Attach the manifestref this release was found in for further processing - metadata - .metadata - .insert(manifestref_key, manifestref.clone()); - - // Process the manifest architecture if given - if let Some(arch) = arch { - // Encode the architecture as SemVer information - metadata.version.build = vec![semver::Identifier::AlphaNumeric(arch.clone())]; - - // Attach the architecture for later processing + { + Ok(Some(mut metadata)) => { + // Attach the manifestref this release was found in for further processing metadata .metadata - .insert("io.openshift.upgrades.graph.release.arch".to_owned(), arch); - }; + .insert(manifestref_key, manifestref.clone()); - metadata - }); + // Process the manifest architecture if given + if let Some(arch) = arch { + // Encode the architecture as SemVer information + metadata.version.build = + vec![semver::Identifier::AlphaNumeric(arch.clone())]; + + // Attach the architecture for later processing + metadata + .metadata + .insert("io.openshift.upgrades.graph.release.arch".to_owned(), arch); + }; + + Some(metadata) + } + Ok(None) => None, + Err(e) => { + cache.write().await.remove(&manifestref); + error!("Failed to find release metadata for {}: {}", &tag, e); + return Err(e); + } + }; trace!("[{}] Caching release metadata", &tag); cache @@ -660,16 +661,21 @@ async fn find_first_release_metadata( ); match tokio::task::spawn_blocking(move || assemble_metadata(&blob, metadata_filename)) - .await? - { + .await + .context(format!( + "[{}] Could not assemble metadata from layer ({})", + &tag, &layer_digest + ))? { Ok(metadata) => { return Ok(Some(metadata)); } Err(e) => { - debug!( + return Err(format_err!( "[{}] Could not assemble metadata from layer ({}): {}", - &tag, &layer_digest, e, - ); + &tag, + &layer_digest, + e + )) } } }