From 7dad639ec9f3c00671e25f1f5bae050c476a047a Mon Sep 17 00:00:00 2001 From: Richard Kovacs Date: Mon, 5 Feb 2024 15:23:57 +0100 Subject: [PATCH] Use scope Close instead of patching --- cloud/scope/vpc.go | 36 ++++++++++++++---- controller/linodemachine_controller.go | 8 +++- controller/linodevpc_controller.go | 37 ++++++++++++------- controller/linodevpc_controller_helpers.go | 5 ++- .../linodevpc_controller_helpers_test.go | 9 +++-- 5 files changed, 65 insertions(+), 30 deletions(-) diff --git a/cloud/scope/vpc.go b/cloud/scope/vpc.go index 6a06de0df..a5a68c208 100644 --- a/cloud/scope/vpc.go +++ b/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) } diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index a960305ca..5657c2769 100644 --- a/controller/linodemachine_controller.go +++ b/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 diff --git a/controller/linodevpc_controller.go b/controller/linodevpc_controller.go index 439b92f7f..8beb96abc 100644 --- a/controller/linodevpc_controller.go +++ b/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,7 +250,7 @@ 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 } @@ -249,7 +258,7 @@ func (r *LinodeVPCReconciler) reconcileDelete(ctx context.Context, logger logr.L // 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 { diff --git a/controller/linodevpc_controller_helpers.go b/controller/linodevpc_controller_helpers.go index 5e9a3ff03..44d50d6ec 100644 --- a/controller/linodevpc_controller_helpers.go +++ b/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) diff --git a/controller/linodevpc_controller_helpers_test.go b/controller/linodevpc_controller_helpers_test.go index 1408200ac..c63918017 100644 --- a/controller/linodevpc_controller_helpers_test.go +++ b/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")