✨ Add optional Priority field to reconcile.Result #3333
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/L
|
/assign @alvaroaleman /hold |
k8s-ci-robot
added
the
do-not-merge/hold
| } | ||
| ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc() | ||
| ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, labelError).Inc() | ||
| if !result.IsZero() { |
There was a problem hiding this comment.
If Priority is set result is not zero anymore. Not ideal, but I don't have a better idea.
Independent of that I think it's not especially bad to get rid of this warning.
I've written code specifically to set ctrl.Result{} to zero to avoid this warning which would have been absolutely not necessary otherwise
There was a problem hiding this comment.
Could explicitly check for Requeue or RequeAfter being set. It is a bit awkward that we now respect a non-error return when an error is returned, that is very uncommon
There was a problem hiding this comment.
Could explicitly check for Requeue or RequeAfter being set.
I'll do that
It is a bit awkward that we now respect a non-error return when an error is returned, that is very uncommon
I think we don't "respect" it (in the sense of using it), we just don't write a warning on log level 0.
We have a bunch of controllers where the main reconcile logic returns either an error or non-zero ctrl.Result{}, but then we patch the reconciled object with defer. If that patch fails we want to return the error.
Because of this code here in controller-runtime we have to make sure that in these situations we set Result to zero to avoid this warning log: https://github.com/kubernetes-sigs/cluster-api/blob/main/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go#L274-L276
Not sure I see the value of this warning over CR just silently prioritizing error over ctrl.Result{} if both is returned
But this discussion is orthogonal to adding the Priority parameter. So let's continue the discussion (I'll re-add the warning for this PR)
There was a problem hiding this comment.
I think we don't "respect" it (in the sense of using it), we just don't write a warning on log level 0.
What I meant is the Priority field, we respect it even in the error case
sbueringer
changed the title
| } | ||
| ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc() | ||
| ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, labelError).Inc() | ||
| if !result.IsZero() { |
There was a problem hiding this comment.
Could explicitly check for Requeue or RequeAfter being set. It is a bit awkward that we now respect a non-error return when an error is returned, that is very uncommon
sbueringer
force-pushed
the
pr-result-priority
branch
from
3b56aca to
1cca75a
Compare
Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-result-priority
branch
from
1cca75a to
c71b8c8
Compare
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 251fa5d530effa8db75ba71c7b37ecca9d374726
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer 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 |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Signed-off-by: Stefan Büringer [email protected]
Part of #2374