diff --git a/pkg/providers/vsphere/constants/constants.go b/pkg/providers/vsphere/constants/constants.go index 24a4d9e59..c13b8fb49 100644 --- a/pkg/providers/vsphere/constants/constants.go +++ b/pkg/providers/vsphere/constants/constants.go @@ -8,11 +8,13 @@ import ( ) const ( - ExtraConfigTrue = "TRUE" - ExtraConfigFalse = "FALSE" - ExtraConfigUnset = "" - ExtraConfigGuestInfoPrefix = "guestinfo." - ExtraConfigRunContainerKey = "RUN.container" + ExtraConfigTrue = "TRUE" + ExtraConfigFalse = "FALSE" + ExtraConfigUnset = "" + ExtraConfigGuestInfoPrefix = "guestinfo." + ExtraConfigRunContainerKey = "RUN.container" + ExtraConfigVMServiceName = "vmservice.name" + ExtraConfigVMServiceNamespace = "vmservice.namespace" // VCVMAnnotation Annotation placed on the VM. VCVMAnnotation = "Virtual Machine managed by the vSphere Virtual Machine service" diff --git a/pkg/providers/vsphere/resources/vm.go b/pkg/providers/vsphere/resources/vm.go index e9ec933a2..34b16f932 100644 --- a/pkg/providers/vsphere/resources/vm.go +++ b/pkg/providers/vsphere/resources/vm.go @@ -4,6 +4,7 @@ package resources import ( + "bytes" "context" "fmt" @@ -75,8 +76,20 @@ func (vm *VirtualMachine) Clone(ctx context.Context, folder *object.Folder, clon return &ref, nil } -func (vm *VirtualMachine) Reconfigure(ctx context.Context, configSpec *vimtypes.VirtualMachineConfigSpec) (*vimtypes.TaskInfo, error) { - vm.logger.V(5).Info("Reconfiguring VM", "configSpec", configSpec) +func (vm *VirtualMachine) Reconfigure( + ctx context.Context, + configSpec *vimtypes.VirtualMachineConfigSpec) (*vimtypes.TaskInfo, error) { + + logger := logr.FromContextOrDiscard(ctx) + + var w bytes.Buffer + enc := vimtypes.NewJSONEncoder(&w) + if err := enc.Encode(configSpec); err != nil { + logger.Error(err, "Failed to marshal ConfigSpec to JSON") + logger.Info("Reconfiguring VM", "configSpec", configSpec) + } else { + logger.Info("Reconfiguring VM", "configSpec", w.String()) + } reconfigureTask, err := vm.vcVirtualMachine.Reconfigure(ctx, *configSpec) if err != nil { diff --git a/pkg/providers/vsphere/session/session.go b/pkg/providers/vsphere/session/session.go index 7d8cc44a7..07a559a45 100644 --- a/pkg/providers/vsphere/session/session.go +++ b/pkg/providers/vsphere/session/session.go @@ -4,6 +4,8 @@ package session import ( + "fmt" + "github.com/vmware/govmomi/find" "github.com/vmware/govmomi/object" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -28,13 +30,11 @@ func (s *Session) invokeFsrVirtualMachine(vmCtx pkgctx.VirtualMachineContext, re task, err := internal.VirtualMachineFSR(vmCtx, resVM.MoRef(), s.Client.VimClient()) if err != nil { - vmCtx.Logger.Error(err, "InvokeFSR call failed") - return err + return fmt.Errorf("failed to invoke FSR: %w", err) } if err = task.Wait(vmCtx); err != nil { - vmCtx.Logger.Error(err, "InvokeFSR task failed") - return err + return fmt.Errorf("failed to wait on FSR task: %w", err) } return nil diff --git a/pkg/providers/vsphere/session/session_vm_update.go b/pkg/providers/vsphere/session/session_vm_update.go index e88f039f8..b21b3bf96 100644 --- a/pkg/providers/vsphere/session/session_vm_update.go +++ b/pkg/providers/vsphere/session/session_vm_update.go @@ -4,7 +4,6 @@ package session import ( - "bytes" "context" "fmt" "maps" @@ -21,6 +20,7 @@ import ( vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" vmopv1common "github.com/vmware-tanzu/vm-operator/api/v1alpha3/common" "github.com/vmware-tanzu/vm-operator/pkg" + "github.com/vmware-tanzu/vm-operator/pkg/conditions" pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config" pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context" "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/clustermodules" @@ -423,17 +423,6 @@ func UpdateConfigSpecAnnotation( } } -func UpdateConfigSpecManagedBy( - config *vimtypes.VirtualMachineConfigInfo, - configSpec *vimtypes.VirtualMachineConfigSpec) { - if config.ManagedBy == nil { - configSpec.ManagedBy = &vimtypes.ManagedByInfo{ - ExtensionKey: vmopv1.ManagedByExtensionKey, - Type: vmopv1.ManagedByExtensionType, - } - } -} - // UpdateConfigSpecGuestID sets the given vmSpecGuestID in the ConfigSpec if it // is not empty and different from the current GuestID in the VM's ConfigInfo. func UpdateConfigSpecGuestID( @@ -462,16 +451,24 @@ func UpdateConfigSpecFirmware( func updateConfigSpec( vmCtx pkgctx.VirtualMachineContext, config *vimtypes.VirtualMachineConfigInfo, - updateArgs *VMUpdateArgs) *vimtypes.VirtualMachineConfigSpec { + updateArgs *VMUpdateArgs) (*vimtypes.VirtualMachineConfigSpec, error) { configSpec := &vimtypes.VirtualMachineConfigSpec{} vmClassSpec := updateArgs.VMClass.Spec - UpdateConfigSpecAnnotation(config, configSpec) - UpdateConfigSpecManagedBy(config, configSpec) + if err := vmopv1util.OverwriteAlwaysResizeConfigSpec( + vmCtx, + *vmCtx.VM, + *config, + configSpec); err != nil { + + return nil, err + } + UpdateConfigSpecExtraConfig( vmCtx, config, configSpec, &updateArgs.ConfigSpec, &vmClassSpec, vmCtx.VM, updateArgs.ExtraConfig) + UpdateConfigSpecAnnotation(config, configSpec) UpdateConfigSpecChangeBlockTracking( vmCtx, config, configSpec, &updateArgs.ConfigSpec, vmCtx.VM.Spec) UpdateConfigSpecFirmware(config, configSpec, vmCtx.VM) @@ -483,7 +480,7 @@ func updateConfigSpec( resize.CompareMemoryAllocation(*config, updateArgs.ConfigSpec, configSpec) } - return configSpec + return configSpec, nil } func (s *Session) prePowerOnVMConfigSpec( @@ -491,7 +488,10 @@ func (s *Session) prePowerOnVMConfigSpec( config *vimtypes.VirtualMachineConfigInfo, updateArgs *VMUpdateArgs) (*vimtypes.VirtualMachineConfigSpec, error) { - configSpec := updateConfigSpec(vmCtx, config, updateArgs) + configSpec, err := updateConfigSpec(vmCtx, config, updateArgs) + if err != nil { + return nil, err + } virtualDevices := object.VirtualDeviceList(config.Hardware.Device) currentDisks := virtualDevices.SelectByType((*vimtypes.VirtualDisk)(nil)) @@ -557,23 +557,16 @@ func (s *Session) prePowerOnVMReconfigure( return err } - defaultConfigSpec := &vimtypes.VirtualMachineConfigSpec{} - if !apiEquality.Semantic.DeepEqual(configSpec, defaultConfigSpec) { - var w bytes.Buffer - enc := vimtypes.NewJSONEncoder(&w) - if err := enc.Encode(configSpec); err != nil { - vmCtx.Logger.Error(err, "Failed to marshal ConfigSpec to JSON") - vmCtx.Logger.Info("Pre PowerOn Reconfigure", "configSpec", configSpec) - } else { - vmCtx.Logger.Info("Pre PowerOn Reconfigure", "configSpec", w.String()) - } + if _, err := doReconfigure( + logr.NewContext( + vmCtx, + vmCtx.Logger.WithName("prePowerOnVMReconfigure"), + ), + vmCtx.VM, + resVM.VcVM(), + *configSpec); err != nil { - taskInfo, err := resVM.Reconfigure(vmCtx, configSpec) - vmutil.UpdateVMGuestIDReconfiguredCondition(vmCtx, *configSpec, taskInfo) - if err != nil { - vmCtx.Logger.Error(err, "pre power on reconfigure failed") - return err - } + return err } if features.VMResize || features.VMResizeCPUMemory { @@ -789,7 +782,16 @@ func (s *Session) poweredOnVMReconfigure( configSpec := &vimtypes.VirtualMachineConfigSpec{} - UpdateConfigSpecExtraConfig(vmCtx, config, configSpec, nil, nil, nil, nil) + if err := vmopv1util.OverwriteAlwaysResizeConfigSpec( + vmCtx, + *vmCtx.VM, + *config, + configSpec); err != nil { + + return false, err + } + + UpdateConfigSpecExtraConfig(vmCtx, config, configSpec, nil, nil, vmCtx.VM, nil) UpdateConfigSpecChangeBlockTracking(vmCtx, config, configSpec, nil, vmCtx.VM.Spec) if pkgcfg.FromContext(vmCtx).Features.IsoSupport { @@ -798,27 +800,26 @@ func (s *Session) poweredOnVMReconfigure( } } - var refetchProps bool + refetchProps, err := doReconfigure( + logr.NewContext( + vmCtx, + vmCtx.Logger.WithName("poweredOnVMReconfigure"), + ), + vmCtx.VM, + resVM.VcVM(), + *configSpec) - defaultConfigSpec := &vimtypes.VirtualMachineConfigSpec{} - if !apiEquality.Semantic.DeepEqual(configSpec, defaultConfigSpec) { - vmCtx.Logger.Info("PoweredOn Reconfigure", "configSpec", configSpec) - if _, err := resVM.Reconfigure(vmCtx, configSpec); err != nil { - vmCtx.Logger.Error(err, "powered on reconfigure failed") - return false, err - } + if err != nil { + return false, err + } - // Special case for CBT: in order for CBT change take effect for a powered on VM, - // a checkpoint save/restore is needed. tracks the implementation of - // this FSR internally to vSphere. - if configSpec.ChangeTrackingEnabled != nil { - if err := s.invokeFsrVirtualMachine(vmCtx, resVM); err != nil { - vmCtx.Logger.Error(err, "Failed to invoke FSR for CBT update") - return false, err - } + // Special case for CBT: in order for CBT change take effect for a powered on VM, + // a checkpoint save/restore is needed. tracks the implementation of + // this FSR internally to vSphere. + if refetchProps && configSpec.ChangeTrackingEnabled != nil { + if err := s.invokeFsrVirtualMachine(vmCtx, resVM); err != nil { + return refetchProps, fmt.Errorf("failed to invoke FSR for CBT update") } - - refetchProps = true } return refetchProps, nil @@ -874,21 +875,34 @@ func (s *Session) resizeVMWhenPoweredStateOff( } if pkgcfg.FromContext(vmCtx).Features.VMResize { - err := vmopv1util.OverwriteResizeConfigSpec(vmCtx, *vmCtx.VM, *moVM.Config, &configSpec) - if err != nil { + if err := vmopv1util.OverwriteResizeConfigSpec( + vmCtx, + *vmCtx.VM, + *moVM.Config, + &configSpec); err != nil { + return false, err } + } else if err := vmopv1util.OverwriteAlwaysResizeConfigSpec( + vmCtx, + *vmCtx.VM, + *moVM.Config, + &configSpec); err != nil { + + return false, err } - refetchProps := false - if !reflect.DeepEqual(configSpec, vimtypes.VirtualMachineConfigSpec{}) { - vmCtx.Logger.Info("Powered off resizing", "configSpec", configSpec) - if _, err := res.NewVMFromObject(vcVM).Reconfigure(vmCtx, &configSpec); err != nil { - vmCtx.Logger.Error(err, "powered off resize reconfigure failed") - return false, err - } + refetchProps, err := doReconfigure( + logr.NewContext( + vmCtx, + vmCtx.Logger.WithName("resizeVMWhenPoweredStateOff"), + ), + vmCtx.VM, + vcVM, + configSpec) - refetchProps = true + if err != nil { + return false, err } if needsResize { @@ -934,9 +948,13 @@ func (s *Session) updateVMDesiredPowerStateOff( vmCtx pkgctx.VirtualMachineContext, vcVM *object.VirtualMachine, getResizeArgsFn func() (*VMResizeArgs, error), - existingPowerState vmopv1.VirtualMachinePowerState) (refetchProps bool, err error) { + existingPowerState vmopv1.VirtualMachinePowerState) (bool, error) { + + var ( + powerOff bool + refetchProps bool + ) - var powerOff bool if existingPowerState == vmopv1.VirtualMachinePowerStateOn { powerOff = true } else if existingPowerState == vmopv1.VirtualMachinePowerStateSuspended { @@ -944,13 +962,13 @@ func (s *Session) updateVMDesiredPowerStateOff( vmCtx.VM.Spec.PowerOffMode == vmopv1.VirtualMachinePowerOpModeTrySoft } if powerOff { - err = res.NewVMFromObject(vcVM).SetPowerState( + if err := res.NewVMFromObject(vcVM).SetPowerState( logr.NewContext(vmCtx, vmCtx.Logger), existingPowerState, vmCtx.VM.Spec.PowerState, - vmCtx.VM.Spec.PowerOffMode) - if err != nil { - return refetchProps, err + vmCtx.VM.Spec.PowerOffMode); err != nil { + + return false, err } refetchProps = true @@ -971,7 +989,7 @@ func (s *Session) updateVMDesiredPowerStateOff( } if f := pkgcfg.FromContext(vmCtx).Features; f.VMResize || f.VMResizeCPUMemory { - refetchProps, err = s.resizeVMWhenPoweredStateOff( + refetch, err := s.resizeVMWhenPoweredStateOff( vmCtx, vcVM, vmCtx.MoVM, @@ -979,6 +997,17 @@ func (s *Session) updateVMDesiredPowerStateOff( if err != nil { return refetchProps, err } + if refetch { + refetchProps = true + } + } else { + refetch, err := defaultReconfigure(vmCtx, vcVM, *vmCtx.MoVM.Config) + if err != nil { + return refetchProps, err + } + if refetch { + refetchProps = true + } } return refetchProps, err @@ -987,22 +1016,33 @@ func (s *Session) updateVMDesiredPowerStateOff( func (s *Session) updateVMDesiredPowerStateSuspended( vmCtx pkgctx.VirtualMachineContext, vcVM *object.VirtualMachine, - existingPowerState vmopv1.VirtualMachinePowerState) (refetchProps bool, err error) { + existingPowerState vmopv1.VirtualMachinePowerState) (bool, error) { + + var ( + refetchProps bool + ) if existingPowerState == vmopv1.VirtualMachinePowerStateOn { - err = res.NewVMFromObject(vcVM).SetPowerState( + if err := res.NewVMFromObject(vcVM).SetPowerState( logr.NewContext(vmCtx, vmCtx.Logger), existingPowerState, vmCtx.VM.Spec.PowerState, - vmCtx.VM.Spec.SuspendMode) - if err != nil { - return refetchProps, err + vmCtx.VM.Spec.SuspendMode); err != nil { + return false, err } refetchProps = true } - return + refetch, err := defaultReconfigure(vmCtx, vcVM, *vmCtx.MoVM.Config) + if err != nil { + return refetchProps, err + } + if refetch { + refetchProps = true + } + + return refetchProps, nil } func (s *Session) updateVMDesiredPowerStateOn( @@ -1168,6 +1208,7 @@ func (s *Session) UpdateVirtualMachine( } } else { vmCtx.Logger.Info("VirtualMachine is paused. PowerState is not updated.") + refetchProps, err = defaultReconfigure(vmCtx, vcVM, *vmCtx.MoVM.Config) } if refetchProps { @@ -1215,3 +1256,76 @@ func isVMPaused(vmCtx pkgctx.VirtualMachineContext) bool { delete(vm.Labels, vmopv1.PausedVMLabelKey) return false } + +func defaultReconfigure( + vmCtx pkgctx.VirtualMachineContext, + vcVM *object.VirtualMachine, + configInfo vimtypes.VirtualMachineConfigInfo) (bool, error) { + + var configSpec vimtypes.VirtualMachineConfigSpec + if err := vmopv1util.OverwriteAlwaysResizeConfigSpec( + vmCtx, + *vmCtx.VM, + configInfo, + &configSpec); err != nil { + + return false, err + } + + return doReconfigure( + logr.NewContext( + vmCtx, + vmCtx.Logger.WithName("defaultReconfigure"), + ), + vmCtx.VM, + vcVM, + configSpec) +} + +func doReconfigure( + ctx context.Context, + vm *vmopv1.VirtualMachine, + vcVM *object.VirtualMachine, + configSpec vimtypes.VirtualMachineConfigSpec) (bool, error) { + + var defaultConfigSpec vimtypes.VirtualMachineConfigSpec + if apiEquality.Semantic.DeepEqual(configSpec, defaultConfigSpec) { + return false, nil + } + + resVM := res.NewVMFromObject(vcVM) + taskInfo, err := resVM.Reconfigure(ctx, &configSpec) + + UpdateVMGuestIDReconfiguredCondition(vm, configSpec, taskInfo) + + if err != nil { + return false, err + } + + return true, err +} + +// UpdateVMGuestIDReconfiguredCondition deletes the VM's GuestIDReconfigured +// condition if the configSpec doesn't contain a guestID, or if the taskInfo +// does not contain an invalid guestID property error. Otherwise, it sets the +// condition to false with the invalid guest ID property value in the reason. +func UpdateVMGuestIDReconfiguredCondition( + vm *vmopv1.VirtualMachine, + configSpec vimtypes.VirtualMachineConfigSpec, + taskInfo *vimtypes.TaskInfo) { + + if configSpec.GuestId != "" && vmutil.IsTaskInfoErrorInvalidGuestID(taskInfo) { + conditions.MarkFalse( + vm, + vmopv1.GuestIDReconfiguredCondition, + "Invalid", + fmt.Sprintf( + "The specified guest ID value is not supported: %s", + configSpec.GuestId, + ), + ) + return + } + + conditions.Delete(vm, vmopv1.GuestIDReconfiguredCondition) +} diff --git a/pkg/providers/vsphere/session/session_vm_update_test.go b/pkg/providers/vsphere/session/session_vm_update_test.go index 6ac2445b7..6e956edda 100644 --- a/pkg/providers/vsphere/session/session_vm_update_test.go +++ b/pkg/providers/vsphere/session/session_vm_update_test.go @@ -1710,3 +1710,125 @@ var _ = Describe("UpdateVirtualMachine", func() { }) }) }) + +var _ = Describe("UpdateVMGuestIDReconfiguredCondition", func() { + + var ( + vmCtx pkgctx.VirtualMachineContext + vmopVM vmopv1.VirtualMachine + configSpec vimtypes.VirtualMachineConfigSpec + taskInfo *vimtypes.TaskInfo + ) + + BeforeEach(func() { + // Init VM with the condition set to verify it's actually deleted. + vmopVM = vmopv1.VirtualMachine{ + Status: vmopv1.VirtualMachineStatus{ + Conditions: []metav1.Condition{ + { + Type: vmopv1.GuestIDReconfiguredCondition, + Status: metav1.ConditionFalse, + }, + }, + }, + } + vmCtx = pkgctx.VirtualMachineContext{ + VM: &vmopVM, + } + configSpec = vimtypes.VirtualMachineConfigSpec{} + taskInfo = &vimtypes.TaskInfo{} + }) + + JustBeforeEach(func() { + session.UpdateVMGuestIDReconfiguredCondition(vmCtx.VM, configSpec, taskInfo) + }) + + Context("ConfigSpec doesn't have a guest ID", func() { + + BeforeEach(func() { + configSpec.GuestId = "" + }) + + It("should delete the existing VM's guest ID condition", func() { + Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) + }) + }) + + Context("ConfigSpec has a guest ID", func() { + + BeforeEach(func() { + configSpec.GuestId = "test-guest-id-value" + }) + + When("TaskInfo is nil", func() { + + BeforeEach(func() { + taskInfo = nil + }) + + It("should delete the VM's guest ID condition", func() { + Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) + }) + }) + + When("TaskInfo.Error is nil", func() { + + BeforeEach(func() { + taskInfo.Error = nil + }) + + It("should delete the VM's guest ID condition", func() { + Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) + }) + }) + + When("TaskInfo.Error.Fault is not an InvalidPropertyFault", func() { + + BeforeEach(func() { + taskInfo.Error = &vimtypes.LocalizedMethodFault{ + Fault: &vimtypes.InvalidName{ + Name: "some-invalid-name", + }, + } + }) + + It("should delete the VM's guest ID condition", func() { + Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) + }) + }) + + When("TaskInfo.Error contains an invalid property error NOT about guestID", func() { + + BeforeEach(func() { + taskInfo.Error = &vimtypes.LocalizedMethodFault{ + Fault: &vimtypes.InvalidArgument{ + InvalidProperty: "config.version", + }, + } + }) + + It("should delete the VM's guest ID condition", func() { + Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) + }) + }) + + When("TaskInfo.Error contains an invalid property error of guestID", func() { + + BeforeEach(func() { + taskInfo.Error = &vimtypes.LocalizedMethodFault{ + Fault: &vimtypes.InvalidArgument{ + InvalidProperty: "configSpec.guestId", + }, + } + }) + + It("should set the VM's guest ID condition to false with the invalid value in the reason", func() { + c := conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition) + Expect(c).NotTo(BeNil()) + Expect(c.Status).To(Equal(metav1.ConditionFalse)) + Expect(c.Reason).To(Equal("Invalid")) + Expect(c.Message).To(Equal("The specified guest ID value is not supported: test-guest-id-value")) + }) + }) + }) +}) diff --git a/pkg/providers/vsphere/virtualmachine/configspec.go b/pkg/providers/vsphere/virtualmachine/configspec.go index 73a6596be..a54f3c86e 100644 --- a/pkg/providers/vsphere/virtualmachine/configspec.go +++ b/pkg/providers/vsphere/virtualmachine/configspec.go @@ -42,6 +42,19 @@ func CreateConfigSpec( Type: vmopv1.ManagedByExtensionType, } + // Ensure ExtraConfig contains the name/namespace of the VM's Kubernetes + // resource. + configSpec.ExtraConfig = util.OptionValues(configSpec.ExtraConfig).Merge( + &vimtypes.OptionValue{ + Key: constants.ExtraConfigVMServiceName, + Value: vmCtx.VM.Name, + }, + &vimtypes.OptionValue{ + Key: constants.ExtraConfigVMServiceNamespace, + Value: vmCtx.VM.Namespace, + }, + ) + // spec.biosUUID is only set when creating a VM and is immutable. // This field should not be updated for existing VMs. if id := vmCtx.VM.Spec.BiosUUID; id != "" { diff --git a/pkg/util/resize/configspec.go b/pkg/util/resize/configspec.go index 555803a6f..ed8e1f5b4 100644 --- a/pkg/util/resize/configspec.go +++ b/pkg/util/resize/configspec.go @@ -25,7 +25,6 @@ func CreateResizeConfigSpec( outCS := vimtypes.VirtualMachineConfigSpec{} compareAnnotation(ci, cs, &outCS) - compareManagedBy(ci, cs, &outCS) compareHardware(ci, cs, &outCS) CompareCPUAllocation(ci, cs, &outCS) compareCPUHotAddOrRemove(ci, cs, &outCS) @@ -82,18 +81,6 @@ func compareAnnotation( } } -// compareManagedBy compares the ConfigInfo.ManagedBy. -func compareManagedBy( - ci vimtypes.VirtualMachineConfigInfo, - cs vimtypes.VirtualMachineConfigSpec, - outCS *vimtypes.VirtualMachineConfigSpec) { - - if ci.ManagedBy == nil { - // Only change the ManagedBy if it is currently unset. - outCS.ManagedBy = cs.ManagedBy - } -} - // compareHardware compares the ConfigInfo.Hardware. func compareHardware( ci vimtypes.VirtualMachineConfigInfo, diff --git a/pkg/util/resize/configspec_test.go b/pkg/util/resize/configspec_test.go index fee61b1ae..41d39d6d6 100644 --- a/pkg/util/resize/configspec_test.go +++ b/pkg/util/resize/configspec_test.go @@ -49,15 +49,6 @@ var _ = Describe("CreateResizeConfigSpec", func() { ConfigSpec{Annotation: "my-annotation"}, ConfigSpec{Annotation: "my-annotation"}), - Entry("ManagedBy is currently set", - ConfigInfo{ManagedBy: &vimtypes.ManagedByInfo{Type: "my-managed-by"}}, - ConfigSpec{}, - ConfigSpec{}), - Entry("ManagedBy is currently unset", - ConfigInfo{}, - ConfigSpec{ManagedBy: &vimtypes.ManagedByInfo{Type: "my-managed-by"}}, - ConfigSpec{ManagedBy: &vimtypes.ManagedByInfo{Type: "my-managed-by"}}), - Entry("NumCPUs needs updating", ConfigInfo{Hardware: vimtypes.VirtualHardware{NumCPU: 2}}, ConfigSpec{NumCPUs: 4}, diff --git a/pkg/util/vmopv1/resize_overwrite.go b/pkg/util/vmopv1/resize_overwrite.go index 6431efbb5..5fa45d54a 100644 --- a/pkg/util/vmopv1/resize_overwrite.go +++ b/pkg/util/vmopv1/resize_overwrite.go @@ -18,11 +18,15 @@ import ( // the current VM state to the ConfigSpec. These are fields that we can change without the // VM Class. func OverwriteResizeConfigSpec( - _ context.Context, + ctx context.Context, vm vmopv1.VirtualMachine, ci vimtypes.VirtualMachineConfigInfo, cs *vimtypes.VirtualMachineConfigSpec) error { + if err := OverwriteAlwaysResizeConfigSpec(ctx, vm, ci, cs); err != nil { + return err + } + if adv := vm.Spec.Advanced; adv != nil { ptr.OverwriteWithUser(&cs.ChangeTrackingEnabled, adv.ChangeBlockTracking, ci.ChangeTrackingEnabled) } @@ -33,6 +37,21 @@ func OverwriteResizeConfigSpec( return nil } +// OverwriteAlwaysResizeConfigSpec applies any set fields in the VM Spec or +// changes required from the current VM state to the ConfigSpec. These are +// fields that change without the VM Class. +func OverwriteAlwaysResizeConfigSpec( + _ context.Context, + vm vmopv1.VirtualMachine, + ci vimtypes.VirtualMachineConfigInfo, + cs *vimtypes.VirtualMachineConfigSpec) error { + + overwriteManagedBy(vm, ci, cs) + overwriteExtraConfigNamespaceName(vm, ci, cs) + + return nil +} + func overwriteGuestID( vm vmopv1.VirtualMachine, ci vimtypes.VirtualMachineConfigInfo, @@ -53,6 +72,7 @@ func overwriteExtraConfig( var toMerge []vimtypes.BaseOptionValue + toMerge = append(toMerge, ensureNamespaceName(vm, ci, cs)...) toMerge = append(toMerge, overrideMMIOSize(vm, ci, cs)...) toMerge = append(toMerge, clearMMPowerOffEC(vm, ci, cs)...) toMerge = append(toMerge, updateV1Alpha1CompatibleEC(vm, ci, cs)...) @@ -60,6 +80,86 @@ func overwriteExtraConfig( cs.ExtraConfig = util.OptionValues(cs.ExtraConfig).Merge(toMerge...) } +func overwriteExtraConfigNamespaceName( + vm vmopv1.VirtualMachine, + ci vimtypes.VirtualMachineConfigInfo, + cs *vimtypes.VirtualMachineConfigSpec) { + + var toMerge []vimtypes.BaseOptionValue + + toMerge = append(toMerge, ensureNamespaceName(vm, ci, cs)...) + + cs.ExtraConfig = util.OptionValues(cs.ExtraConfig).Merge(toMerge...) +} + +func overwriteManagedBy( + _ vmopv1.VirtualMachine, + ci vimtypes.VirtualMachineConfigInfo, + cs *vimtypes.VirtualMachineConfigSpec) { + + var current vimtypes.ManagedByInfo + if ci.ManagedBy != nil { + current = *ci.ManagedBy + } + + if cs.ManagedBy == nil { + cs.ManagedBy = &vimtypes.ManagedByInfo{} + } + + user := vimtypes.ManagedByInfo{ + ExtensionKey: vmopv1.ManagedByExtensionKey, + Type: vmopv1.ManagedByExtensionType, + } + + overwrite(cs.ManagedBy, user, current) + + var empty vimtypes.ManagedByInfo + if cs.ManagedBy != nil && *cs.ManagedBy == empty { + cs.ManagedBy = nil + } +} + +func ensureNamespaceName( + vm vmopv1.VirtualMachine, + ci vimtypes.VirtualMachineConfigInfo, + cs *vimtypes.VirtualMachineConfigSpec) []vimtypes.BaseOptionValue { + + outEC := []vimtypes.BaseOptionValue{} + curEC := util.OptionValues(ci.ExtraConfig).StringMap() + inEC := util.OptionValues(cs.ExtraConfig).StringMap() + + fn := func(key string, expectedVal string) { + // Does the VM have the key set in EC? + if v, ok := curEC[key]; ok { + if v == expectedVal { + // The key is present and correct; is the ConfigSpec trying to + // set it again? + if _, ok := inEC[key]; ok { + // Remove the entry from the ConfigSpec. + cs.ExtraConfig = util.OptionValues(cs.ExtraConfig).Delete(key) + } + } else { + // The key is present but incorrect. + outEC = append(outEC, &vimtypes.OptionValue{ + Key: key, + Value: expectedVal, + }) + } + } else { + // The key is not present. + outEC = append(outEC, &vimtypes.OptionValue{ + Key: key, + Value: expectedVal, + }) + } + } + + fn(constants.ExtraConfigVMServiceNamespace, vm.Namespace) + fn(constants.ExtraConfigVMServiceName, vm.Name) + + return outEC +} + func overrideMMIOSize( vm vmopv1.VirtualMachine, ci vimtypes.VirtualMachineConfigInfo, diff --git a/pkg/util/vmopv1/resize_overwrite_test.go b/pkg/util/vmopv1/resize_overwrite_test.go index c7c84e969..554b88f43 100644 --- a/pkg/util/vmopv1/resize_overwrite_test.go +++ b/pkg/util/vmopv1/resize_overwrite_test.go @@ -15,6 +15,7 @@ import ( vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/constants" + pkgutil "github.com/vmware-tanzu/vm-operator/pkg/util" vmopv1util "github.com/vmware-tanzu/vm-operator/pkg/util/vmopv1" "github.com/vmware-tanzu/vm-operator/test/builder" ) @@ -39,6 +40,62 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { return *vm } + vmConfigInfoManagedBy := func(ci ConfigInfo, args ...string) ConfigInfo { + var ( + exKey = vmopv1.ManagedByExtensionKey + exType = vmopv1.ManagedByExtensionType + ) + + if len(args) > 0 { + exKey = args[0] + } + if len(args) > 1 { + exType = args[1] + } + + ci.ManagedBy = &vimtypes.ManagedByInfo{ + ExtensionKey: exKey, + Type: exType, + } + + return ci + } + + vmConfigSpecManagedBy := func(cs ConfigSpec, args ...string) ConfigSpec { + var ( + exKey = vmopv1.ManagedByExtensionKey + exType = vmopv1.ManagedByExtensionType + ) + + if len(args) > 0 { + exKey = args[0] + } + if len(args) > 1 { + exType = args[1] + } + + cs.ManagedBy = &vimtypes.ManagedByInfo{ + ExtensionKey: exKey, + Type: exType, + } + + return cs + } + + vmConfigSpecNamespaceName := func(cs ConfigSpec) ConfigSpec { + cs.ExtraConfig = pkgutil.OptionValues(cs.ExtraConfig).Merge( + &vimtypes.OptionValue{ + Key: constants.ExtraConfigVMServiceNamespace, + Value: "", + }, + &vimtypes.OptionValue{ + Key: constants.ExtraConfigVMServiceName, + Value: "", + }, + ) + return cs + } + DescribeTable("Resize Overrides", func(vm vmopv1.VirtualMachine, ci ConfigInfo, @@ -53,69 +110,94 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { vmopv1.VirtualMachine{}, ConfigInfo{}, ConfigSpec{}, - ConfigSpec{}), - + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("CBT not set in VM Spec but in ConfigSpec", vmAdvSpec(vmopv1.VirtualMachineAdvancedSpec{}), ConfigInfo{}, ConfigSpec{ChangeTrackingEnabled: truePtr}, - ConfigSpec{ChangeTrackingEnabled: truePtr}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{ChangeTrackingEnabled: truePtr}))), Entry("CBT set in VM Spec takes precedence over ConfigSpec", vmAdvSpec(vmopv1.VirtualMachineAdvancedSpec{ChangeBlockTracking: falsePtr}), ConfigInfo{}, ConfigSpec{ChangeTrackingEnabled: truePtr}, - ConfigSpec{ChangeTrackingEnabled: falsePtr}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{ChangeTrackingEnabled: falsePtr}))), Entry("CBT set in VM Spec but not in ConfigSpec", vmAdvSpec(vmopv1.VirtualMachineAdvancedSpec{ChangeBlockTracking: truePtr}), ConfigInfo{}, ConfigSpec{}, - ConfigSpec{ChangeTrackingEnabled: truePtr}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{ChangeTrackingEnabled: truePtr}))), Entry("CBT set in VM Spec with same value in ConfigInfo", vmAdvSpec(vmopv1.VirtualMachineAdvancedSpec{ChangeBlockTracking: truePtr}), ConfigInfo{ChangeTrackingEnabled: truePtr}, ConfigSpec{}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("CBT set in ConfigSpec with same value in ConfigInfo", vmAdvSpec(vmopv1.VirtualMachineAdvancedSpec{}), ConfigInfo{ChangeTrackingEnabled: truePtr}, ConfigSpec{ChangeTrackingEnabled: truePtr}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("CBT set in VM Spec with same value in ConfigInfo but different value in ConfigSpec", vmAdvSpec(vmopv1.VirtualMachineAdvancedSpec{ChangeBlockTracking: truePtr}), ConfigInfo{ChangeTrackingEnabled: truePtr}, ConfigSpec{ChangeTrackingEnabled: falsePtr}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("Guest not set in VM Spec but in ConfigSpec is ignored", vmGuestID(""), ConfigInfo{}, ConfigSpec{GuestId: "foo"}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("GuestID set in VM Spec takes precedence over ConfigSpec", vmGuestID("foo"), ConfigInfo{}, ConfigSpec{GuestId: "bar"}, - ConfigSpec{GuestId: "foo"}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{GuestId: "foo"}))), Entry("GuestID set in VM Spec but not in ConfigSpec", vmGuestID("foo"), ConfigInfo{}, ConfigSpec{}, - ConfigSpec{GuestId: "foo"}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{GuestId: "foo"}))), Entry("GuestID set in VM Spec with same value in ConfigInfo", vmGuestID("foo"), ConfigInfo{GuestId: "foo"}, ConfigSpec{}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("GuestID set in ConfigSpec with same value in ConfigInfo", vmGuestID(""), ConfigInfo{GuestId: "foo"}, ConfigSpec{GuestId: "foo"}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), Entry("GuestID set in VM Spec with same value in ConfigInfo but different value in ConfigSpec", vmGuestID("foo"), ConfigInfo{GuestId: "foo"}, ConfigSpec{GuestId: "bar"}, - ConfigSpec{}), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), + + Entry("ManagedBy not set in ConfigInfo or ConfigSpec", + vmopv1.VirtualMachine{}, + ConfigInfo{}, + ConfigSpec{}, + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), + Entry("ManagedBy set in ConfigInfo and not in ConfigSpec", + vmopv1.VirtualMachine{}, + vmConfigInfoManagedBy(ConfigInfo{}), + ConfigSpec{}, + vmConfigSpecNamespaceName(ConfigSpec{})), + Entry("ManagedBy set in ConfigInfo with same value in ConfigSpec", + vmopv1.VirtualMachine{}, + vmConfigInfoManagedBy(ConfigInfo{}), + vmConfigSpecManagedBy(ConfigSpec{}), + vmConfigSpecNamespaceName(ConfigSpec{})), + Entry("ManagedBy set in ConfigInfo with wrong value in ConfigSpec", + vmopv1.VirtualMachine{}, + vmConfigInfoManagedBy(ConfigInfo{}), + vmConfigSpecManagedBy(ConfigSpec{}, "fake", "fake"), + vmConfigSpecNamespaceName(ConfigSpec{})), + Entry("ManagedBy not set in ConfigInfo with wrong value in ConfigSpec", + vmopv1.VirtualMachine{}, + ConfigInfo{}, + vmConfigSpecManagedBy(ConfigSpec{}, "fake", "fake"), + vmConfigSpecNamespaceName(vmConfigSpecManagedBy(ConfigSpec{}))), ) Context("ExtraConfig", func() { @@ -123,6 +205,9 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { vm vmopv1.VirtualMachine ci ConfigInfo cs ConfigSpec + + nameVal = &vimtypes.OptionValue{Key: constants.ExtraConfigVMServiceName, Value: vm.Name} + namespaceVal = &vimtypes.OptionValue{Key: constants.ExtraConfigVMServiceNamespace, Value: vm.Namespace} ) BeforeEach(func() { @@ -137,6 +222,51 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { Expect(err).ToNot(HaveOccurred()) }) + Context("Namespace and name", func() { + When("VM already has expected EC values", func() { + BeforeEach(func() { + ci.ExtraConfig = append(ci.ExtraConfig, nameVal, namespaceVal) + }) + It("no updates", func() { + Expect(cs.ExtraConfig).To(BeEmpty()) + }) + }) + When("VM ConfigSpec already has expected EC values", func() { + BeforeEach(func() { + cs.ExtraConfig = append(cs.ExtraConfig, nameVal, namespaceVal) + }) + It("same changes", func() { + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) + }) + }) + When("VM has none of expected EC values", func() { + It("adds it", func() { + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) + }) + }) + When("VM has different than expected EC values", func() { + BeforeEach(func() { + ov1, ov2 := *nameVal, *namespaceVal + ov1.Value = "fake" + ov2.Value = "fake" + ci.ExtraConfig = append(ci.ExtraConfig, &ov1, &ov2) + }) + It("updates it", func() { + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) + }) + }) + Context("VM and ConfigSpec already have expected values", func() { + BeforeEach(func() { + ci.ExtraConfig = append(ci.ExtraConfig, nameVal, namespaceVal) + cs.ExtraConfig = append(cs.ExtraConfig, nameVal, namespaceVal) + }) + + It("removes updates", func() { + Expect(cs.ExtraConfig).To(BeEmpty()) + }) + }) + }) + Context("MMIO ExtraConfig", func() { const mmIOValue = "42" @@ -163,7 +293,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("no updates", func() { - Expect(cs.ExtraConfig).To(BeEmpty()) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) }) }) @@ -173,13 +303,13 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("same changes", func() { - Expect(cs.ExtraConfig).To(ConsistOf(mmIOOptVal, mmIOSizeOptVal)) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal, mmIOOptVal, mmIOSizeOptVal)) }) }) Context("VM has none of expected MMIO EC values", func() { It("adds updates", func() { - Expect(cs.ExtraConfig).To(ConsistOf(mmIOOptVal, mmIOSizeOptVal)) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal, mmIOOptVal, mmIOSizeOptVal)) }) }) @@ -191,7 +321,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("updates it", func() { - Expect(cs.ExtraConfig).To(ConsistOf(mmIOOptVal)) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal, mmIOOptVal)) }) }) @@ -202,7 +332,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("removes updates", func() { - Expect(cs.ExtraConfig).To(BeEmpty()) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) }) }) }) @@ -215,7 +345,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { Context("VM does not have MMPowerOff EC", func() { It("no updates", func() { - Expect(cs.ExtraConfig).To(BeEmpty()) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) }) }) @@ -225,7 +355,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("no updates", func() { - Expect(cs.ExtraConfig).To(BeEmpty()) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) }) }) @@ -237,7 +367,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("removes it", func() { - Expect(cs.ExtraConfig).To(ConsistOf(mmPowerOffOptVal)) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal, mmPowerOffOptVal)) }) }) }) @@ -258,7 +388,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { Context("v1a1 compat is not present", func() { It("no updates", func() { - Expect(cs.ExtraConfig).To(BeEmpty()) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) }) }) @@ -268,7 +398,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("updates to enabled", func() { - Expect(cs.ExtraConfig).To(ConsistOf(v1a1CompatEnabledOptVal)) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal, v1a1CompatEnabledOptVal)) }) }) @@ -278,7 +408,7 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { }) It("no updates", func() { - Expect(cs.ExtraConfig).To(BeEmpty()) + Expect(cs.ExtraConfig).To(ConsistOf(nameVal, namespaceVal)) }) }) }) diff --git a/pkg/util/vsphere/vm/guest_id.go b/pkg/util/vsphere/vm/guest_id.go index 6a4f99197..718148783 100644 --- a/pkg/util/vsphere/vm/guest_id.go +++ b/pkg/util/vsphere/vm/guest_id.go @@ -4,55 +4,26 @@ package vm import ( - "fmt" "strings" vimtypes "github.com/vmware/govmomi/vim25/types" - - vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" - "github.com/vmware-tanzu/vm-operator/pkg/conditions" - pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context" ) // GuestIDProperty is the property name for the guest ID in the config spec. const GuestIDProperty = "configSpec.guestId" -// UpdateVMGuestIDReconfiguredCondition deletes the VM's GuestIDReconfigured -// condition if the configSpec doesn't contain a guestID, or if the taskInfo -// does not contain an invalid guestID property error. Otherwise, it sets the -// condition to false with the invalid guest ID property value in the reason. -func UpdateVMGuestIDReconfiguredCondition( - vmctx pkgctx.VirtualMachineContext, - configSpec vimtypes.VirtualMachineConfigSpec, - taskInfo *vimtypes.TaskInfo) { - if configSpec.GuestId == "" { - conditions.Delete(vmctx.VM, vmopv1.GuestIDReconfiguredCondition) - return +// IsTaskInfoErrorInvalidGuestID returns true if the provided taskInfo contains +// an error with a fault that indicates the underlying cause is an invalid guest +// ID. +func IsTaskInfoErrorInvalidGuestID(taskInfo *vimtypes.TaskInfo) bool { + if taskInfo == nil { + return false } - - var invalidGuestID bool - - defer func() { - if invalidGuestID { - conditions.MarkFalse( - vmctx.VM, - vmopv1.GuestIDReconfiguredCondition, - "Invalid", - fmt.Sprintf("The specified guest ID value is not supported: %s", - configSpec.GuestId)) - } else { - conditions.Delete(vmctx.VM, vmopv1.GuestIDReconfiguredCondition) - } - }() - - if taskInfo == nil || taskInfo.Error == nil { - return + if taskInfo.Error == nil { + return false } - - fault, ok := taskInfo.Error.Fault.(*vimtypes.InvalidArgument) - if !ok { - return + if f, ok := taskInfo.Error.Fault.(*vimtypes.InvalidArgument); ok { + return strings.Contains(f.InvalidProperty, GuestIDProperty) } - - invalidGuestID = strings.Contains(fault.InvalidProperty, GuestIDProperty) + return false } diff --git a/pkg/util/vsphere/vm/guest_id_test.go b/pkg/util/vsphere/vm/guest_id_test.go index a7915aa47..ab095d83c 100644 --- a/pkg/util/vsphere/vm/guest_id_test.go +++ b/pkg/util/vsphere/vm/guest_id_test.go @@ -8,135 +8,62 @@ import ( . "github.com/onsi/gomega" vimtypes "github.com/vmware/govmomi/vim25/types" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" - "github.com/vmware-tanzu/vm-operator/pkg/conditions" - pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context" vmutil "github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm" ) func guestIDTests() { - - Context("UpdateVMGuestIDReconfiguredCondition", func() { - - var ( - vmCtx pkgctx.VirtualMachineContext - vmopVM vmopv1.VirtualMachine - configSpec vimtypes.VirtualMachineConfigSpec - taskInfo *vimtypes.TaskInfo - ) - - BeforeEach(func() { - // Init VM with the condition set to verify it's actually deleted. - vmopVM = vmopv1.VirtualMachine{ - Status: vmopv1.VirtualMachineStatus{ - Conditions: []metav1.Condition{ - { - Type: vmopv1.GuestIDReconfiguredCondition, - Status: metav1.ConditionFalse, - }, + DescribeTable("IsTaskInfoErrorInvalidGuestID", + func(taskInfo *vimtypes.TaskInfo, expected bool) { + Expect(vmutil.IsTaskInfoErrorInvalidGuestID(taskInfo)).To(Equal(expected)) + }, + Entry( + "taskInfo is nil", + nil, + false, + ), + Entry( + "taskInfo.Error is nil", + &vimtypes.TaskInfo{}, + false, + ), + Entry( + "taskInfo.Error.Fault is nil", + &vimtypes.TaskInfo{ + Error: &vimtypes.LocalizedMethodFault{}, + }, + false, + ), + Entry( + "taskInfo.Error.Fault is not InvalidArgument", + &vimtypes.TaskInfo{ + Error: &vimtypes.LocalizedMethodFault{ + Fault: &vimtypes.AdminDisabled{}, + }, + }, + false, + ), + Entry( + "taskInfo.Error.Fault is InvalidArgument with wrong property", + &vimtypes.TaskInfo{ + Error: &vimtypes.LocalizedMethodFault{ + Fault: &vimtypes.InvalidArgument{ + InvalidProperty: "configSpec.name", }, }, - } - vmCtx = pkgctx.VirtualMachineContext{ - VM: &vmopVM, - } - configSpec = vimtypes.VirtualMachineConfigSpec{} - taskInfo = &vimtypes.TaskInfo{} - }) - - JustBeforeEach(func() { - vmutil.UpdateVMGuestIDReconfiguredCondition(vmCtx, configSpec, taskInfo) - }) - - Context("ConfigSpec doesn't have a guest ID", func() { - - BeforeEach(func() { - configSpec.GuestId = "" - }) - - It("should delete the existing VM's guest ID condition", func() { - Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) - }) - }) - - Context("ConfigSpec has a guest ID", func() { - - BeforeEach(func() { - configSpec.GuestId = "test-guest-id-value" - }) - - When("TaskInfo is nil", func() { - - BeforeEach(func() { - taskInfo = nil - }) - - It("should delete the VM's guest ID condition", func() { - Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) - }) - }) - - When("TaskInfo.Error is nil", func() { - - BeforeEach(func() { - taskInfo.Error = nil - }) - - It("should delete the VM's guest ID condition", func() { - Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) - }) - }) - - When("TaskInfo.Error.Fault is not an InvalidPropertyFault", func() { - - BeforeEach(func() { - taskInfo.Error = &vimtypes.LocalizedMethodFault{ - Fault: &vimtypes.InvalidName{ - Name: "some-invalid-name", - }, - } - }) - - It("should delete the VM's guest ID condition", func() { - Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) - }) - }) - - When("TaskInfo.Error contains an invalid property error NOT about guestID", func() { - - BeforeEach(func() { - taskInfo.Error = &vimtypes.LocalizedMethodFault{ - Fault: &vimtypes.InvalidArgument{ - InvalidProperty: "config.version", - }, - } - }) - - It("should delete the VM's guest ID condition", func() { - Expect(conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition)).To(BeNil()) - }) - }) - - When("TaskInfo.Error contains an invalid property error of guestID", func() { - - BeforeEach(func() { - taskInfo.Error = &vimtypes.LocalizedMethodFault{ - Fault: &vimtypes.InvalidArgument{ - InvalidProperty: "configSpec.guestId", - }, - } - }) - - It("should set the VM's guest ID condition to false with the invalid value in the reason", func() { - c := conditions.Get(&vmopVM, vmopv1.GuestIDReconfiguredCondition) - Expect(c).NotTo(BeNil()) - Expect(c.Status).To(Equal(metav1.ConditionFalse)) - Expect(c.Reason).To(Equal("Invalid")) - Expect(c.Message).To(Equal("The specified guest ID value is not supported: test-guest-id-value")) - }) - }) - }) - }) + }, + false, + ), + Entry( + "taskInfo.Error.Fault is InvalidArgument with guestID", + &vimtypes.TaskInfo{ + Error: &vimtypes.LocalizedMethodFault{ + Fault: &vimtypes.InvalidArgument{ + InvalidProperty: vmutil.GuestIDProperty, + }, + }, + }, + true, + ), + ) }