Skip to content

Revert "TRT-2869: Revert "NO-JIRA: Re-enable tests for the recommend cmd if alertsByCVO"" - #31462

Open
hongkailiu wants to merge 2 commits into
openshift:mainfrom
hongkailiu:TRT-2869
Open

Revert "TRT-2869: Revert "NO-JIRA: Re-enable tests for the recommend cmd if alertsByCVO""#31462
hongkailiu wants to merge 2 commits into
openshift:mainfrom
hongkailiu:TRT-2869

Conversation

@hongkailiu

@hongkailiu hongkailiu commented Jul 31, 2026

Copy link
Copy Markdown
Member

The pull reverts #31460 + an error message adjustment for openshift/oc#2336 which fixed the error which was expected to see in #31440 but failed.

Summary by CodeRabbit

  • Tests
    • Updated upgrade recommendation tests to account for different alert evaluation outcomes.
    • Added coverage verifying guidance to use the upgrade acceptance command when version targets require it.
    • Improved diagnostics by displaying command output when an expected error is not returned.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 31, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 31, 2026

Copy link
Copy Markdown

@hongkailiu: This pull request references TRT-2869 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 bug to target the "5.0.0" version, but no target version was set.

Details

In response to this:

/hold

rebase after #31460 gets in.

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 openshift-ci Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 31, 2026
@openshift-ci

openshift-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@openshift-ci

openshift-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2026
@hongkailiu

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3

@openshift-ci

openshift-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7464d050-8cd6-11f1-89bd-bf4e4b48806f-0

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Walkthrough

The conditional recommendation tests now execute when CVO evaluates alerts. They use alternate listing output, require oc adm upgrade accept guidance for version-target errors, and include captured output in failed assertions.

Changes

Conditional recommendation test behavior

Layer / File(s) Summary
CVO conditional assertions
test/extended/cli/adm_upgrade/recommend.go
The tests remove the temporary skip, select output patterns by alertsByCVO, check oc adm upgrade accept guidance for CVO version-target errors, and keep successful assertions for non-CVO cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error The new Gomega failure message logs raw combined stdout/stderr; oc adm upgrade recommend output includes the update-service URL built from an internal ClusterIP. Do not log raw command output. Log a fixed diagnostic or redact URLs, hostnames, credentials, and other command data before adding failure context.
✅ Passed checks (14 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All eight Ginkgo titles in recommend.go are static string literals; dynamic versions, tokens, namespaces, and output appear only in test bodies or diagnostics.
Test Structure And Quality ✅ Passed The changed It blocks remain focused, add output context for missing errors, use the repository’s bounded 200-second deployment wait, and retain cleanup for the cluster-scoped binding.
Microshift Test Compatibility ✅ Passed The PR changes assertions only; it adds no Ginkgo tests. The existing top-level Describe guards all nested tests with IsMicroShiftCluster() and g.Skip().
Single Node Openshift (Sno) Test Compatibility ✅ Passed The PR changes existing recommend test bodies only; it adds no It, Describe, Context, or When declarations and introduces no multi-node or HA assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed PASS: The commit changes only error assertions in a CLI test; it adds or modifies no deployment manifest, operator, controller, or scheduling constraint.
Ote Binary Stdout Contract ✅ Passed The PR changes only assertions and patterns inside Ginkgo It callbacks; it adds no fmt.Print, log, klog, stdout, or suite-level output writes.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR only changes assertions in an existing test. Its local service uses net.JoinHostPort and binds to ::; example.com URLs are mock graph data, not external connections.
No-Weak-Crypto ✅ Passed The branch changes only recommend.go; searches found no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, custom crypto, or non-constant-time comparison. The sha256 image digest is not SHA1.
Container-Privileges ✅ Passed The PR changes only error-message assertions in recommend.go; no privileged, host namespace, SYS_ADMIN, escalation, or root settings were added.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately identifies the change as reverting the prior revert that disabled recommendation tests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Jul 31, 2026
@hongkailiu

Copy link
Copy Markdown
Member Author

It requires openshift/oc#2319 too.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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)
test/extended/cli/adm_upgrade/recommend.go (1)

136-136: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle a missing FeatureGate/cluster as disabled.

When FeatureGate/cluster is absent, alertsEvaluatedByCVO must return (false, nil). Propagate 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` at line 136, Update
alertsEvaluatedByCVO so a missing FeatureGate/cluster is treated as disabled and
returns (false, nil), while propagating all other retrieval or parsing errors
unchanged. Use the existing feature-gate lookup and error-handling path rather
than altering unrelated version parsing.

Source: Learnings

🤖 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 `@test/extended/cli/adm_upgrade/recommend.go`:
- Line 136: Update alertsEvaluatedByCVO so a missing FeatureGate/cluster is
treated as disabled and returns (false, nil), while propagating all other
retrieval or parsing errors unchanged. Use the existing feature-gate lookup and
error-handling path rather than altering unrelated version parsing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 398f236f-0241-4314-a02c-00c698689acb

📥 Commits

Reviewing files that changed from the base of the PR and between 0f26807 and 8092ff4.

📒 Files selected for processing (1)
  • test/extended/cli/adm_upgrade/recommend.go

@hongkailiu

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

@openshift-ci

openshift-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/39c59860-8d5a-11f1-9cc2-c83496b50918-0

@hongkailiu

Copy link
Copy Markdown
Member Author

#31462 (comment) did not work.

INFO[2026-08-01T03:38:37Z] Requesting a release from https://arm64.ocp.releases.ci.openshift.org/api/v1/releasestream/5.0.0-0.nightly-arm64/latest 
INFO[2026-08-01T03:38:37Z] Resolved release nightly-arm64 to registry.ci.openshift.org/ocp-arm64/release-5-arm64:5.0.0-0.nightly-arm64-2026-08-01-030824

and

$ oc adm release info --registry-config /tmp/t.d.json registry.ci.openshift.org/ocp-arm64/release-5-arm64:5.0.0-0.nightly-arm64-2026-08-01-030824 -o json | jq -r '.references.spec.tags[]|select(.name=="cli")|.annotations["io.openshift.build.commit.id"]'
0f340425c9edb4a546c66ce10cc37942b1561861

Waiting for openshift/oc#2336 to get in nightly.

@hongkailiu

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

@openshift-ci

openshift-ci Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/edda1010-8e13-11f1-9e31-9adf96895633-0

The error message was introduced by openshift/oc#2336.
@hongkailiu

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

@openshift-ci

openshift-ci Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/df6bdec0-8e5c-11f1-9126-b65c639a938f-0

@hongkailiu

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

@openshift-ci

openshift-ci Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/dbb1b390-8e75-11f1-8242-acccccc116f8-0

@hongkailiu
hongkailiu marked this pull request as ready for review August 2, 2026 13:27
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-aws-csi
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-csi
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-upi

@openshift-ci

openshift-ci Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-ovn 87d5d4f link true /test e2e-gcp-ovn
ci/prow/e2e-aws-ovn-microshift-serial 87d5d4f link true /test e2e-aws-ovn-microshift-serial

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@hongkailiu

Copy link
Copy Markdown
Member Author

The jobs from this comment are green.

It includes the job that was test in #31460 (comment) (and all shared jobs with serial + techpreview).

/verified by @hongkailiu

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2026
@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 2, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@hongkailiu: This PR has been marked as verified by @hongkailiu.

Details

In response to this:

The jobs from this comment are green.

It includes the job that was test in #31460 (comment) (and all shared jobs with serial + techpreview).

/verified by @hongkailiu

/hold cancel

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

Copy link
Copy Markdown
Member Author

/retest-required

@hongkailiu

Copy link
Copy Markdown
Member Author

/payload 5.0 nightly

@openshift-ci

openshift-ci Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info.

@hongkailiu

Copy link
Copy Markdown
Member Author

/payload 5.0 nightly blocking

@openshift-ci

openshift-ci Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@hongkailiu: trigger 14 job(s) of type blocking for the nightly release of OCP 5.0

  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-upgrade-ovn-single-node
  • periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips
  • periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn-upgrade
  • periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-rt-upgrade
  • periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-azure-aks-ovn-conformance
  • periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance
  • periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2
  • periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-2of2
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3
  • periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3
  • periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv4
  • periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv6

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c48b22b0-8eba-11f1-8a3b-b6a99aad5403-0

@hongkailiu

Copy link
Copy Markdown
Member Author

/test e2e-gcp-ovn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants