Address smoe more review comments

Signed-off-by: Stefan Büringer buringerst@vmware.com
kubernetes-sigs · Oct 18, 2024 · 375bac1 · 375bac1
1 parent fd0f4e2
commit 375bac1
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 6 deletions.
diff --git a/internal/controllers/machine/machine_controller_status.go b/internal/controllers/machine/machine_controller_status.go
@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/clustercache"
@@ -73,8 +74,6 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, s *scope) {
 
 	setAvailableCondition(ctx, s.machine)
 
-	// TODO: Update the Deleting condition.
-
 	setMachinePhaseAndLastUpdated(ctx, s.machine)
 }
 
@@ -586,10 +585,10 @@ func setReadyCondition(ctx context.Context, machine *clusterv1.Machine) {
 // calculateDeletingConditionForSummary calculates a Deleting condition for the calculation of the Ready condition
 // (which is done via a summary). This is necessary to avoid including the verbose details of the Deleting condition
 // message in the summary.
-// This is important to ensure we have a limited amount of unique messages across Machines. This allows us to deduplicate
-// messages when aggregating Ready conditions of many Machines into the MachinesReady condition of e.g. the MachineSet.
+// This is also important to ensure we have a limited amount of unique messages across Machines thus allowing to
+// nicely aggregate Ready conditions from many Machines into the MachinesReady condition of e.g. the MachineSet.
 // For the same reason we are only surfacing messages with "more than 30m" instead of using the exact durations.
-// We assume that 30m is a duration after which it makes sense to notify users that Node drains and waiting for volume
+// 30 minutes is a duration after which we assume it makes sense to emphasize that Node drains and waiting for volume
 // detach are still in progress.
 func calculateDeletingConditionForSummary(machine *clusterv1.Machine) v1beta2conditions.ConditionWithOwnerInfo {
 	deletingCondition := v1beta2conditions.Get(machine, clusterv1.MachineDeletingV1Beta2Condition)

diff --git a/util/conditions/v1beta2/options.go b/util/conditions/v1beta2/options.go
@@ -79,7 +79,8 @@ func (t IgnoreTypesIfMissing) ApplyToSummary(opts *SummaryOptions) {
 	opts.ignoreTypesIfMissing = t
 }
 
-// OverrideConditions allows to override conditions from the source object that should be used in the summary.
+// OverrideConditions allows to override conditions read from the source object only for the scope of a summary operation.
+// The condition on the source object will preserve the original value.
 type OverrideConditions []ConditionWithOwnerInfo
 
 // ApplyToSummary applies this configuration to the given summary options.

diff --git a/util/conditions/v1beta2/summary.go b/util/conditions/v1beta2/summary.go
@@ -83,6 +83,10 @@ func NewSummaryCondition(sourceObj Getter, targetConditionType string, opts ...S
 	}
 	overrideConditionsByType := map[string]ConditionWithOwnerInfo{}
 	for _, c := range summarizeOpt.overrideConditions {
+		if _, ok := overrideConditionsByType[c.Type]; ok {
+			return nil, errors.Errorf("override condition %s specified multiple times", c.Type)
+		}
+
 		overrideConditionsByType[c.Type] = c
 
 		if _, ok := conditionsByType[c.Type]; !ok {

diff --git a/util/conditions/v1beta2/summary_test.go b/util/conditions/v1beta2/summary_test.go
@@ -236,6 +236,36 @@ func TestSummary(t *testing.T) {
 				Message: "!C: Message-C-additional", // Picking the message from the additional condition (info dropped)
 			},
 		},
+		{
+			name: "Error if the same override condition is specified multiple times",
+			conditions: []metav1.Condition{
+				{Type: "A", Status: metav1.ConditionTrue, Reason: "Reason-A", Message: "Message-A"},  // info
+				{Type: "!C", Status: metav1.ConditionTrue, Reason: "Reason-C", Message: "Message-C"}, // issue
+			},
+			conditionType: clusterv1.AvailableV1Beta2Condition,
+			options: []SummaryOption{ForConditionTypes{"A", "!C"}, NegativePolarityConditionTypes{"!C"}, IgnoreTypesIfMissing{"!C"},
+				OverrideConditions{
+					{
+						OwnerResource: ConditionOwnerInfo{
+							Kind: "Phase3Obj",
+							Name: "SourceObject",
+						},
+						Condition: metav1.Condition{
+							Type: "!C", Status: metav1.ConditionTrue, Reason: "Reason-C-additional", Message: "Message-C-additional", // issue
+						},
+					},
+					{
+						OwnerResource: ConditionOwnerInfo{
+							Kind: "Phase3Obj",
+							Name: "SourceObject",
+						},
+						Condition: metav1.Condition{
+							Type: "!C", Status: metav1.ConditionTrue, Reason: "Reason-C-additional", Message: "Message-C-additional", // issue
+						},
+					},
+				}}, // OverrideCondition is specified multiple times
+			wantErr: true,
+		},
 		{
 			name: "Error if override condition does not exist in source object",
 			conditions: []metav1.Condition{