From 5407081c6c206610ace2650222885d071f590fae Mon Sep 17 00:00:00 2001 From: Shyam Radhakrishnan Date: Mon, 30 Oct 2023 19:42:47 +0530 Subject: [PATCH] Fix vm state bug and deletion bug (#340) * Do not put machine in failed state if VM is in stopping or stopped state, and use ID from instance during deletion --- cloud/scope/machine.go | 9 +++++-- cloud/scope/machine_test.go | 9 ++++++- controllers/ocimachine_controller.go | 7 ++++- controllers/ocimachine_controller_test.go | 32 +++++++++++++++++++++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go index fd6e8a73..bfa713c0 100644 --- a/cloud/scope/machine.go +++ b/cloud/scope/machine.go @@ -301,8 +301,8 @@ func (m *MachineScope) getFreeFormTags() map[string]string { } // DeleteMachine terminates the instance using InstanceId from the OCIMachine spec and deletes the boot volume -func (m *MachineScope) DeleteMachine(ctx context.Context) error { - req := core.TerminateInstanceRequest{InstanceId: m.OCIMachine.Spec.InstanceId, +func (m *MachineScope) DeleteMachine(ctx context.Context, instance *core.Instance) error { + req := core.TerminateInstanceRequest{InstanceId: instance.Id, PreserveBootVolume: common.Bool(false)} _, err := m.ComputeClient.TerminateInstance(ctx, req) return err @@ -405,6 +405,11 @@ func (m *MachineScope) SetReady() { m.OCIMachine.Status.Ready = true } +// SetNotReady sets the OCIMachine Ready Status to false. +func (m *MachineScope) SetNotReady() { + m.OCIMachine.Status.Ready = false +} + // IsReady returns the ready status of the machine. func (m *MachineScope) IsReady() bool { return m.OCIMachine.Status.Ready diff --git a/cloud/scope/machine_test.go b/cloud/scope/machine_test.go index 5a7770a5..7fe3a80b 100644 --- a/cloud/scope/machine_test.go +++ b/cloud/scope/machine_test.go @@ -2393,6 +2393,7 @@ func TestInstanceDeletion(t *testing.T) { expectedEvent string eventNotExpected string matchError error + instance *core.Instance errorSubStringMatch bool testSpecificSetup func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) }{ @@ -2406,6 +2407,9 @@ func TestInstanceDeletion(t *testing.T) { PreserveBootVolume: common.Bool(false), })).Return(core.TerminateInstanceResponse{}, nil) }, + instance: &core.Instance{ + Id: common.String("test"), + }, }, { name: "delete instance error", @@ -2418,6 +2422,9 @@ func TestInstanceDeletion(t *testing.T) { PreserveBootVolume: common.Bool(false), })).Return(core.TerminateInstanceResponse{}, errors.New("could not terminate instance")) }, + instance: &core.Instance{ + Id: common.String("test"), + }, }, } @@ -2427,7 +2434,7 @@ func TestInstanceDeletion(t *testing.T) { defer teardown(t, g) setup(t, g) tc.testSpecificSetup(ms, computeClient) - err := ms.DeleteMachine(context.Background()) + err := ms.DeleteMachine(context.Background(), tc.instance) if tc.errorExpected { g.Expect(err).To(Not(BeNil())) if tc.errorSubStringMatch { diff --git a/controllers/ocimachine_controller.go b/controllers/ocimachine_controller.go index a9035895..2c0596f2 100644 --- a/controllers/ocimachine_controller.go +++ b/controllers/ocimachine_controller.go @@ -326,6 +326,10 @@ func (r *OCIMachineReconciler) reconcileNormal(ctx context.Context, logger logr. machineScope.Info("Instance is pending") conditions.MarkFalse(machineScope.OCIMachine, infrastructurev1beta2.InstanceReadyCondition, infrastructurev1beta2.InstanceNotReadyReason, clusterv1.ConditionSeverityInfo, "") return reconcile.Result{RequeueAfter: 10 * time.Second}, nil + case core.InstanceLifecycleStateStopping, core.InstanceLifecycleStateStopped: + machineScope.SetNotReady() + conditions.MarkFalse(machineScope.OCIMachine, infrastructurev1beta2.InstanceReadyCondition, infrastructurev1beta2.InstanceNotReadyReason, clusterv1.ConditionSeverityInfo, "") + return reconcile.Result{}, nil case core.InstanceLifecycleStateRunning: machineScope.Info("Instance is active") if machine.Status.Addresses == nil || len(machine.Status.Addresses) == 0 { @@ -387,6 +391,7 @@ func (r *OCIMachineReconciler) reconcileNormal(ctx context.Context, logger logr. } fallthrough default: + machineScope.SetNotReady() conditions.MarkFalse(machineScope.OCIMachine, infrastructurev1beta2.InstanceReadyCondition, infrastructurev1beta2.InstanceProvisionFailedReason, clusterv1.ConditionSeverityError, "") machineScope.SetFailureReason(capierrors.CreateMachineError) machineScope.SetFailureMessage(errors.Errorf("Instance status %q is unexpected", instance.LifecycleState)) @@ -446,7 +451,7 @@ func (r *OCIMachineReconciler) reconcileDelete(ctx context.Context, machineScope if err != nil { return reconcile.Result{}, err } - if err := machineScope.DeleteMachine(ctx); err != nil { + if err := machineScope.DeleteMachine(ctx, instance); err != nil { machineScope.Error(err, "Error deleting Instance") return ctrl.Result{}, errors.Wrapf(err, "error deleting instance %s", machineScope.Name()) } diff --git a/controllers/ocimachine_controller_test.go b/controllers/ocimachine_controller_test.go index 07c9c5b0..7a1009f8 100644 --- a/controllers/ocimachine_controller_test.go +++ b/controllers/ocimachine_controller_test.go @@ -302,6 +302,38 @@ func TestNormalReconciliationFunction(t *testing.T) { g.Expect(result.RequeueAfter).To(Equal(300 * time.Second)) }, }, + { + name: "instance in stopped state", + errorExpected: false, + conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrastructurev1beta2.InstanceNotReadyReason}}, + testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) { + computeClient.EXPECT().GetInstance(gomock.Any(), gomock.Eq(core.GetInstanceRequest{ + InstanceId: common.String("test"), + })). + Return(core.GetInstanceResponse{ + Instance: core.Instance{ + Id: common.String("test"), + LifecycleState: core.InstanceLifecycleStateStopped, + }, + }, nil) + }, + }, + { + name: "instance in stopping state", + errorExpected: false, + conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrastructurev1beta2.InstanceNotReadyReason}}, + testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) { + computeClient.EXPECT().GetInstance(gomock.Any(), gomock.Eq(core.GetInstanceRequest{ + InstanceId: common.String("test"), + })). + Return(core.GetInstanceResponse{ + Instance: core.Instance{ + Id: common.String("test"), + LifecycleState: core.InstanceLifecycleStateStopping, + }, + }, nil) + }, + }, { name: "instance in terminated state", errorExpected: true,