Skip to content

Remove ContainsFinalizer check before adding it ✨🐛 #4789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove ContainsFinalizer check before adding it ✨🐛 #4789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions docs/book/src/cronjob-tutorial/testdata/finalizer_example.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ func (r *CronJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
// The object is not being deleted, so if it does not have our finalizer,
// then let's add the finalizer and update the object. This is equivalent
// to registering our finalizer.
if !controllerutil.ContainsFinalizer(cronJob, myFinalizerName) {
controllerutil.AddFinalizer(cronJob, myFinalizerName)
if added := controllerutil.AddFinalizer(cronJob, myFinalizerName); added {
if err := r.Update(ctx, cronJob); err != nil {
return ctrl.Result{}, err
}
Expand Down
10 changes: 2 additions & 8 deletions ...ugins/golang/deploy-image/v1alpha1/scaffolds/internal/templates/controllers/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,8 @@ func (r *{{ .Resource.Kind }}Reconciler) Reconcile(ctx context.Context, req ctrl
// Let's add a finalizer. Then, we can define some operations which should
// occur before the custom resource is deleted.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers
if !controllerutil.ContainsFinalizer({{ lower .Resource.Kind }}, {{ lower .Resource.Kind }}Finalizer) {
log.Info("Adding Finalizer for {{ .Resource.Kind }}")
if ok := controllerutil.AddFinalizer({{ lower .Resource.Kind }}, {{ lower .Resource.Kind }}Finalizer); !ok {
err = fmt.Errorf("finalizer for {{ .Resource.Kind }} was not added")
log.Error(err, "Failed to add finalizer for {{ .Resource.Kind }}")
return ctrl.Result{}, err
}

if added := controllerutil.AddFinalizer({{ lower .Resource.Kind }}, {{ lower .Resource.Kind }}Finalizer); added {
log.Info("Finalizer added for {{ .Resource.Kind }}")
if err = r.Update(ctx, {{ lower .Resource.Kind }}); err != nil {
log.Error(err, "Failed to update custom resource to add finalizer")
return ctrl.Result{}, err
Expand Down
10 changes: 2 additions & 8 deletions testdata/project-v4-multigroup/internal/controller/example.com/busybox_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,8 @@ func (r *BusyboxReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
// Let's add a finalizer. Then, we can define some operations which should
// occur before the custom resource is deleted.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers
if !controllerutil.ContainsFinalizer(busybox, busyboxFinalizer) {
log.Info("Adding Finalizer for Busybox")
if ok := controllerutil.AddFinalizer(busybox, busyboxFinalizer); !ok {
err = fmt.Errorf("finalizer for Busybox was not added")
log.Error(err, "Failed to add finalizer for Busybox")
return ctrl.Result{}, err
}

if added := controllerutil.AddFinalizer(busybox, busyboxFinalizer); added {
log.Info("Finalizer added for Busybox")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the suggestion and for taking the time to improve the code! 🙏

The reason for keeping the original implementation as it is, is because:

Separation of concerns:
We first check if the finalizer is already present using ContainsFinalizer, and only attempt to add it if necessary. This makes the logic clear and avoids unnecessary operations.

Efficient error handling:
By explicitly checking first, we can log and manage errors more precisely at each step, which makes debugging and monitoring easier.

Avoid unnecessary API server writes:
Updating the resource only when a change is needed helps reduce API server load, avoids unnecessary reconciliation retries, and keeps the controller efficient.

Even though AddFinalizer internally checks, doing the explicit check first improves clarity, and bring the above benefits which seems more aligned.

For these reasons, I think it might be better to keep the original implementation as it is.

Thanks again for your contribution and for sparking this valuable discussion! 🌟

Copy link
Member

@camilamacedo86 camilamacedo86 Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

If you do not mind, I think we should close this one as not accepted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick and thorough review
I agree that using ContainsFinalizer and AddFinalizer together improves readability. However, I still believe the original code block is misleading and can be improved. It appears to contain dead code within the conditional block:

		if ok := controllerutil.AddFinalizer({{ lower .Resource.Kind }}, {{ lower .Resource.Kind }}Finalizer); !ok {
			err = fmt.Errorf("finalizer for {{ .Resource.Kind }} was not added")
			log.Error(err, "Failed to add finalizer for {{ .Resource.Kind }}")
			return ctrl.Result{}, err
		}

Since the presence of the finalizer is checked before this block (by ContainsFinalizer), the condition where AddFinalizer returns false (and thus the error logging) will never be reached. Examining the implementation of AddFinalizer confirms this, as it only appends the finalizer and returns true if it wasn't already present.

My proposal aims for a more precise approach:

	if !controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) {
		log.Info("Adding Finalizer for Memcached")
		controllerutil.AddFinalizer(memcached, memcachedFinalizer)
		if err = r.Update(ctx, memcached); err != nil {
			log.Error(err, "Failed to update custom resource to add finalizer")
			return ctrl.Result{}, err
		}
	}

This revised code explicitly checks for the finalizer's absence, logs the intention to add it, performs the addition, and then handles potential errors during the update operation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camilamacedo86 Would you agree with this approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @matrus2

Thank you for looking into that.
Your observation seems to be accurate, and your proposal seems to solve it well. 👍

Could you please update this PR with the proposal?
Also, please ensure that you squash the commits for we have a good git history.

Thank you a lot 🥇

if err = r.Update(ctx, busybox); err != nil {
log.Error(err, "Failed to update custom resource to add finalizer")
return ctrl.Result{}, err
Expand Down
10 changes: 2 additions & 8 deletions testdata/project-v4-multigroup/internal/controller/example.com/memcached_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,8 @@ func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// Let's add a finalizer. Then, we can define some operations which should
// occur before the custom resource is deleted.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers
if !controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) {
log.Info("Adding Finalizer for Memcached")
if ok := controllerutil.AddFinalizer(memcached, memcachedFinalizer); !ok {
err = fmt.Errorf("finalizer for Memcached was not added")
log.Error(err, "Failed to add finalizer for Memcached")
return ctrl.Result{}, err
}

if added := controllerutil.AddFinalizer(memcached, memcachedFinalizer); added {
log.Info("Finalizer added for Memcached")
if err = r.Update(ctx, memcached); err != nil {
log.Error(err, "Failed to update custom resource to add finalizer")
return ctrl.Result{}, err
Expand Down
10 changes: 2 additions & 8 deletions testdata/project-v4-with-plugins/internal/controller/busybox_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,8 @@ func (r *BusyboxReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
// Let's add a finalizer. Then, we can define some operations which should
// occur before the custom resource is deleted.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers
if !controllerutil.ContainsFinalizer(busybox, busyboxFinalizer) {
log.Info("Adding Finalizer for Busybox")
if ok := controllerutil.AddFinalizer(busybox, busyboxFinalizer); !ok {
err = fmt.Errorf("finalizer for Busybox was not added")
log.Error(err, "Failed to add finalizer for Busybox")
return ctrl.Result{}, err
}

if added := controllerutil.AddFinalizer(busybox, busyboxFinalizer); added {
log.Info("Finalizer added for Busybox")
if err = r.Update(ctx, busybox); err != nil {
log.Error(err, "Failed to update custom resource to add finalizer")
return ctrl.Result{}, err
Expand Down
10 changes: 2 additions & 8 deletions testdata/project-v4-with-plugins/internal/controller/memcached_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,8 @@ func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// Let's add a finalizer. Then, we can define some operations which should
// occur before the custom resource is deleted.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers
if !controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) {
log.Info("Adding Finalizer for Memcached")
if ok := controllerutil.AddFinalizer(memcached, memcachedFinalizer); !ok {
err = fmt.Errorf("finalizer for Memcached was not added")
log.Error(err, "Failed to add finalizer for Memcached")
return ctrl.Result{}, err
}

if added := controllerutil.AddFinalizer(memcached, memcachedFinalizer); added {
log.Info("Finalizer added for Memcached")
if err = r.Update(ctx, memcached); err != nil {
log.Error(err, "Failed to update custom resource to add finalizer")
return ctrl.Result{}, err
Expand Down
Loading