OTA-2084: Conditionally deploy console plugin when AgenticRun CRD is present#1425
OTA-2084: Conditionally deploy console plugin when AgenticRun CRD is present#1425jhadvig wants to merge 3 commits into
Conversation
|
@jhadvig: This pull request references OTA-2084 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
WalkthroughThe change embeds console-plugin manifests, adds create/update/delete operations, gates them on CRD availability, wires the plugin image from payload data, and makes the agentic-run controller worker and queue triggers unconditional. ChangesAgentic Run Console Plugin
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CVO
participant AgenticRunController
participant EmbeddedAssets
participant KubernetesAPI
CVO->>AgenticRunController: Set console plugin image
CVO->>AgenticRunController: Queue reconciliation
AgenticRunController->>KubernetesAPI: Check agenticruns CRD
AgenticRunController->>EmbeddedAssets: Load manifests
EmbeddedAssets-->>AgenticRunController: Return YAML assets
AgenticRunController->>KubernetesAPI: Create or update console plugin resources
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/cvo/cvo.go (1)
594-601: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the nearby controller documentation.
Operator.agenticRunControllerstill says “if enabled”, but this worker now always starts and the controller gates itself on CRD availability. Update that field comment to describe the current lifecycle.As per path instructions, “When modifying code, check that nearby comments, kubernetes.io/description annotations, and doc strings still accurately describe the new behavior.”
🤖 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 594 - 601, Update the documentation comment for Operator.agenticRunController to state that its worker always starts and the controller gates operation based on CRD availability, removing the outdated “if enabled” wording. Do not change the worker implementation or unrelated comments.Source: Path instructions
pkg/agenticrun/consoleplugin.go (1)
3-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRun gofmt on
pkg/agenticrun/consoleplugin.go. The Kubernetes imports are out of gofmt order.🤖 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/consoleplugin.go` around lines 3 - 16, Run gofmt on consoleplugin.go to reorder the Kubernetes imports into standard formatting order; do not change the import set or other behavior.Source: Coding guidelines
🤖 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 `@pkg/agenticrun/controller.go`:
- Around line 141-153: Update Controller.crdAvailable to accept a
context.Context parameter and pass that context to the dynamicClient
Resource(...).List call instead of context.Background(). Update every caller to
provide its reconciliation context, preserving the existing CRD availability
caching and error handling.
- Around line 174-188: Update the plugin lifecycle handling around
cleanupConsolePluginManifests and c.ensureConsolePlugin so any cleanup or ensure
failure is returned from the sync method after logging, rather than returning
success. Preserve the existing state updates only on successful operations,
allowing the queue’s retry path to retry transient API failures.
---
Nitpick comments:
In `@pkg/agenticrun/consoleplugin.go`:
- Around line 3-16: Run gofmt on consoleplugin.go to reorder the Kubernetes
imports into standard formatting order; do not change the import set or other
behavior.
In `@pkg/cvo/cvo.go`:
- Around line 594-601: Update the documentation comment for
Operator.agenticRunController to state that its worker always starts and the
controller gates operation based on CRD availability, removing the outdated “if
enabled” wording. Do not change the worker implementation or unrelated comments.
🪄 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: 2cd51b30-9df9-4c6e-b2df-51a0fec57c2d
📒 Files selected for processing (16)
pkg/agenticrun/bindata/assets.gopkg/agenticrun/bindata/assets/configmap.yamlpkg/agenticrun/bindata/assets/consoleplugin.yamlpkg/agenticrun/bindata/assets/deployment.yamlpkg/agenticrun/bindata/assets/namespace.yamlpkg/agenticrun/bindata/assets/networkpolicy.yamlpkg/agenticrun/bindata/assets/service.yamlpkg/agenticrun/bindata/assets/serviceaccount.yamlpkg/agenticrun/consoleplugin.gopkg/agenticrun/controller.gopkg/agenticrun/controller_test.gopkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/status.gopkg/payload/render_test.go
💤 Files with no reviewable changes (7)
- pkg/agenticrun/bindata/assets/service.yaml
- pkg/agenticrun/bindata/assets/networkpolicy.yaml
- pkg/agenticrun/bindata/assets/consoleplugin.yaml
- pkg/agenticrun/bindata/assets/configmap.yaml
- pkg/payload/render_test.go
- pkg/agenticrun/bindata/assets/serviceaccount.yaml
- pkg/agenticrun/bindata/assets/namespace.yaml
| func (c *Controller) crdAvailable() bool { | ||
| if time.Since(c.crdLastChecked) < crdCheckInterval { | ||
| return c.crdAvailableCache | ||
| } | ||
| c.crdLastChecked = time.Now() | ||
| if c.dynamicClient == nil { | ||
| c.crdAvailableCache = false | ||
| return false | ||
| } | ||
| agenticRunGVR := schema.GroupVersionResource{Group: "agentic.openshift.io", Version: "v1alpha1", Resource: "agenticruns"} | ||
| _, err := c.dynamicClient.Resource(agenticRunGVR).List(context.Background(), metav1.ListOptions{Limit: 1}) | ||
| c.crdAvailableCache = err == nil || !kerrors.IsNotFound(err) && !isNoMatchError(err) | ||
| return c.crdAvailableCache |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the reconciliation context for the CRD probe.
context.Background() disconnects the discovery request from shutdown and request cancellation. Accept ctx in crdAvailable and pass it to List.
As per path instructions, “context.Context for cancellation and timeouts.”
🤖 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.go` around lines 141 - 153, Update
Controller.crdAvailable to accept a context.Context parameter and pass that
context to the dynamicClient Resource(...).List call instead of
context.Background(). Update every caller to provide its reconciliation context,
preserving the existing CRD availability caching and error handling.
Source: Path instructions
| if !c.crdAvailable() { | ||
| if err := cleanupConsolePluginManifests(ctx, c.client); err != nil { | ||
| klog.V(i.Normal).Infof("Failed to clean up console plugin: %v", err) | ||
| } | ||
| c.consolePluginEnsured = false | ||
| return nil | ||
| } | ||
|
|
||
| if !c.consolePluginEnsured { | ||
| if err := c.ensureConsolePlugin(ctx); err != nil { | ||
| klog.V(i.Normal).Infof("Failed to ensure console plugin: %v", err) | ||
| } else { | ||
| c.consolePluginEnsured = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate plugin lifecycle failures for retry.
Cleanup and apply failures are logged but reported as a successful sync. This prevents the queue’s error retry path from promptly retrying a transient API failure, leaving stale or missing plugin resources until another unrelated enqueue.
Proposed fix
if !c.crdAvailable(ctx) {
if err := cleanupConsolePluginManifests(ctx, c.client); err != nil {
- klog.V(i.Normal).Infof("Failed to clean up console plugin: %v", err)
+ return fmt.Errorf("cleaning up console plugin: %w", err)
}
c.consolePluginEnsured = false
return nil
}
if !c.consolePluginEnsured {
if err := c.ensureConsolePlugin(ctx); err != nil {
- klog.V(i.Normal).Infof("Failed to ensure console plugin: %v", err)
+ return fmt.Errorf("ensuring console plugin: %w", err)
} else {
c.consolePluginEnsured = true
}
}🤖 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.go` around lines 174 - 188, Update the plugin
lifecycle handling around cleanupConsolePluginManifests and
c.ensureConsolePlugin so any cleanup or ensure failure is returned from the sync
method after logging, rather than returning success. Preserve the existing state
updates only on successful operations, allowing the queue’s retry path to retry
transient API failures.
|
Manually validated on a cluster-bot cluster (5.0.0-0-2026-07-20-121922):
|
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 `@pkg/agenticrun/bindata/assets/networkpolicy-allow-console.yaml`:
- Around line 12-15: Update the ingress rule in the network policy to add a from
clause containing namespaceSelector and podSelector selectors matching the
OpenShift console labels, while preserving TCP port 9001. Ensure only console
pods can access the plugin assets.
🪄 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: a798cbad-4be8-4a6f-94a5-499c252f50e3
📒 Files selected for processing (2)
pkg/agenticrun/bindata/assets/networkpolicy-allow-console.yamlpkg/agenticrun/consoleplugin.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/agenticrun/consoleplugin.go
| ingress: | ||
| - ports: | ||
| - port: 9001 | ||
| protocol: TCP |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files 'pkg/agenticrun/bindata/assets/*' | sed -n '1,120p'
printf '\n== target file ==\n'
cat -n pkg/agenticrun/bindata/assets/networkpolicy-allow-console.yaml
printf '\n== related networkpolicy assets ==\n'
rg -n --no-heading -A6 -B4 'kind: NetworkPolicy|podSelector:|namespaceSelector:|policyTypes:|ingress:' pkg/agenticrun/bindata/assets -g '*.yaml' -g '*.yml'Repository: openshift/cluster-version-operator
Length of output: 3749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== console plugin manifest ==\n'
cat -n pkg/agenticrun/bindata/assets/consoleplugin.yaml
printf '\n== service/deployment excerpts ==\n'
rg -n --no-heading -A8 -B4 'kind: (Service|Deployment)|labels:|selector:|ports:|9001|console' pkg/agenticrun/bindata/assets/{service.yaml,deployment.yaml,consoleplugin.yaml} -g '*.yaml'Repository: openshift/cluster-version-operator
Length of output: 8737
Restrict ingress to the OpenShift console.
This rule omits from, so any source can reach port 9001 on the plugin pods. Add a namespaceSelector/podSelector for the console’s labels so only the console can fetch the plugin assets.
🤖 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/bindata/assets/networkpolicy-allow-console.yaml` around lines
12 - 15, Update the ingress rule in the network policy to add a from clause
containing namespaceSelector and podSelector selectors matching the OpenShift
console labels, while preserving TCP port 9001. Ensure only console pods can
access the plugin assets.
| } | ||
|
|
||
| obj.SetResourceVersion(existing.GetResourceVersion()) | ||
| if err := client.Update(ctx, obj); err != nil { |
There was a problem hiding this comment.
This looks like it might hot-loop on updates? Without AgenticRun in the CI runs, I guess we don't have Kube API audit logs to know, but I'd have expected us to be using lib/resourceapply client-side existing-vs.-required diff logic to compare the existing we pulled from the cluster with our local-manifest expectations, and to only update when we saw changes we thought needed to be pushed into the cluster.
| } | ||
| if original != nil && len(config.Status.History) < len(original.Status.History) { | ||
| klog.V(internal.Normal).Infof("Reconciling agentic runs because ClusterVersion.status.history got pruned") | ||
| optr.agenticRunController.Queue().Add(optr.agenticRunController.QueueKey()) |
There was a problem hiding this comment.
Hmm, I see this logic dates back to 6de7f5a (#1382), but I'm personally not clear on why a history prune would need an agentic-run controller poke. @hongkailiu, do you remember why you'd needed to do that?
There was a problem hiding this comment.
IIRC, CVO has the logic to prune CV.history when it grows to some length.
The proposals on the cluster are related to the versions in CV.history.
Thus, in my mind, CVO pruning CV.history leads to reconciliation on proposals .
There was a problem hiding this comment.
ok so if I read it correctly the logic should stay as is ...
There was a problem hiding this comment.
The proposals on the cluster are related to the versions in
CV.history.
Are they? The AgenticRuns themselves are based on forward-looking available and conditional updates, not history. dynamicClient gives the ability to pull in whatever data though, so we also need to drop into runReadinessJSON. Looking at the current readiness checks, there is a history mention:
$ git grep -il history pkg/readiness
pkg/readiness/checks_test.go
pkg/readiness/cluster_conditions.goThat's including the most recent 5 history entries in recent_history. But I don't see anything in either the CVO repo or https://github.com/openshift/agentic-skills trying to consume recent_history. Maybe we can drop it? And we're currently strongly preferring to preserve the 5 most recent history entries when pruning anyway, so it seems unlikely to me that history pruning would need fresh AgenticRun prompt generation, even if we do need to keep recent_history in the prompt for some reason.
There was a problem hiding this comment.
You're right that AgenticRuns are forward-looking (based on available/conditional updates, not history). But deleteAgenticRuns also uses history as a keep-list — line 344 preserves runs whose target version appears in cv.Status.History, so they don't get deleted as "irrelevant" just because the version is no longer an available update target (e.g. after the cluster already upgraded to it).
When history is pruned (>100 entries), a version drops from that keep-list. Without the trigger, the associated run would linger until the next non-cached syncAvailableUpdates poke, which could be 2-4 minutes away (the controller has no self-requeue — it only runs when explicitly poked).
That said, it's a belt-and-suspenders measure.. the 2-4 minute delay is inconsequential for stale metadata, and history pruning only fires on clusters with 100+ update history entries. But the trigger is 4 lines, negligible cost, and makes the intent explicit... thats why Id lean toward keeping it, but honestly I ok with both approches.
…present Move the cluster-update-console-plugin manifests from install/ into embedded assets (pkg/agenticrun/bindata/). The agenticrun controller conditionally applies them at runtime when it detects the AgenticRun CRD via API discovery, replacing the TechPreviewNoUpgrade annotation gating. This allows the console plugin to deploy on any cluster where OLS agentic is installed, regardless of feature set, while avoiding unnecessary deployments on clusters without OLS. Key changes: - Embed 8 plugin manifests via //go:embed, strip gating annotations - Add crdAvailable() with 5-minute cached CRD check via apiextensions - Always start the controller goroutine (self-gating via CRD check) - Add ensureConsolePlugin() with diff-based updates and skip flag - Add cleanupConsolePlugin() when CRD disappears (OLS uninstalled) - Add HyperShift and Console capability guards - Remove shouldEnableAgenticRunController() from cvo.go - Pass plugin image from image-references via SetConsolePluginImage() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/test e2e-hypershift e2e-aws-ovn-techpreview e2e-agnostic-ovn-techpreview-serial-2of3 |
After deploying the console plugin resources, enable it by adding cluster-update-console-plugin to spec.plugins on the console operator config. On cleanup (CRD removed), disable it by removing the entry. This makes the plugin automatically visible in the console without manual patching of the console operator config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| if !c.crdAvailable() { | ||
| if err := disableConsolePlugin(ctx, c.client); err != nil { | ||
| klog.V(i.Normal).Infof("Failed to disable console plugin: %v", err) |
There was a problem hiding this comment.
If we fail to disable the console plugin, we want to avoid cleaning up those manifests, right? I'm not clear on all the conditions where disabling the console plugin would fail, but cleaning out the backing resources while the console plugin was still requested in the console configuration seems like it might cause console-loading problems.
| if c.shouldDeployConsolePlugin() && !c.consolePluginEnsured { | ||
| if err := c.ensureConsolePlugin(ctx); err != nil { | ||
| klog.V(i.Normal).Infof("Failed to ensure console plugin: %v", err) | ||
| } else if err := enableConsolePlugin(ctx, c.client); err != nil { |
There was a problem hiding this comment.
Make sure the Operator finishes installing the dynamic plugin before you run the following patch command.
It's not entirely clear what "finishes installing" means. Is it "wait until at least one Pod that backs the Service is Ready=True"? Or how gracefully does the console handle the enabled ConsolePlugin being down? I'm noticing now that we don't set replicas in the Deployment manifest, so it defaults to 1. And we don't have a covering PodDisruptionBudget (yet!). So I imagine we're often down for a bit (and possibly for an extended period) while Nodes are being drained and whatnot, and if that is handled fairly smoothly (just our cluster-update page 503ing for a bit, but the console otherwise working?), maybe we don't need anything more sophisticated here to smooth out initial-enablement time?
| return nil | ||
| } | ||
|
|
||
| if c.shouldDeployConsolePlugin() && !c.consolePluginEnsured { |
There was a problem hiding this comment.
!c.consolePluginEnsured here assumes our local-memory-cached consolePluginEnsured is accurate. Possibly fine for a tech-preview offering. But before we hit production, we'll probably want something that more actively reconciles the owned resources (like the sync worker manifest reconciliation this pull is moving us away from]) that will notice and stomp any concerning changes that leak into these managed resources (e.g. from careless admins). Hmm, and we'll also want to look at the other knobs that drive manifest inclusion like ClusterVersion's spec.overrides, to allow careful admins to wave off the CVO when we're doing a poor job of managing a particular resource and the cluster admin wants to take over until they can update to pick up a fix.
|
/retest |
- Skip manifest cleanup if disableConsolePlugin fails, to avoid removing backing resources while the plugin is still registered in the console config (wking review openshift#1) - Wait for at least one available replica before enabling the plugin in the console config, so the console doesn't try to load a plugin whose backing pod isn't ready yet (wking review openshift#2) - consolePluginEnsured remains a TP-acceptable volatile cache; active reconciliation of owned resources is deferred to pre-GA (wking openshift#3) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/retest |
1 similar comment
|
/retest |
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
There are some outstanding nits, but nothing here that's so serious that I see a reason to block this tech-preview feature work. The oc adm upgrade recommend output divergence is also unrelated to this pull.
/override ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3
/lgtm
/verified by CI
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: jhadvig, 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
|
@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. |
|
@jhadvig: 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. |
4 participants
Summary
install/into embedded assets (pkg/agenticrun/bindata/)Why
The OCPSTRAT-2241 descoping decision requires the feature to be TP but not
TechPreviewNoUpgrade. The plugin should install on any cluster where the lightspeed-agentic-operator provides the AgenticRun CRD, regardless of feature set.Keeping manifests in
install/without the TP annotation would deploy the plugin on every cluster, wasting resources. This PR follows theconsole-operatorbindata pattern: embed manifests via//go:embed, apply them conditionally from the agenticrun controller.Changes
Bindata structure (
pkg/agenticrun/bindata/):assets.go—//go:embed assets/*wrapperassets/*.yaml— 7 plugin manifests (namespace, serviceaccount, networkpolicy, configmap, deployment, service, consoleplugin) with gating annotations stripped and${IMAGE}placeholder for the deployment imageController (
pkg/agenticrun/controller.go):crdAvailable()— checks foragenticruns.agentic.openshift.iovia dynamic client with 5-minute TTL cacheensureConsolePlugin()— loads embedded manifests, substitutes image, applies via controller-runtime clientcleanupConsolePlugin()— deletes plugin resources when CRD is absent (OLS uninstalled)consolePluginEnsuredflag — skips redundant applies on subsequent syncsSetConsolePluginImage()— receives image fromimage-references, resets ensured flag on changeCVO wiring (
pkg/cvo/cvo.go):shouldEnableAgenticRunController()— CRD check is now inside the controller'sSync()image-referencesinInitializeFromPayload()Queue management (
availableupdates.go,status.go):shouldEnableAgenticRunController()guards — controller queue always receives itemsSync() flow
Test plan
go test ./pkg/agenticrun/ ./pkg/cvo/)make build)🤖 Generated with Claude Code
Summary by CodeRabbit