From 0923aa8f3ea2e2a251e3bfb1525442215b06b2b3 Mon Sep 17 00:00:00 2001 From: Dragoljub Djuric Date: Wed, 19 Feb 2025 23:18:53 +0100 Subject: [PATCH] fix: EXC-1889 Hook condition should be checked after every mgmt canister call (#3988) This PR ensures that the condition for a low-wasm memory hook will be met after every mgmt canister call. Since we already have tests that condition is checked after Updating settings and Installing code, we will only add tests that snapshot operations influence hook in the floow-up https://github.com/dfinity/ic/pull/4023/files. --- .../src/canister_manager.rs | 3 - .../src/canister_manager/tests.rs | 154 +++++++----------- .../src/execution_environment.rs | 144 ++++++++++------ .../src/execution_environment/tests.rs | 2 +- .../canister_state/system_state/task_queue.rs | 6 - 5 files changed, 161 insertions(+), 148 deletions(-) diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 95b035a5d4b..9098699b225 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -643,9 +643,6 @@ impl CanisterManager { if let Some(wasm_memory_limit) = settings.wasm_memory_limit() { canister.system_state.wasm_memory_limit = Some(wasm_memory_limit); } - // Change of `wasm_memory_limit` or `new_wasm_memory_threshold` or `memory_allocation`, - // can influence the satisfaction of the condition for `low_wasm_memory` hook. - canister.update_on_low_wasm_memory_hook_condition(); } /// Tries to apply the requested settings on the canister identified by diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index 23adbfb557f..34fe19ac8f1 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -8128,118 +8128,88 @@ fn helper_update_settings_updates_hook_status( updated_memory_state: MemoryState, execute_hook_after_install: bool, ) { - with_setup(|canister_manager, mut state, subnet_id| { - let mut round_limits = RoundLimits { - instructions: as_round_instructions(EXECUTION_PARAMETERS.instruction_limits.message()), - subnet_available_memory: (*MAX_SUBNET_AVAILABLE_MEMORY), - subnet_available_callbacks: SUBNET_CALLBACK_SOFT_LIMIT as i64, - compute_allocation_used: state.total_compute_allocation(), - }; + let mut test = ExecutionTestBuilder::new().build(); - let wat = format!("(module (memory {used_wasm_memory_pages}))"); + let mut initial_settings = CanisterSettingsArgsBuilder::new(); - let wasm = wat::parse_str(wat).unwrap(); + if let Some(MemoryAllocation::Reserved(memory_allocation)) = + initial_memory_state.memory_allocation + { + initial_settings = initial_settings.with_memory_allocation(memory_allocation.get()); + } - let sender = canister_test_id(100).get(); + if let Some(wasm_memory_limit) = initial_memory_state.wasm_memory_limit { + initial_settings = initial_settings.with_wasm_memory_limit(wasm_memory_limit.get()); + } - let initial_settings = CanisterSettings { - wasm_memory_limit: initial_memory_state.wasm_memory_limit, - wasm_memory_threshold: initial_memory_state.wasm_memory_threshold, - memory_allocation: initial_memory_state.memory_allocation, - ..Default::default() - }; + if let Some(wasm_memory_threshold) = initial_memory_state.wasm_memory_threshold { + initial_settings = initial_settings.with_wasm_memory_threshold(wasm_memory_threshold.get()); + } - let canister_id = canister_manager - .create_canister( - canister_change_origin_from_principal(&sender), - subnet_id, - *INITIAL_CYCLES, - initial_settings, - MAX_NUMBER_OF_CANISTERS, - &mut state, - SMALL_APP_SUBNET_MAX_SIZE, - &mut round_limits, - ResourceSaturation::default(), - &no_op_counter(), - ) - .0 - .unwrap(); + let canister_id = test + .create_canister_with_settings(*INITIAL_CYCLES, initial_settings.build()) + .unwrap(); - let res = install_code( - &canister_manager, - InstallCodeContext { - origin: canister_change_origin_from_principal(&sender), - canister_id, - wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), - arg: vec![], - compute_allocation: None, - memory_allocation: None, - mode: CanisterInstallModeV2::Install, - }, - &mut state, - &mut round_limits, - ); - assert!(res.1.is_ok(), "{:?}", res.1); + let wat = format!("(module (memory {used_wasm_memory_pages}))"); - let mut c_state = res.2.unwrap(); + let wasm = wat::parse_str(wat).unwrap(); - if execute_hook_after_install { - c_state.system_state.task_queue.pop_front(); - } + test.install_canister(canister_id, wasm).unwrap(); - assert_eq!( - c_state.system_state.task_queue.peek_hook_status(), - initial_memory_state.hook_status - ); + if execute_hook_after_install { + test.canister_state_mut(canister_id) + .system_state + .task_queue + .pop_front(); + } - state.put_canister_state(c_state); + assert_eq!( + test.canister_state_mut(canister_id) + .system_state + .task_queue + .peek_hook_status(), + initial_memory_state.hook_status + ); - let mut settings = CanisterSettingsBuilder::new(); + let mut settings = CanisterSettingsArgsBuilder::new(); - if updated_memory_state.wasm_memory_threshold != initial_memory_state.wasm_memory_threshold - { - if let Some(updated_wasm_memory_threshold) = updated_memory_state.wasm_memory_threshold - { - settings = settings.with_wasm_memory_threshold(updated_wasm_memory_threshold); - } + if updated_memory_state.wasm_memory_threshold != initial_memory_state.wasm_memory_threshold { + if let Some(updated_wasm_memory_threshold) = updated_memory_state.wasm_memory_threshold { + settings = settings.with_wasm_memory_threshold(updated_wasm_memory_threshold.get()); } + } - if updated_memory_state.wasm_memory_limit != initial_memory_state.wasm_memory_limit { - if let Some(updated_wasm_memory_limit) = updated_memory_state.wasm_memory_limit { - settings = settings.with_wasm_memory_limit(updated_wasm_memory_limit); - } + if updated_memory_state.wasm_memory_limit != initial_memory_state.wasm_memory_limit { + if let Some(updated_wasm_memory_limit) = updated_memory_state.wasm_memory_limit { + settings = settings.with_wasm_memory_limit(updated_wasm_memory_limit.get()); } + } - if updated_memory_state.memory_allocation != initial_memory_state.memory_allocation { - if let Some(updated_memory_allocation) = updated_memory_state.memory_allocation { - settings = settings.with_memory_allocation(updated_memory_allocation); - } + if updated_memory_state.memory_allocation != initial_memory_state.memory_allocation { + if let Some(MemoryAllocation::Reserved(updated_memory_allocation)) = + updated_memory_state.memory_allocation + { + settings = settings.with_memory_allocation(updated_memory_allocation.get()); } + } - let canister = state.canister_state_mut(&canister_id).unwrap(); + let payload = UpdateSettingsArgs { + canister_id: canister_id.get(), + settings: settings.build(), + sender_canister_version: None, + } + .encode(); - assert!(canister_manager - .update_settings( - Time::from_nanos_since_unix_epoch(777), - canister_change_origin_from_principal(&sender), - settings.build(), - canister, - &mut round_limits, - ResourceSaturation::default(), - SMALL_APP_SUBNET_MAX_SIZE, - ) - .is_ok()); + test.subnet_message(Method::UpdateSettings, payload) + .unwrap(); - assert_eq!( - state - .canister_state_mut(&canister_id) - .unwrap() - .system_state - .task_queue - .peek_hook_status(), - updated_memory_state.hook_status - ); - }) + assert_eq!( + test.canister_state_mut(canister_id) + .system_state + .task_queue + .peek_hook_status(), + updated_memory_state.hook_status + ); } #[test] diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index 862365fd629..01899dc5dd5 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -154,7 +154,7 @@ pub enum ExecuteMessageResult { enum ExecuteSubnetMessageResult { Processing, Finished { - response: Result, UserError>, + response: Result<(Vec, Option), UserError>, refund: Cycles, }, } @@ -737,7 +737,7 @@ impl ExecutionEnvironment { &mut state, &self.metrics.canister_not_found_error, ) - .map(|()| EmptyBlob.encode()) + .map(|()| (EmptyBlob.encode(), Some(args.get_canister_id()))) .map_err(|err| err.into()) }); ExecuteSubnetMessageResult::Finished { @@ -758,15 +758,17 @@ impl ExecutionEnvironment { let result = match CanisterSettings::try_from(args.settings) { Err(err) => Err(err.into()), - Ok(settings) => self.update_settings( - timestamp_nanos, - msg.canister_change_origin(sender_canister_version), - settings, - canister_id, - &mut state, - round_limits, - registry_settings.subnet_size, - ), + Ok(settings) => self + .update_settings( + timestamp_nanos, + msg.canister_change_origin(sender_canister_version), + settings, + canister_id, + &mut state, + round_limits, + registry_settings.subnet_size, + ) + .map(|res| (res, Some(canister_id))), }; // The induction cost of `UpdateSettings` is charged // after applying the new settings to allow users to @@ -824,6 +826,7 @@ impl ExecutionEnvironment { &mut state, registry_settings.subnet_size, ) + .map(|res| (res, Some(args.get_canister_id()))) }); ExecuteSubnetMessageResult::Finished { response: res, @@ -839,6 +842,7 @@ impl ExecutionEnvironment { record.num_requested_changes(), &state, ) + .map(|res| (res, Some(record.canister_id()))) }); ExecuteSubnetMessageResult::Finished { response: res, @@ -853,6 +857,7 @@ impl ExecutionEnvironment { Ok(Ic00Method::StartCanister) => { let res = CanisterIdRecord::decode(payload).and_then(|args| { self.start_canister(args.get_canister_id(), *msg.sender(), &mut state) + .map(|res| (res, Some(args.get_canister_id()))) }); ExecuteSubnetMessageResult::Finished { response: res, @@ -876,7 +881,7 @@ impl ExecutionEnvironment { let result = self .canister_manager .delete_canister(*msg.sender(), args.get_canister_id(), &mut state) - .map(|()| EmptyBlob.encode()) + .map(|()| (EmptyBlob.encode(), Some(args.get_canister_id()))) .map_err(|err| err.into()); info!( @@ -906,7 +911,7 @@ impl ExecutionEnvironment { } }; ExecuteSubnetMessageResult::Finished { - response: res, + response: res.map(|res| (res, None)), refund: msg.take_cycles(), } } @@ -1066,11 +1071,14 @@ impl ExecutionEnvironment { args.derivation_path.into_inner(), ) .map(|res| { - ECDSAPublicKeyResponse { - public_key: res.public_key, - chain_code: res.chain_key, - } - .encode() + ( + ECDSAPublicKeyResponse { + public_key: res.public_key, + chain_code: res.chain_key, + } + .encode(), + None, + ) }) } }, @@ -1145,11 +1153,14 @@ impl ExecutionEnvironment { args.derivation_path.into_inner(), ) .map(|res| { - SchnorrPublicKeyResponse { - public_key: res.public_key, - chain_code: res.chain_key, - } - .encode() + ( + SchnorrPublicKeyResponse { + public_key: res.public_key, + chain_code: res.chain_key, + } + .encode(), + None, + ) }) } }, @@ -1270,7 +1281,9 @@ impl ExecutionEnvironment { canister_id, args.derivation_path.into_inner(), ) - .map(|public_key| VetKdPublicKeyResult { public_key }.encode()) + .map(|public_key| { + (VetKdPublicKeyResult { public_key }.encode(), None) + }) } }, }; @@ -1317,7 +1330,12 @@ impl ExecutionEnvironment { registry_settings.subnet_size, &self.metrics.canister_creation_error, ) - .map(|canister_id| CanisterIdRecord::from(canister_id).encode()) + .map(|canister_id| { + ( + CanisterIdRecord::from(canister_id).encode(), + Some(canister_id), + ) + }) .map_err(|err| err.into()), Err(err) => Err(err.into()), } @@ -1330,6 +1348,7 @@ impl ExecutionEnvironment { Ok(Ic00Method::ProvisionalTopUpCanister) => { let res = ProvisionalTopUpCanisterArgs::decode(payload).and_then(|args| { + let canister_id = args.get_canister_id(); self.add_cycles( *msg.sender(), args.get_canister_id(), @@ -1337,6 +1356,7 @@ impl ExecutionEnvironment { &mut state, ®istry_settings.provisional_whitelist, ) + .map(|res| (res, Some(canister_id))) }); ExecuteSubnetMessageResult::Finished { response: res, @@ -1371,7 +1391,7 @@ impl ExecutionEnvironment { &mut state, ) { Ok(Some(payload)) => ExecuteSubnetMessageResult::Finished { - response: Ok(payload), + response: Ok((payload, None)), refund: msg.take_cycles(), }, Ok(None) => ExecuteSubnetMessageResult::Processing, @@ -1406,6 +1426,7 @@ impl ExecutionEnvironment { let resource_saturation = self.subnet_memory_saturation(&round_limits.subnet_available_memory); let res = UploadChunkArgs::decode(payload).and_then(|args| { + let canister_id = args.get_canister_id(); self.upload_chunk( *msg.sender(), &mut state, @@ -1414,6 +1435,7 @@ impl ExecutionEnvironment { registry_settings.subnet_size, &resource_saturation, ) + .map(|res| (res, Some(canister_id))) }); ExecuteSubnetMessageResult::Finished { response: res, @@ -1422,8 +1444,11 @@ impl ExecutionEnvironment { } Ok(Ic00Method::ClearChunkStore) => { - let res = ClearChunkStoreArgs::decode(payload) - .and_then(|args| self.clear_chunk_store(*msg.sender(), &mut state, args)); + let res = ClearChunkStoreArgs::decode(payload).and_then(|args| { + let canister_id = args.get_canister_id(); + self.clear_chunk_store(*msg.sender(), &mut state, args) + .map(|res| (res, Some(canister_id))) + }); ExecuteSubnetMessageResult::Finished { response: res, refund: msg.take_cycles(), @@ -1431,8 +1456,11 @@ impl ExecutionEnvironment { } Ok(Ic00Method::StoredChunks) => { - let res = StoredChunksArgs::decode(payload) - .and_then(|args| self.stored_chunks(*msg.sender(), &state, args)); + let res = StoredChunksArgs::decode(payload).and_then(|args| { + let canister_id = args.get_canister_id(); + self.stored_chunks(*msg.sender(), &state, args) + .map(|res| (res, Some(canister_id))) + }); ExecuteSubnetMessageResult::Finished { response: res, refund: msg.take_cycles(), @@ -1447,7 +1475,7 @@ impl ExecutionEnvironment { let res = NodeMetricsHistoryArgs::decode(payload) .and_then(|args| self.node_metrics_history(&state, args)); ExecuteSubnetMessageResult::Finished { - response: res, + response: res.map(|res| (res, None)), refund: msg.take_cycles(), } } @@ -1459,7 +1487,7 @@ impl ExecutionEnvironment { let res = SubnetInfoArgs::decode(payload) .and_then(|args| self.subnet_info(replica_version, args)); ExecuteSubnetMessageResult::Finished { - response: res, + response: res.map(|res| (res, None)), refund: msg.take_cycles(), } } @@ -1483,6 +1511,7 @@ impl ExecutionEnvironment { refund: msg.take_cycles(), }, Ok(args) => { + let canister_id = args.get_canister_id(); let (result, instructions_used) = self.take_canister_snapshot( *msg.sender(), &mut state, @@ -1491,7 +1520,7 @@ impl ExecutionEnvironment { round_limits, ); let msg_result = ExecuteSubnetMessageResult::Finished { - response: result, + response: result.map(|res| (res, Some(canister_id))), refund: msg.take_cycles(), }; @@ -1508,6 +1537,7 @@ impl ExecutionEnvironment { }, Ok(args) => { let origin = msg.canister_change_origin(args.get_sender_canister_version()); + let canister_id = args.get_canister_id(); let (result, instructions_used) = self.load_canister_snapshot( registry_settings.subnet_size, *msg.sender(), @@ -1517,7 +1547,7 @@ impl ExecutionEnvironment { origin, ); let msg_result = ExecuteSubnetMessageResult::Finished { - response: result, + response: result.map(|res| (res, Some(canister_id))), refund: msg.take_cycles(), }; @@ -1527,8 +1557,11 @@ impl ExecutionEnvironment { }, Ok(Ic00Method::ListCanisterSnapshots) => { - let res = ListCanisterSnapshotArgs::decode(payload) - .and_then(|args| self.list_canister_snapshot(*msg.sender(), &mut state, args)); + let res = ListCanisterSnapshotArgs::decode(payload).and_then(|args| { + let canister_id = args.get_canister_id(); + self.list_canister_snapshot(*msg.sender(), &mut state, args) + .map(|res| (res, Some(canister_id))) + }); ExecuteSubnetMessageResult::Finished { response: res, refund: msg.take_cycles(), @@ -1537,7 +1570,9 @@ impl ExecutionEnvironment { Ok(Ic00Method::DeleteCanisterSnapshot) => { let res = DeleteCanisterSnapshotArgs::decode(payload).and_then(|args| { + let canister_id = args.get_canister_id(); self.delete_canister_snapshot(*msg.sender(), &mut state, args, round_limits) + .map(|res| (res, Some(canister_id))) }); ExecuteSubnetMessageResult::Finished { response: res, @@ -1572,7 +1607,7 @@ impl ExecutionEnvironment { /// Observes a subnet message metrics and outputs the given subnet response. fn finish_subnet_message_execution( &self, - state: ReplicatedState, + mut state: ReplicatedState, message: CanisterCall, result: ExecuteSubnetMessageResult, since: Instant, @@ -1582,10 +1617,23 @@ impl ExecutionEnvironment { ExecuteSubnetMessageResult::Finished { response, .. } => { // Request has been executed. Observe metrics and respond. let method_name = String::from(message.method_name()); + + let res = match response { + Ok((res, canister_id)) => { + if let Some(canister_id) = canister_id { + if let Some(canister_state) = state.canister_state_mut(canister_id) { + canister_state.update_on_low_wasm_memory_hook_condition(); + } + } + Ok(res) + } + Err(err) => Err(err.code()), + }; + self.metrics.observe_subnet_message( method_name.as_str(), since.elapsed().as_secs_f64(), - &response.as_ref().map_err(|err| err.code()), + &res, ); } } @@ -1858,7 +1906,12 @@ impl ExecutionEnvironment { ); ExecuteSubnetMessageResult::Finished { response: res - .map(|new_canister_id| CanisterIdRecord::from(new_canister_id).encode()) + .map(|new_canister_id| { + ( + CanisterIdRecord::from(new_canister_id).encode(), + Some(new_canister_id), + ) + }) .map_err(|err| err.into()), refund: cycles, } @@ -1945,7 +1998,7 @@ impl ExecutionEnvironment { ); } ExecuteSubnetMessageResult::Finished { - response: Ok(EmptyBlob.encode()), + response: Ok((EmptyBlob.encode(), Some(canister_id))), refund: Cycles::zero(), } } @@ -2022,7 +2075,7 @@ impl ExecutionEnvironment { }, StopCanisterResult::AlreadyStopped { cycles_to_return } => { ExecuteSubnetMessageResult::Finished { - response: Ok(EmptyBlob.encode()), + response: Ok((EmptyBlob.encode(), Some(canister_id))), refund: cycles_to_return, } } @@ -2508,7 +2561,7 @@ impl ExecutionEnvironment { ExecuteSubnetMessageResult::Processing => state, ExecuteSubnetMessageResult::Finished { response, refund } => { let payload = match response { - Ok(payload) => Payload::Data(payload), + Ok((payload, ..)) => Payload::Data(payload), Err(err) => Payload::Reject(err.into()), }; @@ -2552,7 +2605,7 @@ impl ExecutionEnvironment { ); } let status = match response { - Ok(payload) => IngressStatus::Known { + Ok((payload, ..)) => IngressStatus::Known { receiver: ingress.receiver.get(), user_id: ingress.source, time: state.time(), @@ -3068,7 +3121,7 @@ impl ExecutionEnvironment { let execution_duration = since.elapsed().as_secs_f64(); match dts_result { DtsInstallCodeResult::Finished { - mut canister, + canister, mut message, call_id, instructions_used, @@ -3093,7 +3146,7 @@ impl ExecutionEnvironment { result.new_wasm_hash, instructions_used.display()); - Ok(EmptyBlob.encode()) + Ok((EmptyBlob.encode(), Some(canister_id))) } Err(err) => { info!( @@ -3106,7 +3159,6 @@ impl ExecutionEnvironment { Err(err.into()) } }; - canister.update_on_low_wasm_memory_hook_condition(); state.put_canister_state(canister); let refund = message.take_cycles(); // The message can be removed because a response was produced. diff --git a/rs/execution_environment/src/execution_environment/tests.rs b/rs/execution_environment/src/execution_environment/tests.rs index 0c0f46c4f8d..d5f7f01f1af 100644 --- a/rs/execution_environment/src/execution_environment/tests.rs +++ b/rs/execution_environment/src/execution_environment/tests.rs @@ -718,7 +718,7 @@ fn get_canister_status_from_another_canister_when_memory_low() { let controller = test.universal_canister().unwrap(); let binary = wat::parse_str("(module)").unwrap(); let canister = test.create_canister(Cycles::new(1_000_000_000_000)); - let memory_allocation = NumBytes::from(300); + let memory_allocation = NumBytes::from(450); test.install_canister_with_allocation(canister, binary, None, Some(memory_allocation.get())) .unwrap(); let canister_status_args = Encode!(&CanisterIdRecord::from(canister)).unwrap(); diff --git a/rs/replicated_state/src/canister_state/system_state/task_queue.rs b/rs/replicated_state/src/canister_state/system_state/task_queue.rs index 87826d45204..3da76916d4a 100644 --- a/rs/replicated_state/src/canister_state/system_state/task_queue.rs +++ b/rs/replicated_state/src/canister_state/system_state/task_queue.rs @@ -391,12 +391,6 @@ pub fn is_low_wasm_memory_hook_condition_satisfied( let wasm_capacity = memory_allocation.map_or_else( || wasm_memory_limit, |memory_allocation| { - debug_assert!( - memory_usage_without_wasm_memory <= memory_allocation, - "Used non-Wasm memory: {:?} is larger than memory allocation: {:?}.", - memory_usage_without_wasm_memory, - memory_allocation - ); std::cmp::min( memory_allocation.saturating_sub(&memory_usage_without_wasm_memory), wasm_memory_limit,