[reconfigurator] Executor internal cleanup around iterators #7722
Conversation
| // We expect to only be called with expunged zones that are ready | ||
| // for cleanup; skip any with a different disposition. | ||
| if !config.disposition.is_ready_for_cleanup() { | ||
| return None; |
There was a problem hiding this comment.
This was the offender Sean pointed out. I think it's now reasonable to just remove this entirely: the private _impl function is fine to assume it's only called with zones that are ready for cleanup, and it's obvious that that's true in its single caller (other than tests).
| ) -> Result<(), Vec<anyhow::Error>> { | ||
| deploy_nodes_impl( | ||
| opctx, | ||
| blueprint.all_omicron_zones(BlueprintZoneDisposition::any), |
There was a problem hiding this comment.
I know you're just moving this - but should this disposition be is_in_service? Why is any acceptable here?
There was a problem hiding this comment.
I know you're just moving this - but should this disposition be
is_in_service? Why isanyacceptable here?
John kept this in because the keeper code was doing it's own filtering, and the goal was not to change any behavior. But looking at this execution code again, it does seem wrong. There's no need to send updates to expunged zones. I was worried that the keeper config would be in correct, but this doesn't change that config, it only changes which keepers we send the config to.
There was a problem hiding this comment.
It's not obvious to me that this set of zones is only used for which sleds to talk to - maybe it is? E.g., it looks like the IPs of these zones end up sent to everyone - maybe that's also wrong if they're expunged, but I know there was a lot of nitpicky stuff about how much can be changed at once for clickhouse. Do you mind if I file an issue and let you or Karen take a look at changing this?
There was a problem hiding this comment.
Yes, please file an issue and I'll take care of it. Thanks!
There was a problem hiding this comment.
but I know there was a lot of nitpicky stuff about how much can be changed at once for clickhouse.
This is definitely true, but I believe only for the keeper's raft config, which looks like it doesn't change. I will have to review all that code, and possibly add another test. None of this is trivial, so agree it warrants a deeper look.
jgallagher
force-pushed
the
john/execution-iter-cleanup
branch
from
a80b39f to
21953e0
Compare
Builds on #7713, and is followup from #7713 (comment). In #7652 I changed all the executor substeps to take iterators instead of
&BTreeMapreferences that no longer existed, but that introduced a weird split where the top-level caller had to filter the blueprint down to just the items that the inner functions expected. @smklein pointed out one place where the inner code was being extra defensive, which was just more confusing than anything.This PR removes that split: the top-level executor now always passes a full
&Blueprintdown, and the inner modules are responsible for doing their own filtering as appropriate. To easy testing, I kept the versions that take an iterator of already-filtered items as private*_implfunctions that the new functions-that-take-a-full-Blueprintthemselves call too.