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

Add migration logs to pallet v2 #8243

Merged
2 commits merged into from
Mar 3, 2021
Merged

Add migration logs to pallet v2 #8243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d1d288
Add logs to proc macro pallet.
kianenigma Mar 2, 2021
0e168fe
update logs.
kianenigma Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions frame/support/procedural/src/pallet/expand/hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,29 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream {
let pallet_ident = &def.pallet_struct.pallet;
let where_clause = &def.hooks.where_clause;
let frame_system = &def.frame_system;
let has_runtime_upgrade = def.hooks.has_runtime_upgrade;
Copy link
Member

@athei athei Mar 2, 2021

Choose a reason for hiding this comment

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

Is this true when a pallet has a OnRuntimeUpgrade hook or only when the storage version is lower than the pallet version?

Copy link
Contributor Author

@kianenigma kianenigma Mar 2, 2021

Choose a reason for hiding this comment

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

only when you have a function with name on_runime_upgrade in your #[pallet_hooks].

Perhaps I can name it has_custom_, because even if you don't have this, the storage version is upgraded.

Copy link
Member

@athei athei Mar 2, 2021

Choose a reason for hiding this comment

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

So once we add the hook we will get

"⚠️ running migration for {} and setting new storage version to {:?}",

forever?

Even if the current upgrade did not require a migration because no changes to the data structures were made?

Copy link
Contributor Author

@kianenigma kianenigma Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, if you don't have the migration you should not have fn on_runtime_upgrade in your code, as it can be accidentally re-applied.

You might say I am opinionated here, but this is exactly the scenario of how we almost burned polkadot's balance structure once, by applying a migration twice, due to forgetting to remove it from on_runtime_upgrade. While we have now storage versions to also prevent that, my opinion is due to such incidents and catching them early.

Copy link
Contributor

@gui1117 gui1117 Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is this true when a pallet has a OnRuntimeUpgrade hook or only when the storage version is lower than the pallet version?

it is true when a pallet has a OnRuntimeUpgrade implementation (even if it is {}).

EDIT: It was an answer to the first message

Copy link
Contributor

@apopiak apopiak Mar 2, 2021

Choose a reason for hiding this comment

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

Yeah I'm unsure how to resolve the conflict between keeping very long migration functions around vs not supporting upgrades from old versions. My current preferred compromise is providing this sort of full migration in the migration mod or crate and then calling whatever the pallet author thinks is best in on_runtime_upgrade. Then downstream users still have access to all the migrations, but you don't have to keep them around in on_runtime_upgrade.

Copy link
Member

@athei athei Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if version <= 10 { panic!("version too old not supported") }
if version <= 11 { /* migrate from 10 to 11 */ }
if version <= 12 { /* migrate from 11 to 12 */ }
if version <= 13 { /* migrate from 12 to 13 */ }

This is actually what I was planning to do. Apart from being annoying its the only thing that I can come up with that "just works". Maybe we can build some macros or functions around this pattern to make it more bearable. Because for users this is the best case.

Copy link
Contributor Author

@kianenigma kianenigma Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For now I will change the log to something that reflects it, so that we can move on:

⚠️ Pallet-xxx is declaring internal migrations (which may or may not execute) and updates storage version to xxx

This is because what you are doing, is neat, but what I am building is mostly for pallets that make it into polkadot and current workflow there is that before each release we want to delete all the old migrations, and such warnings can help with that.

Copy link
Member

@athei athei Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand. I am fine with that. Contracts should be easily upgradeable for all runtimes. Therefore I need to go down this route.

Copy link
Contributor Author

@kianenigma kianenigma Mar 3, 2021

Choose a reason for hiding this comment

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

updated, then can you approve? :D


let log_runtime_upgrade = if has_runtime_upgrade {
// a migration is defined here.
quote::quote! {
#frame_support::log::info!(
target: #frame_support::LOG_TARGET,
"⚠️ {} declares internal migrations (which *might* execute), setting storage version to {:?}",
pallet_name,
new_storage_version,
);
}
} else {
// default.
quote::quote! {
#frame_support::log::info!(
target: #frame_support::LOG_TARGET,
"✅ no migration for {}, setting storage version to {:?}",
pallet_name,
new_storage_version,
);
}
};

quote::quote_spanned!(def.hooks.attr_span =>
impl<#type_impl_gen>
Expand Down Expand Up @@ -60,14 +83,22 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream {
for #pallet_ident<#type_use_gen> #where_clause
{
fn on_runtime_upgrade() -> #frame_support::weights::Weight {
// log info about the upgrade.
let new_storage_version = #frame_support::crate_to_pallet_version!();
let pallet_name = <
<T as #frame_system::Config>::PalletInfo
as
#frame_support::traits::PalletInfo
>::name::<Self>().unwrap_or("<unknown pallet name>");
#log_runtime_upgrade

let result = <
Self as #frame_support::traits::Hooks<
<T as #frame_system::Config>::BlockNumber
>
>::on_runtime_upgrade();

#frame_support::crate_to_pallet_version!()
.put_into_storage::<<T as #frame_system::Config>::PalletInfo, Self>();
new_storage_version.put_into_storage::<<T as #frame_system::Config>::PalletInfo, Self>();

let additional_write = <
<T as #frame_system::Config>::DbWeight as #frame_support::traits::Get<_>
Expand Down
8 changes: 8 additions & 0 deletions frame/support/procedural/src/pallet/parse/hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ pub struct HooksDef {
pub where_clause: Option<syn::WhereClause>,
/// The span of the pallet::hooks attribute.
pub attr_span: proc_macro2::Span,
/// Boolean flag, set to true if the `on_runtime_upgrade` method of hooks was implemented.
pub has_runtime_upgrade: bool,
}

impl HooksDef {
Expand Down Expand Up @@ -66,10 +68,16 @@ impl HooksDef {
return Err(syn::Error::new(item_trait.span(), msg));
}

let has_runtime_upgrade = item.items.iter().any(|i| match i {
syn::ImplItem::Method(method) => method.sig.ident == "on_runtime_upgrade",
Copy link
Contributor Author

@kianenigma kianenigma Mar 2, 2021

Choose a reason for hiding this comment

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

Happy to write a test for this, if someone can point out where, or find another way to avoid hardcoding the name here.

_ => false,
});

Ok(Self {
attr_span,
index,
instances,
has_runtime_upgrade,
where_clause: item.generics.where_clause.clone(),
})
}
Expand Down
41 changes: 19 additions & 22 deletions frame/support/src/dispatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,29 +1329,29 @@ macro_rules! decl_module {
{
fn on_runtime_upgrade() -> $return {
$crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!("on_runtime_upgrade"));
let result: $return = (|| { $( $impl )* })();
Copy link
Contributor Author

@kianenigma kianenigma Mar 2, 2021

Choose a reason for hiding this comment

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

here I just re-ordered stuff: first log that the migration is about to happen, and then do it.


let new_storage_version = $crate::crate_to_pallet_version!();
new_storage_version
.put_into_storage::<<$trait_instance as $system::Config>::PalletInfo, Self>();

let additional_write = <
<$trait_instance as $system::Config>::DbWeight as $crate::traits::Get<_>
>::get().writes(1);

let pallet_name = <<
$trait_instance
as
$system::Config
>::PalletInfo as $crate::traits::PalletInfo>::name::<Self>().expect("pallet will have name in the runtime; qed");
>::PalletInfo as $crate::traits::PalletInfo>::name::<Self>().unwrap_or("<unknown pallet name>");
let new_storage_version = $crate::crate_to_pallet_version!();

$crate::log::info!(
target: $crate::LOG_TARGET,
"⚠️ running migration for {} and setting new storage version to {:?}",
"⚠️ {} declares internal migrations (which *might* execute), setting storage version to {:?}",
pallet_name,
new_storage_version,
);

let result: $return = (|| { $( $impl )* })();

new_storage_version
.put_into_storage::<<$trait_instance as $system::Config>::PalletInfo, Self>();

let additional_write = <
<$trait_instance as $system::Config>::DbWeight as $crate::traits::Get<_>
>::get().writes(1);

result.saturating_add(additional_write)
}

Expand All @@ -1378,27 +1378,24 @@ macro_rules! decl_module {
{
fn on_runtime_upgrade() -> $crate::dispatch::Weight {
$crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!("on_runtime_upgrade"));

let new_storage_version = $crate::crate_to_pallet_version!();
new_storage_version
.put_into_storage::<<$trait_instance as $system::Config>::PalletInfo, Self>();

let pallet_name = <<
$trait_instance
as
$system::Config
>::PalletInfo as $crate::traits::PalletInfo>::name::<Self>().expect("pallet will have name in the runtime; qed");
>::PalletInfo as $crate::traits::PalletInfo>::name::<Self>().unwrap_or("<unknown pallet name>");
let new_storage_version = $crate::crate_to_pallet_version!();

$crate::log::info!(
target: $crate::LOG_TARGET,
"✅ no migration for '{}' and setting new storage version to {:?}",
"✅ no migration for {}, setting storage version to {:?}",
pallet_name,
new_storage_version,
);

<
<$trait_instance as $system::Config>::DbWeight as $crate::traits::Get<_>
>::get().writes(1)
new_storage_version
.put_into_storage::<<$trait_instance as $system::Config>::PalletInfo, Self>();

<<$trait_instance as $system::Config>::DbWeight as $crate::traits::Get<_>>::get().writes(1)
}

#[cfg(feature = "try-runtime")]
Expand Down