trigger inventory collection after blueprint execution #5130
Conversation
| @@ -71,6 +78,10 @@ impl BackgroundTask for BlueprintExecutor { | |||
| ) | |||
| .await; | |||
|
|
|||
| // Trigger anybody waiting for this to finish. | |||
| self.count = self.count + 1; | |||
| self.tx.send(self.count); | |||
There was a problem hiding this comment.
I'll take "watch is hard to use correctly for $200, Alex"; this should probably be send_replace or send_modify so it doesn't spuriously fail if there are no subscribers.
There was a problem hiding this comment.
Actually, if we used send_modify could we drop self.count altogether?
self.tx.send_modify(|count| *count += 1)There was a problem hiding this comment.
Fixed in 70dfed3
| config.inventory.period_secs, | ||
| Box::new(collector), | ||
| opctx.child(BTreeMap::new()), | ||
| vec![Box::new(rx_blueprint_exec)], |
There was a problem hiding this comment.
I think we currently have blueprint exec set to every 60s, and inventory collection set to every 600s. Is this going to 10x the inventory collection frequency?
There was a problem hiding this comment.
If a target is set, yes. You could vie the 600s after this as "if there's nothing going on, collect at least this often".
I imagine we'll want to revisit these triggers and timeouts when we get to fully automating this but I think this is okay for now.
There was a problem hiding this comment.
Even as we're doing this by hand, we'll go from 600s to 60s as soon as we set a target, but it will never go back, right? Once we've set a target there will always be a target.
There was a problem hiding this comment.
I guess what I'm kinda wishing for here is "only trigger the collection if blueprint realization actually changed something", but that might be quite difficult.
There was a problem hiding this comment.
Agreed, but yeah, that seems hard to know. Certainly if we get almost any errors, we have to assume something may have changed. We could have each module in the execution part return information about whether it might have done anything that would require re-inventorying. Concretely, we'd need sled agent to tell us whether it made any changes when we did PUT /omicron-zones. This seems like a potentially fine optimization, but also just an optimization (and bugs in this area would be really annoying). I think it makes sense to wait until this becomes more of a problem. What do you think?
There was a problem hiding this comment.
Waiting seems fine. I just have vague concerns around "we thought inventory is relatively expensive, so we put it on a pretty long timer, but now we're speeding that up by a factor of 10".
I guess while we're still in manual land, we could disable the current target after we get to the desired state, and that would let us turn this back down, since the executor bails out for a disabled target the same way it does for no target.
|
This is great. Thanks @davepacheco. While it still may be worthy it to implement #5058, it is now much less of a priority! |
|
Auto-merge failed because of #5027. Rerunning that job... |
This came up in the context of #5111 and here in particular:
I figured that more generally, any time we execute a blueprint, successfully or otherwise, we may have made some changes and we ought to re-collect inventory.