Use scope Close instead of patching

cloud/scope/vpc.go

@@ -17,19 +17,31 @@ limitations under the License.
 package scope
 
 import (
+	"context"
 	"errors"
 	"fmt"
 
-	infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 	"github.com/linode/linodego"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 )
 
+// VPCScope defines the basic context for an actuator to operate upon.
+type VPCScope struct {
+	client client.Client
+
+	PatchHelper  *patch.Helper
+	LinodeClient *linodego.Client
+	LinodeVPC    *infrav1alpha1.LinodeVPC
+}
+
 // VPCScopeParams defines the input parameters used to create a new Scope.
 type VPCScopeParams struct {
 	Client    client.Client
-	LinodeVPC *infrav1.LinodeVPC
+	LinodeVPC *infrav1alpha1.LinodeVPC
 }
 
 func validateVPCScopeParams(params VPCScopeParams) error {
@@ -62,11 +74,19 @@ func NewVPCScope(apiKey string, params VPCScopeParams) (*VPCScope, error) {
 	}, nil
 }
 
-// VPCScope defines the basic context for an actuator to operate upon.
-type VPCScope struct {
-	client client.Client
+// PatchObject persists the machine configuration and status.
+func (s *VPCScope) PatchObject(ctx context.Context) error {
+	return s.PatchHelper.Patch(ctx, s.LinodeVPC)
+}
 
-	PatchHelper  *patch.Helper
-	LinodeClient *linodego.Client
-	LinodeVPC    *infrav1.LinodeVPC
+// Close closes the current scope persisting the machine configuration and status.
+func (s *VPCScope) Close(ctx context.Context) error {
+	return s.PatchObject(ctx)
+}
+
+// AddFinalizer adds a finalizer and immediately patches the object to avoid any race conditions
+func (s *VPCScope) AddFinalizer(ctx context.Context) error {
+	controllerutil.AddFinalizer(s.LinodeVPC, infrav1alpha1.GroupVersion.String())
+
+	return s.Close(ctx)
 }

controller/linodemachine_controller.go

@@ -191,6 +191,7 @@ func (r *LinodeMachineReconciler) reconcile(
 	machineScope.LinodeMachine.Status.FailureMessage = util.Pointer("")
 
 	failureReason := cerrs.MachineStatusError("UnknownError")
+	//nolint:dupl // Code duplication is simplicity in this case.
 	defer func() {
 		if err != nil {
 			machineScope.LinodeMachine.Status.FailureReason = util.Pointer(failureReason)
@@ -210,8 +211,11 @@ func (r *LinodeMachineReconciler) reconcile(
 	}()
 
 	// Add the finalizer if not already there
-	if err := machineScope.AddFinalizer(ctx); err != nil {
-		return res, err
+	err = machineScope.AddFinalizer(ctx)
+	if err != nil {
+		logger.Error(err, "Failed to add finalizer")
+
+		return
 	}
 
 	// Delete

controller/linodevpc_controller.go

@@ -36,10 +36,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/go-logr/logr"
-	infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 	"github.com/linode/cluster-api-provider-linode/cloud/scope"
 	"github.com/linode/cluster-api-provider-linode/util"
 	"github.com/linode/cluster-api-provider-linode/util/reconciler"
+
+	infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 )
 
 // LinodeVPCReconciler reconciles a LinodeVPC object
@@ -73,7 +74,7 @@ func (r *LinodeVPCReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	log := ctrl.LoggerFrom(ctx).WithName("LinodeVPCReconciler").WithValues("name", req.NamespacedName.String())
 
-	linodeVPC := &infrav1.LinodeVPC{}
+	linodeVPC := &infrav1alpha1.LinodeVPC{}
 	if err := r.Client.Get(ctx, req.NamespacedName, linodeVPC); err != nil {
 		log.Error(err, "Failed to fetch LinodeVPC")
 
@@ -107,7 +108,8 @@ func (r *LinodeVPCReconciler) reconcile(
 	vpcScope.LinodeVPC.Status.FailureReason = nil
 	vpcScope.LinodeVPC.Status.FailureMessage = util.Pointer("")
 
-	failureReason := infrav1.VPCStatusError("UnknownError")
+	failureReason := infrav1alpha1.VPCStatusError("UnknownError")
+	//nolint:dupl // Code duplication is simplicity in this case.
 	defer func() {
 		if err != nil {
 			vpcScope.LinodeVPC.Status.FailureReason = util.Pointer(failureReason)
@@ -118,7 +120,8 @@ func (r *LinodeVPCReconciler) reconcile(
 			r.Recorder.Event(vpcScope.LinodeVPC, corev1.EventTypeWarning, string(failureReason), err.Error())
 		}
 
-		if patchErr := vpcScope.PatchHelper.Patch(ctx, vpcScope.LinodeVPC); patchErr != nil && utilerrors.FilterOut(patchErr) != nil {
+		// Always close the scope when exiting this function so we can persist any LinodeMachine changes.
+		if patchErr := vpcScope.Close(ctx); patchErr != nil && utilerrors.FilterOut(patchErr) != nil {
 			logger.Error(patchErr, "failed to patch LinodeVPC")
 
 			err = errors.Join(err, patchErr)
@@ -127,18 +130,24 @@ func (r *LinodeVPCReconciler) reconcile(
 
 	// Delete
 	if !vpcScope.LinodeVPC.ObjectMeta.DeletionTimestamp.IsZero() {
-		failureReason = infrav1.DeleteVPCError
+		failureReason = infrav1alpha1.DeleteVPCError
 
 		res, err = r.reconcileDelete(ctx, logger, vpcScope)
 
 		return
 	}
 
-	controllerutil.AddFinalizer(vpcScope.LinodeVPC, infrav1.GroupVersion.String())
+	// Add the finalizer if not already there
+	err = vpcScope.AddFinalizer(ctx)
+	if err != nil {
+		logger.Error(err, "Failed to add finalizer")
+
+		return
+	}
 
 	// Update
 	if vpcScope.LinodeVPC.Spec.VPCID != nil {
-		failureReason = infrav1.UpdateVPCError
+		failureReason = infrav1alpha1.UpdateVPCError
 
 		logger = logger.WithValues("vpcID", *vpcScope.LinodeVPC.Spec.VPCID)
 
@@ -148,7 +157,7 @@ func (r *LinodeVPCReconciler) reconcile(
 	}
 
 	// Create
-	failureReason = infrav1.CreateVPCError
+	failureReason = infrav1alpha1.CreateVPCError
 
 	err = r.reconcileCreate(ctx, vpcScope, logger)
 
@@ -161,9 +170,9 @@ func (r *LinodeVPCReconciler) reconcileCreate(ctx context.Context, vpcScope *sco
 	if err := r.reconcileVPC(ctx, vpcScope, logger); err != nil {
 		logger.Error(err, "Failed to create VPC")
 
-		conditions.MarkFalse(vpcScope.LinodeVPC, clusterv1.ReadyCondition, string(infrav1.CreateVPCError), clusterv1.ConditionSeverityError, err.Error())
+		conditions.MarkFalse(vpcScope.LinodeVPC, clusterv1.ReadyCondition, string(infrav1alpha1.CreateVPCError), clusterv1.ConditionSeverityError, err.Error())
 
-		r.Recorder.Event(vpcScope.LinodeVPC, corev1.EventTypeWarning, string(infrav1.CreateVPCError), err.Error())
+		r.Recorder.Event(vpcScope.LinodeVPC, corev1.EventTypeWarning, string(infrav1alpha1.CreateVPCError), err.Error())
 
 		return err
 	}
@@ -179,9 +188,9 @@ func (r *LinodeVPCReconciler) reconcileUpdate(ctx context.Context, logger logr.L
 	if err := r.reconcileVPC(ctx, vpcScope, logger); err != nil {
 		logger.Error(err, "Failed to update VPC")
 
-		conditions.MarkFalse(vpcScope.LinodeVPC, clusterv1.ReadyCondition, string(infrav1.UpdateVPCError), clusterv1.ConditionSeverityError, err.Error())
+		conditions.MarkFalse(vpcScope.LinodeVPC, clusterv1.ReadyCondition, string(infrav1alpha1.UpdateVPCError), clusterv1.ConditionSeverityError, err.Error())
 
-		r.Recorder.Event(vpcScope.LinodeVPC, corev1.EventTypeWarning, string(infrav1.UpdateVPCError), err.Error())
+		r.Recorder.Event(vpcScope.LinodeVPC, corev1.EventTypeWarning, string(infrav1alpha1.UpdateVPCError), err.Error())
 
 		return err
 	}
@@ -241,15 +250,15 @@ func (r *LinodeVPCReconciler) reconcileDelete(ctx context.Context, logger logr.L
 	r.Recorder.Event(vpcScope.LinodeVPC, corev1.EventTypeNormal, clusterv1.DeletedReason, "VPC has cleaned up")
 
 	vpcScope.LinodeVPC.Spec.VPCID = nil
-	controllerutil.RemoveFinalizer(vpcScope.LinodeVPC, infrav1.GroupVersion.String())
+	controllerutil.RemoveFinalizer(vpcScope.LinodeVPC, infrav1alpha1.GroupVersion.String())
 
 	return res, nil
 }
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *LinodeVPCReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	_, err := ctrl.NewControllerManagedBy(mgr).
-		For(&infrav1.LinodeVPC{}).
+		For(&infrav1alpha1.LinodeVPC{}).
 		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(mgr.GetLogger(), r.WatchFilterValue)).
 		Build(r)
 	if err != nil {

controller/linodevpc_controller_helpers.go

@@ -23,10 +23,11 @@ import (
 	"errors"
 
 	"github.com/go-logr/logr"
-	infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 	"github.com/linode/cluster-api-provider-linode/cloud/scope"
 	"github.com/linode/cluster-api-provider-linode/util"
 	"github.com/linode/linodego"
+
+	infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 )
 
 func (r *LinodeVPCReconciler) reconcileVPC(ctx context.Context, vpcScope *scope.VPCScope, logger logr.Logger) error {
@@ -72,7 +73,7 @@ func (r *LinodeVPCReconciler) reconcileVPC(ctx context.Context, vpcScope *scope.
 	return nil
 }
 
-func linodeVPCSpecToVPCCreateConfig(vpcSpec infrav1.LinodeVPCSpec) *linodego.VPCCreateOptions {
+func linodeVPCSpecToVPCCreateConfig(vpcSpec infrav1alpha1.LinodeVPCSpec) *linodego.VPCCreateOptions {
 	var buf bytes.Buffer
 	enc := gob.NewEncoder(&buf)
 	err := enc.Encode(vpcSpec)

controller/linodevpc_controller_helpers_test.go

@@ -5,19 +5,20 @@ import (
 	"encoding/gob"
 	"testing"
 
-	infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1"
 )
 
 func TestLinodeVPCSpecToCreateVPCConfig(t *testing.T) {
 	t.Parallel()
 
-	vpcSpec := infrav1.LinodeVPCSpec{
+	vpcSpec := infrav1alpha1.LinodeVPCSpec{
 		Label:       "label",
 		Description: "description",
 		Region:      "region",
-		Subnets: []infrav1.VPCSubnetCreateOptions{
+		Subnets: []infrav1alpha1.VPCSubnetCreateOptions{
 			{
 				Label: "subnet",
 				IPv4:  "ipv4",
@@ -33,7 +34,7 @@ func TestLinodeVPCSpecToCreateVPCConfig(t *testing.T) {
 	err := enc.Encode(createConfig)
 	require.NoError(t, err, "Failed to encode VPCCreateOptions")
 
-	var actualVPCSpec infrav1.LinodeVPCSpec
+	var actualVPCSpec infrav1alpha1.LinodeVPCSpec
 	dec := gob.NewDecoder(&buf)
 	err = dec.Decode(&actualVPCSpec)
 	require.NoError(t, err, "Failed to decode LinodeVPCSpec")