Address some more review comments

Signed-off-by: Stefan Büringer [email protected]

Author: sbueringer

internal/controllers/machine/machine_controller.go

@@ -100,7 +100,11 @@ type Reconciler struct {
 	// during a single reconciliation.
 	nodeDeletionRetryTimeout time.Duration
 	ssaCache                 ssa.Cache
-	reconcileDeleteCache     cache.Cache[cache.ReconcileEntry]
+
+	// reconcileDeleteCache is used to store when reconcileDelete should not be executed before a
+	// specific time for a specific Request. This is used to implement rate-limiting to avoid
+	// e.g. spamming workload clusters with eviction requests during Node drain.
+	reconcileDeleteCache cache.Cache[cache.ReconcileEntry]
 }
 
 func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {

internal/controllers/machine/machine_controller_status.go

@@ -536,7 +536,7 @@ func setReadyCondition(ctx context.Context, machine *clusterv1.Machine) {
 	log := ctrl.LoggerFrom(ctx)
 
 	forConditionTypes := v1beta2conditions.ForConditionTypes{
-		// MachineDeletingV1Beta2Condition is added via AdditionalConditions if DeletionTimestamp is set.
+		clusterv1.MachineDeletingV1Beta2Condition,
 		clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,
 		clusterv1.MachineInfrastructureReadyV1Beta2Condition,
 		clusterv1.MachineNodeHealthyV1Beta2Condition,
@@ -564,7 +564,7 @@ func setReadyCondition(ctx context.Context, machine *clusterv1.Machine) {
 	}
 
 	if !machine.DeletionTimestamp.IsZero() {
-		summaryOpts = append(summaryOpts, v1beta2conditions.AdditionalConditions{calculateDeletingConditionForSummary(machine)})
+		summaryOpts = append(summaryOpts, v1beta2conditions.OverrideConditions{calculateDeletingConditionForSummary(machine)})
 	}
 
 	readyCondition, err := v1beta2conditions.NewSummaryCondition(machine, clusterv1.MachineReadyV1Beta2Condition, summaryOpts...)
@@ -586,6 +586,11 @@ 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.
+// 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
+// detach are still in progress.
 func calculateDeletingConditionForSummary(machine *clusterv1.Machine) v1beta2conditions.ConditionWithOwnerInfo {
 	deletingCondition := v1beta2conditions.Get(machine, clusterv1.MachineDeletingV1Beta2Condition)
 

util/conditions/v1beta2/options.go

@@ -79,12 +79,12 @@ func (t IgnoreTypesIfMissing) ApplyToSummary(opts *SummaryOptions) {
 	opts.ignoreTypesIfMissing = t
 }
 
-// AdditionalConditions allows to add additional conditions that should be used in the summary.
-type AdditionalConditions []ConditionWithOwnerInfo
+// OverrideConditions allows to override conditions from the source object that should be used in the summary.
+type OverrideConditions []ConditionWithOwnerInfo
 
 // ApplyToSummary applies this configuration to the given summary options.
-func (t AdditionalConditions) ApplyToSummary(opts *SummaryOptions) {
-	opts.additionalConditions = t
+func (t OverrideConditions) ApplyToSummary(opts *SummaryOptions) {
+	opts.overrideConditions = t
 }
 
 // CustomMergeStrategy allows to define a custom merge strategy when creating new summary or aggregate conditions.

util/conditions/v1beta2/summary.go

@@ -36,7 +36,7 @@ type SummaryOptions struct {
 	conditionTypes                 []string
 	negativePolarityConditionTypes []string
 	ignoreTypesIfMissing           []string
-	additionalConditions           []ConditionWithOwnerInfo
+	overrideConditions             []ConditionWithOwnerInfo
 }
 
 // ApplyOptions applies the given list options on these options,
@@ -74,23 +74,36 @@ func NewSummaryCondition(sourceObj Getter, targetConditionType string, opts ...S
 	expectedConditionTypes := sets.New[string](summarizeOpt.conditionTypes...)
 	ignoreTypesIfMissing := sets.New[string](summarizeOpt.ignoreTypesIfMissing...)
 	existingConditionTypes := sets.New[string]()
-	allConditionTypes := summarizeOpt.conditionTypes
 
-	// Drops all the conditions not in scope for the merge operation
 	conditions := getConditionsWithOwnerInfo(sourceObj)
+
+	conditionsByType := map[string]ConditionWithOwnerInfo{}
+	for _, c := range conditions {
+		conditionsByType[c.Type] = c
+	}
+	overrideConditionsByType := map[string]ConditionWithOwnerInfo{}
+	for _, c := range summarizeOpt.overrideConditions {
+		overrideConditionsByType[c.Type] = c
+
+		if _, ok := conditionsByType[c.Type]; !ok {
+			return nil, errors.Errorf("override condition %s must exist in source object", c.Type)
+		}
+	}
+
 	conditionsInScope := make([]ConditionWithOwnerInfo, 0, len(expectedConditionTypes))
 	for _, condition := range conditions {
+		// Drops all the conditions not in scope for the merge operation
 		if !expectedConditionTypes.Has(condition.Type) {
 			continue
 		}
-		conditionsInScope = append(conditionsInScope, condition)
-		existingConditionTypes.Insert(condition.Type)
-	}
 
-	for _, condition := range summarizeOpt.additionalConditions {
-		conditionsInScope = append(conditionsInScope, condition)
+		if overrideCondition, ok := overrideConditionsByType[condition.Type]; ok {
+			conditionsInScope = append(conditionsInScope, overrideCondition)
+		} else {
+			conditionsInScope = append(conditionsInScope, condition)
+		}
+
 		existingConditionTypes.Insert(condition.Type)
-		allConditionTypes = append(allConditionTypes, condition.Type)
 	}
 
 	// Add the expected conditions which do not exist, so we are compliant with K8s guidelines
@@ -118,7 +131,7 @@ func NewSummaryCondition(sourceObj Getter, targetConditionType string, opts ...S
 		return nil, errors.New("summary can't be performed when the list of conditions to be summarized is empty")
 	}
 
-	status, reason, message, err := summarizeOpt.mergeStrategy.Merge(conditionsInScope, allConditionTypes)
+	status, reason, message, err := summarizeOpt.mergeStrategy.Merge(conditionsInScope, summarizeOpt.conditionTypes)
 	if err != nil {
 		return nil, err
 	}

util/conditions/v1beta2/summary_test.go

@@ -34,6 +34,7 @@ func TestSummary(t *testing.T) {
 		conditionType string
 		options       []SummaryOption
 		want          *metav1.Condition
+		wantErr       bool
 	}{
 		{
 			name: "One issue",
@@ -210,14 +211,14 @@ func TestSummary(t *testing.T) {
 			},
 		},
 		{
-			name: "Include additional condition",
+			name: "Override condition",
 			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"}, NegativePolarityConditionTypes{"!C"}, IgnoreTypesIfMissing{"!C"},
-				AdditionalConditions{
+			options: []SummaryOption{ForConditionTypes{"A", "!C"}, NegativePolarityConditionTypes{"!C"}, IgnoreTypesIfMissing{"!C"},
+				OverrideConditions{
 					{
 						OwnerResource: ConditionOwnerInfo{
 							Kind: "Phase3Obj",
@@ -227,14 +228,35 @@ func TestSummary(t *testing.T) {
 							Type: "!C", Status: metav1.ConditionTrue, Reason: "Reason-C-additional", Message: "Message-C-additional", // issue
 						},
 					},
-				}}, // AdditionalCondition replaces the same condition from the SourceObject
+				}}, // OverrideCondition replaces the same condition from the SourceObject
 			want: &metav1.Condition{
 				Type:    clusterv1.AvailableV1Beta2Condition,
 				Status:  metav1.ConditionFalse,      // False because !C is an issue
 				Reason:  "Reason-C-additional",      // Picking the reason from the additional condition
 				Message: "!C: Message-C-additional", // Picking the message from the additional condition (info dropped)
 			},
 		},
+		{
+			name: "Error if override condition does not exist in source object",
+			conditions: []metav1.Condition{
+				{Type: "A", Status: metav1.ConditionTrue, Reason: "Reason-A", Message: "Message-A"}, // info
+				// !C is missing in source object
+			},
+			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
+						},
+					},
+				}},
+			wantErr: true,
+		},
 	}
 
 	for _, tt := range tests {
@@ -252,7 +274,7 @@ func TestSummary(t *testing.T) {
 			}
 
 			got, err := NewSummaryCondition(obj, tt.conditionType, tt.options...)
-			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(err != nil).To(Equal(tt.wantErr))
 
 			g.Expect(got).To(Equal(tt.want))
 		})