From b97e434eec561fa80f965a2f40ec87c83a3a439e Mon Sep 17 00:00:00 2001 From: Artemis Everfree Date: Sat, 20 Apr 2024 00:34:22 +0000 Subject: [PATCH] Remove ReqwestBlockIO and VolumeConstructionRequest::Url As far as I can tell, nothing uses this anymore. With the exception of a pantry integration test. That test is not interested in the Reqwest part, and is just interested in a read-only parent with random data in it, so I changed that to use a tempfile read-only parent. Propolis will need to be updated. It has exactly one reference to `VolumeConstructionRequest::Url`: bin/propolis-server/src/lib/initializer.rs 385: VolumeConstructionRequest::Url { id, .. } => id.to_string(), This is in some code that's extracting the UUIDs out of all the VCR types, and also doesn't care about the Reqwest functionality. --- Cargo.lock | 1 - crucible-client-types/src/lib.rs | 5 - integration_tests/src/lib.rs | 120 +++------------------ openapi/crucible-pantry.json | 29 ------ tools/dtrace/README.md | 9 -- tools/dtrace/perf-reqwest.d | 20 ---- upstairs/Cargo.toml | 1 - upstairs/src/block_io.rs | 173 ------------------------------- upstairs/src/lib.rs | 8 +- upstairs/src/volume.rs | 25 ----- 10 files changed, 18 insertions(+), 373 deletions(-) delete mode 100755 tools/dtrace/perf-reqwest.d diff --git a/Cargo.lock b/Cargo.lock index 33053a1d6..b4021b931 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -794,7 +794,6 @@ dependencies = [ "rand 0.8.5", "rand_chacha 0.3.1", "rayon", - "reqwest", "ringbuffer", "schemars", "serde", diff --git a/crucible-client-types/src/lib.rs b/crucible-client-types/src/lib.rs index ba93e6513..4a6686d16 100644 --- a/crucible-client-types/src/lib.rs +++ b/crucible-client-types/src/lib.rs @@ -18,11 +18,6 @@ pub enum VolumeConstructionRequest { sub_volumes: Vec, read_only_parent: Option>, }, - Url { - id: Uuid, - block_size: u64, - url: String, - }, Region { block_size: u64, blocks_per_extent: u64, diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index fc3556429..f4c9f4019 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -19,6 +19,7 @@ mod test { use repair_client::Client; use sha2::Digest; use slog::{info, o, warn, Drain, Logger}; + use std::io::Write; use tempfile::*; use tokio::sync::mpsc; use uuid::*; @@ -746,82 +747,6 @@ mod test { Ok(()) } - #[tokio::test] - async fn integration_test_url() -> Result<()> { - const BLOCK_SIZE: usize = 512; - - let tds = TestDownstairsSet::small(false).await?; - let opts = tds.opts(); - - let server = Server::run(); - server.expect( - Expectation::matching(request::method_path("GET", "/ff.raw")) - .times(1..) - .respond_with(status_code(200).body(vec![0xff; BLOCK_SIZE])), - ); - server.expect( - Expectation::matching(request::method_path("HEAD", "/ff.raw")) - .times(1..) - .respond_with(status_code(200).append_header( - "Content-Length", - format!("{}", BLOCK_SIZE), - )), - ); - - let vcr: VolumeConstructionRequest = - VolumeConstructionRequest::Volume { - id: Uuid::new_v4(), - block_size: BLOCK_SIZE as u64, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size: BLOCK_SIZE as u64, - blocks_per_extent: tds.blocks_per_extent(), - extent_count: tds.extent_count(), - opts, - gen: 1, - }], - read_only_parent: Some(Box::new( - VolumeConstructionRequest::Volume { - id: Uuid::new_v4(), - block_size: BLOCK_SIZE as u64, - sub_volumes: vec![VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size: BLOCK_SIZE as u64, - url: server.url("/ff.raw").to_string(), - }], - read_only_parent: None, - }, - )), - }; - - let volume = Volume::construct(vcr, None, csl()).await?; - volume.activate().await?; - - // Read one block: should be all 0xff - let mut buffer = Buffer::new(1, BLOCK_SIZE); - volume - .read(Block::new(0, BLOCK_SIZE.trailing_zeros()), &mut buffer) - .await?; - - assert_eq!(vec![0xff; BLOCK_SIZE], &buffer[..]); - - // Write one block full of 0x01 - volume - .write( - Block::new(0, BLOCK_SIZE.trailing_zeros()), - BytesMut::from(vec![0x01; BLOCK_SIZE].as_slice()), - ) - .await?; - - // Read one block: should be all 0x01 - let mut buffer = Buffer::new(1, BLOCK_SIZE); - volume - .read(Block::new(0, BLOCK_SIZE.trailing_zeros()), &mut buffer) - .await?; - - assert_eq!(vec![0x01; BLOCK_SIZE], &buffer[..]); - Ok(()) - } - #[tokio::test] async fn integration_test_just_read() -> Result<()> { // Just do a read of a new volume. @@ -4998,32 +4923,20 @@ mod test { #[tokio::test] async fn test_pantry_scrub() { - // Test scrubbing the OVMF image from a URL - // XXX httptest::Server does not support range requests, otherwise that - // should be used here instead. + // Test scrubbing random data from a file on disk. - let base_url = "https://oxide-omicron-build.s3.amazonaws.com"; - let url = format!("{}/OVMF_CODE_20220922.fd", base_url); + const BLOCK_SIZE: usize = 512; - let data = { - let dur = std::time::Duration::from_secs(25); - let client = reqwest::ClientBuilder::new() - .connect_timeout(dur) - .timeout(dur) - .build() - .unwrap(); + // There's no particular reason for this exact count other than, this + // test used to reference OVMF code over HTTP, and that OVMF code was + // this many 512-byte blocks in size. + const BLOCK_COUNT: usize = 3840; - client - .get(&url) - .send() - .await - .unwrap() - .bytes() - .await - .unwrap() - }; + let mut data = vec![0u8; BLOCK_SIZE * BLOCK_COUNT]; + rand::thread_rng().fill(data.as_mut_slice()); - const BLOCK_SIZE: usize = 512; + let mut parent_file = NamedTempFile::new().unwrap(); + parent_file.write_all(&data).unwrap(); // Spin off three downstairs, build our Crucible struct (with a // read-only parent pointing to the random data above) @@ -5033,11 +4946,12 @@ mod test { let volume_id = Uuid::new_v4(); let rop_id = Uuid::new_v4(); - let read_only_parent = Some(Box::new(VolumeConstructionRequest::Url { - id: rop_id, - block_size: BLOCK_SIZE as u64, - url: url.clone(), - })); + let read_only_parent = + Some(Box::new(VolumeConstructionRequest::File { + id: rop_id, + block_size: BLOCK_SIZE as u64, + path: parent_file.path().to_string_lossy().to_string(), + })); let vcr: VolumeConstructionRequest = VolumeConstructionRequest::Volume { diff --git a/openapi/crucible-pantry.json b/openapi/crucible-pantry.json index 1d5066f63..253e62353 100644 --- a/openapi/crucible-pantry.json +++ b/openapi/crucible-pantry.json @@ -687,35 +687,6 @@ "type" ] }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "type": { - "type": "string", - "enum": [ - "url" - ] - }, - "url": { - "type": "string" - } - }, - "required": [ - "block_size", - "id", - "type", - "url" - ] - }, { "type": "object", "properties": { diff --git a/tools/dtrace/README.md b/tools/dtrace/README.md index 1ae187e55..e53b93a11 100644 --- a/tools/dtrace/README.md +++ b/tools/dtrace/README.md @@ -264,15 +264,6 @@ dtrace: script 'perfgw.d' matched 6 probes 134217728 | ``` -## perf-reqwest.d -This is a simple dtrace script that measures latency times for reads -to a volume having a read only parent. The time is from when the -volume read only parent (ReqwestBlockIO) layer receives a read to when -that read has been completed. -``` -pfexec dtrace -s perf-reqwest.d -``` - ## perf-vol.d This dtrace script measures latency times for IOs at the volume layer. This is essentially where an IO first lands in crucible and is measured diff --git a/tools/dtrace/perf-reqwest.d b/tools/dtrace/perf-reqwest.d deleted file mode 100755 index 48e15758d..000000000 --- a/tools/dtrace/perf-reqwest.d +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Trace read ReqwestBlockIO. - */ -crucible_upstairs*:::reqwest-read-start -{ - start[arg0, json(copyinstr(arg1), "ok")] = timestamp; -} - -crucible_upstairs*:::reqwest-read-done -/start[arg0, json(copyinstr(arg1), "ok")]/ -{ - this->uuid = json(copyinstr(arg1), "ok"); - @time[this->uuid, "reqwest-read"] = quantize(timestamp - start[arg0, this->uuid]); - start[arg0, this->uuid] = 0; -} - -tick-5s -{ - printa(@time) -} diff --git a/upstairs/Cargo.toml b/upstairs/Cargo.toml index 0f6d15e6b..affc0a2f5 100644 --- a/upstairs/Cargo.toml +++ b/upstairs/Cargo.toml @@ -52,7 +52,6 @@ usdt.workspace = true uuid.workspace = true aes-gcm-siv.workspace = true rand_chacha.workspace = true -reqwest.workspace = true crucible-workspace-hack.workspace = true nexus-client = { workspace = true, optional = true } internal-dns = { workspace = true, optional = true } diff --git a/upstairs/src/block_io.rs b/upstairs/src/block_io.rs index 21fff1967..e322da3b1 100644 --- a/upstairs/src/block_io.rs +++ b/upstairs/src/block_io.rs @@ -4,7 +4,6 @@ use super::*; use std::fs::{File, OpenOptions}; use std::io::SeekFrom; -use std::sync::atomic::{AtomicU32, Ordering}; /// Implement BlockIO for a file pub struct FileBlockIO { @@ -121,175 +120,3 @@ impl BlockIO for FileBlockIO { }) } } - -// Implement BlockIO over an HTTP(S) url -use reqwest::header::{CONTENT_LENGTH, RANGE}; -use reqwest::Client; -use std::str::FromStr; - -pub struct ReqwestBlockIO { - uuid: Uuid, - block_size: u64, - total_size: u64, - client: Client, - url: String, - count: AtomicU32, // Used for dtrace probes -} - -impl ReqwestBlockIO { - pub async fn new( - id: Uuid, - block_size: u64, - url: String, - ) -> Result { - let client = Client::new(); - - let response = client - .head(&url) - .send() - .await - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - let content_length = response - .headers() - .get(CONTENT_LENGTH) - .ok_or("no content length!") - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - let total_size = u64::from_str( - content_length - .to_str() - .map_err(|e| CrucibleError::GenericError(e.to_string()))?, - ) - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - - Ok(Self { - uuid: id, - block_size, - total_size, - client, - url, - count: AtomicU32::new(0), - }) - } - - // Increment the counter to allow all IOs to have a unique number - // for dtrace probes. - pub fn next_count(&self) -> u32 { - self.count.fetch_add(1, Ordering::Relaxed) - } -} - -#[async_trait] -impl BlockIO for ReqwestBlockIO { - async fn activate(&self) -> Result<(), CrucibleError> { - Ok(()) - } - - async fn deactivate(&self) -> Result<(), CrucibleError> { - Ok(()) - } - - async fn query_is_active(&self) -> Result { - Ok(true) - } - - async fn total_size(&self) -> Result { - Ok(self.total_size) - } - - async fn get_block_size(&self) -> Result { - Ok(self.block_size) - } - - async fn get_uuid(&self) -> Result { - Ok(self.uuid) - } - - async fn read( - &self, - offset: Block, - data: &mut Buffer, - ) -> Result<(), CrucibleError> { - self.check_data_size(data.len()).await?; - let cc = self.next_count(); - cdt::reqwest__read__start!(|| (cc, self.uuid)); - - let start = offset.value * self.block_size; - - let response = self - .client - .get(&self.url) - .header( - RANGE, - format!("bytes={}-{}", start, start + data.len() as u64 - 1), - ) - .send() - .await - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - - let content_length = response - .headers() - .get(CONTENT_LENGTH) - .ok_or("no content length!") - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - let total_size = u64::from_str( - content_length - .to_str() - .map_err(|e| CrucibleError::GenericError(e.to_string()))?, - ) - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - - // Did the HTTP server _not_ honour the Range request? - if total_size != data.len() as u64 { - crucible_bail!( - IoError, - "Requested {} bytes but HTTP server returned {}!", - data.len(), - total_size - ); - } - - let bytes = response - .bytes() - .await - .map_err(|e| CrucibleError::GenericError(e.to_string()))?; - - data.write(0, &bytes); - - cdt::reqwest__read__done!(|| (cc, self.uuid)); - Ok(()) - } - - async fn write( - &self, - _offset: Block, - _data: BytesMut, - ) -> Result<(), CrucibleError> { - crucible_bail!(Unsupported, "write unsupported for ReqwestBlockIO") - } - - async fn write_unwritten( - &self, - _offset: Block, - _data: BytesMut, - ) -> Result<(), CrucibleError> { - crucible_bail!( - Unsupported, - "write_unwritten unsupported for ReqwestBlockIO" - ) - } - - async fn flush( - &self, - _snapshot_details: Option, - ) -> Result<(), CrucibleError> { - Ok(()) - } - - async fn show_work(&self) -> Result { - Ok(WQCounts { - up_count: 0, - ds_count: 0, - active_count: 0, - }) - } -} diff --git a/upstairs/src/lib.rs b/upstairs/src/lib.rs index ce350a1a3..bed833dcd 100644 --- a/upstairs/src/lib.rs +++ b/upstairs/src/lib.rs @@ -46,7 +46,7 @@ pub mod in_memory; pub use in_memory::InMemoryBlockIO; pub mod block_io; -pub use block_io::{FileBlockIO, ReqwestBlockIO}; +pub use block_io::FileBlockIO; pub mod block_req; pub(crate) use block_req::{BlockOpWaiter, BlockRes}; @@ -312,10 +312,6 @@ impl Debug for ReplaceResult { /// gw__*__done: An IO is completed and the Upstairs has sent the /// completion notice to the guest. /// -/// reqwest__read__[start|done] a probe covering BlockIO reqwest read -/// requests. These happen if a volume has a read only parent and either -/// there is no sub volume, or the sub volume did not contain any data. -/// /// volume__*__done: An IO is completed at the volume layer. #[usdt::provider(provider = "crucible_upstairs")] mod cdt { @@ -381,8 +377,6 @@ mod cdt { fn gw__noop__done(_: u64) {} fn gw__reopen__done(_: u64, _: usize) {} fn extent__or__done(_: u64) {} - fn reqwest__read__start(_: u32, _: Uuid) {} - fn reqwest__read__done(_: u32, _: Uuid) {} fn volume__read__done(_: u32, _: Uuid) {} fn volume__write__done(_: u32, _: Uuid) {} fn volume__writeunwritten__done(_: u32, _: Uuid) {} diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index 3b10f9a15..f24391243 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -998,19 +998,6 @@ impl Volume { Ok(vol) } - VolumeConstructionRequest::Url { - id, - block_size, - url, - } => { - let mut vol = Volume::new(block_size, log.clone()); - vol.add_subvolume(Arc::new( - ReqwestBlockIO::new(id, block_size, url).await?, - )) - .await?; - Ok(vol) - } - VolumeConstructionRequest::Region { block_size, blocks_per_extent, @@ -1115,12 +1102,6 @@ impl Volume { sub_volumes, read_only_parent, } => (id, block_size, sub_volumes, read_only_parent), - VolumeConstructionRequest::Url { .. } => { - crucible_bail!( - ReplaceRequestInvalid, - "Cannot replace URL VCR" - ) - } VolumeConstructionRequest::Region { .. } => { crucible_bail!( @@ -1145,12 +1126,6 @@ impl Volume { sub_volumes, read_only_parent, } => (id, block_size, sub_volumes, read_only_parent), - VolumeConstructionRequest::Url { .. } => { - crucible_bail!( - ReplaceRequestInvalid, - "Cannot replace URL VCR" - ) - } VolumeConstructionRequest::Region { .. } => { crucible_bail!(