Repository navigation
Conversation
|
I've started on removing the "replace before reconcile" ability for this PR. |
With 2/3 activation possible, we can no longer allow a downstairs to be replaced before we have gone through reconciliation. Added a new crutest test for 2/3 activation. Removed or changed a few tests that attempted to replace a downstairs before activation.
|
I've removed the tests and ability to replace a downstairs before activation, which I believe is what we want now that we have 2/3 activation. I also added a specific crutest test for 2/3 activation. I'll most likely build on that test and roll something into the nightly test set to do a 2/3 activation test. |
| } | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
The new VolumeInfo struct and this function we can use in a bunch of other places and make better tests out of it. This can replace some bad bash scripts that do the same thing, but poorly.
|
Before this goes in (or before propolis/omicron point to it) we must have oxidecomputer/omicron#8528 sorted. |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
upstairs/src/downstairs.rs— When the third Downstairs is offline, min-quorum activation can never finish if repair takes longer than one reconnect cycle (about 10-20s). Every failed reconnect by the non-participating client callsDownstairs::reinitialize. That aborts reconciliation whenever any other client is inReconcile(downstairs.rs:735-745). Fix: abort only when the client being reinitialized is one ofreconcile.participating, or is itself inReconcile, matching the participant-only check already added inon_reconciliation_ack.Why this was flagged
Setup: GoActive with DS0 and DS1 in WaitQuorum and DS2 unreachable. After
NEGOTIATION_DELAY,on_min_quorum(upstairs.rs:2142-2165) starts reconciliation for DS0 and DS1 only and marks DS2 Faulted. DS2's IO task keeps failing to connect (ClientRunResult::ConnectionFailed/ConnectionTimeout, client.rs:2387/2405). It retries everyCLIENT_RECONNECT_DELAY(10s). Each failure arrives asClientAction::TaskStoppedand goes toon_client_task_stopped, thenDownstairs::reinitialize(DS2). The special case at downstairs.rs:735-745 sees DS0/DS1 inReconcileand callsabort_reconciliation, which halts both participants withFailedReconcile. They renegotiate, the 500ms timer fires again, and the cycle repeats. Any reconciliation longer than DS2's retry interval never completes, so activation hangs. This is exactly the one-Downstairs-down case RFD 542 targets. On base, activation simply waited for all three. The participant-only check added inon_reconciliation_ack(downstairs.rs:1954) does not help, because this abort path never consultsparticipating.Verification:
Downstairs::reinitialize(downstairs.rs:735-745) callsabort_reconciliation(up_state)if ANY client is inNegotiationState::Reconcile. It does not check whether the client being reinitialized is inreconcile.participating. On the base branch reconciliation needed all three clients, so a client that couldn't connect never overlapped with a running reconciliation.
| ); | ||
| self.downstairs.clients[unready_ds] | ||
| .set_connection_mode_faulted(); |
There was a problem hiding this comment.
🔴 The Upstairs can panic if the min-quorum timer fires, or reconciliation finishes, while the third Downstairs is briefly in Stopping after a failed negotiation. Here set_connection_mode_faulted panics with "not connecting". On the Ok(false) path and at reconciliation end, Downstairs::on_reconciliation_done panics with "invalid state in on_reconciliation_done" (downstairs.rs:2098). Fix: both places must tolerate a non-participating client in any state. Skip or defer Stopping clients so they come back on the live-repair path, at upstairs.rs:2165 and downstairs.rs:2097-2101. [also at: upstairs/src/downstairs.rs:2101 - A third Downstairs that reconnects during min-quorum reconciliation can now panic the Upstairs at the end of reconciliation.]
Why this was flagged
During GoActive, a Downstairs that fails negotiation goes through abort_negotiation (upstairs.rs:1816) into DsStateData::Stopping(NegotiationFailed). It stays there until ClientAction::TaskStopped reinitializes it. If NEGOTIATION_DELAY expires in that window while the other two clients are in WaitQuorum, on_min_quorum runs. With Ok(true) it calls set_connection_mode_faulted on the unready client, and client.rs:783 panics because the client is not Connecting. With Ok(false) it calls on_reconciliation_done(false), which reaches the s => panic! arm at downstairs.rs:2098. The same panic fires when min-quorum reconciliation finishes while the third client is in Stopping. On the base branch reconciliation needed all three clients in WaitQuorum, and nothing called these functions on a stopping client. The result is that the Upstairs process aborts during activation.
Verification: Verification: upstairs.rs:1816-1831 calls abort_negotiation, and halt_io_task (client.rs:263-269) moves the client to DsStateData::Stopping(r). If MinQuorumReconciliation fires first, client.rs:783-787 panics with "not connecting", or downstairs.rs:2098 panics. On the base, on_reconciliation_done just called set_active on all clients.
| assert!(did_work); | ||
| c.set_active(); | ||
| } | ||
| DsState::Connecting { .. } => c.set_connection_mode_faulted(), |
There was a problem hiding this comment.
🔴 An active volume whose three Downstairs all need live-repair, with matching extents, now never brings any Downstairs back to Active, so guest IO keeps failing. The new catch-all arm treats LiveRepairReady clients like the excluded min-quorum client. set_connection_mode_faulted is a no-op for them, so all three stay in LiveRepairReady. Fix: in on_reconciliation_done, move Connecting { LiveRepairReady, Faulted } clients to Active when this is the all-three-need-repair path (as the base did), while the excluded third client of a min-quorum activation is still only marked faulted.
Why this was flagged
The trigger needs the Upstairs to be Active, all three clients faulted, and all three reconnected into LiveRepairReady with identical, clean extent metadata. ensure_downstairs_consistency (upstairs.rs:955-965) sees LiveRepairStart::AllNeedRepair. reconcile_from_live_repair_ready returns false ("All extents match"), so it calls on_reconciliation_done(false). On the base, Downstairs::on_reconciliation_done called set_active() on every client. Now each client matches the DsState::Connecting { .. } arm at downstairs.rs:2097. That call only re-sets mode Faulted and does not touch a non-WaitQuorum state, so nothing changes. Every later apply repeats the same no-op cycle. active_client_count() stays 0, so reads and writes fail with "too many inactive clients" (upstairs.rs:1463-1471, 1590-1595). test_downstairs_three_live_repair only covers the variant where reconciliation has work.
Verification: upstairs.rs:961-964 calls self.on_reconciliation_done(false) when start_reconciliation returns false with "All extents match" (downstairs.rs:1082-1084). In the new Downstairs::on_reconciliation_done (downstairs.rs:2082-2101), LiveRepairReady clients fall into DsState::Connecting { .. } => c.set_connection_mode_faulted(). On the base, on_reconciliation_done called c.set_active() unconditionally on every client.
| if !matches!(self.state, UpstairsState::Active) { | ||
| warn!(self.log, "Replace before active request"); | ||
| done.send_result(Err( | ||
| CrucibleError::ReplaceRequestInvalid( | ||
| "replace not allowed before upstairs is active" | ||
| .to_string(), | ||
| ), | ||
| )); | ||
| } else { | ||
| let r = self.downstairs.replace(id, old, new, &self.state); | ||
| done.send_result(r); | ||
| } |
There was a problem hiding this comment.
🔴 Running instances whose disk has finished its read-only-parent scrub can no longer have a read-only-parent (snapshot) target replaced. The base branch returned Started. The new guard at upstairs.rs:1218 rejects every non-Active Upstairs before Downstairs::replace checks whether it owns old. The scrub deactivates the ROP for good at volume.rs:699, leaving it Initializing, so Volume::replace_downstairs hits ? on the ROP and returns ReplaceRequestInvalid. Fix: decide ownership first (Missing/CompletedAlready) and reject only an Upstairs mid-negotiation (GoActive). A deactivated, owning Upstairs should just swap target_addr in ConnectionMode::New.
Why this was flagged
The trigger is a disk with a read_only_parent, such as one created from an image or snapshot. When scrub() finishes it calls read_only_parent.deactivate() (volume.rs:699), and that Upstairs settles in UpstairsState::Initializing (upstairs.rs:694-697). Nothing ever reactivates it. A replacement for a ROP target then arrives through Volume::target_replace (volume.rs:1965, called from pantry.rs:475 or any embedder). VolumeInner::replace_downstairs (volume.rs:1092-1134) gets Missing from the active sub_volumes. It then calls rop.replace_downstairs(...).await?. The new branch at upstairs.rs:1218-1225 answers Err(ReplaceRequestInvalid("replace not allowed before upstairs is active")) without running Downstairs::replace (downstairs.rs:2648). The whole target_replace therefore fails, and retries fail the same way for the life of the volume. On the base branch, Downstairs::replace ran and returned Ok(ReplaceResult::Started). Nothing downstream of the ? in volume.rs:1099/1118 catches it.
Verification: Volume::scrub calls read_only_parent.deactivate() when it finishes (volume.rs:699). Nothing in volume.rs activates the ROP again. The new guard rejects it before self.downstairs.replace gets to check whether it owns old. The ? passes that error up (volume.rs:1117-1118). Base Downstairs::replace returned Ok(ReplaceResult::Started). So base succeeded where the change now returns an error.
Updated a test that needed a slightly different starting point since things on main have moved on since this PR started.
A new tests to verify that if we activate with 2/3, the third downstairs does not get marked as been active.
This plumbs in the BeenActive status of a downstairs into the min-quorum decision making process. To avoid having a replacement downstairs count as one of two in the min-quorum activation, we added the BeenActive status for each downstairs. Only BeenActive downstairs can count toward min-quorum. This also adds a bunch of tests around this new feature.
|
I'm going to fix #2072 before I merge more into |
(This is a revival of #1733, rebased onto
main)This implements RFD 542 and closes #1690.
Here's the quick version:
WaitQuorum, we schedule an event to fire afterNEGOTIATION_DELAY(currently 500 ms)I'm opening this as a draft because I want to see how the CI tests go. The PR includes integration tests for common and uncommon orderings of events, but it's hard to hit every possible path due to specific timing requirements; I'm very open to suggestions for other tests.
Before merging, we also need to figure out oxidecomputer/omicron#8528
This PR also needs the stack of issues ending with #2078 to land, and we must merge with those, then retest the changes here.