[reconfigurator] Abandon orphaned sagas during execution - #10990
[reconfigurator] Abandon orphaned sagas during execution#10990karencfv wants to merge 8 commits into
Conversation
| /// remaining with the set of configuration. | ||
| NexusExternalConfig, | ||
|
|
||
| /// Sagas assigneed to any expunged Nexus whose generation is older than the |
There was a problem hiding this comment.
| /// Sagas assigneed to any expunged Nexus whose generation is older than the | |
| /// Sagas assigned to any expunged Nexus whose generation is older than the |
| let conn = self.pool_connection_authorized(opctx).await?; | ||
|
|
||
| use nexus_db_schema::schema::saga::dsl; | ||
| diesel::update( |
There was a problem hiding this comment.
There's nothing to prevent this query from updating an unbounded number of rows. I guess given that we expect this to hit almost nothing almost all the time, and only a handful of sagas ever, maybe it's fine? But usually I try to construct interfaces like this in a way that requires pagination (e.g., list a bounded number of sagas in this state first, then update those sagas).
Update: Ah, I see this is modeled directly after sagas_reassign_sec(), where there's a comment mentioning this risk and why we're punting on it. Maybe just copy that here, too?
| // Populate the database with a few different sagas: each SEC gets one | ||
| // saga in each of the running, unwinding, and done states. | ||
| // | ||
| // Then we'll pass *all* of the SECs through sagas_abandon_orphans() |
There was a problem hiding this comment.
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?
| /// 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. | ||
| pub(crate) async fn abandon_orphan_sagas( |
There was a problem hiding this comment.
We use the phrase "oldest in-service Nexus generation" a few times (in the PR description and several times in this file). What does this mean? Isn't there only one in-service Nexus generation at a time? I think this is just the "current" Nexus generation.
Edit: I see where this phrasing comes from now. This is saying: there may be in-service Nexus zones from multiple generations, and we look at the oldest ones. I found it confusing because at any given time, there is really only one Nexus generation active (the handoff process ensures that), even though there may be zones running from multiple generations.
| // Find ids of stale expunged Nexus zones | ||
| let stale_sec_ids = find_expunged_older_generation(log, blueprint); | ||
|
|
||
| debug!( |
There was a problem hiding this comment.
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.
| "orphan: current_sec Nexus is expunged and unreassignable \ | ||
| (older than all in-service generations)" |
There was a problem hiding this comment.
| "orphan: current_sec Nexus is expunged and unreassignable \ | |
| (older than all in-service generations)" | |
| "orphan: current_sec is expunged and too old for this saga to be re-assigned" |
(feel free to reword, but the current wording makes it sound like the Nexus is unreassignable, which isn't right, and it uses the "older than all in-service generations" phrasing that I think doesn't make sense)
| // saga in each of the running, unwinding, and done states. | ||
| // | ||
| // Then we'll pass *all* of the SECs through sagas_abandon_orphans() | ||
| // and check exactly which sagas were changed by this. |
There was a problem hiding this comment.
I think you also want a test that passes 0 SECs.
| /// (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. |
There was a problem hiding this comment.
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.
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| //! Re-assign sagas from expunged Nexus zones | ||
| //! Handle sagas from expunged Nexus zones |
There was a problem hiding this comment.
| //! 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)
There was a problem hiding this comment.
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!
| // We chose the oldest of the live generations for a few reasons. During | ||
| // Nexus handover there is a possibility that there will be more than one | ||
| // generation of Nexuses in-service, and the target blueprint could become | ||
| // stale before this code executes. We are conservative and only take sagas | ||
| // that are strictly older than this generation and could never be | ||
| // reassigned. | ||
| let Some(oldest_live_generation) = blueprint | ||
| .in_service_nexus_zones() | ||
| .map(|(_, _, nexus)| nexus.nexus_generation) | ||
| .min() | ||
| else { | ||
| return vec![]; | ||
| }; | ||
| debug!( | ||
| log, | ||
| "abandon orphan sagas: retrieved oldest in-service Nexus generation"; | ||
| "oldest_in_service_generation" => %oldest_live_generation, | ||
| ); |
There was a problem hiding this comment.
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.
- 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.
- 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.
- 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.
- 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".
There was a problem hiding this comment.
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?
2 participants
Implements a way to abandon orphaned sagas during blueprint execution.
To be conservative, we only abandon sagas whose
current_secis 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.