End Reconcile after Finalizer Addition #64
Conversation
The reconcile should end when a finalizer has been added. Without ending the reconcile a new reconcile will allways be triggered and will result in a duplicate reboot to the node (until FAR would have a status to the CR)
|
Skipping CI for Draft Pull Request. |
|
/test 4.13-openshift-e2e |
| @@ -106,6 +106,10 @@ func (r *FenceAgentsRemediationReconciler) Reconcile(ctx context.Context, req ct | |||
| if err := r.Client.Update(context.Background(), far); err != nil { | |||
| return emptyResult, fmt.Errorf("failed to add finalizer to the CR - %w", err) | |||
| } | |||
| r.Log.Info("Finalizer was added", "CR Name", req.Name) | |||
| return emptyResult, nil | |||
There was a problem hiding this comment.
I do not understand how this prevents a duplicate reboot. Can you explain please?
There was a problem hiding this comment.
I thought the PR description would explain it, but I will try again.
Before this change on a valid CR (by name), then the first reconcile will finish with emptyResult after running all the reconcile logic (including the FA execution). But since on the last (and first) reconcile call we have been updating the CR by adding a finalizer, then it triggers a second reconcile.
On this second reconcile it will try to execute the FA, since the CR doesn't have a status or any if block that we sparse the CR from running again the FA (it probably will fail until it eventually succeeds to reboot the node).
The reconcile won't finish be requeued until the FA will return success response, and we will end up with duplicate reboot.
After this change when we add the finalizer then we finish the reconcile with emptyResult. Since the CR was updated then on the second reconcile run will add taint and exeucte FA for the first time. Therefore, the PR change would stop the FA execution from the first reconcile run.
I am aware that other stuff, such as remediating the node which host the FAR PR, could trigger the reconcile again, and we will end up with another reboot but this problem will be resolved later with ECOPROJECT-1411
There was a problem hiding this comment.
Ah ok, I mixed 1411 and 1415, sorry.
By returning the empty result here you only implicitly requeue because you know that it will happen because of the CR modification. I would prefer an explicit requeue. Did you try that? Or will it be reconciled twice then? 🤔
There was a problem hiding this comment.
By returning the empty result here you only implicitly requeue because you know that it will happen because of the CR modification
Correct.
Did you try that? Or will it be reconciled twice then?
Correct again, see https://github.com/medik8s/fence-agents-remediation/pull/64/files#diff-05147ee3f0d225b5a60754b8b9bff2c569deb0a9d7eaddae33af680291d493a1R111-R112. I tried with Requeue: true or RequeueAfter: 1 * time.Second, and it resulted in a second reconcile. So until there's a status this PR will add this workaround...
|
/test 4.13-openshift-e2e |
| @@ -106,6 +106,10 @@ func (r *FenceAgentsRemediationReconciler) Reconcile(ctx context.Context, req ct | |||
| if err := r.Client.Update(context.Background(), far); err != nil { | |||
| return emptyResult, fmt.Errorf("failed to add finalizer to the CR - %w", err) | |||
| } | |||
| r.Log.Info("Finalizer was added", "CR Name", req.Name) | |||
| return emptyResult, nil | |||
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7, slintes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The reconcile should end when a finalizer has been added. Without ending the reconcile a new reconcile will always be triggered and will result in a duplicate reboot to the node (until FAR would have a status to the CR).
ECOPROJECT-1415