diff --git a/.github/workflows/hakari.yml b/.github/workflows/hakari.yml index b5a7504066..70b57a450a 100644 --- a/.github/workflows/hakari.yml +++ b/.github/workflows/hakari.yml @@ -24,7 +24,7 @@ jobs: with: toolchain: stable - name: Install cargo-hakari - uses: taiki-e/install-action@21526ba3bb38834e625c185ae4f2f942f1fb8f27 # v2 + uses: taiki-e/install-action@d211c4be5a95cbcd52a0870dda7d63a107a58368 # v2 with: tool: cargo-hakari - name: Check workspace-hack Cargo.toml is up-to-date diff --git a/Cargo.lock b/Cargo.lock index 2272cdc276..ed971fbb52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -458,7 +458,7 @@ dependencies = [ [[package]] name = "bhyve_api" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=54398875a2125227d13827d4236dce943c019b1c#54398875a2125227d13827d4236dce943c019b1c" +source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" dependencies = [ "bhyve_api_sys", "libc", @@ -468,7 +468,7 @@ dependencies = [ [[package]] name = "bhyve_api_sys" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=54398875a2125227d13827d4236dce943c019b1c#54398875a2125227d13827d4236dce943c019b1c" +source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" dependencies = [ "libc", "strum", @@ -1281,7 +1281,7 @@ dependencies = [ [[package]] name = "crucible-agent-client" version = "0.0.1" -source = "git+https://github.com/oxidecomputer/crucible?rev=51a3121c8318fc7ac97d74f917ce1d37962e785f#51a3121c8318fc7ac97d74f917ce1d37962e785f" +source = "git+https://github.com/oxidecomputer/crucible?rev=945f040d259ca8013d3fb26f510453da7cd7b1a6#945f040d259ca8013d3fb26f510453da7cd7b1a6" dependencies = [ "anyhow", "chrono", @@ -1297,7 +1297,7 @@ dependencies = [ [[package]] name = "crucible-pantry-client" version = "0.0.1" -source = "git+https://github.com/oxidecomputer/crucible?rev=51a3121c8318fc7ac97d74f917ce1d37962e785f#51a3121c8318fc7ac97d74f917ce1d37962e785f" +source = "git+https://github.com/oxidecomputer/crucible?rev=945f040d259ca8013d3fb26f510453da7cd7b1a6#945f040d259ca8013d3fb26f510453da7cd7b1a6" dependencies = [ "anyhow", "chrono", @@ -1314,7 +1314,7 @@ dependencies = [ [[package]] name = "crucible-smf" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/crucible?rev=51a3121c8318fc7ac97d74f917ce1d37962e785f#51a3121c8318fc7ac97d74f917ce1d37962e785f" +source = "git+https://github.com/oxidecomputer/crucible?rev=945f040d259ca8013d3fb26f510453da7cd7b1a6#945f040d259ca8013d3fb26f510453da7cd7b1a6" dependencies = [ "crucible-workspace-hack", "libc", @@ -1617,9 +1617,9 @@ dependencies = [ [[package]] name = "derive-where" -version = "1.2.5" +version = "1.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "146398d62142a0f35248a608f17edf0dde57338354966d6e41d0eb2d16980ccb" +checksum = "48d9b1fc2a6d7e19c89e706a3769e31ee862ac7a4c810c7c0ff3910e1a42a4ce" dependencies = [ "proc-macro2", "quote", @@ -1822,7 +1822,7 @@ dependencies = [ "omicron-test-utils", "omicron-workspace-hack", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "pretty-hex 0.4.0", "schemars", "serde", @@ -1927,7 +1927,7 @@ dependencies = [ "hyper", "indexmap 2.1.0", "multer", - "openapiv3 2.0.0-rc.1", + "openapiv3", "paste", "percent-encoding", "proc-macro2", @@ -3283,7 +3283,7 @@ dependencies = [ "omicron-test-utils", "omicron-workspace-hack", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "schemars", "serde", "serde_json", @@ -4077,7 +4077,7 @@ dependencies = [ "omicron-sled-agent", "omicron-test-utils", "omicron-workspace-hack", - "openapiv3 1.0.3", + "openapiv3", "openssl", "oso", "oximeter", @@ -4217,7 +4217,6 @@ dependencies = [ "schemars", "serde", "serde_json", - "serde_with", "steno", "strum", "uuid", @@ -4603,7 +4602,7 @@ dependencies = [ "omicron-workspace-hack", "once_cell", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "schemars", "serde", "serde_json", @@ -4680,7 +4679,7 @@ dependencies = [ "omicron-workspace-hack", "once_cell", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "openssl", "oxide-client", "oximeter", @@ -4878,7 +4877,7 @@ dependencies = [ "omicron-workspace-hack", "once_cell", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "opte-ioctl", "oximeter", "oximeter-instruments", @@ -5008,7 +5007,7 @@ dependencies = [ "num-iter", "num-traits", "once_cell", - "openapiv3 2.0.0-rc.1", + "openapiv3", "petgraph", "postgres-types", "ppv-lite86", @@ -5104,26 +5103,15 @@ checksum = "624a8340c38c1b80fd549087862da4ba43e08858af025b236e509b6649fc13d5" [[package]] name = "openapi-lint" version = "0.4.0" -source = "git+https://github.com/oxidecomputer/openapi-lint?branch=main#bb69a3a4a184d966bac2a0df2be5c9038d9867d0" +source = "git+https://github.com/oxidecomputer/openapi-lint?branch=main#ef442ee4343e97b6d9c217d3e7533962fe7d7236" dependencies = [ "heck 0.4.1", "indexmap 2.1.0", "lazy_static", - "openapiv3 1.0.3", + "openapiv3", "regex", ] -[[package]] -name = "openapiv3" -version = "1.0.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "75e56d5c441965b6425165b7e3223cc933ca469834f4a8b4786817a1f9dc4f13" -dependencies = [ - "indexmap 2.1.0", - "serde", - "serde_json", -] - [[package]] name = "openapiv3" version = "2.0.0-rc.1" @@ -5307,6 +5295,7 @@ dependencies = [ "rstest", "schemars", "serde", + "serde_json", "strum", "thiserror", "trybuild", @@ -5347,7 +5336,7 @@ dependencies = [ "omicron-test-utils", "omicron-workspace-hack", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "oximeter", "oximeter-client", "oximeter-db", @@ -6109,7 +6098,7 @@ dependencies = [ "heck 0.4.1", "http", "indexmap 2.1.0", - "openapiv3 2.0.0-rc.1", + "openapiv3", "proc-macro2", "quote", "regex", @@ -6127,7 +6116,7 @@ name = "progenitor-macro" version = "0.4.0" source = "git+https://github.com/oxidecomputer/progenitor?branch=main#9339b57628e1e76b1d7131ef93a6c0db2ab0a762" dependencies = [ - "openapiv3 2.0.0-rc.1", + "openapiv3", "proc-macro2", "progenitor-impl", "quote", @@ -6142,7 +6131,7 @@ dependencies = [ [[package]] name = "propolis-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=54398875a2125227d13827d4236dce943c019b1c#54398875a2125227d13827d4236dce943c019b1c" +source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" dependencies = [ "async-trait", "base64 0.21.5", @@ -6163,7 +6152,7 @@ dependencies = [ [[package]] name = "propolis-mock-server" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=54398875a2125227d13827d4236dce943c019b1c#54398875a2125227d13827d4236dce943c019b1c" +source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" dependencies = [ "anyhow", "atty", @@ -6193,7 +6182,7 @@ dependencies = [ [[package]] name = "propolis_types" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=54398875a2125227d13827d4236dce943c019b1c#54398875a2125227d13827d4236dce943c019b1c" +source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" dependencies = [ "schemars", "serde", @@ -9582,7 +9571,7 @@ dependencies = [ "omicron-test-utils", "omicron-workspace-hack", "openapi-lint", - "openapiv3 1.0.3", + "openapiv3", "rand 0.8.5", "reqwest", "schemars", diff --git a/Cargo.toml b/Cargo.toml index 05522f71f5..a47f8bb780 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -171,9 +171,9 @@ cookie = "0.18" criterion = { version = "0.5.1", features = [ "async_tokio" ] } crossbeam = "0.8" crossterm = { version = "0.27.0", features = ["event-stream"] } -crucible-agent-client = { git = "https://github.com/oxidecomputer/crucible", rev = "51a3121c8318fc7ac97d74f917ce1d37962e785f" } -crucible-pantry-client = { git = "https://github.com/oxidecomputer/crucible", rev = "51a3121c8318fc7ac97d74f917ce1d37962e785f" } -crucible-smf = { git = "https://github.com/oxidecomputer/crucible", rev = "51a3121c8318fc7ac97d74f917ce1d37962e785f" } +crucible-agent-client = { git = "https://github.com/oxidecomputer/crucible", rev = "945f040d259ca8013d3fb26f510453da7cd7b1a6" } +crucible-pantry-client = { git = "https://github.com/oxidecomputer/crucible", rev = "945f040d259ca8013d3fb26f510453da7cd7b1a6" } +crucible-smf = { git = "https://github.com/oxidecomputer/crucible", rev = "945f040d259ca8013d3fb26f510453da7cd7b1a6" } curve25519-dalek = "4" datatest-stable = "0.2.3" display-error-chain = "0.2.0" @@ -181,7 +181,7 @@ ddm-admin-client = { path = "clients/ddm-admin-client" } db-macros = { path = "nexus/db-macros" } debug-ignore = "1.0.5" derive_more = "0.99.17" -derive-where = "1.2.5" +derive-where = "1.2.6" diesel = { version = "2.1.4", features = ["postgres", "r2d2", "chrono", "serde_json", "network-address", "uuid"] } diesel-dtrace = { git = "https://github.com/oxidecomputer/diesel-dtrace", branch = "main" } dns-server = { path = "dns-server" } @@ -263,7 +263,7 @@ oxide-client = { path = "clients/oxide-client" } oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "24ceba1969269e4d81bda83d8968d7d7f713c46b", features = [ "api", "std" ] } once_cell = "1.18.0" openapi-lint = { git = "https://github.com/oxidecomputer/openapi-lint", branch = "main" } -openapiv3 = "1.0" +openapiv3 = "2.0.0-rc.1" # must match samael's crate! openssl = "0.10" openssl-sys = "0.9" @@ -292,9 +292,9 @@ pretty-hex = "0.4.0" proc-macro2 = "1.0" progenitor = { git = "https://github.com/oxidecomputer/progenitor", branch = "main" } progenitor-client = { git = "https://github.com/oxidecomputer/progenitor", branch = "main" } -bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "54398875a2125227d13827d4236dce943c019b1c" } -propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "54398875a2125227d13827d4236dce943c019b1c" } -propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "54398875a2125227d13827d4236dce943c019b1c" } +bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" } +propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" } +propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" } proptest = "1.4.0" quote = "1.0" rand = "0.8.5" diff --git a/common/src/nexus_config.rs b/common/src/nexus_config.rs index 94c39b4436..740823e755 100644 --- a/common/src/nexus_config.rs +++ b/common/src/nexus_config.rs @@ -339,6 +339,8 @@ pub struct BackgroundTaskConfig { pub nat_cleanup: NatCleanupConfig, /// configuration for inventory tasks pub inventory: InventoryConfig, + /// configuration for phantom disks task + pub phantom_disks: PhantomDiskConfig, } #[serde_as] @@ -386,7 +388,7 @@ pub struct NatCleanupConfig { pub struct InventoryConfig { /// period (in seconds) for periodic activations of this background task /// - /// Each activation fetches information about all harware and software in + /// Each activation fetches information about all hardware and software in /// the system and inserts it into the database. This generates a moderate /// amount of data. #[serde_as(as = "DurationSeconds")] @@ -405,6 +407,14 @@ pub struct InventoryConfig { pub disable: bool, } +#[serde_as] +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct PhantomDiskConfig { + /// period (in seconds) for periodic activations of this background task + #[serde_as(as = "DurationSeconds")] + pub period_secs: Duration, +} + /// Configuration for a nexus server #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct PackageConfig { @@ -508,8 +518,9 @@ mod test { BackgroundTaskConfig, Config, ConfigDropshotWithTls, ConsoleConfig, Database, DeploymentConfig, DnsTasksConfig, DpdConfig, ExternalEndpointsConfig, InternalDns, InventoryConfig, LoadError, - LoadErrorKind, MgdConfig, NatCleanupConfig, PackageConfig, SchemeName, - TimeseriesDbConfig, Tunables, UpdatesConfig, + LoadErrorKind, MgdConfig, NatCleanupConfig, PackageConfig, + PhantomDiskConfig, SchemeName, TimeseriesDbConfig, Tunables, + UpdatesConfig, }; use crate::address::{Ipv6Subnet, RACK_PREFIX}; use crate::api::internal::shared::SwitchLocation; @@ -663,6 +674,7 @@ mod test { inventory.period_secs = 10 inventory.nkeep = 11 inventory.disable = false + phantom_disks.period_secs = 30 [default_region_allocation_strategy] type = "random" seed = 0 @@ -764,7 +776,10 @@ mod test { period_secs: Duration::from_secs(10), nkeep: 11, disable: false, - } + }, + phantom_disks: PhantomDiskConfig { + period_secs: Duration::from_secs(30), + }, }, default_region_allocation_strategy: crate::nexus_config::RegionAllocationStrategy::Random { @@ -822,6 +837,7 @@ mod test { inventory.period_secs = 10 inventory.nkeep = 3 inventory.disable = false + phantom_disks.period_secs = 30 [default_region_allocation_strategy] type = "random" "##, diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index 9f91d38504..df5248b52d 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -515,6 +515,32 @@ fn print_task_details(bgtask: &BackgroundTask, details: &serde_json::Value) { ); } }; + } else if name == "phantom_disks" { + #[derive(Deserialize)] + struct TaskSuccess { + /// how many phantom disks were deleted ok + phantom_disk_deleted_ok: usize, + + /// how many phantom disks could not be deleted + phantom_disk_deleted_err: usize, + } + + match serde_json::from_value::(details.clone()) { + Err(error) => eprintln!( + "warning: failed to interpret task details: {:?}: {:?}", + error, details + ), + Ok(success) => { + println!( + " number of phantom disks deleted: {}", + success.phantom_disk_deleted_ok + ); + println!( + " number of phantom disk delete errors: {}", + success.phantom_disk_deleted_err + ); + } + }; } else { println!( "warning: unknown background task: {:?} \ diff --git a/dev-tools/omdb/tests/env.out b/dev-tools/omdb/tests/env.out index fd50d80c81..c08f592852 100644 --- a/dev-tools/omdb/tests/env.out +++ b/dev-tools/omdb/tests/env.out @@ -66,6 +66,10 @@ task: "nat_v4_garbage_collector" predetermined retention policy +task: "phantom_disks" + detects and un-deletes phantom disks + + --------------------------------------------- stderr: note: using Nexus URL http://127.0.0.1:REDACTED_PORT @@ -131,6 +135,10 @@ task: "nat_v4_garbage_collector" predetermined retention policy +task: "phantom_disks" + detects and un-deletes phantom disks + + --------------------------------------------- stderr: note: Nexus URL not specified. Will pick one from DNS. @@ -183,6 +191,10 @@ task: "nat_v4_garbage_collector" predetermined retention policy +task: "phantom_disks" + detects and un-deletes phantom disks + + --------------------------------------------- stderr: note: Nexus URL not specified. Will pick one from DNS. diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index 6bc3a85e8a..65520ab59c 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -260,6 +260,10 @@ task: "nat_v4_garbage_collector" predetermined retention policy +task: "phantom_disks" + detects and un-deletes phantom disks + + --------------------------------------------- stderr: note: using Nexus URL http://127.0.0.1:REDACTED_PORT/ @@ -357,6 +361,14 @@ task: "inventory_collection" last collection started: last collection done: +task: "phantom_disks" + configured period: every 30s + currently executing: no + last completed activation: iter 2, triggered by an explicit signal + started at (s ago) and ran for ms + number of phantom disks deleted: 0 + number of phantom disk delete errors: 0 + --------------------------------------------- stderr: note: using Nexus URL http://127.0.0.1:REDACTED_PORT/ diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index c4f839efbe..51501b4894 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1322,7 +1322,7 @@ table! { /// /// This should be updated whenever the schema is changed. For more details, /// refer to: schema/crdb/README.adoc -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(18, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(19, 0, 0); allow_tables_to_appear_in_same_query!( system_update, @@ -1391,3 +1391,7 @@ allow_tables_to_appear_in_same_query!( switch_port, switch_port_settings_bgp_peer_config ); + +allow_tables_to_appear_in_same_query!(disk, virtual_provisioning_resource); + +allow_tables_to_appear_in_same_query!(volume, virtual_provisioning_resource); diff --git a/nexus/db-model/src/sled_provision_state.rs b/nexus/db-model/src/sled_provision_state.rs index 6cf81b9c70..b2b1ee39dc 100644 --- a/nexus/db-model/src/sled_provision_state.rs +++ b/nexus/db-model/src/sled_provision_state.rs @@ -34,19 +34,14 @@ impl From for views::SledProvisionState { } } -impl TryFrom for SledProvisionState { - type Error = UnknownSledProvisionState; - - fn try_from(state: views::SledProvisionState) -> Result { +impl From for SledProvisionState { + fn from(state: views::SledProvisionState) -> Self { match state { views::SledProvisionState::Provisionable => { - Ok(SledProvisionState::Provisionable) + SledProvisionState::Provisionable } views::SledProvisionState::NonProvisionable => { - Ok(SledProvisionState::NonProvisionable) - } - views::SledProvisionState::Unknown => { - Err(UnknownSledProvisionState) + SledProvisionState::NonProvisionable } } } diff --git a/nexus/db-queries/src/db/datastore/disk.rs b/nexus/db-queries/src/db/datastore/disk.rs index a0d9bf12c3..26d439b350 100644 --- a/nexus/db-queries/src/db/datastore/disk.rs +++ b/nexus/db-queries/src/db/datastore/disk.rs @@ -25,11 +25,14 @@ use crate::db::model::DiskUpdate; use crate::db::model::Instance; use crate::db::model::Name; use crate::db::model::Project; +use crate::db::model::VirtualProvisioningResource; +use crate::db::model::Volume; use crate::db::pagination::paginated; use crate::db::queries::disk::DiskSetClauseForAttach; use crate::db::update_and_check::UpdateAndCheck; use crate::db::update_and_check::UpdateStatus; use async_bb8_diesel::AsyncRunQueryDsl; +use chrono::DateTime; use chrono::Utc; use diesel::prelude::*; use omicron_common::api; @@ -564,7 +567,7 @@ impl DataStore { /// Updates a disk record to indicate it has been deleted. /// - /// Returns the volume ID of associated with the deleted disk. + /// Returns the disk before any modifications are made by this function. /// /// Does not attempt to modify any resources (e.g. regions) which may /// belong to the disk. @@ -652,4 +655,289 @@ impl DataStore { } } } + + /// Set a disk to faulted and un-delete it + /// + /// If the disk delete saga unwinds, then the disk should _not_ remain + /// deleted: disk delete saga should be triggered again in order to fully + /// complete, and the only way to do that is to un-delete the disk. Set it + /// to faulted to ensure that it won't be used. + pub async fn project_undelete_disk_set_faulted_no_auth( + &self, + disk_id: &Uuid, + ) -> Result<(), Error> { + use db::schema::disk::dsl; + let conn = self.pool_connection_unauthorized().await?; + + let faulted = api::external::DiskState::Faulted.label(); + + let result = diesel::update(dsl::disk) + .filter(dsl::time_deleted.is_not_null()) + .filter(dsl::id.eq(*disk_id)) + .set(( + dsl::time_deleted.eq(None::>), + dsl::disk_state.eq(faulted), + )) + .check_if_exists::(*disk_id) + .execute_and_check(&conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Disk, + LookupType::ById(*disk_id), + ), + ) + })?; + + match result.status { + UpdateStatus::Updated => Ok(()), + UpdateStatus::NotUpdatedButExists => { + let disk = result.found; + let disk_state = disk.state(); + + if disk.time_deleted().is_none() + && disk_state.state() == &api::external::DiskState::Faulted + { + // To maintain idempotency, if the disk has already been + // faulted, don't throw an error. + return Ok(()); + } else { + // NOTE: This is a "catch-all" error case, more specific + // errors should be preferred as they're more actionable. + return Err(Error::InternalError { + internal_message: String::from( + "disk exists, but cannot be faulted", + ), + }); + } + } + } + } + + /// Find disks that have been deleted but still have a + /// `virtual_provisioning_resource` record: this indicates that a disk + /// delete saga partially succeeded, then unwound, which (before the fixes + /// in customer-support#58) would mean the disk was deleted but the project + /// it was in could not be deleted (due to an erroneous number of bytes + /// "still provisioned"). + pub async fn find_phantom_disks(&self) -> ListResultVec { + use db::schema::disk::dsl; + use db::schema::virtual_provisioning_resource::dsl as resource_dsl; + use db::schema::volume::dsl as volume_dsl; + + let conn = self.pool_connection_unauthorized().await?; + + let potential_phantom_disks: Vec<( + Disk, + Option, + Option, + )> = dsl::disk + .filter(dsl::time_deleted.is_not_null()) + .left_join( + resource_dsl::virtual_provisioning_resource + .on(resource_dsl::id.eq(dsl::id)), + ) + .left_join(volume_dsl::volume.on(dsl::volume_id.eq(volume_dsl::id))) + .select(( + Disk::as_select(), + Option::::as_select(), + Option::::as_select(), + )) + .load_async(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + // The first forward steps of the disk delete saga (plus the volume + // delete sub saga) are as follows: + // + // 1. soft-delete the disk + // 2. call virtual_provisioning_collection_delete_disk + // 3. soft-delete the disk's volume + // + // Before the fixes as part of customer-support#58, steps 1 and 3 did + // not have undo steps, where step 2 did. In order to detect when the + // disk delete saga unwound, find entries where + // + // 1. the disk and volume are soft-deleted + // 2. the `virtual_provisioning_resource` exists + // + // It's important not to conflict with any currently running disk delete + // saga. + + Ok(potential_phantom_disks + .into_iter() + .filter(|(disk, resource, volume)| { + if let Some(volume) = volume { + // In this branch, the volume record exists. Because it was + // returned by the query above, if it is soft-deleted we + // then know the saga unwound before the volume record could + // be hard deleted. This won't conflict with a running disk + // delete saga, because the resource record should be None + // if the disk and volume were already soft deleted (if + // there is one, the saga will be at or past step 3). + disk.time_deleted().is_some() + && volume.time_deleted.is_some() + && resource.is_some() + } else { + // In this branch, the volume record was hard-deleted. The + // saga could still have unwound after hard deleting the + // volume record, so proceed with filtering. This won't + // conflict with a running disk delete saga because the + // resource record should be None if the disk was soft + // deleted and the volume was hard deleted (if there is one, + // the saga should be almost finished as the volume hard + // delete is the last thing it does). + disk.time_deleted().is_some() && resource.is_some() + } + }) + .map(|(disk, _, _)| disk) + .collect()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + use crate::db::datastore::datastore_test; + use nexus_test_utils::db::test_setup_database; + use nexus_types::external_api::params; + use omicron_common::api::external; + use omicron_test_utils::dev; + + #[tokio::test] + async fn test_undelete_disk_set_faulted_idempotent() { + let logctx = + dev::test_setup_log("test_undelete_disk_set_faulted_idempotent"); + let log = logctx.log.new(o!()); + let mut db = test_setup_database(&log).await; + let (opctx, db_datastore) = datastore_test(&logctx, &db).await; + + let silo_id = opctx.authn.actor().unwrap().silo_id().unwrap(); + + let (authz_project, _db_project) = db_datastore + .project_create( + &opctx, + Project::new( + silo_id, + params::ProjectCreate { + identity: external::IdentityMetadataCreateParams { + name: "testpost".parse().unwrap(), + description: "please ignore".to_string(), + }, + }, + ), + ) + .await + .unwrap(); + + let disk = db_datastore + .project_create_disk( + &opctx, + &authz_project, + Disk::new( + Uuid::new_v4(), + authz_project.id(), + Uuid::new_v4(), + params::DiskCreate { + identity: external::IdentityMetadataCreateParams { + name: "first-post".parse().unwrap(), + description: "just trying things out".to_string(), + }, + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512) + .unwrap(), + }, + size: external::ByteCount::from(2147483648), + }, + db::model::BlockSize::Traditional, + DiskRuntimeState::new(), + ) + .unwrap(), + ) + .await + .unwrap(); + + let (.., authz_disk, db_disk) = LookupPath::new(&opctx, &db_datastore) + .disk_id(disk.id()) + .fetch() + .await + .unwrap(); + + db_datastore + .disk_update_runtime( + &opctx, + &authz_disk, + &db_disk.runtime().detach(), + ) + .await + .unwrap(); + + db_datastore + .project_delete_disk_no_auth( + &authz_disk.id(), + &[external::DiskState::Detached], + ) + .await + .unwrap(); + + // Assert initial state - deleting the Disk will make LookupPath::fetch + // not work. + { + LookupPath::new(&opctx, &db_datastore) + .disk_id(disk.id()) + .fetch() + .await + .unwrap_err(); + } + + // Function under test: call this twice to ensure it's idempotent + + db_datastore + .project_undelete_disk_set_faulted_no_auth(&authz_disk.id()) + .await + .unwrap(); + + // Assert state change + + { + let (.., db_disk) = LookupPath::new(&opctx, &db_datastore) + .disk_id(disk.id()) + .fetch() + .await + .unwrap(); + + assert!(db_disk.time_deleted().is_none()); + assert_eq!( + db_disk.runtime().disk_state, + external::DiskState::Faulted.label().to_string() + ); + } + + db_datastore + .project_undelete_disk_set_faulted_no_auth(&authz_disk.id()) + .await + .unwrap(); + + // Assert state is the same after the second call + + { + let (.., db_disk) = LookupPath::new(&opctx, &db_datastore) + .disk_id(disk.id()) + .fetch() + .await + .unwrap(); + + assert!(db_disk.time_deleted().is_none()); + assert_eq!( + db_disk.runtime().disk_state, + external::DiskState::Faulted.label().to_string() + ); + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 3679fa8196..9d6bf2d22f 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -100,6 +100,7 @@ inventory.period_secs = 600 inventory.nkeep = 5 # Disable inventory collection altogether (for emergencies) inventory.disable = false +phantom_disks.period_secs = 30 [default_region_allocation_strategy] # allocate region on 3 random distinct zpools, on 3 random distinct sleds. diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index d27248ffdc..cfa023a013 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -11,6 +11,7 @@ use super::dns_servers; use super::external_endpoints; use super::inventory_collection; use super::nat_cleanup; +use super::phantom_disks; use nexus_db_model::DnsGroup; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; @@ -52,6 +53,9 @@ pub struct BackgroundTasks { /// task handle for the task that collects inventory pub task_inventory_collection: common::TaskHandle, + + /// task handle for the task that detects phantom disks + pub task_phantom_disks: common::TaskHandle, } impl BackgroundTasks { @@ -122,7 +126,7 @@ impl BackgroundTasks { // Background task: inventory collector let task_inventory_collection = { let collector = inventory_collection::InventoryCollector::new( - datastore, + datastore.clone(), resolver, &nexus_id.to_string(), config.inventory.nkeep, @@ -143,6 +147,22 @@ impl BackgroundTasks { task }; + // Background task: phantom disk detection + let task_phantom_disks = { + let detector = phantom_disks::PhantomDiskDetector::new(datastore); + + let task = driver.register( + String::from("phantom_disks"), + String::from("detects and un-deletes phantom disks"), + config.phantom_disks.period_secs, + Box::new(detector), + opctx.child(BTreeMap::new()), + vec![], + ); + + task + }; + BackgroundTasks { driver, task_internal_dns_config, @@ -153,6 +173,7 @@ impl BackgroundTasks { external_endpoints, nat_cleanup, task_inventory_collection, + task_phantom_disks, } } diff --git a/nexus/src/app/background/mod.rs b/nexus/src/app/background/mod.rs index 954207cb3c..70b20224d4 100644 --- a/nexus/src/app/background/mod.rs +++ b/nexus/src/app/background/mod.rs @@ -12,6 +12,7 @@ mod external_endpoints; mod init; mod inventory_collection; mod nat_cleanup; +mod phantom_disks; mod status; pub use common::Driver; diff --git a/nexus/src/app/background/phantom_disks.rs b/nexus/src/app/background/phantom_disks.rs new file mode 100644 index 0000000000..b038d70ac6 --- /dev/null +++ b/nexus/src/app/background/phantom_disks.rs @@ -0,0 +1,104 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Background task for detecting and un-deleting phantom disks +//! +//! A "phantom" disk is one where a disk delete saga partially completed but +//! unwound: before a fix for customer-support#58, this would leave disks +//! deleted but would also leave a `virtual_provisioning_resource` record for +//! that disk. There would be no way to re-trigger the disk delete saga as the +//! disk was deleted, so the project that disk was in could not be deleted +//! because associated virtual provisioning resources were still being consumed. +//! +//! The fix for customer-support#58 changes the disk delete saga's unwind to +//! also un-delete the disk and set it to faulted. This enables it to be deleted +//! again. Correcting the disk delete saga's unwind means that phantom disks +//! will not be created in the future when the disk delete saga unwinds, but +//! this background task is required to apply the same fix for disks that are +//! already in this phantom state. + +use super::common::BackgroundTask; +use futures::future::BoxFuture; +use futures::FutureExt; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::DataStore; +use serde_json::json; +use std::sync::Arc; + +pub struct PhantomDiskDetector { + datastore: Arc, +} + +impl PhantomDiskDetector { + pub fn new(datastore: Arc) -> Self { + PhantomDiskDetector { datastore } + } +} + +impl BackgroundTask for PhantomDiskDetector { + fn activate<'a, 'b, 'c>( + &'a mut self, + opctx: &'b OpContext, + ) -> BoxFuture<'c, serde_json::Value> + where + 'a: 'c, + 'b: 'c, + { + async { + let log = &opctx.log; + warn!(&log, "phantom disk task started"); + + let phantom_disks = match self.datastore.find_phantom_disks().await + { + Ok(phantom_disks) => phantom_disks, + Err(e) => { + warn!(&log, "error from find_phantom_disks: {:?}", e); + return json!({ + "error": + format!("failed find_phantom_disks: {:#}", e) + }); + } + }; + + let mut phantom_disk_deleted_ok = 0; + let mut phantom_disk_deleted_err = 0; + + for disk in phantom_disks { + warn!(&log, "phantom disk {} found!", disk.id()); + + // If a phantom disk is found, then un-delete it and set it to + // faulted: this will allow a user to request deleting it again. + + let result = self + .datastore + .project_undelete_disk_set_faulted_no_auth(&disk.id()) + .await; + + if let Err(e) = result { + error!( + &log, + "error un-deleting disk {} and setting to faulted: {:#}", + disk.id(), + e, + ); + phantom_disk_deleted_err += 1; + } else { + info!( + &log, + "phandom disk {} un-deleted andset to faulted ok", + disk.id(), + ); + phantom_disk_deleted_ok += 1; + } + } + + warn!(&log, "phantom disk task done"); + json!({ + "phantom_disk_deleted_ok": phantom_disk_deleted_ok, + "phantom_disk_deleted_err": phantom_disk_deleted_err, + }) + } + .boxed() + } +} diff --git a/nexus/src/app/sagas/disk_delete.rs b/nexus/src/app/sagas/disk_delete.rs index f2d80d64f5..8f6d74da0a 100644 --- a/nexus/src/app/sagas/disk_delete.rs +++ b/nexus/src/app/sagas/disk_delete.rs @@ -32,10 +32,8 @@ pub(crate) struct Params { declare_saga_actions! { disk_delete; DELETE_DISK_RECORD -> "deleted_disk" { - // TODO: See the comment on the "DeleteRegions" step, - // we may want to un-delete the disk if we cannot remove - // underlying regions. + sdd_delete_disk_record + - sdd_delete_disk_record_undo } SPACE_ACCOUNT -> "no_result1" { + sdd_account_space @@ -117,6 +115,21 @@ async fn sdd_delete_disk_record( Ok(disk) } +async fn sdd_delete_disk_record_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + + osagactx + .datastore() + .project_undelete_disk_set_faulted_no_auth(¶ms.disk_id) + .await + .map_err(ActionError::action_failed)?; + + Ok(()) +} + async fn sdd_account_space( sagactx: NexusActionContext, ) -> Result<(), ActionError> { diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 1c7b760e63..a113451fc7 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -4629,10 +4629,7 @@ async fn sled_set_provision_state( let opctx = crate::context::op_context_for_external_api(&rqctx).await?; // Convert the external `SledProvisionState` into our internal data model. - let new_state = - db::model::SledProvisionState::try_from(provision_state).map_err( - |error| HttpError::for_bad_request(None, format!("{error}")), - )?; + let new_state = db::model::SledProvisionState::from(provision_state); let sled_lookup = nexus.sled_lookup(&opctx, &path.sled_id)?; diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index fbed9aed8e..a4436234f0 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -98,6 +98,7 @@ inventory.period_secs = 600 inventory.nkeep = 3 # Disable inventory collection altogether (for emergencies) inventory.disable = false +phantom_disks.period_secs = 30 [default_region_allocation_strategy] # we only have one sled in the test environment, so we need to use the diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index a5a8339c34..f7403275b1 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -1241,6 +1241,138 @@ async fn test_disk_virtual_provisioning_collection( ); } +#[nexus_test] +async fn test_disk_virtual_provisioning_collection_failed_delete( + cptestctx: &ControlPlaneTestContext, +) { + // Confirm that there's a panic deleting a project if a disk deletion fails + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx().nexus; + let datastore = nexus.datastore(); + + let disk_test = DiskTest::new(&cptestctx).await; + + populate_ip_pool(&client, "default", None).await; + let project_id1 = create_project(client, PROJECT_NAME).await.identity.id; + + let opctx = + OpContext::for_tests(cptestctx.logctx.log.new(o!()), datastore.clone()); + + // Create a 1 GB disk + let disk_size = ByteCount::from_gibibytes_u32(1); + let disks_url = get_disks_url(); + let disk_one = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: "disk-one".parse().unwrap(), + description: String::from("sells rainsticks"), + }, + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + size: disk_size, + }; + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&disk_one)) + .expect_status(Some(StatusCode::CREATED)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("unexpected failure creating 1 GiB disk"); + + // Assert correct virtual provisioning collection numbers + let virtual_provisioning_collection = datastore + .virtual_provisioning_collection_get(&opctx, project_id1) + .await + .unwrap(); + assert_eq!( + virtual_provisioning_collection.virtual_disk_bytes_provisioned.0, + disk_size + ); + + // Set the third agent to fail when deleting regions + let zpool = &disk_test.zpools[2]; + let dataset = &zpool.datasets[0]; + disk_test + .sled_agent + .get_crucible_dataset(zpool.id, dataset.id) + .await + .set_region_deletion_error(true) + .await; + + // Delete the disk - expect this to fail + let disk_url = format!("/v1/disks/{}?project={}", "disk-one", PROJECT_NAME); + + NexusRequest::new( + RequestBuilder::new(client, Method::DELETE, &disk_url) + .expect_status(Some(StatusCode::INTERNAL_SERVER_ERROR)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("unexpected success deleting 1 GiB disk"); + + // The virtual provisioning collection numbers haven't changed + let virtual_provisioning_collection = datastore + .virtual_provisioning_collection_get(&opctx, project_id1) + .await + .unwrap(); + assert_eq!( + virtual_provisioning_collection.virtual_disk_bytes_provisioned.0, + disk_size + ); + + // And the disk is now faulted + let disk = disk_get(&client, &disk_url).await; + assert_eq!(disk.state, DiskState::Faulted); + + // Set the third agent to respond normally + disk_test + .sled_agent + .get_crucible_dataset(zpool.id, dataset.id) + .await + .set_region_deletion_error(false) + .await; + + // Request disk delete again + NexusRequest::new( + RequestBuilder::new(client, Method::DELETE, &disk_url) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("unexpected failure deleting 1 GiB disk"); + + // Delete the project's default VPC subnet and VPC + let subnet_url = + format!("/v1/vpc-subnets/default?project={}&vpc=default", PROJECT_NAME); + NexusRequest::object_delete(&client, &subnet_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make request"); + + let vpc_url = format!("/v1/vpcs/default?project={}", PROJECT_NAME); + NexusRequest::object_delete(&client, &vpc_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make request"); + + // The project can be deleted now + let url = format!("/v1/projects/{}", PROJECT_NAME); + NexusRequest::new( + RequestBuilder::new(client, Method::DELETE, &url) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("unexpected failure deleting project"); +} + // Test disk size accounting #[nexus_test] async fn test_disk_size_accounting(cptestctx: &ControlPlaneTestContext) { diff --git a/nexus/types/Cargo.toml b/nexus/types/Cargo.toml index 8cbbd8626c..9cb94a8484 100644 --- a/nexus/types/Cargo.toml +++ b/nexus/types/Cargo.toml @@ -14,7 +14,6 @@ parse-display.workspace = true schemars = { workspace = true, features = ["chrono", "uuid1"] } serde.workspace = true serde_json.workspace = true -serde_with.workspace = true steno.workspace = true strum.workspace = true uuid.workspace = true diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index e608e6ba78..047bd71814 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -17,7 +17,6 @@ use omicron_common::api::external::{ }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use serde_with::rust::deserialize_ignore_any; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::net::IpAddr; @@ -327,12 +326,6 @@ pub enum SledProvisionState { /// resources will continue to be on this sled unless manually migrated /// off. NonProvisionable, - - /// This is a state that isn't known yet. - /// - /// This is defined to avoid API breakage. - #[serde(other, deserialize_with = "deserialize_ignore_any")] - Unknown, } /// An operator's view of an instance running on a given sled diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 7785d232d9..caf1414f53 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2568,9 +2568,60 @@ "datum", "type" ] + }, + { + "type": "object", + "properties": { + "datum": { + "$ref": "#/components/schemas/MissingDatum" + }, + "type": { + "type": "string", + "enum": [ + "missing" + ] + } + }, + "required": [ + "datum", + "type" + ] } ] }, + "DatumType": { + "description": "The type of an individual datum of a metric.", + "type": "string", + "enum": [ + "bool", + "i8", + "u8", + "i16", + "u16", + "i32", + "u32", + "i64", + "u64", + "f32", + "f64", + "string", + "bytes", + "cumulative_i64", + "cumulative_u64", + "cumulative_f32", + "cumulative_f64", + "histogram_i8", + "histogram_u8", + "histogram_i16", + "histogram_u16", + "histogram_i32", + "histogram_u32", + "histogram_i64", + "histogram_u64", + "histogram_f32", + "histogram_f64" + ] + }, "DiskRuntimeState": { "description": "Runtime state of the Disk, which includes its attach state and some minimal metadata", "type": "object", @@ -4128,9 +4179,77 @@ "content", "type" ] + }, + { + "type": "object", + "properties": { + "content": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + } + }, + "required": [ + "datum_type" + ] + }, + "type": { + "type": "string", + "enum": [ + "missing_datum_requires_start_time" + ] + } + }, + "required": [ + "content", + "type" + ] + }, + { + "type": "object", + "properties": { + "content": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + } + }, + "required": [ + "datum_type" + ] + }, + "type": { + "type": "string", + "enum": [ + "missing_datum_cannot_have_start_time" + ] + } + }, + "required": [ + "content", + "type" + ] } ] }, + "MissingDatum": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + }, + "start_time": { + "nullable": true, + "type": "string", + "format": "date-time" + } + }, + "required": [ + "datum_type" + ] + }, "Name": { "title": "A name unique within the parent collection", "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'. Names cannot be a UUID though they may contain a UUID.", diff --git a/openapi/nexus.json b/openapi/nexus.json index 2c04e7259e..6076663a2d 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -9940,9 +9940,60 @@ "datum", "type" ] + }, + { + "type": "object", + "properties": { + "datum": { + "$ref": "#/components/schemas/MissingDatum" + }, + "type": { + "type": "string", + "enum": [ + "missing" + ] + } + }, + "required": [ + "datum", + "type" + ] } ] }, + "DatumType": { + "description": "The type of an individual datum of a metric.", + "type": "string", + "enum": [ + "bool", + "i8", + "u8", + "i16", + "u16", + "i32", + "u32", + "i64", + "u64", + "f32", + "f64", + "string", + "bytes", + "cumulative_i64", + "cumulative_u64", + "cumulative_f32", + "cumulative_f64", + "histogram_i8", + "histogram_u8", + "histogram_i16", + "histogram_u16", + "histogram_i32", + "histogram_u32", + "histogram_i64", + "histogram_u64", + "histogram_f32", + "histogram_f64" + ] + }, "DerEncodedKeyPair": { "type": "object", "properties": { @@ -12596,6 +12647,22 @@ "items" ] }, + "MissingDatum": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + }, + "start_time": { + "nullable": true, + "type": "string", + "format": "date-time" + } + }, + "required": [ + "datum_type" + ] + }, "Name": { "title": "A name unique within the parent collection", "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'. Names cannot be a UUID though they may contain a UUID.", @@ -13519,13 +13586,6 @@ "enum": [ "non_provisionable" ] - }, - { - "description": "This is a state that isn't known yet.\n\nThis is defined to avoid API breakage.", - "type": "string", - "enum": [ - "unknown" - ] } ] }, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index f3462ce55c..3a88b6cc9c 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2898,9 +2898,60 @@ "datum", "type" ] + }, + { + "type": "object", + "properties": { + "datum": { + "$ref": "#/components/schemas/MissingDatum" + }, + "type": { + "type": "string", + "enum": [ + "missing" + ] + } + }, + "required": [ + "datum", + "type" + ] } ] }, + "DatumType": { + "description": "The type of an individual datum of a metric.", + "type": "string", + "enum": [ + "bool", + "i8", + "u8", + "i16", + "u16", + "i32", + "u32", + "i64", + "u64", + "f32", + "f64", + "string", + "bytes", + "cumulative_i64", + "cumulative_u64", + "cumulative_f32", + "cumulative_f64", + "histogram_i8", + "histogram_u8", + "histogram_i16", + "histogram_u16", + "histogram_i32", + "histogram_u32", + "histogram_i64", + "histogram_u64", + "histogram_f32", + "histogram_f64" + ] + }, "DeleteVirtualNetworkInterfaceHost": { "description": "The data needed to identify a virtual IP for which a sled maintains an OPTE virtual-to-physical mapping such that that mapping can be deleted.", "type": "object", @@ -4824,9 +4875,77 @@ "content", "type" ] + }, + { + "type": "object", + "properties": { + "content": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + } + }, + "required": [ + "datum_type" + ] + }, + "type": { + "type": "string", + "enum": [ + "missing_datum_requires_start_time" + ] + } + }, + "required": [ + "content", + "type" + ] + }, + { + "type": "object", + "properties": { + "content": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + } + }, + "required": [ + "datum_type" + ] + }, + "type": { + "type": "string", + "enum": [ + "missing_datum_cannot_have_start_time" + ] + } + }, + "required": [ + "content", + "type" + ] } ] }, + "MissingDatum": { + "type": "object", + "properties": { + "datum_type": { + "$ref": "#/components/schemas/DatumType" + }, + "start_time": { + "nullable": true, + "type": "string", + "format": "date-time" + } + }, + "required": [ + "datum_type" + ] + }, "Name": { "title": "A name unique within the parent collection", "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'. Names cannot be a UUID though they may contain a UUID.", diff --git a/oximeter/db/notes.txt b/oximeter/db/notes.txt deleted file mode 100644 index 66c3871d46..0000000000 --- a/oximeter/db/notes.txt +++ /dev/null @@ -1,232 +0,0 @@ -Some notes on querying - -For pagination: - -- Timeseries name is enough for paginated list timeseries endpoint. -It's just normal keyset pagination. - -- For the timeseries data, we'll be using limit/offset pagination. We'll -run the query to get the consistent timeseries keys each time. This is -the `ScanParams` part of the `WhichPage`. The `PageSelector` is the offset. - - -Now, how to run more complex queries? A good example is something like, -aggregating the timeseries across all but one field. For example, let's -look at the Nexus HTTP latency data. The fields are: - -- name (String) -- id (Uuid) -- route (String) -- method (String) -- status_code (I64) - -Imagine we wanted to look at the average latency by route, so averaged -across all methods and status codes. (Let's ingore name/id) - -We need to group the timeseries keys by route, to find the set of keys -consistent with each different route. ClickHouse provides the `groupArray` -function, which is an aggregate function that collects multiple values -into an array. So we can do: - -``` -SELECT - field_value, - groupArray(timeseries_key) -FROM fields_string -WHERE field_name = 'route' -GROUP BY field_value; - - -┌─field_value───────────────────────────────────────────┬─groupArray(timeseries_key)────────────────┐ -│ /metrics/producers │ [1916712826069192294,6228796576473532827] │ -│ /metrics/collectors │ [1500085842574282480] │ -│ /metrics/collect/e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c │ [15389669872422126367] │ -│ /sled_agents/fb0f7546-4d46-40ca-9d56-cbb810684ca7 │ [1166666993114742619] │ -└───────────────────────────────────────────────────────┴───────────────────────────────────────────┘ -``` - -This gives an array of timeseries keys where the route is each of the values -on the left. - -So at a very high level, we can average all the timeseries values where the keys -are in each of these different arrays. - - -This kinda works. It produces an array of arrays, the counts for each of the -histograms, grouped by the field value. - -``` -SELECT - field_value, - groupArray(counts) -FROM -( - SELECT - field_value, - timeseries_key - FROM fields_string - WHERE field_name = 'route' -) AS f0 -INNER JOIN -( - SELECT * - FROM measurements_histogramf64 -) AS meas USING (timeseries_key) -GROUP BY field_value -``` - -We can extend this `groupArray(bins), groupArray(counts)` to get both. - - -Ok, we're getting somewhere. The aggregation "combinators" modify the behavior of -aggregations, in pretty suprising and powerful ways. For example: - -``` -SELECT - field_value, - sumForEach(counts) -FROM -( - SELECT - field_value, - timeseries_key - FROM fields_string - WHERE field_name = 'route' -) AS f0 -INNER JOIN -( - SELECT * - FROM measurements_histogramf64 -) AS meas USING (timeseries_key) -GROUP BY field_value -``` - -This applies the `-ForEach` combinator to the sum aggregation. This applies the -aggregation to corresponding elements of a sequence (table?) of arrays. We can -do this with any of the aggregations, `avg`, `min`, etc. - - -The `-Resample` combinator also looks interesting. It uses its arguments to create -a set of intervals, and applies the aggregation within each of those intervals. -So sort of a group-by interval or window function. - -Another useful method is `toStartOfInterval`. This takes a timestamp and an interval, -say 5 seconds, or 10 minutes, and returns the interval into which that timestamp -falls. Could be very helpful for aligning/binning data to time intervals. But -it does "round", in that the bins don't start at the first timestamp, but at -the rounded-down interval from that timestamp. - -It's possible to build intervals that start exactly at the first timestamp with: - -``` -SELECT - timestamp, - toStartOfInterval(timestamp, toIntervalMinute(1)) + ( - SELECT toSecond(min(timestamp)) - FROM measurements_histogramf64 - ) -FROM measurements_histogramf64 -``` - -Or some other rounding shenanigans. - - -Putting lots of this together: - -``` -SELECT - f0.field_name, - f0.field_value, - f1.field_name, - f1.field_value, - minForEach(bins), - avgForEach(counts) -FROM -( - SELECT - field_name, - field_value, - timeseries_key - FROM fields_string - WHERE field_name = 'route' -) AS f0 -INNER JOIN -( - SELECT - field_name, - field_value, - timeseries_key - FROM fields_i64 - WHERE field_name = 'status_code' -) AS f1 ON f0.timeseries_key = f1.timeseries_key -INNER JOIN -( - SELECT * - FROM measurements_histogramf64 -) AS meas ON f1.timeseries_key = meas.timeseries_key -GROUP BY - f0.field_name, - f0.field_value, - f1.field_name, - f1.field_value -``` - -This selects the field name/value, and the bin and average count for each -histogram, grouping by route and status code. - -These inner select statements look similar to the ones we already -implement in `field.as_query`. But in that case we select *, and here we -probably don't want to do that to avoid errors about things not being -in aggregations or group by's. - -This works (or is syntactically valid) for scalars, if we replace the -combinators with their non-combinator version: e.g, `avgForEach` -> `avg`. - - -Other rando thoughts. - -It'd be nice to have the query builder be able to handle all these, but -I'm not sure how worth it that is. For example, I don't even think we need -the timeseries keys in this query. For the fields where we are specifying -a condition, we have subqueries like: - -``` -SELECT * -FROM fields_{TYPE} -WHERE field_name = NAME -AND field_value OP VALUE; -``` - -For ones where we _don't_ care, we just have the first three lines: - -``` -SELECT * -FROM fields_{TYPE} -WHERE field_name = NAME; -``` - -We can join successive entries on timeseries keys. - -For straight SELECT queries, that's pretty much it, like we have currently. -For AGGREGATION queries, we need to - -- Have a group-by for each (field_name, field_value) pair. This is true -even when we're unselective on the field, because we are still taking that -field, and we still need to group the keys accordingly. -- Select the consistent timeseries keys. This is so we can correlate the -results of the aggregation back to the field names/values which we still -get from the key-select query. -- Apply the aggregation to the measurements. For scalars, this just the -aggregation. For histograms, this is the `-Array` or `-ForEach` combinator -for that aggregation, depending on what we're applying. -- ??? to the timestamps? -- some alignment, grouping, subsampling? It seems -this has to come from the aggregation query, because there's not a useful -default. - -Speaking of defaults, how do these functions behave with missing data? -Or more subtly, what happens if two histograms (say) have the same number -of bins, but the actual bin edges are different? ClickHouse itself doesn't -deal with this AFAICT, which means we'd need to do that in the client. -Ah, but that is unlikely, since we're only aggregating data from the -same timeseries, with the same key. So far anyway. I'm not sure what'll -happen when we start correlating data between timeseries. diff --git a/oximeter/db/schema/replicated/4/up01.sql b/oximeter/db/schema/replicated/4/up01.sql new file mode 100644 index 0000000000..f36745ae2e --- /dev/null +++ b/oximeter/db/schema/replicated/4/up01.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_bool_local MODIFY COLUMN datum Nullable(UInt8) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up02.sql b/oximeter/db/schema/replicated/4/up02.sql new file mode 100644 index 0000000000..0f76398652 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up02.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_bool MODIFY COLUMN datum Nullable(UInt8) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up03.sql b/oximeter/db/schema/replicated/4/up03.sql new file mode 100644 index 0000000000..175b23d71b --- /dev/null +++ b/oximeter/db/schema/replicated/4/up03.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i8_local MODIFY COLUMN datum Nullable(Int8) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up04.sql b/oximeter/db/schema/replicated/4/up04.sql new file mode 100644 index 0000000000..4c8f22d8e6 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up04.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i8 MODIFY COLUMN datum Nullable(Int8) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up05.sql b/oximeter/db/schema/replicated/4/up05.sql new file mode 100644 index 0000000000..82490a81ca --- /dev/null +++ b/oximeter/db/schema/replicated/4/up05.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u8_local MODIFY COLUMN datum Nullable(UInt8) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up06.sql b/oximeter/db/schema/replicated/4/up06.sql new file mode 100644 index 0000000000..c689682127 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up06.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u8 MODIFY COLUMN datum Nullable(UInt8) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up07.sql b/oximeter/db/schema/replicated/4/up07.sql new file mode 100644 index 0000000000..43eb40515b --- /dev/null +++ b/oximeter/db/schema/replicated/4/up07.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i16_local MODIFY COLUMN datum Nullable(Int16) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up08.sql b/oximeter/db/schema/replicated/4/up08.sql new file mode 100644 index 0000000000..1d983a3c83 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up08.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i16 MODIFY COLUMN datum Nullable(Int16) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up09.sql b/oximeter/db/schema/replicated/4/up09.sql new file mode 100644 index 0000000000..e52c2adf5f --- /dev/null +++ b/oximeter/db/schema/replicated/4/up09.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u16_local MODIFY COLUMN datum Nullable(UInt16) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up10.sql b/oximeter/db/schema/replicated/4/up10.sql new file mode 100644 index 0000000000..d8a69fff1a --- /dev/null +++ b/oximeter/db/schema/replicated/4/up10.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u16 MODIFY COLUMN datum Nullable(UInt16) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up11.sql b/oximeter/db/schema/replicated/4/up11.sql new file mode 100644 index 0000000000..b3c2d8de92 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up11.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i32_local MODIFY COLUMN datum Nullable(Int32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up12.sql b/oximeter/db/schema/replicated/4/up12.sql new file mode 100644 index 0000000000..65fca2e1b2 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up12.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i32 MODIFY COLUMN datum Nullable(Int32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up13.sql b/oximeter/db/schema/replicated/4/up13.sql new file mode 100644 index 0000000000..df7c520e35 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up13.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u32_local MODIFY COLUMN datum Nullable(UInt32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up14.sql b/oximeter/db/schema/replicated/4/up14.sql new file mode 100644 index 0000000000..a4cb43fb90 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up14.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u32 MODIFY COLUMN datum Nullable(UInt32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up15.sql b/oximeter/db/schema/replicated/4/up15.sql new file mode 100644 index 0000000000..f7583dbdee --- /dev/null +++ b/oximeter/db/schema/replicated/4/up15.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i64_local MODIFY COLUMN datum Nullable(Int64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up16.sql b/oximeter/db/schema/replicated/4/up16.sql new file mode 100644 index 0000000000..b458243d74 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up16.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i64 MODIFY COLUMN datum Nullable(Int64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up17.sql b/oximeter/db/schema/replicated/4/up17.sql new file mode 100644 index 0000000000..9229a97704 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up17.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u64_local MODIFY COLUMN datum Nullable(UInt64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up18.sql b/oximeter/db/schema/replicated/4/up18.sql new file mode 100644 index 0000000000..6e2a2a5191 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up18.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u64 MODIFY COLUMN datum Nullable(UInt64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up19.sql b/oximeter/db/schema/replicated/4/up19.sql new file mode 100644 index 0000000000..8f16b5d41e --- /dev/null +++ b/oximeter/db/schema/replicated/4/up19.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_f32_local MODIFY COLUMN datum Nullable(Float32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up20.sql b/oximeter/db/schema/replicated/4/up20.sql new file mode 100644 index 0000000000..9263592740 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up20.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_f32 MODIFY COLUMN datum Nullable(Float32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up21.sql b/oximeter/db/schema/replicated/4/up21.sql new file mode 100644 index 0000000000..72abba6216 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up21.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_f64_local MODIFY COLUMN datum Nullable(Float64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up22.sql b/oximeter/db/schema/replicated/4/up22.sql new file mode 100644 index 0000000000..0d8522bc03 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up22.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_f64 MODIFY COLUMN datum Nullable(Float64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up23.sql b/oximeter/db/schema/replicated/4/up23.sql new file mode 100644 index 0000000000..96b94c2895 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up23.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativei64_local MODIFY COLUMN datum Nullable(Int64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up24.sql b/oximeter/db/schema/replicated/4/up24.sql new file mode 100644 index 0000000000..55df76c25f --- /dev/null +++ b/oximeter/db/schema/replicated/4/up24.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativei64 MODIFY COLUMN datum Nullable(Int64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up25.sql b/oximeter/db/schema/replicated/4/up25.sql new file mode 100644 index 0000000000..fac7369482 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up25.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativeu64_local MODIFY COLUMN datum Nullable(UInt64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up26.sql b/oximeter/db/schema/replicated/4/up26.sql new file mode 100644 index 0000000000..182b2b4704 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up26.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativeu64 MODIFY COLUMN datum Nullable(UInt64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up27.sql b/oximeter/db/schema/replicated/4/up27.sql new file mode 100644 index 0000000000..b482d00f81 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up27.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativef32_local MODIFY COLUMN datum Nullable(Float32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up28.sql b/oximeter/db/schema/replicated/4/up28.sql new file mode 100644 index 0000000000..cefbe56395 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up28.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativef32 MODIFY COLUMN datum Nullable(Float32) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up29.sql b/oximeter/db/schema/replicated/4/up29.sql new file mode 100644 index 0000000000..59e21f353d --- /dev/null +++ b/oximeter/db/schema/replicated/4/up29.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativef64_local MODIFY COLUMN datum Nullable(Float64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up30.sql b/oximeter/db/schema/replicated/4/up30.sql new file mode 100644 index 0000000000..a609e6ad3c --- /dev/null +++ b/oximeter/db/schema/replicated/4/up30.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativef64 MODIFY COLUMN datum Nullable(Float64) \ No newline at end of file diff --git a/oximeter/db/schema/replicated/4/up31.sql b/oximeter/db/schema/replicated/4/up31.sql new file mode 100644 index 0000000000..3726895dd0 --- /dev/null +++ b/oximeter/db/schema/replicated/4/up31.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_string_local MODIFY COLUMN datum Nullable(String); diff --git a/oximeter/db/schema/replicated/4/up32.sql b/oximeter/db/schema/replicated/4/up32.sql new file mode 100644 index 0000000000..5a09705e7e --- /dev/null +++ b/oximeter/db/schema/replicated/4/up32.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_string MODIFY COLUMN datum Nullable(String); diff --git a/oximeter/db/schema/replicated/db-init.sql b/oximeter/db/schema/replicated/db-init.sql index 4429f41364..27df02b709 100644 --- a/oximeter/db/schema/replicated/db-init.sql +++ b/oximeter/db/schema/replicated/db-init.sql @@ -24,7 +24,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_bool_local ON CLUSTER oximeter_ timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt8 + datum Nullable(UInt8) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_bool_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -35,7 +35,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_bool ON CLUSTER oximeter_cluste timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt8 + datum Nullable(UInt8) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_bool_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -44,7 +44,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i8_local ON CLUSTER oximeter_cl timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int8 + datum Nullable(Int8) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_i8_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -55,7 +55,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i8 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int8 + datum Nullable(Int8) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_i8_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -64,7 +64,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u8_local ON CLUSTER oximeter_cl timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt8 + datum Nullable(UInt8) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_u8_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -75,7 +75,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u8 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt8 + datum Nullable(UInt8) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_u8_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -84,7 +84,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i16_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int16 + datum Nullable(Int16) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_i16_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -95,7 +95,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i16 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int16 + datum Nullable(Int16) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_i16_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -104,7 +104,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u16_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt16 + datum Nullable(UInt16) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_u16_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -115,7 +115,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u16 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt16 + datum Nullable(UInt16) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_u16_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -124,7 +124,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i32_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int32 + datum Nullable(Int32) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_i32_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -135,7 +135,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i32 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int32 + datum Nullable(Int32) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_i32_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -144,7 +144,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u32_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt32 + datum Nullable(UInt32) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_u32_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -155,7 +155,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u32 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt32 + datum Nullable(UInt32) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_u32_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -164,7 +164,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i64_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int64 + datum Nullable(Int64) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_i64_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -175,7 +175,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i64 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int64 + datum Nullable(Int64) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_i64_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -184,7 +184,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u64_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt64 + datum Nullable(UInt64) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_u64_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -195,7 +195,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u64 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt64 + datum Nullable(UInt64) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_u64_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -204,7 +204,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_f32_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Float32 + datum Nullable(Float32) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_f32_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -215,7 +215,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_f32 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Float32 + datum Nullable(Float32) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_f32_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -224,7 +224,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_f64_local ON CLUSTER oximeter_c timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Float64 + datum Nullable(Float64) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_f64_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -235,7 +235,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_f64 ON CLUSTER oximeter_cluster timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Float64 + datum Nullable(Float64) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_f64_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -244,7 +244,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_string_local ON CLUSTER oximete timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum String + datum Nullable(String) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_string_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -255,7 +255,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_string ON CLUSTER oximeter_clus timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum String + datum Nullable(String) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_string_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -285,7 +285,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativei64_local ON CLUSTER timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Int64 + datum Nullable(Int64) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_cumulativei64_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -297,7 +297,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativei64 ON CLUSTER oximet timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Int64 + datum Nullable(Int64) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_cumulativei64_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -307,7 +307,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativeu64_local ON CLUSTER timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum UInt64 + datum Nullable(UInt64) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_cumulativeu64_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -319,7 +319,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativeu64 ON CLUSTER oximet timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum UInt64 + datum Nullable(UInt64) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_cumulativeu64_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -329,7 +329,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativef32_local ON CLUSTER timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Float32 + datum Nullable(Float32) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_cumulativef32_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -341,7 +341,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativef32 ON CLUSTER oximet timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Float32 + datum Nullable(Float32) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_cumulativef32_local', xxHash64(splitByChar(':', timeseries_name)[1])); @@ -351,7 +351,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativef64_local ON CLUSTER timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Float64 + datum Nullable(Float64) ) ENGINE = ReplicatedMergeTree('/clickhouse/tables/{shard}/measurements_cumulativef64_local', '{replica}') ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -363,7 +363,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativef64 ON CLUSTER oximet timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Float64 + datum Nullable(Float64) ) ENGINE = Distributed('oximeter_cluster', 'oximeter', 'measurements_cumulativef64_local', xxHash64(splitByChar(':', timeseries_name)[1])); diff --git a/oximeter/db/schema/single-node/4/up01.sql b/oximeter/db/schema/single-node/4/up01.sql new file mode 100644 index 0000000000..ccccc9c5fb --- /dev/null +++ b/oximeter/db/schema/single-node/4/up01.sql @@ -0,0 +1,9 @@ +/* + * To support missing measurements, we are making all scalar datum columns + * Nullable, so that a NULL value (None in Rust) represents a missing datum at + * the provided timestamp. + * + * Note that arrays cannot be made Nullable, so we need to use an empty array as + * the sentinel value implying a missing measurement. + */ +ALTER TABLE oximeter.measurements_bool MODIFY COLUMN datum Nullable(UInt8) diff --git a/oximeter/db/schema/single-node/4/up02.sql b/oximeter/db/schema/single-node/4/up02.sql new file mode 100644 index 0000000000..4c8f22d8e6 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up02.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i8 MODIFY COLUMN datum Nullable(Int8) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up03.sql b/oximeter/db/schema/single-node/4/up03.sql new file mode 100644 index 0000000000..c689682127 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up03.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u8 MODIFY COLUMN datum Nullable(UInt8) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up04.sql b/oximeter/db/schema/single-node/4/up04.sql new file mode 100644 index 0000000000..1d983a3c83 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up04.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i16 MODIFY COLUMN datum Nullable(Int16) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up05.sql b/oximeter/db/schema/single-node/4/up05.sql new file mode 100644 index 0000000000..d8a69fff1a --- /dev/null +++ b/oximeter/db/schema/single-node/4/up05.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u16 MODIFY COLUMN datum Nullable(UInt16) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up06.sql b/oximeter/db/schema/single-node/4/up06.sql new file mode 100644 index 0000000000..65fca2e1b2 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up06.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i32 MODIFY COLUMN datum Nullable(Int32) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up07.sql b/oximeter/db/schema/single-node/4/up07.sql new file mode 100644 index 0000000000..a4cb43fb90 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up07.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u32 MODIFY COLUMN datum Nullable(UInt32) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up08.sql b/oximeter/db/schema/single-node/4/up08.sql new file mode 100644 index 0000000000..b458243d74 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up08.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_i64 MODIFY COLUMN datum Nullable(Int64) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up09.sql b/oximeter/db/schema/single-node/4/up09.sql new file mode 100644 index 0000000000..6e2a2a5191 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up09.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_u64 MODIFY COLUMN datum Nullable(UInt64) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up10.sql b/oximeter/db/schema/single-node/4/up10.sql new file mode 100644 index 0000000000..9263592740 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up10.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_f32 MODIFY COLUMN datum Nullable(Float32) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up11.sql b/oximeter/db/schema/single-node/4/up11.sql new file mode 100644 index 0000000000..0d8522bc03 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up11.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_f64 MODIFY COLUMN datum Nullable(Float64) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up12.sql b/oximeter/db/schema/single-node/4/up12.sql new file mode 100644 index 0000000000..55df76c25f --- /dev/null +++ b/oximeter/db/schema/single-node/4/up12.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativei64 MODIFY COLUMN datum Nullable(Int64) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up13.sql b/oximeter/db/schema/single-node/4/up13.sql new file mode 100644 index 0000000000..182b2b4704 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up13.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativeu64 MODIFY COLUMN datum Nullable(UInt64) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up14.sql b/oximeter/db/schema/single-node/4/up14.sql new file mode 100644 index 0000000000..cefbe56395 --- /dev/null +++ b/oximeter/db/schema/single-node/4/up14.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativef32 MODIFY COLUMN datum Nullable(Float32) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up15.sql b/oximeter/db/schema/single-node/4/up15.sql new file mode 100644 index 0000000000..a609e6ad3c --- /dev/null +++ b/oximeter/db/schema/single-node/4/up15.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_cumulativef64 MODIFY COLUMN datum Nullable(Float64) \ No newline at end of file diff --git a/oximeter/db/schema/single-node/4/up16.sql b/oximeter/db/schema/single-node/4/up16.sql new file mode 100644 index 0000000000..5a09705e7e --- /dev/null +++ b/oximeter/db/schema/single-node/4/up16.sql @@ -0,0 +1 @@ +ALTER TABLE oximeter.measurements_string MODIFY COLUMN datum Nullable(String); diff --git a/oximeter/db/schema/single-node/db-init.sql b/oximeter/db/schema/single-node/db-init.sql index ee5e91c4b7..510c1071c8 100644 --- a/oximeter/db/schema/single-node/db-init.sql +++ b/oximeter/db/schema/single-node/db-init.sql @@ -24,7 +24,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_bool timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt8 + datum Nullable(UInt8) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -35,7 +35,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i8 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int8 + datum Nullable(Int8) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -46,7 +46,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u8 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt8 + datum Nullable(UInt8) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -57,7 +57,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i16 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int16 + datum Nullable(Int16) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -68,7 +68,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u16 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt16 + datum Nullable(UInt16) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -79,7 +79,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i32 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int32 + datum Nullable(Int32) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -90,7 +90,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u32 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt32 + datum Nullable(UInt32) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -101,7 +101,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_i64 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Int64 + datum Nullable(Int64) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -112,7 +112,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_u64 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum UInt64 + datum Nullable(UInt64) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -123,7 +123,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_f32 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Float32 + datum Nullable(Float32) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -134,7 +134,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_f64 timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum Float64 + datum Nullable(Float64) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -145,7 +145,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_string timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), - datum String + datum Nullable(String) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, timestamp) @@ -156,6 +156,13 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_bytes timeseries_name String, timeseries_key UInt64, timestamp DateTime64(9, 'UTC'), + /* + * NOTE: Right now we can't unambiguously record a nullable byte array. + * Arrays cannot be nested in `Nullable()` types, and encoding the array as + * a string isn't palatable for a few reasons. + * See: https://github.com/oxidecomputer/omicron/issues/4551 for more + * details. + */ datum Array(UInt8) ) ENGINE = MergeTree() @@ -168,7 +175,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativei64 timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Int64 + datum Nullable(Int64) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -180,7 +187,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativeu64 timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum UInt64 + datum Nullable(UInt64) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -192,7 +199,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativef32 timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Float32 + datum Nullable(Float32) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -205,7 +212,7 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_cumulativef64 timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), - datum Float64 + datum Nullable(Float64) ) ENGINE = MergeTree() ORDER BY (timeseries_name, timeseries_key, start_time, timestamp) @@ -217,6 +224,16 @@ CREATE TABLE IF NOT EXISTS oximeter.measurements_histogrami8 timeseries_key UInt64, start_time DateTime64(9, 'UTC'), timestamp DateTime64(9, 'UTC'), + /* + * NOTE: Array types cannot be Nullable, see + * https://clickhouse.com/docs/en/sql-reference/data-types/nullable + * for more details. + * + * This means we need to use empty arrays to indicate a missing value. This + * is unfortunate, and at this point relies on the fact that an + * `oximeter::Histogram` cannot have zero bins. If that changes, we'll need + * to figure out another way to represent missing samples here. + */ bins Array(Int8), counts Array(UInt64) ) diff --git a/oximeter/db/src/client.rs b/oximeter/db/src/client.rs index e1ed06554c..c8a7db20cb 100644 --- a/oximeter/db/src/client.rs +++ b/oximeter/db/src/client.rs @@ -1190,7 +1190,7 @@ mod tests { use super::*; use crate::query; use crate::query::field_table_name; - use crate::query::measurement_table_name; + use bytes::Bytes; use chrono::Utc; use omicron_test_utils::dev::clickhouse::{ ClickHouseCluster, ClickHouseInstance, @@ -1198,8 +1198,10 @@ mod tests { use omicron_test_utils::dev::test_setup_log; use oximeter::histogram::Histogram; use oximeter::test_util; + use oximeter::types::MissingDatum; use oximeter::Datum; use oximeter::FieldValue; + use oximeter::Measurement; use oximeter::Metric; use oximeter::Target; use std::net::Ipv6Addr; @@ -2957,76 +2959,102 @@ mod tests { Ok(()) } + async fn test_recall_missing_scalar_measurement_impl( + measurement: Measurement, + client: &Client, + ) -> Result<(), Error> { + let start_time = if measurement.datum().is_cumulative() { + Some(Utc::now()) + } else { + None + }; + let missing_datum = Datum::from( + MissingDatum::new(measurement.datum_type(), start_time).unwrap(), + ); + let missing_measurement = Measurement::new(Utc::now(), missing_datum); + test_recall_measurement_impl(missing_measurement, client).await?; + Ok(()) + } + async fn recall_measurement_bool_test( client: &Client, ) -> Result<(), Error> { let datum = Datum::Bool(true); - let as_json = serde_json::Value::from(1_u64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_i8_test(client: &Client) -> Result<(), Error> { let datum = Datum::I8(1); - let as_json = serde_json::Value::from(1_i8); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_u8_test(client: &Client) -> Result<(), Error> { let datum = Datum::U8(1); - let as_json = serde_json::Value::from(1_u8); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_i16_test(client: &Client) -> Result<(), Error> { let datum = Datum::I16(1); - let as_json = serde_json::Value::from(1_i16); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_u16_test(client: &Client) -> Result<(), Error> { let datum = Datum::U16(1); - let as_json = serde_json::Value::from(1_u16); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_i32_test(client: &Client) -> Result<(), Error> { let datum = Datum::I32(1); - let as_json = serde_json::Value::from(1_i32); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_u32_test(client: &Client) -> Result<(), Error> { let datum = Datum::U32(1); - let as_json = serde_json::Value::from(1_u32); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_i64_test(client: &Client) -> Result<(), Error> { let datum = Datum::I64(1); - let as_json = serde_json::Value::from(1_i64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } async fn recall_measurement_u64_test(client: &Client) -> Result<(), Error> { let datum = Datum::U64(1); - let as_json = serde_json::Value::from(1_u64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } @@ -3034,9 +3062,9 @@ mod tests { async fn recall_measurement_f32_test(client: &Client) -> Result<(), Error> { const VALUE: f32 = 1.1; let datum = Datum::F32(VALUE); - // NOTE: This is intentionally an f64. - let as_json = serde_json::Value::from(1.1_f64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } @@ -3044,18 +3072,43 @@ mod tests { async fn recall_measurement_f64_test(client: &Client) -> Result<(), Error> { const VALUE: f64 = 1.1; let datum = Datum::F64(VALUE); - let as_json = serde_json::Value::from(VALUE); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } + async fn recall_measurement_string_test( + client: &Client, + ) -> Result<(), Error> { + let value = String::from("foo"); + let datum = Datum::String(value.clone()); + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) + .await?; + Ok(()) + } + + async fn recall_measurement_bytes_test( + client: &Client, + ) -> Result<(), Error> { + let value = Bytes::from(vec![0, 1, 2]); + let datum = Datum::Bytes(value.clone()); + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + // NOTE: We don't currently support missing byte array samples. + Ok(()) + } + async fn recall_measurement_cumulative_i64_test( client: &Client, ) -> Result<(), Error> { let datum = Datum::CumulativeI64(1.into()); - let as_json = serde_json::Value::from(1_i64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } @@ -3064,8 +3117,9 @@ mod tests { client: &Client, ) -> Result<(), Error> { let datum = Datum::CumulativeU64(1.into()); - let as_json = serde_json::Value::from(1_u64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } @@ -3074,8 +3128,9 @@ mod tests { client: &Client, ) -> Result<(), Error> { let datum = Datum::CumulativeF64(1.1.into()); - let as_json = serde_json::Value::from(1.1_f64); - test_recall_measurement_impl::(datum, None, as_json, client) + let measurement = Measurement::new(Utc::now(), datum); + test_recall_measurement_impl(measurement.clone(), client).await?; + test_recall_missing_scalar_measurement_impl(measurement, client) .await?; Ok(()) } @@ -3089,13 +3144,15 @@ mod tests { Datum: From>, serde_json::Value: From, { - let (bins, counts) = hist.to_arrays(); let datum = Datum::from(hist); - let as_json = serde_json::Value::Array( - counts.into_iter().map(Into::into).collect(), + let measurement = Measurement::new(Utc::now(), datum); + let missing_datum = Datum::Missing( + MissingDatum::new(measurement.datum_type(), Some(Utc::now())) + .unwrap(), ); - test_recall_measurement_impl(datum, Some(bins), as_json, client) - .await?; + let missing_measurement = Measurement::new(Utc::now(), missing_datum); + test_recall_measurement_impl(measurement, client).await?; + test_recall_measurement_impl(missing_measurement, client).await?; Ok(()) } @@ -3192,54 +3249,23 @@ mod tests { Ok(()) } - async fn test_recall_measurement_impl + Copy>( - datum: Datum, - maybe_bins: Option>, - json_datum: serde_json::Value, + async fn test_recall_measurement_impl( + measurement: Measurement, client: &Client, ) -> Result<(), Error> { // Insert a record from this datum. const TIMESERIES_NAME: &str = "foo:bar"; const TIMESERIES_KEY: u64 = 101; - let mut inserted_row = serde_json::Map::new(); - inserted_row - .insert("timeseries_name".to_string(), TIMESERIES_NAME.into()); - inserted_row - .insert("timeseries_key".to_string(), TIMESERIES_KEY.into()); - inserted_row.insert( - "timestamp".to_string(), - Utc::now() - .format(crate::DATABASE_TIMESTAMP_FORMAT) - .to_string() - .into(), - ); - - // Insert the start time and possibly bins. - if let Some(start_time) = datum.start_time() { - inserted_row.insert( - "start_time".to_string(), - start_time - .format(crate::DATABASE_TIMESTAMP_FORMAT) - .to_string() - .into(), - ); - } - if let Some(bins) = &maybe_bins { - let bins = serde_json::Value::Array( - bins.iter().copied().map(Into::into).collect(), + let (measurement_table, inserted_row) = + crate::model::unroll_measurement_row_impl( + TIMESERIES_NAME.to_string(), + TIMESERIES_KEY, + &measurement, ); - inserted_row.insert("bins".to_string(), bins); - inserted_row.insert("counts".to_string(), json_datum); - } else { - inserted_row.insert("datum".to_string(), json_datum); - } - let inserted_row = serde_json::Value::from(inserted_row); - - let measurement_table = measurement_table_name(datum.datum_type()); - let row = serde_json::to_string(&inserted_row).unwrap(); let insert_sql = format!( - "INSERT INTO oximeter.{measurement_table} FORMAT JSONEachRow {row}", + "INSERT INTO {measurement_table} FORMAT JSONEachRow {inserted_row}", ); + println!("Inserted row: {}", inserted_row); client .execute(insert_sql) .await @@ -3247,21 +3273,22 @@ mod tests { // Select it exactly back out. let select_sql = format!( - "SELECT * FROM oximeter.{} LIMIT 2 FORMAT {};", + "SELECT * FROM {} WHERE timestamp = '{}' FORMAT {};", measurement_table, + measurement.timestamp().format(crate::DATABASE_TIMESTAMP_FORMAT), crate::DATABASE_SELECT_FORMAT, ); let body = client .execute_with_body(select_sql) .await .expect("Failed to select measurement row"); - println!("{}", body); - let actual_row: serde_json::Value = serde_json::from_str(&body) - .expect("Failed to parse measurement row JSON"); - println!("{actual_row:?}"); - println!("{inserted_row:?}"); + let (_, actual_row) = crate::model::parse_measurement_from_row( + &body, + measurement.datum_type(), + ); + println!("Actual row: {actual_row:?}"); assert_eq!( - actual_row, inserted_row, + actual_row, measurement, "Actual and expected measurement rows do not match" ); Ok(()) @@ -3311,6 +3338,10 @@ mod tests { recall_measurement_f64_test(&client).await.unwrap(); + recall_measurement_string_test(&client).await.unwrap(); + + recall_measurement_bytes_test(&client).await.unwrap(); + recall_measurement_cumulative_i64_test(&client).await.unwrap(); recall_measurement_cumulative_u64_test(&client).await.unwrap(); diff --git a/oximeter/db/src/model.rs b/oximeter/db/src/model.rs index 715e025a04..d92e646e89 100644 --- a/oximeter/db/src/model.rs +++ b/oximeter/db/src/model.rs @@ -26,6 +26,7 @@ use oximeter::types::Field; use oximeter::types::FieldType; use oximeter::types::FieldValue; use oximeter::types::Measurement; +use oximeter::types::MissingDatum; use oximeter::types::Sample; use serde::Deserialize; use serde::Serialize; @@ -43,7 +44,7 @@ use uuid::Uuid; /// - [`crate::Client::initialize_db_with_version`] /// - [`crate::Client::ensure_schema`] /// - The `clickhouse-schema-updater` binary in this crate -pub const OXIMETER_VERSION: u64 = 3; +pub const OXIMETER_VERSION: u64 = 4; // Wrapper type to represent a boolean in the database. // @@ -212,6 +213,7 @@ impl From for DbFieldType { } } } + #[derive(Clone, Copy, Debug, Serialize, Deserialize, PartialEq)] pub enum DbDatumType { Bool, @@ -402,7 +404,7 @@ macro_rules! declare_measurement_row { timeseries_key: TimeseriesKey, #[serde(with = "serde_timestamp")] timestamp: DateTime, - datum: $datum_type, + datum: Option<$datum_type>, } impl_table_name!{$name, "measurements", $data_type} @@ -433,7 +435,7 @@ macro_rules! declare_cumulative_measurement_row { start_time: DateTime, #[serde(with = "serde_timestamp")] timestamp: DateTime, - datum: $datum_type, + datum: Option<$datum_type>, } impl_table_name!{$name, "measurements", $data_type} @@ -456,6 +458,22 @@ struct DbHistogram { pub counts: Vec, } +// We use an empty histogram to indicate a missing sample. +// +// While ClickHouse supports nullable types, the inner type can't be a +// "composite", which includes arrays. I.e., `Nullable(Array(UInt8))` can't be +// used. This is unfortunate, but we are aided by the fact that it's not +// possible to have an `oximeter` histogram that contains zero bins right now. +// This is checked by a test in `oximeter::histogram`. +// +// That means we can currently use an empty array from the database as a +// sentinel for a missing sample. +impl DbHistogram { + fn null() -> Self { + Self { bins: vec![], counts: vec![] } + } +} + impl From<&Histogram> for DbHistogram where T: traits::HistogramSupport, @@ -647,270 +665,571 @@ pub(crate) fn unroll_measurement_row(sample: &Sample) -> (String, String) { let timeseries_name = sample.timeseries_name.clone(); let timeseries_key = crate::timeseries_key(sample); let measurement = &sample.measurement; + unroll_measurement_row_impl(timeseries_name, timeseries_key, measurement) +} + +/// Given a sample's measurement, return a table name and row to insert. +/// +/// This returns a tuple giving the name of the table, and the JSON +/// representation for the serialized row to be inserted into that table, +/// written out as a string. +pub(crate) fn unroll_measurement_row_impl( + timeseries_name: String, + timeseries_key: TimeseriesKey, + measurement: &Measurement, +) -> (String, String) { let timestamp = measurement.timestamp(); let extract_start_time = |measurement: &Measurement| { measurement .start_time() .expect("Cumulative measurements must have a start time") }; + match measurement.datum() { Datum::Bool(inner) => { + let datum = Some(DbBool::from(*inner)); let row = BoolMeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: DbBool::from(*inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::I8(inner) => { + let datum = Some(*inner); let row = I8MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::U8(inner) => { + let datum = Some(*inner); let row = U8MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::I16(inner) => { + let datum = Some(*inner); let row = I16MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::U16(inner) => { + let datum = Some(*inner); let row = U16MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::I32(inner) => { + let datum = Some(*inner); let row = I32MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::U32(inner) => { + let datum = Some(*inner); let row = U32MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::I64(inner) => { + let datum = Some(*inner); let row = I64MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::U64(inner) => { + let datum = Some(*inner); let row = U64MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::F32(inner) => { + let datum = Some(*inner); let row = F32MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::F64(inner) => { + let datum = Some(*inner); let row = F64MeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: *inner, + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::String(ref inner) => { + Datum::String(inner) => { + let datum = Some(inner.clone()); let row = StringMeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: inner.clone(), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::Bytes(ref inner) => { + Datum::Bytes(inner) => { + let datum = Some(inner.clone()); let row = BytesMeasurementRow { timeseries_name, timeseries_key, timestamp, - datum: inner.clone(), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::CumulativeI64(inner) => { + let datum = Some(inner.value()); let row = CumulativeI64MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: inner.value(), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::CumulativeU64(inner) => { + let datum = Some(inner.value()); let row = CumulativeU64MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: inner.value(), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::CumulativeF32(inner) => { + let datum = Some(inner.value()); let row = CumulativeF32MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: inner.value(), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } Datum::CumulativeF64(inner) => { + let datum = Some(inner.value()); let row = CumulativeF64MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: inner.value(), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramI8(ref inner) => { + Datum::HistogramI8(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramI8MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramU8(ref inner) => { + Datum::HistogramU8(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramU8MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramI16(ref inner) => { + Datum::HistogramI16(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramI16MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramU16(ref inner) => { + Datum::HistogramU16(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramU16MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramI32(ref inner) => { + Datum::HistogramI32(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramI32MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramU32(ref inner) => { + Datum::HistogramU32(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramU32MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramI64(ref inner) => { + Datum::HistogramI64(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramI64MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramU64(ref inner) => { + Datum::HistogramU64(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramU64MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramF32(ref inner) => { + Datum::HistogramF32(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramF32MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } - Datum::HistogramF64(ref inner) => { + Datum::HistogramF64(inner) => { + let datum = DbHistogram::from(inner); let row = HistogramF64MeasurementRow { timeseries_name, timeseries_key, start_time: extract_start_time(measurement), timestamp, - datum: DbHistogram::from(inner), + datum, }; (row.table_name(), serde_json::to_string(&row).unwrap()) } + Datum::Missing(missing) => { + match missing.datum_type() { + DatumType::Bool => { + let row = BoolMeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::I8 => { + let row = I8MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::U8 => { + let row = U8MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::I16 => { + let row = I16MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::U16 => { + let row = U16MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::I32 => { + let row = I32MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::U32 => { + let row = U32MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::I64 => { + let row = I64MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::U64 => { + let row = U64MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::F32 => { + let row = F32MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::F64 => { + let row = F64MeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::String => { + let row = StringMeasurementRow { + timeseries_name, + timeseries_key, + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::Bytes => { + // See https://github.com/oxidecomputer/omicron/issues/4551. + // + // This is actually unreachable today because the constuctor + // for `oximeter::types::MissingDatum` fails when using a + // `DatumType::Bytes`. + unreachable!(); + } + DatumType::CumulativeI64 => { + let row = CumulativeI64MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::CumulativeU64 => { + let row = CumulativeU64MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::CumulativeF32 => { + let row = CumulativeF32MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::CumulativeF64 => { + let row = CumulativeF64MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: None, + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramI8 => { + let row = HistogramI8MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramU8 => { + let row = HistogramU8MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramI16 => { + let row = HistogramI16MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramU16 => { + let row = HistogramU16MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramI32 => { + let row = HistogramI32MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramU32 => { + let row = HistogramU32MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramI64 => { + let row = HistogramI64MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramU64 => { + let row = HistogramU64MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramF32 => { + let row = HistogramF32MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + DatumType::HistogramF64 => { + let row = HistogramF64MeasurementRow { + timeseries_name, + timeseries_key, + start_time: extract_start_time(measurement), + timestamp, + datum: DbHistogram::null(), + }; + (row.table_name(), serde_json::to_string(&row).unwrap()) + } + } + } } } @@ -984,7 +1303,7 @@ struct DbTimeseriesScalarGaugeSample { timeseries_key: TimeseriesKey, #[serde(with = "serde_timestamp")] timestamp: DateTime, - datum: T, + datum: Option, } // A scalar timestamped sample from a cumulative timeseries, as extracted from a query to the @@ -996,7 +1315,7 @@ struct DbTimeseriesScalarCumulativeSample { start_time: DateTime, #[serde(with = "serde_timestamp")] timestamp: DateTime, - datum: T, + datum: Option, } // A histogram timestamped sample from a timeseries, as extracted from a query to the database. @@ -1014,9 +1333,15 @@ struct DbTimeseriesHistogramSample { impl From> for Measurement where Datum: From, + T: FromDbScalar, { fn from(sample: DbTimeseriesScalarGaugeSample) -> Measurement { - let datum = Datum::from(sample.datum); + let datum = match sample.datum { + Some(datum) => Datum::from(datum), + None => { + Datum::Missing(MissingDatum::new(T::DATUM_TYPE, None).unwrap()) + } + }; Measurement::new(sample.timestamp, datum) } } @@ -1024,12 +1349,19 @@ where impl From> for Measurement where Datum: From>, - T: traits::Cumulative, + T: traits::Cumulative + FromDbCumulative, { fn from(sample: DbTimeseriesScalarCumulativeSample) -> Measurement { - let cumulative = - Cumulative::with_start_time(sample.start_time, sample.datum); - let datum = Datum::from(cumulative); + let datum = match sample.datum { + Some(datum) => Datum::from(Cumulative::with_start_time( + sample.start_time, + datum, + )), + None => Datum::Missing( + MissingDatum::new(T::DATUM_TYPE, Some(sample.start_time)) + .unwrap(), + ), + }; Measurement::new(sample.timestamp, datum) } } @@ -1037,26 +1369,157 @@ where impl From> for Measurement where Datum: From>, - T: traits::HistogramSupport, + T: traits::HistogramSupport + FromDbHistogram, { fn from(sample: DbTimeseriesHistogramSample) -> Measurement { - let datum = Datum::from( - Histogram::from_arrays( - sample.start_time, - sample.bins, - sample.counts, + let datum = if sample.bins.is_empty() { + assert!(sample.counts.is_empty()); + Datum::Missing( + MissingDatum::new(T::DATUM_TYPE, Some(sample.start_time)) + .unwrap(), ) - .unwrap(), - ); + } else { + Datum::from( + Histogram::from_arrays( + sample.start_time, + sample.bins, + sample.counts, + ) + .unwrap(), + ) + }; Measurement::new(sample.timestamp, datum) } } +// Helper trait providing the DatumType for a corresponding scalar DB value. +// +// This is used in `parse_timeseries_scalar_gauge_measurement`. +trait FromDbScalar { + const DATUM_TYPE: DatumType; +} + +impl FromDbScalar for DbBool { + const DATUM_TYPE: DatumType = DatumType::Bool; +} + +impl FromDbScalar for i8 { + const DATUM_TYPE: DatumType = DatumType::I8; +} + +impl FromDbScalar for u8 { + const DATUM_TYPE: DatumType = DatumType::U8; +} + +impl FromDbScalar for i16 { + const DATUM_TYPE: DatumType = DatumType::I16; +} + +impl FromDbScalar for u16 { + const DATUM_TYPE: DatumType = DatumType::U16; +} + +impl FromDbScalar for i32 { + const DATUM_TYPE: DatumType = DatumType::I32; +} + +impl FromDbScalar for u32 { + const DATUM_TYPE: DatumType = DatumType::U32; +} + +impl FromDbScalar for i64 { + const DATUM_TYPE: DatumType = DatumType::I64; +} + +impl FromDbScalar for u64 { + const DATUM_TYPE: DatumType = DatumType::U64; +} + +impl FromDbScalar for f32 { + const DATUM_TYPE: DatumType = DatumType::F32; +} + +impl FromDbScalar for f64 { + const DATUM_TYPE: DatumType = DatumType::F64; +} + +impl FromDbScalar for String { + const DATUM_TYPE: DatumType = DatumType::String; +} + +impl FromDbScalar for Bytes { + const DATUM_TYPE: DatumType = DatumType::Bytes; +} + +trait FromDbCumulative { + const DATUM_TYPE: DatumType; +} + +impl FromDbCumulative for i64 { + const DATUM_TYPE: DatumType = DatumType::CumulativeI64; +} + +impl FromDbCumulative for u64 { + const DATUM_TYPE: DatumType = DatumType::CumulativeU64; +} + +impl FromDbCumulative for f32 { + const DATUM_TYPE: DatumType = DatumType::CumulativeF32; +} + +impl FromDbCumulative for f64 { + const DATUM_TYPE: DatumType = DatumType::CumulativeF64; +} + +trait FromDbHistogram { + const DATUM_TYPE: DatumType; +} + +impl FromDbHistogram for i8 { + const DATUM_TYPE: DatumType = DatumType::HistogramI8; +} + +impl FromDbHistogram for u8 { + const DATUM_TYPE: DatumType = DatumType::HistogramU8; +} + +impl FromDbHistogram for i16 { + const DATUM_TYPE: DatumType = DatumType::HistogramI16; +} + +impl FromDbHistogram for u16 { + const DATUM_TYPE: DatumType = DatumType::HistogramU16; +} + +impl FromDbHistogram for i32 { + const DATUM_TYPE: DatumType = DatumType::HistogramI32; +} + +impl FromDbHistogram for u32 { + const DATUM_TYPE: DatumType = DatumType::HistogramU32; +} + +impl FromDbHistogram for i64 { + const DATUM_TYPE: DatumType = DatumType::HistogramI64; +} + +impl FromDbHistogram for u64 { + const DATUM_TYPE: DatumType = DatumType::HistogramU64; +} + +impl FromDbHistogram for f32 { + const DATUM_TYPE: DatumType = DatumType::HistogramF32; +} + +impl FromDbHistogram for f64 { + const DATUM_TYPE: DatumType = DatumType::HistogramF64; +} + fn parse_timeseries_scalar_gauge_measurement<'a, T>( line: &'a str, ) -> (TimeseriesKey, Measurement) where - T: Deserialize<'a> + Into, + T: Deserialize<'a> + Into + FromDbScalar, Datum: From, { let sample = @@ -1068,7 +1531,7 @@ fn parse_timeseries_scalar_cumulative_measurement<'a, T>( line: &'a str, ) -> (TimeseriesKey, Measurement) where - T: Deserialize<'a> + traits::Cumulative, + T: Deserialize<'a> + traits::Cumulative + FromDbCumulative, Datum: From>, { let sample = @@ -1081,7 +1544,7 @@ fn parse_timeseries_histogram_measurement( line: &str, ) -> (TimeseriesKey, Measurement) where - T: Into + traits::HistogramSupport, + T: Into + traits::HistogramSupport + FromDbHistogram, Datum: From>, { let sample = @@ -1459,6 +1922,27 @@ mod tests { } } + // Test that we correctly unroll a row when the measurement is missing its + // datum. + #[test] + fn test_unroll_missing_measurement_row() { + let sample = test_util::make_sample(); + let missing_sample = test_util::make_missing_sample(); + let (table_name, row) = unroll_measurement_row(&sample); + let (missing_table_name, missing_row) = + unroll_measurement_row(&missing_sample); + let row = serde_json::from_str::(&row).unwrap(); + let missing_row = + serde_json::from_str::(&missing_row).unwrap(); + println!("{row:#?}"); + println!("{missing_row:#?}"); + assert_eq!(table_name, missing_table_name); + assert_eq!(row.timeseries_name, missing_row.timeseries_name); + assert_eq!(row.timeseries_key, missing_row.timeseries_key); + assert!(row.datum.is_some()); + assert!(missing_row.datum.is_none()); + } + #[test] fn test_unroll_measurement_row() { let sample = test_util::make_hist_sample(); @@ -1473,14 +1957,13 @@ mod tests { ) .unwrap(); let measurement = &sample.measurement; - if let Datum::HistogramF64(hist) = measurement.datum() { - assert_eq!( - hist, &unpacked_hist, - "Unpacking histogram from database representation failed" - ); - } else { + let Datum::HistogramF64(hist) = measurement.datum() else { panic!("Expected a histogram measurement"); - } + }; + assert_eq!( + hist, &unpacked_hist, + "Unpacking histogram from database representation failed" + ); assert_eq!(unpacked.start_time, measurement.start_time().unwrap()); } @@ -1582,12 +2065,11 @@ mod tests { assert_eq!(key, 12); assert_eq!(measurement.start_time().unwrap(), start_time); assert_eq!(measurement.timestamp(), timestamp); - if let Datum::HistogramI64(hist) = measurement.datum() { - assert_eq!(hist.n_bins(), 3); - assert_eq!(hist.n_samples(), 2); - } else { + let Datum::HistogramI64(hist) = measurement.datum() else { panic!("Expected a histogram sample"); - } + }; + assert_eq!(hist.n_bins(), 3); + assert_eq!(hist.n_samples(), 2); } #[test] @@ -1624,4 +2106,14 @@ mod tests { "Histogram reconstructed from paired arrays is not correct" ); } + #[test] + fn test_parse_bytes_measurement() { + let s = r#"{"timeseries_key": 101, "timestamp": "2023-11-21 18:25:21.963714255", "datum": "\u0001\u0002\u0003"}"#; + let (_, meas) = parse_timeseries_scalar_gauge_measurement::(&s); + println!("{meas:?}"); + let Datum::Bytes(b) = meas.datum() else { + unreachable!(); + }; + assert_eq!(b.to_vec(), vec![1, 2, 3]); + } } diff --git a/oximeter/oximeter/Cargo.toml b/oximeter/oximeter/Cargo.toml index 8a69494d5a..0cb2d8cace 100644 --- a/oximeter/oximeter/Cargo.toml +++ b/oximeter/oximeter/Cargo.toml @@ -21,4 +21,5 @@ omicron-workspace-hack.workspace = true [dev-dependencies] approx.workspace = true rstest.workspace = true +serde_json.workspace = true trybuild.workspace = true diff --git a/oximeter/oximeter/src/histogram.rs b/oximeter/oximeter/src/histogram.rs index c399384ffa..aaf9297ca4 100644 --- a/oximeter/oximeter/src/histogram.rs +++ b/oximeter/oximeter/src/histogram.rs @@ -1353,13 +1353,10 @@ mod tests { } #[test] - fn test_foo() { - let bins: Vec = 10u16.bins(1, 3, 30.try_into().unwrap()).unwrap(); - println!("{bins:?}"); - dbg!(bins.len()); - let hist = Histogram::new(&bins).unwrap(); - for bin in hist.iter() { - println!("{}", bin.range); - } + fn test_empty_bins_not_supported() { + assert!(matches!( + Histogram::::new(&[]).unwrap_err(), + HistogramError::EmptyBins + )); } } diff --git a/oximeter/oximeter/src/test_util.rs b/oximeter/oximeter/src/test_util.rs index f3750d6d83..a9778d03bc 100644 --- a/oximeter/oximeter/src/test_util.rs +++ b/oximeter/oximeter/src/test_util.rs @@ -48,19 +48,27 @@ pub struct TestHistogram { pub datum: Histogram, } +const ID: Uuid = uuid::uuid!("e00ced4d-39d1-446a-ae85-a67f05c9750b"); + pub fn make_sample() -> Sample { let target = TestTarget::default(); - let metric = TestMetric { id: Uuid::new_v4(), good: true, datum: 1 }; + let metric = TestMetric { id: ID, good: true, datum: 1 }; Sample::new(&target, &metric).unwrap() } +pub fn make_missing_sample() -> Sample { + let target = TestTarget::default(); + let metric = TestMetric { id: ID, good: true, datum: 1 }; + Sample::new_missing(&target, &metric).unwrap() +} + pub fn make_hist_sample() -> Sample { let target = TestTarget::default(); let mut hist = histogram::Histogram::new(&[0.0, 5.0, 10.0]).unwrap(); hist.sample(1.0).unwrap(); hist.sample(2.0).unwrap(); hist.sample(6.0).unwrap(); - let metric = TestHistogram { id: Uuid::new_v4(), good: true, datum: hist }; + let metric = TestHistogram { id: ID, good: true, datum: hist }; Sample::new(&target, &metric).unwrap() } diff --git a/oximeter/oximeter/src/traits.rs b/oximeter/oximeter/src/traits.rs index 096abb8023..0934d231e3 100644 --- a/oximeter/oximeter/src/traits.rs +++ b/oximeter/oximeter/src/traits.rs @@ -30,8 +30,15 @@ use std::ops::AddAssign; /// definition can be thought of as a schema, and an instance of that struct as identifying an /// individual target. /// -/// Target fields may have one of a set of supported types: `bool`, `i64`, `String`, `IpAddr`, or -/// `Uuid`. Any number of fields greater than zero is supported. +/// Target fields may have one of a set of supported types: +/// +/// - `bool` +/// - any fixed-width integer, e.g., `u8` or `i64` +/// - `String` +/// - `IpAddr` +/// - `Uuid` +/// +/// Any number of fields greater than zero is supported. /// /// Examples /// -------- @@ -105,9 +112,28 @@ pub trait Target { /// One field of the struct is special, describing the actual measured data that the metric /// represents. This should be a field named `datum`, or another field (with any name you choose) /// annotated with the `#[datum]` attribute. This field represents the underlying data for the -/// metric, and must be one of the supported types, implementing the [`Datum`] trait. This can -/// be any of: `i64`, `f64`, `bool`, `String`, or `Bytes` for gauges, and `Cumulative` or -/// `Histogram` for cumulative metrics, where `T` is `i64` or `f64`. +/// metric, and must be one of the supported types, implementing the [`Datum`] trait. +/// +/// For gauge types, this can be any of: +/// +/// - `bool` +/// - a fixed-width integer, e.g. `u8` or `i64` +/// - `f32` or `f64` +/// - `String` +/// - `Bytes` +/// +/// Cumulative types can be any of `Cumulative`, where `T` is +/// +/// - `i64` +/// - `u64` +/// - `f32` +/// - `f64` +/// +/// Histogram types can be any `Histogram`, wher `T` is: +/// +/// - a fixed-width integer, e.g. `u8` or `i64` +/// - `f32` +/// - `f64` /// /// The value of the metric's data is _measured_ by using the `measure()` method, which returns a /// [`Measurement`]. This describes a timestamped data point for the metric. diff --git a/oximeter/oximeter/src/types.rs b/oximeter/oximeter/src/types.rs index 325974781e..23dbe2be6b 100644 --- a/oximeter/oximeter/src/types.rs +++ b/oximeter/oximeter/src/types.rs @@ -369,6 +369,7 @@ pub enum Datum { HistogramU64(histogram::Histogram), HistogramF32(histogram::Histogram), HistogramF64(histogram::Histogram), + Missing(MissingDatum), } impl Datum { @@ -402,6 +403,7 @@ impl Datum { Datum::HistogramU64(_) => DatumType::HistogramU64, Datum::HistogramF32(_) => DatumType::HistogramF32, Datum::HistogramF64(_) => DatumType::HistogramF64, + Datum::Missing(ref inner) => inner.datum_type(), } } @@ -440,6 +442,7 @@ impl Datum { Datum::HistogramU64(ref inner) => Some(inner.start_time()), Datum::HistogramF32(ref inner) => Some(inner.start_time()), Datum::HistogramF64(ref inner) => Some(inner.start_time()), + Datum::Missing(ref inner) => inner.start_time(), } } } @@ -495,6 +498,60 @@ impl From<&str> for Datum { } } +#[derive(Clone, Copy, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +pub struct MissingDatum { + datum_type: DatumType, + start_time: Option>, +} + +impl MissingDatum { + pub fn datum_type(&self) -> DatumType { + self.datum_type + } + + pub fn start_time(&self) -> Option> { + self.start_time + } + + pub fn new( + datum_type: DatumType, + start_time: Option>, + ) -> Result { + // See https://github.com/oxidecomputer/omicron/issues/4551. + if datum_type == DatumType::Bytes { + return Err(MetricsError::DatumError(String::from( + "Missing samples from byte array types are not supported", + ))); + } + if datum_type.is_cumulative() && start_time.is_none() { + return Err(MetricsError::MissingDatumRequiresStartTime { + datum_type, + }); + } + if !datum_type.is_cumulative() && start_time.is_some() { + return Err(MetricsError::MissingDatumCannotHaveStartTime { + datum_type, + }); + } + Ok(Self { datum_type, start_time }) + } +} + +impl From for Datum { + fn from(d: MissingDatum) -> Datum { + Datum::Missing(d) + } +} + +impl From<&M> for MissingDatum { + fn from(metric: &M) -> Self { + MissingDatum { + datum_type: metric.datum_type(), + start_time: metric.start_time(), + } + } +} + /// A `Measurement` is a timestamped datum from a single metric #[derive(Clone, Debug, PartialEq, JsonSchema, Serialize, Deserialize)] pub struct Measurement { @@ -516,6 +573,11 @@ impl Measurement { Self { timestamp, datum: datum.into() } } + /// Return true if this measurement represents a missing datum. + pub fn is_missing(&self) -> bool { + matches!(self.datum, Datum::Missing(_)) + } + /// Return the datum for this measurement pub fn datum(&self) -> &Datum { &self.datum @@ -561,6 +623,12 @@ pub enum MetricsError { /// A field name is duplicated between the target and metric. #[error("Field '{name}' is duplicated between the target and metric")] DuplicateFieldName { name: String }, + + #[error("Missing datum of type {datum_type} requires a start time")] + MissingDatumRequiresStartTime { datum_type: DatumType }, + + #[error("Missing datum of type {datum_type} cannot have a start time")] + MissingDatumCannotHaveStartTime { datum_type: DatumType }, } impl From for omicron_common::api::external::Error { @@ -734,6 +802,29 @@ impl Sample { }) } + /// Construct a new missing sample, recorded at the time of the supplied + /// timestamp. + pub fn new_missing_with_timestamp( + timestamp: DateTime, + target: &T, + metric: &M, + ) -> Result + where + T: traits::Target, + M: traits::Metric, + { + let target_fields = FieldSet::from_target(target); + let metric_fields = FieldSet::from_metric(metric); + Self::verify_field_names(&target_fields, &metric_fields)?; + let datum = Datum::Missing(MissingDatum::from(metric)); + Ok(Self { + timeseries_name: crate::timeseries_name(target, metric), + target: target_fields, + metric: metric_fields, + measurement: Measurement { timestamp, datum }, + }) + } + /// Construct a new sample, created at the time the function is called. /// /// This materializes the data from the target and metric, and stores that information along @@ -746,6 +837,18 @@ impl Sample { Self::new_with_timestamp(Utc::now(), target, metric) } + /// Construct a new sample with a missing measurement. + pub fn new_missing( + target: &T, + metric: &M, + ) -> Result + where + T: traits::Target, + M: traits::Metric, + { + Self::new_missing_with_timestamp(Utc::now(), target, metric) + } + /// Return the fields for this sample. /// /// This returns the target fields and metric fields, chained, although there is no distinction @@ -951,7 +1054,7 @@ mod tests { fn test_measurement() { let measurement = Measurement::new(chrono::Utc::now(), 0i64); assert_eq!(measurement.datum_type(), DatumType::I64); - assert_eq!(measurement.start_time(), None); + assert!(measurement.start_time().is_none()); let datum = Cumulative::new(0i64); let measurement = Measurement::new(chrono::Utc::now(), datum); diff --git a/schema/crdb/18.0.0/up01.sql b/schema/crdb/18.0.0/up01.sql index 6cfa92f4c2..018bb36dcb 100644 --- a/schema/crdb/18.0.0/up01.sql +++ b/schema/crdb/18.0.0/up01.sql @@ -1 +1,4 @@ -ALTER TABLE omicron.public.external_ip ADD COLUMN IF NOT EXISTS project_id UUID; +CREATE UNIQUE INDEX IF NOT EXISTS lookup_deleted_disk ON omicron.public.disk ( + id +) WHERE + time_deleted IS NOT NULL; diff --git a/schema/crdb/19.0.0/up01.sql b/schema/crdb/19.0.0/up01.sql new file mode 100644 index 0000000000..6cfa92f4c2 --- /dev/null +++ b/schema/crdb/19.0.0/up01.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.external_ip ADD COLUMN IF NOT EXISTS project_id UUID; diff --git a/schema/crdb/18.0.0/up02.sql b/schema/crdb/19.0.0/up02.sql similarity index 100% rename from schema/crdb/18.0.0/up02.sql rename to schema/crdb/19.0.0/up02.sql diff --git a/schema/crdb/18.0.0/up03.sql b/schema/crdb/19.0.0/up03.sql similarity index 100% rename from schema/crdb/18.0.0/up03.sql rename to schema/crdb/19.0.0/up03.sql diff --git a/schema/crdb/18.0.0/up04.sql b/schema/crdb/19.0.0/up04.sql similarity index 100% rename from schema/crdb/18.0.0/up04.sql rename to schema/crdb/19.0.0/up04.sql diff --git a/schema/crdb/18.0.0/up05.sql b/schema/crdb/19.0.0/up05.sql similarity index 100% rename from schema/crdb/18.0.0/up05.sql rename to schema/crdb/19.0.0/up05.sql diff --git a/schema/crdb/18.0.0/up06.sql b/schema/crdb/19.0.0/up06.sql similarity index 100% rename from schema/crdb/18.0.0/up06.sql rename to schema/crdb/19.0.0/up06.sql diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index eaeaed4d1d..0bf365a2f1 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1026,6 +1026,11 @@ CREATE UNIQUE INDEX IF NOT EXISTS lookup_disk_by_instance ON omicron.public.disk ) WHERE time_deleted IS NULL AND attach_instance_id IS NOT NULL; +CREATE UNIQUE INDEX IF NOT EXISTS lookup_deleted_disk ON omicron.public.disk ( + id +) WHERE + time_deleted IS NOT NULL; + CREATE TABLE IF NOT EXISTS omicron.public.image ( /* Identity metadata (resource) */ id UUID PRIMARY KEY, @@ -3057,7 +3062,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '18.0.0', NULL) + ( TRUE, NOW(), NOW(), '19.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index bd21d42109..fb6de8d38a 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -101,13 +101,11 @@ use sled_storage::manager::StorageHandle; use slog::Logger; use std::collections::BTreeMap; use std::collections::HashSet; -use std::iter; use std::iter::FromIterator; use std::net::{IpAddr, Ipv6Addr, SocketAddr}; use std::str::FromStr; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use std::time::{SystemTime, UNIX_EPOCH}; use tokio::io::AsyncWriteExt; use tokio::sync::Mutex; use tokio::sync::{oneshot, MutexGuard}; @@ -2933,10 +2931,7 @@ impl ServiceManager { Ok(()) } - pub fn boottime_rewrite<'a>( - &self, - zones: impl Iterator, - ) { + pub fn boottime_rewrite(&self) { if self .inner .time_synced @@ -2947,33 +2942,13 @@ impl ServiceManager { return; } - let now = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("SystemTime before UNIX EPOCH"); - - info!(self.inner.log, "Setting boot time to {:?}", now); - - let files: Vec = zones - .map(|z| z.root()) - .chain(iter::once(Utf8PathBuf::from("/"))) - .flat_map(|r| [r.join("var/adm/utmpx"), r.join("var/adm/wtmpx")]) - .collect(); - - for file in files { - let mut command = std::process::Command::new(PFEXEC); - let cmd = command.args(&[ - "/usr/platform/oxide/bin/tmpx", - &format!("{}", now.as_secs()), - &file.as_str(), - ]); - match execute(cmd) { - Err(e) => { - warn!(self.inner.log, "Updating {} failed: {}", &file, e); - } - Ok(_) => { - info!(self.inner.log, "Updated {}", &file); - } - } + // Call out to the 'tmpx' utility program which will rewrite the wtmpx + // and utmpx databases in every zone, including the global zone, to + // reflect the adjusted system boot time. + let mut command = std::process::Command::new(PFEXEC); + let cmd = command.args(&["/usr/platform/oxide/bin/tmpx", "-Z"]); + if let Err(e) = execute(cmd) { + warn!(self.inner.log, "Updating [wu]tmpx databases failed: {}", e); } } @@ -2982,7 +2957,7 @@ impl ServiceManager { if let Some(true) = self.inner.skip_timesync { info!(self.inner.log, "Configured to skip timesync checks"); - self.boottime_rewrite(existing_zones.values()); + self.boottime_rewrite(); return Ok(TimeSync { sync: true, ref_id: 0, @@ -3036,7 +3011,7 @@ impl ServiceManager { && correction.abs() <= 0.05; if sync { - self.boottime_rewrite(existing_zones.values()); + self.boottime_rewrite(); } Ok(TimeSync { diff --git a/sled-agent/src/sim/http_entrypoints_pantry.rs b/sled-agent/src/sim/http_entrypoints_pantry.rs index 64b26a83a4..8430dc0731 100644 --- a/sled-agent/src/sim/http_entrypoints_pantry.rs +++ b/sled-agent/src/sim/http_entrypoints_pantry.rs @@ -96,7 +96,7 @@ struct JobPollResponse { /// Poll to see if a Pantry background job is done #[endpoint { method = GET, - path = "/crucible/pantry/0/job/{id}/is_finished", + path = "/crucible/pantry/0/job/{id}/is-finished", }] async fn is_job_finished( rc: RequestContext>, @@ -139,6 +139,7 @@ async fn job_result_ok( } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] pub enum ExpectedDigest { Sha256(String), } @@ -157,7 +158,7 @@ struct ImportFromUrlResponse { /// Import data from a URL into a volume #[endpoint { method = POST, - path = "/crucible/pantry/0/volume/{id}/import_from_url", + path = "/crucible/pantry/0/volume/{id}/import-from-url", }] async fn import_from_url( rc: RequestContext>, @@ -213,7 +214,7 @@ struct BulkWriteRequest { /// Bulk write data into a volume at a specified offset #[endpoint { method = POST, - path = "/crucible/pantry/0/volume/{id}/bulk_write", + path = "/crucible/pantry/0/volume/{id}/bulk-write", }] async fn bulk_write( rc: RequestContext>, diff --git a/sled-agent/src/sim/storage.rs b/sled-agent/src/sim/storage.rs index 2528a258d7..101228934d 100644 --- a/sled-agent/src/sim/storage.rs +++ b/sled-agent/src/sim/storage.rs @@ -40,6 +40,7 @@ struct CrucibleDataInner { running_snapshots: HashMap>, on_create: Option, region_creation_error: bool, + region_deletion_error: bool, creating_a_running_snapshot_should_fail: bool, next_port: u16, } @@ -53,6 +54,7 @@ impl CrucibleDataInner { running_snapshots: HashMap::new(), on_create: None, region_creation_error: false, + region_deletion_error: false, creating_a_running_snapshot_should_fail: false, next_port: crucible_port, } @@ -129,6 +131,10 @@ impl CrucibleDataInner { ); } + if self.region_deletion_error { + bail!("region deletion error!"); + } + let id = Uuid::from_str(&id.0).unwrap(); if let Some(region) = self.regions.get_mut(&id) { if region.state == State::Failed { @@ -229,6 +235,10 @@ impl CrucibleDataInner { self.region_creation_error = value; } + fn set_region_deletion_error(&mut self, value: bool) { + self.region_deletion_error = value; + } + fn create_running_snapshot( &mut self, id: &RegionId, @@ -391,6 +401,10 @@ impl CrucibleData { self.inner.lock().await.set_region_creation_error(value); } + pub async fn set_region_deletion_error(&self, value: bool) { + self.inner.lock().await.set_region_deletion_error(value); + } + pub async fn create_running_snapshot( &self, id: &RegionId, diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index 94c8f5572e..d330f32ab6 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -46,6 +46,7 @@ inventory.period_secs = 600 inventory.nkeep = 3 # Disable inventory collection altogether (for emergencies) inventory.disable = false +phantom_disks.period_secs = 30 [default_region_allocation_strategy] # by default, allocate across 3 distinct sleds diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index fcaa6176a8..cbd4851613 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -46,6 +46,7 @@ inventory.period_secs = 600 inventory.nkeep = 3 # Disable inventory collection altogether (for emergencies) inventory.disable = false +phantom_disks.period_secs = 30 [default_region_allocation_strategy] # by default, allocate without requirement for distinct sleds. diff --git a/tools/install_builder_prerequisites.sh b/tools/install_builder_prerequisites.sh index d3ecd8eaa8..1ce133dff3 100755 --- a/tools/install_builder_prerequisites.sh +++ b/tools/install_builder_prerequisites.sh @@ -131,6 +131,8 @@ function install_packages { "library/libxmlsec1" # "bindgen leverages libclang to preprocess, parse, and type check C and C++ header files." "pkg:/ooce/developer/clang-$CLANGVER" + "system/library/gcc-runtime" + "system/library/g++-runtime" ) # Install/update the set of packages. diff --git a/update-engine/src/display/group_display.rs b/update-engine/src/display/group_display.rs index cfd37aac16..0e04361ce4 100644 --- a/update-engine/src/display/group_display.rs +++ b/update-engine/src/display/group_display.rs @@ -153,7 +153,7 @@ impl GroupDisplay { self.stats.apply_result(result); if result.before != result.after { - slog::info!( + slog::debug!( self.log, "add_event_report caused state transition"; "prefix" => &state.prefix,