(7/N) Use nexus_generation, update it #8936
Conversation
smklein
force-pushed
the
image_reporting
branch
from
9b1f7e3 to
86cee63
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
2 times, most recently
from
d1bd3fb to
bf8f274
Compare
smklein
force-pushed
the
image_reporting
branch
from
86cee63 to
91a884e
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
2 times, most recently
from
62a6819 to
30ecc07
Compare
smklein
force-pushed
the
image_reporting
branch
from
91a884e to
73d7258
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
30ecc07 to
8c0f5c9
Compare
smklein
force-pushed
the
image_reporting
branch
from
73d7258 to
8e77874
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
8c0f5c9 to
a2a7fb5
Compare
smklein
force-pushed
the
image_reporting
branch
from
8e77874 to
5578ec2
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
a2a7fb5 to
1067194
Compare
smklein
force-pushed
the
image_reporting
branch
2 times, most recently
from
0b2efdd to
9c09f60
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
1067194 to
bb4a47e
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
bb4a47e to
df27c58
Compare
smklein
force-pushed
the
image_reporting
branch
from
9c09f60 to
c073f52
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
df27c58 to
54480df
Compare
smklein
force-pushed
the
image_reporting
branch
from
c073f52 to
1d6d7a6
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
2 times, most recently
from
6da9f39 to
c4c748f
Compare
smklein
force-pushed
the
image_reporting
branch
from
1d6d7a6 to
997c67b
Compare
smklein
force-pushed
the
nexus_gen_usage
branch
from
c4c748f to
6750df3
Compare
smklein
force-pushed
the
image_reporting
branch
from
997c67b to
5524608
Compare
dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout
Outdated
Show resolved
Hide resolved
| match current_nexus_generation { | ||
| Some(current) => write!( | ||
| f, | ||
| "zone gen ({zone_generation}) >= currently-running \ |
There was a problem hiding this comment.
(I haven't reviewed the planner changes yet, so maybe this question will make more sense once I do.)
Why do we report that the generation is >= the currently-running Nexus's generation for "waiting to expunge"? I thought we'd be expunging zones with a generation < the currently-running Nexus.
There was a problem hiding this comment.
Yeah, you're right - we only do want to expunge zones with a generation < the currently-running Nexus. That's the check we're making in the planner that would need to fail to generate this planning report.
This is called from do_plan_zone_updates in the caller, when we're iterating over out_of_date_zones. So basically:
- We found a zone with an image that does not match what the TUF repo expects it to be (it's part of
out_of_date_zones). - We tried to check
can_zone_be_updated, to see if we can expunge this zone. When we did that, the generation for this observed zone looks "as new or newer" than our currently-running zone.
So, TL;DR: The context for this is report is that "the target TUF repo changed, and this zone looks out-of-date".
I think this is probably most common when the "zone we're trying to expunge" has a generation exactly equal to the active generation, but I wanted to be precise about this check. I think if the "zone we're trying to expunge" has a generation greater than the active generation, that would imply we changed TUF repos mid-update, which we've previously discussed banning anyway.
There was a problem hiding this comment.
This goes to the question I had about "what is a ZoneWaitingToExpunge". What is it we're trying to report here? It doesn't seem like there's anything wrong or blocking?
There was a problem hiding this comment.
What is it we're trying to report here? It doesn't seem like there's anything wrong or blocking?
We are trying to report that there is a zone which has an image that does not match the target TUF repo. For almost all zones, this means "we want to expunge it". For Nexus, however, we only want to expunge it it's not actively running. The logic for this is entirely contained within can_zone_be_updated in nexus/reconfigurator/planning/src/planner.rs.
It doesn't seem like there's anything wrong or blocking?
Well it's not a "wrong" case for sure, but it is blocking, in IMO kinda the same way that "not enough redundancy for a service" could prevent it from being expunged. In this case, we want to expunge these Nexus zones, we just aren't ready to do so, because handoff has not finished.
There was a problem hiding this comment.
Well it's not a "wrong" case for sure, but it is blocking, in IMO kinda the same way that "not enough redundancy for a service" could prevent it from being expunged.
I don't think I follow this. In the "not enough redundancy" case, we block the update from proceeding until redundancy is restored. But these messages are showing up even if everything is fine and normal, and they eventually go away once we're far enough along in the update. That doesn't seem blocking?
There was a problem hiding this comment.
I tried to update the text in 4ea1405, and to make this more precise (only matching "exactly" the current version).
If we feel this is still redundant with the "... remaining out-of-date zones" message, I can remove it, but I did want to communicate something in the report about why "even though you might want this zone to be updated, there is a good reason why we aren't doing it right now"
There was a problem hiding this comment.
I still don't really understand what it's trying to communicate. Maybe that's because I do think it's redundant? I think I would claim that at the point at which we've started to update zones, but before we're done with all of them, there's nothing to report about Nexus other than including it in the "remaining out-of-date zones" count. All of those zones are either "waiting to be expunged" or "waiting to be updated in place" (depending on their flavor).
Maybe there's something more specific to report once we get to the point of starting new Nexus zones and performing the handoff? But I'm not sure exactly what that would be, or how we'd see it. We only get to see the reports if a new blueprint is produced that makes some change (otherwise we throw it away), so I'd think we'd see reports for
- the blueprint that added the three new Nexus zones once all the non-Nexus zones are updated
- the blueprint that bumped the top-level Nexus generation once quiescing is done
- the blueprint (created by new Nexus) that expunges the old Nexus zones
but at none of those points are we really waiting for the old Nexus zones to be expunged, I think? Are there more blueprints coming out during the handoff beyond those three?
There was a problem hiding this comment.
I think I'd be most interested in this report if:
- All zones are updated, except for Nexus
- According to the planner input, the old Nexuses are still running
- They generate a blueprint saying "we aren't modifying anything, but there are still out-of-date zones"
In this case, I think it's important to identify that "Even though the system has not converged to an updated state, we are waiting for something -- this is what, and this is why".
Otherwise, we'd see: "blueprint with no changes, even though we know some zones are out-of-date"
Would it be more clear if I limited the scope of this report to "only emit this record if Nexus is the only out-of-date zone"?
There was a problem hiding this comment.
We only get to see the reports if a new blueprint is produced that makes some change (otherwise we throw it away)
do planning inputs only propagate if something else in the blueprint changes?
If that's true, then I think we could fail to communicate the ZoneUnsafeToShutdown reports too - these reports also identify why "we might not be making any changes to your blueprint, even though there is work to be done"
There was a problem hiding this comment.
I refactored this a bit in 9843b4a:
- This minimizes the window when we report about Nexus zones, until they're the only remaining zone to be updated
- This removes all the noise from the reconfigurator-cli tests
- I explicitly added this case to the test suite, where we try to perform an update from an old Nexus, and see that we're blocked. We also see the report in this case, which will identify why we aren't proceeding.
| .collect(); | ||
|
|
||
| // Define image sources: A (same as existing Nexus) and B (new) | ||
| let image_source_a = BlueprintZoneImageSource::InstallDataset; |
There was a problem hiding this comment.
This kinda strengthens my fear that deciding generation based on image sources is a little fraught; two Nexus zones on two different sleds both with the source InstallDataset might be completely unrelated. (Presumably they aren't in practice! And it'd be very surprising. But an image source of InstallDataset means we'd need to check the zone manifest in inventory to know whether the zone image sources actually match.)
There was a problem hiding this comment.
Just to confirm, are you suggesting I handle this case, or that the nexus_generation logic would be fraught regardless?
This is what we said about this in RFD 588:
When creating a new Nexus instance, the planner picks the nexus_generation as follows:
If the new instance’s image matches that of any existing Nexus instance, expunged or otherwise, use the same blueprint generation as that instance. (This should always match the current blueprint generation or the next one (during an upgrade).)
If the new instance’s image does not match that of any existing Nexus instance, choose one generation number higher than all existing instances. (This should always match the current blueprint generation plus one.)
The intuition here is that a new generation is defined by deploying a new Nexus image. If there’s a new Nexus image, we’ll need a handoff. If not, we don’t.
There was a problem hiding this comment.
Yeah, I kind of feel like you shouldn't be able to add a new one while any of them has source InstallDataset? I feel like we need to wait for image resolution to finish.
There was a problem hiding this comment.
Yeah, I think my concern is that the RFD bit you quoted talks in terms of "matching" images. InstallDataset alone does not give enough information to know whether it matches another sled's InstallDataset image (even of the same zone type). I think given we block zone updates until all zone image sources are known (or will, once #8921 lands), we should also gate doing anything related to Nexus handoff on the same thing.
There was a problem hiding this comment.
| * 0 zones waiting to be expunged: | ||
| * zone 466a9f29-62bf-4e63-924a-b9efdb86afec (nexus): zone gen (1) >= currently-running Nexus gen (1) |
There was a problem hiding this comment.
This seems confusing -- it sounds like we're trying to expunge these zones because they're at a newer generation?
It also seems like these messages come and go over the next several steps.
I think this is probably just a problem with the text, not the behavior. I'm not sure what it's trying to communicate though.
There was a problem hiding this comment.
I tried to update the text in 4ea1405.
There was a problem hiding this comment.
Also, made this report only appear when it's more relevant - see: 9843b4a
| * skipping noop zone image source check on sled d81c6a84-79b8-4958-ae41-ea46c9b19763: all 6 zones are already from artifacts | ||
| * 1 pending MGS update: | ||
| * model0:serial0: RotBootloader(PendingMgsUpdateRotBootloaderDetails { expected_stage0_version: ArtifactVersion("0.0.1"), expected_stage0_next_version: NoValidVersion }) | ||
| * only placed 0/2 desired nexus zones |
There was a problem hiding this comment.
What's going on with these? I only skimmed the test so far but I think at this point in the test we're expecting to be doing an update, but we can't proceed because there are pending MGS updates. Why is it saying it only placed 0/2 desired Nexus zones?
There was a problem hiding this comment.
Okay, after doing a little digging, I think this is kinda nasty, and probably should be considered related to #8921.
At this point in the test, there are three sleds:
- One has nexus @ InstallDataset
- One has nexus @ 1.0.0
- One has nexus @ 2.0.0
This should normally not be possible, but I believe is occurring because the three sleds have been manually edited. They also appear to all have been constructed with "nexus_generation = 1", which matches the blueprint-level "nexus_generation".
(This would be flagged by blippy as a corrupt blueprint - using the same generation for different images - but I don't think anyone is checking in this test)
This means we give really weird input to the planner here: We say that all three Nexuses are active, because they're all in-service sleds running the desired version of Nexus.
Without the #8921 changes, we will try to proceed placing discretionary zones, even though there is an InstallDataset present. As a part of this, we find the "currently in-charge Nexus image", which happens to find InstallDataset first.
Then, the planner tries to ensure that the "currently in-charge Nexus image" has sufficient redundancy. It sees that one sled has (Nexus, InstallDataset), and wants "two more Nexuses at this image" to reach the target. This is where the "2" in "0/2" comes from.
Next, the planner calls add_discretionary_zones, but the place_zone logic prevents zones of the same type from being placed on the same sled. Basically, all sleds report "I have a nexus already (ignoring the image source), so you can't place a new nexus here". This means we place zero new Nexuses.
This triggers the report.out_of_eligible_sleds call, which creates this message in the output.
I think the "pending MGS updates" logic would prevent zones from being updated, but this logic is happening in the context of "trying to restore redundancy, when the planner thinks we're running at reduced capacity" -- this is entirely in the domain of "zone add", not "zone update".
There was a problem hiding this comment.
To be super explicit: This "three-different-versions-at-once" behavior is also on main:
omicron/dev-tools/reconfigurator-cli/tests/output/cmds-mupdate-update-flow-stdout
Lines 1697 to 1841 in c3ea904
There was a problem hiding this comment.
Here's my consolation, after chatting with @sunshowers :
This is only happening because of the following config option:
> # Set the add_zones_with_mupdate_override planner config to ensure that zone
> # adds happen despite zone image sources not being Artifact.
> set planner-config --add-zones-with-mupdate-override true
planner config updated:
* add zones with mupdate override: false -> true
If this switch wasn't set, and we mupdated into this situation, we would normally refuse to do any add/update operations at all, while in this forcefully corrupt state.
There was a problem hiding this comment.
Hmm. @sunshowers, should we turn off this flag at this point in the test? (Is it weird that most of this test runs with that flag on?)
There was a problem hiding this comment.
+1 on this question; I know when I've had to update this test I haven't understood the changes I've made as well as I'd like. It's a pretty intricate test though so I'm not sure what to suggest for making it clearer.
| match current_nexus_generation { | ||
| Some(current) => write!( | ||
| f, | ||
| "zone gen ({zone_generation}) >= currently-running \ |
There was a problem hiding this comment.
This goes to the question I had about "what is a ZoneWaitingToExpunge". What is it we're trying to report here? It doesn't seem like there's anything wrong or blocking?
| .collect(); | ||
| let second_sled_id = sled_ids[1]; | ||
|
|
||
| // Use a different image source (artifact vs install dataset) |
There was a problem hiding this comment.
Actually, I wonder if this should produce an error if any Nexus zones have source "install dataset"? Because we don't know that the image is actually different.
There was a problem hiding this comment.
I'm going to defer to the conversation in #8936 (comment) - I'm down to restrict the usage of the install dataset, but doing this requires patching tests that rain changed in #8921.
Once that merges, I'm willing to add more explicit checks, but none of the "zone add" logic in the planner should execute anyway if there are any InstallDataset zones in use.
| hash: ArtifactHash([0x42; 32]), | ||
| }; | ||
|
|
||
| // Add another Nexus zone with different image source - should increment generation |
There was a problem hiding this comment.
Since test_nexus_generation_assignment_new_generation() already tested that if you pass a particular generation to sled_add_zone_nexus() then you get a zone with that generation, I wonder if we should just have this test call determine_nexus_generation() with both cases: one with the same image and one with a new one. I don't think we really need to add the zones and check that they got what we expected.
There was a problem hiding this comment.
I've removed this test out of necessity, with moving determine_nexus_generation out of the planner.
(Updated in 8d17221)
| /// - If any existing Nexus zone has the same image source, reuse its generation | ||
| /// - Otherwise, use the highest existing generation + 1 | ||
| /// - If no existing zones exist, return an error |
There was a problem hiding this comment.
I do feel like this probably belongs in the planner. There are no callers within the builder.
| .collect(); | ||
|
|
||
| // Define image sources: A (same as existing Nexus) and B (new) | ||
| let image_source_a = BlueprintZoneImageSource::InstallDataset; |
There was a problem hiding this comment.
Yeah, I kind of feel like you shouldn't be able to add a new one while any of them has source InstallDataset? I feel like we need to wait for image resolution to finish.
| let zones_currently_updating = | ||
| self.get_zones_not_yet_propagated_to_inventory(); | ||
| if !zones_currently_updating.is_empty() { | ||
| info!( |
There was a problem hiding this comment.
It's a little surprising there's no update to report for this. But maybe that happens elsewhere?
There was a problem hiding this comment.
I think this is a problem on main too. Filed #9047.
| // For Nexus, we need to confirm that the active generation has | ||
| // moved beyond this zone. |
There was a problem hiding this comment.
Calling this function for Nexus feels a little weird. For other zones, I think we're saying: "we're about to update this one zone, either by expunging it [and then subsequently adding one] or else replacing it. Can we do that now?" For Nexus, though, this will only return true after we've done the handoff to the new fleet.
I'm not sure it's worth reworking. Assuming not, I'd clarify this comment a bit:
| // For Nexus, we need to confirm that the active generation has | |
| // moved beyond this zone. | |
| // For Nexus, we're only ready to "update" this zone once control has been handed off to a newer generation of Nexus zones. (Once that happens, we're not really going to update this zone, just expunge it.) |
There was a problem hiding this comment.
Refactored this a bit, but done in 4beb420.
There was a problem hiding this comment.
I had a few last suggestions but this is looking good to me! Given how tricky this is, I'd like to get @jgallagher's +1 too.
| let (nexus_updateable_zones, non_nexus_updateable_zones): ( | ||
| Vec<_>, | ||
| Vec<_>, | ||
| ) = out_of_date_zones | ||
| .into_iter() | ||
| .filter(|(_, zone, _)| { | ||
| self.are_zones_ready_for_updates(mgs_updates) | ||
| && self.can_zone_be_shut_down_safely(&zone, &mut report) | ||
| }) | ||
| .partition(|(_, zone, _)| zone.zone_type.is_nexus()); |
There was a problem hiding this comment.
Take it or leave it: I'm wondering if we can make should_nexus_zone_be_expunged() more type-safe (so it can't panic on bad input) with something like (this is untested):
| let (nexus_updateable_zones, non_nexus_updateable_zones): ( | |
| Vec<_>, | |
| Vec<_>, | |
| ) = out_of_date_zones | |
| .into_iter() | |
| .filter(|(_, zone, _)| { | |
| self.are_zones_ready_for_updates(mgs_updates) | |
| && self.can_zone_be_shut_down_safely(&zone, &mut report) | |
| }) | |
| .partition(|(_, zone, _)| zone.zone_type.is_nexus()); | |
| let (nexus_updateable_zones, non_nexus_updateable_zones): ( | |
| Vec<_>, | |
| Vec<_>, | |
| ) = out_of_date_zones | |
| .into_iter() | |
| .filter(|(_, zone, _)| { | |
| self.are_zones_ready_for_updates(mgs_updates) | |
| && self.can_zone_be_shut_down_safely(&zone, &mut report) | |
| }) | |
| .map(|(sled_id, zone, image_source)| { | |
| let nexus_config = match &zone.z_type { | |
| blueprint_zone_type::Nexus(nexus_config) => nexus_config, | |
| _ => None, | |
| }; | |
| (sled_id, zone, image_source, nexus_config) | |
| }) | |
| .partition(|(_, _, _, nexus_config)| nexus_config.is_none()); |
Then we'll already have the nexus_config and can pass it to should_nexus_zone_be_expunged(). Not a big deal.
There was a problem hiding this comment.
I would be more on-board for this, but I think the type of nexus_config would be wrapped in an option here, which would need to be unwrapped anyway?
| // identify why it is not ready for update. | ||
| fn can_zone_be_updated( | ||
| // | ||
| // Precondition: zone must be a Nexus zone |
There was a problem hiding this comment.
| // Precondition: zone must be a Nexus zone | |
| // Precondition: zone must be a Nexus zone and be running an out-of-date image |
(trying to communicate that this shouldn't be used in any case other than "update")
There was a problem hiding this comment.
Updating phrasing in e1c9f06
| * skipping noop zone image source check on sled d81c6a84-79b8-4958-ae41-ea46c9b19763: all 6 zones are already from artifacts | ||
| * 1 pending MGS update: | ||
| * model0:serial0: RotBootloader(PendingMgsUpdateRotBootloaderDetails { expected_stage0_version: ArtifactVersion("0.0.1"), expected_stage0_next_version: NoValidVersion }) | ||
| * only placed 0/2 desired nexus zones |
There was a problem hiding this comment.
Hmm. @sunshowers, should we turn off this flag at this point in the test? (Is it weird that most of this test runs with that flag on?)
There was a problem hiding this comment.
This can be in a follow-on PR, but I feel like we should update this test to finish the update now. (If it's easy, it'd be nice to get in this PR -- it'd be a tidy confirmation that all is working.)
There was a problem hiding this comment.
Updated in e1c9f06 - looks like it works!
(though it looks like it also was updated in #9059 by @jgallagher )
| .unwrap(); | ||
| let image_source = BlueprintZoneImageSource::InstallDataset; | ||
|
|
||
| // Add first Nexus zone - should get generation 1 |
There was a problem hiding this comment.
Two nitpicky questions:
- Is it that it should get generation 1, or that it should get
builder.parent_blueprint().nexus_generationsince that's what we're passing in? - If the latter, do we still need this test now that the method isn't doing any logic to pick a generation and is just using whatever we pass in?
There was a problem hiding this comment.
I first cleaned up this test to clarify "the output matches the input", but I agree with you - I think it's a bit silly to have a test that the value is just a pass-through.
Removed in e1c9f06
|
|
||
| // We may need to bump the top-level Nexus generation number | ||
| // to update Nexus zones. | ||
| let nexus_generation_bump = self.do_plan_nexus_generation_update()?; |
There was a problem hiding this comment.
Should this be guarded by any of the "are prior steps still pending" checks like we have on updating zones above? My gut feeling is that in a normal update this wouldn't matter (we wouldn't be in a position for do_plan_nexus_generation_update() to do anything unless everything else is already done anyway). Maybe there are some weird cases where it might come up? Sled addition at a particularly unlucky time or something?
I think the answer is "no, don't guard it" - if we're in a position where we're ready to trigger a handoff, we should probably go ahead and do that even if the planner thinks there's now something else to do "earlier" in the update process, and the new Nexus can finish up whatever is left? But it seems like a weird enough case I wanted to ask.
There was a problem hiding this comment.
I believe that inside the body of do_plan_nexus_generation_update, we're already guarding against the stuff we care about, and validate that these things have propagated to inventory (e.g., new Nexuses booting) if we care about it.
I just scanned through do_plan_nexus_generation_update - it seems like any spot where we need to validate some property iterating over zones, it checks both the blueprint and the pending changes in the sled_editors.
I believe that, in the case where we're adding a zone and then performing this check:
- We'll be able to see the newly planned zone (e.g., looking that all zones are on a newer image)
- We won't see it in inventory, and could raise an error if we need to see it there (e.g., needing sufficient new Nexuses to actually be running)
| // | ||
| // This presumably includes the currently-executing Nexus where | ||
| // this logic is being considered. | ||
| let Some(current_gen) = self.lookup_current_nexus_generation()? else { |
There was a problem hiding this comment.
Should lookup_current_nexus_generation() return a Result<Generation, _> instead of a Result<Option<Generation>, _>? Or a different question: Can we get Ok(None) here from a well-formed parent blueprint?
There was a problem hiding this comment.
Sure, this actually simplifies the space of possible reported states if we treat it like an error. Done in aef9586
| panic!("did not converge after {MAX_PLANNING_ITERATIONS} iterations"); | ||
| } | ||
|
|
||
| struct BlueprintGenerator { |
There was a problem hiding this comment.
This work is already done so I wouldn't change it in this PR, but I don't love this - it feels like it's replicating what we built reconfigurator-cli to do for the tests it enables (set up a system, generate TUF repos, walk through various scenarios, etc., in a way that's easier to read and maintain than unit tests). Do you think we could replace these tests with reconfigurator-cli-based ones, or are they poking at things that would be hard to do there?
There was a problem hiding this comment.
I'd be happy to take this on as a follow-up to this PR. Candidly I didn't realize reconfigurator-cli was flexible enough to support blueprint editing like we do in tests, but I can take a look at that now.
| * skipping noop zone image source check on sled d81c6a84-79b8-4958-ae41-ea46c9b19763: all 6 zones are already from artifacts | ||
| * 1 pending MGS update: | ||
| * model0:serial0: RotBootloader(PendingMgsUpdateRotBootloaderDetails { expected_stage0_version: ArtifactVersion("0.0.1"), expected_stage0_next_version: NoValidVersion }) | ||
| * only placed 0/2 desired nexus zones |
There was a problem hiding this comment.
+1 on this question; I know when I've had to update this test I haven't understood the changes I've made as well as I'd like. It's a pretty intricate test though so I'm not sure what to suggest for making it clearer.
Blueprint Planner
Fixes #8843, #8854