You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Sort incompatible CSVs in the Upgradeable condition message - #3952
skews.String() sorts a copy of the skews before it builds the Upgradeable condition message of the operator-lifecycle-manager ClusterOperator: skews with a property error first, then by namespace and name. A regression test feeds three skews in three orders and expects one message.
Motivation for the change:
incompatibleOperators lists the CSVs from the informer cache, which returns them in map order. The old String() kept that order, so the same set of blocking CSVs produced different messages between reconciles. conditionsEqual compares the message, so each reorder counted as a transition: the controller rewrote the ClusterOperator status and moved lastTransitionTime although nothing had changed. With two blocking CSVs on OCP 4.22 the message alternated between the two orders every one to two minutes.
TestSkewsStringIsDeterministic fails on the previous implementation, which filled the non-error entries from the end of the slice, and passes with the change.
make lint and make unit pass locally.
Reproduced and verified on an OCP 4.22.20 cluster with two CSVs that carry olm.maxOpenShiftVersion: "4.22". Before the change, 142 samples of the condition over 300 s showed two distinct messages and six distinct lastTransitionTime values. With the patched olm-operator, 141 samples over 300 s showed one message and one lastTransitionTime. Removing the CSVs set Upgradeable back to True.
The OLM v0 extended suite olmv0/extended/candidate/function ran against the patched operator on the same cluster: 203 specs, 163 passed, 39 skipped (topology filters), 1 failed. The failed spec (opm supports creating a catalog using semver veneer) exercises the opm CLI and died with exit status 127 because jq is missing in the test runner. It is unrelated to this change.
Reviewer Checklist
Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Bug fixes are accompanied by regression test(s)
e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
Docs updated or added to /doc
Commit messages sensible and descriptive
Tests marked as [FLAKE] are truly flaky and have an issue
Diagnostic summaries now display entries with errors first, followed by other entries sorted by namespace and name. The ordering remains consistent even when the entries are provided in a different order, making the summary easier to scan and compare across runs.
incompatibleOperators lists the CSVs from the informer cache. The cache
returns them in map order, which changes between reconciles.
skews.String() kept that order, so the same set of blocking CSVs
produced different messages. conditionsEqual compares the message, so
each reorder counted as a condition transition: the controller rewrote
the ClusterOperator status and moved lastTransitionTime although
nothing had changed. With two blocking CSVs on OCP 4.22 the message
alternated between the two orders every one to two minutes.
skews.String() now sorts a copy of the skews before it builds the
message: skews with a property error first, as before, then by
namespace and name. The same set of CSVs gives the same message in any
input order, so the condition only transitions when the set changes.
The regression test feeds three skews in three orders and expects one
message. It fails on the previous implementation, which filled the
non-error entries from the end of the slice.
Fixes OCPBUGS-129618
Signed-off-by: Harald Klein <hklein@redhat.com>
Co-Authored-By: Claude <noreply@anthropic.com>
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign grokspawn for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Details
Needs approval from an approver in each of these files:
I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.
Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
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.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Walkthrough
skews.String now formats entries in a deterministic order. The function sorts a copy of the entries, and a test checks that three input orderings produce the same message.
Changes: Skew formatting
Layer / File(s)
Summary
Sort skews before formatting pkg/controller/operators/openshift/helpers.go, pkg/controller/operators/openshift/helpers_test.go
skews.String sorts a copy of the entries, placing entries with errors first and sorting by namespace and name. A test checks that three permutations produce the same output.
No merge-blocking risk was identified in this deterministic formatting change.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: sorting incompatible CSVs in the Upgradeable condition message.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files.
Finishing Touches: 💡 1
🧪 Generate unit tests (beta)
Create a new PR
🛠️ Fix failing CI checks: 💡
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
golangci-lint (2.14.0)
golangci-lint (2.14.0) skipped for .golangci.yaml: configuration requires version: "2". Migrate older configurations using golangci-lint migrate and review the result; see https://golangci-lint.run/docs/product/migration-guide/.
Comment @coderabbitai help to get the list of available commands.
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
needs-ok-to-testIndicates a PR that requires an org member to verify it is safe to test.
1 participant
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.
Description of the change:
skews.String()sorts a copy of the skews before it builds theUpgradeablecondition message of theoperator-lifecycle-managerClusterOperator: skews with a property error first, then by namespace and name. A regression test feeds three skews in three orders and expects one message.Motivation for the change:
incompatibleOperatorslists the CSVs from the informer cache, which returns them in map order. The oldString()kept that order, so the same set of blocking CSVs produced different messages between reconciles.conditionsEqualcompares the message, so each reorder counted as a transition: the controller rewrote the ClusterOperator status and movedlastTransitionTimealthough nothing had changed. With two blocking CSVs on OCP 4.22 the message alternated between the two orders every one to two minutes.Fixes https://issues.redhat.com/browse/OCPBUGS-129618
Architectural changes:
None.
Testing remarks:
TestSkewsStringIsDeterministicfails on the previous implementation, which filled the non-error entries from the end of the slice, and passes with the change.make lintandmake unitpass locally.olm.maxOpenShiftVersion: "4.22". Before the change, 142 samples of the condition over 300 s showed two distinct messages and six distinctlastTransitionTimevalues. With the patched olm-operator, 141 samples over 300 s showed one message and onelastTransitionTime. Removing the CSVs setUpgradeableback toTrue.olmv0/extended/candidate/functionran against the patched operator on the same cluster: 203 specs, 163 passed, 39 skipped (topology filters), 1 failed. The failed spec (opm supports creating a catalog using semver veneer) exercises theopmCLI and died with exit status 127 becausejqis missing in the test runner. It is unrelated to this change.Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue🤖 Generated with Claude Code
Summary by CodeRabbit