Lazily scan COSI contents and stream images in order #478
Conversation
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR refactors the COSI image streaming architecture to lazily scan COSI contents and stream images in the order they appear in the COSI file rather than in Host Configuration order. The refactoring removes lifetime parameters from image-related types and replaces the closure-based reader pattern with a callback-based streaming API that processes entries sequentially.
Changes:
- Replaced closure-based image reading with a callback-based
read_imagesmethod that streams images in COSI order - Removed early validation that all images in metadata exist in the COSI file during initialization
- Added
CorruptOsImageerror variant for cases where expected images are not found during deployment
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/trident_api/src/error.rs | Added CorruptOsImage error variant and fixed trailing whitespace |
| crates/trident/src/subsystems/esp.rs | Migrated ESP deployment to use the new read_images streaming API |
| crates/trident/src/osimage/mod.rs | Updated core API to remove lifetime parameters and add read_images method |
| crates/trident/src/osimage/mock.rs | Updated mock implementation to support the new streaming API |
| crates/trident/src/osimage/cosi/mod.rs | Implemented lazy entry scanning and removed upfront validation of image existence |
| crates/trident/src/osimage/cosi/metadata.rs | Removed CosiEntry field from ImageFile as entries are no longer stored |
| crates/trident/src/engine/storage/image.rs | Migrated filesystem deployment to use read_images with validation of all expected images |
| os_image.read_images(|path, stream| { | ||
| if path != esp_img.image_file.path { | ||
| return ControlFlow::Continue(()); | ||
| } | ||
|
|
||
| if esp_img.image_file.sha384 != computed_sha384 { | ||
| bail!( | ||
| "SHA384 mismatch for disk image {}: expected {}, got {}", | ||
| let (temp_file, computed_sha384) = match load_raw_image( | ||
| &esp_extraction_dir, | ||
| os_image.source(), | ||
| esp_img.image_file.sha384, | ||
| computed_sha384 | ||
| ); | ||
| } | ||
| HashingReader384::new(stream), | ||
| ) { | ||
| Ok(r) => r, | ||
| Err(e) => { | ||
| return ControlFlow::Break( | ||
| Err(e) | ||
| .structured(ServicingError::DeployESPImages) | ||
| .message("Failed to load raw image"), | ||
| ) | ||
| } | ||
| }; | ||
| if esp_img.image_file.sha384 != computed_sha384 { | ||
| return ControlFlow::Break( | ||
| Err(TridentError::new(ServicingError::DeployESPImages)).message(format!( | ||
| "SHA384 mismatch for disk image {}: expected {}, got {}", | ||
| os_image.source(), | ||
| esp_img.image_file.sha384, | ||
| computed_sha384 | ||
| )), | ||
| ); | ||
| } | ||
|
|
||
| copy_file_artifacts(temp_file.path(), ctx, mount_point) | ||
| ControlFlow::Break( | ||
| copy_file_artifacts(temp_file.path(), ctx, mount_point) | ||
| .structured(ServicingError::DeployESPImages) | ||
| .message("Failed to load raw image"), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Missing validation that the ESP image was actually found and deployed. If the ESP image path in the metadata doesn't match any entry in the COSI file, read_images will complete without calling the callback with a matching path, and this function will return Ok(()) without having deployed anything. This silently fails to deploy the ESP image.
Consider adding a check after read_images returns to ensure the image was found and deployed, similar to the validation in engine/storage/image.rs lines 143-146.
| ControlFlow::Break( | ||
| copy_file_artifacts(temp_file.path(), ctx, mount_point) | ||
| .structured(ServicingError::DeployESPImages) | ||
| .message("Failed to load raw image"), |
There was a problem hiding this comment.
Misleading error message. The error "Failed to load raw image" is used for an error from copy_file_artifacts, not from load_raw_image. This should say something like "Failed to copy file artifacts" to accurately reflect what operation failed.
| .message("Failed to load raw image"), | |
| .message("Failed to copy file artifacts"), |
| cosi_reader.section_reader(image.file.entry.offset, image.file.entry.size) | ||
| }) | ||
| }, | ||
| path: image.file.path.clone(), |
There was a problem hiding this comment.
Unnecessary clone. Since image is already cloned and owned (line 133), image.file.path can be moved directly instead of being cloned. This would be consistent with line 150 where verity.file.path is moved without cloning.
| path: image.file.path.clone(), | |
| path: image.file.path, |
|
|
||
| pub(crate) fn read_images<F>(&self, f: F) -> Result<(), TridentError> | ||
| where | ||
| F: FnMut(&Path, Box<dyn Read>) -> ControlFlow<Result<(), TridentError>>, |
There was a problem hiding this comment.
note for the future regarding &Path, might be nice to have some generic filesystem matcher trait for whenever we want to enable anything other that is not cosi and might not have the same concept of a path as an unique identifier for an image
This PR switches Trident's streaming logic to run in the order that images appear in the COSI rather than the order we see them in the HC.
Still need to fix an issue with verity