Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Runtime storage migrations checks from the release checklist #1659

Open
muharem opened this issue Sep 21, 2022 · 3 comments
Open

Runtime storage migrations checks from the release checklist #1659

muharem opened this issue Sep 21, 2022 · 3 comments

Comments

@muharem
Copy link
Contributor

muharem commented Sep 21, 2022

Problem

Manual storage migration checks from the release checklist slow down release process and introduce possibility of a human error.

Context

In order to improve current release process (read more) we review the release checklist. There are several items related to the runtime storage migration:

  • previously completed migrations are removed for any public (non-private/test) networks (check 1);
  • no migrations added in the last release that would need to be removed (check 2);
  • verify new migrations complete successfully, and the runtime state is correctly updated for any public (non-private/test) networks (check 3);

These checks are manual and I believe can have an alternative automated solution.

A release engineer (anyone who leads a particular release) might have a little context of old and new migrations within the runtimes. The engineer must check where (networks) the last runtime builds were applied and get the full context of all new migrations.
It might take a lot of time within the release process. As a result the check might be ignored and not work.

Proposal 1, idempotent migrations

First two checks can be dropped with the rule to include only idempotent migrations into the runtime. This would mean that any migration included in a runtime can be run multiple times and result will be always the same.

Frame already provides supporting tools to write such migrations: StorageVersion storage key, GetStorageVersion trait.

Runtime owners will need to verify new migrations during PR reviews and a note can be left to guide contributors (example).

This will be a general solution, any exception for a particular release is possible and should be led by an owner.

Proposal 2, automated tests for migrations

The third check can be dropped if we treat migrations as common functionality (ex. dispatchable call). An engineer (owner) should test migrations during development and provide an automation tests (unit / integration).
There are some additional tools provided by frame to automate and check correctness of a state: pre_upgrade and post_upgrade hooks.

This will be a general solution, the owner if required checks migration results during the release process or/and after the deployment.

@gilescope
Copy link
Contributor

gilescope commented Sep 22, 2022

"idempotent Migrations" - I think they do tend to be written this way. My understanding is there's no harm in letting them build up and clearing them down every so often.

@NachoPal
Copy link
Contributor

NachoPal commented Oct 4, 2022

First two checks can be dropped with the rule to include only idempotent migrations into the runtime. This would mean that any migration included in a runtime can be run multiple times and result will be always the same.

Even if the result is always the same, I think it is good to clean up old migrations. I am not familiar with idempotent migrations, but might it be possible that after the first run, they still consume some weight? If that is the case is, that is another reason to keep it tidy to avoid too heavy runtime upgrades.

@NachoPal
Copy link
Contributor

NachoPal commented Oct 4, 2022

Also, I think you forgot to mention that we have to check if migrations should be applied to the release. For that we have to check Polkadot/Kusama release runtimes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants