From d0db5eff49bf025d45ef2ee742c73206f7e0ec5b Mon Sep 17 00:00:00 2001 From: Mohamed Chiheb Ben Jemaa Date: Wed, 17 Jan 2024 15:13:43 +0100 Subject: [PATCH 1/3] Fix the VM name lazy intialize --- internal/service/vmservice/find.go | 15 ++++++++++++++- internal/service/vmservice/vm.go | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/service/vmservice/find.go b/internal/service/vmservice/find.go index b63680f5..7feabda0 100644 --- a/internal/service/vmservice/find.go +++ b/internal/service/vmservice/find.go @@ -35,6 +35,9 @@ var ( // ErrVMNotFound VM is not Found in Proxmox. ErrVMNotFound = errors.New("vm not found") + + // ErrVMNotInitialized VM is not Initialized in Proxmox. + ErrVMNotInitialized = errors.New("vm not Initialized") ) // FindVM returns the Proxmox VM if the vmID is set, otherwise @@ -50,6 +53,10 @@ func FindVM(ctx context.Context, scope *scope.MachineScope) (*proxmox.VirtualMac scope.Error(err, "unable to find vm") return nil, ErrVMNotFound } + if vm.Name != scope.ProxmoxMachine.GetName() { + scope.Error(err, "VM is not initialized yet") + return nil, ErrVMNotInitialized + } return vm, nil } @@ -72,11 +79,17 @@ func updateVMLocation(ctx context.Context, s *scope.MachineScope) error { // We are looking for a machine with the ID and check if the name matches. // Then we have to update the node in the machine and cluster status. - vm, err := s.InfraCluster.ProxmoxClient.FindVMResource(ctx, uint64(vmID)) + rsc, err := s.InfraCluster.ProxmoxClient.FindVMResource(ctx, uint64(vmID)) if err != nil { return err } + // find the VM, to make sure the vm config is up-to-date. + vm, err := s.InfraCluster.ProxmoxClient.GetVM(ctx, rsc.Node, vmID) + if err != nil { + return errors.Wrapf(err, "unable to find vm with id %d", rsc.VMID) + } + // Requeue if machine doesn't have a name yet. // It seems that the Proxmox source API does not always provide // the latest information about the resources in the cluster. diff --git a/internal/service/vmservice/vm.go b/internal/service/vmservice/vm.go index 5d9caa47..7dd95908 100644 --- a/internal/service/vmservice/vm.go +++ b/internal/service/vmservice/vm.go @@ -107,6 +107,8 @@ func ensureVirtualMachine(ctx context.Context, machineScope *scope.MachineScope) // we always want to trigger reconciliation at this point. return false, err + case errors.Is(err, ErrVMNotInitialized): + return true, err case !errors.Is(err, ErrVMNotCreated): return false, err } From 6b94411d8dc3691c89ffa3e6c8bf8c78a55dcf7b Mon Sep 17 00:00:00 2001 From: Mohamed Chiheb Ben Jemaa Date: Wed, 17 Jan 2024 15:33:05 +0100 Subject: [PATCH 2/3] Fix tests --- internal/service/vmservice/find_test.go | 27 ++++++++++++++++++++-- internal/service/vmservice/helpers_test.go | 4 ++-- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/internal/service/vmservice/find_test.go b/internal/service/vmservice/find_test.go index 7e172896..2560a9e6 100644 --- a/internal/service/vmservice/find_test.go +++ b/internal/service/vmservice/find_test.go @@ -76,15 +76,31 @@ func TestFindVM_NotCreated(t *testing.T) { require.ErrorIs(t, err, ErrVMNotCreated) } +func TestFindVM_NotInitialized(t *testing.T) { + ctx := context.TODO() + machineScope, proxmoxClient, _ := setupReconcilerTest(t) + vm := newRunningVM() + vm.Name = "bar" + machineScope.ProxmoxMachine.Spec.VirtualMachineID = ptr.To(int64(vm.VMID)) + machineScope.ProxmoxMachine.Status.ProxmoxNode = ptr.To("node2") + + proxmoxClient.EXPECT().GetVM(ctx, "node2", int64(123)).Return(vm, nil).Once() + + _, err := FindVM(ctx, machineScope) + require.ErrorIs(t, err, ErrVMNotInitialized) +} + func TestUpdateVMLocation_MissingName(t *testing.T) { ctx := context.TODO() machineScope, proxmoxClient, _ := setupReconcilerTest(t) vm := newRunningVM() vmr := newVMResource() vmr.Name = "" + vm.Name = "" machineScope.ProxmoxMachine.Spec.VirtualMachineID = ptr.To(int64(vm.VMID)) proxmoxClient.EXPECT().FindVMResource(ctx, uint64(123)).Return(vmr, nil).Once() + proxmoxClient.EXPECT().GetVM(ctx, "node1", int64(123)).Return(vm, nil).Once() require.Error(t, updateVMLocation(ctx, machineScope)) } @@ -94,10 +110,13 @@ func TestUpdateVMLocation_NameMismatch(t *testing.T) { machineScope, proxmoxClient, _ := setupReconcilerTest(t) vm := newRunningVM() vmr := newVMResource() - vmr.Name = "foo" + name := "foo" + vmr.Name = name + vm.Name = name machineScope.ProxmoxMachine.Spec.VirtualMachineID = ptr.To(int64(vm.VMID)) proxmoxClient.EXPECT().FindVMResource(ctx, uint64(123)).Return(vmr, nil).Once() + proxmoxClient.EXPECT().GetVM(ctx, "node1", int64(123)).Return(vm, nil).Once() require.Error(t, updateVMLocation(ctx, machineScope)) require.True(t, machineScope.HasFailed(), "expected failureReason and failureMessage to be set") @@ -116,6 +135,7 @@ func TestUpdateVMLocation_UpdateNode(t *testing.T) { }, false) proxmoxClient.EXPECT().FindVMResource(ctx, uint64(123)).Return(vmr, nil).Once() + proxmoxClient.EXPECT().GetVM(ctx, "node1", int64(123)).Return(vm, nil).Once() require.NoError(t, updateVMLocation(ctx, machineScope)) require.Equal(t, vmr.Node, *machineScope.ProxmoxMachine.Status.ProxmoxNode) @@ -137,11 +157,14 @@ func TestUpdateVMLocation_WithoutTaskNameMismatch(t *testing.T) { machineScope, proxmoxClient, _ := setupReconcilerTest(t) vm := newRunningVM() vmr := newVMResource() - vmr.Name = "foo" + name := "foo" + vmr.Name = name + vm.Name = name machineScope.ProxmoxMachine.Spec.VirtualMachineID = ptr.To(int64(vm.VMID)) machineScope.ProxmoxMachine.Status.TaskRef = nil proxmoxClient.EXPECT().FindVMResource(ctx, uint64(123)).Return(vmr, nil).Once() + proxmoxClient.EXPECT().GetVM(ctx, "node1", int64(123)).Return(vm, nil).Once() require.Error(t, updateVMLocation(ctx, machineScope)) require.True(t, machineScope.HasFailed(), "expected failureReason and failureMessage to be set") diff --git a/internal/service/vmservice/helpers_test.go b/internal/service/vmservice/helpers_test.go index ace17a38..f14867ce 100644 --- a/internal/service/vmservice/helpers_test.go +++ b/internal/service/vmservice/helpers_test.go @@ -236,7 +236,7 @@ func newVMResource() *proxmox.ClusterResource { func newRunningVM() *proxmox.VirtualMachine { return &proxmox.VirtualMachine{ VirtualMachineConfig: &proxmox.VirtualMachineConfig{}, - Name: "running", + Name: "test", Node: "node1", Status: proxmox.StatusVirtualMachineRunning, VMID: 123, @@ -258,7 +258,7 @@ func newPausedVM() *proxmox.VirtualMachine { func newStoppedVM() *proxmox.VirtualMachine { return &proxmox.VirtualMachine{ VirtualMachineConfig: &proxmox.VirtualMachineConfig{}, - Name: "stopped", + Name: "test", Node: "node1", Status: proxmox.StatusVirtualMachineStopped, QMPStatus: proxmox.StatusVirtualMachineStopped, From 04c60271ec6c68165f923133c339687d9e778bef Mon Sep 17 00:00:00 2001 From: Mohamed Chiheb Ben Jemaa Date: Wed, 17 Jan 2024 15:45:00 +0100 Subject: [PATCH 3/3] Address reviews --- internal/service/vmservice/find.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/vmservice/find.go b/internal/service/vmservice/find.go index 7feabda0..55cf23f5 100644 --- a/internal/service/vmservice/find.go +++ b/internal/service/vmservice/find.go @@ -37,7 +37,7 @@ var ( ErrVMNotFound = errors.New("vm not found") // ErrVMNotInitialized VM is not Initialized in Proxmox. - ErrVMNotInitialized = errors.New("vm not Initialized") + ErrVMNotInitialized = errors.New("vm not initialized") ) // FindVM returns the Proxmox VM if the vmID is set, otherwise @@ -54,7 +54,7 @@ func FindVM(ctx context.Context, scope *scope.MachineScope) (*proxmox.VirtualMac return nil, ErrVMNotFound } if vm.Name != scope.ProxmoxMachine.GetName() { - scope.Error(err, "VM is not initialized yet") + scope.Error(err, "vm is not initialized yet") return nil, ErrVMNotInitialized } return vm, nil