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

migration: scope key migration to stores #9005

Merged
merged 12 commits into from
Jul 16, 2022
Merged

Conversation

tychoish
Copy link
Contributor

@tychoish tychoish commented Jul 14, 2022

The original migration script assumed that there was essentially one store and that keys with any prefix could appear in any store, which while not apparent from the code, is not true. As a result the initial implementation of the migration was too flexible and would attempt to faithfully migrate code with any prefix in any collection.

The challenge with this approach, is that some keys can appear to to have prefixes that don't apply. In particularly the indexed data could appear to be evidence or another key, and be migrated into the (relative) unknown, resulting in apparent data loss. The inverse, would be much less likely but theoretically possible.

Thankfully, if we only attempt to migrate keys in stores with the corresponding prefix. This change restructures the migration so that we pass the name of the store into the migration function, and then define migrations with store names so we can correlate and scope migrations appropriately.

Additional follow on work will integrate migrations into database constructors as part of node construction.

Addresses: #8889

scripts/keymigrate/migrate.go Outdated Show resolved Hide resolved
scripts/keymigrate/migrate.go Outdated Show resolved Hide resolved
@cmwaters
Copy link
Contributor

@tychoish can you add a description of the PR and possibly link the reference issue (which I believe is the automating upgrades one)

@tychoish
Copy link
Contributor Author

@tychoish can you add a description of the PR and possibly link the reference issue (which I believe is the automating upgrades one)

Done!

@williambanfield
Copy link
Contributor

Thankfully, if we only attempt to migrate keys in stores with the corresponding prefix.

Does this fix allow the problem of key migration collisions to completely go away?

@tychoish
Copy link
Contributor Author

Does this fix allow the problem of key migration collisions to completely go away?

I believe so. The caveats/risks of key migration remain:

  • Particularly of the transaction index and indexed events it's possible that the migration could be non-idempotent. This is very slim, but proving idempotentcy is hard.
  • There could still be bugs in the migration, which could render some data being unreachable, likely in a way that could be pretty silent. This is not a new risk, and proving that it's not there is hard.

There aren't a case of systemic migration bugs that are dependent on the input data, which was the case before.

While this change does not automate the run of the migration script, doing so is still not without costs:

  • running the migration script for a table before opening that table takes time, and we don't have a good way right now of storing "we're done with the migration" for easy nooping.
  • idempotentcy would be nicer to be more sure of.

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

👍

scripts/keymigrate/migrate.go Outdated Show resolved Hide resolved
@cmwaters
Copy link
Contributor

running the migration script for a table before opening that table takes time, and we don't have a good way right now of storing "we're done with the migration" for easy nooping.

This would be nice to achieve for automatic migrations

@tychoish
Copy link
Contributor Author

running the migration script for a table before opening that table takes time, and we don't have a good way right now of storing "we're done with the migration" for easy nooping.

This would be nice to achieve for automatic migrations

Definitely. Shouldn't block this PR, and maybe would be a good ramping project for someone.

@tychoish tychoish merged commit cc07318 into master Jul 16, 2022
@tychoish tychoish deleted the tycho/key-migration-scoping branch July 16, 2022 01:14
mergify bot pushed a commit that referenced this pull request Jul 16, 2022
mergify bot pushed a commit that referenced this pull request Jul 16, 2022
tychoish added a commit that referenced this pull request Jul 16, 2022
(cherry picked from commit cc07318)

Co-authored-by: Sam Kleinman <garen@tychoish.com>
lbrown2007 pushed a commit that referenced this pull request Jul 19, 2022
lbrown2007 pushed a commit that referenced this pull request Jul 20, 2022
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.

4 participants