Skip to content

Commit e5ff3fa

Browse files
committed
Updated multi disk support based on reviews
1 parent fd1aaee commit e5ff3fa

File tree

6 files changed

+101
-66
lines changed

6 files changed

+101
-66
lines changed

apis/v1beta1/types.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,9 +213,8 @@ type VirtualMachineCloneSpec struct {
213213

214214
// VSphereDisk is an additional disk to add to the VM that is not part of the VM OVA template.
215215
type VSphereDisk struct {
216-
// Name is used to identify the disk definition. If Name is not specified, the disk will still be created.
217-
// The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not
218-
// required to be unique.
216+
// Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to
217+
// clearly identify purpose of the disk.
219218
// +kubebuilder:validation:Required
220219
Name string `json:"name,omitempty"`
221220
// SizeGiB is the size of the disk in GiB.

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -983,9 +983,8 @@ spec:
983983
properties:
984984
name:
985985
description: |-
986-
Name is used to identify the disk definition. If Name is not specified, the disk will still be created.
987-
The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not
988-
required to be unique.
986+
Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to
987+
clearly identify purpose of the disk.
989988
type: string
990989
sizeGiB:
991990
description: SizeGiB is the size of the disk in GiB.

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -853,9 +853,8 @@ spec:
853853
properties:
854854
name:
855855
description: |-
856-
Name is used to identify the disk definition. If Name is not specified, the disk will still be created.
857-
The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not
858-
required to be unique.
856+
Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to
857+
clearly identify purpose of the disk.
859858
type: string
860859
sizeGiB:
861860
description: SizeGiB is the size of the disk in GiB.

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1073,9 +1073,8 @@ spec:
10731073
properties:
10741074
name:
10751075
description: |-
1076-
Name is used to identify the disk definition. If Name is not specified, the disk will still be created.
1077-
The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not
1078-
required to be unique.
1076+
Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to
1077+
clearly identify purpose of the disk.
10791078
type: string
10801079
sizeGiB:
10811080
description: SizeGiB is the size of the disk in GiB.

pkg/services/govmomi/vcenter/clone.go

Lines changed: 76 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,9 @@ const (
4343
fullCloneDiskMoveType = types.VirtualMachineRelocateDiskMoveOptionsMoveAllDiskBackingsAndConsolidate
4444
linkCloneDiskMoveType = types.VirtualMachineRelocateDiskMoveOptionsCreateNewChildDiskBacking
4545

46-
// maxUnitNumber constant used to define they most devices that can be assigned to a controller. Not all controllers support up to 30, but max is 30.
47-
// Documentation on limits / behavior: https://docs.vmware.com/en/VMware-vSphere/8.0/vsphere-vm-administration/GUID-5872D173-A076-42FE-8D0B-9DB0EB0E7362.html#:~:text=If%20you%20add%20a%20hard,values%20from%200%20to%2014.
46+
// maxUnitNumber constant is used to define the maximum number of devices that can be assigned to a virtual machine's controller.
47+
// Not all controllers support up to 30, but the maximum is 30.
48+
// xref: https://docs.vmware.com/en/VMware-vSphere/8.0/vsphere-vm-administration/GUID-5872D173-A076-42FE-8D0B-9DB0EB0E7362.html#:~:text=If%20you%20add%20a%20hard,values%20from%200%20to%2014.
4849
maxUnitNumber = 30
4950
)
5051

@@ -146,7 +147,6 @@ func Clone(ctx context.Context, vmCtx *capvcontext.VMContext, bootstrapData []by
146147
if err != nil {
147148
return errors.Wrapf(err, "error getting disk spec for %q", ctx)
148149
}
149-
log.V(4).Info("Got the following disks", "disks", diskSpecs)
150150
deviceSpecs = append(deviceSpecs, diskSpecs...)
151151
}
152152

@@ -156,6 +156,7 @@ func Clone(ctx context.Context, vmCtx *capvcontext.VMContext, bootstrapData []by
156156
if err != nil {
157157
return errors.Wrapf(err, "error getting data disks")
158158
}
159+
log.V(4).Info("Adding the following data disks", "disks", dataDisks)
159160
deviceSpecs = append(deviceSpecs, dataDisks...)
160161
}
161162

@@ -409,7 +410,6 @@ func getDiskConfigSpec(disk *types.VirtualDisk, diskCloneCapacityKB int64) (type
409410
func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, devices object.VirtualDeviceList) ([]types.BaseVirtualDeviceConfigSpec, error) {
410411
log := ctrl.LoggerFrom(ctx)
411412
additionalDisks := []types.BaseVirtualDeviceConfigSpec{}
412-
unitOffset := int32(1)
413413

414414
disks := devices.SelectByType((*types.VirtualDisk)(nil))
415415
if len(disks) == 0 {
@@ -419,15 +419,21 @@ func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, de
419419
// There is at least one disk
420420
primaryDisk := disks[0].(*types.VirtualDisk)
421421

422+
// Get the controller of the primary disk.
423+
controller, ok := devices.FindByKey(primaryDisk.ControllerKey).(types.BaseVirtualController)
424+
if !ok {
425+
return nil, errors.Errorf("unable to find controller with key=%v", primaryDisk.ControllerKey)
426+
}
427+
428+
controllerKey := controller.GetVirtualController().Key
429+
unitNumberAssigner, err := newUnitNumberAssigner(controller, devices, additionalDisks)
430+
if err != nil {
431+
return nil, err
432+
}
433+
422434
for i, dataDisk := range dataDiskDefs {
423435
log.V(2).Info("Adding disk", "name", dataDisk.Name, "spec", dataDisk)
424436

425-
// Get the controller of the primary disk.
426-
controller, ok := devices.FindByKey(primaryDisk.ControllerKey).(types.BaseVirtualController)
427-
if !ok {
428-
return nil, errors.Errorf("unable to find controller with key=%v", primaryDisk.ControllerKey)
429-
}
430-
431437
dev := &types.VirtualDisk{
432438
VirtualDevice: types.VirtualDevice{
433439
Key: devices.NewKey() - int32(i),
@@ -443,30 +449,30 @@ func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, de
443449
CapacityInKB: int64(dataDisk.SizeGiB) * 1024 * 1024,
444450
}
445451

446-
// Assign unit number to the next slot on the controller.
447-
if err := assignUnitNumber(ctx, dev, devices, additionalDisks, controller, unitOffset); err != nil {
448-
return nil, errors.Wrap(err, "failed to assign device unit number to disk")
452+
vd := dev.GetVirtualDevice()
453+
vd.ControllerKey = controllerKey
454+
455+
// Assign unit number to the new disk. Should be next available slot on the controller.
456+
unitNumber, err := unitNumberAssigner.assign()
457+
if err != nil {
458+
return nil, err
449459
}
460+
vd.UnitNumber = &unitNumber
450461

451-
// Update unitOffset to current device unitNumber as starting point for next disk to be assigned.
452-
unitOffset = *dev.UnitNumber
462+
log.V(4).Info("Created device for data disk device", "name", dataDisk.Name, "spec", dataDisk, "device", dev)
453463

454-
diskConfigSpec := types.VirtualDeviceConfigSpec{
464+
additionalDisks = append(additionalDisks, &types.VirtualDeviceConfigSpec{
455465
Device: dev,
456466
Operation: types.VirtualDeviceConfigSpecOperationAdd,
457467
FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate,
458-
}
459-
460-
log.V(4).Info("Generated device", "dev", dev)
461-
462-
additionalDisks = append(additionalDisks, &diskConfigSpec)
468+
})
463469
}
464470

465471
return additionalDisks, nil
466472
}
467473

468474
// assignUnitNumber assigns a controller unit number to a device.
469-
func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec, controller types.BaseVirtualController, offset int32) error {
475+
/*func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec, controller types.BaseVirtualController, offset int32) error {
470476
if offset > maxUnitNumber {
471477
return errors.Errorf("%d exceeds maximum number of units %d", offset, maxUnitNumber)
472478
}
@@ -523,6 +529,54 @@ func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, exist
523529
524530
// If we are here, we did not find a unit number. Return error.
525531
return errors.Errorf("all unit numbers are already in-use")
532+
}*/
533+
534+
type unitNumberAssigner struct {
535+
used []bool
536+
offset int32
537+
}
538+
539+
func newUnitNumberAssigner(controller types.BaseVirtualController, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec) (*unitNumberAssigner, error) {
540+
if controller == nil {
541+
return nil, errors.New("controller parameter cannot be nil")
542+
}
543+
used := make([]bool, maxUnitNumber)
544+
// SCSIControllers also use a unit.
545+
if scsiController, ok := controller.(types.BaseVirtualSCSIController); ok {
546+
used[scsiController.GetVirtualSCSIController().ScsiCtlrUnitNumber] = true
547+
}
548+
controllerKey := controller.GetVirtualController().Key
549+
// Mark all unit numbers of existing devices as used
550+
for _, device := range existingDevices {
551+
d := device.GetVirtualDevice()
552+
if d.ControllerKey == controllerKey && d.UnitNumber != nil {
553+
used[*d.UnitNumber] = true
554+
}
555+
}
556+
// Mark all unit numbers of new devices as used
557+
/*for _, device := range newDevices {
558+
d := device.GetVirtualDeviceConfigSpec().Device.GetVirtualDevice()
559+
if d.ControllerKey == controllerKey && d.UnitNumber != nil {
560+
used[*d.UnitNumber] = true
561+
}
562+
}*/
563+
// Set offset to 0, it will auto-increment on the first assignment.
564+
return &unitNumberAssigner{used: used, offset: 0}, nil
565+
}
566+
567+
func (a *unitNumberAssigner) assign() (int32, error) {
568+
if int(a.offset) > len(a.used) {
569+
return -1, fmt.Errorf("all unit numbers are already in-use")
570+
}
571+
for i, isInUse := range a.used[a.offset:] {
572+
unit := int32(i) + a.offset
573+
if !isInUse {
574+
a.used[unit] = true
575+
a.offset++
576+
return unit, nil
577+
}
578+
}
579+
return -1, fmt.Errorf("all unit numbers are already in-use")
526580
}
527581

528582
const ethCardType = "vmxnet3"

pkg/services/govmomi/vcenter/clone_test.go

Lines changed: 17 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ func TestCreateDiskSpec(t *testing.T) {
237237
}
238238
}
239239

240-
func TestAssignUnitNumber(t *testing.T) {
240+
func TestAddingDataDisks(t *testing.T) {
241241
model, session, server := initSimulator(t)
242242
t.Cleanup(model.Remove)
243243
t.Cleanup(server.Close)
@@ -262,7 +262,6 @@ func TestAssignUnitNumber(t *testing.T) {
262262
devices object.VirtualDeviceList
263263
controller types.BaseVirtualController
264264
dataDisks []infrav1.VSphereDisk
265-
startingOffset int
266265
expectedUnitNumber []int
267266
err string
268267
}{
@@ -288,19 +287,18 @@ func TestAssignUnitNumber(t *testing.T) {
288287
expectedUnitNumber: []int{1, 2},
289288
},
290289
{
291-
name: "Add data disk with 1 ova disk but bad offset",
292-
devices: deviceList,
293-
controller: controller,
294-
dataDisks: createDataDiskDefinitions(1),
295-
startingOffset: 50,
296-
err: "50 exceeds maximum number of units 30",
290+
name: "Add too many data disks with 1 ova disk",
291+
devices: deviceList,
292+
controller: controller,
293+
dataDisks: createDataDiskDefinitions(30),
294+
err: "all unit numbers are already in-use",
297295
},
298296
{
299297
name: "Add data disk with no ova disk",
300298
devices: nil,
301299
controller: nil,
302300
dataDisks: createDataDiskDefinitions(1),
303-
err: "controller parameter cannot be nil",
301+
err: "Invalid disk count: 0",
304302
},
305303
{
306304
name: "Add too many data disks with 1 ova disk",
@@ -314,38 +312,25 @@ func TestAssignUnitNumber(t *testing.T) {
314312
for _, test := range testCases {
315313
tc := test
316314
t.Run(tc.name, func(t *testing.T) {
317-
offset := int32(tc.startingOffset)
318-
newDevices := []types.BaseVirtualDeviceConfigSpec{}
319315
var funcError error
320316

321-
// Create data disk
322-
for index, dataDisk := range tc.dataDisks {
323-
dev := createVirtualDisk(tc.devices.NewKey()-int32(index), tc.controller, dataDisk.SizeGiB)
317+
// Create the data disks
318+
newDisks, funcError := createDataDisks(ctx.TODO(), tc.dataDisks, tc.devices)
319+
if (tc.err != "" && funcError == nil) || (tc.err == "" && funcError != nil) || (funcError != nil && tc.err != funcError.Error()) {
320+
t.Fatalf("Expected to get '%v' error from assignUnitNumber, got: '%v'", tc.err, funcError)
321+
}
324322

325-
funcError = assignUnitNumber(ctx.TODO(), dev, tc.devices, newDevices, tc.controller, offset)
323+
// Validate the configs of new data disks
324+
for index, disk := range newDisks {
326325

327326
if funcError != nil {
328327
break
329328
}
330329

331-
if tc.err == "" && *dev.UnitNumber != int32(tc.expectedUnitNumber[index]) {
332-
t.Fatalf("Expected to get unitNumber '%d' error from assignUnitNumber, got: '%d'", tc.expectedUnitNumber[index], *dev.UnitNumber)
330+
unitNumber := *disk.GetVirtualDeviceConfigSpec().Device.GetVirtualDevice().UnitNumber
331+
if tc.err == "" && unitNumber != int32(tc.expectedUnitNumber[index]) {
332+
t.Fatalf("Expected to get unitNumber '%d' error from assignUnitNumber, got: '%d'", tc.expectedUnitNumber[index], unitNumber)
333333
}
334-
335-
// The rest of this loop just gets ready for next test by adding current disk to the newDevices
336-
offset = *dev.UnitNumber
337-
338-
diskConfigSpec := types.VirtualDeviceConfigSpec{
339-
Device: dev,
340-
Operation: types.VirtualDeviceConfigSpecOperationAdd,
341-
FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate,
342-
}
343-
344-
newDevices = append(newDevices, &diskConfigSpec)
345-
}
346-
347-
if (tc.err != "" && funcError == nil) || (tc.err == "" && funcError != nil) || (funcError != nil && tc.err != funcError.Error()) {
348-
t.Fatalf("Expected to get '%v' error from assignUnitNumber, got: '%v'", tc.err, funcError)
349334
}
350335
})
351336
}

0 commit comments

Comments
 (0)