minibar: add control-plane-agent task to minibar #2363
Conversation
|
Your CI build is unhappy because |
| ) -> Result<(), RequestError<ControlPlaneAgentError>> { | ||
| Err(ControlPlaneAgentError::DataUnavailable.into()) | ||
| } | ||
|
|
||
| pub(crate) fn get_host_phase2_data( | ||
| &mut self, | ||
| _image_hash: [u8; 32], | ||
| _offset: u64, | ||
| _data: Leased<idol_runtime::W, [u8]>, | ||
| ) -> Result<usize, RequestError<ControlPlaneAgentError>> { | ||
| Err(ControlPlaneAgentError::DataUnavailable.into()) | ||
| } | ||
|
|
||
| pub(crate) fn startup_options_impl( | ||
| &self, | ||
| ) -> Result<HostStartupOptions, RequestError<ControlPlaneAgentError>> { | ||
| // We don't have a host to give startup options; no one should be | ||
| // calling this method. | ||
| Err(ControlPlaneAgentError::InvalidStartupOptions.into()) |
There was a problem hiding this comment.
perhaps these errors all ought to be ControlPlaneAgentError::OperationUnsupported? I think that more accurately communicates "I am not a board that knows how to do that"
There was a problem hiding this comment.
...oh, i guess this is what the PSC and Sidecar implementations return for these methods. That feels a little weird to me, but I guess it's better to do what we do elsewhere, maybe? It could be worth checking if that error variant even existed when it was decided that PSC/Sidecar should return these...
There was a problem hiding this comment.
I don't spend much time in this code, but I agree that OperationUnsupported feels like a much better error here (and probably those other places too!). This would also apply to set_startup_options_impl below.
There was a problem hiding this comment.
I'm a little inclined to leave this the way it is on PSC and Sidecar in this PR, and then change all of them separately.
| ) -> Result<(), RequestError<ControlPlaneAgentError>> { | ||
| Err(ControlPlaneAgentError::DataUnavailable.into()) | ||
| } | ||
|
|
||
| pub(crate) fn get_host_phase2_data( | ||
| &mut self, | ||
| _image_hash: [u8; 32], | ||
| _offset: u64, | ||
| _data: Leased<idol_runtime::W, [u8]>, | ||
| ) -> Result<usize, RequestError<ControlPlaneAgentError>> { | ||
| Err(ControlPlaneAgentError::DataUnavailable.into()) | ||
| } | ||
|
|
||
| pub(crate) fn startup_options_impl( | ||
| &self, | ||
| ) -> Result<HostStartupOptions, RequestError<ControlPlaneAgentError>> { | ||
| // We don't have a host to give startup options; no one should be | ||
| // calling this method. | ||
| Err(ControlPlaneAgentError::InvalidStartupOptions.into()) |
There was a problem hiding this comment.
I don't spend much time in this code, but I agree that OperationUnsupported feels like a much better error here (and probably those other places too!). This would also apply to set_startup_options_impl below.
| @@ -0,0 +1,207 @@ | |||
| // This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
This entire file seems like a lot of copy/paste from task/control-plane-agent/src/mgs_sidecar/ignition.rs can we see if there's anything we can keep in common?
There was a problem hiding this comment.
aa83c69 factors this out so that the majority of the ignition controller code is defined once and shared by Sidecar and Minibar.
I note that minibar lacks code for actually sending ignition commands, which is present on sidecar. Perhaps we ought to put that in both systems as well? It seems like it could be useful to be able to actually send ignition commands to the attached sled...
There was a problem hiding this comment.
okay, I just went ahead and enabled all Ignition functionality on both Minibar and Sidecar, because why not? 73df690
|
LGTM but I think we should give this a quick test on a Real Sidecar too to make sure none of the refactoring broke anything (unlikely! but you never know!) |
In manufacturing contexts, we'd like to like to read Ignition port state to address https://github.com/oxidecomputer/facade/issues/298.
We'd like to do this over the management network with
faux-mgs.For the minibar to to respond to
faux-mgsmessages, it needs the thecontrol-place-agenttask.This PR is an attempt at the bare minimum needed to add
control-plane-agentto minibar. This is primarily copy+paste / compiler error message driven. cc @Aaron-Hartwig for comments on if there's something more / different for what we want here.I've tested this PR on the
argonmanufacturing station in Emeryville - a minibar-lite with a rev B board.This change is required for the ignition test in the additional facade tests: https://github.com/oxidecomputer/facade/pull/339