From eef7b90a8ed8fc976929727eaac5649ed6efd4a2 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 7 Nov 2025 13:19:17 -0500 Subject: [PATCH] to-disk: Include source imgref in cache hash to prevent incorrect reuse Different image references (imgrefs) pointing to the same digest should not share cached disk images, because the source imgref determines the upgrade source for the installed system. Add the `source_imgref` field to the cache metadata structures and include it in the cache hash computation. Additionally, add a `--dry-run` flag to `to-disk` that prints whether the disk would be regenerated (`would-regenerate`) or reused (`would-reuse`) without actually creating it. This is useful for testing and scripting. Closes: https://github.com/bootc-dev/bcvk/issues/138 Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters --- crates/integration-tests/src/tests/to_disk.rs | 86 +++++++++++++++++++ crates/kit/src/cache_metadata.rs | 53 ++++++++++-- crates/kit/src/libvirt/base_disks.rs | 4 +- crates/kit/src/to_disk.rs | 36 ++++++-- docs/src/man/bcvk-to-disk.md | 4 + 5 files changed, 167 insertions(+), 16 deletions(-) diff --git a/crates/integration-tests/src/tests/to_disk.rs b/crates/integration-tests/src/tests/to_disk.rs index ad73057..54dbb3f 100644 --- a/crates/integration-tests/src/tests/to_disk.rs +++ b/crates/integration-tests/src/tests/to_disk.rs @@ -214,3 +214,89 @@ fn test_to_disk_caching() -> Result<()> { Ok(()) } integration_test!(test_to_disk_caching); + +/// Test that different image references with the same digest create separate cached disks +fn test_to_disk_different_imgref_same_digest() -> Result<()> { + let temp_dir = TempDir::new().expect("Failed to create temp directory"); + + // First, pull the test image + let test_image = get_test_image(); + let output = Command::new("podman") + .args(["pull", &test_image]) + .output() + .expect("Failed to run podman pull"); + assert!( + output.status.success(), + "Failed to pull test image: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Create a second tag pointing to the same digest + let second_tag = format!("{}-alias", test_image); + let output = Command::new("podman") + .args(["tag", &test_image, &second_tag]) + .output() + .expect("Failed to run podman tag"); + assert!( + output.status.success(), + "Failed to tag image: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // Create first disk with original image reference + let disk_path = Utf8PathBuf::try_from(temp_dir.path().join("test-disk.img")) + .expect("temp path is not UTF-8"); + + let output1 = run_bcvk(&[ + "to-disk", + "--label", + INTEGRATION_TEST_LABEL, + &test_image, + disk_path.as_str(), + ])?; + + assert!( + output1.success(), + "First to-disk run failed with exit code: {:?}. stdout: {}, stderr: {}", + output1.exit_code(), + output1.stdout, + output1.stderr + ); + + let metadata1 = + std::fs::metadata(&disk_path).expect("Failed to get disk metadata after first run"); + assert!(metadata1.len() > 0, "Disk image is empty"); + + // Use --dry-run with the aliased image reference (same digest, different imgref) + // to verify it would regenerate instead of reusing the cache + let output2 = run_bcvk(&[ + "to-disk", + "--dry-run", + "--label", + INTEGRATION_TEST_LABEL, + &second_tag, + disk_path.as_str(), + ])?; + + assert!( + output2.success(), + "Dry-run with different imgref failed with exit code: {:?}. stdout: {}, stderr: {}", + output2.exit_code(), + output2.stdout, + output2.stderr + ); + + // The dry-run should report it would regenerate because the source imgref is different + assert!( + output2.stdout.contains("would-regenerate"), + "Dry-run should report 'would-regenerate' for different imgref. stdout: {}, stderr: {}", + output2.stdout, + output2.stderr + ); + + // Clean up: remove the aliased tag + let _ = Command::new("podman").args(["rmi", &second_tag]).output(); + + Ok(()) +} +integration_test!(test_to_disk_different_imgref_same_digest); diff --git a/crates/kit/src/cache_metadata.rs b/crates/kit/src/cache_metadata.rs index 61e297b..45e8841 100644 --- a/crates/kit/src/cache_metadata.rs +++ b/crates/kit/src/cache_metadata.rs @@ -30,6 +30,10 @@ struct CacheInputs { /// SHA256 digest of the source container image image_digest: String, + /// Source image reference (e.g., "quay.io/centos-bootc/centos-bootc:stream9") + /// This is crucial because it determines the upgrade source for the installed system + source_imgref: String, + /// Filesystem type used for installation (e.g., "ext4", "xfs", "btrfs") filesystem: Option, @@ -52,6 +56,10 @@ pub struct DiskImageMetadata { /// SHA256 digest of the source container image pub digest: String, + /// Source image reference (e.g., "quay.io/centos-bootc/centos-bootc:stream9") + /// This is crucial because it determines the upgrade source for the installed system + pub source_imgref: String, + /// Filesystem type used for installation (e.g., "ext4", "xfs", "btrfs") pub filesystem: Option, @@ -73,6 +81,7 @@ impl DiskImageMetadata { pub fn compute_cache_hash(&self) -> String { let inputs = CacheInputs { image_digest: self.digest.clone(), + source_imgref: self.source_imgref.clone(), filesystem: self.filesystem.clone(), root_size: self.root_size.clone(), composefs_backend: self.composefs_backend, @@ -152,11 +161,12 @@ impl DiskImageMetadata { } impl DiskImageMetadata { - /// Create new metadata from InstallOptions and image digest - pub fn from(options: &InstallOptions, image: &str) -> Self { + /// Create new metadata from InstallOptions, image digest, and source imgref + pub fn from(options: &InstallOptions, image_digest: &str, source_imgref: &str) -> Self { Self { version: 1, - digest: image.to_owned(), + digest: image_digest.to_owned(), + source_imgref: source_imgref.to_owned(), filesystem: options.filesystem.clone(), root_size: options.root_size.clone(), kernel_args: options.karg.clone(), @@ -179,6 +189,7 @@ pub(crate) enum ValidationError { pub fn check_cached_disk( path: &Path, image_digest: &str, + source_imgref: &str, install_options: &InstallOptions, ) -> Result> { if !path.exists() { @@ -187,7 +198,7 @@ pub fn check_cached_disk( } // Create metadata for the current request to compute expected hash - let expected_meta = DiskImageMetadata::from(install_options, image_digest); + let expected_meta = DiskImageMetadata::from(install_options, image_digest, source_imgref); let expected_hash = expected_meta.compute_cache_hash(); // Read the cache hash from the disk image @@ -242,14 +253,16 @@ mod tests { root_size: Some("20G".to_string()), ..Default::default() }; - let metadata1 = DiskImageMetadata::from(&install_options1, "sha256:abc123"); + let metadata1 = + DiskImageMetadata::from(&install_options1, "sha256:abc123", "quay.io/test/image:v1"); let install_options2 = InstallOptions { filesystem: Some("ext4".to_string()), root_size: Some("20G".to_string()), ..Default::default() }; - let metadata2 = DiskImageMetadata::from(&install_options2, "sha256:abc123"); + let metadata2 = + DiskImageMetadata::from(&install_options2, "sha256:abc123", "quay.io/test/image:v1"); // Same inputs should generate same hash assert_eq!( @@ -257,13 +270,14 @@ mod tests { metadata2.compute_cache_hash() ); - // Different inputs should generate different hashes + // Different digest should generate different hashes let install_options3 = InstallOptions { filesystem: Some("ext4".to_string()), root_size: Some("20G".to_string()), ..Default::default() }; - let metadata3 = DiskImageMetadata::from(&install_options3, "sha256:xyz789"); + let metadata3 = + DiskImageMetadata::from(&install_options3, "sha256:xyz789", "quay.io/test/image:v1"); assert_ne!( metadata1.compute_cache_hash(), @@ -276,18 +290,38 @@ mod tests { root_size: Some("20G".to_string()), ..Default::default() }; - let metadata4 = DiskImageMetadata::from(&install_options4, "sha256:abc123"); + let metadata4 = + DiskImageMetadata::from(&install_options4, "sha256:abc123", "quay.io/test/image:v1"); assert_ne!( metadata1.compute_cache_hash(), metadata4.compute_cache_hash() ); + + // Different source imgref should generate different hash even with same digest + let install_options5 = InstallOptions { + filesystem: Some("ext4".to_string()), + root_size: Some("20G".to_string()), + ..Default::default() + }; + let metadata5 = DiskImageMetadata::from( + &install_options5, + "sha256:abc123", + "quay.io/different/image:latest", + ); + + assert_ne!( + metadata1.compute_cache_hash(), + metadata5.compute_cache_hash(), + "Different source imgrefs with same digest should generate different cache hashes" + ); } #[test] fn test_cache_inputs_serialization() -> Result<()> { let inputs = CacheInputs { image_digest: "sha256:abc123".to_string(), + source_imgref: "quay.io/test/image:v1".to_string(), filesystem: Some("ext4".to_string()), root_size: Some("20G".to_string()), kernel_args: vec!["console=ttyS0".to_string()], @@ -299,6 +333,7 @@ mod tests { let deserialized: CacheInputs = serde_json::from_str(&json)?; assert_eq!(inputs.image_digest, deserialized.image_digest); + assert_eq!(inputs.source_imgref, deserialized.source_imgref); assert_eq!(inputs.filesystem, deserialized.filesystem); assert_eq!(inputs.root_size, deserialized.root_size); assert_eq!(inputs.kernel_args, deserialized.kernel_args); diff --git a/crates/kit/src/libvirt/base_disks.rs b/crates/kit/src/libvirt/base_disks.rs index 73e7f01..6aafd7d 100644 --- a/crates/kit/src/libvirt/base_disks.rs +++ b/crates/kit/src/libvirt/base_disks.rs @@ -19,7 +19,7 @@ pub fn find_or_create_base_disk( install_options: &InstallOptions, connect_uri: Option<&str>, ) -> Result { - let metadata = DiskImageMetadata::from(install_options, image_digest); + let metadata = DiskImageMetadata::from(install_options, image_digest, source_image); let cache_hash = metadata.compute_cache_hash(); // Extract short hash for filename (first 16 chars after "sha256:") @@ -43,6 +43,7 @@ pub fn find_or_create_base_disk( if crate::cache_metadata::check_cached_disk( base_disk_path.as_std_path(), image_digest, + source_image, install_options, )? .is_ok() @@ -130,6 +131,7 @@ fn create_base_disk( let metadata_valid = crate::cache_metadata::check_cached_disk( temp_disk_path.as_std_path(), image_digest, + source_image, install_options, ) .context("Querying cached disk")?; diff --git a/crates/kit/src/to_disk.rs b/crates/kit/src/to_disk.rs index 3d85e67..4c1d577 100644 --- a/crates/kit/src/to_disk.rs +++ b/crates/kit/src/to_disk.rs @@ -138,6 +138,10 @@ pub struct ToDiskAdditionalOpts { help = "Add metadata to the container in key=value form" )] pub label: Vec, + + /// Check if the disk would be regenerated without actually creating it + #[clap(long)] + pub dry_run: bool, } /// Configuration options for installing a bootc container image to disk @@ -302,7 +306,7 @@ impl ToDiskOpts { /// for details on the installation workflow and architecture. pub fn run(opts: ToDiskOpts) -> Result<()> { // Phase 0: Check for existing cached disk image - if opts.target_disk.exists() { + let would_reuse = if opts.target_disk.exists() { debug!( "Target disk {} already exists, checking cache metadata", opts.target_disk @@ -316,9 +320,14 @@ pub fn run(opts: ToDiskOpts) -> Result<()> { match crate::cache_metadata::check_cached_disk( opts.target_disk.as_std_path(), &image_digest, + &opts.source_image, &opts.install, )? { Ok(()) => { + if opts.additional.dry_run { + println!("would-reuse"); + return Ok(()); + } println!( "Reusing existing cached disk image (digest {image_digest}) at: {}", opts.target_disk @@ -327,12 +336,27 @@ pub fn run(opts: ToDiskOpts) -> Result<()> { } Err(e) => { debug!("Existing disk does not match requirements, recreating: {e}"); - // Remove the existing disk so we can recreate it - std::fs::remove_file(&opts.target_disk).with_context(|| { - format!("Failed to remove existing disk {}", opts.target_disk) - })?; + if !opts.additional.dry_run { + // Remove the existing disk so we can recreate it + std::fs::remove_file(&opts.target_disk).with_context(|| { + format!("Failed to remove existing disk {}", opts.target_disk) + })?; + } + false } } + } else { + false + }; + + // In dry-run mode, report whether we would regenerate + if opts.additional.dry_run { + if would_reuse { + println!("would-reuse"); + } else { + println!("would-regenerate"); + } + return Ok(()); } // Phase 1: Validation and preparation @@ -512,7 +536,7 @@ fn write_disk_metadata( let digest = inspect.digest.to_string(); // Prepare metadata using the new helper method - let metadata = DiskImageMetadata::from(install_options, &digest); + let metadata = DiskImageMetadata::from(install_options, &digest, source_image); // Write metadata using rustix fsetxattr let file = std::fs::OpenOptions::new() diff --git a/docs/src/man/bcvk-to-disk.md b/docs/src/man/bcvk-to-disk.md index aee5e6f..c082e83 100644 --- a/docs/src/man/bcvk-to-disk.md +++ b/docs/src/man/bcvk-to-disk.md @@ -111,6 +111,10 @@ The installation process: Add metadata to the container in key=value form +**--dry-run** + + Check if the disk would be regenerated without actually creating it + # ARGUMENTS