Skip to content

Commit

Permalink
Standardize on "propolis_id" name in most places
Browse files Browse the repository at this point in the history
Use 'propolis_id' to refer to the ID of a VMM object, except in parts of the
datastore where it is very clear that what's being passed is the primary key
into the Vmm table.
  • Loading branch information
gjcolombo committed Oct 6, 2023
1 parent e688552 commit 78f72e5
Show file tree
Hide file tree
Showing 14 changed files with 42 additions and 38 deletions.
2 changes: 1 addition & 1 deletion clients/nexus-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ impl From<omicron_common::api::internal::nexus::SledInstanceState>
) -> Self {
Self {
instance_state: s.instance_state.into(),
vmm_id: s.vmm_id,
propolis_id: s.propolis_id,
vmm_state: s.vmm_state.into(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion clients/sled-agent-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl From<types::SledInstanceState>
fn from(s: types::SledInstanceState) -> Self {
Self {
instance_state: s.instance_state.into(),
vmm_id: s.vmm_id,
propolis_id: s.propolis_id,
vmm_state: s.vmm_state.into(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion common/src/api/internal/nexus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub struct SledInstanceState {
pub instance_state: InstanceRuntimeState,

/// The ID of the VMM whose state is being reported.
pub vmm_id: Uuid,
pub propolis_id: Uuid,

/// The most recent state of the sled's VMM process.
pub vmm_state: VmmRuntimeState,
Expand Down
4 changes: 2 additions & 2 deletions nexus/db-queries/src/db/queries/network_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1812,10 +1812,10 @@ mod tests {
async fn instance_set_active_vmm(
db_datastore: &DataStore,
mut instance: Instance,
vmm_id: Option<Uuid>,
propolis_id: Option<Uuid>,
) -> Instance {
let new_runtime = model::InstanceRuntimeState {
propolis_id: vmm_id,
propolis_id,
gen: instance.runtime_state.gen.next().into(),
..instance.runtime_state.clone()
};
Expand Down
36 changes: 20 additions & 16 deletions nexus/src/app/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,7 @@ impl super::Nexus {
opctx: &OpContext,
authz_instance: &authz::Instance,
db_instance: &db::model::Instance,
vmm_id: &Uuid,
propolis_id: &Uuid,
initial_vmm: &db::model::Vmm,
) -> Result<(), Error> {
opctx.authorize(authz::Action::Modify, authz_instance).await?;
Expand Down Expand Up @@ -1002,7 +1002,7 @@ impl super::Nexus {
hardware: instance_hardware,
instance_runtime: db_instance.runtime().clone().into(),
vmm_runtime: initial_vmm.clone().into(),
propolis_id: *vmm_id,
propolis_id: *propolis_id,
propolis_addr: SocketAddr::new(
initial_vmm.propolis_ip.ip(),
PROPOLIS_PORT,
Expand Down Expand Up @@ -1064,15 +1064,15 @@ impl super::Nexus {
.instance_and_vmm_update_runtime(
instance_id,
&new_state.instance_state.into(),
&new_state.vmm_id,
&new_state.propolis_id,
&new_state.vmm_state.into(),
)
.await;

slog::debug!(&self.log,
"Attempted DB update after instance PUT";
"instance_id" => %instance_id,
"vmm_id" => %new_state.vmm_id,
"propolis_id" => %new_state.propolis_id,
"result" => ?update_result);

update_result
Expand Down Expand Up @@ -1276,12 +1276,12 @@ impl super::Nexus {
new_runtime_state: &nexus::SledInstanceState,
) -> Result<(), Error> {
let log = &self.log;
let vmm_id = new_runtime_state.vmm_id;
let propolis_id = new_runtime_state.propolis_id;

info!(log, "received new runtime state from sled agent";
"instance_id" => %instance_id,
"instance_state" => ?new_runtime_state.instance_state,
"propolis_id" => %vmm_id,
"propolis_id" => %propolis_id,
"vmm_state" => ?new_runtime_state.vmm_state);

// Update OPTE and Dendrite if the instance's active sled assignment
Expand Down Expand Up @@ -1322,7 +1322,7 @@ impl super::Nexus {
&db::model::InstanceRuntimeState::from(
new_runtime_state.instance_state.clone(),
),
&vmm_id,
&propolis_id,
&db::model::VmmRuntimeState::from(
new_runtime_state.vmm_state.clone(),
),
Expand All @@ -1332,24 +1332,28 @@ impl super::Nexus {
// If the VMM is now in a terminal state, make sure its resources get
// cleaned up.
if let Ok((_, true)) = result {
let vmm_terminated = matches!(
let propolis_terminated = matches!(
new_runtime_state.vmm_state.state,
InstanceState::Destroyed | InstanceState::Failed
);

if vmm_terminated {
if propolis_terminated {
info!(log, "vmm is terminated, cleaning up resources";
"instance_id" => %instance_id,
"propolis_id" => %vmm_id);
"propolis_id" => %propolis_id);

self.db_datastore
.sled_reservation_delete(opctx, vmm_id)
.sled_reservation_delete(opctx, propolis_id)
.await?;

if !self.db_datastore.vmm_mark_deleted(opctx, &vmm_id).await? {
if !self
.db_datastore
.vmm_mark_deleted(opctx, &propolis_id)
.await?
{
warn!(log, "failed to mark vmm record as deleted";
"instance_id" => %instance_id,
"propolis_id" => %vmm_id,
"propolis_id" => %propolis_id,
"vmm_state" => ?new_runtime_state.vmm_state);
}
}
Expand All @@ -1359,7 +1363,7 @@ impl super::Nexus {
Ok((instance_updated, vmm_updated)) => {
info!(log, "instance and vmm updated by sled agent";
"instance_id" => %instance_id,
"propolis_id" => %vmm_id,
"propolis_id" => %propolis_id,
"instance_updated" => instance_updated,
"vmm_updated" => vmm_updated);
Ok(())
Expand All @@ -1372,7 +1376,7 @@ impl super::Nexus {
error!(log, "instance/vmm update unexpectedly returned \
an object not found error";
"instance_id" => %instance_id,
"vmm_id" => %vmm_id);
"propolis_id" => %propolis_id);
Ok(())
}

Expand All @@ -1383,7 +1387,7 @@ impl super::Nexus {
Err(error) => {
warn!(log, "failed to update instance from sled agent";
"instance_id" => %instance_id,
"vmm_id" => %vmm_id,
"propolis_id" => %propolis_id,
"error" => ?error);
Err(error)
}
Expand Down
10 changes: 5 additions & 5 deletions nexus/src/app/sagas/instance_migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ async fn sim_create_vmm_record(

info!(osagactx.log(), "creating vmm record for migration destination";
"instance_id" => %instance_id,
"vmm_id" => %propolis_id,
"propolis_id" => %propolis_id,
"sled_id" => %sled_id);

super::instance_common::create_and_insert_vmm_record(
Expand All @@ -231,7 +231,7 @@ async fn sim_destroy_vmm_record(

let vmm = sagactx.lookup::<db::model::Vmm>("dst_vmm_record")?;
info!(osagactx.log(), "destroying vmm record for migration unwind";
"vmm_id" => %vmm.id);
"propolis_id" => %vmm.id);

super::instance_common::destroy_vmm_record(
osagactx.datastore(),
Expand Down Expand Up @@ -260,7 +260,7 @@ async fn sim_set_migration_ids(
"instance_id" => %db_instance.id(),
"sled_id" => %src_sled_id,
"migration_id" => %migration_id,
"dst_vmm_id" => %dst_propolis_id,
"dst_propolis_id" => %dst_propolis_id,
"prev_runtime_state" => ?db_instance.runtime());

let updated_record = osagactx
Expand Down Expand Up @@ -338,7 +338,7 @@ async fn sim_ensure_destination_propolis(

info!(osagactx.log(), "ensuring migration destination vmm exists";
"instance_id" => %db_instance.id(),
"dst_vmm_id" => %vmm.id,
"dst_propolis_id" => %vmm.id,
"dst_vmm_state" => ?vmm);

let (.., authz_instance) = LookupPath::new(&opctx, &osagactx.datastore())
Expand Down Expand Up @@ -438,7 +438,7 @@ async fn sim_instance_migrate(
info!(osagactx.log(), "initiating migration from destination sled";
"instance_id" => %db_instance.id(),
"dst_vmm_record" => ?dst_vmm,
"src_vmm_id" => %src_propolis_id);
"src_propolis_id" => %src_propolis_id);

// TODO-correctness: This needs to be retried if a transient error occurs to
// avoid a problem like the following:
Expand Down
4 changes: 2 additions & 2 deletions nexus/src/app/sagas/instance_start.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ async fn sis_ensure_registered(
let instance_id = db_instance.id();
let sled_id = sagactx.lookup::<Uuid>("sled_id")?;
let vmm_record = sagactx.lookup::<db::model::Vmm>("vmm_record")?;
let vmm_id = sagactx.lookup::<Uuid>("propolis_id")?;
let propolis_id = sagactx.lookup::<Uuid>("propolis_id")?;

info!(osagactx.log(), "start saga: ensuring instance is registered on sled";
"instance_id" => %instance_id,
Expand All @@ -471,7 +471,7 @@ async fn sis_ensure_registered(
&opctx,
&authz_instance,
&db_instance,
&vmm_id,
&propolis_id,
&vmm_record,
)
.await
Expand Down
4 changes: 2 additions & 2 deletions nexus/tests/integration_tests/instances.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3372,13 +3372,13 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) {
.fetch()
.await
.unwrap();
let vmm_id = db_instance
let propolis_id = db_instance
.runtime()
.propolis_id
.expect("running instance should have vmm");
let localhost = std::net::IpAddr::V6(std::net::Ipv6Addr::LOCALHOST);
let updated_vmm = datastore
.vmm_overwrite_ip_for_test(&opctx, &vmm_id, localhost.into())
.vmm_overwrite_ip_for_test(&opctx, &propolis_id, localhost.into())
.await
.unwrap();
assert_eq!(updated_vmm.propolis_ip.ip(), localhost);
Expand Down
4 changes: 2 additions & 2 deletions openapi/nexus-internal.json
Original file line number Diff line number Diff line change
Expand Up @@ -4965,7 +4965,7 @@
}
]
},
"vmm_id": {
"propolis_id": {
"description": "The ID of the VMM whose state is being reported.",
"type": "string",
"format": "uuid"
Expand All @@ -4981,7 +4981,7 @@
},
"required": [
"instance_state",
"vmm_id",
"propolis_id",
"vmm_state"
]
},
Expand Down
4 changes: 2 additions & 2 deletions openapi/sled-agent.json
Original file line number Diff line number Diff line change
Expand Up @@ -2720,7 +2720,7 @@
}
]
},
"vmm_id": {
"propolis_id": {
"description": "The ID of the VMM whose state is being reported.",
"type": "string",
"format": "uuid"
Expand All @@ -2736,7 +2736,7 @@
},
"required": [
"instance_state",
"vmm_id",
"propolis_id",
"vmm_state"
]
},
Expand Down
2 changes: 1 addition & 1 deletion sled-agent/src/common/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ impl InstanceStates {
SledInstanceState {
instance_state: self.instance.clone(),
vmm_state: self.vmm.clone(),
vmm_id: self.propolis_id,
propolis_id: self.propolis_id,
}
}

Expand Down
2 changes: 1 addition & 1 deletion sled-agent/src/sim/collection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ mod test {
let state = SledInstanceState {
instance_state: instance_vmm,
vmm_state,
vmm_id: propolis_id,
propolis_id,
};

SimObject::new_simulated_auto(&state, logctx.log.new(o!()))
Expand Down
2 changes: 1 addition & 1 deletion sled-agent/src/sim/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ impl Simulatable for SimInstance {
state: InstanceStates::new(
current.instance_state,
current.vmm_state,
current.vmm_id,
current.propolis_id,
),
last_response: InstanceStateMonitorResponse {
gen: 1,
Expand Down
2 changes: 1 addition & 1 deletion sled-agent/src/sim/sled_agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ impl SledAgent {
SledInstanceState {
instance_state: instance_runtime,
vmm_state: vmm_runtime,
vmm_id: propolis_id,
propolis_id,
},
None,
)
Expand Down

0 comments on commit 78f72e5

Please sign in to comment.