Merge pull request #77610 from tedyu/rev-limit

Impose length limit when concatenating revision history

pkg/controller/deployment/sync.go

@@ -124,6 +124,11 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *apps.Deploym
 	return newRS, allOldRSs, nil
 }
 
+const (
+	// limit revision history length to 100 element (~2000 chars)
+	maxRevHistoryLengthInChars = 2000
+)
+
 // Returns a replica set that matches the intent of the given deployment. Returns nil if the new replica set doesn't exist yet.
 // 1. Get existing new RS (the RS that the given deployment targets, whose pod template is the same as deployment's).
 // 2. If there's existing new RS, update its revision number if it's smaller than (maxOldRevision + 1), where maxOldRevision is the max revision number among all old RSes.
@@ -145,7 +150,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *apps.Deployment, rsList, old
 		rsCopy := existingNewRS.DeepCopy()
 
 		// Set existing new replica set's annotation
-		annotationsUpdated := deploymentutil.SetNewReplicaSetAnnotations(d, rsCopy, newRevision, true)
+		annotationsUpdated := deploymentutil.SetNewReplicaSetAnnotations(d, rsCopy, newRevision, true, maxRevHistoryLengthInChars)
 		minReadySecondsNeedsUpdate := rsCopy.Spec.MinReadySeconds != d.Spec.MinReadySeconds
 		if annotationsUpdated || minReadySecondsNeedsUpdate {
 			rsCopy.Spec.MinReadySeconds = d.Spec.MinReadySeconds
@@ -209,7 +214,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *apps.Deployment, rsList, old
 
 	*(newRS.Spec.Replicas) = newReplicasCount
 	// Set new replica set's annotation
-	deploymentutil.SetNewReplicaSetAnnotations(d, &newRS, newRevision, false)
+	deploymentutil.SetNewReplicaSetAnnotations(d, &newRS, newRevision, false, maxRevHistoryLengthInChars)
 	// Create the new ReplicaSet. If it already exists, then we need to check for possible
 	// hash collisions. If there is any other error, we need to report it in the status of
 	// the Deployment.

pkg/controller/deployment/util/deployment_util.go

@@ -227,7 +227,7 @@ func Revision(obj runtime.Object) (int64, error) {
 
 // SetNewReplicaSetAnnotations sets new replica set's annotations appropriately by updating its revision and
 // copying required deployment annotations to it; it returns true if replica set's annotation is changed.
-func SetNewReplicaSetAnnotations(deployment *apps.Deployment, newRS *apps.ReplicaSet, newRevision string, exists bool) bool {
+func SetNewReplicaSetAnnotations(deployment *apps.Deployment, newRS *apps.ReplicaSet, newRevision string, exists bool, revHistoryLimitInChars int) bool {
 	// First, copy deployment's annotations (except for apply and revision annotations)
 	annotationChanged := copyDeploymentAnnotationsToReplicaSet(deployment, newRS)
 	// Then, update replica set's revision annotation
@@ -261,14 +261,25 @@ func SetNewReplicaSetAnnotations(deployment *apps.Deployment, newRS *apps.Replic
 	// If a revision annotation already existed and this replica set was updated with a new revision
 	// then that means we are rolling back to this replica set. We need to preserve the old revisions
 	// for historical information.
-	if ok && annotationChanged {
+	if ok && oldRevisionInt < newRevisionInt {
 		revisionHistoryAnnotation := newRS.Annotations[RevisionHistoryAnnotation]
 		oldRevisions := strings.Split(revisionHistoryAnnotation, ",")
 		if len(oldRevisions[0]) == 0 {
 			newRS.Annotations[RevisionHistoryAnnotation] = oldRevision
 		} else {
-			oldRevisions = append(oldRevisions, oldRevision)
+			totalLen := len(revisionHistoryAnnotation) + len(oldRevision) + 1
-			newRS.Annotations[RevisionHistoryAnnotation] = strings.Join(oldRevisions, ",")
+			// index for the starting position in oldRevisions
+			start := 0
+			for totalLen > revHistoryLimitInChars && start < len(oldRevisions) {
+				totalLen = totalLen - len(oldRevisions[start]) - 1
+				start++
+			}
+			if totalLen <= revHistoryLimitInChars {
+				oldRevisions = append(oldRevisions[start:], oldRevision)
+				newRS.Annotations[RevisionHistoryAnnotation] = strings.Join(oldRevisions, ",")
+			} else {
+				klog.Warningf("Not appending revision due to length limit of %v reached", revHistoryLimitInChars)
+			}
 		}
 	}
 	// If the new replica set is about to be created, we need to add replica annotations to it.

pkg/controller/deployment/util/deployment_util_test.go

@@ -1274,13 +1274,19 @@ func TestAnnotationUtils(t *testing.T) {
 
 	//Test Case 1: Check if anotations are copied properly from deployment to RS
 	t.Run("SetNewReplicaSetAnnotations", func(t *testing.T) {
-		//Try to set the increment revision from 1 through 20
+		//Try to set the increment revision from 11 through 20
-		for i := 0; i < 20; i++ {
+		for i := 10; i < 20; i++ {
 
 			nextRevision := fmt.Sprintf("%d", i+1)
-			SetNewReplicaSetAnnotations(&tDeployment, &tRS, nextRevision, true)
+			SetNewReplicaSetAnnotations(&tDeployment, &tRS, nextRevision, true, 5)
 			//Now the ReplicaSets Revision Annotation should be i+1
 
+			if i >= 12 {
+				expectedHistoryAnnotation := fmt.Sprintf("%d,%d", i-1, i)
+				if tRS.Annotations[RevisionHistoryAnnotation] != expectedHistoryAnnotation {
+					t.Errorf("Revision History Expected=%s Obtained=%s", expectedHistoryAnnotation, tRS.Annotations[RevisionHistoryAnnotation])
+				}
+			}
 			if tRS.Annotations[RevisionAnnotation] != nextRevision {
 				t.Errorf("Revision Expected=%s Obtained=%s", nextRevision, tRS.Annotations[RevisionAnnotation])
 			}