Skip to content

Commit 50f4409

Browse files
rawaharsrawahars
committed
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 <harshrawat@microsoft.com>
1 parent 3adc5c9 commit 50f4409

4 files changed

Lines changed: 176 additions & 30 deletions

File tree

‎internal/builder/vm/lcow/devices.go‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -68,33 +68,37 @@ func parseDeviceOptions(
6868

6969
// Create VPMem controller configuration
7070
var vpMemController *hcsschema.VirtualPMemController
71-
if vpmemCount > 0 && rootFsFile == vmutils.VhdFile {
72-
// If booting from VHD via VPMem, configure the VPMem device for rootfs
71+
if vpmemCount > 0 {
72+
// Initialize VPMem controller with specified count and size.
7373
vpMemController = &hcsschema.VirtualPMemController{
7474
MaximumCount: vpmemCount,
7575
MaximumSizeBytes: vpmemSize,
76-
Devices: make(map[string]hcsschema.VirtualPMemDevice),
7776
}
7877

79-
// Determine image format based on file extension.
80-
// filepath.Ext returns the extension with the leading dot (e.g. ".vhdx").
81-
imageFormat := "Vhd1"
82-
if strings.HasSuffix(strings.ToLower(filepath.Ext(rootFsFile)), "vhdx") {
83-
imageFormat = "Vhdx"
84-
}
78+
// If booting from VHD via VPMem, configure the VPMem device for rootfs
79+
if rootFsFile == vmutils.VhdFile {
80+
vpMemController.Devices = make(map[string]hcsschema.VirtualPMemDevice)
81+
82+
// Determine image format based on file extension.
83+
// filepath.Ext returns the extension with the leading dot (e.g. ".vhdx").
84+
imageFormat := "Vhd1"
85+
if strings.HasSuffix(strings.ToLower(filepath.Ext(rootFsFile)), "vhdx") {
86+
imageFormat = "Vhdx"
87+
}
8588

86-
// Add rootfs VHD as VPMem device 0
87-
vpMemController.Devices["0"] = hcsschema.VirtualPMemDevice{
88-
HostPath: rootFsFullPath,
89-
ReadOnly: true,
90-
ImageFormat: imageFormat,
91-
}
89+
// Add rootfs VHD as VPMem device 0
90+
vpMemController.Devices["0"] = hcsschema.VirtualPMemDevice{
91+
HostPath: rootFsFullPath,
92+
ReadOnly: true,
93+
ImageFormat: imageFormat,
94+
}
9295

93-
log.G(ctx).WithFields(logrus.Fields{
94-
"device": "0",
95-
"path": rootFsFullPath,
96-
"imageFormat": imageFormat,
97-
}).Debug("configured VPMem device for VHD rootfs boot")
96+
log.G(ctx).WithFields(logrus.Fields{
97+
"device": "0",
98+
"path": rootFsFullPath,
99+
"imageFormat": imageFormat,
100+
}).Debug("configured VPMem device for VHD rootfs boot")
101+
}
98102
}
99103

100104
// ===============================Parse SCSI configuration===============================

‎internal/builder/vm/lcow/specs.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -353,17 +353,22 @@ func parseSandboxOptions(ctx context.Context, platform string, annotations map[s
353353
func parseStorageQOSOptions(ctx context.Context, annotations map[string]string) (*hcsschema.StorageQoS, error) {
354354
log.G(ctx).Debug("parseStorageQOSOptions: starting storage QOS options parsing")
355355

356-
storageQOSConfig := &hcsschema.StorageQoS{
357-
IopsMaximum: oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSIopsMaximum, 0),
358-
BandwidthMaximum: oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSBandwidthMaximum, 0),
359-
}
356+
iopsMaximum := oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSIopsMaximum, 0)
357+
bandwidthMaximum := oci.ParseAnnotationsInt32(ctx, annotations, shimannotations.StorageQoSBandwidthMaximum, 0)
360358

361359
log.G(ctx).WithFields(logrus.Fields{
362-
"qosBandwidthMax": storageQOSConfig.BandwidthMaximum,
363-
"qosIopsMax": storageQOSConfig.IopsMaximum,
360+
"qosBandwidthMax": bandwidthMaximum,
361+
"qosIopsMax": iopsMaximum,
364362
}).Debug("parseStorageQOSOptions completed successfully")
365363

366-
return storageQOSConfig, nil
364+
if iopsMaximum > 0 || bandwidthMaximum > 0 {
365+
return &hcsschema.StorageQoS{
366+
IopsMaximum: iopsMaximum,
367+
BandwidthMaximum: bandwidthMaximum,
368+
}, nil
369+
}
370+
371+
return nil, nil
367372
}
368373

369374
// setAdditionalOptions sets additional options from annotations.

‎internal/builder/vm/lcow/specs_test.go‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,14 @@ func TestBuildSandboxConfig(t *testing.T) {
147147
if doc.VirtualMachine.ComputeTopology.Memory.SizeInMB != 1024 {
148148
t.Errorf("expected default memory 1024MB, got %v", doc.VirtualMachine.ComputeTopology.Memory.SizeInMB)
149149
}
150+
// StorageQoS should be nil when no QoS annotations are set.
151+
if doc.VirtualMachine.StorageQoS != nil {
152+
t.Errorf("expected StorageQoS to be nil when no QoS annotations set, got %+v", doc.VirtualMachine.StorageQoS)
153+
}
154+
// CpuGroup should be nil when no CPUGroupID annotation is provided.
155+
if doc.VirtualMachine.ComputeTopology.Processor.CpuGroup != nil {
156+
t.Errorf("expected CpuGroup to be nil when no CPUGroupID annotation set, got %+v", doc.VirtualMachine.ComputeTopology.Processor.CpuGroup)
157+
}
150158
},
151159
},
152160
{
@@ -1024,6 +1032,75 @@ func TestBuildSandboxConfig_EdgeCases(t *testing.T) {
10241032
}
10251033
},
10261034
},
1035+
{
1036+
name: "StorageQoS is nil when both values are zero",
1037+
opts: &runhcsoptions.Options{
1038+
SandboxPlatform: "linux/amd64",
1039+
BootFilesRootPath: validBootFilesPath,
1040+
},
1041+
spec: &vm.Spec{
1042+
Annotations: map[string]string{
1043+
shimannotations.StorageQoSIopsMaximum: "0",
1044+
shimannotations.StorageQoSBandwidthMaximum: "0",
1045+
},
1046+
},
1047+
validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) {
1048+
t.Helper()
1049+
if doc.VirtualMachine.StorageQoS != nil {
1050+
t.Errorf("expected StorageQoS to be nil when both IOPS and bandwidth are zero, got %+v", doc.VirtualMachine.StorageQoS)
1051+
}
1052+
},
1053+
},
1054+
{
1055+
name: "StorageQoS set when only IOPS is non-zero",
1056+
opts: &runhcsoptions.Options{
1057+
SandboxPlatform: "linux/amd64",
1058+
BootFilesRootPath: validBootFilesPath,
1059+
},
1060+
spec: &vm.Spec{
1061+
Annotations: map[string]string{
1062+
shimannotations.StorageQoSIopsMaximum: "5000",
1063+
},
1064+
},
1065+
validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) {
1066+
t.Helper()
1067+
qos := doc.VirtualMachine.StorageQoS
1068+
if qos == nil {
1069+
t.Fatal("expected StorageQoS to be set when IOPS is non-zero")
1070+
}
1071+
if qos.IopsMaximum != 5000 {
1072+
t.Errorf("expected IOPS 5000, got %v", qos.IopsMaximum)
1073+
}
1074+
if qos.BandwidthMaximum != 0 {
1075+
t.Errorf("expected bandwidth 0, got %v", qos.BandwidthMaximum)
1076+
}
1077+
},
1078+
},
1079+
{
1080+
name: "StorageQoS set when only bandwidth is non-zero",
1081+
opts: &runhcsoptions.Options{
1082+
SandboxPlatform: "linux/amd64",
1083+
BootFilesRootPath: validBootFilesPath,
1084+
},
1085+
spec: &vm.Spec{
1086+
Annotations: map[string]string{
1087+
shimannotations.StorageQoSBandwidthMaximum: "1000000",
1088+
},
1089+
},
1090+
validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) {
1091+
t.Helper()
1092+
qos := doc.VirtualMachine.StorageQoS
1093+
if qos == nil {
1094+
t.Fatal("expected StorageQoS to be set when bandwidth is non-zero")
1095+
}
1096+
if qos.BandwidthMaximum != 1000000 {
1097+
t.Errorf("expected bandwidth 1000000, got %v", qos.BandwidthMaximum)
1098+
}
1099+
if qos.IopsMaximum != 0 {
1100+
t.Errorf("expected IOPS 0, got %v", qos.IopsMaximum)
1101+
}
1102+
},
1103+
},
10271104
{
10281105
name: "empty annotations map with defaults",
10291106
opts: &runhcsoptions.Options{
@@ -1554,6 +1631,41 @@ func TestBuildSandboxConfig_BootOptions(t *testing.T) {
15541631
if len(vpmem.Devices) == 0 {
15551632
t.Error("expected VPMem device to be configured for rootfs")
15561633
}
1634+
dev, ok := vpmem.Devices["0"]
1635+
if !ok {
1636+
t.Fatal("expected VPMem device at index 0")
1637+
}
1638+
if !dev.ReadOnly {
1639+
t.Error("expected VPMem device to be read-only")
1640+
}
1641+
if dev.ImageFormat != "Vhd1" {
1642+
t.Errorf("expected image format Vhd1, got %s", dev.ImageFormat)
1643+
}
1644+
},
1645+
},
1646+
{
1647+
name: "VPMem with initrd boot creates controller but no devices",
1648+
opts: &runhcsoptions.Options{
1649+
SandboxPlatform: "linux/amd64",
1650+
BootFilesRootPath: initrdOnlyPath,
1651+
},
1652+
spec: &vm.Spec{
1653+
Annotations: map[string]string{
1654+
shimannotations.VPMemCount: "32",
1655+
},
1656+
},
1657+
validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) {
1658+
t.Helper()
1659+
vpmem := doc.VirtualMachine.Devices.VirtualPMem
1660+
if vpmem == nil {
1661+
t.Fatal("expected VirtualPMem controller to be created even with initrd boot")
1662+
}
1663+
if vpmem.MaximumCount != 32 {
1664+
t.Errorf("expected VPMem count 32, got %v", vpmem.MaximumCount)
1665+
}
1666+
if len(vpmem.Devices) != 0 {
1667+
t.Errorf("expected no VPMem devices with initrd boot, got %d", len(vpmem.Devices))
1668+
}
15571669
},
15581670
},
15591671
{
@@ -1737,6 +1849,28 @@ func TestBuildSandboxConfig_DeviceOptions(t *testing.T) {
17371849
}
17381850
},
17391851
},
1852+
{
1853+
name: "same device with different function index is not duplicate",
1854+
spec: &vm.Spec{
1855+
Devices: []specs.WindowsDevice{
1856+
{
1857+
ID: `PCIP\VEN_1234&DEV_5678/1`,
1858+
IDType: "vpci-instance-id",
1859+
},
1860+
{
1861+
ID: `PCIP\VEN_1234&DEV_5678/2`,
1862+
IDType: "vpci-instance-id",
1863+
},
1864+
},
1865+
},
1866+
validate: func(t *testing.T, doc *hcsschema.ComputeSystem, sandboxOpts *SandboxOptions) {
1867+
t.Helper()
1868+
vpci := doc.VirtualMachine.Devices.VirtualPci
1869+
if len(vpci) != 2 {
1870+
t.Errorf("expected 2 assigned devices (different function indexes), got %d", len(vpci))
1871+
}
1872+
},
1873+
},
17401874
}
17411875

17421876
runTestCases(t, ctx, defaultOpts, tests)

‎internal/builder/vm/lcow/topology.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,13 @@ func parseCPUOptions(ctx context.Context, opts *runhcsoptions.Options, annotatio
4848

4949
// CPU group configuration
5050
cpuGroupID := oci.ParseAnnotationsString(annotations, shimannotations.CPUGroupID, "")
51-
if cpuGroupID != "" && osversion.Build() < osversion.V21H1 {
52-
return nil, vmutils.ErrCPUGroupCreateNotSupported
51+
if cpuGroupID != "" {
52+
// If CPU group ID is provided, validate it and set it in the CPU configuration.
53+
if osversion.Build() < osversion.V21H1 {
54+
return nil, vmutils.ErrCPUGroupCreateNotSupported
55+
}
56+
cpu.CpuGroup = &hcsschema.CpuGroup{Id: cpuGroupID}
5357
}
54-
cpu.CpuGroup = &hcsschema.CpuGroup{Id: cpuGroupID}
5558

5659
// Resource Partition ID parsing.
5760
resourcePartitionID := oci.ParseAnnotationsString(annotations, shimannotations.ResourcePartitionID, "")

0 commit comments

Comments
 (0)