Support for pre-upgrade command in Cosmovisor

[aaronc]

Ref:

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

As described in #9692 (comment):

To summarize, we will have convention where cosmovisor executes a $ pre-upgrade command (note, this command doesn't exist in the SDK -- the application defines it) prior to starting the app. We then evaluate the following exit status codes:

0: success; continue
1: assume command does not exist; continue
30: assume command exists but failed; fail
31: assume command exists but we should retry; keep retrying until 0 or 30 is returned

I would also add that pre-upgrade handler should basically look like what we're doing now for "pre-upgrades" of the multi-store: https://github.com/cosmos/gaia/blob/main/app/app.go#L428-L439. It would check for an upgrade name and skip-heights. So the pre-upgrade command will need to take --unsafe-skip-upgrades just like start does.

Also, it's an open question whether pre-upgrade should read the upgrade info file off the disk or get that passed to it by cosmovisor.

I think we should document how to do this in the SDK and provide any known migrations needed for things like app.toml in the SDK, but I don't think we can really generalized this pre-upgrade command. Apps will need to implement it for the upgrades they have planned.

[robert-zaremba]

solution1: #9692 (comment)

[robert-zaremba]

Alternative Design

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 then ad download and description.

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

CAVEAT: if we agree to use this proposal as the solution for #9894 then we will need to include a small update in the upcoming v0.44 security release to enable the UpgradeInfo extension.

@marbar3778 , @alexanderbez , @alessio -- looking for your feedback so we can quickly push it and combine with the v0.44 if we agree so.

[robert-zaremba]

Another way of handling it would be to overload the existing UpgradeInfo.Info attribute. We can handle multiple versions of it's content in the cosmovisor. However we won't have a nice struct validation in the app.

[robert-zaremba]

Important note: it seams we can change the UpgradeInfo even so it's part of the protobuf. We can do that because it's not a part of the Msg server nor a Query server. However it's a part of the legacy router (which we are deprecating). So the extending Msg restriction doesn't apply here.

[robert-zaremba]

@jgimeno , @spoo-bar , @tychoish , @alexanderbez -- here is the draft implementation: #10032

[tac0turtle]

The approach @robert-zaremba outlined seems like a good approach.

[spoo-bar]

Now that #10032 also allows for post upgrade scripts, maybe the same exit status codes could be used for post-upgrade also?

0: success; continue
1: assume command does not exist; continue
30: assume command exists but failed; fail
31: assume command exists but we should retry; keep retrying until 0 or 30 is returned

But regarding 30, since the upgradation of binary is already done, saying the upgrade failed doesn't sound right.
Is there a better approach to handle this case? Should the upgrade and postupgrade be atomic?

[anilcse]

Alternative Design

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 then ad download and description.

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

CAVEAT: if we agree to use this proposal as the solution for #9894 then we will need to include a small update in the upcoming v0.44 security release to enable the UpgradeInfo extension.

@marbar3778 , @alexanderbez , @alessio -- looking for your feedback so we can quickly push it and combine with the v0.44 if we agree so.

ACK, I believe we can keep download information in the script as well. Coz, what needs to be done with downloaded file might be a question for Devs. Some might be interested to execute the binary, some might need to move it to specific directory etc.

[robert-zaremba]

ACK, I believe we can keep download information in the script as well.

@anilcse - to some extend. The idea is that we firstly download the script (if download is not empty) and then we execute it. We don't want to put an ugly and long script in the run attribute with rules for different OSes.

[anilcse]

ACK, I believe we can keep download information in the script as well.

@anilcse - to some extend. The idea is that we firstly download the script (if download is not empty) and then we execute it. We don't want to put an ugly and long script in the run attribute with rules for different OSes.

Understood, that sounds good 👍

[spoo-bar]

@robert-zaremba Any ideas on how cosmovisor should handle errors by post upgrade scripts?
The binary has already been upgraded, so cosmovisor cant say the upgrade failed.

[robert-zaremba]

Decision for the v0.44 release.

Since the Alternative Design (solution 2) requires a small proto update (extending the x/upgrade Plan structure) we don't want to rush it include it into v0.44.0. Instead:

• let's continue feat!: update x/upgrade to handle additional instructions #10032 only with the pre-upgrade. CC: @spoo-bar
• I will move the new design I outlined here to a new issue (Extend Software Upgrade Proposal Plan #10047) and update my draft PRs.

The advantages of the solution-2 is that:

• we don't need to implement ops responsibility in the app binary
• we provide more flexibility for different setups / chains.
• It goes in line with a broader

[robert-zaremba]

Any ideas on how cosmovisor should handle errors by post upgrade scripts?

@spoo-bar - let's don't do post upgrade in this PR / issue. I suggest to continue the design discussion in #10047

Also for the future communication, we need to agree, if we want to present the outlined solution from this PR (app pre-upgrade) as a long term one or as a temporal one (note: with my proposal it's still possible to reuse app pre-upgrade command).

[alessio]

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

What makes this design superior to the "basic"? Instead of a sketched design, I'd appreciate it if you could provide the following:

• use case
• list of acceptance criteria (preferable in the Given/When/Then language)

Thanks!

[robert-zaremba]

Here is a discussion for the next cosmovisor improvements: #9928

We didn't deprecate the app pre-upgrade concept. We were rather thinking of upgrading the x/upgrade Plan / UpdateInfo structure to instrument all kind of tools responsible for handling updates. This way we have more generic solutions with instructions for upgrade as well as other features planned for future cosmovisor versions (eg library swaps).
So instead of hardcoding a rule in cosmovisor of "always try to run app pre-upgrade", this instruction will be part of the upgrade plan.

Acceptance criteria for the suggested Plan upgrade is:

• given upgrade plan, a tool developer or a dev ops have all information for doing a successful chain upgrade
• given upgrade plan, tool developers can independently design other orchestration tool
• given upgrade plan, when more complex upgrade is needed, a proposal can include more instructions (eg aforementioned library swap).

We were discussing it during the WG session: https://hackmd.io/jeYE8fwDTlCdFhMYUW9mpQ

Hope this clarifies your confusions.