Merge branch 'zoran/CON-1035-divergence-check2' into 'master'

fix(ic-backup, CON-1035) Improve more divergence check in order to reduce flakiness

There was a flakiness that the test would pick the first checkpoint and modify it's memory in order to produce divergence. In a rare occasion that there is more than one checkpoint, it might pick the one that was already replayed and the test would fail. This change makes it pick the last one if there are more, and it is the one from which the replay will start.

Closes CON-1035 

Closes CON-1035

See merge request dfinity-lab/public/ic!12577

Author: zoran-dfinity

rs/backup/src/backup_helper.rs

@@ -901,7 +901,7 @@ fn last_dir_height(dir: &PathBuf, radix: u32) -> u64 {
     }
 }
 
-fn last_checkpoint(dir: &Path) -> u64 {
+pub fn last_checkpoint(dir: &Path) -> u64 {
     last_dir_height(&dir.join("checkpoints"), 16)
 }
 

rs/tests/src/orchestrator/backup_manager.rs

@@ -42,7 +42,7 @@ use crate::{
     },
     util::{block_on, get_nns_node},
 };
-use ic_backup::backup_helper::ls_path;
+use ic_backup::backup_helper::{last_checkpoint, ls_path};
 use ic_backup::config::{ColdStorage, Config, SubnetConfig};
 use ic_backup::util::sleep_secs;
 use ic_base_types::SubnetId;
@@ -77,7 +77,7 @@ pub fn config(env: TestEnv) {
 pub fn test(env: TestEnv) {
     let log = env.logger();
 
-    // Create all directories
+    info!(log, "Create all directories");
     let root_dir = tempfile::TempDir::new()
         .expect("failed to create a temporary directory")
         .path()
@@ -91,6 +91,7 @@ pub fn test(env: TestEnv) {
         .path()
         .to_path_buf();
 
+    info!(log, "Fetch the replica version");
     let nns_node = get_nns_node(&env.topology_snapshot());
     let node_ip: IpAddr = nns_node.get_ip_addr();
     let subnet_id = env.topology_snapshot().root_subnet_id();
@@ -99,6 +100,10 @@ pub fn test(env: TestEnv) {
     let initial_replica_version = ReplicaVersion::try_from(replica_version.clone())
         .expect("Assigned replica version should be valid");
 
+    info!(
+        log,
+        "Copy the binaries needed for replay of the current version"
+    );
     let backup_binaries_dir = backup_dir.join("binaries").join(&replica_version);
     fs::create_dir_all(&backup_binaries_dir).expect("failure creating backup binaries directory");
 
@@ -109,6 +114,10 @@ pub fn test(env: TestEnv) {
     copy_file(&binaries_path, &backup_binaries_dir, "sandbox_launcher");
     copy_file(&binaries_path, &backup_binaries_dir, "canister_sandbox");
 
+    info!(
+        log,
+        "Download the binaries needed for replay of the mainnet version"
+    );
     let mainnet_version = env
         .read_dependency_to_string("testnet/mainnet_nns_revision.txt")
         .expect("could not read mainnet version!");
@@ -130,6 +139,7 @@ pub fn test(env: TestEnv) {
         .expect("chmod command failed");
     chmod.wait_with_output().expect("chmod execution failed");
 
+    info!(log, "Run ECDSA signature test");
     let nns_node = env.get_first_healthy_nns_node_snapshot();
     let agent = nns_node.build_default_agent();
     let nns_canister = block_on(MessageCanister::new(
@@ -146,6 +156,7 @@ pub fn test(env: TestEnv) {
     );
     run_ecdsa_signature_test(&nns_canister, &log, key);
 
+    info!(log, "Install universal canister");
     let log2 = log.clone();
     let id = nns_node.effective_canister_id();
     let canister_id_hex: String = block_on({
@@ -155,13 +166,13 @@ pub fn test(env: TestEnv) {
         }
     });
 
-    // Update the registry with the backup key
+    info!(log, "Update the registry with the backup key");
     let payload = get_updatesubnetpayload_with_keys(subnet_id, None, Some(vec![backup_public_key]));
     block_on(update_subnet_record(nns_node.get_public_url(), payload));
     let backup_mean = AuthMean::PrivateKey(backup_private_key);
     wait_until_authentication_is_granted(&node_ip, "backup", &backup_mean);
 
-    // Fetch NNS public key
+    info!(log, "Fetch NNS public key");
     let nns_public_key = env
         .prep_dir("")
         .expect("missing NNS public key")
@@ -229,6 +240,7 @@ pub fn test(env: TestEnv) {
         &log,
     ));
 
+    info!(log, "Wait for archived checkpoint");
     let archive_dir = backup_dir.join("archive").join(subnet_id.to_string());
     // make sure we have some archive of the old version before upgrading to the new one
     loop {
@@ -305,6 +317,7 @@ pub fn test(env: TestEnv) {
     info!(log, "Modify memory file: {:?}", memory_artifact_path);
     modify_byte_in_file(memory_artifact_path).expect("Modifying a byte failed");
 
+    info!(log, "Start again the backup process in a separate thread");
     let mut command = Command::new(&ic_backup_path);
     command
         .arg("--config-file")
@@ -317,10 +330,12 @@ pub fn test(env: TestEnv) {
         .expect("Failed to start backup process");
     info!(log, "Started process: {}", child.id());
 
-    assert!(cold_storage_exists(
-        &log,
-        cold_storage_dir.join(subnet_id.to_string())
-    ));
+    if !cold_storage_exists(&log, cold_storage_dir.join(subnet_id.to_string())) {
+        info!(log, "Kill child process");
+        child.kill().expect("Error killing backup process");
+        panic!("No cold storage");
+    }
+
     info!(log, "Artifacts and states are moved to cold storage");
 
     let mut hash_mismatch = false;
@@ -365,17 +380,15 @@ fn some_checkpoint_dir(backup_dir: &Path, subnet_id: &SubnetId) -> Option<PathBu
     let dir = backup_dir
         .join("data")
         .join(subnet_id.to_string())
-        .join("ic_state")
-        .join("checkpoints");
+        .join("ic_state");
     if !dir.exists() {
         return None;
     }
-    if let Ok(mut cps) = fs::read_dir(dir) {
-        if let Some(Ok(cp)) = cps.next() {
-            return Some(cp.path());
-        }
+    let lcp = last_checkpoint(&dir);
+    if lcp == 0 {
+        return None;
     }
-    None
+    Some(dir.join(format!("checkpoints/{:016x}", lcp)))
 }
 
 fn modify_byte_in_file(file_path: PathBuf) -> std::io::Result<()> {