using defer for status and logging improvements in ipaddressclaim_controller by faebr · Pull Request #496 · netbox-community/netbox-operator

faebr · 2026-01-19T13:39:21Z

wip

…troller

internal/controller/ipaddress_controller.go

alexandernorth · 2026-01-23T09:20:39Z

internal/controller/ipaddress_controller.go

@@ -174,31 +175,24 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (

 	netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel)
 	if err != nil {


Do we not want to set a conditionMessage for this error return?

probably yes, or we could fetch the conditionMessage from the error message and set this if conditionMessage=="". What do you think @bruelea ?

Agree, the error message could be used here. Using the error would be more intuitive than setting the conditionMessage variable.

alexandernorth · 2026-01-23T09:21:42Z

internal/controller/ipaddress_controller.go

+	if o.Status.SyncState == netboxv1.SyncStateSucceeded {
+		debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress))


If we reached here, we should have succeeded right? So I think this if statement is redundant

agreed, will delete it

There were cases where this line was reached but the ip address was actually not reserved in NetBox. So the function should be refactored to avoid this case.

alexandernorth · 2026-01-23T09:25:58Z

internal/controller/ipaddress_controller.go

 	}, nil
 }
+
+func (r *IpAddressReconciler) UpdateConditions(ctx context.Context, o *netboxv1.IpAddress, msg string) error {


This is good, but might be a bit more readable using a switch statement e.g.

switch { case o.Status.IpAddressUrl == "": // code case o.Status.SyncState == netboxv1.SyncStateFailed: // more code default: // ready case }

The DeletionTimestamp check still needs to be done, but perhaps it could be inverted and have an early return? As there is only one state to go into if this is the case

alexandernorth · 2026-01-23T09:32:57Z

internal/controller/ipaddressclaim_controller.go

+		// If err is a StatusError, we've reported it in status conditions, so return nil to controller-runtime
+		// This prevents exponential backoff for user-facing errors that are already visible in status
+		// Regular errors are still returned to trigger retry with backoff
+		if err != nil && IsStatusError(err) {


Why not follow the pattern given by controller-runtime's client.IgnoreNotFound(err)?
We could then do err = IgnoreStatusError(err)

StatusError could be named a little more clearly - it seems to be errors caused when communicating with Netbox? And other errors are errors caused by issues during the reconcile loop, do I understand correctly here?

I really like the proposal in the first comment!

I agree the naming is bad, well actually this is just to define if the error should in the end be returned by the controller loop, so visible with the stacktrace in the logs or not. But basically you are right, StatusError just means the reconciliation did not fully succed but there was no error in the code. Could be claim error, no ip's left or any other error caught somehwere which is not unexpected but due to some user input or system state.

alexandernorth · 2026-01-23T09:37:58Z

internal/controller/ipaddressclaim_controller.go

 // 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 *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) {


Perhaps rename err to resultErr as in this function err is shadowed in some code paths which makes it harder to see if this is what will be returned, or if it's just temporary

alexandernorth · 2026-01-23T09:40:14Z

internal/controller/ipaddressclaim_controller.go

+// updateStatus updates the IpAddressClaim status based on the current state of the owned IpAddress.
+// This function is called as a deferred function in Reconcile to ensure status is always updated.
+// It captures any reconcile errors to include them in the status condition message.
+func (r *IpAddressClaimReconciler) updateStatus(ctx context.Context, claim *netboxv1.IpAddressClaim, lookupKey types.NamespacedName, reconcileErr error, debugLogger logr.Logger) error {


We should decide if we never update in case of an error in this function (current state). The alternative is to call Update even if we fail setting some of these statuses. Not saying this is wrong, but we should consider which way we want to go with this

mhm good point, what do you think about adding a second defer function which only calls the Status().Update()? :D

I checked the code of the EventStatusRecoder again, it Report() always returns nil because the Status().Update() was removed to the updateStatus function and the return variable should be removed too.
The only error here would be if the client fails to the the IpAddress CR, in that case the status conditions should still be updated.

alexandernorth · 2026-01-23T09:42:37Z

internal/controller/ipaddressclaim_controller.go

+					return fmt.Errorf("failed to report IpAssigned false condition: %w", err)
+				}
+			} else {
+				// No error and no IpAddress - this shouldn't happen in normal flow, skip update


Is this case not something we would want to report to the user somehow?
Also, this currently would skip even setting the initial ready condition from the top of this function (in case it wasn't present) making the user not see any progress, unless they have debug logging switched on and checked the operator logs

Ah these status conditions always become complexe, but good point, I will actually remove this if condition

alexandernorth · 2026-01-23T12:17:35Z

internal/controller/ipaddressclaim_controller.go

+	}
+
+	// IpAddress exists - report successful IP assignment if not already reported
+	if apismeta.FindStatusCondition(claim.Status.Conditions, netboxv1.ConditionIpAssignedTrue.Type) == nil {


This only checks for a Status Condition with the type IPAssigned, not checking if it is true - is that the intent?
And what if we don't assign an IP? Don't we want to set AssignedFalse here?

True this should check if it is true and set to true if not.
So the idea is that if the ipAddress cr does not exist, this is cought further up in the
/ Fetch the latest IpAddress object ipAddress := &netboxv1.IpAddress{} if err := r.Client.Get(ctx, lookupKey, ipAddress); err != nil { if apierrors.IsNotFound(err) {

alexandernorth · 2026-01-23T12:19:22Z

internal/controller/ipaddressclaim_controller.go

+	}
+
+	// Update status based on IpAddress readiness
+	if apismeta.IsStatusConditionTrue(ipAddress.Status.Conditions, "Ready") {


Should this use the defined constants as in the other if statements?

The idea here is to check if the ipAddress CR which was generated from this claim has the ready condition set to true. If so, then the claim can also be set to true. Or am I missing something here

bruelea · 2026-01-30T15:14:51Z

internal/controller/ipaddress_controller.go

@@ -174,31 +175,24 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (

 	netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel)
 	if err != nil {


Agree, the error message could be used here. Using the error would be more intuitive than setting the conditionMessage variable.

bruelea · 2026-01-30T15:18:13Z

internal/controller/ipaddress_controller.go

+	if o.Status.SyncState == netboxv1.SyncStateSucceeded {
+		debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress))


There were cases where this line was reached but the ip address was actually not reserved in NetBox. So the function should be refactored to avoid this case.

bruelea · 2026-01-30T15:22:35Z

internal/controller/ipaddressclaim_controller.go

+func (r *IpAddressClaimReconciler) updateStatus(ctx context.Context, claim *netboxv1.IpAddressClaim, lookupKey types.NamespacedName, reconcileErr error, debugLogger logr.Logger) error {
+	debugLogger.Info("updating ipaddressclaim status")
+
+	// Initialize status conditions if this is a new resource


Can't the initialization be skipped now if the status and Ready Condition is updated with the defer function?

yes good point, not needed :)

bruelea · 2026-01-30T15:24:45Z

internal/controller/ipaddressclaim_controller.go

+// updateStatus updates the IpAddressClaim status based on the current state of the owned IpAddress.
+// This function is called as a deferred function in Reconcile to ensure status is always updated.
+// It captures any reconcile errors to include them in the status condition message.
+func (r *IpAddressClaimReconciler) updateStatus(ctx context.Context, claim *netboxv1.IpAddressClaim, lookupKey types.NamespacedName, reconcileErr error, debugLogger logr.Logger) error {


I checked the code of the EventStatusRecoder again, it Report() always returns nil because the Status().Update() was removed to the updateStatus function and the return variable should be removed too.
The only error here would be if the client fails to the the IpAddress CR, in that case the status conditions should still be updated.

bruelea · 2026-03-06T08:34:18Z

internal/controller/ipaddressclaim_controller.go

+// Status updates the IpAddressClaim status based on the current state of the owned IpAddress.
+// This function is called as a deferred function in Reconcile to ensure status is always updated.
+// It captures any reconcile errors to include them in the status condition message.
+func (r *IpAddressClaimReconciler) updateStatus(ctx context.Context, claim *netboxv1.IpAddressClaim, lookupKey types.NamespacedName, reconcileRes ctrl.Result, reconcileErr error, logger logr.Logger) (result ctrl.Result, err error) {


Instead of adding the logger as input variable a new logger could be created inside of the function from the context logger := log.FromContext(ctx).

bruelea · 2026-03-06T08:38:50Z

internal/controller/ipaddressclaim_controller.go

+// It captures any reconcile errors to include them in the status condition message.
+func (r *IpAddressClaimReconciler) updateStatus(ctx context.Context, claim *netboxv1.IpAddressClaim, lookupKey types.NamespacedName, reconcileRes ctrl.Result, reconcileErr error, logger logr.Logger) (result ctrl.Result, err error) {
+	// Set default return values
+	result = reconcileRes


Can this be omitted by using the same variable names for the input and output variables?

no this does not work sadly, there is a compilation error if the input and output are named the same

internal/controller/ipaddressclaim_controller.go

Co-authored-by: bruelea <166021996+bruelea@users.noreply.github.com>

… for so long

bruelea

For the e2e tests we could switch to only test the event type, reason and soon also the action. This should be enough information to check if the expected event was created. If needed there can be an additional check to see if the message contains a certain string like for example the ip address that was claimed.

bruelea · 2026-03-10T17:25:20Z

internal/controller/ipaddress_controller.go

-				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 {
+				return ctrl.Result{Requeue: true}, NewDomainError("failed to delete ip address from netbox: %w", err)


In case there's an error can the responsibility to set the reconcileResult to ctrl.Result{Requeue: true} be move to the r.updateStatus() method? Then it only needs to be set in the code if there should be a specific requeue time, for example if the lease could not be locked.

bruelea · 2026-03-10T17:28:19Z

internal/controller/ipaddress_controller.go

+
+	// 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


can this be moved to line 150?

bruelea · 2026-03-10T17:42:19Z

internal/controller/ipaddress_controller.go

-	if err != nil {
-		return ctrl.Result{}, err
-	}
+	logger.V(4).Info("reserved ip address in netbox", "ip", o.Spec.IpAddress)


This line is also reached if the ip address was only updated. Can this log message be removed?

bruelea · 2026-03-10T17:42:22Z

internal/controller/ipaddress_controller.go

-	}
+	logger.V(4).Info("reserved ip address in netbox", "ip", o.Spec.IpAddress)

 	logger.Info("reconcile loop finished")


Should this be part of the defer function too? Otherwise it could also be removed.

bruelea · 2026-03-10T17:49:05Z

internal/controller/ipaddressclaim_controller.go

 	}

-	err = r.Get(ctx, ipAddressLookupKey, ipAddress)
+	err := r.Get(ctx, ipAddressLookupKey, ipAddress)


Suggested change

err := r.Get(ctx, ipAddressLookupKey, ipAddress)

err = r.Get(ctx, ipAddressLookupKey, ipAddress)

To avoid shadowing of the error which was created before. There are other locations in the code where this change could be applied too.

bruelea · 2026-03-10T17:51:08Z

internal/controller/ipaddressclaim_controller.go

 		if !locked {
 			// lock for parent prefix was not available, rescheduling
 			errorMsg := fmt.Sprintf("failed to lock parent prefix %s", o.Spec.ParentPrefix)
 			r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg)


Suggested change

r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg)

Creating the event should be covered by the defer now.

bruelea · 2026-03-10T18:09:22Z

internal/controller/ipaddress_controller.go

+			netboxv1.ConditionIpaddressReadyTrue, corev1.EventTypeNormal, nil)
+	}
+
+	if apierrors.IsConflict(err) {


If the update of the annotation was moved to the defer statement too. Could it be updated with the same request to avoid conflicts?

bruelea · 2026-03-10T18:11:42Z

internal/controller/iprange_controller.go

 				err := r.NetboxClient.DeleteIpRange(ctx, int32(o.Status.IpRangeId))
 				if err != nil {
-					err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,
+					r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,


Suggested change

r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,

bruelea · 2026-03-10T18:12:19Z

internal/controller/iprange_controller.go

 				if err != nil {
-					err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,
+					r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,
 						corev1.EventTypeWarning, err)


Suggested change

corev1.EventTypeWarning, err)

corev1.EventTypeWarning, err)

if err != nil {

return ctrl.Result{}, NewDomainErr(err)

}

bruelea · 2026-03-10T18:18:58Z

internal/controller/iprange_controller.go

+		r.EventStatusRecorder.Report(ctx, o,
+			netboxv1.ConditionIpRangeReadyFalse, corev1.EventTypeWarning, reconcileErr)
+	default:
+		r.EventStatusRecorder.Report(ctx, o,


The iprange could become Ready=false again if the update request fails. If the default case is reached the ready condition can be set to true if the reconcileErr is nil or otherwise to false.

faebr added 2 commits January 19, 2026 14:37

using defer for status and logging improvements in ipaddressclaim_con…

71c7422

…troller

use statusError type to set condition in deffered funciton

3ed5559

faebr force-pushed the feature/defer-for-status branch from 386f5a1 to 3ed5559 Compare January 19, 2026 23:48

bruelea added 4 commits January 20, 2026 00:55

add function to update status condition

baf6142

remove status updates from reconcile function

bbfd008

remove status update from eventStatusRecorcer

6d56030

fix typo in type definition

e1b744b

alexandernorth reviewed Jan 23, 2026

View reviewed changes

bruelea reviewed Jan 30, 2026

View reviewed changes

bruelea and others added 14 commits January 30, 2026 17:00

remove error from Report function

b3d40e2

refactor claim defer closure

14f39ac

implement more feedback, deefercetption

f83ccab

ipaddress and ipaddressclaim fixes

73b27ba

Merge branch 'main' into feature/defer-for-status

c1543bf

linter fixes

7f4b9a6

removing unneeded code and minor issues fix

69f97ff

Merge branch 'main' into feature/defer-for-status

e4cf55b

defer leaselock order inversed

736bc52

add the status.update back in the report for the other controllers

24759f6

fix updating outdated resource and rolout on kind-deploy

5fbf243

fixes after integratino-tests

1504851

typo fix and code simplification

a5cb809

remove unneeded status condition initialisation

840bbb4

faebr marked this pull request as ready for review March 5, 2026 10:43

Merge branch 'main' into feature/defer-for-status

adb449e

bruelea reviewed Mar 6, 2026

View reviewed changes

faebr and others added 4 commits March 6, 2026 10:12

Update internal/controller/ipaddressclaim_controller.go

dfaba79

Co-authored-by: bruelea <166021996+bruelea@users.noreply.github.com>

implement feedback for logger

2452bd4

allign ipaddress controller with ipaddressclaim

3dcb748

align other controllers to use defer

9c15e3e

faebr and others added 6 commits March 6, 2026 14:33

review fixes

f889864

decrese leaselocker timeout so failing locks don't block the operator…

4db8531

… for so long

using patch to update the resoure

5ff6e22

Merge branch 'main' into feature/defer-for-status

ba3cfd5

flatten defer in non-claim controllers

5ea40b1

Merge branch 'main' into feature/defer-for-status

968c81c

bruelea requested changes Mar 10, 2026

View reviewed changes

		@@ -174,31 +175,24 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (

		netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel)
		if err != nil {

		if o.Status.SyncState == netboxv1.SyncStateSucceeded {
		debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress))

	err := r.Get(ctx, ipAddressLookupKey, ipAddress)
	err = r.Get(ctx, ipAddressLookupKey, ipAddress)

Conversation

faebr commented Jan 19, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bruelea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!