feat: force and upgrade by url or version

Three new arguments are added to the `upgrade` command: `--force`, `--url` and `--version`.

The `--url` and `--version` arguments provide two different sources for the upgrade, as opposed to
just upgrading to the latest version. With `--url`, a custom binary can be provided, which will be
used for the backwards compatibility test. The `--version` flag enables upgrading to a specific
version rather than the latest. Both of these can be used with the `--force` flag to downgrade to an
arbitrary version or to accept an upgrade from a binary with the same version, the latter of which
will again be used in the backwards compatibility test.

Integration tests provide coverage of these new features. Each of the tests really needs to run on
its own machine, otherwise they interfere with each other, and tests can't make assumptions about
how many services there are. So we add twelve additional jobs to the merge workflow here, which is
for four tests on three operating systems. However, these tests should run pretty quickly.

The `stop` command was modified such that it will no longer return an error if the service is in the
`ADDED` state, i.e., it has not been started before. This enables us to test the upgrade process
without initially starting the service, which could introduce complications. The `get_safenode_port`
function was also changed to return an `Option` rather than a `Result` for the same reason.

.github/workflows/merge.yml

@@ -146,8 +146,8 @@ jobs:
       - shell: bash
         run: cargo test --release --bin safenode-manager
 
-  node-manager-integration-tests:
-    name: node manager integration tests
+  node-manager-e2e-tests:
+    name: node manager e2e tests
     runs-on: ${{ matrix.os }}
     strategy:
       fail-fast: false
@@ -169,7 +169,7 @@ jobs:
           ${{ matrix.elevated }} rustup default stable
           ${{ matrix.elevated }} cargo test --package sn-node-manager --release --test e2e -- --nocapture
 
-      # A simple test seemed to confirm that the Powershell step runs as admin by default.
+      # Powershell step runs as admin by default.
       - name: run integration test in powershell
         if: matrix.os == 'windows-latest'
         shell: pwsh
@@ -182,6 +182,54 @@ jobs:
 
           cargo test --release --package sn-node-manager --test e2e -- --nocapture
 
+  # Each upgrade test needs its own VM, otherwise they will interfere with each other.
+  node-manager-upgrade-tests:
+    name: node manager upgrade tests
+    runs-on: ${{ matrix.os }}
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - { os: ubuntu-latest, elevated: sudo env PATH="$PATH", test: upgrade_to_latest_version }
+          - { os: ubuntu-latest, elevated: sudo env PATH="$PATH", test: force_upgrade_when_two_binaries_have_the_same_version }
+          - { os: ubuntu-latest, elevated: sudo env PATH="$PATH", test: force_downgrade_to_a_previous_version }
+          - { os: ubuntu-latest, elevated: sudo env PATH="$PATH", test: upgrade_from_older_version_to_specific_version }
+          - { os: macos-latest, elevated: sudo, test: upgrade_to_latest_version }
+          - { os: macos-latest, elevated: sudo, test: force_upgrade_when_two_binaries_have_the_same_version }
+          - { os: macos-latest, elevated: sudo, test: force_downgrade_to_a_previous_version }
+          - { os: macos-latest, elevated: sudo, test: upgrade_from_older_version_to_specific_version }
+          - { os: windows-latest, test: upgrade_to_latest_version }
+          - { os: windows-latest, test: force_upgrade_when_two_binaries_have_the_same_version }
+          - { os: windows-latest, test: force_downgrade_to_a_previous_version }
+          - { os: windows-latest, test: upgrade_from_older_version_to_specific_version }
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Install Rust
+        uses: dtolnay/rust-toolchain@stable
+      - uses: Swatinem/rust-cache@v2
+
+      - shell: bash
+        if: matrix.os == 'ubuntu-latest' || matrix.os == 'macos-latest'
+        run: |
+          ${{ matrix.elevated }} rustup default stable
+          ${{ matrix.elevated }} cargo test --package sn-node-manager --release \
+            --test upgrades ${{ matrix.test }} -- --nocapture
+
+      # Powershell step runs as admin by default.
+      - name: run integration test in powershell
+        if: matrix.os == 'windows-latest'
+        shell: pwsh
+        run: |
+          curl -L -o WinSW.exe $env:WINSW_URL
+
+          New-Item -ItemType Directory -Force -Path "$env:GITHUB_WORKSPACE\bin"
+          Move-Item -Path WinSW.exe -Destination "$env:GITHUB_WORKSPACE\bin"
+          $env:PATH += ";$env:GITHUB_WORKSPACE\bin"
+
+          cargo test --package sn-node-manager --release `
+            --test upgrades ${{ matrix.test }} -- --nocapture
+
   e2e:
     if: "!startsWith(github.event.head_commit.message, 'chore(release):')"
     name: E2E tests

sn_node_manager/Cargo.toml

@@ -55,4 +55,5 @@ assert_fs = "1.0.13"
 assert_matches = "1.5.0"
 async-trait = "0.1"
 mockall = "0.11.3"
+reqwest = { version = "0.11", default-features = false, features = ["json", "rustls-tls"] }
 predicates = "2.0"

sn_node_manager/src/control.rs

@@ -19,6 +19,7 @@ use sn_protocol::node_registry::{Node, NodeRegistry, NodeStatus};
 use std::path::PathBuf;
 
 pub enum UpgradeResult {
+    Forced(String, String),
     NotRequired,
     Upgraded(String, String),
     Error(String),
@@ -77,10 +78,13 @@ pub async fn start(
 
 pub async fn stop(node: &mut Node, service_control: &dyn ServiceControl) -> Result<()> {
     match node.status {
-        NodeStatus::Added => Err(eyre!(
-            "Service {} has not been started since it was installed",
-            node.service_name
-        )),
+        NodeStatus::Added => {
+            println!(
+                "Service {} has not been started since it was installed",
+                node.service_name
+            );
+            Ok(())
+        }
         NodeStatus::Removed => Err(eyre!("Service {} has been removed", node.service_name)),
         NodeStatus::Running => {
             let pid = node.pid.unwrap();
@@ -278,46 +282,63 @@ pub async fn remove(
     Ok(())
 }
 
+pub struct UpgradeOptions {
+    pub bootstrap_peers: Vec<Multiaddr>,
+    pub env_variables: Option<Vec<(String, String)>>,
+    pub force: bool,
+    pub start_node: bool,
+    pub target_safenode_path: PathBuf,
+    pub target_version: Version,
+}
+
 pub async fn upgrade(
+    options: UpgradeOptions,
     node: &mut Node,
-    bootstrap_peers: &[Multiaddr],
-    env_variables: &Option<Vec<(String, String)>>,
-    upgraded_safenode_path: &PathBuf,
-    latest_version: &Version,
     service_control: &dyn ServiceControl,
     rpc_client: &dyn RpcActions,
 ) -> Result<UpgradeResult> {
     let current_version = Version::parse(&node.version)?;
-    if current_version == *latest_version {
+    if !options.force
+        && (current_version == options.target_version || options.target_version < current_version)
+    {
         return Ok(UpgradeResult::NotRequired);
     }
 
     stop(node, service_control).await?;
-    std::fs::copy(upgraded_safenode_path, &node.safenode_path)?;
+    std::fs::copy(options.target_safenode_path, &node.safenode_path)?;
 
     // Install the service again to make sure we re-use the same node port.
-    // Initially the node_port is generated on random. We obtain this port from the Node::listen_addr field to be re-used.
+    // Windows requires that the service be uninstalled first.
+    service_control.uninstall(&node.service_name.clone())?;
     service_control.install(ServiceConfig {
         local: node.local,
         data_dir_path: node.data_dir_path.clone(),
         genesis: node.genesis,
         name: node.service_name.clone(),
-        node_port: Some(node.get_safenode_port()?),
-        bootstrap_peers: bootstrap_peers.to_owned(),
+        node_port: node.get_safenode_port(),
+        bootstrap_peers: options.bootstrap_peers,
         rpc_socket_addr: node.rpc_socket_addr,
         log_dir_path: node.log_dir_path.clone(),
         safenode_path: node.safenode_path.clone(),
         service_user: node.user.clone(),
-        env_variables: env_variables.clone(),
+        env_variables: options.env_variables.clone(),
     })?;
 
-    start(node, service_control, rpc_client, VerbosityLevel::Normal).await?;
-    node.version = latest_version.to_string();
+    if options.start_node {
+        start(node, service_control, rpc_client, VerbosityLevel::Normal).await?;
+    }
+    node.version = options.target_version.to_string();
 
-    Ok(UpgradeResult::Upgraded(
-        current_version.to_string(),
-        latest_version.to_string(),
-    ))
+    match options.force {
+        true => Ok(UpgradeResult::Forced(
+            current_version.to_string(),
+            options.target_version.to_string(),
+        )),
+        false => Ok(UpgradeResult::Upgraded(
+            current_version.to_string(),
+            options.target_version.to_string(),
+        )),
+    }
 }
 
 fn format_status(status: &NodeStatus) -> String {
@@ -681,7 +702,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn stop_should_return_error_for_attempt_to_stop_installed_service() -> Result<()> {
+    async fn stop_should_not_return_error_for_attempt_to_stop_installed_service() -> Result<()> {
         let mock_service_control = MockServiceControl::new();
 
         let mut node = Node {
@@ -705,16 +726,15 @@ mod tests {
         let result = stop(&mut node, &mock_service_control).await;
 
         match result {
-            Ok(()) => panic!("This test should result in an error"),
-            Err(e) => {
-                assert_eq!(
-                    "Service safenode1 has not been started since it was installed",
-                    e.to_string()
+            Ok(()) => Ok(()),
+            Err(_) => {
+                panic!(
+                    "The stop command should be idempotent and do nothing for a stopped service"
                 );
             }
         }
 
-        Ok(())
+        // Ok(())
     }
 
     #[tokio::test]