[karencfv]

Adds a new internal SledEvacuating instance state to indicate that an instance has been stopped due to a sled evacuation. It differs to NoVmm in the sense that we know the instance will be automatically started again soon. In addition, we don't allow any changes (e.g. add/detach a disk, deletion, etc) during this state, and we map it to the external instance state Rebooting.

Also, the instance update saga is modified to account for VMMs in this new state.

NB: This work will be tested through #11380. Once that is merged, the vmm_stop_for_update background task's tests will fail since they currently check for a NoVmm state. This PR will include the changes in those tests. I just want to get some eyes in this PR since it's basically ready to go otherwise.

Depends on: #11380

Related: #11169

[hawkw]

<3

[hawkw]

Hmm, so this is a bit funny, because if we follow the intended meaning of this set of states, especially based on the comment, SledEvacuating should probably be added to this list of states, but...I really don't think it actually should be. Looking at the places where we actually consume NOT_INCARNATED_STATES, it is mostly used to determine whether you can change the instance's vCPU/memory config:

omicron/nexus/db-queries/src/db/datastore/instance.rs

Lines 1283 to 1300 in 8e361f6

	async fn instance_set_cpu_and_mem_on_conn(
	&self,
	conn: &async_bb8_diesel::Connection<DbConnection>,
	err: &OptionalError<Error>,
	authz_instance: &authz::Instance,
	ncpus: InstanceCpuCount,
	memory: ByteCount,
	cpu_platform: Option<InstanceCpuPlatform>,
	) -> Result<(), diesel::result::Error> {
	use nexus_db_schema::schema::instance::dsl as instance_dsl;
	let query = diesel::update(instance_dsl::instance)
	.into_boxed()
	.filter(instance_dsl::id.eq(authz_instance.id()))
	.filter(
	instance_dsl::state
	.eq_any(InstanceState::NOT_INCARNATED_STATES),
	);

...or change its boot disk:

omicron/nexus/db-queries/src/db/datastore/instance.rs

Lines 1188 to 1226 in 8e361f6

	async fn instance_set_boot_disk_on_conn(
	&self,
	conn: &async_bb8_diesel::Connection<DbConnection>,
	err: &OptionalError<Error>,
	authz_instance: &authz::Instance,
	boot_disk_id: Option<Uuid>,
	) -> Result<(), diesel::result::Error> {
	use nexus_db_schema::schema::disk::dsl as disk_dsl;
	use nexus_db_schema::schema::instance::dsl as instance_dsl;
	if let Some(disk_id) = boot_disk_id {
	// Ensure the disk is currently attached before updating the
	// database.
	let expected_state =
	api::external::DiskState::Attached(authz_instance.id());
	let attached_disk: Option<Uuid> = disk_dsl::disk
	.filter(disk_dsl::id.eq(disk_id))
	.filter(disk_dsl::attach_instance_id.eq(authz_instance.id()))
	.filter(disk_dsl::disk_state.eq(expected_state.label()))
	.select(disk_dsl::id)
	.first_async::<Uuid>(conn)
	.await
	.optional()?;
	if attached_disk.is_none() {
	return Err(
	err.bail(Error::conflict("boot disk must be attached"))
	);
	}
	}
	let query = diesel::update(instance_dsl::instance)
	.into_boxed()
	.filter(instance_dsl::id.eq(authz_instance.id()))
	.filter(
	instance_dsl::state
	.eq_any(InstanceState::NOT_INCARNATED_STATES),
	);

And I think it's probably safer to not let you mess with stuff if the VM is in the process of being restarted, because it just sort of may or may not actually work depending on whether you happen to click the button before or after we've started a new start saga, and that's kind of a disappointing experience. So...maybe it's worth a comment in here noting "it seems like SledEvacuating belongs here but ..." so that future readers can understand that (and why you might want to change it later if we decide to relax that restriction...)

[karencfv]

Yep! not allowing changes to the instance was one of the main drivers in using a new state here.

From #11169 (comment):

• NoVmm is part of InstanceState::NOT_INCARNATED_STATES. This constant is used to allow users to update cpu/memory and change the boot disk when the instance is in one of these states. We may want to allow users to update these parameters during an update in the future, but for now we should take small steps. During an update there are a lot of things going on, we should be conservative and disallow these changes to start with. Once we see the process running smoothly we can relax these constraints and allow users to update cpu/memory/boot disk during an update.
• Other actions like disk attach/detach, deletion, NIC changes and external IP attaches allow changes when the Instance is in a NoVmm state. for the same reasons above, we want to disallow those actions during an update for now.

I can add a comment though

[hawkw]

Yeah, I think that's right, I just figured we should explicitly mention it in case someone else comes along and wonders about it.

[hawkw]

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

[karencfv]

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)