to-disk: Include source imgref in cache hash to prevent incorrect reuse (#141)

cgwalters · web-flow · commit 84b02c2bf596 · 2025-11-07T14:44:18.000-05:00
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: #138 Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
diff --git 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
@@ -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<String>,
 
@@ -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<String>,
 
@@ -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,
@@ -154,11 +163,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(),
@@ -181,6 +191,7 @@ pub(crate) enum ValidationError {
 pub fn check_cached_disk(
     path: &Path,
     image_digest: &str,
+    source_imgref: &str,
     install_options: &InstallOptions,
 ) -> Result<Result<(), ValidationError>> {
     if !path.exists() {
@@ -189,7 +200,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
@@ -246,28 +257,31 @@ 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!(
             metadata1.compute_cache_hash(),
             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(),
@@ -280,18 +294,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()],
@@ -303,6 +337,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
@@ -19,7 +19,7 @@ pub fn find_or_create_base_disk(
     install_options: &InstallOptions,
     connect_uri: Option<&str>,
 ) -> Result<Utf8PathBuf> {
-    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
@@ -138,6 +138,10 @@ pub struct ToDiskAdditionalOpts {
         help = "Add metadata to the container in key=value form"
     )]
     pub label: Vec<String>,
+
+    /// 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
@@ -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
+
 <!-- END GENERATED OPTIONS -->
 
 # ARGUMENTS
