From c0b4f5a4243d6efd59fe706de7cfd5299d9d7f4b Mon Sep 17 00:00:00 2001 From: Shyam Radhakrishnan Date: Wed, 4 Oct 2023 18:54:44 +0530 Subject: [PATCH] Add machine reconcile delete on termination (#336) * Add support for machine deletion after instance termination --- api/v1beta2/ocimachine_types.go | 3 +- controllers/ocimachine_controller.go | 23 ++++- controllers/ocimachine_controller_test.go | 102 ++++++++++++++++++---- 3 files changed, 107 insertions(+), 21 deletions(-) diff --git a/api/v1beta2/ocimachine_types.go b/api/v1beta2/ocimachine_types.go index 11629239..5052590f 100644 --- a/api/v1beta2/ocimachine_types.go +++ b/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 diff --git a/controllers/ocimachine_controller.go b/controllers/ocimachine_controller.go index 1206e1cd..2dc2281f 100644 --- a/controllers/ocimachine_controller.go +++ b/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) diff --git a/controllers/ocimachine_controller_test.go b/controllers/ocimachine_controller_test.go index 80f360fb..09275d2e 100644 --- a/controllers/ocimachine_controller_test.go +++ b/controllers/ocimachine_controller_test.go @@ -22,19 +22,17 @@ 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" @@ -42,6 +40,8 @@ import ( "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) + } }) } }