CI-3567: Release CRD conditions - #422
Conversation
| // ReleaseConditionActive indicates whether the release is currently active in the cluster. | ||
| // Status=True means the release is actively serving traffic. | ||
| // Status=False means the release has been superseded by another release. | ||
| ReleaseConditionActive = "Active" |
There was a problem hiding this comment.
As mentioned in our discussion and #412 (comment), "phase" status field is considered deprecated, and should use conditions instead:
Some resources in the v1 API contain fields called phase, and associated message, reason, and other status fields. The pattern of using phase is deprecated. Newer API types should use conditions instead.
I do think we should refactor this to use Condition "Active" instead.
@0x0013 - As you suggested here, Phase with values (Active|Inactive) is now a Active condition with status (True|False).
| type HistoryEntry struct { | ||
| // The id of the history entry | ||
| // Unique ID of the history entry | ||
| ID int `json:"id"` | ||
| // Timestamp of when the history entry was added | ||
| Timestamp metav1.Time `json:"timestamp,omitempty"` | ||
|
|
||
| CommonStatusFields `json:",inline"` |
There was a problem hiding this comment.
The status.history will contain:
ID int
Timestamp metav1.Time
Message string
DeploymentStartTime metav1.Time
DeploymentEndTime metav1.Time
SupersededBy SupersededObj
// possibly SupersededFrom, if we decide we want to keep it|
Answering your comment in previous PR:
The idea for ---
# old release
apiVersion: deploy.gocardless.com/v1alpha1
kind: Release
metadata:
name: foo-1111
config:
revisions:
bar:
id: v1.0
status:
conditions:
- type: Active
status: False
history:
- supersededBy:
releaseRef: foo-2222
timestamp: 01:00
supersededFrom:
releaseRef: foo-0000
timestamp: 00:30
---
# active release
apiVersion: deploy.gocardless.com/v1alpha1
kind: Release
metadata:
name: foo-2222
config:
revisions:
bar:
id: v2.0
status:
conditions:
- type: Active
status: True
supersededFrom:
releaseRef: foo-1111
timestamp: 01:00Now, if we want to initiate a rollback, without a specific target, and find the latest release that we could roll back to, we can simply:
Finding the active release is trivial. However, in absence of
So, the process of reconstructing the chain from the current latest active release, is much more involved. I hope this clears it up. I'll comment on other parts later. |
There was a problem hiding this comment.
I've added a couple minor comments.
One thing I'm wondering is about when we populate the history list. If I understand correctly (and correct me if I'm wrong), the current intention is to hold both the historical activations, as well as the current one, in the history. The drawback here would be that we duplicate the information in the main status, and also that we have to change the history entry once the release is deactivated (to add the reference to the next release).
An alternative approach, might be to only populate the history with historical activations, and that also has 2 slightly different options. So in aggregate, the potential options would be:
- Add the historical entry immediately (duplicating also the current one), and modify the historical entry in case of change (e.g. if the release is superseded)
- Add the historical entry when the release is inactivated either by another deployment, or by rollback. We store only historical activations in the history, though we still duplicate the main status of an inactive release, but the entries in
historywill not change - Only add a
historyentry when the release gets re-activated a second time, effectively when the main status needs to be overwritten. We don't duplicate the main status, and the history entries don't change
I personally like option (3) the best, because the entries don't need to change, and in the vast majority of cases, the history list stays empty - it is only ever populated for releases which have been reactivated. In other words - we only add the history entry when we need to, as otherwise we would lose the necessary information, because the current status would be overwritten.
| // ReasonSuperseded indicates the release was superseded by a newer release. | ||
| ReasonSuperseded = "Superseded" |
There was a problem hiding this comment.
Given that a release can transition to a different release either with a new release, or a rollback, I think we should either change the code comment with "superseded by a different release", or add another reason such as "Rollback", to indicate the possibility that it was transitioned to an older release.
There was a problem hiding this comment.
Thanks for the suggestion - I believe there is a value in having both of the reasons, as you might re-activate a previous release without using the rollback procedure (e.g. using utopia set), so I will keep both ReasonSuperseded and ReasonRollback.
| @@ -90,31 +94,38 @@ type ReleaseStatusEntry struct { | |||
| DeploymentEndTime metav1.Time `json:"deploymentEndTime,omitempty"` | |||
There was a problem hiding this comment.
As mentioned, thinking about in terms of what's needed technically, and what is useful - these fields are needed for health evaluation of the currently active release (so, in the main status), but not in the history.
However, they might still be useful to see in the history, for example to reconstruct an incident timeline. So I'm fine to keep them either way - in common fields (as they are), or just in the main status.
There was a problem hiding this comment.
I would prefer to keep them in history, as you mentioned, to reconstruct an incident timeline, although this information will be available in the ArgoCD release history.
| // Unique ID of the history entry | ||
| ID int `json:"id"` | ||
| // Timestamp of when the history entry was added | ||
| Timestamp metav1.Time `json:"timestamp,omitempty"` |
There was a problem hiding this comment.
If we add the ref for the previous release, this timestamp might be redundant - I assume they will hold the same value.
There was a problem hiding this comment.
I agree that the field will be redundant if we only record transitions of the Active condition. However, my understanding is that we want to record any status changes, and we would be interested in having a timestamp of when those happened.
If the outcome of the conversation around what to record in the history is only record reactivations, yes, I agree that this field is redundant.
There was a problem hiding this comment.
However, my understanding is that we want to record any status changes, and we would be interested in having a timestamp of when those happened.
See my comment below, but I think this is a good case for using an Event (for non-activation transitions).
My intention was to only store the past statuses of the Release, i.e. store only status information which is no longer relevant. The logic would be
With option 3, we have the following transition of the Active condition for a release: |
| // Unique ID of the history entry | ||
| ID int `json:"id"` | ||
| // Timestamp of when the history entry was added | ||
| Timestamp metav1.Time `json:"timestamp,omitempty"` |
There was a problem hiding this comment.
I agree that the field will be redundant if we only record transitions of the Active condition. However, my understanding is that we want to record any status changes, and we would be interested in having a timestamp of when those happened.
If the outcome of the conversation around what to record in the history is only record reactivations, yes, I agree that this field is redundant.
| ReasonSuccessfulAnalysis = "SuccessfulAnalysis" | ||
|
|
||
| // ReasonFailedAnalysis indicates the release failed health analysis checks. | ||
| ReasonFailedAnalysis = "FailedAnalysis" |
There was a problem hiding this comment.
Same as previous comment - I think it makes sense to use Condition "Healthy" to mark this.
I don't think we need to store the past health state in the history. In general, we will be interested in the latest health state for this Release, in order to determine whether it is a valid rollback candidate. However, there could be an argument made that historical health status might be useful, in case a deployment becomes unhealthy on subsequent evaluation, after already being healthy. However, this would most likely occur in the case of external impact, so I'm not sure if we want to cover that use case.
@0x0013 - As you suggested here, HealthStatus and HealthStatusLastChecked are now the Healthy condition with status (True|False|Unknown).
There was a problem hiding this comment.
nit: I would prefer the words in reason to be reversed - AnalysisFailed and AnalysisSucceeded, but we can also discuss after merge.
|
At this time of the review process, the Release resource will look something like. Note This might not be the final version of the resource. Any changes will be documented in this PR. apiVersion: deploy.gocardless.com/v1alpha1
kind: Release
metadata:
name: my-app-release-v1.2.0
namespace: production
labels:
app: my-app
environment: production
spec:
config:
targetName: my-app-production
revisions:
- name: application-revision
id: 9af0d59284d6eb2ce7ce7ee3024b319ab50e67ae
source: https://github.com/myorg/my-app.git
type: container_image
- name: infrastructure-revision
id: 9eaf5890b3a03c34f3772505d30caf6c0cd08249
metadata:
author: jane.smith@example.com
branch: release/v1.2.0
message: "Release v1.2.0 - stable version"
status:
observedGeneration: 15
message: "Release re-activated after rollback from failed v1.3.0"
deploymentStartTime: "2024-12-12T14:30:00Z"
deploymentEndTime: "2024-12-12T14:35:00Z"
conditions:
- type: Active
status: "True"
reason: Rollback
message: "Release re-activated due to rollback from v1.3.0"
lastTransitionTime: "2024-12-12T14:35:00Z"
observedGeneration: 1
- type: Healthy
status: "True"
reason: SuccessfulAnalysis
message: "Health checks passing after rollback"
lastTransitionTime: "2024-12-12T14:40:00Z"
observedGeneration: 1
previousRelease:
releaseRef: my-app-release-v1.1.5
transitionTime: "2024-12-12T10:00:00Z"
history:
- id: 1
timestamp: "2024-12-12T10:00:00Z"
message: "Initial deployment of v1.2.0"
deploymentStartTime: "2024-12-12T09:55:00Z"
deploymentEndTime: "2024-12-12T10:00:00Z"
previousRelease:
releaseRef: my-app-release-v1.1.5
transitionTime: "2024-12-12T10:00:00Z"
- id: 2
timestamp: "2024-12-12T10:05:00Z"
message: "Release marked as active and healthy"
- id: 3
timestamp: "2024-12-12T12:00:00Z"
message: "Release superseded by v1.3.0"
nextRelease:
releaseRef: my-app-release-v1.3.0
transitionTime: "2024-12-12T12:00:00Z"
- id: 4
timestamp: "2024-12-12T14:30:00Z"
message: "Rollback initiated - v1.3.0 failed health checks"
deploymentStartTime: "2024-12-12T14:30:00Z"
- id: 5
timestamp: "2024-12-12T14:35:00Z"
message: "Release re-activated after rollback"
deploymentStartTime: "2024-12-12T14:30:00Z"
deploymentEndTime: "2024-12-12T14:35:00Z"
- id: 6
timestamp: "2024-12-12T14:40:00Z"
message: "Health analysis passed after rollback" |
Thanks for elaboration, I had misunderstood the current/previous approach. Yes, you are right that, in your example, we would only have a single history entry, at the point of transition (3). For now, I'm not convinced that we need to record every transition in history. I think the transitions can be implied. Let's investigate user's point of view as in your example, with a status:
conditions:
- type: Active
status: "True"
reason: Rollback
message: "Release re-activated due to rollback from v1.3.0"
previousRelease:
releaseRef: my-app-release-v1.3.0
transitionTime: "2024-12-12T10:50:00Z"
history:
- id: 1
timestamp: "2024-12-12T10:00:00Z" # not sure if needed - duplicates .transitionTime of either previous or next release
message: "Initial deployment of v1.2.0" # not convinced if needed, but illustrative for now
deploymentStartTime: "2024-12-12T09:55:00Z"
deploymentEndTime: "2024-12-12T10:00:00Z"
previousRelease:
releaseRef: null
transitionTime: "2024-12-12T10:00:00Z"
nextRelease:
releaseRef: my-app-release-v1.3.0
transitionTime: "2024-12-12T10:10:00Z"Looking at this resource, we can imply all necessary information:
The benefit from this is that we have a very clear and concise list, with each element representing an instance when this release was active, denoted by the duration between As mentioned above, in this case we can also clearly see how many times this release has been active - it's just the lenght( I've also noted above that in this case, the Hope that helps. Happy to chat more about this. |
|
I agree with @0x0013's points above with regards to keeping the @goelozev looking at your example however, I can see how the non-activation transitions might still be useful to a human wanting to observe a timeline of events, particularly if they're only interested in the most recent few transitions. One option would be to record them as Events, which is naturally where I'd go to find such information.
I think the other benefit of this option is that, for a reconcile event for a given |
|
@0x0013, @IsaacJames - thanks both for your thorough reviews!
I see how that could be an issue, and I agree that having fewer history entries is preferable for clarity and later integration with other tooling we have planned. I agree that we should only append reactivations to the history. I've removed the @IsaacJames, as for recording transitions, thanks for your suggestion to use Kubernetes events. I will have a look how to get them implemented in the controller. After the alternations, the CR will look like (ignore timestamps, as they might be inaccurate): apiVersion: deploy.gocardless.com/v1alpha1
kind: Release
metadata:
name: my-app-release-v1.2.0
namespace: production
labels:
app: my-app
environment: production
spec:
config:
targetName: my-app-production
revisions:
- name: application-revision
id: 9af0d59284d6eb2ce7ce7ee3024b319ab50e67ae
source: https://github.com/myorg/my-app.git
type: container_image
- name: infrastructure-revision
id: 9eaf5890b3a03c34f3772505d30caf6c0cd08249
metadata:
author: jane.smith@example.com
branch: release/v1.2.0
message: "Release v1.2.0 - stable version"
status:
observedGeneration: 15
message: "Release re-activated after rollback from failed v1.3.0"
deploymentStartTime: "2024-12-12T14:30:00Z"
deploymentEndTime: "2024-12-12T14:35:00Z"
conditions:
- type: Active
status: "True"
reason: Rollback
message: "Release re-activated due to rollback from v1.3.0"
lastTransitionTime: "2024-12-12T14:35:00Z"
observedGeneration: 1
- type: Healthy
status: "True"
reason: SuccessfulAnalysis
message: "Health checks passing after rollback"
lastTransitionTime: "2024-12-12T14:40:00Z"
observedGeneration: 1
previousRelease:
releaseRef: my-app-release-v1.3.0
transitionTime: "2024-12-12T14:35:00Z"
history:
- id: 1
message: "Initial deployment of v1.2.0"
deploymentStartTime: "2024-12-12T09:55:00Z"
deploymentEndTime: "2024-12-12T10:00:00Z"
previousRelease:
releaseRef: null
transitionTime: "2024-12-12T10:00:00Z"
nextRelease:
releaseRef: my-app-release-v1.3.0
transitionTime: "2024-12-12T10:10:00Z"
|
|
After our conversation today, I've made the following changes to the
With these modifications, the Release custom resource will have the shape of: apiVersion: deploy.gocardless.com/v1alpha1
kind: Release
metadata:
name: my-app-release-v1.2.0
namespace: production
labels:
app: my-app
environment: production
config:
targetName: my-app-production
revisions:
- name: application-revision
id: "9af0d59284d6eb2ce7ce7ee3024b319ab50e67ae"
source: "https://github.com/myorg/my-app.git"
type: container_image
- name: infrastructure-revision
id: "9eaf5890b3a03c34f3772505d30caf6c0cd08249"
metadata:
author: jane.smith@example.com
branch: release/v1.2.0
message: "Release v1.2.0 - stable version"
status:
observedGeneration: 15
message: "Release re-activated after rollback from failed v1.3.0"
deploymentStartTime: "2024-12-12T14:30:00Z"
deploymentEndTime: "2024-12-12T14:35:00Z"
previousRelease:
releaseRef: my-app-release-v1.3.0
transitionTime: "2024-12-12T14:35:00Z"
nextRelease:
releaseRef: ""
transitionTime: "2024-12-12T14:35:00Z"
signature: "81e12e31da614eaecc560e75b94b457c440b4227"
conditions:
- type: Active
status: "True"
reason: Rollback
message: "Release re-activated due to rollback from v1.3.0"
lastTransitionTime: "2024-12-12T14:35:00Z"
observedGeneration: 15
- type: Healthy
status: "True"
reason: SuccessfulAnalysis
message: "Health checks passing after rollback"
lastTransitionTime: "2024-12-12T14:40:00Z"
observedGeneration: 15
Added some printer columns to, so when listing _Release_s using > kubectl get releases
NAME TARGET_NAME ACTIVE HEALTHY SIGNATURE STARTED_AT ENDED_AT AGE
mock-08d9b451 mock False Unknown 2025-12-15T15:47:07Z 2025-12-15T15:49:07Z 26h
mock-188d05e9 mock True Unknown 2025-12-15T15:47:31Z 2025-12-15T15:49:31Z 26h
mock-76efe065 mock False Unknown 2025-12-15T15:47:04Z 2025-12-15T15:49:04Z 26h
mock-c61a3c4e mock False Unknown 2025-12-15T15:47:20Z 2025-12-15T15:49:20Z 26h
mock-fffc28e5 mock False Unknown 2025-12-15T15:47:24Z 2025-12-15T15:49:24Z 26h |
IsaacJames
left a comment
There was a problem hiding this comment.
Looks good! Thanks for sticking through all of our comments.
0x0013
left a comment
There was a problem hiding this comment.
Looks good, and thanks for refactoring after our extensive discussions.
I've left a few comments, but none of them are blocking, and can be discussed or changed later.
| ReasonSuccessfulAnalysis = "SuccessfulAnalysis" | ||
|
|
||
| // ReasonFailedAnalysis indicates the release failed health analysis checks. | ||
| ReasonFailedAnalysis = "FailedAnalysis" |
There was a problem hiding this comment.
nit: I would prefer the words in reason to be reversed - AnalysisFailed and AnalysisSucceeded, but we can also discuss after merge.
| // +kubebuilder:validation:Enum:=Active;Inactive | ||
| Phase Phase `json:"phase"` | ||
| // The generation observed by the release controller. | ||
| ObservedGeneration int64 `json:"observedGeneration,omitempty"` |
There was a problem hiding this comment.
Do we need to note the observed generation, if we don't reuse the Release resource, but create a new one if a rollback occurs?
There was a problem hiding this comment.
Short answer - not really!
However, our current mechanism to ensure that the Release .config is a webhook, which can fail (e.g. the controller is down), and someone could potentially kubectl edit the resource while the webhook fails. In this way, the observed generation will indicate that the status fields do not reflect the latest generation of the resource.
Another reason to keep it is that it is used in conditions. With our current plans for the implementation of the controller, the value will always be 1.
I don't see a reason to keep it except for the first one I described. I would rather keep it, but if you don't agree, do let me know!
There was a problem hiding this comment.
I think the value of having it is very limited. In conditions, it is usually useful to indicate that the condition might be outdated (if it was evaluated for a past generation). However, I think that only applies to in cases when spec/config can change. In our case, since the Release is representative, rather than declarative, we will only ever have one spec of the Release to converge to.
We also don't depend on the config to evaluate whether the release is Active, and we don't (currently) support repeat analysis to change Healthy state. So even if we did allow config changes, they wouldn't be represented.
I think we can remove it for now. It seems that adding it, if we ever need it, would still be a non-breaking change, whereas removing it would be breaking.
There was a problem hiding this comment.
Fair enough, removed it!
| History []HistoryEntry `json:"history,omitempty"` | ||
| // Signature is deterministic hash constructed out of the release revisions. | ||
| // The signature is constructed out of the sum of names and ids of each revision. | ||
| Signature string `json:"signature,omitempty"` |
There was a problem hiding this comment.
Not a comment on the code here, but rather the example - I think the lenght of hash is too long in the example, and should be shorter, especially if we output it in kubectl get -o wide view.
There was a problem hiding this comment.
Good point, I will trim it to 7 characters similarly to git's short commit SHA.
Unfortunately, Kubernetes CRDs additionalPrinterColumns doesn't allow you to format those columns in a specific way, so I think I might introduce a secondary signature field called SignatureShort = Signature[:7] just for the purposes of printing. I will also keep the Signature column for the very unlikely scenario where two releases end up with the same short, but different long versions.
There was a problem hiding this comment.
I'm not sure if we even need to store the long one. Our amount of releases will be quite limited, especially given that we will prune the old releases, so collision risk is minimal. And even theoretically, I think a collision would only present a problem if it happened for the same target. I think a single hash, trimmed to, let's say, 10 chars, would be good (probably even 8 would be fine, but 10 still seems short enough for visual representation).
Thoughts?
There was a problem hiding this comment.
Sounds fair, let's have one signature field trimmed to 10 characters!
| // +kubebuilder:printcolumn:name="Started_At",type="string",JSONPath=".status.deploymentStartTime" | ||
| // +kubebuilder:printcolumn:name="Ended_At",type="string",JSONPath=".status.deploymentEndTime" |
There was a problem hiding this comment.
I'm not sure how useful this is in the output. One of them is good, representing the time from when the release was/is active.
I'm not sure if this is possible, but for the other field, I think it would be more useful to have nextRelease.transitionTime, thus, user could easily see the whole start/end of when the release was active, which could be very useful to inspect. In fact, having both deployment start/end times in the concise output, might mislead someone to think that is exactly what it represents, whereas in reality, it only represents the time when the sync started/ended.
The reason I'm not sure if it's possible, is that nextRelease could be null, and thus the sub-field .transitionTime would not exist, and I don't know how it would be represented in the output.
I'm aware that the intended client tool would be utopia, not kubectl, but if we do include these printcolumns, would still be nice to make them as useful (and yet concise) as possible.
There was a problem hiding this comment.
The mock-3843f3f6 release in the following example is the Active one, therefore nextRelease is nil:
> k get releases
NAME TARGET_NAME ACTIVE HEALTHY SIGNATURE STARTED_AT ENDED_AT AGE
mock-3843f3f6 mock True Unknown 2441d93 2025-12-17T16:58:16Z 45m
mock-653bbfac mock False Unknown 2c5da2a 2025-12-17T16:58:16Z 2025-12-17T16:58:16Z 45m
mock-945e253b mock False Unknown 87b85e9 2025-12-17T16:58:16Z 2025-12-17T16:58:16Z 45m
mock-99f39729 mock False Unknown 031a2a2 2025-12-17T16:58:16Z 2025-12-17T16:58:16Z 45m
mock-ecbb17eb mock False Unknown eb3c4a9 2025-12-17T16:58:16Z 2025-12-17T16:58:16Z 45mI've made the change around your suggestion to use the status.nextRelease.transitionTime in the printer columns.
There was a problem hiding this comment.
Thanks, I think this is really good for UX. The STARTED_AT will line up with the ENDED_AT of the previous release. If I'm investigating an incident timeline, I know exactly where to look.
One minor drawback is that a problem could theoretically start slightly before STARTED_AT, because the sync started before that, and would fall at the end time of previous release. But I think it's a fair trade-off - observing such situation should still clearly indicate which release might be the cause.
the history.timestamp is redundant as can be implient from the previousRelease.transitionTime
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
after a discussion today, we decided that we would like to allow duplicate releases for a set of revisions, hence we won't need a history status field, as the list of Releases in a namespace will be the history itself
708e918 to
98a5f04
Compare
0x0013
left a comment
There was a problem hiding this comment.
Looks good!
Thanks for taking into account my comments.
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
After the conditions discussion in our previous pull, this one changes the Release status object to use conditions.
Please refer to the discussion below for the latest structure of the Release resource.