Check hashes during repair and upon startup #1791
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
mkeeter
force-pushed
the
mkeeter/check-hashes
branch
2 times, most recently
from
01ac0e5 to
db67a9f
Compare
mkeeter
force-pushed
the
mkeeter/check-hashes
branch
from
db67a9f to
13b1577
Compare
This is a spin-off from #1791; it adds validation to `crucible-downstairs` but does not validate extent after receiving them during repair operations. --------- Co-authored-by: Alan Hanson <[email protected]>
mkeeter
force-pushed
the
mkeeter/check-hashes
branch
from
d54ad9a to
6b2b366
Compare
mkeeter
changed the title
crucible-downstairs and during repair
mkeeter
force-pushed
the
mkeeter/check-hashes
branch
from
6b2b366 to
4f84cf7
Compare
mkeeter
changed the title
| "validation falied for extent {number}: {err:?}" | ||
| ); | ||
| } | ||
| panic!("validation failed"); |
There was a problem hiding this comment.
I worry about this panic because we can't recover from it like we can the "check during repair" panic. Eventually the downstairs service will be in maintenance and the Upstairs can't do anything to fix this.
There was a problem hiding this comment.
Given we are "hunting" with this change, and not living with it forever, I think I like that we can't recover. I want things to stop as soon as we have detected a problem.
Looking for downstairs in maintenance (as well as core files) would help determine when we saw a problem.
I worry that perhaps we have a bad extent, but it gets "repaired" before we find it and we miss our window.
| } | ||
| }; | ||
| if let Err(e) = new_extent.validate() { | ||
| panic!("Failed to validate live-repair extent {eid}: {e:?}"); |
There was a problem hiding this comment.
I believe when we fail here, it will leave behind a copy_dir.
That should be handled "properly" when the downstairs restarts, as it should discard it and make a new one. We would lose the "bad" file, but the downstairs log should tell us what we need to know
| Err(CrucibleError::GenericError( | ||
| "`validate` is not implemented for Sqlite extent".to_owned(), | ||
| )) | ||
| // SQLite databases are always perfect and have no problems |
| extent_dir(dir, number).join(extent_file_name(number, ExtentType::Data)) | ||
| let e = extent_file_name(number, ExtentType::Data); | ||
|
|
||
| // XXX terrible hack: if someone has already provided a full directory tree |
There was a problem hiding this comment.
We need this so we can verify the incoming copy of an extent?
There was a problem hiding this comment.
Yeah, opening an extent takes the extent's root directory and number, then builds the extent file path internally. This is annoying if we want to open a specific raw file!
Since we're building a test image directly from this PR (and probably not merging it), I didn't bother to clean this up further.
| } | ||
| }; | ||
| if let Err(e) = new_extent.validate() { | ||
| panic!("Failed to validate live-repair extent {eid}: {e:?}"); |
There was a problem hiding this comment.
I believe when we fail here, it will leave behind a copy_dir.
That should be handled "properly" when the downstairs restarts, as it should discard it and make a new one. We would lose the "bad" file, but the downstairs log should tell us what we need to know
|
The CI failure is because checking downstairs on startup is slower, so there's a race between |
Updated the verify command to allow you to specify how many threads to use while verifying.
3 participants
This calls
Extent::validateafter repair and when opening an region, panicking on failures.See #1792 for details about extent validation.