-
Notifications
You must be signed in to change notification settings - Fork 217
OTA-1860: Stop blocking patch updates when cluster version overrides are set #1314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
OTA-1860: Stop blocking patch updates when cluster version overrides are set #1314
Conversation
|
@harche: This pull request references OTA-1860 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 "4.22.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:
WalkthroughClusterVersion override detection was changed to return a non-blocking precondition warning (new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harche 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 |
fd7fba1 to
05bcd44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/payload/precondition/clusterversion/upgradeable.go (1)
34-47:⚠️ Potential issue | 🔴 Critical
ClusterVersionOverridesConditiondoes not filter the exempted ClusterImagePolicy override.The function at lines 34-47 blocks upgrades on any
Unmanaged: trueoverride without exemption logic. Verification found noIsAllowedOverride()function or override exemption filtering anywhere in the codebase—neither inpkg/cvo/upgradeable.go,pkg/internal/constants.go, nor elsewhere. This means the exempted openshift ClusterImagePolicy override (groupconfig.openshift.io, kindClusterImagePolicy, nameopenshift) will incorrectly block minor/major version upgrades, despite PR objectives to exempt it.The precondition path (line 104) marks override warnings as
NonBlockingWarning: truefor patch updates, but for minor/major updates, the condition is not used. Instead, line 127 propagates the Upgradeable condition status directly, meaning blocking overrides will still prevent these upgrades. The exemption filtering needs to be implemented in this function to check the override's group, kind, and name before returning a blocking condition.
05bcd44 to
e8f2d9a
Compare
|
/retest ci/prow/verify-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cvo/upgradeable.go (1)
250-262:⚠️ Potential issue | 🟠 MajorBoth
clusterVersionOverridesUpgradeable.Check()and the preconditionClusterVersionOverridesCondition()lack filtering for allowed overrides.Per the commit message "Exempt openshift ClusterImagePolicy override from blocking upgrades," these methods should exempt the openshift
ClusterImagePolicyoverride from blocking upgrades. Currently, any unmanaged override blocks the upgrade in both code paths:
pkg/cvo/upgradeable.golines 250–262pkg/payload/precondition/clusterversion/upgradeable.golines 35–47Both implementations iterate through overrides and return a blocking condition if any has
Unmanaged=true. Add filtering logic to exempt the allowed override before checking the condition.
e8f2d9a to
662cb27
Compare
|
@harche: This pull request references OTA-1860 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 "4.22.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. |
fd02a73 to
b8f5a1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/payload/precondition/clusterversion/upgradeable.go (1)
34-47:⚠️ Potential issue | 🔴 Critical
IsAllowedOverridefiltering is not implemented.The PR objectives state "Added IsAllowedOverride() helper to exempt the openshift ClusterImagePolicy override" and "Filter allowed overrides in precondition and upgradeable checks." However:
- The
IsAllowedOverride()function does not exist in the codebaseClusterVersionOverridesCondition()iterates all overrides without filteringclusterVersionOverridesUpgradeable.Check()inpkg/cvo/upgradeable.gouses identical logic—no filtering appliedIf a cluster has only the exempted override, both implementations will still block upgrades, contrary to the stated PR objectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/payload/precondition/clusterversion/upgradeable.go (1)
34-47:⚠️ Potential issue | 🔴 Critical
IsAllowedOverridefiltering is not implemented.The PR objectives state "Added IsAllowedOverride() helper to exempt the openshift ClusterImagePolicy override" and "Filter allowed overrides in precondition and upgradeable checks." However:
- The
IsAllowedOverride()function does not exist in the codebaseClusterVersionOverridesCondition()iterates all overrides without filteringclusterVersionOverridesUpgradeable.Check()inpkg/cvo/upgradeable.gouses identical logic—no filtering appliedIf a cluster has only the exempted override, both implementations will still block upgrades, contrary to the stated PR objectives.
|
This is effectively reverting #364, although now that we have |
pkg/cvo/upgradeable.go
Outdated
|
|
||
| cond.Reason = "ClusterVersionOverridesSet" | ||
| cond.Message = "Disabling ownership via cluster version overrides prevents upgrades. Please remove overrides before continuing." | ||
| cond.Message = "Disabling ownership via cluster version overrides prevents upgrades between minor or major versions. Please remove overrides before continuing." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably need to qualify "Please remove overrides before continuing." to "Please remove overrides before requesting a minor or major version update.". Or just drop the sentence. Or something. Because with this pull, you no longer need to remove the overrides before continuing with a patch update.
Otherwise the current pull state looks good to me; thanks!
b8f5a1f to
dba8302
Compare
|
/test okd-scos-images |
…are set Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dba8302 to
6f8f984
Compare
|
@harche: The following tests 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. |
Summary
cv.spec.overrideshas any entry withunmanaged: true, CVO setsUpgradeable=Falseand blocks all upgrades, including z-stream/patch updates. This is overly restrictive — patch updates deliver security fixes and bug fixes that should not be blocked just because an override is set.Upgradeable=Falsewhen overrides are set — that behavior is unchanged.Behavior after this change
Upgradeable=FalseUpgradeable=FalseTest plan
Jira: https://issues.redhat.com/browse/OTA-1860
🤖 Generated with Claude Code