Skip to content

Commit 5970cbc

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #11503 from fabriziopandini/refine-v1beta2-uptodate-and-rollout-conditions
🌱 Refine v1beta2 UpToDate and Rollout conditions
2 parents befa7c7 + 590b7f3 commit 5970cbc

File tree

11 files changed

+210
-67
lines changed

11 files changed

+210
-67
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -979,7 +979,10 @@ func reconcileMachineUpToDateCondition(_ context.Context, controlPlane *internal
979979
if machinesNotUptoDateNames.Has(machine.Name) {
980980
message := ""
981981
if reasons, ok := machinesNotUptoDateConditionMessages[machine.Name]; ok {
982-
message = strings.Join(reasons, "; ")
982+
for i := range reasons {
983+
reasons[i] = fmt.Sprintf("* %s", reasons[i])
984+
}
985+
message = strings.Join(reasons, "\n")
983986
}
984987

985988
v1beta2conditions.Set(machine, metav1.Condition{

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2134,7 +2134,7 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneAndMachinesCondition
21342134
Type: clusterv1.MachineUpToDateV1Beta2Condition,
21352135
Status: metav1.ConditionFalse,
21362136
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
2137-
Message: "Version v1.30.0, v1.31.0 required",
2137+
Message: "* Version v1.30.0, v1.31.0 required",
21382138
},
21392139
},
21402140
},

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ func setRollingOutCondition(_ context.Context, kcp *controlplanev1.KubeadmContro
226226
}
227227
rollingOutReplicas++
228228
if upToDateCondition.Message != "" {
229-
rolloutReasons.Insert(strings.Split(upToDateCondition.Message, "; ")...)
229+
rolloutReasons.Insert(strings.Split(upToDateCondition.Message, "\n")...)
230230
}
231231
}
232232

@@ -247,17 +247,14 @@ func setRollingOutCondition(_ context.Context, kcp *controlplanev1.KubeadmContro
247247
// Surface rollout reasons ensuring that if there is a version change, it goes first.
248248
reasons := rolloutReasons.UnsortedList()
249249
sort.Slice(reasons, func(i, j int) bool {
250-
if strings.HasPrefix(reasons[i], "Version") && !strings.HasPrefix(reasons[j], "Version") {
250+
if strings.HasPrefix(reasons[i], "* Version") && !strings.HasPrefix(reasons[j], "* Version") {
251251
return true
252252
}
253-
if !strings.HasPrefix(reasons[i], "Version") && strings.HasPrefix(reasons[j], "Version") {
253+
if !strings.HasPrefix(reasons[i], "* Version") && strings.HasPrefix(reasons[j], "* Version") {
254254
return false
255255
}
256256
return reasons[i] < reasons[j]
257257
})
258-
for i := range reasons {
259-
reasons[i] = fmt.Sprintf("* %s", reasons[i])
260-
}
261258
message += fmt.Sprintf("\n%s", strings.Join(reasons, "\n"))
262259
}
263260
v1beta2conditions.Set(kcp, metav1.Condition{

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -169,20 +169,21 @@ func Test_setRollingOutCondition(t *testing.T) {
169169
Reason: clusterv1.InternalErrorV1Beta2Reason,
170170
},
171171
}}}},
172-
{ObjectMeta: metav1.ObjectMeta{Name: "m3"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{
172+
{ObjectMeta: metav1.ObjectMeta{Name: "m4"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{
173173
{
174-
Type: clusterv1.MachineUpToDateV1Beta2Condition,
175-
Status: metav1.ConditionFalse,
176-
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
177-
Message: "Version v1.25.0, v1.26.0 required",
174+
Type: clusterv1.MachineUpToDateV1Beta2Condition,
175+
Status: metav1.ConditionFalse,
176+
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
177+
Message: "* Failure domain failure-domain1, failure-domain2 required\n" +
178+
"* InfrastructureMachine is not up-to-date",
178179
},
179180
}}}},
180-
{ObjectMeta: metav1.ObjectMeta{Name: "m4"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{
181+
{ObjectMeta: metav1.ObjectMeta{Name: "m3"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{
181182
{
182183
Type: clusterv1.MachineUpToDateV1Beta2Condition,
183184
Status: metav1.ConditionFalse,
184185
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
185-
Message: "Failure domain failure-domain1, failure-domain2 required; InfrastructureMachine is not up-to-date",
186+
Message: "* Version v1.25.0, v1.26.0 required",
186187
},
187188
}}}},
188189
},

‎internal/controllers/machinedeployment/machinedeployment_status.go

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ func setRollingOutCondition(_ context.Context, machineDeployment *clusterv1.Mach
179179
}
180180
rollingOutReplicas++
181181
if upToDateCondition.Message != "" {
182-
rolloutReasons.Insert(strings.Split(upToDateCondition.Message, "; ")...)
182+
rolloutReasons.Insert(strings.Split(upToDateCondition.Message, "\n")...)
183183
}
184184
}
185185

@@ -200,17 +200,14 @@ func setRollingOutCondition(_ context.Context, machineDeployment *clusterv1.Mach
200200
// Surface rollout reasons ensuring that if there is a version change, it goes first.
201201
reasons := rolloutReasons.UnsortedList()
202202
sort.Slice(reasons, func(i, j int) bool {
203-
if strings.HasPrefix(reasons[i], "Version") && !strings.HasPrefix(reasons[j], "Version") {
203+
if strings.HasPrefix(reasons[i], "* Version") && !strings.HasPrefix(reasons[j], "* Version") {
204204
return true
205205
}
206-
if !strings.HasPrefix(reasons[i], "Version") && strings.HasPrefix(reasons[j], "Version") {
206+
if !strings.HasPrefix(reasons[i], "* Version") && strings.HasPrefix(reasons[j], "* Version") {
207207
return false
208208
}
209209
return reasons[i] < reasons[j]
210210
})
211-
for i := range reasons {
212-
reasons[i] = fmt.Sprintf("* %s", reasons[i])
213-
}
214211
message += fmt.Sprintf("\n%s", strings.Join(reasons, "\n"))
215212
}
216213
v1beta2conditions.Set(machineDeployment, metav1.Condition{

‎internal/controllers/machinedeployment/machinedeployment_status_test.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -292,17 +292,18 @@ func Test_setRollingOutCondition(t *testing.T) {
292292
Status: metav1.ConditionUnknown,
293293
Reason: clusterv1.InternalErrorV1Beta2Reason,
294294
})),
295-
fakeMachine("machine-3", withV1Beta2Condition(metav1.Condition{
296-
Type: clusterv1.MachineUpToDateV1Beta2Condition,
297-
Status: metav1.ConditionFalse,
298-
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
299-
Message: "Version v1.25.0, v1.26.0 required",
300-
})),
301295
fakeMachine("machine-4", withV1Beta2Condition(metav1.Condition{
296+
Type: clusterv1.MachineUpToDateV1Beta2Condition,
297+
Status: metav1.ConditionFalse,
298+
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
299+
Message: "* Failure domain failure-domain1, failure-domain2 required\n" +
300+
"* InfrastructureMachine is not up-to-date",
301+
})),
302+
fakeMachine("machine-3", withV1Beta2Condition(metav1.Condition{
302303
Type: clusterv1.MachineUpToDateV1Beta2Condition,
303304
Status: metav1.ConditionFalse,
304305
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
305-
Message: "Failure domain failure-domain1, failure-domain2 required; InfrastructureMachine is not up-to-date",
306+
Message: "* Version v1.25.0, v1.26.0 required",
306307
})),
307308
},
308309
getMachinesSucceeded: true,

‎internal/controllers/machineset/machineset_controller.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -641,11 +641,14 @@ func newMachineUpToDateCondition(s *scope) *metav1.Condition {
641641
}
642642

643643
if !upToDate {
644+
for i := range conditionMessages {
645+
conditionMessages[i] = fmt.Sprintf("* %s", conditionMessages[i])
646+
}
644647
return &metav1.Condition{
645648
Type: clusterv1.MachineUpToDateV1Beta2Condition,
646649
Status: metav1.ConditionFalse,
647650
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
648-
Message: strings.Join(conditionMessages, "; "),
651+
Message: strings.Join(conditionMessages, "\n"),
649652
}
650653
}
651654

‎internal/controllers/machineset/machineset_controller_test.go

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2659,7 +2659,7 @@ func TestNewMachineUpToDateCondition(t *testing.T) {
26592659
Type: clusterv1.MachineUpToDateV1Beta2Condition,
26602660
Status: metav1.ConditionFalse,
26612661
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
2662-
Message: "Version v1.30.0, v1.31.0 required",
2662+
Message: "* Version v1.30.0, v1.31.0 required",
26632663
},
26642664
},
26652665
{
@@ -2720,7 +2720,7 @@ func TestNewMachineUpToDateCondition(t *testing.T) {
27202720
Type: clusterv1.MachineUpToDateV1Beta2Condition,
27212721
Status: metav1.ConditionFalse,
27222722
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
2723-
Message: "MachineDeployment spec.rolloutAfter expired",
2723+
Message: "* MachineDeployment spec.rolloutAfter expired",
27242724
},
27252725
},
27262726
{
@@ -2753,6 +2753,38 @@ func TestNewMachineUpToDateCondition(t *testing.T) {
27532753
Reason: clusterv1.MachineUpToDateV1Beta2Reason,
27542754
},
27552755
},
2756+
{
2757+
name: "not up-to-date, version changed, rollout After expired",
2758+
machineDeployment: &clusterv1.MachineDeployment{
2759+
Spec: clusterv1.MachineDeploymentSpec{
2760+
RolloutAfter: &metav1.Time{Time: reconciliationTime.Add(-1 * time.Hour)}, // rollout after expired
2761+
Template: clusterv1.MachineTemplateSpec{
2762+
Spec: clusterv1.MachineSpec{
2763+
Version: ptr.To("v1.30.0"),
2764+
},
2765+
},
2766+
},
2767+
},
2768+
machineSet: &clusterv1.MachineSet{
2769+
ObjectMeta: metav1.ObjectMeta{
2770+
CreationTimestamp: metav1.Time{Time: reconciliationTime.Add(-2 * time.Hour)}, // MS created before rollout after
2771+
},
2772+
Spec: clusterv1.MachineSetSpec{
2773+
Template: clusterv1.MachineTemplateSpec{
2774+
Spec: clusterv1.MachineSpec{
2775+
Version: ptr.To("v1.31.0"),
2776+
},
2777+
},
2778+
},
2779+
},
2780+
expectCondition: &metav1.Condition{
2781+
Type: clusterv1.MachineUpToDateV1Beta2Condition,
2782+
Status: metav1.ConditionFalse,
2783+
Reason: clusterv1.MachineNotUpToDateV1Beta2Reason,
2784+
Message: "* Version v1.31.0, v1.30.0 required\n" +
2785+
"* MachineDeployment spec.rolloutAfter expired",
2786+
},
2787+
},
27562788
}
27572789
for _, tt := range tests {
27582790
t.Run(tt.name, func(t *testing.T) {

‎util/conditions/v1beta2/merge_strategies.go

Lines changed: 62 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package v1beta2
1919
import (
2020
"fmt"
2121
"reflect"
22+
"regexp"
2223
"sort"
2324
"strings"
2425

@@ -265,36 +266,7 @@ func (d *defaultMergeStrategy) Merge(conditions []ConditionWithOwnerInfo, condit
265266
// When including messages from conditions, they are sorted by issue/unknown and by the implicit order of condition types
266267
// provided by the user (it is considered as order of relevance).
267268
if isSummaryOperation {
268-
messages := []string{}
269-
270-
// Note: use conditions because we want to preserve the order of relevance defined by the users (the order of condition types).
271-
for _, condition := range conditions {
272-
priority := d.getPriorityFunc(condition.Condition)
273-
if priority == InfoMergePriority {
274-
// Drop info messages when we are surfacing issues or unknown.
275-
if status != metav1.ConditionTrue {
276-
continue
277-
}
278-
// Drop info conditions with empty messages.
279-
if condition.Message == "" {
280-
continue
281-
}
282-
}
283-
284-
m := fmt.Sprintf("* %s:", condition.Type)
285-
if condition.Message != "" {
286-
m += indentIfMultiline(condition.Message)
287-
} else {
288-
m += fmt.Sprintf(" %s", condition.Reason)
289-
}
290-
messages = append(messages, m)
291-
}
292-
293-
if d.summaryMessageTransformFunc != nil {
294-
messages = d.summaryMessageTransformFunc(messages)
295-
}
296-
297-
message = strings.Join(messages, "\n")
269+
message = summaryMessage(conditions, d, status)
298270
}
299271

300272
// When performing the aggregate operation, we are merging one single condition from potentially many objects.
@@ -377,6 +349,40 @@ func splitConditionsByPriority(conditions []ConditionWithOwnerInfo, getPriority
377349
return issueConditions, unknownConditions, infoConditions
378350
}
379351

352+
// summaryMessage returns message for the summary operation.
353+
func summaryMessage(conditions []ConditionWithOwnerInfo, d *defaultMergeStrategy, status metav1.ConditionStatus) string {
354+
messages := []string{}
355+
356+
// Note: use conditions because we want to preserve the order of relevance defined by the users (the order of condition types).
357+
for _, condition := range conditions {
358+
priority := d.getPriorityFunc(condition.Condition)
359+
if priority == InfoMergePriority {
360+
// Drop info messages when we are surfacing issues or unknown.
361+
if status != metav1.ConditionTrue {
362+
continue
363+
}
364+
// Drop info conditions with empty messages.
365+
if condition.Message == "" {
366+
continue
367+
}
368+
}
369+
370+
m := fmt.Sprintf("* %s:", condition.Type)
371+
if condition.Message != "" {
372+
m += indentIfMultiline(condition.Message)
373+
} else {
374+
m += fmt.Sprintf(" %s", condition.Reason)
375+
}
376+
messages = append(messages, m)
377+
}
378+
379+
if d.summaryMessageTransformFunc != nil {
380+
messages = d.summaryMessageTransformFunc(messages)
381+
}
382+
383+
return strings.Join(messages, "\n")
384+
}
385+
380386
// aggregateMessages returns messages for the aggregate operation.
381387
func aggregateMessages(conditions []ConditionWithOwnerInfo, n *int, dropEmpty bool, getPriority func(condition metav1.Condition) MergePriority, otherMessages map[MergePriority]string) (messages []string) {
382388
// create a map with all the messages and the list of objects reporting the same message.
@@ -546,13 +552,37 @@ func sortObj(i, j string, cpMachines sets.Set[string]) bool {
546552
return i < j
547553
}
548554

555+
var re = regexp.MustCompile(`\s*\*\s+`)
556+
549557
func indentIfMultiline(m string) string {
550558
msg := ""
551-
if strings.Contains(m, "\n") || strings.HasPrefix(m, "* ") {
559+
// If it is a multiline string or if it start with a bullet, indent the message.
560+
if strings.Contains(m, "\n") || re.MatchString(m) {
552561
msg += "\n"
562+
563+
// Split the message in lines, and add a prefix; prefix can be
564+
// " " when indenting a line starting in a bullet
565+
// " * " when indenting a line starting without a bullet (indent + add a bullet)
566+
// " " when indenting a line starting with a bullet, but other lines required adding a bullet
553567
lines := strings.Split(m, "\n")
568+
prefix := " "
569+
hasLinesWithoutBullet := false
570+
for i := range lines {
571+
if !re.MatchString(lines[i]) {
572+
hasLinesWithoutBullet = true
573+
break
574+
}
575+
}
554576
for i, l := range lines {
555-
lines[i] = " " + l
577+
prefix := prefix
578+
if hasLinesWithoutBullet {
579+
if !re.MatchString(lines[i]) {
580+
prefix += "* "
581+
} else {
582+
prefix += " "
583+
}
584+
}
585+
lines[i] = prefix + l
556586
}
557587
msg += strings.Join(lines, "\n")
558588
} else {

0 commit comments

Comments
 (0)