OCPCLOUD-2664: Update operatorstatus to write correct sub-Conditions#552
OCPCLOUD-2664: Update operatorstatus to write correct sub-Conditions#552mdbooth wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-2664 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. |
WalkthroughReplaced reconcile condition representations with a lightweight ChangesCondition state refactor & status write semantics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Updates conditions written by operatorstatus to be in line with their definitions in the clusterv1 package.
mdbooth
force-pushed
the
clusteroperator-conditions
branch
from
15e99cf to
a927a34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/operatorstatus/controller_status.go (1)
268-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't skip the status patch when only obsolete managed conditions need removal.
If an upgraded
ClusterOperatorstill has a legacy<Controller>Degradedcondition and the desiredProgressing/Availablevalues are otherwise unchanged,mergeConditionsreturnsfalseand Line 285 exits before patching. That leaves the stale condition behind indefinitely, so existing objects never fully converge to the new condition set.Please treat “managed condition present in
co.Status.Conditionsbut absent fromconditions” as an update, and add a regression test that seeds a legacy degraded condition.Possible fix
import ( "context" "errors" "fmt" + "strings" "time" @@ updated := mergeConditions(conditions, co.Status.Conditions) + if !updated { + desiredTypes := map[configv1.ClusterStatusConditionType]struct{}{} + for _, cond := range conditions { + desiredTypes[*cond.Type] = struct{}{} + } + + for i := range co.Status.Conditions { + condType := co.Status.Conditions[i].Type + if strings.HasPrefix(string(condType), string(r.ControllerResultGenerator)) { + if _, ok := desiredTypes[condType]; !ok { + updated = true + break + } + } + } + } if !updated { return nil }🤖 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/operatorstatus/controller_status.go` around lines 268 - 287, The current logic builds a desired conditions slice and calls mergeConditions, but mergeConditions returns false when desired conditions equal existing ones even if existing contains obsolete managed conditions (e.g., legacy <Controller>Degraded) that should be removed; update mergeConditions (or wrap it) so it treats a managed condition present in co.Status.Conditions but missing from the desired conditions slice as a change (i.e., mark updated=true and remove that condition), using the same identifying keys as r.condition/ConditionProgressingSuffix/ConditionAvailableSuffix and findClusterOperatorCondition to detect managed condition types; also add a regression test that seeds an existing ClusterOperator.Status.Conditions with a legacy degraded condition and asserts the controller patches it away when desired Progressing/Available are 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.
Outside diff comments:
In `@pkg/operatorstatus/controller_status.go`:
- Around line 268-287: The current logic builds a desired conditions slice and
calls mergeConditions, but mergeConditions returns false when desired conditions
equal existing ones even if existing contains obsolete managed conditions (e.g.,
legacy <Controller>Degraded) that should be removed; update mergeConditions (or
wrap it) so it treats a managed condition present in co.Status.Conditions but
missing from the desired conditions slice as a change (i.e., mark updated=true
and remove that condition), using the same identifying keys as
r.condition/ConditionProgressingSuffix/ConditionAvailableSuffix and
findClusterOperatorCondition to detect managed condition types; also add a
regression test that seeds an existing ClusterOperator.Status.Conditions with a
legacy degraded condition and asserts the controller patches it away when
desired Progressing/Available are unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 598d4b85-2dec-4cbd-ba56-1ba518e4242f
⛔ Files ignored due to path filters (2)
vendor/github.com/go-logr/logr/testr/testr.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
pkg/controllers/installer/helpers_test.gopkg/controllers/installer/installer_controller_test.gopkg/controllers/revision/revision_controller_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operatorstatus/controller_status_test.go
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks, mostly LGTM, a couple of nits, a couple of Qs.
| // a ReconcileResult must always have an explicit progressing condition | ||
| progressing partialCondition | ||
|
|
||
| // the available condition is optional, and will be maintained from the | ||
| // current state if not set explicitly | ||
| available *partialCondition | ||
|
|
There was a problem hiding this comment.
So are we planning not to set degraded ever at all?
There was a problem hiding this comment.
Yes, in the aggregator. I'm thinking: any Progressing condition (error or not) which persists for more than duration X, where X is the colour of your bike shed.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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
|
/override e2e-openstack-ovn-techpreview Permafailing |
|
@mdbooth: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
/override pull-ci-openshift-cluster-capi-operator-main-e2e-openstack-ovn-techpreview |
|
@mdbooth: Overrode contexts on behalf of mdbooth: ci/prow/e2e-openstack-ovn-techpreview 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-gcp-ovn-techpreview Unrelated issue. |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-gcp-ovn-techpreview 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 kubernetes-sigs/prow repository. |
|
@mdbooth: 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. |
3 participants
Summary by CodeRabbit
Refactor
Bug Fixes
Tests