diff --git a/rs/execution_environment/src/canister_logs.rs b/rs/execution_environment/src/canister_logs.rs index a1845b70c818..a0306dc1f70b 100644 --- a/rs/execution_environment/src/canister_logs.rs +++ b/rs/execution_environment/src/canister_logs.rs @@ -6,6 +6,7 @@ use ic_management_canister_types_private::{ FetchCanisterLogsRequest, FetchCanisterLogsResponse, LogVisibilityV2, }; use ic_replicated_state::CanisterState; +use ic_replicated_state::metadata_state::UnflushedCheckpointOps; use ic_types::{NumBytes, NumInstructions, PrincipalId}; pub(crate) fn fetch_canister_logs( @@ -24,7 +25,7 @@ pub(crate) fn fetch_canister_logs( canister_id, reply: Some(reply), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index c7d7c53a1feb..485da60a882b 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -46,7 +46,7 @@ use ic_replicated_state::canister_state::system_state::wasm_chunk_store::{ self, CHUNK_SIZE, ChunkValidationResult, WasmChunkHash, WasmChunkStore, }; use ic_replicated_state::metadata_state::{ - UnflushedCheckpointOp, subnet_call_context_manager::InstallCodeCallId, + UnflushedCheckpointOps, subnet_call_context_manager::InstallCodeCallId, }; use ic_replicated_state::page_map::{Buffer, PageAllocatorFileDescriptor}; use ic_replicated_state::{ @@ -776,7 +776,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase, - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -1042,7 +1042,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: rejects, stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -1087,7 +1087,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply, heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove, stop_contexts_to_reject: vec![], @@ -1121,7 +1121,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject, @@ -1662,7 +1662,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -1694,7 +1694,7 @@ impl CanisterManager { canister_id, reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -1809,7 +1809,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(reply.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -1857,7 +1857,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(reply.encode()), heap_delta_increase: chunk_bytes, - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -1899,7 +1899,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -2117,9 +2117,13 @@ impl CanisterManager { let new_snapshot = CanisterSnapshot::from_canister(canister, time).map_err(CanisterManagerError::from)?; - // Delete old snapshot identified by `replace_snapshot`. + // Delete old snapshot identified by `replace_snapshot`, recording the deletion + // so that its directory is also deleted from the tip. + let mut unflushed_checkpoint_ops = UnflushedCheckpointOps::default(); if let Some(replace_snapshot) = replace_snapshot { - canister.canister_snapshots.remove(replace_snapshot); + canister + .canister_snapshots + .remove(replace_snapshot, &mut unflushed_checkpoint_ops); } let heap_delta = new_snapshot.heap_delta(); @@ -2129,6 +2133,7 @@ impl CanisterManager { canister .canister_snapshots .push(snapshot_id, Arc::new(new_snapshot)); + unflushed_checkpoint_ops.take_snapshot(canister_id, snapshot_id); // Optionally uninstall the canister's code atomically after taking the // snapshot, recording the corresponding `CanisterCodeUninstall` canister @@ -2176,10 +2181,7 @@ impl CanisterManager { canister_id, reply: Some(reply.encode()), heap_delta_increase: heap_delta, - unflushed_checkpoint_op: Some(UnflushedCheckpointOp::TakeSnapshot( - canister_id, - snapshot_id, - )), + unflushed_checkpoint_ops, deleted_call_context_responses, stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -2553,14 +2555,17 @@ impl CanisterManager { let heap_delta = new_canister.heap_delta(); *canister = new_canister; + + // Record the load, so that the canister's files in the tip are replaced by the + // snapshot's. + let mut unflushed_checkpoint_ops = UnflushedCheckpointOps::default(); + unflushed_checkpoint_ops.load_snapshot(canister_id, snapshot_id); + Ok(CanisterManagerResponse { canister_id, reply: Some(EmptyBlob.encode()), heap_delta_increase: heap_delta, - unflushed_checkpoint_op: Some(UnflushedCheckpointOp::LoadSnapshot( - canister_id, - snapshot_id, - )), + unflushed_checkpoint_ops, deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -2627,13 +2632,18 @@ impl CanisterManager { resource_saturation, )?; - canister.canister_snapshots.remove(delete_snapshot_id); + // Delete the snapshot, recording the deletion so that its directory is also + // deleted from the tip. + let mut unflushed_checkpoint_ops = UnflushedCheckpointOps::default(); + canister + .canister_snapshots + .remove(delete_snapshot_id, &mut unflushed_checkpoint_ops); Ok(CanisterManagerResponse { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops, deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -2763,7 +2773,7 @@ impl CanisterManager { canister_id, reply: Some(reply.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -2853,9 +2863,13 @@ impl CanisterManager { )?; round_limits.instructions -= as_round_instructions(instructions); - // Delete old snapshot identified by `replace_snapshot`. + // Delete old snapshot identified by `replace_snapshot`, recording the deletion + // so that its directory is also deleted from the tip. + let mut unflushed_checkpoint_ops = UnflushedCheckpointOps::default(); if let Some(replace_snapshot) = args.replace_snapshot() { - canister.canister_snapshots.remove(replace_snapshot); + canister + .canister_snapshots + .remove(replace_snapshot, &mut unflushed_checkpoint_ops); } // Create new snapshot. @@ -2879,7 +2893,7 @@ impl CanisterManager { canister_id, reply: Some(reply.encode()), heap_delta_increase: heap_delta, - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops, deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -3001,7 +3015,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(0), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], @@ -3044,7 +3058,7 @@ impl CanisterManager { canister_id: canister.canister_id(), reply: Some(EmptyBlob.encode()), heap_delta_increase: NumBytes::new(bytes_written), - unflushed_checkpoint_op: None, + unflushed_checkpoint_ops: UnflushedCheckpointOps::default(), deleted_call_context_responses: vec![], stop_call_id_to_remove: None, stop_contexts_to_reject: vec![], diff --git a/rs/execution_environment/src/canister_manager/types.rs b/rs/execution_environment/src/canister_manager/types.rs index 8f6611131d2b..8931230d1da7 100644 --- a/rs/execution_environment/src/canister_manager/types.rs +++ b/rs/execution_environment/src/canister_manager/types.rs @@ -13,7 +13,7 @@ use ic_replicated_state::{ CanisterState, canister_state::canister_snapshots::CanisterSnapshotError, canister_state::system_state::wasm_chunk_store::{WasmChunkStore, chunk_size}, - metadata_state::UnflushedCheckpointOp, + metadata_state::UnflushedCheckpointOps, metadata_state::subnet_call_context_manager::InstallCodeCallId, }; use ic_types::{ @@ -319,9 +319,9 @@ pub(crate) struct CanisterManagerResponse { /// The heap delta increase produced by processing /// the current request. pub heap_delta_increase: NumBytes, - /// An unflushed checkpoint operation that must be handled + /// Unflushed checkpoint operations that must be handled /// before the next checkpoint. - pub unflushed_checkpoint_op: Option, + pub unflushed_checkpoint_ops: UnflushedCheckpointOps, /// (Reject) responses from call contexts that were marked as "deleted" while processing the current request. /// Note. A call context is marked as "deleted" when a canister is uninstalled. pub deleted_call_context_responses: Vec, diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index d0f38945b84c..3c9c2f24ff21 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -2147,12 +2147,10 @@ impl ExecutionEnvironment { .heap_delta_debit .saturating_add(&response.heap_delta_increase); } - if let Some(unflushed_checkpoint_op) = response.unflushed_checkpoint_op { - state - .metadata - .unflushed_checkpoint_ops - .push(unflushed_checkpoint_op); - } + state + .metadata + .unflushed_checkpoint_ops + .extend(response.unflushed_checkpoint_ops); if let Some(snapshot_id) = response.snapshot_to_make_immutable && let Some(canister) = state.canister_state_make_mut(&snapshot_id.get_canister_id()) diff --git a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs index c756a4f1a50f..7e8cca6845b4 100644 --- a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs @@ -1059,6 +1059,91 @@ fn delete_canister_snapshot_succeeds() { ); } +/// Tests that every way of deleting a snapshot records an +/// `UnflushedCheckpointOp::DeleteSnapshot`, so that the snapshot's directory is deleted +/// from the tip. +#[test] +fn snapshot_deletions_record_unflushed_checkpoint_ops() { + const CYCLES: Cycles = Cycles::new(1_000_000_000_000); + + let mut test = ExecutionTestBuilder::new().build(); + let canister_id = test + .canister_from_cycles_and_binary(CYCLES, UNIVERSAL_CANISTER_WASM.to_vec()) + .unwrap(); + + // Installing a canister does not require any checkpoint ops. + assert!(test.state().metadata.unflushed_checkpoint_ops.is_empty()); + + // Taking a snapshot requires copying the canister's files to the snapshot's + // directory in the tip. + let (snapshot_id, _) = helper_take_snapshot(&mut test, canister_id); + assert_eq!( + test.state_mut().metadata.unflushed_checkpoint_ops.take(), + vec![UnflushedCheckpointOp::TakeSnapshot( + canister_id, + snapshot_id + )] + ); + + // Replacing the snapshot requires deleting the replaced snapshot's directory before + // creating the new one. + let args = TakeCanisterSnapshotArgs::new(canister_id, Some(snapshot_id), None, None); + let result = test.subnet_message("take_canister_snapshot", args.encode()); + let response = CanisterSnapshotResponse::decode(&result.unwrap().bytes()).unwrap(); + let new_snapshot_id = response.snapshot_id(); + assert_eq!( + test.state_mut().metadata.unflushed_checkpoint_ops.take(), + vec![ + UnflushedCheckpointOp::DeleteSnapshot(snapshot_id), + UnflushedCheckpointOp::TakeSnapshot(canister_id, new_snapshot_id), + ] + ); + + // And so does explicitly deleting a snapshot. + let args = DeleteCanisterSnapshotArgs::new(canister_id, new_snapshot_id); + test.subnet_message("delete_canister_snapshot", args.encode()) + .unwrap(); + assert_eq!( + test.state_mut().metadata.unflushed_checkpoint_ops.take(), + vec![UnflushedCheckpointOp::DeleteSnapshot(new_snapshot_id)] + ); +} + +/// Tests that deleting a canister also records an +/// `UnflushedCheckpointOp::DeleteSnapshot` for each of its snapshots, so that their +/// directories are deleted from the tip along with the canister's. +#[test] +fn delete_canister_records_unflushed_checkpoint_ops_for_its_snapshots() { + const CYCLES: Cycles = Cycles::new(1_000_000_000_000); + + let mut test = ExecutionTestBuilder::new().build(); + let canister_id = test + .canister_from_cycles_and_binary(CYCLES, UNIVERSAL_CANISTER_WASM.to_vec()) + .unwrap(); + + let (snapshot_id, _) = helper_take_snapshot(&mut test, canister_id); + assert_eq!( + test.state_mut().metadata.unflushed_checkpoint_ops.take(), + vec![UnflushedCheckpointOp::TakeSnapshot( + canister_id, + snapshot_id + )] + ); + + let _ = test.stop_canister(canister_id); + test.process_stopping_canisters(); + test.delete_canister(canister_id).unwrap(); + + // Both the snapshot's and the canister's directories must be deleted from the tip. + assert_eq!( + test.state_mut().metadata.unflushed_checkpoint_ops.take(), + vec![ + UnflushedCheckpointOp::DeleteSnapshot(snapshot_id), + UnflushedCheckpointOp::DeleteCanister(canister_id), + ] + ); +} + #[test] fn list_canister_snapshot_fails_canister_not_found() { let own_subnet = subnet_test_id(1); diff --git a/rs/execution_environment/src/scheduler.rs b/rs/execution_environment/src/scheduler.rs index 01c05424b7e1..cbc2d3dab734 100644 --- a/rs/execution_environment/src/scheduler.rs +++ b/rs/execution_environment/src/scheduler.rs @@ -33,6 +33,7 @@ use ic_registry_subnet_type::SubnetType; use ic_replicated_state::SubnetSchedule; use ic_replicated_state::canister_state::NextExecution; use ic_replicated_state::canister_state::execution_state::NextScheduledMethod; +use ic_replicated_state::metadata_state::UnflushedCheckpointOps; use ic_replicated_state::page_map::PageAllocatorFileDescriptor; use ic_replicated_state::{ CanisterState, CanisterStates, ExecutionTask, InputQueueType, NetworkTopology, ReplicatedState, @@ -840,6 +841,11 @@ impl SchedulerImpl { .duration_between_allocation_charges(), ); let mut all_rejects = Vec::new(); + // The deletions of the snapshots of the canisters uninstalled below, recorded + // so that their directories are also deleted from the tip. Accumulated here + // because `state.metadata` is not accessible from within the closure; merged + // into the state's operations after the loop. + let mut unflushed_checkpoint_ops = UnflushedCheckpointOps::default(); // TODO(DSM-103): Charge all canisters every N rounds / seconds (and otherwise // do nothing). Ensure that paused execution canisters are charged eventually. state.canisters_for_each_mut(|_id, canister| { @@ -885,7 +891,9 @@ impl SchedulerImpl { canister .system_state .burn_remaining_balance_for_uninstall(cost_schedule); - canister.canister_snapshots.delete_snapshots(); + canister + .canister_snapshots + .delete_snapshots(&mut unflushed_checkpoint_ops); info!( self.log, @@ -896,6 +904,11 @@ impl SchedulerImpl { } }); + state + .metadata + .unflushed_checkpoint_ops + .extend(unflushed_checkpoint_ops); + // Send rejects to any requests that were forcibly closed while uninstalling. for rejects in all_rejects.into_iter() { process_responses( diff --git a/rs/execution_environment/src/scheduler/tests/charging.rs b/rs/execution_environment/src/scheduler/tests/charging.rs index 48b1bafb3537..8671d80da2d4 100644 --- a/rs/execution_environment/src/scheduler/tests/charging.rs +++ b/rs/execution_environment/src/scheduler/tests/charging.rs @@ -11,6 +11,7 @@ use ic_management_canister_types_private::{ }; use ic_registry_subnet_type::SubnetType; use ic_replicated_state::canister_state::system_state::PausedExecutionId; +use ic_replicated_state::metadata_state::UnflushedCheckpointOp; use ic_replicated_state::testing::SystemStateTesting; use ic_types::messages::{CanisterMessageOrTask, CanisterTask}; use ic_types::time::UNIX_EPOCH; @@ -541,6 +542,16 @@ fn snapshot_is_deleted_when_canister_is_out_of_cycles() { .is_some() ); + let snapshot_id = *test + .state() + .canister_state(&canister_id) + .unwrap() + .canister_snapshots + .iter() + .next() + .unwrap() + .0; + // Uninstall canister due to `out_of_cycles`. test.set_time(initial_time + 1000 * test.duration_between_allocation_charges()); // Checkpoint round, to force charging for storage. @@ -568,6 +579,15 @@ fn snapshot_is_deleted_when_canister_is_out_of_cycles() { .execution_state .is_none() ); + // Taking and deleting the snapshot were recorded as checkpoint operations, so that + // the snapshot's directory is created in and then deleted from the tip. + assert_eq!( + test.state_mut().metadata.unflushed_checkpoint_ops.take(), + vec![ + UnflushedCheckpointOp::TakeSnapshot(canister_id, snapshot_id), + UnflushedCheckpointOp::DeleteSnapshot(snapshot_id), + ] + ); } #[test] diff --git a/rs/replicated_state/src/canister_state/canister_snapshots.rs b/rs/replicated_state/src/canister_state/canister_snapshots.rs index aac5a4ff0f54..425c548e0593 100644 --- a/rs/replicated_state/src/canister_state/canister_snapshots.rs +++ b/rs/replicated_state/src/canister_state/canister_snapshots.rs @@ -5,6 +5,7 @@ use crate::{ execution_state::Memory, system_state::wasm_chunk_store::{self, ValidatedChunk, WasmChunkStore}, }, + metadata_state::UnflushedCheckpointOps, page_map::{Buffer, PageAllocatorFileDescriptor, PersistenceError}, }; use ic_config::embedders::{MAX_GLOBALS, WASM_MAX_SIZE}; @@ -100,20 +101,37 @@ impl CanisterSnapshots { self.snapshots.iter_mut() } - /// Remove snapshot identified by `snapshot_id` from the collection of snapshots. - pub fn remove(&mut self, snapshot_id: SnapshotId) { + /// Removes the snapshot identified by `snapshot_id` from the collection of + /// snapshots; and records the removal in `unflushed_checkpoint_ops`, so that the + /// snapshot's directory is also deleted from the tip. + /// + /// Takes `unflushed_checkpoint_ops` (rather than having the caller record the + /// operation) so that the operation cannot be forgotten. Callers with no access to + /// `SystemMetadata` (e.g. `CanisterManager`, which only mutates a single + /// `CanisterState`) can pass in a temporary `UnflushedCheckpointOps` and have it + /// merged into the state's operations via `UnflushedCheckpointOps::extend()`. + pub fn remove( + &mut self, + snapshot_id: SnapshotId, + unflushed_checkpoint_ops: &mut UnflushedCheckpointOps, + ) { if let Some(snapshot) = self.snapshots.remove(&snapshot_id) { self.memory_usage -= snapshot.size(); + unflushed_checkpoint_ops.delete_snapshot(snapshot_id); } } - /// Remove all snapshots from the collections of snapshots. - /// Returns the list of deleted snapshots. - pub fn delete_snapshots(&mut self) -> Vec { - let result = self.snapshots.keys().cloned().collect(); + /// Removes all snapshots from the collection of snapshots; and records the removals + /// in `unflushed_checkpoint_ops`, so that the snapshots' directories are also + /// deleted from the tip. + /// + /// See `remove()` for why this takes `unflushed_checkpoint_ops`. + pub fn delete_snapshots(&mut self, unflushed_checkpoint_ops: &mut UnflushedCheckpointOps) { + for snapshot_id in self.snapshots.keys() { + unflushed_checkpoint_ops.delete_snapshot(*snapshot_id); + } self.snapshots.clear(); self.memory_usage = NumBytes::new(0); - result } /// Lists all snapshots. @@ -667,7 +685,7 @@ mod tests { assert_eq!(snapshot_manager.snapshots.len(), 1); - snapshot_manager.remove(snapshot_id); + snapshot_manager.remove(snapshot_id, &mut UnflushedCheckpointOps::default()); assert_eq!(snapshot_manager.snapshots.len(), 0); } @@ -702,14 +720,14 @@ mod tests { ); // Deleting a snapshot updates the `memory_usage`. - snapshot_manager.remove(first_snapshot_id); + snapshot_manager.remove(first_snapshot_id, &mut UnflushedCheckpointOps::default()); assert_eq!( snapshot_manager.memory_taken(), NumBytes::from(snapshot2_size) ); // Deleting the second snapshot brings us back to 0 memory taken. - snapshot_manager.remove(second_snapshot_id); + snapshot_manager.remove(second_snapshot_id, &mut UnflushedCheckpointOps::default()); assert_eq!(snapshot_manager.memory_taken(), NumBytes::from(0)); } } diff --git a/rs/replicated_state/src/metadata_state.rs b/rs/replicated_state/src/metadata_state.rs index e2e9d7e1e00c..91463aa8c99d 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -7,6 +7,7 @@ mod tests; use self::subnet_call_context_manager::SubnetCallContextManager; use self::subnet_schedule::SubnetSchedule; use crate::CanisterQueues; +use crate::CanisterState; use crate::CheckpointLoadingMetrics; use ic_base_types::{CanisterId, SnapshotId}; use ic_btc_replica_types::BlockBlob; @@ -2385,6 +2386,8 @@ pub enum UnflushedCheckpointOp { RenameCanister(CanisterId, CanisterId), /// A canister was deleted. DeleteCanister(CanisterId), + /// A snapshot was deleted. + DeleteSnapshot(SnapshotId), } /// A collection of unflushed checkpoint operations in the order that they were applied to the state. @@ -2432,12 +2435,40 @@ impl UnflushedCheckpointOps { )); } - /// Records the deletion of a canister. Private to the crate because the only way - /// of permanently removing a canister is `ReplicatedState::remove_canister()`, - /// which calls this on the caller's behalf. - pub(crate) fn delete_canister(&mut self, canister_id: CanisterId) { + /// Records the deletion of a canister, together with the deletion of all its + /// snapshots (which are deleted along with the canister). Private to the crate + /// because the only ways of permanently removing a canister are + /// `ReplicatedState::remove_canister()` and the two subnet split methods, which + /// call this on the caller's behalf. + /// + /// Takes the `CanisterState` rather than just the canister ID because the + /// canister's snapshots live in it (`SystemMetadata` cannot map a canister ID to + /// its snapshot IDs); recording their deletion here rather than at the call sites + /// means it cannot be overlooked. + pub(crate) fn delete_canister(&mut self, canister_state: &CanisterState) { + for (snapshot_id, _) in canister_state.canister_snapshots.iter() { + self.delete_snapshot(*snapshot_id); + } + self.operations.push(UnflushedCheckpointOp::DeleteCanister( + canister_state.canister_id(), + )); + } + + /// Records the deletion of a canister snapshot. Private to the crate because the + /// only ways of permanently removing a snapshot are `CanisterSnapshots::remove()`, + /// `CanisterSnapshots::delete_snapshots()` and the deletion of the snapshot's + /// canister, all of which call this on the caller's behalf. + pub(crate) fn delete_snapshot(&mut self, snapshot_id: SnapshotId) { self.operations - .push(UnflushedCheckpointOp::DeleteCanister(canister_id)); + .push(UnflushedCheckpointOp::DeleteSnapshot(snapshot_id)); + } + + /// Appends all operations of `other`, preserving their order. + /// + /// Used to merge in the operations recorded while mutating a single `CanisterState` + /// (which has no access to `SystemMetadata`) into the state's operations. + pub fn extend(&mut self, other: UnflushedCheckpointOps) { + self.operations.extend(other.operations); } } diff --git a/rs/replicated_state/src/replicated_state.rs b/rs/replicated_state/src/replicated_state.rs index 2df13ce040d5..71ee58ef5ec7 100644 --- a/rs/replicated_state/src/replicated_state.rs +++ b/rs/replicated_state/src/replicated_state.rs @@ -592,8 +592,9 @@ impl ReplicatedState { } /// Permanently removes the canister and its scheduling priority from the subnet - /// schedule; and records the removal as an unflushed checkpoint operation, so that - /// the canister's directory is also deleted from the tip. + /// schedule; and records the removal of the canister and of all its snapshots as + /// unflushed checkpoint operations, so that their directories are also deleted from + /// the tip. /// /// Use `take_canister_state()` instead if the canister is only temporarily removed /// from the state (e.g. to work around borrow checker limitations). @@ -602,7 +603,7 @@ impl ReplicatedState { let canister_state = self.canister_states.remove(canister_id)?; self.metadata .unflushed_checkpoint_ops - .delete_canister(*canister_id); + .delete_canister(&canister_state); Some(canister_state) } @@ -1471,8 +1472,9 @@ impl ReplicatedState { assert!(consensus_queue.is_empty()); // Retain only canisters hosted by `subnet_id`; and record the removal of the - // others, so that their directories are deleted from tip by the flush of these - // operations, making `TipRequest::FilterTipCanisters` a pure safety net. + // others (and of their snapshots), so that their directories are deleted from + // tip by the flush of these operations, making `TipRequest::FilterTipCanisters` + // a pure safety net. // // TODO: Validate that canisters are split across no more than 2 subnets. let is_local_canister = |canister_id: &CanisterId| { @@ -1486,11 +1488,11 @@ impl ReplicatedState { .filter(|canister_id| !is_local_canister(canister_id)) .cloned() .collect(); - canister_states.retain(|canister_id, _| is_local_canister(canister_id)); for canister_id in dropped_canister_ids { + let canister_state = canister_states.remove(&canister_id).unwrap(); metadata .unflushed_checkpoint_ops - .delete_canister(canister_id); + .delete_canister(&canister_state); } // All subnet messages (ingress and canister) only remain on subnet A' because: @@ -1602,7 +1604,8 @@ impl ReplicatedState { /// /// * Retaining only the canisters that are to be hosted by `subnet_id`, as /// determined by the routing table (*hosted canisters*); and recording the - /// removal of the rest as `UnflushedCheckpointOp::DeleteCanister`, so that + /// removal of the rest as `UnflushedCheckpointOp::DeleteCanister` (plus an + /// `UnflushedCheckpointOp::DeleteSnapshot` per snapshot of theirs), so that /// their directories are explicitly deleted from tip, in order relative to the /// other checkpoint operations. /// * Retaining only the snapshots of *hosted canisters*. @@ -1657,24 +1660,25 @@ impl ReplicatedState { }); // Retain only canisters hosted by this subnet; and record the removal of the - // others, so that their directories are deleted from tip by the flush of these - // operations, in order relative to the other checkpoint operations. + // others (and of their snapshots), so that their directories are deleted from + // tip by the flush of these operations, in order relative to the other + // checkpoint operations. // // A splitting batch always requires a full state hash, so the split round is // always a checkpoint round and `FilterTipCanisters` would remove the very same // directories later in the same round. Recording the removals makes every - // canister directory mutation in tip an explicit, ordered operation, leaving - // `FilterTipCanisters` as a pure safety net. + // canister and snapshot directory mutation in tip an explicit, ordered + // operation, leaving `FilterTipCanisters` as a pure safety net. let dropped_canister_ids: Vec = canister_states .all_keys() .filter(|canister_id| lookup_subnet(canister_id) != Some(subnet_id)) .cloned() .collect(); - canister_states.retain(|canister_id, _| lookup_subnet(canister_id) == Some(subnet_id)); for canister_id in dropped_canister_ids { + let canister_state = canister_states.remove(&canister_id).unwrap(); metadata .unflushed_checkpoint_ops - .delete_canister(canister_id); + .delete_canister(&canister_state); } // Adjust `CanisterQueues::(local|remote)_subnet_input_schedule` based on which diff --git a/rs/replicated_state/tests/replicated_state.rs b/rs/replicated_state/tests/replicated_state.rs index c9893615d89d..76f77ff22f42 100644 --- a/rs/replicated_state/tests/replicated_state.rs +++ b/rs/replicated_state/tests/replicated_state.rs @@ -1353,8 +1353,8 @@ fn online_split() { .push(snapshot_id, snapshot.into()); snapshot_id }; - let canister_1_snapshot_id = take_shapshot(CANISTER_1); - let canister_2_snapshot_id = take_shapshot(CANISTER_2); + take_shapshot(CANISTER_1); + take_shapshot(CANISTER_2); // Add aborted `install_code` tasks to both canisters. let mut add_aborted_install_code_task = |canister_id| { @@ -1387,7 +1387,9 @@ fn online_split() { // Start off with the original state (plus new routing table). let mut expected = fixture.state.clone(); - // Only `CANISTER_1` should be left. + // Only `CANISTER_1` should be left; with the removal of `CANISTER_2` and of its + // snapshot recorded as checkpoint operations, so that their directories are deleted + // from tip. expected.remove_canister(&CANISTER_2); // The input schedules of `CANISTER_1` should have been repartitioned. let mut canister_state_arc = expected.take_canister_state(&CANISTER_1).unwrap(); @@ -1395,10 +1397,6 @@ fn online_split() { canister_state .system_state .split_input_schedules(&CANISTER_1, expected.canister_states()); - // The snapshot of `CANISTER_2` should have been deleted. - canister_state - .canister_snapshots - .remove(canister_2_snapshot_id); expected.put_canister_state(canister_state_arc); // And the split marker should be set. @@ -1420,7 +1418,9 @@ fn online_split() { let mut expected = fixture.state.clone(); // New subnet ID. expected.metadata.own_subnet_id = SUBNET_B; - // Only `CANISTER_2` should be hosted. + // Only `CANISTER_2` should be hosted; with the removal of `CANISTER_1` and of its + // snapshot recorded as checkpoint operations, so that their directories are deleted + // from tip. expected.remove_canister(&CANISTER_1); // The input schedules of `CANISTER_2` should have been repartitioned. let mut canister_state_arc = expected.take_canister_state(&CANISTER_2).unwrap(); @@ -1430,10 +1430,6 @@ fn online_split() { .split_input_schedules(&CANISTER_2, expected.canister_states()); // The in-progress `install_code` task should have been silently dropped. canister_state.system_state.task_queue = Default::default(); - // The snapshot of `CANISTER_1` should have been deleted. - canister_state - .canister_snapshots - .remove(canister_1_snapshot_id); expected.put_canister_state(canister_state_arc); // Streams, subnet queues and refunds should be empty. diff --git a/rs/state_layout/src/state_layout.rs b/rs/state_layout/src/state_layout.rs index c27b61cb420f..7b150ed6034c 100644 --- a/rs/state_layout/src/state_layout.rs +++ b/rs/state_layout/src/state_layout.rs @@ -518,19 +518,23 @@ impl TipHandler { } /// Deletes snapshots from tip if they are not in `ids`. + /// + /// Returns the IDs of the deleted snapshots. pub fn filter_tip_snapshots( &mut self, height: Height, ids: &BTreeSet, - ) -> Result<(), LayoutError> { + ) -> Result, LayoutError> { let tip = self.tip(height)?; let snapshots_on_disk = tip.snapshot_ids()?; + let mut deleted_snapshot_ids = Vec::new(); for id in snapshots_on_disk { if !ids.contains(&id) { - tip.snapshot(&id)?.delete_dir()?; + tip.delete_snapshot_dir(&id)?; + deleted_snapshot_ids.push(id); } } - Ok(()) + Ok(deleted_snapshot_ids) } /// Deletes the directory of the given canister from tip. @@ -546,6 +550,21 @@ impl TipHandler { tip.delete_canister_dir(&canister_id) } + /// Deletes the directory of the given snapshot from tip. + /// + /// This is a no-op if the snapshot has no directory in tip, e.g. because it was + /// created from uploaded metadata (which copies no files from the canister, so no + /// directory is created for it) and deleted before the first flush of its + /// `PageMap`s. + pub fn delete_snapshot_directory( + &mut self, + height: Height, + snapshot_id: SnapshotId, + ) -> Result<(), LayoutError> { + let tip = self.tip(height)?; + tip.delete_snapshot_dir(&snapshot_id) + } + /// Moves the entire canister directory from one canister id to another. pub fn move_canister_directory( &mut self, @@ -1886,16 +1905,19 @@ impl CheckpointLayout { &self, snapshot_id: &SnapshotId, ) -> Result, LayoutError> { - SnapshotLayout::new( - self.0 - .root - .join(SNAPSHOTS_DIR) - .join(hex::encode( - snapshot_id.get_canister_id().get_ref().as_slice(), - )) - .join(hex::encode(snapshot_id.as_slice())), - self, - ) + SnapshotLayout::new(self.snapshot_path(snapshot_id), self) + } + + /// The path of the given snapshot's directory. As opposed to `snapshot()`, this + /// does not create the directory. + fn snapshot_path(&self, snapshot_id: &SnapshotId) -> PathBuf { + self.0 + .root + .join(SNAPSHOTS_DIR) + .join(hex::encode( + snapshot_id.get_canister_id().get_ref().as_slice(), + )) + .join(hex::encode(snapshot_id.as_slice())) } pub fn height(&self) -> Height { @@ -2065,6 +2087,16 @@ where }), } } + + /// Removes the entire directory of the given snapshot; and the enclosing directory + /// named after the snapshot's canister, if this was the canister's last snapshot. + /// + /// This is a no-op if the snapshot has no directory, e.g. because it was created + /// from uploaded metadata (which copies no files from the canister, so no directory + /// is created for it) and deleted before the first flush of its `PageMap`s. + pub fn delete_snapshot_dir(&self, snapshot_id: &SnapshotId) -> Result<(), LayoutError> { + delete_snapshot_dir(&self.snapshot_path(snapshot_id)) + } } impl CheckpointLayout { @@ -2542,24 +2574,38 @@ where { /// Remove the entire directory for the snapshot. pub fn delete_dir(&self) -> Result<(), LayoutError> { - let map_error = |err| LayoutError::IoError { - path: self.raw_path(), - message: "Cannot remove snapshot.".to_string(), - io_err: err, - }; + delete_snapshot_dir(&self.raw_path()) + } +} - std::fs::remove_dir_all(self.raw_path()).map_err(map_error)?; +/// Removes the entire directory of a snapshot; and the enclosing directory named after +/// the snapshot's canister, if this was the canister's last snapshot. +/// +/// This is a no-op if the snapshot has no directory, e.g. because it was created from +/// uploaded metadata (which copies no files from the canister, so no directory is +/// created for it) and deleted before the first flush of its `PageMap`s. +fn delete_snapshot_dir(snapshot_path: &Path) -> Result<(), LayoutError> { + let map_error = |err| LayoutError::IoError { + path: snapshot_path.to_path_buf(), + message: "Cannot remove snapshot.".to_string(), + io_err: err, + }; - // Remove the parent directory named after the canister if this was the last snapshot of that canister. - // Unwrap is safe as snapshots are not at located at `/`. - let parent = self.raw_path().parent().unwrap().to_owned(); + match std::fs::remove_dir_all(snapshot_path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(map_error(err)), + } - if parent.read_dir().map_err(map_error)?.next().is_none() { - std::fs::remove_dir(&parent).map_err(map_error)?; - } + // Remove the parent directory named after the canister if this was the last snapshot of that canister. + // Unwrap is safe as snapshots are not located at `/`. + let parent = snapshot_path.parent().unwrap(); - Ok(()) + if parent.read_dir().map_err(map_error)?.next().is_none() { + std::fs::remove_dir(parent).map_err(map_error)?; } + + Ok(()) } fn open_for_write(path: &Path) -> Result { diff --git a/rs/state_layout/src/state_layout/tests.rs b/rs/state_layout/src/state_layout/tests.rs index fc3c45cd1319..73cde100f919 100644 --- a/rs/state_layout/src/state_layout/tests.rs +++ b/rs/state_layout/src/state_layout/tests.rs @@ -1220,6 +1220,70 @@ fn can_add_and_delete_canister_snapshots( } } +/// Tests that deleting a snapshot's directory is idempotent, i.e. that deleting a +/// snapshot that has no directory is a no-op rather than a `NotFound` I/O error. +/// +/// This is relied upon by the flush of `UnflushedCheckpointOp::DeleteSnapshot`: a +/// snapshot created from uploaded metadata has no directory in tip until its `PageMap`s +/// are first flushed, so deleting it before then would otherwise fail. +#[test] +fn delete_snapshot_dir_is_idempotent() { + let tmp = tmpdir("checkpoint"); + let checkpoint_layout: CheckpointLayout = + CheckpointLayout::new_untracked(tmp.path().to_owned(), Height::new(0)).unwrap(); + + let canister_id = canister_test_id(100); + let snapshot_id = SnapshotId::from((canister_id, 0)); + let other_snapshot_id = SnapshotId::from((canister_id, 1)); + + let snapshot_ids = || { + let mut snapshot_ids = checkpoint_layout.snapshot_ids().unwrap(); + snapshot_ids.sort(); + snapshot_ids + }; + let num_canister_dirs = || { + std::fs::read_dir(checkpoint_layout.raw_path().join(SNAPSHOTS_DIR)) + .unwrap() + .count() + }; + + // Deleting a snapshot that never had a directory is a no-op. + checkpoint_layout.delete_snapshot_dir(&snapshot_id).unwrap(); + assert!(snapshot_ids().is_empty()); + + // Create the directories of two snapshots of the same canister. + checkpoint_layout.snapshot(&snapshot_id).unwrap(); + checkpoint_layout.snapshot(&other_snapshot_id).unwrap(); + let mut expected_snapshot_ids = vec![snapshot_id, other_snapshot_id]; + expected_snapshot_ids.sort(); + assert_eq!(snapshot_ids(), expected_snapshot_ids); + assert_eq!(num_canister_dirs(), 1); + + // Deleting one of them retains the other, as well as the canister's directory. + checkpoint_layout.delete_snapshot_dir(&snapshot_id).unwrap(); + assert_eq!(snapshot_ids(), vec![other_snapshot_id]); + assert_eq!(num_canister_dirs(), 1); + + // And deleting it again is a no-op. + checkpoint_layout.delete_snapshot_dir(&snapshot_id).unwrap(); + assert_eq!(snapshot_ids(), vec![other_snapshot_id]); + assert_eq!(num_canister_dirs(), 1); + + // Deleting the canister's last snapshot also removes the canister's directory. + checkpoint_layout + .delete_snapshot_dir(&other_snapshot_id) + .unwrap(); + assert!(snapshot_ids().is_empty()); + assert_eq!(num_canister_dirs(), 0); + + // As is deleting it again. + checkpoint_layout + .delete_snapshot_dir(&other_snapshot_id) + .unwrap(); + assert!(snapshot_ids().is_empty()); + assert_eq!(num_canister_dirs(), 0); +} + #[test] fn test_encode_decode_empty_task_queue() { let task_queue = TaskQueue::default(); diff --git a/rs/state_manager/src/lib.rs b/rs/state_manager/src/lib.rs index 5e3456c5d512..14773fb699b0 100644 --- a/rs/state_manager/src/lib.rs +++ b/rs/state_manager/src/lib.rs @@ -129,6 +129,13 @@ const CRITICAL_ERROR_REPLICATED_STATE_ALTERED_AFTER_CHECKPOINT: &str = pub(crate) const CRITICAL_ERROR_TIP_CANISTERS_FILTERED: &str = "state_manager_tip_canisters_filtered"; +/// Critical error tracking snapshot directories unexpectedly removed from tip by +/// `TipRequest::FilterTipCanisters`, which is only meant to be a safety net: every +/// snapshot directory removal should be covered by an explicit +/// `UnflushedCheckpointOp::DeleteSnapshot`. +pub(crate) const CRITICAL_ERROR_TIP_SNAPSHOTS_FILTERED: &str = + "state_manager_tip_snapshots_filtered"; + /// How long to keep archived and diverged states. const ARCHIVED_DIVERGED_CHECKPOINT_MAX_AGE: Duration = Duration::from_secs(30 * 24 * 60 * 60); // 30 days @@ -244,6 +251,7 @@ pub struct CheckpointMetrics { load_checkpoint_soft_invariant_broken: IntCounter, replicated_state_altered_after_checkpoint: IntCounter, tip_canisters_filtered: IntCounter, + tip_snapshots_filtered: IntCounter, tip_handler_request_duration: HistogramVec, num_page_maps_by_load_status: IntGaugeVec, num_loaded_wasm_files_by_source: IntGaugeVec, @@ -284,6 +292,9 @@ impl CheckpointMetrics { let tip_canisters_filtered = metrics_registry.error_counter(CRITICAL_ERROR_TIP_CANISTERS_FILTERED); + let tip_snapshots_filtered = + metrics_registry.error_counter(CRITICAL_ERROR_TIP_SNAPSHOTS_FILTERED); + let tip_handler_request_duration = metrics_registry.histogram_vec( "state_manager_tip_handler_request_duration_seconds", "Duration to execute requests to Tip handling thread in seconds.", @@ -310,6 +321,7 @@ impl CheckpointMetrics { load_checkpoint_soft_invariant_broken, replicated_state_altered_after_checkpoint, tip_canisters_filtered, + tip_snapshots_filtered, tip_handler_request_duration, num_page_maps_by_load_status, num_loaded_wasm_files_by_source, diff --git a/rs/state_manager/src/tip.rs b/rs/state_manager/src/tip.rs index 546e02a2f39e..a6de0c57380f 100644 --- a/rs/state_manager/src/tip.rs +++ b/rs/state_manager/src/tip.rs @@ -1,6 +1,7 @@ use crate::{ CRITICAL_ERROR_CHUNK_ID_USAGE_NEARING_LIMITS, CRITICAL_ERROR_TIP_CANISTERS_FILTERED, - CheckpointError, NUMBER_OF_CHECKPOINT_THREADS, PageMapType, SharedState, StateManagerMetrics, + CRITICAL_ERROR_TIP_SNAPSHOTS_FILTERED, CheckpointError, NUMBER_OF_CHECKPOINT_THREADS, + PageMapType, SharedState, StateManagerMetrics, checkpoint::validate_and_finalize_checkpoint_and_remove_unverified_marker, compute_bundled_manifest, manifest::{BaseManifestInfo, RehashManifest}, @@ -107,14 +108,13 @@ pub(crate) enum TipRequest { /// Filter canisters and snapshots in tip. Remove ones not present in the sets. /// /// Canisters deleted during execution and canisters dropped by a subnet split are - /// removed from tip via `UnflushedCheckpointOp::DeleteCanister`, so this is only a - /// safety net for canisters that disappeared from the state without a corresponding - /// operation. Actually removing a canister directory here therefore raises the - /// `CRITICAL_ERROR_TIP_CANISTERS_FILTERED` critical error. - /// - /// Snapshot deletions, on the other hand, are not recorded as checkpoint - /// operations, so filtering is the regular mechanism for removing the directories - /// of deleted snapshots from tip. + /// removed from tip via `UnflushedCheckpointOp::DeleteCanister`; snapshots deleted + /// during execution (explicitly, or along with their canister) are removed from tip + /// via `UnflushedCheckpointOp::DeleteSnapshot`. So this is only a safety net for + /// canisters and snapshots that disappeared from the state without a corresponding + /// operation. Actually removing a canister or snapshot directory here therefore + /// raises the `CRITICAL_ERROR_TIP_CANISTERS_FILTERED` resp. + /// `CRITICAL_ERROR_TIP_SNAPSHOTS_FILTERED` critical error. /// /// State: `tip_folder_state.has_filtered_canisters = true` FilterTipCanisters { @@ -330,7 +330,7 @@ pub(crate) fn spawn_tip_thread( ); metrics.checkpoint_metrics.tip_canisters_filtered.inc(); } - tip_handler + let filtered_snapshot_ids = tip_handler .filter_tip_snapshots(height, &snapshot_ids) .unwrap_or_else(|err| { fatal!( @@ -340,6 +340,19 @@ pub(crate) fn spawn_tip_thread( err ) }); + if !filtered_snapshot_ids.is_empty() { + // Every snapshot directory removal should be covered by an + // explicit `UnflushedCheckpointOp::DeleteSnapshot`, making this + // a mere safety net. + error!( + log, + "{}: Removed snapshot directories without a corresponding checkpoint operation at height @{}: {:?}", + CRITICAL_ERROR_TIP_SNAPSHOTS_FILTERED, + height, + filtered_snapshot_ids, + ); + metrics.checkpoint_metrics.tip_snapshots_filtered.inc(); + } } TipRequest::TipToCheckpointAndSwitch { @@ -842,7 +855,7 @@ fn switch_to_checkpoint( } /// Update the tip directory files with the most recent checkpoint operations. -/// `operations` is an ordered list of all created/restored snapshots and renamed or deleted canisters since the last flush. +/// `operations` is an ordered list of all created/restored/deleted snapshots and renamed or deleted canisters since the last flush. fn flush_unflushed_checkpoint_ops( log: &ReplicaLogger, tip_handler: &mut TipHandler, @@ -864,6 +877,9 @@ fn flush_unflushed_checkpoint_ops( UnflushedCheckpointOp::DeleteCanister(canister_id) => { tip_handler.delete_canister_directory(height, canister_id)?; } + UnflushedCheckpointOp::DeleteSnapshot(snapshot_id) => { + tip_handler.delete_snapshot_directory(height, snapshot_id)?; + } } } @@ -1269,8 +1285,9 @@ fn serialize_protos_to_checkpoint_readwrite( /// with no logs. /// /// Any page deltas (for canisters or snapshots) have already been persisted via -/// a `FlushPageMapDelta` request by this point. And files for deleted canisters -/// and snapshots have been deleted via `FilterTipCanisters`. +/// a `FlushPageMapDelta` request by this point. And the directories of deleted +/// canisters and snapshots have been deleted by the flush of the corresponding +/// `UnflushedCheckpointOp`s (with `FilterTipCanisters` as a safety net). fn serialize_wasm_binaries( state: &ReplicatedState, tip: &CheckpointLayout>, diff --git a/rs/state_manager/tests/state_manager.rs b/rs/state_manager/tests/state_manager.rs index 50941392ca84..b637db453973 100644 --- a/rs/state_manager/tests/state_manager.rs +++ b/rs/state_manager/tests/state_manager.rs @@ -17,7 +17,7 @@ use ic_logger::replica_logger::no_op_logger; use ic_management_canister_types_private::{ CanisterChangeDetails, CanisterChangeOrigin, CanisterInstallModeV2, CanisterSnapshotDataKind, InstallChunkedCodeArgs, LoadCanisterSnapshotArgs, ReadCanisterSnapshotDataArgs, - TakeCanisterSnapshotArgs, UploadChunkArgs, + TakeCanisterSnapshotArgs, UploadCanisterSnapshotMetadataArgs, UploadChunkArgs, }; use ic_metrics::MetricsRegistry; use ic_registry_routing_table::{CANISTER_IDS_PER_SUBNET, CanisterIdRange, RoutingTable}; @@ -26,13 +26,13 @@ use ic_registry_subnet_type::SubnetType; use ic_replicated_state::{ ExecutionState, ExportedFunctions, Memory, NetworkTopology, NumWasmPages, PageMap, ReplicatedState, Stream, SubnetTopology, - canister_state::canister_snapshots::CanisterSnapshot, + canister_state::canister_snapshots::{CanisterSnapshot, ValidatedSnapshotMetadata}, canister_state::{execution_state::WasmBinary, system_state::wasm_chunk_store::WasmChunkStore}, metadata_state::{ - ApiBoundaryNodeEntry, UnflushedCheckpointOp, + ApiBoundaryNodeEntry, UnflushedCheckpointOp, UnflushedCheckpointOps, testing::{NetworkTopologyTesting, SystemMetadataTesting}, }, - page_map::{PageIndex, Shard, StorageLayout}, + page_map::{PageIndex, Shard, StorageLayout, TestPageAllocatorFileDescriptorImpl}, testing::{ReplicatedStateTesting, StreamTesting, SystemStateTesting}, }; use ic_state_layout::{ @@ -293,6 +293,23 @@ fn take_canister_snapshot( state.put_canister_state(canister_arc); } +/// Deletes the given snapshot from the state, recording the deletion as an unflushed +/// checkpoint operation (as `CanisterManager::delete_canister_snapshot()` does). +fn delete_canister_snapshot(state: &mut ReplicatedState, snapshot_id: SnapshotId) { + let canister_id = snapshot_id.get_canister_id(); + let mut canister_arc = state.take_canister_state(&canister_id).unwrap(); + let canister = Arc::make_mut(&mut canister_arc); + let mut unflushed_checkpoint_ops = UnflushedCheckpointOps::default(); + canister + .canister_snapshots + .remove(snapshot_id, &mut unflushed_checkpoint_ops); + state + .metadata + .unflushed_checkpoint_ops + .extend(unflushed_checkpoint_ops); + state.put_canister_state(canister_arc); +} + #[test] fn lsmt_merge_overhead() { fn checkpoint_size(checkpoint: &CheckpointLayout) -> f64 { @@ -7606,10 +7623,7 @@ fn can_create_and_delete_canister_snapshot() { let (_height, mut state) = state_manager.take_tip(); - let canister = state - .canister_state_make_mut(&canister_test_id(100)) - .unwrap(); - canister.canister_snapshots.remove(snapshot_id); + delete_canister_snapshot(&mut state, snapshot_id); state_manager.commit_and_certify(state, CertificationScope::Full, None); state_manager.flush_tip_channel(); @@ -8626,6 +8640,243 @@ fn deleted_canister_is_removed_from_tip() { deleted_canister_is_removed_from_tip_impl(CertificationScope::Full); } +/// Tests that a deleted snapshot is removed from the tip by the flush of the recorded +/// `UnflushedCheckpointOp::DeleteSnapshot`, i.e. without relying on +/// `FilterTipCanisters`. +#[test] +fn deleted_snapshot_is_removed_from_tip() { + fn deleted_snapshot_is_removed_from_tip_impl(certification_scope: CertificationScope) { + state_manager_test(|_metrics, state_manager| { + let canister_id = canister_test_id(100); + let snapshot_id = SnapshotId::from((canister_id, 0)); + + // Install a canister, take a snapshot of it and checkpoint the state, so + // that the snapshot has a directory in the tip. + let (_height, mut state) = state_manager.take_tip(); + insert_dummy_canister(&mut state, canister_id); + let snapshot = CanisterSnapshot::from_canister( + state.canister_state(&canister_id).unwrap(), + state.time(), + ) + .unwrap(); + take_canister_snapshot(&mut state, canister_id, snapshot_id, snapshot); + state_manager.commit_and_certify(state, CertificationScope::Full, None); + state_manager.flush_tip_channel(); + + let (height, mut state) = state_manager.take_tip(); + let tip = CheckpointLayout::::new_untracked( + state_manager.state_layout().raw_path().join("tip"), + height, + ) + .unwrap(); + assert_eq!(tip.snapshot_ids().unwrap(), vec![snapshot_id]); + + delete_canister_snapshot(&mut state, snapshot_id); + assert_eq!( + state + .system_metadata() + .unflushed_checkpoint_ops + .clone() + .take(), + vec![UnflushedCheckpointOp::DeleteSnapshot(snapshot_id)] + ); + + // Trigger a flush either at the checkpoint or by committing exactly + // `NUM_ROUNDS_BEFORE_CHECKPOINT_TO_WRITE_OVERLAY` rounds before the checkpoint. + if certification_scope == CertificationScope::Full { + state_manager.commit_and_certify(state, certification_scope.clone(), None); + } else { + state_manager.commit_and_certify( + state, + certification_scope.clone(), + Some(BatchSummary { + next_checkpoint_height: Height( + 2 + NUM_ROUNDS_BEFORE_CHECKPOINT_TO_WRITE_OVERLAY, + ), + current_interval_length: Height(500), + }), + ); + } + state_manager.flush_tip_channel(); + + // The snapshot directory is gone from the tip, even without a checkpoint; + // and so is the canister's directory under `snapshots`. + assert!(tip.snapshot_ids().unwrap().is_empty()); + // But the canister itself is still there. + assert_eq!(tip.canister_ids().unwrap(), vec![canister_id]); + // And the checkpoint op has been flushed. + let (_height, state) = state_manager.take_tip(); + assert!(state.system_metadata().unflushed_checkpoint_ops.is_empty()); + }); + } + deleted_snapshot_is_removed_from_tip_impl(CertificationScope::Metadata); + deleted_snapshot_is_removed_from_tip_impl(CertificationScope::Full); +} + +/// Tests that the flush of `UnflushedCheckpointOp::DeleteSnapshot` is idempotent, i.e. +/// that it does not fail with an I/O error if the snapshot has no directory in the tip. +/// +/// This is the case for a snapshot created from uploaded metadata (see +/// `CanisterManager::create_snapshot_from_metadata()`, which records no +/// `UnflushedCheckpointOp::TakeSnapshot` as there is nothing to copy from the canister) +/// and deleted before the first `TipRequest::FlushPageMapDelta` that still sees it in +/// the state, which is what would have created its directory: the snapshot's `PageMap`s +/// start out with no files in tip, so they are always flushed (and hence their layout, +/// and with it the snapshot's directory, created) even though they hold no data. +#[test] +fn deleting_snapshot_without_tip_directory_is_a_noop() { + state_manager_test(|metrics, state_manager| { + let canister_id = canister_test_id(100); + let snapshot_id = SnapshotId::from((canister_id, 0)); + + // Install a canister and checkpoint the state. + let (_height, mut state) = state_manager.take_tip(); + insert_dummy_canister(&mut state, canister_id); + state_manager.commit_and_certify(state, CertificationScope::Full, None); + state_manager.flush_tip_channel(); + + let (height, mut state) = state_manager.take_tip(); + let tip = CheckpointLayout::::new_untracked( + state_manager.state_layout().raw_path().join("tip"), + height, + ) + .unwrap(); + assert!(tip.snapshot_ids().unwrap().is_empty()); + + // Create a snapshot from uploaded metadata, as + // `CanisterManager::create_snapshot_from_metadata()` does: its `PageMap`s are + // brand new (as opposed to a snapshot taken from a canister, whose `PageMap`s + // are clones of the canister's), so they hold no data and have no files in tip. + // Add it without recording an `UnflushedCheckpointOp::TakeSnapshot`, and delete + // it again within the same round, i.e. before any flush sees it in the state. + // Its directory is therefore never created in the tip. + let metadata = + ValidatedSnapshotMetadata::validate(UploadCanisterSnapshotMetadataArgs::new( + canister_id, + None, + 4, // wasm_module_size + vec![], + 0, // wasm_memory_size + 0, // stable_memory_size + vec![], + None, + None, + )) + .unwrap(); + let snapshot = CanisterSnapshot::from_metadata( + &metadata, + state.time(), + state + .canister_state(&canister_id) + .unwrap() + .system_state + .canister_version(), + Arc::new(TestPageAllocatorFileDescriptorImpl::new()), + ); + let canister = state.canister_state_make_mut(&canister_id).unwrap(); + canister + .canister_snapshots + .push(snapshot_id, Arc::new(snapshot)); + delete_canister_snapshot(&mut state, snapshot_id); + assert_eq!( + state + .system_metadata() + .unflushed_checkpoint_ops + .clone() + .take(), + vec![UnflushedCheckpointOp::DeleteSnapshot(snapshot_id)] + ); + + // Flushing the delete operation for the snapshot without a directory in the tip + // must not fail (it would `fatal!` the tip thread, killing the test). + state_manager.commit_and_certify( + state, + CertificationScope::Metadata, + Some(BatchSummary { + next_checkpoint_height: Height(2 + NUM_ROUNDS_BEFORE_CHECKPOINT_TO_WRITE_OVERLAY), + current_interval_length: Height(500), + }), + ); + state_manager.flush_tip_channel(); + + assert!(tip.snapshot_ids().unwrap().is_empty()); + let (_height, state) = state_manager.take_tip(); + assert!(state.system_metadata().unflushed_checkpoint_ops.is_empty()); + + // And the subsequent checkpoint succeeds, without raising a critical error. + state_manager.commit_and_certify(state, CertificationScope::Full, None); + state_manager.flush_tip_channel(); + assert!(tip.snapshot_ids().unwrap().is_empty()); + assert_error_counters(metrics); + }); +} + +/// Tests that the snapshots of a deleted canister are removed from the tip by the flush +/// of the recorded `UnflushedCheckpointOp::DeleteSnapshot`s, i.e. without relying on +/// `FilterTipCanisters`. +#[test] +fn snapshots_of_deleted_canister_are_removed_from_tip() { + state_manager_test(|_metrics, state_manager| { + let canister_id = canister_test_id(100); + let snapshot_id = SnapshotId::from((canister_id, 0)); + + // Install a canister, take a snapshot of it and checkpoint the state, so that + // both have a directory in the tip. + let (_height, mut state) = state_manager.take_tip(); + insert_dummy_canister(&mut state, canister_id); + let snapshot = CanisterSnapshot::from_canister( + state.canister_state(&canister_id).unwrap(), + state.time(), + ) + .unwrap(); + take_canister_snapshot(&mut state, canister_id, snapshot_id, snapshot); + state_manager.commit_and_certify(state, CertificationScope::Full, None); + state_manager.flush_tip_channel(); + + let (height, mut state) = state_manager.take_tip(); + let tip = CheckpointLayout::::new_untracked( + state_manager.state_layout().raw_path().join("tip"), + height, + ) + .unwrap(); + assert_eq!(tip.canister_ids().unwrap(), vec![canister_id]); + assert_eq!(tip.snapshot_ids().unwrap(), vec![snapshot_id]); + + // Deleting the canister records the deletion of its snapshot, too. + state.remove_canister(&canister_id).unwrap(); + assert_eq!( + state + .system_metadata() + .unflushed_checkpoint_ops + .clone() + .take(), + vec![ + UnflushedCheckpointOp::DeleteSnapshot(snapshot_id), + UnflushedCheckpointOp::DeleteCanister(canister_id), + ] + ); + + // Commit without a checkpoint, but flush the operations by committing exactly + // `NUM_ROUNDS_BEFORE_CHECKPOINT_TO_WRITE_OVERLAY` rounds before the checkpoint. + state_manager.commit_and_certify( + state, + CertificationScope::Metadata, + Some(BatchSummary { + next_checkpoint_height: Height(2 + NUM_ROUNDS_BEFORE_CHECKPOINT_TO_WRITE_OVERLAY), + current_interval_length: Height(500), + }), + ); + state_manager.flush_tip_channel(); + + // Both directories are gone from the tip, even without a checkpoint. + assert!(tip.canister_ids().unwrap().is_empty()); + assert!(tip.snapshot_ids().unwrap().is_empty()); + // And the checkpoint ops have been flushed. + let (_height, state) = state_manager.take_tip(); + assert!(state.system_metadata().unflushed_checkpoint_ops.is_empty()); + }); +} + /// Tests that `FilterTipCanisters` raises a critical error if it actually removes a /// canister directory from tip: every such removal is expected to be covered by an /// explicit `UnflushedCheckpointOp::DeleteCanister`, so filtering is only a safety net. @@ -8669,6 +8920,58 @@ fn filtering_canister_from_tip_raises_critical_error() { }); } +/// Tests that `FilterTipCanisters` raises a critical error if it actually removes a +/// snapshot directory from tip: every such removal is expected to be covered by an +/// explicit `UnflushedCheckpointOp::DeleteSnapshot`, so filtering is only a safety net. +#[test] +fn filtering_snapshot_from_tip_raises_critical_error() { + state_manager_test(|metrics, state_manager| { + let canister_id = canister_test_id(100); + let snapshot_id = SnapshotId::from((canister_id, 0)); + + // Install a canister, take a snapshot of it and checkpoint the state, so that + // the snapshot has a directory in the tip. + let (_height, mut state) = state_manager.take_tip(); + insert_dummy_canister(&mut state, canister_id); + let snapshot = CanisterSnapshot::from_canister( + state.canister_state(&canister_id).unwrap(), + state.time(), + ) + .unwrap(); + take_canister_snapshot(&mut state, canister_id, snapshot_id, snapshot); + state_manager.commit_and_certify(state, CertificationScope::Full, None); + state_manager.flush_tip_channel(); + + let (height, mut state) = state_manager.take_tip(); + let tip = CheckpointLayout::::new_untracked( + state_manager.state_layout().raw_path().join("tip"), + height, + ) + .unwrap(); + assert_eq!(tip.snapshot_ids().unwrap(), vec![snapshot_id]); + assert_error_counters(metrics); + + // Drop the snapshot from the state without recording a `DeleteSnapshot` + // operation (by discarding the operations recorded by `remove()`), so that its + // directory is only removed by `FilterTipCanisters`. + let canister = state.canister_state_make_mut(&canister_id).unwrap(); + canister + .canister_snapshots + .remove(snapshot_id, &mut UnflushedCheckpointOps::default()); + assert!(state.system_metadata().unflushed_checkpoint_ops.is_empty()); + + state_manager.commit_and_certify(state, CertificationScope::Full, None); + state_manager.flush_tip_channel(); + + // The snapshot directory is gone from the tip, but a critical error was raised. + assert!(tip.snapshot_ids().unwrap().is_empty()); + assert_eq!( + metric_vec(&[(&[("error", "state_manager_tip_snapshots_filtered")], 1)]), + nonzero_values(fetch_int_counter_vec(metrics, "critical_errors")) + ); + }); +} + #[test_strategy::proptest(ProptestConfig { cases: 20, ..ProptestConfig::default() })] fn stream_store_encode_decode( #[strategy(arb_stream(