Skip to content

Commit b2bd2fa

Browse files
fabriziopandinifabriziopandini
committed
Address comments
1 parent e3df532 commit b2bd2fa

11 files changed

+172
-107
lines changed

‎api/v1beta1/machinedeployment_types.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ const (
8888
// field of the MachineDeployment is not set.
8989
MachineDeploymentAvailableWaitingForReplicasSetV1Beta2Reason = WaitingForReplicasSetV1Beta2Reason
9090

91-
// MachineDeploymentAvailableWaitingForAvailableReplicasSetV1Beta2Reason surfaces when the .status.v1beta2.availableReplicas
91+
// MachineDeploymentAvailableWaitingForAvailableReplicasSetV1Beta2Reason surfaces when the .status.v1beta2.availableReplicas
9292
// field of the MachineDeployment is not set.
9393
MachineDeploymentAvailableWaitingForAvailableReplicasSetV1Beta2Reason = "WaitingForAvailableReplicasSet"
9494

@@ -130,7 +130,7 @@ const (
130130

131131
// MachineDeployment's ScalingUp condition and corresponding reasons that will be used in v1Beta2 API version.
132132
const (
133-
// MachineDeploymentScalingUpV1Beta2Condition is true if available replicas < desired replicas.
133+
// MachineDeploymentScalingUpV1Beta2Condition is true if actual replicas < desired replicas.
134134
MachineDeploymentScalingUpV1Beta2Condition = ScalingUpV1Beta2Condition
135135

136136
// MachineDeploymentScalingUpV1Beta2Reason surfaces when actual replicas < desired replicas.
@@ -149,7 +149,7 @@ const (
149149

150150
// MachineDeployment's ScalingDown condition and corresponding reasons that will be used in v1Beta2 API version.
151151
const (
152-
// MachineDeploymentScalingDownV1Beta2Condition is true if replicas > desired replicas.
152+
// MachineDeploymentScalingDownV1Beta2Condition is true if actual replicas > desired replicas.
153153
MachineDeploymentScalingDownV1Beta2Condition = ScalingDownV1Beta2Condition
154154

155155
// MachineDeploymentScalingDownV1Beta2Reason surfaces when actual replicas > desired replicas.
@@ -168,7 +168,7 @@ const (
168168

169169
// MachineDeployment's Remediating condition and corresponding reasons that will be used in v1Beta2 API version.
170170
const (
171-
// MachineDeploymentRemediatingV1Beta2Condition details about ongoing remediation of the controlled machines, if any.
171+
// MachineDeploymentRemediatingV1Beta2Condition surfaces details about ongoing remediation of the controlled machines, if any.
172172
MachineDeploymentRemediatingV1Beta2Condition = RemediatingV1Beta2Condition
173173

174174
// MachineDeploymentRemediatingV1Beta2Reason surfaces when the MachineDeployment has at least one machine with HealthCheckSucceeded set to false

‎api/v1beta1/v1beta2_condition_consts.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ const (
9090
// AvailableV1Beta2Reason applies to a condition surfacing object availability.
9191
AvailableV1Beta2Reason = "Available"
9292

93-
// NotAvailableV1Beta2Reason applies to a condition surfacing object not satisfying availability per-requisites.
93+
// NotAvailableV1Beta2Reason applies to a condition surfacing object not satisfying availability criteria.
9494
NotAvailableV1Beta2Reason = "NotAvailable"
9595

9696
// ScalingUpV1Beta2Reason surfaces when an object is scaling up.

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

+8
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,10 @@ func setRemediatingCondition(ctx context.Context, kcp *controlplanev1.KubeadmCon
406406
}
407407

408408
func aggregateStaleMachines(machines collections.Machines) string {
409+
if len(machines) == 0 {
410+
return ""
411+
}
412+
409413
machineNames := []string{}
410414
for _, machine := range machines {
411415
if !machine.GetDeletionTimestamp().IsZero() && time.Since(machine.GetDeletionTimestamp().Time) > time.Minute*30 {
@@ -436,6 +440,10 @@ func aggregateStaleMachines(machines collections.Machines) string {
436440
}
437441

438442
func aggregateUnhealthyMachines(machines collections.Machines) string {
443+
if len(machines) == 0 {
444+
return ""
445+
}
446+
439447
machineNames := []string{}
440448
for _, machine := range machines {
441449
machineNames = append(machineNames, machine.GetName())

‎internal/controllers/machinedeployment/machinedeployment_controller.go

+15-5
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
116116
return nil
117117
}
118118

119-
func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
119+
func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ctrl.Result, reterr error) {
120120
log := ctrl.LoggerFrom(ctx)
121121

122122
// Fetch the MachineDeployment instance.
@@ -173,6 +173,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
173173
if err := patchMachineDeployment(ctx, patchHelper, deployment, patchOpts...); err != nil {
174174
reterr = kerrors.NewAggregate([]error{reterr, err})
175175
}
176+
177+
if reterr != nil {
178+
retres = ctrl.Result{}
179+
}
176180
}()
177181

178182
// Handle deletion reconciliation loop.
@@ -188,7 +192,9 @@ type scope struct {
188192
cluster *clusterv1.Cluster
189193
machineSets []*clusterv1.MachineSet
190194
bootstrapTemplateNotFound bool
195+
bootstrapTemplateExists bool
191196
infrastructureTemplateNotFound bool
197+
infrastructureTemplateExists bool
192198
getAndAdoptMachineSetsForDeploymentSucceeded bool
193199
}
194200

@@ -297,15 +303,16 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope) error {
297303
return errors.Errorf("missing MachineDeployment strategy")
298304
}
299305

306+
templateExists := s.infrastructureTemplateExists && (md.Spec.Template.Spec.Bootstrap.ConfigRef == nil || s.bootstrapTemplateExists)
300307
if md.Spec.Strategy.Type == clusterv1.RollingUpdateMachineDeploymentStrategyType {
301308
if md.Spec.Strategy.RollingUpdate == nil {
302309
return errors.Errorf("missing MachineDeployment settings for strategy type: %s", md.Spec.Strategy.Type)
303310
}
304-
return r.rolloutRolling(ctx, md, s.machineSets)
311+
return r.rolloutRolling(ctx, md, s.machineSets, templateExists)
305312
}
306313

307314
if md.Spec.Strategy.Type == clusterv1.OnDeleteMachineDeploymentStrategyType {
308-
return r.rolloutOnDelete(ctx, md, s.machineSets)
315+
return r.rolloutOnDelete(ctx, md, s.machineSets, templateExists)
309316
}
310317

311318
return errors.Errorf("unexpected deployment strategy type: %s", md.Spec.Strategy.Type)
@@ -323,8 +330,6 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) error {
323330
return nil
324331
}
325332

326-
log.Info("Waiting for MachineSets to be deleted", "MachineSets", clog.ObjNamesString(s.machineSets))
327-
328333
// else delete owned machinesets.
329334
for _, ms := range s.machineSets {
330335
if ms.DeletionTimestamp.IsZero() {
@@ -335,6 +340,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) error {
335340
}
336341
}
337342

343+
log.Info("Waiting for MachineSets to be deleted", "MachineSets", clog.ObjNamesString(s.machineSets))
338344
return nil
339345
}
340346

@@ -479,6 +485,8 @@ func (r *Reconciler) getTemplatesAndSetOwner(ctx context.Context, s *scope) erro
479485
return err
480486
}
481487
s.infrastructureTemplateNotFound = true
488+
} else {
489+
s.infrastructureTemplateExists = true
482490
}
483491
// Make sure to reconcile the external bootstrap reference, if any.
484492
if md.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
@@ -487,6 +495,8 @@ func (r *Reconciler) getTemplatesAndSetOwner(ctx context.Context, s *scope) erro
487495
return err
488496
}
489497
s.bootstrapTemplateNotFound = true
498+
} else {
499+
s.bootstrapTemplateExists = true
490500
}
491501
}
492502
return nil

‎internal/controllers/machinedeployment/machinedeployment_rolling.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ import (
2929
)
3030

3131
// rolloutRolling implements the logic for rolling a new MachineSet.
32-
func (r *Reconciler) rolloutRolling(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error {
33-
newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, true)
32+
func (r *Reconciler) rolloutRolling(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, templateExists bool) error {
33+
newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, true, templateExists)
3434
if err != nil {
3535
return err
3636
}

‎internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ import (
3232
)
3333

3434
// rolloutOnDelete implements the logic for the OnDelete MachineDeploymentStrategyType.
35-
func (r *Reconciler) rolloutOnDelete(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error {
36-
newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, true)
35+
func (r *Reconciler) rolloutOnDelete(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, templateExists bool) error {
36+
newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, true, templateExists)
3737
if err != nil {
3838
return err
3939
}

‎internal/controllers/machinedeployment/machinedeployment_status.go

+36-27
Original file line numberDiff line numberDiff line change
@@ -37,35 +37,38 @@ import (
3737

3838
func (r *Reconciler) updateStatus(ctx context.Context, s *scope) (retErr error) {
3939
// Get all Machines controlled by this MachineDeployment.
40-
var machines, machinesToBeRemediated, unHealthyMachines collections.Machines
40+
var machines, machinesToBeRemediated, unhealthyMachines collections.Machines
41+
var getMachinesSucceeded bool
4142
if selectorMap, err := metav1.LabelSelectorAsMap(&s.machineDeployment.Spec.Selector); err == nil {
4243
machineList := &clusterv1.MachineList{}
4344
if err := r.Client.List(ctx, machineList, client.InNamespace(s.machineDeployment.Namespace), client.MatchingLabels(selectorMap)); err != nil {
4445
retErr = errors.Wrap(err, "failed to list machines")
46+
} else {
47+
getMachinesSucceeded = true
48+
machines = collections.FromMachineList(machineList)
49+
machinesToBeRemediated = machines.Filter(collections.IsUnhealthyAndOwnerRemediated)
50+
unhealthyMachines = machines.Filter(collections.IsUnhealthy)
4551
}
46-
machines = collections.FromMachineList(machineList)
47-
machinesToBeRemediated = machines.Filter(collections.IsUnhealthyAndOwnerRemediated)
48-
unHealthyMachines = machines.Filter(collections.IsUnhealthy)
4952
} else {
5053
retErr = errors.Wrap(err, "failed to convert label selector to a map")
5154
}
5255

53-
// If the controller failed to read MachineSets, do not update replica counters.
54-
if !s.getAndAdoptMachineSetsForDeploymentSucceeded {
56+
// If the controller could read MachineSets, update replica counters.
57+
if s.getAndAdoptMachineSetsForDeploymentSucceeded {
5558
setReplicas(s.machineDeployment, s.machineSets)
5659
}
5760

5861
setAvailableCondition(ctx, s.machineDeployment, s.getAndAdoptMachineSetsForDeploymentSucceeded)
5962

6063
setScalingUpCondition(ctx, s.machineDeployment, s.machineSets, s.bootstrapTemplateNotFound, s.infrastructureTemplateNotFound, s.getAndAdoptMachineSetsForDeploymentSucceeded)
61-
setScalingDownCondition(ctx, s.machineDeployment, s.machineSets, machines, s.getAndAdoptMachineSetsForDeploymentSucceeded)
64+
setScalingDownCondition(ctx, s.machineDeployment, s.machineSets, machines, s.getAndAdoptMachineSetsForDeploymentSucceeded, getMachinesSucceeded)
6265

63-
setMachinesReadyCondition(ctx, s.machineDeployment, machines)
64-
setMachinesUpToDateCondition(ctx, s.machineDeployment, machines)
66+
setMachinesReadyCondition(ctx, s.machineDeployment, machines, getMachinesSucceeded)
67+
setMachinesUpToDateCondition(ctx, s.machineDeployment, machines, getMachinesSucceeded)
6568

66-
setRemediatingCondition(ctx, s.machineDeployment, machinesToBeRemediated, unHealthyMachines)
69+
setRemediatingCondition(ctx, s.machineDeployment, machinesToBeRemediated, unhealthyMachines, getMachinesSucceeded)
6770

68-
setDeletingCondition(ctx, s.machineDeployment, s.machineSets, machines, s.getAndAdoptMachineSetsForDeploymentSucceeded)
71+
setDeletingCondition(ctx, s.machineDeployment, s.machineSets, machines, s.getAndAdoptMachineSetsForDeploymentSucceeded, getMachinesSucceeded)
6972

7073
return retErr
7174
}
@@ -168,7 +171,7 @@ func setScalingUpCondition(_ context.Context, machineDeployment *clusterv1.Machi
168171
if !machineDeployment.DeletionTimestamp.IsZero() {
169172
desiredReplicas = 0
170173
}
171-
currentReplicas := mdutil.GetReplicaCountForMachineSets(machineSets)
174+
currentReplicas := mdutil.GetActualReplicaCountForMachineSets(machineSets)
172175

173176
missingReferencesMessage := calculateMissingReferencesMessage(machineDeployment, bootstrapObjectNotFound, infrastructureObjectNotFound)
174177

@@ -199,7 +202,7 @@ func setScalingUpCondition(_ context.Context, machineDeployment *clusterv1.Machi
199202
})
200203
}
201204

202-
func setScalingDownCondition(_ context.Context, machineDeployment *clusterv1.MachineDeployment, machineSets []*clusterv1.MachineSet, machines collections.Machines, getAndAdoptMachineSetsForDeploymentSucceeded bool) {
205+
func setScalingDownCondition(_ context.Context, machineDeployment *clusterv1.MachineDeployment, machineSets []*clusterv1.MachineSet, machines collections.Machines, getAndAdoptMachineSetsForDeploymentSucceeded, getMachinesSucceeded bool) {
203206
// If we got unexpected errors in listing the machines sets (this should never happen), surface them.
204207
if !getAndAdoptMachineSetsForDeploymentSucceeded {
205208
v1beta2conditions.Set(machineDeployment, metav1.Condition{
@@ -225,14 +228,16 @@ func setScalingDownCondition(_ context.Context, machineDeployment *clusterv1.Mac
225228
if !machineDeployment.DeletionTimestamp.IsZero() {
226229
desiredReplicas = 0
227230
}
228-
currentReplicas := mdutil.GetReplicaCountForMachineSets(machineSets)
231+
currentReplicas := mdutil.GetActualReplicaCountForMachineSets(machineSets)
229232

230233
// Scaling down.
231234
if currentReplicas > desiredReplicas {
232235
message := fmt.Sprintf("Scaling down from %d to %d replicas", currentReplicas, desiredReplicas)
233-
staleMessage := aggregateStaleMachines(machines)
234-
if staleMessage != "" {
235-
message += fmt.Sprintf(" and %s", staleMessage)
236+
if getMachinesSucceeded {
237+
staleMessage := aggregateStaleMachines(machines)
238+
if staleMessage != "" {
239+
message += fmt.Sprintf(" and %s", staleMessage)
240+
}
236241
}
237242
v1beta2conditions.Set(machineDeployment, metav1.Condition{
238243
Type: clusterv1.MachineDeploymentScalingDownV1Beta2Condition,
@@ -251,10 +256,10 @@ func setScalingDownCondition(_ context.Context, machineDeployment *clusterv1.Mac
251256
})
252257
}
253258

254-
func setMachinesReadyCondition(ctx context.Context, machineDeployment *clusterv1.MachineDeployment, machines collections.Machines) {
259+
func setMachinesReadyCondition(ctx context.Context, machineDeployment *clusterv1.MachineDeployment, machines collections.Machines, getMachinesSucceeded bool) {
255260
log := ctrl.LoggerFrom(ctx)
256261
// If we got unexpected errors in listing the machines (this should never happen), surface them.
257-
if machines == nil {
262+
if !getMachinesSucceeded {
258263
v1beta2conditions.Set(machineDeployment, metav1.Condition{
259264
Type: clusterv1.MachineDeploymentMachinesReadyV1Beta2Condition,
260265
Status: metav1.ConditionUnknown,
@@ -291,10 +296,10 @@ func setMachinesReadyCondition(ctx context.Context, machineDeployment *clusterv1
291296
v1beta2conditions.Set(machineDeployment, *readyCondition)
292297
}
293298

294-
func setMachinesUpToDateCondition(ctx context.Context, machineDeployment *clusterv1.MachineDeployment, machines collections.Machines) {
299+
func setMachinesUpToDateCondition(ctx context.Context, machineDeployment *clusterv1.MachineDeployment, machines collections.Machines, getMachinesSucceeded bool) {
295300
log := ctrl.LoggerFrom(ctx)
296301
// If we got unexpected errors in listing the machines (this should never happen), surface them.
297-
if machines == nil {
302+
if !getMachinesSucceeded {
298303
v1beta2conditions.Set(machineDeployment, metav1.Condition{
299304
Type: clusterv1.MachineDeploymentMachinesUpToDateV1Beta2Condition,
300305
Status: metav1.ConditionUnknown,
@@ -331,8 +336,8 @@ func setMachinesUpToDateCondition(ctx context.Context, machineDeployment *cluste
331336
v1beta2conditions.Set(machineDeployment, *upToDateCondition)
332337
}
333338

334-
func setRemediatingCondition(ctx context.Context, machineDeployment *clusterv1.MachineDeployment, machinesToBeRemediated, unhealthyMachines collections.Machines) {
335-
if machinesToBeRemediated == nil || unhealthyMachines == nil {
339+
func setRemediatingCondition(ctx context.Context, machineDeployment *clusterv1.MachineDeployment, machinesToBeRemediated, unhealthyMachines collections.Machines, getMachinesSucceeded bool) {
340+
if !getMachinesSucceeded {
336341
v1beta2conditions.Set(machineDeployment, metav1.Condition{
337342
Type: clusterv1.MachineDeploymentRemediatingV1Beta2Condition,
338343
Status: metav1.ConditionUnknown,
@@ -378,9 +383,9 @@ func setRemediatingCondition(ctx context.Context, machineDeployment *clusterv1.M
378383
})
379384
}
380385

381-
func setDeletingCondition(_ context.Context, machineDeployment *clusterv1.MachineDeployment, machineSets []*clusterv1.MachineSet, machines collections.Machines, getAndAdoptMachineSetsForDeploymentSucceeded bool) {
386+
func setDeletingCondition(_ context.Context, machineDeployment *clusterv1.MachineDeployment, machineSets []*clusterv1.MachineSet, machines collections.Machines, getAndAdoptMachineSetsForDeploymentSucceeded, getMachinesSucceeded bool) {
382387
// If we got unexpected errors in listing the machines sets or machines (this should never happen), surface them.
383-
if !getAndAdoptMachineSetsForDeploymentSucceeded || machines == nil {
388+
if !getAndAdoptMachineSetsForDeploymentSucceeded || !getMachinesSucceeded {
384389
v1beta2conditions.Set(machineDeployment, metav1.Condition{
385390
Type: clusterv1.MachineDeploymentDeletingV1Beta2Condition,
386391
Status: metav1.ConditionUnknown,
@@ -401,7 +406,7 @@ func setDeletingCondition(_ context.Context, machineDeployment *clusterv1.Machin
401406

402407
message := ""
403408
if len(machines) > 0 {
404-
message = fmt.Sprintf("Deleting %d replicas", len(machines))
409+
message = fmt.Sprintf("Deleting %d Machines", len(machines))
405410
staleMessage := aggregateStaleMachines(machines)
406411
if staleMessage != "" {
407412
message += fmt.Sprintf(" and %s", staleMessage)
@@ -413,7 +418,7 @@ func setDeletingCondition(_ context.Context, machineDeployment *clusterv1.Machin
413418
}
414419
v1beta2conditions.Set(machineDeployment, metav1.Condition{
415420
Type: clusterv1.MachineDeploymentDeletingV1Beta2Condition,
416-
Status: metav1.ConditionFalse,
421+
Status: metav1.ConditionTrue,
417422
Reason: clusterv1.MachineDeploymentDeletingDeletionTimestampSetV1Beta2Reason,
418423
Message: message,
419424
})
@@ -474,6 +479,10 @@ func aggregateStaleMachines(machines collections.Machines) string {
474479
}
475480

476481
func aggregateUnhealthyMachines(machines collections.Machines) string {
482+
if len(machines) == 0 {
483+
return ""
484+
}
485+
477486
machineNames := []string{}
478487
for _, machine := range machines {
479488
machineNames = append(machineNames, machine.GetName())

0 commit comments

Comments
 (0)