using defer for status updates

Makefile

@@ -192,6 +192,7 @@ deploy-kind: docker-build-local manifests kustomize
 	kind load docker-image ${LOCAL_IMG}
 	kind load docker-image ${LOCAL_IMG}  # fixes an issue with podman where the image is not correctly tagged after the first kind load docker-image
 	$(KUSTOMIZE) build kind | $(KUBECTL) apply -f -
+	$(KUBECTL) rollout restart deployment/netbox-operator-controller-manager -n netbox-operator-system
 
 .PHONY: undeploy-kind
 undeploy-kind: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.

api/v1/ipaddress_types.go

@@ -76,6 +76,10 @@ type IpAddressStatus struct {
 	// URL depends on the runtime config of NetBox Operator
 	IpAddressUrl string `json:"url,omitempty"`
 
+	// Indicates if Sync with the backend was successful
+	// If connection to the backend failed but the spec did not change it is set to unknown
+	SyncState SyncState `json:"syncState,omitempty"`
+
 	// Conditions represent the latest available observations of an object's state
 	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
 }
@@ -130,9 +134,9 @@ var ConditionIpaddressReadyFalse = metav1.Condition{
 	Message: "Failed to reserve IP in NetBox",
 }
 
-var ConditionIpaddressReadyFalseDeletionFailed = metav1.Condition{
-	Type:    "Ready",
-	Status:  "False",
-	Reason:  "FailedToDeleteIpInNetbox",
-	Message: "Failed to delete IP in NetBox",
-}
+type SyncState string
+
+const (
+	SyncStateSucceeded SyncState = "Succeeded"
+	SyncStateFailed    SyncState = "Failed"
+)

config/crd/bases/netbox.dev_ipaddresses.yaml

@@ -181,6 +181,11 @@ spec:
                 description: The ID of the resource in NetBox
                 format: int64
                 type: integer
+              syncState:
+                description: |-
+                  Indicates if Sync with the backend was successful
+                  If connection to the backend failed but the spec did not change it is set to unknown
+                type: string
               url:
                 description: |-
                   The URL to the resource in the NetBox UI. Note that the base of this

internal/controller/ipaddress_controller.go

@@ -61,65 +61,67 @@ type IpAddressReconciler struct {
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
-func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (reconcileResult ctrl.Result, reconcileErr error) {
 	logger := log.FromContext(ctx)
-	debugLogger := logger.V(4)
 
 	logger.Info("reconcile loop started")
 
 	o := &netboxv1.IpAddress{}
+	conditionMessage := ""
+
 	err := r.Get(ctx, req.NamespacedName, o)
 	if err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	// cancelLock stops the lease renewal goroutine on early returns (lease expires naturally).
+	// Explicit cancelLock()+UnlockWithRetry() runs inline after the critical section.
+	var cancelLock context.CancelFunc
+
+	deferredFunc := func() {
+		if err := r.UpdateConditions(ctx, o, conditionMessage); err != nil {
+			reconcileErr = errors.Join(reconcileErr, err)
+		}
+		logger.Info("reconcile loop ended")
+	}
+	defer func() { deferredFunc() }()
+
 	// if being deleted
 	if !o.DeletionTimestamp.IsZero() {
-		if controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) {
-			if !o.Spec.PreserveInNetbox {
-				err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId)
-				if err != nil {
-					if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalseDeletionFailed, corev1.EventTypeWarning, err); errReport != nil {
-						return ctrl.Result{}, errReport
-					}
-					return ctrl.Result{Requeue: true}, nil
-				}
-			}
+		if !controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) {
+			return ctrl.Result{}, nil
+		}
 
-			debugLogger.Info("removing the finalizer")
-			removed := controllerutil.RemoveFinalizer(o, IpAddressFinalizerName)
-			if !removed {
-				return ctrl.Result{}, errors.New("failed to remove the finalizer")
+		if !o.Spec.PreserveInNetbox && o.Status.IpAddressId != 0 {
+			if err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId); err != nil {
+				conditionMessage = err.Error()
+				return ctrl.Result{Requeue: true}, nil
 			}
+		}
 
-			err = r.Update(ctx, o)
-			if err != nil {
-				return ctrl.Result{}, err
-			}
+		logger.V(4).Info("removing the finalizer")
+		removed := controllerutil.RemoveFinalizer(o, IpAddressFinalizerName)
+		if !removed {
+			return ctrl.Result{}, errors.New("failed to remove the finalizer")
+		}
+
+		if err = r.Update(ctx, o); err != nil {
+			return ctrl.Result{}, err
 		}
 
-		// end loop if deletion timestamp is not zero
 		return ctrl.Result{}, nil
 	}
 
 	// if PreserveIpInNetbox flag is false then register finalizer if not yet registered
 	if !o.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) {
-		debugLogger.Info("adding the finalizer")
+		logger.V(4).Info("adding the finalizer")
 		controllerutil.AddFinalizer(o, IpAddressFinalizerName)
 		if err := r.Update(ctx, o); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
 
-	// Set ready to false initially
-	if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil {
-		err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil)
-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err)
-		}
-	}
-
-	// 1. try to lock lease of parent prefix if IpAddress status condition is not true
+	// 1. try to lock lease of parent prefix if IpAddressUrl is not set in status
 	// and IpAddress is owned by an IpAddressClaim
 	or := o.OwnerReferences
 	var ll *leaselocker.LeaseLocker
@@ -145,19 +147,21 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			return ctrl.Result{}, err
 		}
 
-		lockCtx, cancel := context.WithCancel(ctx)
-		defer cancel()
-
-		// create lock
+		var lockCtx context.Context
+		lockCtx, cancelLock = context.WithCancel(ctx)
+		defer func() {
+			if cancelLock != nil {
+				cancelLock() // ensure renewal goroutine stops on any return path
+			}
+		}()
 		locked := ll.TryLock(lockCtx)
 		if !locked {
-			errorMsg := fmt.Sprintf("failed to lock parent prefix %s", ipAddressClaim.Spec.ParentPrefix)
-			r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg)
+			conditionMessage = fmt.Sprintf("failed to lock parent prefix %s", ipAddressClaim.Spec.ParentPrefix)
 			return ctrl.Result{
 				RequeueAfter: 2 * time.Second,
 			}, nil
 		}
-		debugLogger.Info(fmt.Sprintf("successfully locked parent prefix %s", ipAddressClaim.Spec.ParentPrefix))
+		logger.V(4).Info("successfully locked parent prefix", "prefix", ipAddressClaim.Spec.ParentPrefix)
 	}
 
 	// 2. reserve or update ip address in netbox
@@ -174,79 +178,63 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel)
 	if err != nil {
+		o.Status.SyncState = netboxv1.SyncStateFailed
 		if errors.Is(err, api.ErrRestorationHashMismatch) && o.Status.IpAddressId == 0 {
 			// if there is a restoration hash mismatch and the IpAddressId status field is not set,
 			// delete the ip address so it can be recreated by the ip address claim controller
-			// this will only affect resources that are created by a claim controller (and have a restoration hash custom field
 			logger.Info("restoration hash mismatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress)
-			err = r.Delete(ctx, o)
-			if err != nil {
-				if updateStatusErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse,
-					corev1.EventTypeWarning, err); updateStatusErr != nil {
-					return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+
-						"after deletion of ip address cr failed: %w", updateStatusErr, err)
-				}
+			if err = r.Delete(ctx, o); err != nil {
+				conditionMessage = "failed to delete IpAddress CR with restoration hash mismatch"
 				return ctrl.Result{Requeue: true}, nil
 			}
+			// cancel the deferred condition update since the object has been deleted
+			// and no longer needs status updates
+			deferredFunc = func() {
+				logger.Info("reconcile loop ended, object was deleted")
+			}
 			return ctrl.Result{}, nil
-
 		}
 
-		if updateStatusErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse,
-			corev1.EventTypeWarning, err, o.Spec.IpAddress); updateStatusErr != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+
-				"after reservation of ip in netbox failed: %w", updateStatusErr, err)
-		}
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	// 3. unlock lease of parent prefix
+	// 3. unlock lease of parent prefix — allocation is done, lock no longer needed
 	if ll != nil {
+		cancelLock()
 		ll.UnlockWithRetry(ctx)
 	}
 
-	// 4. update status fields
-	o.Status.IpAddressId = netboxIpAddressModel.ID
-	o.Status.IpAddressUrl = config.GetBaseUrl() + "/ipam/ip-addresses/" + strconv.FormatInt(netboxIpAddressModel.ID, 10)
-	err = r.Client.Status().Update(ctx, o)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
+	// 4. update annotations
 	if annotations == nil {
 		annotations = make(map[string]string, 1)
 	}
 
 	annotations[IPManagedCustomFieldsAnnotationName], err = generateManagedCustomFieldsAnnotation(o.Spec.CustomFields)
 	if err != nil {
-		logger.Error(err, "failed to update last metadata annotation")
+		logger.Error(err, "failed to generate managed custom fields annotation")
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	err = accessor.SetAnnotations(o, annotations)
-	if err != nil {
+	if err = accessor.SetAnnotations(o, annotations); err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// update object to store lastIpAddressMetadata annotation
 	if err := r.Update(ctx, o); err != nil {
 		return ctrl.Result{}, err
 	}
 
+	// 4. update status fields (set after r.Update to avoid being overwritten by API response)
+	o.Status.SyncState = netboxv1.SyncStateSucceeded
+	o.Status.IpAddressId = netboxIpAddressModel.ID
+	o.Status.IpAddressUrl = config.GetBaseUrl() + "/ipam/ip-addresses/" + strconv.FormatInt(netboxIpAddressModel.ID, 10)
+
 	// check if created ip address contains entire description from spec
 	_, found := strings.CutPrefix(netboxIpAddressModel.Description, req.String()+" // "+o.Spec.Description)
 	if !found {
 		r.EventStatusRecorder.Recorder().Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description")
 	}
 
-	debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress))
-
-	err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyTrue, corev1.EventTypeNormal, nil)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	logger.Info("reconcile loop finished")
+	logger.V(4).Info("reserved ip address in netbox", "ip", o.Spec.IpAddress)
 
 	return ctrl.Result{}, nil
 }
@@ -290,3 +278,31 @@ func generateNetboxIpAddressModelFromIpAddressSpec(spec *netboxv1.IpAddressSpec,
 		},
 	}, nil
 }
+
+func (r *IpAddressReconciler) UpdateConditions(ctx context.Context, o *netboxv1.IpAddress, conditionMessage string) error {
+	var additionalMsgs []string
+	if conditionMessage != "" {
+		additionalMsgs = []string{conditionMessage}
+	}
+
+	switch {
+	case !o.DeletionTimestamp.IsZero():
+		r.EventStatusRecorder.Report(ctx, o,
+			netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil, additionalMsgs...)
+	case o.Status.IpAddressUrl == "":
+		r.EventStatusRecorder.Report(ctx, o,
+			netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeWarning, nil, additionalMsgs...)
+	case o.Status.SyncState == netboxv1.SyncStateFailed:
+		r.EventStatusRecorder.Report(ctx, o,
+			netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeWarning, nil, additionalMsgs...)
+	default:
+		r.EventStatusRecorder.Report(ctx, o,
+			netboxv1.ConditionIpaddressReadyTrue, corev1.EventTypeNormal, nil, additionalMsgs...)
+	}
+
+	if err := r.Status().Update(ctx, o); err != nil {
+		return client.IgnoreNotFound(err)
+	}
+
+	return nil
+}

internal/controller/ipaddress_controller_test.go

@@ -24,6 +24,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	apismeta "k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -53,7 +54,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 		IpamMocksIpAddress []func(*mock_interfaces.MockIpamInterface, chan error),
 		TenancyMocks []func(*mock_interfaces.MockTenancyInterface, chan error),
 		restorationHashMismatch bool, // To check for deletion if restoration hash does not match
-		expectedConditionReady bool, // Expected state of the ConditionReady condition
+		expectedConditionReady metav1.Condition, // Expected state of the ConditionReady condition
 		expectedCRStatus netboxv1.IpAddressStatus, // Expected status of the CR
 	) {
 		By("Setting up mocks")
@@ -99,11 +100,11 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 			}, timeout, interval).Should(BeTrue())
 
 			// Now check if conditions are set as expected
-			Eventually(func() bool {
-				err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
-				return err == nil &&
-					apismeta.IsStatusConditionTrue(createdCR.Status.Conditions, netboxv1.ConditionIpaddressReadyTrue.Type) == expectedConditionReady
-			}, timeout, interval).Should(BeTrue())
+			Eventually(k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)).Should(Succeed())
+
+			Eventually(apismeta.IsStatusConditionPresentAndEqual(createdCR.Status.Conditions, expectedConditionReady.Type, expectedConditionReady.Status)).Should(BeTrue())
+
+			Eventually(apismeta.FindStatusCondition(createdCR.Status.Conditions, expectedConditionReady.Type).Reason).Should(Equal(expectedConditionReady.Reason))
 
 			// Check that the expected ip address is present in the status
 			Expect(createdCR.Status.IpAddressId).To(Equal(expectedCRStatus.IpAddressId))
@@ -130,7 +131,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 			[]func(*mock_interfaces.MockTenancyInterface, chan error){
 				mockTenancyTenancyTenantsList,
 			},
-			false, true, ExpectedIpAddressStatus),
+			false, netboxv1.ConditionIpaddressReadyTrue, ExpectedIpAddressStatus),
 		Entry("Create IpAddress CR, ip address already reserved in NetBox, preserved in netbox, ",
 			defaultIpAddressCR(true),
 			[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -140,7 +141,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 			[]func(*mock_interfaces.MockTenancyInterface, chan error){
 				mockTenancyTenancyTenantsList,
 			},
-			false, true, ExpectedIpAddressStatus),
+			false, netboxv1.ConditionIpaddressReadyTrue, ExpectedIpAddressStatus),
 		Entry("Create IpAddress CR, ip address already reserved in NetBox",
 			defaultIpAddressCR(false),
 			[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -151,7 +152,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 			[]func(*mock_interfaces.MockTenancyInterface, chan error){
 				mockTenancyTenancyTenantsList,
 			},
-			false, true, ExpectedIpAddressStatus),
+			false, netboxv1.ConditionIpaddressReadyTrue, ExpectedIpAddressStatus),
 		Entry("Create IpAddress CR, reserve or update failure",
 			defaultIpAddressCR(false),
 			[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -162,7 +163,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 			[]func(*mock_interfaces.MockTenancyInterface, chan error){
 				mockTenancyTenancyTenantsList,
 			},
-			false, false, ExpectedIpAddressFailedStatus),
+			false, netboxv1.ConditionIpaddressReadyFalse, ExpectedIpAddressFailedStatus),
 		Entry("Create IpAddress CR, restoration hash mismatch",
 			defaultIpAddressCreatedByClaim(true),
 			[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -171,6 +172,6 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
 			[]func(*mock_interfaces.MockTenancyInterface, chan error){
 				mockTenancyTenancyTenantsList,
 			},
-			true, false, nil),
+			true, nil, nil),
 	)
 })