Skip to content

Comments

Adds Builder and VM Manager interfaces + implementation for UVM#2597

Open
rawahars wants to merge 4 commits intomicrosoft:mainfrom
rawahars:feat/vm-package
Open

Adds Builder and VM Manager interfaces + implementation for UVM#2597
rawahars wants to merge 4 commits intomicrosoft:mainfrom
rawahars:feat/vm-package

Conversation

@rawahars
Copy link
Contributor

This pull request introduces a major refactor and formalization of the Utility VM (UVM) builder and management interfaces. It separates the responsibilities for VM configuration, host-side management, and guest-side actions into clear, testable layers. The changes include a new, well-documented builder interface, VM manager interface, concrete builder and VM manager implementations, along with comprehensive unit tests for the builder's functionality.

The most important changes are:

Architectural Refactor and Documentation

  • Added a detailed README.md to the internal/vm package, clearly explaining the separation of concerns between the Builder, VM Manager, and Guest Manager layers, and documenting the responsibilities and flow for each layer.

Builder Interface and Implementation

  • Replaced the old UVMBuilder interface with a new, comprehensive Builder interface in internal/vm/builder.go, providing modular access to memory, processor, NUMA, device, boot, and storage QoS configuration managers.
  • Introduced a concrete implementation of the Builder interface in internal/vm/builder/builder.go for creating and configuring hcsschema.ComputeSystem documents.

VM Manager Interface and Implementation

  • Created a new comprehensive UVM interface which corresponds to VM manager functionality.
  • All the VM host side modification are responsibility of this interface.

Testing

  • Added comprehensive unit tests for builder verification.

These changes establish a foundation for future VM configuration and management features.

Refactor UVM responsibilities into clear builder vs vmmanager packages and
document the intended 3-layer VM model.

Key changes:
- Introduce internal/vm/builder for HCS compute system construction.
- Introduce internal/vm/vmmanager for host-side lifecycle/resource control.
- Move UVM interfaces and shared types into focused locations.
- Add internal/vm/README.md describing layer boundaries and usage flow.
- Remove legacy internal/vm/hcs files and rename/move implementations.

Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
request := &hcsschema.ModifySettingRequest{
ResourcePath: fmt.Sprintf(resourcepaths.VirtualPCIResourceFormat, vmbusGUID),
RequestType: guestrequest.RequestTypeAdd,
Settings: hcsschema.VirtualPciDevice{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should actually have the API for the PCI device, and then this one is actually add function


var _ ProcessorOptions = (*UtilityVM)(nil)

func (uvmb *UtilityVM) SetProcessorLimits(config *hcsschema.VirtualMachineProcessor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is odd. If you had the VirtualMachineProcessor you would have already set its .Count/.Limit/,Weight and they would be in the builder right? Seems like if we need this overload we would want it to reflect a smaller set of just count/limit/weight overrides

case vm.Windows:
doc.VirtualMachine.Devices.VirtualSmb = &hcsschema.VirtualSmb{}
case vm.Linux:
doc.VirtualMachine.Devices.Plan9 = &hcsschema.Plan9{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this at the time of its use right? We dont want 9pfs if the caller has no shares right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC these are added pre-emptively so that HCS can perform warm-up logic on its end.

Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have dedicated types for Windows and Linux builders and managers?
that way, we can specialize via function calls

type UtilityVM interface {
	BootOptions 
	DeviceOptions 
	// ...
}

type _ UtilityVM = &Linux 
type _ UtilityVM = &Windows

type Linux struct {
	builder
}

type Windows struct {
	builder
}

type builder struct {
	doc             *hcsschema.ComputeSystem
	assignedDevices map[hcsschema.VirtualPciFunction]*vPCIDevice
}

// ...

func (b *builder) SetUEFIBoot(bootEntry *hcsschema.UefiBootEntry) {
	// ...
}

func (b *Linux) SetLinuxKernelDirectBoot(options *hcsschema.LinuxKernelDirect) error {
	if osversion.Get().Build < 18286 {
		return errors.New("Linux kernel direct boot requires at least Windows version 18286")
	}
	uvmb.doc.VirtualMachine.Chipset.LinuxKernelDirect = options
	return nil
}

func (b *Windows) SetLinuxKernelDirectBoot(options *hcsschema.LinuxKernelDirect) error {
	return ErrUnsupportedOperation
}

// ...

"github.com/pkg/errors"
)

func (uvmb *UtilityVM) AddVSMB(settings hcsschema.VirtualSmb) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i though vSMB was windows only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes vSMB is Windows only.

The idea of builder is to have a generic HCS Config builder. It's the responsibility of the caller i.e. VMController to configure the builder correctly.

Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants