Skip to content

Add rollback controller reconcile loop - #438

Merged
IsaacJames merged 19 commits into
pre-release-5.1.0from
CI-3637-add-rollback-controller
Jan 16, 2026
Merged

Add rollback controller reconcile loop#438
IsaacJames merged 19 commits into
pre-release-5.1.0from
CI-3637-add-rollback-controller

Conversation

@IsaacJames

@IsaacJames IsaacJames commented Jan 5, 2026

Copy link
Copy Markdown

Rollback Controller Implementation

This PR implements the RollbackReconciler which manages Rollback resources by triggering deployments via a configurable CICD backend and polling for completion.

internal/controller/deploy/rollback_controller.go

  • Reconciliation loop:
    • Watches Rollback resources and triggers deployments via the injected Deployer
    • Polls for deployment status until completion (succeeded/failed)
    • Tracks attempt count with configurable retry limit
    • Updates rollback status conditions (InProgress, Succeeded)
    • Finds the currently active release to populate FromReleaseName

cmd/rollback-manager/main.go

  • Configurable CICD backend via --cicd-backend flag (noop, github)
  • GitHub token via --github-token flag / ROLLBACK_MANAGER_GITHUB_TOKEN env var
  • Rollback history limit via --rollback-history-limit flag

Reconcile Loop

┌─────────────────────────────────────────────────────────────────────────────┐
│                           Rollback Reconcile Loop                           │
└─────────────────────────────────────────────────────────────────────────────┘

                              ┌──────────────┐
                              │   Rollback   │
                              │   Created    │
                              └──────┬───────┘
                                     │
                                     ▼
                        ┌────────────────────────┐
                        │  Succeeded condition   │───Yes───▶ Done (skip)
                        │       exists?          │
                        └────────────┬───────────┘
                                     │ No
                                     ▼
                        ┌────────────────────────┐
                        │  Fetch target Release  │───NotFound───▶ markRollbackFailed
                        └────────────┬───────────┘
                                     │ Found
                                     ▼
                        ┌────────────────────────┐
                        │  InProgress == True?   │
                        └────────────┬───────────┘
                                     │
                    ┌────────────────┴────────────────┐
                    │ No                              │ Yes
                    ▼                                 ▼
        ┌───────────────────────┐       ┌───────────────────────┐
        │  triggerDeployment()  │       │ pollDeploymentStatus()│
        └───────────┬───────────┘       └───────────┬───────────┘
                    │                               │
                    ▼                               ▼
        ┌───────────────────────┐       ┌───────────────────────┐
        │  AttemptCount >= 3?   │       │   Deployment Status   │
        └───────────┬───────────┘       └───────────┬───────────┘
                    │                               │
        ┌───────────┴───────────┐       ┌───────────┼───────────┬───────────┐
        │ Yes                   │ No    │ Succeeded │ Failed    │ Pending/  │
        ▼                       ▼       ▼           ▼           │ InProgress│
   markRollback           Call CICD  markRollback  Retry?       ▼           
   Failed()               Deployer   Succeeded()     │      Requeue         
                              │                      │      (15s)           
                              ▼               ┌──────┴──────┐               
                    Set InProgress=True       │ Yes         │ No            
                    Requeue (15s)             ▼             ▼               
                                        triggerDeployment  markRollback     
                                        (immediate retry)  Failed()         

Error Handling

  • Retryable errors (5xx, rate limits): Requeue after 15s, increment attempt count
  • Non-retryable errors: Mark rollback as failed immediately
  • Max retries exceeded: Mark rollback as failed after 3 attempts

Rollback Status Conditions

Condition Status Meaning
InProgress True Deployment is running
InProgress False + Succeeded Terminal state reached
Succeeded True Rollback completed successfully
Succeeded False Rollback failed (all retries exhausted)

Manager Configuration

--cicd-backend       CICD backend to use (noop, github) [default: noop]
--github-token       GitHub API token (required for github backend)

@goelozev goelozev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

Comment thread internal/controller/deploy/rollback_controller.go Outdated
return false
}

// TODO: move this into api/deploy/v1alpha1/release_helpers.go once release reconciler PR is merged

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@IsaacJames
IsaacJames force-pushed the CI-3637-add-rollback-controller branch 2 times, most recently from e41713a to 2adf735 Compare January 12, 2026 15:25
@IsaacJames IsaacJames changed the title WIP: add rollback controller reconcile loop Add rollback controller reconcile loop Jan 12, 2026
@IsaacJames
IsaacJames marked this pull request as ready for review January 12, 2026 16:15
@IsaacJames
IsaacJames force-pushed the CI-3637-add-rollback-controller branch 2 times, most recently from 1eb2a73 to 05eccce Compare January 12, 2026 17:35
@IsaacJames
IsaacJames force-pushed the CI-3637-add-rollback-controller branch from 05eccce to c203ff0 Compare January 12, 2026 17:53

@goelozev goelozev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fabulous!

Comment thread internal/controller/deploy/rollback_controller.go Outdated
Comment thread internal/controller/deploy/integration/rollback_test.go

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

Nicely done!

Added a couple comments.

Comment thread internal/controller/deploy/rollback_controller.go Outdated
Comment thread internal/controller/deploy/rollback_controller.go Outdated
Comment on lines +203 to +209
meta.SetStatusCondition(&rollback.Status.Conditions, metav1.Condition{
Type: deployv1alpha1.RollbackConditionInProgress,
Status: metav1.ConditionTrue,
Reason: "Retrying",
Message: fmt.Sprintf("Deployment attempt %d failed: %s. Retrying...", rollback.Status.AttemptCount, statusResp.Message),
LastTransitionTime: now,
})

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.

we're setting the condition here, but it seems it will get overwritten almost immediately in triggerDeployment.

@IsaacJames IsaacJames Jan 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not necessarily - if triggerDeployment fails to trigger the deployment with a retryable error, the condition won't be updated and the Rollback will get requeued for reconciliation. That said, I'm now wondering if the rollback.Status.Message message set on line 144 would be better set on the InProgress condition.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@0x0013 I've decided to simplify this a bit and agree it doesn't make sense to set the InProgress=True condition here in pollDeploymentStatus. Instead, we now set it once at the start of triggerDeployment with Reason=DeploymentTriggering and once at the end after a successful trigger with Reason=DeploymentTriggered.

This should make the behaviour clearer when a Rollback gets stuck trying and failing to trigger a deployment via the cicd backend, as the reason DeploymentTriggering will remain, whilst the failures themselves will be recorded in the rollback.Status.Message.

Status: metav1.ConditionFalse,
Reason: "Completed",
Message: "Rollback deployment completed",
LastTransitionTime: now,

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.

Same as noted previously, the transition time should get set by setStatusCondition.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm leaning towards leaving this (and the LastTransitionTime below) as set explicitly since this is the only transition the InProgress condition will make to False (and likewise below with the Succeeded condition to True).

The value I see is that by setting rollback.Status.CompletionTime and the LastTransitionTime of both conditions in this function to the same now value, it'll be clearer that the conditions transitioned to the terminal states as part of the completion of the rollback (particularly programmatically, if we ever need to compare the timestamps).

@IsaacJames
IsaacJames force-pushed the CI-3637-add-rollback-controller branch from aa85c7c to a7ac338 Compare January 14, 2026 11:17
@IsaacJames
IsaacJames requested review from 0x0013 and goelozev January 14, 2026 11:18

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

Added a few more comments.

One thing I'm not fully convinced about, is setting the RollbackConditionInProgress with the "DeploymentTriggering" reason. While I understand the idea behind the behavior, I think I would prefer for RollbackConditionInProgress==True to mean that Deployment has already been triggered. It would make it clearer what the condition state implies.

However, I'm not yet fully convinced myself, and I will think a little more whether the current approach could be problematic down the line.

Comment thread internal/controller/deploy/rollback_controller.go
Comment thread internal/controller/deploy/rollback_controller.go Outdated
Comment thread internal/controller/deploy/rollback_controller.go Outdated
Comment thread internal/controller/deploy/rollback_controller.go Outdated
Comment thread internal/controller/deploy/rollback_controller.go Outdated
@IsaacJames
IsaacJames requested a review from 0x0013 January 14, 2026 17:05
@0x0013

0x0013 commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

@IsaacJames another thing not directly related to changes in this PR, but rather the reference fields in the CRD:
do you think the spec.toReleaseName and status.fromReleaseName should better be named fromReleaseRef and toReleaseRef?

See Object References section in API Conventions for details.

Using the example in the doc, the full ref would then be:

toReleaseRef:
    name: foo

@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 left another small comment, in case you have ideas how to refactor it. But otherwise, this looks good - great job!

Comment thread cmd/rollback-manager/main.go Outdated
}
}

func createDeployer(ctx context.Context, backend, githubToken string) (cicd.Deployer, error) {

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 a fan of having a github-specific argument, given that you've nicely made the Deployer an interface.. But I can't currently come up with an elegant way to do it otherwise, so I guess it's OK, with the argument only being checked when github is being used.

@IsaacJames IsaacJames Jan 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Actually, given githubToken is a global variable, I can just get rid of the argument in this method.

@IsaacJames
IsaacJames merged commit f621382 into pre-release-5.1.0 Jan 16, 2026
5 checks passed
@IsaacJames
IsaacJames deleted the CI-3637-add-rollback-controller branch January 16, 2026 09:35
@goelozev goelozev mentioned this pull request Jan 19, 2026
0x0013 pushed a commit that referenced this pull request Jan 29, 2026
0x0013 pushed a commit that referenced this pull request Mar 13, 2026
0x0013 pushed a commit that referenced this pull request Mar 16, 2026
goelozev pushed a commit that referenced this pull request Apr 15, 2026
goelozev pushed a commit that referenced this pull request Apr 15, 2026
goelozev pushed a commit that referenced this pull request Apr 15, 2026
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