fix(spur-net): honor requested image architecture by hnotshe · Pull Request #438 · ROCm/spur

hnotshe · 2026-07-14T02:37:18Z

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.

shiv-tyagi

LGTM

hnotshe · 2026-07-15T23:28:54Z

@shiv-tyagi Thanks for the review! All checks are green -- could I please get some help merging?

yansun1996 · 2026-07-16T05:48:17Z

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 media_type.contains("gzip") || digest.starts_with("sha256:") effectively forces gzip on every layer, so a non-gzip/zstd layer would silently produce a partial rootfs; and arm v6/v7 variants aren't disambiguated since Platform doesn't deserialize variant.

yansun1996 · 2026-07-16T05:48:32Z

-    // 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)


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.

Good catch. First-time installs now publish the sidecar before exposing the payload. Replacements invalidate the old sidecar before publishing the new payload, then publish the new sidecar last, so an interruption forces a cache miss rather than pairing a payload with stale metadata. Both paths have filesystem-level regression coverage.

yansun1996 · 2026-07-16T05:48:33Z


-    // Clean up temp dir
+    let finalize_result = (|| -> anyhow::Result<()> {
+        std::fs::write(&staged_arch_path, oci_architecture(arch))?;


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.

Fixed by validating direct manifests through their OCI config blob before layer extraction. The config os and normalized architecture must match the requested Linux platform before the sidecar can be recorded, with regression coverage for matching and mismatched platforms.

yansun1996 · 2026-07-16T05:48:34Z

 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


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.

This is a separate execution-time validation boundary. This PR keeps daemon-side imports host-native and stays focused on honoring the requested architecture during import; changing resolve_image and launch behavior would expand beyond issue #343 acceptance criteria. I am leaving that follow-up out of this patch.

yansun1996 · 2026-07-16T05:48:35Z


    // Create temp directory for rootfs assembly
    let tmp_dir = output_dir.join(format!(".pulling_{}", sanitized));
+    if tmp_dir.exists() {


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.

The shared staging directory is gone. Each pull now uses a UUID-suffixed directory, and same-image pulls are serialized through the cache recheck and final publication, so concurrent imports cannot delete or interleave one another's artifacts.

yansun1996 · 2026-07-16T05:48:36Z

+        "x86_64" => "amd64",
+        "aarch64" => "arm64",
+        "x86" => "386",
+        arch => arch,


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.

That limitation remains intentional here. The issue contract is architecture selection, while arm v6/v7 support needs a variant input and normalization contract rather than silently choosing the first arm entry. I am keeping variant handling out of this patch.

yansun1996 · 2026-07-16T05:48:37Z

        .args([
            rootfs_dir.to_str().unwrap(),
-            sqsh_path.to_str().unwrap(),
+            staged_sqsh_path.to_str().unwrap(),


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.

Cleaned this up by passing the filesystem paths directly to Command, removing both UTF-8 conversions and the library unwrap() calls.

shiv-tyagi · 2026-07-21T05:01:19Z

@hnotshe Can you please address Yan's comments?

# Conflicts: # crates/spur-cli/src/image.rs # crates/spur-net/src/oci.rs

# Conflicts: # crates/spur-net/src/oci.rs

codecov-commenter · 2026-08-10T21:59:31Z

Codecov Report

❌ Patch coverage is 68.96552% with 99 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #438      +/-   ##
==========================================
+ Coverage   76.81%   76.82%   +0.01%     
==========================================
  Files         169      169              
  Lines       68824    69115     +291     
==========================================
+ Hits        52866    53096     +230     
- Misses      15958    16019      +61

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fix(spur-net): honor requested image architecture

c6335be

hnotshe marked this pull request as ready for review July 14, 2026 21:59

hnotshe requested review from powderluv, sajmera-pensando, sgopinath1, shiv-tyagi and yansun1996 as code owners July 14, 2026 21:59

shiv-tyagi previously approved these changes Jul 15, 2026

View reviewed changes

yansun1996 reviewed Jul 16, 2026

View reviewed changes

yansun1996 requested changes Jul 28, 2026

View reviewed changes

hnotshe added 2 commits August 10, 2026 07:14

Merge remote-tracking branch 'upstream/main' into fix/image-import-arch

d6b61f3

# Conflicts: # crates/spur-cli/src/image.rs # crates/spur-net/src/oci.rs

fix(spur-net): address architecture import review feedback

1f36a38

hnotshe dismissed shiv-tyagi’s stale review via 1f36a38 August 10, 2026 17:22

hnotshe added 3 commits August 10, 2026 12:34

Merge remote-tracking branch 'upstream/main' into fix/image-import-arch

18c2a04

# Conflicts: # crates/spur-net/src/oci.rs

Merge remote-tracking branch 'upstream/main' into fix/image-import-arch

01063f3

Merge remote-tracking branch 'upstream/main' into fix/image-import-arch

8206790

Uh oh!

Conversation

hnotshe commented Jul 14, 2026

Uh oh!

shiv-tyagi left a comment

Choose a reason for hiding this comment

Uh oh!

hnotshe commented Jul 15, 2026

Uh oh!

yansun1996 commented Jul 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnotshe Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiv-tyagi commented Jul 21, 2026

Uh oh!

codecov-commenter commented Aug 10, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hnotshe Aug 10, 2026 •

edited

Loading