Skip to content

🌱 Migrate from Requeue to RequeueAfter in kcp #13028

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
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

🌱 Migrate from Requeue to RequeueAfter in kcp #13028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f109634
migrate from Requeue to RequeueAfter in kcp
sivchari Nov 21, 2025
952c490
drop Requeue entirely
sivchari Nov 21, 2025
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
4 changes: 2 additions & 2 deletions controlplane/kubeadm/internal/controllers/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
patchHelper, err := patch.NewHelper(kcp, r.Client)
if err != nil {
log.Error(err, "Failed to configure the patch helper")
return ctrl.Result{Requeue: true}, nil
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
Comment on lines 214 to +215
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

This should just be (this is pretty old code :))

		return ctrl.Result{}, err

(also drop the log in l.214)

}

if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, kcp); err != nil || isPaused || requeue {
Expand Down Expand Up @@ -559,7 +559,7 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, controlPl
workloadCluster, err := controlPlane.GetWorkloadCluster(ctx)
if err != nil {
log.V(2).Info("cannot get remote client to workload cluster, will requeue", "cause", err)
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

Same here, let's just drop the log and return the error directly (and use an empty result)

return ctrl.Result{Requeue: true}, nil
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
}

// Update kube-proxy daemonset.
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/controllers/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ func TestReconcileClusterNoEndpoints(t *testing.T) {
result, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(kcp)})
g.Expect(err).ToNot(HaveOccurred())
// TODO: this should stop to re-queue as soon as we have a proper remote cluster cache in place.
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: false, RequeueAfter: 20 * time.Second}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))
g.Expect(r.Client.Get(ctx, util.ObjectKey(kcp), kcp)).To(Succeed())

// Always expect that the Finalizer is set on the passed in resource
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/controllers/remediation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileUnhealthyMachines(ctx context.C
controlplanev1.RemediationInProgressAnnotation: remediationInProgressValue,
})

return ctrl.Result{Requeue: true}, nil
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

Suggested change
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
return ctrl.Result{}, nil // No need to requeue here. Machine deletion above triggers reconciliation.

}

// Gets the machine to be remediated, which is the "most broken" among the unhealthy machines, determined as the machine
Expand Down
6 changes: 3 additions & 3 deletions controlplane/kubeadm/internal/controllers/scale.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (r *KubeadmControlPlaneReconciler) initializeControlPlane(ctx context.Conte
newMachine.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(newMachine.Namespace, newMachine.Spec.Bootstrap.ConfigRef.Name))

// Requeue the control plane, in case there are additional operations to perform
return ctrl.Result{Requeue: true}, nil
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

Suggested change
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
return ctrl.Result{}, nil // No need to requeue here. Machine creation above triggers reconciliation.

}

func (r *KubeadmControlPlaneReconciler) scaleUpControlPlane(ctx context.Context, controlPlane *internal.ControlPlane) (ctrl.Result, error) {
Expand Down Expand Up @@ -94,7 +94,7 @@ func (r *KubeadmControlPlaneReconciler) scaleUpControlPlane(ctx context.Context,
newMachine.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(newMachine.Namespace, newMachine.Spec.Bootstrap.ConfigRef.Name))

// Requeue the control plane, in case there are other operations to perform
return ctrl.Result{Requeue: true}, nil
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

Suggested change
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
return ctrl.Result{}, nil // No need to requeue here. Machine creation above triggers reconciliation.

}

func (r *KubeadmControlPlaneReconciler) scaleDownControlPlane(
Expand Down Expand Up @@ -148,7 +148,7 @@ func (r *KubeadmControlPlaneReconciler) scaleDownControlPlane(
Info(fmt.Sprintf("Machine %s deleting (scale down)", machineToDelete.Name), "Machine", klog.KObj(machineToDelete))

// Requeue the control plane, in case there are additional operations to perform
return ctrl.Result{Requeue: true}, nil
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

Suggested change
return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
return ctrl.Result{}, nil // No need to requeue here. Machine deletion above triggers reconciliation.

}

// preflightChecks checks if the control plane is stable before proceeding with a scale up/scale down operation,
Expand Down
8 changes: 4 additions & 4 deletions controlplane/kubeadm/internal/controllers/scale_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func TestKubeadmControlPlaneReconciler_initializeControlPlane(t *testing.T) {
}

result, err := r.initializeControlPlane(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))
Copy link
Member

@sbueringer sbueringer Nov 21, 2025

Choose a reason for hiding this comment

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

please change the unit tests (also in other files) to check for result.IsZero to BeTrue() (where applicable)

g.Expect(err).ToNot(HaveOccurred())

machineList := &clusterv1.MachineList{}
Expand Down Expand Up @@ -164,7 +164,7 @@ func TestKubeadmControlPlaneReconciler_scaleUpControlPlane(t *testing.T) {
}

result, err := r.scaleUpControlPlane(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))
g.Expect(err).ToNot(HaveOccurred())

controlPlaneMachines := clusterv1.MachineList{}
Expand Down Expand Up @@ -297,7 +297,7 @@ func TestKubeadmControlPlaneReconciler_scaleDownControlPlane_NoError(t *testing.
g.Expect(err).ToNot(HaveOccurred())
result, err := r.scaleDownControlPlane(context.Background(), controlPlane, machineToDelete)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))

controlPlaneMachines := clusterv1.MachineList{}
g.Expect(fakeClient.List(context.Background(), &controlPlaneMachines)).To(Succeed())
Expand Down Expand Up @@ -341,7 +341,7 @@ func TestKubeadmControlPlaneReconciler_scaleDownControlPlane_NoError(t *testing.
g.Expect(err).ToNot(HaveOccurred())
result, err := r.scaleDownControlPlane(context.Background(), controlPlane, machineToDelete)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))

controlPlaneMachines := clusterv1.MachineList{}
g.Expect(fakeClient.List(context.Background(), &controlPlaneMachines)).To(Succeed())
Expand Down
8 changes: 4 additions & 4 deletions controlplane/kubeadm/internal/controllers/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func TestKubeadmControlPlaneReconciler_RolloutStrategy_ScaleUp(t *testing.T) {
controlPlane.InjectTestManagementCluster(r.managementCluster)

result, err := r.initializeControlPlane(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(10 * time.Second))
g.Expect(err).ToNot(HaveOccurred())

// initial setup
Expand All @@ -142,7 +142,7 @@ func TestKubeadmControlPlaneReconciler_RolloutStrategy_ScaleUp(t *testing.T) {
machinesUpToDateResults[m.Name] = internal.UpToDateResult{EligibleForInPlaceUpdate: false}
}
result, err = r.updateControlPlane(ctx, controlPlane, needingUpgrade, machinesUpToDateResults)
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))
g.Expect(err).ToNot(HaveOccurred())
bothMachines := &clusterv1.MachineList{}
g.Eventually(func(g Gomega) {
Expand Down Expand Up @@ -193,7 +193,7 @@ func TestKubeadmControlPlaneReconciler_RolloutStrategy_ScaleUp(t *testing.T) {
// run upgrade the second time, expect we scale down
result, err = r.updateControlPlane(ctx, controlPlane, machinesRequireUpgrade, machinesUpToDateResults)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))
finalMachine := &clusterv1.MachineList{}
g.Eventually(func(g Gomega) {
g.Expect(env.List(ctx, finalMachine, client.InNamespace(cluster.Namespace))).To(Succeed())
Expand Down Expand Up @@ -287,7 +287,7 @@ func TestKubeadmControlPlaneReconciler_RolloutStrategy_ScaleDown(t *testing.T) {
machinesUpToDateResults[m.Name] = internal.UpToDateResult{EligibleForInPlaceUpdate: false}
}
result, err = r.updateControlPlane(ctx, controlPlane, needingUpgrade, machinesUpToDateResults)
g.Expect(result).To(BeComparableTo(ctrl.Result{Requeue: true}))
g.Expect(result.RequeueAfter).To(Equal(20 * time.Second))
g.Expect(err).ToNot(HaveOccurred())
remainingMachines := &clusterv1.MachineList{}
g.Expect(fakeClient.List(ctx, remainingMachines, client.InNamespace(cluster.Namespace))).To(Succeed())
Expand Down
16 changes: 0 additions & 16 deletions internal/util/controller/controller_test.go
View file
Open in desktop
Copy link
Member Author

@sivchari sivchari Nov 21, 2025

Choose a reason for hiding this comment

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

Mix them without separating.

Copy link
Member

@sbueringer sbueringer Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just intentionally added these in the PR merged yesterday. Please keep them (please see the godocs I explicitly added to the nolint statements)

(please revert all changes to this file, this util will soon'ish also be used in other controller / repos. It has to handle Requeue appropriately and must have the corresponding unit test coverage for it)

Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ func TestReconcileMetrics(t *testing.T) {
res, err = r.Reconcile(t.Context(), req)
g.Expect(err).To(HaveOccurred())
g.Expect(res.RequeueAfter).To(Equal(time.Duration(0)))
g.Expect(res.Requeue).To(BeFalse()) //nolint:staticcheck // We have to handle Requeue until it is removed
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelError))).To(Equal(1))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelRequeueAfter))).To(Equal(0))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelRequeue))).To(Equal(0))
Expand All @@ -199,26 +198,11 @@ func TestReconcileMetrics(t *testing.T) {
res, err = r.Reconcile(t.Context(), req)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(res.RequeueAfter).To(Equal(5 * time.Second))
g.Expect(res.Requeue).To(BeFalse()) //nolint:staticcheck // We have to handle Requeue until it is removed
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelError))).To(Equal(1))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelRequeueAfter))).To(Equal(1))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelRequeue))).To(Equal(0))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelSuccess))).To(Equal(1))
g.Expect(histogramMetricValue(reconcileTime.WithLabelValues(r.name))).To(Equal(3))

// Requeue
r.reconciler = reconcile.Func(func(_ context.Context, _ reconcile.Request) (reconcile.Result, error) {
return reconcile.Result{Requeue: true}, nil
})
res, err = r.Reconcile(t.Context(), req)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(res.RequeueAfter).To(Equal(time.Duration(0)))
g.Expect(res.Requeue).To(BeTrue()) //nolint:staticcheck // We have to handle Requeue until it is removed
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelError))).To(Equal(1))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelRequeueAfter))).To(Equal(1))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelRequeue))).To(Equal(1))
g.Expect(counterMetricValue(reconcileTotal.WithLabelValues(r.name, labelSuccess))).To(Equal(1))
g.Expect(histogramMetricValue(reconcileTime.WithLabelValues(r.name))).To(Equal(4))
})
}

Expand Down
Loading