[10/n] [sled-agent] validate zone images as written in zone manifest #8190

sunshowers · 2025-05-20T04:06:00Z

Included in this PR:

Add validation for zone images
Write tests for success and failure cases
Extend the test_installinator_fetch wicketd integration test to also ensure installinator + sled-agent have a coherent view of the world

The last point forced me to expose a bunch of the status via a public interface. We'll be able to reuse this information (though probably not in this full form) and send it up as part of the inventory.

Depends on:

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

jgallagher · 2025-06-03T16:24:35Z

installinator/src/write.rs

    fn from_directory(dir: &Utf8Path) -> Result<Self> {
-        let control_plane_dir = dir.join("zones");
+        // The install dataset goes into a directory called "install".
+        let control_plane_dir = dir.join("install");


Was this just wrong before? (Hopefully only used by tests??)

Yeah it was only used by tests. Actually hooking sled-agent's zone logic to installinator found the issue.

jgallagher · 2025-06-03T16:29:53Z

sled-agent/zone-images/src/mupdate_override.rs

+    pub boot_disk_override:
+        Result<Option<MupdateOverrideInfo>, MupdateOverrideReadError>,
+
+    /// Status of the non-boot disks. This results in warnings.


Nit / question - this only results in warnings in bad cases, right? The typical case is MatchesPresent which would not emit warnings?

Yep, updated the doc to make this clearer.

jgallagher · 2025-06-03T18:11:29Z

sled-agent/zone-images/src/mupdate_override.rs

+                    )?;
+                }
+                ArtifactReadResult::Error(error) => {
+                    writeln!(f, "  {}: error ({})", artifact.file_name, error)?;


Should this print the whole error chain since ArtifactReadResult itself isn't an Error?

Possibly -- in practice there isn't going to be a source, but I've added InlineErrorChain::new just in case there is one in the future.

jgallagher · 2025-06-03T18:13:47Z

sled-agent/zone-images/src/mupdate_override.rs

+                let artifacts =
+                    MupdateOverrideArtifactsResult::new(dataset_dir, data);
+                if artifacts.is_valid() {
+                    // If there are errors, return them as appropriate.


Is this comment in the wrong branch? If artifacts.is_valid(), there are no errors right?

Yeah. Though I ended up redoing most of this in the latest update since I had to split up the zone manifest and mupdate override into separate files.

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

sunshowers · 2025-06-06T03:44:47Z

sled-agent/zone-images/src/zone_manifest.rs

+#[derive(Clone, Debug, PartialEq)]
+pub enum ZoneManifestNonBootResult {
+    /// The manifest is present and matches the value on the boot disk.
+    /// Information about individual zone hashes matching is stored in the
+    /// `ZoneManifestArtifactsResult`.
+    Matches(ZoneManifestArtifactsResult),
+
+    /// The manifest is absent -- this is an error case because it is expected
+    /// to be present.
+    NotFound,
+
+    /// A mismatch between the boot disk and the other disk was detected.
+    Mismatch(ZoneManifestNonBootMismatch),
+
+    /// An error occurred while reading the mupdate override info on this disk.
+    ReadError(InstallMetadataReadError),
+}
+


Many of these types are duplicated with some minor modifications -- deliberately so, since there are subtle differences (e.g. the underlying install_dataset_metadata doesn't error out if the file is missing, but here we do).

Created using spr 1.3.6-beta.1

jgallagher · 2025-06-16T12:56:35Z

sled-agent/zone-images/src/errors.rs

+/// Testing aid.
+impl PartialEq for ArcSerdeJsonError {


Would it make sense to put this behind #[cfg(test)]? Maybe doesn't matter, since I wouldn't expect real code to be comparing errors anyway...

It would be ok for now, but in an upcoming PR I move this into a more central location and it would become more annoying to manage.

jgallagher · 2025-06-16T18:39:07Z

sled-agent/zone-images/src/zone_manifest.rs

+                manifest,
+            )),
+            Ok(None) => {
+                // The file is missing -- this is an error.


Discussed this some in chat - I think this will break a couple of manufacturing / dev workflows, because they produce sleds that start up without installinator having run (and therefore without having had a chance to get a zones.json file). We could change the workflows to write out a zones.json (with some amount of work / logistical pain, I think), but it also seems okay for us to hash the zones we find ourselves if there's no zones.json present at all.

Sorry I didn't think about this the first time around :(

Yep, working on this.

Created using spr 1.3.6-beta.1

jgallagher

Thanks for all the back and forth on this - the new synthesized manifest LGTM.

jgallagher · 2025-06-18T15:21:02Z

sled-agent/zone-images/src/test_utils.rs

+            ]
+            .into_iter()
+            .collect(),
+            mupdate_id: MupdateUuid::new_v4(),


I think this is causing the new test failures, maybe? The display impl now includes this ID, which is different each time the test runs?

This is what I get for writing code at midnight. Thanks.

Created using spr 1.3.6-beta.1

* Move `ZoneImageFileSource` to illumos-utils and use it in the zone image builder. * Move default file name logic to sled-agent-zone-images, since it cares about that in the context of mupdate override logic. Depends on: * #8190 * everything before that

sunshowers added 4 commits May 20, 2025 04:05

[spr] changes to main this commit is based on

1dabbbb

Created using spr 1.3.6-beta.1 [skip ci]

[spr] initial version

1035ba6

Created using spr 1.3.6-beta.1

[spr] changes introduced through rebase

66ba3a4

Created using spr 1.3.6-beta.1 [skip ci]

ready, I think

265979f

Created using spr 1.3.6-beta.1

sunshowers changed the title ~~[9/n] [wip] [sled-agent] validate zone images as written in mupdate-override dataset~~ [10/n] [sled-agent] validate zone images as written in mupdate-override dataset May 29, 2025

sunshowers added 2 commits May 29, 2025 01:10

rebase

75faa95

Created using spr 1.3.6-beta.1

[spr] changes introduced through rebase

7336cb6

Created using spr 1.3.6-beta.1 [skip ci]

sunshowers marked this pull request as ready for review May 29, 2025 01:14

clippy

756312b

Created using spr 1.3.6-beta.1

sunshowers requested review from jgallagher and andrewjstone May 29, 2025 06:54

sunshowers mentioned this pull request May 29, 2025

[11/n] [sled-agent] move around ZoneImageFileSource logic #8235

Merged

jgallagher approved these changes Jun 3, 2025

View reviewed changes

sunshowers added 5 commits June 6, 2025 02:59

[spr] changes introduced through rebase

f5d4254

Created using spr 1.3.6-beta.1 [skip ci]

separate out zone manifest into its own file

afc07f8

Created using spr 1.3.6-beta.1

actually do so

d6ae89c

Created using spr 1.3.6-beta.1

address review comments

6942a3d

Created using spr 1.3.6-beta.1

add tests, clean up

96cc611

Created using spr 1.3.6-beta.1

sunshowers commented Jun 6, 2025

View reviewed changes

fix clippy + small refactor

93d82e1

Created using spr 1.3.6-beta.1

jgallagher self-requested a review June 6, 2025 12:08

sunshowers mentioned this pull request Jun 6, 2025

[12/n] [sled-agent] don't start install dataset zones on zone manifest error #8237

Merged

5 tasks

sunshowers changed the title ~~[10/n] [sled-agent] validate zone images as written in mupdate-override dataset~~ [10/n] [sled-agent] validate zone images as written in zone manifest Jun 9, 2025

sunshowers changed the base branch from sunshowers/spr/main.9n-wip-sled-agent-validate-zone-images-as-written-in-mupdate-override-dataset to main June 10, 2025 00:32

sunshowers added 4 commits June 10, 2025 00:32

add more tests, add summary line in output

e158747

Created using spr 1.3.6-beta.1

a little more refactoring

725f75a

Created using spr 1.3.6-beta.1

rebase on main

4cccaac

Created using spr 1.3.6-beta.1

-> make_internal_disks_rx

156e16b

Created using spr 1.3.6-beta.1

jgallagher reviewed Jun 16, 2025

View reviewed changes

synthesize manifest if it's missing

ad73216

Created using spr 1.3.6-beta.1

sunshowers added 2 commits June 17, 2025 18:51

clippy

ba14ce1

Created using spr 1.3.6-beta.1

add source to display impl

a795a51

Created using spr 1.3.6-beta.1

jgallagher approved these changes Jun 18, 2025

View reviewed changes

fix the mupdate ID

f079b31

Created using spr 1.3.6-beta.1

sunshowers enabled auto-merge (squash) June 18, 2025 17:42

sunshowers merged commit 5465ccf into main Jun 18, 2025
17 checks passed

sunshowers deleted the sunshowers/spr/9n-wip-sled-agent-validate-zone-images-as-written-in-mupdate-override-dataset branch June 18, 2025 19:05

[10/n] [sled-agent] validate zone images as written in zone manifest #8190

[10/n] [sled-agent] validate zone images as written in zone manifest #8190

Uh oh!

Conversation

sunshowers commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sunshowers Jun 6, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgallagher left a comment

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!

Uh oh!

Uh oh!

sunshowers commented May 20, 2025 •

edited

Loading

sunshowers Jun 6, 2025 •

edited

Loading