nexus: prune old blueprints - #10309
Merged
Merged
Conversation
5 tasks
davepacheco
force-pushed
the
dap/blueprint-archive-prune
branch
from
April 22, 2026 04:13
cf54f5c to
4c4454f
Compare
davepacheco
marked this pull request as ready for review
August 31, 2026 20:18
Collaborator
Author
|
This is ready for review. It's pretty big but I'm not sure how to usefully split it up. Suggestions welcome. Right now, it's +3,236/-35 LOC. Of that: 1,800 LOC is wholly new test code, split into about 500 LOC for the datastore interfaces and the rest for the pruner task itself. |
davepacheco
changed the base branch from
main
to
dap/blueprint-prune-datastore
August 31, 2026 20:49
jgallagher
self-requested a review
September 1, 2026 21:10
jgallagher
reviewed
Sep 2, 2026
jgallagher
left a comment
jgallagher
left a comment
Contributor
There was a problem hiding this comment.
Looks great; mostly questions and nits.
Should we also update the wording in the reconfigurator ops guide about people being expected to clean up blueprints after updates? https://github.com/oxidecomputer/omicron/blob/f93149ec58dba4c1f8ebc6c6a4c720f14d85cdbe/docs/reconfigurator-ops-guide.adoc#73-debugging-planner-error-about-too-many-blueprints
| //! - These may have been created by a human who imported the blueprint or | ||
| //! ran the planner explicitly, but then never made it the target. | ||
| //! | ||
| //! Both of these cases should be uncommon enough that we can ignore them. |
Contributor
There was a problem hiding this comment.
In terms of quantity, I agree with this, with maybe a tiny caveat that if there is (or becomes) some way for the planner to get wedged strangely where it's creating blueprints, not making them the target, but not deleting them, we could get more than we expect here.
However, I have a vague concern that we might at some point start to assume "we've been pruning blueprints for a while, so we must not have any blueprints that predate SOME_CONDITION". That's the kind of assumption we generally try not to make without strong evidence, I think, but it still happens. (I'm thinking of the assumption around the bootstore we had recently that removed compatibility with a version that was like 10 releases old, but it turned out we had a customer or two that had never made any network config changes and the bootstore didn't self-update, so their bootstore was still relying on that compatibility.)
I don't want to block this PR on this, certainly, but I wonder if it's worth filing an issue for this, at least? Off the top of my head the thing I'd propose is that we could probably safely prune any blueprint satisfying:
- it's not in
bp_target - its parent is not in
bp_target - its parent blueprint has been deleted
1 gets us "all the blueprints that won't be pruned by this task"; 2 keeps us from deleting a blueprint that's potentially going to be made the next target; 3 keeps us from deleting a chain of blueprints that were manually created/uploaded that could be made the target (as long as that chain were created in order).
| KeepWhat::StartingFromVersion(version) => { | ||
| info!( | ||
| log, | ||
| "will prune blueprints up through version"; |
Contributor
There was a problem hiding this comment.
Nit - this is only true if we don't hit max_delete_attempts first, right? I'm not sure what to suggest wording-wise; maybe something like "found max version of pruneable blueprints"?
| "blueprint_id" => blueprint_id.to_string(), | ||
| ); | ||
|
|
||
| match datastore.blueprint_delete(opctx, &authz_blueprint).await { |
Contributor
There was a problem hiding this comment.
If we have a series of same-blueprint target rows (i.e., from toggling enabled), are we relying on falling into the "blueprint already deleted" result of this query? Is it worth keeping track of blueprints we've already deleted to avoid that? Actually, I guess we'd only need to keep track of the most-recently-deleted blueprint and check if the next row is still the same?
Collaborator
Author
There was a problem hiding this comment.
We are relying on that (intentionally). We could avoid the call, but there's not much downside and I'd rather keep the logic simpler.
| // clones to generate the same random sequences). Really what | ||
| // we need is an interface through which the builder has a | ||
| // mutable borrow of the RNG or else returns it back to us when | ||
| // it's done. |
Contributor
There was a problem hiding this comment.
Do you want to add a builder.build_returning_rng() that gives back both the new blueprint and ownership of the rng?
Collaborator
Author
There was a problem hiding this comment.
Something like that would work, yeah.
| // should never do this. However, if we're not careful, this kind of | ||
| // corruption could cause the pruner to delete the system's current | ||
| // target blueprint, which would be so bad that we go out of our way to | ||
| // make sure we never do that. |
Contributor
There was a problem hiding this comment.
I think we talked about this but just confirming - the pruner checking for this is itself an extra safety on top of blueprint_delete() already refusing to delete the current target, right? That might be worth noting in this (otherwise very-scary-sounding) comment?
Collaborator
Author
There was a problem hiding this comment.
Yeah, there are at least three layers preventing this: blueprint_delete() itself, the check inside the pruner ahead of the call to blueprint_delete() (which is what this is exercising), and the fact that determine_pruneable should never report the current target as pruneable.
I'll rewrite this comment to explain it better.
The following is a lot more detail than you need but I already wrote it and it seemed worth putting somewhere.
To be clear, there are two different things that would be very bad: deleting a blueprint that is currently the target (whether or not you delete the last bp_target row) and deleting the last row in the bp_target table (even if the blueprint that was the latest target were still around).
In terms of deleting the blueprint itself:
blueprint_delete()has a last-ditch check to make sure you don't try to delete the blueprint itself that is referenced by the last row in thebp_targettable.- The only thing calling this is the pruner, which uses
determine_pruneablefirst to figure out what it can delete, anddetermine_pruneableis written to keep the lastNblueprints, andNis a compile-timeNonZero. So it should never report that you can delete the target itself. (But see below.) - Right before calling
blueprint_delete(), there's an emergency check inprune_batch_blueprints_implthat the pruner isn't trying to delete the target blueprint. This would catch a theoretical case where thenkeepversion logic indetermine_pruneablehad a bug, as long as the logic to determine "what is the current target blueprint" was still correct. (Perhaps unlikely, but easy enough to check.)
In terms of deleting the last row in the bp_target table:
- You'd have to be using
bp_target_delete_up_to, which requires the return value fromdetermine_pruneable, which has the safeguard above. bp_target_delete_up_toalso has an emergency check that it's not somehow deleting the last row.
You could kind of divide these into "policy choices enforced by using determine_pruneable" and "emergency checks in case everything else went wrong".
This test is arguably dubious. I'm trying to exercise one of the emergency checks by constructing a kind of database broken-ness that would fool the determine_pruneable safeguard. Namely, that function's correctness depends on the fact that if a blueprint A appears in bp_target followed by a different blueprint B, then A will never appear at some later point in the sequence. That should be true. This test invalidates that in a way that, without the emergency checks, would cause the pruner to delete the actual target blueprint (because it found the earlier reference).
| /// count of `bp_target` rows deleted | ||
| pub ntargets_deleted: usize, | ||
| /// warnings encountered while pruning | ||
| pub warnings: Vec<String>, |
Contributor
There was a problem hiding this comment.
Should this be errors? This is populated by PruneTracker::errors, and if we have a (non-spurious) error deleting a blueprint, we may stay stuck on it indefinitely, since it'll always be the oldest one we try to delete?
Collaborator
Author
There was a problem hiding this comment.
Sure. I didn't want people to mistakenly think that some deletion hadn't also happened if these were encountered, but I've updated the field name and documented that.
Collaborator
Author
I did this in a separate PR (#11224) so we could iterate on the wording there -- thanks for the quick review! |
This was referenced Sep 2, 2026
davepacheco
commented
Sep 2, 2026
davepacheco
left a comment
davepacheco
left a comment
Collaborator
Author
There was a problem hiding this comment.
I think I've addressed all the feedback here and could use a (hopefully quick) re-review. Thanks!
jgallagher
approved these changes
Sep 3, 2026
Collaborator
Author
|
I did some manual testing on the
This is all tested in the automated tests but I wanted to smoke test them on a real system in case I'd missed some way in which real systems look different than the test suite. DetailsInitial stateHere's the state after the upgrade: That shows bp_target has 94 rows and there are 94 blueprints (this is off by one because of the header row): The pruner sees all 94 and prunes nothing: Basic operationConfigure it to keep only 85. This should cause 9 to be pruned: We have to check all three Nexus instances to see what all the pruners did: Good. 9 blueprints were pruned and they were the oldest 9. There are 9 fewer in the table: and they don't show up at all in Disabled operationNext, I disabled the task and also set The pruner ran promptly and did nothing because it's disabled: I enabled it again: and saw it prune another 10 as expected (I caught this before two had noticed the flag change): We're down another 10: Interaction with
|
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
2 participants
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 #11203. Part of #7278. This adds a new background task that prunes old blueprints and
bp_targetrows. This does work, but there's still some work I want to do here: