Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ReqwestBlockIO and VolumeConstructionRequest::Url #1293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove ReqwestBlockIO and VolumeConstructionRequest::Url #1293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 0 additions & 5 deletions crucible-client-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ pub enum VolumeConstructionRequest {
sub_volumes: Vec<VolumeConstructionRequest>,
read_only_parent: Option<Box<VolumeConstructionRequest>>,
},
Url {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I think we need to confirm that there's no existing VolumeConstructionRequest in the wild with this variant - removing this means that record won't deserialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah agreed. i think its unlikely but we've still got to check

id: Uuid,
block_size: u64,
url: String,
},
Region {
block_size: u64,
blocks_per_extent: u64,
Expand Down
120 changes: 17 additions & 103 deletions integration_tests/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
29 changes: 0 additions & 29 deletions openapi/crucible-pantry.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
9 changes: 0 additions & 9 deletions tools/dtrace/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 0 additions & 20 deletions tools/dtrace/perf-reqwest.d
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion upstairs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
Loading