From 3718749c4ae0876095f8fe555387585c66d6093e Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Tue, 21 Feb 2017 16:00:24 -0800 Subject: [PATCH] Fix deployment helper - no assumptions on only one new ReplicaSet --- pkg/controller/deployment/util/BUILD | 1 + .../deployment/util/deployment_util.go | 19 ++++--- .../deployment/util/deployment_util_test.go | 51 +++++++++++++++++-- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/pkg/controller/deployment/util/BUILD b/pkg/controller/deployment/util/BUILD index e4e79acd7f7..bc1b9b4ed88 100644 --- a/pkg/controller/deployment/util/BUILD +++ b/pkg/controller/deployment/util/BUILD @@ -61,6 +61,7 @@ go_test( "//vendor:k8s.io/apimachinery/pkg/api/equality", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/runtime", + "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apimachinery/pkg/util/intstr", "//vendor:k8s.io/client-go/testing", ], diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 7a19aafa83a..08c540ff5a1 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -612,9 +612,13 @@ func EqualIgnoreHash(template1, template2 v1.PodTemplateSpec) bool { // FindNewReplicaSet returns the new RS this given deployment targets (the one with the same pod template). func FindNewReplicaSet(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) (*extensions.ReplicaSet, error) { newRSTemplate := GetNewReplicaSetTemplate(deployment) + sort.Sort(controller.ReplicaSetsByCreationTimestamp(rsList)) for i := range rsList { if EqualIgnoreHash(rsList[i].Spec.Template, newRSTemplate) { - // This is the new ReplicaSet. + // In rare cases, such as after cluster upgrades, Deployment may end up with + // having more than one new ReplicaSets that have the same template as its template, + // see https://github.com/kubernetes/kubernetes/issues/40415 + // We deterministically choose the oldest new ReplicaSet. return rsList[i], nil } } @@ -629,7 +633,12 @@ func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions. // All pods and replica sets are labeled with pod-template-hash to prevent overlapping oldRSs := map[string]*extensions.ReplicaSet{} allOldRSs := map[string]*extensions.ReplicaSet{} - newRSTemplate := GetNewReplicaSetTemplate(deployment) + requiredRSs := []*extensions.ReplicaSet{} + allRSs := []*extensions.ReplicaSet{} + newRS, err := FindNewReplicaSet(deployment, rsList) + if err != nil { + return requiredRSs, allRSs, err + } for _, pod := range podList.Items { podLabelsSelector := labels.Set(pod.ObjectMeta.Labels) for _, rs := range rsList { @@ -637,8 +646,8 @@ func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions. if err != nil { return nil, nil, fmt.Errorf("invalid label selector: %v", err) } - // Filter out replica set that has the same pod template spec as the deployment - that is the new replica set. - if EqualIgnoreHash(rs.Spec.Template, newRSTemplate) { + // Filter out new replica set + if newRS != nil && rs.UID == newRS.UID { continue } allOldRSs[rs.ObjectMeta.Name] = rs @@ -647,12 +656,10 @@ func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions. } } } - requiredRSs := []*extensions.ReplicaSet{} for key := range oldRSs { value := oldRSs[key] requiredRSs = append(requiredRSs, value) } - allRSs := []*extensions.ReplicaSet{} for key := range allOldRSs { value := allOldRSs[key] allRSs = append(allRSs, value) diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index e1294a787c0..7ee81004b58 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -18,7 +18,9 @@ package util import ( "fmt" + "math/rand" "reflect" + "strconv" "testing" "time" @@ -27,6 +29,7 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" core "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/api" @@ -161,6 +164,7 @@ func generateRS(deployment extensions.Deployment) extensions.ReplicaSet { template := GetNewReplicaSetTemplate(&deployment) return extensions.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{ + UID: randomUID(), Name: v1.SimpleNameGenerator.GenerateName("replicaset"), Labels: template.Labels, }, @@ -172,6 +176,10 @@ func generateRS(deployment extensions.Deployment) extensions.ReplicaSet { } } +func randomUID() types.UID { + return types.UID(strconv.FormatInt(rand.Int63(), 10)) +} + // generateDeployment creates a deployment, with the input image as its template func generateDeployment(image string) extensions.Deployment { podLabels := map[string]string{"name": image} @@ -466,9 +474,18 @@ func TestEqualIgnoreHash(t *testing.T) { } func TestFindNewReplicaSet(t *testing.T) { + now := metav1.Now() + later := metav1.Time{Time: now.Add(time.Minute)} + deployment := generateDeployment("nginx") newRS := generateRS(deployment) - newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "different-hash" + newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "hash" + newRS.CreationTimestamp = later + + newRSDup := generateRS(deployment) + newRSDup.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "different-hash" + newRSDup.CreationTimestamp = now + oldDeployment := generateDeployment("nginx") oldDeployment.Spec.Template.Spec.Containers[0].Name = "nginx-old-1" oldRS := generateRS(oldDeployment) @@ -481,11 +498,17 @@ func TestFindNewReplicaSet(t *testing.T) { expected *extensions.ReplicaSet }{ { - test: "Get new ReplicaSet with the same spec but different pod-template-hash value", + test: "Get new ReplicaSet with the same template as Deployment spec but different pod-template-hash value", deployment: deployment, rsList: []*extensions.ReplicaSet{&newRS, &oldRS}, expected: &newRS, }, + { + test: "Get the oldest new ReplicaSet when there are more than one ReplicaSet with the same template", + deployment: deployment, + rsList: []*extensions.ReplicaSet{&newRS, &oldRS, &newRSDup}, + expected: &newRSDup, + }, { test: "Get nil new ReplicaSet", deployment: deployment, @@ -502,13 +525,23 @@ func TestFindNewReplicaSet(t *testing.T) { } func TestFindOldReplicaSets(t *testing.T) { + now := metav1.Now() + later := metav1.Time{Time: now.Add(time.Minute)} + deployment := generateDeployment("nginx") newRS := generateRS(deployment) - newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "different-hash" + newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "hash" + newRS.CreationTimestamp = later + + newRSDup := generateRS(deployment) + newRSDup.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "different-hash" + newRSDup.CreationTimestamp = now + oldDeployment := generateDeployment("nginx") oldDeployment.Spec.Template.Spec.Containers[0].Name = "nginx-old-1" oldRS := generateRS(oldDeployment) oldRS.Status.FullyLabeledReplicas = *(oldRS.Spec.Replicas) + newPod := generatePodFromRS(newRS) oldPod := generatePodFromRS(oldRS) @@ -542,6 +575,18 @@ func TestFindOldReplicaSets(t *testing.T) { }, expected: []*extensions.ReplicaSet{&oldRS}, }, + { + test: "Get old ReplicaSets with two new ReplicaSets, only the oldest new ReplicaSet is seen as new ReplicaSet", + deployment: deployment, + rsList: []*extensions.ReplicaSet{&oldRS, &newRS, &newRSDup}, + podList: &v1.PodList{ + Items: []v1.Pod{ + newPod, + oldPod, + }, + }, + expected: []*extensions.ReplicaSet{&oldRS, &newRS}, + }, { test: "Get empty old ReplicaSets", deployment: deployment,