Skip to content

Commit 3c785a3

Browse files
sbueringersbueringer
committed
Fix review findings
1 parent 4896af1 commit 3c785a3

File tree

4 files changed

+41
-49
lines changed

4 files changed

+41
-49
lines changed

‎controlplane/kubeadm/internal/control_plane.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,12 @@ type ControlPlane struct {
6060

6161
managementCluster ManagementCluster
6262
workloadCluster WorkloadCluster
63+
64+
// deletingReason is the reason that should be used when setting the Deleting condition.
65+
DeletingReason string
66+
67+
// deletingMessage is the message that should be used when setting the Deleting condition.
68+
DeletingMessage string
6369
}
6470

6571
// PreflightCheckResults contains description about pre flight check results blocking machines creation or deletion.

‎controlplane/kubeadm/internal/controllers/controller.go

Lines changed: 15 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,6 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
210210
return ctrl.Result{}, nil
211211
}
212212

213-
s := &scope{}
214-
215213
defer func() {
216214
// Always attempt to update status.
217215
if err := r.updateStatus(ctx, controlPlane); err != nil {
@@ -224,7 +222,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
224222
}
225223
}
226224

227-
r.updateV1Beta2Status(ctx, controlPlane, s)
225+
r.updateV1Beta2Status(ctx, controlPlane)
228226

229227
// Always attempt to Patch the KubeadmControlPlane object and status after each reconciliation.
230228
patchOpts := []patch.Option{}
@@ -258,7 +256,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
258256

259257
if !kcp.ObjectMeta.DeletionTimestamp.IsZero() {
260258
// Handle deletion reconciliation loop.
261-
res, err = r.reconcileDelete(ctx, controlPlane, s)
259+
res, err = r.reconcileDelete(ctx, controlPlane)
262260
if errors.Is(err, clustercache.ErrClusterNotConnected) {
263261
log.V(5).Info("Requeuing because connection to the workload cluster is down")
264262
return ctrl.Result{RequeueAfter: time.Minute}, nil
@@ -591,26 +589,17 @@ func (r *KubeadmControlPlaneReconciler) reconcileClusterCertificates(ctx context
591589
return nil
592590
}
593591

594-
// scope holds the different objects that are read and used during the reconcile.
595-
type scope struct {
596-
// deletingReason is the reason that should be used when setting the Deleting condition.
597-
deletingReason string
598-
599-
// deletingMessage is the message that should be used when setting the Deleting condition.
600-
deletingMessage string
601-
}
602-
603592
// reconcileDelete handles KubeadmControlPlane deletion.
604593
// The implementation does not take non-control plane workloads into consideration. This may or may not change in the future.
605594
// Please see https://github.com/kubernetes-sigs/cluster-api/issues/2064.
606-
func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, controlPlane *internal.ControlPlane, s *scope) (ctrl.Result, error) {
595+
func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, controlPlane *internal.ControlPlane) (ctrl.Result, error) {
607596
log := ctrl.LoggerFrom(ctx)
608597
log.Info("Reconcile KubeadmControlPlane deletion")
609598

610599
// If no control plane machines remain, remove the finalizer
611600
if len(controlPlane.Machines) == 0 {
612-
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason
613-
s.deletingMessage = ""
601+
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason
602+
controlPlane.DeletingMessage = ""
614603

615604
controllerutil.RemoveFinalizer(controlPlane.KCP, controlplanev1.KubeadmControlPlaneFinalizer)
616605
return ctrl.Result{}, nil
@@ -631,8 +620,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
631620
// Gets all machines, not just control plane machines.
632621
allMachines, err := r.managementCluster.GetMachinesForCluster(ctx, controlPlane.Cluster)
633622
if err != nil {
634-
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
635-
s.deletingMessage = "Please check controller logs for errors" //nolint:goconst // Not making this a constant for now
623+
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
624+
controlPlane.DeletingMessage = "Please check controller logs for errors" //nolint:goconst // Not making this a constant for now
636625
return ctrl.Result{}, err
637626
}
638627

@@ -641,8 +630,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
641630
if feature.Gates.Enabled(feature.MachinePool) {
642631
allMachinePools, err = r.managementCluster.GetMachinePoolsForCluster(ctx, controlPlane.Cluster)
643632
if err != nil {
644-
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
645-
s.deletingMessage = "Please check controller logs for errors"
633+
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
634+
controlPlane.DeletingMessage = "Please check controller logs for errors"
646635
return ctrl.Result{}, err
647636
}
648637
}
@@ -651,8 +640,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
651640
log.Info("Waiting for worker nodes to be deleted first")
652641
conditions.MarkFalse(controlPlane.KCP, controlplanev1.ResizedCondition, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "Waiting for worker nodes to be deleted first")
653642

654-
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason
655-
s.deletingMessage = objectsPendingDeleteNames(allMachines, allMachinePools, controlPlane.Cluster)
643+
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason
644+
controlPlane.DeletingMessage = objectsPendingDeleteNames(allMachines, allMachinePools, controlPlane.Cluster)
656645
return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil
657646
}
658647

@@ -689,8 +678,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
689678
r.recorder.Eventf(controlPlane.KCP, corev1.EventTypeWarning, "FailedDelete",
690679
"Failed to delete control plane Machines for cluster %s control plane: %v", klog.KObj(controlPlane.Cluster), err)
691680

692-
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
693-
s.deletingMessage = "Please check controller logs for errors"
681+
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
682+
controlPlane.DeletingMessage = "Please check controller logs for errors"
694683
return ctrl.Result{}, err
695684
}
696685

@@ -710,8 +699,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
710699
message += fmt.Sprintf(" and %s", staleMessage)
711700
}
712701
}
713-
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason
714-
s.deletingMessage = message
702+
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason
703+
controlPlane.DeletingMessage = message
715704
return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil
716705
}
717706

‎controlplane/kubeadm/internal/controllers/controller_test.go

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3036,14 +3036,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
30363036
Cluster: cluster,
30373037
Machines: machines,
30383038
}
3039-
s := &scope{}
30403039

3041-
result, err := r.reconcileDelete(ctx, controlPlane, s)
3040+
result, err := r.reconcileDelete(ctx, controlPlane)
30423041
g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
30433042
g.Expect(err).ToNot(HaveOccurred())
30443043
g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
3045-
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason))
3046-
g.Expect(s.deletingMessage).To(Equal("Deleting 3 Machines"))
3044+
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason))
3045+
g.Expect(controlPlane.DeletingMessage).To(Equal("Deleting 3 Machines"))
30473046

30483047
controlPlaneMachines := clusterv1.MachineList{}
30493048
g.Expect(fakeClient.List(ctx, &controlPlaneMachines)).To(Succeed())
@@ -3065,14 +3064,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
30653064
KCP: kcp,
30663065
Cluster: cluster,
30673066
}
3068-
s = &scope{}
30693067

3070-
result, err = r.reconcileDelete(ctx, controlPlane, s)
3068+
result, err = r.reconcileDelete(ctx, controlPlane)
30713069
g.Expect(result).To(BeComparableTo(ctrl.Result{}))
30723070
g.Expect(err).ToNot(HaveOccurred())
30733071
g.Expect(kcp.Finalizers).To(BeEmpty())
3074-
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
3075-
g.Expect(s.deletingMessage).To(BeEmpty())
3072+
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
3073+
g.Expect(controlPlane.DeletingMessage).To(BeEmpty())
30763074
})
30773075

30783076
t.Run("does not remove any control plane Machines if other Machines exist", func(t *testing.T) {
@@ -3117,14 +3115,14 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
31173115
Cluster: cluster,
31183116
Machines: machines,
31193117
}
3120-
s := &scope{}
31213118

3122-
result, err := r.reconcileDelete(ctx, controlPlane, s)
3119+
result, err := r.reconcileDelete(ctx, controlPlane)
31233120
g.Expect(result).To(BeComparableTo(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
31243121
g.Expect(err).ToNot(HaveOccurred())
3122+
31253123
g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
3126-
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
3127-
g.Expect(s.deletingMessage).To(Equal("Worker Machines: worker"))
3124+
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
3125+
g.Expect(controlPlane.DeletingMessage).To(Equal("Worker Machines: worker"))
31283126

31293127
controlPlaneMachines := clusterv1.MachineList{}
31303128
labels := map[string]string{
@@ -3177,14 +3175,14 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
31773175
Cluster: cluster,
31783176
Machines: machines,
31793177
}
3180-
s := &scope{}
31813178

3182-
result, err := r.reconcileDelete(ctx, controlPlane, s)
3179+
result, err := r.reconcileDelete(ctx, controlPlane)
31833180
g.Expect(result).To(BeComparableTo(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
31843181
g.Expect(err).ToNot(HaveOccurred())
3182+
31853183
g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
3186-
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
3187-
g.Expect(s.deletingMessage).To(Equal("MachinePools: worker"))
3184+
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
3185+
g.Expect(controlPlane.DeletingMessage).To(Equal("MachinePools: worker"))
31883186

31893187
controlPlaneMachines := clusterv1.MachineList{}
31903188
labels := map[string]string{
@@ -3216,14 +3214,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
32163214
KCP: kcp,
32173215
Cluster: cluster,
32183216
}
3219-
s := &scope{}
32203217

3221-
result, err := r.reconcileDelete(ctx, controlPlane, s)
3218+
result, err := r.reconcileDelete(ctx, controlPlane)
32223219
g.Expect(result).To(BeComparableTo(ctrl.Result{}))
32233220
g.Expect(err).ToNot(HaveOccurred())
32243221
g.Expect(kcp.Finalizers).To(BeEmpty())
3225-
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
3226-
g.Expect(s.deletingMessage).To(BeEmpty())
3222+
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
3223+
g.Expect(controlPlane.DeletingMessage).To(BeEmpty())
32273224
})
32283225
}
32293226

‎controlplane/kubeadm/internal/controllers/status.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro
147147

148148
// updateV1Beta2Status reconciles KubeadmControlPlane's status during the entire lifecycle of the object.
149149
// Note: v1beta1 conditions and fields are not managed by this func.
150-
func (r *KubeadmControlPlaneReconciler) updateV1Beta2Status(ctx context.Context, controlPlane *internal.ControlPlane, s *scope) {
150+
func (r *KubeadmControlPlaneReconciler) updateV1Beta2Status(ctx context.Context, controlPlane *internal.ControlPlane) {
151151
// If the code failed initializing the control plane, do not update the status.
152152
if controlPlane == nil {
153153
return
@@ -167,8 +167,8 @@ func (r *KubeadmControlPlaneReconciler) updateV1Beta2Status(ctx context.Context,
167167
setMachinesReadyCondition(ctx, controlPlane.KCP, controlPlane.Machines)
168168
setMachinesUpToDateCondition(ctx, controlPlane.KCP, controlPlane.Machines)
169169
setRemediatingCondition(ctx, controlPlane.KCP, controlPlane.MachinesToBeRemediatedByKCP(), controlPlane.UnhealthyMachines())
170-
setDeletingCondition(ctx, controlPlane.KCP, s.deletingReason, s.deletingMessage)
171-
// TODO: Available, Deleting
170+
setDeletingCondition(ctx, controlPlane.KCP, controlPlane.DeletingReason, controlPlane.DeletingMessage)
171+
// TODO: Available
172172
}
173173

174174
func setReplicas(_ context.Context, kcp *controlplanev1.KubeadmControlPlane, machines collections.Machines) {

0 commit comments

Comments
 (0)