Skip to content

OTA-1959: Case 1 - oc adm upgrade recommend shows both Cincinnati-sourced and alert-sourced risks in output - #2349

Open
nbottari9 wants to merge 1 commit into
openshift:mainfrom
nbottari9:1959-accept-risks-test-cases
Open

OTA-1959: Case 1 - oc adm upgrade recommend shows both Cincinnati-sourced and alert-sourced risks in output#2349
nbottari9 wants to merge 1 commit into
openshift:mainfrom
nbottari9:1959-accept-risks-test-cases

Conversation

@nbottari9

@nbottari9 nbottari9 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Added a new test fixture to ensure that when there are both OSUS/Cincinnati risks and alert risks present, both types of risks are present in the output of oc adm upgrade recommend and oc adm upgrade recommend --version [VERSION]

Summary by CodeRabbit

  • New Features
    • Added upgrade recommendation examples for version 5.0.0, including graph-based updates, conditional risks, alerts, release metadata, and upgrade-blocking reasons.
    • Added examples for feature-gate and infrastructure configurations supporting upgrade recommendation scenarios.
  • Tests
    • Expanded validation for upgrade recommendations, outdated releases, graph requests, synthetic risks, and guidance for accepting unresolved issues.
    • Added coverage for expected version 5.0.0-ec.5 recommendations and CLI output.

@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: LGTM mode

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

openshift-ci-robot commented Aug 5, 2026

Copy link
Copy Markdown

@nbottari9: This pull request references OTA-1959 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 "5.0.0" version, but no target version was set.

Details

In response to this:

Added a new test fixture to ensure that when there are both OSUS/Cincinnati risks and alert risks present, both types of risks are present in the output of oc adm upgrade recommend and oc adm upgrade recommend --version [VERSION]

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.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Walkthrough

Changes

Upgrade recommendation risk fixtures

Layer / File(s) Summary
Cluster context fixtures
pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks-featuregate.yaml, pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks-infrastructure.yaml
Adds cluster FeatureGate and GCP Infrastructure resources for the example.
ClusterVersion graph and risk state
pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks-cv.yaml
Adds graph configuration, four conditional update risks, two conditional updates, status conditions, desired release data, and completed history.
Recommendation outputs and test mapping
pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.json, pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.output, pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.show-outdated-releases-output, pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.version-5.0.0-ec.5-output, pkg/cli/admin/upgrade/recommend/examples_test.go
Adds expected recommendation responses and CLI output, then maps the ClusterVersion fixture to expected version 5.0.0-ec.5.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: tchap, ardaguclu


Important

Pre-merge checks failed

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

❌ Failed checks (1 error)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error Fixtures expose internal CI hostnames and private IPs, including api-int.ci-ln...:6443, console-openshift-console...ci.openshift.org, and 10.130.0.26:8443. Replace internal CI endpoints, cluster names, pod names, and private IPs with stable synthetic values before committing output fixtures.
✅ 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 The PR adds no Ginkgo titles. Its new subtest name uses a committed fixture path and static variant string, with no timestamps, random IDs, node names, namespaces, or IP addresses.
Test Structure And Quality ✅ Passed Not applicable: the PR adds fixtures and one mapping to a standard testing.T fixture test; it adds no Ginkgo It blocks, cluster resource setup, waits, or Ginkgo assertions.
Microshift Test Compatibility ✅ Passed The change adds fixture data and a standard Go TestExamples test; it adds no Ginkgo It, Describe, Context, or When e2e test, so this check does not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The PR adds fixture data and a standard Go TestExamples unit test; it adds no Ginkgo e2e test or multi-node/HA assumption, so SNO compatibility is not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed The change adds ClusterVersion, FeatureGate, and Infrastructure fixtures plus expected outputs; scans found no deployments, controllers, or scheduling constraints.
Ote Binary Stdout Contract ✅ Passed The commit adds only fixture data and one TestExamples map entry; it adds no process-level stdout/logging code, and test output is routed to bytes.Buffer.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The commit adds fixture files and one standard TestExamples mapping entry; no Ginkgo tests, IPv4 assumptions, or external network operations were added, and mockData loads local files.
No-Weak-Crypto ✅ Passed The 502 added lines contain no MD5, SHA-1, DES, RC4, 3DES, Blowfish, or ECB usage; SHA-256 image digests are fixture identifiers, not weak crypto code.
Container-Privileges ✅ Passed The added YAML contains only ClusterVersion, FeatureGate, and Infrastructure objects; scans found no privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, root, or allowPrivilegeEscalation settings.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: testing both Cincinnati-sourced and alert-sourced risks in upgrade recommendation output.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@openshift-ci
openshift-ci Bot requested review from ardaguclu and tchap August 5, 2026 14:41
@nbottari9
nbottari9 force-pushed the 1959-accept-risks-test-cases branch from 8a6bb16 to c84a084 Compare August 5, 2026 14:42
@nbottari9

Copy link
Copy Markdown
Contributor Author

/verified by TestExamples @nbottari9

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 5, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@nbottari9: This PR has been marked as verified by TestExamples @nbottari9.

Details

In response to this:

/verified by TestExamples @nbottari9

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 hongkailiu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case looks good.

Could you record the steps of the testing to get the yamls (and json) in the commit msg (so that the next person can repeat)?

Like the command to create a cluster bot cluster.
Modification to connect it to fauxinnati.
Trigger the alert.
etc and etc.

You do not need to re-doc the steps in the readme tho.
Reference to it is fine.


Updates to 5.1:
VERSION ISSUES
5.1.0 MultipleReasons

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action is required.
Just a remark.
This does not look ideal but I guess we need to save space for all the "outdated" versions.

@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Aug 5, 2026
@nbottari9
nbottari9 force-pushed the 1959-accept-risks-test-cases branch from 2c17302 to 601de10 Compare August 5, 2026 20:03
@openshift-ci-robot

openshift-ci-robot commented Aug 5, 2026

Copy link
Copy Markdown

@nbottari9: This pull request references OTA-1959 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 "5.0.0" version, but no target version was set.

Details

In response to this:

Added a new test fixture to ensure that when there are both OSUS/Cincinnati risks and alert risks present, both types of risks are present in the output of oc adm upgrade recommend and oc adm upgrade recommend --version [VERSION]

Summary by CodeRabbit

  • New Features

  • Added example upgrade recommendations covering upgrade graphs, conditional risks, synthetic alerts, release images, and blocking reasons.

  • Added scenarios showing recommended and non-recommended updates, including guidance to accept unresolved issues before proceeding.

  • Added examples for outdated releases, available channels, cluster capabilities, and feature-gated upgrade behavior.

  • Tests

  • Expanded upgrade recommendation coverage for version 5.0.0 and related release scenarios.

  • Added expected command output for graph requests and outdated-release reporting.

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.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@nbottari9
nbottari9 force-pushed the 1959-accept-risks-test-cases branch 2 times, most recently from d7b878d to e92993b Compare August 5, 2026 20:19
@hongkailiu

Copy link
Copy Markdown
Member
  1. Set update channel. oc adm upgrade channel [CHANNEL]

Replace "[CHANNEL]" with the channel name?

  1. Create TestAlert within the cluster. oc apply -f ~/Downloads/alert.yaml Download TestAlert manifest

Do cat ~/Downloads/alert.yaml before oc apply so that we do not need to make a reference to slack?

…` correctly filters out accepted alert-sourced risks

steps to reproduce:

1) Create a cluster bot cluster: `launch 5.0.0-ec.2 gcp,techpreview`

2) Modify the ClusterVersion manifest to point at an upstream OSUS server. See [fauxinnati](https://fauxinnati-fauxinnati.apps.ota-stage.q2z4.p1.openshiftapps.com/) for more info on mock Cincinnati graph data endpoints. `oc edit clusterversion version` -> add `spec.upstream` with the URL

3) Set update channel. `oc adm upgrade channel risks-always`

At this point, you have configured enough for OSUS/Cincinnati risks to show up with `oc adm upgrade recommend`. Running this, you should see some risks that apply to upgrading the cluster. Further steps will be for setting up test alerts.

4) Create TestAlert within the cluster.

`TestAlert` manifest:

apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
  name: test-update-alert
  namespace: openshift-monitoring
spec:
  groups:
  - name: test
    rules:
    - alert: TestAlert
      expr: vector(1)
      labels:
        severity: warning
        openShiftUpdatePrecheck: "true"
      annotations:
        summary: "Test alert for updates"
        description: "Test alert for updates"

Save this to a file in ~/Downloads, then run `oc apply -f ~/Downloads/alert.yaml`

After a bit, you should see BOTH graph-sourced and alert-sourced risks show up when you run `oc adm upgrade recommend`

5) Gather the manifests needed for a full test fixture. Namely `clusterVersion`, `infrastructure`, `featureGate`, and `alerts`.

clusterVersion (the anchor): `oc get -o yaml clusterversion version >./pkg/cli/admin/upgrade/recommend/examples/[FIXTURE_NAME]-cv.yaml`
infrastructure: `oc get -o yaml infrastructure cluster >./pkg/cli/admin/upgrade/recommend/examples/[FIXTURE_NAME]-infrastructure.yaml`
featureGate: `oc get -o yaml featuregate cluster >./pkg/cli/admin/upgrade/recommend/examples/[FIXTURE_NAME]-featuregate.yaml`
alerts: `oc adm inspect-alerts >./pkg/cli/admin/upgrade/recommend/examples/[FIXTURE_NAME]-alerts.json`

Note: may need `OC_ENABLE_CMD_INSPECT_ALERTS=true` for `oc adm inspect-alerts` to work correctly

6) Modify `examples_test.go` to include an entry for the "specific version" variant.

7) Run the tests and confirm output matches desired results: `go test -v ./pkg/cli/admin/upgrade/recommend...`

Note: to actually create new output files, use `UPDATE=true go test -v ./pkg/cli/admin/upgrade/recommend...`

Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
@nbottari9
nbottari9 force-pushed the 1959-accept-risks-test-cases branch from 4bca7db to 9f2cec7 Compare August 5, 2026 21:03
@nbottari9

Copy link
Copy Markdown
Contributor Author

/verified by @nbottari9

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 5, 2026
@openshift-ci-robot

Copy link
Copy Markdown

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

Details

In response to this:

/verified by @nbottari9

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.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@openshift-ci-robot

openshift-ci-robot commented Aug 5, 2026

Copy link
Copy Markdown

@nbottari9: This pull request references OTA-1959 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 "5.0.0" version, but no target version was set.

Details

In response to this:

Added a new test fixture to ensure that when there are both OSUS/Cincinnati risks and alert risks present, both types of risks are present in the output of oc adm upgrade recommend and oc adm upgrade recommend --version [VERSION]

Summary by CodeRabbit

  • New Features
  • Added upgrade recommendation examples for version 5.0.0, including graph-based updates, conditional risks, alerts, release metadata, and upgrade-blocking reasons.
  • Added examples for feature-gate and infrastructure configurations supporting upgrade recommendation scenarios.
  • Tests
  • Expanded validation for upgrade recommendations, outdated releases, graph requests, synthetic risks, and guidance for accepting unresolved issues.
  • Added coverage for expected version 5.0.0-ec.5 recommendations and CLI output.

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

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-agnostic-ovn-cmd
/test e2e-aws-ovn
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-ovn-upgrade

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongkailiu, nbottari9

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 Aug 6, 2026

Test alert for updates. https://github.com/openshift/runbooks/tree/master/alerts?runbook=notfound

error: There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: SyntheticRiskA,SyntheticRiskB,SyntheticRiskC,TestAlert

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug. I will follow up.

Suggested change
error: There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: SyntheticRiskA,SyntheticRiskB,SyntheticRiskC,TestAlert
error: There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: SyntheticRiskA,SyntheticRiskB,TestAlert

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nbottari9

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2

1 similar comment
@nbottari9

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@nbottari9: all tests passed!

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.

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. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants