🐛 WIP: Stop cluster/topology reconciliation on Status changes #5956
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -77,6 +79,8 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt | |||
| For(&clusterv1.Cluster{}, builder.WithPredicates( | |||
| // Only reconcile Cluster with topology. | |||
| predicates.ClusterHasTopology(ctrl.LoggerFrom(ctx)), | |||
| // Only reconcile on Cluster Generation change to avoid reconciling on Status updates. | |||
| ctrlpredicates.GenerationChangedPredicate{}, | |||
There was a problem hiding this comment.
Change looks reasonable to me. As far as I'm aware there is no case where we want to reconcile the topology when the status changes.
I think it would be the far more generic solution to the problem, as we then don't have to make sure that every error we propagate into the conditions (which is afaik all of them) is constant with each failed reconcile.
Signed-off-by: killianmuldoon <[email protected]>
killianmuldoon
force-pushed
the
fix/no-exponential-backoff
branch
from
7d9965b to
c35aa0e
Compare
| @@ -30,6 +30,7 @@ import ( | |||
| "sigs.k8s.io/controller-runtime/pkg/client" | |||
| "sigs.k8s.io/controller-runtime/pkg/controller" | |||
| "sigs.k8s.io/controller-runtime/pkg/handler" | |||
| ctrlpredicates "sigs.k8s.io/controller-runtime/pkg/predicate" | |||
There was a problem hiding this comment.
nit: I wonder if we should use crpredicates instead. I saw cr as prefix in other providers as prefix for controller-runtime and I think it's more intuitive and reads better.
There was a problem hiding this comment.
Whatever suits - I picked ctrl because it's used to import controller runtime itself in other parts of the code, and ctrl client is used in places. We should try to keep the usage consistent if we can.
There was a problem hiding this comment.
Yup agree. If nobody else has an opinion on that, let's just leave it as is.
I think currently this would affect:
- ctrl => sigs.k8s.io/controller-runtime
- ctrlclient => "sigs.k8s.io/controller-runtime/pkg/client" (which is only very rarely aliased at all, but I don't see a problem there)
- ctrlpredicates "sigs.k8s.io/controller-runtime/pkg/predicate"
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
Closing this for now in favor of #5971 |
Signed-off-by: killianmuldoon [email protected]
Add a predicate to the Cluster Topology controller to only trigger reconciliation when the GenerationNumber of the Cluster resource changes. This change will result in fewer reconciliation loops and fix an issue where a bad spec was causing multiple instant reconciliations.
Fixes #5945