[29/n] [reconfigurator] separate out noop image source decision making #8596
Conversation
Created using spr 1.3.6-beta.1
sunshowers
changed the title
| version, | ||
| hash, | ||
| } => { | ||
| return NoopConvertZoneInfo { |
There was a problem hiding this comment.
Small style nit - maybe move this closure to its own function? I sometimes have a hard time a return at this point doesn't return from the method as a whole.
| // A sled is eligible for conversion if the mupdate_override_id is None | ||
| // | ||
| // The code is structured in this manner because the planner's mupdate | ||
| // override step requires access to the sled's zones, and is responsible for | ||
| // clearing this field if it also clears the mupdate override in the | ||
| // blueprint. |
There was a problem hiding this comment.
Just for my own understanding: We're going to return this to do_plan. Currently it hands it off to do_plan_noop_image_source(), which has to inspect mupdate_override_id to know whether to consider the contents of zones. But in the future, do_plan will first give this to the mupdate override clear step, which may set this to None, right?
I'm trying to figure out if there's a way to get rid of the Maybe here, so do_plan_noop_image_source() doesn't have to make decisions on its own (and because I think it'd be easier to understand if we only had "eligible" and "ineligible"). This might be nonsense, but based on my understanding above, could we:
- make this module return the sled as ineligible with
NoopConvertSledIneligibleReason::MupdateOverride(MupdateOverrideUuid)if there's a mupdate override in place - if there's no mupdate override in place, return
NoopConvertSledStatus::Eligible(basically the same as what we have, but dropping all theMaybes) - in the future, have the mupdate override clear step convert from
NoopConvertSledIneligibleReason::MupdateOverride(MupdateOverrideUuid)toNoopConvertSledStatus::Eligibleif it's able to clear the mupdate override?
There was a problem hiding this comment.
Yeah, the "maybe" is a bit of a wart -- let me see if I can make this easier to understand.
There was a problem hiding this comment.
Simplified it down to eligible and ineligible, but retained the zone map with the MupdateOverride case to enable easier state conversions.
There was a problem hiding this comment.
Yeah, makes sense. Looks great, thanks!
Created using spr 1.3.6-beta.1
sunshowers
deleted the
sunshowers/spr/reconfigurator-separate-out-noop-image-source-decision-making
branch
While working on #8572, we realized that we need to put in a condition saying that if any zones can't be switched over to Artifact, we must not clear the mupdate override field. This effectively requires information about eligibility to be available in two spots: while updating the mupdate override, and while doing these noop conversions.
In order to do that more easily, this PR builds up a decision tree by sled.