diff --git a/pkg/vmprovider/providers/vsphere/virtualmachine/configspec.go b/pkg/vmprovider/providers/vsphere/virtualmachine/configspec.go index 22c92c58f..7ff2d6ab6 100644 --- a/pkg/vmprovider/providers/vsphere/virtualmachine/configspec.go +++ b/pkg/vmprovider/providers/vsphere/virtualmachine/configspec.go @@ -88,8 +88,12 @@ func CreateConfigSpec( } } - // Use firmware type from the image if config spec doesn't have it. - if configSpec.Firmware == "" && imageFirmware != "" { + // Always use the image's firmware type if present. + // This is necessary until the vSphere UI can support creating VM Classes with + // an empty/nil firmware type. Since VM Classes created via the vSphere UI always have + // a default firmware value set (efi), this can cause VM boot failures for unsupported images. + if imageFirmware != "" { + // TODO: Use image firmware only when the class config spec has an empty firmware type. configSpec.Firmware = imageFirmware } diff --git a/pkg/vmprovider/providers/vsphere/virtualmachine/configspec_test.go b/pkg/vmprovider/providers/vsphere/virtualmachine/configspec_test.go index 5d213b46b..299ed4e71 100644 --- a/pkg/vmprovider/providers/vsphere/virtualmachine/configspec_test.go +++ b/pkg/vmprovider/providers/vsphere/virtualmachine/configspec_test.go @@ -71,6 +71,7 @@ var _ = Describe("CreateConfigSpec", func() { }, }, }, + Firmware: "bios", } }) @@ -99,6 +100,17 @@ var _ = Describe("CreateConfigSpec", func() { Expect(ok).To(BeTrue()) }) + + When("image firmware is empty", func() { + BeforeEach(func() { + firmware = "" + }) + + It("config spec has the firmware from the class", func() { + Expect(configSpec.Firmware).ToNot(Equal(firmware)) + Expect(configSpec.Firmware).To(Equal(classConfigSpec.Firmware)) + }) + }) }) }) diff --git a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go index 5ae236f88..b61a865b9 100644 --- a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go +++ b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec.go @@ -47,10 +47,14 @@ func CreateConfigSpec( Type: constants.ManagedByExtensionType, } - if val, ok := vmCtx.VM.Annotations[constants.FirmwareOverrideAnnotation]; ok { + if val, ok := vmCtx.VM.Annotations[constants.FirmwareOverrideAnnotation]; ok && (val == "efi" || val == "bios") { configSpec.Firmware = val - } else if configSpec.Firmware == "" && vmImageStatus != nil { - // Use firmware type from the image if ConfigSpec doesn't have it. + } else if vmImageStatus != nil && vmImageStatus.Firmware != "" { + // Use the image's firmware type if present. + // This is necessary until the vSphere UI can support creating VM Classes with + // an empty/nil firmware type. Since VM Classes created via the vSphere UI always has + // a non-empty firmware value set, this can cause VM boot failures. + // TODO: Use image firmware only when the class config spec has an empty firmware type. configSpec.Firmware = vmImageStatus.Firmware } diff --git a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go index 0dd01e72c..39625fead 100644 --- a/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go +++ b/pkg/vmprovider/providers/vsphere2/virtualmachine/configspec_test.go @@ -13,6 +13,7 @@ import ( vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha2" "github.com/vmware-tanzu/vm-operator/pkg/context" "github.com/vmware-tanzu/vm-operator/pkg/lib" + "github.com/vmware-tanzu/vm-operator/pkg/vmprovider/providers/vsphere2/constants" "github.com/vmware-tanzu/vm-operator/pkg/vmprovider/providers/vsphere2/virtualmachine" "github.com/vmware-tanzu/vm-operator/test/builder" ) @@ -21,6 +22,7 @@ var _ = Describe("CreateConfigSpec", func() { const vmName = "dummy-vm" var ( + vm *vmopv1.VirtualMachine vmCtx context.VirtualMachineContextA2 vmClassSpec *vmopv1.VirtualMachineClassSpec vmImageStatus *vmopv1.VirtualMachineImageStatus @@ -36,7 +38,7 @@ var _ = Describe("CreateConfigSpec", func() { vmImageStatus = &vmopv1.VirtualMachineImageStatus{Firmware: "efi"} minCPUFreq = 2500 - vm := builder.DummyVirtualMachineA2() + vm = builder.DummyVirtualMachineA2() vm.Name = vmName vmCtx = context.VirtualMachineContextA2{ Context: goctx.Background(), @@ -81,6 +83,7 @@ var _ = Describe("CreateConfigSpec", func() { }, }, }, + Firmware: "bios", } }) @@ -108,6 +111,37 @@ var _ = Describe("CreateConfigSpec", func() { _, ok := dSpec.Device.(*vimtypes.VirtualE1000) Expect(ok).To(BeTrue()) }) + + When("Image firmware is empty", func() { + BeforeEach(func() { + vmImageStatus = &vmopv1.VirtualMachineImageStatus{} + }) + + It("config spec has the firmware from the class", func() { + Expect(configSpec.Firmware).ToNot(Equal(vmImageStatus.Firmware)) + Expect(configSpec.Firmware).To(Equal(classConfigSpec.Firmware)) + }) + }) + + When("vm has a valid firmware override annotation", func() { + BeforeEach(func() { + vm.Annotations[constants.FirmwareOverrideAnnotation] = "efi" + }) + + It("config spec has overridden firmware annotation", func() { + Expect(configSpec.Firmware).To(Equal(vm.Annotations[constants.FirmwareOverrideAnnotation])) + }) + }) + + When("vm has an invalid firmware override annotation", func() { + BeforeEach(func() { + vm.Annotations[constants.FirmwareOverrideAnnotation] = "foo" + }) + + It("config spec doesn't have the invalid val", func() { + Expect(configSpec.Firmware).ToNot(Equal(vm.Annotations[constants.FirmwareOverrideAnnotation])) + }) + }) }) })