fix: Use separate snapshots for sequencer and validator workloads #116
Conversation
🟡 Heimdall Review Status
|
tobidae-cb
force-pushed
the
fix-snapshot-bug
branch
from
fb6504b to
be8056f
Compare
runner/service.go
Outdated
| return nil, nil, errors.Wrap(err, "failed to create sequencer test directory") | ||
| } | ||
|
|
||
| err = os.Rename(initialSnapshotPath, sequencerDataDirOverride) |
There was a problem hiding this comment.
I think this will only allow running the test once which may be confusing for reuse_existing. Maybe we should rename that and add an option that allows copying instead of moving here? Or use force_clean to decide?
There was a problem hiding this comment.
What's happening here is that after copying the initial snapshot for the validator run, we want to just "move" the original snapshot data to the sequencer data directory. That way we don't have to do 2 copies (for sequencer, validator) and simply do 1 copy with the final action being a move. Saves time with this approach.
The directory setup doesn't have any bearing on the tests that run. It just does the setup the first time. That being said, I could manage it outside of the runTest module for clarity
There was a problem hiding this comment.
just to clarify, after moving from the initial snapshot directory with reuse_existing, we wouldn't be able to rerun the test again because now the initial snapshot directory doesn't exist right? Or I might be misunderstanding.
Definitely agree there are speed benefits, so we should support moving rather than copying, but I don't think we should require it.
There was a problem hiding this comment.
After moving the initial snapshot directory, all future tests including the current test will use the new directory. You're right that the initial snapshot dir no longer exists but that's fine because the move happens in the first run and then reuses the updated directory for the immediate test and future tests.
If after all the tests are done you decide to run adhoc tests, in this case yeah the initial directory won't exist unless you manually copied it back but this is setup for automated use
runner/service.go
Outdated
| isSnapshot = true // dataDirOverride is only set when using snapshots | ||
| s.log.Info("Using pre-configured datadir", "path", dataDirPath, "role", role) | ||
| } else { | ||
| isSnapshot = snapshot != nil && snapshot.Command != "" |
There was a problem hiding this comment.
I wonder if we could clean this up by moving the snapshot setup logic to it's own struct/file. The logic right now is separated between setupInternalDirectories and setupDataDirs. Previously, setupDataDirs just called setupInternalDirectories for the sequencer and the validator, but now it looks like they're doing similar things and we could just combine the logic. For example, they both call CopyFromInitialSnapshot.
runner/service.go
Outdated
|
|
||
| // tracks persistent test directories for reuse_existing snapshots | ||
| // key: nodeType, value: map["sequencer"|"validator"] -> directory path | ||
| persistentTestDirs map[string]map[string]string |
There was a problem hiding this comment.
we could move this to the manager struct also (or move to the snapshot manager that already exists)
Description
There's currently a bug where both the sequencing and validation runs use the same snapshot which makes the metrics gathered by the validation run look ultra fast. We want to split out the snapshot used for validation into separate runs to ensure we have the right data
Testing