Prototype general VMMS API - #4469
Draft
Hamza El-Saawy (helsaawy) wants to merge 1 commit into
Draft
Hamza El-Saawy (helsaawy) wants to merge 1 commit into
Hamza El-Saawy (helsaawy) wants to merge 1 commit into
Conversation
Expand VM proto API to support Container Platform scenarios. Specifically: - allow multiple SCSI controllers; - add event stream to monitor VM and resource state across VM lifetime Configure formatting for `*.proto` files. [Use TitleCase][id_case] for `Vm`, `Scsi`, and `Pcie` in message and RPC call names. Add `_UNSPECIFIED` [enum value][enum_unspec] and rename enum values to be [UPPER_SNAKE_CASE][enum_case]. [id_case]: https://protobuf.dev/programming-guides/style/#identifier [enum_unspec]: https://protobuf.dev/best-practices/dos-donts/#unspecified-enum [enum_case]: https://protobuf.dev/programming-guides/style/#enums Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Copilot started reviewing on behalf of
Hamza El-Saawy (helsaawy)
September 16, 2026 19:08
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical API compatibility, data-model, and event-stream issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Expands the VM protobuf API for multiple SCSI controllers, lifecycle events, resource/state schemas, and proto formatting.
Changes:
- Adds event, resource, state, and SCSI definitions.
- Renames and expands VM service RPCs and messages.
- Updates build tracking, dependencies, linting, and formatting.
File summaries
| File | Description |
|---|---|
xtask/src/tasks/fmt/lints/workspaced.rs |
Adjusts diff-only workspace linting. |
openvmm/openvmm_ttrpc_vmservice/src/vmservice.state.proto |
Defines VM lifecycle states. |
openvmm/openvmm_ttrpc_vmservice/src/vmservice.scsi.proto |
Defines SCSI controllers and disks. |
openvmm/openvmm_ttrpc_vmservice/src/vmservice.resource.proto |
Defines resource enums. |
openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto |
Expands and renames the VM management API. |
openvmm/openvmm_ttrpc_vmservice/src/vmservice.events.proto |
Defines VM event subscriptions and payloads. |
openvmm/openvmm_ttrpc_vmservice/src/lib.rs |
Retains generated type dependencies. |
openvmm/openvmm_ttrpc_vmservice/Cargo.toml |
Adds prost-types. |
openvmm/openvmm_ttrpc_vmservice/build.rs |
Tracks protobuf inputs for rebuilds. |
Cargo.lock |
Records the new dependency. |
.clang-format |
Adds protobuf formatting configuration. |
Review details
Suppressed comments (5)
openvmm/openvmm_ttrpc_vmservice/src/vmservice.events.proto:90
VmEventhas noEventTypefield; the event kind is encoded by thepayloadoneof below. This comment describes a discriminator that does not exist in the wire contract and can mislead API consumers.
// The payload type may match the EventType field, or could be an Error.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.events.proto:26
- These new filter fields are plural except for
include_resource_event, while the comment describes multiple resource events. The singular name is inconsistent with the rest of this request and will leak into every generated client; rename it toinclude_resource_eventsbefore this API is consumed.
optional bool include_resource_event = 4;
openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:101
- These fields use upper-camel identifiers even though protobuf field names should be lower_snake_case; the generated API will expose a style exception for only these three booleans. Use lowercase field names while the schema is still being revised.
bool Add = 1;
bool Remove = 2;
bool Update = 3;
openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:68
- The RPC documentation is grammatically malformed and does not clearly state when events are delivered versus when the stream closes. Rewrite it to describe the stream as delivering transitions during the VM lifetime and closing after
TeardownVm.
// SubscribeVMEvents will block until the VM is has its all of it's resources freed via TeardownVm,
// returning a stream events describing VM state transitions and other changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.state.proto:12
VM_STATE_UNSPECIFIEDalready represents an unknown/unset state, so documentingVM_STATE_CRASHEDas also covering unknown states makes clients report failures for an indeterminate state. Keep this value limited to an actual crash and useUNSPECIFIEDfor unknown.
VM_STATE_CRASHED = 1; // Crashed, or in an unknown state.
- Files reviewed: 10/11 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| rpc CapabilitiesVM(google.protobuf.Empty) returns (CapabilitiesVMResponse); | ||
|
|
||
| // PropertiesVM will take in a list of properties that the virtstack will return | ||
| rpc ServiceCapabilities(google.protobuf.Empty) returns (ServiceCapabilitiesResponse); |
|
|
||
| // SubscribeVMEvents will block until the VM is has its all of it's resources freed via TeardownVm, | ||
| // returning a stream events describing VM state transitions and other changes. | ||
| rpc SubscribeVmEvents(SubscribeVmEventsRequest) returns (stream VmEvent); |
| repeated NICConfig nic_config = 3; | ||
| // SCSI disks, mapped by the SCSI controller GUID to add them under. | ||
| // An empty string will denote a default GUID controller (e.g., ba6163d9-04a1-4d29-b605-72e2ffb1dc7f). | ||
| map<string, ScsiDisk> scsi_disks = 1; |
Comment on lines
+652
to
654
| enum VpmemDiskType { | ||
| VPMEM_DISK_UNSPECIFIED = 0; | ||
| } |
Comment on lines
+27
to
+28
| // Include events for VM requests. | ||
| optional bool include_request_events = 5; |
|
|
||
| // Represents VM state. | ||
| enum VmState { | ||
| VM_STATE_UNSPECIFIED = 0; |
Comment on lines
+174
to
+178
| if !self.only_diffed { | ||
| for member in self.members.iter() { | ||
| content.unfixable(&format!( | ||
| "workspace member {} does not exist", | ||
| member.display() |
| VmEventInitiator initiator = 2; | ||
| } | ||
|
|
||
| // VmResourceEvent describes an event assoicated with a particular VM resource. |
Comment on lines
+42
to
+48
| // CreateVm will create the virtual machine with the configuration in the CreateVmRequest. | ||
| // The virtual machine will be in a paused state, power wise, after CreateVm. | ||
| // ResumeVm can be called to transition the VM into a running state. | ||
| // | ||
| // Additionally, the server will send a VM_STATE_PAUSED VmEvent on the SubscribeVmEvents | ||
| // stream to indicate completion of VM creation. | ||
| rpc CreateVm(CreateVmRequest) returns (google.protobuf.Empty); |
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
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.
Expand VM proto API to support Container Platform scenarios.
Specifically:
Configure formatting for
*.protofiles.Use TitleCase for
Vm,Scsi, andPciein message and RPC call names.Add
_UNSPECIFIEDenum value and rename enum values to be UPPER_SNAKE_CASE.