Skip to content

Conversation

jgallagher
Copy link
Contributor

This isn't ready to merge yet; in particular, there aren't any new planner unit tests. I'm opening this now to avoid conflicting with changes @karencfv is making to the reconfigurator-cli tests (which do have updates for host OS planning on this branch).

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just a heads up that I'm changing some of the unit test structure for mgs_updates/mod.rs to account for more components here -> https://github.com/oxidecomputer/omicron/pull/8664/files#diff-7c888dfce867a1f4a2b025ee3a82410a7ef7b14785ce5a795083aa4ff148dcd9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing this! It shouldn't clash too terribly with what I had before d4f31f9 I'll keep a look out.

@jgallagher
Copy link
Contributor Author

Thanks! Just a heads up that I'm changing some of the unit test structure for mgs_updates/mod.rs to account for more components here -> https://github.com/oxidecomputer/omicron/pull/8664/files#diff-7c888dfce867a1f4a2b025ee3a82410a7ef7b14785ce5a795083aa4ff148dcd9

Ahh, thanks. Honestly I'm finding the unit test stuff a little unwieldy, so I was going to take a swing at some cleanup before adding host OS unit tests. I was building off of main for that cleanup, but maybe I should build off of your branch instead?

@jgallagher jgallagher force-pushed the john/host-os-updates-planner branch from eae3c4e to 0fe7dd2 Compare August 14, 2025 21:03
@jgallagher jgallagher changed the base branch from main to john/mgs-update-test-cleanup August 14, 2025 21:03
Base automatically changed from john/mgs-update-test-cleanup to main August 15, 2025 17:01
@jgallagher jgallagher force-pushed the john/host-os-updates-planner branch from 0fe7dd2 to 8deb34f Compare August 15, 2025 20:16
@jgallagher jgallagher marked this pull request as ready for review August 15, 2025 20:16
@jgallagher
Copy link
Contributor Author

I think this is ready for review (demo'd today on dublin!).

The diff is quite large but the vast majority is test code, and over half the diff is expectorate output. It's probably still worth reviewing that output at least at a superficial level, particularly the cmds-target-release-stdout test that walks through a whole-system update (at least up to the point of updating zones).

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nits and clarifications here. I didn't get very far through dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout because something early on changed that I didn't understand.

> inventory-generate
generated inventory collection 61f451b3-2121-4ed6-91c7-a550054f6c21 from configured sleds

> blueprint-plan latest latest
INFO performed noop image source checks on sled, sled_id: 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c, num_total: 9, num_already_artifact: 0, num_eligible: 0, num_ineligible: 9
INFO performed noop image source checks on sled, sled_id: 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6, num_total: 8, num_already_artifact: 0, num_eligible: 0, num_ineligible: 8
INFO performed noop image source checks on sled, sled_id: d81c6a84-79b8-4958-ae41-ea46c9b19763, num_total: 8, num_already_artifact: 0, num_eligible: 0, num_ineligible: 8
INFO MGS-driven update completed (will remove it and re-evaluate board), artifact_version: 1.0.0, artifact_hash: 04e4a7fdb84acca92c8fd3235e26d64ea61bef8a5f98202589fd346989c5720a, expected_transient_boot_preference: None, expected_pending_persistent_boot_preference: None, expected_persistent_boot_preference: A, expected_active_slot: ExpectedActiveRotSlot { slot: A, version: ArtifactVersion("0.0.2") }, expected_inactive_version: NoValidVersion, component: rot, sp_slot: 0, sp_type: Sled, serial_number: serial0, part_number: model0
INFO configuring MGS-driven update, artifact_version: 1.0.0, artifact_hash: 7e6667e646ad001b54c8365a3d309c03f89c59102723d38d01697ee8079fe670, expected_inactive_version: NoValidVersion, expected_active_version: 0.0.1, component: sp, sp_slot: 0, sp_type: Sled, serial_number: serial0, part_number: model0
INFO MGS-driven update not yet completed (will keep it), artifact_version: 1.0.0, artifact_hash: 04e4a7fdb84acca92c8fd3235e26d64ea61bef8a5f98202589fd346989c5720a, expected_transient_boot_preference: None, expected_pending_persistent_boot_preference: None, expected_persistent_boot_preference: A, expected_active_slot: ExpectedActiveRotSlot { slot: A, version: ArtifactVersion("0.0.2") }, expected_inactive_version: NoValidVersion, component: rot, sp_slot: 0, sp_type: Sled, serial_number: serial0, part_number: model0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah crap, great catch. My line 757 has the sled-update-rot line incorrectly commented out. Will fix and push up the new output shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed now, if you want to continue. I'll claim I've read through it and I think the output matches what the comments claim should be happening, but I thought I'd done that before and clearly missed this. 🤦

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it was really tricky to get right! Looks great

Comment on lines 416 to 417
// TODO Need to choose gimlet vs cosmo here! Need help from tufaceous to
// tell us which is which.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we open up an issue to follow up on this?

Copy link
Contributor Author

@jgallagher jgallagher Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a few! #7873, #8623, and #8777 are all closely related to this. I think the last one is the most specific; I'll add a link.

@jgallagher jgallagher merged commit 0d5d6c2 into main Aug 19, 2025
16 checks passed
@jgallagher jgallagher deleted the john/host-os-updates-planner branch August 19, 2025 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants