Repository: kubernetes-sigs/cluster-api
Pull request #3152 (2020-06-05 14:10:13)

🌱 Improve conditions summary
**What this PR does / why we need it**:
This PR 
1. Makes it possible to specify a list of conditions to be used for generating a summary, so we can control how the semantic for the Ready conditions change over time (before it was automatically changing whenever a new condition was added)
2. Makes the step x of y notation to use the above list of conditions to determine the y value
3. Makes it possible to use the step x of y notation during the provisioning of a machine, but then it switches backs to the default notation when other conditions appear (e.g MHC conditions)

All the existing summary conditions are then updated to use the new merge options.

**Which issue(s) this PR fixes**:
Fixes https://github.com/kubernetes-sigs/cluster-api/issues/3151


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cluster-api/pull/3152#" title="Author self-approved">fabriziopandini</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api/blob/master/OWNERS)~~ [fabriziopandini]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
fabriziopandini: /hold
this is going to change given that we will compute Summary on a list of conditions defined upfront; after that the WithCounter option will rely on the same list to define the To value without requiring any value in input

Patch:
diff --git a/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go b/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go
index 6c9b9963c..67291cb14 100644
--- a/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go
+++ b/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go
@@ -199,7 +199,13 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 	// Attempt to Patch the KubeadmConfig object and status after each reconciliation if no error occurs.
 	defer func() {
 		// always update the readyCondition; the summary is represented using the "1 of x completed" notation.
-		conditions.SetSummary(config, conditions.WithStepCounter(1))
+		conditions.SetSummary(config,
+			conditions.WithConditions(
+				bootstrapv1.DataSecretAvailableCondition,
+				bootstrapv1.CertificatesAvailableCondition,
+			),
+			conditions.WithStepCounter(),
+		)
 
 		if err := patchHelper.Patch(ctx, config); err != nil {
 			log.Error(rerr, "Failed to patch config")
@@ -286,7 +292,7 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex
 	// initialize the DataSecretAvailableCondition if missing.
 	// this is required in order to avoid the condition's LastTransitionTime to flicker in case of errors surfacing
 	// using the DataSecretGeneratedFailedReason
-	if !(conditions.Has(scope.Config, bootstrapv1.DataSecretAvailableCondition)) {
+	if conditions.GetReason(scope.Config, bootstrapv1.DataSecretAvailableCondition) != bootstrapv1.DataSecretGenerationFailedReason {
 		conditions.MarkFalse(scope.Config, bootstrapv1.DataSecretAvailableCondition, bootstrapv1.WaitingForControlPlaneAvailableReason, clusterv1.ConditionSeverityInfo, "")
 	}
 
diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go
index c686570c2..248c3706d 100644
--- a/controllers/machine_controller.go
+++ b/controllers/machine_controller.go
@@ -167,11 +167,17 @@ func (r *MachineReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr e
 	}
 
 	defer func() {
-		// always update the readyCondition with the summary of the machine conditions.
+		// Always update the readyCondition with the summary of the machine conditions.
 		conditions.SetSummary(m,
-			// we want to surface infrastructure problems first, then the others.
-			conditions.WithConditionOrder(clusterv1.InfrastructureReadyCondition),
-			conditions.WithStepCounter(clusterv1.MachineSummaryConditionsCount),
+			conditions.WithConditions(
+				clusterv1.BootstrapReadyCondition,
+				clusterv1.InfrastructureReadyCondition,
+				// TODO: add MHC conditions here
+			),
+			conditions.WithStepCounterIfOnly(
+				clusterv1.BootstrapReadyCondition,
+				clusterv1.InfrastructureReadyCondition,
+			),
 		)
 
 		r.reconcilePhase(ctx, m)
diff --git a/util/conditions/getter.go b/util/conditions/getter.go
index 24ef2a626..8be83a84a 100644
--- a/util/conditions/getter.go
+++ b/util/conditions/getter.go
@@ -119,21 +119,65 @@ func GetLastTransitionTime(from Getter, t clusterv1.ConditionType) *metav1.Time
 func summary(from Getter, options ...MergeOption) *clusterv1.Condition {
 	conditions := from.GetConditions()
 
+	mergeOpt := &mergeOptions{}
+	for _, o := range options {
+		o(mergeOpt)
+	}
+
+	// Identifies the conditions in scope for the Summary by taking all the existing conditions except Ready,
+	// or, if a list of conditions types is specified, only the conditions the condition in that list.
 	conditionsInScope := make([]localizedCondition, 0, len(conditions))
 	for i := range conditions {
 		c := conditions[i]
-		if c.Type != clusterv1.ReadyCondition {
-			conditionsInScope = append(conditionsInScope, localizedCondition{
-				Condition: &c,
-				Getter:    from,
-			})
+		if c.Type == clusterv1.ReadyCondition {
+			continue
 		}
+
+		if mergeOpt.conditionTypes != nil {
+			found := false
+			for _, t := range mergeOpt.conditionTypes {
+				if c.Type == t {
+					found = true
+					break
+				}
+			}
+			if !found {
+				continue
+			}
+		}
+
+		conditionsInScope = append(conditionsInScope, localizedCondition{
+			Condition: &c,
+			Getter:    from,
+		})
 	}
 
-	mergeOpt := &mergeOptions{}
-	for _, o := range options {
-		o(mergeOpt)
+	// If it is required to add a step counter only if a subset of condition exists, check if the conditions
+	// in scope are included in this subset.
+	if mergeOpt.addStepCounterIfOnlyConditionTypes != nil {
+		mergeOpt.addStepCounter = true
+		for _, c := range conditionsInScope {
+			for _, t := range mergeOpt.addStepCounterIfOnlyConditionTypes {
+				if c.Type != t {
+					mergeOpt.addStepCounter = false
+					break
+				}
+			}
+		}
 	}
+
+	// If it is required to add a step counter, determine the total number of conditions defaulting
+	// to the selected conditions or, if defined, to the total number of conditions type to be considered.
+	if mergeOpt.addStepCounter {
+		mergeOpt.stepCounter = len(conditionsInScope)
+		if mergeOpt.conditionTypes != nil {
+			mergeOpt.stepCounter = len(mergeOpt.conditionTypes)
+		}
+		if mergeOpt.addStepCounterIfOnlyConditionTypes != nil {
+			mergeOpt.stepCounter = len(mergeOpt.addStepCounterIfOnlyConditionTypes)
+		}
+	}
+
 	return merge(conditionsInScope, clusterv1.ReadyCondition, mergeOpt)
 }
 
@@ -200,7 +244,8 @@ func aggregate(from []Getter, targetCondition clusterv1.ConditionType, options .
 	}
 
 	mergeOpt := &mergeOptions{
-		stepCounter: len(from),
+		addStepCounter: true,
+		stepCounter:    len(from),
 	}
 	for _, o := range options {
 		o(mergeOpt)
diff --git a/util/conditions/merge_strategies.go b/util/conditions/merge_strategies.go
index 5bc5e1ae1..6e9f4faf7 100644
--- a/util/conditions/merge_strategies.go
+++ b/util/conditions/merge_strategies.go
@@ -26,29 +26,47 @@ import (
 // mergeOptions allows to set strategies for merging a set of conditions into a single condition,
 // and more specifically for computing the target Reason and the target Message.
 type mergeOptions struct {
-	conditionOrder []clusterv1.ConditionType
-	addSourceRef   bool
-	stepCounter    int
+	conditionTypes                     []clusterv1.ConditionType
+	addSourceRef                       bool
+	addStepCounter                     bool
+	addStepCounterIfOnlyConditionTypes []clusterv1.ConditionType
+	stepCounter                        int
 }
 
 // MergeOption defines an option for computing a summary of conditions.
 type MergeOption func(*mergeOptions)
 
-// WithConditionOrder instructs merge about the condition order to be used when
-// merging conditions Reason and Message into the Target Condition.
-// The remaining conditions (not included in this list) will be sorted by type, and in case of
-// two conditions with the same type, by the source object name.
-func WithConditionOrder(t ...clusterv1.ConditionType) MergeOption {
+// WithConditions instructs merge about the condition types to consider when doing a merge operation;
+// if this option is not specified, all the conditions (excepts Ready) will be considered. This is required
+// so we can provide some guarantees about the semantic of the target condition without worrying about
+// side effects if someone or something adds custom conditions to the objects.
+//
+// NOTE: The order of conditions types defines the priority for determining the Reason and Message for the
+// target condition.
+// IMPORTANT: This options works only while generating the Summary condition.
+func WithConditions(t ...clusterv1.ConditionType) MergeOption {
 	return func(c *mergeOptions) {
-		c.conditionOrder = t
+		c.conditionTypes = t
 	}
 }
 
 // WithStepCounter instructs merge to add a "x of y completed" string to the message,
-// where x is the number of conditions with Status=true and y is the number passed to this method.
-func WithStepCounter(to int) MergeOption {
+// where x is the number of conditions with Status=true and y is the number of conditions in scope.
+func WithStepCounter() MergeOption {
 	return func(c *mergeOptions) {
-		c.stepCounter = to
+		c.addStepCounter = true
+	}
+}
+
+// If it is required to add a step counter only if a subset of condition exists, check if the conditions
+// in scope are included in this subset. This applies for example on Machines, where we want to use
+// the step counter notation while provisioning the machine, but then we want to move away from this notation
+// as soon as the machine is provisioned and e.g. a Machine health check condition is generated
+//
+// IMPORTANT: This options works only while generating the Summary condition.
+func WithStepCounterIfOnly(t ...clusterv1.ConditionType) MergeOption {
+	return func(c *mergeOptions) {
+		c.addStepCounterIfOnlyConditionTypes = t
 	}
 }
 
@@ -62,7 +80,7 @@ func AddSourceRef() MergeOption {
 // getReason returns the reason to be applied to the condition resulting by merging a set of condition groups.
 // The reason is computed according to the given mergeOptions.
 func getReason(groups conditionGroups, options *mergeOptions) string {
-	return getFirstReason(groups, options.conditionOrder, options.addSourceRef)
+	return getFirstReason(groups, options.conditionTypes, options.addSourceRef)
 }
 
 // getFirstReason returns the first reason from the ordered list of conditions in the top group.
@@ -91,7 +109,7 @@ func localizeReason(reason string, from Getter) string {
 // summary of existing errors is automatically added.
 func getMessage(groups conditionGroups, options *mergeOptions) string {
 	errorAndWarningSummary := getErrorAndWarningSummary(groups)
-	if options.stepCounter > 0 {
+	if options.addStepCounter {
 		counter := getStepCounterMessage(groups, options.stepCounter)
 		if errorAndWarningSummary != "" {
 			return fmt.Sprintf("%s, %s", counter, errorAndWarningSummary)
@@ -100,7 +118,7 @@ func getMessage(groups conditionGroups, options *mergeOptions) string {
 
 	}
 
-	firstMessage := getFirstMessage(groups, options.conditionOrder)
+	firstMessage := getFirstMessage(groups, options.conditionTypes)
 	if errorAndWarningSummary != "" {
 		return fmt.Sprintf("%s (%s)", firstMessage, errorAndWarningSummary)
 	}


Test patch:
diff --git a/test/infrastructure/docker/controllers/dockercluster_controller.go b/test/infrastructure/docker/controllers/dockercluster_controller.go
index 17cbd7b67..ab76a024d 100644
--- a/test/infrastructure/docker/controllers/dockercluster_controller.go
+++ b/test/infrastructure/docker/controllers/dockercluster_controller.go
@@ -91,7 +91,12 @@ func (r *DockerClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 	// Always attempt to Patch the DockerCluster object and status after each reconciliation.
 	defer func() {
 		// always update the readyCondition; the summary is represented using the "1 of x completed" notation.
-		conditions.SetSummary(dockerCluster, conditions.WithStepCounter(1))
+		conditions.SetSummary(dockerCluster,
+			conditions.WithConditions(
+				infrav1.LoadBalancerAvailableCondition,
+			),
+			conditions.WithStepCounter(),
+		)
 
 		if err := patchHelper.Patch(ctx, dockerCluster); err != nil {
 			log.Error(err, "failed to patch DockerCluster")
diff --git a/test/infrastructure/docker/controllers/dockermachine_controller.go b/test/infrastructure/docker/controllers/dockermachine_controller.go
index f19e3728c..f7e45c42a 100644
--- a/test/infrastructure/docker/controllers/dockermachine_controller.go
+++ b/test/infrastructure/docker/controllers/dockermachine_controller.go
@@ -117,7 +117,13 @@ func (r *DockerMachineReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 	// Always attempt to Patch the DockerMachine object and status after each reconciliation.
 	defer func() {
 		// always update the readyCondition; the summary is represented using the "1 of x completed" notation.
-		conditions.SetSummary(dockerMachine, conditions.WithStepCounter(infrav1.ConditionsCount))
+		conditions.SetSummary(dockerMachine,
+			conditions.WithConditions(
+				infrav1.ContainerProvisionedCondition,
+				infrav1.BootstrapExecSucceededCondition,
+			),
+			conditions.WithStepCounter(),
+		)
 
 		if err := patchHelper.Patch(ctx, dockerMachine); err != nil {
 			log.Error(err, "failed to patch DockerMachine")
diff --git a/util/conditions/getter_test.go b/util/conditions/getter_test.go
index ea5ae3e04..6c77e4d41 100644
--- a/util/conditions/getter_test.go
+++ b/util/conditions/getter_test.go
@@ -133,12 +133,14 @@ func TestMirror(t *testing.T) {
 func TestSummary(t *testing.T) {
 	foo := TrueCondition("foo")
 	bar := FalseCondition("bar", "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1")
+	baz := FalseCondition("baz", "reason falseInfo2", clusterv1.ConditionSeverityInfo, "message falseInfo2")
 	existingReady := FalseCondition(clusterv1.ReadyCondition, "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1") //NB. existing ready has higher priority than other conditions
 
 	tests := []struct {
-		name string
-		from Getter
-		want *clusterv1.Condition
+		name    string
+		from    Getter
+		options []MergeOption
+		want    *clusterv1.Condition
 	}{
 		{
 			name: "Returns nil when there are no conditions to summarize",
@@ -150,6 +152,42 @@ func TestSummary(t *testing.T) {
 			from: getterWithConditions(foo, bar),
 			want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"),
 		},
+		{
+			name:    "Returns ready condition with the summary of existing conditions (using WithStepCounter options)",
+			from:    getterWithConditions(foo, bar),
+			options: []MergeOption{WithStepCounter()},
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "1 of 2 completed"),
+		},
+		{
+			name:    "Returns ready condition with the summary of selected conditions (using WithConditions options)",
+			from:    getterWithConditions(foo, bar),
+			options: []MergeOption{WithConditions("foo")}, // bar should be ignored
+			want:    TrueCondition(clusterv1.ReadyCondition),
+		},
+		{
+			name:    "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounter options)",
+			from:    getterWithConditions(foo, bar, baz),
+			options: []MergeOption{WithConditions("foo", "bar"), WithStepCounter()}, // baz should be ignored, total steps should be 2
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "1 of 2 completed"),
+		},
+		{
+			name:    "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounterIfOnlyConditions options)",
+			from:    getterWithConditions(bar),
+			options: []MergeOption{WithConditions("bar", "baz"), WithStepCounterIfOnly("bar")}, // there is only bar, the step counter should be set and counts only a subset of conditions
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "0 of 1 completed"),
+		},
+		{
+			name:    "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounterIfOnlyConditions options)",
+			from:    getterWithConditions(bar, baz),
+			options: []MergeOption{WithConditions("bar", "baz"), WithStepCounterIfOnly("bar")}, // there is also baz, so the step counter should not be set
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"),
+		},
+		{
+			name:    "Ready condition respects merge order",
+			from:    getterWithConditions(bar, baz),
+			options: []MergeOption{WithConditions("baz", "bar")}, // baz should take precedence on bar
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo2", clusterv1.ConditionSeverityInfo, "message falseInfo2"),
+		},
 		{
 			name: "Ignores existing Ready condition when computing the summary",
 			from: getterWithConditions(existingReady, foo, bar),
@@ -161,7 +199,7 @@ func TestSummary(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			got := summary(tt.from)
+			got := summary(tt.from, tt.options...)
 			if tt.want == nil {
 				g.Expect(got).To(BeNil())
 				return