From 482efba8def114f7bb815c19557f2f46b61ecf49 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Thu, 17 Mar 2016 15:39:05 -0700 Subject: [PATCH] Copy annotations back from RS to Deployment on rollback --- .../deployment/deployment_controller.go | 47 +++++++++++++++++-- test/e2e/deployment.go | 20 ++++++++ test/e2e/util.go | 19 ++++++++ 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index df37cada5f744..c81df83793dd3 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/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) diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go index 4bd048f88db18..0c5ac85fe48df 100644 --- a/test/e2e/deployment.go +++ b/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. diff --git a/test/e2e/util.go b/test/e2e/util.go index e32fb2b42bd15..1b7ca9482dd3a 100644 --- a/test/e2e/util.go +++ b/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}