Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: change completed condition so it only triggers on pod hash changes also adds an event for when it does changes. #2203

Merged
merged 27 commits into from
Aug 24, 2022
Merged

fix: change completed condition so it only triggers on pod hash changes also adds an event for when it does changes. #2203

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
cfdff34
feat: add healthy event/condition and fix completed event/condition
zachaller Aug 19, 2022
4f6c55a
fix unit tests
zachaller Aug 19, 2022
a126e36
rename vars to make more sense and remove healthy event becase it wil…
zachaller Aug 19, 2022
7a4492c
fix unit tests
zachaller Aug 19, 2022
46d7ffd
possible fix for e2e
zachaller Aug 19, 2022
0da4c16
fix e2e
zachaller Aug 19, 2022
447c30a
unit test for complete function
zachaller Aug 19, 2022
326689c
small cleanup and changes to not check generation
zachaller Aug 19, 2022
114f073
fix unit test and proper behavior
zachaller Aug 19, 2022
3f96a3c
Fix e2e
zachaller Aug 19, 2022
995d973
rename and fix one unit test
zachaller Aug 19, 2022
c089087
fix unit tests
zachaller Aug 19, 2022
7f5b287
fix unit test
zachaller Aug 19, 2022
58caf4d
add seperate test for TestRolloutComplete
zachaller Aug 19, 2022
42dc139
renames
zachaller Aug 19, 2022
09cd520
fix e2e
zachaller Aug 20, 2022
dded8f5
Set Completed to false
zachaller Aug 22, 2022
1ff9a32
Add event
zachaller Aug 22, 2022
d9f49c8
fix e2e
zachaller Aug 22, 2022
16ee38a
refactor
zachaller Aug 22, 2022
5321593
Fix all but one unit test
zachaller Aug 23, 2022
26e3933
fix last unit test
zachaller Aug 23, 2022
81e7aac
lint
zachaller Aug 23, 2022
6d75be7
cleanup
zachaller Aug 23, 2022
7ddd590
Rename
zachaller Aug 24, 2022
d0523f6
More renames
zachaller Aug 24, 2022
8031146
small comment change
zachaller Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/apis/rollouts/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,6 +989,8 @@ const (
RolloutPaused RolloutConditionType = "Paused"
// RolloutCompleted means that rollout is in a completed state. It is still progressing at this point.
zachaller marked this conversation as resolved.
Show resolved Hide resolved
RolloutCompleted RolloutConditionType = "Completed"
// HealthyAndCompleted means that rollout is in a completed state and is healthy.
HealthyAndCompleted RolloutConditionType = "HealthyAndCompleted"
zachaller marked this conversation as resolved.
Show resolved Hide resolved
)

// 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
Open in desktop

Large diffs are not rendered by default.

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

Large diffs are not rendered by default.

38 changes: 25 additions & 13 deletions rollout/canary_test.go
View file
Open in desktop
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
Open in desktop
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.RolloutHealthyAndCompletedMessage
if !isHealthy {
status = corev1.ConditionFalse
msg = conditions.RolloutNotHealthyAndCompletedMessage
}
condition := v1alpha1.RolloutCondition{
LastTransitionTime: timeutil.MetaNow(),
LastUpdateTime: timeutil.MetaNow(),
Message: msg,
Reason: conditions.RolloutHealthyAndCompletedReason,
Status: status,
Type: v1alpha1.HealthyAndCompleted,
}
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