Skip to content

Commit

Permalink
Fix review findings
Browse files Browse the repository at this point in the history
  • Loading branch information
@sbueringer
sbueringer committed Nov 6, 2024
1 parent 9fdc5c9 commit bcc2974
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 49 deletions.
6 changes: 6 additions & 0 deletions controlplane/kubeadm/internal/control_plane.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ type ControlPlane struct {

managementCluster ManagementCluster
workloadCluster WorkloadCluster

// deletingReason is the reason that should be used when setting the Deleting condition.
DeletingReason string

// deletingMessage is the message that should be used when setting the Deleting condition.
DeletingMessage string
}

// PreflightCheckResults contains description about pre flight check results blocking machines creation or deletion.
Expand Down
41 changes: 15 additions & 26 deletions controlplane/kubeadm/internal/controllers/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,6 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
return ctrl.Result{}, nil
}

s := &scope{}

defer func() {
// Always attempt to update status.
if err := r.updateStatus(ctx, controlPlane); err != nil {
Expand All @@ -224,7 +222,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
}
}

r.updateV1Beta2Status(ctx, controlPlane, s)
r.updateV1Beta2Status(ctx, controlPlane)

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

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

// scope holds the different objects that are read and used during the reconcile.
type scope struct {
// deletingReason is the reason that should be used when setting the Deleting condition.
deletingReason string

// deletingMessage is the message that should be used when setting the Deleting condition.
deletingMessage string
}

// reconcileDelete handles KubeadmControlPlane deletion.
// The implementation does not take non-control plane workloads into consideration. This may or may not change in the future.
// Please see https://github.com/kubernetes-sigs/cluster-api/issues/2064.
func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, controlPlane *internal.ControlPlane, s *scope) (ctrl.Result, error) {
func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, controlPlane *internal.ControlPlane) (ctrl.Result, error) {
log := ctrl.LoggerFrom(ctx)
log.Info("Reconcile KubeadmControlPlane deletion")

// If no control plane machines remain, remove the finalizer
if len(controlPlane.Machines) == 0 {
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason
s.deletingMessage = ""
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason
controlPlane.DeletingMessage = ""

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

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

s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason
s.deletingMessage = objectsPendingDeleteNames(allMachines, allMachinePools, controlPlane.Cluster)
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason
controlPlane.DeletingMessage = objectsPendingDeleteNames(allMachines, allMachinePools, controlPlane.Cluster)
return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil
}

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

s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
s.deletingMessage = "Please check controller logs for errors"
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingInternalErrorV1Beta2Reason
controlPlane.DeletingMessage = "Please check controller logs for errors"
return ctrl.Result{}, err
}

Expand All @@ -710,8 +699,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
message += fmt.Sprintf(" and %s", staleMessage)
}
}
s.deletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason
s.deletingMessage = message
controlPlane.DeletingReason = controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason
controlPlane.DeletingMessage = message
return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil
}

Expand Down
37 changes: 17 additions & 20 deletions controlplane/kubeadm/internal/controllers/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3036,14 +3036,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
Cluster: cluster,
Machines: machines,
}
s := &scope{}

result, err := r.reconcileDelete(ctx, controlPlane, s)
result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
g.Expect(err).ToNot(HaveOccurred())
g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason))
g.Expect(s.deletingMessage).To(Equal("Deleting 3 Machines"))
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForMachineDeletionV1Beta2Reason))
g.Expect(controlPlane.DeletingMessage).To(Equal("Deleting 3 Machines"))

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

result, err = r.reconcileDelete(ctx, controlPlane, s)
result, err = r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{}))
g.Expect(err).ToNot(HaveOccurred())
g.Expect(kcp.Finalizers).To(BeEmpty())
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
g.Expect(s.deletingMessage).To(BeEmpty())
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
g.Expect(controlPlane.DeletingMessage).To(BeEmpty())
})

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

result, err := r.reconcileDelete(ctx, controlPlane, s)
result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
g.Expect(err).ToNot(HaveOccurred())

g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
g.Expect(s.deletingMessage).To(Equal("Worker Machines: worker"))
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
g.Expect(controlPlane.DeletingMessage).To(Equal("Worker Machines: worker"))

controlPlaneMachines := clusterv1.MachineList{}
labels := map[string]string{
Expand Down Expand Up @@ -3177,14 +3175,14 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
Cluster: cluster,
Machines: machines,
}
s := &scope{}

result, err := r.reconcileDelete(ctx, controlPlane, s)
result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
g.Expect(err).ToNot(HaveOccurred())

g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
g.Expect(s.deletingMessage).To(Equal("MachinePools: worker"))
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingWaitingForWorkersDeletionV1Beta2Reason))
g.Expect(controlPlane.DeletingMessage).To(Equal("MachinePools: worker"))

controlPlaneMachines := clusterv1.MachineList{}
labels := map[string]string{
Expand Down Expand Up @@ -3216,14 +3214,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
KCP: kcp,
Cluster: cluster,
}
s := &scope{}

result, err := r.reconcileDelete(ctx, controlPlane, s)
result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(BeComparableTo(ctrl.Result{}))
g.Expect(err).ToNot(HaveOccurred())
g.Expect(kcp.Finalizers).To(BeEmpty())
g.Expect(s.deletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
g.Expect(s.deletingMessage).To(BeEmpty())
g.Expect(controlPlane.DeletingReason).To(Equal(controlplanev1.KubeadmControlPlaneDeletingDeletionCompletedV1Beta2Reason))
g.Expect(controlPlane.DeletingMessage).To(BeEmpty())
})
}

Expand Down
6 changes: 3 additions & 3 deletions controlplane/kubeadm/internal/controllers/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro

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

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

0 comments on commit bcc2974

Please sign in to comment.