[control-operator] finalizer blocks deletion of task until all pods are gone#830
[control-operator] finalizer blocks deletion of task until all pods are gone#830justonedev1 wants to merge 2 commits into
Conversation
knopers8
left a comment
knopers8
left a comment
There was a problem hiding this comment.
The logic seems OK, but please try to simplify the logic and reduce nesting similarly to environment's handleFinalizer. Perhaps in a separate PR, but as you prefer.
9b20acf to
ca5ca50
Compare
knopers8
left a comment
knopers8
left a comment
There was a problem hiding this comment.
Thanks for the refactoring, it does look clearer! I would still have a few suggestions and questions though.
|
|
||
| // 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) { |
There was a problem hiding this comment.
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.
| 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?
| // 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) { |
There was a problem hiding this comment.
please document what the function returns, especially what the bool means.
| } | ||
| if !controllerutil.ContainsFinalizer(t, taskFinalizer) { | ||
| return ctrl.Result{}, true, nil | ||
| } |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
| 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.
2 participants
can be merged, if you think that it is good enough