From 9546fd540eebbc6e93c74c4a1bccce9b87b88ca1 Mon Sep 17 00:00:00 2001 From: Ted Yu Date: Wed, 8 May 2019 10:14:16 -0700 Subject: [PATCH 1/2] Impose length limit when concatenating revision history --- pkg/controller/deployment/sync.go | 8 ++++++-- .../deployment/util/deployment_util.go | 19 +++++++++++++++---- .../deployment/util/deployment_util_test.go | 12 +++++++++--- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index b59aec417db..a92904dd3d5 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -124,6 +124,10 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *apps.Deploym return newRS, allOldRSs, nil } +const ( + 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 +149,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 +213,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. diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index a731cc7bb5e..36e83f28aef 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/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) - newRS.Annotations[RevisionHistoryAnnotation] = strings.Join(oldRevisions, ",") + totalLen := len(revisionHistoryAnnotation) + len(oldRevision) + 1 + // 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. diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 5e21f501f91..38fed0e4fbc 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/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 - for i := 0; i < 20; i++ { + //Try to set the increment revision from 11 through 20 + 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]) } From 9418affa4df2ca15d7ba67507b74c443a8c281d3 Mon Sep 17 00:00:00 2001 From: Ted Yu Date: Tue, 14 May 2019 03:11:19 +0800 Subject: [PATCH 2/2] add comment on rev history length limit --- pkg/controller/deployment/sync.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index a92904dd3d5..b0a6e7e3f14 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -125,6 +125,7 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *apps.Deploym } const ( + // limit revision history length to 100 element (~2000 chars) maxRevHistoryLengthInChars = 2000 )