Obtain Host Panic/Boot Failure Messages - #2518
Conversation
Co-authored-by: Eliza Weisman <eliza@elizas.website>
| // this would only happen if packrat crashes. We could store some fake info here to | ||
| // continue preparing an ereport, but THAT is going to be a problem anyway because | ||
| // we send ereports to, you guessed it: packrat, which just crashed. | ||
| let response = self |
There was a problem hiding this comment.
There's probably something better we can do here as well, the only failure modes for a write_host_bootfail (and host panic) call are:
- That's illegal (not supported, OR client dies and revokes the lease)
- Packrat died
Neither of these are particulary good things (the former doesn't even matter here), and there might be a better way to express the idolatry IPC API to make this more clear in the generated code.
|
MGS message changes are tracked in oxidecomputer/management-gateway-service#495. |
|
Should be able to move this out of WIP tomorrow, progress update here: #2504 (comment). I'll address Eliza's comments before marking as ready for review. |
jamesmunns
changed the title
|
This is IMO ready to review, there are still a couple of |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
i have more reviewing to do but here are a few things i left comments on
| reply: Result( | ||
| ok: "HostInfoWriteOutput", | ||
| err: ServerDeath, | ||
| ), |
There was a problem hiding this comment.
The actual functions are Infallible. Other functions model this as a Simple reply type to avoid the need for the extra unwrap. otoh, these functions also copy from a lease so they do have a possibility of a ServerDeath. I think you're right that we might want idolatry to handle this.
* Rework `host` commands This aims to support oxidecomputer/hubris#2518, but also remedy oxidecomputer/hubris#2586. * Update snapshot tests * De-dupe some code
|
Just confirming that this still works after testing: oxidecomputer/management-gateway-service#495 (comment) |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
Overall, this looks great! I commented on a bunch of naming quibbles and other unimportant things, but the implementation looks solid. Do what you want with my suggestions. :)
| encoding: Hubpack, | ||
| ), | ||
| "read_host_bootfail_fragment": ( | ||
| doc: "Read a portion of the host's boot failure message", |
There was a problem hiding this comment.
maybe this ought to explain how the request works? although I suppose the request type has RustDoc, it could be nice to say explicitly what None means...
| host_panic_payload: [u8; 4096], | ||
| host_bootfail_payload: [u8; 4096], | ||
| host_panic_state: Option<HostPanicMetadata>, | ||
| host_bootfail_state: Option<HostBootFailMetadata>, |
There was a problem hiding this comment.
take it or leave it: imo, we can drop host_ from all of these since it lives in a struct named "host info", but you do you
|
@hawkw no major arguments here, I'll make the minor cleanups tomorrow and hit merge unless you want me to wait until you can sign off again. Thanks for the comments! |
3 participants
Not yet complete.
Closes #2504
Currently based on #2503