Extend Software Upgrade Proposal Plan

[robert-zaremba]

Summary

Towards more sophisticated chain upgrades we would like to extend the Software Upgrade Proposal Plan with more details.

Dependencies:

• ADR for x/upgrade Upgrade Plan #10583

Problem Definition

Ref:

• Ensure Tendermint v0.35 key migration is supported in Cosmos SDK, Cosmovisor and/or Gaia #9894
• Add migrations for app.toml #9692
• Support for pre-upgrade command in Cosmovisor #9973

Proposal

Instead of setting the upgrade process fully in the app, I propose to make it more flexible by extending the UpgradeInfo structure by adding PreUpgrade attribute:

type UpgradeInfo struct {
    Name string
    Info string
    PreUpgrade PreUpgradeInstructions
}

This is how the the PreUpgradeInstructions can look like:

{
  run: "script.sh" | "appd pre-upgrade  more-options",
  download: {
     "linux/amd64": "https://github.com/cosmos/cosmos-sdk/raw/master/cosmovisor/testdata/repo/chain3-zip_dir/autod.zip?checksum=sha256:8951f52a0aea8617de0ae459a20daf704c29d259c425e60d520e363df0f166b4"
  },
  description: "some more information about the upgrade process for devops"
}

• run will tell cosmovisor (or an admin) what to run. It can be a shell command (if we support only POSIX shells), or an updated app binary, as in the original solution.
• download record is optional has the same as the binaries we use to encode binaries to download in UpdateInfo.Info.
• description: optional, more information for devops.

NOTE: we use a concrete type instead of string to be specific about the content.

This solution will solve other use-cases where we will need more advanced upgrade scenarios (eg moving files or downloading external libraries).

We can implement it in phases - and only start with run and description, and then add download.

I was presenting this solution during the Cosmovisor WG call last Friday and got ack as a superior then the "basic" one proposed originally.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[robert-zaremba]

CC: @tychoish , @jgimeno , @spoo-bar , @aaronc , @alexanderbez

[spoo-bar]

I think Plan.Info can also be changed to use named properties to store its info instead of storing it as string.
Basically use the same Asset type as proposed in the PR for UpgradeInstructions

cosmos-sdk/proto/cosmos/upgrade/v1beta1/upgrade.proto

Lines 66 to 76 in 1a6d03b

	message Asset {
	option (gogoproto.equal) = true;
	// Platform identifier. It's composed from OS and CPU architecture. Example: "linux/amd64"
	string platform = 1;
	// URL to a script or binary to download. Should be related to the UpgradeInstructions pre_run or post_run
	// commands. If multiple files are needed, then they should be packed in a gzip archive.
	string url = 2;
	// Checksum is a sha256 hex encoded checksum of an artifact referenced by the URL.
	string checksum = 3;
	}

Currently, cosmovisor just makes an effort to understand the Info value. But as the json value is string it can store whatever.

[spoo-bar]

@robert-zaremba If you aren't maybe I can continue working on this issue?

[robert-zaremba]

@spoo-bar - we firstly need to finish a design of it. I started coding a PoC 1.5 month ago.

[robert-zaremba]

We need to decide:

• What to do with Info field. I'm rather against doing a breaking change. We can keep it as a non machine interpret-able instructions.
• What other attributes we want to add to the Plan. I proposed PreUpgrade, but following my previous point, we can create Upgrade - where we will put binaries.

Any thoughts? cc: @aaronc , @alessio , @spoo-bar , @anilcse , @dwedul-figure

[dwedul-figure]

I like the "fallback" approach here.

Add the new fields. Then, when handling an upgrade, if any of them have value, then the Info field is treated as just an info field, and we handle everything using the new stuff. If all of them are empty, then fallback to the old way of using the Info field the way we are now. The documentation should note this, and also state that in the future, the Info field fallback functionality might be removed.

An alternative might be to create a whole new proto and deprecate the current one. The nice thing with this is being able to use deprecation tags/indicators on the old proto. I feel it'd be significantly more work to create and wire this new proto (side-by-side with the current one) than to just add the fields to the existing proto and use the fallback approach on the Info field.

[tac0turtle]

This needs to land with 0.46 if we plan on including tendermint 0.35+ with it.

[robert-zaremba]

The support for tendermint v0.35 is already there (including migration support). This feature extends what we already have in master, and it's not necessary for Tendermint v0.35 migration.

[tac0turtle]

oh okay, it was my understanding it was not supported. If this was supported we should of tried to get this out in 0.45. Is it tested?

can you point me to a pr

[robert-zaremba]

We don't need it in 0.45. We need it for 0.46 which will be tagged from `master.
related issue: #9973