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

Author: shyamradhakrishnan

cloud/scope/machine.go

@@ -300,8 +300,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
@@ -404,6 +404,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

cloud/scope/machine_test.go

@@ -2391,6 +2391,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)
 	}{
@@ -2404,6 +2405,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",
@@ -2416,6 +2420,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"),
+			},
 		},
 	}
 
@@ -2425,7 +2432,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 {

controllers/ocimachine_controller.go

@@ -281,6 +281,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 {
@@ -342,6 +346,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))
@@ -401,7 +406,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())
 		}

controllers/ocimachine_controller_test.go

@@ -300,6 +300,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,