From 8d41ea5e005bbe7accf41abb7efa18558553d6ab Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Wed, 26 Jan 2022 11:49:09 -0800 Subject: [PATCH 1/9] fix: implement pod template hash calculate function Signed-off-by: Andrii Perenesenko --- utils/hash/hash.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 utils/hash/hash.go diff --git a/utils/hash/hash.go b/utils/hash/hash.go new file mode 100644 index 0000000000..09d14f380c --- /dev/null +++ b/utils/hash/hash.go @@ -0,0 +1,34 @@ +package hash + +import ( + "encoding/binary" + "encoding/json" + "fmt" + "hash/fnv" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/rand" +) + +// ComputePodTemplateHash returns a hash value calculated from the Rollout's pod template. +// The hash will be safe encoded to avoid bad words. +func ComputePodTemplateHash(template *corev1.PodTemplateSpec, collisionCount *int32) string { + podTemplateSpecHasher := fnv.New32a() + stepsBytes, err := json.Marshal(template) + if err != nil { + panic(err) + } + _, err = podTemplateSpecHasher.Write(stepsBytes) + if err != nil { + panic(err) + } + if collisionCount != nil { + collisionCountBytes := make([]byte, 8) + binary.LittleEndian.PutUint32(collisionCountBytes, uint32(*collisionCount)) + _, err = podTemplateSpecHasher.Write(collisionCountBytes) + if err != nil { + panic(err) + } + } + return rand.SafeEncodeString(fmt.Sprint(podTemplateSpecHasher.Sum32())) +} From d03a5d508dea8807aee1e564a3573fd5af0bf7bf Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Wed, 26 Jan 2022 11:51:57 -0800 Subject: [PATCH 2/9] fix: using new hashing for FindNewReplicaSet and other places Signed-off-by: Andrii Perenesenko --- experiments/replicaset.go | 4 ++-- rollout/experiment.go | 15 ++++++++------- rollout/sync.go | 5 +++-- utils/experiment/experiment.go | 5 +++-- utils/replicaset/replicaset.go | 35 ++++++++++++++++++++++++---------- 5 files changed, 41 insertions(+), 23 deletions(-) diff --git a/experiments/replicaset.go b/experiments/replicaset.go index 1b9bd7c16f..d91843a03a 100644 --- a/experiments/replicaset.go +++ b/experiments/replicaset.go @@ -13,13 +13,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" patchtypes "k8s.io/apimachinery/pkg/types" - "k8s.io/kubernetes/pkg/controller" labelsutil "k8s.io/kubernetes/pkg/util/labels" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/conditions" "github.com/argoproj/argo-rollouts/utils/defaults" experimentutil "github.com/argoproj/argo-rollouts/utils/experiment" + "github.com/argoproj/argo-rollouts/utils/hash" logutil "github.com/argoproj/argo-rollouts/utils/log" "github.com/argoproj/argo-rollouts/utils/record" replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" @@ -161,7 +161,7 @@ func newReplicaSetFromTemplate(experiment *v1alpha1.Experiment, template v1alpha delete(newRSTemplate.Labels, v1alpha1.DefaultRolloutUniqueLabelKey) } } - podHash := controller.ComputeHash(&newRSTemplate, collisionCount) + podHash := hash.ComputePodTemplateHash(&newRSTemplate, collisionCount) newRSTemplate.Labels = labelsutil.CloneAndAddLabel(newRSTemplate.Labels, v1alpha1.DefaultRolloutUniqueLabelKey, podHash) // Add podTemplateHash label to selector. diff --git a/rollout/experiment.go b/rollout/experiment.go index 67b7e534b4..aeac6e8049 100644 --- a/rollout/experiment.go +++ b/rollout/experiment.go @@ -4,19 +4,20 @@ import ( "context" "fmt" + appsv1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" analysisutil "github.com/argoproj/argo-rollouts/utils/analysis" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/defaults" experimentutil "github.com/argoproj/argo-rollouts/utils/experiment" + "github.com/argoproj/argo-rollouts/utils/hash" "github.com/argoproj/argo-rollouts/utils/record" replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" - appsv1 "k8s.io/api/apps/v1" - "k8s.io/apimachinery/pkg/api/errors" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/kubernetes/pkg/controller" ) // GetExperimentFromTemplate takes the canary experiment step and converts it to an experiment @@ -25,7 +26,7 @@ func GetExperimentFromTemplate(r *v1alpha1.Rollout, stableRS, newRS *appsv1.Repl if step == nil { return nil, nil } - podHash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) + podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) currentStep := int32(0) if r.Status.CurrentStepIndex != nil { currentStep = *r.Status.CurrentStepIndex diff --git a/rollout/sync.go b/rollout/sync.go index 337cfa462a..5472dbb526 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -23,6 +23,7 @@ import ( "github.com/argoproj/argo-rollouts/utils/defaults" "github.com/argoproj/argo-rollouts/utils/diff" experimentutil "github.com/argoproj/argo-rollouts/utils/experiment" + "github.com/argoproj/argo-rollouts/utils/hash" logutil "github.com/argoproj/argo-rollouts/utils/log" "github.com/argoproj/argo-rollouts/utils/record" replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" @@ -139,7 +140,7 @@ func (c *rolloutContext) createDesiredReplicaSet() (*appsv1.ReplicaSet, error) { newRSTemplate := *c.rollout.Spec.Template.DeepCopy() // Add default anti-affinity rule if antiAffinity bool set and RSTemplate meets requirements newRSTemplate.Spec.Affinity = replicasetutil.GenerateReplicaSetAffinity(*c.rollout) - podTemplateSpecHash := controller.ComputeHash(&c.rollout.Spec.Template, c.rollout.Status.CollisionCount) + podTemplateSpecHash := hash.ComputePodTemplateHash(&c.rollout.Spec.Template, c.rollout.Status.CollisionCount) newRSTemplate.Labels = labelsutil.CloneAndAddLabel(c.rollout.Spec.Template.Labels, v1alpha1.DefaultRolloutUniqueLabelKey, podTemplateSpecHash) // Add podTemplateHash label to selector. newRSSelector := labelsutil.CloneSelectorAndAddLabel(c.rollout.Spec.Selector, v1alpha1.DefaultRolloutUniqueLabelKey, podTemplateSpecHash) @@ -378,7 +379,7 @@ func (c *rolloutContext) calculateBaseStatus() v1alpha1.RolloutStatus { // newRS potentially might be nil when called by syncReplicasOnly(). For this // to happen, the user would have had to simultaneously change the number of replicas, and // the pod template spec at the same time. - currentPodHash = controller.ComputeHash(&c.rollout.Spec.Template, c.rollout.Status.CollisionCount) + currentPodHash = hash.ComputePodTemplateHash(&c.rollout.Spec.Template, c.rollout.Status.CollisionCount) c.log.Infof("Assuming %s for new replicaset pod hash", currentPodHash) } else { currentPodHash = c.newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] diff --git a/utils/experiment/experiment.go b/utils/experiment/experiment.go index cf7b4b6344..3bd3d07a96 100644 --- a/utils/experiment/experiment.go +++ b/utils/experiment/experiment.go @@ -8,11 +8,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" patchtypes "k8s.io/apimachinery/pkg/types" - "k8s.io/kubernetes/pkg/controller" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" rolloutsclient "github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/typed/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/defaults" + "github.com/argoproj/argo-rollouts/utils/hash" timeutil "github.com/argoproj/argo-rollouts/utils/time" ) @@ -131,8 +131,9 @@ func GetCollisionCountForTemplate(experiment *v1alpha1.Experiment, template v1al // ReplicasetNameFromExperiment gets the replicaset name based off of the experiment and the template func ReplicasetNameFromExperiment(experiment *v1alpha1.Experiment, template v1alpha1.TemplateSpec) string { + // todo: review this method for deletion as it's not using collisionCount := GetCollisionCountForTemplate(experiment, template) - podTemplateSpecHash := controller.ComputeHash(&template.Template, collisionCount) + podTemplateSpecHash := hash.ComputePodTemplateHash(&template.Template, collisionCount) return fmt.Sprintf("%s-%s-%s", experiment.Name, template.Name, podTemplateSpecHash) } diff --git a/utils/replicaset/replicaset.go b/utils/replicaset/replicaset.go index 2292eaa1bc..e5ae91fe9f 100644 --- a/utils/replicaset/replicaset.go +++ b/utils/replicaset/replicaset.go @@ -24,6 +24,7 @@ import ( "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" "github.com/argoproj/argo-rollouts/utils/defaults" + "github.com/argoproj/argo-rollouts/utils/hash" logutil "github.com/argoproj/argo-rollouts/utils/log" timeutil "github.com/argoproj/argo-rollouts/utils/time" ) @@ -39,12 +40,17 @@ func FindNewReplicaSet(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) * } rsList = newRSList sort.Sort(controller.ReplicaSetsByCreationTimestamp(rsList)) - // First, attempt to find the replicaset by the replicaset naming formula - replicaSetName := fmt.Sprintf("%s-%s", rollout.Name, controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount)) - for _, rs := range rsList { - if rs.Name == replicaSetName { - return rs - } + // First, attempt to find the replicaset using our own hashing + podHash := hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + if rs := searchRsByHash(rsList, podHash); rs != nil { + return rs + } + // Second, attempt to find the replicaset with old hash implementation + oldHash := controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + if rs := searchRsByHash(rsList, oldHash); rs != nil { + logCtx := logutil.WithRollout(rollout) + logCtx.Infof("ComputePodTemplateHash hash changed (new hash: %s, old hash: %s)", podHash, oldHash) + return rs } // Iterate the ReplicaSet list again, this time doing a deep equal against the template specs. // This covers the corner case in which the reason we did not find the replicaset, was because @@ -61,7 +67,7 @@ func FindNewReplicaSet(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) * desired := rollout.Spec.Template.DeepCopy() if PodTemplateEqualIgnoreHash(live, desired) { logCtx := logutil.WithRollout(rollout) - logCtx.Infof("ComputeHash change detected (expected: %s, actual: %s)", replicaSetName, rs.Name) + logCtx.Infof("ComputePodTemplateHash hash changed (expected: %s, actual: %s)", podHash, rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) return rs } } @@ -69,6 +75,15 @@ func FindNewReplicaSet(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) * return nil } +func searchRsByHash(rsList []*appsv1.ReplicaSet, hash string) *appsv1.ReplicaSet { + for _, rs := range rsList { + if rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] == hash { + return rs + } + } + return nil +} + func GetRolloutAffinity(rollout v1alpha1.Rollout) *v1alpha1.AntiAffinity { var antiAffinityStrategy *v1alpha1.AntiAffinity if rollout.Spec.Strategy.BlueGreen != nil && rollout.Spec.Strategy.BlueGreen.AntiAffinity != nil { @@ -87,7 +102,7 @@ func GetRolloutAffinity(rollout v1alpha1.Rollout) *v1alpha1.AntiAffinity { func GenerateReplicaSetAffinity(rollout v1alpha1.Rollout) *corev1.Affinity { antiAffinityStrategy := GetRolloutAffinity(rollout) - currentPodHash := controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + currentPodHash := hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) affinitySpec := rollout.Spec.Template.Spec.Affinity.DeepCopy() if antiAffinityStrategy != nil && rollout.Status.StableRS != "" && rollout.Status.StableRS != currentPodHash { antiAffinityRule := CreateInjectedAntiAffinityRule(rollout) @@ -193,7 +208,7 @@ func RemoveInjectedAntiAffinityRule(affinity *corev1.Affinity, rollout v1alpha1. func IfInjectedAntiAffinityRuleNeedsUpdate(affinity *corev1.Affinity, rollout v1alpha1.Rollout) bool { _, podAffinityTerm := HasInjectedAntiAffinityRule(affinity, rollout) - currentPodHash := controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + currentPodHash := hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) if podAffinityTerm != nil && rollout.Status.StableRS != currentPodHash { for _, labelSelectorRequirement := range podAffinityTerm.LabelSelector.MatchExpressions { if labelSelectorRequirement.Key == v1alpha1.DefaultRolloutUniqueLabelKey && labelSelectorRequirement.Values[0] != rollout.Status.StableRS { @@ -456,7 +471,7 @@ func CheckPodSpecChange(rollout *v1alpha1.Rollout, newRS *appsv1.ReplicaSet) boo if rollout.Status.CurrentPodHash == "" { return false } - podHash := controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + podHash := hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) if newRS != nil { podHash = GetPodTemplateHash(newRS) } From 088064772a5f0b05a624418603da5a1577c5371b Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Wed, 26 Jan 2022 11:52:18 -0800 Subject: [PATCH 3/9] fix tests Signed-off-by: Andrii Perenesenko --- rollout/analysis_test.go | 6 +++--- rollout/bluegreen_test.go | 4 ++-- rollout/canary_test.go | 10 +++++----- rollout/controller_test.go | 4 ++-- utils/conditions/rollouts_test.go | 4 ++-- utils/replicaset/replicaset_test.go | 9 +++++---- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 9f25d1c5f7..ed21a4c848 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -8,13 +8,13 @@ import ( "testing" "time" + "github.com/argoproj/argo-rollouts/utils/hash" timeutil "github.com/argoproj/argo-rollouts/utils/time" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/kubernetes/pkg/controller" "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" @@ -58,7 +58,7 @@ func clusterAnalysisTemplate(name string) *v1alpha1.ClusterAnalysisTemplate { func clusterAnalysisRun(cat *v1alpha1.ClusterAnalysisTemplate, analysisRunType string, r *v1alpha1.Rollout) *v1alpha1.AnalysisRun { labels := map[string]string{} - podHash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) + podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) var name string if analysisRunType == v1alpha1.RolloutTypeStepLabel { labels = analysisutil.StepLabels(*r.Status.CurrentStepIndex, podHash, "") @@ -89,7 +89,7 @@ func clusterAnalysisRun(cat *v1alpha1.ClusterAnalysisTemplate, analysisRunType s func analysisRun(at *v1alpha1.AnalysisTemplate, analysisRunType string, r *v1alpha1.Rollout) *v1alpha1.AnalysisRun { labels := map[string]string{} - podHash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) + podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) var name string if analysisRunType == v1alpha1.RolloutTypeStepLabel { labels = analysisutil.StepLabels(*r.Status.CurrentStepIndex, podHash, "") diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 5bd965df61..5cae668da3 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -11,12 +11,12 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" core "k8s.io/client-go/testing" - "k8s.io/kubernetes/pkg/controller" "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" + "github.com/argoproj/argo-rollouts/utils/hash" rolloututil "github.com/argoproj/argo-rollouts/utils/rollout" timeutil "github.com/argoproj/argo-rollouts/utils/time" ) @@ -34,7 +34,7 @@ func newBlueGreenRollout(name string, replicas int, revisionHistoryLimit *int32, AbortScaleDownDelaySeconds: &abortScaleDownDelaySeconds, } rollout.Status.CurrentStepHash = conditions.ComputeStepHash(rollout) - rollout.Status.CurrentPodHash = controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + rollout.Status.CurrentPodHash = hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) return rollout } diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 0961dad44c..9b4504000d 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -15,13 +15,13 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" k8sinformers "k8s.io/client-go/informers" k8sfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/kubernetes/pkg/controller" "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" + "github.com/argoproj/argo-rollouts/utils/hash" logutil "github.com/argoproj/argo-rollouts/utils/log" "github.com/argoproj/argo-rollouts/utils/record" replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" @@ -39,7 +39,7 @@ func newCanaryRollout(name string, replicas int, revisionHistoryLimit *int32, st } rollout.Status.CurrentStepIndex = stepIndex rollout.Status.CurrentStepHash = conditions.ComputeStepHash(rollout) - rollout.Status.CurrentPodHash = controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) + rollout.Status.CurrentPodHash = hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) rollout.Status.Selector = metav1.FormatLabelSelector(rollout.Spec.Selector) rollout.Status.Phase, rollout.Status.Message = rolloututil.CalculateRolloutPhase(rollout.Spec, rollout.Status) return rollout @@ -54,7 +54,7 @@ func bumpVersion(rollout *v1alpha1.Rollout) *v1alpha1.Rollout { newRevisionStr := strconv.FormatInt(int64(newRevision), 10) annotations.SetRolloutRevision(newRollout, newRevisionStr) newRollout.Spec.Template.Spec.Containers[0].Image = "foo/bar" + newRevisionStr - newRollout.Status.CurrentPodHash = controller.ComputeHash(&newRollout.Spec.Template, newRollout.Status.CollisionCount) + newRollout.Status.CurrentPodHash = hash.ComputePodTemplateHash(&newRollout.Spec.Template, newRollout.Status.CollisionCount) newRollout.Status.CurrentStepHash = conditions.ComputeStepHash(newRollout) newRollout.Status.Phase, newRollout.Status.Message = rolloututil.CalculateRolloutPhase(newRollout.Spec, newRollout.Status) return newRollout @@ -841,7 +841,7 @@ func TestRollBackToStable(t *testing.T) { } }` newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "") - expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, controller.ComputeHash(&r2.Spec.Template, r2.Status.CollisionCount), newConditions) + expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, hash.ComputePodTemplateHash(&r2.Spec.Template, r2.Status.CollisionCount), newConditions) patch := f.getPatchedRollout(patchIndex) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } @@ -929,7 +929,7 @@ func TestRollBackToStableAndStepChange(t *testing.T) { "conditions": %s } }` - newPodHash := controller.ComputeHash(&r2.Spec.Template, r2.Status.CollisionCount) + newPodHash := hash.ComputePodTemplateHash(&r2.Spec.Template, r2.Status.CollisionCount) newStepHash := conditions.ComputeStepHash(r2) newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "") expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newPodHash, newStepHash, newConditions) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 8a71878027..551caa7010 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -34,7 +34,6 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" corev1defaults "k8s.io/kubernetes/pkg/apis/core/v1" - "k8s.io/kubernetes/pkg/controller" "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/controller/metrics" @@ -47,6 +46,7 @@ import ( "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" "github.com/argoproj/argo-rollouts/utils/defaults" + "github.com/argoproj/argo-rollouts/utils/hash" ingressutil "github.com/argoproj/argo-rollouts/utils/ingress" istioutil "github.com/argoproj/argo-rollouts/utils/istio" "github.com/argoproj/argo-rollouts/utils/queue" @@ -401,7 +401,7 @@ func updateBaseRolloutStatus(r *v1alpha1.Rollout, availableReplicas, updatedRepl } func newReplicaSet(r *v1alpha1.Rollout, replicas int) *appsv1.ReplicaSet { - podHash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) + podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) rsLabels := map[string]string{ v1alpha1.DefaultRolloutUniqueLabelKey: podHash, } diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index c633a4e40c..822856746a 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -8,10 +8,10 @@ import ( "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/kubernetes/pkg/controller" "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/argoproj/argo-rollouts/utils/hash" ) var ( @@ -363,7 +363,7 @@ func TestRolloutComplete(t *testing.T) { }, } r.Generation = 123 - podHash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) + podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) r.Status.CurrentPodHash = podHash return r } diff --git a/utils/replicaset/replicaset_test.go b/utils/replicaset/replicaset_test.go index ada18a8c1b..f2a9fe717b 100644 --- a/utils/replicaset/replicaset_test.go +++ b/utils/replicaset/replicaset_test.go @@ -23,6 +23,7 @@ import ( "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" + "github.com/argoproj/argo-rollouts/utils/hash" timeutil "github.com/argoproj/argo-rollouts/utils/time" ) @@ -60,14 +61,14 @@ func generateRollout(image string) v1alpha1.Rollout { // generateRS creates a replica set, with the input rollout's template as its template func generateRS(rollout v1alpha1.Rollout) appsv1.ReplicaSet { template := rollout.Spec.Template.DeepCopy() - podTemplateHash := controller.ComputeHash(&rollout.Spec.Template, nil) + podTemplateHash := hash.ComputePodTemplateHash(&rollout.Spec.Template, nil) template.Labels = map[string]string{ v1alpha1.DefaultRolloutUniqueLabelKey: podTemplateHash, } return appsv1.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{ UID: uuid.NewUUID(), - Name: fmt.Sprintf("%s-%s", rollout.Name, controller.ComputeHash(&rollout.Spec.Template, nil)), + Name: fmt.Sprintf("%s-%s", rollout.Name, podTemplateHash), Labels: template.Labels, }, Spec: appsv1.ReplicaSetSpec{ @@ -637,7 +638,7 @@ func TestCheckPodSpecChange(t *testing.T) { ro := generateRollout("nginx") rs := generateRS(ro) assert.False(t, CheckPodSpecChange(&ro, &rs)) - ro.Status.CurrentPodHash = controller.ComputeHash(&ro.Spec.Template, ro.Status.CollisionCount) + ro.Status.CurrentPodHash = hash.ComputePodTemplateHash(&ro.Spec.Template, ro.Status.CollisionCount) assert.False(t, CheckPodSpecChange(&ro, &rs)) ro.Status.CurrentPodHash = "different-hash" @@ -828,7 +829,7 @@ func TestGenerateReplicaSetAffinity(t *testing.T) { assert.Equal(t, "", ro.Status.StableRS) assert.Nil(t, GenerateReplicaSetAffinity(ro)) // StableRS is equal to CurrentPodHash - ro.Status.StableRS = controller.ComputeHash(&ro.Spec.Template, nil) + ro.Status.StableRS = hash.ComputePodTemplateHash(&ro.Spec.Template, nil) assert.Nil(t, GenerateReplicaSetAffinity(ro)) // Injects anti-affinity rule with RequiredDuringSchedulingIgnoredDuringExecution into empty RS Affinity object From 4b20be92c211be603f3de7223f9f34ff365db3ca Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Thu, 27 Jan 2022 12:10:37 -0800 Subject: [PATCH 4/9] fix tests with deprecated hash Signed-off-by: Andrii Perenesenko --- rollout/controller_test.go | 2 +- utils/conditions/rollouts_test.go | 4 ++-- utils/experiment/experiment_test.go | 10 ++++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 551caa7010..f8e922ee3b 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -1300,7 +1300,7 @@ func TestPodTemplateHashEquivalence(t *testing.T) { var err error // NOTE: This test will fail on every k8s library upgrade. // To fix it, update expectedReplicaSetName to match the new hash. - expectedReplicaSetName := "guestbook-586d86c77b" + expectedReplicaSetName := "guestbook-6c5667f666" r1 := newBlueGreenRollout("guestbook", 1, nil, "active", "") r1Resources := ` diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 822856746a..f6e3e02550 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -411,13 +411,13 @@ func TestRolloutComplete(t *testing.T) { { name: "BlueGreen complete", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 5, 5, 5, true, "85f7cf5fc7", "85f7cf5fc7"), + r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74"), expected: true, }, { name: "BlueGreen complete with extra old replicas", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 6, 5, 5, true, "85f7cf5fc7", "85f7cf5fc7"), + r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74"), expected: true, }, { diff --git a/utils/experiment/experiment_test.go b/utils/experiment/experiment_test.go index 194065cbf7..2c87bda888 100644 --- a/utils/experiment/experiment_test.go +++ b/utils/experiment/experiment_test.go @@ -5,13 +5,14 @@ import ( "testing" "time" - "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" - "github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" kubetesting "k8s.io/client-go/testing" "k8s.io/utils/pointer" + + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake" ) func TestHasFinished(t *testing.T) { @@ -90,6 +91,7 @@ func TestGetTemplateStatusMapping(t *testing.T) { assert.Equal(t, int32(1), mapping["test"].Replicas) assert.Equal(t, int32(2), mapping["test2"].Replicas) } + func TestReplicaSetNameFromExperiment(t *testing.T) { templateName := "template" template := v1alpha1.TemplateSpec{ @@ -100,14 +102,14 @@ func TestReplicaSetNameFromExperiment(t *testing.T) { Name: "foo", }, } - assert.Equal(t, "foo-template-85f7cf5fc7", ReplicasetNameFromExperiment(e, template)) + assert.Equal(t, "foo-template-76bbb58f74", ReplicasetNameFromExperiment(e, template)) newTemplateStatus := v1alpha1.TemplateStatus{ Name: templateName, CollisionCount: pointer.Int32Ptr(1), } e.Status.TemplateStatuses = append(e.Status.TemplateStatuses, newTemplateStatus) - assert.Equal(t, "foo-template-56ccbc9b64", ReplicasetNameFromExperiment(e, template)) + assert.Equal(t, "foo-template-688c48b575", ReplicasetNameFromExperiment(e, template)) } func TestExperimentByCreationTimestamp(t *testing.T) { From df23ebf64d1d8c0b04e1eacb7b96e55fe148555f Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Thu, 27 Jan 2022 14:26:24 -0800 Subject: [PATCH 5/9] test TestFindNewReplicaSet, TestHashUtils Signed-off-by: Andrii Perenesenko --- utils/hash/hash_test.go | 49 +++++++++++++++++++++++++++++ utils/replicaset/replicaset_test.go | 22 ++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 utils/hash/hash_test.go diff --git a/utils/hash/hash_test.go b/utils/hash/hash_test.go new file mode 100644 index 0000000000..5700344060 --- /dev/null +++ b/utils/hash/hash_test.go @@ -0,0 +1,49 @@ +package hash + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestHashUtils(t *testing.T) { + templateRed := generatePodTemplate("red") + + hashRed := ComputePodTemplateHash(&templateRed, nil) + + t.Run("HashForSameTemplates", func(t *testing.T) { + template := generatePodTemplate("red") + podHash := ComputePodTemplateHash(&template, nil) + assert.Equal(t, hashRed, podHash) + }) + t.Run("HashForDifferentTemplates", func(t *testing.T) { + template := generatePodTemplate("blue") + podHash := ComputePodTemplateHash(&template, nil) + assert.NotEqual(t, hashRed, podHash) + }) +} + +func generatePodTemplate(image string) corev1.PodTemplateSpec { + podLabels := map[string]string{"name": image} + + return corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: podLabels, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: image, + Image: image, + ImagePullPolicy: corev1.PullAlways, + TerminationMessagePath: corev1.TerminationMessagePathDefault, + }, + }, + DNSPolicy: corev1.DNSClusterFirst, + RestartPolicy: corev1.RestartPolicyAlways, + SecurityContext: &corev1.PodSecurityContext{}, + }, + } +} diff --git a/utils/replicaset/replicaset_test.go b/utils/replicaset/replicaset_test.go index f2a9fe717b..7e07967fa6 100644 --- a/utils/replicaset/replicaset_test.go +++ b/utils/replicaset/replicaset_test.go @@ -79,6 +79,26 @@ func generateRS(rollout v1alpha1.Rollout) appsv1.ReplicaSet { } } +func TestFindNewReplicaSet(t *testing.T) { + ro := generateRollout("red") + rs1 := generateRS(ro) + rs1.Labels["name"] = "red" + *(rs1.Spec.Replicas) = 1 + + t.Run("FindNewReplicaSet by hash", func(t *testing.T) { + // rs has the current hash + rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] = hash.ComputePodTemplateHash(&ro.Spec.Template, ro.Status.CollisionCount) + actual := FindNewReplicaSet(&ro, []*appsv1.ReplicaSet{&rs1}) + assert.Equal(t, &rs1, actual) + }) + t.Run("FindNewReplicaSet by deprecated hash", func(t *testing.T) { + // rs has the deprecated hash + rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] = controller.ComputeHash(&ro.Spec.Template, ro.Status.CollisionCount) + actual := FindNewReplicaSet(&ro, []*appsv1.ReplicaSet{&rs1}) + assert.Equal(t, &rs1, actual) + }) +} + func TestFindOldReplicaSets(t *testing.T) { now := metav1.Now() before := metav1.Time{Time: now.Add(-time.Minute)} @@ -86,7 +106,7 @@ func TestFindOldReplicaSets(t *testing.T) { rollout := generateRollout("nginx") newRS := generateRS(rollout) *(newRS.Spec.Replicas) = 1 - newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] = "hash" + newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] = hash.ComputePodTemplateHash(&rollout.Spec.Template, rollout.Status.CollisionCount) newRS.CreationTimestamp = now oldRollout := generateRollout("nginx") From 1f0a6d12e35156114a179a1a79165f911992e12f Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Thu, 27 Jan 2022 14:30:00 -0800 Subject: [PATCH 6/9] fix the doc for ComputePodTemplateHash Signed-off-by: Andrii Perenesenko --- utils/hash/hash.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/hash/hash.go b/utils/hash/hash.go index 09d14f380c..52d82d3cc7 100644 --- a/utils/hash/hash.go +++ b/utils/hash/hash.go @@ -10,7 +10,7 @@ import ( "k8s.io/apimachinery/pkg/util/rand" ) -// ComputePodTemplateHash returns a hash value calculated from the Rollout's pod template. +// ComputePodTemplateHash returns a hash value calculated from pod template. // The hash will be safe encoded to avoid bad words. func ComputePodTemplateHash(template *corev1.PodTemplateSpec, collisionCount *int32) string { podTemplateSpecHasher := fnv.New32a() From 32d00b24ed79ed8e9caaec4d73fc31e3e13af770 Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Thu, 27 Jan 2022 16:23:23 -0800 Subject: [PATCH 7/9] test for ComputePodTemplateHash with collisionCount Signed-off-by: Andrii Perenesenko --- utils/hash/hash_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/utils/hash/hash_test.go b/utils/hash/hash_test.go index 5700344060..8a0edf11f4 100644 --- a/utils/hash/hash_test.go +++ b/utils/hash/hash_test.go @@ -6,21 +6,20 @@ import ( "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" ) func TestHashUtils(t *testing.T) { templateRed := generatePodTemplate("red") - hashRed := ComputePodTemplateHash(&templateRed, nil) + template := generatePodTemplate("red") t.Run("HashForSameTemplates", func(t *testing.T) { - template := generatePodTemplate("red") podHash := ComputePodTemplateHash(&template, nil) assert.Equal(t, hashRed, podHash) }) t.Run("HashForDifferentTemplates", func(t *testing.T) { - template := generatePodTemplate("blue") - podHash := ComputePodTemplateHash(&template, nil) + podHash := ComputePodTemplateHash(&template, pointer.Int32(1)) assert.NotEqual(t, hashRed, podHash) }) } From fc49997f9228d071f5886a04082c8dffb04751cd Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Fri, 28 Jan 2022 09:41:26 -0800 Subject: [PATCH 8/9] refactoring avoid double FindNewReplicaSet call: pass newRS as a parameter to the FindOldReplicaSets as it using only right after the FindNewReplicaSet Signed-off-by: Andrii Perenesenko --- rollout/controller.go | 2 +- utils/replicaset/replicaset.go | 3 +-- utils/replicaset/replicaset_test.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/rollout/controller.go b/rollout/controller.go index 46ca2e1099..e17fbafc37 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -433,7 +433,7 @@ func (c *Controller) newRolloutContext(rollout *v1alpha1.Rollout) (*rolloutConte } newRS := replicasetutil.FindNewReplicaSet(rollout, rsList) - olderRSs := replicasetutil.FindOldReplicaSets(rollout, rsList) + olderRSs := replicasetutil.FindOldReplicaSets(rollout, rsList, newRS) stableRS := replicasetutil.GetStableRS(rollout, newRS, olderRSs) otherRSs := replicasetutil.GetOtherRSs(rollout, newRS, stableRS, rsList) diff --git a/utils/replicaset/replicaset.go b/utils/replicaset/replicaset.go index e5ae91fe9f..333b5117fd 100644 --- a/utils/replicaset/replicaset.go +++ b/utils/replicaset/replicaset.go @@ -231,9 +231,8 @@ func NeedsRestart(rollout *v1alpha1.Rollout) bool { } // FindOldReplicaSets returns the old replica sets targeted by the given Rollout, with the given slice of RSes. -func FindOldReplicaSets(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) []*appsv1.ReplicaSet { +func FindOldReplicaSets(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet) []*appsv1.ReplicaSet { var allRSs []*appsv1.ReplicaSet - newRS := FindNewReplicaSet(rollout, rsList) for _, rs := range rsList { // Filter out new replica set if newRS != nil && rs.UID == newRS.UID { diff --git a/utils/replicaset/replicaset_test.go b/utils/replicaset/replicaset_test.go index 7e07967fa6..f1a2a80fd7 100644 --- a/utils/replicaset/replicaset_test.go +++ b/utils/replicaset/replicaset_test.go @@ -143,7 +143,7 @@ func TestFindOldReplicaSets(t *testing.T) { for _, test := range tests { t.Run(test.Name, func(t *testing.T) { - allRS := FindOldReplicaSets(&test.rollout, test.rsList) + allRS := FindOldReplicaSets(&test.rollout, test.rsList, &newRS) sort.Sort(controller.ReplicaSetsByCreationTimestamp(allRS)) sort.Sort(controller.ReplicaSetsByCreationTimestamp(test.expected)) if !reflect.DeepEqual(allRS, test.expected) { From 54a54d83a70aa55f4ca5e87bd7280aa9b29af8c5 Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Fri, 28 Jan 2022 12:20:58 -0800 Subject: [PATCH 9/9] fix time dependent flaky test Signed-off-by: Andrii Perenesenko --- rollout/experiment_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rollout/experiment_test.go b/rollout/experiment_test.go index b002913d0f..788bab57c0 100644 --- a/rollout/experiment_test.go +++ b/rollout/experiment_test.go @@ -358,9 +358,9 @@ func TestPauseRolloutAfterInconclusiveExperiment(t *testing.T) { "message": "%s" } }` - now := metav1.Now().UTC().Format(time.RFC3339) - conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchFmt, v1alpha1.PauseReasonInconclusiveExperiment, now, conditions, v1alpha1.PauseReasonInconclusiveExperiment)) + now := timeutil.Now().UTC().Format(time.RFC3339) + c := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchFmt, v1alpha1.PauseReasonInconclusiveExperiment, now, c, v1alpha1.PauseReasonInconclusiveExperiment)) assert.Equal(t, expectedPatch, patch) }