Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: stmgr: add env to disable premigrations #10283

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

travisperson
Copy link
Contributor

Setting the environment variable LOTUS_DISABLE_PRE_MIGRATIONS=1 will discard premigrations for all upgrade.

Related Issues

#10048

Proposed Changes

  • Add environment variable LOTUS_DISABLE_PRE_MIGRATIONS which when set to 1 stops pre-migrations from being executed.
  • Migrations are still scheduled and part of the statement managers' migration table.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

log.Warn("SKIPPING pre-migration", "height", height)
return
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original thought was to just set the PreMigration array for each migration to empty during the construction of the state manager when this environment var is set. This would lead to nothing getting scheduled.

If we had a way to print the current schedule / next scheduled task I think this would still be the correct way to go. However, I don't believe we have that functionality. So instead I'm opting to just exit pre-migrations early.

This will ensure that a log message is created around the time the pre-migration was supposed to run stating that it was skipped. I think this will make debugging things a bit easier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a pretty good way to go about it, honestly.

@travisperson
Copy link
Contributor Author

Is there any documentation in this lotus here that also should be updated as part of this PR? Otherwise I think the migration documentation I want to write will be the location of this new environment variable.

@travisperson travisperson marked this pull request as ready for review February 16, 2023 17:36
@travisperson travisperson requested a review from a team as a code owner February 16, 2023 17:36
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

This would be relatively easy to test -- we can spin up custom upgrade schedules in itests (eg.

Migration: filcns.UpgradeActorsV10,
). I think we could have a test that simply panics on pre-migration, and succeeds on the actual migration -- we succeed if we get to the next network version.

I don't think it's critical to have this test, but worth doing.

log.Warn("SKIPPING pre-migration", "height", height)
return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a pretty good way to go about it, honestly.

Setting the environment variable `LOTUS_DISABLE_PRE_MIGRATIONS=1` will
discard premigrations for all upgrade.
@travisperson travisperson force-pushed the feat/disable-pre-migrations branch from 0e18e96 to 2dcaddf Compare February 17, 2023 23:12
@travisperson travisperson merged commit bf2ac13 into master Feb 21, 2023
@travisperson travisperson deleted the feat/disable-pre-migrations branch February 21, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants