Repository navigation
Conversation
c4208a9 to
67e9722
Compare
67e9722 to
918d2d3
Compare
leftwo
changed the title
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // The old downstairs is no longer part of the volume. Give it a | ||
| // new region, so when it is used as a replacement it is like the | ||
| // new region the product would provide. | ||
| recreate_downstairs(&dsc_client, old_ds as u32, &log).await?; |
There was a problem hiding this comment.
🟡 (optional) An operator running crutest replace can have the wrong region wiped. This happens when --replacement is not dsc client ds_total, or when the targets come from --vcr. The call recreate_downstairs(&dsc_client, old_ds as u32, &log) assumes targets[i] is dsc cid i, and nothing checks that. If the order differs, the call stops and recreates a different downstairs, which may still be live in the volume. Fix: resolve each target to its dsc cid by matching dsc_get_port(cid) against the target port, or check at startup that they match and bail otherwise. This covers the replacement address and VCR-supplied targets.
Why this was flagged
replace_workload treats the targets index as the dsc client id at main.rs:3036. Targets built from dsc (main.rs:830-843) cover cids 0..sv_count*3-1. The replacement is whatever the user passes (main.rs:1326). With --vcr, the targets come from vcr.targets() (main.rs:779) in VCR order. Two inputs break the mapping. One is a dsc with 5 regions and --replacement set to cid 4's port; the old_ds=3 iteration then recreates cid 3 and not the downstairs that was actually swapped out. The other is a VCR whose target order differs from the dsc cids; there recreate_downstairs stops, wipes and restarts a downstairs that is still an active member of the volume (main.rs:2545-2585), so the run fails or loses data. On the base branch, replace_workload never stopped or recreated any region, so these inputs could not wipe a live downstairs. The CI script happens to line up (4 regions, replacement 8840 = cid 3), but nothing validates the mapping.
Verification: Nit. main.rs:3036 calls recreate_downstairs(&dsc_client, old_ds as u32, &log), where old_ds is an index into targets. Nothing checks that targets[old_ds] really is dsc cid old_ds. Unlike ReplaceBeforeActive and ReplaceReconcile, the Replace branch has no region count check. With --vcr order, a live downstairs gets wiped. Every in-repo caller passes the matching cid.
After each live repair in the replace workload, give the downstairs that was replaced a new, empty region before it is used as the next replacement. This makes it look like the new region the control-plane would provide, instead of reusing a region that was already part of the volume and would come with been_active status we don't want. Add a dsc endpoint instead of targets to the replace workload in the live repair buildomat job.. If the replace task ends, stop the IO task instead of running IO until the count is reached.
918d2d3 to
2afab7e
Compare
In the crutest replace workload after each live repair, give the
downstairs that was replaced a new empty region before it is used as
the next replacement. This matches what the control plane does
and avoids re-use of an existing region which comes with been_active
set (which we don't want)
This uses the dsc recreate from #2073 and the crutest recreate_downstairs
from #2074.
The replace workload now requires --dsc, so update the buildomat job
to switch to that instead of providing targets.
If the replace task ends, the IO task now stops instead of running IO
until the count is reached, so a replace failure is reported right away.