feat(rust): Implement upgrade and uninstall commands #5171

BradLewis · 2023-06-26T01:02:24Z

I've created this as a draft PR for now to get your feedback on this approach. It probably needs more work with things like the error handling and the messages, but I figured it'd be worthwhile getting feedback on the more "happy" paths before working on making that better. Let me know what you think!

Current behavior

Currently there is no way for the user to upgrade or uninstall ockam directly through the binary.

Proposed changes

Resolves #4916

This PR adds both an upgrade and uninstall command.

Upgrade

For the upgrade command it works as follows:

Checks to see if a new update is available by checking the github releases.
Stops all the running nodes.
Checks to see if it was installed via homebrew. If it was it runs brew upgrade ockam. It will then restart all the nodes it stopped in step 2.
If it was installed via the install.sh script it will check to see if it can find location of the script by looking at $OCKAM_HOME or $HOME/.ockam. If it can't find it then it stops the upgrade.
If all is okay to upgrade, it will delete the existing ockam binary.
It will then upgrade ockam using the install.sh script, specifying the specific version and the install path.
Once the upgrade is completed, it'll restart all the nodes that were stopped in step 2.

In order to do this I made some slight changes, namely writing the install.sh script to the $OCKAM_HOME folder, and add the $OCKAM_HOME variable to the environment variable files.

I also implemented some logic to fetch the latest versions from the releases on github rather than the upgrade.json file as I found that the upgrade.json file seems to be out of date? (0.87.0 and 0.88.0 are empty, and 0.86.0 points to 0.87.0 etc). Not sure if this is intentional however.

Uninstall

For the uninstall command,

it will first check if it was uninstalled through the install.sh script or with homebrew.
If it was installed with homebrew, it'll run brew uninstall ockam, otherwise it'll delete the ockam binary.
It will then clear all references to $OCKAM_HOME and then delete the $OCKAM_HOME directory.

I'm not sure about this final step however as it'll delete all the metadata on nodes and whatnot the user has, which could be troublesome if they just wanted to do a simple uninstall/reinstall to fix an issue or something. Let me know what you think of that.

Checks

All commits in this Pull Request are signed and Verified by Github.
All commits in this Pull Request follow the Ockam commit message convention.
I accept the Ockam Community Code of Conduct.
I have accepted the Ockam Contributor License Agreement by adding my Git/Github details in a row at the end of the CONTRIBUTORS.csv file in a separate pull request to the build-trust/ockam-contributors repository. The easiest way to do this is to edit the CONTRIBUTORS.csv file in the github web UI and create a PR, this will mark the commit as verified.

mrinalwadhwa · 2023-06-26T02:43:12Z

@BradLewis this is awesome, thank you for spending time on this. I'll try the commands and leave some thoughts soon

install.sh

implementations/rust/ockam/ockam_command/src/util/installer.rs

BradLewis · 2023-06-27T21:49:48Z

Okay I've refactored the implementation quite a bit and improved the error handling as well. This split the upgrade and uninstall process for homebrew and binary installations into 2 separate logical flows. For homebrew it's pretty simple, just run the respective commands. For the binary installation it's a little more involved.

For upgrade:

Backup binary and environment files
Remove ockam lines in the environment files
Download installation script
Run the installation script to install the latest version and rewrite all the environment files.
If any point in the process fails, everything gets restored from backup

For installation it follows a similar pattern, with everything being backed up and restored if anything goes wrong.

Let me know what you think, and if you like this approach I'll open this as a non-draft PR 🙂

etorreborre

Hi @BradLewis I left a bunch of comments and suggestions for you to consider. Next I would like to test your PR a bit on my machine to double-check that it is also working for me. How have you tested it so far?

implementations/rust/ockam/ockam_command/src/upgrade.rs

implementations/rust/ockam/ockam_command/src/util/github.rs

etorreborre · 2023-07-04T15:02:34Z

implementations/rust/ockam/ockam_command/src/util/github.rs

+    if let Ok(r) = resp {
+        if let Ok(upgrade) = r.json::<UpgradeFile>().await {
+            if let Some(message) = upgrade.upgrade_message {
+                eprintln!("\n{}", message.yellow());


We need to use the opts.terminal instead of eprintln here.

This was also part of the original code that checked for the latest version from the version.json file on GitHub, I just moved it to this file. I can refactor this as part of this PR as well if you would like.

I could also rework this to not use the version.json file and instead just pull the latest version from the GitHub releases?

I can refactor this as part of this PR as well if you would like.

I see. Yes that would be nice. We are trying to move away from bare printlns in order to completely control the rendering in the terminal.

I could also rework this to not use the version.json file and instead just pull the latest version from the GitHub releases?

I don't understand that part. Are you talking about the upgrade.json file? I actually tried to download it from https://github.com/build-trust/ockam/releases/download/ockam_v0.90.0/upgrade.json and it comes back empty. Is that the same for you too? Because then this means that we also need to fix that part somewhere in our release process (FYI @metaclips)

Yeah exactly. I mentioned it in the description for this PR, but I found that the upgrade.json were never really up-to-date. You can play around with it by changing the version in the Cargo.toml file. Currently I only no suggestions for 0.89.0, 0.88.0 and 0.87.0. If I set it to 0.86.0 then it informs me that there is an update available, but that the latest version is 0.87.0.

Currently it pulls this information from the release of the version that the user is using, meaning these files would need to be updated every release to be effective.
That's why I ended up just pulling the latest version from the release instead of just using the version.json file.

implementations/rust/ockam/ockam_command/src/util/installer.rs

BradLewis · 2023-07-05T12:11:19Z

Hi @BradLewis I left a bunch of comments and suggestions for you to consider. Next I would like to test your PR a bit on my machine to double-check that it is also working for me. How have you tested it so far?

So for testing I've been setting the version to something lower, eg 0.88.0, building the project and then moving the binary to the location I would like it to be. For testing with brew, I replace the binary that brew installs, for the script installation, I replace the binary there and for the standalone installation I just run it from the cargo build directory.

From there I just make sure all the nodes have restarted and if it was installed via the script, that all the environment files were updated correctly.

I will say this is definitely one that needs to be tested thoroughly given all the changes it can make on the end users machine. I split the logic into 3 flows (installation via brew, installation via the install script, and someone just downloading the binary from the GitHub releases), and then tried to infer which of the 3 were used, but there might be missed edge cases there.

One thing I haven't been able to test is the upgrade via brew as I'm not sure how to install an old version. But the uninstall command works as expected.

implementations/rust/ockam/ockam_command/src/upgrade.rs

adrianbenavides · 2023-07-06T06:38:04Z

implementations/rust/ockam/ockam_command/src/upgrade.rs

-fn upgrade_check_is_disabled() -> bool {
-    get_env_with_default("OCKAM_DISABLE_UPGRADE_CHECK", false).unwrap_or(false)
+fn upgrade_ockam(opts: &CommandGlobalOpts) -> miette::Result<()> {
+    let stopped_nodes_names = stop_all_running_nodes(opts)?;


Wouldn't it be safer to move this at least after let installer = get_installer(opts)?;? Or, even better, right before restarting the nodes. I'm not sure what will happen to running processes after doing the upgrade (are they killed after removing the current binary version?).

So, if you run the upgrade without stopping the nodes, everything will still be running, so we could only do the restart cycle after the upgrade has run successfully just to ensure the nodes get upgraded to the latest version.

As an aside to this, with all the changes made with the nodes logic since the 0.90.0 release, the nodes will no longer be restarted correctly on this branch. I'm pretty sure this is just due to the nodes trying to be restarted with the 0.90.0 version after you run the upgrade? I created a new branch https://github.com/BradLewis/ockam/tree/bradlewis/upgrade-test that is just sitting on the latest release just to test this and everything is restarted as expected.

That's interesting... can you share more details on how to reproduce the restart problem?

Sure.

Create a node using ockam node create n using any version of ockam.

Change the version in the Cargo.toml file for ockam_command to something below 0.90.0.

Run cargo run upgrade --yes to "upgrade" the ockam binary to the latest version. This should just replace the one in target/debug/ockam with the latest one from github.

Everything appears to run smoothly, however if you run cargo run node list, you'll see that the node is stopped.

I'm pretty sure this is because it's trying to restart the nodes using the 0.90.0 release binary, but the "new" nodes are not compatible with that version.

If you run ockam node list you'll see that it thinks there are no nodes on the system

❯ ockam node list ┌───────────────────┐ │ Nodes │ └───────────────────┘ ! No nodes found on this system. ❯ cargo run node list Finished dev [unoptimized + debuginfo] target(s) in 0.20s Running `/Users/bradlewis/repos/ockam/target/debug/ockam node list` ┌───────────────────┐ │ Nodes │ └───────────────────┘ │ Node n (default) UP │ Process id 21821 │ Node m UP │ Process id 22503

You can also easily reproduce this behaviour by first creating a node with release 0.90.0, then a new one with cargo run and then listing the nodes

❯ ockam node create n Creating Node n... ✔︎ Node n created successfully To see more details on this node, run: ockam node show ❯ ockam node list ┌───────────────────┐ │ Nodes │ └───────────────────┘ │ Node n (default) UP │ Process id 26852 ❯ cargo run node create m Finished dev [unoptimized + debuginfo] target(s) in 0.20s Running `/Users/bradlewis/repos/ockam/target/debug/ockam node create m` Creating Node m... ✔︎ Node m created successfully To see more details on this node, run: ockam node show ❯ cargo run node list Finished dev [unoptimized + debuginfo] target(s) in 0.20s Running `/Users/bradlewis/repos/ockam/target/debug/ockam node list` ┌───────────────────┐ │ Nodes │ └───────────────────┘ │ Node n (default) UP │ Process id 26852 │ Node m UP │ Process id 27086 ❯ ockam node list ┌───────────────────┐ │ Nodes │ └───────────────────┘ ! No nodes found on this system.

Given all this I imagine what's happening is that when the nodes are trying to be restarted, they are trying to be restarted with the 0.90.0 binary, but the 0.90.0 binary doesn't see the nodes so nothing happens. Not sure why there isn't any error though.

If I run this all instead on my other branch I created, everything works as you would expect.

mrinalwadhwa reviewed Jun 26, 2023

View reviewed changes

install.sh Outdated Show resolved Hide resolved

tigercat2000 reviewed Jun 26, 2023

View reviewed changes

implementations/rust/ockam/ockam_command/src/util/installer.rs Outdated Show resolved Hide resolved

BradLewis force-pushed the bradlewis/implement-upgrade-command branch 4 times, most recently from 3418080 to c153f5c Compare June 27, 2023 21:43

BradLewis marked this pull request as ready for review June 28, 2023 22:45

BradLewis requested a review from a team as a code owner June 28, 2023 22:45

BradLewis force-pushed the bradlewis/implement-upgrade-command branch 4 times, most recently from 2b9ee3e to 78760ec Compare June 30, 2023 20:23

etorreborre reviewed Jul 4, 2023

View reviewed changes

BradLewis force-pushed the bradlewis/implement-upgrade-command branch from 78760ec to 8feee2a Compare July 4, 2023 23:21

BradLewis changed the title ~~feature(rust): Implement upgrade and uninstall commands~~ feat(rust): Implement upgrade and uninstall commands Jul 5, 2023

BradLewis force-pushed the bradlewis/implement-upgrade-command branch 2 times, most recently from 9f82eeb to f8f4e2a Compare July 5, 2023 11:42

adrianbenavides reviewed Jul 6, 2023

View reviewed changes

implementations/rust/ockam/ockam_command/src/upgrade.rs Show resolved Hide resolved

adrianbenavides reviewed Jul 6, 2023

View reviewed changes

BradLewis force-pushed the bradlewis/implement-upgrade-command branch 2 times, most recently from f9dd663 to ded8afc Compare July 16, 2023 12:25

BradLewis force-pushed the bradlewis/implement-upgrade-command branch from ded8afc to 991c88d Compare July 29, 2023 23:27

adrianbenavides self-requested a review August 18, 2023 07:35

BradLewis added 4 commits September 10, 2023 15:32

feat(rust): implement upgrade and uninstall commands

3a16bcb

feat(rust): pull install script from github and backup existing binary

be728d0

refactor(rust): refactor implementation and improve error management

e9b4a7e

fix(rust): improve accuracy of brew installation check

1718ba2

BradLewis added 5 commits September 10, 2023 15:32

feat(rust): add direct binary installer

fb44f65

refactor(rust): address pr comments

17ad25d

refactor(rust): switch eprintlns to use opts.terminal

6d32c2a

fix(rust): fix rebase conflicts

a56bb35

fix(rust): fix clippy warnings

ee3a09e

BradLewis force-pushed the bradlewis/implement-upgrade-command branch from 991c88d to ee3a09e Compare September 10, 2023 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust): Implement upgrade and uninstall commands #5171

feat(rust): Implement upgrade and uninstall commands #5171

BradLewis commented Jun 26, 2023 •

edited

Loading

mrinalwadhwa commented Jun 26, 2023

BradLewis commented Jun 27, 2023 •

edited

Loading

etorreborre left a comment

etorreborre Jul 4, 2023

BradLewis Jul 4, 2023 •

edited

Loading

etorreborre Jul 5, 2023

etorreborre Jul 5, 2023

BradLewis Jul 5, 2023

BradLewis commented Jul 5, 2023 •

edited

Loading

adrianbenavides Jul 6, 2023

BradLewis Jul 6, 2023 •

edited

Loading

adrianbenavides Jul 6, 2023

BradLewis Jul 6, 2023 •

edited

Loading

feat(rust): Implement upgrade and uninstall commands #5171

Are you sure you want to change the base?

feat(rust): Implement upgrade and uninstall commands #5171

Conversation

BradLewis commented Jun 26, 2023 • edited Loading

Current behavior

Proposed changes

Upgrade

Uninstall

Checks

mrinalwadhwa commented Jun 26, 2023

BradLewis commented Jun 27, 2023 • edited Loading

etorreborre left a comment

Choose a reason for hiding this comment

etorreborre Jul 4, 2023

Choose a reason for hiding this comment

BradLewis Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

etorreborre Jul 5, 2023

Choose a reason for hiding this comment

etorreborre Jul 5, 2023

Choose a reason for hiding this comment

BradLewis Jul 5, 2023

Choose a reason for hiding this comment

BradLewis commented Jul 5, 2023 • edited Loading

adrianbenavides Jul 6, 2023

Choose a reason for hiding this comment

BradLewis Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

adrianbenavides Jul 6, 2023

Choose a reason for hiding this comment

BradLewis Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

BradLewis commented Jun 26, 2023 •

edited

Loading

BradLewis commented Jun 27, 2023 •

edited

Loading

BradLewis Jul 4, 2023 •

edited

Loading

BradLewis commented Jul 5, 2023 •

edited

Loading

BradLewis Jul 6, 2023 •

edited

Loading

BradLewis Jul 6, 2023 •

edited

Loading