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: snapshot: remove existing chain #11032

Merged
merged 4 commits into from
Jul 20, 2023
Merged

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Jun 30, 2023

Related Issues

Closes: #10958

Proposed Changes

Add a flag (remove-existing-chain) to remove the existing chain data on import of a snapshot, reducing manual operation needed when node operators want to prune their current chain.

Additional Info

Some open questions:

  • Do we only want the removal of the chain data to be possible when importing a snapshot?
    • Do we want it to default to true when importing a snapshot?
  • Do we want to clear the chain data dirs, or delete it?
    • Are there any edge-cases where deleting the chain data dir is bad before importing a snapshot?

Testing:

  • Tested on a node without SplitStore
  • Tested on a node with SplitStore

Docs (todo):

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

Remove existing chain on a snapshot-import

log info instead of print

log info instead of print

Remove defer lock

Remove defer lock, and explicitly close the lockedRepo instead
@rjan90 rjan90 requested a review from a team as a code owner June 30, 2023 13:33
@rjan90
Copy link
Contributor Author

rjan90 commented Jun 30, 2023

Do we want it to be the default to true when importing a snapshot?

With regards to this:

  • Defaulting it to true would help a lot of new users which may not be familiar with the need to remove the existing chain data before importing a Snapshot. But if we want to go in this direction, I think we need to only make it possible when importing a snapshot.

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Overall looks good, has it been tested?

@rjan90
Copy link
Contributor Author

rjan90 commented Jun 30, 2023

Overall looks good, has it been tested?

It has been tested on a node running with SplitStore, and on a node running without SplitStore and it removed the chain data successfully on both before starting the import of the snapshot.

Two question I have for you @ZenGround0 is:

  • What is the difference between clearSplitstoreDir and deleteSplitstoreKeys functions in the lotus-shed splitstore clear command you created are (I borrowed code from that)? I currently use deleteSplitstoreDir as it seemed most applicable for what we wanted to achieve here, but would like to understand the nuance.
  • Are there any edge-cases where deleting the SplitStore dir is wrong vs just its content. Afaik the folder is re-created if its not there, which should be fine during reimporting of snapshots.

Remove unused clearSplitStoreDir function
Extract removeExistingChain to seperate func
Check error
@rjan90
Copy link
Contributor Author

rjan90 commented Jul 4, 2023

Updated the PR with some changes based on feedback/reviews:

  • Removed unused clearSplitStoreDir function ()
  • Originally I had this:

    lotus/cmd/lotus/daemon.go

    Lines 313 to 317 in 5201744

    cfg, err := lockedRepo.Config()
    if err != nil {
    lockedRepo.Close()
    return xerrors.Errorf("error getting config: %w", err)
    }

    But Andy shared that If .Config() panics, close won't happen here. So I extracted the removeExistingChain function, and used defer to ensure that the lockedRepo is closed, even if a panic occurs. (dedfa14)
  • Fixed errCheck

@rjan90
Copy link
Contributor Author

rjan90 commented Jul 4, 2023

Testing on a SplitStore-node:

[Chainstore]
  # type: bool
  # env var: LOTUS_CHAINSTORE_ENABLESPLITSTORE
  EnableSplitstore = true

See that there is data in the chain-folder:

/mnt/lotuschain/.lotus/datastore/chain$ ls
000000.vlog  000010.vlog  000020.vlog  000030.vlog  000040.vlog  000050.vlog  000060.vlog  000070.vlog  000080.vlog  000090.vlog  000320.sst  000453.sst  000522.sst  000555.sst  000605.sst  000615.sst  000629.sst  000639.sst
-----

See that there is data in the splitstore-folder:

/mnt/lotuschain/.lotus/datastore/splitstore$ ls
hot.badger  markset.badger

Run lotus daemon --import-snapshot /home/3005040_2023_07_04T08_00_00Z.car.zst --remove-existing-chain=true --halt-after-import

....
2023-07-04T15:24:02.887+0200	INFO	main	lotus/daemon.go:774	removing splitstore directory...
2023-07-04T15:24:03.709+0200	INFO	main	lotus/daemon.go:787	removing chain directory:/mnt/lotuschain/.lotus/datastore/chain
2023-07-04T15:24:12.949+0200	INFO	main	lotus/daemon.go:794	chain and splitstore data have been removed
......
2023-07-04T15:24:13.058+0200	INFO	main	lotus/daemon.go:543	importing chain from /home/3005040_2023_07_04T08_00_00Z.car.zst...
 382.48 MiB / 49.02 GiB [=>--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------]   0.76% 21.15 MiB/s 39m10s

Check that existing chain data was removed before the import-snapshot started:

/mnt/lotuschain/.lotus/datastore$ ls
chain  client  metadata  staging

And

/mnt/lotuschain/.lotus/datastore/chain$ ls
000000.vlog  KEYREGISTRY  LOCK  MANIFEST

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with the late review here

What is the difference between clearSplitstoreDir and deleteSplitstoreKeys

I forget what I was doing, the clear.. function doesn't delete the top level dir, you used the simpler / better one. One thing however: I think we should confirm that if you delete the splitstore data and run the node with splitstore then it correctly recreates the splitstore dir on startup. I think you might have already tested this, just double checking.

Docs (todo):

Since we're not making this the default I think we're good to merge without changing chain management docs.

@rjan90 rjan90 merged commit 9624dc5 into master Jul 20, 2023
@rjan90 rjan90 deleted the feat/remove-existing-chain branch July 20, 2023 16:41
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.

flag to remove existing repo data on snapshot import
2 participants