From 5b4a6e3a5e984438fa35d80f79ca830cacbf4c70 Mon Sep 17 00:00:00 2001 From: Andriy Berestovskyy Date: Sat, 28 Sep 2024 11:24:35 +0200 Subject: [PATCH] test: EXC-1652: Future proof canister snapshots (#1677) Introducing small functions that requires developers to consider the potential impact of adding or removing canister state fields on the canister snapshot logic. This covers changes in CanisterState, SystemState, ExecutionState, WasmBinary and SchedulerState. --- .../tests/canister_snapshots.rs | 71 ++++++++++++++++++- .../src/canister_state/system_state.rs | 44 ++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) 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 c6c5a0edf32..c5dea329c6d 100644 --- a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs @@ -13,7 +13,9 @@ use ic_management_canister_types::{ }; use ic_registry_subnet_type::SubnetType; use ic_replicated_state::{ - canister_snapshots::SnapshotOperation, canister_state::system_state::CyclesUseCase, + canister_snapshots::SnapshotOperation, + canister_state::{execution_state::WasmBinary, system_state::CyclesUseCase}, + CanisterState, ExecutionState, SchedulerState, }; use ic_test_utilities_execution_environment::{ get_output_messages, ExecutionTest, ExecutionTestBuilder, @@ -28,6 +30,7 @@ use ic_types::{ use ic_universal_canister::{call_args, wasm, UNIVERSAL_CANISTER_WASM}; use more_asserts::assert_gt; use serde_bytes::ByteBuf; +use std::borrow::Borrow; #[test] fn take_canister_snapshot_decode_round_trip() { @@ -1908,3 +1911,69 @@ fn snapshot_must_include_globals() { .unwrap(); assert_eq!(result, WasmResult::Reply(vec![1, 0, 0, 0])); } + +/// Early warning system / stumbling block forcing the authors of changes adding +/// or removing canister state fields to think about and/or ask the Execution +/// team to think about any repercussions to the canister snapshot logic. +/// +/// If you do find yourself having to make changes to this function, it is quite +/// possible that you have not broken anything. But there is a non-zero chance +/// for changes to the structure of the canister state to also require changes +/// to the canister snapshot logic or risk breaking it. Which is why this brute +/// force check exists. +/// +/// See `CanisterSnapshot::from_canister()` for more context. +#[allow(dead_code)] +fn canister_snapshot_change_guard_do_not_modify_without_reading_doc_comment() { + let mut test = ExecutionTestBuilder::new().build(); + let uc = test.universal_canister().unwrap(); + let canister_state = test.canister_state(uc).clone(); + + // + // DO NOT MODIFY WITHOUT READING DOC COMMENT! + // + let CanisterState { + // There is a separate test for SystemState. + system_state: _, + execution_state, + scheduler_state, + } = canister_state; + + // + // DO NOT MODIFY WITHOUT READING DOC COMMENT! + // + let ExecutionState { + canister_root: _, + wasm_binary, + wasm_memory: _, + stable_memory: _, + exported_globals: _, + exports: _, + metadata: _, + last_executed_round: _, + next_scheduled_method: _, + } = execution_state.unwrap(); + + // + // DO NOT MODIFY WITHOUT READING DOC COMMENT! + // + let WasmBinary { + binary: _, + embedder_cache: _, + } = wasm_binary.borrow(); + + // + // DO NOT MODIFY WITHOUT READING DOC COMMENT! + // + let SchedulerState { + last_full_execution_round: _, + compute_allocation: _, + accumulated_priority: _, + priority_credit: _, + long_execution_mode: _, + heap_delta_debit: _, + install_code_debit: _, + time_of_last_allocation_charge: _, + total_query_stats: _, + } = scheduler_state; +} diff --git a/rs/replicated_state/src/canister_state/system_state.rs b/rs/replicated_state/src/canister_state/system_state.rs index 3016c916631..1808c52a794 100644 --- a/rs/replicated_state/src/canister_state/system_state.rs +++ b/rs/replicated_state/src/canister_state/system_state.rs @@ -1767,4 +1767,48 @@ pub mod testing { self.split_input_schedules(own_canister_id, local_canisters) } } + + /// Early warning system / stumbling block forcing the authors of changes adding + /// or removing system state fields to think about and/or ask the Execution + /// team to think about any repercussions to the canister snapshot logic. + /// + /// If you do find yourself having to make changes to this function, it is quite + /// possible that you have not broken anything. But there is a non-zero chance + /// for changes to the structure of the system state to also require changes + /// to the canister snapshot logic or risk breaking it. Which is why this brute + /// force check exists. + /// + /// See `CanisterSnapshot::from_canister()` for more context. + #[allow(dead_code)] + fn canister_snapshot_change_guard_do_not_modify_without_reading_doc_comment() { + // + // DO NOT MODIFY WITHOUT READING DOC COMMENT! + // + let _system_state = SystemState { + controllers: Default::default(), + canister_id: 0.into(), + queues: Default::default(), + memory_allocation: Default::default(), + wasm_memory_threshold: Default::default(), + freeze_threshold: Default::default(), + status: CanisterStatus::Stopped, + certified_data: Default::default(), + canister_metrics: Default::default(), + cycles_balance: Default::default(), + ingress_induction_cycles_debit: Default::default(), + reserved_balance: Default::default(), + reserved_balance_limit: Default::default(), + task_queue: Default::default(), + global_timer: CanisterTimer::Inactive, + canister_version: Default::default(), + canister_history: Default::default(), + wasm_chunk_store: WasmChunkStore::new_for_testing(), + log_visibility: Default::default(), + canister_log: Default::default(), + wasm_memory_limit: Default::default(), + next_snapshot_id: Default::default(), + snapshots_memory_usage: Default::default(), + on_low_wasm_memory_hook_status: Default::default(), + }; + } }