Finalizer example possible bug

[lorenzofelletti]

The finalizer example in the book performs a r.Update(ctx, cronJob) here, but does not requeue in case the update is successful.

Since the code roughly does

func (r *CronJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	//...

	if cronJob.ObjectMeta.DeletionTimestamp.IsZero() {
		if !controllerutil.ContainsFinalizer(cronJob, myFinalizerName) {
			controllerutil.AddFinalizer(cronJob, myFinalizerName)
			if err := r.Update(ctx, cronJob); err != nil {
				return ctrl.Result{}, err
			}
		}
	}

	// reconciliation logic
}

Wouldn't that code as is cause an error when the reconciliation logic attempt to update the status (e.g. here)?
Shouldn't it be better to return ctrl.Result{Requeue: true}, nil on success, so that the finalizer is added and the reconciler immediately retriggered (thus enabling us to fetch the latest version of the resource)?

[camilamacedo86]

Hi @lorenzofelletti,

The best approach would be to refactor the code of the CronJob and MultiVersion samples to follow the structure generated by the DeployImage plugin (which scaffolds the code to manage an image provided via the command line).

Example:

• Controller Implementation: busybox_controller.go
• Controller Test: busybox_controller_test.go

The best practice is to fetch the resource before updating it. This avoids scenarios where an update fails due to an intermediate change, which could cause the reconciliation process to restart. Refer to the example below for guidance:

kubebuilder/testdata/project-v4-with-plugins/internal/controller/busybox_controller.go

Lines 101 to 118 in 5f8342e

	// Let's just set the status as Unknown when no status is available
	if busybox.Status.Conditions == nil || len(busybox.Status.Conditions) == 0 {
	meta.SetStatusCondition(&busybox.Status.Conditions, metav1.Condition{Type: typeAvailableBusybox, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: "Starting reconciliation"})
	if err = r.Status().Update(ctx, busybox); err != nil {
	log.Error(err, "Failed to update Busybox status")
	return ctrl.Result{}, err
	}
	// Let's re-fetch the busybox Custom Resource after updating the status
	// so that we have the latest state of the resource on the cluster and we will avoid
	// raising the error "the object has been modified, please apply
	// your changes to the latest version and try again" which would re-trigger the reconciliation
	// if we try to update it again in the following operations
	if err := r.Get(ctx, req.NamespacedName, busybox); err != nil {
	log.Error(err, "Failed to re-fetch busybox")
	return ctrl.Result{}, err
	}
	}

To align with the above examples, we have indeed some refactoring tasks:

• kubebuilder/issues/4140
• kubebuilder/issues/4019

If you wish to work on and help us improve the CronJob and MultiVersion in order of them be exactly the same controller and controller test scaffold done by DeployImage plugin with only the required logic for this example on top that would be great. Help is more than welcome.

[camilamacedo86]

Furthermore we created the issue : #4291
So, I am closing this one and I hope that you do not mind.

If you see that is required please feel free to re-open this one.