disk creation saga needs different mechanism to wait on creation

[davepacheco]

In the last two Friday demos we tried a disk creation that appeared to work, but Nexus's state said "creating" for way longer than expected (minutes). The reason appeared to be exponential backoff in the saga, with each timeout triggering an attempt to create the disk again (using a request that would succeed if the disk were already created):

omicron/nexus/src/sagas.rs

Lines 1228 to 1271 in fd3dab1

	let region_request = CreateRegion {
	block_size: region.block_size().to_bytes(),
	extent_count: region.extent_count().try_into().unwrap(),
	extent_size: region.blocks_per_extent().try_into().unwrap(),
	// TODO: Can we avoid casting from UUID to string?
	// NOTE: This'll require updating the crucible agent client.
	id: RegionId(region.id().to_string()),
	encrypted: region.encrypted(),
	cert_pem: None,
	key_pem: None,
	root_pem: None,
	};
	let create_region = || async {
	let region = client
	.region_create(&region_request)
	.await
	.map_err(|e| BackoffError::Permanent(e.into()))?;
	match region.state {
	RegionState::Requested => Err(BackoffError::transient(anyhow!(
	"Region creation in progress"
	))),
	RegionState::Created => Ok(region),
	_ => Err(BackoffError::Permanent(anyhow!(
	"Failed to create region, unexpected state: {:?}",
	region.state
	))),
	}
	};
	let log_create_failure = |_, delay| {
	warn!(
	log,
	"Region requested, not yet created. Retrying in {:?}", delay
	);
	};
	let region = backoff::retry_notify(
	backoff::internal_service_policy(),
	create_region,
	log_create_failure,
	)
	.await
	.map_err(|e| Error::internal_error(&e.to_string()))?;

I don't think it makes sense to use exponential backoff for this case because there's no indication that the remote side is overloaded -- it just isn't finished yet. The internal_service_policy() being used at L1266 is intended for background connections or requests to internal services where we want to retry indefinitely -- definitely nothing latency-sensitive. We could create one that's much more aggressive (e.g., once/second), but really, this isn't retrying a failure, it's just waiting for something to happen. There are other ways to do this:

1. For instance creation, we have the sled send us a notification when the instance state changes. I like this pattern because it also works for the case where the state changes unexpectedly (e.g., a bhyve crash), though I think it creates a different problem if the notification is received by a different Nexus than the one operating the saga.
2. Josh suggested a long poll -- instead of having the request complete immediately with a status meaning "it's not ready yet", it could wait a bounded amount of time (say, 30 seconds). If the request completes within that, the server responds immediately saying so. If not, it responds with "not ready yet" and the client immediately retries.

[smklein]

For comparison to the instance creation case, we do the following:

1. Call instance_set_runtime during sic_instance_ensure.
2. Call instance_put with all the pre-requisite instance information
3. Within the sled agent, this synchronously starts the instance, and only returns once it completes.

However, we also get updates whenever the instance state changes, even after this initial request. This is because the sled agent itself implements a long-polling call to propolis, which only receives a response on state change.

TL;DR:

• I think using a non-exponential backoff poll would be a potential solution to this issue
• I think long-polling from Nexus -> Crucible would be a potential solution to this issue
• I think this implementation is actually similar to the instance creation saga, as both synchronously wait for completion

[davepacheco]

For comparison to the instance creation case, we do the following:

1. Call [instance_set_runtime](https://github.com/oxidecomputer/omicron/blob/d2bf956eb3d8c74e634668062ed96ae26ac9e566/nexus/src/sagas.rs#L830-L839) during `sic_instance_ensure`.

2. Call [instance_put](https://github.com/oxidecomputer/omicron/blob/d2bf956eb3d8c74e634668062ed96ae26ac9e566/nexus/src/nexus.rs#L1884-L1894) with all the pre-requisite instance information

3. Within the sled agent, this _synchronously_ starts the instance, and only returns once it completes.

Hmm. When I did the initial work on the mock sled agent, the intent of instance_put was to return as soon as possible with an acknowledgement of the request and an intermediate state (usually Starting). Then subsequently Sled Agent would emit a notification (in the form of an HTTP request back to Nexus) when the VM changed state (usually to Running). The reason is mainly that in my experience, VM boot can take an arbitrarily long time when things aren't working well, and that's when you want a clear understanding of status and the ability to keep asking about it, both at the Nexus and Sled Agent levels. Also, TCP/HTTP aren't that well-suited to arbitrarily-long requests -- connections remain in use for the duration, transient failures disrupt them and the client doesn't know what the state is (not a big deal here because it's idempotent), it's hard to tell if the server has just forgotten about it, etc.

When you say that the initial instance_put request synchronously starts the instance, do you mean that it waits for it to reach "running"? That'd be different from what I think the mock sled agent does, and I think that'd be a problem for these reasons.

However, we also get updates whenever the instance state changes, even after this initial request. This is because the sled agent itself implements a long-polling call to propolis, which only receives a response on state change.

TL;DR:

* I think using a non-exponential backoff poll would be a potential solution to this issue

While I agree that non-exponential polling would be an improvement, it still adds significant unnecessary latency to every provision. More than this specific case, I'm worried we'll replicate it elsewhere too. This feels like how you wind up with provisions taking 20 seconds, most of which are spent sleeping.

* I think long-polling from Nexus -> Crucible would be a potential solution to this issue

* I think this implementation is actually similar to the instance creation saga, as both synchronously wait for completion

Aren't we here because this implementation uses exponential backoff polling, not a synchronously request?

[smklein]

Here's the callstack on the sled agent side of things:

• instance_put
• SledAgent::instance_ensure
• InstanceManager::ensure

Within InstanceManager::ensure, this part is particularly notable:

omicron/sled-agent/src/instance_manager.rs

Lines 158 to 168 in d2bf956

	// If we created a new instance, start or migrate it - but do so outside
	// the "instances" lock, since initialization may take a while.
	//
	// Additionally, this makes it possible to manage the "instance_ticket",
	// which might need to grab the lock to remove the instance during
	// teardown.
	if let Some(instance_ticket) = maybe_instance_ticket {
	instance.start(instance_ticket, migrate).await?;
	}
	instance.transition(target).await.map_err(|e| e.into())

Instance::start creates a Propolis Zone, starts the Propolis service, sends an "instance_ensure" request to the Propolis server.

This is "synchronous" in the sense that it waits for the Propolis server to be up and running, and for it to receive the request. It's "asynchronous", I suppose, in the sense that we don't actually wait for it to be "running", just "created".

[smklein]

Part of the reason "instance monitoring" works well on the instance side of things - where the sled agent long-polls into propolis, and notifies nexus of state changes - is that the lifetime of an "instance" is always a subset of the "sled agent" it's running on (modulo live migration, but in this context I'd consider that to be a "different instance").

In comparison, if we create a disk, it may exist on crucible downstairs services spread across multiple sleds. The lifetime is not coupled with an instance, and it makes less sense for any particular sled to be responsible for polling the disk status. Theoretically Nexus could be constantly long-polling to all disks in perpetuity, but that seems like a fairly high cost.

Alternatively, the underlying crucible downstairs service could make an upcall to Nexus, triggering state change updates, without an intermediary?

[davepacheco]

I'm confused about what we're talking about now -- I thought we were talking about requests that Nexus makes to Crucible Agent while creating a disk? That's different from general monitoring of disk state (at least potentially), and I'm not sure why sled agent would be the client?

[smklein]

In the context of "what can we do for disk creation", yes, we could definitely just have Nexus long-poll to Crucible once, and have no further mechanism for receiving notifications about state changes.

However, I thought you were suggesting something akin to the instance creation saga, where we would return when the disk is in the "creating" state, and update it to the "running" state at some later point in time?

[davepacheco]

Yeah, so for that case, I would think whatever was responsible for doing those transitions would call back to Nexus.

Would it make sense to schedule a discussion? Or is this feeling like an unhelpful line of discussion?