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

kvdb: refactor as preparation for DB migration command in lndinit #5561

Merged
merged 8 commits into from
Oct 13, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Jul 23, 2021

EDIT: The main migration tool has moved to lightninglabs/lndinit#21 (still not ready for production/mainnet use!)

The code in this PR is now purely a refactor to allow lndinit to access the required library code.

@guggero guggero added this to the v0.14.0 milestone Jul 23, 2021
@guggero guggero added database Related to the database/storage of LND kvdb migration labels Jul 23, 2021
@guggero guggero force-pushed the database-migration-lndinit branch 2 times, most recently from 5773d84 to 7702c90 Compare September 23, 2021 14:24
@guggero guggero marked this pull request as ready for review September 23, 2021 14:26
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Awesome!!! ⚡ Just a quick glance, will do a more thorough review later!

return nil
}

err := dest.Put(k, v)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're missing the sequence migration in this case.

ctx, cancel := context.WithTimeout(ctx, etcdTimeout)
defer cancel()

_, err := cli.Put(ctx, key, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

One quick low hanging speedup is to gather key/values to a buffer then when the buffer hits say 100-1000 items then flush it to etcd in one txn. It's doable since the individual keys don't depend on each other.

)
})
if err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure but perhaps we could delete all keys at this point? Just to avoid leaving any junk in the DB.

@Roasbeef Roasbeef modified the milestones: v0.14.0, v0.15.0 Sep 27, 2021
@Roasbeef Roasbeef removed their request for review September 29, 2021 03:46
@joostjager joostjager mentioned this pull request Sep 29, 2021
@guggero
Copy link
Collaborator Author

guggero commented Dec 7, 2021

!lightninglabs-deploy mute 2022-Feb-01

@lightninglabs-deploy
Copy link

@guggero, remember to re-request review from reviewers when ready

@guggero
Copy link
Collaborator Author

guggero commented Feb 1, 2022

!lightninglabs-deploy mute

@djkazic
Copy link
Contributor

djkazic commented Mar 3, 2022

Is this safe to use for an existing bolt lnd installation?

@guggero
Copy link
Collaborator Author

guggero commented Mar 5, 2022

Is this safe to use for an existing bolt lnd installation?

No, this hasn't been tested sufficiently and isn't recommended to be used (which is also the main reason for this not to be merged yet).

@Roasbeef Roasbeef modified the milestones: v0.15.0, v0.16.0 Apr 12, 2022
@kiwiidb
Copy link
Contributor

kiwiidb commented Jul 4, 2022

Hey there,

I've compiled this code and tried to test it out on one of our regtest nodes. However I've failed to do a migration due to the fact that the current channeldb version is already 27, while this version of lndinit is still compiled with channeldb version 23.

2022-07-04 11:47:10.431 LNDINIT: Checking tombstone marker on source DB
2022-07-04 11:47:10.431 LNDINIT: Checking DB version of source DB
2022-07-04 11:47:10.432 LNDINIT: Runtime error: refusing to migrate source database with version 27 while latest known DB version is 23; please upgrade the DB before using the data migration tool

I'd love to test out the migration (on regtest) if these dependencies could be brought up-to-date.

@guggero guggero force-pushed the database-migration-lndinit branch from e0850d0 to 645239a Compare July 4, 2022 14:31
@guggero guggero changed the title kvdb: migrate data between different backends kvdb: refactor as preparation for DB migration command in lndinit Jul 4, 2022
@guggero
Copy link
Collaborator Author

guggero commented Jul 4, 2022

Done, @kiwiidb see lightninglabs/lndinit#21.

Please don't use this for production/mainnet data!

@guggero guggero force-pushed the database-migration-lndinit branch from 645239a to bd95d5c Compare July 4, 2022 14:51
@guggero
Copy link
Collaborator Author

guggero commented Jul 4, 2022

This is now just basic refactoring code, I think we can review and merge, @bhandras.

@guggero guggero requested a review from bhandras July 4, 2022 14:53
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM ✨

@guggero guggero requested a review from yyforyongyu July 5, 2022 09:15
kvdb/etcd/bucket.go Outdated Show resolved Hide resolved
channeldb/meta.go Show resolved Hide resolved
}

key := markerBucket.Get(markerKey)
if len(key) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to check key against nil here if we want to validate the markerKey's existence. Checking len(key) == 0 seems to have a different meaning as it's checking what's stored, not whether it's stored. Also nit but we may rename key to value or data here as it's the value we get from the bucket...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct, there is a difference between an empty slice and nil. But in our case it doesn't matter which it is, it just means the marker wasn't added correctly. So both cases are equivalent in this case. Added a comment to clarify and also renamed the variable.

channeldb/meta.go Show resolved Hide resolved
channeldb/meta.go Outdated Show resolved Hide resolved
channeldb/meta.go Show resolved Hide resolved
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Nice set of unit tests! linter complaints, otherwise LGTM💡


// Only adding the marker bucket should not be enough to be counted as
// a marker, we explicitly also want the value to be set.
err = db.Update(func(tx walletdb.ReadWriteTx) error {
Copy link
Member

Choose a reason for hiding this comment

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

👍

In order for us to be able to re-use the code for initializing an etcd
client connection, we extract and export the NewEtcdClient function.
We'll need to know whether a database was migrated to the latest version
in our upcoming data migration tool. To be able to determine the current
version of a DB and the total number of migrations in existence, we need
to export some of the functions in channeldb.
We also add some helper functions for adding tombstone and other markers
to a database.
To avoid running a channel DB that was successfully migrated to another
system by accident, we check if there is a tombstone marker before we
open the DB for use.
For the data migration tool we need to know what database namespaces
there are so we can loop over them.
@guggero guggero merged commit 4b82717 into lightningnetwork:master Oct 13, 2022
@guggero guggero deleted the database-migration-lndinit branch October 13, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Related to the database/storage of LND kvdb migration
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants