metamorphic: remote storage fixes for crossversion #4760
Conversation
The metamorphic test uses in-mem remote storage, which doesn't work when starting with an initial state (as in the crossversion tests). Note that there is code to save the remote storage contents to disk when the store is in-memory, but they are not read back when used as initial state. This commit changes to using FS-based remote storage in `shared` and `external` subdirs inside the data dir. Note that the store itself can still be in-memory; the data gets saved automatically with the store. We improve the FS-based Storage implementation to sync data and list objects. We can now allow simulating crashes in the metamorphic test when shared storage is enabled.
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sumeerbhola)
-- commits line 26 at r2:
I didn't follow this. Is this referring to background errors like from compactions which are attempting to write to shared storage before the creator ID has been set? Would it be simpler if we have CreateOnShared != none to first open just the object provider and assign a creator ID to the object provider? Bc in the context of the metamorphic test we don't have the bootstrapping problem to allocate a creator ID
metamorphic/ops.go line 1887 at r3 (raw file):
// // Note that the number is not based on the seed, in case we run using the // same seed that was used in a previous run with the same store.
if we made the name based on the generation time seed, wouldn't the current state of the prng not be the same because the previous run's different operations? Or is this referring to previous runs of the same ops? In which case can't we reset the test state to isolate identical re-runs?
metamorphic/test.go line 198 at r2 (raw file):
}) if err != nil { fmt.Printf("Open(): %v\n", err)
nit: debug detritus?
metamorphic/test.go line 207 at r2 (raw file):
}) if err != nil { fmt.Printf("SetCreatorID(): %v\n", err)
nit: debug detritus?
There was a problem hiding this comment.
TFTR!
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jbowens and @sumeerbhola)
Previously, jbowens (Jackson Owens) wrote…
I didn't follow this. Is this referring to background errors like from compactions which are attempting to write to shared storage before the creator ID has been set? Would it be simpler if we have
CreateOnShared != noneto first open just the object provider and assign a creator ID to the object provider? Bc in the context of the metamorphic test we don't have the bootstrapping problem to allocate a creator ID
Right, flushes or compactions fail. That would work.. but I figure better to test the external facing APIs. I'll try and see how it looks.
metamorphic/ops.go line 1887 at r3 (raw file):
Previously, jbowens (Jackson Owens) wrote…
if we made the name based on the generation time seed, wouldn't the current state of the prng not be the same because the previous run's different operations? Or is this referring to previous runs of the same ops? In which case can't we reset the test state to isolate identical re-runs?
I was thinking of the case where we manually set the seed and first run on e.g. 25.2 and then run again on master using initial-state.
Perhaps a cleaner alternative is to check the list of existing files.
metamorphic/test.go line 198 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
nit: debug detritus?
I think it's helpful to keep
08e312d to
c817ef8
Compare
If the metamorphic test has `CreateOnShared` set to something other than "none" and we start with an initial store which did not have remote storage configured, there can be background errors right after opening the store, before we get a chance to call `SetCreatorID()`. The metamorphic test fails on these background errors. To fix this, we always open with `CreateOnSharedNone` first, and if necessary reopen after setting the creator ID.
External object names can collide with existing objects when starting with an initial state. This change adds a random unique number to the filenames.
c817ef8 to
5ca45a8
Compare
|
Upon backporting, it looks that the first commit breaks the |
These commits fix various aspects around external and shared storage in the context of the crossversion test, where we start with an initial state.
Informs #4732
metamorphic: use FS-based remote storage
The metamorphic test uses in-mem remote storage, which doesn't work
when starting with an initial state (as in the crossversion tests).
Note that there is code to save the remote storage contents to disk
when the store is in-memory, but they are not read back when used as
initial state.
This commit changes to using FS-based remote storage in
sharedand
externalsubdirs inside the data dir. Note that the store itselfcan still be in-memory; the data gets saved automatically with the
store.
We improve the FS-based Storage implementation to sync data and list
objects. We can now allow simulating crashes in the metamorphic test
when shared storage is enabled.
metamorphic: support CreateOnShared on existing store
If the metamorphic test has
CreateOnSharedset to something otherthan "none" and we start with an initial store which did not have
remote storage configured, there can be background errors right after
opening the store, before we get a chance to call
SetCreatorID().The metamorphic test fails on these background errors.
To fix this, we always open with
CreateOnSharedNonefirst, and ifnecessary reopen after setting the creator ID.
metamorphic: add random number to external object names
External object names can collide with existing objects when starting
with an initial state. This change adds a random unique number to the
filenames.