planner: do not update any zones until we're sure SPs are updated

[davepacheco]

There's a check in #8269 that we've finished all SP updates before moving on to zone updates. But it isn't quite right because it doesn't account for cases where we're unable to proceed with an SP update for some reason. I hope that fixing this will be easy after #8284 (because we'll avoid proceeding if this step returns any "events we're waiting for").

[karencfv]

But it isn't quite right because it doesn't account for cases where we're unable to proceed with an SP update for some reason.

Just to make sure I'm understanding this correctly. If we're unable to proceed with an SP update for some reason, the planner removes it from the list of pending MGS updates and carries on with the next SP component update. We want to have a mechanism to keep track of these SP updates that were unable to complete, and hold updating zones until all of these failed SP updates have completed successfully. Is this accurate?

[jgallagher]

Yeah, but I think I would phrase it slightly differently: we want to hold off updating zones until all the things that could be updated via a PendingMgsUpdate (bootloader, RoT, SP, host OS) are confirmed to be running the new version.

I'm not sure how we should handle the case where a sled is in the blueprint but its SP is not in inventory. The safest thing is to treat that sled as "not yet updated" and hold zone updates, I think?

[davepacheco]

I believe that's correct. I was imagining something like this:

• Within the "MGS updates" planning report, we create a new struct (say DeviceUpdateSkipped) with the baseboard id and component name we're unable to update and an enum of reasons (like "MissingFromInventory" or "NoMatchingArtifact")
• We pass the in-progress "MGS updates" planning report down to the try_make_update* family of functions.
• In these functions, where they currently return None and log a warning (like here for example), we instead call some method on the planning report that appends one of these DeviceUpdateSkipped structs to the report. (Alternatively, instead of returning Option<PendingMgsUpdate>, these return Result<PendingMgsUpdate, CantUpdateReason> where CantUpdateError is that enum above. Then we use these to assemble the DeviceUpdateSkipped at the top level. This might be nicer.)
• Have do_plan() call some method on the "MGS updates" planning report to tell if we're ready to move on to zone updates. That method would basically check if there were no pending updates and there were none of these new DeviceUpdateSkipped records.

I'm not attached to this approach. It just seemed like a natural way to take advantage of the fact that we already have code paths that identify these situations and use them to both report the problem (so that we can communicate why an update is stuck) and also prevent subsequent actions.

[karencfv]

Thanks for confirming! I'll try out that approach and see how it all fits together.

[jgallagher]

I'm not sure how we should handle the case where a sled is in the blueprint but its SP is not in inventory. The safest thing is to treat that sled as "not yet updated" and hold zone updates, I think?

This might be wrong. We talked about a similar case in the context of mupdate override, and decided there that a sled missing from inventory blocking updates is probably overly conservative. Ideally we'd record in the blueprint that all the SPs we know about have been updated, I think? So that if a sled were missing from inventory we'd know whether it had been updated already or still needed to be.

I'm not sure what to do for this particular case. In this step of @davepacheco's outline above:

• In these functions, where they currently return None and log a warning (like here for example), we instead call some method on the planning report that appends one of these DeviceUpdateSkipped structs to the report. (Alternatively, instead of returning Option<PendingMgsUpdate>, these return Result<PendingMgsUpdate, CantUpdateReason> where CantUpdateError is that enum above. Then we use these to assemble the DeviceUpdateSkipped at the top level. This might be nicer.)

I think the natural thing to do is return a CantUpdateReason::NotInInventory, which means a sled missing from inventory would block updates (even if we've actually already updated that sled). Maybe worth a brief discussion live?

[davepacheco]

@jgallagher The problem you're describing makes sense. I agree we could latch something in the blueprint to fix that. Intuitively, the risk calculation feels different to me for this case, but I'm not sure that's right. For the mupdate override case, the argument was something like: it's not worth blocking all update activity just to verify that the host OS matches a known artifact, which we think should always be true in practice anyway. That is, the risk of something going wrong wasn't that high and the check felt maybe not worth the cost. Here, though, we're trying to avoid deploying new zones when the host OS isn't yet new enough to talk to them. This is both more likely to happen (this could totally happen during an update if, say, a sled is offline because we just bounced its SP but we haven't updated its host OS yet -- we wouldn't want to just move on to zone updates) and more impactful (it seems pretty likely to result in an API mismatch).

That's the argument for why it seems more reasonable to me to wait for something to return to inventory before proceeding past zone updates. The counter-argument would be: either way, we're saying update activity is blocked on all hosts being present. Either that's an acceptable thing or not? In that case maybe I'd phrase this as: it's an okay constraint for now, and a future enhancement could allow updates to proceed even when hosts were offline if we've previously validated the constraint we care about.

I'll add this to the agenda for the update call today.

[karencfv]

On another note, it appears to me that with this we're only accounting for host OS phase 1 but if host OS phase 2 is skipped for some reason. The planner would still be blind to this. I haven't come up with an approach to handle this yet, but something worth thinking about

[jgallagher]

On another note, it appears to me that with this we're only accounting for host OS phase 1 but if host OS phase 2 is skipped for some reason. The planner would still be blind to this. I haven't come up with an approach to handle this yet, but something worth thinking about

Can you expand on this? The two phases are closely related and the planner isn't really blind to them, I don't think:

• When planning a host OS update, the planner updates the desired phase 2 in that sled's config and records a PendingMgsUpdate for the associated phase 1 of the same sled.
• During execution, Nexus won't start the phase 1 MGS update until it confirms that sled-agent has already written the phase 2.
• After performing the phase 1 update, we don't consider it complete until the sled has rebooted and we've confirmed in inventory that we've booted from the expected image (which means both phase 1 and phase 2 are matched and complete).

[karencfv]

After performing the phase 1 update, we don't consider it complete until the sled has rebooted and we've confirmed in inventory that we've booted from the expected image (which means both phase 1 and phase 2 are matched and complete)

Gotcha, this was the part I was uncertain about. Thanks!

[karencfv]

I'll add this to the agenda for the update call today.

We discussed this issue during the update sync. We came to the conclusion that for now we will treat the case of a missing sled in inventory will block an update. It is the safest option for now, and also adds the least amount of complexity to the blueprints. If we feel like we have the need to relax this over time, we can revisit this decision.