Repository navigation
allow operators to override the sled-add prechecks - #11638
Merged
Merged
Conversation
Add a new version of the Nexus external API, SLED_ADD_PRECHECK_OVERRIDES, in which the request to add sleds to a rack accepts `override_prechecks`. It has two booleans, both false by default: * `allow_without_settled_target_release` skips the checks that the system has settled on its target release (no update in progress, no mupdate waiting to be cleared, and updates not suspended). * `allow_unverified_sled_software` skips the checks that the new sleds' SP, RoT, RoT bootloader, and host phase 1 images match the target release. Overridden checks still run when possible. Any problem they find is logged as a warning instead of being returned. A board that is not a sled is always refused. The older `sled_add` endpoint was removed from the API before this version, so it always runs every check.
ahl
approved these changes
Oct 30, 2026
ahl
left a comment
ahl
left a comment
Contributor
There was a problem hiding this comment.
looks great. thanks for doing this.
| @@ -26949,6 +26949,18 @@ | |||
| "RackMembershipAddSledsRequest": { | |||
| "type": "object", | |||
| "properties": { | |||
| "override_prechecks": { | |||
Contributor
There was a problem hiding this comment.
why "prechecks" rather than, say, "checks"?
Collaborator
Author
There was a problem hiding this comment.
Either is fine with me. "precheck" is slightly more evocative to me of something we're going to run before we do anything, and if they fail, nothing will have happened. "checks" could be something else (e.g., a check that the system is healthy after? I don't know). I really don't care though.
Comment on lines
+28493
to
+28494
| "allow_without_settled_target_release": { | ||
| "description": "Add the requested sleds even if the system has not settled on its current target release\n\nThis allows sleds to be added when the system is in the middle of a software update or if it has recently been MUPdated and that MUPdate hasn't been resolved. This is not generally supported because in order for adding a sled to work, the versions of software on that sled must match the rest of the control plane, but that's not well-defined in these two cases\n\nThis override is provided for working around bugs and only in emergency situations. It should not generally be used.", |
Contributor
There was a problem hiding this comment.
in our docs, a single space after a period is much more prevalent.
| ] | ||
| }, | ||
| "SledAddPrecheckOverrides": { | ||
| "description": "Instructs the system to override safety checks when adding this sled", |
Contributor
There was a problem hiding this comment.
Suggested change
| "description": "Instructs the system to override safety checks when adding this sled", | |
| "description": "Instructs the system to override safety checks when adding a sled", |
?
| @@ -26949,6 +26949,18 @@ | |||
| "RackMembershipAddSledsRequest": { | |||
| "type": "object", | |||
| "properties": { | |||
| "override_prechecks": { | |||
| "description": "Optional overrides for various safety checks\n\nOverriding checks is dangerous and should only be done at the direction of Oxide support.", | |||
Contributor
There was a problem hiding this comment.
Is there anything worth adding with regard to the specific danger e.g. what the consequences of misuse might be?
Collaborator
Author
There was a problem hiding this comment.
I tried to do this with the specific checks in this struct, but honestly, it's too complicated and inside-baseball to give a good explanation for most customers. These overrides are here in case we run into a situation where it's really important to add a sled and either (1) the logic for the checks is implemented wrong, or (2) we (i.e., engineering or maybe support) have determined it to be okay. I tried to convey that in the docs by saying with:
/// This override is provided for working around bugs and only in emergency
/// situations. It should not generally be used.
It's worth being explicit with ourselves about it though. I'll start with the second one. Concretely, if the sled software is mismatched, the most likely failure mode (other than it just working because some of it happens to be close enough) is that the process of adding it just stops without explanation. The request might succeed, but the async process of adding it fails and there's no way for you to see it. However, you could reasonably use allow_unverified_sled_software: true if you knew that the release on the sled really was matched (e.g., if one was 23.1 and the other was 23.2 and you knew there were no meaningful changes to the SP or sled agent API in those releases). Again, I still wouldn't advise it unless we needed to.
The other check, allow_without_settled_target_release, is trickier. There are a few ways this check can fail:
- We don't support adding a sled if you've never set the target release or if there's an unresolved MUPdate because in both cases we don't know what version of software should be on the sled. But if you're overriding that as above, you don't care about this, either.
- We don't support adding a sled in the middle of an update because in that case, the correct version of software on it depends on where we are in the update. If it's running the old SP+host OS, but we've already started updating other stuff, that's not valid. In practice, it might be okay as long as Nexus hasn't been updated yet, but it's possible that it has been updated and then might not be able to talk to this host OS. In that case, again, the update will get stuck and it won't be evident why. But again: it's possible for engineering, PA, or a support person to know that these aren't risks and proceed anyway.
jgallagher
approved these changes
Nov 2, 2026
| self.sled_check_versions( | ||
| opctx, | ||
| [&baseboard_id], | ||
| &SledAddPrecheckOverrides::default(), |
Contributor
There was a problem hiding this comment.
I think I picked this up from you a long time ago - should we use a more descriptive name than default here? SledAddPrecheckOverrides::none() or SledAddPrecheckOverrides::no_overrides() or something like that?
Collaborator
Author
There was a problem hiding this comment.
Yeah, I wasn't super excited about this. I can get behind default when it's super clear what the semantics are (like maybe for i32), or when the default can't reasonably be computed by the caller (like the default path to store user configuration, which varies from system to system), or when it's very unusual to need to change it (like in this case, hopefully). I like the idea of not impl'ing Default on the type and using something like no_overrides(), but it seemed wrong at the API level to make people write out that they're not overriding anything. The natural way to let them leave that out is to impl Default on this type. But I could as well use the serde annotation to get the default from a helper, though. I don't have a strong feeling either way here.
Collaborator
Author
There was a problem hiding this comment.
I went down this path, but I think using #[serde(default = ...)] instead of #[serde(default)] might have the side effect that users of generated clients have to provide these fields instead of getting a default? That seems meaningfully worse to me. What do you think @jgallagher @ahl?
Contributor
There was a problem hiding this comment.
That... seems terrible and not at all what I'd expect. Is that because the default only applies to deserialization and doesn't make it to the OpenAPI spec?
Collaborator
Author
There was a problem hiding this comment.
I didn't poke too hard at it, to be honest. 7142d62 is the commit in which I undid this. In the "before", it was using #[serde(default = ...)]. Nothing about the default value for this top-level field makes it into the OpenAPI document. That's also true in the "after" version with #[serde(default)], but the difference is that the individual boolean fields themselves have default values and those do make it into the OpenAPI document. Since these are plain objects, I imagine a client can infer that they can construct the whole struct from the defaults, but I'm not sure if that happens in practice or if that's technically implied by the spec.
Collaborator
Author
Manual testing notesSummaryThis should be adequately tested by the tests here, but given the importance, I wanted to test on a racklette, too. I deployed commit a166dd1 to the
Proves:
I did not check the case of being in the middle of an update. Testing detailsFor each of the expungements, I did this, based on the support procedure:
I had to blow through this warning each time:
but I determined that should be fine and was hard to avoid. It's fine because this warning is trying to ensure that the components on the sled aren't running any more and I've determined that by virtue of rebooting it. (The first time I also confirmed that sled agent had been unable to adopt disks and start any zones.) It's hard to avoid because inventory will keep seeing the SP since it's not going away in any of this. This warning was written prior to support for kicking the sled out of TQ and I think it could use some editing. I'll file an issue for that. Now, for the rest of the test details. DetailsInitial state: To find the rack id: I'll walk through the first expungement for completeness: This eventually got to: After rebooting the sled I looked at sled agent and found: which is good, I think. Finally: Reconfigurator moved a bunch of stuff to different sleds: I waited for execution of that to complete: and it's now gone: or, rather, decommissioned: Now, let's add it back using the documented procedure: That (and each successful "add" after this) created three new blueprints: one adds the sled with little information, one adds the internal DNS zone, and the last one adds the Crucible zones: Once again, I checked that the system was at rest by checking the executor had executed the blueprint with no errors and the planner isn't waiting for anything (at least, not for anything to execute): No services in maintenance, and each sled has a bunch of zones: Following my test plan above, I expunged the sled again. Then I did a MUPdate recovery with the wrong repo (which I'd previously uploaded): I was initially surprised that this made no blueprints. It did fill out the Update page in the console with a target release, etc. And it makes sense there'd be no blueprints. The blueprints we normally expect during MUPdate resolution are one that clears the MUPdate override (which was already done in this case a while ago, right after RSS) and one that resolves artifacts (which hasn't happened yet since it's the wrong repo). The planning report looks very subtly different: It's no longer complaining about the target release generation. I then added the sled again: Excellent -- this is what we should expect when the target release has been set but there's an uncompleted MUPdate recovery. I found at this point that I had to switch to and: I have created three files that differ from Now we can start using them with Now, if we override the MUPdate check, we get a version mismatch: Here, it's comparing the version deployed to the one in the target release. Remember that the target release is wrong -- I deliberately used the wrong TUF repo for the first MUPdate recovery. So that message is right: the sled is running a different SP version than expected. If we override just that, we get the MUPdate error again: But if we override both, we can add the sled: Great. Now, I was surprised that in this case, Reconfigurator wrote one blueprint to add the sled, but didn't add any zones. I believe this is actually expected, though, because we don't proceed with "add" operations when there's been a MUPdate. If that explanation is right, then doing MUPdate recovery with the right TUF repo should immediately cause it to go deploy things. Sure enough: and we're at rest, with a much cleaner planning report, since we're now at a normal situation with Reconfigurator in charge: I expunged the sled again: Re-adding in this state should work. Again, this is a normal state, similar to being between self-service upgrades. And the sled has the right software on it. No overrides should be required: This worked. It took a little longer than expected to move through the blueprints, but this was because TUF artifact replication was bogged down trying to talk to now-expunged sleds. This cleared up as expected. I expunged the sled again. At this point, the last test is to verify that we get the expected error if we put a different SP image on there. (I tried a different RoT image at first, but we haven't changed the RoT image in months and I didn't have a different one handy.) Sled 17 reset. It didn't take long for inventory to reflect it: Now, let's try adding it. With no overrides, we get the expected version mismatch error: With an override of the system state check, we get the same: but if we override just the version, it works: and Reconfigurator went ahead and updated the SP and deployed the usual stuff: This covers the whole test plan. I'm a little surprised that it updated Crucible zones at the end. |
davepacheco
added a commit
that referenced
this pull request
Nov 3, 2026
<del>Depends on #11636.</del> 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-aware `sled_add` path 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.
This was referenced Nov 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.