Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions nexus/db-model/src/instance_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ impl_enum_type!(
Vmm => b"vmm"
Failed => b"failed"
Destroyed => b"destroyed"
SledEvacuating => b"sled_evacuating"
);

impl InstanceState {
Expand All @@ -49,6 +50,7 @@ impl InstanceState {
InstanceState::Vmm => "VMM",
InstanceState::Failed => "failed",
InstanceState::Destroyed => "destroyed",
InstanceState::SledEvacuating => "sled_evacuating",
}
}

Expand All @@ -71,6 +73,7 @@ impl From<InstanceState> for instance::InstanceState {
InstanceState::Vmm => Output::Running,
InstanceState::Failed => Output::Failed,
InstanceState::Destroyed => Output::Destroyed,
InstanceState::SledEvacuating => Output::Rebooting,
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
///
/// This must be updated when you change the database schema. Refer to
/// schema/crdb/README.adoc in the root of this repository for details.
pub const SCHEMA_VERSION: Version = Version::new(307, 0, 0);
pub const SCHEMA_VERSION: Version = Version::new(308, 0, 0);

/// List of all past database schema versions, in *reverse* order
///
Expand All @@ -28,6 +28,7 @@ pub static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
// | leaving the first copy as an example for the next person.
// v
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
KnownVersion::new(308, "instance-state-sled-evacuating"),
KnownVersion::new(307, "add-rack-id-to-inv-sled-agent"),
KnownVersion::new(306, "inv-service-processor-rack-id"),
KnownVersion::new(305, "lookup-sled-by-bp-availability"),
Expand Down
13 changes: 13 additions & 0 deletions nexus/db-model/src/vmm_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,11 @@ impl<'s> InstanceStateComputer<'s> {
// - An instance with no VMM is always "stopped" (as long as it's
// not "starting" etc.)
(InstanceState::NoVmm, None) => instance::InstanceState::Stopped,
// - An instance with no VMM because it was in a sled that is being
Comment thread
karencfv marked this conversation as resolved.
Resolved
// evacuated is always "rebooting"
(InstanceState::SledEvacuating, None) => {
instance::InstanceState::Rebooting
}
Comment on lines +368 to +370

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

<3

// - The instance should not be in `InstanceState::Vmm` if there is
// no active VMM record, this is probably a bug, but return
// `InstanceState::Stopped`, because that's basically true
Expand All @@ -370,6 +375,14 @@ impl<'s> InstanceStateComputer<'s> {
);
instance::InstanceState::Stopped
}
(InstanceState::SledEvacuating, _vmm_state) => {
debug_assert_eq!(
_vmm_state, None,
"if the instance is in `InstanceState::SledEvacuating`, \
there should be no VMM state"
);
instance::InstanceState::Rebooting
}
Comment thread
hawkw marked this conversation as resolved.
Resolved
// - If there's no VMM state, use the instance's state.
(instance_state, None) => instance_state.into(),
(instance_state, _vmm_state) => {
Expand Down
4 changes: 3 additions & 1 deletion nexus/src/app/sagas/instance_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,9 @@ pub(super) async fn networking_resource_instance_state(
// - starting: see below.
match (found_instance_state, found_vmm_state) {
// If there's no VMM, the instance is definitely not on any sled.
(InstanceState::NoVmm, _) | (_, Some(VmmState::SagaUnwound)) => {
(InstanceState::NoVmm, _)
| (InstanceState::SledEvacuating, _)
| (_, Some(VmmState::SagaUnwound)) => {
propolis_and_sled_id = None;
}

Expand Down
51 changes: 35 additions & 16 deletions nexus/src/app/sagas/instance_update/mod.rs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was going to say that if we want to have the instance_reincarnation task reincarnate SledEvacuated instances, we will also want to make the final saga node activate it (just to reduce the latency for when the VM gets restarted), here:
https://github.com/karencfv/omicron/blob/abf4fa55f5980ff261be1f70323828b67200c303/nexus/src/app/sagas/instance_update/mod.rs#L1622-L1630
...but after actually looking at that code again, I see that it is just checking if the reincarnation task should be activated by calling should_reincarnate(), and the subsequent change to make these instances eligible to be restarted by the instance-reincarnation task will change that anyway, so we don't actually need to update that function separately from making that change.

HOWEVER, I did notice there's a log line there that says "transitioned to Failed" which we should probably change to say the actual state we're transitioning the instance to, because it might not be Failed: https://github.com/karencfv/omicron/blob/abf4fa55f5980ff261be1f70323828b67200c303/nexus/src/app/sagas/instance_update/mod.rs#L1633-L1635

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up! I'll make those changes in #11504 (this PR is just a placeholder right now with a couple of changes, so not ready at all)

Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ impl UpdatesRequired {

let mut update_required = false;
let mut active_vmm_failed = false;
let mut active_vmm_update_disposition_gen = None;
let mut network_config = None;
let mut update_sled_reservations_for_migration_success = None;

Expand All @@ -564,23 +565,31 @@ impl UpdatesRequired {
// instance's new state to `Failed` rather than to `NoVmm`.
if active_vmm.state == VmmState::Failed {
active_vmm_failed = true;
} else if snapshot.instance.intended_state
== InstanceIntendedState::Running
&& snapshot.migration.is_none()
{
// Did the active VMM shut itself down, when it was
// intended to be running *and* we were not migrating
// out? If so, update the instance's intended state to
// indicate that it stopped itself.
new_intent = Some(InstanceIntendedState::GuestShutdown);
info!(
log,
"instance update (active VMM destroyed): guest shut \
itself down";
"instance_id" => %instance_id,
"propolis_id" => %active_vmm.id,
);
}
// If the active VMM is marked as stopped for an update, its
// sled is being evacuated. Move the instance's new state to
// `SledEvacuating` rather than to `NoVmm`.
else if let Some(generation) = active_vmm
.stop_for_update_disposition_generation {
active_vmm_update_disposition_gen = Some(generation);

} else if snapshot.instance.intended_state
== InstanceIntendedState::Running
&& snapshot.migration.is_none()
Comment thread
karencfv marked this conversation as resolved.
Resolved
{
// Did the active VMM shut itself down, when it was
// intended to be running *and* we were not migrating
// out? If so, update the instance's intended state to
// indicate that it stopped itself.
new_intent = Some(InstanceIntendedState::GuestShutdown);
info!(
log,
"instance update (active VMM destroyed): guest shut \
itself down";
"instance_id" => %instance_id,
"propolis_id" => %active_vmm.id,
);
}

Some(id)
} else {
Expand Down Expand Up @@ -740,6 +749,16 @@ impl UpdatesRequired {
// We no longer have a VMM.
new_runtime.nexus_state = if active_vmm_failed {
InstanceState::Failed
} else if let Some(g) = active_vmm_update_disposition_gen {
debug!(
Comment thread
karencfv marked this conversation as resolved.
Resolved
log,
"instance update: VMM was stopped due to its sled being \
evacuated for an update. Setting new runtime instance \
state to SledEvacuating.";
"instance_id" => %instance_id,
"update_disposition_generation" => ?g
);
InstanceState::SledEvacuating
} else {
InstanceState::NoVmm
};
Expand Down
10 changes: 8 additions & 2 deletions schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1315,7 +1315,13 @@ CREATE TYPE IF NOT EXISTS omicron.public.instance_state_v2 AS ENUM (
'failed',

/* The instance has been destroyed. */
'destroyed'
'destroyed',

/*
* The instance has no active VMM due to a sled being evacutated.
* Corresponds to the "rebooting" external state.
*/
'sled_evacuating'
);

CREATE TYPE IF NOT EXISTS omicron.public.vmm_state AS ENUM (
Expand Down Expand Up @@ -9567,7 +9573,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '307.0.0', NULL)
(TRUE, NOW(), NOW(), '308.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
7 changes: 7 additions & 0 deletions schema/crdb/instance-state-sled-evacuating/up1.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
ALTER TYPE
omicron.public.instance_state_v2
ADD VALUE IF NOT EXISTS
'sled_evacuating'
AFTER
'destroyed'
;