diff --git a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go index 3ad6de9cc..5ae236f88 100644 --- a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go +++ b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go @@ -23,8 +23,8 @@ func CreateConfigSpec( vmCtx context.VirtualMachineContextA2, vmClassConfigSpec *types.VirtualMachineConfigSpec, vmClassSpec *vmopv1.VirtualMachineClassSpec, - minFreq uint64, - imageFirmware string) *types.VirtualMachineConfigSpec { + vmImageStatus *vmopv1.VirtualMachineImageStatus, + minFreq uint64) *types.VirtualMachineConfigSpec { configSpec := types.VirtualMachineConfigSpec{} @@ -49,9 +49,9 @@ func CreateConfigSpec( if val, ok := vmCtx.VM.Annotations[constants.FirmwareOverrideAnnotation]; ok { configSpec.Firmware = val - } else if configSpec.Firmware == "" { - // Use firmware type from the image if config spec doesn't have it. - configSpec.Firmware = imageFirmware + } else if configSpec.Firmware == "" && vmImageStatus != nil { + // Use firmware type from the image if ConfigSpec doesn't have it. + configSpec.Firmware = vmImageStatus.Firmware } // TODO: Otherwise leave as-is? Our ChangeBlockTracking could be better as a *bool. @@ -103,8 +103,9 @@ func CreateConfigSpec( return &configSpec } -// CreateConfigSpecForPlacement2 creates a ConfigSpec that is suitable for Placement. -func CreateConfigSpecForPlacement2( +// CreateConfigSpecForPlacement creates a ConfigSpec that is suitable for Placement. +// baseConfigSpec will likely be - or at least derived from - the ConfigSpec returned by CreateConfigSpec above. +func CreateConfigSpecForPlacement( vmCtx context.VirtualMachineContextA2, baseConfigSpec *types.VirtualMachineConfigSpec, storageClassesToIDs map[string]string) *types.VirtualMachineConfigSpec { @@ -157,73 +158,6 @@ func CreateConfigSpecForPlacement2( } } - return &configSpec -} - -// CreateConfigSpecForPlacement creates a ConfigSpec to use for placement. Once CL deploy can accept -// a ConfigSpec, this should largely - or ideally entirely - be folded into CreateConfigSpec() above. -func CreateConfigSpecForPlacement( - vmCtx context.VirtualMachineContextA2, - vmClassSpec *vmopv1.VirtualMachineClassSpec, - minFreq uint64, - storageClassesToIDs map[string]string, - imageFirmware string, - vmClassConfigSpec *types.VirtualMachineConfigSpec) *types.VirtualMachineConfigSpec { - - configSpec := CreateConfigSpec(vmCtx, vmClassConfigSpec, vmClassSpec, minFreq, imageFirmware) - - // Add a dummy disk for placement: PlaceVmsXCluster expects there to always be at least one disk. - // Until we're in a position to have the OVF envelope here, add a dummy disk satisfy it. - configSpec.DeviceChange = append(configSpec.DeviceChange, &types.VirtualDeviceConfigSpec{ - Operation: types.VirtualDeviceConfigSpecOperationAdd, - FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate, - Device: &types.VirtualDisk{ - CapacityInBytes: 1024 * 1024, - VirtualDevice: types.VirtualDevice{ - Key: -42, - Backing: &types.VirtualDiskFlatVer2BackingInfo{ - ThinProvisioned: pointer.Bool(true), - }, - }, - }, - Profile: []types.BaseVirtualMachineProfileSpec{ - &types.VirtualMachineDefinedProfileSpec{ - ProfileId: storageClassesToIDs[vmCtx.VM.Spec.StorageClass], - }, - }, - }) - - // With DaynDate FSS, PCI devices are specified via ConfigSpec. Ignore such devices from the - // VM Class Hardware to avoid duplicate devices being added to the placement spec. - if !lib.IsVMClassAsConfigFSSDaynDateEnabled() { - for _, dev := range CreatePCIDevicesFromVMClass(vmClassSpec.Hardware.Devices) { - configSpec.DeviceChange = append(configSpec.DeviceChange, &types.VirtualDeviceConfigSpec{ - Operation: types.VirtualDeviceConfigSpecOperationAdd, - Device: dev, - }) - } - } - - if lib.IsInstanceStorageFSSEnabled() { - isVolumes := instancestorage.FilterVolumes(vmCtx.VM) - - for idx, dev := range CreateInstanceStorageDiskDevices(isVolumes) { - configSpec.DeviceChange = append(configSpec.DeviceChange, &types.VirtualDeviceConfigSpec{ - Operation: types.VirtualDeviceConfigSpecOperationAdd, - FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate, - Device: dev, - Profile: []types.BaseVirtualMachineProfileSpec{ - &types.VirtualMachineDefinedProfileSpec{ - ProfileId: storageClassesToIDs[isVolumes[idx].PersistentVolumeClaim.InstanceVolumeClaim.StorageClass], - ProfileData: &types.VirtualMachineProfileRawData{ - ExtensionKey: "com.vmware.vim.sps", - }, - }, - }, - }) - } - } - // TODO: Add more devices and fields // - boot disks from OVA // - storage profile/class @@ -233,21 +167,20 @@ func CreateConfigSpecForPlacement( // - any way to do the cluster modules for anti-affinity? // - whatever else I'm forgetting - return configSpec + return &configSpec } // ConfigSpecFromVMClassDevices creates a ConfigSpec that adds the standalone hardware devices from // the VMClass if any. This ConfigSpec will be used as the class ConfigSpec to CreateConfigSpec, with // the rest of the class fields - like CPU count - applied on top. func ConfigSpecFromVMClassDevices(vmClassSpec *vmopv1.VirtualMachineClassSpec) *types.VirtualMachineConfigSpec { - devices := vmClassSpec.Hardware.Devices - - if len(devices.VGPUDevices) == 0 && len(devices.DynamicDirectPathIODevices) == 0 { + devsFromClass := CreatePCIDevicesFromVMClass(vmClassSpec.Hardware.Devices) + if len(devsFromClass) == 0 { return nil } configSpec := &types.VirtualMachineConfigSpec{} - for _, dev := range CreatePCIDevicesFromVMClass(devices) { + for _, dev := range devsFromClass { configSpec.DeviceChange = append(configSpec.DeviceChange, &types.VirtualDeviceConfigSpec{ Operation: types.VirtualDeviceConfigSpecOperationAdd, Device: dev, diff --git a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go index b17ccd6fd..0dd01e72c 100644 --- a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go +++ b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go @@ -23,8 +23,8 @@ var _ = Describe("CreateConfigSpec", func() { var ( vmCtx context.VirtualMachineContextA2 vmClassSpec *vmopv1.VirtualMachineClassSpec + vmImageStatus *vmopv1.VirtualMachineImageStatus minCPUFreq uint64 - firmware string configSpec *vimtypes.VirtualMachineConfigSpec classConfigSpec *vimtypes.VirtualMachineConfigSpec err error @@ -33,8 +33,8 @@ var _ = Describe("CreateConfigSpec", func() { BeforeEach(func() { vmClass := builder.DummyVirtualMachineClassA2() vmClassSpec = &vmClass.Spec + vmImageStatus = &vmopv1.VirtualMachineImageStatus{Firmware: "efi"} minCPUFreq = 2500 - firmware = "efi" vm := builder.DummyVirtualMachineA2() vm.Name = vmName @@ -50,8 +50,8 @@ var _ = Describe("CreateConfigSpec", func() { vmCtx, nil, vmClassSpec, - minCPUFreq, - firmware) + vmImageStatus, + minCPUFreq) Expect(configSpec).ToNot(BeNil()) Expect(err).To(BeNil()) @@ -61,7 +61,7 @@ var _ = Describe("CreateConfigSpec", func() { Expect(configSpec.MemoryMB).To(BeEquivalentTo(4 * 1024)) Expect(configSpec.CpuAllocation).ToNot(BeNil()) Expect(configSpec.MemoryAllocation).ToNot(BeNil()) - Expect(configSpec.Firmware).To(Equal(firmware)) + Expect(configSpec.Firmware).To(Equal(vmImageStatus.Firmware)) }) Context("Use VM Class ConfigSpec", func() { @@ -89,8 +89,8 @@ var _ = Describe("CreateConfigSpec", func() { vmCtx, classConfigSpec, vmClassSpec, - minCPUFreq, - firmware) + vmImageStatus, + minCPUFreq) Expect(configSpec).ToNot(BeNil()) }) @@ -102,12 +102,11 @@ var _ = Describe("CreateConfigSpec", func() { Expect(configSpec.MemoryMB).To(BeEquivalentTo(4 * 1024)) Expect(configSpec.CpuAllocation).ToNot(BeNil()) Expect(configSpec.MemoryAllocation).ToNot(BeNil()) - Expect(configSpec.Firmware).To(Equal(firmware)) + Expect(configSpec.Firmware).To(Equal(vmImageStatus.Firmware)) Expect(configSpec.DeviceChange).To(HaveLen(1)) dSpec := configSpec.DeviceChange[0].GetVirtualDeviceConfigSpec() _, ok := dSpec.Device.(*vimtypes.VirtualE1000) Expect(ok).To(BeTrue()) - }) }) }) @@ -116,19 +115,13 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { var ( vmCtx context.VirtualMachineContextA2 - vmClassSpec *vmopv1.VirtualMachineClassSpec - minCPUFreq uint64 - firmware string storageClassesToIDs map[string]string + baseConfigSpec *vimtypes.VirtualMachineConfigSpec configSpec *vimtypes.VirtualMachineConfigSpec - classConfigSpec *vimtypes.VirtualMachineConfigSpec ) BeforeEach(func() { - vmClass := builder.DummyVirtualMachineClassA2() - vmClassSpec = &vmClass.Spec - minCPUFreq = 2500 - firmware = "efi" + baseConfigSpec = &vimtypes.VirtualMachineConfigSpec{} storageClassesToIDs = map[string]string{} vm := builder.DummyVirtualMachineA2() @@ -142,14 +135,53 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { JustBeforeEach(func() { configSpec = virtualmachine.CreateConfigSpecForPlacement( vmCtx, - vmClassSpec, - minCPUFreq, - storageClassesToIDs, - firmware, - classConfigSpec) + baseConfigSpec, + storageClassesToIDs) Expect(configSpec).ToNot(BeNil()) }) + Context("Returns expected ConfigSpec", func() { + BeforeEach(func() { + baseConfigSpec = &vimtypes.VirtualMachineConfigSpec{ + Name: "dummy-VM", + Annotation: "test-annotation", + NumCPUs: 42, + MemoryMB: 4096, + Firmware: "secret-sauce", + DeviceChange: []vimtypes.BaseVirtualDeviceConfigSpec{ + &vimtypes.VirtualDeviceConfigSpec{ + Operation: vimtypes.VirtualDeviceConfigSpecOperationAdd, + Device: &vimtypes.VirtualPCIPassthrough{ + VirtualDevice: vimtypes.VirtualDevice{ + Backing: &vimtypes.VirtualPCIPassthroughVmiopBackingInfo{ + Vgpu: "SampleProfile2", + }, + }, + }, + }, + }, + } + }) + + It("Placement ConfigSpec contains expected field set sans ethernet device from class config spec", func() { + Expect(configSpec.Annotation).ToNot(BeEmpty()) + Expect(configSpec.Annotation).To(Equal(baseConfigSpec.Annotation)) + Expect(configSpec.NumCPUs).To(Equal(baseConfigSpec.NumCPUs)) + Expect(configSpec.MemoryMB).To(Equal(baseConfigSpec.MemoryMB)) + Expect(configSpec.CpuAllocation).To(Equal(baseConfigSpec.CpuAllocation)) + Expect(configSpec.MemoryAllocation).To(Equal(baseConfigSpec.MemoryAllocation)) + Expect(configSpec.Firmware).To(Equal(baseConfigSpec.Firmware)) + + Expect(configSpec.DeviceChange).To(HaveLen(2)) + dSpec := configSpec.DeviceChange[0].GetVirtualDeviceConfigSpec() + _, ok := dSpec.Device.(*vimtypes.VirtualPCIPassthrough) + Expect(ok).To(BeTrue()) + dSpec1 := configSpec.DeviceChange[1].GetVirtualDeviceConfigSpec() + _, ok = dSpec1.Device.(*vimtypes.VirtualDisk) + Expect(ok).To(BeTrue()) + }) + }) + Context("When InstanceStorage is configured", func() { const storagePolicyID = "storage-id-42" var oldIsInstanceStorageFSSEnabled func() bool @@ -160,7 +192,6 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { builder.AddDummyInstanceStorageVolumeA2(vmCtx.VM) storageClassesToIDs[builder.DummyStorageClassName] = storagePolicyID - classConfigSpec = nil }) AfterEach(func() { @@ -173,64 +204,20 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { assertInstanceStorageDeviceChange(configSpec.DeviceChange[2], 512, storagePolicyID) }) }) +}) - Context("when DaynDate FSS is enabled", func() { - var oldDaynDateFSSFn func() bool - - BeforeEach(func() { - oldDaynDateFSSFn = lib.IsVMClassAsConfigFSSDaynDateEnabled - lib.IsVMClassAsConfigFSSDaynDateEnabled = func() bool { return true } - }) +var _ = Describe("ConfigSpecFromVMClassDevices", func() { - AfterEach(func() { - lib.IsVMClassAsConfigFSSDaynDateEnabled = oldDaynDateFSSFn - }) + var ( + vmClassSpec *vmopv1.VirtualMachineClassSpec + configSpec *vimtypes.VirtualMachineConfigSpec + ) - Context("When class config spec is not nil", func() { - BeforeEach(func() { - classConfigSpec = &vimtypes.VirtualMachineConfigSpec{ - Name: "dummy-VM", - Annotation: "test-annotation", - DeviceChange: []vimtypes.BaseVirtualDeviceConfigSpec{ - &vimtypes.VirtualDeviceConfigSpec{ - Operation: vimtypes.VirtualDeviceConfigSpecOperationAdd, - Device: &vimtypes.VirtualPCIPassthrough{ - VirtualDevice: vimtypes.VirtualDevice{ - Backing: &vimtypes.VirtualPCIPassthroughVmiopBackingInfo{ - Vgpu: "SampleProfile2", - }, - }, - }, - }, - }, - } - }) - - AfterEach(func() { - classConfigSpec = nil - }) - - It("Placement ConfigSpec contains expected field set sans ethernet device from class config spec", func() { - Expect(configSpec.Annotation).ToNot(BeEmpty()) - Expect(configSpec.Annotation).To(Equal(classConfigSpec.Annotation)) - Expect(configSpec.NumCPUs).To(BeEquivalentTo(vmClassSpec.Hardware.Cpus)) - Expect(configSpec.MemoryMB).To(BeEquivalentTo(4 * 1024)) - Expect(configSpec.CpuAllocation).ToNot(BeNil()) - Expect(configSpec.MemoryAllocation).ToNot(BeNil()) - Expect(configSpec.Firmware).To(Equal(firmware)) - Expect(configSpec.DeviceChange).To(HaveLen(2)) - dSpec := configSpec.DeviceChange[0].GetVirtualDeviceConfigSpec() - _, ok := dSpec.Device.(*vimtypes.VirtualPCIPassthrough) - Expect(ok).To(BeTrue()) - dSpec1 := configSpec.DeviceChange[1].GetVirtualDeviceConfigSpec() - _, ok = dSpec1.Device.(*vimtypes.VirtualDisk) - Expect(ok).To(BeTrue()) - }) - }) - }) + Context("when Class specifies GPU/DDPIO in Hardware", func() { - Context("when Class specifies GPUs in Hardware", func() { BeforeEach(func() { + vmClassSpec = &vmopv1.VirtualMachineClassSpec{} + vmClassSpec.Hardware.Devices.VGPUDevices = []vmopv1.VGPUDevice{{ ProfileName: "createplacementspec-profile", }} @@ -242,19 +229,15 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { }} }) - AfterEach(func() { - vmClassSpec.Hardware.Devices.VGPUDevices = nil - vmClassSpec.Hardware.Devices.DynamicDirectPathIODevices = nil + JustBeforeEach(func() { + configSpec = virtualmachine.ConfigSpecFromVMClassDevices(vmClassSpec) }) - It("Placement ConfigSpec contains the GPU from the VM Class", func() { - Expect(configSpec.DeviceChange).To(HaveLen(3)) // one dummy disk + 2 Pass through devices - - dspec := configSpec.DeviceChange[0].GetVirtualDeviceConfigSpec() - _, ok := dspec.Device.(*vimtypes.VirtualDisk) - Expect(ok).To(BeTrue()) + It("Returns expected ConfigSpec", func() { + Expect(configSpec).ToNot(BeNil()) + Expect(configSpec.DeviceChange).To(HaveLen(2)) // One each for GPU an DDPIO above - dSpec1 := configSpec.DeviceChange[1].GetVirtualDeviceConfigSpec() + dSpec1 := configSpec.DeviceChange[0].GetVirtualDeviceConfigSpec() dev1, ok := dSpec1.Device.(*vimtypes.VirtualPCIPassthrough) Expect(ok).To(BeTrue()) pciDev1 := dev1.GetVirtualDevice() @@ -262,7 +245,7 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { Expect(ok1).To(BeTrue()) Expect(pciBacking1.Vgpu).To(Equal(vmClassSpec.Hardware.Devices.VGPUDevices[0].ProfileName)) - dSpec2 := configSpec.DeviceChange[2].GetVirtualDeviceConfigSpec() + dSpec2 := configSpec.DeviceChange[1].GetVirtualDeviceConfigSpec() dev2, ok2 := dSpec2.Device.(*vimtypes.VirtualPCIPassthrough) Expect(ok2).To(BeTrue()) pciDev2 := dev2.GetVirtualDevice() @@ -273,7 +256,6 @@ var _ = Describe("CreateConfigSpecForPlacement", func() { Expect(pciBacking2.CustomLabel).To(Equal(vmClassSpec.Hardware.Devices.DynamicDirectPathIODevices[0].CustomLabel)) }) }) - }) func assertInstanceStorageDeviceChange( diff --git a/pkg/vmprovider/providers/vsphere2/vmprovider.go b/pkg/vmprovider/providers/vsphere2/vmprovider.go index d6b82d82b..161b24262 100644 --- a/pkg/vmprovider/providers/vsphere2/vmprovider.go +++ b/pkg/vmprovider/providers/vsphere2/vmprovider.go @@ -279,7 +279,6 @@ func (vs *vSphereVMProvider) UpdateContentLibraryItem(ctx goctx.Context, itemID, func (vs *vSphereVMProvider) getOpID(vm *vmopv1.VirtualMachine, operation string) string { const charset = "0123456789abcdef" - // TODO: Is this actually useful? id := make([]byte, 8) for i := range id { idx := rand.Intn(len(charset)) //nolint:gosec diff --git a/pkg/vmprovider/providers/vsphere2/vmprovider_vm.go b/pkg/vmprovider/providers/vsphere2/vmprovider_vm.go index caa371e9c..d4f740d60 100644 --- a/pkg/vmprovider/providers/vsphere2/vmprovider_vm.go +++ b/pkg/vmprovider/providers/vsphere2/vmprovider_vm.go @@ -377,7 +377,7 @@ func (vs *vSphereVMProvider) vmCreateDoPlacement( vcClient *vcclient.Client, createArgs *VMCreateArgs) error { - placementConfigSpec := virtualmachine.CreateConfigSpecForPlacement2( + placementConfigSpec := virtualmachine.CreateConfigSpecForPlacement( vmCtx, createArgs.ConfigSpec, createArgs.StorageClassesToIDs) @@ -867,8 +867,8 @@ func (vs *vSphereVMProvider) vmCreateGenConfigSpec( vmCtx, vmClassConfigSpec, &createArgs.VMClass.Spec, - minCPUFreq, - createArgs.ImageStatus.Firmware) + createArgs.ImageStatus, + minCPUFreq) // TODO: This should be in CreateConfigSpec() if createArgs.ConfigSpec.Version == "" { @@ -1114,14 +1114,14 @@ func (vs *vSphereVMProvider) vmUpdateGetArgs( } } - imageFirmware := "" + var vmImageStatus *vmopv1.VirtualMachineImageStatus // Only get VM image when this is the VM first boot. if isVMFirstBoot(vmCtx) { - _, _, vmImageStatus, err := GetVirtualMachineImageSpecAndStatus(vmCtx, vs.k8sClient) + var err error + _, _, vmImageStatus, err = GetVirtualMachineImageSpecAndStatus(vmCtx, vs.k8sClient) if err != nil { return nil, err } - imageFirmware = vmImageStatus.Firmware // The only use of this is for the global JSON_EXTRA_CONFIG to set the image name. // The global extra config should only be set during first boot. @@ -1155,8 +1155,8 @@ func (vs *vSphereVMProvider) vmUpdateGetArgs( vmCtx, vmClassConfigSpec, &updateArgs.VMClass.Spec, - updateArgs.MinCPUFreq, - imageFirmware) + vmImageStatus, + updateArgs.MinCPUFreq) return updateArgs, nil }