Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
This PR updates the canister manager to charge canisters that are executing `take_canister_snapshot` or `load_canister_snapshot`. Closes EXC-1567 --------- Co-authored-by: Andriy Berestovskyy <[email protected]>
- Loading branch information
1 parent
9356151
commit 85f58e9
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,6 +124,7 @@ pub(crate) struct CanisterMgrConfig { | |
| heap_delta_rate_limit: NumBytes, | ||
| upload_wasm_chunk_instructions: NumInstructions, | ||
| wasm_chunk_store_max_size: NumBytes, | ||
| canister_snapshot_baseline_instructions: NumInstructions, | ||
| } | ||
|
|
||
| impl CanisterMgrConfig { | ||
|
|
@@ -143,6 +144,7 @@ impl CanisterMgrConfig { | |
| heap_delta_rate_limit: NumBytes, | ||
| upload_wasm_chunk_instructions: NumInstructions, | ||
| wasm_chunk_store_max_size: NumBytes, | ||
| canister_snapshot_baseline_instructions: NumInstructions, | ||
| ) -> Self { | ||
| Self { | ||
| subnet_memory_capacity, | ||
|
|
@@ -159,6 +161,7 @@ impl CanisterMgrConfig { | |
| heap_delta_rate_limit, | ||
| upload_wasm_chunk_instructions, | ||
| wasm_chunk_store_max_size, | ||
| canister_snapshot_baseline_instructions, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1767,7 +1770,7 @@ impl CanisterManager { | |
| /// and delete it before creating a new one. | ||
| /// Failure to do so will result in the creation of a new snapshot being unsuccessful. | ||
| /// | ||
| /// If the new snapshot cannot be created, an appropriate error will be returned. | ||
| pub(crate) fn take_canister_snapshot( | ||
| &self, | ||
| subnet_size: usize, | ||
|
|
@@ -1777,28 +1780,39 @@ impl CanisterManager { | |
| state: &mut ReplicatedState, | ||
| round_limits: &mut RoundLimits, | ||
| resource_saturation: &ResourceSaturation, | ||
| ) -> ( | ||
| Result<CanisterSnapshotResponse, CanisterManagerError>, | ||
| NumInstructions, | ||
| ) { | ||
| // Check sender is a controller. | ||
| if let Err(err) = validate_controller(canister, &sender) { | ||
| return (Err(err), NumInstructions::new(0)); | ||
| }; | ||
|
|
||
| match replace_snapshot { | ||
| // Check that replace snapshot ID exists if provided. | ||
| Some(replace_snapshot) => { | ||
| match state.canister_snapshots.get(replace_snapshot) { | ||
| None => { | ||
| // If not found, the operation fails due to invalid parameters. | ||
| return ( | ||
| Err(CanisterManagerError::CanisterSnapshotNotFound { | ||
| canister_id: canister.canister_id(), | ||
| snapshot_id: replace_snapshot, | ||
| }), | ||
| NumInstructions::new(0), | ||
| ); | ||
| } | ||
| Some(snapshot) => { | ||
| // Verify the provided replacement snapshot belongs to this canister. | ||
| if snapshot.canister_id() != canister.canister_id() { | ||
| return ( | ||
| Err(CanisterManagerError::CanisterSnapshotInvalidOwnership { | ||
| canister_id: canister.canister_id(), | ||
| snapshot_id: replace_snapshot, | ||
| }), | ||
| NumInstructions::new(0), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1811,22 +1825,28 @@ impl CanisterManager { | |
| .snapshots_count(&canister.canister_id()) | ||
| >= MAX_NUMBER_OF_SNAPSHOTS_PER_CANISTER | ||
| { | ||
| return ( | ||
| Err(CanisterManagerError::CanisterSnapshotLimitExceeded { | ||
| canister_id: canister.canister_id(), | ||
| limit: MAX_NUMBER_OF_SNAPSHOTS_PER_CANISTER, | ||
| }), | ||
| NumInstructions::new(0), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if self.config.rate_limiting_of_heap_delta == FlagStatus::Enabled | ||
| && canister.scheduler_state.heap_delta_debit >= self.config.heap_delta_rate_limit | ||
| { | ||
| return ( | ||
| Err(CanisterManagerError::CanisterHeapDeltaRateLimited { | ||
| canister_id: canister.canister_id(), | ||
| value: canister.scheduler_state.heap_delta_debit, | ||
| limit: self.config.heap_delta_rate_limit, | ||
| }), | ||
| NumInstructions::new(0), | ||
| ); | ||
| } | ||
|
|
||
| let new_snapshot_size = canister.snapshot_size_bytes(); | ||
|
|
@@ -1859,14 +1879,17 @@ impl CanisterManager { | |
| ); | ||
|
|
||
| if canister.system_state.balance() < threshold + reservation_cycles { | ||
| return ( | ||
| Err(CanisterManagerError::InsufficientCyclesInMemoryGrow { | ||
| bytes: new_snapshot_size, | ||
| available: canister.system_state.balance(), | ||
| threshold, | ||
| }), | ||
| NumInstructions::new(0), | ||
| ); | ||
| } | ||
| // Verify that the subnet has enough memory. | ||
| if let Err(err) = round_limits | ||
| .subnet_available_memory | ||
| .check_available_memory(new_snapshot_size, NumBytes::from(0), NumBytes::from(0)) | ||
| .map_err( | ||
|
|
@@ -1879,9 +1902,12 @@ impl CanisterManager { | |
| .max(0) as u64, | ||
| ), | ||
| }, | ||
| ) | ||
| { | ||
| return (Err(err), NumInstructions::new(0)); | ||
| }; | ||
| // Reserve needed cycles if the subnet is becoming saturated. | ||
| if let Err(err) = canister | ||
| .system_state | ||
| .reserve_cycles(reservation_cycles) | ||
| .map_err(|err| match err { | ||
|
|
@@ -1900,17 +1926,64 @@ impl CanisterManager { | |
| limit, | ||
| } | ||
| } | ||
| }) | ||
| { | ||
| return (Err(err), NumInstructions::new(0)); | ||
| }; | ||
| // Actually deduct memory from the subnet. It's safe to unwrap | ||
| // here because we already checked the available memory above. | ||
| round_limits.subnet_available_memory | ||
| .try_decrement(new_snapshot_size, NumBytes::from(0), NumBytes::from(0)) | ||
| .expect("Error: Cannot fail to decrement SubnetAvailableMemory after checking for availability"); | ||
| } | ||
|
|
||
| let current_memory_usage = canister.memory_usage() + new_snapshot_size; | ||
| let message_memory = canister.message_memory_usage(); | ||
| let compute_allocation = canister.compute_allocation(); | ||
| let reveal_top_up = canister.controllers().contains(&sender); | ||
| let instructions = self.config.canister_snapshot_baseline_instructions | ||
| + NumInstructions::new(new_snapshot_size.get()); | ||
|
|
||
| // Charge for the take snapshot of the canister. | ||
| let prepaid_cycles = match self | ||
| .cycles_account_manager | ||
| .prepay_execution_cycles( | ||
| &mut canister.system_state, | ||
| current_memory_usage, | ||
| message_memory, | ||
| compute_allocation, | ||
| instructions, | ||
| subnet_size, | ||
| reveal_top_up, | ||
| ) | ||
| .map_err(CanisterManagerError::CanisterSnapshotNotEnoughCycles) | ||
| { | ||
| Ok(c) => c, | ||
| Err(err) => return (Err(err), NumInstructions::new(0)), | ||
| }; | ||
|
|
||
| // To keep the invariant that `prepay_execution_cycles` is always paired | ||
| // with `refund_unused_execution_cycles` we refund zero immediately. | ||
| self.cycles_account_manager.refund_unused_execution_cycles( | ||
| &mut canister.system_state, | ||
| NumInstructions::from(0), | ||
| instructions, | ||
| prepaid_cycles, | ||
| // This counter is incremented if we refund more | ||
| // instructions than initially charged, which is impossible | ||
| // here. | ||
| &IntCounter::new("no_op", "no_op").unwrap(), | ||
| subnet_size, | ||
| &self.log, | ||
| ); | ||
|
|
||
| // Create new snapshot. | ||
| let new_snapshot = match CanisterSnapshot::from_canister(canister, state.time()) | ||
| .map_err(CanisterManagerError::from) | ||
| { | ||
| Ok(s) => s, | ||
| Err(err) => return (Err(err), instructions), | ||
| }; | ||
|
|
||
| // Delete old snapshot identified by `replace_snapshot` ID. | ||
| if let Some(replace_snapshot) = replace_snapshot { | ||
|
|
@@ -1943,15 +2016,19 @@ impl CanisterManager { | |
| .canister_snapshots | ||
| .push(snapshot_id, Arc::new(new_snapshot)); | ||
| canister.system_state.snapshots_memory_usage += new_snapshot_size; | ||
| ( | ||
| Ok(CanisterSnapshotResponse::new( | ||
| &snapshot_id, | ||
| state.time().as_nanos_since_unix_epoch(), | ||
| new_snapshot_size, | ||
| )), | ||
| instructions, | ||
| ) | ||
| } | ||
|
|
||
| pub(crate) fn load_canister_snapshot( | ||
| &self, | ||
| subnet_size: usize, | ||
| sender: PrincipalId, | ||
| canister: &CanisterState, | ||
| snapshot_id: SnapshotId, | ||
|
|
@@ -2080,6 +2157,46 @@ impl CanisterManager { | |
| ); | ||
| } | ||
|
|
||
| let compute_allocation = new_canister.compute_allocation(); | ||
| let message_memory = canister.message_memory_usage(); | ||
| let reveal_top_up = canister.controllers().contains(&sender); | ||
| let instructions = self.config.canister_snapshot_baseline_instructions | ||
| + instructions_used | ||
| + NumInstructions::new(snapshot.size().get()); | ||
|
|
||
| // Charge for loading the snapshot of the canister. | ||
| let prepaid_cycles = match self.cycles_account_manager.prepay_execution_cycles( | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. 
AleDema
|
||
| &mut new_canister.system_state, | ||
| new_memory_usage, | ||
| message_memory, | ||
| compute_allocation, | ||
| instructions, | ||
| subnet_size, | ||
| reveal_top_up, | ||
| ) { | ||
| Ok(prepaid_cycles) => prepaid_cycles, | ||
| Err(err) => { | ||
| return ( | ||
| instructions_used, | ||
| Err(CanisterManagerError::CanisterSnapshotNotEnoughCycles(err)), | ||
| ) | ||
| } | ||
| }; | ||
|
|
||
| // To keep the invariant that `prepay_execution_cycles` is always paired | ||
| // with `refund_unused_execution_cycles` we refund zero immediately. | ||
| self.cycles_account_manager.refund_unused_execution_cycles( | ||
| &mut new_canister.system_state, | ||
| NumInstructions::from(0), | ||
| instructions, | ||
| prepaid_cycles, | ||
| // This counter is incremented if we refund more | ||
| // instructions than initially charged, which is impossible | ||
| // here. | ||
| &IntCounter::new("no_op", "no_op").unwrap(), | ||
| subnet_size, | ||
| &self.log, | ||
| ); | ||
| // Increment canister version. | ||
| new_canister.system_state.canister_version += 1; | ||
| new_canister.system_state.add_canister_change( | ||
|
|
@@ -2286,6 +2403,7 @@ pub(crate) enum CanisterManagerError { | |
| canister_id: CanisterId, | ||
| limit: usize, | ||
| }, | ||
| CanisterSnapshotNotEnoughCycles(CanisterOutOfCyclesError), | ||
| LongExecutionAlreadyInProgress { | ||
| canister_id: CanisterId, | ||
| }, | ||
|
|
@@ -2334,6 +2452,7 @@ impl AsErrorHelp for CanisterManagerError { | |
| | CanisterManagerError::CanisterSnapshotInvalidOwnership { .. } | ||
| | CanisterManagerError::CanisterSnapshotExecutionStateNotFound { .. } | ||
| | CanisterManagerError::CanisterSnapshotLimitExceeded { .. } | ||
| | CanisterManagerError::CanisterSnapshotNotEnoughCycles { .. } | ||
| | CanisterManagerError::LongExecutionAlreadyInProgress { .. } | ||
| | CanisterManagerError::MissingUpgradeOptionError { .. } | ||
| | CanisterManagerError::InvalidUpgradeOptionError { .. } => ErrorHelp::UserError { | ||
|
|
@@ -2633,6 +2752,12 @@ impl From<CanisterManagerError> for UserError { | |
| ) | ||
| ) | ||
| } | ||
| CanisterSnapshotNotEnoughCycles(err) => { | ||
| Self::new( | ||
| ErrorCode::CanisterOutOfCycles, | ||
| format!("Canister snapshotting failed with `{}`{additional_help}", err), | ||
| ) | ||
| } | ||
| LongExecutionAlreadyInProgress { canister_id } => { | ||
| Self::new( | ||
| ErrorCode::CanisterRejectedMessage, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Maybe I don't have the full context of what's happening in this function.
It looks a bit weird to me that we are executing all the code to load the canister snapshot into the canister state, including creating the new canister from the execution state obtained from the snapshot, and only then paying/checking if the canister has enough cycles.
This means, correct me if I'm wrong, that we are spending the compute power we account for even if the canister doesn't have enough cycles for it.
@AlexandraZapuc @berestovskyy am I missing something here?