[reconfigurator] Refactor MGS driven updates #9118
Conversation
| #[error("failed Host OS update: {0:?}")] | ||
| HostOs(FailedHostOsUpdateReason), |
There was a problem hiding this comment.
All of these should be using error sources and not embedding debug strings; e.g.,
| #[error("failed Host OS update: {0:?}")] | |
| HostOs(FailedHostOsUpdateReason), | |
| #[error("failed Host OS update")] | |
| HostOs(#[from] FailedHostOsUpdateReason), |
(We also have https://github.com/oxidecomputer/omicron/blob/main/docs/error-types-and-logging.adoc if you want much more detail on this.)
Should the error messages also be "failed to plan"? As written it sounds like we failed to perform an update.
There was a problem hiding this comment.
Yep, that makes total sense thanks! I should really read that doc again 😄
| .pending_persistent_boot_preference, | ||
| expected_transient_boot_preference: rot_state | ||
| .transient_boot_preference, | ||
| Ok(MgsUpdateOutcome::Pending( |
There was a problem hiding this comment.
Style idea, not sure if I like it - what would you think about constructors on MgsUpdateOutcome to avoid (a) having to call Box::new() everywhere and (b) having to call PendingHostPhase2Changes::empty() for non-host-OS updates? Something like
// construct a non-host OS pending outcome
MgsUpdateOutcome::pending_without_host_phase_2(pending_mgs_update);
// construct a host OS pending outcome
MgsUpdateOutcome::pending_with_host_phase_2(pending_mgs_update, pending_host_phase2_changes);There was a problem hiding this comment.
I feel like pending_with_host_phase_2 doesn't really differ from just directly setting MgsUpdateOutcome::Pending(update, pending_host_phase_2_changes). But, I like having the constructor for setting a pending update only. I've changed it this way. Let me know what you think!
There was a problem hiding this comment.
Thanks for taking a look @jgallagher! I think I've addressed all of your comments. Please let me know if I missed anything :)
| .pending_persistent_boot_preference, | ||
| expected_transient_boot_preference: rot_state | ||
| .transient_boot_preference, | ||
| Ok(MgsUpdateOutcome::Pending( |
There was a problem hiding this comment.
I feel like pending_with_host_phase_2 doesn't really differ from just directly setting MgsUpdateOutcome::Pending(update, pending_host_phase_2_changes). But, I like having the constructor for setting a pending update only. I've changed it this way. Let me know what you think!
| #[error("failed Host OS update: {0:?}")] | ||
| HostOs(FailedHostOsUpdateReason), |
There was a problem hiding this comment.
Yep, that makes total sense thanks! I should really read that doc again 😄
| pub trait FailedMgsUpdateReasonComponent { | ||
| /// Converts a failed planned update from a specific component into a | ||
| /// FailedMgsUpdateReason | ||
| fn into_generic(self) -> FailedMgsUpdateReason; | ||
| } |
There was a problem hiding this comment.
I don't think we need this trait - given the #[from]s inside each of the FailedMgsUpdateReason variants, we can say either err.into() or FailedMgsUpdateReason::from(err) to convert any of the component-specific errors.
| pending_host_os_phase2_changes, | ||
| )) => { | ||
| pending_actions.add_pending_update(update); | ||
| // Host OS updates also return phase 2 changes. |
There was a problem hiding this comment.
Tiny nit - this comment seems a little confusing/out of place, since update_attempt might not be a host OS update. I could see striking this altogether, or maybe making it clear that this set will be empty for non-host OS updates? Something like "If update_attempt is a host OS update, stage the phase 2 changes. For any other type, this set will be empty."?
2 participants
Just a few style changes as suggested in #9001
Closes: #9068