[nexus] add slot number to external API sled responses - #11077
Conversation
hawkw
added
api
| part_number: sled.part_number.clone(), | ||
| }; | ||
| let sp = inv.sps.get(&bbid)?; | ||
| Some(sp.sp_slot as u8) |
There was a problem hiding this comment.
I pretty strongly dislike as casts for their silent failure modes. I strongly suspect sp_slot being a u16 is the fault of 4-years-ago-me, but given that's what we're using, is there a reason not to use u16 over u8 for the sled slot too? Both allow plenty of "should be invalid today" values, but not having to convert seems nice.
If we really want to use u8, are we sure enough about that that this could be u8::try_from(sp.sp_slot).expect("...") to change the "silent failure mode" into "loud failure mode"?
| revision: SqlU32, | ||
| pub serial_number: String, | ||
| pub part_number: String, | ||
| pub revision: SqlU32, |
There was a problem hiding this comment.
Having to make all these fields pub is kind of a bummer. Not sure this is a good idea but thinking out loud about how we've handled this other places: could we keep these private if we hang a method off of Sled to do the conversion that's currently in db_sled_to_external, and let it take an extra arg (either the slot or the whole inventory) to fill in the missing info?
There was a problem hiding this comment.
Hm, yeah, I think making the conversion a method on the db::model::Sled type is probably the nicer thing. I don't love having it take the slot as an argument because I didn't really want to permit you to construct a sled with any arbitrary slot --- ideally, the entire thing that does the SP slot lookup and constructing the external_api::Sled would be encapsulated. But I suppose it could just take the entire inventory reference, even though that feels a bit goofy.
| }, | ||
| "slot": { | ||
| "nullable": true, | ||
| "description": "The physical slot in the rack where this sled is currently located, or null if its location is not known at this time.", |
There was a problem hiding this comment.
This is a turbo nit but I'm tempted to say this should say something like "was most recently located" or "was most recently observed" instead of "is currently located" as a nod toward the fact that inventory is always stale.
|
@jgallagher thanks for the review, I pretty much agree with all your suggestions. I think 602180c addresses everything? |
| "type": "integer", | ||
| "format": "uint16", | ||
| "minimum": 0 | ||
| }, |
There was a problem hiding this comment.
Can’t argue with that. API APPROVED
3 participants
As described in #11048, it would be nice if the public API
/system/hardwareinventory endpoints included the physical location of the inventoried baseboards. This PR adds that to the sled APIs, by consulting the inventory to look up the latest location for the sled baseboard's SP, if one exists. The returnedslotnumber is nullable, as the inventory may not include an entry for the SP.The code here is a bit tortured, because gluing together something from the inventory onto a DB query requires constructing the new API models pretty close to the actual HTTP API, rather than in
nexus-db-modelsin aFromconversion. I think this is probably not the worst thing, but it's a bit gross.Fixes #11075