Skip to content

Revert "OTA-1997: Allow the CVO to use the agentic-skills payload image when creating proposals" - #1430

Closed
redhat-chai-bot wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:revert-1427
Closed

Revert "OTA-1997: Allow the CVO to use the agentic-skills payload image when creating proposals"#1430
redhat-chai-bot wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:revert-1427

Conversation

@redhat-chai-bot

@redhat-chai-bot redhat-chai-bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

This reverts merge commit 977be36 (PR #1427, commit 47bfd33).

Root Cause

PR #1427 changed the CVO's own Deployment manifest to reference {{ index .Images "agentic-skills" }}. During a 4.22→5.0 major upgrade, the 4.22 CVO loads the 5.0 payload manifests but its manifestRenderConfig has no Images field. The Go template renders the field to an empty string, causing the CVO to silently skip its own Deployment — so it never self-updates. This creates a circular deadlock: the old CVO can't deploy the new CVO.

Impact

The 5.0 CI stream has been blocked for 50+ hours (last accepted: 5.0.0-0.ci-2026-07-23-195533). Every 4.22→5.0 major upgrade verification job fails.

Evidence

/label jira/valid-bug


@stbenjam requested in Slack thread

Summary by CodeRabbit

  • Updates
    • Standardized the Agentic skills container image used by the service.
    • Agentic run creation now proceeds using the configured default skills image.
    • Deployment configuration and generated manifests now reference the updated image consistently.
    • Removed the obsolete image mapping from installation configuration.

…simage"

This reverts commit 977be36, reversing
changes made to 6e71933.
@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 26, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

@redhat-chai-bot: This pull request references OTA-1997 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:

This reverts merge commit 977be36 (PR #1427, commit 47bfd33).

Root Cause

PR #1427 changed the CVO's own Deployment manifest to reference {{ index .Images "agentic-skills" }}. During a 4.22→5.0 major upgrade, the 4.22 CVO loads the 5.0 payload manifests but its manifestRenderConfig has no Images field. The Go template renders the field to an empty string, causing the CVO to silently skip its own Deployment — so it never self-updates. This creates a circular deadlock: the old CVO can't deploy the new CVO.

Impact

The 5.0 CI stream has been blocked for 50+ hours (last accepted: 5.0.0-0.ci-2026-07-23-195533). Every 4.22→5.0 major upgrade verification job fails.

Evidence

/label jira/valid-bug


@stbenjam requested in Slack thread

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 Jul 26, 2026

Copy link
Copy Markdown

Walkthrough

The pull request replaces the configurable agentic skills image reference with a fixed OpenShift CI image, removes its ImageStream tag, adds the same image as the controller default, removes an empty-image sync guard, and updates manifest and controller tests.

Changes

Agentic skills image configuration

Layer / File(s) Summary
Fixed image wiring
install/0000_00_cluster-version-operator_30_deployment.yaml, install/image-references, pkg/payload/render_test.go, pkg/payload/testdata/...
The deployment and rendered manifest use quay.io/openshift/ci:ocp_5.0_agentic-skills; the ImageStream tag and test image override are removed.
Controller image default and reconciliation
pkg/agenticrun/controller.go
SkillsImage defaults to the fixed image, and synchronization no longer returns early when it is empty.
AgenticRun expectations
pkg/agenticrun/controller_test.go
Controller tests update generated AgenticRun image expectations and rely on the new default image.

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

Suggested reviewers: racheljpg, jhadvig

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the revert of the agentic-skills payload image change to the CVO.
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 No Ginkgo titles were added/changed; the diff only updates image strings, and existing test names are static, deterministic table entries.
Test Structure And Quality ✅ Passed Changed tests are table-driven unit tests with clear failure messages; they use fake clients/files only, and there are no cluster waits, cleanup gaps, or multi-behavior Ginkgo specs.
Microshift Test Compatibility ✅ Passed Only unit tests and manifest fixtures changed; no new Ginkgo e2e tests, no MicroShift-unsupported APIs or skipped/guarded test logic to review.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo/e2e tests were added; the PR only changes unit tests and payload/manifest fixtures, with no multi-node or SNO-specific assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed Only the skills image default/manifest value changed; no replicas, affinities, spread constraints, or node selectors/tolerations were introduced or modified.
Ote Binary Stdout Contract ✅ Passed PASS: touched code adds no stdout writes; the only init() just registers schemes, and no fmt.Print/os.Stdout/log-to-stdout calls appear in the changed files.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PASS: The PR only touches unit tests/manifests; no new Ginkgo e2e tests, IPv4 literals, or external-network dependencies were added.
No-Weak-Crypto ✅ Passed Touched files only change image defaults/manifests; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret/token comparisons were added.
Container-Privileges ✅ Passed No new privileged settings were introduced; the only relevant field is preexisting hostNetwork:true, unchanged in the manifest and mirrored in testdata.
No-Sensitive-Data-In-Logs ✅ Passed Diff only changes image defaults and test data; no new logging calls or sensitive values were added.
✨ 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 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: redhat-chai-bot
Once this PR has been reviewed and has the lgtm label, please assign pratikmahajan 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:

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 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

@redhat-chai-bot: The label(s) jira/valid-bug cannot be applied or removed, because you are not in one of the allowed teams and are not an allowed user. Must be a member of one of these teams: openshift-patch-managers, openshift-staff-engineers, openshift-release-oversight, openshift-sustaining-engineers

Details

In response to this:

This reverts merge commit 977be36 (PR #1427, commit 47bfd33).

Root Cause

PR #1427 changed the CVO's own Deployment manifest to reference {{ index .Images "agentic-skills" }}. During a 4.22→5.0 major upgrade, the 4.22 CVO loads the 5.0 payload manifests but its manifestRenderConfig has no Images field. The Go template renders the field to an empty string, causing the CVO to silently skip its own Deployment — so it never self-updates. This creates a circular deadlock: the old CVO can't deploy the new CVO.

Impact

The 5.0 CI stream has been blocked for 50+ hours (last accepted: 5.0.0-0.ci-2026-07-23-195533). Every 4.22→5.0 major upgrade verification job fails.

Evidence

/label jira/valid-bug


@stbenjam requested in Slack thread

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.

@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)
pkg/agenticrun/controller_test.go (1)

140-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make TestController_Sync independent of ambient environment variables.

NewController calls DefaultConfig(), and Config.SkillsImage reads the current process value of LIGHTSPEED_SKILLS_IMAGE. Because this test no longer sets the configuration explicitly, its expected image can change in developer/CI environments when that env var is set. Set it with t.Setenv or inject the expected config before constructing the controller.

🤖 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 `@pkg/agenticrun/controller_test.go` around lines 140 - 145, Update
TestController_Sync to control LIGHTSPEED_SKILLS_IMAGE with t.Setenv before
constructing the controller, using the image value expected by the test. Ensure
each test run is independent of the ambient process environment while preserving
the existing Sync assertions.
🤖 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 `@pkg/agenticrun/controller_test.go`:
- Around line 140-145: Update TestController_Sync to control
LIGHTSPEED_SKILLS_IMAGE with t.Setenv before constructing the controller, using
the image value expected by the test. Ensure each test run is independent of the
ambient process environment while preserving the existing Sync assertions.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 92bad69b-171d-4d2d-9cf5-2050e458b5d9

📥 Commits

Reviewing files that changed from the base of the PR and between 977be36 and 34d7e71.

📒 Files selected for processing (6)
  • install/0000_00_cluster-version-operator_30_deployment.yaml
  • install/image-references
  • pkg/agenticrun/controller.go
  • pkg/agenticrun/controller_test.go
  • pkg/payload/render_test.go
  • pkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml
💤 Files with no reviewable changes (2)
  • install/image-references
  • pkg/payload/render_test.go

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

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants