[reconfigurator] remove the add-zones-with-mupdate-override planner config#10462
Conversation
Created using spr 1.3.6-beta.1
jgallagher
left a comment
There was a problem hiding this comment.
LGTM, just a couple very minor questions.
| /// Set the planner's configuration. | ||
| /// | ||
| /// Returns the previous value. | ||
| pub fn set_planner_config( |
There was a problem hiding this comment.
Maybe worth keeping this method if we're keeping PlannerConfig around? I don't have strong feelings either way about that, but if we think we might add fields to it in the future, we'd end up putting this method right back, I assume.
There was a problem hiding this comment.
Yeah let's keep it for symmetry with get_planner_config.
| writeln!(indented, "planner config:")?; | ||
| write!(indented, "{}", planner_config.display())?; | ||
| // When there are fields in `PlannerConfigDiff`, this is where their | ||
| // display would go. |
There was a problem hiding this comment.
Would this be slightly more futureproof if we kept planner_config in the destructure above, and added a
let PlannerConfig {} = planner_config;here?
There was a problem hiding this comment.
Ah so the thing is that what ends up here is PlannerConfigDiff which is a daft-generated type, not PlannerConfig. There is a PhantomData field within the diff struct that's kept private so it can't be destructured.
Created using spr 1.3.6-beta.1
While attempting to resolve #10420, we realized that the add-zones-with-mupdate-override config simply cannot result in a blueprint with coherent semantics in some cases.
Concretely, consider the situation where:
Artifact)If this planner config is set, we would attempt to place a Nexus zone on one of the sleds. What version should it have?
Artifactimage source or the new one.This planner config was not used in practice (documentation for our support team has no mention of it). I've left the infrastructure to add new planner configs in place, though -- happy to remove that as well if y'all feel like that makes sense. (Previously daft would not handle empty structs properly -- as part of this PR I've fixed that.)
This has no impact on racks that have never have a target release set -- for those racks, the image source is always
InstallDatasetso this is moot (and pre-existing code handles the "target release generation is one" case already).Fixes #10420.