Skip to content

Commit

Permalink
api: one ensure API to rule them all
Browse files Browse the repository at this point in the history
Remove the existing instance ensure APIs and collapse them into a single
API that takes a set of instance properties and a mechanism for creating
the instance, which can be either creating a new VM from a spec or
initializing as a live migration target. In the latter case, the client
can also supply a list of instance spec components that the client
wishes to override the components in the source's spec. This gives
Omicron a way to replace backend information (Crucible generation
number, host VNIC names) that changes over live migration.

Remove all of the instance ensure types that supported the old ensure
interface. One notable consequence of this is that the `DiskRequest`
type also caused propolis-server's versions of Crucible's
`VolumeConstructionRequest` and `CrucibleOpts` types to appear in the
generated client. Omicron code tends to pull these types in from
propolis-client (often transitively through sled-agent-client). This is
a useful way of making sure Omicron components serialize/deserialize
VCRs the same way Propolis does, so preserve it by re-exporting these
types from propolis-client.

Remove the parts of the mock server that validated disk, NIC, and
cloud-init volume requests. The Omicron tests that use the mock server
primarily check that its serial console interface works as expected and
don't heavily exercise these checks. (Omicron CI's end-to-end test will
fail if sled-agent mishandles the Propolis ensure API.)

Finally, remove a few other unused types from the propolis_api_types
crate.

Tests: cargo test, PHD, driving ad hoc VMs with propolis-cli.
  • Loading branch information
gjcolombo committed Dec 2, 2024
1 parent 8c0f774 commit d8967c7
Show file tree
Hide file tree
Showing 23 changed files with 578 additions and 1,356 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 0 additions & 1 deletion bin/mock-server/src/lib/api_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ impl TryFrom<types::PciPath> for propolis_types::PciPath {
.map_err(|e| e.to_string())
}
}
pub use propolis_types::PciPath;

// Duplicate the parameter types for the endpoints related to the serial console

Expand Down
60 changes: 0 additions & 60 deletions bin/mock-server/src/lib/copied.rs

This file was deleted.

68 changes: 3 additions & 65 deletions bin/mock-server/src/lib/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,23 @@

//! Implementation of a mock Propolis server
use std::io::{Error as IoError, ErrorKind};
use std::sync::Arc;

use base64::Engine;
use dropshot::{
channel, endpoint, ApiDescription, HttpError, HttpResponseCreated,
HttpResponseOk, HttpResponseUpdatedNoContent, Query, RequestContext,
TypedBody, WebsocketConnection,
};
use futures::SinkExt;
use slog::{error, info, o, Logger};
use slog::{error, o, Logger};
use thiserror::Error;
use tokio::sync::{watch, Mutex};
use tokio_tungstenite::tungstenite::protocol::{Role, WebSocketConfig};
use tokio_tungstenite::tungstenite::Message;
use tokio_tungstenite::WebSocketStream;

mod api_types;
mod copied;

use crate::copied::{slot_to_pci_path, SlotType};
use api_types::types as api;
use api_types::types::{self as api, InstanceEnsureRequest};

#[derive(Debug, Eq, PartialEq, Error)]
pub enum Error {
Expand Down Expand Up @@ -141,12 +136,7 @@ async fn instance_ensure(
) -> Result<HttpResponseCreated<api::InstanceEnsureResponse>, HttpError> {
let server_context = rqctx.context();
let request = request.into_inner();
let (properties, nics, disks, cloud_init_bytes) = (
request.properties,
request.nics,
request.disks,
request.cloud_init_bytes,
);
let InstanceEnsureRequest { properties, .. } = request;

// Handle an already-initialized instance
let mut instance = server_context.instance.lock().await;
Expand All @@ -160,58 +150,6 @@ async fn instance_ensure(
migrate: None,
}));
}

// Perform some basic validation of the requested properties
for nic in &nics {
info!(server_context.log, "Creating NIC: {:#?}", nic);
slot_to_pci_path(nic.slot, SlotType::Nic).map_err(|e| {
let err = IoError::new(
ErrorKind::InvalidData,
format!("Cannot parse vnic PCI: {}", e),
);
HttpError::for_internal_error(format!(
"Cannot build instance: {}",
err
))
})?;
}

for disk in &disks {
info!(server_context.log, "Creating Disk: {:#?}", disk);
slot_to_pci_path(disk.slot, SlotType::Disk).map_err(|e| {
let err = IoError::new(
ErrorKind::InvalidData,
format!("Cannot parse disk PCI: {}", e),
);
HttpError::for_internal_error(format!(
"Cannot build instance: {}",
err
))
})?;
info!(server_context.log, "Disk {} created successfully", disk.name);
}

if let Some(cloud_init_bytes) = &cloud_init_bytes {
info!(server_context.log, "Creating cloud-init disk");
slot_to_pci_path(api::Slot(0), SlotType::CloudInit).map_err(|e| {
let err = IoError::new(ErrorKind::InvalidData, e.to_string());
HttpError::for_internal_error(format!(
"Cannot build instance: {}",
err
))
})?;
base64::engine::general_purpose::STANDARD
.decode(cloud_init_bytes)
.map_err(|e| {
let err = IoError::new(ErrorKind::InvalidInput, e.to_string());
HttpError::for_internal_error(format!(
"Cannot build instance: {}",
err
))
})?;
info!(server_context.log, "cloud-init disk created");
}

*instance = Some(InstanceContext::new(properties, &server_context.log));
Ok(HttpResponseCreated(api::InstanceEnsureResponse { migrate: None }))
}
Expand Down
Loading

0 comments on commit d8967c7

Please sign in to comment.