Skip to content

Commit

Permalink
fix: change completed condition so it only triggers on pod hash chang…
Browse files Browse the repository at this point in the history 
…es also adds an event for when it does changes. (argoproj#2203)

* feat: add healthy event/condition and fix completed event/condition

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix unit tests

Signed-off-by: zachaller <zachaller@hotmail.com>

* rename vars to make more sense and remove healthy event becase it will never be consistent

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix unit tests

Signed-off-by: zachaller <zachaller@hotmail.com>

* possible fix for e2e

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix e2e

Signed-off-by: zachaller <zachaller@hotmail.com>

* unit test for complete function

Signed-off-by: zachaller <zachaller@hotmail.com>

* small cleanup and changes to not check generation

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix unit test and proper behavior

Signed-off-by: zachaller <zachaller@hotmail.com>

* Fix e2e

Signed-off-by: zachaller <zachaller@hotmail.com>

* rename and fix one unit test

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix unit tests

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix unit test

Signed-off-by: zachaller <zachaller@hotmail.com>

* add seperate test for TestRolloutComplete

Signed-off-by: zachaller <zachaller@hotmail.com>

* renames

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix e2e

Signed-off-by: zachaller <zachaller@hotmail.com>

* Set Completed to false

Signed-off-by: zachaller <zachaller@hotmail.com>

* Add event

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix e2e

Signed-off-by: zachaller <zachaller@hotmail.com>

* refactor

Signed-off-by: zachaller <zachaller@hotmail.com>

* Fix all but one unit test

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix last unit test

Signed-off-by: zachaller <zachaller@hotmail.com>

* lint

Signed-off-by: zachaller <zachaller@hotmail.com>

* cleanup

Signed-off-by: zachaller <zachaller@hotmail.com>

* Rename

Signed-off-by: zachaller <zachaller@hotmail.com>

* More renames

Signed-off-by: zachaller <zachaller@hotmail.com>

* small comment change

Signed-off-by: zachaller <zachaller@hotmail.com>

Signed-off-by: zachaller <zachaller@hotmail.com>
Signed-off-by: Xijun Dai <daixijun1990@gmail.com>
  • Loading branch information
@zachaller @daixijun
zachaller authored and daixijun committed Sep 27, 2022
1 parent 28b6c6d commit 6b3a907
Show file tree
Hide file tree
Showing 12 changed files with 358 additions and 143 deletions.
7 changes: 6 additions & 1 deletion pkg/apis/rollouts/v1alpha1/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -987,8 +987,13 @@ const (
RolloutReplicaFailure RolloutConditionType = "ReplicaFailure"
// RolloutPaused means that rollout is in a paused state. It is still progressing at this point.
RolloutPaused RolloutConditionType = "Paused"
// RolloutCompleted means that rollout is in a completed state. It is still progressing at this point.
// RolloutCompleted indicates that the rollout completed its update to the desired revision and is not in the middle
// of any update. Note that a Completed rollout could also be considered Progressing or Degraded, if its Pods become
// unavailable sometime after the update completes.
RolloutCompleted RolloutConditionType = "Completed"
// RolloutHealthy means that rollout is in a completed state and is healthy. Which means that all the pods have been updated
// and are passing their health checks and are ready to serve traffic.
RolloutHealthy RolloutConditionType = "Healthy"
)

// RolloutCondition describes the state of a rollout at a certain point.
Expand Down
81 changes: 62 additions & 19 deletions rollout/analysis_test.go
View file

Large diffs are not rendered by default.

127 changes: 76 additions & 51 deletions rollout/bluegreen_test.go
View file

Large diffs are not rendered by default.

38 changes: 25 additions & 13 deletions rollout/canary_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) {
}
}`

conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "")
conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false)
now := timeutil.MetaNow().UTC().Format(time.RFC3339)
expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, v1alpha1.PauseReasonCanaryPauseStep, now, conditions, v1alpha1.PauseReasonCanaryPauseStep)
expectedPatch := calculatePatch(r2, expectedPatchWithoutObservedGen)
Expand Down Expand Up @@ -239,6 +239,9 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) {
availableCondition, _ := newAvailableCondition(true)
conditions.SetRolloutCondition(&r2.Status, availableCondition)

completedCondition, _ := newCompletedCondition(false)
conditions.SetRolloutCondition(&r2.Status, completedCondition)

rs1 := newReplicaSetWithStatus(r1, 10, 10)
rs2 := newReplicaSetWithStatus(r2, 0, 0)

Expand Down Expand Up @@ -337,7 +340,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) {
"currentStepIndex": 1
}
}`
generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "")
generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false)
expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchTemplate, generatedConditions))
assert.Equal(t, expectedPatch, patch)
}
Expand Down Expand Up @@ -379,7 +382,7 @@ func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) {
}
}`

expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, ""))
expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", true))
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
}

Expand Down Expand Up @@ -421,7 +424,7 @@ func TestResetCurrentStepIndexOnStepChange(t *testing.T) {
"conditions": %s
}
}`
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "")
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false)
expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, expectedCurrentStepHash, newConditions)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)

Expand Down Expand Up @@ -462,7 +465,7 @@ func TestResetCurrentStepIndexOnPodSpecChange(t *testing.T) {
"conditions": %s
}
}`
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "")
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false)

expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, newConditions)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
Expand Down Expand Up @@ -502,7 +505,7 @@ func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) {
}
}`

newConditions := generateConditionsPatch(false, conditions.ReplicaSetUpdatedReason, rs, false, "")
newConditions := generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true)

assert.Equal(t, calculatePatch(r, fmt.Sprintf(expectedPatch, newConditions)), patch)
}
Expand Down Expand Up @@ -542,7 +545,7 @@ func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) {
"conditions": %s
}
}`
expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatch(false, conditions.ReplicaSetUpdatedReason, rs, false, ""))
expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true))

assert.Equal(t, calculatePatch(r, expectedPatch), patch)
}
Expand Down Expand Up @@ -840,7 +843,7 @@ func TestRollBackToStable(t *testing.T) {
"conditions": %s
}
}`
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "")
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true)
expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, hash.ComputePodTemplateHash(&r2.Spec.Template, r2.Status.CollisionCount), newConditions)
patch := f.getPatchedRollout(patchIndex)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
Expand Down Expand Up @@ -883,7 +886,7 @@ func TestGradualShiftToNewStable(t *testing.T) {
"conditions": %s
}
}`
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "")
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false)
expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newConditions)
patch := f.getPatchedRollout(patchIndex)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
Expand Down Expand Up @@ -931,7 +934,7 @@ func TestRollBackToStableAndStepChange(t *testing.T) {
}`
newPodHash := hash.ComputePodTemplateHash(&r2.Spec.Template, r2.Status.CollisionCount)
newStepHash := conditions.ComputeStepHash(r2)
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "")
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true)
expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newPodHash, newStepHash, newConditions)
patch := f.getPatchedRollout(patchIndex)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
Expand Down Expand Up @@ -969,7 +972,7 @@ func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) {
"conditions": %s
}
}`
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs3, false, "")
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs3, false, "", false)
assert.Equal(t, calculatePatch(r3, fmt.Sprintf(expectedPatch, newConditions)), patch)
}

Expand Down Expand Up @@ -1005,6 +1008,9 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) {
availableCondition, _ := newAvailableCondition(true)
conditions.SetRolloutCondition(&r2.Status, availableCondition)

completedCondition, _ := newCompletedCondition(false)
conditions.SetRolloutCondition(&r2.Status, completedCondition)

r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation))
f.rolloutLister = append(f.rolloutLister, r2)
f.objects = append(f.objects, r2)
Expand Down Expand Up @@ -1053,6 +1059,9 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) {
availableCondition, _ := newAvailableCondition(true)
conditions.SetRolloutCondition(&r2.Status, availableCondition)

completedCondition, _ := newCompletedCondition(false)
conditions.SetRolloutCondition(&r2.Status, completedCondition)

f.rolloutLister = append(f.rolloutLister, r2)
f.objects = append(f.objects, r2)

Expand Down Expand Up @@ -1617,6 +1626,9 @@ func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) {
availableCondition, _ := newAvailableCondition(true)
conditions.SetRolloutCondition(&r2.Status, availableCondition)

completedCondition, _ := newCompletedCondition(false)
conditions.SetRolloutCondition(&r2.Status, completedCondition)

f.kubeobjects = append(f.kubeobjects, rs1)
f.replicaSetLister = append(f.replicaSetLister, rs1)
f.rolloutLister = append(f.rolloutLister, r2)
Expand Down Expand Up @@ -1672,7 +1684,7 @@ func TestHandleCanaryAbort(t *testing.T) {
}
}`
errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2)
newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "")
newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "", false)
assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch)
})

Expand Down Expand Up @@ -1710,7 +1722,7 @@ func TestHandleCanaryAbort(t *testing.T) {
}
}`
errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 1)
newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r1, false, "")
newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r1, false, "", true)
assert.Equal(t, calculatePatch(r1, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch)
})
}
76 changes: 64 additions & 12 deletions rollout/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,28 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) {
return condition, string(conditionBytes)
}

func newHealthyCondition(isHealthy bool) (v1alpha1.RolloutCondition, string) {
status := corev1.ConditionTrue
msg := conditions.RolloutHealthyMessage
if !isHealthy {
status = corev1.ConditionFalse
msg = conditions.RolloutNotHealthyMessage
}
condition := v1alpha1.RolloutCondition{
LastTransitionTime: timeutil.MetaNow(),
LastUpdateTime: timeutil.MetaNow(),
Message: msg,
Reason: conditions.RolloutHealthyReason,
Status: status,
Type: v1alpha1.RolloutHealthy,
}
conditionBytes, err := json.Marshal(condition)
if err != nil {
panic(err)
}
return condition, string(conditionBytes)
}

func newCompletedCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) {
status := corev1.ConditionTrue
if !isCompleted {
Expand Down Expand Up @@ -315,33 +337,57 @@ func newAvailableCondition(available bool) (v1alpha1.RolloutCondition, string) {
return condition, string(conditionBytes)
}

func generateConditionsPatch(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string) string {
func generateConditionsPatch(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string {
_, availableCondition := newAvailableCondition(available)
_, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage)
_, completedCondition := newCompletedCondition(isCompleted)
if availableConditionFirst {
return fmt.Sprintf("[%s, %s]", availableCondition, progressingCondition)
return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, completedCondition)
}
return fmt.Sprintf("[%s, %s]", progressingCondition, availableCondition)
return fmt.Sprintf("[%s, %s, %s]", progressingCondition, availableCondition, completedCondition)
}

func generateConditionsPatchWithPause(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isPaused bool) string {
func generateConditionsPatchWithPause(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isPaused bool, isCompleted bool) string {
_, availableCondition := newAvailableCondition(available)
_, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage)
_, pauseCondition := newPausedCondition(isPaused)
_, completedCondition := newCompletedCondition(isCompleted)
if availableConditionFirst {
return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, completedCondition, progressingCondition, pauseCondition)
}
return fmt.Sprintf("[%s, %s, %s, %s]", progressingCondition, pauseCondition, availableCondition, completedCondition)
}

func generateConditionsPatchWithHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool, isCompleted bool) string {
_, availableCondition := newAvailableCondition(available)
_, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage)
_, healthyCondition := newHealthyCondition(isHealthy)
_, completedCondition := newCompletedCondition(isCompleted)
if availableConditionFirst {
return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, pauseCondition)
return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, completedCondition, healthyCondition, progressingCondition)
}
return fmt.Sprintf("[%s, %s, %s]", progressingCondition, pauseCondition, availableCondition)
return fmt.Sprintf("[%s, %s, %s, %s]", completedCondition, healthyCondition, progressingCondition, availableCondition)
}

func generateConditionsPatchWithComplete(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string {
func generateConditionsPatchWithCompleted(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string {
_, availableCondition := newAvailableCondition(available)
_, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage)
_, completeCondition := newCompletedCondition(isCompleted)
if availableConditionFirst {
return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition)
return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, completeCondition)
}
return fmt.Sprintf("[%s, %s, %s]", progressingCondition, availableCondition, completeCondition)
}

func generateConditionsPatchWithCompletedHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool, isCompleted bool) string {
_, completedCondition := newCompletedCondition(isCompleted)
_, availableCondition := newAvailableCondition(available)
_, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage)
_, healthyCondition := newHealthyCondition(isHealthy)
if availableConditionFirst {
return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, healthyCondition, completedCondition, progressingCondition)
}
return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition)
return fmt.Sprintf("[%s, %s, %s, %s]", healthyCondition, completedCondition, progressingCondition, availableCondition)
}

func updateConditionsPatch(r v1alpha1.Rollout, newCondition v1alpha1.RolloutCondition) string {
Expand All @@ -351,7 +397,7 @@ func updateConditionsPatch(r v1alpha1.Rollout, newCondition v1alpha1.RolloutCond
}

// func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool, available bool, progressingStatus string) *v1alpha1.Rollout {
func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable string, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool, available bool) *v1alpha1.Rollout {
func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable string, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool, available bool, isCompleted bool) *v1alpha1.Rollout {
newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas)
selector := newRollout.Spec.Selector.DeepCopy()
if active != "" {
Expand All @@ -363,6 +409,8 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable s
newRollout.Status.StableRS = stable
cond, _ := newAvailableCondition(available)
newRollout.Status.Conditions = append(newRollout.Status.Conditions, cond)
completeCond, _ := newCompletedCondition(isCompleted)
newRollout.Status.Conditions = append(newRollout.Status.Conditions, completeCond)
if pause {
now := timeutil.MetaNow()
cond := v1alpha1.PauseCondition{
Expand Down Expand Up @@ -1390,6 +1438,8 @@ func TestComputeHashChangeTolerationBlueGreen(t *testing.T) {
conditions.SetRolloutCondition(&r.Status, availableCondition)
progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs, "")
conditions.SetRolloutCondition(&r.Status, progressingCondition)
completedCondition, _ := newCompletedCondition(true)
conditions.SetRolloutCondition(&r.Status, completedCondition)
r.Status.Phase, r.Status.Message = rolloututil.CalculateRolloutPhase(r.Spec, r.Status)

podTemplate := corev1.PodTemplate{
Expand Down Expand Up @@ -1434,6 +1484,8 @@ func TestComputeHashChangeTolerationCanary(t *testing.T) {
conditions.SetRolloutCondition(&r.Status, availableCondition)
progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs, "")
conditions.SetRolloutCondition(&r.Status, progressingCondition)
completedCondition, _ := newCompletedCondition(true)
conditions.SetRolloutCondition(&r.Status, completedCondition)

podTemplate := corev1.PodTemplate{
Template: rs.Spec.Template,
Expand Down Expand Up @@ -1461,7 +1513,7 @@ func TestSwitchBlueGreenToCanary(t *testing.T) {
activeSvc := newService("active", 80, nil, r)
rs := newReplicaSetWithStatus(r, 1, 1)
rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
r = updateBlueGreenRolloutStatus(r, "", rsPodHash, rsPodHash, 1, 1, 1, 1, false, true)
r = updateBlueGreenRolloutStatus(r, "", rsPodHash, rsPodHash, 1, 1, 1, 1, false, true, false)
// StableRS is set to avoid running the migration code. When .status.canary.stableRS is removed, the line below can be deleted
//r.Status.Canary.StableRS = rsPodHash
r.Spec.Strategy.BlueGreen = nil
Expand All @@ -1479,7 +1531,7 @@ func TestSwitchBlueGreenToCanary(t *testing.T) {
f.run(getKey(r, t))
patch := f.getPatchedRollout(i)

addedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs, true, "")
addedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs, true, "", true)
expectedPatch := fmt.Sprintf(`{
"status": {
"blueGreen": {
Expand Down
Loading

0 comments on commit 6b3a907

Please sign in to comment.