Add machine reconcile delete on termination (#336)

* Add support for machine deletion after instance termination

Author: shyamradhakrishnan

api/v1beta2/ocimachine_types.go

@@ -28,7 +28,8 @@ import (
 const (
 	// MachineFinalizer allows ReconcileMachine to clean up OCI resources associated with OCIMachine before
 	// removing it from the apiserver.
-	MachineFinalizer = "ocimachine.infrastructure.cluster.x-k8s.io"
+	MachineFinalizer                   = "ocimachine.infrastructure.cluster.x-k8s.io"
+	DeleteMachineOnInstanceTermination = "ociclusters.x-k8s.io/delete-machine-on-instance-termination"
 )
 
 // OCIMachineSpec defines the desired state of OCIMachine

controllers/ocimachine_controller.go

@@ -234,6 +234,13 @@ func (r *OCIMachineReconciler) OCIClusterToOCIMachines(ctx context.Context) hand
 func (r *OCIMachineReconciler) reconcileNormal(ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope) (ctrl.Result, error) {
 	controllerutil.AddFinalizer(machineScope.OCIMachine, infrastructurev1beta2.MachineFinalizer)
 	machine := machineScope.OCIMachine
+	infraMachine := machineScope.Machine
+
+	annotations := infraMachine.GetAnnotations()
+	deleteMachineOnTermination := false
+	if annotations != nil {
+		_, deleteMachineOnTermination = annotations[infrastructurev1beta2.DeleteMachineOnInstanceTermination]
+	}
 	// Make sure bootstrap data is available and populated.
 	if machineScope.Machine.Spec.Bootstrap.DataSecretName == nil {
 		r.Recorder.Event(machine, corev1.EventTypeNormal, infrastructurev1beta2.WaitingForBootstrapDataReason, "Bootstrap data secret reference is not yet available")
@@ -304,7 +311,21 @@ func (r *OCIMachineReconciler) reconcileNormal(ctx context.Context, logger logr.
 			"Instance is in ready state")
 		conditions.MarkTrue(machineScope.OCIMachine, infrastructurev1beta2.InstanceReadyCondition)
 		machineScope.SetReady()
-		return reconcile.Result{}, nil
+		if deleteMachineOnTermination {
+			// typically, if the VM is terminated, we should get machine events, so ideally, the 300 seconds
+			// requeue time is not required, but in case, the event is missed, adding the requeue time
+			return reconcile.Result{RequeueAfter: 300 * time.Second}, nil
+		} else {
+			return reconcile.Result{}, nil
+		}
+	case core.InstanceLifecycleStateTerminated:
+		if deleteMachineOnTermination && infraMachine.DeletionTimestamp == nil {
+			logger.Info("Deleting underlying machine as instance is terminated")
+			if err := machineScope.Client.Delete(ctx, infraMachine); err != nil {
+				return reconcile.Result{}, errors.Wrapf(err, "failed to delete machine %s/%s", infraMachine.Namespace, infraMachine.Name)
+			}
+		}
+		fallthrough
 	default:
 		conditions.MarkFalse(machineScope.OCIMachine, infrastructurev1beta2.InstanceReadyCondition, infrastructurev1beta2.InstanceProvisionFailedReason, clusterv1.ConditionSeverityError, "")
 		machineScope.SetFailureReason(capierrors.CreateMachineError)

controllers/ocimachine_controller_test.go

@@ -22,26 +22,26 @@ import (
 	"testing"
 	"time"
 
-	"github.com/oracle/cluster-api-provider-oci/cloud/ociutil"
-	"github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer/mock_nlb"
-	"github.com/oracle/cluster-api-provider-oci/cloud/services/vcn/mock_vcn"
-	"github.com/oracle/oci-go-sdk/v65/networkloadbalancer"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/gomega"
 	infrastructurev1beta2 "github.com/oracle/cluster-api-provider-oci/api/v1beta2"
+	"github.com/oracle/cluster-api-provider-oci/cloud/ociutil"
 	"github.com/oracle/cluster-api-provider-oci/cloud/scope"
 	"github.com/oracle/cluster-api-provider-oci/cloud/services/compute/mock_compute"
+	"github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer/mock_nlb"
+	"github.com/oracle/cluster-api-provider-oci/cloud/services/vcn/mock_vcn"
 	"github.com/oracle/oci-go-sdk/v65/common"
 	"github.com/oracle/oci-go-sdk/v65/core"
+	"github.com/oracle/oci-go-sdk/v65/networkloadbalancer"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/conditions"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -194,19 +194,22 @@ func TestNormalReconciliationFunction(t *testing.T) {
 	teardown := func(t *testing.T, g *WithT) {
 		mockCtrl.Finish()
 	}
-	tests := []struct {
+	type test struct {
 		name               string
 		errorExpected      bool
 		expectedEvent      string
 		eventNotExpected   string
 		conditionAssertion []conditionAssertion
-		testSpecificSetup  func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient)
-	}{
+		deleteMachines     []clusterv1.Machine
+		testSpecificSetup  func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient)
+		validate           func(g *WithT, t *test, result ctrl.Result)
+	}
+	tests := []test{
 		{
 			name:               "instance in provisioning state",
 			errorExpected:      false,
 			conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrastructurev1beta2.InstanceNotReadyReason}},
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
+			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"),
 				})).
@@ -222,7 +225,7 @@ func TestNormalReconciliationFunction(t *testing.T) {
 			name:               "instance in running state",
 			errorExpected:      false,
 			conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionTrue, "", ""}},
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
 				machineScope.OCIMachine.Status.Addresses = []clusterv1.MachineAddress{
 					{
 						Type:    clusterv1.MachineInternalIP,
@@ -240,12 +243,67 @@ func TestNormalReconciliationFunction(t *testing.T) {
 					}, nil)
 			},
 		},
+		{
+			name:               "instance in running state, reconcile every 5 minutes",
+			errorExpected:      false,
+			conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionTrue, "", ""}},
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
+				if machineScope.Machine.ObjectMeta.Annotations == nil {
+					machineScope.Machine.ObjectMeta.Annotations = make(map[string]string)
+				}
+				machineScope.Machine.ObjectMeta.Annotations[infrastructurev1beta2.DeleteMachineOnInstanceTermination] = ""
+				machineScope.OCIMachine.Status.Addresses = []clusterv1.MachineAddress{
+					{
+						Type:    clusterv1.MachineInternalIP,
+						Address: "1.1.1.1",
+					},
+				}
+				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.InstanceLifecycleStateRunning,
+						},
+					}, nil)
+			},
+		},
+		{
+			name:               "instance in running state, reconcile every 5 minutes",
+			errorExpected:      false,
+			conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionTrue, "", ""}},
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
+				if machineScope.Machine.ObjectMeta.Annotations == nil {
+					machineScope.Machine.ObjectMeta.Annotations = make(map[string]string)
+				}
+				machineScope.Machine.ObjectMeta.Annotations[infrastructurev1beta2.DeleteMachineOnInstanceTermination] = ""
+				machineScope.OCIMachine.Status.Addresses = []clusterv1.MachineAddress{
+					{
+						Type:    clusterv1.MachineInternalIP,
+						Address: "1.1.1.1",
+					},
+				}
+				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.InstanceLifecycleStateRunning,
+						},
+					}, nil)
+			},
+			validate: func(g *WithT, t *test, result ctrl.Result) {
+				g.Expect(result.RequeueAfter).To(Equal(300 * time.Second))
+			},
+		},
 		{
 			name:               "instance in terminated state",
 			errorExpected:      true,
 			expectedEvent:      "invalid lifecycle state TERMINATED",
 			conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityError, infrastructurev1beta2.InstanceProvisionFailedReason}},
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
+			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"),
 				})).
@@ -256,13 +314,16 @@ func TestNormalReconciliationFunction(t *testing.T) {
 						},
 					}, nil)
 			},
+			validate: func(g *WithT, t *test, result ctrl.Result) {
+				g.Expect(result.RequeueAfter).To(Equal(0 * time.Second))
+			},
 		},
 		{
 			name:               "control plane backend vnic attachments call error",
 			errorExpected:      true,
 			expectedEvent:      "server error",
 			conditionAssertion: []conditionAssertion{{infrastructurev1beta2.InstanceReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityError, infrastructurev1beta2.InstanceIPAddressNotFound}},
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbclient *mock_nlb.MockNetworkLoadBalancerClient) {
 				machineScope.Machine.ObjectMeta.Labels = make(map[string]string)
 				machineScope.Machine.ObjectMeta.Labels[clusterv1.MachineControlPlaneLabel] = "true"
 				computeClient.EXPECT().GetInstance(gomock.Any(), gomock.Eq(core.GetInstanceRequest{
@@ -285,7 +346,7 @@ func TestNormalReconciliationFunction(t *testing.T) {
 		{
 			name:          "control plane backend backend exists",
 			errorExpected: false,
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
 				machineScope.Machine.ObjectMeta.Labels = make(map[string]string)
 				machineScope.Machine.ObjectMeta.Labels[clusterv1.MachineControlPlaneLabel] = "true"
 				computeClient.EXPECT().GetInstance(gomock.Any(), gomock.Eq(core.GetInstanceRequest{
@@ -342,7 +403,7 @@ func TestNormalReconciliationFunction(t *testing.T) {
 		{
 			name:          "backend creation",
 			errorExpected: false,
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
 				machineScope.Machine.ObjectMeta.Labels = make(map[string]string)
 				machineScope.Machine.ObjectMeta.Labels[clusterv1.MachineControlPlaneLabel] = "true"
 				computeClient.EXPECT().GetInstance(gomock.Any(), gomock.Eq(core.GetInstanceRequest{
@@ -417,7 +478,7 @@ func TestNormalReconciliationFunction(t *testing.T) {
 		{
 			name:          "ip address exists",
 			errorExpected: false,
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
 				machineScope.Machine.ObjectMeta.Labels = make(map[string]string)
 				machineScope.OCIMachine.Status.Addresses = []clusterv1.MachineAddress{
 					{
@@ -476,7 +537,7 @@ func TestNormalReconciliationFunction(t *testing.T) {
 		{
 			name:          "backend creation fails",
 			errorExpected: true,
-			testSpecificSetup: func(machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
+			testSpecificSetup: func(t *test, machineScope *scope.MachineScope, computeClient *mock_compute.MockComputeClient, vcnClient *mock_vcn.MockClient, nlbClient *mock_nlb.MockNetworkLoadBalancerClient) {
 				machineScope.Machine.ObjectMeta.Labels = make(map[string]string)
 				machineScope.Machine.ObjectMeta.Labels[clusterv1.MachineControlPlaneLabel] = "true"
 				computeClient.EXPECT().GetInstance(gomock.Any(), gomock.Eq(core.GetInstanceRequest{
@@ -555,9 +616,9 @@ func TestNormalReconciliationFunction(t *testing.T) {
 			g := NewWithT(t)
 			defer teardown(t, g)
 			setup(t, g)
-			tc.testSpecificSetup(ms, computeClient, vcnClient, nlbClient)
+			tc.testSpecificSetup(&tc, ms, computeClient, vcnClient, nlbClient)
 			ctx := context.Background()
-			_, err := r.reconcileNormal(ctx, log.FromContext(ctx), ms)
+			result, err := r.reconcileNormal(ctx, log.FromContext(ctx), ms)
 			if len(tc.conditionAssertion) > 0 {
 				expectConditions(g, ociMachine, tc.conditionAssertion)
 			}
@@ -569,6 +630,9 @@ func TestNormalReconciliationFunction(t *testing.T) {
 			if tc.expectedEvent != "" {
 				g.Eventually(recorder.Events).Should(Receive(ContainSubstring(tc.expectedEvent)))
 			}
+			if tc.validate != nil {
+				tc.validate(g, &tc, result)
+			}
 		})
 	}
 }