Skip to content

CI-3567: Release CRD conditions - #422

Merged
goelozev merged 22 commits into
pre-release-5.1.0from
CI-3567-release-crds
Dec 18, 2025
Merged

CI-3567: Release CRD conditions#422
goelozev merged 22 commits into
pre-release-5.1.0from
CI-3567-release-crds

Conversation

@goelozev

@goelozev goelozev commented Dec 10, 2025

Copy link
Copy Markdown
Member

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.

@goelozev
goelozev requested a review from a team December 10, 2025 17:48
@goelozev
goelozev changed the base branch from master to pre-release-5.1.0 December 10, 2025 17:50
@goelozev goelozev changed the title [WIP] CI-3567: Release CRD status refactor CI-3567: Release CRD conditions Dec 10, 2025
Comment on lines +12 to +15
// 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"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread api/deploy/v1alpha1/release_types.go Outdated
Comment on lines +99 to +105
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"`

@goelozev goelozev Dec 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@0x0013

0x0013 commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

Answering your comment in previous PR:

On your second point about supersededFrom - isn't that the release itself? Or I might have misunderstood!

The idea for supersededFrom (and I'm not convinced of using that specific field name, it could be something like previousRelease), is that it would be a reference of the previous release. I'll try to illustrate with a simplified example of 2 releases. Note that most fields are deliberately omitted for illustration only.

---
# 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:00

Now, 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:

  1. Parse the supersededFrom field in the current active release;
  2. Parse the Release from previous step and check if it is a suitable rollback candidate (by any measure we deem sufficient, such as health status);
  3. If not, parse the history list to find the timestamp identified in step (1);
  4. Repeat step (2) with the supersededFrom field in the identified element from history list.

Finding the active release is trivial. However, in absence of supersededFrom field, we would have to:

  1. Identify the active release;
  2. Loop through each Release object;
  3. Loop through each history list element, checking if supersededTo refers to the active release;
  4. When the correct ref is found, check if the release is suitable for rollback;
  5. If that release is unsuitable, we need to repeat from step (2) again, looking for the reference to the identified Release until we find a suitable candidate

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.

@0x0013 0x0013 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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)
  2. 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 history will not change
  3. Only add a history entry 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.

Comment thread api/deploy/v1alpha1/release_types.go Outdated
Comment on lines +28 to +29
// ReasonSuperseded indicates the release was superseded by a newer release.
ReasonSuperseded = "Superseded"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread api/deploy/v1alpha1/release_types.go Outdated
Comment on lines 90 to 94
@@ -90,31 +94,38 @@ type ReleaseStatusEntry struct {
DeploymentEndTime metav1.Time `json:"deploymentEndTime,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread api/deploy/v1alpha1/release_types.go Outdated
// Unique ID of the history entry
ID int `json:"id"`
// Timestamp of when the history entry was added
Timestamp metav1.Time `json:"timestamp,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add the ref for the previous release, this timestamp might be redundant - I assume they will hold the same value.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@goelozev

goelozev commented Dec 12, 2025

Copy link
Copy Markdown
Member Author

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).

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 previous.status != current.status then history.push(makeHistoryEntry(previous.status)).

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:

  1. 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)
  2. 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 history will not change
  3. Only add a history entry 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.

With option 3, we have the following transition of the Active condition for a release: (1) Active=true -> (2) Active=false -> (3) Active=true and the release is at the (3) transition, and if I'm a user and I'm querying a Release status, I would expect historical data for the other 2 transitions i.e. we will have 2 entries in the history array. With your suggestion for option (3) , I understand we will only have would have only 1 entry. Correct me if my understanding is wrong!

Comment thread api/deploy/v1alpha1/release_types.go Outdated
// Unique ID of the history entry
ID int `json:"id"`
// Timestamp of when the history entry was added
Timestamp metav1.Time `json:"timestamp,omitempty"`

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread api/deploy/v1alpha1/release_types.go Outdated
ReasonSuccessfulAnalysis = "SuccessfulAnalysis"

// ReasonFailedAnalysis indicates the release failed health analysis checks.
ReasonFailedAnalysis = "FailedAnalysis"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would prefer the words in reason to be reversed - AnalysisFailed and AnalysisSucceeded, but we can also discuss after merge.

@goelozev
goelozev marked this pull request as ready for review December 12, 2025 12:26
@goelozev

Copy link
Copy Markdown
Member Author

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"

@0x0013

0x0013 commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

With option 3, we have the following transition of the Active condition for a release: (1) Active=true -> (2) Active=false -> (3) Active=true and the release is at the (3) transition, and if I'm a user and I'm querying a Release status, I would expect historical data for the other 2 transitions i.e. we will have 2 entries in the history array. With your suggestion for option (3) , I understand we will only have would have only 1 entry. Correct me if my understanding is wrong!

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 Release that has had 3 transitions:

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 release is currently active (condition Active - this is transition (3) in your example)
  • Previously active release was v1.3.0
  • In the past, the release has been activated exactly 1 times (length of .status.history)
  • The previous activation (id 1) is currently inactive (by definition, otherwise it would not be in history)
  • The previous activation (id 1) was superseded by v1.3.0 (via .nextRelease - this is transition (2) in your example)
  • The previous activation (id 1) was active before inactivation (by definition, releases have to be active before becoming inactive)
  • The previous activation (id 1) was the first ever release of this target (because previousRelease.releaseRef is null, but in most cases, we would see a different release here. This is transition (1) in your example)

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 previousRelease and nextRelease timestamps. Also, the schema of each element is consistent, as they have a clear link to the previous/next releases following, as well as the deployment start/end times. In contrast, if we record each transition, each element could be a different "type", requiring more complex parsing to first identify which kind of entry it is, and then do some conditional logic to parse each type differently.

As mentioned above, in this case we can also clearly see how many times this release has been active - it's just the lenght(.status.history) + 1 (the activation in root of .status) - whereas with transitions, it's not as clear. In your resource example above, with 6 history entries, I have to do quite a lot more mental parsing - count which history entries have a previousRelease field, then check whether the current status is active, and add 1 if it is active (because that's not in history), or don't add 1 if it's inactive (because that has been recorded in history and already counted).

I've also noted above that in this case, the timestamp of a history entry seems redundant, because it would implicitly be the same as the prev/next release references (not really important which of prev/next, as we have both available, denoting both when the release was activated, and when it was deactivated).

Hope that helps. Happy to chat more about this.

@IsaacJames

IsaacJames commented Dec 15, 2025

Copy link
Copy Markdown

I agree with @0x0013's points above with regards to keeping the status.history to activation transitions only. The more concise history is easier to parse once familiar with the implications described. It'll likely simplify parsing logic in the controller and Utopia CLI too.

@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.

  1. Only add a history entry 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 think the other benefit of this option is that, for a reconcile event for a given Release resource, we're eliminating the cascading of status.history updates to previous release resources. This seems like it'll simplify the reconciliation logic.

@goelozev

Copy link
Copy Markdown
Member Author

@0x0013, @IsaacJames - thanks both for your thorough reviews!

As mentioned above, in this case we can also clearly see how many times this release has been active - it's just the lenght(.status.history) + 1 (the activation in root of .status) - whereas with transitions, it's not as clear. In your resource example above, with 6 history entries, I have to do quite a lot more mental parsing - count which history entries have a previousRelease field, then check whether the current status is active, and add 1 if it is active (because that's not in history), or don't add 1 if it's inactive (because that has been recorded in history and already counted).

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 history.timestamp field, as @0x0013 suggested that information can be implied.

@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"
      

Comment thread api/deploy/v1alpha1/release_types.go Outdated
@goelozev

goelozev commented Dec 16, 2025

Copy link
Copy Markdown
Member Author

@0x0013, @IsaacJames

After our conversation today, I've made the following changes to the Status struct:

  • Removed History as we discussed - this field won't be needed as we plan to have duplicate releases for a set of revisions, and the release history can be inferred from simply listing the releases.
  • Added a Signature field, which will be a deterministic SHA generated on the basis of the revision, similar to the GenerateReleaseName function.

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 you get:

> 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
IsaacJames self-requested a review December 17, 2025 16:17

@IsaacJames IsaacJames left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for sticking through all of our comments.

@0x0013 0x0013 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread api/deploy/v1alpha1/release_types.go Outdated
ReasonSuccessfulAnalysis = "SuccessfulAnalysis"

// ReasonFailedAnalysis indicates the release failed health analysis checks.
ReasonFailedAnalysis = "FailedAnalysis"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would prefer the words in reason to be reversed - AnalysisFailed and AnalysisSucceeded, but we can also discuss after merge.

Comment thread api/deploy/v1alpha1/release_types.go Outdated
// +kubebuilder:validation:Enum:=Active;Inactive
Phase Phase `json:"phase"`
// The generation observed by the release controller.
ObservedGeneration int64 `json:"observedGeneration,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@goelozev goelozev Dec 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fair, let's have one signature field trimmed to 10 characters!

Comment thread api/deploy/v1alpha1/release_types.go Outdated
Comment on lines +137 to +138
// +kubebuilder:printcolumn:name="Started_At",type="string",JSONPath=".status.deploymentStartTime"
// +kubebuilder:printcolumn:name="Ended_At",type="string",JSONPath=".status.deploymentEndTime"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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   45m

I've made the change around your suggestion to use the status.nextRelease.transitionTime in the printer columns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

goelozev and others added 11 commits December 18, 2025 12:06
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
@goelozev
goelozev force-pushed the CI-3567-release-crds branch from 708e918 to 98a5f04 Compare December 18, 2025 11:10

@0x0013 0x0013 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Thanks for taking into account my comments.

@goelozev
goelozev merged commit 20eb4a3 into pre-release-5.1.0 Dec 18, 2025
5 checks passed
@goelozev
goelozev deleted the CI-3567-release-crds branch December 18, 2025 13:19
0x0013 pushed a commit that referenced this pull request Dec 18, 2025
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
@goelozev goelozev mentioned this pull request Jan 19, 2026
0x0013 pushed a commit that referenced this pull request Jan 29, 2026
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
0x0013 pushed a commit that referenced this pull request Mar 13, 2026
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
0x0013 pushed a commit that referenced this pull request Mar 16, 2026
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
goelozev added a commit that referenced this pull request Apr 15, 2026
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
goelozev added a commit that referenced this pull request Apr 15, 2026
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
goelozev added a commit that referenced this pull request Apr 15, 2026
Co-authored-by: Isaac James <isaac.james@hotmail.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants