-
Notifications
You must be signed in to change notification settings - Fork 43
sled-agent: move instance configuration generation to Nexus #8002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also want to test manually that Propolis-directed region replacements still work as intended with this change (they depend on the virtual platform module having used the relevant disk record's ID as the relevant Propolis backend ID).
This will need a fresh commit hash/SHA from the Propolis repo after oxidecomputer/propolis#899 merges, but I think it is otherwise more or less ready for review (though it could probably use some unit tests of the new virtual platform logic...). |
@hawkw - thanks for taking a look! I think all your feedback is addressed in 31ee29c. I've also updated this change to point to the latest mainline Propolis commit, which picks up propolis#899 (re-export Propolis client types for sled-agent to use in its API), propolis#900 (Crucible bump), and propolis#901 (packaging fix). This change now updates Omicron's Crucible dependency as well (cc @leftwo). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me --- I dunno if @iximeow wants to review this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for giving me a second to take a look through this too. it's good!!
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] | ||
pub struct InstanceHardware { | ||
pub properties: InstanceProperties, | ||
pub struct InstanceSledLocalConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like splitting apart "externally-visible foundation for this VM" and "stuff we need to plumb for the VM to have the universe a user thinks it should". i dunno how much you intended that up front, but InstanceHardware
being both was a little confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I stumbled across this half by accident (ISTR it fell out of an earlier draft where I was still trying to use some generated propolis-client types for certain things). But I'm with you--I'm really happy with how it turned out!
let pci_path = slot_to_pci_bdf(slot, PciDeviceKind::Disk)?; | ||
let device = ComponentV0::NvmeDisk(NvmeDisk { | ||
backend_id: SpecKey::Uuid(disk.id()), | ||
pci_path, | ||
serial_number: zero_padded_nvme_serial_from_str( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah having a box to put this kind of Weird Stuff seems like a really nice separation too.
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] | ||
pub struct InstanceHardware { | ||
pub properties: InstanceProperties, | ||
pub struct InstanceSledLocalConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I stumbled across this half by accident (ISTR it fell out of an earlier draft where I was still trying to use some generated propolis-client types for certain things). But I'm with you--I'm really happy with how it turned out!
5d08f98
to
4443230
Compare
One of the determinations in RFD 505 is that Nexus should be the component that's in charge of determining how to configure a VM given a set of database records describing its instance (the Instance itself, its attached Disks and NetworkInterfaces, etc.). To summarize the rationale in the RFD, the hope is that this will promote two nice properties:
To achieve this:
The main pain point in this change is that sled-agent's API now includes types that it picked up from the propolis-client API, which caused sled-agent's OpenAPI document to balloon with "duplicate" schema descriptions it inherited from propolis-client's generated types. I'm not sure if there's a great way around this (aside from changing the generated Propolis client to
replace
all its generated types with their "native" counterparts); I'm open to suggestions here.Tested by booting a VM in a dev cluster, booting a comparable VM on rack2, and comparing their instance specs (as returned by Propolis's
/instance/spec
API) to make sure they specified the same components with the same configuration.