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.

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

Split out mock propolis-server from real thing #556

Merged
merged 1 commit into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ jobs:
- uses: dtolnay/rust-toolchain@439cf607258077187679211f12aa6f19af4a0af7 # master @ 2023-10-08
with:
toolchain: stable
- name: Build mock-only server
run: cargo build --bin propolis-server --features mock-only
- name: Build
run: cargo build --verbose
- name: Build mock-only server
run: cargo build -p propolis-mock-server --verbose
- name: Test Libraries
run: cargo test --lib --verbose

24 changes: 23 additions & 1 deletion Cargo.lock

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

38 changes: 38 additions & 0 deletions bin/mock-server/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
[package]
name = "propolis-mock-server"
version = "0.0.0"
license = "MPL-2.0"
edition = "2021"

[lib]
name = "propolis_mock_server"
path = "src/lib/lib.rs"
doc = false
doctest = false
test = false

[[bin]]
name = "propolis-mock-server"
path = "src/main.rs"
doc = false
doctest = false
test = false

[dependencies]
atty.workspace = true
anyhow.workspace = true
clap = { workspace = true, features = ["derive"] }
base64.workspace = true
dropshot = { workspace = true }
futures.workspace = true
hyper.workspace = true
propolis-client = { workspace = true, features = ["generated"] }
serde_json.workspace = true
slog.workspace = true
slog-async.workspace = true
slog-dtrace.workspace = true
slog-term.workspace = true
slog-bunyan.workspace = true
thiserror.workspace = true
tokio = { workspace = true, features = ["full"] }
tokio-tungstenite.workspace = true
64 changes: 64 additions & 0 deletions bin/mock-server/src/lib/copied.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

If rust-analyzer is giving me an accurate picture of who's using things in this module, I would just drop it entirely: the only reason you need ServerSpecBuilderError is to serve as the return type to slot_to_pci_path, and in the mock, none of that function's callers actually care what error variant they got or about emitting their own ServerSpecBuilderError to a caller. So I'd just copy slot_to_pci_path to lib.rs, change its error type, and call it a day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you're alright with that change being slightly delayed, my follow-on for this does clean up that error case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm suggesting something more radical than what's in 4919115: not only can we drop the InnerBuilderError variant from mock_server::copied::ServerSpecBuilderError, I suspect we can get rid of the entire enum and just have mock_server::copied::slot_to_pci_path return some other error type for its Err variant (at which point I'd probably just get rid of the copied module entirely and just move the private copy of slot_to_pci_path to the lib).

All that said, this is just cleanup and aesthetics, no need to hold this particular PR for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you're happy with clean-up like that happening later, then I'm inclined to move forward with things as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we can tackle that in a later PR.

Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// 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/.

//! Bits copied from propolis-server, rather than splitting them out into some
//! shared dependency

use propolis_client::handmade::api;
use propolis_client::instance_spec::{v0::builder::SpecBuilderError, PciPath};

use thiserror::Error;

#[derive(Clone, Copy, Debug)]
pub(crate) enum SlotType {
Nic,
Disk,
CloudInit,
}

#[allow(unused)]
#[derive(Debug, Error)]
pub(crate) enum ServerSpecBuilderError {
#[error(transparent)]
InnerBuilderError(#[from] SpecBuilderError),

#[error("The string {0} could not be converted to a PCI path")]
PciPathNotParseable(String),

#[error(
"Could not translate PCI slot {0} for device type {1:?} to a PCI path"
)]
PciSlotInvalid(u8, SlotType),

#[error("Unrecognized storage device interface {0}")]
UnrecognizedStorageDevice(String),

#[error("Unrecognized storage backend type {0}")]
UnrecognizedStorageBackend(String),

#[error("Device {0} requested missing backend {1}")]
DeviceMissingBackend(String, String),

#[error("Error in server config TOML: {0}")]
ConfigTomlError(String),

#[error("Error serializing {0} into spec element: {1}")]
SerializationError(String, serde_json::error::Error),
}

pub(crate) fn slot_to_pci_path(
slot: api::Slot,
ty: SlotType,
) -> Result<PciPath, ServerSpecBuilderError> {
match ty {
// Slots for NICS: 0x08 -> 0x0F
SlotType::Nic if slot.0 <= 7 => PciPath::new(0, slot.0 + 0x8, 0),
// Slots for Disks: 0x10 -> 0x17
SlotType::Disk if slot.0 <= 7 => PciPath::new(0, slot.0 + 0x10, 0),
// Slot for CloudInit
SlotType::CloudInit if slot.0 == 0 => PciPath::new(0, slot.0 + 0x18, 0),
_ => return Err(ServerSpecBuilderError::PciSlotInvalid(slot.0, ty)),
}
.map_err(|_| ServerSpecBuilderError::PciSlotInvalid(slot.0, ty))
}
Loading
Loading