Skip to content

Commit 6ec38a2

Browse files
fabriziopandinifabriziopandini
committed
Address comments
# Conflicts: # internal/controllers/machineset/machineset_controller.go
1 parent e316155 commit 6ec38a2

File tree

7 files changed

+78
-86
lines changed

7 files changed

+78
-86
lines changed

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -214,18 +214,20 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
214214

215215
defer func() {
216216
// Always attempt to update status.
217+
var connFailure *internal.RemoteClusterConnectionError
217218
if err := r.updateStatus(ctx, controlPlane); err != nil {
218-
log.Error(err, "Failed to update KubeadmControlPlane status")
219-
reterr = kerrors.NewAggregate([]error{reterr, err})
219+
if errors.As(err, &connFailure) {
220+
log.Error(err, "Could not connect to workload cluster to fetch status")
221+
} else {
222+
reterr = kerrors.NewAggregate([]error{reterr, errors.Wrap(err, "Failed to update KubeadmControlPlane status")})
223+
}
220224
}
221225

222226
if err := r.updateV1Beta1Status(ctx, controlPlane); err != nil {
223-
var connFailure *internal.RemoteClusterConnectionError
224227
if errors.As(err, &connFailure) {
225228
log.Error(err, "Could not connect to workload cluster to fetch status")
226229
} else {
227-
log.Error(err, "Failed to update KubeadmControlPlane deprecated v1beta1 status")
228-
reterr = kerrors.NewAggregate([]error{reterr, err})
230+
reterr = kerrors.NewAggregate([]error{reterr, errors.Wrap(err, "Failed to update KubeadmControlPlane deprecated v1beta1 status")})
229231
}
230232
}
231233

@@ -235,8 +237,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
235237
patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
236238
}
237239
if err := patchKubeadmControlPlane(ctx, patchHelper, kcp, patchOpts...); err != nil {
238-
log.Error(err, "Failed to patch KubeadmControlPlane")
239-
reterr = kerrors.NewAggregate([]error{reterr, err})
240+
reterr = kerrors.NewAggregate([]error{reterr, errors.Wrap(err, "Failed to patch KubeadmControlPlane")})
240241
}
241242

242243
// Only requeue if there is no error, Requeue or RequeueAfter and the object does not have a deletion timestamp.

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,6 @@ func setControlPlaneInitialized(ctx context.Context, controlPlane *internal.Cont
164164
controlPlane.KCP.Status.Initialization = &controlplanev1.KubeadmControlPlaneInitializationStatus{}
165165
}
166166
controlPlane.KCP.Status.Initialization.ControlPlaneInitialized = true
167-
v1beta1conditions.MarkTrue(controlPlane.KCP, controlplanev1.AvailableV1Beta1Condition)
168167
}
169168
}
170169
return nil
@@ -765,7 +764,7 @@ func setAvailableCondition(_ context.Context, kcp *controlplanev1.KubeadmControl
765764
}
766765

767766
// setLastRemediation surface lastRemediation data in status.
768-
// LastRemediation is the remediation currently in progress, in any, or the
767+
// LastRemediation is the remediation currently in progress, if any, or the
769768
// most recent of the remediation we are keeping track on machines.
770769
func setLastRemediation(_ context.Context, controlPlane *internal.ControlPlane) error {
771770
var lastRemediation *RemediationData

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,15 @@ func TestKubeadmControlPlaneReconciler_setControlPlaneInitialized(t *testing.T)
5858
g.Expect(err).ToNot(HaveOccurred())
5959

6060
g.Expect(controlPlane.KCP.Status.Initialization).To(BeNil())
61+
62+
setInitializedCondition(ctx, controlPlane.KCP)
63+
c := conditions.Get(controlPlane.KCP, controlplanev1.KubeadmControlPlaneInitializedCondition)
64+
g.Expect(c).ToNot(BeNil())
65+
g.Expect(*c).To(conditions.MatchCondition(metav1.Condition{
66+
Type: controlplanev1.KubeadmControlPlaneInitializedCondition,
67+
Status: metav1.ConditionFalse,
68+
Reason: controlplanev1.KubeadmControlPlaneNotInitializedReason,
69+
}, conditions.IgnoreLastTransitionTime(true)))
6170
})
6271
t.Run("ControlPlaneInitialized true if the kubeadm config exists", func(t *testing.T) {
6372
g := NewWithT(t)
@@ -78,6 +87,15 @@ func TestKubeadmControlPlaneReconciler_setControlPlaneInitialized(t *testing.T)
7887

7988
g.Expect(controlPlane.KCP.Status.Initialization).ToNot(BeNil())
8089
g.Expect(controlPlane.KCP.Status.Initialization.ControlPlaneInitialized).To(BeTrue())
90+
91+
setInitializedCondition(ctx, controlPlane.KCP)
92+
c := conditions.Get(controlPlane.KCP, controlplanev1.KubeadmControlPlaneInitializedCondition)
93+
g.Expect(c).ToNot(BeNil())
94+
g.Expect(*c).To(conditions.MatchCondition(metav1.Condition{
95+
Type: controlplanev1.KubeadmControlPlaneInitializedCondition,
96+
Status: metav1.ConditionTrue,
97+
Reason: controlplanev1.KubeadmControlPlaneInitializedReason,
98+
}, conditions.IgnoreLastTransitionTime(true)))
8199
})
82100
}
83101

‎docs/book/src/developer/providers/migrations/v1.10-to-v1.11.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ proposal because most of the changes described below are a consequence of the wo
4444
- `status.deprecated.v1beta1.updatedReplicas` with old semantic (and name) for `MachineDeployments`, `KubeadmControlPlane`
4545
- `status.deprecated.v1beta1.fullyLabeledReplicas` for `MachineSet`
4646
- The `Cluster` resource reports replica counters for both control plane and worker machines.
47-
- MachineDeployment `status.phases` are now computed used the same logic used for `ScalingUp` and `ScalingDown` conditions.
47+
- MachineDeployment `status.phases` are now computed using the same logic used for `ScalingUp` and `ScalingDown` conditions.
4848

4949
### Cluster API Contract changes
5050

‎internal/controllers/machinedeployment/machinedeployment_status.go

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2828
"k8s.io/apimachinery/pkg/util/sets"
2929
"k8s.io/klog/v2"
30-
"k8s.io/utils/ptr"
3130
ctrl "sigs.k8s.io/controller-runtime"
3231
"sigs.k8s.io/controller-runtime/pkg/client"
3332

@@ -105,27 +104,24 @@ func setPhase(_ context.Context, machineDeployment *clusterv1.MachineDeployment,
105104
return
106105
}
107106

108-
desiredReplicas := ptr.Deref(machineDeployment.Spec.Replicas, 0)
109107
if !machineDeployment.DeletionTimestamp.IsZero() {
110-
desiredReplicas = 0
108+
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseScalingDown)
109+
return
111110
}
111+
112+
desiredReplicas := *machineDeployment.Spec.Replicas
112113
currentReplicas := mdutil.GetActualReplicaCountForMachineSets(machineSets)
113114

114-
if desiredReplicas < currentReplicas {
115+
switch {
116+
case desiredReplicas == currentReplicas:
117+
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseRunning)
118+
case desiredReplicas < currentReplicas:
115119
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseScalingDown)
116-
return
117-
}
118-
if desiredReplicas > currentReplicas {
120+
case desiredReplicas > currentReplicas:
119121
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseScalingUp)
120-
return
121-
}
122-
if desiredReplicas == currentReplicas {
123-
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseRunning)
124-
return
122+
default:
123+
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseUnknown)
125124
}
126-
127-
// NOTE: this should never happen.
128-
machineDeployment.Status.Phase = string(clusterv1.MachineDeploymentPhaseUnknown)
129125
}
130126

131127
func setAvailableCondition(_ context.Context, machineDeployment *clusterv1.MachineDeployment, getAndAdoptMachineSetsForDeploymentSucceeded bool) {

‎internal/controllers/machinedeployment/machinedeployment_status_test.go

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ func Test_setRollingOutCondition(t *testing.T) {
339339
}
340340

341341
func Test_setScalingUpCondition(t *testing.T) {
342-
defaultMachineDeployment := &clusterv1.MachineDeployment{
342+
machineDeploymentWith0Replicas := &clusterv1.MachineDeployment{
343343
Spec: clusterv1.MachineDeploymentSpec{
344344
Replicas: ptr.To[int32](0),
345345
Template: clusterv1.MachineTemplateSpec{
@@ -361,10 +361,10 @@ func Test_setScalingUpCondition(t *testing.T) {
361361
},
362362
}
363363

364-
scalingUpMachineDeploymentWith3Replicas := defaultMachineDeployment.DeepCopy()
365-
scalingUpMachineDeploymentWith3Replicas.Spec.Replicas = ptr.To[int32](3)
364+
machineDeploymentWith3Replicas := machineDeploymentWith0Replicas.DeepCopy()
365+
machineDeploymentWith3Replicas.Spec.Replicas = ptr.To[int32](3)
366366

367-
deletingMachineDeploymentWith3Replicas := defaultMachineDeployment.DeepCopy()
367+
deletingMachineDeploymentWith3Replicas := machineDeploymentWith0Replicas.DeepCopy()
368368
deletingMachineDeploymentWith3Replicas.DeletionTimestamp = ptr.To(metav1.Now())
369369
deletingMachineDeploymentWith3Replicas.Spec.Replicas = ptr.To[int32](3)
370370

@@ -380,7 +380,7 @@ func Test_setScalingUpCondition(t *testing.T) {
380380
}{
381381
{
382382
name: "getAndAdoptMachineSetsForDeploymentSucceeded failed",
383-
machineDeployment: defaultMachineDeployment,
383+
machineDeployment: machineDeploymentWith0Replicas.DeepCopy(),
384384
bootstrapTemplateNotFound: false,
385385
infrastructureTemplateNotFound: false,
386386
getAndAdoptMachineSetsForDeploymentSucceeded: false,
@@ -395,7 +395,7 @@ func Test_setScalingUpCondition(t *testing.T) {
395395
{
396396
name: "replicas not set",
397397
machineDeployment: func() *clusterv1.MachineDeployment {
398-
md := defaultMachineDeployment.DeepCopy()
398+
md := machineDeploymentWith0Replicas.DeepCopy()
399399
md.Spec.Replicas = nil
400400
return md
401401
}(),
@@ -412,7 +412,7 @@ func Test_setScalingUpCondition(t *testing.T) {
412412
},
413413
{
414414
name: "not scaling up and no machines",
415-
machineDeployment: defaultMachineDeployment,
415+
machineDeployment: machineDeploymentWith0Replicas.DeepCopy(),
416416
bootstrapTemplateNotFound: false,
417417
infrastructureTemplateNotFound: false,
418418
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -425,7 +425,7 @@ func Test_setScalingUpCondition(t *testing.T) {
425425
},
426426
{
427427
name: "not scaling up with machines",
428-
machineDeployment: scalingUpMachineDeploymentWith3Replicas,
428+
machineDeployment: machineDeploymentWith3Replicas.DeepCopy(),
429429
machineSets: []*clusterv1.MachineSet{
430430
fakeMachineSet("ms1", withStatusReplicas(1)),
431431
fakeMachineSet("ms2", withStatusReplicas(2)),
@@ -442,7 +442,7 @@ func Test_setScalingUpCondition(t *testing.T) {
442442
},
443443
{
444444
name: "not scaling up and no machines and bootstrapConfig object not found",
445-
machineDeployment: defaultMachineDeployment,
445+
machineDeployment: machineDeploymentWith0Replicas.DeepCopy(),
446446
bootstrapTemplateNotFound: true,
447447
infrastructureTemplateNotFound: false,
448448
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -456,7 +456,7 @@ func Test_setScalingUpCondition(t *testing.T) {
456456
},
457457
{
458458
name: "not scaling up and no machines and infrastructure object not found",
459-
machineDeployment: defaultMachineDeployment,
459+
machineDeployment: machineDeploymentWith0Replicas.DeepCopy(),
460460
bootstrapTemplateNotFound: false,
461461
infrastructureTemplateNotFound: true,
462462
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -470,7 +470,7 @@ func Test_setScalingUpCondition(t *testing.T) {
470470
},
471471
{
472472
name: "not scaling up and no machines and bootstrapConfig and infrastructure object not found",
473-
machineDeployment: defaultMachineDeployment,
473+
machineDeployment: machineDeploymentWith0Replicas.DeepCopy(),
474474
bootstrapTemplateNotFound: true,
475475
infrastructureTemplateNotFound: true,
476476
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -484,7 +484,7 @@ func Test_setScalingUpCondition(t *testing.T) {
484484
},
485485
{
486486
name: "scaling up",
487-
machineDeployment: scalingUpMachineDeploymentWith3Replicas,
487+
machineDeployment: machineDeploymentWith3Replicas.DeepCopy(),
488488
bootstrapTemplateNotFound: false,
489489
infrastructureTemplateNotFound: false,
490490
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -498,7 +498,7 @@ func Test_setScalingUpCondition(t *testing.T) {
498498
},
499499
{
500500
name: "scaling up with machines",
501-
machineDeployment: scalingUpMachineDeploymentWith3Replicas,
501+
machineDeployment: machineDeploymentWith3Replicas.DeepCopy(),
502502
machineSets: []*clusterv1.MachineSet{
503503
fakeMachineSet("ms1", withStatusReplicas(1)),
504504
fakeMachineSet("ms2", withStatusReplicas(1)),
@@ -516,7 +516,7 @@ func Test_setScalingUpCondition(t *testing.T) {
516516
},
517517
{
518518
name: "scaling up and blocked by bootstrap object",
519-
machineDeployment: scalingUpMachineDeploymentWith3Replicas,
519+
machineDeployment: machineDeploymentWith3Replicas.DeepCopy(),
520520
bootstrapTemplateNotFound: true,
521521
infrastructureTemplateNotFound: false,
522522
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -530,7 +530,7 @@ func Test_setScalingUpCondition(t *testing.T) {
530530
},
531531
{
532532
name: "scaling up and blocked by infrastructure object",
533-
machineDeployment: scalingUpMachineDeploymentWith3Replicas,
533+
machineDeployment: machineDeploymentWith3Replicas.DeepCopy(),
534534
bootstrapTemplateNotFound: false,
535535
infrastructureTemplateNotFound: true,
536536
getAndAdoptMachineSetsForDeploymentSucceeded: true,
@@ -544,7 +544,7 @@ func Test_setScalingUpCondition(t *testing.T) {
544544
},
545545
{
546546
name: "deleting, don't show block message when templates are not found",
547-
machineDeployment: deletingMachineDeploymentWith3Replicas,
547+
machineDeployment: deletingMachineDeploymentWith3Replicas.DeepCopy(),
548548
machineSets: []*clusterv1.MachineSet{{}, {}, {}},
549549
bootstrapTemplateNotFound: true,
550550
infrastructureTemplateNotFound: true,
@@ -554,7 +554,7 @@ func Test_setScalingUpCondition(t *testing.T) {
554554
Status: metav1.ConditionFalse,
555555
Reason: clusterv1.MachineDeploymentNotScalingUpReason,
556556
},
557-
expectedPhase: clusterv1.MachineDeploymentPhaseRunning,
557+
expectedPhase: clusterv1.MachineDeploymentPhaseScalingDown,
558558
},
559559
}
560560
for _, tt := range tests {
@@ -762,7 +762,7 @@ After above Pods have been removed from the Node, the following Pods will be evi
762762
Status: metav1.ConditionFalse,
763763
Reason: clusterv1.MachineDeploymentNotScalingDownReason,
764764
},
765-
expectedPhase: clusterv1.MachineDeploymentPhaseRunning,
765+
expectedPhase: clusterv1.MachineDeploymentPhaseScalingDown,
766766
},
767767
{
768768
name: "deleting machine deployment having 1 replica",

0 commit comments

Comments
 (0)