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.

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
 	}
 

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

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
 	}
 

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