fix: Change completed condition to healthy condition and add a new completed condition #2147
Conversation
Signed-off-by: zachaller <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2147 +/- ##
==========================================
+ Coverage 82.21% 82.22% +0.01%
==========================================
Files 121 121
Lines 17865 17966 +101
==========================================
+ Hits 14687 14772 +85
- Misses 2413 2423 +10
- Partials 765 771 +6
Continue to review full report at Codecov.
|
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
|
lgtm. @jessesuen please review when you get a chance |
Signed-off-by: zachaller <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Since we are introducing new condition for Healthy vs. Completed I feel like we need to also introduce and emit the corresponding Kubernetes event for when a Rollout completes an update.
This also means we won't technically break backwards compatibility for users who are looking for that event.
| // RolloutHealthy means that rollout is in a completed state. It is still progressing at this point. | ||
| RolloutHealthy RolloutConditionType = "Healthy" | ||
| // RolloutCompleted means that rollout is in a completed state. It is still progressing at this point. | ||
| RolloutCompleted RolloutConditionType = "Completed" |
There was a problem hiding this comment.
Looks like we need to fix the descriptions. I suggest:
// RolloutHealthy indicates that the rollout is fully updated and all of its pods are available. A Healthy rollout implies that it is also Completed (but not vice versa).
RolloutHealthy RolloutConditionType = "Healthy"
// RolloutCompleted indicates that the rollout has completed its update to the desired version. A rollout can be both Completed as well as Degraded/Progressing (e.g. if its Pods are unavailable sometime after a completed update).
RolloutCompleted RolloutConditionType = "Completed"| @@ -107,7 +107,7 @@ spec: | |||
| "RolloutStepCompleted", // Rollout step 2/2 completed (pause: 3s) | |||
| "RolloutResumed", // Rollout is resumed | |||
| "ScalingReplicaSet", // Scaled down ReplicaSet abort-retry-promote-698fbfb9dc (revision 1) from 1 to 0 | |||
| "RolloutCompleted", // Rollout completed update to revision 2 (75dcb5ddd6): Completed all 2 canary steps | |||
| "RolloutHealthy", // Rollout healthy update to revision 2 (75dcb5ddd6): Completed all 2 canary steps | |||
There was a problem hiding this comment.
Why don't we see the RolloutCompleted in the events?
| // RolloutHealthyReason is added in a rollout when it is healthy. | ||
| RolloutHealthyReason = "RolloutHealthy" | ||
| // RolloutHealthyMessage is added when the rollout is healthy | ||
| RolloutHealthyMessage = "Rollout healthy update to revision %d (%s): %s" | ||
|
|
There was a problem hiding this comment.
Including revision number doesn't make sense anymore for when a Rollout becomes Healthy. I feel we need to emit two events, one for each condition (Healthy vs. Completed). Users will care separately when a Rollout finishes an update, but they will also care about from when a Rollout degrades (e.g. because of a pod restart) and subsequently recovers.
There was a problem hiding this comment.
The only place this is used is here https://github.com/argoproj/argo-rollouts/pull/2147/files/ee78ef0b85d3af3b21cd533930dc10b35b70c78d#diff-594206431376a15241cfdd95572e5d6e586929f867ca28ff63d1f679c0334d77R936-R938 which I added a TODO as well because I did not think it was the correct event to emit. I will look at removing that message and changing that event to something that makes sense.
There was a problem hiding this comment.
Ah yes. I think that part of the code should not change and we should continue to emit RolloutCompletedReason when we switch stable hash to current hash. Not that the rollout is Healthy.
closes #2103
Signed-off-by: zachaller [email protected]