Repository navigation
nexus: use debug dropbox to save Reconfigurator state - #10306
Merged
Merged
Conversation
davepacheco
force-pushed
the
dap/blueprint-archive-nexus
branch
from
April 22, 2026 02:44
ce37d0b to
aefe883
Compare
davepacheco
commented
Apr 22, 2026
Collaborator
Author
|
This is ready for review. I'm leaving it "draft" so I don't accidentally land it with the Cargo.toml change unfixed. |
davepacheco
force-pushed
the
dap/blueprint-archive-nexus
branch
from
April 22, 2026 03:46
5cdfcfa to
d575bc3
Compare
5 tasks
jgallagher
approved these changes
Apr 24, 2026
jgallagher
left a comment
jgallagher
left a comment
Contributor
There was a problem hiding this comment.
LGTM - only one real concern and a few nits.
| input, | ||
| vec![(*collection).clone()], | ||
| vec![(*parent).clone(), blueprint.clone()], | ||
| target, |
Contributor
There was a problem hiding this comment.
Will it be confusing if we persist a state claiming this new blueprint is the target, but then fail to make it the target and fail to remove it because sled-agent already slurped it up? We've now archived a state file that claims a blueprint was the target when it never was.
Collaborator
Author
There was a problem hiding this comment.
Yeah, we wound up discussing this here: #10307 (comment)
The only way I can think to improve this is to change the file format so that instead of storing the hopeful target here as the current target, we have a new optional field called blueprint_target_hopeful or something whose semantics are "the blueprint that we're trying to make the target right after we write this file". That would be more precise and accurate and would avoid being misleading.
Then the question is: what do we do with this? If you load this file into reconfigurator-cli either by itself or as the last one in a sequence (which would almost always be the case), then the CLI could either fail ("cannot load a file with an ambiguous target blueprint"), require you to say what you want ("you must specify if you want the loaded state to reflect the target blueprint when this state file was written or the blueprint that we hoped was the target immediately after it"), or pick one and warn you ("assuming that the hopeful target is the target, but beware that it's possible that it wasn't"). If we punt it to the human, what are they going to do? I imagine they'll ignore the warning if they can or else pick the hopeful target because that's pretty likely to be the right one. If all this is correct, I'm not sure it's worth doing any of this work because the end result is the same (although maybe the person has a chance of being more aware of this unusual case?).
Another option would be to add that "hopeful" field as above, but write two files whenever we change the target:
- The first one is written after we create the new blueprint but before we make it the target. It's called
autoplanner-intentor something. The target field reflects the old target (so it's not misleading) and the new hopeful field points to our new blueprint. - The second one is written after we make the new blueprint the target. It's called
autoplanner-commitor something. The target field points to the new blueprint and the new hopeful field isNone.
After we write the commit one, we could attempt to remove the intent one, since it would be superfluous.
Then when debugging, you'd generally have a whole bunch of "commit" files and occasionally an "intent" one, either because it was archived during the brief window before it was removed or else because Nexus actually crashed shortly after creating it.
Again: what would we do with this information? We'd still have to make the above changes to reconfigurator-cli. Namely, when it needs to determine the target blueprint from a file, and the hopeful field is set, then it either needs to pick one and warn the user or else make the user choose one or the other. The only difference (maybe a big one?) is that this would be much less common -- it would only come up if the latest file a person loaded was an "intent" file rather than a "commit" one.
I'm on the fence about whether any of this is worthwhile. What do you think?
Collaborator
Author
There was a problem hiding this comment.
I went ahead and implemented the multi-file approach. In both the planner and the internal API, when we set a new target, we:
- generate and save an "intent" file that reflects not that we've set the target, but that we're trying to
- set the target
- on failure, as before, we attempt to remove the generated file and then bail out
- on success, we save a second file reflecting that we have set the target and then try to delete the intent file
reconfigurator-cli doesn't do much different except warn you if it finds that the latest file has this intent property set so that you know.
I wouldn't say I love the implementation, but it has the runtime properties we want:
- we should always have debug data for every time the blueprint changes (unless we have a permanent sled failure or something like that)
- the file is not misleading about the current state
- minimized noise in terms of spurious files (there will be some intent files that get archived before they get cleaned up, but it will be clear that they don't reflect real system state)
Collaborator
Author
|
Okay, this has changed quite a bit after adding tests that the files get created when we want them to be. I'd recommend doing a full re-review (sorry). This PR contains a few different changes that could theoretically be separated, but I'm keeping them together for practical reasons and because none of them makes sense to land on
I can separate some out if it's useful but it seems weird for example to land these helpers that aren't used, or even to review them separately from where they're used since their shape depends on what we're doing with them. There is one fix here that's pretty unrelated and I'm going to see if I can pull that out. |
Collaborator
Author
|
I pulled that fix out into #11125 (and it turns out that was a bug I only hit while developing one of the tests here -- it doesn't actually break anything here). |
Collaborator
Author
|
This is finally ready for re-review. |
jgallagher
self-requested a review
August 24, 2026 13:13
jgallagher
reviewed
Aug 24, 2026
| /// Temporary directory used for tests | ||
| /// | ||
| /// Unlike typical temporary directories, this one is preserved by default, but | ||
| /// cleaned up on `Drop` if the caller invokes `cleanup_successful()`. The |
Contributor
There was a problem hiding this comment.
I'm not sure this matters much since cleanup_successful() takes ownership of self, but reading this I assumed cleanup_successful() was setting something that Drop read when deciding whether to clean up. It's actually cleanup_successful() that does the cleanup though, right?
Suggested change
| /// cleaned up on `Drop` if the caller invokes `cleanup_successful()`. The | |
| /// cleaned up if the caller invokes `cleanup_successful()`. The |
| }, | ||
|
|
||
| /// We've encountered an error and will be ignoring subsequent operations. | ||
| Failed(#[cfg_attr(not(test), expect(dead_code))] anyhow::Error), |
Contributor
There was a problem hiding this comment.
The cfg_attr here is because nothing ever reads the error outside of tests? Do we need to keep the error at all, then, or could this be just Failed with no associated data?
Collaborator
Author
There was a problem hiding this comment.
We read the specific error in the tests, and I think that's fairly important in order to have confidence that we're testing what we think we are. If you're asking if we could only have the anyhow::Error under cfg(test), yes, but that would require sprinkling cfg(test) in a lot more places that seemed worse to me on net. I'll take a look though.
Contributor
There was a problem hiding this comment.
Ehh if it's painful I wouldn't worry about it. Maybe a short comment explaining the choice is fine too?
| datastore, | ||
| planning_context.planning_input, | ||
| IdOrdMap::from_iter([inventory]), | ||
| IdOrdMap::from_iter([blueprint.clone()]), |
Contributor
There was a problem hiding this comment.
Should this include the parent blueprint too? If we write the intent file but not the final one, we'll have written a file that doesn't include the current target blueprint. I don't think reconfigurator-cli will load that on its own, right?
| ..intent_state | ||
| }; | ||
|
|
||
| // unwrap(): we checked this in `SetTargetDebugFile::new()`. |
Contributor
There was a problem hiding this comment.
Nit:
Suggested change
| // unwrap(): we checked this in `SetTargetDebugFile::new()`. | |
| // unwrap(): we checked this in `SetTargetDebugWriterReady::new()`. |
However, I'm not sure this is technically true: we checked that the intended target blueprint was present in SetTargetDebugWriterReady::new(), but there's no guarantee here that the new_target we've been given by the caller actually matches that, right?
Collaborator
Author
There was a problem hiding this comment.
Yeah, that's accurate. I considered checking for that case but intended to handle it -- but forgot about this unwrap() in the editing. I've updated this to log an error and stop in this case.
| .expect("autoplanner should have created new blueprint within 60s"); | ||
|
|
||
| let files = dropbox.load_new::<UnstableReconfiguratorState>(); | ||
| assert_eq!(files.len(), 1); |
Contributor
There was a problem hiding this comment.
Is this racy with the wait_for_condition above? The planner makes the new blueprint the target (any point after which we can get here), and then calls write_committed() to emit the final state file. So here we could see either of these in addition to what the test expects:
- just the intent file (if we land in between the db query and
write_committed()) - both the intent file and the final state file (if we land in between depositing the final state file and cancelling the intent file)
Collaborator
Author
There was a problem hiding this comment.
Nice catch. I've inserted a call to wait for another activation to finish. (That's more than we need, but it didn't seem worth adding another helper.)
|
|
||
| // Case: autoplanner produces no files when it doesn't generate a new | ||
| // blueprint. | ||
| let _ = run_blueprint_planner(&cptestctx.lockstep_client).await; |
Contributor
There was a problem hiding this comment.
Do we need to check the return value here? If the planner failed we'd have an empty dropbox, but that's obviously not the intent of the test
| pub async fn write_committed( | ||
| self, | ||
| new_target: BlueprintTarget, | ||
| commit_reason: BlueprintDebugAction, |
Contributor
There was a problem hiding this comment.
Is it sensible/meaningful for a caller to pass different reasons to write_intent() / write_committed()? If not, should we take a single reason on construction instead?
Collaborator
Author
There was a problem hiding this comment.
For context, BlueprintDebugAction is really an abstraction (if you can call it that) over the final filename. All it's used for is to determine what the file should be called, and that's just so that people can tell at a glance when they have a bunch of files what they represent (e.g., "the autoplan run that generated X" vs. "the time an operator set the target blueprint to Y").
Today, there are two paths through this helper:
- The autoplanner passes
AutoplanIntentfor the intent file and thenAutoplanfor the commit one. - Setting the target via the internal API passes
TargetIntentfor the intent file and thenTargetfor the commit one.
So to answer your question: yes, you'd always be expected to pass different reasons to these functions. But these functions don't care about that. They treat the argument like a "filename" argument and let the caller decide.
I'm not sure if this is where you're going, but there are things not checked here by the compiler that in principle could be. Most obviously, you could pass AutoplanIntent and then Target or the other way around. Or you could pass Plan. (Plan is a fifth variant that's used when you use the internal API to re-run the planner but aren't setting the new blueprint as the target. There's no need for an intent file in that case and it does not go through this helper.) The way I'd phrase this is that the way this is implemented today, the filenames are fully the responsibility of the caller. This helper just uses whatever it's given. It doesn't know or care that there are only two callers, with two corresponding pairs of BlueprintDebugAction.
If we wanted to improve this, the obvious thing would be to define something like this:
/// who/what is setting the target
enum SetTargetWhy {
Autoplanner,
/// the "set target" internal API
SetTarget,
}
impl SetTargetAction {
fn intent_action(&self) -> BlueprintDebugAction { .. },
fn commit_action(&self) -> BlueprintDebugAction { .. },
}I didn't like this for two reasons: combined with the existing enum, it felt like a lot of boilerplate and conceptual overhead just for the purpose of naming the file usefully; and it felt too much like having an argument called "which of my two callers are you so that I can do the right thing for you", rather than letting the callers just specify what it is they want. I don't have a strong feeling about it.
Contributor
There was a problem hiding this comment.
Ahh, gotcha. Sorry, I should've looked more closely at the callers. That all sounds perfectly reasonable.
| // potentially debug the planning choices we made. The details of | ||
| // writing out the files are handled by `SetTargetDebugWriter`. | ||
| // | ||
| // The part of the process is best-effort. If any of it fails, we'll |
Contributor
There was a problem hiding this comment.
Suggested change
| // The part of the process is best-effort. If any of it fails, we'll | |
| // This part of the process is best-effort. If any of it fails, we'll |
| pub target_blueprint: BlueprintTarget, | ||
| /// If non-`None`, then this file was created as the system was attempting | ||
| /// to make this blueprint the new target | ||
| // We use `serde(default)` to supporting deserializing older-format state |
Contributor
There was a problem hiding this comment.
Suggested change
| // We use `serde(default)` to supporting deserializing older-format state | |
| // We use `serde(default)` to support deserializing older-format state |
| target_blueprint: BlueprintTarget, | ||
| intended_target_blueprint: Option<BlueprintUuid>, | ||
| ) -> Result<UnstableReconfiguratorState, anyhow::Error> { | ||
| // It's also useful to include information about any DNS generations |
Contributor
There was a problem hiding this comment.
Nit - this comment sounds slightly weird now that it's the first line of a new function
Collaborator
Author
There was a problem hiding this comment.
Ha, good call.
jgallagher
approved these changes
Aug 24, 2026
Collaborator
Author
|
This PR and #10308 changed a lot since my last end-to-end test on a racklette so I wanted to repeat that. Here's what I did:
Both of those last two commits have the changes in this branch. So what we'd expect is:
tl;dr: all that matched what I found. Note: many of the commands below use the Test processDetailsInitial state: Performing MUPdate recovery: I'm not sure I'd done this before. I saw some intermediate states: but it pretty quickly converged as expected: Next, starting a self-service update:
I started the update in the browser. It finished after about an hour: and everything looks good. I started a second update that also worked fine: I ran the planner by hand and made the new blueprint the target: Now, do the export/import path: Full Reconfigurator history at the end: DetailsChecking the files producedDetailsFirst, collect them: I copied these over to my dev system and extracted them. Total file counts: That's 104 files, which matches what I extracted. For convenience, I linked them all into one spot: Now, loading it up into That all looks good! I was initially worried about the error around blueprint 91fd92c5-a907-47d4-9f4f-201eddc80acc before I realized: this is the last one that was created by the Nexus zones that predate my changes. So that's expected -- that's what happens when one of these files is missing. Now, based on Checking other behaviorDetailsI did these checks after the first update, not the second. At this point we only had a few archived files. Excellent. Any issues reported by sled agent? Reviewing the above:
Any other worrying messages related to archival? That all looks okay to me. In terms of the period we care about, it's these blueprints: There's only one rotated log file so far. It covers our period: The last warning-level messages were from right after startup: Looking at distinct warning-level messages: Looks fine. Some example messages from the planner:
This is all looking pretty good. |
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.
Part of #7278.