OCPCLOUD-3442: Migrate four controllers to per-controller condition reporting - #577
OCPCLOUD-3442: Migrate four controllers to per-controller condition reporting#577stefanonardo wants to merge 1 commit into
Conversation
|
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:
WalkthroughControllers now receive manager-provided dependencies and use standardized reconciliation results to write ClusterOperator status. Core, infrastructure, kubeconfig, and secret synchronization conditions are aggregated. Tests now validate sub-controller conditions and error states. ChangesController status reconciliation migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant ResultGenerator
participant WriteClusterOperatorStatus
participant ClusterOperator
Controller->>ResultGenerator: create reconciliation result
ResultGenerator->>WriteClusterOperatorStatus: provide conditions
WriteClusterOperatorStatus->>ClusterOperator: persist status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
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
583d1d8 to
cbd5749
Compare
| } | ||
|
|
||
| return ctrl.Result{}, nil | ||
| return ResultGenerator.Success() |
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.
There was a problem hiding this comment.
we don' watch it, I'll requeue it after 1 minute
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
|
/test unit |
181c66b to
8253aeb
Compare
8253aeb to
c21f4b8
Compare
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test unit |
|
/retest |
1 similar comment
|
/retest |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@mdbooth: This PR has been marked as verified by 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Scheduling tests matching the |
|
/retest |
|
/hold Revision c21f4b8 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
|
/retest |
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>
c21f4b8 to
ede5ec8
Compare
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
@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. |
|
@stefanonardo: The following test failed, say
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. |
|
/test e2e-metal3-capi-techpreview |
|
/retest |
3 participants
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 consumersSecretReference), report success instead of blocking the install🤖 Generated with Claude Code