From be66239ac8ce586f2f218685ef9cb5359ad9f4e2 Mon Sep 17 00:00:00 2001 From: Sreyas Natarajan <53065832+sreyasn@users.noreply.github.com> Date: Wed, 4 Oct 2023 14:54:31 -0700 Subject: [PATCH] Reverse configSpec.firmware order of precedence to Image then Classes (#238) This change reverses of the order of precedence to populate the configSpec from the image first then the class. This is necessary with vsphere UI unable to create VM Classes with an empty firmware, potentially creating scenarios for VM boot failures when the wrong firmware type is populated. The image is known to always have the supported firmware type for boot. This reversal can be changed back once vSphere GUI supports creating VM Classes with an optional firmware type for a user. --- .../vsphere/virtualmachine/configspec.go | 8 +++-- .../vsphere/virtualmachine/configspec_test.go | 12 +++++++ .../vsphere2/virtualmachine/configspec.go | 10 ++++-- .../virtualmachine/configspec_test.go | 36 ++++++++++++++++++- 4 files changed, 60 insertions(+), 6 deletions(-) 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])) + }) + }) }) })