[reconfigurator] Abandon orphaned sagas during execution

[karencfv]

Implements a way to abandon orphaned sagas during blueprint execution.

To be conservative, we only abandon sagas whose current_sec is an expunged (and ready-for-cleanup) Nexus zone in the target blueprint whose generation is strictly older than the oldest in-service Nexus generation. Any Nexus that isn't in the target blueprint is left untouched, since we can't confirm its state.

Closes: #10911

This should be merged AFTER release 22: I would prefer it if this commit spent some time on the dogfood rack before shipping it to customers.

[davepacheco]

How about also adding a saga in each state for a different SEC that we don't provide to sagas_abandon_orphans and then make sure that it doesn't touch those?

[davepacheco]

Hmm. In the event this ever goes wrong, it'll be important for us to know with confidence whether this operation happened and if it updated any sagas. We nearly have that here, except that if Nexus crashes, we might have no message despite having abandoned some sagas.

One option would be just make this an info-level message.

I'd also consider splitting the message at L151 into two cases:

• count == 0: info-level, "no orphaned sagas to abandon"
• count > 0: error-level, "abandoned orphaned sagas" -- we'd almost never expect to see this

Another option would be to split this into one datastore method to list any orphans and a separate one to abandon an orphan. Then we could issue a log message for each saga as we go to abandon it. Then we'd have the saga id in the logs, too. But this may be overkill.

[jgallagher]

Should we still bump this log up to info! in case we crash after abandoning sagas but before logging the counts?

[karencfv]

oops! missed that bit, thanks for catching it

[davepacheco]

I think you also want a test that passes 0 SECs.

[davepacheco]

I think it's worth a sentence here explaining that this is safe even if we're currently executing an old blueprint because (1) a zone can never become un-expunged, and (2) the generation never goes backwards. The combination means that no other Nexus can ever assign this saga.

[davepacheco]

Suggested change

	//! Handle sagas from expunged Nexus zones
	//! Re-assign or abandon sagas from expunged Nexus zones

(take it or leave it -- I just thought "handle" felt unnecessarily vague)

[karencfv]

I left it vague thinking that in the future if we add some other functionality for handling sagas we wouldn't have to change the description again. Happy to change if you feel strongly about being more specific though!

[davepacheco]

Hmm. Why not do what find_expunged_same_generation() does, which is essentially to find expunged Nexus zones from generations older than the current Nexus zone's generation?

Thinking out loud about how this would be different.

1. Normal operation. The code here will only find zones at generation N, so the min is N, then it returns zones with generation less than N. This is the same as the alternative logic, which uses the current zone's generation (N) and returns zones with generation less than that.
2. During update, after deploying generation N + 1 zones, but before triggering handoff. This code will find zones at generation N and N + 1, but the min is still N, and this zone is still N, so the behavior for both approaches is the same.
3. During update, after deploying generation N + 1 zones, immediately after handoff. This code will find zones at generation N and N + 1, but the min is now N, so it will abandon sagas assigned to generations less than N. The alternative logic would abandon sagas at generations less than N + 1. This is slightly different. I think the alternative is slightly better, but it doesn't really matter because we'll quickly be in the next case.
4. During update, after handoff, after expunging the old (generation N) zones. This code finds only the generation N + 1 zones, so the min is N + 1, so it abandons sagas from generations less than that. The two approaches are equivalent again. (This is really the same as case 1.)

I can see the sense in which this seems more conservative. I'm not sure it makes much difference. But it does seem pretty confusing to me that we have parallel functions that do very similar things (find_expunged_same_generation and find_expunged_older_generation) that use different approaches to determine "current generation".

[karencfv]

During update, after deploying generation N + 1 zones, immediately after handoff. This code will find zones at generation N and N + 1, but the min is now N, so it will abandon sagas assigned to generations less than N. The alternative logic would abandon sagas at generations less than N + 1. This is slightly different. I think the alternative is slightly better, but it doesn't really matter because we'll quickly be in the next case.

This is the specific case I was thinking of. i would like to handle the case where for some reason (a bug?) We end up in this state for a "longer than expected" amount of time. In this scenario, we can give sagas that could potentially still finish gracefully a chance to do so (even if there is the slimmest chance of this happening). If they really should be abandoned, then they will be the next time a blueprint is executed. No harm done either way. According to my understanding, abandoning a saga should be "last resort", and choosing "stale generations" this way is more conservative.

In regards to the naming potentially misleading people (find_expunged_same_generation and find_expunged_older_generation) about how the functions work, I could name this one something completely different to avoid confusion. What do you think?

[jgallagher]

This is the specific case I was thinking of. i would like to handle the case where for some reason (a bug?) We end up in this state for a "longer than expected" amount of time. In this scenario, we can give sagas that could potentially still finish gracefully a chance to do so (even if there is the slimmest chance of this happening).

I'm not sure I understand the case we're worried about here. If we're after handoff, the Nexuses at generation N are no longer able to acquire db connections, so can't be executing sagas, right? That means there's no way they could still finish, regardless of how much time there is between "handoff to generation N+1" and "generation N+1 expunges generation N".

Or in other words: If we're executing this function as Nexus generation N+1, and there's some Nexus at generation N still running and executing sagas, we're already in a (horrible) undefined state that almost certainly means we'll have to wipe the rack to recover.

[karencfv]

Is there a possibility of a window where Nexus (N) is still in quiescing state, but we have bumped the Nexus generation to N+1?

Or in other words: If we're executing this function as Nexus generation N+1, and there's some Nexus at generation N still running and executing sagas, we're already in a (horrible) undefined state that almost certainly means we'll have to wipe the rack to recover.

Perhaps? I'm mostly just paranoid about accidentally abandoning a saga that shouldn't have been. I don't see any harm in being overly conservative. Or is there?

[karencfv]

Is there a possibility of a window where Nexus (N) is still in quiescing state, but we have bumped the Nexus generation to N+1?

So, right after the Nexus generation is bumped to N+1 sagas from a Nexus (N) could still be draining no? Then we wouldn't want to abandon those. Or am I reading this wrong?

[jgallagher]

Is there a possibility of a window where Nexus (N) is still in quiescing state, but we have bumped the Nexus generation to N+1?

So, right after the Nexus generation is bumped to N+1 sagas from a Nexus (N) could still be draining no? Then we wouldn't want to abandon those. Or am I reading this wrong?

I'm not sure what you mean by "the Nexus generation is bumped". IIUC, there are multiple distinct events here:

1. Nexus zones are added to the blueprint at generation N+1: This happens while Nexus generation N is still running. The N+1 generation zones start but remain inert until handoff happens. Any Nexus executing this task is still at generation N, even though there are gen N+1 Nexus zones in the blueprint and deployed.
2. Handoff begins: All the generation N Nexus zones start quiescing - they no longer allow new sagas to start, but still allow db connections. Any Nexus executing is still at generation N.
3. Handoff proceeds: Db connections are no longer allowed, but we're waiting for all the existing connections to be closed. Any Nexus executing is still at generation N.
4. Handoff happens: all the generation N Nexuses are no longer allowed to access the DB, and the generation N+1 Nexus can. They perform schema migration. During this step there are (effectively) no Nexuses executing anything meaningful.
5. Handoff done: the Nexus generation N+1 set are now running. They'll expunge the generation N Nexuses pretty soon. This is the first point where the N+1 generation Nexuses could execute this code, and at this point it's fine to abandon sagas assigned to the expunged Nexuses from generation N.

Edit: https://rfd.shared.oxide.computer/rfd/0588#_basic_sequence_of_nexus_handoff_during_upgrade describes this more correctly and in more detail 😅

[karencfv]

Ah! aha. Ok, I understand now. Thanks for the link as well!

That said, from what I understand abandoning a saga is pretty catastrophic if done incorrectly. In the presence of a bug or some unexpected behaviour, would it not be helpful to be as conservative as we can be?

From my understanding, the only difference with this approach is that we may take an additional blueprint execution round to abandon a saga. Is there any downside to being conservative?

[jgallagher]

Sorry, this got quite long. I feel like I'm rambling around a topic I can't 100% pin down, so I erred on the side of too much thinking out loud / too many details. Happy to chat about this live at some point if it would help.

---

From my understanding, the only difference with this approach is that we may take an additional blueprint execution round to abandon a saga. Is there any downside to being conservative?

I don't think the current implementation will produce incorrect behavior - you're right that we should eventually abandon sagas (although it may require multiple rounds - I'm not sure if we expunge all the old Nexus zones at once or go one at a time. either way this is pretty fast generally). But I don't think I'd phrase it as "conservative", exactly; the downside of the current implementation is that it's confusing, and implies either a misunderstanding of something fundamental about the way Nexus operates or that other parts of the system are incorrect for not being conservative the same way.

By "something fundamental about the way Nexus operates", I mean that Nexus is extremely tightly tied to the database schema version that it expects, and attempting to operate Nexus against any different schema version is more or less guaranteed to fail in spectacular and disastrous ways. (The one time this happened on a customer rack was due to a malformed partial mupdate, and the damage was bad enough that we wiped that rack and reinitialized it.) So when I read this comment, which is explaining why the current implementation chooses to be conservative:

/// To be conservative, we only abandon sagas whose `current_sec` is an expunged
/// (and ready-for-cleanup) Nexus zone in the target blueprint whose generation
/// is strictly older than the oldest generation of running Nexus zones.
///
/// For a brief moment during an update, after deploying a new-generation Nexus,
/// and immediately after handoff, there will be zones at generation N and N + 1
/// (although the zones at generation N must be fully quiesced and not using the
/// database). By retrieving all in-service Nexus zones and choosing the oldest
/// generation that we find, we handle the case where for some reason we end up
/// in this state for a "longer than expected" amount of time. In this scenario,
/// we give any saga that could still finish gracefully a chance to do so.

I come away confused:

• "oldest generation of running Nexus zones" implies there can be multiple generations of Nexus running at the same time. This is technically accurate, but only in a very specific way (the Nexus processes of two generations can be running at once, but in that case, either the old or the new zones are holding themselves inert, which is not what I think of when I see "running").
• "we handle the case where for some reason we end up in this state for a 'longer than expected' amount of time" - Which state from the handoff process is this referring to? If we're in any state other than "handoff is done and Nexus generation N+1 is running", this code won't do anything, so I'm not sure what we're handling.
• "we give any saga that could still finish gracefully a chance to do so" - This is already built into the handoff procedure, specifically the "quiesce sagas" state. If Nexus generation N+1 is executing this code, even if there are still Nexuses at generation N in the blueprint, we must be past that step and therefore there is no saga that could still finish from generation N.

By "other parts of the system are incorrect", I mean that there are other parts of Reconfigurator (both planning and execution) that already rely on the fact that the generation of the currently-operating Nexus must be the only generation in service. There may be more but that includes at least these two:

• DNS blueprint execution reads the currently-active Nexus generation in order to only include that generation's Nexus set in external DNS
• The planner determines whether to expunge a Nexus zone by checking whether the currently-running Nexus generation is the latest; if it finds Nexus zones at some other generation, it will expunge them.

Both of those are incorrect if there's any possible way the conservative implementation here could lead to a saga completing that wouldn't have otherwise, right?

Given all of that, when I try to figure out how I'd reword this comment to explain why we're being conservative in this way, it seems harder to do that than to not be conservative, and instead to be consistent with the rest of the system.

[karencfv]

implies either a misunderstanding of something fundamental about the way Nexus operates or that other parts of the system are incorrect for not being conservative the same way.
...
By "other parts of the system are incorrect", I mean that there are other parts of Reconfigurator (both planning and execution) that already rely on the fact that the generation of the currently-operating Nexus must be the only generation in service.

Yeah, OK, I can see how making this specific case more conservative could create confusion about the implementations in the rest of the codebase around which generation is in service.

Thanks for taking the time to write out your reasoning!

I'll change the code to use the current Nexus generation instead

[karencfv]

Thanks for the review @davepacheco! I think I've addressed all of your comments. Let me know what you think.

[davepacheco]

Thanks! My feedback here is all pretty minor.

Given the risk here (abandoning the wrong saga), it'd be nice to get another set of eyes on it.

[davepacheco]

Suggested change

	/// For a brief moment during an update, after deploying a new-generation Nexus,
	/// and immediately after handoff, there will effectively be zones at generation
	/// N and N + 1. By retrieving all in-service Nexus zones and choosing the
	/// oldest generation, we handle the case where for some reason we end up in
	/// this state for a "longer than expected" amount of time. In this scenario, we
	/// give any saga that could still finish gracefully a chance to do so. If they
	/// really should be abandoned, then they will be the next time a blueprint is
	/// executed.
	/// For a brief moment during an update, after deploying a new-generation Nexus,
	/// and immediately after handoff, there will be zones at generation
	/// N and N + 1 (although the zones at generation N must be fully quiesced and
	/// not using the database). By retrieving all in-service Nexus zones and choosing the
	/// oldest generation that we find, we handle the case where for some reason we end up in
	/// this state for a "longer than expected" amount of time. In this scenario, we
	/// give any saga that could still finish gracefully a chance to do so. If they
	/// really should be abandoned, then they will be once the planner creates a blueprint
	/// that expunges the older zones and that blueprint is executed.

As I add these clarifying notes, I'm not sure how any such saga could be running or could finish gracefully unless something is seriously wrong with either handoff or re-assignment. I don't mind being conservative here, but I also don't want somebody in the future to read this comment and think that this behavior is necessary because of some condition we expect could reasonably happen.

[jgallagher]

Is the bulk of this comment accurately describing a real state we're in if we change the intro to something like this? It's not just during handoff that we can see Nexuses at two generations, right? That starts well before handoff.

Toward the end of an update, Nexus (at generation N) deploys new Nexus zones running the new version and marks them as generation N + 1. Until we move into the quiesce-and-handoff process, the Nexuses at generation N are still running, and must not consider their own sagas as orphans. This function must only abandon sagas that are assigned to expunged-and-ready-for-cleanup Nexus zones whose generation is at most N - 1 (i.e., 1 below the minimum generation of any in-service Nexus zone in the blueprint).

[davepacheco]

@jgallagher It's true that there are two generations from some period before handoff until some period after. And your snippet there is correct. I think the comment is written that way because the only window where the implementation here differs from the (totally different) implementation of (similarly-seeming semantics) find_expunged_same_generation() is in the period after handoff and before the old zones are expunged.

[jgallagher]

Is the bulk of this comment accurately describing a real state we're in if we change the intro to something like this? It's not just during handoff that we can see Nexuses at two generations, right? That starts well before handoff.

Toward the end of an update, Nexus (at generation N) deploys new Nexus zones running the new version and marks them as generation N + 1. Until we move into the quiesce-and-handoff process, the Nexuses at generation N are still running, and must not consider their own sagas as orphans. This function must only abandon sagas that are assigned to expunged-and-ready-for-cleanup Nexus zones whose generation is at most N - 1 (i.e., 1 below the minimum generation of any in-service Nexus zone in the blueprint).

[jgallagher]

Should we still bump this log up to info! in case we crash after abandoning sagas but before logging the counts?

[jgallagher]

This is the specific case I was thinking of. i would like to handle the case where for some reason (a bug?) We end up in this state for a "longer than expected" amount of time. In this scenario, we can give sagas that could potentially still finish gracefully a chance to do so (even if there is the slimmest chance of this happening).

I'm not sure I understand the case we're worried about here. If we're after handoff, the Nexuses at generation N are no longer able to acquire db connections, so can't be executing sagas, right? That means there's no way they could still finish, regardless of how much time there is between "handoff to generation N+1" and "generation N+1 expunges generation N".

Or in other words: If we're executing this function as Nexus generation N+1, and there's some Nexus at generation N still running and executing sagas, we're already in a (horrible) undefined state that almost certainly means we'll have to wipe the rack to recover.

[karencfv]

Thanks both for taking a look @davepacheco @jgallagher, I've addressed all the comments. Let me know what you think!

[karencfv]

oops! missed that bit, thanks for catching it

[karencfv]

Is there a possibility of a window where Nexus (N) is still in quiescing state, but we have bumped the Nexus generation to N+1?

Or in other words: If we're executing this function as Nexus generation N+1, and there's some Nexus at generation N still running and executing sagas, we're already in a (horrible) undefined state that almost certainly means we'll have to wipe the rack to recover.

Perhaps? I'm mostly just paranoid about accidentally abandoning a saga that shouldn't have been. I don't see any harm in being overly conservative. Or is there?

[karencfv]

Thanks for the thorough reviews! I changed the older generation logic and added an additional test. This should hopefully be ready to go

[karencfv]

implies either a misunderstanding of something fundamental about the way Nexus operates or that other parts of the system are incorrect for not being conservative the same way.
...
By "other parts of the system are incorrect", I mean that there are other parts of Reconfigurator (both planning and execution) that already rely on the fact that the generation of the currently-operating Nexus must be the only generation in service.

Yeah, OK, I can see how making this specific case more conservative could create confusion about the implementations in the rest of the codebase around which generation is in service.

Thanks for taking the time to write out your reasoning!

I'll change the code to use the current Nexus generation instead

[jgallagher]

Thanks, LGTM. Just one nit and a possible minor cleanup.

[jgallagher]

Suggested change

	/// expunged and no in-service Nexus remains in its generation, any saga still
	/// expunged and the active Nexuses are of a higher generation, any saga still

[jgallagher]

This function looks almost identical to find_expunged_same_generation; I wonder if we should combine them? Untested but something like

enum FindExpungedNexusFilter {
    SameGeneration,
    OlderGeneration,
}

fn find_expunged_nexus_ids(
    blueprint: &Blueprint,
    nexus_id: SecId,
    reason: BlueprintExpungedZoneAccessReason,
    filter: FindExpungedNexusFilter,
) -> Result<Vec<SecId>, Error> {
    let nexus_zone_id = OmicronZoneUuid::from_untyped_uuid(nexus_id.0);
    let active_nexus_generation =
        blueprint.find_generation_for_self(nexus_zone_id)?;
    Ok(blueprint
        .expunged_nexus_zones_ready_for_cleanup(reason)
        .filter_map(|(_sled_id, zone_config, nexus_config)| {
            let include = match filter {
                FindExpungedNexusFilter::SameGeneration => {
                    nexus_config.nexus_generation == active_nexus_generation
                }
                FindExpungedNexusFilter::OlderGeneration => {
                    nexus_config.nexus_generation < active_nexus_generation
                }
            };
            include.then_some(zone_config.id)
        })
        .map(|id| SecId(id.into_untyped_uuid()))
        .collect())
}

I'm on the fence over whether this is an improvement. I think it is? But don't feel strongly. (Maybe reason should be filter.reason() instead of a separate argument?)