Skip to content

[ENHANCEMENT] remove unnecessary finalizers in favor of existing owner refereces #400

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

Merged
jgbernalp merged 1 commit into from
May 7, 2026
Merged

[ENHANCEMENT] remove unnecessary finalizers in favor of existing owner refereces #400

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
98 changes: 18 additions & 80 deletions controllers/perses/perses_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func (r *PersesReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
subreconcilersForPerses := []subreconciler.FnWithRequest{
r.handleDelete,
r.setStatusToUnknown,
r.addFinalizer,
r.removeFinalizer,
r.validateVolumes,
r.reconcileProvisioning,
r.reconcileService,
Expand Down Expand Up @@ -216,37 +216,34 @@ func (r *PersesReconciler) setStatusToUnknown(ctx context.Context, req ctrl.Requ
}

if needsRequeue {
// requeue after adding unknown status to allow adding the finalizer to succeed
// see explanation on setting a status on creation here
// https://github.com/kubernetes-sigs/controller-runtime/blob/1dce6213f6c078f3170921b3a774304d066d5bd4/pkg/controller/controllerutil/controllerutil.go#L378
// requeue after adding unknown status to allow the initial reconciliation to proceed
return subreconciler.RequeueWithDelay(time.Second * 5)
}

return subreconciler.ContinueReconciling()
}

func (r *PersesReconciler) addFinalizer(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) {
// removeFinalizer strips the legacy finalizer from existing resources.
// Owner references handle child resource cleanup via cascading deletion,
// so the finalizer is unnecessary. This migration step ensures resources
// upgraded from earlier versions can be deleted cleanly.
func (r *PersesReconciler) removeFinalizer(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) {
perses, ok := persesFromContext(ctx)
if !ok {
log.Error("perses not found in context")
return subreconciler.RequeueWithError(fmt.Errorf("perses not found in context"))
}

// Let's add a finalizer. Then, we can define some operations which should
// occurs before the custom resource to be deleted.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers
if !controllerutil.ContainsFinalizer(perses, common.PersesFinalizer) {
log.Info("Adding Finalizer for Perses")
if controllerutil.ContainsFinalizer(perses, common.PersesFinalizer) {
log.Info("Removing legacy finalizer from Perses resource")

err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
// Re-fetch the perses Custom Resource before update
// so that we have the latest state of the resource on the cluster
if err := r.Get(ctx, req.NamespacedName, perses); err != nil {
return err
}

if ok := controllerutil.AddFinalizer(perses, common.PersesFinalizer); !ok {
return fmt.Errorf("failed to add finalizer into the custom resource")
if ok := controllerutil.RemoveFinalizer(perses, common.PersesFinalizer); !ok {
return fmt.Errorf("failed to remove finalizer from the custom resource")
}

return r.Update(ctx, perses)
Expand All @@ -257,7 +254,7 @@ func (r *PersesReconciler) addFinalizer(ctx context.Context, req ctrl.Request) (
log.Info("perses resource not found. Ignoring since object must be deleted")
return subreconciler.DoNotRequeue()
}
log.WithError(err).Error("Failed to update custom resource to add finalizer")
log.WithError(err).Error("Failed to remove legacy finalizer from custom resource")
return subreconciler.RequeueWithError(err)
}
}
Expand All @@ -272,88 +269,29 @@ func (r *PersesReconciler) handleDelete(ctx context.Context, req ctrl.Request) (
return subreconciler.RequeueWithError(fmt.Errorf("perses not found in context"))
}

// Check if the Perses instance is marked to be deleted, which is
// indicated by the deletion timestamp being set.
isPersesMarkedToBeDeleted := perses.GetDeletionTimestamp() != nil
if isPersesMarkedToBeDeleted {
if perses.GetDeletionTimestamp() != nil {
// Strip the legacy finalizer if still present so the resource is not stuck.
if controllerutil.ContainsFinalizer(perses, common.PersesFinalizer) {
log.Info("Performing Finalizer Operations for Perses before delete CR")

// Set status to indicate finalization is in progress
_, err := r.updatePersesStatus(ctx, req, func(p *v1alpha2.Perses) {
meta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{
Type: common.TypeDegradedPerses, Status: metav1.ConditionUnknown,
Reason: "Finalizing", Message: fmt.Sprintf("Performing finalizer operations for the custom resource: %s ", p.Name)})
})
if err != nil {
return subreconciler.RequeueWithError(err)
}

// Perform all operations required before remove the finalizer and allow
// the Kubernetes API to remove the custom resource.
r.doFinalizerOperationsForPerses(perses)

// TODO(user): If you add operations to the doFinalizerOperationsForPerses method
// then you need to ensure that all worked fine before deleting and updating the Downgrade status
// otherwise, you should requeue here.

// Update status to indicate finalization is complete
_, err = r.updatePersesStatus(ctx, req, func(p *v1alpha2.Perses) {
meta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{
Type: common.TypeDegradedPerses, Status: metav1.ConditionTrue,
Reason: "Finalizing", Message: fmt.Sprintf("Finalizer operations for custom resource %s name were successfully accomplished", p.Name)})
})
if err != nil {
return subreconciler.RequeueWithError(err)
}

// Remove finalizer in a separate retry block
err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
if err := r.Get(ctx, req.NamespacedName, perses); err != nil {
return err
}

log.Info("Removing Finalizer for Perses after successfully perform the operations")
if ok := controllerutil.RemoveFinalizer(perses, common.PersesFinalizer); !ok {
return fmt.Errorf("failed to remove finalizer for Perses")
}

controllerutil.RemoveFinalizer(perses, common.PersesFinalizer)
return r.Update(ctx, perses)
})

if err != nil {
log.WithError(err).Error("Failed to remove finalizer for Perses")
log.WithError(err).Error("Failed to remove legacy finalizer during deletion")
return subreconciler.RequeueWithError(err)
}
}

log.Info("Perses resource is being deleted, skipping reconciliation")
return subreconciler.DoNotRequeue()
}

return subreconciler.ContinueReconciling()
}

func (r *PersesReconciler) doFinalizerOperationsForPerses(perses *v1alpha2.Perses) {
// TODO(user): Add the cleanup steps that the operator
// needs to do before the CR can be deleted. Examples
// of finalizers include performing backups and deleting
// resources that are not owned by this CR, like a PVC.

// Note: It is not recommended to use finalizers with the purpose of delete resources which are
// created and managed in the reconciliation. These ones, such as the Deployment created on this reconcile,
// are defined as depended of the custom resource. See that we use the method ctrl.SetControllerReference.
// to set the ownerRef which means that the Deployment will be deleted by the Kubernetes API.
// More info: https://kubernetes.io/docs/tasks/administer-cluster/use-cascading-deletion/

// The following implementation will raise an event
if r.Recorder != nil {
r.Recorder.Event(perses, "Warning", "Deleting",
fmt.Sprintf("Custom Resource %s is being deleted from the namespace %s",
perses.Name,
perses.Namespace))
}
}

func (r *PersesReconciler) validateVolumes(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) {
perses, ok := persesFromContext(ctx)
if !ok {
Expand Down
80 changes: 22 additions & 58 deletions controllers/perses_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,19 +271,6 @@ var _ = Describe("Perses controller", func() {
return k8sClient.Get(ctx, configMapNamespaceName, found)
}, time.Minute, time.Second).Should(Succeed())

By("Checking the latest Status Condition added to the Perses instance")
Eventually(func() error {
if len(perses.Status.Conditions) != 0 {
latestStatusCondition := perses.Status.Conditions[len(perses.Status.Conditions)-1]
expectedLatestStatusCondition := metav1.Condition{Type: common.TypeAvailablePerses,
Status: metav1.ConditionTrue, Reason: "Finalizing",
Message: fmt.Sprintf("Finalizer operations for custom resource %s name were successfully accomplished", perses.Name)}
if latestStatusCondition != expectedLatestStatusCondition {
return fmt.Errorf("The latest status condition added to the perses instance is not as expected")
}
}
return nil
}, time.Minute, time.Second).Should(Succeed())
})

It("should successfully reconcile a custom resource for Perses with storage", func() {
Expand Down Expand Up @@ -415,19 +402,6 @@ var _ = Describe("Perses controller", func() {
return k8sClient.Get(ctx, configMapNamespaceName, found)
}, time.Minute, time.Second).Should(Succeed())

By("Checking the latest Status Condition added to the Perses instance")
Eventually(func() error {
if len(perses.Status.Conditions) != 0 {
latestStatusCondition := perses.Status.Conditions[len(perses.Status.Conditions)-1]
expectedLatestStatusCondition := metav1.Condition{Type: common.TypeAvailablePerses,
Status: metav1.ConditionTrue, Reason: "Finalizing",
Message: fmt.Sprintf("Finalizer operations for custom resource %s name were successfully accomplished", perses.Name)}
if latestStatusCondition != expectedLatestStatusCondition {
return fmt.Errorf("The latest status condition added to the perses instance is not as expected")
}
}
return nil
}, time.Minute, time.Second).Should(Succeed())
})

It("should successfully reconcile a custom resource for Perses with provisioning secrets", func() {
Expand Down Expand Up @@ -574,15 +548,16 @@ var _ = Describe("Perses controller", func() {
Expect(k8sClient.Delete(ctx, secret)).To(Succeed())
})

It("should successfully delete Perses and remove the finalizer", func() {
PersesDeleteName := "perses-delete-test"
typeNamespaceName := types.NamespacedName{Name: PersesDeleteName, Namespace: persesNamespace}
It("should strip a legacy finalizer from an existing Perses resource during reconciliation", func() {
PersesFinalizerName := "perses-finalizer-migration-test"
typeNamespaceName := types.NamespacedName{Name: PersesFinalizerName, Namespace: persesNamespace}

By("Creating the custom resource for the Kind Perses")
By("Creating a Perses resource with a pre-existing legacy finalizer")
perses := &persesv1alpha2.Perses{
ObjectMeta: metav1.ObjectMeta{
Name: PersesDeleteName,
Namespace: persesNamespace,
Name: PersesFinalizerName,
Namespace: persesNamespace,
Finalizers: []string{common.PersesFinalizer},
},
Spec: persesv1alpha2.PersesSpec{
Image: ptr.To(persesImage),
Expand All @@ -600,57 +575,46 @@ var _ = Describe("Perses controller", func() {
err := k8sClient.Create(ctx, perses)
Expect(err).To(Not(HaveOccurred()))

By("Checking if the custom resource was successfully created")
Eventually(func() error {
By("Verifying the finalizer is present before reconciliation")
Eventually(func() bool {
found := &persesv1alpha2.Perses{}
return k8sClient.Get(ctx, typeNamespaceName, found)
}, time.Minute, time.Second).Should(Succeed())
if err := k8sClient.Get(ctx, typeNamespaceName, found); err != nil {
return false
}
return slices.Contains(found.Finalizers, common.PersesFinalizer)
}, time.Minute, time.Second).Should(BeTrue())

persesReconciler := &persescontroller.PersesReconciler{
Client: k8sClient,
APIReader: k8sClient,
Scheme: k8sClient.Scheme(),
}

By("Reconciling to add the finalizer")
By("Reconciling to trigger legacy finalizer removal")
Eventually(func() error {
_, err := persesReconciler.Reconcile(ctx, reconcile.Request{
NamespacedName: typeNamespaceName,
})
return err
}, time.Second*10, time.Millisecond*250).Should(Succeed())

By("Checking if the finalizer was added")
By("Checking the legacy finalizer was removed")
Eventually(func() bool {
found := &persesv1alpha2.Perses{}
err := k8sClient.Get(ctx, typeNamespaceName, found)
if err != nil {
if err := k8sClient.Get(ctx, typeNamespaceName, found); err != nil {
return false
}
return slices.Contains(found.Finalizers, common.PersesFinalizer)
}, time.Minute, time.Second).Should(BeTrue())

By("Deleting the custom resource")
persesToDelete := &persesv1alpha2.Perses{}
err = k8sClient.Get(ctx, typeNamespaceName, persesToDelete)
Expect(err).To(Not(HaveOccurred()))
err = k8sClient.Delete(ctx, persesToDelete)
Expect(err).To(Not(HaveOccurred()))
return !slices.Contains(found.Finalizers, common.PersesFinalizer)
}, time.Minute, time.Second).Should(BeTrue(), "Legacy finalizer should be removed after reconciliation")

By("Reconciling after deletion to trigger finalizer removal")
Eventually(func() error {
_, err := persesReconciler.Reconcile(ctx, reconcile.Request{
NamespacedName: typeNamespaceName,
})
return err
}, time.Second*10, time.Millisecond*250).Should(Succeed())
By("Deleting the custom resource without getting stuck")
Expect(k8sClient.Delete(ctx, perses)).To(Succeed())

By("Checking if the Perses resource was fully deleted (finalizer removed)")
Eventually(func() bool {
found := &persesv1alpha2.Perses{}
err := k8sClient.Get(ctx, typeNamespaceName, found)
return errors.IsNotFound(err)
}, time.Minute, time.Second).Should(BeTrue(), "Perses resource should be deleted after finalizer removal")
}, time.Minute, time.Second).Should(BeTrue(), "Perses resource should be deleted cleanly without a finalizer")
})

It("should include user-defined volumes and volumeMounts in the workload", func() {
Expand Down
Loading