Skip to content

[WIP] OCPBUGS-56269: Remove registry cache placeholder on scrape failure #1025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PratikMahajan wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[WIP] OCPBUGS-56269: Remove registry cache placeholder on scrape failure #1025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f07d4f
ignore single-arch shards of multi-arch images
PratikMahajan May 14, 2025
fe23e23
Revert "do not panic on sha-mismatch for multi-arch images"
PratikMahajan May 14, 2025
ed9c72f
remove registry-cache placeholders on release scrape failures
PratikMahajan May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 7 additions & 27 deletions cincinnati/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -219,7 +205,7 @@ impl Graph {
R: Into<Release>,
{
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);
Expand All @@ -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;
Expand Down
18 changes: 18 additions & 0 deletions cincinnati/src/plugins/internal/graph_builder/release.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,24 @@ pub struct Metadata {
pub metadata: MapImpl<String, String>,
}

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!(
Expand Down
79 changes: 47 additions & 32 deletions cincinnati/src/plugins/internal/graph_builder/release_scrape_dockerv2/registry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -469,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
Expand Down Expand Up @@ -651,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 ({}): {}",
Copy link
Member

@wking wking May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey look, there's some CI coverage in cargo-test:

cincinnati: plugins::internal::graph_builder::release_scrape_dockerv2::plugin::network_tests::scrape_public_with_first_empty_tag_must_succeed expand_less	1s
{Error: failed to fetch all release metadata from registry.ci.openshift.org/cincinnati-ci-public/cincinnati-test-emptyfirsttag-public-manual test failure with exit code 101 Error: failed to fetch all release metadata from registry.ci.openshift.org/cincinnati-ci-public/cincinnati-test-emptyfirsttag-public-manual

Caused by:
    [0.0.0] Could not assemble metadata from layer (sha256:4ca545ee6d5db5c1170386eeb39b2ffe3bd46e5d4a73a9acbebc805f19607eb3): 'release-manifests/release-metadata' not found}

cincinnati: plugins::internal::graph_builder::release_scrape_dockerv2::plugin::network_tests::scrape_public_multiarch_manual_succeeds expand_less	1s
{Error: failed to fetch all release metadata from registry.ci.openshift.org/cincinnati-ci-public/cincinnati-test-public-multiarch-manual test failure with exit code 101 Error: failed to fetch all release metadata from registry.ci.openshift.org/cincinnati-ci-public/cincinnati-test-public-multiarch-manual

Caused by:
    [0.0.3-amd64] Could not assemble metadata from layer (sha256:4ca545ee6d5db5c1170386eeb39b2ffe3bd46e5d4a73a9acbebc805f19607eb3): 'release-manifests/release-metadata' not found}

I'm not clear yet on whether that's turning up fixtures/tests we should adjust, or pointing out issues in our new code.

Copy link
Member

@wking wking May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, seems like that's "we didn't find find release-manifests/release-metadata in the top layer", and we need to adjust our error handling here to allow it to continue further in the for layer_digest in layer_digests loop, in case we can find the metadata file in a deeper layer. I'm not sure how to express that in Rust 😅 I think we might need to adjust assemble_metadata to return an Option, so it can say "I was successfully able to process this blob, but I didn't find metadata_filename inside it"?

&tag, &layer_digest, e,
);
&tag,
&layer_digest,
e
))
}
}
}
Expand Down