OTA-1997: Allow the CVO to use the agentic-skills payload image when creating proposals - #1433
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe change sources the agentic skills image from payload tags, skips AgenticRun synchronization when it is unset, and replaces the deployment’s skills-image environment variable with templated ChangesAgentic skills image wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ImageStream
participant CVO
participant Controller
participant AgenticRun
ImageStream->>CVO: Declare agentic-skills DockerImage tag
CVO->>CVO: Discover agentic skills image from payload tags
CVO->>Controller: SetSkillsImage(image)
Controller->>Controller: Sync()
Controller->>AgenticRun: Reconcile using configured skills image
Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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.
🧹 Nitpick comments (2)
pkg/agenticrun/controller_test.go (1)
143-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the setter and empty-image path.
This test bypasses
SetSkillsImageby mutatingc.configdirectly, and its only case uses a non-empty image. Use the setter and add a table case asserting that no AgenticRun is created when the image is empty.🤖 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` at line 143, Update the test setup in the relevant AgenticRun test to configure the image through SetSkillsImage instead of mutating c.config.SkillsImage directly. Convert the test to table-driven cases and add an empty-image case that asserts no AgenticRun is created, while preserving the existing non-empty-image coverage.pkg/cvo/cvo.go (1)
449-456: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for payload-driven skills-image wiring.
The new producer path is not exercised by the current tests, which assign
c.config.SkillsImagedirectly. Add anInitializeFromPayloadtest covering theagentic-skillstag and a non-DockerImagereference; otherwise a parsing regression could silently disable AgenticRun creation.🤖 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/cvo/cvo.go` around lines 449 - 456, Add test coverage for InitializeFromPayload that supplies an agentic-skills tag with a DockerImage source and verifies the skills image is wired into AgenticRun creation; also include a non-DockerImage reference and verify it is ignored. Use the existing payload fixtures and assertions, rather than directly assigning c.config.SkillsImage.
🤖 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.
Nitpick comments:
In `@pkg/agenticrun/controller_test.go`:
- Line 143: Update the test setup in the relevant AgenticRun test to configure
the image through SetSkillsImage instead of mutating c.config.SkillsImage
directly. Convert the test to table-driven cases and add an empty-image case
that asserts no AgenticRun is created, while preserving the existing
non-empty-image coverage.
In `@pkg/cvo/cvo.go`:
- Around line 449-456: Add test coverage for InitializeFromPayload that supplies
an agentic-skills tag with a DockerImage source and verifies the skills image is
wired into AgenticRun creation; also include a non-DockerImage reference and
verify it is ignored. Use the existing payload fixtures and assertions, rather
than directly assigning c.config.SkillsImage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8c7b5fc3-4af6-45e9-8d8e-809f7ebd1f6a
📒 Files selected for processing (5)
install/0000_00_cluster-version-operator_30_deployment.yamlpkg/agenticrun/controller.gopkg/agenticrun/controller_test.gopkg/cvo/cvo.gopkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml
💤 Files with no reviewable changes (2)
- pkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml
- install/0000_00_cluster-version-operator_30_deployment.yaml
| - name: CLUSTER_PROFILE | ||
| value: '{{ .ClusterProfile }}' | ||
| - name: LIGHTSPEED_SKILLS_IMAGE | ||
| value: "quay.io/openshift/ci:ocp_5.0_agentic-skills" |
There was a problem hiding this comment.
This is the main change since #1427, right? We'd previously used index .Images "agentic-skills" there, and it makes sense to me to not set the env-var at all, which is what we're doing this time around.
| Namespace: envOrDefault("LIGHTSPEED_AGENTIC_RUN_NAMESPACE", "openshift-lightspeed"), | ||
| PromptConfigMap: envOrDefault("LIGHTSPEED_PROMPT_CONFIGMAP", "cluster-update-advisory-prompt"), | ||
| SkillsImage: envOrDefault("LIGHTSPEED_SKILLS_IMAGE", "quay.io/openshift/ci:ocp_5.0_agentic-skills"), | ||
| SkillsImage: os.Getenv("LIGHTSPEED_SKILLS_IMAGE"), |
There was a problem hiding this comment.
We don't need the Getenv anymore, right? We're getting the skills image from the image-references in the payload. Hmm, which we'll want to restore to what we had in #1427, and which #1431 reverted, right? I think we need that image-references entry to get the agentic-skills ImageStreamTag pulled into the release payload via openshift/oc#2287.
There was a problem hiding this comment.
Sorry, yes, you're right! I've updated it now so we don't get this anymore since we don't need it
There was a problem hiding this comment.
And added the image-references entry!
There was a problem hiding this comment.
One additional follow-up on removing the environment variable.
3a46d7c to
63171fd
Compare
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 `@install/image-references`:
- Around line 5-8: Add the required metadata to the agentic-skills ImageStream,
including metadata.name, the cluster-profile annotations, and the
kubernetes.io/description annotation, following the conventions used by other
install manifest ImageStreams so it renders and installs consistently.
🪄 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: 9235ebb4-b41d-4556-a033-58e9e1b53fdc
📒 Files selected for processing (6)
install/0000_00_cluster-version-operator_30_deployment.yamlinstall/image-referencespkg/agenticrun/controller.gopkg/agenticrun/controller_test.gopkg/cvo/cvo.gopkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml
💤 Files with no reviewable changes (2)
- pkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml
- install/0000_00_cluster-version-operator_30_deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cvo/cvo.go
- pkg/agenticrun/controller.go
- pkg/agenticrun/controller_test.go
| - name: agentic-skills | ||
| from: | ||
| kind: DockerImage | ||
| name: placeholder.url.oc.will.replace.this.example.org:agentic-skills |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Complete the ImageStream metadata before adding this tag.
As supplied, this install manifest has no metadata.name, cluster-profile annotations, or kubernetes.io/description. Add the required metadata and annotations so the ImageStream can be rendered and installed consistently.
🤖 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 `@install/image-references` around lines 5 - 8, Add the required metadata to
the agentic-skills ImageStream, including metadata.name, the cluster-profile
annotations, and the kubernetes.io/description annotation, following the
conventions used by other install manifest ImageStreams so it renders and
installs consistently.
Source: Path instructions
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| } | ||
|
|
||
| if c.config.SkillsImage == "" { | ||
| klog.V(i.Normal).Infof("Skipping agentic run creation: LIGHTSPEED_SKILLS_IMAGE is not set") |
There was a problem hiding this comment.
This error message needs to be updated now that we've removed the env. var. completely. Maybe just:
| klog.V(i.Normal).Infof("Skipping agentic run creation: LIGHTSPEED_SKILLS_IMAGE is not set") | |
| klog.V(i.Normal).Infof("Skipping agentic run creation: skills image is not set") |
There was a problem hiding this comment.
Thanks - updated!
63171fd to
e9599b1
Compare
|
/test unit |
|
/payload-aggregate periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade 5 |
|
@wking: trigger 2 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/da564900-89e5-11f1-96c6-88d2db72748d-0 |
Review: Does this PR solve the underlying problem from the RCA?Short answer: Yes — for the specific regression. The root cause was that #1427 placed This PR removes the Remaining concerns worth calling out1. The systemic guard is still missing (RCA §7.3) The silent-skip behavior in The RCA recommends: "never silently skip the CVO's own Deployment. If that manifest fails to render, this is fatal and must surface as That guard would be valuable as a follow-up — it would turn a silent, hard-to-diagnose ~50h outage into an immediate loud failure. 2. Presubmit coverage gap As discussed in this thread, the 3. Minor: The SummaryThe fix correctly addresses the root cause by moving the skills image resolution from manifest-template-time to Go-runtime-time, which is robust against cross-version Posted via Chai Bot from this Slack thread at the request of a team member. |
wking
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@racheljpg: 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. 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. |
|
Some kind of unrelated DNS issue: /override ci/prow/e2e-agnostic-ovn e2e-agnosic-operator stumbled on install, so try that one again: /test e2e-agnostic-operator
/override ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3
/override ci/prow/e2e-agnostic-ovn-techpreview-serial-2of3
/override ci/prow/e2e-agnostic-ovn-techpreview
/override ci/prow/e2e-hypershift-conformance |
|
@wking: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
Same logic, better spelling /override ci/prow/e2e-agnostic-ovn |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-ovn, ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3, ci/prow/e2e-agnostic-ovn-techpreview-serial-2of3, ci/prow/e2e-aws-ovn-techpreview, ci/prow/e2e-hypershift-conformance 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 kubernetes-sigs/prow repository. |
|
earlier aggregates both terminated. So we'll need to run these again after rebasing around #1432. |
openshift-ci
Bot
added
the
needs-rebase
e9599b1 to
9657b79
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/payload-aggregate periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade 5 |
|
@racheljpg: trigger 2 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/c0484d70-8a1b-11f1-87c6-3f8a04ff6860-0 |
9657b79 to
3c86f2c
Compare
|
ignore those periodics above - I triggered those before realising there was an issue with my rebase /payload-aggregate periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade 5 |
|
@racheljpg: trigger 2 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/19b442d0-8a65-11f1-9d35-9ebb5991575b-0 |
|
/retest |
|
Pre-Verified I deployed the CVO in a live 5.0.0 ec4 cluster with a custom Agentic Skill image (built from upstream main and exposed on OCP internal registry). Post deploy validations
$ oc get deployment -n openshift-cluster-version cluster-version-operator -o json | jq '.spec.template.spec.containers[0].env[] | select(.name | test("LIGHTSPEED|SKILL"))'
$ oc exec -n openshift-cluster-version $(oc get pod -n openshift-cluster-version -l k8s-app=cluster-version-operator -o name) -- cat /release-manifests/image-references | jq .
{
"kind": "ImageStream",
"apiVersion": "image.openshift.io/v1",
"metadata": {
"name": "5.0.0-ec.4",
"creationTimestamp": null
},
"spec": {
"tags": [
{
"name": "cluster-version-operator",
"annotations": {
"io.openshift.build.versions": "kubernetes=1.35.0"
},
"from": {
"kind": "DockerImage",
"name": "cluster-version-operator:latest"
},
"generation": null,
"importPolicy": {},
"referencePolicy": {
"type": ""
}
},
{
"name": "agentic-skills",
"from": {
"kind": "DockerImage",
"name": "image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076"
},
"generation": null,
"importPolicy": {},
"referencePolicy": {
"type": ""
}
}
]
}
}
$ oc logs -f cluster-version-operator-7d765b6f76-5h6d6 | grep -i -E "skills|agentic"
I0728 11:36:50.578093 1 controller.go:192] Started syncing CVO configuration "ClusterVersionOperator/agenticrun-lifecycle-controller"
I0728 11:36:50.617821 1 controller.go:169] AgenticRun CustomResourceDefinition available? true (<nil>)
I0728 11:36:50.621938 1 controller.go:249] Skipping agentic run creation: cluster is progressing (Working towards 5.0.0-ec.4: 6 of 19 done (31% complete))
I0728 11:39:35.682717 1 controller.go:192] Started syncing CVO configuration "ClusterVersionOperator/agenticrun-lifecycle-controller"
$ oc get agenticrun -n openshift-lightspeed
NAME AGE
ota-5-0-0-ec-4-to-5-0-1 4s
ota-5-0-0-ec-4-to-5-0-10 6s
ota-5-0-0-ec-4-to-5-0-7 5s
ota-5-0-0-ec-4-to-5-0-8 5s
ota-5-0-0-ec-4-to-5-0-9 5s
ota-5-0-0-ec-4-to-5-1-0 6s
ota-5-0-0-ec-4-to-5-1-1 6s
ota-5-0-0-ec-4-to-5-1-2 6s
ota-5-0-0-ec-4-to-5-1-3 6s
ota-5-0-0-ec-4-to-5-1-4 6s
$ oc get agenticruns -n openshift-lightspeed -o jsonpath='{range .items[*]}{.spec.tools.skills[0].image}{"\n"}{end}'
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076
image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/agentic-skills@sha256:749114dd17b427c96a2759249b3f8cd901ca7f69c811ee042551d089f4b07076 |
|
/lgtm |
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
From the most recent aggregate jobs, the AWS aggregate had a Job execution failed: Pod got deleted unexpectedly harness failure, leaving us blind to how things would have gone there. The Azure aggregate failed, but some individual runs passed, and they all completed an update:
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/openshift-cluster-version-operator-1433-ci-5.0-upgrade-from-stable-4.22-e2e-azure-ovn-upgrade/2082032596555927552/artifacts/e2e-azure-ovn-upgrade-2/gather-extra/artifacts/clusterversion.json | jq -r '.items[].status.history[] | .startedTime + " " + .completionTime + " " + .state + " " + .version'
2026-07-28T10:21:30Z 2026-07-28T11:20:50Z Completed 5.0.0-0.ci-2026-07-28-092705-test-ci-op-bil3gzvz-latest
2026-07-28T09:40:06Z 2026-07-28T10:04:55Z Completed 4.22.0-0.ci-2026-07-28-031447
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/openshift-cluster-version-operator-1433-ci-5.0-upgrade-from-stable-4.22-e2e-azure-ovn-upgrade/2082032597453508608/artifacts/e2e-azure-ovn-upgrade-4/gather-extra/artifacts/clusterversion.json | jq -r '.items[].status.history[] | .startedTime + " " + .completionTime + " " + .state + " " + .version'
2026-07-28T10:22:48Z 2026-07-28T11:25:47Z Completed 5.0.0-0.ci-2026-07-28-092705-test-ci-op-bil3gzvz-latest
2026-07-28T09:40:11Z 2026-07-28T10:03:06Z Completed 4.22.0-0.ci-2026-07-28-031447
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/openshift-cluster-version-operator-1433-ci-5.0-upgrade-from-stable-4.22-e2e-azure-ovn-upgrade/2082032596115525632/artifacts/e2e-azure-ovn-upgrade-1/gather-extra/artifacts/clusterversion.json | jq -r '.items[].status.history[] | .startedTime + " " + .completionTime + " " + .state + " " + .version'
2026-07-28T10:22:17Z 2026-07-28T11:26:01Z Completed 5.0.0-0.ci-2026-07-28-092705-test-ci-op-bil3gzvz-latest
2026-07-28T09:40:46Z 2026-07-28T10:08:41Z Completed 4.22.0-0.ci-2026-07-28-031447So looks fixed to me:
/lgtm
/verified by periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-azure-ovn-upgrade
openshift-ci-robot
added
the
verified
|
@wking: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jrangelramos, racheljpg, wking 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
|
/override ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3 |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3 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 kubernetes-sigs/prow repository. |
|
@racheljpg: all tests passed! 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. |
5 participants
Hello. This is a PR to address some issues that were implemented by the previous iteration of this PR.
It had to be reverted, because it introduced a field in the CVO deployment that the 4.22 CVO couldn't render, so it broke during upgrades.
This PR should hopefully address that issue.
Thanks!
Assisted-by: Claude Code
Summary by CodeRabbit
CLUSTER_PROFILEsetting.agentic-skillsimage reference for AgenticRun skills.