Skip tests temporarily for the new output of the recommend cmd#31417
Skip tests temporarily for the new output of the recommend cmd#31417hongkailiu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe upgrade recommendation tests now detect HyperShift and CVO alert evaluation during setup, use the cluster feature gate to determine evaluation mode, reuse stored state, and skip incompatible conditional recommendation assertions. ChangesUpgrade recommendation evaluation
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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
![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 `@test/extended/cli/adm_upgrade/recommend.go`:
- Around line 225-228: The conditional-recommendations BeforeAll must skip
immediately when alertsByCVO is true, before any cluster setup or command
execution. Add the single guard at the start of that BeforeAll, then remove both
late per-test skips after Output() at test/extended/cli/adm_upgrade/recommend.go
lines 225-228 and 256-260; both sites require removal only, with no separate
replacement.
🪄 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: 1652b610-e0c4-46fb-b83a-8446c3fcbf72
📒 Files selected for processing (1)
test/extended/cli/adm_upgrade/recommend.go
| if alertsByCVO { | ||
| g.Skip("Skip temporarily until the implementation lands") | ||
| // TODO: define the new pattern for the implementation | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Skip the entire conditional context before setup.
When alertsByCVO is true, the context has already modified cluster state, created a cluster-admin service account/binding, waited 16 seconds, and still invokes the command before skipping. Move one guard to the start of the conditional-recommendations BeforeAll, then remove both per-test guards.
test/extended/cli/adm_upgrade/recommend.go#L225-L228: remove the late skip afterOutput().test/extended/cli/adm_upgrade/recommend.go#L256-L260: remove the duplicate late skip afterOutput().
📍 Affects 1 file
test/extended/cli/adm_upgrade/recommend.go#L225-L228(this comment)test/extended/cli/adm_upgrade/recommend.go#L256-L260
🤖 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 225 - 228, The
conditional-recommendations BeforeAll must skip immediately when alertsByCVO is
true, before any cluster setup or command execution. Add the single guard at the
start of that BeforeAll, then remove both late per-test skips after Output() at
test/extended/cli/adm_upgrade/recommend.go lines 225-228 and 256-260; both sites
require removal only, with no separate replacement.
We are changing the output for the cmd when CVO handles the risks from alerts OTA-1814. It will break the tests [1] that expects the old output. So we skip the tests temporarily to allow the implementation to get in and plan to re-enable them afterwords for the new output. [1]. https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3/2079871952771616768
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 `@test/extended/cli/adm_upgrade/recommend.go`:
- Around line 283-285: Update alertsEvaluatedByCVO around the FeatureGates().Get
call so apierrors.IsNotFound(err) returns (false, nil), treating a missing
FeatureGate/cluster as disabled; continue returning false with the original
error for all other failures.
🪄 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: df3ebaa8-bda2-4665-9f9d-aeec5e126f0b
📒 Files selected for processing (1)
test/extended/cli/adm_upgrade/recommend.go
| featureGate, err := client.ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline test/extended/cli/adm_upgrade/recommend.go --view expanded || true
printf '\n== Relevant slice ==\n'
sed -n '250,320p' test/extended/cli/adm_upgrade/recommend.go
printf '\n== Search for FeatureGate handling in repo ==\n'
rg -n "FeatureGates\\(\\)\\.Get\\(|IsNotFound\\(|FeatureGate/cluster|default disabled|feature set" test/extended -S || trueRepository: openshift/origin
Length of output: 34001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== CVO reference check from the linked commit context ==\n'
python3 - <<'PY'
import urllib.request, json, ssl
url='https://raw.githubusercontent.com/openshift/cluster-version-operator/97a5bd23f73a649f1ca8d0364eca57318f3ed274/pkg/start/start.go'
print(urllib.request.urlopen(url, context=ssl.create_default_context(), timeout=20).read().decode())
PYRepository: openshift/origin
Length of output: 2498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Directly inspect adjacent helper logic ==\n'
sed -n '1,220p' test/extended/cli/adm_upgrade/recommend.goRepository: openshift/origin
Length of output: 9656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Minimal behavioral check: identify whether the helper still returns an error
# directly when FeatureGates().Get fails, or already special-cases NotFound.
python3 - <<'PY'
from pathlib import Path
text = Path('test/extended/cli/adm_upgrade/recommend.go').read_text()
needle = 'FeatureGates().Get(ctx, "cluster", metav1.GetOptions{})'
idx = text.find(needle)
print(text[idx-220:idx+260] if idx != -1 else 'needle not found')
PYRepository: openshift/origin
Length of output: 635
🌐 Web query:
openshift cluster-version-operator FeatureGate/cluster NotFound default feature set disabled pkg/start/start.go
💡 Result:
In OpenShift, the log message "Error getting featuregate value:... NotFound... default feature set" typically originates from the Cluster Version Operator (CVO) during its startup phase in pkg/start/start.go [1][2]. This message indicates that the CVO attempted to retrieve the cluster-scoped FeatureGate configuration object (named "cluster") from the API server and encountered an error [3][4][2]. Key aspects of this behavior include: 1. Handling of "NotFound" Errors: If the API server returns a NotFound error for the FeatureGate/cluster resource, the CVO interprets this to mean that no specific feature set has been configured [2]. In this case, it defaults to the "default" feature set, which corresponds to an empty string ("") [5][2]. This is the expected behavior in new or unconfigured clusters where the resource may not yet exist [4]. 2. Context and Evolution: - Early versions of the CVO would log this as a warning and continue, sometimes leading to issues where the operator would not correctly reconcile subsequent changes to feature gates [1][3][4]. - Subsequent updates improved the robustness of this process, moving from simple polling to using informers backed by caches to ensure the CVO has a consistent view of the cluster configuration [6][7]. - In later versions, persistent errors (other than NotFound) that prevent the CVO from retrieving the feature gate configuration are often treated as fatal, causing the operator to crash-loop until the API server is reachable and responsive, ensuring the cluster does not operate in an inconsistent state [2]. 3. Troubleshooting: If you see this message and the cluster is not functioning as expected, it generally points to a communication issue between the CVO and the Kubernetes API server (e.g., "connection refused" or network timeouts during early bootstrapping) [3][4][8]. If the error persists after the control plane is fully ready, it may indicate a deeper configuration issue with the config.openshift.io/v1 API group [4][6]. For developers or administrators debugging this, these logs are typically found in the cluster-version-operator pod within the openshift-cluster-version namespace [3][4]. Top references for this behavior can be found in the OpenShift Cluster Version Operator repository, particularly in files related to pkg/start and PRs addressing feature gate initialization (e.g., PR #736, #1206) [3][6][2].
Citations:
- 1: openshift/cluster-version-operator@90b1454
- 2: openshift/cluster-version-operator@11afdc2
- 3: Bug 2050946: pkg/featurechangestopper: Seed queue to guard against incorrect startingTechPreviewState cluster-version-operator#736
- 4: https://bugzilla.redhat.com/show_bug.cgi?id=2050946
- 5: https://github.com/openshift/api/blob/master/config/v1/types_feature.go
- 6: OTA-1531: Rework error handling in FeatureGate processing cluster-version-operator#1206
- 7: OTA-1531: [5/x] cvo: read cluster
FeatureGateearly cluster-version-operator#1190 - 8: https://bugzilla.redhat.com/show_bug.cgi?id=2053359
Handle a missing FeatureGate/cluster as disabled test/extended/cli/adm_upgrade/recommend.go:283-285 alertsEvaluatedByCVO currently returns an error on NotFound, which can fail BeforeAll on clusters using the default feature set. Return (false, nil) for apierrors.IsNotFound(err) and keep propagating other 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 283 - 285, Update
alertsEvaluatedByCVO around the FeatureGates().Get call so
apierrors.IsNotFound(err) returns (false, nil), treating a missing
FeatureGate/cluster as disabled; continue returning false with the original
error for all other failures.
|
@hongkailiu: This PR was included in a payload test run from openshift/oc#2322
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/31da0c00-8682-11f1-9066-6af507414644-0 |
|
Scheduling required tests: |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 openshift/oc#2322 |
|
@hongkailiu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e347f500-869c-11f1-8fbe-6e680a895c06-0 |
|
/retest-required |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@hongkailiu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7a344010-86a0-11f1-8e83-d5dc0cc61d83-0 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
My attempt to test with oc#2322 did not go far. Neither did Asking for help in forum (internal). So I step back with #31417 (comment) without oc#2322. The following shows that the skip from this pull works. $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/openshift-origin-31417-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3/2080294904646340608/artifacts/e2e-aws-ovn-techpreview-serial/openshift-e2e-test/artifacts/e2e.log | rg 'When the update service has conditional recommendations'
started: 0/16/33 "[Serial][sig-cli] oc adm upgrade recommend When the update service has conditional recommendations runs successfully when listing all updates [Suite:openshift/conformance/serial]"
skipped: (1.6s) 2026-07-23T16:47:03 "[Serial][sig-cli] oc adm upgrade recommend When the update service has conditional recommendations runs successfully when listing all updates [Suite:openshift/conformance/serial]"Comparing it to pull-ci-openshift-origin-main-e2e-aws-ovn-serial-2of2, the same case is not skipped as expected. $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/31417/pull-ci-openshift-origin-main-e2e-aws-ovn-serial-2of2/2080256831942299648/artifacts/e2e-aws-ovn-serial/openshift-e2e-test/artifacts/e2e.log | rg 'When the update service has conditional recommendations'
started: 0/4/46 "[Serial][sig-cli] oc adm upgrade recommend When the update service has conditional recommendations runs successfully when listing all updates [Suite:openshift/conformance/serial]"
passed: (20.5s) 2026-07-23T14:21:03 "[Serial][sig-cli] oc adm upgrade recommend When the update service has conditional recommendations runs successfully when listing all updates [Suite:openshift/conformance/serial]"/verified by @hongkailiu |
openshift-ci-robot
added
the
verified
|
@hongkailiu: 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. |
|
@hongkailiu: 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. |
2 participants
We are changing the output for the cmd when CVO handles the risks from alerts OTA-1814. It will break the tests [1] that expects the old output.
So we skip the tests temporarily to allow the implementation to get in and plan to re-enable them afterwords for the new output.
[1]. https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3/2079871952771616768
Summary by CodeRabbit