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

documentation for in-place migrations with x/upgrade module #8967

Merged
merged 62 commits into from
Apr 14, 2021

Conversation

technicallyty
Copy link
Contributor

Description

Adds documentation for both module and application developers who want to utilize the in-place store migration functionality with x/upgrade.

closes: #8940


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Read through it, it looks good! I added some improvement proposals

docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/building-modules/README.md Outdated Show resolved Hide resolved
docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/core/README.md Outdated Show resolved Hide resolved
docs/core/upgrade.md Outdated Show resolved Hide resolved
docs/core/upgrade.md Outdated Show resolved Hide resolved
docs/core/upgrade.md Show resolved Hide resolved
technicallyty and others added 15 commits March 24, 2021 13:36
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Blocking this PR for now. We might need bigger warnings on this docs, like "be careful what you're doing here".

Also, would like to add a small section about #8988 (comment). If you have context on that @technicallyty, feel free to take a stab at it. happy to discuss this with you more in details during a call.

We might also need a paragraph on InitGenesis stuff #8989 (comment), but that's still WIP

@technicallyty
Copy link
Contributor Author

Yeah that makes sense. There should be clear cut ways for an app dev to smoothly introduce new modules w/ genesis etc

docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
docs/building-modules/upgrade.md Outdated Show resolved Hide resolved
@zmanian
Copy link
Member

zmanian commented Apr 7, 2021

Once a chain is upgraded using an in place store migration, how does a new node sync?

During the in place store migration there is no genesis file generates for a new node to sync from.

State sync can be used to instant sync to height.

This question should be addressed in this document.

@amaury1093
Copy link
Contributor

amaury1093 commented Apr 9, 2021

Once a chain is upgraded using an in place store migration, how does a new node sync?

If the upgrade looks like binary_1 -> height N -> binary_2 then the full node must use those same binaries at those same heights, e.g. with a cosmovisor swap at N. Cosmovisor handles downloading binaries on the fly, so in the cosmovisor use-case there's no user intervention during the sync (or shouldn't be, I'm not sure anyone's tested in-place migrations with cosmovisor, see #9069).

This question should be addressed in this document.

Makes sense. @technicallyty would you be able to add a section on full node sync?

@technicallyty
Copy link
Contributor Author

technicallyty commented Apr 9, 2021

Once a chain is upgraded using an in place store migration, how does a new node sync?

If the upgrade looks like binary_1 -> height N -> binary_2 then the full node must use those same binaries at those same heights, e.g. with a cosmovisor swap at N. Cosmovisor handles downloading binaries on the fly, so in the cosmovisor use-case there's no user intervention during the sync (or shouldn't be, I'm not sure anyone's tested in-place migrations with cosmovisor, see #9069).

This question should be addressed in this document.

Makes sense. @technicallyty would you be able to add a section on full node sync?

added sync section. Not sure how specific I needed to get here, so let me know if im missing any important notes.

@zmanian
Copy link
Member

zmanian commented Apr 11, 2021

Sync section looks good to me

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

- Exporting the entire application state to a JSON file using the `export` CLI command, making changes, and then starting a new binary with the changed JSON file as the genesis file. See the [Chain Upgrade Guide](../migrations/chain-upgrade-guide-040.md#upgrade-procedure).

- Version v0.43 and later can perform upgrades in place to significantly decrease the upgrade time for chains with a larger state. Use the [Migration Upgrade Guide](../building-modules/upgrade.md) guide to set up your application modules to take advantage of in-place upgrades.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember reading a sentence saying something like "this document describes the 2nd method", imo it's still useful to add it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@technicallyty
Copy link
Contributor Author

updated the section about fromVM == 0 to reflect this PR

Copy link
Contributor

@barriebyron barriebyron left a comment

Choose a reason for hiding this comment

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

let's describe the purpose of the doc instead of "specified " or "mentioned above"

looks good!

docs/core/upgrade.md Outdated Show resolved Hide resolved
Co-authored-by: Barrie Byron <barrie.byron@tendermint.com>
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

Thank you for this, it answered some open questions of mine

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 14, 2021
@mergify mergify bot merged commit 849fab1 into master Apr 14, 2021
@mergify mergify bot deleted the ty-8940-upgrade_docs branch April 14, 2021 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation for x/upgrade in-place migrations
5 participants