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.

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

Fix/race condition for backup canisters #497

Merged
merged 4 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/canister/platform_orchestrator/can.did
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ service : (PlatformOrchestratorInitArgs) -> {
deposit_cycles_to_canister : (principal, nat) -> (Result_1);
deregister_subnet_orchestrator : (principal, bool) -> ();
download_snapshot : (nat64, nat64) -> (blob) query;
fixup_individual_cainsters_in_thebreaking_condition_network : () -> ();
fixup_individual_canisters_in_a_subnet : (principal) -> (Result);
get_all_available_subnet_orchestrators : () -> (vec principal) query;
get_all_global_admins : () -> (vec principal) query;
get_all_subnet_orchestrators : () -> (vec principal) query;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
use ic_cdk_macros::update;
use shared_utils::common::utils::task::run_task_concurrently;

use crate::{guard::is_caller::is_caller_global_admin_or_controller, utils::registered_subnet_orchestrator::RegisteredSubnetOrchestrator, CANISTER_DATA};


#[update(guard = "is_caller_global_admin_or_controller")]
async fn fixup_individual_cainsters_in_thebreaking_condition_network() {
let subnet_orchestrators = CANISTER_DATA.with_borrow(|canister_data| canister_data.all_subnet_orchestrator_canisters_list.clone().into_iter());
let fixup_individual_canisters_in_subnet_futures = subnet_orchestrators.map(|subnet_orchestrator| async move {
let registered_subnetorchestrator = RegisteredSubnetOrchestrator::new(subnet_orchestrator)?;
registered_subnetorchestrator.fixup_individual_cansiters_mapping().await
});

run_task_concurrently(fixup_individual_canisters_in_subnet_futures, 10, |_| {}, || false).await;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
use candid::Principal;
use ic_cdk::update;

use crate::{guard::is_caller::is_caller_global_admin_or_controller, utils::registered_subnet_orchestrator::RegisteredSubnetOrchestrator};

#[update(guard = "is_caller_global_admin_or_controller")]
async fn fixup_individual_canisters_in_a_subnet(subnet_orchestrator: Principal) -> Result<(), String>{
let resgistered_subnet_orchestrator = RegisteredSubnetOrchestrator::new(subnet_orchestrator)?;

resgistered_subnet_orchestrator.fixup_individual_cansiters_mapping().await

}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ pub mod upgrade_canisters_in_network;
mod upgrade_individual_canisters_in_a_subnet_with_latest_wasm;
mod upgrade_specific_individual_canister;
pub mod upload_wasms;
pub mod fixup_individual_canisters_in_a_subnet;
pub mod fixup_individual_cainsters_in_the_network;

#[query]
pub fn get_version() -> String {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ impl RegisteredSubnetOrchestrator {
res
}

pub async fn fixup_individual_cansiters_mapping(&self) -> Result<(), String> {
ic_cdk::call::<_, ()>(self.canister_id, "fixup_individual_canisters_mapping", ()).await.map_err(|e| e.1)
}

pub async fn make_individual_canister_logs_private(
&self,
individual_canister_id: Principal,
Expand Down
1 change: 1 addition & 0 deletions src/canister/user_index/can.did
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ service : (UserIndexInitArgs) -> {
Result_3,
);
download_snapshot : (nat64, nat64) -> (blob) query;
fixup_individual_canisters_mapping : () -> ();
get_current_list_of_all_well_known_principal_values : () -> (
vec record { KnownPrincipalType; principal },
) query;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
use candid::Principal;
use ic_cdk_macros::update;
use shared_utils::common::utils::{permissions::is_caller_controller, task::run_task_concurrently};

use crate::CANISTER_DATA;

#[update(guard = "is_caller_controller")]
pub async fn fixup_individual_canisters_mapping() {

let user_canisters = CANISTER_DATA.with_borrow(|canister_data| canister_data.user_principal_id_to_canister_id_map.clone().into_iter());
let user_canisters_futures = user_canisters.map(|(user_principal, user_canister)| async move {
let result = ic_cdk::call::<_, (String,)>(user_canister, "get_version", ()).await;
if let Err(e) = result {
//IC0536 is the code for invalid method name. If the canister does not have get_version that means it has a wams installed that does not correspond to individual wasm.
if e.1.contains("IC0536") {
Err(user_principal)
} else {
Ok(())
}
} else {
Ok(())
}
});

let result_callback = |res: Result<(), Principal>| {
if let Err(e) = res {
CANISTER_DATA.with_borrow_mut(|canister_data| {
canister_data.user_principal_id_to_canister_id_map.remove(&e);
})
}
};

ic_cdk::spawn(run_task_concurrently(user_canisters_futures, 10, result_callback, || false));

let available_canisters = CANISTER_DATA.with_borrow(|canister_data| {
canister_data.available_canisters.clone()
});



let available_canisters_futures = available_canisters.into_iter().map(|available_canister| async move {
let result = ic_cdk::call::<_, (String,)>(available_canister, "get_version", ()).await;
if let Err(e) = result {
if e.1.contains("IC0536") {
Err(available_canister)
} else {
Ok(())
}
} else {
Ok(())
}
});



let result_callback = |res: Result<(), Principal>| {
if let Err(e) = res {
CANISTER_DATA.with_borrow_mut(|canister_data| {
canister_data.available_canisters.remove(&e);
})
}
};

ic_cdk::spawn(run_task_concurrently(available_canisters_futures, 10, result_callback, || false));
}
2 changes: 2 additions & 0 deletions src/canister/user_index/src/api/canister_management/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ pub mod start_upgrades_for_individual_canisters;
pub mod update_canisters_access_time;
pub mod update_user_canister_restart_timers;
pub mod upgrade_all_creator_dao_governance_canisters_in_the_network;
pub mod fixup_individual_canisters_mappings;


#[update]
pub async fn get_user_canister_status(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ async fn new_user_signup(user_id: Principal) -> Result<Principal, String> {
.with_borrow_mut(|canister_data| {
let mut available_canisters = canister_data.available_canisters.iter().cloned();
let canister_id = available_canisters.next();
canister_data.available_canisters = available_canisters.collect();
canister_data.available_canisters = available_canisters.collect(); // available canisters
canister_id
})
.ok_or("Not Available".into());
Expand Down Expand Up @@ -197,21 +197,18 @@ async fn provision_new_available_canisters(individual_user_template_canister_was
CANISTER_DATA.with_borrow(|canister_data| canister_data.available_canisters.len() as u64);
let max_canister_count = available_canister_count + canister_count;

let mut backup_pool_canister = CANISTER_DATA
.with_borrow(|canister_data| canister_data.backup_canister_pool.clone())
.into_iter();

let install_canister_wasm_futures = (0..canister_count).map(move |_| {
let individual_user_template_canister_wasm_version =
individual_user_template_canister_wasm.version.clone();

let individual_user_template_canister_wasm =
individual_user_template_canister_wasm.wasm_blob.clone();

let canister_id = backup_pool_canister.next().unwrap();
let canister_id = CANISTER_DATA.with_borrow_mut(|canister_data| {
let canister_id = canister_data.backup_canister_pool.iter().next().copied().unwrap();
canister_data.backup_canister_pool.remove(&canister_id);
canister_id

CANISTER_DATA.with_borrow_mut(|canister_data| {
canister_data.backup_canister_pool.remove(&canister_id)
});

async move {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,47 @@ pub fn provision_new_available_and_backup_canisters_on_signup_if_required() {
.unwrap();

assert!(subnet_available_canister_cnt > 0);

/*** Test fixup should not remove valid canisters */

pocket_ic
.update_call(
subnet_orchestrator_canister_id,
platform_canister_id,
"fixup_individual_canisters_mapping",
candid::encode_one(()).unwrap(),
)
.map(|res| {
let result: () = match res {
WasmResult::Reply(payload) => candid::decode_one(&payload).unwrap(),
_ => panic!("get subnet available capacity call failed"),
};
result
})
.unwrap();

for _ in 0..110 {
pocket_ic.tick();
}

let subnet_available_cnt_after_fixup = pocket_ic
.query_call(
subnet_orchestrator_canister_id,
Principal::anonymous(),
"get_subnet_available_capacity",
candid::encode_one(()).unwrap(),
)
.map(|res| {
let available_capacity: u64 = match res {
WasmResult::Reply(payload) => candid::decode_one(&payload).unwrap(),
_ => panic!("get subnet available capacity call failed"),
};
available_capacity
})
.unwrap();

assert_eq!(
subnet_available_cnt_after_fixup,
subnet_available_canister_cnt
)
}