From e5ff3fa65321c08b3b30dc60e4825179e2649f44 Mon Sep 17 00:00:00 2001 From: vr4manta Date: Thu, 5 Dec 2024 11:08:26 -0500 Subject: [PATCH] Updated multi disk support based on reviews --- apis/v1beta1/types.go | 5 +- ...ture.cluster.x-k8s.io_vspheremachines.yaml | 5 +- ...ster.x-k8s.io_vspheremachinetemplates.yaml | 5 +- ...structure.cluster.x-k8s.io_vspherevms.yaml | 5 +- pkg/services/govmomi/vcenter/clone.go | 98 ++++++++++++++----- pkg/services/govmomi/vcenter/clone_test.go | 49 ++++------ 6 files changed, 101 insertions(+), 66 deletions(-) diff --git a/apis/v1beta1/types.go b/apis/v1beta1/types.go index c4467f67ea..ce9d909ee8 100644 --- a/apis/v1beta1/types.go +++ b/apis/v1beta1/types.go @@ -213,9 +213,8 @@ type VirtualMachineCloneSpec struct { // VSphereDisk is an additional disk to add to the VM that is not part of the VM OVA template. type VSphereDisk struct { - // Name is used to identify the disk definition. If Name is not specified, the disk will still be created. - // The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not - // required to be unique. + // Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to + // clearly identify purpose of the disk. // +kubebuilder:validation:Required Name string `json:"name,omitempty"` // SizeGiB is the size of the disk in GiB. diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml index 4dfcfc9ab6..ae8026b9d0 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml @@ -983,9 +983,8 @@ spec: properties: name: description: |- - Name is used to identify the disk definition. If Name is not specified, the disk will still be created. - The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not - required to be unique. + Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to + clearly identify purpose of the disk. type: string sizeGiB: description: SizeGiB is the size of the disk in GiB. diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml index ab83bad2eb..fb870f3760 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml @@ -853,9 +853,8 @@ spec: properties: name: description: |- - Name is used to identify the disk definition. If Name is not specified, the disk will still be created. - The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not - required to be unique. + Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to + clearly identify purpose of the disk. type: string sizeGiB: description: SizeGiB is the size of the disk in GiB. diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml index 2f873730e2..817c5e8d76 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml @@ -1073,9 +1073,8 @@ spec: properties: name: description: |- - Name is used to identify the disk definition. If Name is not specified, the disk will still be created. - The Name should be unique so that it can be used to clearly identify purpose of the disk, but is not - required to be unique. + Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to + clearly identify purpose of the disk. type: string sizeGiB: description: SizeGiB is the size of the disk in GiB. diff --git a/pkg/services/govmomi/vcenter/clone.go b/pkg/services/govmomi/vcenter/clone.go index 3cd6548921..43bf31debd 100644 --- a/pkg/services/govmomi/vcenter/clone.go +++ b/pkg/services/govmomi/vcenter/clone.go @@ -43,8 +43,9 @@ const ( fullCloneDiskMoveType = types.VirtualMachineRelocateDiskMoveOptionsMoveAllDiskBackingsAndConsolidate linkCloneDiskMoveType = types.VirtualMachineRelocateDiskMoveOptionsCreateNewChildDiskBacking - // 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. - // 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. + // maxUnitNumber constant is used to define the maximum number of devices that can be assigned to a virtual machine's controller. + // Not all controllers support up to 30, but the maximum is 30. + // 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. maxUnitNumber = 30 ) @@ -146,7 +147,6 @@ func Clone(ctx context.Context, vmCtx *capvcontext.VMContext, bootstrapData []by if err != nil { return errors.Wrapf(err, "error getting disk spec for %q", ctx) } - log.V(4).Info("Got the following disks", "disks", diskSpecs) deviceSpecs = append(deviceSpecs, diskSpecs...) } @@ -156,6 +156,7 @@ func Clone(ctx context.Context, vmCtx *capvcontext.VMContext, bootstrapData []by if err != nil { return errors.Wrapf(err, "error getting data disks") } + log.V(4).Info("Adding the following data disks", "disks", dataDisks) deviceSpecs = append(deviceSpecs, dataDisks...) } @@ -409,7 +410,6 @@ func getDiskConfigSpec(disk *types.VirtualDisk, diskCloneCapacityKB int64) (type func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, devices object.VirtualDeviceList) ([]types.BaseVirtualDeviceConfigSpec, error) { log := ctrl.LoggerFrom(ctx) additionalDisks := []types.BaseVirtualDeviceConfigSpec{} - unitOffset := int32(1) disks := devices.SelectByType((*types.VirtualDisk)(nil)) if len(disks) == 0 { @@ -419,15 +419,21 @@ func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, de // There is at least one disk primaryDisk := disks[0].(*types.VirtualDisk) + // Get the controller of the primary disk. + controller, ok := devices.FindByKey(primaryDisk.ControllerKey).(types.BaseVirtualController) + if !ok { + return nil, errors.Errorf("unable to find controller with key=%v", primaryDisk.ControllerKey) + } + + controllerKey := controller.GetVirtualController().Key + unitNumberAssigner, err := newUnitNumberAssigner(controller, devices, additionalDisks) + if err != nil { + return nil, err + } + for i, dataDisk := range dataDiskDefs { log.V(2).Info("Adding disk", "name", dataDisk.Name, "spec", dataDisk) - // Get the controller of the primary disk. - controller, ok := devices.FindByKey(primaryDisk.ControllerKey).(types.BaseVirtualController) - if !ok { - return nil, errors.Errorf("unable to find controller with key=%v", primaryDisk.ControllerKey) - } - dev := &types.VirtualDisk{ VirtualDevice: types.VirtualDevice{ Key: devices.NewKey() - int32(i), @@ -443,30 +449,30 @@ func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, de CapacityInKB: int64(dataDisk.SizeGiB) * 1024 * 1024, } - // Assign unit number to the next slot on the controller. - if err := assignUnitNumber(ctx, dev, devices, additionalDisks, controller, unitOffset); err != nil { - return nil, errors.Wrap(err, "failed to assign device unit number to disk") + vd := dev.GetVirtualDevice() + vd.ControllerKey = controllerKey + + // Assign unit number to the new disk. Should be next available slot on the controller. + unitNumber, err := unitNumberAssigner.assign() + if err != nil { + return nil, err } + vd.UnitNumber = &unitNumber - // Update unitOffset to current device unitNumber as starting point for next disk to be assigned. - unitOffset = *dev.UnitNumber + log.V(4).Info("Created device for data disk device", "name", dataDisk.Name, "spec", dataDisk, "device", dev) - diskConfigSpec := types.VirtualDeviceConfigSpec{ + additionalDisks = append(additionalDisks, &types.VirtualDeviceConfigSpec{ Device: dev, Operation: types.VirtualDeviceConfigSpecOperationAdd, FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate, - } - - log.V(4).Info("Generated device", "dev", dev) - - additionalDisks = append(additionalDisks, &diskConfigSpec) + }) } return additionalDisks, nil } // assignUnitNumber assigns a controller unit number to a device. -func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec, controller types.BaseVirtualController, offset int32) error { +/*func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec, controller types.BaseVirtualController, offset int32) error { if offset > maxUnitNumber { return errors.Errorf("%d exceeds maximum number of units %d", offset, maxUnitNumber) } @@ -523,6 +529,54 @@ func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, exist // If we are here, we did not find a unit number. Return error. return errors.Errorf("all unit numbers are already in-use") +}*/ + +type unitNumberAssigner struct { + used []bool + offset int32 +} + +func newUnitNumberAssigner(controller types.BaseVirtualController, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec) (*unitNumberAssigner, error) { + if controller == nil { + return nil, errors.New("controller parameter cannot be nil") + } + used := make([]bool, maxUnitNumber) + // SCSIControllers also use a unit. + if scsiController, ok := controller.(types.BaseVirtualSCSIController); ok { + used[scsiController.GetVirtualSCSIController().ScsiCtlrUnitNumber] = true + } + controllerKey := controller.GetVirtualController().Key + // Mark all unit numbers of existing devices as used + for _, device := range existingDevices { + d := device.GetVirtualDevice() + if d.ControllerKey == controllerKey && d.UnitNumber != nil { + used[*d.UnitNumber] = true + } + } + // Mark all unit numbers of new devices as used + /*for _, device := range newDevices { + d := device.GetVirtualDeviceConfigSpec().Device.GetVirtualDevice() + if d.ControllerKey == controllerKey && d.UnitNumber != nil { + used[*d.UnitNumber] = true + } + }*/ + // Set offset to 0, it will auto-increment on the first assignment. + return &unitNumberAssigner{used: used, offset: 0}, nil +} + +func (a *unitNumberAssigner) assign() (int32, error) { + if int(a.offset) > len(a.used) { + return -1, fmt.Errorf("all unit numbers are already in-use") + } + for i, isInUse := range a.used[a.offset:] { + unit := int32(i) + a.offset + if !isInUse { + a.used[unit] = true + a.offset++ + return unit, nil + } + } + return -1, fmt.Errorf("all unit numbers are already in-use") } const ethCardType = "vmxnet3" diff --git a/pkg/services/govmomi/vcenter/clone_test.go b/pkg/services/govmomi/vcenter/clone_test.go index 71008c66ed..1d8524a169 100644 --- a/pkg/services/govmomi/vcenter/clone_test.go +++ b/pkg/services/govmomi/vcenter/clone_test.go @@ -237,7 +237,7 @@ func TestCreateDiskSpec(t *testing.T) { } } -func TestAssignUnitNumber(t *testing.T) { +func TestAddingDataDisks(t *testing.T) { model, session, server := initSimulator(t) t.Cleanup(model.Remove) t.Cleanup(server.Close) @@ -262,7 +262,6 @@ func TestAssignUnitNumber(t *testing.T) { devices object.VirtualDeviceList controller types.BaseVirtualController dataDisks []infrav1.VSphereDisk - startingOffset int expectedUnitNumber []int err string }{ @@ -288,19 +287,18 @@ func TestAssignUnitNumber(t *testing.T) { expectedUnitNumber: []int{1, 2}, }, { - name: "Add data disk with 1 ova disk but bad offset", - devices: deviceList, - controller: controller, - dataDisks: createDataDiskDefinitions(1), - startingOffset: 50, - err: "50 exceeds maximum number of units 30", + name: "Add too many data disks with 1 ova disk", + devices: deviceList, + controller: controller, + dataDisks: createDataDiskDefinitions(30), + err: "all unit numbers are already in-use", }, { name: "Add data disk with no ova disk", devices: nil, controller: nil, dataDisks: createDataDiskDefinitions(1), - err: "controller parameter cannot be nil", + err: "Invalid disk count: 0", }, { name: "Add too many data disks with 1 ova disk", @@ -314,38 +312,25 @@ func TestAssignUnitNumber(t *testing.T) { for _, test := range testCases { tc := test t.Run(tc.name, func(t *testing.T) { - offset := int32(tc.startingOffset) - newDevices := []types.BaseVirtualDeviceConfigSpec{} var funcError error - // Create data disk - for index, dataDisk := range tc.dataDisks { - dev := createVirtualDisk(tc.devices.NewKey()-int32(index), tc.controller, dataDisk.SizeGiB) + // Create the data disks + newDisks, funcError := createDataDisks(ctx.TODO(), tc.dataDisks, tc.devices) + if (tc.err != "" && funcError == nil) || (tc.err == "" && funcError != nil) || (funcError != nil && tc.err != funcError.Error()) { + t.Fatalf("Expected to get '%v' error from assignUnitNumber, got: '%v'", tc.err, funcError) + } - funcError = assignUnitNumber(ctx.TODO(), dev, tc.devices, newDevices, tc.controller, offset) + // Validate the configs of new data disks + for index, disk := range newDisks { if funcError != nil { break } - if tc.err == "" && *dev.UnitNumber != int32(tc.expectedUnitNumber[index]) { - t.Fatalf("Expected to get unitNumber '%d' error from assignUnitNumber, got: '%d'", tc.expectedUnitNumber[index], *dev.UnitNumber) + unitNumber := *disk.GetVirtualDeviceConfigSpec().Device.GetVirtualDevice().UnitNumber + if tc.err == "" && unitNumber != int32(tc.expectedUnitNumber[index]) { + t.Fatalf("Expected to get unitNumber '%d' error from assignUnitNumber, got: '%d'", tc.expectedUnitNumber[index], unitNumber) } - - // The rest of this loop just gets ready for next test by adding current disk to the newDevices - offset = *dev.UnitNumber - - diskConfigSpec := types.VirtualDeviceConfigSpec{ - Device: dev, - Operation: types.VirtualDeviceConfigSpecOperationAdd, - FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate, - } - - newDevices = append(newDevices, &diskConfigSpec) - } - - if (tc.err != "" && funcError == nil) || (tc.err == "" && funcError != nil) || (funcError != nil && tc.err != funcError.Error()) { - t.Fatalf("Expected to get '%v' error from assignUnitNumber, got: '%v'", tc.err, funcError) } }) }