diff --git a/rs/execution_environment/src/execution/install_code.rs b/rs/execution_environment/src/execution/install_code.rs index 98076b144b3..d4e5754305d 100644 --- a/rs/execution_environment/src/execution/install_code.rs +++ b/rs/execution_environment/src/execution/install_code.rs @@ -11,7 +11,7 @@ use ic_interfaces::execution_environment::{ }; use ic_logger::{error, fatal, info, warn}; use ic_management_canister_types::{ - CanisterChangeDetails, CanisterChangeOrigin, CanisterInstallModeV2, CanisterLog, + CanisterChangeDetails, CanisterChangeOrigin, CanisterInstallModeV2, }; use ic_replicated_state::canister_state::system_state::ReservationError; use ic_replicated_state::metadata_state::subnet_call_context_manager::InstallCodeCallId; @@ -20,7 +20,7 @@ use ic_state_layout::{CanisterLayout, CheckpointLayout, ReadOnly}; use ic_sys::PAGE_SIZE; use ic_system_api::ExecutionParameters; use ic_types::{ - funds::Cycles, messages::CanisterCall, CanisterTimer, ComputeAllocation, Height, + funds::Cycles, messages::CanisterCall, CanisterLog, CanisterTimer, ComputeAllocation, Height, MemoryAllocation, NumInstructions, Time, }; use ic_wasm_types::WasmHash; diff --git a/rs/execution_environment/tests/canister_logging.rs b/rs/execution_environment/tests/canister_logging.rs index 8887c516686..20f268e0e13 100644 --- a/rs/execution_environment/tests/canister_logging.rs +++ b/rs/execution_environment/tests/canister_logging.rs @@ -5,7 +5,7 @@ use ic_config::subnet_config::SubnetConfig; use ic_management_canister_types::{ CanisterIdRecord, CanisterInstallMode, CanisterLogRecord, CanisterSettingsArgs, CanisterSettingsArgsBuilder, DataSize, FetchCanisterLogsRequest, FetchCanisterLogsResponse, - LogVisibility, Payload, MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE, + LogVisibility, Payload, }; use ic_registry_subnet_type::SubnetType; use ic_state_machine_tests::{ @@ -14,7 +14,9 @@ use ic_state_machine_tests::{ }; use ic_test_utilities_execution_environment::{get_reply, wat_canister, wat_fn}; use ic_test_utilities_metrics::fetch_histogram_stats; -use ic_types::{ingress::WasmResult, CanisterId, Cycles, NumInstructions}; +use ic_types::{ + ingress::WasmResult, CanisterId, Cycles, NumInstructions, MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE, +}; use more_asserts::{assert_le, assert_lt}; use proptest::{prelude::ProptestConfig, prop_assume}; use std::time::{Duration, SystemTime}; diff --git a/rs/interfaces/src/execution_environment.rs b/rs/interfaces/src/execution_environment.rs index a5869a3f40c..822d4216d68 100644 --- a/rs/interfaces/src/execution_environment.rs +++ b/rs/interfaces/src/execution_environment.rs @@ -4,7 +4,7 @@ mod errors; pub use errors::{CanisterOutOfCyclesError, HypervisorError, TrapCode}; use ic_base_types::NumBytes; use ic_error_types::UserError; -use ic_management_canister_types::{CanisterLog, EcdsaKeyId}; +use ic_management_canister_types::EcdsaKeyId; use ic_registry_provisional_whitelist::ProvisionalWhitelist; use ic_registry_subnet_type::SubnetType; use ic_sys::{PageBytes, PageIndex}; @@ -16,12 +16,14 @@ use ic_types::{ AnonymousQuery, AnonymousQueryResponse, CertificateDelegation, MessageId, SignedIngressContent, UserQuery, }, - Cycles, ExecutionRound, Height, NumInstructions, NumOsPages, Randomness, Time, + CanisterLog, Cycles, ExecutionRound, Height, NumInstructions, NumOsPages, Randomness, Time, }; use serde::{Deserialize, Serialize}; -use std::{collections::BTreeMap, ops}; -use std::{collections::BTreeSet, convert::TryFrom}; -use std::{convert::Infallible, fmt}; +use std::{ + collections::{BTreeMap, BTreeSet}, + convert::{Infallible, TryFrom}, + fmt, ops, +}; use tower::util::BoxCloneService; /// Instance execution statistics. The stats are cumulative and diff --git a/rs/replicated_state/src/canister_state.rs b/rs/replicated_state/src/canister_state.rs index af32dfc1032..aafef70d446 100644 --- a/rs/replicated_state/src/canister_state.rs +++ b/rs/replicated_state/src/canister_state.rs @@ -8,7 +8,7 @@ use crate::canister_state::queues::CanisterOutputQueuesIterator; use crate::canister_state::system_state::{CanisterStatus, ExecutionTask, SystemState}; use crate::{InputQueueType, StateError}; pub use execution_state::{EmbedderCache, ExecutionState, ExportedFunctions, Global}; -use ic_management_canister_types::{CanisterLog, CanisterStatusType, LogVisibility}; +use ic_management_canister_types::{CanisterStatusType, LogVisibility}; use ic_registry_subnet_type::SubnetType; use ic_types::batch::TotalQueryStats; use ic_types::methods::SystemMethod; @@ -16,8 +16,8 @@ use ic_types::time::UNIX_EPOCH; use ic_types::{ messages::{CanisterMessage, Ingress, Request, RequestOrResponse, Response}, methods::WasmMethod, - AccumulatedPriority, CanisterId, ComputeAllocation, ExecutionRound, MemoryAllocation, NumBytes, - PrincipalId, Time, + AccumulatedPriority, CanisterId, CanisterLog, ComputeAllocation, ExecutionRound, + MemoryAllocation, NumBytes, PrincipalId, Time, }; use ic_types::{LongExecutionMode, NumInstructions}; use phantom_newtype::AmountOf; diff --git a/rs/replicated_state/src/canister_state/system_state.rs b/rs/replicated_state/src/canister_state/system_state.rs index a20660fe28a..f018da4e9fe 100644 --- a/rs/replicated_state/src/canister_state/system_state.rs +++ b/rs/replicated_state/src/canister_state/system_state.rs @@ -12,7 +12,7 @@ pub use call_context_manager::{CallContext, CallContextAction, CallContextManage use ic_base_types::NumSeconds; use ic_logger::{error, ReplicaLogger}; use ic_management_canister_types::{ - CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, CanisterLog, LogVisibility, + CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, LogVisibility, }; use ic_protobuf::proxy::{try_from_option_field, ProxyDecodeError}; use ic_protobuf::state::canister_state_bits::v1 as pb; @@ -22,7 +22,9 @@ use ic_types::messages::{ Request, RequestOrResponse, Response, StopCanisterContext, }; use ic_types::nominal_cycles::NominalCycles; -use ic_types::{CanisterId, CanisterTimer, Cycles, MemoryAllocation, NumBytes, PrincipalId, Time}; +use ic_types::{ + CanisterId, CanisterLog, CanisterTimer, Cycles, MemoryAllocation, NumBytes, PrincipalId, Time, +}; use lazy_static::lazy_static; use maplit::btreeset; use prometheus::IntCounter; diff --git a/rs/state_layout/src/state_layout.rs b/rs/state_layout/src/state_layout.rs index 63aa61d1a58..b16746a0fa8 100644 --- a/rs/state_layout/src/state_layout.rs +++ b/rs/state_layout/src/state_layout.rs @@ -4,7 +4,7 @@ use crate::utils::do_copy; use ic_base_types::{NumBytes, NumSeconds}; use ic_config::flag_status::FlagStatus; use ic_logger::{error, info, warn, ReplicaLogger}; -use ic_management_canister_types::{CanisterLog, LogVisibility}; +use ic_management_canister_types::LogVisibility; use ic_metrics::{buckets::decimal_buckets, MetricsRegistry}; use ic_protobuf::{ proxy::{try_from_option_field, ProxyDecodeError}, @@ -25,8 +25,8 @@ use ic_replicated_state::{ use ic_sys::{fs::sync_path, mmap::ScopedMmap}; use ic_types::{ batch::TotalQueryStats, nominal_cycles::NominalCycles, AccumulatedPriority, CanisterId, - ComputeAllocation, Cycles, ExecutionRound, Height, MemoryAllocation, NumInstructions, - PrincipalId, SnapshotId, Time, + CanisterLog, ComputeAllocation, Cycles, ExecutionRound, Height, MemoryAllocation, + NumInstructions, PrincipalId, SnapshotId, Time, }; use ic_utils::thread::parallel_map; use ic_wasm_types::{CanisterModule, WasmHash}; diff --git a/rs/state_machine_tests/src/lib.rs b/rs/state_machine_tests/src/lib.rs index 7503f3d4d49..ea98e3c4e55 100644 --- a/rs/state_machine_tests/src/lib.rs +++ b/rs/state_machine_tests/src/lib.rs @@ -33,7 +33,7 @@ use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::{CertificationScope, StateHashError, StateManager, StateReader}; use ic_logger::ReplicaLogger; use ic_management_canister_types::{ - self as ic00, CanisterIdRecord, CanisterLog, InstallCodeArgs, Method, Payload, + self as ic00, CanisterIdRecord, InstallCodeArgs, Method, Payload, }; pub use ic_management_canister_types::{ CanisterHttpResponsePayload, CanisterInstallMode, CanisterSettingsArgs, CanisterStatusResultV2, @@ -114,7 +114,7 @@ use ic_types::{ SignedIngressContent, UserQuery, EXPECTED_MESSAGE_ID_LENGTH, }, xnet::StreamIndex, - CountBytes, CryptoHashOfPartialState, Height, NodeId, Randomness, RegistryVersion, + CanisterLog, CountBytes, CryptoHashOfPartialState, Height, NodeId, Randomness, RegistryVersion, }; pub use ic_types::{ ingress::{IngressState, IngressStatus, WasmResult}, diff --git a/rs/system_api/src/lib.rs b/rs/system_api/src/lib.rs index 6d264ef365a..119863b3758 100644 --- a/rs/system_api/src/lib.rs +++ b/rs/system_api/src/lib.rs @@ -16,7 +16,6 @@ use ic_interfaces::execution_environment::{ TrapCode::{self, CyclesAmountTooBigFor64Bit}, }; use ic_logger::{error, ReplicaLogger}; -use ic_management_canister_types::CanisterLog; use ic_registry_subnet_type::SubnetType; use ic_replicated_state::{ canister_state::WASM_PAGE_SIZE_IN_BYTES, memory_required_to_push_request, Memory, NumWasmPages, @@ -27,7 +26,7 @@ use ic_types::{ ingress::WasmResult, messages::{CallContextId, RejectContext, Request, MAX_INTER_CANISTER_PAYLOAD_IN_BYTES}, methods::{SystemMethod, WasmClosure}, - CanisterId, CanisterTimer, ComputeAllocation, Cycles, MemoryAllocation, NumBytes, + CanisterId, CanisterLog, CanisterTimer, ComputeAllocation, Cycles, MemoryAllocation, NumBytes, NumInstructions, NumOsPages, PrincipalId, SubnetId, Time, MAX_STABLE_MEMORY_IN_BYTES, }; use ic_utils::deterministic_operations::deterministic_copy_from_slice; diff --git a/rs/system_api/src/sandbox_safe_system_state.rs b/rs/system_api/src/sandbox_safe_system_state.rs index 17386c46644..f2d2cf067f1 100644 --- a/rs/system_api/src/sandbox_safe_system_state.rs +++ b/rs/system_api/src/sandbox_safe_system_state.rs @@ -10,9 +10,8 @@ use ic_error_types::{ErrorCode, RejectCode, UserError}; use ic_interfaces::execution_environment::{HypervisorError, HypervisorResult}; use ic_logger::{info, ReplicaLogger}; use ic_management_canister_types::{ - CanisterLog, CreateCanisterArgs, InstallChunkedCodeArgs, InstallCodeArgsV2, - Method as Ic00Method, Payload, ProvisionalCreateCanisterWithCyclesArgs, UninstallCodeArgs, - UpdateSettingsArgs, IC_00, + CreateCanisterArgs, InstallChunkedCodeArgs, InstallCodeArgsV2, Method as Ic00Method, Payload, + ProvisionalCreateCanisterWithCyclesArgs, UninstallCodeArgs, UpdateSettingsArgs, IC_00, }; use ic_nns_constants::CYCLES_MINTING_CANISTER_ID; use ic_registry_subnet_type::SubnetType; @@ -24,7 +23,7 @@ use ic_types::{ messages::{CallContextId, CallbackId, RejectContext, Request, RequestMetadata, NO_DEADLINE}, methods::Callback, time::CoarseTime, - CanisterTimer, ComputeAllocation, Cycles, MemoryAllocation, NumInstructions, Time, + CanisterLog, CanisterTimer, ComputeAllocation, Cycles, MemoryAllocation, NumInstructions, Time, }; use ic_wasm_types::WasmEngineError; use serde::{Deserialize, Serialize}; diff --git a/rs/system_api/tests/system_api.rs b/rs/system_api/tests/system_api.rs index a964521395c..85d5d3cebb3 100644 --- a/rs/system_api/tests/system_api.rs +++ b/rs/system_api/tests/system_api.rs @@ -9,7 +9,7 @@ use ic_interfaces::execution_environment::{ SubnetAvailableMemory, SystemApi, TrapCode, }; use ic_logger::replica_logger::no_op_logger; -use ic_management_canister_types::{DataSize, MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE}; +use ic_management_canister_types::DataSize; use ic_registry_subnet_type::SubnetType; use ic_replicated_state::{ testing::CanisterQueuesTesting, CallOrigin, Memory, NetworkTopology, SystemState, @@ -33,6 +33,7 @@ use ic_types::{ time, time::UNIX_EPOCH, CanisterTimer, CountBytes, Cycles, NumInstructions, PrincipalId, Time, + MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE, }; use std::{ collections::BTreeSet, diff --git a/rs/types/management_canister_types/src/lib.rs b/rs/types/management_canister_types/src/lib.rs index 64306f0ee00..9fff6e75ddd 100644 --- a/rs/types/management_canister_types/src/lib.rs +++ b/rs/types/management_canister_types/src/lib.rs @@ -31,14 +31,7 @@ pub use provisional::{ProvisionalCreateCanisterWithCyclesArgs, ProvisionalTopUpC use serde::Serialize; use serde_bytes::ByteBuf; use std::mem::size_of; -use std::{ - collections::{BTreeSet, VecDeque}, - convert::TryFrom, - error::Error, - fmt, - slice::Iter, - str::FromStr, -}; +use std::{collections::BTreeSet, convert::TryFrom, error::Error, fmt, slice::Iter, str::FromStr}; use strum_macros::{Display, EnumIter, EnumString}; /// The id of the management canister. @@ -2556,115 +2549,6 @@ impl From for CanisterLogRecord { } } -/// The maximum allowed size of a canister log buffer. -pub const MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE: usize = 4 * 1024; - -// TODO(EXC-1572): needs refactoring to find a proper place to put this. -/// Holds canister log records and keeps track of the next canister log record index. -#[derive(Default, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] -pub struct CanisterLog { - next_idx: u64, - records: VecDeque, -} - -impl CanisterLog { - /// Creates a new `CanisterLog` with the given next index and records. - pub fn new(next_idx: u64, records: Vec) -> Self { - Self { - next_idx, - records: VecDeque::from(records), - } - } - - /// Creates a new `CanisterLog` with the given next index and an empty records list. - pub fn new_with_next_index(next_idx: u64) -> Self { - Self { - next_idx, - records: Default::default(), - } - } - - /// Returns the next canister log record index. - pub fn next_idx(&self) -> u64 { - self.next_idx - } - - /// Returns the canister log records. - pub fn records(&self) -> &VecDeque { - &self.records - } - - /// Clears the canister log records. - pub fn clear(&mut self) { - self.records.clear(); - } - - /// Returns the maximum allowed size of a canister log buffer. - pub fn capacity(&self) -> usize { - MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE - } - - /// Returns the used space in the canister log buffer. - pub fn used_space(&self) -> usize { - self.records.data_size() - } - - /// Returns the remaining space in the canister log buffer. - pub fn remaining_space(&self) -> usize { - MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE.saturating_sub(self.records.data_size()) - } - - /// Removes old records to make enough free space for new data within the limit. - fn make_free_space_within_limit(&mut self, new_data_size: usize) { - let mut total_size = new_data_size + self.records.data_size(); - while total_size > MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE { - if let Some(removed_record) = self.records.pop_front() { - total_size -= removed_record.data_size(); - } else { - break; // No more records to pop, limit reached. - } - } - } - - /// Adds a new log record. - pub fn add_record(&mut self, is_enabled: bool, timestamp_nanos: u64, content: &[u8]) { - if !is_enabled { - // If logging is disabled do not add new records, - // but still make sure the buffer is within limit. - self.make_free_space_within_limit(0); - return; - } - - // LINT.IfChange - // Keep the new log record size within limit, - // this must be in sync with `logging_charge_bytes` in `system_api.rs`. - let max_content_size = - MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE - CanisterLogRecord::default().data_size(); - let size = content.len().min(max_content_size); - let record = CanisterLogRecord { - idx: self.next_idx, - timestamp_nanos, - content: content[..size].to_vec(), - }; - self.make_free_space_within_limit(record.data_size()); - self.records.push_back(record); - // LINT.ThenChange(logging_charge_bytes_rule) - // Update the next canister log record index. - self.next_idx += 1; - } - - /// Moves all the logs from `other` to `self`. - pub fn append(&mut self, other: &mut Self) { - // Assume records sorted cronologically (with increasing idx) and - // update the system state's next index with the last record's index. - if let Some(last) = other.records.back() { - self.next_idx = last.idx + 1; - } - self.make_free_space_within_limit(other.records.data_size()); - self.records.append(&mut other.records); - } -} - /// `CandidType` for `FetchCanisterLogsResponse` /// ```text /// record { diff --git a/rs/types/types/src/canister_log.rs b/rs/types/types/src/canister_log.rs new file mode 100644 index 00000000000..ada33e8b13e --- /dev/null +++ b/rs/types/types/src/canister_log.rs @@ -0,0 +1,112 @@ +use candid::Deserialize; +use ic_management_canister_types::{CanisterLogRecord, DataSize}; +use serde::Serialize; +use std::collections::VecDeque; + +/// The maximum allowed size of a canister log buffer. +pub const MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE: usize = 4 * 1024; + +/// Holds canister log records and keeps track of the next canister log record index. +#[derive(Default, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] +pub struct CanisterLog { + next_idx: u64, + records: VecDeque, +} + +impl CanisterLog { + /// Creates a new `CanisterLog` with the given next index and records. + pub fn new(next_idx: u64, records: Vec) -> Self { + Self { + next_idx, + records: VecDeque::from(records), + } + } + + /// Creates a new `CanisterLog` with the given next index and an empty records list. + pub fn new_with_next_index(next_idx: u64) -> Self { + Self { + next_idx, + records: Default::default(), + } + } + + /// Returns the next canister log record index. + pub fn next_idx(&self) -> u64 { + self.next_idx + } + + /// Returns the canister log records. + pub fn records(&self) -> &VecDeque { + &self.records + } + + /// Clears the canister log records. + pub fn clear(&mut self) { + self.records.clear(); + } + + /// Returns the maximum allowed size of a canister log buffer. + pub fn capacity(&self) -> usize { + MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE + } + + /// Returns the used space in the canister log buffer. + pub fn used_space(&self) -> usize { + self.records.data_size() + } + + /// Returns the remaining space in the canister log buffer. + pub fn remaining_space(&self) -> usize { + MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE.saturating_sub(self.records.data_size()) + } + + /// Removes old records to make enough free space for new data within the limit. + fn make_free_space_within_limit(&mut self, new_data_size: usize) { + let mut total_size = new_data_size + self.records.data_size(); + while total_size > MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE { + if let Some(removed_record) = self.records.pop_front() { + total_size -= removed_record.data_size(); + } else { + break; // No more records to pop, limit reached. + } + } + } + + /// Adds a new log record. + pub fn add_record(&mut self, is_enabled: bool, timestamp_nanos: u64, content: &[u8]) { + if !is_enabled { + // If logging is disabled do not add new records, + // but still make sure the buffer is within limit. + self.make_free_space_within_limit(0); + return; + } + + // LINT.IfChange + // Keep the new log record size within limit, + // this must be in sync with `logging_charge_bytes` in `system_api.rs`. + let max_content_size = + MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE - CanisterLogRecord::default().data_size(); + let size = content.len().min(max_content_size); + let record = CanisterLogRecord { + idx: self.next_idx, + timestamp_nanos, + content: content[..size].to_vec(), + }; + self.make_free_space_within_limit(record.data_size()); + self.records.push_back(record); + // LINT.ThenChange(logging_charge_bytes_rule) + // Update the next canister log record index. + self.next_idx += 1; + } + + /// Moves all the logs from `other` to `self`. + pub fn append(&mut self, other: &mut Self) { + // Assume records sorted cronologically (with increasing idx) and + // update the system state's next index with the last record's index. + if let Some(last) = other.records.back() { + self.next_idx = last.idx + 1; + } + self.make_free_space_within_limit(other.records.data_size()); + self.records.append(&mut other.records); + } +} diff --git a/rs/types/types/src/lib.rs b/rs/types/types/src/lib.rs index d497ed2fddf..86386b84538 100644 --- a/rs/types/types/src/lib.rs +++ b/rs/types/types/src/lib.rs @@ -67,6 +67,7 @@ pub mod artifact; pub mod artifact_kind; pub mod batch; pub mod canister_http; +pub mod canister_log; pub mod consensus; pub mod crypto; pub mod funds; @@ -89,6 +90,7 @@ pub mod xnet; #[cfg(test)] pub mod exhaustive; +pub use crate::canister_log::{CanisterLog, MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE}; pub use crate::replica_version::ReplicaVersion; pub use crate::time::Time; pub use funds::*;