Implement pre-migration framework #5434
Conversation
Stebalien
requested review from
Kubuxu,
magik6k and
whyrusleeping
as code owners
Stebalien
force-pushed
the
feat/pre-migrate
branch
from
5e0a35a to
a93a186
Compare
| migration := &migration{ | ||
| upgrade: upgrade.Migration, | ||
| preMigrations: upgrade.PreMigrations, | ||
| cache: nv10.NewMemMigrationCache(), |
There was a problem hiding this comment.
a bit messy but I guess it is fine
There was a problem hiding this comment.
Yeah, we should probably move it elsewhere, but this works for now. Really, it was that or copy it.
This can significantly speedup state migrations.
This was really confusing.
Otherwise, we may end up referencing blocks we never wrote to disk.
This was causing a lot of warnings about not implementing View.
Stebalien
force-pushed
the
feat/pre-migrate
branch
from
5aaf6af to
56054b0
Compare
chain/stmgr/forks.go
Outdated
| When: 120, | ||
| NotAfter: 60, | ||
| }, { | ||
| PreMigration: PreUpgradeActorsV3, | ||
| When: 30, | ||
| NotAfter: 20, |
There was a problem hiding this comment.
Probably should get those epochs from build/
There was a problem hiding this comment.
Maybe? I mean, these are just saying "do the pre-migration N epochs before the upgrade height". I'm not sure if being able to configure these in the build is all that useful.
chain/stmgr/forks.go
Outdated
| // TODO: tune this. | ||
| config := nv10.Config{MaxWorkers: 1} |
There was a problem hiding this comment.
Do we have good/any idea on what this should be?
There was a problem hiding this comment.
I'm setting the pre-migration to half the CPUs, and the migration to all the CPUs. I'll also try at 2x CPUs just to see if it matters.
There was a problem hiding this comment.
Can you take a look at the current values?
| for _, head := range change { | ||
| for len(schedule) > 0 { | ||
| op := &schedule[0] | ||
| if head.Val.Height() < op.after { |
There was a problem hiding this comment.
We could add some confidence here to make reverts less likely to have impact here (if they even matter)
|
|
||
| // Finally, when the head changes, see if there's anything we need to do. | ||
| for change := range sm.cs.SubHeadChanges(ctx) { | ||
| for _, head := range change { |
There was a problem hiding this comment.
Should we also drop the migration caches here after say upgradeEpoch+finality to not keep a bunch of stuff in memory?
There was a problem hiding this comment.
Hm. Yes, we should. I'll nil out the "ops" as we pass them.
There was a problem hiding this comment.
Well, actually, it's not quite that simple. I'll see if there's a better way to do this.
chain/stmgr/forks.go
Outdated
| // When specifies that this pre-migration should be started at most When epochs before the upgrade. | ||
| When abi.ChainEpoch | ||
|
|
||
| // NotAfter specifies that this pre-migration should not be started NotAfter epochs before |
There was a problem hiding this comment.
It's a bit more than that, right -- it also means that an in-process migration will be cancelled at this epoch?
There was a problem hiding this comment.
Yeah, I'm going to to fix that. Really, we need three points in time:
- Start after.
- Don't start after.
- Stop at.
There was a problem hiding this comment.
Could you take a look now?
Co-authored-by: Aayush Rajasekaran <[email protected]>
There are now three times: 1. StartWithin: start within X epochs of the upgrade. 2. DontStartWithin: don't start within X epochs of the upgrade. 3. StopWithin: stop within X epochs of the upgrade.
This PR adds:
some number of epochs before the upgrade happens.
Finally, this PR wires up pre-migrations for the actors V3 upgrade.
fixes #5415
TODO: