From c8e1590eea6464a6a4981b575eded73c7316be7f Mon Sep 17 00:00:00 2001 From: akutz Date: Tue, 29 Oct 2024 09:17:36 -0500 Subject: [PATCH] Support async create VM This patch introduces an async variant on the provider's `CreateOrUpdateVirtualMachine` function. This new function, `CreateOrUpdateVirtualMachineAsync`, returns `<-chan error, error`, for when a non-blocking create is used. Please note, support for async create is only enabled when the async signal feature is enabled. This is because the new async create workflow no longer: - Falls into a post-create reconfigure, instead allowing async signal to enqueue a new reconcile when the create has completed. - Requeues the VM after N time based on the VM's state, ex. when the VM is powered on and does not yet have an IP address. This also now relies on async signal when the feature is enabled. Finally, while a non-blocking create does mean the reconciler threads are no longer consumed by create operations, it does not mean VM Op will allow unbounded, concurrent creates. Because each non-blocking create operation consumes a goroutine, the number of concurrent create operations is still limited. The limit is the same as the number of threads previously allowed to do create operations. The difference is, with async create disabled, if 16 threads are doing creates, that is 16 threads that cannot do anything else. With async create enabled, if 16 goroutines are doing create, there are still 16 reconciler threads available to do other things. --- .../virtualmachine_controller.go | 49 ++- .../virtualmachine_controller_intg_test.go | 120 +++--- .../virtualmachine_controller_unit_test.go | 125 ++++-- ...lmachinereplicaset_controller_intg_test.go | 18 +- pkg/providers/fake/fake_vm_provider.go | 50 ++- pkg/providers/vm_provider_interface.go | 1 + pkg/providers/vsphere/vmprovider_vm.go | 367 ++++++++++++------ pkg/providers/vsphere/vmprovider_vm2_test.go | 17 +- .../vsphere/vmprovider_vm_resize_test.go | 72 ++-- pkg/providers/vsphere/vmprovider_vm_test.go | 314 ++++++++------- pkg/providers/vsphere/vsphere_suite_test.go | 89 +++++ 11 files changed, 798 insertions(+), 424 deletions(-) diff --git a/controllers/virtualmachine/virtualmachine/virtualmachine_controller.go b/controllers/virtualmachine/virtualmachine/virtualmachine_controller.go index 80965b00e..91153fc2e 100644 --- a/controllers/virtualmachine/virtualmachine/virtualmachine_controller.go +++ b/controllers/virtualmachine/virtualmachine/virtualmachine_controller.go @@ -321,6 +321,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } + // Do not requeue if async signal is enabled. + if pkgcfg.FromContext(ctx).Features.WorkloadDomainIsolation && + !pkgcfg.FromContext(ctx).AsyncSignalDisabled { + + return ctrl.Result{}, nil + } + + // Requeue after N amount of time according to the state of the VM. return ctrl.Result{RequeueAfter: requeueDelay(vmCtx)}, nil } @@ -338,7 +346,7 @@ func requeueDelay(ctx *pkgctx.VirtualMachineContext) time.Duration { return pkgcfg.FromContext(ctx).CreateVMRequeueDelay } - if ctx.VM.Status.PowerState == vmopv1.VirtualMachinePowerStateOn { + if ctx.VM.Status.PowerState == "" || ctx.VM.Status.PowerState == vmopv1.VirtualMachinePowerStateOn { networkSpec := ctx.VM.Spec.Network if networkSpec != nil && !networkSpec.Disabled { networkStatus := ctx.VM.Status.Network @@ -439,11 +447,46 @@ func (r *Reconciler) ReconcileNormal(ctx *pkgctx.VirtualMachineContext) (reterr // Upgrade schema fields where needed upgradeSchema(ctx) - err := r.VMProvider.CreateOrUpdateVirtualMachine(ctx, ctx.VM) + var ( + err error + chanErr <-chan error + ) + + if pkgcfg.FromContext(ctx).Features.WorkloadDomainIsolation && + !pkgcfg.FromContext(ctx).AsyncSignalDisabled { + // + // Non-blocking create + // + chanErr, err = r.VMProvider.CreateOrUpdateVirtualMachineAsync(ctx, ctx.VM) + } else { + // + // Blocking create + // + err = r.VMProvider.CreateOrUpdateVirtualMachine(ctx, ctx.VM) + } switch { case ctxop.IsCreate(ctx): - r.Recorder.EmitEvent(ctx.VM, "Create", err, false) + if chanErr == nil { + // + // Blocking create + // + r.Recorder.EmitEvent(ctx.VM, "Create", err, false) + } else { + // + // Non-blocking create + // + if err != nil { + // Failed before goroutine. + r.Recorder.EmitEvent(ctx.VM, "Create", err, false) + } else { + // Emit event once goroutine is complete. + go func(obj client.Object) { + err := <-chanErr + r.Recorder.EmitEvent(obj, "Create", err, false) + }(ctx.VM.DeepCopy()) + } + } case ctxop.IsUpdate(ctx): r.Recorder.EmitEvent(ctx.VM, "Update", err, false) case err != nil: diff --git a/controllers/virtualmachine/virtualmachine/virtualmachine_controller_intg_test.go b/controllers/virtualmachine/virtualmachine/virtualmachine_controller_intg_test.go index 36a606843..38c7a2063 100644 --- a/controllers/virtualmachine/virtualmachine/virtualmachine_controller_intg_test.go +++ b/controllers/virtualmachine/virtualmachine/virtualmachine_controller_intg_test.go @@ -105,11 +105,13 @@ func intgTestsReconcile() { dummyInstanceUUID := uuid.NewString() BeforeEach(func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + return nil + }, + ) }) AfterEach(func() { @@ -136,13 +138,15 @@ func intgTestsReconcile() { }) By("Set InstanceUUID in CreateOrUpdateVM", func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - // Just using InstanceUUID here for a field to update. - vm.Status.InstanceUUID = dummyInstanceUUID - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + // Just using InstanceUUID here for a field to update. + vm.Status.InstanceUUID = dummyInstanceUUID + return nil + }, + ) }) By("VirtualMachine should have InstanceUUID set", func() { @@ -155,13 +159,14 @@ func intgTestsReconcile() { }) By("Set Created condition in CreateOrUpdateVM", func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - vm.Status.PowerState = vmopv1.VirtualMachinePowerStateOn - conditions.MarkTrue(vm, vmopv1.VirtualMachineConditionCreated) - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + conditions.MarkTrue(vm, vmopv1.VirtualMachineConditionCreated) + return nil + }, + ) }) By("VirtualMachine should have Created condition set", func() { @@ -174,14 +179,16 @@ func intgTestsReconcile() { }) By("Set IP address in CreateOrUpdateVM", func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - vm.Status.Network = &vmopv1.VirtualMachineNetworkStatus{ - PrimaryIP4: dummyIPAddress, - } - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + vm.Status.Network = &vmopv1.VirtualMachineNetworkStatus{ + PrimaryIP4: dummyIPAddress, + } + return nil + }, + ) }) By("VirtualMachine should have IP address set", func() { @@ -213,13 +220,15 @@ func intgTestsReconcile() { biosUUID := uuid.NewString() BeforeEach(func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - vm.Status.InstanceUUID = instanceUUID - vm.Status.BiosUUID = biosUUID - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + vm.Status.InstanceUUID = instanceUUID + vm.Status.BiosUUID = biosUUID + return nil + }, + ) }) // NOTE: mutating webhook sets the default spec.instanceUUID, but is not run in this test - @@ -261,13 +270,15 @@ func intgTestsReconcile() { }) It("Reconciles after VirtualMachineClass change", func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - // Set this so requeueDelay() returns 0. - conditions.MarkTrue(vm, vmopv1.VirtualMachineConditionCreated) - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + // Set this so requeueDelay() returns 0. + conditions.MarkTrue(vm, vmopv1.VirtualMachineConditionCreated) + return nil + }, + ) Expect(ctx.Client.Create(ctx, vm)).To(Succeed()) // Wait for initial reconcile. @@ -283,12 +294,14 @@ func intgTestsReconcile() { instanceUUID := uuid.NewString() - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - vm.Status.InstanceUUID = instanceUUID - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + vm.Status.InstanceUUID = instanceUUID + return nil + }, + ) vmClass := builder.DummyVirtualMachineClass(vm.Spec.ClassName) vmClass.Namespace = vm.Namespace @@ -390,6 +403,7 @@ var _ = Describe( ctx = pkgcfg.UpdateContext( ctx, func(config *pkgcfg.Config) { + config.AsyncSignalDisabled = false config.Features.WorkloadDomainIsolation = true }, ) @@ -400,10 +414,14 @@ var _ = Describe( provider.VSphereClientFn = func(ctx context.Context) (*vsclient.Client, error) { return vsclient.NewClient(ctx, vcSimCtx.VCClientConfig) } - provider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, obj *vmopv1.VirtualMachine) error { - atomic.AddInt32(&numCreateOrUpdateCalls, 1) - return nil - } + providerfake.SetCreateOrUpdateFunction( + ctx, + provider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + atomic.AddInt32(&numCreateOrUpdateCalls, 1) + return nil + }, + ) vcSimCtx = builder.NewIntegrationTestContextForVCSim( ctx, diff --git a/controllers/virtualmachine/virtualmachine/virtualmachine_controller_unit_test.go b/controllers/virtualmachine/virtualmachine/virtualmachine_controller_unit_test.go index 4de219cfc..6bbb4dbe6 100644 --- a/controllers/virtualmachine/virtualmachine/virtualmachine_controller_unit_test.go +++ b/controllers/virtualmachine/virtualmachine/virtualmachine_controller_unit_test.go @@ -6,7 +6,6 @@ package virtualmachine_test import ( "context" "errors" - "fmt" "strings" . "github.com/onsi/ginkgo/v2" @@ -77,6 +76,8 @@ func unitTestsReconcile() { ctx = suite.NewUnitTestContextForController(initObjects...) pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { config.MaxDeployThreadsOnProvider = 16 + config.Features.WorkloadDomainIsolation = false + config.AsyncSignalDisabled = true }) fakeProbeManagerIf := proberfake.NewFakeProberManager() @@ -160,9 +161,13 @@ func unitTestsReconcile() { Context("ProberManager", func() { It("Should call add to Prober Manager if ReconcileNormal fails", func() { - fakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - return errors.New(providerError) - } + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + return errors.New(providerError) + }, + ) err := reconciler.ReconcileNormal(vmCtx) Expect(err).To(HaveOccurred()) @@ -179,50 +184,112 @@ func unitTestsReconcile() { }) }) - It("Should emit a CreateSuccess event if ReconcileNormal causes a successful VM creation", func() { - fakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - ctxop.MarkCreate(ctx) - return nil - } + When("blocking create", func() { + JustBeforeEach(func() { + pkgcfg.SetContext(vmCtx, func(config *pkgcfg.Config) { + config.Features.WorkloadDomainIsolation = true + config.AsyncSignalDisabled = true + }) + }) + It("Should emit a CreateSuccess event if ReconcileNormal causes a successful VM creation", func() { + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + ctxop.MarkCreate(ctx) + return nil + }, + ) + Expect(reconciler.ReconcileNormal(vmCtx)).Should(Succeed()) + expectEvents(ctx, "CreateSuccess") + }) - Expect(reconciler.ReconcileNormal(vmCtx)).Should(Succeed()) - expectEvents(ctx, "CreateSuccess") + It("Should emit CreateFailure event if ReconcileNormal causes a failed VM create", func() { + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + ctxop.MarkCreate(ctx) + return errors.New("fake") + }, + ) + + Expect(reconciler.ReconcileNormal(vmCtx)).ShouldNot(Succeed()) + expectEvents(ctx, "CreateFailure") + }) }) - It("Should emit CreateFailure event if ReconcileNormal causes a failed VM create", func() { - fakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - ctxop.MarkCreate(ctx) - return fmt.Errorf("fake") - } + When("non-blocking create", func() { + JustBeforeEach(func() { + pkgcfg.SetContext(vmCtx, func(config *pkgcfg.Config) { + config.Features.WorkloadDomainIsolation = true + config.AsyncSignalDisabled = false + }) + }) + It("Should emit a CreateSuccess event if ReconcileNormal causes a successful VM creation", func() { + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + ctxop.MarkCreate(ctx) + return nil + }, + ) + Expect(reconciler.ReconcileNormal(vmCtx)).Should(Succeed()) + expectEvents(ctx, "CreateSuccess") + }) - Expect(reconciler.ReconcileNormal(vmCtx)).ShouldNot(Succeed()) - expectEvents(ctx, "CreateFailure") + It("Should emit CreateFailure event if ReconcileNormal causes a failed VM create", func() { + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + ctxop.MarkCreate(ctx) + return errors.New("fake") + }, + ) + + Expect(reconciler.ReconcileNormal(vmCtx)).ShouldNot(Succeed()) + expectEvents(ctx, "CreateFailure") + }) }) It("Should emit UpdateSuccess event if ReconcileNormal causes a successful VM update", func() { - fakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - ctxop.MarkUpdate(ctx) - return nil - } + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + ctxop.MarkUpdate(ctx) + return nil + }, + ) Expect(reconciler.ReconcileNormal(vmCtx)).Should(Succeed()) expectEvents(ctx, "UpdateSuccess") }) It("Should emit UpdateFailure event if ReconcileNormal causes a failed VM update", func() { - fakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - ctxop.MarkUpdate(ctx) - return fmt.Errorf("fake") - } + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + ctxop.MarkUpdate(ctx) + return errors.New("fake") + }, + ) Expect(reconciler.ReconcileNormal(vmCtx)).ShouldNot(Succeed()) expectEvents(ctx, "UpdateFailure") }) It("Should emit ReconcileNormalFailure if ReconcileNormal fails for neither create or update op", func() { - fakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - return fmt.Errorf("fake") - } + providerfake.SetCreateOrUpdateFunction( + vmCtx, + fakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + return errors.New("fake") + }, + ) Expect(reconciler.ReconcileNormal(vmCtx)).ShouldNot(Succeed()) expectEvents(ctx, "ReconcileNormalFailure") diff --git a/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller_intg_test.go b/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller_intg_test.go index b7a032f39..be8bc6eaa 100644 --- a/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller_intg_test.go +++ b/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller_intg_test.go @@ -19,7 +19,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/constants/testlabels" - + providerfake "github.com/vmware-tanzu/vm-operator/pkg/providers/fake" "github.com/vmware-tanzu/vm-operator/test/builder" ) @@ -124,13 +124,15 @@ func intgTestsReconcile() { dummyInstanceUUID := "instanceUUID1234" BeforeEach(func() { - intgFakeVMProvider.Lock() - intgFakeVMProvider.CreateOrUpdateVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error { - // Used below just to check for something in the Status is updated. - vm.Status.InstanceUUID = dummyInstanceUUID - return nil - } - intgFakeVMProvider.Unlock() + providerfake.SetCreateOrUpdateFunction( + ctx, + intgFakeVMProvider, + func(ctx context.Context, vm *vmopv1.VirtualMachine) error { + // Used below just to check for something in the Status is + // updated. + vm.Status.InstanceUUID = dummyInstanceUUID + return nil + }) }) AfterEach(func() { diff --git a/pkg/providers/fake/fake_vm_provider.go b/pkg/providers/fake/fake_vm_provider.go index b30fcd6cc..167f4bb5c 100644 --- a/pkg/providers/fake/fake_vm_provider.go +++ b/pkg/providers/fake/fake_vm_provider.go @@ -16,6 +16,7 @@ import ( imgregv1a1 "github.com/vmware-tanzu/image-registry-operator-api/api/v1alpha1" vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" + pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config" "github.com/vmware-tanzu/vm-operator/pkg/providers" vsclient "github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/client" ) @@ -28,9 +29,10 @@ import ( // expected to evolve as more tests get added in the future. type funcs struct { - CreateOrUpdateVirtualMachineFn func(ctx context.Context, vm *vmopv1.VirtualMachine) error - DeleteVirtualMachineFn func(ctx context.Context, vm *vmopv1.VirtualMachine) error - PublishVirtualMachineFn func(ctx context.Context, vm *vmopv1.VirtualMachine, + CreateOrUpdateVirtualMachineFn func(ctx context.Context, vm *vmopv1.VirtualMachine) error + CreateOrUpdateVirtualMachineAsyncFn func(ctx context.Context, vm *vmopv1.VirtualMachine) (<-chan error, error) + DeleteVirtualMachineFn func(ctx context.Context, vm *vmopv1.VirtualMachine) error + PublishVirtualMachineFn func(ctx context.Context, vm *vmopv1.VirtualMachine, vmPub *vmopv1.VirtualMachinePublishRequest, cl *imgregv1a1.ContentLibrary, actID string) (string, error) GetVirtualMachineGuestHeartbeatFn func(ctx context.Context, vm *vmopv1.VirtualMachine) (vmopv1.GuestHeartbeatStatus, error) GetVirtualMachinePropertiesFn func(ctx context.Context, vm *vmopv1.VirtualMachine, propertyPaths []string) (map[string]any, error) @@ -92,6 +94,16 @@ func (s *VMProvider) CreateOrUpdateVirtualMachine(ctx context.Context, vm *vmopv return nil } +func (s *VMProvider) CreateOrUpdateVirtualMachineAsync(ctx context.Context, vm *vmopv1.VirtualMachine) (<-chan error, error) { + s.Lock() + defer s.Unlock() + if s.CreateOrUpdateVirtualMachineAsyncFn != nil { + return s.CreateOrUpdateVirtualMachineAsyncFn(ctx, vm) + } + s.addToVMMap(vm) + return nil, nil +} + func (s *VMProvider) DeleteVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine) error { s.Lock() defer s.Unlock() @@ -394,3 +406,35 @@ func NewVMProvider() *VMProvider { } return &provider } + +func SetCreateOrUpdateFunction( + ctx context.Context, + provider *VMProvider, + fn func( + ctx context.Context, + vm *vmopv1.VirtualMachine, + ) error, +) { + provider.Lock() + defer provider.Unlock() + + if pkgcfg.FromContext(ctx).Features.WorkloadDomainIsolation { + if !pkgcfg.FromContext(ctx).AsyncSignalDisabled { + + provider.CreateOrUpdateVirtualMachineAsyncFn = func( + ctx context.Context, + vm *vmopv1.VirtualMachine) (<-chan error, error) { + + chanErr := make(chan error) + close(chanErr) + + return chanErr, fn(ctx, vm) + } + + return + } + + } + + provider.CreateOrUpdateVirtualMachineFn = fn +} diff --git a/pkg/providers/vm_provider_interface.go b/pkg/providers/vm_provider_interface.go index 0e2a9df09..12d5a96e2 100644 --- a/pkg/providers/vm_provider_interface.go +++ b/pkg/providers/vm_provider_interface.go @@ -19,6 +19,7 @@ import ( // VirtualMachineProviderInterface is a pluggable interface for VM Providers. type VirtualMachineProviderInterface interface { CreateOrUpdateVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine) error + CreateOrUpdateVirtualMachineAsync(ctx context.Context, vm *vmopv1.VirtualMachine) (<-chan error, error) DeleteVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine) error PublishVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine, vmPub *vmopv1.VirtualMachinePublishRequest, cl *imgregv1a1.ContentLibrary, actID string) (string, error) diff --git a/pkg/providers/vsphere/vmprovider_vm.go b/pkg/providers/vsphere/vmprovider_vm.go index a6f0ef6d0..8deca1b7b 100644 --- a/pkg/providers/vsphere/vmprovider_vm.go +++ b/pkg/providers/vsphere/vmprovider_vm.go @@ -22,6 +22,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" apierrorsutil "k8s.io/apimachinery/pkg/util/errors" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" imgregv1a1 "github.com/vmware-tanzu/image-registry-operator-api/api/v1alpha1" @@ -30,6 +31,7 @@ import ( "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" + ctxop "github.com/vmware-tanzu/vm-operator/pkg/context/operation" vcclient "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/client" "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/clustermodules" "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/constants" @@ -75,7 +77,9 @@ type vmResizeArgs = session.VMResizeArgs var ( createCountLock sync.Mutex concurrentCreateCount int +) +var ( // SkipVMImageCLProviderCheck skips the checks that a VM Image has a Content Library item provider // since a VirtualMachineImage created for a VM template won't have either. This has been broken for // a long time but was otherwise masked on how the tests used to be organized. @@ -86,10 +90,31 @@ func (vs *vSphereVMProvider) CreateOrUpdateVirtualMachine( ctx context.Context, vm *vmopv1.VirtualMachine) error { + return vs.createOrUpdateVirtualMachine(ctx, vm, nil) +} + +func (vs *vSphereVMProvider) CreateOrUpdateVirtualMachineAsync( + ctx context.Context, + vm *vmopv1.VirtualMachine) (<-chan error, error) { + + chanErr := make(chan error) + err := vs.createOrUpdateVirtualMachine(ctx, vm, chanErr) + return chanErr, err +} + +func (vs *vSphereVMProvider) createOrUpdateVirtualMachine( + ctx context.Context, + vm *vmopv1.VirtualMachine, + chanErr chan error) error { + vmCtx := pkgctx.VirtualMachineContext{ - Context: context.WithValue(ctx, vimtypes.ID{}, vs.getOpID(vm, "createOrUpdateVM")), - Logger: log.WithValues("vmName", vm.NamespacedName()), - VM: vm, + Context: context.WithValue( + ctx, + vimtypes.ID{}, + vs.getOpID(vm, "createOrUpdateVM"), + ), + Logger: log.WithValues("vmName", vm.NamespacedName()), + VM: vm, } client, err := vs.getVcClient(vmCtx) @@ -98,36 +123,91 @@ func (vs *vSphereVMProvider) CreateOrUpdateVirtualMachine( } // Set the VC UUID annotation on the VM before attempting creation or - // update. Among other things, the annotation facilitates differential handling - // of restore and fail-over operations. + // update. Among other things, the annotation facilitates differential + // handling of restore and fail-over operations. if vm.Annotations == nil { vm.Annotations = make(map[string]string) } - vm.Annotations[vmopv1.ManagerID] = client.VimClient().ServiceContent.About.InstanceUuid + vCenterInstanceUUID := client.VimClient().ServiceContent.About.InstanceUuid + vm.Annotations[vmopv1.ManagerID] = vCenterInstanceUUID - vcVM, err := vs.getVM(vmCtx, client, false) + // Check to see if the VM can be found on the underlying platform. + foundVM, err := vs.getVM(vmCtx, client, false) if err != nil { return err } - if vcVM == nil { - var createArgs *VMCreateArgs + if foundVM == nil { + // Record this as a create operation. + ctxop.MarkCreate(vmCtx) + + // Do not allow more than N create threads/goroutines. + // + // - In blocking create mode, this ensures there are reconciler threads + // available to reconcile non-create operations. + // + // - In non-blocking create mode, this ensures the number of goroutines + // spawned to create VMs does not take up too much memory. + allowed, createDeferFn := vs.vmCreateConcurrentAllowed(vmCtx) + if !allowed { + return nil + } + defer createDeferFn() - vcVM, createArgs, err = vs.createVirtualMachine(vmCtx, client) + createArgs, err := vs.getCreateArgs(vmCtx, client) if err != nil { return err } - if vcVM == nil { - // Creation was not ready or blocked for some reason. We depend on the controller - // to eventually retry the create. - return nil + if chanErr == nil { + + // + // Do a blocking create + // + + newVM, err := vs.createVirtualMachine(vmCtx, client, createArgs) + if err != nil { + return err + } + + // Fall-through to an update post-reconfigure. + return vs.createdVirtualMachineFallthroughUpdate( + vmCtx, + newVM, + client, + createArgs) } - return vs.createdVirtualMachineFallthroughUpdate(vmCtx, vcVM, client, createArgs) + // + // Do a non-blocking create + // + + // Create a copy of the context and replace its VM with a copy to + // ensure modifications in the goroutine below are not impacted or + // impact the operations above us in the call stack. + copyOfCtx := vmCtx + copyOfCtx.VM = vmCtx.VM.DeepCopy() + + // Start a goroutine to create the VM in the background. + go vs.createVirtualMachineAsync( + copyOfCtx, + client, + createArgs, + chanErr) + + // Return with no error. The VM will be re-enqueued once the create + // completes with success or failure. + return nil + } + + if chanErr != nil { + defer close(chanErr) } - return vs.updateVirtualMachine(vmCtx, vcVM, client, nil) + // Record this as an update operation. + ctxop.MarkUpdate(vmCtx) + + return vs.updateVirtualMachine(vmCtx, foundVM, client, nil) } func (vs *vSphereVMProvider) DeleteVirtualMachine( @@ -388,78 +468,148 @@ func (vs *vSphereVMProvider) vmCreatePathName( return nil } -func (vs *vSphereVMProvider) createVirtualMachine( +func (vs *vSphereVMProvider) getCreateArgs( vmCtx pkgctx.VirtualMachineContext, - vcClient *vcclient.Client) (*object.VirtualMachine, *VMCreateArgs, error) { + vcClient *vcclient.Client) (*VMCreateArgs, error) { createArgs, err := vs.vmCreateGetArgs(vmCtx, vcClient) if err != nil { - return nil, nil, err + return nil, err } - err = vs.vmCreateDoPlacement(vmCtx, vcClient, createArgs) - if err != nil { - return nil, nil, err + if err := vs.vmCreateDoPlacement(vmCtx, vcClient, createArgs); err != nil { + return nil, err } - err = vs.vmCreateGetFolderAndRPMoIDs(vmCtx, vcClient, createArgs) - if err != nil { - return nil, nil, err + if err := vs.vmCreateGetFolderAndRPMoIDs(vmCtx, vcClient, createArgs); err != nil { + return nil, err } - err = vs.vmCreatePathName(vmCtx, vcClient, createArgs) - if err != nil { - return nil, nil, err + if err := vs.vmCreatePathName(vmCtx, vcClient, createArgs); err != nil { + return nil, err } - err = vs.vmCreateFixupConfigSpec(vmCtx, vcClient, createArgs) - if err != nil { - return nil, nil, err + if err := vs.vmCreateFixupConfigSpec(vmCtx, vcClient, createArgs); err != nil { + return nil, err } - err = vs.vmCreateIsReady(vmCtx, vcClient, createArgs) - if err != nil { - return nil, nil, err + if err := vs.vmCreateIsReady(vmCtx, vcClient, createArgs); err != nil { + return nil, err } - // BMV: This is about where we used to do this check but it prb make more sense to do - // earlier, as to limit wasted work. Before DoPlacement() is likely the best place so - // the window between the placement decision and creating the VM on VC is small(ish). - allowed, createDeferFn := vs.vmCreateConcurrentAllowed(vmCtx) - if !allowed { - return nil, nil, nil + return createArgs, nil +} + +func (vs *vSphereVMProvider) createdVirtualMachineFallthroughUpdate( + vmCtx pkgctx.VirtualMachineContext, + vcVM *object.VirtualMachine, + vcClient *vcclient.Client, + createArgs *VMCreateArgs) error { + + // TODO: In the common case, we'll call directly into update right after + // create succeeds, and can use the createArgs to avoid doing a bunch of + // lookup work again. + + return vs.updateVirtualMachine(vmCtx, vcVM, vcClient, createArgs) +} + +func (vs *vSphereVMProvider) vmCreateConcurrentAllowed( + vmCtx pkgctx.VirtualMachineContext) (bool, func()) { + maxDeployThreads := pkgcfg.FromContext(vmCtx).GetMaxDeployThreadsOnProvider() + + createCountLock.Lock() + if concurrentCreateCount >= maxDeployThreads { + createCountLock.Unlock() + vmCtx.Logger.Info("Too many create VirtualMachine already occurring. Re-queueing request") + return false, nil } - defer createDeferFn() + + concurrentCreateCount++ + createCountLock.Unlock() + + decrementFn := func() { + createCountLock.Lock() + concurrentCreateCount-- + createCountLock.Unlock() + } + + return true, decrementFn +} + +func (vs *vSphereVMProvider) createVirtualMachine( + ctx pkgctx.VirtualMachineContext, + vcClient *vcclient.Client, + args *VMCreateArgs) (*object.VirtualMachine, error) { moRef, err := vmlifecycle.CreateVirtualMachine( - vmCtx, + ctx, vcClient.RestClient(), vcClient.VimClient(), vcClient.Finder(), - &createArgs.CreateArgs) + &args.CreateArgs) if err != nil { - vmCtx.Logger.Error(err, "CreateVirtualMachine failed") - conditions.MarkFalse(vmCtx.VM, vmopv1.VirtualMachineConditionCreated, "Error", err.Error()) - return nil, nil, err + ctx.Logger.Error(err, "CreateVirtualMachine failed") + conditions.MarkFalse( + ctx.VM, + vmopv1.VirtualMachineConditionCreated, + "Error", + err.Error()) + return nil, err } - vmCtx.VM.Status.UniqueID = moRef.Reference().Value - conditions.MarkTrue(vmCtx.VM, vmopv1.VirtualMachineConditionCreated) + ctx.VM.Status.UniqueID = moRef.Reference().Value + conditions.MarkTrue(ctx.VM, vmopv1.VirtualMachineConditionCreated) - return object.NewVirtualMachine(vcClient.VimClient(), *moRef), createArgs, nil + return object.NewVirtualMachine(vcClient.VimClient(), *moRef), nil } -func (vs *vSphereVMProvider) createdVirtualMachineFallthroughUpdate( - vmCtx pkgctx.VirtualMachineContext, - vcVM *object.VirtualMachine, +func (vs *vSphereVMProvider) createVirtualMachineAsync( + ctx pkgctx.VirtualMachineContext, vcClient *vcclient.Client, - createArgs *VMCreateArgs) error { + args *VMCreateArgs, + chanErr chan error) { - // TODO: In the common case, we'll call directly into update right after create succeeds, and - // can use the createArgs to avoid doing a bunch of lookup work again. + defer close(chanErr) - return vs.updateVirtualMachine(vmCtx, vcVM, vcClient, createArgs) + moRef, vimErr := vmlifecycle.CreateVirtualMachine( + ctx, + vcClient.RestClient(), + vcClient.VimClient(), + vcClient.Finder(), + &args.CreateArgs) + + if vimErr != nil { + chanErr <- vimErr + ctx.Logger.Error(vimErr, "CreateVirtualMachine failed") + } + + _, k8sErr := controllerutil.CreateOrPatch( + ctx, + vs.k8sClient, + ctx.VM, + func() error { + + if vimErr != nil { + conditions.MarkFalse( + ctx.VM, + vmopv1.VirtualMachineConditionCreated, + "Error", + vimErr.Error()) + return nil //nolint:nilerr + } + + ctx.VM.Status.UniqueID = moRef.Reference().Value + conditions.MarkTrue(ctx.VM, vmopv1.VirtualMachineConditionCreated) + + return nil + }, + ) + + if k8sErr != nil { + chanErr <- k8sErr + ctx.Logger.Error(k8sErr, "Failed to patch VM status after create") + } } // VMUpdatePropertiesSelector is the set of VM properties fetched at the start @@ -484,53 +634,58 @@ func (vs *vSphereVMProvider) updateVirtualMachine( vmCtx.Logger.V(4).Info("Updating VirtualMachine") - { - // Hack - create just enough of the Session that's needed for update + // TODO(akutz) This is a hack to create just enough of the Session needed + // for the update. - if err := vcVM.Properties( - vmCtx, - vcVM.Reference(), - VMUpdatePropertiesSelector, - &vmCtx.MoVM); err != nil { + if err := vcVM.Properties( + vmCtx, + vcVM.Reference(), + VMUpdatePropertiesSelector, + &vmCtx.MoVM); err != nil { - return err - } + return err + } - if vmCtx.MoVM.ResourcePool == nil { - // Same error as govmomi VirtualMachine::ResourcePool(). - return fmt.Errorf("VM doesn't have a resourcePool") - } + // Check to see if the VM has any outstanding tasks associated with it. If + // there are, then we should return early from this reconciler thread. This + // is because active tasks prevent us from making changes, and the task's + // completion will enqueue a reconcile for the VM, sending it back into the + // reconcile workflow. - clusterMoRef, err := vcenter.GetResourcePoolOwnerMoRef( - vmCtx, - vcVM.Client(), - vmCtx.MoVM.ResourcePool.Value) - if err != nil { - return err - } + if vmCtx.MoVM.ResourcePool == nil { + // Same error as govmomi VirtualMachine::ResourcePool(). + return fmt.Errorf("VM doesn't have a resourcePool") + } - ses := &session.Session{ - K8sClient: vs.k8sClient, - Client: vcClient.Client, - Finder: vcClient.Finder(), - ClusterMoRef: clusterMoRef, - } + clusterMoRef, err := vcenter.GetResourcePoolOwnerMoRef( + vmCtx, + vcVM.Client(), + vmCtx.MoVM.ResourcePool.Value) + if err != nil { + return err + } - getUpdateArgsFn := func() (*vmUpdateArgs, error) { - // TODO: Use createArgs if we already got them, except for: - // - createArgs.ConfigSpec.Crypto - _ = createArgs - return vs.vmUpdateGetArgs(vmCtx) - } + ses := &session.Session{ + K8sClient: vs.k8sClient, + Client: vcClient.Client, + Finder: vcClient.Finder(), + ClusterMoRef: clusterMoRef, + } - getResizeArgsFn := func() (*vmResizeArgs, error) { - return vs.vmResizeGetArgs(vmCtx) - } + getUpdateArgsFn := func() (*vmUpdateArgs, error) { + // TODO: Use createArgs if we already got them, except for: + // - createArgs.ConfigSpec.Crypto + _ = createArgs + return vs.vmUpdateGetArgs(vmCtx) + } - err = ses.UpdateVirtualMachine(vmCtx, vcVM, getUpdateArgsFn, getResizeArgsFn) - if err != nil { - return err - } + getResizeArgsFn := func() (*vmResizeArgs, error) { + return vs.vmResizeGetArgs(vmCtx) + } + + err = ses.UpdateVirtualMachine(vmCtx, vcVM, getUpdateArgsFn, getResizeArgsFn) + if err != nil { + return err } // Back up the VM at the end after a successful update. TKG nodes are skipped @@ -771,28 +926,6 @@ func (vs *vSphereVMProvider) vmCreateIsReady( return nil } -func (vs *vSphereVMProvider) vmCreateConcurrentAllowed(vmCtx pkgctx.VirtualMachineContext) (bool, func()) { - maxDeployThreads := pkgcfg.FromContext(vmCtx).GetMaxDeployThreadsOnProvider() - - createCountLock.Lock() - if concurrentCreateCount >= maxDeployThreads { - createCountLock.Unlock() - vmCtx.Logger.Info("Too many create VirtualMachine already occurring. Re-queueing request") - return false, nil - } - - concurrentCreateCount++ - createCountLock.Unlock() - - decrementFn := func() { - createCountLock.Lock() - concurrentCreateCount-- - createCountLock.Unlock() - } - - return true, decrementFn -} - func (vs *vSphereVMProvider) vmCreateGetArgs( vmCtx pkgctx.VirtualMachineContext, vcClient *vcclient.Client) (*VMCreateArgs, error) { diff --git a/pkg/providers/vsphere/vmprovider_vm2_test.go b/pkg/providers/vsphere/vmprovider_vm2_test.go index e081a8837..0f786675f 100644 --- a/pkg/providers/vsphere/vmprovider_vm2_test.go +++ b/pkg/providers/vsphere/vmprovider_vm2_test.go @@ -70,6 +70,7 @@ func vmE2ETests() { JustBeforeEach(func() { ctx = suite.NewTestContextForVCSim(testConfig, initObjects...) pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.AsyncSignalDisabled = true config.MaxDeployThreadsOnProvider = 1 }) vmProvider = vsphere.NewVSphereVMProviderFromClient(ctx, ctx.Client, ctx.Recorder) @@ -137,10 +138,10 @@ func vmE2ETests() { }) It("DoIt without an NPE", func() { - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) - err = vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err = createOrUpdateVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(vm.Status.UniqueID).ToNot(BeEmpty()) @@ -188,7 +189,7 @@ func vmE2ETests() { }) It("DoIt", func() { - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("network interface is not ready yet")) Expect(conditions.IsFalse(vm, vmopv1.VirtualMachineConditionNetworkReady)).To(BeTrue()) @@ -222,7 +223,7 @@ func vmE2ETests() { Expect(ctx.Client.Status().Update(ctx, netInterface)).To(Succeed()) }) - err = vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err = createOrUpdateVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("has expected conditions", func() { @@ -296,7 +297,7 @@ func vmE2ETests() { }) It("DoIt", func() { - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("network interface is not ready yet")) Expect(conditions.IsFalse(vm, vmopv1.VirtualMachineConditionNetworkReady)).To(BeTrue()) @@ -331,7 +332,7 @@ func vmE2ETests() { Expect(ctx.Client.Status().Update(ctx, netInterface)).To(Succeed()) }) - err = vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err = createOrUpdateVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("has expected conditions", func() { @@ -407,7 +408,7 @@ func vmE2ETests() { }) It("DoIt", func() { - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("subnetPort is not ready yet")) Expect(conditions.IsFalse(vm, vmopv1.VirtualMachineConditionNetworkReady)).To(BeTrue()) @@ -439,7 +440,7 @@ func vmE2ETests() { Expect(ctx.Client.Status().Update(ctx, subnetPort)).To(Succeed()) }) - err = vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err = createOrUpdateVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("has expected conditions", func() { diff --git a/pkg/providers/vsphere/vmprovider_vm_resize_test.go b/pkg/providers/vsphere/vmprovider_vm_resize_test.go index ffeed3595..01e2e678f 100644 --- a/pkg/providers/vsphere/vmprovider_vm_resize_test.go +++ b/pkg/providers/vsphere/vmprovider_vm_resize_test.go @@ -10,7 +10,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/vmware/govmomi/object" "github.com/vmware/govmomi/vim25/mo" vimtypes "github.com/vmware/govmomi/vim25/types" "k8s.io/apimachinery/pkg/api/resource" @@ -47,6 +46,7 @@ func vmResizeTests() { JustBeforeEach(func() { ctx = suite.NewTestContextForVCSim(testConfig, initObjects...) pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.AsyncSignalDisabled = true config.MaxDeployThreadsOnProvider = 1 }) vmProvider = vsphere.NewVSphereVMProviderFromClient(ctx, ctx.Client, ctx.Recorder) @@ -68,21 +68,6 @@ func vmResizeTests() { return w.Bytes() } - createOrUpdateAndGetVcVM := func( - ctx *builder.TestContextForVCSim, - vm *vmopv1.VirtualMachine) (*object.VirtualMachine, error) { - - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) - if err != nil { - return nil, err - } - - ExpectWithOffset(1, vm.Status.UniqueID).ToNot(BeEmpty()) - vcVM := ctx.GetVMFromMoID(vm.Status.UniqueID) - ExpectWithOffset(1, vcVM).ToNot(BeNil()) - return vcVM, nil - } - createVMClass := func(cs vimtypes.VirtualMachineConfigSpec, name ...string) *vmopv1.VirtualMachineClass { var class *vmopv1.VirtualMachineClass if len(name) == 1 { @@ -197,8 +182,7 @@ func vmResizeTests() { vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff vm.Spec.StorageClass = ctx.StorageClassName - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) }) Context("Powered off VM", func() { @@ -216,7 +200,7 @@ func vmResizeTests() { newVMClass := createVMClass(newCS) vm.Spec.ClassName = newVMClass.Name - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -237,7 +221,7 @@ func vmResizeTests() { newVMClass := createVMClass(newCS) vm.Spec.ClassName = newVMClass.Name - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -274,7 +258,7 @@ func vmResizeTests() { updateVMClassPolicies(newVMClass, polices) By("Resize set reservations", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -288,7 +272,7 @@ func vmResizeTests() { By("Resize back to initial class removes reservations", func() { vm.Spec.ClassName = vmClass.Name - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -318,7 +302,7 @@ func vmResizeTests() { vm.Spec.ClassName = newVMClass.Name vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -355,7 +339,7 @@ func vmResizeTests() { updateVMClassPolicies(newVMClass, polices) vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -381,7 +365,7 @@ func vmResizeTests() { delete(vm.Annotations, vmopv1util.LastResizedAnnotationKey) vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -406,7 +390,7 @@ func vmResizeTests() { Expect(vmopv1util.SetLastResizedAnnotationClassName(vm, vmClass.Name)).To(Succeed()) vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -429,7 +413,7 @@ func vmResizeTests() { updateVMClass(vmClass, newCS) vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -452,7 +436,7 @@ func vmResizeTests() { vm.Annotations[vmopv1.VirtualMachineSameVMClassResizeAnnotation] = "" vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -470,8 +454,7 @@ func vmResizeTests() { It("Resize Pending", func() { vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOn)) newCS := configSpec @@ -480,7 +463,7 @@ func vmResizeTests() { newVMClass := createVMClass(newCS) vm.Spec.ClassName = newVMClass.Name - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("Does not resize", func() { @@ -502,8 +485,7 @@ func vmResizeTests() { It("Has Same Class Resize Annotation", func() { vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOn)) newCS := configSpec @@ -512,7 +494,7 @@ func vmResizeTests() { updateVMClass(vmClass, newCS) vm.Annotations[vmopv1.VirtualMachineSameVMClassResizeAnnotation] = "" - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("Does not resize", func() { @@ -546,7 +528,7 @@ func vmResizeTests() { updateVMClass(vmClass, newCS) By("Does not resize without annotation", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -557,7 +539,7 @@ func vmResizeTests() { }) vm.Annotations[vmopv1.VirtualMachineSameVMClassResizeAnnotation] = "" - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -580,7 +562,7 @@ func vmResizeTests() { delete(vm.Annotations, vmopv1util.LastResizedAnnotationKey) By("Does not resize without same class annotation", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -594,7 +576,7 @@ func vmResizeTests() { }) vm.Annotations[vmopv1.VirtualMachineSameVMClassResizeAnnotation] = "" - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -608,8 +590,7 @@ func vmResizeTests() { It("Powered On brownfield VM", func() { vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOn)) // Remove annotation so the VM appears to be from before this feature. @@ -622,7 +603,7 @@ func vmResizeTests() { updateVMClass(vmClass, newCS) vm.Annotations[vmopv1.VirtualMachineSameVMClassResizeAnnotation] = "" - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("Does not resize powered on VM", func() { @@ -678,8 +659,7 @@ func vmResizeTests() { vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff vm.Spec.StorageClass = ctx.StorageClassName - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) }) Context("ChangeBlockTracking", func() { @@ -688,7 +668,7 @@ func vmResizeTests() { ChangeBlockTracking: vimtypes.NewBool(true), } - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -711,7 +691,7 @@ func vmResizeTests() { ChangeBlockTracking: vimtypes.NewBool(true), } - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -739,7 +719,7 @@ func vmResizeTests() { ChangeBlockTracking: vimtypes.NewBool(true), } - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine diff --git a/pkg/providers/vsphere/vmprovider_vm_test.go b/pkg/providers/vsphere/vmprovider_vm_test.go index 899f59d3a..9c850fcd2 100644 --- a/pkg/providers/vsphere/vmprovider_vm_test.go +++ b/pkg/providers/vsphere/vmprovider_vm_test.go @@ -73,6 +73,9 @@ func vmTests() { BeforeEach(func() { parentCtx = ctxop.WithContext(pkgcfg.NewContext()) + pkgcfg.SetContext(parentCtx, func(config *pkgcfg.Config) { + config.AsyncSignalDisabled = true + }) testConfig = builder.VCSimTestConfig{ WithContentLibrary: true, WithWorkloadIsolation: true, @@ -158,21 +161,6 @@ func vmTests() { vsphere.SkipVMImageCLProviderCheck = false }) - createOrUpdateAndGetVcVM := func( - ctx *builder.TestContextForVCSim, - vm *vmopv1.VirtualMachine) (*object.VirtualMachine, error) { - - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) - if err != nil { - return nil, err - } - - ExpectWithOffset(1, vm.Status.UniqueID).ToNot(BeEmpty()) - vcVM := ctx.GetVMFromMoID(vm.Status.UniqueID) - ExpectWithOffset(1, vcVM).ToNot(BeNil()) - return vcVM, nil - } - Context("VM Class and ConfigSpec", func() { var ( @@ -226,7 +214,7 @@ func vmTests() { if !skipCreateOrUpdateVM { var err error - vcVM, err = createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err = createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) } }) @@ -269,7 +257,7 @@ func vmTests() { assertClassNotFound := func(className string) { var err error - vcVM, err = createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err = createOrUpdateAndGetVcVM(ctx, vmProvider, vm) ExpectWithOffset(1, err).ToNot(BeNil()) ExpectWithOffset(1, err.Error()).To(ContainSubstring( fmt.Sprintf("virtualmachineclasses.vmoperator.vmware.com %q not found", className))) @@ -304,7 +292,7 @@ func vmTests() { assertPoweredOnNoVMClassCondition := func() { var err error - vcVM, err = createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err = createOrUpdateAndGetVcVM(ctx, vmProvider, vm) ExpectWithOffset(1, err).ToNot(HaveOccurred()) ExpectWithOffset(1, vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOn)) powerState, err := vcVM.PowerState(ctx) @@ -337,7 +325,7 @@ func vmTests() { }) It("should return an error", func() { var err error - vcVM, err = createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err = createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).To(MatchError("cannot synthesize class from nil ConfigInfo")) Expect(vcVM).To(BeNil()) }) @@ -1096,7 +1084,7 @@ func vmTests() { } Expect(ctx.Client.Create(ctx, pvc1)).To(Succeed()) - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1143,7 +1131,7 @@ func vmTests() { } Expect(ctx.Client.Create(ctx, pvc1)).To(Succeed()) - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1274,7 +1262,7 @@ func vmTests() { }) It("Basic VM", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1375,7 +1363,7 @@ func vmTests() { vm.Labels[kubeutil.CAPVClusterRoleLabelKey] = "" vm.Labels[kubeutil.CAPWClusterRoleLabelKey] = "" - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1396,7 +1384,7 @@ func vmTests() { vm.Annotations[vmopv1.ForceEnableBackupAnnotation] = "true" - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1486,17 +1474,44 @@ func vmTests() { Expect(m.MarkDefault(ctx, ctx.NativeKeyProviderID)).To(Succeed()) }) - It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) - Expect(vm.Status.Crypto).ToNot(BeNil()) - Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( - []vmopv1.VirtualMachineEncryptionType{ - vmopv1.VirtualMachineEncryptionTypeConfig, - })) - Expect(vm.Status.Crypto.ProviderID).To(Equal(ctx.NativeKeyProviderID)) - Expect(vm.Status.Crypto.KeyID).ToNot(BeEmpty()) - Expect(conditions.IsTrue(vm, vmopv1.VirtualMachineEncryptionSynced)).To(BeTrue()) + When("using sync create", func() { + BeforeEach(func() { + pkgcfg.SetContext(parentCtx, func(config *pkgcfg.Config) { + config.AsyncSignalDisabled = true + config.Features.WorkloadDomainIsolation = true + }) + }) + It("should succeed", func() { + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) + Expect(vm.Status.Crypto).ToNot(BeNil()) + Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( + []vmopv1.VirtualMachineEncryptionType{ + vmopv1.VirtualMachineEncryptionTypeConfig, + })) + Expect(vm.Status.Crypto.ProviderID).To(Equal(ctx.NativeKeyProviderID)) + Expect(vm.Status.Crypto.KeyID).ToNot(BeEmpty()) + Expect(conditions.IsTrue(vm, vmopv1.VirtualMachineEncryptionSynced)).To(BeTrue()) + }) + }) + + When("using async create", func() { + BeforeEach(func() { + pkgcfg.SetContext(parentCtx, func(config *pkgcfg.Config) { + config.AsyncSignalDisabled = false + config.Features.WorkloadDomainIsolation = true + }) + }) + It("should succeed", func() { + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) + Expect(vm.Status.Crypto).ToNot(BeNil()) + Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( + []vmopv1.VirtualMachineEncryptionType{ + vmopv1.VirtualMachineEncryptionTypeConfig, + })) + Expect(vm.Status.Crypto.ProviderID).To(Equal(ctx.NativeKeyProviderID)) + Expect(vm.Status.Crypto.KeyID).ToNot(BeEmpty()) + Expect(conditions.IsTrue(vm, vmopv1.VirtualMachineEncryptionSynced)).To(BeTrue()) + }) }) }) @@ -1507,8 +1522,7 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( []vmopv1.VirtualMachineEncryptionType{ @@ -1528,8 +1542,7 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( []vmopv1.VirtualMachineEncryptionType{ @@ -1565,12 +1578,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1590,12 +1601,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1616,12 +1625,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1640,8 +1647,7 @@ func vmTests() { When("there is no vTPM", func() { It("should not error, but have condition", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) c := conditions.Get(vm, vmopv1.VirtualMachineEncryptionSynced) Expect(c).ToNot(BeNil()) @@ -1656,12 +1662,10 @@ func vmTests() { hasVTPM = true }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1707,12 +1711,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1733,12 +1735,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1760,12 +1760,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1787,12 +1785,10 @@ func vmTests() { }) It("should succeed", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).To(BeNil()) - _, err = createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.Crypto).ToNot(BeNil()) Expect(vm.Status.Crypto.Encrypted).To(HaveExactElements( @@ -1827,7 +1823,7 @@ func vmTests() { }) It("VM should not have PCI devices from VM Class", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1847,7 +1843,7 @@ func vmTests() { It("Creates VM", func() { Expect(vm.Spec.StorageClass).To(BeEmpty()) - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1870,7 +1866,7 @@ func vmTests() { // TODO: Dedupe this with "Basic VM" above It("Clones VM", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -1942,7 +1938,7 @@ func vmTests() { ctx.ContentLibraryItemTemplate("DC0_C0_RP0_VM0", imageName) vm.Spec.ImageName = imageName - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) }) @@ -1953,7 +1949,7 @@ func vmTests() { It("creates VM in placement selected zone", func() { Expect(vm.Labels).ToNot(HaveKey(topology.KubernetesTopologyZoneLabelKey)) - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) azName, ok := vm.Labels[topology.KubernetesTopologyZoneLabelKey] @@ -1975,7 +1971,7 @@ func vmTests() { azName := ctx.ZoneNames[rand.Intn(len(ctx.ZoneNames))] vm.Labels[topology.KubernetesTopologyZoneLabelKey] = azName - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("VM is created in the zone's ResourcePool", func() { @@ -2033,7 +2029,7 @@ func vmTests() { Expect(ctx.Client.Status().Update(ctx, pvc1)).To(Succeed()) vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(vm.Status.Zone).To(Equal(azName)) }) @@ -2069,7 +2065,7 @@ func vmTests() { } It("creates VM without instance storage", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) }) @@ -2089,7 +2085,7 @@ func vmTests() { } Expect(ctx.Client.Update(ctx, vmClass)).To(Succeed()) - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).To(MatchError("instance storage PVCs are not bound yet")) Expect(conditions.IsTrue(vm, vmopv1.VirtualMachineConditionCreated)).To(BeFalse()) @@ -2111,7 +2107,7 @@ func vmTests() { // Simulate what would be set by volume controller. vm.Annotations[constants.InstanceStoragePVCsBoundAnnotationKey] = "" - err = vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("status update pending for persistent volume: %s on VM", isVol0.Name))) @@ -2125,7 +2121,7 @@ func vmTests() { }) By("VM is now created", func() { - _, err = createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(conditions.IsTrue(vm, vmopv1.VirtualMachineConditionCreated)).To(BeTrue()) }) @@ -2133,12 +2129,12 @@ func vmTests() { }) It("Powers VM off", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) - Expect(err).ToNot(HaveOccurred()) - + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOn)) + vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) + Expect(err).ToNot(HaveOccurred()) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOff)) state, err := vcVM.PowerState(ctx) @@ -2148,7 +2144,7 @@ func vmTests() { It("returns error when StorageClass is required but none specified", func() { vm.Spec.StorageClass = "" - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(MatchError("StorageClass is required but not specified")) c := conditions.Get(vm, vmopv1.VirtualMachineConditionStorageReady) @@ -2161,14 +2157,14 @@ func vmTests() { }) It("Can be called multiple times", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine Expect(vcVM.Properties(ctx, vcVM.Reference(), nil, &o)).To(Succeed()) modified := o.Config.Modified - _, err = createOrUpdateAndGetVcVM(ctx, vm) + _, err = createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(vcVM.Properties(ctx, vcVM.Reference(), nil, &o)).To(Succeed()) @@ -2200,7 +2196,7 @@ func vmTests() { Transport: vmopv1.VirtualMachineMetadataExtraConfigTransport, } */ - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -2257,7 +2253,7 @@ func vmTests() { It("Should not have a nic", func() { Expect(vm.Spec.Network.Disabled).To(BeTrue()) - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(conditions.Has(vm, vmopv1.VirtualMachineConditionNetworkReady)).To(BeFalse()) @@ -2288,7 +2284,7 @@ func vmTests() { }) It("Has expected devices", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(conditions.IsTrue(vm, vmopv1.VirtualMachineConditionNetworkReady)).To(BeTrue()) @@ -2324,7 +2320,7 @@ func vmTests() { }) It("Succeeds", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -2341,7 +2337,7 @@ func vmTests() { }) It("Succeeds", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -2362,7 +2358,7 @@ func vmTests() { }) It("Succeeds", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -2383,7 +2379,7 @@ func vmTests() { } vm.Spec.Advanced.BootDiskCapacity = &newSize vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var o mo.VirtualMachine @@ -2399,7 +2395,7 @@ func vmTests() { It("CSI Volumes workflow", func() { vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn @@ -2417,7 +2413,7 @@ func vmTests() { }, } - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("status update pending for persistent volume: %s on VM", cnsVolumeName))) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOff)) @@ -2434,7 +2430,7 @@ func vmTests() { }, } - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("persistent volume: %s not attached to VM", cnsVolumeName))) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOff)) @@ -2447,21 +2443,21 @@ func vmTests() { Attached: true, }, } - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Status.PowerState).To(Equal(vmopv1.VirtualMachinePowerStateOn)) }) }) }) It("Reverse lookups existing VM into correct zone", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(vm.Labels).To(HaveKeyWithValue(topology.KubernetesTopologyZoneLabelKey, zoneName)) Expect(vm.Status.Zone).To(Equal(zoneName)) delete(vm.Labels, topology.KubernetesTopologyZoneLabelKey) - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) Expect(vm.Labels).To(HaveKeyWithValue(topology.KubernetesTopologyZoneLabelKey, zoneName)) Expect(vm.Status.Zone).To(Equal(zoneName)) }) @@ -2488,7 +2484,7 @@ func vmTests() { }) It("VM is created in child Folder and ResourcePool", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) By("has expected condition", func() { @@ -2513,7 +2509,7 @@ func vmTests() { }) It("Cluster Modules", func() { - vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) + vcVM, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) var members []vimtypes.ManagedObjectReference @@ -2528,7 +2524,7 @@ func vmTests() { It("Returns error with non-existence cluster module", func() { vm.Annotations["vsphere-cluster-module-group"] = "bogusClusterMod" - err := vmProvider.CreateOrUpdateVirtualMachine(ctx, vm) + err := createOrUpdateVM(ctx, vmProvider, vm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("ClusterModule bogusClusterMod not found")) }) @@ -2543,7 +2539,7 @@ func vmTests() { }) JustBeforeEach(func() { - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) }) Context("when the VM is off", func() { @@ -2579,7 +2575,7 @@ func vmTests() { }) It("returns NotFound when VM does not exist", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) Expect(vmProvider.DeleteVirtualMachine(ctx, vm)).To(Succeed()) @@ -2588,7 +2584,7 @@ func vmTests() { }) It("Deletes existing VM when zone info is missing", func() { - _, err := createOrUpdateAndGetVcVM(ctx, vm) + _, err := createOrUpdateAndGetVcVM(ctx, vmProvider, vm) Expect(err).ToNot(HaveOccurred()) uniqueID := vm.Status.UniqueID @@ -2604,7 +2600,7 @@ func vmTests() { Context("Guest Heartbeat", func() { JustBeforeEach(func() { - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) }) It("return guest heartbeat", func() { @@ -2617,7 +2613,7 @@ func vmTests() { Context("Web console ticket", func() { JustBeforeEach(func() { - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) }) It("return ticket", func() { @@ -2630,7 +2626,7 @@ func vmTests() { Context("VM hardware version", func() { JustBeforeEach(func() { - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) }) It("return version", func() { @@ -2639,59 +2635,59 @@ func vmTests() { Expect(version).To(Equal(vimtypes.VMX9)) }) }) - }) - Context("Create/Update/Delete ISO backed VirtualMachine", func() { - var ( - vm *vmopv1.VirtualMachine - vmClass *vmopv1.VirtualMachineClass - ) - - BeforeEach(func() { - vmClass = builder.DummyVirtualMachineClassGenName() - vm = builder.DummyBasicVirtualMachine("test-vm", "") - - // Reduce diff from old tests: by default don't create an NIC. - if vm.Spec.Network == nil { - vm.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{} - } - vm.Spec.Network.Disabled = true - }) - - JustBeforeEach(func() { - vmClass.Namespace = nsInfo.Namespace - Expect(ctx.Client.Create(ctx, vmClass)).To(Succeed()) + Context("Create/Update/Delete ISO backed VirtualMachine", func() { + var ( + vm *vmopv1.VirtualMachine + vmClass *vmopv1.VirtualMachineClass + ) - clusterVMImage := &vmopv1.ClusterVirtualMachineImage{} - Expect(ctx.Client.Get(ctx, client.ObjectKey{Name: ctx.ContentLibraryIsoImageName}, clusterVMImage)).To(Succeed()) + BeforeEach(func() { + vmClass = builder.DummyVirtualMachineClassGenName() + vm = builder.DummyBasicVirtualMachine("test-vm", "") - vm.Namespace = nsInfo.Namespace - vm.Spec.ClassName = vmClass.Name - vm.Spec.ImageName = clusterVMImage.Name - vm.Spec.Image.Kind = cvmiKind - vm.Spec.Image.Name = clusterVMImage.Name - vm.Spec.StorageClass = ctx.StorageClassName - vm.Spec.Cdrom = []vmopv1.VirtualMachineCdromSpec{{ - Name: "cdrom0", - Image: vmopv1.VirtualMachineImageRef{ - Name: cvmiKind, - Kind: clusterVMImage.Name, - }, - }} - }) + // Reduce diff from old tests: by default don't create an NIC. + if vm.Spec.Network == nil { + vm.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{} + } + vm.Spec.Network.Disabled = true + }) - Context("return config", func() { JustBeforeEach(func() { - Expect(vmProvider.CreateOrUpdateVirtualMachine(ctx, vm)).To(Succeed()) + vmClass.Namespace = nsInfo.Namespace + Expect(ctx.Client.Create(ctx, vmClass)).To(Succeed()) + + clusterVMImage := &vmopv1.ClusterVirtualMachineImage{} + Expect(ctx.Client.Get(ctx, client.ObjectKey{Name: ctx.ContentLibraryIsoImageName}, clusterVMImage)).To(Succeed()) + + vm.Namespace = nsInfo.Namespace + vm.Spec.ClassName = vmClass.Name + vm.Spec.ImageName = clusterVMImage.Name + vm.Spec.Image.Kind = cvmiKind + vm.Spec.Image.Name = clusterVMImage.Name + vm.Spec.StorageClass = ctx.StorageClassName + vm.Spec.Cdrom = []vmopv1.VirtualMachineCdromSpec{{ + Name: "cdrom0", + Image: vmopv1.VirtualMachineImageRef{ + Name: cvmiKind, + Kind: clusterVMImage.Name, + }, + }} }) - It("return config.files", func() { - vmPathName := "config.files.vmPathName" - props, err := vmProvider.GetVirtualMachineProperties(ctx, vm, []string{vmPathName}) - Expect(err).NotTo(HaveOccurred()) - var path object.DatastorePath - path.FromString(props[vmPathName].(string)) - Expect(path.Datastore).NotTo(BeEmpty()) + Context("return config", func() { + JustBeforeEach(func() { + Expect(createOrUpdateVM(ctx, vmProvider, vm)).To(Succeed()) + }) + + It("return config.files", func() { + vmPathName := "config.files.vmPathName" + props, err := vmProvider.GetVirtualMachineProperties(ctx, vm, []string{vmPathName}) + Expect(err).NotTo(HaveOccurred()) + var path object.DatastorePath + path.FromString(props[vmPathName].(string)) + Expect(path.Datastore).NotTo(BeEmpty()) + }) }) }) }) diff --git a/pkg/providers/vsphere/vsphere_suite_test.go b/pkg/providers/vsphere/vsphere_suite_test.go index f6b749b7d..2ec31ba7e 100644 --- a/pkg/providers/vsphere/vsphere_suite_test.go +++ b/pkg/providers/vsphere/vsphere_suite_test.go @@ -4,10 +4,19 @@ package vsphere_test import ( + "fmt" "testing" . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/vmware/govmomi/object" + "sigs.k8s.io/controller-runtime/pkg/client" + + vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" + pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config" + ctxop "github.com/vmware-tanzu/vm-operator/pkg/context/operation" + "github.com/vmware-tanzu/vm-operator/pkg/providers" "github.com/vmware-tanzu/vm-operator/test/builder" ) @@ -31,3 +40,83 @@ func TestVSphereProvider(t *testing.T) { var _ = BeforeSuite(suite.BeforeSuite) var _ = AfterSuite(suite.AfterSuite) + +func createOrUpdateVM( + ctx *builder.TestContextForVCSim, + provider providers.VirtualMachineProviderInterface, + vm *vmopv1.VirtualMachine) error { + + if pkgcfg.FromContext(ctx).Features.WorkloadDomainIsolation { + if !pkgcfg.FromContext(ctx).AsyncSignalDisabled { + By("non-blocking createOrUpdateVM") + return createOrUpdateVMAsync(ctx, provider, vm) + } + } + + By("blocking createOrUpdateVM") + return provider.CreateOrUpdateVirtualMachine(ctx, vm) +} + +func createOrUpdateAndGetVcVM( + ctx *builder.TestContextForVCSim, + provider providers.VirtualMachineProviderInterface, + vm *vmopv1.VirtualMachine) (*object.VirtualMachine, error) { + + if err := createOrUpdateVM(ctx, provider, vm); err != nil { + return nil, err + } + + ExpectWithOffset(1, vm.Status.UniqueID).ToNot(BeEmpty()) + vcVM := ctx.GetVMFromMoID(vm.Status.UniqueID) + ExpectWithOffset(1, vcVM).ToNot(BeNil()) + return vcVM, nil +} + +func createOrUpdateVMAsync( + testCtx *builder.TestContextForVCSim, + provider providers.VirtualMachineProviderInterface, + vm *vmopv1.VirtualMachine) error { + + // This ensures there is no current operation set on the context. + ctx := ctxop.WithContext(testCtx) + + chanErr, err := provider.CreateOrUpdateVirtualMachineAsync(ctx, vm) + if err != nil { + return err + } + + // Unlike the VM controller, this test helper blocks until the async + // parts of CreateOrUpdateVM are complete. This is to avoid a large + // refactor for now. + for err2 := range chanErr { + if err2 != nil { + if err == nil { + err = err2 + } else { + err = fmt.Errorf("%w,%w", err, err2) + } + } + } + if err != nil { + return err + } + + if ctxop.IsCreate(ctx) { + // The async create operation does not fall-through to the + // update logic, so we need to call CreateOrUpdateVirtualMachine + // a second time to cause the update. + ExpectWithOffset(1, testCtx.Client.Get( + ctx, + client.ObjectKeyFromObject(vm), + vm)).To(Succeed()) + + if _, err := provider.CreateOrUpdateVirtualMachineAsync( + ctx, + vm); err != nil { + + return err + } + } + + return nil +}