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 <[email protected]>

* fix unit tests

Signed-off-by: zachaller <[email protected]>

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

Signed-off-by: zachaller <[email protected]>

* fix unit tests

Signed-off-by: zachaller <[email protected]>

* possible fix for e2e

Signed-off-by: zachaller <[email protected]>

* fix e2e

Signed-off-by: zachaller <[email protected]>

* unit test for complete function

Signed-off-by: zachaller <[email protected]>

* small cleanup and changes to not check generation

Signed-off-by: zachaller <[email protected]>

* fix unit test and proper behavior

Signed-off-by: zachaller <[email protected]>

* Fix e2e

Signed-off-by: zachaller <[email protected]>

* rename and fix one unit test

Signed-off-by: zachaller <[email protected]>

* fix unit tests

Signed-off-by: zachaller <[email protected]>

* fix unit test

Signed-off-by: zachaller <[email protected]>

* add seperate test for TestRolloutComplete

Signed-off-by: zachaller <[email protected]>

* renames

Signed-off-by: zachaller <[email protected]>

* fix e2e

Signed-off-by: zachaller <[email protected]>

* Set Completed to false

Signed-off-by: zachaller <[email protected]>

* Add event

Signed-off-by: zachaller <[email protected]>

* fix e2e

Signed-off-by: zachaller <[email protected]>

* refactor

Signed-off-by: zachaller <[email protected]>

* Fix all but one unit test

Signed-off-by: zachaller <[email protected]>

* fix last unit test

Signed-off-by: zachaller <[email protected]>

* lint

Signed-off-by: zachaller <[email protected]>

* cleanup

Signed-off-by: zachaller <[email protected]>

* Rename

Signed-off-by: zachaller <[email protected]>

* More renames

Signed-off-by: zachaller <[email protected]>

* small comment change

Signed-off-by: zachaller <[email protected]>

Signed-off-by: zachaller <[email protected]>
  • Loading branch information
@zachaller @jenciso
zachaller authored and jenciso committed Oct 25, 2022
1 parent 242e73e commit 67c6b32
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 67c6b32

Please sign in to comment.