NO-JIRA: Re-enable tests for the recommend cmd if alertsByCVO - #31440
NO-JIRA: Re-enable tests for the recommend cmd if alertsByCVO#31440hongkailiu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. 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. |
openshift-ci
Bot
added
do-not-merge/hold
|
Skipping CI for Draft Pull Request. |
WalkthroughThe upgrade recommendation test now detects HyperShift and the ChangesUpgrade risk test behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (14 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: 2
🧹 Nitpick comments (1)
test/extended/cli/adm_upgrade/recommend.go (1)
280-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the helper comment rationale-focused.
Replace the behavior restatement and pinned implementation link with a short explanation of why HyperShift is excluded.
As per coding guidelines, Go comments should be minimal and explain why rather than what.
🤖 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 `@test/extended/cli/adm_upgrade/recommend.go` around lines 280 - 281, Update the comment for alertsEvaluatedByCVO to briefly explain why HyperShift or hosted clusters are excluded, removing the behavior restatement and pinned upstream implementation link.Source: Coding guidelines
🤖 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 `@test/extended/cli/adm_upgrade/recommend.go`:
- Around line 243-247: Implement the CVO-specific expectation in the alertsByCVO
branch of the recommendation check: define the expected output pattern for
CVO-evaluated accept risks and assert it there instead of allowing the shared
matchRegexp call to use the legacy pattern. Preserve the existing legacy regex
path when alertsByCVO is false.
- Around line 286-289: Update the FeatureGates().Get error handling to return
false, nil when apierrors.IsNotFound(err) indicates the cluster FeatureGate
resource is missing, while continuing to return false, err for all other errors.
---
Nitpick comments:
In `@test/extended/cli/adm_upgrade/recommend.go`:
- Around line 280-281: Update the comment for alertsEvaluatedByCVO to briefly
explain why HyperShift or hosted clusters are excluded, removing the behavior
restatement and pinned upstream implementation link.
🪄 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: 761a1860-2775-4386-a781-fd1df64ba976
📒 Files selected for processing (1)
test/extended/cli/adm_upgrade/recommend.go
| if alertsByCVO { | ||
| // TODO: define the new pattern for the implementation if alertsByCVO | ||
| } | ||
| err = matchRegexp(out, pattern) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("the actual output is \n%s", out)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement the CVO-enabled expectation.
The alertsByCVO branch is empty, so Line 246 still applies the legacy regex when CVO evaluates accept risks. Define and assert the CVO-specific output pattern in this branch.
🤖 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 `@test/extended/cli/adm_upgrade/recommend.go` around lines 243 - 247, Implement
the CVO-specific expectation in the alertsByCVO branch of the recommendation
check: define the expected output pattern for CVO-evaluated accept risks and
assert it there instead of allowing the shared matchRegexp call to use the
legacy pattern. Preserve the existing legacy regex path when alertsByCVO is
false.
| featureGate, err := client.ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect existing repository conventions for Kubernetes NotFound handling.
rg -n --type go -C2 'apierrors\.IsNotFound|FeatureGates\(\)\.Get\(' test/extended vendor/github.com/openshiftRepository: openshift/origin
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=test/extended/cli/adm_upgrade/recommend.go
echo "== relevant slice =="
sed -n '240,320p' "$file" | cat -n
echo
echo "== imports =="
sed -n '1,60p' "$file" | cat -n
echo
echo "== function references =="
rg -n 'alertsEvaluatedByCVO|FeatureGates\(\)\.Get|apierrors\.IsNotFound|FeatureGate' "$file" test/extended/cli/adm_upgrade -C 2Repository: openshift/origin
Length of output: 11157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact function and its call sites to understand intended fallback behavior.
rg -n 'func alertsEvaluatedByCVO|alertsEvaluatedByCVO\(' test/extended/cli/adm_upgrade -C 3Repository: openshift/origin
Length of output: 1498
🌐 Web query:
OpenShift cluster-version-operator cvo.go FeatureGateClusterUpdateAcceptRisks FeatureGates Get cluster not found disabled
💡 Result:
The FeatureGateClusterUpdateAcceptRisks is a feature gate introduced to the OpenShift Cluster Version Operator (CVO) to support the reconciliation of accepted risks during cluster updates [1][2][3]. This feature allows cluster administrators to specify accepted conditional update risks in the ClusterVersion CRD under spec.desiredUpdate.acceptRisks, provided the feature gate is enabled [1][3]. Regarding your observation of a "cluster not found" error related to feature gates in CVO: 1. Nature of the Error: In the context of the CVO and its feature gate processing, errors like 'Get "https://127.0.0.1:6443/apis/config.openshift.io/v1/featuregates/cluster": dial tcp 127.0.0.1:6443: connect: connection refused' often occur during operator startup [4]. These typically indicate that the CVO attempted to access the FeatureGate resource before the API server was fully responsive or before the informer cache had finished syncing [5][4][6]. 2. Resolution and Handling: Historically, the CVO has been updated to handle these transient failures more gracefully [7]. Recent architectural improvements have shifted FeatureGate retrieval to rely on informers rather than direct API polling [5]. If a "cluster not found" error occurs, it is frequently due to an incorrect initialization order where a 'Get' call on a lister is executed before the informer factory has been started and synchronized [6]. The fix generally involves ensuring the informer factory is fully prepared (started and synchronized) before attempting to retrieve the 'cluster' FeatureGate instance [5][6]. 3. Context for Development: If you are encountering this while modifying or debugging code in cvo.go or related packages, ensure that your logic respects the lifecycle of the controller's informer factory [6]. Using cached listers properly after ensuring cache synchronization is the standard pattern to avoid "not found" or "connection refused" errors [5][6].
Citations:
- 1: OTA-1546: Reconcile cv.spec.desiredUpdate.acceptRisks cluster-version-operator#1284
- 2: https://pkg.go.dev/github.com/openshift/api/features
- 3: [OTA-1545] Extend ClusterVersion for accepted risks api#2360
- 4: Bug 2050946: pkg/featurechangestopper: Seed queue to guard against incorrect startingTechPreviewState cluster-version-operator#736
- 5: OTA-1531: Rework error handling in FeatureGate processing cluster-version-operator#1206
- 6: openshift/cluster-version-operator@a040ae4
- 7: Bug 2029750: pkg/start: Log and continue when we fail to retrieve the feature gate cluster-version-operator#706
🌐 Web query:
site:github.com/openshift/cluster-version-operator FeatureGates().Get("cluster") FeatureGateClusterUpdateAcceptRisks NotFound
💡 Result:
The message "FeatureGates.Get("cluster") FeatureGateClusterUpdateAcceptRisks NotFound" refers to an interaction within the OpenShift Cluster Version Operator (CVO) [1][2]. This error (or diagnostic log) indicates that the CVO is attempting to retrieve the cluster-wide FeatureGate configuration, specifically checking for the presence or status of the FeatureGateClusterUpdateAcceptRisks feature [2][3]. Key technical context: 1. FeatureGate Interaction: The CVO uses the standard Kubernetes/OpenShift API to retrieve the "cluster" FeatureGate object [3][4]. If this object is not found (returned as a NotFound error by the API), the operator typically assumes a default state [3]. 2. Specific Feature Gate: FeatureGateClusterUpdateAcceptRisks is a feature introduced to allow the CVO to reconcile user-accepted risks during cluster updates [2]. 3. Expected Behavior: In the OpenShift architecture, if the FeatureGate object for "cluster" is not found, the operator defaults to standard behavior [3]. If you are encountering this as an error message or log entry, it generally signifies that the operator is operating under the assumption that the feature is disabled or not explicitly configured [2][3]. If you are seeing this during cluster operations, it usually indicates that the ClusterVersion Operator has successfully handled the absence of this feature gate configuration and is proceeding with default logic [3]. If the operator is failing to start or function correctly, verify the status of your FeatureGate configuration in the cluster [3].
Citations:
- 1: https://github.com/openshift/cluster-version-operator/blob/c3a28cdddd27027cd4188540f0a3ae04833e5268/OWNERS
- 2: OTA-1546: Reconcile cv.spec.desiredUpdate.acceptRisks cluster-version-operator#1284
- 3: openshift/cluster-version-operator@11afdc2
- 4: openshift/cluster-version-operator@c323882
Treat a missing FeatureGate/cluster as disabled. Return false, nil for apierrors.IsNotFound(err) so clusters without that resource don't fail BeforeAll; keep propagating other GET errors.
🤖 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 `@test/extended/cli/adm_upgrade/recommend.go` around lines 286 - 289, Update
the FeatureGates().Get error handling to return false, nil when
apierrors.IsNotFound(err) indicates the cluster FeatureGate resource is missing,
while continuing to return false, err for all other errors.
Source: Learnings
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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
It was disabled in openshift#31417.
23d8c3b to
f1b04d8
Compare
2 participants
It was disabled in #31417.
Requiring openshift/oc#2322
/hold
Summary by CodeRabbit
--versiontarget’s accept guidance outcome.