Rip InstanceRuntimeState out of the internal API#6556
Merged
Conversation
Passing the `InstanceEnsureBody` all the way through to the instance-runner task, instead of unpacking it and passing its pieces as arguments, lets us get rid of the `#[allow::clippy_too_many_arguments]` that are sprinkled around. It also means that if a subsequent change adds additional fields to the instance-ensure payload, we don't need to change all these function signatures just to add them.
gjcolombo
approved these changes
Sep 11, 2024
Co-authored-by: Greg Colombo <greg@oxidecomputer.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
omicron_common::api::internal::InstanceRuntimeStatetype was oncea key component of how Nexus and sled-agents communicated about
instances. Now, however, it's outlived its usefulness: #5749 changed
Nexus'
cpapi_instances_putand sled-agent'sinstance_getAPIs tooperate exclusively on
VmmRuntimeStates rather thanInstanceRuntimeState, with the sled-agent almost completely unaware ofInstanceRuntimeState.At present, the
InstanceRuntimeStatetype remains in the internal APIjust because it's used in the
InstanceEnsureBodytype, which is sentto sled-agent's
instance_ensure_registeredAPI when a new VMM isregistered. However, looking at the code that
instance_ensure_registeredcalls into in sled-agent, we see that theonly thing from the
InstanceRuntimeStatethat the sled-agent actuallyuses is the migration ID of the current migration (if the instance is
migrating in). Soooo...we can just replace the whole
InstanceRuntimeStatethere with amigration_id: Option<Uuid>. Oncethat's done, there are now no longer any internal APIs in Nexus or in
sled-agent that actually use
InstanceRuntimeState, so the type can beremoved from the internal API entirely. All of the code in Nexus that
deals with instance runtime states already has changed to operate
exclusively on the
nexus_db_modelversion of the struct, so removingthe API type is actually quite straightforward.
In addition, I've refactored the sled-agent
instance_ensure_registeredcode paths a little bit to pass around the
InstanceEnsureBody, ratherthan unpacking its fields at the HTTP entrypoint and then passing them
all through the chain of method calls that actually gets it to the
instance-runner task. Just passing the struct around means we can remove
a bunch of
#[allow(clippy::too_many_arguments)]attributes. It alsomakes changing the contents of the instance-ensure payload a bit easier,
as we'll no longer need to change the arguments list in the whole maze
of tiny little functions, all alike, that pass around all the pieces of
the
InstanceEnsureBody.