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,