Skip to content

fix(gateway-operator): recover interrupted and exhausted deployments - #3136

Open
sanjulaonline wants to merge 4 commits into
wso2:mainfrom
sanjulaonline:fix/3105-recover-failed-gateway-deployments
Open

fix(gateway-operator): recover interrupted and exhausted deployments#3136
sanjulaonline wants to merge 4 commits into
wso2:mainfrom
sanjulaonline:fix/3105-recover-failed-gateway-deployments

Conversation

@sanjulaonline

@sanjulaonline sanjulaonline commented Aug 4, 2026

Copy link
Copy Markdown

Purpose

An APIGateway can end up permanently wedged. Two independent defects combine, and each is worth fixing on its own.

1. An interrupted Helm operation is never recovered. InstallOrUpgrade chose between install and upgrade purely on whether release history existed:

histClient := action.NewHistory(actionConfig)
_, err = histClient.Run(opts.ReleaseName)
releaseExists := err == nil
if releaseExists { return c.upgrade(...) }
return c.install(...)

A release stuck in pending-install does have history, so the operator called upgrade, which Helm refuses with another operation (install/upgrade/rollback) is in progress. Nothing inspected rel.Info.Status, so no pending state was ever recovered. Two further problems in the same three lines: the returned history was discarded entirely, and releaseExists := err == nil treated any error — a transient storage failure, an unreachable cluster — as "the release is absent", which would start a fresh install over a live release.

2. After retry exhaustion the CR cannot be re-driven. handleGatewayDeploymentError recorded exhaustion as GatewayTrackingStatusDeployed while persisting Programmed=False / DeploymentFailed with observedGeneration == metadata.generation. decideAndProcess then had two live branches: one requiring Programmed=True, the other gated on crGeneration > statusObservedGen. An equal-generation failure satisfies neither, so control fell through to "nothing to do". Restarting the operator, re-annotating, fixing the configRef ConfigMap and waiting for the resync all did nothing. There was also an equal-generation branch nested inside the crGeneration > statusObservedGen block, which was unreachable.

3. A failed status write wedged the gateway too (found during a second-pass audit of this PR). The tracker was moved to Processing before setGatewayInitialConditions, and to Deployed before the Programmed=True patch. If the patch failed, the reconcile returned an error — but the tracker now told the next reconcile that another operation owned the generation, so it skipped and returned nil, ending the requeue chain that the error had started.

Fixes #3105

Related, deliberately out of scope: #3104 is the most common trigger for (1) — a RollingUpdate + ReadWriteOnce deadlock that fails the operator's Wait: true Helm operation — but it is a separate Helm-chart problem and is not touched here.

Goals

Recover a Helm release left in a pending state, and make an exhausted APIGateway recoverable without editing its generation — without introducing a hot reconcile loop.

Approach

Helm recovery

planRelease is a pure function deciding the operation from the release's stored history:

Latest revision status Action
no history / not found install
deployed upgrade
failed, superseded, unknown upgrade (existing behaviour preserved)
uninstalled (history retained) purge, then install
pending-install at revision 1, no successful revision purge, then install
pending-install beyond revision 1, successful revision exists roll back, then upgrade
pending-upgrade, pending-rollback roll back to newest successful revision, then upgrade
any pending state with no successful revision explicit error, nothing destroyed
uninstalling explicit error — the recorded intent was removal, so neither upgrading nor purging is inferable

uninstalled must purge rather than install: Install.availableName in helm v3.21.3 rejects a name whose history is retained with cannot re-use a name that is still in use unless Replace is set, which this package does not set. The uninstall already removed the resources, so only history is discarded.

The safety property for a purge is that no live resources are discarded — not that no successful revision exists. An uninstalled release may still hold earlier superseded revisions in retained history; those resources are already gone.

Other Helm changes: revisions are compared numerically rather than trusting history order; only errors.Is(err, driver.ErrReleaseNotFound) counts as absent and every other error is propagated; install/upgrade use RunWithContext so a cancelled reconcile or operator shutdown aborts the wait; and CleanupOnFail is set on upgrade and rollback. CleanupOnFail deletes resources the failed operation created — it does not roll the release back; the release still records a failed revision, which planRelease upgrades from next time. Atomic is deliberately not enabled: it would change failure semantics and add a second wait inside the operator's Helm timeout, and recovery does not need it.

Reconciliation recovery

  • An explicit GatewayTrackingStatusFailed replaces "record the exhausted deployment as Deployed". The persisted condition still truthfully reports Programmed=False / DeploymentFailed.
  • A new equal-generation branch handles a CR that is observed but not programmed — where an exhausted deployment actually lands. A changed input deploys immediately and resets the retry budget; an unchanged failure requeues without deploying and without writing status until the retry window elapses, then gets one bounded attempt; no tracker entry (operator restart) deploys.
  • The retry window follows the configured sync_period, so RequeueAfter matches the operator's own resync cadence.
  • The input fingerprint covers the configRef ConfigMap values and spec.infrastructure labels/annotations — everything that changes what would be rendered. Both are already-marshalled YAML with sorted keys, so it does not depend on Go map ordering.
  • The unreachable equal-generation branch was removed.
  • A failed status write now drops the tracking entry, so the next reconcile re-derives state from the CR rather than trusting a state whose owning reconcile has returned.

Not supported: kubectl annotate as an immediate retry trigger

The issue lists re-annotating the CR as an attempted recovery. That remains not an immediate trigger, deliberately. metadata.annotations are not deployment inputs — only spec.infrastructure labels/annotations reach the rendered values via commonAnnotations — so fingerprinting them would retry on changes that cannot alter the outcome. It is also a hot-loop risk: this codebase already writes operator-managed annotations onto resources it reconciles (httproute_controller.go), so any such annotation added to APIGateway later would retrigger deployments on the operator's own writes. Making re-annotation a supported reset means introducing a documented reset annotation, which is a public API decision for the maintainers rather than something to add silently here.

The gateway is still not wedged by this: it recovers on the bounded retry window, on an operator restart, and on a ConfigMap correction. TestDecideAndProcess_metadataAnnotationOnlyChangeWaitsForRetryWindow asserts that honest behaviour so the limitation cannot be mistaken for a fix.

User stories

As an operator whose gateway failed to install, I can fix the cause — correct the ConfigMap, restart the operator, or simply wait for the resync — and see the gateway recover, instead of having to delete the APIGateway CR and the Helm release by hand.

Documentation

N/A — no configuration, API or CRD surface changes. Recovery behaviour that was previously impossible now happens automatically.

Automation tests

  • Unit tests

    Helm planning (internal/helm/recovery_test.go): every status in the table above, unordered history, nil entries, numeric revision selection, and the two explicit-error cases.

    Helm dispatch (internal/helm/dispatch_test.go, new): asserts the operations InstallOrUpgrade actually dispatches, in order, through a small releaseExecutor seam — missing history → install; deployed → upgrade; revision-1 pending-install → purge,install; uninstalled → purge,install; pending-upgrade/pending-rollback → rollback,upgrade with the planned revision; unsafe pending state and a non-not-found history error → nothing dispatched. Also: no install if purge fails, no upgrade if rollback fails, operation errors propagate, the caller's context reaches every operation, newPurgeAction keeps KeepHistory=false, and newRollbackAction targets the planned revision. The planner suite alone could not catch a valid plan dispatched wrongly — which is exactly how the uninstalled bug reached review.

    Controller recovery (internal/controller/apigateway_recovery_test.go): exhaustion records Failed and never Deployed; it writes Programmed=False/DeploymentFailed and schedules a bounded retry; restart-with-persisted-failure recovers; ConfigMap change and spec.infrastructure annotation change recover immediately; the retry budget resets on changed inputs; an unchanged failure does not deploy before the window and does not write status across five repeated reconciles; the window elapsing deploys exactly once; a metadata-annotation-only change waits for the window and does not disturb the fingerprint; fingerprints are stable across 50 iterations of differently-ordered annotation maps; already-programmed and newer-generation behaviour are unchanged.

    Status-patch failures (internal/controller/apigateway_statuspatch_test.go, new): a client wrapper fails the status subresource write, then the real code path runs twice to prove the second reconcile retries — for the initial-condition patch, the Programmed=True patch, and the exhaustion patch. The
    exhaustion case runs the decision path twice after the failed write: inside the retry
    window it must not deploy yet still return a bounded RequeueAfter, and once the window
    has elapsed it must deploy exactly once.

    Five controller tests and all three status-patch tests were verified red against the
    previous code before the fix (status = "Deployed", want "Failed"; deploy attempts = 0, want 1 ×4; tracker still holds "Processing"; tracker still holds "Deployed";
    RequeueAfter = 0s and deploy attempts after the retry window = 0 with the
    equal-generation Failed branch disabled).

  • Integration tests

    Not run. make is not available in this environment and the operator's integration/e2e suites need a cluster, so no end-to-end Kind/Helm reproduction of the interrupted install was performed. The recovery paths are covered by the action-level dispatch tests and the controller tests above; the repository's own workflows remain the end-to-end check.

Verification performed, from kubernetes/gateway-operator:

Command Result
go build ./... passed
go vet ./... passed
go test ./... passed — 8 packages ok
go test $(go list ./... | grep -v /e2e) -coverprofile … passed (the make test go-test line)
go test -race ./internal/helm ./internal/controller passed
go test -count=1 ./internal/helm ./internal/controller ×3 passed, no flakes
gofmt -l / gofmt -d on the changed files clean
git diff --check clean

Go is not installed on the machine used, so every command above ran in a golang:1.26 container matching the module's toolchain.

Security checks

The only destructive operations are Helm purges, and both cases are guarded by planRelease: a revision-1 pending-install that never deployed, and an already-uninstalled release whose resources are gone and whose history alone remains. Any pending state without a safe recovery target returns an error instead. uninstalling is never resolved automatically.

Samples

N/A

Related PRs

Test environment

  • Go 1.26 (containerised; no host toolchain)
  • Docker 28.4.0 / Compose 2.39.2
  • Windows 11
  • helm.sh/helm/v3 v3.21.3 (behaviour verified directly against the module source)

Two independent defects each left an APIGateway permanently wedged.

Helm: InstallOrUpgrade chose install versus upgrade purely on whether
release history existed, and treated any history error as "absent". A
release interrupted mid-operation keeps history in a pending state, so
the operator kept calling upgrade and Helm kept refusing it. Read the
full history, decide from the latest revision's status, and recover:
purge and reinstall a revision-1 pending-install (nothing successful to
lose), roll back a pending-upgrade or pending-rollback to the newest
successful revision first, and report a clear error when no safe
revision exists rather than deleting anything. Revisions are compared
numerically, only a genuine not-found means absent, and install/upgrade
now run context-aware with cleanup on failure.

Reconciliation: exhausting the retry budget recorded the gateway as
Deployed while persisting Programmed=False with observedGeneration equal
to the CR generation. Neither the already-programmed branch nor the
newer-generation branch then applied, so reconciliation fell through to
"nothing to do" and only a generation bump could revive it. Record an
explicit Failed state, keep reporting the failure truthfully, and give
the gateway a bounded retry after the configured sync period. A change
to the referenced ConfigMap or to deployment-affecting annotations
retries immediately and resets the retry budget, while an unchanged
failure requeues without deploying or writing status so a status-driven
reconcile cannot hot-loop. Also removes an equal-generation check that
was unreachable inside the newer-generation branch.

Fixes wso2#3105
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The gateway controller now persists failed deployment state, fingerprints deployment inputs, and performs bounded recovery. The Helm client now plans recovery from release history, including pending-release rollback and purge-and-install paths, with context-aware execution.

Changes

Gateway deployment recovery

Layer / File(s) Summary
Deployment tracking and input fingerprints
kubernetes/gateway-operator/internal/controller/apigateway_controller.go, kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go
The controller stores deployment input hashes, injects deployment calls, resets retry counts when inputs change, and centralizes retry-window handling.
Failed gateway recovery decisions
kubernetes/gateway-operator/internal/controller/apigateway_controller.go, kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go
Equal-generation and newer-generation reconciliations recover failed or incomplete tracking states. Tests cover retry windows, changed inputs, restart recovery, and duplicate-deployment prevention.
Retry exhaustion and status-write recovery
kubernetes/gateway-operator/internal/controller/apigateway_controller.go, kubernetes/gateway-operator/internal/controller/apigateway_statuspatch_test.go
Exhausted retries persist Failed tracking state and DeploymentFailed status. Status-write failures clear tracking so later reconciliation can retry.

Helm release recovery

Layer / File(s) Summary
Helm recovery planning
kubernetes/gateway-operator/internal/helm/recovery.go, kubernetes/gateway-operator/internal/helm/recovery_test.go, kubernetes/gateway-operator/internal/helm/dispatch_test.go
Release history now produces install, upgrade, purge-and-install, or rollback-then-upgrade plans. Tests cover statuses, revisions, nil entries, errors, operation names, and rollback selection.
Helm client execution controls
kubernetes/gateway-operator/internal/helm/client.go, kubernetes/gateway-operator/internal/helm/dispatch_test.go
The client executes recovery plans, supports purge and rollback, uses context-aware commands, enables upgrade cleanup, and propagates operation failures.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant APIGateway
  participant GatewayController
  participant GatewayTracker
  participant HelmClient
  APIGateway->>GatewayController: trigger reconciliation
  GatewayController->>GatewayTracker: inspect generation, input hash, and retry state
  GatewayTracker-->>GatewayController: return recovery decision
  GatewayController->>HelmClient: deploy with input hash
  HelmClient-->>GatewayController: return deployment result
  GatewayController->>APIGateway: persist status and retry timing
Loading
sequenceDiagram
  participant HelmClient
  participant HelmHistory
  participant RecoveryPlanner
  participant HelmRelease
  HelmClient->>HelmHistory: read release history
  HelmHistory-->>RecoveryPlanner: return revisions and statuses
  RecoveryPlanner-->>HelmClient: return recovery operation
  HelmClient->>HelmRelease: execute install, upgrade, purge, or rollback
  HelmRelease-->>HelmClient: return operation result
Loading

Suggested reviewers: krishanx92, renuka-fernando, thushani-jayasekera

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #3105 by recovering pending Helm releases and making exhausted deployments recoverable through bounded retries and input changes.
Out of Scope Changes check ✅ Passed The code and tests remain within issue #3105; the related chart defect in issue #3104 is explicitly identified as out of scope.
Title check ✅ Passed The title clearly and concisely summarizes recovery for interrupted and exhausted Gateway Operator deployments.
Description check ✅ Passed The description covers the required sections, explains the changes, documents testing and security checks, and identifies integration tests as not run.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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

Actionable comments posted: 3

🧹 Nitpick comments (5)
kubernetes/gateway-operator/internal/helm/client.go (2)

240-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Reject a non-positive revision before running the rollback.

planRelease always supplies a positive rollbackRevision for operationRollbackThenUpgrade today. If that guarantee changes, action.Rollback treats Version zero as "roll back to the previous revision". The release would then move to a revision the planner never selected. A guard keeps the destructive step tied to the plan.

♻️ Proposed guard
 func (c *Client) rollback(ctx context.Context, actionConfig *action.Configuration, opts InstallOrUpgradeOptions, revision int) error {
 	log := log.FromContext(ctx)
 
+	if revision <= 0 {
+		return fmt.Errorf("refusing to roll back release %q to non-positive revision %d", opts.ReleaseName, revision)
+	}
+
 	client := action.NewRollback(actionConfig)
 	client.Version = revision
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/gateway-operator/internal/helm/client.go` around lines 240 - 249,
Update Client.rollback to validate revision before constructing or running the
rollback action, returning an error immediately when revision is non-positive.
Preserve the existing rollback configuration for positive revisions so the
operation remains tied to the planner-selected revision.

220-225: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Set a bounded default timeout for Helm Wait steps.

action.NewUninstall and action.NewRollback do not set a default Timeout. Helm’s wait logic uses the timeout passed in, so Wait=true and Timeout=0 can create a wait with no configured budget rather than the expected CLI-backed default. Use a shared fallback timeout in purge/rollback; the same fallback should apply to the install/upgrade path too.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/gateway-operator/internal/helm/client.go` around lines 220 - 225,
The Helm action flows in purge, rollback, and install/upgrade need a shared
bounded default timeout when waiting. Define or reuse one fallback timeout and
assign it to each action’s Timeout before applying the optional opts.Timeout
override, preserving explicit positive timeout values across
action.NewUninstall, action.NewRollback, and the install/upgrade action setup.
kubernetes/gateway-operator/internal/controller/apigateway_controller.go (2)

396-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated failed-recovery block.

Lines 399-413 repeat lines 338-353 exactly, including both log messages. Two copies of the same decision will drift when the recovery policy changes. Extract one helper that takes the log context and returns the result.

♻️ Proposed helper
// recoverFailed applies the bounded recovery decision for a failed tracking entry.
func (r *GatewayReconciler) recoverFailed(
	ctx context.Context,
	log *slog.Logger,
	gatewayConfig *apiv1.APIGateway,
	trackingKey string,
	crGeneration int64,
	entry *GatewayTrackingEntry,
	configHash, inputHash string,
) (ctrl.Result, error) {
	deployNow, requeueAfter, reason := r.decideFailedRecovery(entry, inputHash, time.Now())
	if !deployNow {
		log.Debug("Failed APIGateway awaiting its retry window",
			slog.String("name", gatewayConfig.Name),
			slog.Int64("generation", crGeneration),
			slog.Duration("retryIn", requeueAfter))
		return ctrl.Result{RequeueAfter: requeueAfter}, nil
	}
	log.Info("Recovering failed APIGateway deployment",
		slog.String("name", gatewayConfig.Name),
		slog.Int64("generation", crGeneration),
		slog.String("reason", reason))
	return r.deploy(ctx, gatewayConfig, trackingKey, crGeneration, configHash, inputHash)
}

Then both sites become:

 				if trackingEntry.Status == GatewayTrackingStatusFailed {
-					deployNow, requeueAfter, reason := r.decideFailedRecovery(trackingEntry, inputHash, time.Now())
-					if !deployNow {
-						log.Debug("Failed APIGateway awaiting its retry window",
-							slog.String("name", gatewayConfig.Name),
-							slog.Int64("generation", crGeneration),
-							slog.Duration("retryIn", requeueAfter))
-						return ctrl.Result{RequeueAfter: requeueAfter}, nil
-					}
-					log.Info("Recovering failed APIGateway deployment",
-						slog.String("name", gatewayConfig.Name),
-						slog.Int64("generation", crGeneration),
-						slog.String("reason", reason))
-					return r.deploy(ctx, gatewayConfig, trackingKey, crGeneration, currentConfigHash, inputHash)
+					return r.recoverFailed(ctx, log, gatewayConfig, trackingKey, crGeneration,
+						trackingEntry, currentConfigHash, inputHash)
 				}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/gateway-operator/internal/controller/apigateway_controller.go`
around lines 396 - 413, Extract the duplicated failed-recovery logic from the
branches around decideFailedRecovery into a shared
GatewayReconciler.recoverFailed helper, preserving the existing retry-window and
recovery log messages and deploy behavior. Pass the relevant context, logger,
gatewayConfig, tracking key, generation, config hash, input hash, and tracking
entry, then replace both inline blocks with calls to the helper.

750-755: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Apply a minimum floor to the retry window.

retryWindow returns SyncPeriod whenever it is positive. A small configured SyncPeriod, for example 1 second, makes an exhausted gateway run a Helm operation every second forever, because each recovery attempt re-exhausts the budget and re-arms the window. Clamp the window to a minimum so recovery stays bounded independently of the resync cadence.

♻️ Proposed floor
+const minRetryWindow = 1 * time.Minute
+
 func (r *GatewayReconciler) retryWindow() time.Duration {
 	if r.Config != nil && r.Config.Reconciliation.SyncPeriod > 0 {
-		return r.Config.Reconciliation.SyncPeriod
+		if r.Config.Reconciliation.SyncPeriod < minRetryWindow {
+			return minRetryWindow
+		}
+		return r.Config.Reconciliation.SyncPeriod
 	}
 	return 10 * time.Minute
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/gateway-operator/internal/controller/apigateway_controller.go`
around lines 750 - 755, Update GatewayReconciler.retryWindow to enforce a
minimum retry-window duration when Config.Reconciliation.SyncPeriod is
configured, returning the larger of the configured period and the established
floor. Preserve the existing default for missing or non-positive configuration,
so recovery cannot be re-armed at an excessively short cadence.
kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go (1)

478-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering the retry-budget wiring inside processGatewayDeployment.

The decision tests replace deployGateway, so the retryCountFor call in processGatewayDeployment (controller lines 490-499) never runs. retryCountFor is covered directly at lines 556-575, but the wiring that preserves an exhausted count across a recovery attempt is not. Without that coverage, a future change that drops InputHash from the new Processing entry would silently grant a fresh retry budget on every recovery, and every test here would still pass.

Add one test that calls processGatewayDeployment with a seeded exhausted entry and asserts the stored RetryCount and InputHash survive when the inputs are unchanged.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go`
around lines 478 - 492, Add a focused test for processGatewayDeployment that
seeds a processing tracking entry with an exhausted RetryCount and its existing
InputHash, then invokes the deployment with unchanged inputs and verifies the
resulting stored entry preserves both values. Use the existing recovery test
helpers and tracking symbols, ensuring the test exercises retryCountFor wiring
rather than the deployGateway-stubbed decision path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@kubernetes/gateway-operator/internal/helm/client.go`:
- Around line 337-339: Update the comment above client.CleanupOnFail to state
that it removes resources newly created by a failed upgrade, rather than rolling
back the release. In the same Helm client configuration, set client.Atomic =
true to restore the previous revision state when the upgrade fails.

In `@kubernetes/gateway-operator/internal/helm/recovery_test.go`:
- Around line 66-71: Update the “uninstalled installs” case in the recovery
tests to expect the purge operation instead of operationInstall, matching the
planned behavior of planRelease for an uninstalled release history.

In `@kubernetes/gateway-operator/internal/helm/recovery.go`:
- Around line 110-116: Plan uninstalled releases as operationPurgeThenInstall in
the StatusUninstalled branch of recovery.go, ensuring the held release name is
purged before installation. Update the corresponding “uninstalled installs” case
in recovery_test.go to expect the new recovery plan.

---

Nitpick comments:
In `@kubernetes/gateway-operator/internal/controller/apigateway_controller.go`:
- Around line 396-413: Extract the duplicated failed-recovery logic from the
branches around decideFailedRecovery into a shared
GatewayReconciler.recoverFailed helper, preserving the existing retry-window and
recovery log messages and deploy behavior. Pass the relevant context, logger,
gatewayConfig, tracking key, generation, config hash, input hash, and tracking
entry, then replace both inline blocks with calls to the helper.
- Around line 750-755: Update GatewayReconciler.retryWindow to enforce a minimum
retry-window duration when Config.Reconciliation.SyncPeriod is configured,
returning the larger of the configured period and the established floor.
Preserve the existing default for missing or non-positive configuration, so
recovery cannot be re-armed at an excessively short cadence.

In `@kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go`:
- Around line 478-492: Add a focused test for processGatewayDeployment that
seeds a processing tracking entry with an exhausted RetryCount and its existing
InputHash, then invokes the deployment with unchanged inputs and verifies the
resulting stored entry preserves both values. Use the existing recovery test
helpers and tracking symbols, ensuring the test exercises retryCountFor wiring
rather than the deployGateway-stubbed decision path.

In `@kubernetes/gateway-operator/internal/helm/client.go`:
- Around line 240-249: Update Client.rollback to validate revision before
constructing or running the rollback action, returning an error immediately when
revision is non-positive. Preserve the existing rollback configuration for
positive revisions so the operation remains tied to the planner-selected
revision.
- Around line 220-225: The Helm action flows in purge, rollback, and
install/upgrade need a shared bounded default timeout when waiting. Define or
reuse one fallback timeout and assign it to each action’s Timeout before
applying the optional opts.Timeout override, preserving explicit positive
timeout values across action.NewUninstall, action.NewRollback, and the
install/upgrade action setup.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9433164e-3055-419e-8f10-4b0d10f09ad7

📥 Commits

Reviewing files that changed from the base of the PR and between 9f7d4bd and 6325e7b.

📒 Files selected for processing (5)
  • kubernetes/gateway-operator/internal/controller/apigateway_controller.go
  • kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go
  • kubernetes/gateway-operator/internal/helm/client.go
  • kubernetes/gateway-operator/internal/helm/recovery.go
  • kubernetes/gateway-operator/internal/helm/recovery_test.go

Comment thread kubernetes/gateway-operator/internal/helm/client.go Outdated
Comment thread kubernetes/gateway-operator/internal/helm/recovery_test.go
Comment thread kubernetes/gateway-operator/internal/helm/recovery.go
An uninstalled release keeps its name in Helm's storage when history was
retained, so Helm's availableName check rejects a plain install with
"cannot re-use a name that is still in use" unless Install.Replace is
set, which this package does not set. Plan that state as
purge-then-install instead: the uninstall already removed the resources,
so dropping the retained history discards nothing live, and an
already-uninstalled release is purgeable because KeepHistory is false.

Also corrects the upgrade comment: CleanupOnFail deletes resources the
failed upgrade created, it does not roll the release back.

Related to wso2#3105
coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 4, 2026
Remove the History.Max handling. In helm.sh/helm/v3@v3.21.3
action.History.Run returns cfg.Releases.History(name) directly and never
reads Max, so setting it was a no-op and the comments describing it as a
truncation control were wrong. The real improvements stay: the returned
history is now actually used, revisions are compared numerically, and a
history error that is not "release not found" is propagated instead of
being read as an absent release.

Add a release executor seam so InstallOrUpgrade's dispatch is testable:
the planner alone cannot catch a correct plan that is dispatched wrongly,
which is how the uninstalled-release bug reached review. Purge and
rollback action construction is split out so KeepHistory=false and the
planned rollback revision are assertable without a cluster.

Drop the tracking entry when a status write fails. A Processing or
Deployed entry tells the next reconcile that another operation owns the
generation, but the reconcile that owned it has already returned; the
next one then skipped and returned nil, ending the requeue chain the
returned error had started. A failed initial-condition patch and a failed
Programmed=True patch both wedged the gateway this way.

Rename the spec.infrastructure annotation test to say what it covers, and
add the metadata-annotation case from the issue. Changing
metadata.annotations is not a deployment input and is deliberately not an
immediate retry trigger; recovery there is the bounded retry window.

Related to wso2#3105

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

🧹 Nitpick comments (1)
kubernetes/gateway-operator/internal/controller/apigateway_controller.go (1)

21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use an approved post-quantum hash for the deployment input fingerprint.

deploymentInputHash() still uses SHA-256; replace it with SHA-3 or BLAKE3 at a quantum-safe size while keeping InputHash deterministic for retry decisions.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/gateway-operator/internal/controller/apigateway_controller.go`
around lines 21 - 22, Update deploymentInputHash() to replace SHA-256 with an
approved SHA-3 or BLAKE3 hash configured at a quantum-safe output size, while
preserving deterministic InputHash generation for retry decisions and updating
the related imports.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@kubernetes/gateway-operator/internal/controller/apigateway_controller.go`:
- Around line 21-22: Update deploymentInputHash() to replace SHA-256 with an
approved SHA-3 or BLAKE3 hash configured at a quantum-safe output size, while
preserving deterministic InputHash generation for retry decisions and updating
the related imports.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1990cdb7-949b-447c-b6cb-b68f0f441eee

📥 Commits

Reviewing files that changed from the base of the PR and between 6325e7b and c6c86bf.

📒 Files selected for processing (7)
  • kubernetes/gateway-operator/internal/controller/apigateway_controller.go
  • kubernetes/gateway-operator/internal/controller/apigateway_recovery_test.go
  • kubernetes/gateway-operator/internal/controller/apigateway_statuspatch_test.go
  • kubernetes/gateway-operator/internal/helm/client.go
  • kubernetes/gateway-operator/internal/helm/dispatch_test.go
  • kubernetes/gateway-operator/internal/helm/recovery.go
  • kubernetes/gateway-operator/internal/helm/recovery_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • kubernetes/gateway-operator/internal/helm/recovery_test.go
  • kubernetes/gateway-operator/internal/helm/recovery.go
  • kubernetes/gateway-operator/internal/helm/client.go

coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 5, 2026
The exhaustion status-patch test configured a deployment spy but never
exercised it: it called decideFailedRecovery directly and silenced the
unused variable, so it did not prove the reconcile path recovers.

Run the real decision path twice after the failed status write instead.
Inside the retry window it must not deploy and must still return a bounded
RequeueAfter; once the window has elapsed it must deploy exactly once. The
tracker entry now carries the gateway's real input fingerprint so the
unchanged-input path is the one under test.

Also correct the GatewayTracker comment, which still claimed entries
persist until the CR is deleted.

Related to wso2#3105
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.

[Bug]: APIGateway is permanently wedged after retry exhaustion, and a sub-release stuck in pending-install is never recovered

1 participant