From 76b835d5d68657265564f40a4667e8ffc7026091 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 8 Dec 2023 11:50:54 -0800 Subject: [PATCH 01/11] [sled-agent] add preliminary "write boot disk OS" http endpoints (#4633) This PR adds three new endpoints to `sled-agent`: * `POST /boot-disk/{boot_disk}/os/write` to start a new update to one of the two boot disks * `GET /boot-disk/{boot_disk}/os/write/status` to get the status of the most-recently-started update to the specified boot disk * `DELETE /boot-disk/{boot_disk}/os/write/status/{update_id}` to clear the status of a previous update The actual drive-writing-machinery is extracted from `installinator` into `installinator-common`, which is now a new dependency of `sled-agent`. The bulk of the changes in this PR center around being able to run that drive-write from within the context of the pair of dropshot endpoints above, plus a bit of glue for unit testing with "in-memory disks". --- Cargo.lock | 10 + common/src/lib.rs | 2 + common/src/update.rs | 4 +- installinator-common/Cargo.toml | 6 + .../src/block_size_writer.rs | 44 +- installinator-common/src/lib.rs | 4 + installinator-common/src/raw_disk_writer.rs | 123 ++ installinator/src/lib.rs | 1 - installinator/src/write.rs | 67 +- openapi/sled-agent.json | 290 +++ sled-agent/Cargo.toml | 6 + sled-agent/src/boot_disk_os_writer.rs | 1669 +++++++++++++++++ sled-agent/src/config.rs | 6 + sled-agent/src/http_entrypoints.rs | 172 +- sled-agent/src/lib.rs | 1 + sled-agent/src/server.rs | 7 +- sled-agent/src/sled_agent.rs | 13 + smf/sled-agent/gimlet-standalone/config.toml | 5 + smf/sled-agent/gimlet/config.toml | 5 + smf/sled-agent/non-gimlet/config.toml | 5 + 20 files changed, 2374 insertions(+), 66 deletions(-) rename {installinator => installinator-common}/src/block_size_writer.rs (81%) create mode 100644 installinator-common/src/raw_disk_writer.rs create mode 100644 sled-agent/src/boot_disk_os_writer.rs diff --git a/Cargo.lock b/Cargo.lock index ed988f4b14..71cca52057 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3314,12 +3314,16 @@ dependencies = [ "anyhow", "camino", "illumos-utils", + "libc", "omicron-workspace-hack", + "proptest", "schemars", "serde", "serde_json", "serde_with", + "test-strategy", "thiserror", + "tokio", "update-engine", ] @@ -4867,6 +4871,7 @@ dependencies = [ "crucible-agent-client", "ddm-admin-client", "derive_more", + "display-error-chain", "dns-server", "dns-service-client", "dpd-client", @@ -4876,10 +4881,12 @@ dependencies = [ "futures", "gateway-client", "glob", + "hex", "http", "hyper", "hyper-staticfile", "illumos-utils", + "installinator-common", "internal-dns", "ipnetwork", "itertools 0.12.0", @@ -4907,6 +4914,7 @@ dependencies = [ "schemars", "semver 1.0.20", "serde", + "serde_human_bytes", "serde_json", "serial_test", "sha3", @@ -4925,6 +4933,8 @@ dependencies = [ "thiserror", "tofino", "tokio", + "tokio-stream", + "tokio-util", "toml 0.8.8", "usdt", "uuid", diff --git a/common/src/lib.rs b/common/src/lib.rs index 1d2ed0afdb..0d63de90fb 100644 --- a/common/src/lib.rs +++ b/common/src/lib.rs @@ -31,6 +31,8 @@ pub mod postgres_config; pub mod update; pub mod vlan; +pub use update::hex_schema; + #[macro_export] macro_rules! generate_logging_api { ($path:literal) => { diff --git a/common/src/update.rs b/common/src/update.rs index 81256eb526..28d5ae50a6 100644 --- a/common/src/update.rs +++ b/common/src/update.rs @@ -296,7 +296,9 @@ impl FromStr for ArtifactHash { } } -fn hex_schema(gen: &mut SchemaGenerator) -> Schema { +/// Produce an OpenAPI schema describing a hex array of a specific length (e.g., +/// a hash digest). +pub fn hex_schema(gen: &mut SchemaGenerator) -> Schema { let mut schema: SchemaObject = ::json_schema(gen).into(); schema.format = Some(format!("hex string ({N} bytes)")); schema.into() diff --git a/installinator-common/Cargo.toml b/installinator-common/Cargo.toml index 4381de74eb..dd8540c6f8 100644 --- a/installinator-common/Cargo.toml +++ b/installinator-common/Cargo.toml @@ -8,10 +8,16 @@ license = "MPL-2.0" anyhow.workspace = true camino.workspace = true illumos-utils.workspace = true +libc.workspace = true schemars.workspace = true serde.workspace = true serde_json.workspace = true serde_with.workspace = true thiserror.workspace = true +tokio.workspace = true update-engine.workspace = true omicron-workspace-hack.workspace = true + +[dev-dependencies] +proptest.workspace = true +test-strategy.workspace = true diff --git a/installinator/src/block_size_writer.rs b/installinator-common/src/block_size_writer.rs similarity index 81% rename from installinator/src/block_size_writer.rs rename to installinator-common/src/block_size_writer.rs index 3f41a4ee99..1548594b41 100644 --- a/installinator/src/block_size_writer.rs +++ b/installinator-common/src/block_size_writer.rs @@ -11,31 +11,37 @@ use tokio::io::AsyncWrite; /// `BlockSizeBufWriter` is analogous to a tokio's `BufWriter`, except it /// guarantees that writes made to the underlying writer are always -/// _exactly_ the requested block size, with two exceptions: explicitly -/// calling (1) `flush()` or (2) `shutdown()` will write any -/// buffered-but-not-yet-written data to the underlying buffer regardless of -/// its length. +/// _exactly_ the requested block size, with three exceptions: +/// +/// 1. Calling `flush()` will write any currently-buffered data to the +/// underlying writer, regardless of its length. +/// 2. Similarily, calling `shutdown()` will flush any currently-buffered data +/// to the underlying writer. +/// 3. When `BlockSizeBufWriter` attempts to write a block-length amount of data +/// to the underlying writer, if that writer only accepts a portion of that +/// data, `BlockSizeBufWriter` will continue attempting to write the +/// remainder of the block. /// /// When `BlockSizeBufWriter` is dropped, any buffered data it's holding /// will be discarded. It is critical to manually call /// `BlockSizeBufWriter:flush()` or `BlockSizeBufWriter::shutdown()` prior /// to dropping to avoid data loss. -pub(crate) struct BlockSizeBufWriter { +pub struct BlockSizeBufWriter { inner: W, buf: Vec, block_size: usize, } impl BlockSizeBufWriter { - pub(crate) fn with_block_size(block_size: usize, inner: W) -> Self { + pub fn with_block_size(block_size: usize, inner: W) -> Self { Self { inner, buf: Vec::with_capacity(block_size), block_size } } - pub(crate) fn into_inner(self) -> W { + pub fn into_inner(self) -> W { self.inner } - pub(crate) fn block_size(&self) -> usize { + pub fn block_size(&self) -> usize { self.block_size } @@ -46,6 +52,13 @@ impl BlockSizeBufWriter { fn flush_buf(&mut self, cx: &mut Context<'_>) -> Poll> { let mut written = 0; let mut ret = Ok(()); + + // We expect this loop to execute exactly one time: we try to write the + // entirety of `self.buf` to `self.inner`, and presumably it is a type + // that expects to receive a block of data at once, so we'll immediately + // jump to `written == self.buf.len()`. If it returns `Ok(n)` for some + // `n < self.buf.len()`, we'll loop and try to write the rest of the + // data in less-than-block-sized chunks. while written < self.buf.len() { match ready!( Pin::new(&mut self.inner).poll_write(cx, &self.buf[written..]) @@ -128,8 +141,8 @@ impl AsyncWrite for BlockSizeBufWriter { #[cfg(test)] mod tests { use super::*; - use crate::test_helpers::with_test_runtime; use anyhow::Result; + use std::future::Future; use test_strategy::proptest; use tokio::io::AsyncWriteExt; @@ -167,6 +180,19 @@ mod tests { } } + fn with_test_runtime(f: F) -> T + where + F: FnOnce() -> Fut, + Fut: Future, + { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_time() + .start_paused(true) + .build() + .expect("tokio Runtime built successfully"); + runtime.block_on(f()) + } + #[proptest] fn proptest_block_writer( chunks: Vec>, diff --git a/installinator-common/src/lib.rs b/installinator-common/src/lib.rs index b77385840f..4771de7b27 100644 --- a/installinator-common/src/lib.rs +++ b/installinator-common/src/lib.rs @@ -4,6 +4,10 @@ //! Common types shared by the installinator client and server. +mod block_size_writer; mod progress; +mod raw_disk_writer; +pub use block_size_writer::*; pub use progress::*; +pub use raw_disk_writer::*; diff --git a/installinator-common/src/raw_disk_writer.rs b/installinator-common/src/raw_disk_writer.rs new file mode 100644 index 0000000000..35d3862e67 --- /dev/null +++ b/installinator-common/src/raw_disk_writer.rs @@ -0,0 +1,123 @@ +// 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/. + +//! Async writer for raw disks on illumos (e.g., host OS phase 2 images written +//! to M.2 drives). + +use crate::BlockSizeBufWriter; +use illumos_utils::dkio; +use illumos_utils::dkio::MediaInfoExtended; +use std::io; +use std::os::fd::AsRawFd; +use std::path::Path; +use std::pin::Pin; +use std::task::Context; +use std::task::Poll; +use tokio::fs::File; +use tokio::io::AsyncWrite; +use tokio::io::AsyncWriteExt; + +/// Writer for illumos raw disks. +/// +/// Construct an instance via [`RawDiskWriter::open()`], write to it just like +/// any other async writer (it will handle passing writes down to the device in +/// chunks of length [`RawDiskWriter::block_size()`]), and then call +/// [`RawDiskWriter::finalize()`]. It is **critical** to call `finalize()`; +/// failure to do so will likely lead to data loss. +/// +/// `RawDiskWriter` attempts to be as conservative as it can about ensuring data +/// is written: +/// +/// * The device is opened with `O_SYNC` +/// * In `finalize()`, the file is `fsync`'d after any remaining data is flushed +/// * In `finalize()`, the disk write cache is flushed (if supported by the +/// target device) +/// +/// Writing an amount of data that is not a multiple of the device's +/// `block_size()` will likely result in a failure when writing / flushing the +/// final not-correctly-sized chunk. +/// +/// This type is illumos-specific due to using dkio for two things: +/// +/// 1. Determining the logical block size of the device +/// 2. Flushing the disk write cache +pub struct RawDiskWriter { + inner: BlockSizeBufWriter, +} + +impl RawDiskWriter { + /// Open the disk device at `path` for writing, and attempt to determine its + /// logical block size via [`MediaInfoExtended`]. + pub async fn open(path: &Path) -> io::Result { + let f = tokio::fs::OpenOptions::new() + .create(false) + .write(true) + .truncate(false) + .custom_flags(libc::O_SYNC) + .open(path) + .await?; + + let media_info = MediaInfoExtended::from_fd(f.as_raw_fd())?; + + let inner = BlockSizeBufWriter::with_block_size( + media_info.logical_block_size as usize, + f, + ); + + Ok(Self { inner }) + } + + /// The logical block size of the underlying device. + pub fn block_size(&self) -> usize { + self.inner.block_size() + } + + /// Flush any remaining data and attempt to ensure synchronization with the + /// device. + pub async fn finalize(mut self) -> io::Result<()> { + // Flush any remaining data in our buffer + self.inner.flush().await?; + + // `fsync` the file... + let f = self.inner.into_inner(); + f.sync_all().await?; + + // ...and also attempt to flush the disk write cache + tokio::task::spawn_blocking(move || { + match dkio::flush_write_cache(f.as_raw_fd()) { + Ok(()) => Ok(()), + // Some drives don't support `flush_write_cache`; we don't want + // to fail in this case. + Err(err) if err.raw_os_error() == Some(libc::ENOTSUP) => Ok(()), + Err(err) => Err(err), + } + }) + .await + .expect("task panicked") + } +} + +impl AsyncWrite for RawDiskWriter { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + Pin::new(&mut self.inner).poll_write(cx, buf) + } + + fn poll_flush( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + ) -> Poll> { + Pin::new(&mut self.inner).poll_flush(cx) + } + + fn poll_shutdown( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + ) -> Poll> { + Pin::new(&mut self.inner).poll_shutdown(cx) + } +} diff --git a/installinator/src/lib.rs b/installinator/src/lib.rs index c7de189576..3b1d768a7d 100644 --- a/installinator/src/lib.rs +++ b/installinator/src/lib.rs @@ -4,7 +4,6 @@ mod artifact; mod async_temp_file; -mod block_size_writer; mod bootstrap; mod dispatch; mod errors; diff --git a/installinator/src/write.rs b/installinator/src/write.rs index 22dd2adbf6..380595b4cd 100644 --- a/installinator/src/write.rs +++ b/installinator/src/write.rs @@ -6,7 +6,6 @@ use std::{ collections::{btree_map::Entry, BTreeMap, BTreeSet}, fmt, io::{self, Read}, - os::fd::AsRawFd, time::Duration, }; @@ -15,14 +14,11 @@ use async_trait::async_trait; use buf_list::BufList; use bytes::Buf; use camino::{Utf8Path, Utf8PathBuf}; -use illumos_utils::{ - dkio::{self, MediaInfoExtended}, - zpool::{Zpool, ZpoolName}, -}; +use illumos_utils::zpool::{Zpool, ZpoolName}; use installinator_common::{ - ControlPlaneZonesSpec, ControlPlaneZonesStepId, M2Slot, StepContext, - StepProgress, StepResult, StepSuccess, UpdateEngine, WriteComponent, - WriteError, WriteOutput, WriteSpec, WriteStepId, + ControlPlaneZonesSpec, ControlPlaneZonesStepId, M2Slot, RawDiskWriter, + StepContext, StepProgress, StepResult, StepSuccess, UpdateEngine, + WriteComponent, WriteError, WriteOutput, WriteSpec, WriteStepId, }; use omicron_common::update::{ArtifactHash, ArtifactHashId}; use sha2::{Digest, Sha256}; @@ -36,10 +32,7 @@ use update_engine::{ errors::NestedEngineError, events::ProgressUnits, StepSpec, }; -use crate::{ - async_temp_file::AsyncNamedTempFile, block_size_writer::BlockSizeBufWriter, - hardware::Hardware, -}; +use crate::{async_temp_file::AsyncNamedTempFile, hardware::Hardware}; #[derive(Clone, Debug)] struct ArtifactDestination { @@ -754,28 +747,13 @@ impl WriteTransportWriter for AsyncNamedTempFile { } #[async_trait] -impl WriteTransportWriter for BlockSizeBufWriter { +impl WriteTransportWriter for RawDiskWriter { fn block_size(&self) -> Option { - Some(BlockSizeBufWriter::block_size(self)) + Some(RawDiskWriter::block_size(self)) } async fn finalize(self) -> io::Result<()> { - let f = self.into_inner(); - f.sync_all().await?; - - // We only create `BlockSizeBufWriter` for the raw block device storing - // the OS ramdisk. After `fsync`'ing, also flush the write cache. - tokio::task::spawn_blocking(move || { - match dkio::flush_write_cache(f.as_raw_fd()) { - Ok(()) => Ok(()), - // Some drives don't support `flush_write_cache`; we don't want - // to fail in this case. - Err(err) if err.raw_os_error() == Some(libc::ENOTSUP) => Ok(()), - Err(err) => Err(err), - } - }) - .await - .unwrap() + RawDiskWriter::finalize(self).await } } @@ -810,7 +788,7 @@ struct BlockDeviceTransport; #[async_trait] impl WriteTransport for BlockDeviceTransport { - type W = BlockSizeBufWriter; + type W = RawDiskWriter; async fn make_writer( &mut self, @@ -819,12 +797,7 @@ impl WriteTransport for BlockDeviceTransport { destination: &Utf8Path, total_bytes: u64, ) -> Result { - let f = tokio::fs::OpenOptions::new() - .create(false) - .write(true) - .truncate(false) - .custom_flags(libc::O_SYNC) - .open(destination) + let writer = RawDiskWriter::open(destination.as_std_path()) .await .map_err(|error| WriteError::WriteError { component, @@ -834,18 +807,7 @@ impl WriteTransport for BlockDeviceTransport { error, })?; - let media_info = - MediaInfoExtended::from_fd(f.as_raw_fd()).map_err(|error| { - WriteError::WriteError { - component, - slot, - written_bytes: 0, - total_bytes, - error, - } - })?; - - let block_size = u64::from(media_info.logical_block_size); + let block_size = writer.block_size() as u64; // When writing to a block device, we must write a multiple of the block // size. We can assume the image we're given should be @@ -858,12 +820,15 @@ impl WriteTransport for BlockDeviceTransport { total_bytes, error: io::Error::new( io::ErrorKind::InvalidData, - format!("file size ({total_bytes}) is not a multiple of target device block size ({block_size})") + format!( + "file size ({total_bytes}) is not a multiple of \ + target device block size ({block_size})" + ), ), }); } - Ok(BlockSizeBufWriter::with_block_size(block_size as usize, f)) + Ok(writer) } } diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index 3a88b6cc9c..f809cfa57b 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -10,6 +10,132 @@ "version": "0.0.1" }, "paths": { + "/boot-disk/{boot_disk}/os/write": { + "post": { + "summary": "Write a new host OS image to the specified boot disk", + "operationId": "host_os_write_start", + "parameters": [ + { + "in": "path", + "name": "boot_disk", + "required": true, + "schema": { + "$ref": "#/components/schemas/M2Slot" + } + }, + { + "in": "query", + "name": "sha3_256_digest", + "required": true, + "schema": { + "type": "string", + "format": "hex string (32 bytes)" + } + }, + { + "in": "query", + "name": "update_id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "requestBody": { + "content": { + "application/octet-stream": { + "schema": { + "type": "string", + "format": "binary" + } + } + }, + "required": true + }, + "responses": { + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/boot-disk/{boot_disk}/os/write/status": { + "get": { + "summary": "Get the status of writing a new host OS", + "operationId": "host_os_write_status_get", + "parameters": [ + { + "in": "path", + "name": "boot_disk", + "required": true, + "schema": { + "$ref": "#/components/schemas/M2Slot" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BootDiskOsWriteStatus" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/boot-disk/{boot_disk}/os/write/status/{update_id}": { + "delete": { + "summary": "Clear the status of a completed write of a new host OS", + "operationId": "host_os_write_status_delete", + "parameters": [ + { + "in": "path", + "name": "boot_disk", + "required": true, + "schema": { + "$ref": "#/components/schemas/M2Slot" + } + }, + { + "in": "path", + "name": "update_id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "responses": { + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/cockroachdb": { "post": { "summary": "Initializes a CockroachDB cluster", @@ -2135,6 +2261,162 @@ "range" ] }, + "BootDiskOsWriteProgress": { + "description": "Current progress of an OS image being written to disk.", + "oneOf": [ + { + "description": "The image is still being uploaded.", + "type": "object", + "properties": { + "bytes_received": { + "type": "integer", + "format": "uint", + "minimum": 0 + }, + "state": { + "type": "string", + "enum": [ + "receiving_uploaded_image" + ] + } + }, + "required": [ + "bytes_received", + "state" + ] + }, + { + "description": "The image is being written to disk.", + "type": "object", + "properties": { + "bytes_written": { + "type": "integer", + "format": "uint", + "minimum": 0 + }, + "state": { + "type": "string", + "enum": [ + "writing_image_to_disk" + ] + } + }, + "required": [ + "bytes_written", + "state" + ] + }, + { + "description": "The image is being read back from disk for validation.", + "type": "object", + "properties": { + "bytes_read": { + "type": "integer", + "format": "uint", + "minimum": 0 + }, + "state": { + "type": "string", + "enum": [ + "validating_written_image" + ] + } + }, + "required": [ + "bytes_read", + "state" + ] + } + ] + }, + "BootDiskOsWriteStatus": { + "description": "Status of an update to a boot disk OS.", + "oneOf": [ + { + "description": "No update has been started for this disk, or any previously-started update has completed and had its status cleared.", + "type": "object", + "properties": { + "status": { + "type": "string", + "enum": [ + "no_update_started" + ] + } + }, + "required": [ + "status" + ] + }, + { + "description": "An update is currently running.", + "type": "object", + "properties": { + "progress": { + "$ref": "#/components/schemas/BootDiskOsWriteProgress" + }, + "status": { + "type": "string", + "enum": [ + "in_progress" + ] + }, + "update_id": { + "type": "string", + "format": "uuid" + } + }, + "required": [ + "progress", + "status", + "update_id" + ] + }, + { + "description": "The most recent update completed successfully.", + "type": "object", + "properties": { + "status": { + "type": "string", + "enum": [ + "complete" + ] + }, + "update_id": { + "type": "string", + "format": "uuid" + } + }, + "required": [ + "status", + "update_id" + ] + }, + { + "description": "The most recent update failed.", + "type": "object", + "properties": { + "message": { + "type": "string" + }, + "status": { + "type": "string", + "enum": [ + "failed" + ] + }, + "update_id": { + "type": "string", + "format": "uuid" + } + }, + "required": [ + "message", + "status", + "update_id" + ] + } + ] + }, "BundleUtilization": { "description": "The portion of a debug dataset used for zone bundles.", "type": "object", @@ -6485,6 +6767,14 @@ "description": "Zpool names are of the format ox{i,p}_. They are either Internal or External, and should be unique", "type": "string", "pattern": "^ox[ip]_[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" + }, + "M2Slot": { + "description": "An M.2 slot that was written.", + "type": "string", + "enum": [ + "A", + "B" + ] } }, "responses": { diff --git a/sled-agent/Cargo.toml b/sled-agent/Cargo.toml index 61e61709e1..7607d57b95 100644 --- a/sled-agent/Cargo.toml +++ b/sled-agent/Cargo.toml @@ -25,14 +25,17 @@ derive_more.workspace = true dns-server.workspace = true dns-service-client.workspace = true dpd-client.workspace = true +display-error-chain.workspace = true dropshot.workspace = true flate2.workspace = true futures.workspace = true glob.workspace = true +hex.workspace = true http.workspace = true hyper-staticfile.workspace = true gateway-client.workspace = true illumos-utils.workspace = true +installinator-common.workspace = true internal-dns.workspace = true ipnetwork.workspace = true itertools.workspace = true @@ -53,6 +56,7 @@ reqwest = { workspace = true, features = ["rustls-tls", "stream"] } schemars = { workspace = true, features = [ "chrono", "uuid1" ] } semver.workspace = true serde.workspace = true +serde_human_bytes.workspace = true serde_json = {workspace = true, features = ["raw_value"]} sha3.workspace = true sled-agent-client.workspace = true @@ -93,6 +97,8 @@ subprocess.workspace = true slog-async.workspace = true slog-term.workspace = true tempfile.workspace = true +tokio-stream.workspace = true +tokio-util.workspace = true illumos-utils = { workspace = true, features = ["testing", "tmp_keypath"] } sled-storage = { workspace = true, features = ["testing"] } diff --git a/sled-agent/src/boot_disk_os_writer.rs b/sled-agent/src/boot_disk_os_writer.rs new file mode 100644 index 0000000000..a0798ed174 --- /dev/null +++ b/sled-agent/src/boot_disk_os_writer.rs @@ -0,0 +1,1669 @@ +// 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/. + +//! This module provides `BootDiskOsWriter`, via which sled-agent can write new +//! OS images to its boot disks. + +use crate::http_entrypoints::BootDiskOsWriteProgress; +use crate::http_entrypoints::BootDiskOsWriteStatus; +use async_trait::async_trait; +use bytes::Bytes; +use camino::Utf8PathBuf; +use display_error_chain::DisplayErrorChain; +use dropshot::HttpError; +use futures::Stream; +use futures::TryStreamExt; +use installinator_common::M2Slot; +use installinator_common::RawDiskWriter; +use sha3::Digest; +use sha3::Sha3_256; +use slog::Logger; +use std::collections::btree_map::Entry; +use std::collections::BTreeMap; +use std::io; +use std::io::Read; +use std::path::Path; +use std::sync::Arc; +use std::sync::Mutex; +use tokio::fs::File; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncSeekExt; +use tokio::io::AsyncWrite; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::sync::oneshot; +use tokio::sync::oneshot::error::TryRecvError; +use tokio::sync::watch; +use uuid::Uuid; + +impl BootDiskOsWriteStatus { + fn from_result( + update_id: Uuid, + result: &Result<(), Arc>, + ) -> Self { + match result { + Ok(()) => Self::Complete { update_id }, + Err(err) => Self::Failed { + update_id, + message: DisplayErrorChain::new(err).to_string(), + }, + } + } +} + +#[derive(Debug, thiserror::Error)] +pub(crate) enum BootDiskOsWriteError { + // This variant should be impossible in production, as we build with + // panic=abort, but may be constructed in tests (e.g., during tokio runtime + // shutdown). + #[error("internal error (task panic)")] + TaskPanic, + #[error("an update is still running ({0})")] + UpdateRunning(Uuid), + #[error("a previous update completed ({0}); clear its status before starting a new update")] + CannotStartWithoutClearingPreviousStatus(Uuid), + #[error("failed to create temporary file")] + FailedCreatingTempfile(#[source] io::Error), + #[error("failed writing to temporary file")] + FailedWritingTempfile(#[source] io::Error), + #[error("failed downloading image from HTTP client")] + FailedDownloadingImage(#[source] HttpError), + #[error("hash mismatch in image from HTTP client: expected {expected} but got {got}")] + UploadedImageHashMismatch { expected: String, got: String }, + #[error("failed to open disk for writing {path}")] + FailedOpenDiskForWrite { + #[source] + error: io::Error, + path: Utf8PathBuf, + }, + #[error("image size ({image_size}) is not a multiple of disk block size ({disk_block_size})")] + ImageSizeNotMultipleOfBlockSize { + image_size: usize, + disk_block_size: usize, + }, + #[error("failed reading from temporary file")] + FailedReadingTempfile(#[source] io::Error), + #[error("failed writing to disk {path}")] + FailedWritingDisk { + #[source] + error: io::Error, + path: Utf8PathBuf, + }, + #[error("failed to open disk for reading {path}")] + FailedOpenDiskForRead { + #[source] + error: io::Error, + path: Utf8PathBuf, + }, + #[error("failed reading from disk {path}")] + FailedReadingDisk { + #[source] + error: io::Error, + path: Utf8PathBuf, + }, + #[error("hash mismatch after writing disk {path}: expected {expected} but got {got}")] + WrittenImageHashMismatch { + path: Utf8PathBuf, + expected: String, + got: String, + }, + #[error("unexpected update ID {0}: cannot clear status")] + WrongUpdateIdClearingStatus(Uuid), +} + +impl From<&BootDiskOsWriteError> for HttpError { + fn from(error: &BootDiskOsWriteError) -> Self { + let message = DisplayErrorChain::new(error).to_string(); + match error { + BootDiskOsWriteError::UpdateRunning(_) + | BootDiskOsWriteError::CannotStartWithoutClearingPreviousStatus( + _, + ) + | BootDiskOsWriteError::FailedDownloadingImage(_) + | BootDiskOsWriteError::UploadedImageHashMismatch { .. } + | BootDiskOsWriteError::ImageSizeNotMultipleOfBlockSize { + .. + } + | BootDiskOsWriteError::WrongUpdateIdClearingStatus(_) => { + HttpError::for_bad_request(None, message) + } + BootDiskOsWriteError::TaskPanic + | BootDiskOsWriteError::FailedCreatingTempfile(_) + | BootDiskOsWriteError::FailedWritingTempfile(_) + | BootDiskOsWriteError::FailedReadingTempfile(_) + | BootDiskOsWriteError::FailedOpenDiskForWrite { .. } + | BootDiskOsWriteError::FailedOpenDiskForRead { .. } + | BootDiskOsWriteError::FailedWritingDisk { .. } + | BootDiskOsWriteError::FailedReadingDisk { .. } + | BootDiskOsWriteError::WrittenImageHashMismatch { .. } => { + HttpError { + status_code: http::StatusCode::SERVICE_UNAVAILABLE, + error_code: None, + external_message: message.clone(), + internal_message: message, + } + } + } + } +} + +// Note to future maintainers: `installinator` uses the `update_engine` crate to +// drive its process (which includes writing the boot disk). We could also use +// `update_engine` inside `BootDiskOsWriter`; instead, we've hand-rolled a state +// machine with manual progress reporting. The current implementation is +// _probably_ simple enough that this was a reasonable choice, but if it becomes +// more complex (or if additional work needs to be done that `update_engine` +// would make easier), consider switching it over. +#[derive(Debug)] +pub(crate) struct BootDiskOsWriter { + // Note: We use a std Mutex here to avoid cancellation issues with tokio + // Mutex. We never need to keep the lock held longer than necessary to copy + // or replace the current writer state. + states: Mutex>, + log: Logger, +} + +impl BootDiskOsWriter { + pub(crate) fn new(log: &Logger) -> Self { + Self { + states: Mutex::default(), + log: log.new(slog::o!("component" => "BootDiskOsWriter")), + } + } + + /// Attempt to start a new update to the given disk (identified by both its + /// slot and the path to its devfs device). + /// + /// This method will return after the `image_upload` stream has been saved + /// to a local temporary file, but before the update has completed. Callers + /// must poll `status()` to discover when the running update completes (or + /// fails). + /// + /// # Errors + /// + /// This method will return an error and not start an update if any of the + /// following are true: + /// + /// * A previously-started update of this same `boot_disk` is still running + /// * A previously-completed update has not had its status cleared + /// * The `image_upload` stream returns an error + /// * The hash of the data provided by `image_upload` does not match + /// `sha3_256_digest` + /// * Any of a variety of I/O errors occurs while copying from + /// `image_upload` to a temporary file + /// + /// In all but the first case, the error returned will also be saved and + /// returned when `status()` is called (until another update is started). + pub(crate) async fn start_update( + &self, + boot_disk: M2Slot, + disk_devfs_path: Utf8PathBuf, + update_id: Uuid, + sha3_256_digest: [u8; 32], + image_upload: S, + ) -> Result<(), Arc> + where + S: Stream> + Send + 'static, + { + self.start_update_impl( + boot_disk, + disk_devfs_path, + update_id, + sha3_256_digest, + image_upload, + RealDiskInterface {}, + ) + .await + } + + async fn start_update_impl( + &self, + boot_disk: M2Slot, + disk_devfs_path: Utf8PathBuf, + update_id: Uuid, + sha3_256_digest: [u8; 32], + image_upload: S, + disk_writer: Writer, + ) -> Result<(), Arc> + where + S: Stream> + Send + 'static, + Writer: DiskInterface + Send + Sync + 'static, + { + // Construct a closure that will spawn a task to drive this update, but + // don't actually start it yet: we only allow an update to start if + // there's not currently an update running targetting the same slot, so + // we'll spawn this after checking that below. + let spawn_update_task = || { + let (uploaded_image_tx, uploaded_image_rx) = oneshot::channel(); + let (progress_tx, progress_rx) = watch::channel( + BootDiskOsWriteProgress::ReceivingUploadedImage { + bytes_received: 0, + }, + ); + let (complete_tx, complete_rx) = oneshot::channel(); + let task = BootDiskOsWriteTask { + log: self + .log + .new(slog::o!("update_id" => update_id.to_string())), + disk_devfs_path, + sha3_256_digest, + progress_tx, + disk_interface: disk_writer, + }; + tokio::spawn(task.run( + image_upload, + uploaded_image_tx, + complete_tx, + )); + ( + uploaded_image_rx, + TaskRunningState { update_id, progress_rx, complete_rx }, + ) + }; + + // Either call `spawn_update_task` and get back the handle to + // `uploaded_image_rx`, or return an error (if another update for this + // boot disk is still running). + let uploaded_image_rx = { + let mut states = self.states.lock().unwrap(); + match states.entry(boot_disk) { + Entry::Vacant(slot) => { + let (uploaded_image_rx, running) = spawn_update_task(); + slot.insert(WriterState::TaskRunning(running)); + uploaded_image_rx + } + Entry::Occupied(mut slot) => match slot.get_mut() { + WriterState::TaskRunning(running) => { + // It's possible this task is actually complete and a + // result is sitting in the `running.complete_rx` + // oneshot, but for the purposes of starting a new + // update it doesn't matter either way: we'll refuse to + // start. Return the "another update running" error; the + // caller will have to check the `status()`, which will + // trigger a "see if it's actually done after all" + // check. + return Err(Arc::new( + BootDiskOsWriteError::UpdateRunning( + running.update_id, + ), + )); + } + WriterState::Complete(complete) => { + return Err(Arc::new( + BootDiskOsWriteError::CannotStartWithoutClearingPreviousStatus( + complete.update_id, + ))); + } + }, + } + }; + + // We've now spawned a task to drive the update, and we want to wait for + // it to finish copying from `image_upload`. + uploaded_image_rx.await.map_err(|_| BootDiskOsWriteError::TaskPanic)? + } + + /// Clear the status of a finished or failed update with the given ID + /// targetting `boot_disk`. + /// + /// If no update has ever been started for this `boot_disk`, returns + /// `Ok(())`. + /// + /// # Errors + /// + /// Fails if an update to `boot_disk` is currently running; only terminal + /// statuses can be cleared. Fails if the most recent terminal status + /// targetting `boot_disk` had a different update ID. + pub(crate) fn clear_terminal_status( + &self, + boot_disk: M2Slot, + update_id: Uuid, + ) -> Result<(), BootDiskOsWriteError> { + let mut states = self.states.lock().unwrap(); + let mut slot = match states.entry(boot_disk) { + // No status; nothing to clear. + Entry::Vacant(_slot) => return Ok(()), + Entry::Occupied(slot) => slot, + }; + + match slot.get_mut() { + WriterState::Complete(complete) => { + if complete.update_id == update_id { + slot.remove(); + Ok(()) + } else { + Err(BootDiskOsWriteError::WrongUpdateIdClearingStatus( + complete.update_id, + )) + } + } + WriterState::TaskRunning(running) => { + // Check whether the task is _actually_ still running, + // or whether it's done and just waiting for us to + // realize it. + match running.complete_rx.try_recv() { + Ok(result) => { + if running.update_id == update_id { + // This is a little surprising but legal: we've been + // asked to clear the terminal status of this + // update_id, even though we just realized it + // finished. + slot.remove(); + Ok(()) + } else { + let running_update_id = running.update_id; + // A different update just finished; store the + // result we got from the oneshot and don't remove + // the status. + slot.insert(WriterState::Complete( + TaskCompleteState { + update_id: running_update_id, + result, + }, + )); + Err(BootDiskOsWriteError::WrongUpdateIdClearingStatus( + running_update_id + )) + } + } + Err(TryRecvError::Empty) => Err( + BootDiskOsWriteError::UpdateRunning(running.update_id), + ), + Err(TryRecvError::Closed) => { + Err(BootDiskOsWriteError::TaskPanic) + } + } + } + } + } + + /// Get the status of any update running that targets `boot_disk`. + pub(crate) fn status(&self, boot_disk: M2Slot) -> BootDiskOsWriteStatus { + let mut states = self.states.lock().unwrap(); + let mut slot = match states.entry(boot_disk) { + Entry::Vacant(_) => return BootDiskOsWriteStatus::NoUpdateStarted, + Entry::Occupied(slot) => slot, + }; + + match slot.get_mut() { + WriterState::TaskRunning(running) => { + // Is the task actually still running? Check and see if it's + // sent us a result that we just haven't noticed yet. + match running.complete_rx.try_recv() { + Ok(result) => { + let update_id = running.update_id; + let status = BootDiskOsWriteStatus::from_result( + update_id, &result, + ); + slot.insert(WriterState::Complete(TaskCompleteState { + update_id, + result, + })); + status + } + Err(TryRecvError::Empty) => { + let progress = *running.progress_rx.borrow_and_update(); + BootDiskOsWriteStatus::InProgress { + update_id: running.update_id, + progress, + } + } + Err(TryRecvError::Closed) => { + let update_id = running.update_id; + let result = + Err(Arc::new(BootDiskOsWriteError::TaskPanic)); + let status = BootDiskOsWriteStatus::from_result( + update_id, &result, + ); + slot.insert(WriterState::Complete(TaskCompleteState { + update_id, + result, + })); + status + } + } + } + WriterState::Complete(complete) => { + BootDiskOsWriteStatus::from_result( + complete.update_id, + &complete.result, + ) + } + } + } +} + +#[derive(Debug)] +enum WriterState { + /// A task is running to write a new image to a boot disk. + TaskRunning(TaskRunningState), + /// The result of the most recent write. + Complete(TaskCompleteState), +} + +#[derive(Debug)] +struct TaskRunningState { + update_id: Uuid, + progress_rx: watch::Receiver, + complete_rx: oneshot::Receiver>>, +} + +#[derive(Debug)] +struct TaskCompleteState { + update_id: Uuid, + result: Result<(), Arc>, +} + +#[derive(Debug)] +struct BootDiskOsWriteTask { + log: Logger, + sha3_256_digest: [u8; 32], + disk_devfs_path: Utf8PathBuf, + progress_tx: watch::Sender, + disk_interface: W, +} + +impl BootDiskOsWriteTask { + async fn run( + self, + image_upload: S, + uploaded_image_tx: oneshot::Sender< + Result<(), Arc>, + >, + complete_tx: oneshot::Sender>>, + ) where + S: Stream> + Send + 'static, + { + let result = self.run_impl(image_upload, uploaded_image_tx).await; + + // It's possible (albeit unlikely) our caller has discarded the receive + // half of this channel; ignore any send error. + _ = complete_tx.send(result); + } + + async fn run_impl( + self, + image_upload: S, + uploaded_image_tx: oneshot::Sender< + Result<(), Arc>, + >, + ) -> Result<(), Arc> + where + S: Stream> + Send + 'static, + { + // Copy from `image_upload` into a tempfile, then report the result on + // `uploaded_image_tx`. Our dropshot server will not respond to the + // client that requested this update until we finish this step and send + // a response on `uploaded_image_tx`, as `image_upload` is the + // `StreamingBody` attached to their request. + // + // If this step fails, we will send the error to the client who sent the + // request _and_ store a copy of the same error in our current update + // state. + let (image_tempfile, image_size) = match self + .download_body_to_tempfile(image_upload) + .await + .map_err(Arc::new) + { + Ok(tempfile) => { + _ = uploaded_image_tx.send(Ok(())); + tempfile + } + Err(err) => { + _ = uploaded_image_tx.send(Err(Arc::clone(&err))); + return Err(err); + } + }; + + let disk_block_size = self + .copy_tempfile_to_disk(image_tempfile, image_size) + .await + .map_err(Arc::new)?; + + self.validate_written_image(image_size, disk_block_size) + .await + .map_err(Arc::new)?; + + Ok(()) + } + + async fn download_body_to_tempfile( + &self, + image_upload: S, + ) -> Result<(File, usize), BootDiskOsWriteError> + where + S: Stream> + Send + 'static, + { + let tempfile = camino_tempfile::tempfile() + .map_err(BootDiskOsWriteError::FailedCreatingTempfile)?; + + let mut tempfile = + tokio::io::BufWriter::new(tokio::fs::File::from_std(tempfile)); + + let mut image_upload = std::pin::pin!(image_upload); + let mut hasher = Sha3_256::default(); + let mut bytes_received = 0; + + // Stream the uploaded image into our tempfile. + while let Some(bytes) = image_upload + .try_next() + .await + .map_err(BootDiskOsWriteError::FailedDownloadingImage)? + { + hasher.update(&bytes); + tempfile + .write_all(&bytes) + .await + .map_err(BootDiskOsWriteError::FailedWritingTempfile)?; + bytes_received += bytes.len(); + self.progress_tx.send_modify(|progress| { + *progress = BootDiskOsWriteProgress::ReceivingUploadedImage { + bytes_received, + } + }); + } + + // Flush any remaining buffered data. + tempfile + .flush() + .await + .map_err(BootDiskOsWriteError::FailedWritingTempfile)?; + + // Rewind the tempfile. + let mut tempfile = tempfile.into_inner(); + tempfile + .rewind() + .await + .map_err(BootDiskOsWriteError::FailedWritingTempfile)?; + + // Ensure the data the client sent us matches the hash they also sent + // us. A failure here means either the client lied or something has gone + // horribly wrong. + let hash = hasher.finalize(); + let expected_hash_str = hex::encode(&self.sha3_256_digest); + if hash == self.sha3_256_digest.into() { + info!( + self.log, "received uploaded image"; + "bytes_received" => bytes_received, + "hash" => expected_hash_str, + ); + + Ok((tempfile, bytes_received)) + } else { + let computed_hash_str = hex::encode(&hash); + error!( + self.log, "received uploaded image: incorrect hash"; + "bytes_received" => bytes_received, + "computed_hash" => &computed_hash_str, + "expected_hash" => &expected_hash_str, + ); + + Err(BootDiskOsWriteError::UploadedImageHashMismatch { + expected: expected_hash_str, + got: computed_hash_str, + }) + } + } + + /// Copy from `image_tempfile` to the disk device at `self.disk_devfs_path`. + /// Returns the block size of that disk. + async fn copy_tempfile_to_disk( + &self, + image_tempfile: File, + image_size: usize, + ) -> Result { + let mut disk_writer = self + .disk_interface + .open_writer(self.disk_devfs_path.as_std_path()) + .await + .map_err(|error| BootDiskOsWriteError::FailedOpenDiskForWrite { + error, + path: self.disk_devfs_path.clone(), + })?; + + let disk_block_size = disk_writer.block_size(); + + if image_size % disk_block_size != 0 { + return Err( + BootDiskOsWriteError::ImageSizeNotMultipleOfBlockSize { + image_size, + disk_block_size, + }, + ); + } + let num_blocks = image_size / disk_block_size; + + let mut buf = vec![0; disk_block_size]; + let mut image_tempfile = BufReader::new(image_tempfile); + + for block in 0..num_blocks { + image_tempfile + .read_exact(&mut buf) + .await + .map_err(BootDiskOsWriteError::FailedReadingTempfile)?; + + disk_writer.write_all(&buf).await.map_err(|error| { + BootDiskOsWriteError::FailedWritingDisk { + error, + path: self.disk_devfs_path.clone(), + } + })?; + + self.progress_tx.send_modify(|progress| { + *progress = BootDiskOsWriteProgress::WritingImageToDisk { + bytes_written: (block + 1) * buf.len(), + } + }); + } + + disk_writer.finalize().await.map_err(|error| { + BootDiskOsWriteError::FailedWritingDisk { + error, + path: self.disk_devfs_path.clone(), + } + })?; + + info!( + self.log, "copied OS image to disk"; + "path" => %self.disk_devfs_path, + "bytes_written" => image_size, + ); + + Ok(disk_block_size) + } + + async fn validate_written_image( + self, + image_size: usize, + disk_block_size: usize, + ) -> Result<(), BootDiskOsWriteError> { + // We're reading the OS image back from disk and hashing it; this can + // all be synchronous inside a spawn_blocking. + tokio::task::spawn_blocking(move || { + let mut f = self + .disk_interface + .open_reader(self.disk_devfs_path.as_std_path()) + .map_err(|error| { + BootDiskOsWriteError::FailedOpenDiskForRead { + error, + path: self.disk_devfs_path.clone(), + } + })?; + + let mut buf = vec![0; disk_block_size]; + let mut hasher = Sha3_256::default(); + let mut bytes_read = 0; + + while bytes_read < image_size { + // We already confirmed while writing the image that the image + // size is an exact multiple of the disk block size, so we can + // always read a full `buf` here. + f.read_exact(&mut buf).map_err(|error| { + BootDiskOsWriteError::FailedReadingDisk { + error, + path: self.disk_devfs_path.clone(), + } + })?; + + hasher.update(&buf); + bytes_read += buf.len(); + self.progress_tx.send_modify(|progress| { + *progress = + BootDiskOsWriteProgress::ValidatingWrittenImage { + bytes_read, + }; + }); + } + + let expected_hash_str = hex::encode(&self.sha3_256_digest); + let hash = hasher.finalize(); + if hash == self.sha3_256_digest.into() { + info!( + self.log, "validated OS image written to disk"; + "path" => %self.disk_devfs_path, + "hash" => expected_hash_str, + ); + Ok(()) + } else { + let computed_hash_str = hex::encode(&hash); + error!( + self.log, "failed to validate written OS image"; + "bytes_hashed" => image_size, + "computed_hash" => &computed_hash_str, + "expected_hash" => &expected_hash_str, + ); + Err(BootDiskOsWriteError::WrittenImageHashMismatch { + path: self.disk_devfs_path, + expected: expected_hash_str, + got: computed_hash_str, + }) + } + }) + .await + .expect("blocking task panicked") + } +} + +// Utility traits to allow injecting an in-memory "disk" for unit tests. +#[async_trait] +trait DiskWriter: AsyncWrite + Send + Sized + Unpin { + fn block_size(&self) -> usize; + async fn finalize(self) -> io::Result<()>; +} +#[async_trait] +trait DiskInterface: Send + Sync + 'static { + type Writer: DiskWriter; + type Reader: io::Read + Send; + async fn open_writer(&self, path: &Path) -> io::Result; + fn open_reader(&self, path: &Path) -> io::Result; +} + +#[async_trait] +impl DiskWriter for RawDiskWriter { + fn block_size(&self) -> usize { + RawDiskWriter::block_size(self) + } + + async fn finalize(self) -> io::Result<()> { + RawDiskWriter::finalize(self).await + } +} + +struct RealDiskInterface {} + +#[async_trait] +impl DiskInterface for RealDiskInterface { + type Writer = RawDiskWriter; + type Reader = std::fs::File; + + async fn open_writer(&self, path: &Path) -> io::Result { + RawDiskWriter::open(path).await + } + + fn open_reader(&self, path: &Path) -> io::Result { + std::fs::File::open(path) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use futures::future; + use futures::stream; + use installinator_common::BlockSizeBufWriter; + use omicron_test_utils::dev::test_setup_log; + use rand::RngCore; + use std::mem; + use std::pin::Pin; + use std::task::ready; + use std::task::Context; + use std::task::Poll; + use std::time::Duration; + use tokio::sync::mpsc; + use tokio::sync::Semaphore; + use tokio_stream::wrappers::UnboundedReceiverStream; + use tokio_util::sync::PollSemaphore; + + // Most of the tests below end up looping while calling + // `BootDiskOsWriter::status()` waiting for a specific status message to + // arrive. If we get that wrong (or the code under test is wrong!), that + // could end up looping forever, so we run all the relevant bits of the + // tests under a tokio timeout. We expect all the tests to complete very + // quickly in general (< 1 second), so we'll pick something + // outrageously-long-enough that if we hit it, we're almost certainly + // dealing with a hung test. + const TEST_TIMEOUT: Duration = Duration::from_secs(30); + + #[derive(Debug, Clone, PartialEq, Eq)] + struct InMemoryDiskContents { + path: Utf8PathBuf, + data: Vec, + } + + #[derive(Debug, Clone)] + struct InMemoryDiskInterface { + semaphore: Arc, + finalized_writes: Arc>>, + } + + impl InMemoryDiskInterface { + const BLOCK_SIZE: usize = 16; + + fn new(semaphore: Semaphore) -> Self { + Self { + semaphore: Arc::new(semaphore), + finalized_writes: Arc::default(), + } + } + } + + #[async_trait] + impl DiskInterface for InMemoryDiskInterface { + type Writer = InMemoryDiskWriter; + type Reader = io::Cursor>; + + async fn open_writer(&self, path: &Path) -> io::Result { + Ok(InMemoryDiskWriter { + opened_path: path + .to_owned() + .try_into() + .expect("non-utf8 test path"), + data: BlockSizeBufWriter::with_block_size( + Self::BLOCK_SIZE, + Vec::new(), + ), + semaphore: PollSemaphore::new(Arc::clone(&self.semaphore)), + finalized_writes: Arc::clone(&self.finalized_writes), + }) + } + + fn open_reader(&self, path: &Path) -> io::Result { + let written_files = self.finalized_writes.lock().unwrap(); + for contents in written_files.iter() { + if contents.path == path { + return Ok(io::Cursor::new(contents.data.clone())); + } + } + Err(io::Error::new( + io::ErrorKind::Other, + format!("no written file for {}", path.display()), + )) + } + } + + struct InMemoryDiskWriter { + opened_path: Utf8PathBuf, + data: BlockSizeBufWriter>, + semaphore: PollSemaphore, + finalized_writes: Arc>>, + } + + #[async_trait] + impl DiskWriter for InMemoryDiskWriter { + fn block_size(&self) -> usize { + self.data.block_size() + } + + async fn finalize(mut self) -> io::Result<()> { + self.data.flush().await?; + + let mut finalized = self.finalized_writes.lock().unwrap(); + finalized.push(InMemoryDiskContents { + path: self.opened_path, + data: self.data.into_inner(), + }); + + Ok(()) + } + } + + impl AsyncWrite for InMemoryDiskWriter { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + let permit = match ready!(self.semaphore.poll_acquire(cx)) { + Some(permit) => permit, + None => panic!("test semaphore closed"), + }; + let result = Pin::new(&mut self.data).poll_write(cx, buf); + permit.forget(); + result + } + + fn poll_flush( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + ) -> Poll> { + Pin::new(&mut self.data).poll_flush(cx) + } + + fn poll_shutdown( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + ) -> Poll> { + Pin::new(&mut self.data).poll_shutdown(cx) + } + } + + fn expect_in_progress( + status: BootDiskOsWriteStatus, + ) -> BootDiskOsWriteProgress { + let BootDiskOsWriteStatus::InProgress { progress, .. } = status else { + panic!("expected Status::InProgress; got {status:?}"); + }; + progress + } + + #[tokio::test] + async fn boot_disk_os_writer_delivers_upload_progress_and_rejects_bad_hashes( + ) { + let logctx = + test_setup_log("boot_disk_os_writer_delivers_upload_progress_and_rejects_bad_hashes"); + + let writer = Arc::new(BootDiskOsWriter::new(&logctx.log)); + let boot_disk = M2Slot::A; + + // We'll give the writer an intentionally-wrong sha3 digest and confirm + // it rejects the upload based on this. + let claimed_sha3_digest = [0; 32]; + + // Construct an in-memory stream around an mpsc channel as our client + // upload. + let (upload_tx, upload_rx) = mpsc::unbounded_channel(); + + // Spawn the `start_update` onto a background task; this won't end until + // we close (or send an error on) `upload_tx`. + let start_update_task = { + let writer = Arc::clone(&writer); + tokio::spawn(async move { + writer + .start_update( + boot_disk, + "/does-not-matter".into(), + Uuid::new_v4(), + claimed_sha3_digest, + UnboundedReceiverStream::new(upload_rx), + ) + .await + }) + }; + + // As we stream data in, we'll compute the actual hash to check against + // the error we expect to see. + let mut actual_data_hasher = Sha3_256::new(); + + // Run the rest of the test under a timeout to catch any incorrect + // assumptions that result in a hang. + tokio::time::timeout(TEST_TIMEOUT, async move { + // We're racing `writer`'s spawning of the actual update task; spin + // until we transition from "no update" to "receiving uploaded + // image". + loop { + match writer.status(boot_disk) { + BootDiskOsWriteStatus::NoUpdateStarted => { + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::InProgress { progress, .. } => { + assert_eq!( + progress, + BootDiskOsWriteProgress::ReceivingUploadedImage { + bytes_received: 0 + } + ); + break; + } + status @ (BootDiskOsWriteStatus::Complete { .. } + | BootDiskOsWriteStatus::Failed { .. }) => { + panic!("unexpected status {status:?}") + } + } + } + + let mut prev_bytes_received = 0; + + // Send a few chunks of data. After each, we're racing with `writer` + // which has to copy that data to a temp file before the status will + // change, so loop until we see what we expect. Our TEST_TIMEOUT + // ensures we don't stay here forever if something goes wrong. + for i in 1..=10 { + let data_len = i * 100; + let chunk = vec![0; data_len]; + actual_data_hasher.update(&chunk); + upload_tx.send(Ok(Bytes::from(chunk))).unwrap(); + + loop { + let progress = expect_in_progress(writer.status(boot_disk)); + + // If we lost the race, the status is still what it was + // previously; sleep briefly and check again. + if progress + == (BootDiskOsWriteProgress::ReceivingUploadedImage { + bytes_received: prev_bytes_received, + }) + { + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + + // It's not the old status; it should be exactly the new + // status. If it is, update our count and break out of this + // inner loop. + assert_eq!( + progress, + BootDiskOsWriteProgress::ReceivingUploadedImage { + bytes_received: prev_bytes_received + data_len + } + ); + prev_bytes_received += data_len; + println!("chunk {i}: got {progress:?}"); + break; + } + } + + // Close the channel; `writer` should recognize the upload is + // complete, then realize there's a hash mismatch and fail the + // request. + mem::drop(upload_tx); + + // We expect to see an upload hash mismatch error with these hex + // strings. + let expected_hash = hex::encode(claimed_sha3_digest); + let got_hash = hex::encode(actual_data_hasher.finalize()); + + let start_update_result = start_update_task.await.unwrap(); + let error = start_update_result.unwrap_err(); + match &*error { + BootDiskOsWriteError::UploadedImageHashMismatch { + expected, + got, + } => { + assert_eq!(*got, got_hash); + assert_eq!(*expected, expected_hash); + } + _ => panic!("unexpected error {error:?}"), + } + + // The same error should be present in the current update status. + let expected_error = + BootDiskOsWriteError::UploadedImageHashMismatch { + expected: expected_hash.clone(), + got: got_hash.clone(), + }; + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::Failed { message, .. } => { + assert_eq!( + message, + DisplayErrorChain::new(&expected_error).to_string() + ); + } + BootDiskOsWriteStatus::NoUpdateStarted + | BootDiskOsWriteStatus::InProgress { .. } + | BootDiskOsWriteStatus::Complete { .. } => { + panic!("unexpected status {status:?}") + } + } + }) + .await + .unwrap(); + + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn boot_disk_os_writer_writes_data_to_disk() { + let logctx = test_setup_log("boot_disk_os_writer_writes_data_to_disk"); + + // generate a small, random "OS image" consisting of 10 "blocks" + let num_data_blocks = 10; + let data_len = num_data_blocks * InMemoryDiskInterface::BLOCK_SIZE; + let mut data = vec![0; data_len]; + rand::thread_rng().fill_bytes(&mut data); + let data_hash = Sha3_256::digest(&data); + + // generate a disk writer with a 0-permit semaphore; we'll inject + // permits in the main loop below to force single-stepping through + // writing the data + let inject_disk_interface = + InMemoryDiskInterface::new(Semaphore::new(0)); + let shared_semaphore = Arc::clone(&inject_disk_interface.semaphore); + + let writer = Arc::new(BootDiskOsWriter::new(&logctx.log)); + let boot_disk = M2Slot::A; + let disk_devfs_path = "/unit-test/disk"; + + writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + Uuid::new_v4(), + data_hash.into(), + stream::once(future::ready(Ok(Bytes::from(data.clone())))), + inject_disk_interface, + ) + .await + .unwrap(); + + // Run the rest of the test under a timeout to catch any incorrect + // assumptions that result in a hang. + tokio::time::timeout(TEST_TIMEOUT, async move { + // Wait until `writer` has copied our data into a temp file + loop { + let progress = expect_in_progress(writer.status(boot_disk)); + match progress { + BootDiskOsWriteProgress::ReceivingUploadedImage { + bytes_received, + } => { + if bytes_received == data.len() { + break; + } else { + println!( + "got status with {} bytes received", + bytes_received + ); + } + } + _ => panic!("unexpected progress {progress:?}"), + } + } + + for i in 0..num_data_blocks { + // Add one permit to our shared semaphore, allowing one block of + // data to be written to the "disk". + shared_semaphore.add_permits(1); + + // Did we just release the write of the final block? If so, + // break; we'll wait for completion below. + if i + 1 == num_data_blocks { + break; + } + + // Wait until we see the status we expect for a not-yet-last + // block (i.e., that the disk is still being written). + loop { + let progress = expect_in_progress(writer.status(boot_disk)); + match progress { + BootDiskOsWriteProgress::WritingImageToDisk { + bytes_written, + } if (i + 1) * InMemoryDiskInterface::BLOCK_SIZE + == bytes_written => + { + println!("saw expected progress for block {i}"); + break; + } + _ => { + // This is not an error: we could still be in + // `ReceivingUploadedImage` or the previous + // block's `WritingImageToDisk` + println!("saw irrelevant progress {progress:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + } + } + } + + // The last block is being or has been written, and after that the + // writer will reread it to validate the hash. We won't bother + // repeating the same machinery to check each step of that process; + // we'll just wait for the eventual successful completion. + loop { + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::Complete { .. } => break, + BootDiskOsWriteStatus::InProgress { .. } => { + println!("saw irrelevant progress {status:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::NoUpdateStarted + | BootDiskOsWriteStatus::Failed { .. } => { + panic!("unexpected status {status:?}") + } + } + } + }) + .await + .unwrap(); + + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn boot_disk_os_writer_fails_if_reading_from_disk_doesnt_match() { + let logctx = test_setup_log( + "boot_disk_os_writer_fails_if_reading_from_disk_doesnt_match", + ); + + // generate a small, random "OS image" consisting of 10 "blocks" + let num_data_blocks = 10; + let data_len = num_data_blocks * InMemoryDiskInterface::BLOCK_SIZE; + let mut data = vec![0; data_len]; + rand::thread_rng().fill_bytes(&mut data); + let original_data_hash = Sha3_256::digest(&data); + + // generate a disk writer with (effectively) unlimited semaphore + // permits, since we don't need to throttle the "disk writing" + let inject_disk_interface = + InMemoryDiskInterface::new(Semaphore::new(Semaphore::MAX_PERMITS)); + + let writer = Arc::new(BootDiskOsWriter::new(&logctx.log)); + let boot_disk = M2Slot::A; + let disk_devfs_path = "/unit-test/disk"; + + // copy the data and corrupt it, then stage this in + // `inject_disk_interface` so that it returns this corrupted data when + // "reading" the disk + let mut bad_data = data.clone(); + bad_data[0] ^= 1; // bit flip + let bad_data_hash = Sha3_256::digest(&bad_data); + inject_disk_interface.finalized_writes.lock().unwrap().push( + InMemoryDiskContents { + path: disk_devfs_path.into(), + data: bad_data, + }, + ); + + writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + Uuid::new_v4(), + original_data_hash.into(), + stream::once(future::ready(Ok(Bytes::from(data.clone())))), + inject_disk_interface, + ) + .await + .unwrap(); + + // We expect the update to eventually fail; wait for it to do so. + let failure_message = tokio::time::timeout(TEST_TIMEOUT, async move { + loop { + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::Failed { message, .. } => { + return message; + } + BootDiskOsWriteStatus::InProgress { .. } => { + println!("saw irrelevant status {status:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::Complete { .. } + | BootDiskOsWriteStatus::NoUpdateStarted => { + panic!("unexpected status {status:?}"); + } + } + } + }) + .await + .unwrap(); + + // Confirm that the update fails for the reason we expect: when + // re-reading what had been written to disk, it got our corrupt data + // (which hashes to `bad_data_hash`) instead of the expected + // `original_data_hash`. + let expected_error = BootDiskOsWriteError::WrittenImageHashMismatch { + path: disk_devfs_path.into(), + expected: hex::encode(&original_data_hash), + got: hex::encode(&bad_data_hash), + }; + + assert_eq!( + failure_message, + DisplayErrorChain::new(&expected_error).to_string() + ); + + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn boot_disk_os_writer_can_update_both_slots_simultaneously() { + let logctx = test_setup_log( + "boot_disk_os_writer_can_update_both_slots_simultaneously", + ); + + // generate two small, random "OS image"s consisting of 10 "blocks" each + let num_data_blocks = 10; + let data_len = num_data_blocks * InMemoryDiskInterface::BLOCK_SIZE; + let mut data_a = vec![0; data_len]; + let mut data_b = vec![0; data_len]; + rand::thread_rng().fill_bytes(&mut data_a); + rand::thread_rng().fill_bytes(&mut data_b); + let data_hash_a = Sha3_256::digest(&data_a); + let data_hash_b = Sha3_256::digest(&data_b); + + // generate a disk writer with no semaphore permits so the updates block + // until we get a chance to start both of them + let inject_disk_interface = + InMemoryDiskInterface::new(Semaphore::new(0)); + let shared_semaphore = Arc::clone(&inject_disk_interface.semaphore); + + let writer = Arc::new(BootDiskOsWriter::new(&logctx.log)); + let disk_devfs_path_a = "/unit-test/disk/a"; + let disk_devfs_path_b = "/unit-test/disk/b"; + + let update_id_a = Uuid::new_v4(); + let update_id_b = Uuid::new_v4(); + + writer + .start_update_impl( + M2Slot::A, + disk_devfs_path_a.into(), + update_id_a, + data_hash_a.into(), + stream::once(future::ready(Ok(Bytes::from(data_a.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap(); + + writer + .start_update_impl( + M2Slot::B, + disk_devfs_path_b.into(), + update_id_b, + data_hash_b.into(), + stream::once(future::ready(Ok(Bytes::from(data_b.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap(); + + // Both updates have successfully started; unblock the "disks". + shared_semaphore.add_permits(Semaphore::MAX_PERMITS); + + // Wait for both updates to complete successfully. + for boot_disk in [M2Slot::A, M2Slot::B] { + tokio::time::timeout(TEST_TIMEOUT, async { + loop { + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::InProgress { .. } => { + println!("saw irrelevant status {status:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::Complete { update_id } => { + match boot_disk { + M2Slot::A => assert_eq!(update_id, update_id_a), + M2Slot::B => assert_eq!(update_id, update_id_b), + } + break; + } + BootDiskOsWriteStatus::Failed { .. } + | BootDiskOsWriteStatus::NoUpdateStarted => { + panic!("unexpected status {status:?}"); + } + } + } + }) + .await + .unwrap(); + } + + // Ensure each "disk" saw the expected contents. + let expected_disks = [ + InMemoryDiskContents { + path: disk_devfs_path_a.into(), + data: data_a, + }, + InMemoryDiskContents { + path: disk_devfs_path_b.into(), + data: data_b, + }, + ]; + let written_disks = + inject_disk_interface.finalized_writes.lock().unwrap(); + assert_eq!(written_disks.len(), expected_disks.len()); + for expected in expected_disks { + assert!( + written_disks.contains(&expected), + "written disks missing expected contents for {}", + expected.path, + ); + } + + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn boot_disk_os_writer_rejects_new_updates_while_old_running() { + let logctx = test_setup_log( + "boot_disk_os_writer_rejects_new_updates_while_old_running", + ); + + // generate two small, random "OS image"s consisting of 10 "blocks" each + let num_data_blocks = 10; + let data_len = num_data_blocks * InMemoryDiskInterface::BLOCK_SIZE; + let mut data_a = vec![0; data_len]; + let mut data_b = vec![0; data_len]; + rand::thread_rng().fill_bytes(&mut data_a); + rand::thread_rng().fill_bytes(&mut data_b); + let data_hash_a = Sha3_256::digest(&data_a); + let data_hash_b = Sha3_256::digest(&data_b); + + // generate a disk writer with no semaphore permits so the updates block + // until we get a chance to (try to) start both of them + let inject_disk_interface = + InMemoryDiskInterface::new(Semaphore::new(0)); + let shared_semaphore = Arc::clone(&inject_disk_interface.semaphore); + + let writer = Arc::new(BootDiskOsWriter::new(&logctx.log)); + let disk_devfs_path = "/unit-test/disk"; + let boot_disk = M2Slot::A; + + let update_id_a = Uuid::new_v4(); + let update_id_b = Uuid::new_v4(); + + writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + update_id_a, + data_hash_a.into(), + stream::once(future::ready(Ok(Bytes::from(data_a.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap(); + + let error = writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + update_id_b, + data_hash_b.into(), + stream::once(future::ready(Ok(Bytes::from(data_b.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap_err(); + match &*error { + BootDiskOsWriteError::UpdateRunning(running_id) => { + assert_eq!(*running_id, update_id_a); + } + _ => panic!("unexpected error {error}"), + } + + // Both update attempts started; unblock the "disk". + shared_semaphore.add_permits(Semaphore::MAX_PERMITS); + + // Wait for the first update to complete successfully. + tokio::time::timeout(TEST_TIMEOUT, async { + loop { + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::InProgress { .. } => { + println!("saw irrelevant status {status:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::Complete { update_id } => { + assert_eq!(update_id, update_id_a); + break; + } + BootDiskOsWriteStatus::Failed { .. } + | BootDiskOsWriteStatus::NoUpdateStarted => { + panic!("unexpected status {status:?}"); + } + } + } + }) + .await + .unwrap(); + + // Ensure we wrote the contents of the first update. + let expected_disks = [InMemoryDiskContents { + path: disk_devfs_path.into(), + data: data_a, + }]; + let written_disks = + inject_disk_interface.finalized_writes.lock().unwrap(); + assert_eq!(*written_disks, expected_disks); + + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn boot_disk_os_writer_rejects_new_updates_while_old_completed() { + let logctx = test_setup_log( + "boot_disk_os_writer_rejects_new_updates_while_old_completed", + ); + + // generate two small, random "OS image"s consisting of 10 "blocks" each + let num_data_blocks = 10; + let data_len = num_data_blocks * InMemoryDiskInterface::BLOCK_SIZE; + let mut data_a = vec![0; data_len]; + let mut data_b = vec![0; data_len]; + rand::thread_rng().fill_bytes(&mut data_a); + rand::thread_rng().fill_bytes(&mut data_b); + let data_hash_a = Sha3_256::digest(&data_a); + let data_hash_b = Sha3_256::digest(&data_b); + + // generate a disk writer with effectively infinite semaphore permits + let inject_disk_interface = + InMemoryDiskInterface::new(Semaphore::new(Semaphore::MAX_PERMITS)); + + let writer = Arc::new(BootDiskOsWriter::new(&logctx.log)); + let disk_devfs_path = "/unit-test/disk"; + let boot_disk = M2Slot::A; + + let update_id_a = Uuid::new_v4(); + let update_id_b = Uuid::new_v4(); + + writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + update_id_a, + data_hash_a.into(), + stream::once(future::ready(Ok(Bytes::from(data_a.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap(); + + // Wait for the first update to complete successfully. + tokio::time::timeout(TEST_TIMEOUT, async { + loop { + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::InProgress { update_id, .. } => { + assert_eq!(update_id, update_id_a); + println!("saw irrelevant status {status:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::Complete { update_id } => { + assert_eq!(update_id, update_id_a); + break; + } + BootDiskOsWriteStatus::Failed { .. } + | BootDiskOsWriteStatus::NoUpdateStarted => { + panic!("unexpected status {status:?}"); + } + } + } + }) + .await + .unwrap(); + + // Ensure we wrote the contents of the first update. + let expected_disks = [InMemoryDiskContents { + path: disk_devfs_path.into(), + data: data_a, + }]; + { + let mut written_disks = + inject_disk_interface.finalized_writes.lock().unwrap(); + assert_eq!(*written_disks, expected_disks); + written_disks.clear(); + } + + // Check that we get the expected error when attempting to start another + // update to this same disk. + let expected_error = + BootDiskOsWriteError::CannotStartWithoutClearingPreviousStatus( + update_id_a, + ); + let error = writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + update_id_b, + data_hash_b.into(), + stream::once(future::ready(Ok(Bytes::from(data_b.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap_err(); + assert_eq!(error.to_string(), expected_error.to_string()); + + // We should not be able to clear the status with an incorrect update + // ID. + let expected_error = + BootDiskOsWriteError::WrongUpdateIdClearingStatus(update_id_a); + let error = + writer.clear_terminal_status(boot_disk, update_id_b).unwrap_err(); + assert_eq!(error.to_string(), expected_error.to_string()); + + // We should be able to clear the status with the correct update ID, and + // then start the new one. + writer.clear_terminal_status(boot_disk, update_id_a).unwrap(); + writer + .start_update_impl( + boot_disk, + disk_devfs_path.into(), + update_id_b, + data_hash_b.into(), + stream::once(future::ready(Ok(Bytes::from(data_b.clone())))), + inject_disk_interface.clone(), + ) + .await + .unwrap(); + + // Wait for the second update to complete successfully. + tokio::time::timeout(TEST_TIMEOUT, async { + loop { + let status = writer.status(boot_disk); + match status { + BootDiskOsWriteStatus::InProgress { update_id, .. } => { + assert_eq!(update_id, update_id_b); + println!("saw irrelevant status {status:?}"); + tokio::time::sleep(Duration::from_millis(50)).await; + continue; + } + BootDiskOsWriteStatus::Complete { update_id } => { + assert_eq!(update_id, update_id_b); + break; + } + BootDiskOsWriteStatus::Failed { .. } + | BootDiskOsWriteStatus::NoUpdateStarted => { + panic!("unexpected status {status:?}"); + } + } + } + }) + .await + .unwrap(); + + // Ensure we wrote the contents of the second update. + let expected_disks = [InMemoryDiskContents { + path: disk_devfs_path.into(), + data: data_b, + }]; + { + let mut written_disks = + inject_disk_interface.finalized_writes.lock().unwrap(); + assert_eq!(*written_disks, expected_disks); + written_disks.clear(); + } + + logctx.cleanup_successful(); + } +} diff --git a/sled-agent/src/config.rs b/sled-agent/src/config.rs index a596cf83db..058f343e2a 100644 --- a/sled-agent/src/config.rs +++ b/sled-agent/src/config.rs @@ -6,6 +6,7 @@ use crate::updates::ConfigUpdates; use camino::{Utf8Path, Utf8PathBuf}; +use dropshot::ConfigDropshot; use dropshot::ConfigLogging; use illumos_utils::dladm::Dladm; use illumos_utils::dladm::FindPhysicalLinkError; @@ -44,6 +45,11 @@ pub struct SoftPortConfig { #[derive(Clone, Debug, Deserialize)] #[serde(deny_unknown_fields)] pub struct Config { + /// Configuration for the sled agent dropshot server + /// + /// If the `bind_address` is set, it will be ignored. The remaining fields + /// will be respected. + pub dropshot: ConfigDropshot, /// Configuration for the sled agent debug log pub log: ConfigLogging, /// The sled's mode of operation (auto detect or force gimlet/scrimlet). diff --git a/sled-agent/src/http_entrypoints.rs b/sled-agent/src/http_entrypoints.rs index 2dcb35b77e..8c8a5f2a03 100644 --- a/sled-agent/src/http_entrypoints.rs +++ b/sled-agent/src/http_entrypoints.rs @@ -18,14 +18,17 @@ use crate::sled_agent::Error as SledAgentError; use crate::zone_bundle; use bootstore::schemes::v0::NetworkConfig; use camino::Utf8PathBuf; +use display_error_chain::DisplayErrorChain; use dropshot::{ endpoint, ApiDescription, FreeformBody, HttpError, HttpResponseCreated, HttpResponseDeleted, HttpResponseHeaders, HttpResponseOk, - HttpResponseUpdatedNoContent, Path, Query, RequestContext, TypedBody, + HttpResponseUpdatedNoContent, Path, Query, RequestContext, StreamingBody, + TypedBody, }; use illumos_utils::opte::params::{ DeleteVirtualNetworkInterfaceHost, SetVirtualNetworkInterfaceHost, }; +use installinator_common::M2Slot; use omicron_common::api::external::Error; use omicron_common::api::internal::nexus::{ DiskRuntimeState, SledInstanceState, UpdateArtifactId, @@ -36,6 +39,7 @@ use oximeter_producer::collect; use oximeter_producer::ProducerIdPathParams; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use sled_hardware::DiskVariant; use std::collections::BTreeMap; use uuid::Uuid; @@ -75,6 +79,9 @@ pub fn api() -> SledApiDescription { api.register(write_network_bootstore_config)?; api.register(add_sled_to_initialized_rack)?; api.register(metrics_collect)?; + api.register(host_os_write_start)?; + api.register(host_os_write_status_get)?; + api.register(host_os_write_status_delete)?; Ok(()) } @@ -755,3 +762,166 @@ async fn metrics_collect( let producer_id = path_params.into_inner().producer_id; collect(&sa.metrics_registry(), producer_id).await } + +#[derive(Clone, Copy, Debug, Deserialize, JsonSchema, Serialize)] +pub struct BootDiskPathParams { + pub boot_disk: M2Slot, +} + +#[derive(Clone, Copy, Debug, Deserialize, JsonSchema, Serialize)] +pub struct BootDiskUpdatePathParams { + pub boot_disk: M2Slot, + pub update_id: Uuid, +} + +#[derive(Clone, Copy, Debug, Deserialize, JsonSchema, Serialize)] +pub struct BootDiskWriteStartQueryParams { + pub update_id: Uuid, + // TODO do we already have sha2-256 hashes of the OS images, and if so + // should we use that instead? Another option is to use the external API + // `Digest` type, although it predates `serde_human_bytes` so just stores + // the hash as a `String`. + #[serde(with = "serde_human_bytes::hex_array")] + #[schemars(schema_with = "omicron_common::hex_schema::<32>")] + pub sha3_256_digest: [u8; 32], +} + +/// Write a new host OS image to the specified boot disk +#[endpoint { + method = POST, + path = "/boot-disk/{boot_disk}/os/write", +}] +async fn host_os_write_start( + request_context: RequestContext, + path_params: Path, + query_params: Query, + body: StreamingBody, +) -> Result { + let sa = request_context.context(); + let boot_disk = path_params.into_inner().boot_disk; + + // Find our corresponding disk. + let maybe_disk_path = + sa.storage().get_latest_resources().await.disks().values().find_map( + |(disk, _pool)| { + // Synthetic disks panic if asked for their `slot()`, so filter + // them out first; additionally, filter out any non-M2 disks. + if disk.is_synthetic() || disk.variant() != DiskVariant::M2 { + return None; + } + + // Convert this M2 disk's slot to an M2Slot, and skip any that + // don't match the requested boot_disk. + let Ok(slot) = M2Slot::try_from(disk.slot()) else { + return None; + }; + if slot != boot_disk { + return None; + } + + let raw_devs_path = true; + Some(disk.boot_image_devfs_path(raw_devs_path)) + }, + ); + + let disk_path = match maybe_disk_path { + Some(Ok(path)) => path, + Some(Err(err)) => { + let message = format!( + "failed to find devfs path for {boot_disk:?}: {}", + DisplayErrorChain::new(&err) + ); + return Err(HttpError { + status_code: http::StatusCode::SERVICE_UNAVAILABLE, + error_code: None, + external_message: message.clone(), + internal_message: message, + }); + } + None => { + let message = format!("no disk found for slot {boot_disk:?}",); + return Err(HttpError { + status_code: http::StatusCode::SERVICE_UNAVAILABLE, + error_code: None, + external_message: message.clone(), + internal_message: message, + }); + } + }; + + let BootDiskWriteStartQueryParams { update_id, sha3_256_digest } = + query_params.into_inner(); + sa.boot_disk_os_writer() + .start_update( + boot_disk, + disk_path, + update_id, + sha3_256_digest, + body.into_stream(), + ) + .await + .map_err(|err| HttpError::from(&*err))?; + Ok(HttpResponseUpdatedNoContent()) +} + +/// Current progress of an OS image being written to disk. +#[derive( + Debug, Clone, Copy, PartialEq, Eq, Deserialize, JsonSchema, Serialize, +)] +#[serde(tag = "state", rename_all = "snake_case")] +pub enum BootDiskOsWriteProgress { + /// The image is still being uploaded. + ReceivingUploadedImage { bytes_received: usize }, + /// The image is being written to disk. + WritingImageToDisk { bytes_written: usize }, + /// The image is being read back from disk for validation. + ValidatingWrittenImage { bytes_read: usize }, +} + +/// Status of an update to a boot disk OS. +#[derive(Debug, Clone, Deserialize, JsonSchema, Serialize)] +#[serde(tag = "status", rename_all = "snake_case")] +pub enum BootDiskOsWriteStatus { + /// No update has been started for this disk, or any previously-started + /// update has completed and had its status cleared. + NoUpdateStarted, + /// An update is currently running. + InProgress { update_id: Uuid, progress: BootDiskOsWriteProgress }, + /// The most recent update completed successfully. + Complete { update_id: Uuid }, + /// The most recent update failed. + Failed { update_id: Uuid, message: String }, +} + +/// Get the status of writing a new host OS +#[endpoint { + method = GET, + path = "/boot-disk/{boot_disk}/os/write/status", +}] +async fn host_os_write_status_get( + request_context: RequestContext, + path_params: Path, +) -> Result, HttpError> { + let sa = request_context.context(); + let boot_disk = path_params.into_inner().boot_disk; + let status = sa.boot_disk_os_writer().status(boot_disk); + Ok(HttpResponseOk(status)) +} + +/// Clear the status of a completed write of a new host OS +#[endpoint { + method = DELETE, + path = "/boot-disk/{boot_disk}/os/write/status/{update_id}", +}] +async fn host_os_write_status_delete( + request_context: RequestContext, + path_params: Path, +) -> Result { + let sa = request_context.context(); + let BootDiskUpdatePathParams { boot_disk, update_id } = + path_params.into_inner(); + sa.boot_disk_os_writer() + .clear_terminal_status(boot_disk, update_id) + .map_err(|err| HttpError::from(&err))?; + Ok(HttpResponseUpdatedNoContent()) +} diff --git a/sled-agent/src/lib.rs b/sled-agent/src/lib.rs index d77ec7a3c0..527b483ee8 100644 --- a/sled-agent/src/lib.rs +++ b/sled-agent/src/lib.rs @@ -18,6 +18,7 @@ pub mod common; // Modules for the non-simulated sled agent. mod backing_fs; +mod boot_disk_os_writer; pub mod bootstrap; pub mod config; pub(crate) mod dump_setup; diff --git a/sled-agent/src/server.rs b/sled-agent/src/server.rs index 903c8dabaa..b93ad0721c 100644 --- a/sled-agent/src/server.rs +++ b/sled-agent/src/server.rs @@ -70,9 +70,10 @@ impl Server { .await .map_err(|e| e.to_string())?; - let mut dropshot_config = dropshot::ConfigDropshot::default(); - dropshot_config.request_body_max_bytes = 1024 * 1024; - dropshot_config.bind_address = SocketAddr::V6(sled_address); + let dropshot_config = dropshot::ConfigDropshot { + bind_address: SocketAddr::V6(sled_address), + ..config.dropshot + }; let dropshot_log = log.new(o!("component" => "dropshot (SledAgent)")); let http_server = dropshot::HttpServerStarter::new( &dropshot_config, diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 57aea61ae9..5f278b7f38 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -4,6 +4,7 @@ //! Sled agent implementation +use crate::boot_disk_os_writer::BootDiskOsWriter; use crate::bootstrap::config::BOOTSTRAP_AGENT_RACK_INIT_PORT; use crate::bootstrap::early_networking::{ EarlyNetworkConfig, EarlyNetworkSetupError, @@ -264,6 +265,9 @@ struct SledAgentInner { // Object handling production of metrics for oximeter. metrics_manager: MetricsManager, + + // Handle to the traffic manager for writing OS updates to our boot disks. + boot_disk_os_writer: BootDiskOsWriter, } impl SledAgentInner { @@ -545,6 +549,7 @@ impl SledAgent { zone_bundler: long_running_task_handles.zone_bundler.clone(), bootstore: long_running_task_handles.bootstore.clone(), metrics_manager, + boot_disk_os_writer: BootDiskOsWriter::new(&parent_log), }), log: log.clone(), }; @@ -1043,6 +1048,14 @@ impl SledAgent { pub fn metrics_registry(&self) -> &ProducerRegistry { self.inner.metrics_manager.registry() } + + pub(crate) fn storage(&self) -> &StorageHandle { + &self.inner.storage + } + + pub(crate) fn boot_disk_os_writer(&self) -> &BootDiskOsWriter { + &self.inner.boot_disk_os_writer + } } async fn register_metric_producer_with_nexus( diff --git a/smf/sled-agent/gimlet-standalone/config.toml b/smf/sled-agent/gimlet-standalone/config.toml index e714504311..4d06895453 100644 --- a/smf/sled-agent/gimlet-standalone/config.toml +++ b/smf/sled-agent/gimlet-standalone/config.toml @@ -41,6 +41,11 @@ swap_device_size_gb = 256 data_links = ["net0", "net1"] +[dropshot] +# Host OS images are just over 800 MiB currently; set this to 2 GiB to give some +# breathing room. +request_body_max_bytes = 2_147_483_648 + [log] level = "info" mode = "file" diff --git a/smf/sled-agent/gimlet/config.toml b/smf/sled-agent/gimlet/config.toml index 442e76b393..666d55f359 100644 --- a/smf/sled-agent/gimlet/config.toml +++ b/smf/sled-agent/gimlet/config.toml @@ -37,6 +37,11 @@ swap_device_size_gb = 256 data_links = ["cxgbe0", "cxgbe1"] +[dropshot] +# Host OS images are just over 800 MiB currently; set this to 2 GiB to give some +# breathing room. +request_body_max_bytes = 2_147_483_648 + [log] level = "info" mode = "file" diff --git a/smf/sled-agent/non-gimlet/config.toml b/smf/sled-agent/non-gimlet/config.toml index 176f4002a5..432652c50b 100644 --- a/smf/sled-agent/non-gimlet/config.toml +++ b/smf/sled-agent/non-gimlet/config.toml @@ -76,6 +76,11 @@ switch_zone_maghemite_links = ["tfportrear0_0"] data_links = ["net0", "net1"] +[dropshot] +# Host OS images are just over 800 MiB currently; set this to 2 GiB to give some +# breathing room. +request_body_max_bytes = 2_147_483_648 + [log] level = "info" mode = "file" From 5e9251783215ed7e4b86e1359e287e83889c59cd Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Fri, 8 Dec 2023 11:57:49 -0800 Subject: [PATCH 02/11] move the cockroachdb http console to localhost (#4655) I can't find any use of the HTTP console or any of its endpoints for anything, so let's move it off the underlay network. I have not tested this on hardware myself, but the logs for the deploy job here indicate that the `webui` has in fact moved to 127.0.0.1. I couldn't find a way to completely disable the HTTP console, but I think even if I did I would prefer this, as it still lets us access it for in-situ debugging (although I'm not well-versed enough with zones to understand how you would write an SSH forward to get to it with this change). --- smf/cockroachdb/method_script.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/smf/cockroachdb/method_script.sh b/smf/cockroachdb/method_script.sh index ee42ab1891..e5ab4e8eaa 100755 --- a/smf/cockroachdb/method_script.sh +++ b/smf/cockroachdb/method_script.sh @@ -44,6 +44,7 @@ fi args=( '--insecure' '--listen-addr' "[$LISTEN_ADDR]:$LISTEN_PORT" + '--http-addr' '127.0.0.1:8080' '--store' "$DATASTORE" '--join' "$JOIN_ADDRS" ) From 2d93fede364f4dcaca7a62f62f96f2b9bc80e4e5 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 8 Dec 2023 14:21:53 -0800 Subject: [PATCH 03/11] [sled-agent/sled-hardware] remove serial_test dependency (#4656) serial_test is a proc macro which ensures that tests run serially rather than in parallel. However, as documented at https://nexte.st/book/test-groups.html, serial_test doesn't actually work with nextest. So go ahead and remove it as a dependency. The next question is: do we need to replace it with nextest's test groups? It turns out that the answer is "no". The only uses of serial_test in our codebase were to enable testing against a mocked free function (if multiple tests would call that function concurrently, the mocking infrastructure would get confused). However, that is only an issue if multiple tests are run within the same process. Nextest's execution model is process-per-test, which means that this isn't an issue at all. (This is also hinted at by the fact that `serial_test` has effectively been inoperative for months, yet we've had no issues with these tests.) --- Cargo.lock | 40 ------------------------- Cargo.toml | 1 - sled-agent/Cargo.toml | 1 - sled-agent/src/services.rs | 8 ----- sled-hardware/Cargo.toml | 1 - sled-hardware/src/illumos/partitions.rs | 1 - 6 files changed, 52 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 71cca52057..67e1d3784c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1470,19 +1470,6 @@ dependencies = [ "syn 2.0.32", ] -[[package]] -name = "dashmap" -version = "5.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "edd72493923899c6f10c641bdbdeddc7183d6396641d99c1a0d1597f37f92e28" -dependencies = [ - "cfg-if", - "hashbrown 0.14.2", - "lock_api", - "once_cell", - "parking_lot_core 0.9.8", -] - [[package]] name = "data-encoding" version = "2.4.0" @@ -4916,7 +4903,6 @@ dependencies = [ "serde", "serde_human_bytes", "serde_json", - "serial_test", "sha3", "sled-agent-client", "sled-hardware", @@ -7438,31 +7424,6 @@ dependencies = [ "unsafe-libyaml", ] -[[package]] -name = "serial_test" -version = "0.10.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1c789ec87f4687d022a2405cf46e0cd6284889f1839de292cadeb6c6019506f2" -dependencies = [ - "dashmap", - "futures", - "lazy_static", - "log", - "parking_lot 0.12.1", - "serial_test_derive", -] - -[[package]] -name = "serial_test_derive" -version = "0.10.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b64f9e531ce97c88b4778aad0ceee079216071cffec6ac9b904277f8f92e7fe3" -dependencies = [ - "proc-macro2", - "quote", - "syn 1.0.109", -] - [[package]] name = "sha1" version = "0.10.6" @@ -7654,7 +7615,6 @@ dependencies = [ "rand 0.8.5", "schemars", "serde", - "serial_test", "slog", "thiserror", "tofino", diff --git a/Cargo.toml b/Cargo.toml index 2bdd8522eb..f8d2a07977 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -322,7 +322,6 @@ serde_path_to_error = "0.1.14" serde_tokenstream = "0.2" serde_urlencoded = "0.7.1" serde_with = "3.4.0" -serial_test = "0.10" sha2 = "0.10.8" sha3 = "0.10.8" shell-words = "1.1.0" diff --git a/sled-agent/Cargo.toml b/sled-agent/Cargo.toml index 7607d57b95..3f7fd1c7f2 100644 --- a/sled-agent/Cargo.toml +++ b/sled-agent/Cargo.toml @@ -92,7 +92,6 @@ openapi-lint.workspace = true openapiv3.workspace = true pretty_assertions.workspace = true rcgen.workspace = true -serial_test.workspace = true subprocess.workspace = true slog-async.workspace = true slog-term.workspace = true diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index 651d2638e0..837c2a05df 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -3884,7 +3884,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_ensure_service() { let logctx = omicron_test_utils::dev::test_setup_log("test_ensure_service"); @@ -3916,7 +3915,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_ensure_service_which_already_exists() { let logctx = omicron_test_utils::dev::test_setup_log( "test_ensure_service_which_already_exists", @@ -3944,7 +3942,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_services_are_recreated_on_reboot() { let logctx = omicron_test_utils::dev::test_setup_log( "test_services_are_recreated_on_reboot", @@ -3981,7 +3978,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_services_do_not_persist_without_config() { let logctx = omicron_test_utils::dev::test_setup_log( "test_services_do_not_persist_without_config", @@ -4023,7 +4019,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_bad_generations() { // Start like the normal tests. let logctx = @@ -4128,7 +4123,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_old_ledger_migration() { let logctx = omicron_test_utils::dev::test_setup_log( "test_old_ledger_migration", @@ -4193,7 +4187,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_old_ledger_migration_continue() { // This test is just like "test_old_ledger_migration", except that we // deploy a new zone after migration and before shutting down the @@ -4271,7 +4264,6 @@ mod test { } #[tokio::test] - #[serial_test::serial] async fn test_old_ledger_migration_bad() { let logctx = omicron_test_utils::dev::test_setup_log( "test_old_ledger_migration_bad", diff --git a/sled-hardware/Cargo.toml b/sled-hardware/Cargo.toml index 36ba633067..66ecbf9d64 100644 --- a/sled-hardware/Cargo.toml +++ b/sled-hardware/Cargo.toml @@ -31,4 +31,3 @@ libefi-illumos = { git = "https://github.com/oxidecomputer/libefi-illumos", bran [dev-dependencies] illumos-utils = { workspace = true, features = ["testing"] } omicron-test-utils.workspace = true -serial_test.workspace = true diff --git a/sled-hardware/src/illumos/partitions.rs b/sled-hardware/src/illumos/partitions.rs index 4b7e69057d..de62e25cfe 100644 --- a/sled-hardware/src/illumos/partitions.rs +++ b/sled-hardware/src/illumos/partitions.rs @@ -207,7 +207,6 @@ mod test { } #[test] - #[serial_test::serial] fn ensure_partition_layout_u2_format_with_dev_path() { let logctx = test_setup_log("ensure_partition_layout_u2_format_with_dev_path"); From 2a3db4154a73761b559f6e2d9bc2640de15d1c3d Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 8 Dec 2023 14:39:57 -0800 Subject: [PATCH 04/11] [nexus] improve external messages and make more available to clients (#4573) While developing #4520, I observed that we were producing a number of error messages that were: * 503 Service Unavailable, * With only an internal message attached * But where the message is both safe and useful to display to clients. This is my attempt to make the situation slightly better. To achieve this, I made a few changes: 1. Make all the client errors carry a new `MessagePair` struct, which consists of an external message. (Along the way, correct the definition of e.g. the `Conflict` variant: it actually is an external message, not an internal one.) 2. Define a new `InsufficientCapacity` variant that consists of both an external and an internal message. This variant resolves to a 507 Insufficient Storage error, and has a more helpful message than just "Service Unavailable". 3. Turn some current 503 errors into client errors so that the message is available externally. Looking for feedback on this approach! --- certificates/src/lib.rs | 16 +- common/src/api/external/error.rs | 222 ++++++++++++++---- common/src/api/external/mod.rs | 13 +- common/src/vlan.rs | 16 +- docs/http-status-codes.adoc | 3 +- nexus/db-model/src/instance_state.rs | 7 + nexus/db-model/src/semver_version.rs | 10 +- nexus/db-queries/src/db/datastore/disk.rs | 14 +- .../src/db/datastore/external_ip.rs | 16 +- nexus/db-queries/src/db/datastore/ip_pool.rs | 15 +- .../src/db/datastore/ipv4_nat_entry.rs | 12 +- nexus/db-queries/src/db/datastore/mod.rs | 4 +- nexus/db-queries/src/db/datastore/project.rs | 14 +- nexus/db-queries/src/db/datastore/saga.rs | 6 +- nexus/db-queries/src/db/datastore/silo.rs | 14 +- nexus/db-queries/src/db/datastore/sled.rs | 6 +- nexus/db-queries/src/db/datastore/vpc.rs | 39 ++- .../db-queries/src/db/queries/external_ip.rs | 28 ++- .../src/db/queries/region_allocation.rs | 10 +- nexus/src/app/address_lot.rs | 19 +- nexus/src/app/device_auth.rs | 4 +- nexus/src/app/disk.rs | 32 +-- nexus/src/app/external_endpoints.rs | 2 +- nexus/src/app/image.rs | 20 +- nexus/src/app/instance.rs | 75 +++--- nexus/src/app/rack.rs | 7 +- nexus/src/app/session.rs | 2 +- nexus/src/app/silo.rs | 38 ++- nexus/src/app/switch_interface.rs | 6 +- nexus/src/app/update/mod.rs | 12 +- nexus/src/app/vpc_router.rs | 23 +- nexus/src/external_api/http_entrypoints.rs | 5 +- nexus/tests/integration_tests/disks.rs | 6 +- nexus/tests/integration_tests/instances.rs | 16 +- .../tests/integration_tests/router_routes.rs | 2 +- nexus/tests/integration_tests/snapshots.rs | 2 +- .../integration_tests/volume_management.rs | 2 +- sled-agent/src/common/disk.rs | 26 +- sled-agent/src/instance.rs | 12 +- sled-agent/src/sim/collection.rs | 7 +- sled-agent/src/sim/instance.rs | 12 +- 41 files changed, 440 insertions(+), 355 deletions(-) diff --git a/certificates/src/lib.rs b/certificates/src/lib.rs index 6bd7fa32de..442a9cfdd5 100644 --- a/certificates/src/lib.rs +++ b/certificates/src/lib.rs @@ -60,14 +60,14 @@ impl From for Error { | InvalidValidationHostname(_) | ErrorValidatingHostname(_) | NoDnsNameMatchingHostname { .. } - | UnsupportedPurpose => Error::InvalidValue { - label: String::from("certificate"), - message: DisplayErrorChain::new(&error).to_string(), - }, - BadPrivateKey(_) => Error::InvalidValue { - label: String::from("private-key"), - message: DisplayErrorChain::new(&error).to_string(), - }, + | UnsupportedPurpose => Error::invalid_value( + "certificate", + DisplayErrorChain::new(&error).to_string(), + ), + BadPrivateKey(_) => Error::invalid_value( + "private-key", + DisplayErrorChain::new(&error).to_string(), + ), Unexpected(_) => Error::InternalError { internal_message: DisplayErrorChain::new(&error).to_string(), }, diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index e508b7ecba..2661db7bb6 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -35,16 +35,16 @@ pub enum Error { ObjectAlreadyExists { type_name: ResourceType, object_name: String }, /// The request was well-formed, but the operation cannot be completed given /// the current state of the system. - #[error("Invalid Request: {message}")] - InvalidRequest { message: String }, + #[error("Invalid Request: {}", .message.display_internal())] + InvalidRequest { message: MessagePair }, /// Authentication credentials were required but either missing or invalid. /// The HTTP status code is called "Unauthorized", but it's more accurate to /// call it "Unauthenticated". #[error("Missing or invalid credentials")] Unauthenticated { internal_message: String }, /// The specified input field is not valid. - #[error("Invalid Value: {label}, {message}")] - InvalidValue { label: String, message: String }, + #[error("Invalid Value: {label}, {}", .message.display_internal())] + InvalidValue { label: String, message: MessagePair }, /// The request is not authorized to perform the requested operation. #[error("Forbidden")] Forbidden, @@ -55,15 +55,86 @@ pub enum Error { /// The system (or part of it) is unavailable. #[error("Service Unavailable: {internal_message}")] ServiceUnavailable { internal_message: String }, - /// Method Not Allowed - #[error("Method Not Allowed: {internal_message}")] - MethodNotAllowed { internal_message: String }, + + /// There is insufficient capacity to perform the requested operation. + /// + /// This variant is translated to 507 Insufficient Storage, and it carries + /// both an external and an internal message. The external message is + /// intended for operator consumption and is intended to not leak any + /// implementation details. + #[error("Insufficient Capacity: {}", .message.display_internal())] + InsufficientCapacity { message: MessagePair }, #[error("Type version mismatch! {internal_message}")] TypeVersionMismatch { internal_message: String }, - #[error("Conflict: {internal_message}")] - Conflict { internal_message: String }, + #[error("Conflict: {}", .message.display_internal())] + Conflict { message: MessagePair }, +} + +/// Represents an error message which has an external component, along with +/// some internal context possibly attached to it. +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] +pub struct MessagePair { + external_message: String, + internal_context: String, +} + +impl MessagePair { + pub fn new(external_message: String) -> Self { + Self { external_message, internal_context: String::new() } + } + + pub fn new_full( + external_message: String, + internal_context: String, + ) -> Self { + Self { external_message, internal_context } + } + + pub fn external_message(&self) -> &str { + &self.external_message + } + + pub fn internal_context(&self) -> &str { + &self.internal_context + } + + fn with_internal_context(self, context: C) -> Self + where + C: Display + Send + Sync + 'static, + { + let internal_context = if self.internal_context.is_empty() { + context.to_string() + } else { + format!("{}: {}", context, self.internal_context) + }; + Self { external_message: self.external_message, internal_context } + } + + pub fn into_internal_external(self) -> (String, String) { + let internal = self.display_internal().to_string(); + (internal, self.external_message) + } + + // Do not implement `fmt::Display` for this enum because we don't want users to + // accidentally display the internal message to the client. Instead, use a + // private formatter. + fn display_internal(&self) -> MessagePairDisplayInternal<'_> { + MessagePairDisplayInternal(self) + } +} + +struct MessagePairDisplayInternal<'a>(&'a MessagePair); + +impl<'a> Display for MessagePairDisplayInternal<'a> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0.external_message)?; + if !self.0.internal_context.is_empty() { + write!(f, " (with internal context: {})", self.0.internal_context)?; + } + Ok(()) + } } /// Indicates how an object was looked up (for an `ObjectNotFound` error) @@ -119,7 +190,7 @@ impl Error { | Error::InvalidRequest { .. } | Error::InvalidValue { .. } | Error::Forbidden - | Error::MethodNotAllowed { .. } + | Error::InsufficientCapacity { .. } | Error::InternalError { .. } | Error::TypeVersionMismatch { .. } | Error::Conflict { .. } => false, @@ -151,8 +222,20 @@ impl Error { /// /// This should be used for failures due possibly to invalid client input /// or malformed requests. - pub fn invalid_request(message: &str) -> Error { - Error::InvalidRequest { message: message.to_owned() } + pub fn invalid_request(message: impl Into) -> Error { + Error::InvalidRequest { message: MessagePair::new(message.into()) } + } + + /// Generates an [`Error::InvalidValue`] error with the specific label and + /// message. + pub fn invalid_value( + label: impl Into, + message: impl Into, + ) -> Error { + Error::InvalidValue { + label: label.into(), + message: MessagePair::new(message.into()), + } } /// Generates an [`Error::ServiceUnavailable`] error with the specific @@ -166,6 +249,27 @@ impl Error { Error::ServiceUnavailable { internal_message: message.to_owned() } } + /// Generates an [`Error::InsufficientCapacity`] error with external and + /// and internal messages. + /// + /// This should be used for failures where there is insufficient capacity, + /// and where the caller must either take action or wait until capacity is + /// freed. + /// + /// In the future, we may want to provide more help here: e.g. a link to a + /// status or support page. + pub fn insufficient_capacity( + external_message: impl Into, + internal_message: impl Into, + ) -> Error { + Error::InsufficientCapacity { + message: MessagePair::new_full( + external_message.into(), + internal_message.into(), + ), + } + } + /// Generates an [`Error::TypeVersionMismatch`] with a specific message. /// /// TypeVersionMismatch errors are a specific type of error arising from differences @@ -186,8 +290,8 @@ impl Error { /// retried. The internal message should provide more information about the /// source of the conflict and possible actions the caller can take to /// resolve it (if any). - pub fn conflict(message: &str) -> Error { - Error::Conflict { internal_message: message.to_owned() } + pub fn conflict(message: impl Into) -> Error { + Error::Conflict { message: MessagePair::new(message.into()) } } /// Given an [`Error`] with an internal message, return the same error with @@ -201,9 +305,14 @@ impl Error { match self { Error::ObjectNotFound { .. } | Error::ObjectAlreadyExists { .. } - | Error::InvalidRequest { .. } - | Error::InvalidValue { .. } | Error::Forbidden => self, + Error::InvalidRequest { message } => Error::InvalidRequest { + message: message.with_internal_context(context), + }, + Error::InvalidValue { label, message } => Error::InvalidValue { + label, + message: message.with_internal_context(context), + }, Error::Unauthenticated { internal_message } => { Error::Unauthenticated { internal_message: format!( @@ -223,12 +332,9 @@ impl Error { ), } } - Error::MethodNotAllowed { internal_message } => { - Error::MethodNotAllowed { - internal_message: format!( - "{}: {}", - context, internal_message - ), + Error::InsufficientCapacity { message } => { + Error::InsufficientCapacity { + message: message.with_internal_context(context), } } Error::TypeVersionMismatch { internal_message } => { @@ -239,8 +345,8 @@ impl Error { ), } } - Error::Conflict { internal_message } => Error::Conflict { - internal_message: format!("{}: {}", context, internal_message), + Error::Conflict { message } => Error::Conflict { + message: message.with_internal_context(context), }, } } @@ -292,28 +398,29 @@ impl From for HttpError { internal_message, }, - Error::InvalidRequest { message } => HttpError::for_bad_request( - Some(String::from("InvalidRequest")), - message, - ), - - Error::InvalidValue { label, message } => { - let message = - format!("unsupported value for \"{}\": {}", label, message); - HttpError::for_bad_request( - Some(String::from("InvalidValue")), - message, - ) + Error::InvalidRequest { message } => { + let (internal_message, external_message) = + message.into_internal_external(); + HttpError { + status_code: http::StatusCode::BAD_REQUEST, + error_code: Some(String::from("InvalidRequest")), + external_message, + internal_message, + } } - // TODO: RFC-7231 requires that 405s generate an Accept header to describe - // what methods are available in the response - Error::MethodNotAllowed { internal_message } => { - HttpError::for_client_error( - Some(String::from("MethodNotAllowed")), - http::StatusCode::METHOD_NOT_ALLOWED, + Error::InvalidValue { label, message } => { + let (internal_message, external_message) = + message.into_internal_external(); + HttpError { + status_code: http::StatusCode::BAD_REQUEST, + error_code: Some(String::from("InvalidValue")), + external_message: format!( + "unsupported value for \"{}\": {}", + label, external_message + ), internal_message, - ) + } } Error::Forbidden => HttpError::for_client_error( @@ -333,16 +440,35 @@ impl From for HttpError { ) } + Error::InsufficientCapacity { message } => { + let (internal_message, external_message) = + message.into_internal_external(); + // Need to construct an `HttpError` explicitly to present both + // an internal and an external message. + HttpError { + status_code: http::StatusCode::INSUFFICIENT_STORAGE, + error_code: Some(String::from("InsufficientCapacity")), + external_message: format!( + "Insufficient capacity: {}", + external_message + ), + internal_message, + } + } + Error::TypeVersionMismatch { internal_message } => { HttpError::for_internal_error(internal_message) } - Error::Conflict { internal_message } => { - HttpError::for_client_error( - Some(String::from("Conflict")), - http::StatusCode::CONFLICT, + Error::Conflict { message } => { + let (internal_message, external_message) = + message.into_internal_external(); + HttpError { + status_code: http::StatusCode::CONFLICT, + error_code: Some(String::from("Conflict")), + external_message, internal_message, - ) + } } } } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 50516a5da4..a6d729593b 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -316,10 +316,7 @@ impl Name { /// `Name::try_from(String)` that marshals any error into an appropriate /// `Error`. pub fn from_param(value: String, label: &str) -> Result { - value.parse().map_err(|e| Error::InvalidValue { - label: String::from(label), - message: e, - }) + value.parse().map_err(|e| Error::invalid_value(label, e)) } /// Return the `&str` representing the actual name. @@ -2828,10 +2825,10 @@ mod test { assert!(result.is_err()); assert_eq!( result, - Err(Error::InvalidValue { - label: "the_name".to_string(), - message: "name requires at least one character".to_string() - }) + Err(Error::invalid_value( + "the_name", + "name requires at least one character" + )) ); } diff --git a/common/src/vlan.rs b/common/src/vlan.rs index 45776e09ac..5e5765ffe2 100644 --- a/common/src/vlan.rs +++ b/common/src/vlan.rs @@ -20,10 +20,10 @@ impl VlanID { /// Creates a new VLAN ID, returning an error if it is out of range. pub fn new(id: u16) -> Result { if VLAN_MAX < id { - return Err(Error::InvalidValue { - label: id.to_string(), - message: "Invalid VLAN value".to_string(), - }); + return Err(Error::invalid_value( + id.to_string(), + "Invalid VLAN value", + )); } Ok(Self(id)) } @@ -38,9 +38,9 @@ impl fmt::Display for VlanID { impl FromStr for VlanID { type Err = Error; fn from_str(s: &str) -> Result { - Self::new(s.parse().map_err(|e| Error::InvalidValue { - label: s.to_string(), - message: format!("{}", e), - })?) + Self::new( + s.parse::() + .map_err(|e| Error::invalid_value(s, e.to_string()))?, + ) } } diff --git a/docs/http-status-codes.adoc b/docs/http-status-codes.adoc index e02f9ea8a5..4628edcab5 100644 --- a/docs/http-status-codes.adoc +++ b/docs/http-status-codes.adoc @@ -18,7 +18,8 @@ This doc is aimed at the public API. For consistency, we should use the same er ** "403 Forbidden" is used when the user provided valid credentials (they were authenticated), but they're not authorized to access the resource, _and_ we don't mind telling them that the resource exists (e.g., accessing "/sleds"). ** "404 Not Found" is used when the user provided valid credentials (they were authenticated), but they're not authorized to access the resource, and they're not even allowed to know whether it exists (e.g., accessing a particular Project). * "500 Internal Server Error" is used for any kind of _bug_ or unhandled server-side condition. -* "503 Service unavailable" is used when the service (or an internal service on which the service depends) is overloaded or actually unavailable. +* "503 Service Unavailable" is used when the service (or an internal service on which the service depends) is overloaded or actually unavailable. +* "507 Insufficient Storage" is used if there isn't sufficient capacity available for a particular operation (for example, if there isn't enough disk space available to allocate a new virtual disk). There's more discussion about the 400-level and 500-level codes below. diff --git a/nexus/db-model/src/instance_state.rs b/nexus/db-model/src/instance_state.rs index 6baec7afbd..6b4c71da79 100644 --- a/nexus/db-model/src/instance_state.rs +++ b/nexus/db-model/src/instance_state.rs @@ -6,6 +6,7 @@ use super::impl_enum_wrapper; use omicron_common::api::external; use serde::Deserialize; use serde::Serialize; +use std::fmt; use std::io::Write; impl_enum_wrapper!( @@ -40,6 +41,12 @@ impl InstanceState { } } +impl fmt::Display for InstanceState { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.0) + } +} + impl From for sled_agent_client::types::InstanceState { fn from(s: InstanceState) -> Self { use external::InstanceState::*; diff --git a/nexus/db-model/src/semver_version.rs b/nexus/db-model/src/semver_version.rs index 966b436149..8e168e11a2 100644 --- a/nexus/db-model/src/semver_version.rs +++ b/nexus/db-model/src/semver_version.rs @@ -68,12 +68,10 @@ fn to_sortable_string(v: semver::Version) -> Result { let max = u64::pow(10, u32::from(PADDED_WIDTH)) - 1; if v.major > max || v.minor > max || v.patch > max { - return Err(external::Error::InvalidValue { - label: "version".to_string(), - message: format!( - "Major, minor, and patch version must be less than {max}" - ), - }); + return Err(external::Error::invalid_value( + "version", + format!("Major, minor, and patch version must be less than {max}"), + )); } let mut result = format!( diff --git a/nexus/db-queries/src/db/datastore/disk.rs b/nexus/db-queries/src/db/datastore/disk.rs index 26d439b350..94d950f86a 100644 --- a/nexus/db-queries/src/db/datastore/disk.rs +++ b/nexus/db-queries/src/db/datastore/disk.rs @@ -633,16 +633,12 @@ impl DataStore { // destroyed, don't throw an error. return Ok(disk); } else if !ok_to_delete_states.contains(disk_state.state()) { - return Err(Error::InvalidRequest { - message: format!( - "disk cannot be deleted in state \"{}\"", - disk.runtime_state.disk_state - ), - }); + return Err(Error::invalid_request(format!( + "disk cannot be deleted in state \"{}\"", + disk.runtime_state.disk_state + ))); } else if disk_state.is_attached() { - return Err(Error::InvalidRequest { - message: String::from("disk is attached"), - }); + return Err(Error::invalid_request("disk is attached")); } else { // NOTE: This is a "catch-all" error case, more specific // errors should be preferred as they're more actionable. diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index e821082501..ddf396f871 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -219,9 +219,12 @@ impl DataStore { "Requested external IP address not available", )) } else { - TransactionError::CustomError(Error::invalid_request( - "No external IP addresses available", - )) + TransactionError::CustomError( + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), + ) } } DatabaseError(UniqueViolation, ..) if name.is_some() => { @@ -450,10 +453,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(Error::InvalidRequest { - message: "deletion failed due to concurrent modification" - .to_string(), - }); + return Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )); } Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index fb300ef833..4497e3f2b4 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -194,11 +194,9 @@ impl DataStore { .optional() .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if range.is_some() { - return Err(Error::InvalidRequest { - message: - "IP Pool cannot be deleted while it contains IP ranges" - .to_string(), - }); + return Err(Error::invalid_request( + "IP Pool cannot be deleted while it contains IP ranges", + )); } // Delete the pool, conditional on the rcgen not having changed. This @@ -224,10 +222,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(Error::InvalidRequest { - message: "deletion failed due to concurrent modification" - .to_string(), - }); + return Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )); } Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs b/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs index 1caf5617bb..a44fed4cdf 100644 --- a/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs +++ b/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs @@ -124,9 +124,7 @@ impl DataStore { if let Some(nat_entry) = result.first() { Ok(nat_entry.clone()) } else { - Err(Error::InvalidRequest { - message: "no matching records".to_string(), - }) + Err(Error::invalid_request("no matching records")) } } @@ -185,9 +183,7 @@ impl DataStore { if let Some(nat_entry) = result.first() { Ok(nat_entry.clone()) } else { - Err(Error::InvalidRequest { - message: "no matching records".to_string(), - }) + Err(Error::invalid_request("no matching records")) } } @@ -241,9 +237,7 @@ impl DataStore { match latest { Some(value) => Ok(value), - None => Err(Error::InvalidRequest { - message: "sequence table is empty!".to_string(), - }), + None => Err(Error::invalid_request("sequence table is empty!")), } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 2844285f40..761c3f995f 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -1046,7 +1046,7 @@ mod test { "Saw error: \'{err}\', but expected \'{expected}\'" ); - assert!(matches!(err, Error::ServiceUnavailable { .. })); + assert!(matches!(err, Error::InsufficientCapacity { .. })); } let _ = db.cleanup().await; @@ -1191,7 +1191,7 @@ mod test { "Saw error: \'{err}\', but expected \'{expected}\'" ); - assert!(matches!(err, Error::ServiceUnavailable { .. })); + assert!(matches!(err, Error::InsufficientCapacity { .. })); let _ = db.cleanup().await; logctx.cleanup_successful(); diff --git a/nexus/db-queries/src/db/datastore/project.rs b/nexus/db-queries/src/db/datastore/project.rs index ba0c64abfd..a9015ea943 100644 --- a/nexus/db-queries/src/db/datastore/project.rs +++ b/nexus/db-queries/src/db/datastore/project.rs @@ -78,9 +78,9 @@ macro_rules! generate_fn_to_ensure_none_in_project { "a" }; - return Err(Error::InvalidRequest { - message: format!("project to be deleted contains {article} {object}: {label}"), - }); + return Err(Error::invalid_request( + format!("project to be deleted contains {article} {object}: {label}") + )); } Ok(()) @@ -271,11 +271,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(err.bail(Error::InvalidRequest { - message: - "deletion failed due to concurrent modification" - .to_string(), - })); + return Err(err.bail(Error::invalid_request( + "deletion failed due to concurrent modification", + ))); } self.virtual_provisioning_collection_delete_on_connection( diff --git a/nexus/db-queries/src/db/datastore/saga.rs b/nexus/db-queries/src/db/datastore/saga.rs index 2ec0c40799..1cd41a9806 100644 --- a/nexus/db-queries/src/db/datastore/saga.rs +++ b/nexus/db-queries/src/db/datastore/saga.rs @@ -87,8 +87,8 @@ impl DataStore { match result.status { UpdateStatus::Updated => Ok(()), - UpdateStatus::NotUpdatedButExists => Err(Error::InvalidRequest { - message: format!( + UpdateStatus::NotUpdatedButExists => Err(Error::invalid_request( + format!( "failed to update saga {:?} with state {:?}: preconditions not met: \ expected current_sec = {:?}, adopt_generation = {:?}, \ but found current_sec = {:?}, adopt_generation = {:?}, state = {:?}", @@ -100,7 +100,7 @@ impl DataStore { result.found.adopt_generation, result.found.saga_state, ) - }), + )), } } diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index ab48ec458f..437c171fb0 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -351,9 +351,9 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if project_found.is_some() { - return Err(Error::InvalidRequest { - message: "silo to be deleted contains a project".to_string(), - }); + return Err(Error::invalid_request( + "silo to be deleted contains a project", + )); } let now = Utc::now(); @@ -375,11 +375,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(TxnError::CustomError(Error::InvalidRequest { - message: - "silo deletion failed due to concurrent modification" - .to_string(), - })); + return Err(TxnError::CustomError(Error::invalid_request( + "silo deletion failed due to concurrent modification", + ))); } self.virtual_provisioning_collection_delete_on_connection( diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 023384a9bf..7b94d64418 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -217,8 +217,10 @@ impl DataStore { if let Some(err) = err.take() { match err { SledReservationError::NotFound => { - return external::Error::unavail( + return external::Error::insufficient_capacity( "No sleds can fit the requested instance", + "No sled targets found that had enough \ + capacity to fit the requested instance.", ); } } @@ -399,7 +401,7 @@ mod test { ) .await .unwrap_err(); - assert!(matches!(error, external::Error::ServiceUnavailable { .. })); + assert!(matches!(error, external::Error::InsufficientCapacity { .. })); // Now add a provisionable sled and try again. let sled_update = test_new_sled_update(); diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 069ce63028..4f0245e283 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -339,7 +339,10 @@ impl DataStore { opctx.log, "failed to find a VNI after searching entire range"; ); - Err(Error::unavail("Failed to find a free VNI for this VPC")) + Err(Error::insufficient_capacity( + "No free virtual network was found", + "Failed to find a free VNI for this VPC", + )) } // Internal implementation for creating a VPC. @@ -469,11 +472,9 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? .is_some() { - return Err(Error::InvalidRequest { - message: String::from( - "VPC cannot be deleted while VPC Subnets exist", - ), - }); + return Err(Error::invalid_request( + "VPC cannot be deleted while VPC Subnets exist", + )); } // Delete the VPC, conditional on the subnet_gen not having changed. @@ -492,11 +493,9 @@ impl DataStore { ) })?; if updated_rows == 0 { - Err(Error::InvalidRequest { - message: String::from( - "deletion failed to to concurrent modification", - ), - }) + Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )) } else { Ok(()) } @@ -794,12 +793,10 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? .is_some() { - return Err(Error::InvalidRequest { - message: String::from( - "VPC Subnet cannot be deleted while \ - network interfaces in the subnet exist", - ), - }); + return Err(Error::invalid_request( + "VPC Subnet cannot be deleted while network interfaces in the \ + subnet exist", + )); } // Delete the subnet, conditional on the rcgen not having changed. @@ -818,11 +815,9 @@ impl DataStore { ) })?; if updated_rows == 0 { - return Err(Error::InvalidRequest { - message: String::from( - "deletion failed to to concurrent modification", - ), - }); + return Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )); } else { Ok(()) } diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 4e5f59e79c..2a76ea7408 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -997,9 +997,10 @@ mod tests { ); assert_eq!( err, - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); context.success().await; } @@ -1053,9 +1054,10 @@ mod tests { ); assert_eq!( res.unwrap_err(), - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); let res = context @@ -1075,9 +1077,10 @@ mod tests { ); assert_eq!( res.unwrap_err(), - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); context.success().await; } @@ -1306,9 +1309,10 @@ mod tests { .expect_err("Should have failed to allocate after pool exhausted"); assert_eq!( err, - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); // But we should be able to allocate another SNat IP diff --git a/nexus/db-queries/src/db/queries/region_allocation.rs b/nexus/db-queries/src/db/queries/region_allocation.rs index 031be92c08..3c37bf6b2e 100644 --- a/nexus/db-queries/src/db/queries/region_allocation.rs +++ b/nexus/db-queries/src/db/queries/region_allocation.rs @@ -46,19 +46,23 @@ pub fn from_diesel(e: DieselError) -> external::Error { NOT_ENOUGH_UNIQUE_ZPOOLS_SENTINEL, ]; if let Some(sentinel) = matches_sentinel(&e, &sentinels) { + let external_message = "Not enough storage"; match sentinel { NOT_ENOUGH_DATASETS_SENTINEL => { - return external::Error::unavail( + return external::Error::insufficient_capacity( + external_message, "Not enough datasets to allocate disks", ); } NOT_ENOUGH_ZPOOL_SPACE_SENTINEL => { - return external::Error::unavail( + return external::Error::insufficient_capacity( + external_message, "Not enough zpool space to allocate disks. There may not be enough disks with space for the requested region. You may also see this if your rack is in a degraded state, or you're running the default multi-rack topology configuration in a 1-sled development environment.", ); } NOT_ENOUGH_UNIQUE_ZPOOLS_SENTINEL => { - return external::Error::unavail( + return external::Error::insufficient_capacity( + external_message, "Not enough unique zpools selected while allocating disks", ); } diff --git a/nexus/src/app/address_lot.rs b/nexus/src/app/address_lot.rs index b87ae1b09f..847021bdd4 100644 --- a/nexus/src/app/address_lot.rs +++ b/nexus/src/app/address_lot.rs @@ -94,10 +94,9 @@ fn validate_blocks(lot: ¶ms::AddressLotCreate) -> Result<(), Error> { validate_v6_block(first, last)? } _ => { - return Err(Error::InvalidRequest { - message: "Block bounds must be in same address family" - .into(), - }) + return Err(Error::invalid_request( + "Block bounds must be in same address family", + )); } } } @@ -106,18 +105,18 @@ fn validate_blocks(lot: ¶ms::AddressLotCreate) -> Result<(), Error> { fn validate_v4_block(first: &Ipv4Addr, last: &Ipv4Addr) -> Result<(), Error> { if first > last { - return Err(Error::InvalidRequest { - message: "Invalid range, first must be <= last".into(), - }); + return Err(Error::invalid_request( + "Invalid range, first must be <= last", + )); } Ok(()) } fn validate_v6_block(first: &Ipv6Addr, last: &Ipv6Addr) -> Result<(), Error> { if first > last { - return Err(Error::InvalidRequest { - message: "Invalid range, first must be <= last".into(), - }); + return Err(Error::invalid_request( + "Invalid range, first must be <= last", + )); } Ok(()) } diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index c9571ee91f..c70b339a36 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -114,9 +114,7 @@ impl super::Nexus { token, ) .await?; - Err(Error::InvalidRequest { - message: "device authorization request expired".to_string(), - }) + Err(Error::invalid_request("device authorization request expired")) } else { self.db_datastore .device_access_token_create( diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index 5cfecc9f08..5dd49a2efb 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -140,48 +140,48 @@ impl super::Nexus { // Reject disks where the block size doesn't evenly divide the // total size if (params.size.to_bytes() % block_size) != 0 { - return Err(Error::InvalidValue { - label: String::from("size and block_size"), - message: format!( + return Err(Error::invalid_value( + "size and block_size", + format!( "total size must be a multiple of block size {}", block_size, ), - }); + )); } // Reject disks where the size isn't at least // MIN_DISK_SIZE_BYTES if params.size.to_bytes() < MIN_DISK_SIZE_BYTES as u64 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "total size must be at least {}", ByteCount::from(MIN_DISK_SIZE_BYTES) ), - }); + )); } // Reject disks where the MIN_DISK_SIZE_BYTES doesn't evenly // divide the size if (params.size.to_bytes() % MIN_DISK_SIZE_BYTES as u64) != 0 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "total size must be a multiple of {}", ByteCount::from(MIN_DISK_SIZE_BYTES) ), - }); + )); } // Reject disks where the size is greated than MAX_DISK_SIZE_BYTES if params.size.to_bytes() > MAX_DISK_SIZE_BYTES { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "total size must be less than {}", ByteCount::try_from(MAX_DISK_SIZE_BYTES).unwrap() ), - }); + )); } Ok(()) diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index f95c64d3eb..1ab33c5c9c 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -1539,7 +1539,7 @@ mod test { Err(Error::InvalidRequest { message }) => { assert_eq!(rx_label, "empty"); assert_eq!( - message, + message.external_message(), format!( "HTTP request for unknown server name {:?}", authority.host() diff --git a/nexus/src/app/image.rs b/nexus/src/app/image.rs index 5e78b2a096..a7fe75a464 100644 --- a/nexus/src/app/image.rs +++ b/nexus/src/app/image.rs @@ -168,9 +168,11 @@ impl super::Nexus { // disk created from this image has to be larger than it. let size: u64 = 100 * 1024 * 1024; let size: external::ByteCount = - size.try_into().map_err(|e| Error::InvalidValue { - label: String::from("size"), - message: format!("size is invalid: {}", e), + size.try_into().map_err(|e| { + Error::invalid_value( + "size", + format!("size is invalid: {}", e), + ) })?; let new_image_volume = @@ -293,9 +295,9 @@ impl super::Nexus { ) .await } - ImageLookup::SiloImage(_) => Err(Error::InvalidRequest { - message: "Cannot promote a silo image".to_string(), - }), + ImageLookup::SiloImage(_) => { + Err(Error::invalid_request("Cannot promote a silo image")) + } } } @@ -321,9 +323,9 @@ impl super::Nexus { ) .await } - ImageLookup::ProjectImage(_) => Err(Error::InvalidRequest { - message: "Cannot demote a project image".to_string(), - }), + ImageLookup::ProjectImage(_) => { + Err(Error::invalid_request("Cannot demote a project image")) + } } } } diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 0edb2c5ea7..987a8ac794 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -211,13 +211,13 @@ impl super::Nexus { // Reject instances where the memory is not at least // MIN_MEMORY_BYTES_PER_INSTANCE if params.memory.to_bytes() < MIN_MEMORY_BYTES_PER_INSTANCE as u64 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "memory must be at least {}", ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE) ), - }); + )); } // Reject instances where the memory is not divisible by @@ -225,24 +225,24 @@ impl super::Nexus { if (params.memory.to_bytes() % MIN_MEMORY_BYTES_PER_INSTANCE as u64) != 0 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "memory must be divisible by {}", ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE) ), - }); + )); } // Reject instances where the memory is greater than the limit if params.memory.to_bytes() > MAX_MEMORY_BYTES_PER_INSTANCE { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "memory must be less than or equal to {}", ByteCount::try_from(MAX_MEMORY_BYTES_PER_INSTANCE).unwrap() ), - }); + )); } let saga_params = sagas::instance_create::Params { @@ -376,7 +376,7 @@ impl super::Nexus { } if instance.runtime().migration_id.is_some() { - return Err(Error::unavail("instance is already migrating")); + return Err(Error::conflict("instance is already migrating")); } // Kick off the migration saga @@ -785,12 +785,10 @@ impl super::Nexus { if allowed { Ok(InstanceStateChangeRequestAction::SendToSled(sled_id)) } else { - Err(Error::InvalidRequest { - message: format!( - "instance state cannot be changed from state \"{}\"", - effective_state - ), - }) + Err(Error::invalid_request(format!( + "instance state cannot be changed from state \"{}\"", + effective_state + ))) } } @@ -1231,10 +1229,9 @@ impl super::Nexus { // permissions on both) without verifying the shared hierarchy. To // mitigate that we verify that their parent projects have the same ID. if authz_project.id() != authz_project_disk.id() { - return Err(Error::InvalidRequest { - message: "disk must be in the same project as the instance" - .to_string(), - }); + return Err(Error::invalid_request( + "disk must be in the same project as the instance", + )); } // TODO(https://github.com/oxidecomputer/omicron/issues/811): @@ -1614,28 +1611,22 @@ impl super::Nexus { | InstanceState::Starting | InstanceState::Stopping | InstanceState::Stopped - | InstanceState::Failed => Err(Error::ServiceUnavailable { - internal_message: format!( - "cannot connect to serial console of instance in state \ - {:?}", - vmm.runtime.state.0 - ), - }), - InstanceState::Destroyed => Err(Error::ServiceUnavailable { - internal_message: format!( - "cannot connect to serial console of instance in state \ - {:?}", - InstanceState::Stopped), - }), + | InstanceState::Failed => { + Err(Error::invalid_request(format!( + "cannot connect to serial console of instance in state \"{}\"", + vmm.runtime.state.0, + ))) + } + InstanceState::Destroyed => Err(Error::invalid_request( + "cannot connect to serial console of destroyed instance", + )), } } else { - Err(Error::ServiceUnavailable { - internal_message: format!( - "instance is in state {:?} and has no active serial console \ + Err(Error::invalid_request(format!( + "instance is {} and has no active serial console \ server", - instance.runtime().nexus_state - ) - }) + instance.runtime().nexus_state + ))) } } diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 3804841feb..1643ac301d 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -211,10 +211,9 @@ impl super::Nexus { }; let rack_network_config = request.rack_network_config.as_ref().ok_or( - Error::InvalidRequest { - message: "cannot initialize a rack without a network config" - .into(), - }, + Error::invalid_request( + "cannot initialize a rack without a network config", + ), )?; self.db_datastore diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 891124e1ac..7adf1c9bdd 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -154,7 +154,7 @@ impl super::Nexus { | Error::Forbidden | Error::InternalError { .. } | Error::ServiceUnavailable { .. } - | Error::MethodNotAllowed { .. } + | Error::InsufficientCapacity { .. } | Error::TypeVersionMismatch { .. } | Error::Conflict { .. } => { Reason::UnknownError { source: error } diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index a6ffd8ef5e..f5f3fa00e7 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -822,25 +822,24 @@ impl super::Nexus { })?; let response = client.get(url).send().await.map_err(|e| { - Error::InvalidValue { - label: String::from("url"), - message: format!("error querying url: {}", e), - } + Error::invalid_value( + "url", + format!("error querying url: {e}"), + ) })?; if !response.status().is_success() { - return Err(Error::InvalidValue { - label: String::from("url"), - message: format!( - "querying url returned: {}", - response.status() - ), - }); + return Err(Error::invalid_value( + "url", + format!("querying url returned: {}", response.status()), + )); } - response.text().await.map_err(|e| Error::InvalidValue { - label: String::from("url"), - message: format!("error getting text from url: {}", e), + response.text().await.map_err(|e| { + Error::invalid_value( + "url", + format!("error getting text from url: {e}"), + ) })? } @@ -849,12 +848,11 @@ impl super::Nexus { &base64::engine::general_purpose::STANDARD, data, ) - .map_err(|e| Error::InvalidValue { - label: String::from("data"), - message: format!( - "error getting decoding base64 data: {}", - e - ), + .map_err(|e| { + Error::invalid_value( + "data", + format!("error getting decoding base64 data: {e}"), + ) })?; String::from_utf8_lossy(&bytes).into_owned() } diff --git a/nexus/src/app/switch_interface.rs b/nexus/src/app/switch_interface.rs index cfb0541742..0acb2b7fe7 100644 --- a/nexus/src/app/switch_interface.rs +++ b/nexus/src/app/switch_interface.rs @@ -95,9 +95,9 @@ impl super::Nexus { pub fn validate_switch_location(switch_location: &str) -> Result<(), Error> { if switch_location != "switch0" && switch_location != "switch1" { - return Err(Error::InvalidRequest { - message: "Switch location must be switch0 or switch1".into(), - }); + return Err(Error::invalid_request( + "Switch location must be switch0 or switch1", + )); } Ok(()) } diff --git a/nexus/src/app/update/mod.rs b/nexus/src/app/update/mod.rs index 5075e421ae..36d4dbcb9e 100644 --- a/nexus/src/app/update/mod.rs +++ b/nexus/src/app/update/mod.rs @@ -68,14 +68,10 @@ impl super::Nexus { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; let updates_config = self.updates_config.as_ref().ok_or_else(|| { - Error::InvalidRequest { - message: "updates system not configured".into(), - } + Error::invalid_request("updates system not configured") })?; let base_url = self.tuf_base_url(opctx).await?.ok_or_else(|| { - Error::InvalidRequest { - message: "updates system not configured".into(), - } + Error::invalid_request("updates system not configured") })?; let trusted_root = tokio::fs::read(&updates_config.trusted_root) .await @@ -158,9 +154,7 @@ impl super::Nexus { ) -> Result, Error> { let mut base_url = self.tuf_base_url(opctx).await?.ok_or_else(|| { - Error::InvalidRequest { - message: "updates system not configured".into(), - } + Error::invalid_request("updates system not configured") })?; if !base_url.ends_with('/') { base_url.push('/'); diff --git a/nexus/src/app/vpc_router.rs b/nexus/src/app/vpc_router.rs index 81577f88e8..523a450bbd 100644 --- a/nexus/src/app/vpc_router.rs +++ b/nexus/src/app/vpc_router.rs @@ -129,9 +129,7 @@ impl super::Nexus { // router kind cannot be changed, but it might be able to save us a // database round-trip. if db_router.kind == VpcRouterKind::System { - return Err(Error::MethodNotAllowed { - internal_message: "Cannot delete system router".to_string(), - }); + return Err(Error::invalid_request("Cannot delete system router")); } self.db_datastore.vpc_delete_router(opctx, &authz_router).await } @@ -229,14 +227,12 @@ impl super::Nexus { match db_route.kind.0 { RouterRouteKind::Custom | RouterRouteKind::Default => (), _ => { - return Err(Error::MethodNotAllowed { - internal_message: format!( - "routes of type {} from the system table of VPC {:?} \ + return Err(Error::invalid_request(format!( + "routes of type {} from the system table of VPC {:?} \ are not modifiable", - db_route.kind.0, - vpc.id() - ), - }) + db_route.kind.0, + vpc.id() + ))); } } self.db_datastore @@ -255,10 +251,9 @@ impl super::Nexus { // Only custom routes can be deleted // TODO Shouldn't this constraint be checked by the database query? if db_route.kind.0 != RouterRouteKind::Custom { - return Err(Error::MethodNotAllowed { - internal_message: "DELETE not allowed on system routes" - .to_string(), - }); + return Err(Error::invalid_request( + "DELETE not allowed on system routes", + )); } self.db_datastore.router_delete_route(opctx, &authz_route).await } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index a2e5f633df..a6fd7a3ccb 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -5398,10 +5398,7 @@ async fn role_list( WhichPage::First(..) => None, WhichPage::Next(RolePage { last_seen }) => { Some(last_seen.split_once('.').ok_or_else(|| { - Error::InvalidValue { - label: last_seen.clone(), - message: String::from("bad page token"), - } + Error::invalid_value(last_seen.clone(), "bad page token") })?) .map(|(s1, s2)| (s1.to_string(), s2.to_string())) } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index f7403275b1..807c054b64 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -992,7 +992,7 @@ async fn test_disk_backed_by_multiple_region_sets( .body(Some(&new_disk)) // TODO: this fails! the current allocation algorithm does not split // across datasets - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -1026,7 +1026,7 @@ async fn test_disk_too_big(cptestctx: &ControlPlaneTestContext) { NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&new_disk)) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -1457,7 +1457,7 @@ async fn test_disk_size_accounting(cptestctx: &ControlPlaneTestContext) { NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&disk_two)) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 33d4d15d23..9260006c81 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3190,7 +3190,7 @@ async fn test_instances_memory_greater_than_max_size( assert!(error.message.contains("memory must be less than")); } -async fn expect_instance_start_fail_unavailable( +async fn expect_instance_start_fail_507( client: &ClientTestContext, instance_name: &str, ) { @@ -3199,13 +3199,15 @@ async fn expect_instance_start_fail_unavailable( http::Method::POST, &get_instance_start_url(instance_name), ) - .expect_status(Some(http::StatusCode::SERVICE_UNAVAILABLE)); + .expect_status(Some(http::StatusCode::INSUFFICIENT_STORAGE)); NexusRequest::new(builder) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .expect("Expected instance start to fail with SERVICE_UNAVAILABLE"); + .expect( + "Expected instance start to fail with 507 Insufficient Storage", + ); } async fn expect_instance_start_ok( @@ -3296,9 +3298,7 @@ async fn test_cannot_provision_instance_beyond_cpu_capacity( for config in &configs { match config.2 { Ok(_) => expect_instance_start_ok(client, config.0).await, - Err(_) => { - expect_instance_start_fail_unavailable(client, config.0).await - } + Err(_) => expect_instance_start_fail_507(client, config.0).await, } } @@ -3404,9 +3404,7 @@ async fn test_cannot_provision_instance_beyond_ram_capacity( for config in &configs { match config.2 { Ok(_) => expect_instance_start_ok(client, config.0).await, - Err(_) => { - expect_instance_start_fail_unavailable(client, config.0).await - } + Err(_) => expect_instance_start_fail_507(client, config.0).await, } } diff --git a/nexus/tests/integration_tests/router_routes.rs b/nexus/tests/integration_tests/router_routes.rs index 7a7a33d49d..10c594bba9 100644 --- a/nexus/tests/integration_tests/router_routes.rs +++ b/nexus/tests/integration_tests/router_routes.rs @@ -69,7 +69,7 @@ async fn test_router_routes(cptestctx: &ControlPlaneTestContext) { // It errors if you try to delete the default route let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure( client, - StatusCode::METHOD_NOT_ALLOWED, + StatusCode::BAD_REQUEST, Method::DELETE, get_route_url("system", "default").as_str(), ) diff --git a/nexus/tests/integration_tests/snapshots.rs b/nexus/tests/integration_tests/snapshots.rs index a9ed1b7cb7..24b04bf718 100644 --- a/nexus/tests/integration_tests/snapshots.rs +++ b/nexus/tests/integration_tests/snapshots.rs @@ -793,7 +793,7 @@ async fn test_cannot_snapshot_if_no_space(cptestctx: &ControlPlaneTestContext) { }, disk: base_disk_name.into(), })) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 5454e1f68f..466cb5472e 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -363,7 +363,7 @@ async fn test_snapshot_prevents_other_disk( NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&next_disk)) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/sled-agent/src/common/disk.rs b/sled-agent/src/common/disk.rs index 57868937d0..54c56825eb 100644 --- a/sled-agent/src/common/disk.rs +++ b/sled-agent/src/common/disk.rs @@ -118,12 +118,10 @@ impl DiskStates { | DiskState::ImportingFromBulkWrites | DiskState::Destroyed | DiskState::Faulted => { - return Err(Error::InvalidRequest { - message: format!( - "cannot detach from {}", - self.current.disk_state - ), - }); + return Err(Error::invalid_request(format!( + "cannot detach from {}", + self.current.disk_state + ))); } }; } @@ -134,9 +132,9 @@ impl DiskStates { // (which is a no-op anyway). DiskState::Attaching(id) | DiskState::Attached(id) => { if uuid != id { - return Err(Error::InvalidRequest { - message: "disk is already attached".to_string(), - }); + return Err(Error::invalid_request( + "disk is already attached", + )); } return Ok(None); } @@ -157,12 +155,10 @@ impl DiskStates { | DiskState::Detaching(_) | DiskState::Destroyed | DiskState::Faulted => { - return Err(Error::InvalidRequest { - message: format!( - "cannot attach from {}", - self.current.disk_state - ), - }); + return Err(Error::invalid_request(format!( + "cannot attach from {}", + self.current.disk_state + ))); } } } diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index a811678a48..057402c57a 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -863,13 +863,11 @@ impl Instance { } return Err(Error::Transition( - omicron_common::api::external::Error::Conflict { - internal_message: format!( - "wrong instance state generation: expected {}, got {}", - inner.state.instance().gen, - old_runtime.gen - ), - }, + omicron_common::api::external::Error::conflict(format!( + "wrong instance state generation: expected {}, got {}", + inner.state.instance().gen, + old_runtime.gen + )), )); } diff --git a/sled-agent/src/sim/collection.rs b/sled-agent/src/sim/collection.rs index bd6ed4aa90..8dae31863c 100644 --- a/sled-agent/src/sim/collection.rs +++ b/sled-agent/src/sim/collection.rs @@ -777,7 +777,7 @@ mod test { let error = disk.transition(DiskStateRequested::Attached(id2)).unwrap_err(); if let Error::InvalidRequest { message } = error { - assert_eq!("disk is already attached", message); + assert_eq!("disk is already attached", message.external_message()); } else { panic!("unexpected error type"); } @@ -829,7 +829,10 @@ mod test { let error = disk.transition(DiskStateRequested::Attached(id)).unwrap_err(); if let Error::InvalidRequest { message } = error { - assert_eq!("cannot attach from detaching", message); + assert_eq!( + "cannot attach from detaching", + message.external_message() + ); } else { panic!("unexpected error type"); } diff --git a/sled-agent/src/sim/instance.rs b/sled-agent/src/sim/instance.rs index 15ff83c969..8b00adce60 100644 --- a/sled-agent/src/sim/instance.rs +++ b/sled-agent/src/sim/instance.rs @@ -362,13 +362,11 @@ impl SimInstanceInner { } if self.state.instance().gen != old_runtime.gen { - return Err(Error::InvalidRequest { - message: format!( - "wrong Propolis ID generation: expected {}, got {}", - self.state.instance().gen, - old_runtime.gen - ), - }); + return Err(Error::invalid_request(format!( + "wrong Propolis ID generation: expected {}, got {}", + self.state.instance().gen, + old_runtime.gen + ))); } self.state.set_migration_ids(ids, Utc::now()); From ed3671ad082d1b37360a71923c24eb146962930f Mon Sep 17 00:00:00 2001 From: "oxide-renovate[bot]" <146848827+oxide-renovate[bot]@users.noreply.github.com> Date: Sat, 9 Dec 2023 05:41:27 +0000 Subject: [PATCH 05/11] Update Rust crate tokio to 1.35.0 (#4661) --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- workspace-hack/Cargo.toml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 67e1d3784c..8ff57cd451 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8504,9 +8504,9 @@ dependencies = [ [[package]] name = "tokio" -version = "1.34.0" +version = "1.35.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d0c014766411e834f7af5b8f4cf46257aab4036ca95e9d2c144a10f59ad6f5b9" +checksum = "841d45b238a16291a4e1584e61820b8ae57d696cc5015c459c229ccc6990cc1c" dependencies = [ "backtrace", "bytes", diff --git a/Cargo.toml b/Cargo.toml index f8d2a07977..0320962452 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -364,7 +364,7 @@ textwrap = "0.16.0" test-strategy = "0.3.1" thiserror = "1.0" tofino = { git = "http://github.com/oxidecomputer/tofino", branch = "main" } -tokio = "1.34.0" +tokio = "1.35.0" tokio-postgres = { version = "0.7", features = [ "with-chrono-0_4", "with-uuid-1" ] } tokio-stream = "0.1.14" tokio-tungstenite = "0.20" diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index ce65ddf062..88cadda842 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -98,7 +98,7 @@ subtle = { version = "2.5.0" } syn-dff4ba8e3ae991db = { package = "syn", version = "1.0.109", features = ["extra-traits", "fold", "full", "visit"] } syn-f595c2ba2a3f28df = { package = "syn", version = "2.0.32", features = ["extra-traits", "fold", "full", "visit", "visit-mut"] } time = { version = "0.3.27", features = ["formatting", "local-offset", "macros", "parsing"] } -tokio = { version = "1.34.0", features = ["full", "test-util"] } +tokio = { version = "1.35.0", features = ["full", "test-util"] } tokio-postgres = { version = "0.7.10", features = ["with-chrono-0_4", "with-serde_json-1", "with-uuid-1"] } tokio-stream = { version = "0.1.14", features = ["net"] } tokio-util = { version = "0.7.10", features = ["codec", "io-util"] } @@ -200,7 +200,7 @@ syn-dff4ba8e3ae991db = { package = "syn", version = "1.0.109", features = ["extr syn-f595c2ba2a3f28df = { package = "syn", version = "2.0.32", features = ["extra-traits", "fold", "full", "visit", "visit-mut"] } time = { version = "0.3.27", features = ["formatting", "local-offset", "macros", "parsing"] } time-macros = { version = "0.2.13", default-features = false, features = ["formatting", "parsing"] } -tokio = { version = "1.34.0", features = ["full", "test-util"] } +tokio = { version = "1.35.0", features = ["full", "test-util"] } tokio-postgres = { version = "0.7.10", features = ["with-chrono-0_4", "with-serde_json-1", "with-uuid-1"] } tokio-stream = { version = "0.1.14", features = ["net"] } tokio-util = { version = "0.7.10", features = ["codec", "io-util"] } From bad22d463fc6a25f63357f3fffc5d3e156f19163 Mon Sep 17 00:00:00 2001 From: "oxide-renovate[bot]" <146848827+oxide-renovate[bot]@users.noreply.github.com> Date: Sat, 9 Dec 2023 15:48:30 -0800 Subject: [PATCH 06/11] Update Rust crate openapiv3 to 2.0.0 (#4660) --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- workspace-hack/Cargo.toml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8ff57cd451..dd4b206919 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5132,9 +5132,9 @@ dependencies = [ [[package]] name = "openapiv3" -version = "2.0.0-rc.1" +version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "25316406f0191559189c56d99731b63130775de7284d98df5e976ce67882ca8a" +checksum = "cc02deea53ffe807708244e5914f6b099ad7015a207ee24317c22112e17d9c5c" dependencies = [ "indexmap 2.1.0", "serde", diff --git a/Cargo.toml b/Cargo.toml index 0320962452..c7a0c7fd42 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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.19.0" openapi-lint = { git = "https://github.com/oxidecomputer/openapi-lint", branch = "main" } -openapiv3 = "2.0.0-rc.1" +openapiv3 = "2.0.0" # must match samael's crate! openssl = "0.10" openssl-sys = "0.9" diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index 88cadda842..3aff947fd3 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -70,7 +70,7 @@ num-bigint = { version = "0.4.4", features = ["rand"] } num-integer = { version = "0.1.45", features = ["i128"] } num-iter = { version = "0.1.43", default-features = false, features = ["i128"] } num-traits = { version = "0.2.16", features = ["i128", "libm"] } -openapiv3 = { version = "2.0.0-rc.1", default-features = false, features = ["skip_serializing_defaults"] } +openapiv3 = { version = "2.0.0", default-features = false, features = ["skip_serializing_defaults"] } pem-rfc7468 = { version = "0.7.0", default-features = false, features = ["std"] } petgraph = { version = "0.6.4", features = ["serde-1"] } postgres-types = { version = "0.2.6", default-features = false, features = ["with-chrono-0_4", "with-serde_json-1", "with-uuid-1"] } @@ -171,7 +171,7 @@ num-bigint = { version = "0.4.4", features = ["rand"] } num-integer = { version = "0.1.45", features = ["i128"] } num-iter = { version = "0.1.43", default-features = false, features = ["i128"] } num-traits = { version = "0.2.16", features = ["i128", "libm"] } -openapiv3 = { version = "2.0.0-rc.1", default-features = false, features = ["skip_serializing_defaults"] } +openapiv3 = { version = "2.0.0", default-features = false, features = ["skip_serializing_defaults"] } pem-rfc7468 = { version = "0.7.0", default-features = false, features = ["std"] } petgraph = { version = "0.6.4", features = ["serde-1"] } postgres-types = { version = "0.2.6", default-features = false, features = ["with-chrono-0_4", "with-serde_json-1", "with-uuid-1"] } From 0b0f007626c270850f3b715b8bee8bbbee67c951 Mon Sep 17 00:00:00 2001 From: "oxide-renovate[bot]" <146848827+oxide-renovate[bot]@users.noreply.github.com> Date: Sun, 10 Dec 2023 05:45:48 +0000 Subject: [PATCH 07/11] Update taiki-e/install-action digest to 6ee6c3a (#4664) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [taiki-e/install-action](https://togithub.com/taiki-e/install-action) | action | digest | [`d140130` -> `6ee6c3a`](https://togithub.com/taiki-e/install-action/compare/d140130...6ee6c3a) | --- ### Configuration 📅 **Schedule**: Branch creation - "after 8pm,before 6am" in timezone America/Los_Angeles, Automerge - "after 8pm,before 6am" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. â™» **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). Co-authored-by: oxide-renovate[bot] <146848827+oxide-renovate[bot]@users.noreply.github.com> --- .github/workflows/hakari.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/hakari.yml b/.github/workflows/hakari.yml index 0d1aec4c16..b16f1ca9d7 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@d140130aeedb5a946a5769684d32e3a33539f226 # v2 + uses: taiki-e/install-action@6ee6c3ab83eab434138dfa928d72abc7eae14793 # v2 with: tool: cargo-hakari - name: Check workspace-hack Cargo.toml is up-to-date From fe1bed84ff4965a66c345d17a20fdfb371594e3b Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Sun, 10 Dec 2023 13:50:50 -0800 Subject: [PATCH 08/11] Update propolis and crucible versions (#4658) Crucible Start queue backpressure earlier (#1047) Propolis Fix no-deps option for clippy xtask nvme: don't fail on abort cmd (#581) Update openssl and rustix deps Add xtask for pre-push checks Do not require casting for API version cmp better softnpu management command reliability (#570) Log when pause futures complete (#575) Co-authored-by: Alan Hanson --- Cargo.lock | 16 ++++++++-------- Cargo.toml | 12 ++++++------ package-manifest.toml | 12 ++++++------ 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd4b206919..7f966651a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -452,7 +452,7 @@ dependencies = [ [[package]] name = "bhyve_api" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" +source = "git+https://github.com/oxidecomputer/propolis?rev=f1571ce141421cff3d3328f43e7722f5df96fdda#f1571ce141421cff3d3328f43e7722f5df96fdda" dependencies = [ "bhyve_api_sys", "libc", @@ -462,7 +462,7 @@ dependencies = [ [[package]] name = "bhyve_api_sys" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" +source = "git+https://github.com/oxidecomputer/propolis?rev=f1571ce141421cff3d3328f43e7722f5df96fdda#f1571ce141421cff3d3328f43e7722f5df96fdda" dependencies = [ "libc", "strum", @@ -1275,7 +1275,7 @@ dependencies = [ [[package]] name = "crucible-agent-client" version = "0.0.1" -source = "git+https://github.com/oxidecomputer/crucible?rev=945f040d259ca8013d3fb26f510453da7cd7b1a6#945f040d259ca8013d3fb26f510453da7cd7b1a6" +source = "git+https://github.com/oxidecomputer/crucible?rev=fab27994d0bd12725c17d6b478a9bfc2673ad6f4#fab27994d0bd12725c17d6b478a9bfc2673ad6f4" dependencies = [ "anyhow", "chrono", @@ -1291,7 +1291,7 @@ dependencies = [ [[package]] name = "crucible-pantry-client" version = "0.0.1" -source = "git+https://github.com/oxidecomputer/crucible?rev=945f040d259ca8013d3fb26f510453da7cd7b1a6#945f040d259ca8013d3fb26f510453da7cd7b1a6" +source = "git+https://github.com/oxidecomputer/crucible?rev=fab27994d0bd12725c17d6b478a9bfc2673ad6f4#fab27994d0bd12725c17d6b478a9bfc2673ad6f4" dependencies = [ "anyhow", "chrono", @@ -1308,7 +1308,7 @@ dependencies = [ [[package]] name = "crucible-smf" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/crucible?rev=945f040d259ca8013d3fb26f510453da7cd7b1a6#945f040d259ca8013d3fb26f510453da7cd7b1a6" +source = "git+https://github.com/oxidecomputer/crucible?rev=fab27994d0bd12725c17d6b478a9bfc2673ad6f4#fab27994d0bd12725c17d6b478a9bfc2673ad6f4" dependencies = [ "crucible-workspace-hack", "libc", @@ -6161,7 +6161,7 @@ dependencies = [ [[package]] name = "propolis-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" +source = "git+https://github.com/oxidecomputer/propolis?rev=f1571ce141421cff3d3328f43e7722f5df96fdda#f1571ce141421cff3d3328f43e7722f5df96fdda" dependencies = [ "async-trait", "base64", @@ -6182,7 +6182,7 @@ dependencies = [ [[package]] name = "propolis-mock-server" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" +source = "git+https://github.com/oxidecomputer/propolis?rev=f1571ce141421cff3d3328f43e7722f5df96fdda#f1571ce141421cff3d3328f43e7722f5df96fdda" dependencies = [ "anyhow", "atty", @@ -6212,7 +6212,7 @@ dependencies = [ [[package]] name = "propolis_types" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=3e1d129151c3621d28ead5c6e5760693ba6e7fec#3e1d129151c3621d28ead5c6e5760693ba6e7fec" +source = "git+https://github.com/oxidecomputer/propolis?rev=f1571ce141421cff3d3328f43e7722f5df96fdda#f1571ce141421cff3d3328f43e7722f5df96fdda" dependencies = [ "schemars", "serde", diff --git a/Cargo.toml b/Cargo.toml index c7a0c7fd42..5591dcebc9 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 = "945f040d259ca8013d3fb26f510453da7cd7b1a6" } -crucible-pantry-client = { git = "https://github.com/oxidecomputer/crucible", rev = "945f040d259ca8013d3fb26f510453da7cd7b1a6" } -crucible-smf = { git = "https://github.com/oxidecomputer/crucible", rev = "945f040d259ca8013d3fb26f510453da7cd7b1a6" } +crucible-agent-client = { git = "https://github.com/oxidecomputer/crucible", rev = "fab27994d0bd12725c17d6b478a9bfc2673ad6f4" } +crucible-pantry-client = { git = "https://github.com/oxidecomputer/crucible", rev = "fab27994d0bd12725c17d6b478a9bfc2673ad6f4" } +crucible-smf = { git = "https://github.com/oxidecomputer/crucible", rev = "fab27994d0bd12725c17d6b478a9bfc2673ad6f4" } curve25519-dalek = "4" datatest-stable = "0.2.3" display-error-chain = "0.2.0" @@ -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 = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" } -propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" } -propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" } +bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "f1571ce141421cff3d3328f43e7722f5df96fdda" } +propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "f1571ce141421cff3d3328f43e7722f5df96fdda" } +propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "f1571ce141421cff3d3328f43e7722f5df96fdda" } proptest = "1.4.0" quote = "1.0" rand = "0.8.5" diff --git a/package-manifest.toml b/package-manifest.toml index bd60fe9e93..8516a50e65 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -384,10 +384,10 @@ only_for_targets.image = "standard" # 3. Use source.type = "manual" instead of "prebuilt" source.type = "prebuilt" source.repo = "crucible" -source.commit = "945f040d259ca8013d3fb26f510453da7cd7b1a6" +source.commit = "fab27994d0bd12725c17d6b478a9bfc2673ad6f4" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/crucible/image//crucible.sha256.txt -source.sha256 = "f8c23cbf89fd0bbd928d8e3db1357bbea6e6b50560e221f873da5b56ed9d7527" +source.sha256 = "850b468c308cf63ef9e10addee36a923a91b7ab64af0fa0836130c830fb42863" output.type = "zone" [package.crucible-pantry] @@ -395,10 +395,10 @@ service_name = "crucible_pantry" only_for_targets.image = "standard" source.type = "prebuilt" source.repo = "crucible" -source.commit = "945f040d259ca8013d3fb26f510453da7cd7b1a6" +source.commit = "fab27994d0bd12725c17d6b478a9bfc2673ad6f4" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/crucible/image//crucible-pantry.sha256.txt -source.sha256 = "a25b31c81798eb65564dbe259858fdd9715784d212d3508791b1ef0cf6d17da6" +source.sha256 = "893f845caa5d9b146137b503e80d5615cbd6e9d393745e81e772b10a9072b58b" output.type = "zone" # Refer to @@ -409,10 +409,10 @@ service_name = "propolis-server" only_for_targets.image = "standard" source.type = "prebuilt" source.repo = "propolis" -source.commit = "3e1d129151c3621d28ead5c6e5760693ba6e7fec" +source.commit = "f1571ce141421cff3d3328f43e7722f5df96fdda" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/propolis/image//propolis-server.sha256.txt -source.sha256 = "cd341409eb2ffc3d8bec89fd20cad61d170f89d3adf926f6104eb01f4f4da881" +source.sha256 = "6e2607f103419a6338936434f3e67afb7cbe14d6397f2d01982ba94b8d0182a9" output.type = "zone" [package.mg-ddm-gz] From 4ad732573f83f19ffb8f9cc1b3f26b1fc0ebd8c5 Mon Sep 17 00:00:00 2001 From: Augustus Mayo Date: Mon, 11 Dec 2023 09:11:16 -0600 Subject: [PATCH 09/11] Fix paths for reflector updates (#4645) Fixes the paths that maghemite reflector updates should look at when detecting changes. This should fix the missing commit id in the PR titles and bodies Co-authored-by: reflector[bot] <123+reflector[bot]@users.noreply.github.com> --- .github/workflows/update-maghemite.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/update-maghemite.yml b/.github/workflows/update-maghemite.yml index b3611f9987..7ced0adf5e 100644 --- a/.github/workflows/update-maghemite.yml +++ b/.github/workflows/update-maghemite.yml @@ -47,7 +47,7 @@ jobs: - name: Extract new maghemite package version run: | - eval $(cat tools/maghemite_openapi_version | grep COMMIT) + eval $(cat tools/maghemite_mg_openapi_version | grep COMMIT) echo "version=${COMMIT:0:7}" >> $GITHUB_OUTPUT id: updated @@ -55,7 +55,7 @@ jobs: run: | . ./tools/reflector/helpers.sh - PATHS=("tools/maghemite_openapi_version") + PATHS=("tools/maghemite_ddm_openapi_version" "tools/maghemite_mg_openapi_version" "tools/maghemite_mgd_checksums") CHANGES=() commit $TARGET_BRANCH $INT_BRANCH ${{ inputs.reflector_user_id }} PATHS CHANGES From 0c5c559745843996d68fa01c406645248621e45d Mon Sep 17 00:00:00 2001 From: bnaecker Date: Mon, 11 Dec 2023 09:52:36 -0800 Subject: [PATCH 10/11] Add functions to catch timeseries schema changes (#4602) - Move schema types from the `oximeter-db` crate to `oximeter` proper - Add a `SchemaSet` for storing a unique set of timeseries schema, and comparing against a set deserialized from file. This works like expectorate, with a few tweaks, and allows developers to catch any changes. If we want to relax this to catching _compatible_ changes, for some definition of that, we can do that pretty easily. --- Cargo.lock | 5 +- openapi/nexus-internal.json | 14 + openapi/sled-agent.json | 14 + oximeter/collector/src/self_stats.rs | 53 ++ .../tests/output/self-stat-schema.json | 91 +++ oximeter/db/src/client.rs | 4 +- oximeter/db/src/lib.rs | 289 +------- oximeter/db/src/model.rs | 83 +-- oximeter/db/src/query.rs | 44 +- oximeter/oximeter/Cargo.toml | 2 + oximeter/oximeter/src/lib.rs | 4 + oximeter/oximeter/src/schema.rs | 640 ++++++++++++++++++ oximeter/oximeter/src/types.rs | 2 + workspace-hack/Cargo.toml | 4 +- 14 files changed, 881 insertions(+), 368 deletions(-) create mode 100644 oximeter/collector/tests/output/self-stat-schema.json create mode 100644 oximeter/oximeter/src/schema.rs diff --git a/Cargo.lock b/Cargo.lock index 7f966651a5..c379dcfbff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5310,6 +5310,7 @@ dependencies = [ "omicron-common", "omicron-workspace-hack", "oximeter-macro-impl", + "regex", "rstest", "schemars", "serde", @@ -7528,9 +7529,9 @@ dependencies = [ [[package]] name = "similar" -version = "2.2.1" +version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "420acb44afdae038210c99e69aae24109f32f15500aa708e81d46c9f29d55fcf" +checksum = "2aeaf503862c419d66959f5d7ca015337d864e9c49485d771b732e2a20453597" dependencies = [ "bstr 0.2.17", "unicode-segmentation", diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index caf1414f53..f909710ab4 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -4231,6 +4231,20 @@ "content", "type" ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "invalid_timeseries_name" + ] + } + }, + "required": [ + "type" + ] } ] }, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index f809cfa57b..d71f8de644 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -5209,6 +5209,20 @@ "content", "type" ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "invalid_timeseries_name" + ] + } + }, + "required": [ + "type" + ] } ] }, diff --git a/oximeter/collector/src/self_stats.rs b/oximeter/collector/src/self_stats.rs index dd1701203e..8d39e6e282 100644 --- a/oximeter/collector/src/self_stats.rs +++ b/oximeter/collector/src/self_stats.rs @@ -154,8 +154,15 @@ impl CollectionTaskStats { #[cfg(test)] mod tests { + use super::Collections; + use super::Cumulative; + use super::FailedCollections; use super::FailureReason; + use super::OximeterCollector; use super::StatusCode; + use oximeter::schema::SchemaSet; + use std::net::IpAddr; + use std::net::Ipv6Addr; #[test] fn test_failure_reason_serialization() { @@ -168,4 +175,50 @@ mod tests { assert_eq!(variant.to_string(), *as_str); } } + + const fn collector() -> OximeterCollector { + OximeterCollector { + collector_id: uuid::uuid!("cfebaa5f-3ba9-4bb5-9145-648d287df78a"), + collector_ip: IpAddr::V6(Ipv6Addr::LOCALHOST), + collector_port: 12345, + } + } + + fn collections() -> Collections { + Collections { + producer_id: uuid::uuid!("718452ab-7cca-42f6-b8b1-1aaaa1b09104"), + producer_ip: IpAddr::V6(Ipv6Addr::LOCALHOST), + producer_port: 12345, + base_route: String::from("/"), + datum: Cumulative::new(0), + } + } + + fn failed_collections() -> FailedCollections { + FailedCollections { + producer_id: uuid::uuid!("718452ab-7cca-42f6-b8b1-1aaaa1b09104"), + producer_ip: IpAddr::V6(Ipv6Addr::LOCALHOST), + producer_port: 12345, + base_route: String::from("/"), + reason: FailureReason::Unreachable.to_string(), + datum: Cumulative::new(0), + } + } + + // Check that the self-stat timeseries schema have not changed. + #[test] + fn test_no_schema_changes() { + let collector = collector(); + let collections = collections(); + let failed = failed_collections(); + let mut set = SchemaSet::default(); + assert!(set.insert_checked(&collector, &collections).is_none()); + assert!(set.insert_checked(&collector, &failed).is_none()); + + const PATH: &'static str = concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/output/self-stat-schema.json" + ); + set.assert_contents(PATH); + } } diff --git a/oximeter/collector/tests/output/self-stat-schema.json b/oximeter/collector/tests/output/self-stat-schema.json new file mode 100644 index 0000000000..0caf2d27e9 --- /dev/null +++ b/oximeter/collector/tests/output/self-stat-schema.json @@ -0,0 +1,91 @@ +{ + "oximeter_collector:collections": { + "timeseries_name": "oximeter_collector:collections", + "field_schema": [ + { + "name": "base_route", + "field_type": "string", + "source": "metric" + }, + { + "name": "collector_id", + "field_type": "uuid", + "source": "target" + }, + { + "name": "collector_ip", + "field_type": "ip_addr", + "source": "target" + }, + { + "name": "collector_port", + "field_type": "u16", + "source": "target" + }, + { + "name": "producer_id", + "field_type": "uuid", + "source": "metric" + }, + { + "name": "producer_ip", + "field_type": "ip_addr", + "source": "metric" + }, + { + "name": "producer_port", + "field_type": "u16", + "source": "metric" + } + ], + "datum_type": "cumulative_u64", + "created": "2023-12-04T17:49:47.797495948Z" + }, + "oximeter_collector:failed_collections": { + "timeseries_name": "oximeter_collector:failed_collections", + "field_schema": [ + { + "name": "base_route", + "field_type": "string", + "source": "metric" + }, + { + "name": "collector_id", + "field_type": "uuid", + "source": "target" + }, + { + "name": "collector_ip", + "field_type": "ip_addr", + "source": "target" + }, + { + "name": "collector_port", + "field_type": "u16", + "source": "target" + }, + { + "name": "producer_id", + "field_type": "uuid", + "source": "metric" + }, + { + "name": "producer_ip", + "field_type": "ip_addr", + "source": "metric" + }, + { + "name": "producer_port", + "field_type": "u16", + "source": "metric" + }, + { + "name": "reason", + "field_type": "string", + "source": "metric" + } + ], + "datum_type": "cumulative_u64", + "created": "2023-12-04T17:49:47.799970009Z" + } +} \ No newline at end of file diff --git a/oximeter/db/src/client.rs b/oximeter/db/src/client.rs index c8a7db20cb..d295d0dcdf 100644 --- a/oximeter/db/src/client.rs +++ b/oximeter/db/src/client.rs @@ -710,7 +710,7 @@ impl Client { &self, sample: &Sample, ) -> Result, Error> { - let sample_schema = model::schema_for(sample); + let sample_schema = TimeseriesSchema::from(sample); let name = sample_schema.timeseries_name.clone(); let mut schema = self.schema.lock().await; @@ -1873,7 +1873,7 @@ mod tests { client.insert_samples(&[sample.clone()]).await.unwrap(); // The internal map should now contain both the new timeseries schema - let actual_schema = model::schema_for(&sample); + let actual_schema = TimeseriesSchema::from(&sample); let timeseries_name = TimeseriesName::try_from(sample.timeseries_name.as_str()).unwrap(); let expected_schema = client diff --git a/oximeter/db/src/lib.rs b/oximeter/db/src/lib.rs index 425c5189ee..9029319048 100644 --- a/oximeter/db/src/lib.rs +++ b/oximeter/db/src/lib.rs @@ -7,13 +7,23 @@ // Copyright 2023 Oxide Computer Company use crate::query::StringFieldSelector; -use chrono::{DateTime, Utc}; -use dropshot::{EmptyScanParams, PaginationParams}; -pub use oximeter::{DatumType, Field, FieldType, Measurement, Sample}; +use chrono::DateTime; +use chrono::Utc; +use dropshot::EmptyScanParams; +use dropshot::PaginationParams; +pub use oximeter::schema::FieldSchema; +pub use oximeter::schema::FieldSource; +pub use oximeter::schema::TimeseriesName; +pub use oximeter::schema::TimeseriesSchema; +pub use oximeter::DatumType; +pub use oximeter::Field; +pub use oximeter::FieldType; +pub use oximeter::Measurement; +pub use oximeter::Sample; use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; +use serde::Serialize; use std::collections::BTreeMap; -use std::collections::BTreeSet; use std::convert::TryFrom; use std::io; use std::num::NonZeroU32; @@ -23,7 +33,8 @@ use thiserror::Error; mod client; pub mod model; pub mod query; -pub use client::{Client, DbWrite}; +pub use client::Client; +pub use client::DbWrite; pub use model::OXIMETER_VERSION; @@ -78,9 +89,6 @@ pub enum Error { #[error("The field comparison {op} is not valid for the type {ty}")] InvalidFieldCmp { op: String, ty: FieldType }, - #[error("Invalid timeseries name")] - InvalidTimeseriesName, - #[error("Query must resolve to a single timeseries if limit is specified")] InvalidLimitQuery, @@ -117,136 +125,6 @@ pub enum Error { NonSequentialSchemaVersions, } -/// A timeseries name. -/// -/// Timeseries are named by concatenating the names of their target and metric, joined with a -/// colon. -#[derive( - Debug, Clone, PartialEq, PartialOrd, Ord, Eq, Hash, Serialize, Deserialize, -)] -#[serde(try_from = "&str")] -pub struct TimeseriesName(String); - -impl JsonSchema for TimeseriesName { - fn schema_name() -> String { - "TimeseriesName".to_string() - } - - fn json_schema( - _: &mut schemars::gen::SchemaGenerator, - ) -> schemars::schema::Schema { - schemars::schema::SchemaObject { - metadata: Some(Box::new(schemars::schema::Metadata { - title: Some("The name of a timeseries".to_string()), - description: Some( - "Names are constructed by concatenating the target \ - and metric names with ':'. Target and metric \ - names must be lowercase alphanumeric characters \ - with '_' separating words." - .to_string(), - ), - ..Default::default() - })), - instance_type: Some(schemars::schema::InstanceType::String.into()), - string: Some(Box::new(schemars::schema::StringValidation { - pattern: Some(TIMESERIES_NAME_REGEX.to_string()), - ..Default::default() - })), - ..Default::default() - } - .into() - } -} - -impl std::ops::Deref for TimeseriesName { - type Target = String; - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl std::fmt::Display for TimeseriesName { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - -impl std::convert::TryFrom<&str> for TimeseriesName { - type Error = Error; - fn try_from(s: &str) -> Result { - validate_timeseries_name(s).map(|s| TimeseriesName(s.to_string())) - } -} - -impl std::convert::TryFrom for TimeseriesName { - type Error = Error; - fn try_from(s: String) -> Result { - validate_timeseries_name(&s)?; - Ok(TimeseriesName(s)) - } -} - -impl std::str::FromStr for TimeseriesName { - type Err = Error; - fn from_str(s: &str) -> Result { - s.try_into() - } -} - -impl PartialEq for TimeseriesName -where - T: AsRef, -{ - fn eq(&self, other: &T) -> bool { - self.0.eq(other.as_ref()) - } -} - -fn validate_timeseries_name(s: &str) -> Result<&str, Error> { - if regex::Regex::new(TIMESERIES_NAME_REGEX).unwrap().is_match(s) { - Ok(s) - } else { - Err(Error::InvalidTimeseriesName) - } -} - -/// The schema for a timeseries. -/// -/// This includes the name of the timeseries, as well as the datum type of its metric and the -/// schema for each field. -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct TimeseriesSchema { - pub timeseries_name: TimeseriesName, - pub field_schema: BTreeSet, - pub datum_type: DatumType, - pub created: DateTime, -} - -impl TimeseriesSchema { - /// Return the schema for the given field. - pub fn field_schema(&self, name: S) -> Option<&FieldSchema> - where - S: AsRef, - { - self.field_schema.iter().find(|field| field.name == name.as_ref()) - } - - /// Return the target and metric component names for this timeseries - pub fn component_names(&self) -> (&str, &str) { - self.timeseries_name - .split_once(':') - .expect("Incorrectly formatted timseries name") - } -} - -impl PartialEq for TimeseriesSchema { - fn eq(&self, other: &TimeseriesSchema) -> bool { - self.timeseries_name == other.timeseries_name - && self.datum_type == other.datum_type - && self.field_schema == other.field_schema - } -} - impl From for TimeseriesSchema { fn from(schema: model::DbTimeseriesSchema) -> TimeseriesSchema { TimeseriesSchema { @@ -285,25 +163,6 @@ pub struct Timeseries { pub measurements: Vec, } -/// The source from which a field is derived, the target or metric. -#[derive( - Clone, - Copy, - Debug, - PartialEq, - Eq, - PartialOrd, - Ord, - Deserialize, - Serialize, - JsonSchema, -)] -#[serde(rename_all = "snake_case")] -pub enum FieldSource { - Target, - Metric, -} - #[derive( Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize, Serialize, )] @@ -329,24 +188,6 @@ impl From for DbFieldSource { } } -/// The name and type information for a field of a timeseries schema. -#[derive( - Clone, - Debug, - PartialEq, - Eq, - PartialOrd, - Ord, - Deserialize, - Serialize, - JsonSchema, -)] -pub struct FieldSchema { - pub name: String, - pub ty: FieldType, - pub source: FieldSource, -} - /// Type used to paginate request to list timeseries schema. pub type TimeseriesSchemaPaginationParams = PaginationParams; @@ -422,19 +263,6 @@ const DATABASE_NAME: &str = "oximeter"; // See https://clickhouse.com/docs/en/interfaces/formats/#jsoneachrow for details. const DATABASE_SELECT_FORMAT: &str = "JSONEachRow"; -// Regular expression describing valid timeseries names. -// -// Names are derived from the names of the Rust structs for the target and metric, converted to -// snake case. So the names must be valid identifiers, and generally: -// -// - Start with lowercase a-z -// - Any number of alphanumerics -// - Zero or more of the above, delimited by '-'. -// -// That describes the target/metric name, and the timeseries is two of those, joined with ':'. -const TIMESERIES_NAME_REGEX: &str = - "(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*):(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*)"; - #[cfg(test)] mod tests { use super::*; @@ -548,71 +376,16 @@ mod tests { ); } - // Test that we correctly order field across a target and metric. - // - // In an earlier commit, we switched from storing fields in an unordered Vec - // to using a BTree{Map,Set} to ensure ordering by name. However, the - // `TimeseriesSchema` type stored all its fields by chaining the sorted - // fields from the target and metric, without then sorting _across_ them. - // - // This was exacerbated by the error reporting, where we did in fact sort - // all fields across the target and metric, making it difficult to tell how - // the derived schema was different, if at all. - // - // This test generates a sample with a schema where the target and metric - // fields are sorted within them, but not across them. We check that the - // derived schema are actually equal, which means we've imposed that - // ordering when deriving the schema. - #[test] - fn test_schema_field_ordering_across_target_metric() { - let target_field = FieldSchema { - name: String::from("later"), - ty: FieldType::U64, - source: FieldSource::Target, - }; - let metric_field = FieldSchema { - name: String::from("earlier"), - ty: FieldType::U64, - source: FieldSource::Metric, - }; - let timeseries_name: TimeseriesName = "foo:bar".parse().unwrap(); - let datum_type = DatumType::U64; - let field_schema = - [target_field.clone(), metric_field.clone()].into_iter().collect(); - let expected_schema = TimeseriesSchema { - timeseries_name, - field_schema, - datum_type, - created: Utc::now(), - }; - - #[derive(oximeter::Target)] - struct Foo { - later: u64, - } - #[derive(oximeter::Metric)] - struct Bar { - earlier: u64, - datum: u64, - } - - let target = Foo { later: 1 }; - let metric = Bar { earlier: 2, datum: 10 }; - let sample = Sample::new(&target, &metric).unwrap(); - let derived_schema = model::schema_for(&sample); - assert_eq!(derived_schema, expected_schema); - } - #[test] fn test_unsorted_db_fields_are_sorted_on_read() { let target_field = FieldSchema { name: String::from("later"), - ty: FieldType::U64, + field_type: FieldType::U64, source: FieldSource::Target, }; let metric_field = FieldSchema { name: String::from("earlier"), - ty: FieldType::U64, + field_type: FieldType::U64, source: FieldSource::Metric, }; let timeseries_name: TimeseriesName = "foo:bar".parse().unwrap(); @@ -632,7 +405,10 @@ mod tests { // the extracted model type. let db_fields = DbFieldList { names: vec![target_field.name.clone(), metric_field.name.clone()], - types: vec![target_field.ty.into(), metric_field.ty.into()], + types: vec![ + target_field.field_type.into(), + metric_field.field_type.into(), + ], sources: vec![ target_field.source.into(), metric_field.source.into(), @@ -646,23 +422,4 @@ mod tests { }; assert_eq!(expected_schema, TimeseriesSchema::from(db_schema)); } - - #[test] - fn test_field_schema_ordering() { - let mut fields = BTreeSet::new(); - fields.insert(FieldSchema { - name: String::from("second"), - ty: FieldType::U64, - source: FieldSource::Target, - }); - fields.insert(FieldSchema { - name: String::from("first"), - ty: FieldType::U64, - source: FieldSource::Target, - }); - let mut iter = fields.iter(); - assert_eq!(iter.next().unwrap().name, "first"); - assert_eq!(iter.next().unwrap().name, "second"); - assert!(iter.next().is_none()); - } } diff --git a/oximeter/db/src/model.rs b/oximeter/db/src/model.rs index d92e646e89..b1b45eabc4 100644 --- a/oximeter/db/src/model.rs +++ b/oximeter/db/src/model.rs @@ -12,7 +12,6 @@ use crate::FieldSource; use crate::Metric; use crate::Target; use crate::TimeseriesKey; -use crate::TimeseriesName; use crate::TimeseriesSchema; use bytes::Bytes; use chrono::DateTime; @@ -118,7 +117,7 @@ impl From for BTreeSet { .zip(list.sources) .map(|((name, ty), source)| FieldSchema { name, - ty: ty.into(), + field_type: ty.into(), source: source.into(), }) .collect() @@ -131,8 +130,8 @@ impl From> for DbFieldList { let mut types = Vec::with_capacity(list.len()); let mut sources = Vec::with_capacity(list.len()); for field in list.into_iter() { - names.push(field.name); - types.push(field.ty.into()); + names.push(field.name.to_string()); + types.push(field.field_type.into()); sources.push(field.source.into()); } DbFieldList { names, types, sources } @@ -1233,70 +1232,6 @@ pub(crate) fn unroll_measurement_row_impl( } } -/// Return the schema for a `Sample`. -pub(crate) fn schema_for(sample: &Sample) -> TimeseriesSchema { - // The fields are iterated through whatever order the `Target` or `Metric` - // impl chooses. We'll store in a set ordered by field name, to ignore the - // declaration order. - let created = Utc::now(); - let field_schema = sample - .target_fields() - .map(|field| FieldSchema { - name: field.name.clone(), - ty: field.value.field_type(), - source: FieldSource::Target, - }) - .chain(sample.metric_fields().map(|field| FieldSchema { - name: field.name.clone(), - ty: field.value.field_type(), - source: FieldSource::Metric, - })) - .collect(); - TimeseriesSchema { - timeseries_name: TimeseriesName::try_from( - sample.timeseries_name.as_str(), - ) - .expect("Failed to parse timeseries name"), - field_schema, - datum_type: sample.measurement.datum_type(), - created, - } -} - -/// Return the schema for a `Target` and `Metric` -pub(crate) fn schema_for_parts(target: &T, metric: &M) -> TimeseriesSchema -where - T: traits::Target, - M: traits::Metric, -{ - let make_field_schema = |name: &str, - value: FieldValue, - source: FieldSource| { - FieldSchema { name: name.to_string(), ty: value.field_type(), source } - }; - let target_field_schema = - target.field_names().iter().zip(target.field_values()); - let metric_field_schema = - metric.field_names().iter().zip(metric.field_values()); - let field_schema = target_field_schema - .map(|(name, value)| { - make_field_schema(name, value, FieldSource::Target) - }) - .chain(metric_field_schema.map(|(name, value)| { - make_field_schema(name, value, FieldSource::Metric) - })) - .collect(); - TimeseriesSchema { - timeseries_name: TimeseriesName::try_from(oximeter::timeseries_name( - target, metric, - )) - .expect("Failed to parse timeseries name"), - field_schema, - datum_type: metric.datum_type(), - created: Utc::now(), - } -} - // A scalar timestamped sample from a gauge timeseries, as extracted from a query to the database. #[derive(Debug, Clone, Deserialize)] struct DbTimeseriesScalarGaugeSample { @@ -1669,11 +1604,10 @@ pub(crate) fn parse_field_select_row( "Expected pairs of (field_name, field_value) from the field query" ); let (target_name, metric_name) = schema.component_names(); - let mut n_fields = 0; let mut target_fields = Vec::new(); let mut metric_fields = Vec::new(); let mut actual_fields = row.fields.values(); - while n_fields < schema.field_schema.len() { + for _ in 0..schema.field_schema.len() { // Extract the field name from the row and find a matching expected field. let actual_field_name = actual_fields .next() @@ -1682,7 +1616,7 @@ pub(crate) fn parse_field_select_row( .as_str() .expect("Expected a string field name") .to_string(); - let expected_field = schema.field_schema(&name).expect( + let expected_field = schema.schema_for_field(&name).expect( "Found field with name that is not part of the timeseries schema", ); @@ -1690,7 +1624,7 @@ pub(crate) fn parse_field_select_row( let actual_field_value = actual_fields .next() .expect("Missing a field value from a field select query"); - let value = match expected_field.ty { + let value = match expected_field.field_type { FieldType::Bool => { FieldValue::Bool(bool::from(DbBool::from( actual_field_value @@ -1797,7 +1731,6 @@ pub(crate) fn parse_field_select_row( FieldSource::Target => target_fields.push(field), FieldSource::Metric => metric_fields.push(field), } - n_fields += 1; } ( row.timeseries_key, @@ -1874,12 +1807,12 @@ mod tests { let list: BTreeSet<_> = [ FieldSchema { name: String::from("field0"), - ty: FieldType::I64, + field_type: FieldType::I64, source: FieldSource::Target, }, FieldSchema { name: String::from("field1"), - ty: FieldType::IpAddr, + field_type: FieldType::IpAddr, source: FieldSource::Metric, }, ] diff --git a/oximeter/db/src/query.rs b/oximeter/db/src/query.rs index 6a55d3f518..2caefb24c3 100644 --- a/oximeter/db/src/query.rs +++ b/oximeter/db/src/query.rs @@ -101,7 +101,7 @@ impl SelectQueryBuilder { let field_name = field_name.as_ref().to_string(); let field_schema = self .timeseries_schema - .field_schema(&field_name) + .schema_for_field(&field_name) .ok_or_else(|| Error::NoSuchField { timeseries_name: self .timeseries_schema @@ -110,7 +110,7 @@ impl SelectQueryBuilder { field_name: field_name.clone(), })?; let field_value: FieldValue = field_value.into(); - let expected_type = field_schema.ty; + let expected_type = field_schema.field_type; let found_type = field_value.field_type(); if expected_type != found_type { return Err(Error::IncorrectFieldType { @@ -150,7 +150,7 @@ impl SelectQueryBuilder { ) -> Result { let field_schema = self .timeseries_schema - .field_schema(&selector.name) + .schema_for_field(&selector.name) .ok_or_else(|| Error::NoSuchField { timeseries_name: self .timeseries_schema @@ -158,13 +158,14 @@ impl SelectQueryBuilder { .to_string(), field_name: selector.name.clone(), })?; - if !selector.op.valid_for_type(field_schema.ty) { + let field_type = field_schema.field_type; + if !selector.op.valid_for_type(field_type) { return Err(Error::InvalidFieldCmp { op: format!("{:?}", selector.op), - ty: field_schema.ty, + ty: field_schema.field_type, }); } - let field_value = match field_schema.ty { + let field_value = match field_type { FieldType::String => FieldValue::from(&selector.value), FieldType::I8 => parse_selector_field_value::( &field_schema, @@ -214,9 +215,9 @@ impl SelectQueryBuilder { let comparison = FieldComparison { op: selector.op, value: field_value }; let selector = FieldSelector { - name: field_schema.name.clone(), + name: field_schema.name.to_string(), comparison: Some(comparison), - ty: field_schema.ty, + ty: field_type, }; self.field_selectors.insert(field_schema.clone(), selector); Ok(self) @@ -248,7 +249,7 @@ impl SelectQueryBuilder { T: Target, M: Metric, { - let schema = crate::model::schema_for_parts(target, metric); + let schema = TimeseriesSchema::new(target, metric); let mut builder = Self::new(&schema); let target_fields = target.field_names().iter().zip(target.field_values()); @@ -279,9 +280,9 @@ impl SelectQueryBuilder { for field in timeseries_schema.field_schema.iter() { let key = field.clone(); field_selectors.entry(key).or_insert_with(|| FieldSelector { - name: field.name.clone(), + name: field.name.to_string(), comparison: None, - ty: field.ty, + ty: field.field_type, }); } SelectQuery { @@ -309,8 +310,8 @@ where { Ok(FieldValue::from(s.parse::().map_err(|_| { Error::InvalidFieldValue { - field_name: field.name.clone(), - field_type: field.ty, + field_name: field.name.to_string(), + field_type: field.field_type, value: s.to_string(), } })?)) @@ -778,12 +779,12 @@ mod tests { field_schema: [ FieldSchema { name: "f0".to_string(), - ty: FieldType::I64, + field_type: FieldType::I64, source: FieldSource::Target, }, FieldSchema { name: "f1".to_string(), - ty: FieldType::Bool, + field_type: FieldType::Bool, source: FieldSource::Target, }, ] @@ -981,6 +982,7 @@ mod tests { "Expected an exact comparison when building a query from parts", ); + println!("{builder:#?}"); assert_eq!( builder.field_selector(FieldSource::Metric, "baz").unwrap(), &FieldSelector { @@ -1002,12 +1004,12 @@ mod tests { field_schema: [ FieldSchema { name: "f0".to_string(), - ty: FieldType::I64, + field_type: FieldType::I64, source: FieldSource::Target, }, FieldSchema { name: "f1".to_string(), - ty: FieldType::Bool, + field_type: FieldType::Bool, source: FieldSource::Target, }, ] @@ -1065,12 +1067,12 @@ mod tests { field_schema: [ FieldSchema { name: "f0".to_string(), - ty: FieldType::I64, + field_type: FieldType::I64, source: FieldSource::Target, }, FieldSchema { name: "f1".to_string(), - ty: FieldType::Bool, + field_type: FieldType::Bool, source: FieldSource::Target, }, ] @@ -1116,12 +1118,12 @@ mod tests { field_schema: [ FieldSchema { name: "f0".to_string(), - ty: FieldType::I64, + field_type: FieldType::I64, source: FieldSource::Target, }, FieldSchema { name: "f1".to_string(), - ty: FieldType::Bool, + field_type: FieldType::Bool, source: FieldSource::Target, }, ] diff --git a/oximeter/oximeter/Cargo.toml b/oximeter/oximeter/Cargo.toml index 0cb2d8cace..b545c697de 100644 --- a/oximeter/oximeter/Cargo.toml +++ b/oximeter/oximeter/Cargo.toml @@ -11,8 +11,10 @@ chrono.workspace = true num.workspace = true omicron-common.workspace = true oximeter-macro-impl.workspace = true +regex.workspace = true schemars = { workspace = true, features = [ "uuid1", "bytes", "chrono" ] } serde.workspace = true +serde_json.workspace = true strum.workspace = true thiserror.workspace = true uuid.workspace = true diff --git a/oximeter/oximeter/src/lib.rs b/oximeter/oximeter/src/lib.rs index 2ced404eae..1855762abe 100644 --- a/oximeter/oximeter/src/lib.rs +++ b/oximeter/oximeter/src/lib.rs @@ -108,10 +108,14 @@ pub use oximeter_macro_impl::*; extern crate self as oximeter; pub mod histogram; +pub mod schema; pub mod test_util; pub mod traits; pub mod types; +pub use schema::FieldSchema; +pub use schema::TimeseriesName; +pub use schema::TimeseriesSchema; pub use traits::Metric; pub use traits::Producer; pub use traits::Target; diff --git a/oximeter/oximeter/src/schema.rs b/oximeter/oximeter/src/schema.rs new file mode 100644 index 0000000000..b6953fda52 --- /dev/null +++ b/oximeter/oximeter/src/schema.rs @@ -0,0 +1,640 @@ +// 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/. + +// Copyright 2023 Oxide Computer Company + +//! Tools for working with schema for fields and timeseries. + +use crate::types::DatumType; +use crate::types::FieldType; +use crate::types::MetricsError; +use crate::types::Sample; +use crate::Metric; +use crate::Target; +use chrono::DateTime; +use chrono::Utc; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; +use std::collections::btree_map::Entry; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::fmt::Write; +use std::path::Path; + +/// The name and type information for a field of a timeseries schema. +#[derive( + Clone, + Debug, + PartialEq, + Eq, + PartialOrd, + Ord, + Deserialize, + Serialize, + JsonSchema, +)] +pub struct FieldSchema { + pub name: String, + pub field_type: FieldType, + pub source: FieldSource, +} + +/// The source from which a field is derived, the target or metric. +#[derive( + Clone, + Copy, + Debug, + PartialEq, + Eq, + PartialOrd, + Ord, + Deserialize, + Serialize, + JsonSchema, +)] +#[serde(rename_all = "snake_case")] +pub enum FieldSource { + Target, + Metric, +} + +/// A timeseries name. +/// +/// Timeseries are named by concatenating the names of their target and metric, joined with a +/// colon. +#[derive( + Debug, Clone, PartialEq, PartialOrd, Ord, Eq, Hash, Serialize, Deserialize, +)] +#[serde(try_from = "&str")] +pub struct TimeseriesName(String); + +impl JsonSchema for TimeseriesName { + fn schema_name() -> String { + "TimeseriesName".to_string() + } + + fn json_schema( + _: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::SchemaObject { + metadata: Some(Box::new(schemars::schema::Metadata { + title: Some("The name of a timeseries".to_string()), + description: Some( + "Names are constructed by concatenating the target \ + and metric names with ':'. Target and metric \ + names must be lowercase alphanumeric characters \ + with '_' separating words." + .to_string(), + ), + ..Default::default() + })), + instance_type: Some(schemars::schema::InstanceType::String.into()), + string: Some(Box::new(schemars::schema::StringValidation { + pattern: Some(TIMESERIES_NAME_REGEX.to_string()), + ..Default::default() + })), + ..Default::default() + } + .into() + } +} + +impl std::ops::Deref for TimeseriesName { + type Target = String; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::fmt::Display for TimeseriesName { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::convert::TryFrom<&str> for TimeseriesName { + type Error = MetricsError; + fn try_from(s: &str) -> Result { + validate_timeseries_name(s).map(|s| TimeseriesName(s.to_string())) + } +} + +impl std::convert::TryFrom for TimeseriesName { + type Error = MetricsError; + fn try_from(s: String) -> Result { + validate_timeseries_name(&s)?; + Ok(TimeseriesName(s)) + } +} + +impl std::str::FromStr for TimeseriesName { + type Err = MetricsError; + fn from_str(s: &str) -> Result { + s.try_into() + } +} + +impl PartialEq for TimeseriesName +where + T: AsRef, +{ + fn eq(&self, other: &T) -> bool { + self.0.eq(other.as_ref()) + } +} + +fn validate_timeseries_name(s: &str) -> Result<&str, MetricsError> { + if regex::Regex::new(TIMESERIES_NAME_REGEX).unwrap().is_match(s) { + Ok(s) + } else { + Err(MetricsError::InvalidTimeseriesName) + } +} + +/// The schema for a timeseries. +/// +/// This includes the name of the timeseries, as well as the datum type of its metric and the +/// schema for each field. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct TimeseriesSchema { + pub timeseries_name: TimeseriesName, + pub field_schema: BTreeSet, + pub datum_type: DatumType, + pub created: DateTime, +} + +impl From<&Sample> for TimeseriesSchema { + fn from(sample: &Sample) -> Self { + let timeseries_name = sample.timeseries_name.parse().unwrap(); + let mut field_schema = BTreeSet::new(); + for field in sample.target_fields() { + let schema = FieldSchema { + name: field.name.clone(), + field_type: field.value.field_type(), + source: FieldSource::Target, + }; + field_schema.insert(schema); + } + for field in sample.metric_fields() { + let schema = FieldSchema { + name: field.name.clone(), + field_type: field.value.field_type(), + source: FieldSource::Metric, + }; + field_schema.insert(schema); + } + let datum_type = sample.measurement.datum_type(); + Self { timeseries_name, field_schema, datum_type, created: Utc::now() } + } +} + +impl TimeseriesSchema { + /// Construct a timeseries schema from a target and metric. + pub fn new(target: &T, metric: &M) -> Self + where + T: Target, + M: Metric, + { + let timeseries_name = + TimeseriesName::try_from(crate::timeseries_name(target, metric)) + .unwrap(); + let mut field_schema = BTreeSet::new(); + for field in target.fields() { + let schema = FieldSchema { + name: field.name.clone(), + field_type: field.value.field_type(), + source: FieldSource::Target, + }; + field_schema.insert(schema); + } + for field in metric.fields() { + let schema = FieldSchema { + name: field.name.clone(), + field_type: field.value.field_type(), + source: FieldSource::Metric, + }; + field_schema.insert(schema); + } + let datum_type = metric.datum_type(); + Self { timeseries_name, field_schema, datum_type, created: Utc::now() } + } + + /// Construct a timeseries schema from a sample + pub fn from_sample(sample: &Sample) -> Self { + Self::from(sample) + } + + /// Return the schema for the given field. + pub fn schema_for_field(&self, name: S) -> Option<&FieldSchema> + where + S: AsRef, + { + self.field_schema.iter().find(|field| field.name == name.as_ref()) + } + + /// Return the target and metric component names for this timeseries + pub fn component_names(&self) -> (&str, &str) { + self.timeseries_name + .split_once(':') + .expect("Incorrectly formatted timseries name") + } +} + +impl PartialEq for TimeseriesSchema { + fn eq(&self, other: &TimeseriesSchema) -> bool { + self.timeseries_name == other.timeseries_name + && self.datum_type == other.datum_type + && self.field_schema == other.field_schema + } +} + +// Regular expression describing valid timeseries names. +// +// Names are derived from the names of the Rust structs for the target and metric, converted to +// snake case. So the names must be valid identifiers, and generally: +// +// - Start with lowercase a-z +// - Any number of alphanumerics +// - Zero or more of the above, delimited by '-'. +// +// That describes the target/metric name, and the timeseries is two of those, joined with ':'. +const TIMESERIES_NAME_REGEX: &str = + "(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*):(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*)"; + +/// A set of timeseries schema, useful for testing changes to targets or +/// metrics. +#[derive(Debug, Default, Deserialize, PartialEq, Serialize)] +pub struct SchemaSet { + #[serde(flatten)] + inner: BTreeMap, +} + +impl SchemaSet { + /// Insert a timeseries schema, checking for conflicts. + /// + /// This inserts the schema derived from `target` and `metric`. If one + /// does _not_ already exist in `self` or a _matching_ one exists, `None` + /// is returned. + /// + /// If the derived schema _conflicts_ with one in `self`, the existing + /// schema is returned. + pub fn insert_checked( + &mut self, + target: &T, + metric: &M, + ) -> Option + where + T: Target, + M: Metric, + { + let new = TimeseriesSchema::new(target, metric); + let name = new.timeseries_name.clone(); + match self.inner.entry(name) { + Entry::Vacant(entry) => { + entry.insert(new); + None + } + Entry::Occupied(entry) => { + let existing = entry.get(); + if existing == &new { + None + } else { + Some(existing.clone()) + } + } + } + } + + /// Compare the set of schema against the contents of a file. + /// + /// This function loads a `SchemaSet` from the provided JSON file, and + /// asserts that the contained schema matches those in `self`. Note that + /// equality of `TimeseriesSchema` ignores creation timestamps, so this + /// compares the "identity" data: timeseries name, field names, field types, + /// and field sources. + /// + /// This is intentionally similar to `expectorate::assert_contents()`. If + /// the provided file doesn't exist, it's treated as empty. If it does, a + /// `SchemaSet` is deserialized from it and a comparison between that and + /// `self` is done. + /// + /// You can use `EXPECTORATE=overwrite` to overwrite the existing file, + /// rather than panicking. + pub fn assert_contents(&self, path: impl AsRef) { + let path = path.as_ref(); + let v = std::env::var_os("EXPECTORATE"); + let overwrite = + v.as_deref().and_then(std::ffi::OsStr::to_str) == Some("overwrite"); + let expected_contents = serde_json::to_string_pretty(self).unwrap(); + if overwrite { + if let Err(e) = std::fs::write(path, &expected_contents) { + panic!( + "Failed to write contents to '{}': {}", + path.display(), + e + ); + } + } else { + // If the file doesn't exist, it's just empty and we'll create an + // empty set of schema. + let contents = if !path.exists() { + String::from("{}") + } else { + match std::fs::read_to_string(path) { + Err(e) => { + panic!("Failed to read '{}': {}", path.display(), e) + } + Ok(c) => c, + } + }; + let other: Self = serde_json::from_str(&contents).unwrap(); + if self == &other { + return; + } + + let mut diffs = String::new(); + writeln!( + &mut diffs, + "Timeseries schema in \"{}\" do not match\n", + path.display() + ) + .unwrap(); + + // Print schema in self that are not in the file, or mismatched + // schema. + for (name, schema) in self.inner.iter() { + let Some(other_schema) = other.inner.get(name) else { + writeln!( + &mut diffs, + "File is missing timeseries \"{}\"", + name + ) + .unwrap(); + continue; + }; + if schema == other_schema { + continue; + } + writeln!(&mut diffs, "Timeseries \"{name}\" differs").unwrap(); + + // Print out any differences in the datum type. + if schema.datum_type != other_schema.datum_type { + writeln!( + &mut diffs, + " Expected datum type: {}", + schema.datum_type + ) + .unwrap(); + writeln!( + &mut diffs, + " Actual datum type: {}", + other_schema.datum_type + ) + .unwrap(); + } + + // Print fields in self that are not in other, or are mismatched + for field in schema.field_schema.iter() { + let Some(other_field) = + other_schema.field_schema.get(field) + else { + writeln!( + &mut diffs, + " File is missing {:?} field \"{}\"", + field.source, field.name, + ) + .unwrap(); + continue; + }; + if field == other_field { + continue; + } + + writeln!( + &mut diffs, + " File has mismatched field \"{}\"", + field.name + ) + .unwrap(); + writeln!( + &mut diffs, + " Expected type: {}", + field.field_type + ) + .unwrap(); + writeln!( + &mut diffs, + " Actual type: {}", + other_field.field_type + ) + .unwrap(); + writeln!( + &mut diffs, + " Expected source: {:?}", + field.source + ) + .unwrap(); + writeln!( + &mut diffs, + " Actual source: {:?}", + other_field.source + ) + .unwrap(); + } + + // Print fields in other that are not in self, fields that are + // in both but don't match are taken care of in the above loop. + for other_field in other_schema.field_schema.iter() { + if schema.field_schema.contains(other_field) { + continue; + } + + writeln!( + &mut diffs, + " Current set is missing {:?} field \"{}\"", + other_field.source, other_field.name, + ) + .unwrap(); + } + } + + // Print schema that are in the file, but not self. Those that don't + // match are handled in the above block. + for key in other.inner.keys() { + if !self.inner.contains_key(key) { + writeln!( + &mut diffs, + " Current set is missing timeseries \"{}\"", + key + ) + .unwrap(); + } + } + panic!("{}", diffs); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::convert::TryFrom; + use uuid::Uuid; + + #[test] + fn test_timeseries_name() { + let name = TimeseriesName::try_from("foo:bar").unwrap(); + assert_eq!(format!("{}", name), "foo:bar"); + } + + #[test] + fn test_timeseries_name_from_str() { + assert!(TimeseriesName::try_from("a:b").is_ok()); + assert!(TimeseriesName::try_from("a_a:b_b").is_ok()); + assert!(TimeseriesName::try_from("a0:b0").is_ok()); + assert!(TimeseriesName::try_from("a_0:b_0").is_ok()); + + assert!(TimeseriesName::try_from("_:b").is_err()); + assert!(TimeseriesName::try_from("a_:b").is_err()); + assert!(TimeseriesName::try_from("0:b").is_err()); + assert!(TimeseriesName::try_from(":b").is_err()); + assert!(TimeseriesName::try_from("a:").is_err()); + assert!(TimeseriesName::try_from("123").is_err()); + } + + #[derive(Target)] + struct MyTarget { + id: Uuid, + name: String, + } + + const ID: Uuid = uuid::uuid!("ca565ef4-65dc-4ab0-8622-7be43ed72105"); + + impl Default for MyTarget { + fn default() -> Self { + Self { id: ID, name: String::from("name") } + } + } + + #[derive(Metric)] + struct MyMetric { + happy: bool, + datum: u64, + } + + impl Default for MyMetric { + fn default() -> Self { + Self { happy: true, datum: 0 } + } + } + + #[test] + fn test_timeseries_schema_from_parts() { + let target = MyTarget::default(); + let metric = MyMetric::default(); + let schema = TimeseriesSchema::new(&target, &metric); + + assert_eq!(schema.timeseries_name, "my_target:my_metric"); + let f = schema.schema_for_field("id").unwrap(); + assert_eq!(f.name, "id"); + assert_eq!(f.field_type, FieldType::Uuid); + assert_eq!(f.source, FieldSource::Target); + + let f = schema.schema_for_field("name").unwrap(); + assert_eq!(f.name, "name"); + assert_eq!(f.field_type, FieldType::String); + assert_eq!(f.source, FieldSource::Target); + + let f = schema.schema_for_field("happy").unwrap(); + assert_eq!(f.name, "happy"); + assert_eq!(f.field_type, FieldType::Bool); + assert_eq!(f.source, FieldSource::Metric); + assert_eq!(schema.datum_type, DatumType::U64); + } + + #[test] + fn test_timeseries_schema_from_sample() { + let target = MyTarget::default(); + let metric = MyMetric::default(); + let sample = Sample::new(&target, &metric).unwrap(); + let schema = TimeseriesSchema::new(&target, &metric); + let schema_from_sample = TimeseriesSchema::from(&sample); + assert_eq!(schema, schema_from_sample); + } + + // Test that we correctly order field across a target and metric. + // + // In an earlier commit, we switched from storing fields in an unordered Vec + // to using a BTree{Map,Set} to ensure ordering by name. However, the + // `TimeseriesSchema` type stored all its fields by chaining the sorted + // fields from the target and metric, without then sorting _across_ them. + // + // This was exacerbated by the error reporting, where we did in fact sort + // all fields across the target and metric, making it difficult to tell how + // the derived schema was different, if at all. + // + // This test generates a sample with a schema where the target and metric + // fields are sorted within them, but not across them. We check that the + // derived schema are actually equal, which means we've imposed that + // ordering when deriving the schema. + #[test] + fn test_schema_field_ordering_across_target_metric() { + let target_field = FieldSchema { + name: String::from("later"), + field_type: FieldType::U64, + source: FieldSource::Target, + }; + let metric_field = FieldSchema { + name: String::from("earlier"), + field_type: FieldType::U64, + source: FieldSource::Metric, + }; + let timeseries_name: TimeseriesName = "foo:bar".parse().unwrap(); + let datum_type = DatumType::U64; + let field_schema = + [target_field.clone(), metric_field.clone()].into_iter().collect(); + let expected_schema = TimeseriesSchema { + timeseries_name, + field_schema, + datum_type, + created: Utc::now(), + }; + + #[derive(oximeter::Target)] + struct Foo { + later: u64, + } + #[derive(oximeter::Metric)] + struct Bar { + earlier: u64, + datum: u64, + } + + let target = Foo { later: 1 }; + let metric = Bar { earlier: 2, datum: 10 }; + let sample = Sample::new(&target, &metric).unwrap(); + let derived_schema = TimeseriesSchema::from(&sample); + assert_eq!(derived_schema, expected_schema); + } + + #[test] + fn test_field_schema_ordering() { + let mut fields = BTreeSet::new(); + fields.insert(FieldSchema { + name: String::from("second"), + field_type: FieldType::U64, + source: FieldSource::Target, + }); + fields.insert(FieldSchema { + name: String::from("first"), + field_type: FieldType::U64, + source: FieldSource::Target, + }); + let mut iter = fields.iter(); + assert_eq!(iter.next().unwrap().name, "first"); + assert_eq!(iter.next().unwrap().name, "second"); + assert!(iter.next().is_none()); + } +} diff --git a/oximeter/oximeter/src/types.rs b/oximeter/oximeter/src/types.rs index 23dbe2be6b..3d74bec72c 100644 --- a/oximeter/oximeter/src/types.rs +++ b/oximeter/oximeter/src/types.rs @@ -629,6 +629,8 @@ pub enum MetricsError { #[error("Missing datum of type {datum_type} cannot have a start time")] MissingDatumCannotHaveStartTime { datum_type: DatumType }, + #[error("Invalid timeseries name")] + InvalidTimeseriesName, } impl From for omicron_common::api::external::Error { diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index 3aff947fd3..1d14b26a69 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -89,7 +89,7 @@ semver = { version = "1.0.20", features = ["serde"] } serde = { version = "1.0.192", features = ["alloc", "derive", "rc"] } serde_json = { version = "1.0.108", features = ["raw_value"] } sha2 = { version = "0.10.8", features = ["oid"] } -similar = { version = "2.2.1", features = ["inline", "unicode"] } +similar = { version = "2.3.0", features = ["inline", "unicode"] } slog = { version = "2.7.0", features = ["dynamic-keys", "max_level_trace", "release_max_level_debug", "release_max_level_trace"] } snafu = { version = "0.7.5", features = ["futures"] } spin = { version = "0.9.8" } @@ -190,7 +190,7 @@ semver = { version = "1.0.20", features = ["serde"] } serde = { version = "1.0.192", features = ["alloc", "derive", "rc"] } serde_json = { version = "1.0.108", features = ["raw_value"] } sha2 = { version = "0.10.8", features = ["oid"] } -similar = { version = "2.2.1", features = ["inline", "unicode"] } +similar = { version = "2.3.0", features = ["inline", "unicode"] } slog = { version = "2.7.0", features = ["dynamic-keys", "max_level_trace", "release_max_level_debug", "release_max_level_trace"] } snafu = { version = "0.7.5", features = ["futures"] } spin = { version = "0.9.8" } From baf7347b6b6b4416930b6f4eaba0d45db1694982 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 11 Dec 2023 13:58:57 -0500 Subject: [PATCH 11/11] Explicitly use the alloc op context for dpd nat updates (#4654) Passes the alloc opcontext to the boundary switches call to avoid a permissions issue when deleting nat entries while deleting an instance. --------- Co-authored-by: James MacMahon --- nexus/src/app/instance_network.rs | 5 +++-- nexus/tests/integration_tests/instances.rs | 24 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/instance_network.rs b/nexus/src/app/instance_network.rs index abb8c744e1..3db749f43b 100644 --- a/nexus/src/app/instance_network.rs +++ b/nexus/src/app/instance_network.rs @@ -451,8 +451,6 @@ impl super::Nexus { .instance_lookup_external_ips(opctx, instance_id) .await?; - let boundary_switches = self.boundary_switches(opctx).await?; - let mut errors = vec![]; for entry in external_ips { // Soft delete the NAT entry @@ -478,6 +476,9 @@ impl super::Nexus { }?; } + let boundary_switches = + self.boundary_switches(&self.opctx_alloc).await?; + for switch in &boundary_switches { debug!(&self.log, "notifying dendrite of updates"; "instance_id" => %authz_instance.id(), diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 9260006c81..19b507f5bb 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3911,6 +3911,30 @@ async fn test_instance_create_in_silo(cptestctx: &ControlPlaneTestContext) { instance_simulate_with_opctx(nexus, &instance.identity.id, &opctx).await; let instance = instance_get_as(&client, &instance_url, authn).await; assert_eq!(instance.runtime.run_state, InstanceState::Running); + + // Stop the instance + NexusRequest::new( + RequestBuilder::new( + client, + Method::POST, + &format!("/v1/instances/{}/stop", instance.identity.id), + ) + .body(None as Option<&serde_json::Value>) + .expect_status(Some(StatusCode::ACCEPTED)), + ) + .authn_as(AuthnMode::SiloUser(user_id)) + .execute() + .await + .expect("Failed to stop the instance"); + + instance_simulate_with_opctx(nexus, &instance.identity.id, &opctx).await; + + // Delete the instance + NexusRequest::object_delete(client, &instance_url) + .authn_as(AuthnMode::SiloUser(user_id)) + .execute() + .await + .expect("Failed to delete the instance"); } /// Test that appropriate OPTE V2P mappings are created and deleted.