Skip to content

[control-operator] finalizer blocks deletion of task until all pods are gone #830

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
justonedev1 wants to merge 2 commits into
base: master
Choose a base branch
from
+51 −21
Open

[control-operator] finalizer blocks deletion of task until all pods are gone #830

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b26f2a
[control-operator] finalizer blocks deletion of task until all pods a…
justonedev1 Jul 3, 2026
ca5ca50
fixup! [control-operator] finalizer blocks deletion of task until all…
justonedev1 Jul 14, 2026
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
72 changes: 51 additions & 21 deletions control-operator/internal/controller/task_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,35 +270,65 @@ func (r *TaskReconciler) consumeGRPCConsumerIfReady(ctx context.Context, t *alie
return ctrl.Result{}
}

// handleFinalizer sets the finalizer, when Task isn't marked for deletion, and when deletion reconcile is triggered
// it then clears the gRPC connection and deletes the underlying POD and doesn't allow Task to be deleted
// until POD is gone.
// Note: if you add finalizer to a task you cannot delete it unless you remove the finalizer, run:
// kubectl patch task --type=json -p='[{"op": "remove", "path": "/metadata/finalizers"}]'
func (r *TaskReconciler) handleFinalizer(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (ctrl.Result, bool, error) {

@knopers8 knopers8 Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please document what the function returns, especially what the bool means.

if t.DeletionTimestamp.IsZero() {
if !controllerutil.ContainsFinalizer(t, taskFinalizer) {
controllerutil.AddFinalizer(t, taskFinalizer)
if err := r.Update(ctx, t); err != nil {
return ctrl.Result{}, true, err
}
return ctrl.Result{}, true, nil
if controllerutil.AddFinalizer(t, taskFinalizer) {
return ctrl.Result{}, true, r.Update(ctx, t)
}
} else {
if controllerutil.ContainsFinalizer(t, taskFinalizer) {
log.Info("Cleaning up gRPC connection before deletion")
if client, exists := clientsForContainers[t.Name]; exists {
if err := client.Close(); err != nil {
log.Error(err, "Failed to close gRPC client during deletion")
}
delete(clientsForContainers, t.Name)
}
return ctrl.Result{}, false, nil
}

controllerutil.RemoveFinalizer(t, taskFinalizer)
if err := r.Update(ctx, t); err != nil {
return ctrl.Result{}, true, err
}
}
if !controllerutil.ContainsFinalizer(t, taskFinalizer) {
return ctrl.Result{}, true, nil
}

@knopers8 knopers8 Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Which situation could lead us to this case, i.e. scheduled for deletion, but no finalizer? Given that we ask Reconcile to return, won't it result in an infinite loop?

return ctrl.Result{}, false, nil

log.Info("Finalizer found, starting cleanup")

@knopers8 knopers8 Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log.Info("Finalizer found, starting cleanup")
log.Info("Deletion timestamp exists and finalizer found, starting cleanup")

I would propose to be more clear about conditions for starting cleanup, so one is not misled into thinking that it's enough to have finalizer present to start cleanup.

r.cleargRPC(t, log)

if podGone, err := r.podDeleted(ctx, t, log); err != nil || !podGone {
return ctrl.Result{}, true, err
}

controllerutil.RemoveFinalizer(t, taskFinalizer)
return ctrl.Result{}, true, r.Update(ctx, t)
}

// podDeleted deletes the Task's Pod if it still exists, and reports whether it is
// fully gone yet so the caller can wait for termination before removing the finalizer.
func (r *TaskReconciler) podDeleted(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (bool, error) {

@knopers8 knopers8 Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The name of the function is misleading, as it does not indicate that it attempts to delete the pod. Sure, the behaviour is explained better in a comment, but one can only notice it when navigating to the function implementation. Please rename to sth like "deletePod" and also name the return value in the declaration.

Suggested change
func (r *TaskReconciler) podDeleted(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (bool, error) {
func (r *TaskReconciler) deletePod(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (podGone bool, error) {

However, given that you have to negate the returned value after calling podDeleted, you could even return podExists for (arguably) easier reading?

pod := &v1.Pod{}
err := r.Get(ctx, types.NamespacedName{Name: podNameFromTask(t.Name), Namespace: t.Namespace}, pod)
if errors.IsNotFound(err) {
log.Info("POD cleaned")
return true, nil
} else if err != nil {
return false, err
}

if pod.DeletionTimestamp.IsZero() {
log.Info("Deleting pod before removing finalizer")
if err := r.Delete(ctx, pod); err != nil && !errors.IsNotFound(err) {
return false, err
}
}
log.Info("Waiting for pod to terminate before removing finalizer")
return false, nil
}

func (*TaskReconciler) cleargRPC(t *aliecsv1alpha1.Task, log logr.Logger) {
if client, exists := clientsForContainers[t.Name]; exists {
log.Info("Cleaning up gRPC connection")
if err := client.Close(); err != nil {
log.Error(err, "Failed to close gRPC client during deletion")
}
delete(clientsForContainers, t.Name)
log.Info("gRPC cleaned")
}
}

func podNameFromTask(name string) string {
Expand Down
Loading