OCPCLOUD-3442: Migrate four controllers to per-controller condition reporting#577
Open
stefanonardo wants to merge 5 commits into
Open
OCPCLOUD-3442: Migrate four controllers to per-controller condition reporting#577stefanonardo wants to merge 5 commits into
stefanonardo wants to merge 5 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@stefanonardo: This pull request references OCPCLOUD-3442 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (26)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (20)
WalkthroughThis PR updates controller status handling to use shared reconcile results and generated condition reasons, rewires controller startup to pass direct dependencies, and adds status/version update support for ClusterOperator reconciliation. It also adds generate tooling and updates tests for the new status model. ChangesClusterOperator status refactor
🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/controllers/kubeconfig/kubeconfig_test.go`:
- Around line 159-160: The test mutates the server-managed field by calling
tokenSecret.SetCreationTimestamp(...) and then cl.Update(ctx, tokenSecret);
remove the SetCreationTimestamp call and only modify fields that are
client-editable (e.g., annotations) so the test updates tokenSecret via
cl.Update(...) without touching metadata.creationTimestamp; locate the
tokenSecret variable and the SetCreationTimestamp invocation in
kubeconfig_test.go and delete that mutation, ensuring subsequent cl.Update(ctx,
tokenSecret) only changes annotations or other writable fields.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c0c49503-59cf-4e6c-b87f-ccc4dd58187f
⛔ Files ignored due to path filters (2)
vendor/golang.org/x/tools/cmd/stringer/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (26)
Makefilecmd/capi-controllers/main.gocmd/capi-operator/main.gogo.modpkg/commoncmdoptions/commonoptions.gopkg/controllers/clusteroperator/clusteroperator_controller.gopkg/controllers/clusteroperator/clusteroperator_controller_test.gopkg/controllers/clusteroperator/suite_test.gopkg/controllers/common_consts.gopkg/controllers/corecluster/corecluster_controller.gopkg/controllers/corecluster/corecluster_controller_test.gopkg/controllers/infracluster/infracluster_controller.gopkg/controllers/infracluster/infracluster_controller_test.gopkg/controllers/installer/installer_controller.gopkg/controllers/kubeconfig/kubeconfig.gopkg/controllers/kubeconfig/kubeconfig_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/secretsync/secret_sync_controller.gopkg/controllers/secretsync/secret_sync_controller_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/operatorstatus/reason_string.gopkg/operatorstatus/watch_predicates.gopkg/test/conditions.go
💤 Files with no reviewable changes (3)
- pkg/controllers/common_consts.go
- pkg/operatorstatus/operator_status.go
- pkg/commoncmdoptions/commonoptions.go
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
stefanonardo
force-pushed
the
OCPCLOUD-3442
branch
from
eae2e85 to
583d1d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controllers/clusteroperator/clusteroperator_controller_test.go (1)
94-343: ⚡ Quick winUse
should ...phrasing for table entry names.Most updated
Entry(...)descriptions are"when ..."; switch them to"should ... when ..."to match the test naming convention used in this repo.As per coding guidelines: Use descriptive test names in 'should...' format in Ginkgo tests (e.g., 'should do nothing').
🤖 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 `@pkg/controllers/clusteroperator/clusteroperator_controller_test.go` around lines 94 - 343, The table-driven test Entries in clusteroperator_controller_test.go use "when ..." phrasing; update each Entry(...) description to the repository's "should ... when ..." convention (e.g., change "when all sub-controllers report success" to "should report success when all sub-controllers report success") so all Entry(...) strings in the DescribeTable/Entry block (look for the long DescribeTable block containing Entry("when all sub-controllers report success", ..., defaultNodeTimeout) and subsequent Entry calls) follow the "should ..." naming pattern while keeping the rest of each Entry invocation unchanged.
🤖 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 `@pkg/controllers/clusteroperator/clusteroperator_controller_test.go`:
- Around line 184-187: The test claims to exercise the "missing sub-conditions"
path but instead sets InstallerControllerAvailable/Progressing to Unknown;
replace the explicit subCondition entries with a helper that removes those types
so the conditions are actually absent. Add the helper function
withoutConditionTypes(base
[]*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, types
...string) []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration (as
shown in the comment) and change the test invocation from
withOverrides(allSubControllersSuccessful(), subCondition(...),
subCondition(...)) to
withOverrides(withoutConditionTypes(allSubControllersSuccessful(),
"InstallerControllerAvailable", "InstallerControllerProgressing")) so
getSubcontrollerCondition is exercised for absent conditions.
---
Nitpick comments:
In `@pkg/controllers/clusteroperator/clusteroperator_controller_test.go`:
- Around line 94-343: The table-driven test Entries in
clusteroperator_controller_test.go use "when ..." phrasing; update each
Entry(...) description to the repository's "should ... when ..." convention
(e.g., change "when all sub-controllers report success" to "should report
success when all sub-controllers report success") so all Entry(...) strings in
the DescribeTable/Entry block (look for the long DescribeTable block containing
Entry("when all sub-controllers report success", ..., defaultNodeTimeout) and
subsequent Entry calls) follow the "should ..." naming pattern while keeping the
rest of each Entry invocation unchanged.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a7aeda83-7ff2-4d11-acaa-2f829b7ae886
📒 Files selected for processing (13)
cmd/capi-controllers/main.gopkg/commoncmdoptions/commonoptions.gopkg/controllers/clusteroperator/clusteroperator_controller.gopkg/controllers/clusteroperator/clusteroperator_controller_test.gopkg/controllers/corecluster/corecluster_controller.gopkg/controllers/corecluster/corecluster_controller_test.gopkg/controllers/infracluster/infracluster_controller.gopkg/controllers/infracluster/infracluster_controller_test.gopkg/controllers/kubeconfig/kubeconfig.gopkg/controllers/kubeconfig/kubeconfig_test.gopkg/controllers/secretsync/secret_sync_controller.gopkg/controllers/secretsync/secret_sync_controller_test.gopkg/operatorstatus/operator_status.go
💤 Files with no reviewable changes (2)
- pkg/commoncmdoptions/commonoptions.go
- pkg/operatorstatus/operator_status.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/capi-controllers/main.go
- pkg/controllers/infracluster/infracluster_controller_test.go
- pkg/controllers/kubeconfig/kubeconfig_test.go
- pkg/controllers/secretsync/secret_sync_controller_test.go
- pkg/controllers/clusteroperator/clusteroperator_controller.go
- pkg/controllers/infracluster/infracluster_controller.go
- pkg/controllers/corecluster/corecluster_controller_test.go
- pkg/controllers/secretsync/secret_sync_controller.go
stefanonardo
force-pushed
the
OCPCLOUD-3442
branch
from
583d1d8 to
cbd5749
Compare
mdbooth
reviewed
Jun 4, 2026
| } | ||
|
|
||
| return ctrl.Result{}, nil | ||
| return ResultGenerator.Success() |
Contributor
There was a problem hiding this comment.
This should probably be WaitingOnExternal.
You should also check if this controller watches the Infrastructure object (check in SetupWithManager). If it doesn't we'll either need to watch it or also add a RequeueAfter here. Polling should be fine in this case if we don't already have a watch: we don't really expect this to happen.
Author
There was a problem hiding this comment.
we don' watch it, I'll requeue it after 1 minute
stefanonardo
force-pushed
the
OCPCLOUD-3442
branch
from
cbd5749 to
181c66b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/kubeconfig/kubeconfig.go (1)
147-162:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn a waiting result after clearing the token secret.
This branch deliberately empties the token data so another controller can repopulate it. Returning
Success()here briefly reportsAvailable=True/Progressing=Falsebefore the next reconcile flips back toWaitingOnExternal, which makes the ClusterOperator condition false-green during refresh.Suggested fix
if err := r.Update(ctx, tokenSecret); err != nil { return ResultGenerator.Error(fmt.Errorf("unable to clear token secret data: %w", err)) } - return ResultGenerator.Success() + return ResultGenerator.WaitingOnExternal("token secret population").WithRequeueAfter(1 * 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 `@pkg/controllers/kubeconfig/kubeconfig.go` around lines 147 - 162, The code clears tokenSecret.Data and sets tokenRefreshAnnotationKey then returns ResultGenerator.Success(), which briefly marks the operator Available=true; instead return a waiting result so the operator reflects that it is waiting for the external token controller to repopulate the secret: after calling r.Update(ctx, tokenSecret) replace the Success() return with ResultGenerator.WaitingOnExternal() (or the appropriate Waiting result used elsewhere) so reconcile reports a waiting state until the token is restored; update references around tokenSecret, tokenRefreshAnnotationKey, r.Update, and ResultGenerator accordingly.
🤖 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.
Outside diff comments:
In `@pkg/controllers/kubeconfig/kubeconfig.go`:
- Around line 147-162: The code clears tokenSecret.Data and sets
tokenRefreshAnnotationKey then returns ResultGenerator.Success(), which briefly
marks the operator Available=true; instead return a waiting result so the
operator reflects that it is waiting for the external token controller to
repopulate the secret: after calling r.Update(ctx, tokenSecret) replace the
Success() return with ResultGenerator.WaitingOnExternal() (or the appropriate
Waiting result used elsewhere) so reconcile reports a waiting state until the
token is restored; update references around tokenSecret,
tokenRefreshAnnotationKey, r.Update, and ResultGenerator accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 265a4a4f-618d-4950-b037-566ee03e1f5b
📒 Files selected for processing (13)
cmd/capi-controllers/main.gopkg/commoncmdoptions/commonoptions.gopkg/controllers/clusteroperator/clusteroperator_controller.gopkg/controllers/clusteroperator/clusteroperator_controller_test.gopkg/controllers/corecluster/corecluster_controller.gopkg/controllers/corecluster/corecluster_controller_test.gopkg/controllers/infracluster/infracluster_controller.gopkg/controllers/infracluster/infracluster_controller_test.gopkg/controllers/kubeconfig/kubeconfig.gopkg/controllers/kubeconfig/kubeconfig_test.gopkg/controllers/secretsync/secret_sync_controller.gopkg/controllers/secretsync/secret_sync_controller_test.gopkg/operatorstatus/operator_status.go
💤 Files with no reviewable changes (2)
- pkg/operatorstatus/operator_status.go
- pkg/commoncmdoptions/commonoptions.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/controllers/secretsync/secret_sync_controller_test.go
- cmd/capi-controllers/main.go
- pkg/controllers/kubeconfig/kubeconfig_test.go
- pkg/controllers/corecluster/corecluster_controller_test.go
- pkg/controllers/infracluster/infracluster_controller.go
- pkg/controllers/infracluster/infracluster_controller_test.go
- pkg/controllers/clusteroperator/clusteroperator_controller_test.go
- pkg/controllers/secretsync/secret_sync_controller.go
- pkg/controllers/clusteroperator/clusteroperator_controller.go
- pkg/controllers/corecluster/corecluster_controller.go
Author
|
/test unit |
Add WithUpdateOperatorVersion() to ReconcileResult, allowing controllers to optionally update the operator version in the ClusterOperator status when writing their conditions. Also switches controller_status tests from fake client to envtest for accurate SSA field ownership testing.
Convert Reason from string constants to an ordered iota type, enabling severity-based comparison for condition aggregation. Replaces ReasonSyncFailed with the standardised ReasonEphemeralError.
ClusterOperator version is now written by the revision controller instead of the clusteroperator controller.
Rewrite ClusterOperatorController to aggregate per-controller sub-conditions (Available/Progressing) into top-level ClusterOperator conditions.
Migrate CoreCluster, InfraCluster, Kubeconfig, and SecretSync controllers from ClusterOperatorStatusClient (merge-patch, shared top-level conditions) to ControllerResultGenerator (server-side apply, per-controller prefixed conditions). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stefanonardo
force-pushed
the
OCPCLOUD-3442
branch
from
181c66b to
8253aeb
Compare
Contributor
|
@stefanonardo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ClusterOperatorStatusClient(merge-patch, shared top-level conditions) toControllerResultGenerator(server-side apply, per-controller prefixed conditions)AvailableandProgressingconditions on the ClusterOperator object, which are aggregated into top-level conditions by the ClusterOperator controllerClusterOperatorStatusClientandGetClusterOperatorStatusClient— no remaining consumersDepends on #574
Test plan
make buildpassesmake lintreports 0 issues🤖 Generated with Claude Code
Summary by CodeRabbit