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

upgrades: 🚘 migrations should be idempotent #4365

Closed
cratelyn opened this issue May 9, 2024 · 8 comments
Closed

upgrades: 🚘 migrations should be idempotent #4365

cratelyn opened this issue May 9, 2024 · 8 comments
Labels
A-upgrades Area: Relates to chain upgrades C-enhancement Category: an enhancement to the codebase needs-refinement unclear, incomplete, or stub issue that needs work

Comments

@cratelyn
Copy link
Contributor

cratelyn commented May 9, 2024

Is your feature request related to a problem? Please describe.

it would be nice if migrations were idempotent, to help make operator's lives easier while performing chain upgrades.

Describe the solution you'd like

it would be nice if we could examine a version number (q: which?), and only apply a migration if the version number was below some threshold, and returning early if not, rather than writing migrations in such a way as to be idempotent.

@github-project-automation github-project-automation bot moved this to Backlog in Penumbra May 9, 2024
@cratelyn cratelyn added C-enhancement Category: an enhancement to the codebase A-upgrades Area: Relates to chain upgrades labels May 9, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label May 9, 2024
@conorsch
Copy link
Contributor

conorsch commented May 9, 2024

we could examine a version number (q: which?)

Ahoy: https://github.com/penumbra-zone/penumbra/blob/v0.73.2/crates/core/app/src/lib.rs#L24 We also reference this value in the repo's COMPATIBILITY.md, but not yet in local state, which would be supremely useful for migrations.

conorsch added a commit that referenced this issue May 9, 2024
conorsch added a commit that referenced this issue May 9, 2024
@cratelyn
Copy link
Contributor Author

is #4373 is a duplicate of this, @erwanor?

@erwanor
Copy link
Member

erwanor commented May 10, 2024

I don't think it's a duplicate? This is about adding migration-scoped check against specific protocol versions, the other ticket is about changing the halt signaling mechanism and adding a broad "is chain halted" check to pd migrate.

@cratelyn
Copy link
Contributor Author

agreed, i marked it as resolved because i was able to figure that out. thank you!

@hdevalence
Copy link
Member

I think that #4373 would close this, since the migrate command would bail out if the halt-bit is unset, thus making all migrations idempotent at the top-level.

@cronokirby
Copy link
Contributor

Closed in #4373.

@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra May 21, 2024
@conorsch
Copy link
Contributor

Our migrations are not currently idempotent; see reports in #4432. At the very least, we should be exercising rerunning a migration and confirming no error in automated testing #4323 to verify it doesn't error.

@conorsch conorsch reopened this May 22, 2024
@github-project-automation github-project-automation bot moved this from Done to In progress in Penumbra May 22, 2024
@hdevalence
Copy link
Member

I don’t see any evidence in that thread that there is an idenpotency issue.

@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra May 22, 2024
conorsch added a commit that referenced this issue May 24, 2024
We're not actually using this value in storage yet (#4365), but we still
update it for clarity about compatibility.
erwanor pushed a commit that referenced this issue May 24, 2024
## Describe your changes
We're not actually using this value in storage yet (#4365), but we still
update it for clarity about compatibility.

## Issue ticket number and link

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> docs and logging-only app version, does not affect how node comes to
consensus

Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-upgrades Area: Relates to chain upgrades C-enhancement Category: an enhancement to the codebase needs-refinement unclear, incomplete, or stub issue that needs work
Projects
Archived in project
Development

No branches or pull requests

5 participants