Skip to content

fix(controllers): requeue on gate conditions instead of relying on watch events - #129

Merged
openshift-merge-bot[bot] merged 3 commits into
openshift-online:mainfrom
cristianoveiga:fix/nodepool-gate-requeue
Aug 18, 2026
Merged

fix(controllers): requeue on gate conditions instead of relying on watch events#129
openshift-merge-bot[bot] merged 3 commits into
openshift-online:mainfrom
cristianoveiga:fix/nodepool-gate-requeue

Conversation

@cristianoveiga

@cristianoveiga cristianoveiga commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • HC and nodepool controller gates (placement not ready, HC not available, VR not ready, version mismatch) returned reconcile.Result{} with no requeue, relying entirely on watch events to re-trigger reconciliation
  • If no watch event fired (e.g. no update to the cluster object), the controller would stall indefinitely
  • All gates now return RequeueAfter: 15s so controllers periodically re-check gate conditions

Test plan

  • All existing HC controller tests updated and passing
  • All existing nodepool controller tests updated and passing
  • Verify in dev environment that stuck nodepools recover within 15s

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved recovery when cluster placement, availability, or release version information is not yet ready.
    • Systems now retry reconciliation every 15 seconds when required information is missing or versions do not match.
    • Conflict-handling scenarios also retry automatically, reducing the need to wait for unrelated updates.
    • Status reporting and existing error handling remain unchanged.

cristianoveiga and others added 2 commits August 18, 2026 12:29
… events

Gates (placement not ready, HC not available, VR not ready, version
mismatch) returned reconcile.Result{} with no requeue, relying entirely
on watch events to re-trigger reconciliation. If no watch event fired
(e.g. no update to the cluster object), the nodepool would stall
indefinitely. Return RequeueAfter: 15s so the controller periodically
re-checks gate conditions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Same issue as the nodepool controller: placement, VR, and version
mismatch gates returned with no requeue, relying entirely on watch
events. If no watch event fired, the cluster would stall indefinitely.
Return RequeueAfter: 15s so the controller periodically re-checks.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 18, 2026
@openshift-ci

openshift-ci Bot commented Aug 18, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2026
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 81d5f813-40ba-4131-9b1b-d87e9f19d43d

📥 Commits

Reviewing files that changed from the base of the PR and between d749122 and dd4e783.

📒 Files selected for processing (2)
  • controllers/hc/hc_controller.go
  • controllers/nodepool/nodepool_controller.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • controllers/nodepool/nodepool_controller.go
  • controllers/hc/hc_controller.go

Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review.


Walkthrough

HostedCluster and NodePool reconciliation now explicitly requeue after 15 seconds when placement, availability, or version prerequisites are not ready. Tests now expect these requeue results, including status-update conflict cases.

Changes

Reconciliation requeues

Layer / File(s) Summary
Controller requeue logic
controllers/hc/hc_controller.go, controllers/nodepool/nodepool_controller.go
HostedCluster and NodePool reconciliation return timed requeues for unavailable placement, HostedCluster readiness, unresolved versions, and version mismatches.
Requeue test coverage
controllers/hc/hc_controller_test.go, controllers/nodepool/nodepool_controller_test.go
Tests expect timed requeues for pending prerequisite checks and status-update conflict paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to dd4e7

The controllers now periodically recheck gate conditions instead of relying only on watch events; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: patjlm, acwalczyk

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Ai-Attribution ⚠️ Warning AI use is explicit in the PR and commits; all three changed commits use Co-Authored-By: Claude Opus 4.6, with no Assisted-by or Generated-by trailer. Replace the AI Co-Authored-By trailers with the required Red Hat Assisted-by or Generated-by attribution trailers.
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: timed requeues for controller gate conditions instead of relying on watch events.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto ✅ Passed The PR changes requeue behavior, logs, tests, and deletion plumbing; added-line and AST scans found no weak crypto, custom crypto, or secret/token comparisons.
Container-Privileges ✅ Passed The PR changes only HC and NodePool Go controllers. No container or Kubernetes manifest privilege settings were added or changed; existing settings use runAsNonRoot and allowPrivilegeEscalation: fa...
No-Sensitive-Data-In-Logs ✅ Passed The PR changes existing gate log wording and timing only; it preserves pre-existing nodepool IDs and release versions and adds no passwords, tokens, API keys, PII, hostnames, or customer data.
No-Hardcoded-Secrets ✅ Passed The PR diff adds only requeue logs/results and test expectations; no secret-like assignments, credentials, private keys, embedded credentials, or long base64 strings appear in added lines.
No-Injection-Vectors ✅ Passed The PR changes only controller requeue/log behavior and test expectations; changed lines introduce no SQL concatenation, shell/eval/exec, unsafe YAML/pickle, os.system, or HTML injection vectors.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@cristianoveiga
cristianoveiga marked this pull request as ready for review August 18, 2026 16:39
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 18, 2026
@openshift-ci
openshift-ci Bot requested review from acwalczyk and patjlm August 18, 2026 16:39
Log "requeueing after 15s" only after the status update succeeds,
not before — avoids misleading log entries when the status update
returns an error and we don't actually requeue.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2026
@openshift-ci

openshift-ci Bot commented Aug 18, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristianoveiga, patjlm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [cristianoveiga,patjlm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot
openshift-merge-bot Bot merged commit 46b00f1 into openshift-online:main Aug 18, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants