Skip to content

Commit

Permalink
Use less IDs in paths (#117)
Browse files Browse the repository at this point in the history 
## Old Behavior

For most endpoints, we'd use the format: 

```
http://<IP ADDRESS>/instances/<INSTANCE UUID>/<rest of the path, maybe>
```

For many endpoints, this UUID would be redundant with values stored in `properties` struct, and a somewhat silly check would validate this. This path format added little value, other than requiring clients to supply a redundant value.

Why is the UUID redundant? Propolis' server only supports a *single* instance anyway, so the server address should be a sufficient factor for distinguishing instances.

## New Behavior

Don't bother supplying the instance UUID. Simplify a lot of "name -> UUID" pathways where this is no longer necessary.

Also, use `instance` instead of `instances` in the path. There should only be one!
  • Loading branch information
@smklein
smklein authored Apr 19, 2022
1 parent be664c6 commit abd372d
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 179 deletions.
84 changes: 19 additions & 65 deletions cli/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,32 +73,20 @@ enum Command {
},

/// Get the properties of a propolis instance
Get {
/// Instance name
name: String,
},
Get,

/// Transition the instance to a new state
State {
/// Instance name
name: String,

/// The requested state
#[structopt(parse(try_from_str = parse_state))]
state: InstanceStateRequested,
},

/// Drop to a Serial console connected to the instance
Serial {
/// Instance name
name: String,
},
Serial,

/// Migrate instance to new propolis-server
Migrate {
/// Instance name
name: String,

/// Destination propolis-server address
#[structopt(parse(try_from_str = resolve_host))]
dst_server: IpAddr,
Expand Down Expand Up @@ -193,16 +181,9 @@ async fn new_instance(
Ok(())
}

async fn get_instance(client: &Client, name: String) -> anyhow::Result<()> {
// Grab the Instance UUID
let id = client
.instance_get_uuid(&name)
.await
.with_context(|| anyhow!("failed to get instance UUID"))?;

// Get the rest of the Instance properties
async fn get_instance(client: &Client) -> anyhow::Result<()> {
let res = client
.instance_get(id)
.instance_get()
.await
.with_context(|| anyhow!("failed to get instance properties"))?;

Expand All @@ -213,17 +194,10 @@ async fn get_instance(client: &Client, name: String) -> anyhow::Result<()> {

async fn put_instance(
client: &Client,
name: String,
state: InstanceStateRequested,
) -> anyhow::Result<()> {
// Grab the Instance UUID
let id = client
.instance_get_uuid(&name)
.await
.with_context(|| anyhow!("failed to get instance UUID"))?;

client
.instance_state_put(id, state)
.instance_state_put(state)
.await
.with_context(|| anyhow!("failed to set instance state"))?;

Expand Down Expand Up @@ -342,18 +316,8 @@ async fn test_stdin_to_websockets_task() {
assert!(wsrx.recv().await.is_none());
}

async fn serial(
client: &Client,
addr: SocketAddr,
name: String,
) -> anyhow::Result<()> {
// Grab the Instance UUID
let id = client
.instance_get_uuid(&name)
.await
.with_context(|| anyhow!("failed to get instance UUID"))?;

let path = format!("ws://{}/instances/{}/serial", addr, id);
async fn serial(addr: SocketAddr) -> anyhow::Result<()> {
let path = format!("ws://{}/instance/serial", addr);
let (mut ws, _) = tokio_tungstenite::connect_async(path)
.await
.with_context(|| anyhow!("failed to create serial websocket stream"))?;
Expand Down Expand Up @@ -417,21 +381,15 @@ async fn serial(
async fn migrate_instance(
src_client: Client,
dst_client: Client,
src_name: String,
src_addr: SocketAddr,
dst_uuid: Uuid,
) -> anyhow::Result<()> {
// Grab the src instance UUID
let src_uuid = src_client
.instance_get_uuid(&src_name)
.await
.with_context(|| anyhow!("failed to get src instance UUID"))?;

// Grab the instance details
let src_instance = src_client
.instance_get(src_uuid)
.instance_get()
.await
.with_context(|| anyhow!("failed to get src instance properties"))?;
let src_uuid = src_instance.instance.properties.id;

let request = InstanceEnsureRequest {
properties: InstanceProperties {
Expand All @@ -452,11 +410,11 @@ async fn migrate_instance(

// Get the source instance ready
src_client
.instance_state_put(src_uuid, InstanceStateRequested::MigrateStart)
.instance_state_put(InstanceStateRequested::MigrateStart)
.await
.with_context(|| {
anyhow!("failed to place src instance in migrate start state")
})?;
anyhow!("failed to place src instance in migrate start state")
})?;

// Initiate the migration via the destination instance
let migration_id = dst_client
Expand All @@ -475,10 +433,8 @@ async fn migrate_instance(
.map(|(role, client, id)| {
tokio::spawn(async move {
loop {
let state = client
.instance_migrate_status(id, migration_id)
.await?
.state;
let state =
client.instance_migrate_status(migration_id).await?.state;
println!("{}({}) migration state={:?}", role, id, state);
if state == MigrationState::Finish {
return Ok::<_, anyhow::Error>(());
Expand Down Expand Up @@ -542,16 +498,14 @@ async fn main() -> anyhow::Result<()> {
)
.await?
}
Command::Get { name } => get_instance(&client, name).await?,
Command::State { name, state } => {
put_instance(&client, name, state).await?
}
Command::Serial { name } => serial(&client, addr, name).await?,
Command::Migrate { name, dst_server, dst_port, dst_uuid } => {
Command::Get => get_instance(&client).await?,
Command::State { state } => put_instance(&client, state).await?,
Command::Serial => serial(addr).await?,
Command::Migrate { dst_server, dst_port, dst_uuid } => {
let dst_addr = SocketAddr::new(dst_server, dst_port);
let dst_client = Client::new(dst_addr, log.clone());
let dst_uuid = dst_uuid.unwrap_or_else(Uuid::new_v4);
migrate_instance(client, dst_client, name, addr, dst_uuid).await?
migrate_instance(client, dst_client, addr, dst_uuid).await?
}
}

Expand Down
25 changes: 5 additions & 20 deletions client/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,37 +109,25 @@ impl Client {
&self,
request: &api::InstanceEnsureRequest,
) -> Result<api::InstanceEnsureResponse, Error> {
let path = format!(
"http://{}/instances/{}",
self.address, request.properties.id
);
let path = format!("http://{}/instance", self.address,);
let body = Body::from(serde_json::to_string(&request).unwrap());
self.put(path, Some(body)).await
}

/// Returns information about an instance, by UUID.
pub async fn instance_get(
&self,
id: Uuid,
) -> Result<api::InstanceGetResponse, Error> {
let path = format!("http://{}/instances/{}", self.address, id);
self.get(path, None).await
}

/// Gets instance UUID, by name.
pub async fn instance_get_uuid(&self, name: &str) -> Result<Uuid, Error> {
let path = format!("http://{}/instances/{}/uuid", self.address, name);
let path = format!("http://{}/instance", self.address);
self.get(path, None).await
}

/// Long-poll for state changes.
pub async fn instance_state_monitor(
&self,
id: Uuid,
gen: u64,
) -> Result<api::InstanceStateMonitorResponse, Error> {
let path =
format!("http://{}/instances/{}/state-monitor", self.address, id);
let path = format!("http://{}/instance/state-monitor", self.address);
let body = Body::from(
serde_json::to_string(&api::InstanceStateMonitorRequest { gen })
.unwrap(),
Expand All @@ -150,22 +138,19 @@ impl Client {
/// Puts an instance into a new state.
pub async fn instance_state_put(
&self,
id: Uuid,
state: api::InstanceStateRequested,
) -> Result<(), Error> {
let path = format!("http://{}/instances/{}/state", self.address, id);
let path = format!("http://{}/instance/state", self.address);
let body = Body::from(serde_json::to_string(&state).unwrap());
self.put_no_response(path, Some(body)).await
}

/// Get the status of an ongoing migration
pub async fn instance_migrate_status(
&self,
id: Uuid,
migration_id: Uuid,
) -> Result<api::InstanceMigrateStatusResponse, Error> {
let path =
format!("http://{}/instances/{}/migrate/status", self.address, id);
let path = format!("http://{}/instance/migrate/status", self.address);
let body = Body::from(
serde_json::to_string(&api::InstanceMigrateStatusRequest {
migration_id,
Expand Down
12 changes: 1 addition & 11 deletions server/src/lib/migrate/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ struct Device {
/// connection and begin the migration in a separate task.
pub async fn source_start(
rqctx: Arc<RequestContext<Context>>,
instance_id: Uuid,
migration_id: Uuid,
) -> Result<Response<Body>, MigrateError> {
// Create a new log context for the migration
Expand All @@ -258,10 +257,6 @@ pub async fn source_start(
let context =
context.as_mut().ok_or_else(|| MigrateError::InstanceNotInitialized)?;

if instance_id != context.properties.id {
return Err(MigrateError::UuidMismatch);
}

// Bail if the instance hasn't been preset to Migrate Start state.
if !matches!(
context.instance.current_state(),
Expand Down Expand Up @@ -359,7 +354,6 @@ pub async fn source_start(
/// process (destination-side).
pub async fn dest_initiate(
rqctx: Arc<RequestContext<Context>>,
instance_id: Uuid,
migrate_info: api::InstanceMigrateInitiateRequest,
) -> Result<api::InstanceMigrateInitiateResponse, MigrateError> {
let migration_id = migrate_info.migration_id;
Expand All @@ -376,10 +370,6 @@ pub async fn dest_initiate(
let context =
context.as_mut().ok_or_else(|| MigrateError::InstanceNotInitialized)?;

if instance_id != context.properties.id {
return Err(MigrateError::UuidMismatch);
}

let mut migrate_task = rqctx.context().migrate_task.lock().await;

// This should be a fresh propolis-server
Expand All @@ -388,7 +378,7 @@ pub async fn dest_initiate(
// TODO: https
// TODO: We need to make sure the src_addr is a valid target
let src_migrate_url = format!(
"http://{}/instances/{}/migrate/start",
"http://{}/instance/{}/migrate/start",
migrate_info.src_addr, migrate_info.src_uuid
);
info!(log, "Begin migration"; "src_migrate_url" => &src_migrate_url);
Expand Down
Loading

0 comments on commit abd372d

Please sign in to comment.