[Reconfigurator] Host OS update planner support#8832
Conversation
karencfv
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for sharing this! It shouldn't clash too terribly with what I had before d4f31f9 I'll keep a look out.
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? |
eae3c4e to
0fe7dd2
Compare
0fe7dd2 to
8deb34f
Compare
|
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 |
davepacheco
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🤦
karencfv
left a comment
There was a problem hiding this comment.
This looks like it was really tricky to get right! Looks great
| // TODO Need to choose gimlet vs cosmo here! Need help from tufaceous to | ||
| // tell us which is which. |
There was a problem hiding this comment.
Should we open up an issue to follow up on this?
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).