Updated multi disk support based on reviews

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.

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.

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.

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.

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"

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)
 			}
 		})
 	}