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.

Sign up for GitHub

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

docs: simply ADR-47 #16150

Closed
wants to merge 5 commits into from
Closed

docs: simply ADR-47 #16150

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ba93f7
docs: simply ADR-47
julienrbrt May 15, 2023
1b3ebab
fix copy/paste from PR.
julienrbrt May 15, 2023
febe9e0
fix copy/paste from PR.
julienrbrt May 15, 2023
6211e12
Merge branch 'main' into julien/adr-47
julienrbrt May 15, 2023
4c3d766
wording
julienrbrt May 15, 2023
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
161 changes: 59 additions & 102 deletions docs/architecture/adr-047-extend-upgrade-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,28 @@
## Changelog

* Nov, 23, 2021: Initial Draft
* May 15, 2023: Remove `pre_run` and `post_run` fields for simplifying the ADR

## Status

PROPOSED Not Implemented
ACCEPTED Not Implemented

## Abstract

This ADR expands the existing x/upgrade `Plan` proto message to include new fields for defining pre-run and post-run processes within upgrade tooling.
It also defines a structure for providing downloadable artifacts involved in an upgrade.
This ADR expands the existing x/upgrade `Plan` proto message to include new fields for defining a structure for providing downloadable artifacts involved in an upgrade.

## Context

The `upgrade` module in conjunction with Cosmovisor are designed to facilitate and automate a blockchain's transition from one version to another.

Users submit a software upgrade governance proposal containing an upgrade `Plan`.
The [Plan](https://github.com/cosmos/cosmos-sdk/blob/v0.44.5/proto/cosmos/upgrade/v1beta1/upgrade.proto#L12) currently contains the following fields:
The [Plan](https://github.com/cosmos/cosmos-sdk/blob/main/proto/cosmos/upgrade/v1beta1/upgrade.proto#L13-L45) currently contains the following fields:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This link needs to be to a specific version instead of a link to whatever main currently has, specially since it include line numbers.


* `name`: A short string identifying the new version.
* `height`: The chain height at which the upgrade is to be performed.
* `info`: A string containing information about the upgrade.

The `info` string can be anything.
However, Cosmovisor will try to use the `info` field to automatically download a new version of the blockchain executable.
The `info` string can be anything. Cosmovisor tries to use the `info` field to automatically download a new version of the blockchain executable.
For the auto-download to work, Cosmovisor expects it to be either a stringified JSON object (with a specific structure defined through documentation), or a URL that will return such JSON.
The JSON object identifies URLs used to download the new blockchain executable for different platforms (OS and Architecture, e.g. "linux/amd64").
Such a URL can either return the executable file directly or can return an archive containing the executable and possibly other assets.
Expand All @@ -42,63 +42,30 @@ Currently, there is no mechanism that makes Cosmovisor run a command after the u
The current upgrade process has this timeline:

1. An upgrade governance proposal is submitted and approved.
1. The upgrade height is reached.
1. The `x/upgrade` module writes the `upgrade_info.json` file.
1. The chain halts.
1. Cosmovisor backs up the data directory (if set up to do so).
1. Cosmovisor downloads the new executable (if not already in place).
1. Cosmovisor executes the `${DAEMON_NAME} pre-upgrade`.
1. Cosmovisor restarts the app using the new version and same args originally provided.
2. The upgrade height is reached.
3. The `x/upgrade` module writes the `upgrade_info.json` file.
4. The chain halts.
5. Cosmovisor backs up the data directory (if set up to do so).
6. Cosmovisor downloads the new executable (if not already in place).
7. Cosmovisor executes the `${DAEMON_NAME} pre-upgrade`.
8. Cosmovisor restarts the app using the new version and same args originally provided.

## Decision

### Protobuf Updates

We will update the `x/upgrade.Plan` message for providing upgrade instructions.
The upgrade instructions will contain a list of artifacts available for each platform.
It allows for the definition of a pre-run and post-run commands.
These commands are not consensus guaranteed; they will be executed by Cosmosvisor (or other) during its upgrade handling.
The `x/upgrade.Plan` message is updated for providing upgrade artifacts. The artifacts available define each platform.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is discussing things not yet done, it should be future tense. Also, the second sentence is weird to me. I'm not quite sure what it's trying to say.

Suggested change
The `x/upgrade.Plan` message is updated for providing upgrade artifacts. The artifacts available define each platform.
The `x/upgrade.Plan` message will be updated to provide a list of artifacts, one for each available platform.


```protobuf
message Plan {
// ... (existing fields)

UpgradeInstructions instructions = 6;
}
```

The new `UpgradeInstructions instructions` field MUST be optional.

```protobuf
message UpgradeInstructions {
string pre_run = 1;
string post_run = 2;
repeated Artifact artifacts = 3;
string description = 4;
repeated Artifact artifacts = 6;
}
```

All fields in the `UpgradeInstructions` are optional.
* `pre_run` is a command to run prior to the upgraded chain restarting.
If defined, it will be executed after halting and downloading the new artifact but before restarting the upgraded chain.
The working directory this command runs from MUST be `{DAEMON_HOME}/cosmovisor/{upgrade name}`.
This command MUST behave the same as the current [pre-upgrade](https://github.com/cosmos/cosmos-sdk/blob/v0.44.5/docs/migrations/pre-upgrade.md) command.
It does not take in any command-line arguments and is expected to terminate with the following exit codes:

| Exit status code | How it is handled in Cosmosvisor |
|------------------|---------------------------------------------------------------------------------------------------------------------|
| `0` | Assumes `pre-upgrade` command executed successfully and continues the upgrade. |
| `1` | Default exit code when `pre-upgrade` command has not been implemented. |
| `30` | `pre-upgrade` command was executed but failed. This fails the entire upgrade. |
| `31` | `pre-upgrade` command was executed but failed. But the command is retried until exit code `1` or `30` are returned. |
If defined, then the app supervisors (e.g. Cosmovisor) MUST NOT run `app pre-run`.
* `post_run` is a command to run after the upgraded chain has been started. If defined, this command MUST be only executed at most once by an upgrading node.
The output and exit code SHOULD be logged but SHOULD NOT affect the running of the upgraded chain.
The working directory this command runs from MUST be `{DAEMON_HOME}/cosmovisor/{upgrade name}`.
* `artifacts` define items to be downloaded.
It SHOULD have only one entry per platform.
* `description` contains human-readable information about the upgrade and might contain references to external resources.
It SHOULD NOT be used for structured processing information.
The new `artifacts` field is optional. It defines items to be downloaded.
It SHOULD have only one entry per platform.

```protobuf
message Artifact {
Expand Down Expand Up @@ -133,76 +100,68 @@ For example, if the `url` is `"https://example.com?checksum=md5:d41d8cd98f00b204

### Upgrade Module Updates

If an upgrade `Plan` does not use the new `UpgradeInstructions` field, existing functionality will be maintained.
If an upgrade `Plan` does not use the new `Artifacts` field, existing functionality is maintained.
The parsing of the `info` field as either a URL or `binaries` JSON will be deprecated.
During validation, if the `info` field is used as such, a warning will be issued, but not an error.
During validation, if the `info` field is used as such, a warning is issued, but not an error.

We will update the creation of the `upgrade-info.json` file to include the `UpgradeInstructions`.
We will update the creation of the `upgrade-info.json` file to include the `Artifacts`.

We will update the optional validation available via CLI to account for the new `Plan` structure.
We will add the following validation:

1. If `UpgradeInstructions` are provided:
1. There MUST be at least one entry in `artifacts`.
1. All of the `artifacts` MUST have a unique `platform`.
1. For each `Artifact`, if the `url` contains a `checksum` query parameter:
1. The `checksum` query parameter value MUST be in the format of `{checksum_algo}:{checksum}`.
1. The `{checksum}` from the query parameter MUST equal the `checksum` provided in the `Artifact`.
1. The `{checksum_algo}` from the query parameter MUST equal the `checksum_algo` provided in the `Artifact`.
1. The following validation is currently done using the `info` field. We will apply similar validation to the `UpgradeInstructions`.
1. There MUST be at least one entry in `artifacts`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This contradicts the claim that the new artifacts are optional.

All of the `artifacts` MUST have a unique `platform`.
For each `Artifact`, if the `url` contains a `checksum` query parameter:
1. The `checksum` query parameter value MUST be in the format of `{checksum_algo}:{checksum}`.
2. The `{checksum}` from the query parameter MUST equal the `checksum` provided in the `Artifact`.
3. The `{checksum_algo}` from the query parameter MUST equal the `checksum_algo` provided in the `Artifact`.
2. The following validation is currently done using the `info` field. We will apply similar validation to the `Artifact` field.
For each `Artifact`:
1. The `platform` MUST have the format `{OS}/{CPU}` or be `"any"`.
1. The `url` field MUST NOT be empty.
1. The `url` field MUST be a proper URL.
1. A `checksum` MUST be provided either in the `checksum` field or as a query parameter in the `url`.
1. If the `checksum` field has a value and the `url` also has a `checksum` query parameter, the two values MUST be equal.
1. The `url` MUST return either a file or an archive containing either `bin/{DAEMON_NAME}` or `{DAEMON_NAME}`.
1. If a `checksum` is provided (in the field or as a query param), the checksum of the result of the `url` MUST equal the provided checksum.
1. The `platform` MUST have the format `{OS}/{CPU}` or be `"any"`.
2. The `url` field MUST NOT be empty.
3. The `url` field MUST be a proper URL.
4. A `checksum` MUST be provided either in the `checksum` field or as a query parameter in the `url`.
5. If the `checksum` field has a value and the `url` also has a `checksum` query parameter, the two values MUST be equal.
6. The `url` MUST return either a file or an archive containing either `bin/{DAEMON_NAME}` or `{DAEMON_NAME}`.
7. If a `checksum` is provided (in the field or as a query param), the checksum of the result of the `url` MUST equal the provided checksum.

Downloading of an `Artifact` will happen the same way that URLs from `info` are currently downloaded.

### Cosmovisor Updates

If the `upgrade-info.json` file does not contain any `UpgradeInstructions`, existing functionality will be maintained.
If the `upgrade-info.json` file does not contain any `Artifacts`, existing functionality is maintained.

We will update Cosmovisor to look for and handle the new `UpgradeInstructions` in `upgrade-info.json`.
If the `UpgradeInstructions` are provided, we will do the following:
Cosmovisor is to be updated to look for and handle the new `Artifacts` in `upgrade-info.json`.
If the `Artifacts` are provided, the following should be done:

1. The `info` field will be ignored.
1. The `artifacts` field will be used to identify the artifact to download based on the `platform` that Cosmovisor is running in.
1. If a `checksum` is provided (either in the field or as a query param in the `url`), and the downloaded artifact has a different checksum, the upgrade process will be interrupted and Cosmovisor will exit with an error.
1. If a `pre_run` command is defined, it will be executed at the same point in the process where the `app pre-upgrade` command would have been executed.
It will be executed using the same environment as other commands run by Cosmovisor.
1. If a `post_run` command is defined, it will be executed after executing the command that restarts the chain.
It will be executed in a background process using the same environment as the other commands.
Any output generated by the command will be logged.
Once complete, the exit code will be logged.
1. The `info` field is ignored.
2. The `artifacts` field is used to identify the artifact to download based on the `platform` that Cosmovisor is running in.
3. If a `checksum` is provided (either in the field or as a query param in the `url`), and the downloaded artifact has a different checksum, the upgrade process is interrupted and Cosmovisor will exit with an error.

We will deprecate the use of the `info` field for anything other than human readable information.
A warning will be logged if the `info` field is used to define the assets (either by URL or JSON).
The `info` field should be deprecated for anything other than human readable information.
A warning is logged if the `info` field is used to define the assets (either by URL or JSON).
Comment on lines -181 to +142
Copy link
Collaborator

Choose a reason for hiding this comment

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

After thinking about this for the last year+, I don't think this deprecation is a good idea. The specific functionality being removed that is still needed, is the ability to provide a URL that returns the artifact download URLs. The new artifacts stuff requires the artifact download URLs to be directly in the upgrade plan.


The new upgrade timeline is very similar to the current one. Changes are in bold:
The new upgrade timeline stays the same, namely:

1. An upgrade governance proposal is submitted and approved.
1. The upgrade height is reached.
1. The `x/upgrade` module writes the `upgrade_info.json` file **(now possibly with `UpgradeInstructions`)**.
1. The chain halts.
1. Cosmovisor backs up the data directory (if set up to do so).
1. Cosmovisor downloads the new executable (if not already in place).
1. Cosmovisor executes **the `pre_run` command if provided**, or else the `${DAEMON_NAME} pre-upgrade` command.
1. Cosmovisor restarts the app using the new version and same args originally provided.
1. **Cosmovisor immediately runs the `post_run` command in a detached process.**
2. The upgrade height is reached.
3. The `x/upgrade` module writes the `upgrade_info.json` file.
4. The chain halts.
5. Cosmovisor backs up the data directory (if set up to do so).
6. Cosmovisor downloads the new executable (if not already in place).
7. Cosmovisor executes the `${DAEMON_NAME} pre-upgrade` command (if available).
8. Cosmovisor restarts the app using the new version and same args originally provided.

## Consequences

### Backwards Compatibility

Since the only change to existing definitions is the addition of the `instructions` field to the `Plan` message, and that field is optional, there are no backwards incompatibilities with respects to the proto messages.
Additionally, current behavior will be maintained when no `UpgradeInstructions` are provided, so there are no backwards incompatibilities with respects to either the upgrade module or Cosmovisor.
Since the only change to existing definitions is the addition of the `Artifacts` field to the `Plan` message, and that field is optional, there are no backwards incompatibilities with respects to the proto messages.
Additionally, current behavior is maintained when no `Artifacts` are provided, so there are no backwards incompatibilities with respects to either the upgrade module or Cosmovisor.

### Forwards Compatibility

In order to utilize the `UpgradeInstructions` as part of a software upgrade, both of the following must be true:
In order to utilize the `Artifacts` as part of a software upgrade, both of the following must be true:

1. The chain must already be using a sufficiently advanced version of the Cosmos SDK.
1. The chain's nodes must be using a sufficiently advanced version of Cosmovisor.
Expand All @@ -216,12 +175,11 @@ In order to utilize the `UpgradeInstructions` as part of a software upgrade, bot
### Negative

1. The `Plan` message becomes larger. This is negligible because A) the `x/upgrades` module only stores at most one upgrade plan, and B) upgrades are rare enough that the increased gas cost isn't a concern.
1. There is no option for providing a URL that will return the `UpgradeInstructions`.
1. The only way to provide multiple assets (executables and other files) for a platform is to use an archive as the platform's artifact.
2. The only way to provide multiple assets (executables and other files) for a platform is to use an archive as the platform's artifact.

### Neutral

1. Existing functionality of the `info` field is maintained when the `UpgradeInstructions` aren't provided.
1. Existing functionality of the `info` field is maintained when the `Artifacts` aren't provided.

## Further Discussions

Expand All @@ -242,9 +200,8 @@ In order to utilize the `UpgradeInstructions` as part of a software upgrade, bot

## References

* [Current upgrade.proto](https://github.com/cosmos/cosmos-sdk/blob/v0.44.5/proto/cosmos/upgrade/v1beta1/upgrade.proto)
* [Upgrade Module README](https://github.com/cosmos/cosmos-sdk/blob/v0.44.5/x/upgrade/spec/README.md)
* [Cosmovisor README](https://github.com/cosmos/cosmos-sdk/blob/cosmovisor/v1.0.0/cosmovisor/README.md)
* [Pre-upgrade README](https://github.com/cosmos/cosmos-sdk/blob/v0.44.5/docs/migrations/pre-upgrade.md)
* [Draft/POC PR #10032](https://github.com/cosmos/cosmos-sdk/pull/10032)
* [Current upgrade.proto](https://github.com/cosmos/cosmos-sdk/blob/main/proto/cosmos/upgrade/v1beta1/upgrade.proto)
* [Upgrade Module README](https://github.com/cosmos/cosmos-sdk/blob/main/x/upgrade/README.md)
* [Cosmovisor README](https://github.com/cosmos/cosmos-sdk/blob/main/tools/cosmovisor/README.md)
* [Pre-upgrade README](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/migrations/01-pre-upgrade.md)
Comment on lines +203 to +206
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Pre-upgrade README link is incorrect. The old one still works, but I'm not sure where it was moved to now.

Also, these links need to be to specific version tags and not to /main/ (or any other branch). They're here to provide context which is lost if the content changes later, or worse, the file moves and the link no longer works.

* [RFC 1738: Uniform Resource Locators](https://www.ietf.org/rfc/rfc1738.txt)