Merge pull request kubernetes#23160 from janetkuo/rollback-copy-annot…

…ation

Auto commit by PR queue bot

pkg/controller/deployment/deployment_controller.go

@@ -898,6 +898,24 @@ func setNewReplicaSetAnnotations(deployment *extensions.Deployment, newRS *exten
 	return annotationChanged
 }
 
+// skipCopyAnnotation returns true if we should skip copying the annotation with the given annotation key
+// TODO: How to decide which annotations should / should not be copied?
+//       See https://github.com/kubernetes/kubernetes/pull/20035#issuecomment-179558615
+func skipCopyAnnotation(key string) bool {
+	// Skip apply annotations and revision annotations.
+	return key == kubectl.LastAppliedConfigAnnotation || key == deploymentutil.RevisionAnnotation
+}
+
+func getSkippedAnnotations(annotations map[string]string) map[string]string {
+	skippedAnnotations := make(map[string]string)
+	for k, v := range annotations {
+		if skipCopyAnnotation(k) {
+			skippedAnnotations[k] = v
+		}
+	}
+	return skippedAnnotations
+}
+
 // copyDeploymentAnnotationsToReplicaSet copies deployment's annotations to replica set's annotations,
 // and returns true if replica set's annotation is changed.
 // Note that apply and revision annotations are not copied.
@@ -907,13 +925,10 @@ func copyDeploymentAnnotationsToReplicaSet(deployment *extensions.Deployment, rs
 		rs.Annotations = make(map[string]string)
 	}
 	for k, v := range deployment.Annotations {
-		// TODO: How to decide which annotations should / should not be copied?
-		// See https://github.com/kubernetes/kubernetes/pull/20035#issuecomment-179558615
-		// Skip apply annotations and revision annotations.
 		// newRS revision is updated automatically in getNewReplicaSet, and the deployment's revision number is then updated
 		// by copying its newRS revision number. We should not copy deployment's revision to its newRS, since the update of
 		// deployment revision number may fail (revision becomes stale) and the revision number in newRS is more reliable.
-		if k == kubectl.LastAppliedConfigAnnotation || k == deploymentutil.RevisionAnnotation || rs.Annotations[k] == v {
+		if skipCopyAnnotation(k) || rs.Annotations[k] == v {
 			continue
 		}
 		rs.Annotations[k] = v
@@ -922,6 +937,18 @@ func copyDeploymentAnnotationsToReplicaSet(deployment *extensions.Deployment, rs
 	return rsAnnotationsChanged
 }
 
+// setDeploymentAnnotationsTo sets deployment's annotations as given RS's annotations.
+// This action should be done if and only if the deployment is rolling back to this rs.
+// Note that apply and revision annotations are not changed.
+func setDeploymentAnnotationsTo(deployment *extensions.Deployment, rollbackToRS *extensions.ReplicaSet) {
+	deployment.Annotations = getSkippedAnnotations(deployment.Annotations)
+	for k, v := range rollbackToRS.Annotations {
+		if !skipCopyAnnotation(k) {
+			deployment.Annotations[k] = v
+		}
+	}
+}
+
 func (dc *DeploymentController) updateDeploymentRevision(deployment *extensions.Deployment, revision string) error {
 	if deployment.Annotations == nil {
 		deployment.Annotations = make(map[string]string)
@@ -1237,6 +1264,18 @@ func (dc *DeploymentController) rollbackToTemplate(deployment *extensions.Deploy
 	if !reflect.DeepEqual(deploymentutil.GetNewReplicaSetTemplate(deployment), rs.Spec.Template) {
 		glog.Infof("Rolling back deployment %s to template spec %+v", deployment.Name, rs.Spec.Template.Spec)
 		deploymentutil.SetFromReplicaSetTemplate(deployment, rs.Spec.Template)
+		// set RS (the old RS we'll rolling back to) annotations back to the deployment;
+		// otherwise, the deployment's current annotations (should be the same as current new RS) will be copied to the RS after the rollback.
+		//
+		// For example,
+		// A Deployment has old RS1 with annotation {change-cause:create}, and new RS2 {change-cause:edit}.
+		// Note that both annotations are copied from Deployment, and the Deployment should be annotated {change-cause:edit} as well.
+		// Now, rollback Deployment to RS1, we should update Deployment's pod-template and also copy annotation from RS1.
+		// Deployment is now annotated {change-cause:create}, and we have new RS1 {change-cause:create}, old RS2 {change-cause:edit}.
+		//
+		// If we don't copy the annotations back from RS to deployment on rollback, the Deployment will stay as {change-cause:edit},
+		// and new RS1 becomes {change-cause:edit} (copied from deployment after rollback), old RS2 {change-cause:edit}, which is not correct.
+		setDeploymentAnnotationsTo(deployment, rs)
 		performedRollback = true
 	} else {
 		glog.V(4).Infof("Rolling back to a revision that contains the same template as current deployment %s, skipping rollback...", deployment.Name)

test/e2e/deployment.go

@@ -657,6 +657,8 @@ func testRollbackDeployment(f *Framework) {
 	deploymentStrategyType := extensions.RollingUpdateDeploymentStrategyType
 	Logf("Creating deployment %s", deploymentName)
 	d := newDeployment(deploymentName, deploymentReplicas, deploymentPodLabels, deploymentImageName, deploymentImage, deploymentStrategyType, nil)
+	createAnnotation := map[string]string{"action": "create", "author": "minion"}
+	d.Annotations = createAnnotation
 	_, err := c.Extensions().Deployments(ns).Create(d)
 	Expect(err).NotTo(HaveOccurred())
 	defer stopDeployment(c, f.Client, ns, deploymentName)
@@ -668,12 +670,18 @@ func testRollbackDeployment(f *Framework) {
 	err = waitForDeploymentStatus(c, ns, deploymentName, deploymentReplicas, deploymentReplicas-1, deploymentReplicas+1, 0)
 	Expect(err).NotTo(HaveOccurred())
 
+	// Current newRS annotation should be "create"
+	err = checkNewRSAnnotations(c, ns, deploymentName, createAnnotation)
+	Expect(err).NotTo(HaveOccurred())
+
 	// 2. Update the deployment to create redis pods.
 	updatedDeploymentImage := redisImage
 	updatedDeploymentImageName := redisImageName
+	updateAnnotation := map[string]string{"action": "update", "log": "I need to update it"}
 	deployment, err := updateDeploymentWithRetries(c, ns, d.Name, func(update *extensions.Deployment) {
 		update.Spec.Template.Spec.Containers[0].Name = updatedDeploymentImageName
 		update.Spec.Template.Spec.Containers[0].Image = updatedDeploymentImage
+		update.Annotations = updateAnnotation
 	})
 	Expect(err).NotTo(HaveOccurred())
 
@@ -688,6 +696,10 @@ func testRollbackDeployment(f *Framework) {
 	err = waitForDeploymentStatus(c, ns, deploymentName, deploymentReplicas, deploymentReplicas-1, deploymentReplicas+1, 0)
 	Expect(err).NotTo(HaveOccurred())
 
+	// Current newRS annotation should be "update"
+	err = checkNewRSAnnotations(c, ns, deploymentName, updateAnnotation)
+	Expect(err).NotTo(HaveOccurred())
+
 	// 3. Update the deploymentRollback to rollback to revision 1
 	revision := int64(1)
 	Logf("rolling back deployment %s to revision %d", deploymentName, revision)
@@ -707,6 +719,10 @@ func testRollbackDeployment(f *Framework) {
 	err = waitForDeploymentStatus(c, ns, deploymentName, deploymentReplicas, deploymentReplicas-1, deploymentReplicas+1, 0)
 	Expect(err).NotTo(HaveOccurred())
 
+	// Current newRS annotation should be "create", after the rollback
+	err = checkNewRSAnnotations(c, ns, deploymentName, createAnnotation)
+	Expect(err).NotTo(HaveOccurred())
+
 	// 4. Update the deploymentRollback to rollback to last revision
 	revision = 0
 	Logf("rolling back deployment %s to last revision", deploymentName)
@@ -723,6 +739,10 @@ func testRollbackDeployment(f *Framework) {
 
 	err = waitForDeploymentStatus(c, ns, deploymentName, deploymentReplicas, deploymentReplicas-1, deploymentReplicas+1, 0)
 	Expect(err).NotTo(HaveOccurred())
+
+	// Current newRS annotation should be "update", after the rollback
+	err = checkNewRSAnnotations(c, ns, deploymentName, updateAnnotation)
+	Expect(err).NotTo(HaveOccurred())
 }
 
 // testRollbackDeploymentRSNoRevision tests that deployment supports rollback even when there's old replica set without revision.

test/e2e/util.go

@@ -2620,6 +2620,25 @@ func waitForDeploymentRevisionAndImage(c clientset.Interface, ns, deploymentName
 	return nil
 }
 
+// checkNewRSAnnotations check if the new RS's annotation is as expected
+func checkNewRSAnnotations(c clientset.Interface, ns, deploymentName string, expectedAnnotations map[string]string) error {
+	deployment, err := c.Extensions().Deployments(ns).Get(deploymentName)
+	if err != nil {
+		return err
+	}
+	newRS, err := deploymentutil.GetNewReplicaSet(deployment, c)
+	if err != nil {
+		return err
+	}
+	for k, v := range expectedAnnotations {
+		// Skip checking revision annotations
+		if k != deploymentutil.RevisionAnnotation && v != newRS.Annotations[k] {
+			return fmt.Errorf("Expected new RS annotations = %+v, got %+v", expectedAnnotations, newRS.Annotations)
+		}
+	}
+	return nil
+}
+
 func waitForPodsReady(c *clientset.Clientset, ns, name string, minReadySeconds int) error {
 	label := labels.SelectorFromSet(labels.Set(map[string]string{"name": name}))
 	options := api.ListOptions{LabelSelector: label}