Expunging a physically removed disk wedges sled-agent #7622
Labels
Sled Agent
Related to the Per-Sled Configuration and Management
Comments
jgallagher
added
the
Sled Agent
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
(This is a derived from one of the issues described in https://github.com/oxidecomputer/colo/issues/102.)
During the colo R13 upgrade, blueprint execution was failing due to a nonexistent disk:
This disk was physically removed a long time ago (February 2024), but was still marked as in-service and active in the database and the blueprint.
We expunged the disk via
omdb, generated a new blueprint, and made it the target. Execution still failed, but now due to a timeout trying toPUT /omicron-physical-disksto the sled where that disk was removed:I believe the request itself is correct: it no longer notes the disk that was removed (serial number
A079E79D). The timeout is the same symptom we saw in #6904, but the logs (and the cause) are different in this case. Grepping for the sequence ofphysical disks ensurelogs insideSledAgent::omicron_physical_disks_ensure()and honing in on the point at which we switched fromGeneration(2)toGeneration(3)(i.e., the new config that omits the removed disk):The logs continue like this: in all subsequent requests, we see the
Updated storageandPropagating new generation of diskslogs for generation 3, but we never see the next expected log,Updated storage monitor. This strongly implies we're getting stuck insideStorageMonitorHandle::await_generation(). That method useswatch::Receiver::wait_for()with no timeout, which will wait indefinitely for the condition to be true. I think this means we're "leaking" a tokio task on everyPUT /omicron-physical-diskscall from a blueprint executor RPW, which the logs appear to confirm. Prior to the switch toGeneration(3), we see manyrequest completedlogs; e.g.,After we start receiving the
Generation(3)requests, we see the warnings from dropshot that the client is disconnecting (with alatency_usconsistent with the 60 second timeout Nexus uses for its sled-agent clients):but we never see the logs dropshot would emit once the handler actually completes (
request completed after handler was already cancelled), which is consistent with all of those requests still being stuck in thewait_for()call, still waiting for the storage monitor to realize we're now at generation 3.It's kinda hard to follow how the storage monitor is supposed to get updates here. Working backwards, I think what is supposed to happen is:
StorageMonitor::handle_resource_update()updates the watch channel with a new generation after updating the dump deviceStorageMonitor::run()callshandle_resource_update()whenStorageManager::wait_for_changes()returnsStorageManager::wait_for_changes()is itself waiting for changes on a different watch channel:self.disk_updatesself.disk_updateschannel is the receiving side; the sending side comes fromStorageResourcesStorageResourceswhere new values are sent into thedisk_updateschannel, but I believe only one of them is in thePUT /omicron-physical-diskspath: the one at the end ofsynchronize_disk_management()(becausesynchronize_disk_managementis called byomicron_physical_disks_ensure_internalwhich is ultimately called by the endpoint handler)The
disk_updateswrite insynchronize_disk_management()is conditional on theupdatedboolean, and I think it is not set to true in the "Nexus told us to stop using a disk that is no longer physically present" case. There are only two places where it's set to true: if we start managing a new disk, which is not applicable in this case, or if we stop managing an old disk. If this disk was still present, we would be in the latter case, but it's not present in thedisksmap at all, which the logs confirm: prior to our move to generation 3, we see the warnings emitted in the "the control plane told us to use a disk we don't have" branch:I'm not sure what the fix here should be. The generation inside
self.disksis written inset_config. I'm not sure if it would be correct to send an update to thedisks_updatedchannel there, before any actual disk management changes have taken place? Maybesynchronize_disk_management()should be told whether the generation has changed (somehow?) and seed theupdatedboolean with that value? Also on the table: we have a couple issues in this area that are leading toward major rework of thePUTendpoints (both to merge zones+disks+datasets into one request, #7309, and to make the handling of those requests more asynchronous, #5086), and we've had several bugs like this one (some of which are still open, such as #7546) that we could address as part of that rework. (A goal of that rework should be to simplify a lot of this; tracing disk updates through multiple long-running async tasks and chained watch channels makes it pretty hard to figure out what's supposed to be happening.)The text was updated successfully, but these errors were encountered: