Repository navigation
verify system state and sled software when adding a sled - #11637
Conversation
…ction Add `Collection::caboose_with_version_for()`, which looks up a caboose and parses its version, along with helpers for the active SP and RoT bootloader cabooses. Add `ExpectedVersion::for_inactive_caboose()` to turn the result for an inactive slot into the version expected there. Split the code that chooses each component's target artifact out of the SP, RoT, RoT bootloader, and host OS planners into its own function. The SP, RoT, and RoT bootloader versions take the active caboose that the planner already looked up, so each caboose is looked up only once. Split the planner's `Failed*UpdateReason` types so that errors from looking up a caboose's version (`CabooseVersionError`) and from choosing a target artifact (`TargetArtifactError`, `HostOsTargetArtifactError`) are their own types. The messages are unchanged. This changes the nexus-lockstep API and the shape of the planning report's JSON.
When a target release is set, refuse to add a sled unless the system has settled on that release (no update in progress, no mupdate waiting to be cleared, and updates not suspended) and the new sled's SP, RoT, RoT bootloader, and host phase 1 images match it. The RoT's boot preferences must also show no update that has not fully taken effect. This applies to both `sled_add` and `rack_membership_add_sleds`. The comparison lives in the planner's `mgs_updates::deployed_versions` module so that it uses the same artifact selection as the planner. Add `Collection::sp_for()` and `Collection::active_host_phase_1_hash_for()`, which report a missing item with a new `NotInInventory` error.
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Do we also need to add these checks to the multirack join path (cc @andrewjstone)? I'm not sure if we can do that yet since it depends on polling the joining rack's MGS as part of inventory. But something to add to that TODO list (I hear it's not long enough).
| .find_hw_baseboard_id(opctx, &baseboard_id) | ||
| .await?; | ||
|
|
||
| // Check the sled's software before allowing it to be added to the |
There was a problem hiding this comment.
I know you mentioned it's a lot of work to actually drop the API version that calls this, but should we drop this implementation and return a 410 GONE or something like that? If we allow old clients to hit this implementation, are we allowing a sled to be added without adding it to TQ?
There was a problem hiding this comment.
That's a fair question. I don't know why that wasn't done initially. I think it's an orthogonal question, in that this change doesn't make anything worse -- it actually adds the validation to that path.
@andrewjstone what do you think we should do here?
There was a problem hiding this comment.
I like the idea of returning a 410 Gone. I think it wasn't done originally because the endpoint was added before the code was done merging. Either that or I completely overlooked it. In practice, support has been the only ones adding sleds so far, and definitely so at the time, so I figured we'd just upgrade out of it and not have an issue. But sure, a customer could be holding on to a super old client.
| self.datastore().target_release_get_current(opctx).await?; | ||
| let release_source = target_release.release_source().map_err(|e| { | ||
| Error::internal_error(&format!( | ||
| "loading current target release: {e:#}" |
There was a problem hiding this comment.
This is right as long as e is an anyhow::Error, but I'd probably write this as
| "loading current target release: {e:#}" | |
| "loading current target release: {}", InlineErrorChain::new(&*e), |
so if the error type changes we don't end up losing the inner errors.
I think this is only possibly an issue upon rack adoption. The multirack join service shouldn't actually insert into the sled-table. I have the gate PR up still, although it relies on @sunshowers upcoming changes. |
Yeah, sorry, I meant in the rack adoption path. Specifically, we don't want to adopt a rack if it's not running the same version as the protorack. Ideally we could reuse these checks to enforce that, although again there's the problem of getting inventory for the new rack's SPs before it's joined... |
|
@jgallagher This is ready for re-review now, if you want to take another look. |
Depends on #11637 and I intend to land these together, but I separated them out for review. This PR makes the checks added in 11637 overridable. The goal here is safe-by-default, overridable by someone who knows what they're doing, and a naive operator shouldn't be able to override it without realizing it's risky.
Depends on #11636.This PR adds checks to the "add sled" API that verify the RFD 747 conditions for self-service sled add: that there's no self-service update in progress and that the sled's software is running the right versions (as best we can tell, which ought to be good enough in practice). This version of the PR removes the old, non-TQ-awaresled_addpath that existed for compatibility with older clients. That was removed in R19 but was still being served for older clients. We should get rid of this altogether, but that's a lot more work. (See #11641.)I've got a follow-up in #11638 that makes these new checks overridable. This is important because this is the only way to add sleds today and I think support should be able to say "I know this version is compatible enough" or something like that.