From 6db0e48a3536f9357f58a1137a6e3bb22bb8f7f3 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Mon, 16 Mar 2026 15:32:05 +0530 Subject: [PATCH] fix CPUGroup, VPMEM controller and StorageQoS parsing The CPUGroup was being set unconditionally while it needs to be set only if the value is non-empty. StorageQoS should only be set if IopsMaximum and BandwidthMaximum are both non-zero. If VPMEM count is greater than 0, we must initialize the controller. Signed-off-by: Harsh Rawat --- internal/builder/vm/lcow/devices.go | 44 ++++---- internal/builder/vm/lcow/specs.go | 19 ++-- internal/builder/vm/lcow/specs_test.go | 134 +++++++++++++++++++++++++ internal/builder/vm/lcow/topology.go | 9 +- 4 files changed, 176 insertions(+), 30 deletions(-) diff --git a/internal/builder/vm/lcow/devices.go b/internal/builder/vm/lcow/devices.go index 2175f56086..ef668c7371 100644 --- a/internal/builder/vm/lcow/devices.go +++ b/internal/builder/vm/lcow/devices.go @@ -68,33 +68,37 @@ func parseDeviceOptions( // Create VPMem controller configuration var vpMemController *hcsschema.VirtualPMemController - if vpmemCount > 0 && rootFsFile == vmutils.VhdFile { - // If booting from VHD via VPMem, configure the VPMem device for rootfs + if vpmemCount > 0 { + // Initialize VPMem controller with specified count and size. vpMemController = &hcsschema.VirtualPMemController{ MaximumCount: vpmemCount, MaximumSizeBytes: vpmemSize, - Devices: make(map[string]hcsschema.VirtualPMemDevice), } - // Determine image format based on file extension. - // filepath.Ext returns the extension with the leading dot (e.g. ".vhdx"). - imageFormat := "Vhd1" - if strings.HasSuffix(strings.ToLower(filepath.Ext(rootFsFile)), "vhdx") { - imageFormat = "Vhdx" - } + // If booting from VHD via VPMem, configure the VPMem device for rootfs + if rootFsFile == vmutils.VhdFile { + vpMemController.Devices = make(map[string]hcsschema.VirtualPMemDevice) + + // Determine image format based on file extension. + // filepath.Ext returns the extension with the leading dot (e.g. ".vhdx"). + imageFormat := "Vhd1" + if strings.HasSuffix(strings.ToLower(filepath.Ext(rootFsFile)), "vhdx") { + imageFormat = "Vhdx" + } - // Add rootfs VHD as VPMem device 0 - vpMemController.Devices["0"] = hcsschema.VirtualPMemDevice{ - HostPath: rootFsFullPath, - ReadOnly: true, - ImageFormat: imageFormat, - } + // Add rootfs VHD as VPMem device 0 + vpMemController.Devices["0"] = hcsschema.VirtualPMemDevice{ + HostPath: rootFsFullPath, + ReadOnly: true, + ImageFormat: imageFormat, + } - log.G(ctx).WithFields(logrus.Fields{ - "device": "0", - "path": rootFsFullPath, - "imageFormat": imageFormat, - }).Debug("configured VPMem device for VHD rootfs boot") + log.G(ctx).WithFields(logrus.Fields{ + "device": "0", + "path": rootFsFullPath, + "imageFormat": imageFormat, + }).Debug("configured VPMem device for VHD rootfs boot") + } } // ===============================Parse SCSI configuration=============================== diff --git a/internal/builder/vm/lcow/specs.go b/internal/builder/vm/lcow/specs.go index ff67ab6a74..71f9b4bb6f 100644 --- a/internal/builder/vm/lcow/specs.go +++ b/internal/builder/vm/lcow/specs.go @@ -353,17 +353,22 @@ func parseSandboxOptions(ctx context.Context, platform string, annotations map[s func parseStorageQOSOptions(ctx context.Context, annotations map[string]string) (*hcsschema.StorageQoS, error) { log.G(ctx).Debug("parseStorageQOSOptions: starting storage QOS options parsing") - storageQOSConfig := &hcsschema.StorageQoS{ - IopsMaximum: oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSIopsMaximum, 0), - BandwidthMaximum: oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSBandwidthMaximum, 0), - } + iopsMaximum := oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSIopsMaximum, 0) + bandwidthMaximum := oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSBandwidthMaximum, 0) log.G(ctx).WithFields(logrus.Fields{ - "qosBandwidthMax": storageQOSConfig.BandwidthMaximum, - "qosIopsMax": storageQOSConfig.IopsMaximum, + "qosBandwidthMax": bandwidthMaximum, + "qosIopsMax": iopsMaximum, }).Debug("parseStorageQOSOptions completed successfully") - return storageQOSConfig, nil + if iopsMaximum > 0 || bandwidthMaximum > 0 { + return &hcsschema.StorageQoS{ + IopsMaximum: iopsMaximum, + BandwidthMaximum: bandwidthMaximum, + }, nil + } + + return nil, nil } // setAdditionalOptions sets additional options from annotations. diff --git a/internal/builder/vm/lcow/specs_test.go b/internal/builder/vm/lcow/specs_test.go index 54a917b030..fb5673aad8 100644 --- a/internal/builder/vm/lcow/specs_test.go +++ b/internal/builder/vm/lcow/specs_test.go @@ -147,6 +147,14 @@ func TestBuildSandboxConfig(t *testing.T) { if doc.VirtualMachine.ComputeTopology.Memory.SizeInMB != 1024 { t.Errorf("expected default memory 1024MB, got %v", doc.VirtualMachine.ComputeTopology.Memory.SizeInMB) } + // StorageQoS should be nil when no QoS annotations are set. + if doc.VirtualMachine.StorageQoS != nil { + t.Errorf("expected StorageQoS to be nil when no QoS annotations set, got %+v", doc.VirtualMachine.StorageQoS) + } + // CpuGroup should be nil when no CPUGroupID annotation is provided. + if doc.VirtualMachine.ComputeTopology.Processor.CpuGroup != nil { + t.Errorf("expected CpuGroup to be nil when no CPUGroupID annotation set, got %+v", doc.VirtualMachine.ComputeTopology.Processor.CpuGroup) + } }, }, { @@ -1024,6 +1032,75 @@ func TestBuildSandboxConfig_EdgeCases(t *testing.T) { } }, }, + { + name: "StorageQoS is nil when both values are zero", + opts: &runhcsoptions.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: validBootFilesPath, + }, + spec: &vm.Spec{ + Annotations: map[string]string{ + shimannotations.StorageQoSIopsMaximum: "0", + shimannotations.StorageQoSBandwidthMaximum: "0", + }, + }, + validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) { + t.Helper() + if doc.VirtualMachine.StorageQoS != nil { + t.Errorf("expected StorageQoS to be nil when both IOPS and bandwidth are zero, got %+v", doc.VirtualMachine.StorageQoS) + } + }, + }, + { + name: "StorageQoS set when only IOPS is non-zero", + opts: &runhcsoptions.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: validBootFilesPath, + }, + spec: &vm.Spec{ + Annotations: map[string]string{ + shimannotations.StorageQoSIopsMaximum: "5000", + }, + }, + validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) { + t.Helper() + qos := doc.VirtualMachine.StorageQoS + if qos == nil { + t.Fatal("expected StorageQoS to be set when IOPS is non-zero") + } + if qos.IopsMaximum != 5000 { + t.Errorf("expected IOPS 5000, got %v", qos.IopsMaximum) + } + if qos.BandwidthMaximum != 0 { + t.Errorf("expected bandwidth 0, got %v", qos.BandwidthMaximum) + } + }, + }, + { + name: "StorageQoS set when only bandwidth is non-zero", + opts: &runhcsoptions.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: validBootFilesPath, + }, + spec: &vm.Spec{ + Annotations: map[string]string{ + shimannotations.StorageQoSBandwidthMaximum: "1000000", + }, + }, + validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) { + t.Helper() + qos := doc.VirtualMachine.StorageQoS + if qos == nil { + t.Fatal("expected StorageQoS to be set when bandwidth is non-zero") + } + if qos.BandwidthMaximum != 1000000 { + t.Errorf("expected bandwidth 1000000, got %v", qos.BandwidthMaximum) + } + if qos.IopsMaximum != 0 { + t.Errorf("expected IOPS 0, got %v", qos.IopsMaximum) + } + }, + }, { name: "empty annotations map with defaults", opts: &runhcsoptions.Options{ @@ -1554,6 +1631,41 @@ func TestBuildSandboxConfig_BootOptions(t *testing.T) { if len(vpmem.Devices) == 0 { t.Error("expected VPMem device to be configured for rootfs") } + dev, ok := vpmem.Devices["0"] + if !ok { + t.Fatal("expected VPMem device at index 0") + } + if !dev.ReadOnly { + t.Error("expected VPMem device to be read-only") + } + if dev.ImageFormat != "Vhd1" { + t.Errorf("expected image format Vhd1, got %s", dev.ImageFormat) + } + }, + }, + { + name: "VPMem with initrd boot creates controller but no devices", + opts: &runhcsoptions.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: initrdOnlyPath, + }, + spec: &vm.Spec{ + Annotations: map[string]string{ + shimannotations.VPMemCount: "32", + }, + }, + validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) { + t.Helper() + vpmem := doc.VirtualMachine.Devices.VirtualPMem + if vpmem == nil { + t.Fatal("expected VirtualPMem controller to be created even with initrd boot") + } + if vpmem.MaximumCount != 32 { + t.Errorf("expected VPMem count 32, got %v", vpmem.MaximumCount) + } + if len(vpmem.Devices) != 0 { + t.Errorf("expected no VPMem devices with initrd boot, got %d", len(vpmem.Devices)) + } }, }, { @@ -1737,6 +1849,28 @@ func TestBuildSandboxConfig_DeviceOptions(t *testing.T) { } }, }, + { + name: "same device with different function index is not duplicate", + spec: &vm.Spec{ + Devices: []specs.WindowsDevice{ + { + ID: `PCIP\VEN_1234&DEV_5678/1`, + IDType: "vpci-instance-id", + }, + { + ID: `PCIP\VEN_1234&DEV_5678/2`, + IDType: "vpci-instance-id", + }, + }, + }, + validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) { + t.Helper() + vpci := doc.VirtualMachine.Devices.VirtualPci + if len(vpci) != 2 { + t.Errorf("expected 2 assigned devices (different function indexes), got %d", len(vpci)) + } + }, + }, } runTestCases(t, ctx, defaultOpts, tests) diff --git a/internal/builder/vm/lcow/topology.go b/internal/builder/vm/lcow/topology.go index 6239e1cc1a..7cc8c53dd4 100644 --- a/internal/builder/vm/lcow/topology.go +++ b/internal/builder/vm/lcow/topology.go @@ -48,10 +48,13 @@ func parseCPUOptions(ctx context.Context, opts *runhcsoptions.Options, annotatio // CPU group configuration cpuGroupID := oci.ParseAnnotationsString(annotations, shimannotations.CPUGroupID, "") - if cpuGroupID != "" && osversion.Build() < osversion.V21H1 { - return nil, vmutils.ErrCPUGroupCreateNotSupported + if cpuGroupID != "" { + // If CPU group ID is provided, validate it and set it in the CPU configuration. + if osversion.Build() < osversion.V21H1 { + return nil, vmutils.ErrCPUGroupCreateNotSupported + } + cpu.CpuGroup = &hcsschema.CpuGroup{Id: cpuGroupID} } - cpu.CpuGroup = &hcsschema.CpuGroup{Id: cpuGroupID} // Resource Partition ID parsing. resourcePartitionID := oci.ParseAnnotationsString(annotations, shimannotations.ResourcePartitionID, "")