fix(spur-net): honor requested image architecture - #438
Conversation
|
@shiv-tyagi Thanks for the review! All checks are green -- could I please get some help merging? |
|
Solid change. The staged-replacement design is careful — same-filesystem rename, the existing artifact is left intact on a failed pull, and sidecar-less artifacts are safely rebuilt once — and the new pure functions are well-tested and network-free. No blocking issues; notes inline. Two items are pre-existing (not regressions from this PR), flagged just for context: the layer-extraction gate |
| // Clean up temp dir | ||
| let finalize_result = (|| -> anyhow::Result<()> { | ||
| std::fs::write(&staged_arch_path, oci_architecture(arch))?; | ||
| std::fs::rename(&staged_sqsh_path, &sqsh_path) |
There was a problem hiding this comment.
The install is two separate renames (.sqsh then .sqsh.arch). A crash between them leaves the new payload with a stale-or-missing arch sidecar. It is self-healing (the next pull rebuilds), but since the sidecar is the source of truth for the "rebuild on arch change" decision, writing the metadata first and renaming the payload in last (payload as the commit point) would make "sqsh present implies arch recorded" hold.
|
|
||
| // Clean up temp dir | ||
| let finalize_result = (|| -> anyhow::Result<()> { | ||
| std::fs::write(&staged_arch_path, oci_architecture(arch))?; |
There was a problem hiding this comment.
The sidecar is written as the requested arch unconditionally. On the single-manifest path (a registry returning an image manifest directly rather than an index), no platform verification runs, so the recorded arch could be wrong and defeat the rebuild-on-arch-change guard. Verifying the fetched manifest's platform (or documenting the limitation) would close this.
| pub async fn import_image(uri: &str) -> anyhow::Result<PathBuf> { | ||
| let dir = image_dir(); | ||
| spur_net::pull_image(uri, &dir).await | ||
| spur_net::pull_image(uri, &dir, std::env::consts::ARCH).await |
There was a problem hiding this comment.
Requested arch is honored on the pull path, but import_image here hardcodes host arch and resolve_image matches purely by filename (never reading the .sqsh.arch sidecar) — so a cross-arch artifact could be launched on the wrong host unchecked. Likely out of scope for this PR's title, but worth a follow-up; the sidecar added here is the enabling primitive for that check.
|
|
||
| // Create temp directory for rootfs assembly | ||
| let tmp_dir = output_dir.join(format!(".pulling_{}", sanitized)); | ||
| if tmp_dir.exists() { |
There was a problem hiding this comment.
Minor: the .pulling_<name> temp dir is deterministic per image, so two concurrent pulls of the same image race — and this unconditional remove_dir_all can wipe an in-flight pull's working tree. A PID/uuid suffix would make it safe. Low likelihood for interactive imports.
| "x86_64" => "amd64", | ||
| "aarch64" => "arm64", | ||
| "x86" => "386", | ||
| arch => arch, |
There was a problem hiding this comment.
Minor: the normalization fallthrough passes unknown arches through unchanged; combined with Platform not deserializing variant, 32-bit arm (v6/v7) cannot be disambiguated and the first arm entry wins. Fine for the amd64/arm64 common case — flagging for completeness.
| .args([ | ||
| rootfs_dir.to_str().unwrap(), | ||
| sqsh_path.to_str().unwrap(), | ||
| staged_sqsh_path.to_str().unwrap(), |
There was a problem hiding this comment.
Nit: to_str().unwrap() in a library crate trips the no-unwrap-in-lib guideline (AGENTS.md). Pre-existing, but since this line is in the diff, a .context("non-UTF-8 image path")? would be a cheap tidy-up.
|
@hnotshe Can you please address Yan's comments? |
3 participants
Closes #343.
Thread the requested image architecture through the native OCI pull path and use it when resolving multi-architecture manifest lists. Host architecture names are normalized to their OCI equivalents, and missing variants now report the requested architecture.
Imported squashfs artifacts record their architecture so an existing image for another architecture is rebuilt instead of silently reused. Replacement is staged so a failed pull does not destroy the existing artifact.
Tested with workspace Clippy and the full locked test suite on Linux.