Merge pull request #836 from tkashem/csv-conditions-length

Set limit on length of Status.Conditions of a csv

Author: openshift-merge-robot

pkg/api/apis/operators/v1alpha1/clusterserviceversion.go

@@ -10,6 +10,11 @@ import (
 
 const (
 	CopiedLabelKey = "olm.copiedFrom"
+
+	// ConditionsLengthLimit is the maximum length of Status.Conditions of a
+	// given ClusterServiceVersion object. The oldest condition(s) are removed
+	// from the list as it grows over time to keep it at limit.
+	ConditionsLengthLimit = 20
 )
 
 // obsoleteReasons are the set of reasons that mean a CSV should no longer be processed as active
@@ -67,6 +72,18 @@ func (c *ClusterServiceVersion) SetPhaseWithEvent(phase ClusterServiceVersionPha
 
 // SetPhase sets the current phase and adds a condition if necessary
 func (c *ClusterServiceVersion) SetPhase(phase ClusterServiceVersionPhase, reason ConditionReason, message string, now metav1.Time) {
+	newCondition := func() ClusterServiceVersionCondition {
+		return ClusterServiceVersionCondition{
+			Phase:              c.Status.Phase,
+			LastTransitionTime: c.Status.LastTransitionTime,
+			LastUpdateTime:     c.Status.LastUpdateTime,
+			Message:            message,
+			Reason:             reason,
+		}
+	}
+
+	defer c.TrimConditionsIfLimitExceeded()
+
 	c.Status.LastUpdateTime = now
 	if c.Status.Phase != phase {
 		c.Status.Phase = phase
@@ -75,23 +92,13 @@ func (c *ClusterServiceVersion) SetPhase(phase ClusterServiceVersionPhase, reaso
 	c.Status.Message = message
 	c.Status.Reason = reason
 	if len(c.Status.Conditions) == 0 {
-		c.Status.Conditions = append(c.Status.Conditions, ClusterServiceVersionCondition{
-			Phase:              c.Status.Phase,
-			LastTransitionTime: c.Status.LastTransitionTime,
-			LastUpdateTime:     c.Status.LastUpdateTime,
-			Message:            message,
-			Reason:             reason,
-		})
+		c.Status.Conditions = append(c.Status.Conditions, newCondition())
+		return
 	}
+
 	previousCondition := c.Status.Conditions[len(c.Status.Conditions)-1]
 	if previousCondition.Phase != c.Status.Phase || previousCondition.Reason != c.Status.Reason {
-		c.Status.Conditions = append(c.Status.Conditions, ClusterServiceVersionCondition{
-			Phase:              c.Status.Phase,
-			LastTransitionTime: c.Status.LastTransitionTime,
-			LastUpdateTime:     c.Status.LastUpdateTime,
-			Message:            message,
-			Reason:             reason,
-		})
+		c.Status.Conditions = append(c.Status.Conditions, newCondition())
 	}
 }
 
@@ -190,3 +197,12 @@ func (set InstallModeSet) Supports(operatorNamespace string, namespaces []string
 
 	return nil
 }
+
+func (c *ClusterServiceVersion) TrimConditionsIfLimitExceeded() {
+	if len(c.Status.Conditions) <= ConditionsLengthLimit {
+		return
+	}
+
+	firstIndex := len(c.Status.Conditions) - ConditionsLengthLimit
+	c.Status.Conditions = c.Status.Conditions[firstIndex:len(c.Status.Conditions)]
+}

pkg/api/apis/operators/v1alpha1/clusterserviceversion_test.go

@@ -4,9 +4,11 @@ import (
 	"fmt"
 	"testing"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestSetRequirementStatus(t *testing.T) {
@@ -282,3 +284,93 @@ func TestSupports(t *testing.T) {
 		})
 	}
 }
+
+func TestSetPhaseWithConditions(t *testing.T) {
+	tests := []struct {
+		description   string
+		limit         int
+		currentLength int
+		startIndex    int
+	}{
+		{
+			// The original list is already at limit (length == limit).
+			// We expect the oldest element ( item at 0 index) to be removed.
+			description:   "TestSetPhaseWithConditionsLengthAtLimit",
+			limit:         ConditionsLengthLimit,
+			currentLength: ConditionsLengthLimit,
+
+			// The first element from the original list should be dropped from
+			// the new list.
+			startIndex: 1,
+		},
+		{
+			// The original list is 1 length away from limit.
+			// We don't expect the list to be trimmed.
+			description:   "TestSetPhaseWithConditionsLengthBelowLimit",
+			limit:         ConditionsLengthLimit,
+			currentLength: ConditionsLengthLimit - 1,
+
+			// Everything in the original list should be preserved.
+			startIndex: 0,
+		},
+		{
+			// The original list has N more element(s) than allowed limit.
+			// We expect (N + 1) oldest elements to be deleted to keep the list
+			// at limit.
+			description:   "TestSetPhaseWithConditionsLimitExceeded",
+			limit:         ConditionsLengthLimit,
+			currentLength: ConditionsLengthLimit + 10,
+
+			// The first 11 (N=10 plus 1 to make room for the newly added
+			// condition) elements from the original list should be dropped.
+			startIndex: 11,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			csv := ClusterServiceVersion{}
+			csv.Status.Conditions = helperNewConditions(tt.currentLength)
+
+			now := metav1.Now()
+
+			oldConditionsWant := csv.Status.Conditions[tt.startIndex:]
+			lastAddedConditionWant := ClusterServiceVersionCondition{
+				Phase:              ClusterServiceVersionPhase("Pending"),
+				LastTransitionTime: now,
+				LastUpdateTime:     now,
+				Message:            "message",
+				Reason:             ConditionReason("reason"),
+			}
+
+			csv.SetPhase("Pending", "reason", "message", now)
+
+			conditionsGot := csv.Status.Conditions
+			assert.Equal(t, tt.limit, len(conditionsGot))
+
+			oldConditionsGot := conditionsGot[0 : len(conditionsGot)-1]
+			assert.EqualValues(t, oldConditionsWant, oldConditionsGot)
+
+			lastAddedConditionGot := conditionsGot[len(conditionsGot)-1]
+			assert.Equal(t, lastAddedConditionWant, lastAddedConditionGot)
+		})
+	}
+}
+
+func helperNewConditions(count int) []ClusterServiceVersionCondition {
+	conditions := make([]ClusterServiceVersionCondition, 0)
+
+	for i := 1; i <= count; i++ {
+		now := metav1.Now()
+		condition := ClusterServiceVersionCondition{
+			Phase:              ClusterServiceVersionPhase(fmt.Sprintf("phase-%d", i)),
+			LastTransitionTime: now,
+			LastUpdateTime:     now,
+			Message:            fmt.Sprintf("message-%d", i),
+			Reason:             ConditionReason(fmt.Sprintf("reason-%d", i)),
+		}
+		conditions = append(conditions, condition)
+	}
+
+	return conditions
+}