OTA-1956: testing install: Split multi-document YAML and add missing annotations#1398
Open
jhadvig wants to merge 10 commits into
Open
OTA-1956: testing install: Split multi-document YAML and add missing annotations#1398jhadvig wants to merge 10 commits into
jhadvig wants to merge 10 commits into
Conversation
The console folks are pushing to decentralize console implementation from [1], so we've created a new console plugin for cluster updates [2]. It's built by both CI [3] and ART [4]. Checking on app.ci ImageStreams: $ oc whoami -c default/api-ci-l2s4-p1-openshiftapps-com:6443/wking $ oc -n ocp get -o json imagestream 5.0 | jq -r '.status.tags[] | select(.tag == "cluster-update-console-plugin").items[] | .created + " " + .image' 2026-05-01T20:55:38Z sha256:10e4f1b5763f40372823173b2a9528777ff8e97d416c5447e10df023c0e35656 2026-04-29T05:00:56Z sha256:b0433455cbbff13bdda03ee78371e18c139adebc00432dea716e8cdf83eeb042 2026-04-17T11:10:31Z sha256:e1296b64ffb35757fb2fb56bb5dd9cbd55c7f17f9f59c0a10a2d71a0ad6702d3 $ oc -n ocp get -o json imagestream 5.0-art-latest | jq -r '.status.tags[] | select(.tag == "cluster-update-console-plugin").items[] | .created + " " + .image' 2026-05-12T19:45:33Z sha256:b943be0ae0eba97c27741d0184e99a77ea928749cc578ae7e17a8e5329652642 2026-05-12T14:42:55Z sha256:c29ba37ef5a426de5320d680d3fb58befc530274e6df4c32b4dc4fd0acaaaae0 2026-05-12T09:02:35Z sha256:901bc6aac1142fe1da2a756bb4d91ae8fe14b459ce2bf9904ceae6d2fc818fc2 2026-05-12T04:38:56Z sha256:1f4e8b200d97f82db1784b4d7fc9f0bd3ccaf2cc664fd5f0ff6b80485da16950 2026-05-11T22:54:34Z sha256:d369ca7c73d7a3abe159e9e6f5644f63e0b091f0b75f202773a023e91c7faaf6 This commit sets up an image-references file [5], so 'oc adm release new ...' knows that we'll want that image injected in the Deployment manifest. I'm using placeholder.url.oc.will.replace.this.example.org as part of my placeholder name. That's similar to the machine-config operator's use of placeholder.url.oc.will.replace.this.org [6], except that I'm using a subdomain of the reserved example.com [7], to avoid any possible confusion with an actually in-use domain. The new manifests are in run-level 50, which is the default, so they can roll out in parallel with other components to avoid slowing updates. The new manifests are tried to the Console capability [8] and the TechPreviewNoUpgrade feature set [9] (in the absence of a specific feature gate for this functionality). I'm just carrying the old exclude.release.openshift.io/internal-openshift-hosted annotation over from other CVO manifests. It predates cluster profiles [10], and I'm not sure anyone still uses it, but it seems like the CVO should be consistent about whether it matters or not anymore. Perhaps we can drop it from all CVO manifests in follow-up work. Otherwise these manifests are loosely based on my attempts to meld the plugin's Help chart templates [11] with existing CVO manifest conventions. [1]: https://github.com/openshift/console [2]: https://github.com/openshift/cluster-update-console-plugin [3]: openshift/release#77945 [4]: openshift-eng/ocp-build-data#10393 [5]: https://github.com/openshift/enhancements/blob/4f67eee19ad16f1d5e9e8a2622b708e2ea6d8e6a/dev-guide/cluster-version-operator/dev/operators.md#how-do-i-ensure-the-right-images-get-used-by-my-manifests [6]: https://github.com/openshift/machine-config-operator/blob/99cb8a46e6a31b2b72d6a8371c6cd4ee45393263/install/image-references#L10 [7]: https://www.rfc-editor.org/rfc/rfc6761#section-6.5 [8]: https://github.com/openshift/enhancements/blob/4f67eee19ad16f1d5e9e8a2622b708e2ea6d8e6a/enhancements/installer/component-selection.md#manifest-annotations [9]: https://github.com/openshift/enhancements/blob/4f67eee19ad16f1d5e9e8a2622b708e2ea6d8e6a/enhancements/update/cvo-techpreview-manifests.md#proposal [10]: https://github.com/openshift/enhancements/blob/4f67eee19ad16f1d5e9e8a2622b708e2ea6d8e6a/enhancements/update/ibm-public-cloud-support.md#cluster-version-operator-changes-for-beta [11]: https://github.com/openshift/cluster-update-console-plugin/tree/9778f4fc0c19e60cad55a45591a066b6b7a3cb12/charts/openshift-console-plugin/templates
Avoid:
$ go test ./pkg/payload
...
--- FAIL: Test_cvoManifests (0.02s)
--- FAIL: Test_cvoManifests/install_dir (0.02s)
render_test.go:355: failed to load manifests: error parsing: error unmarshaling JSON: while decoding JSON: Resource with fields Group: "image.openshift.io" Kind: "ImageStream" Name: "" must contain kubernetes required fields kind and name
...
These image-refernces files are helpers for 'oc adm release new ...',
they don't need all the properties set that they'd need to be pushed
into a cluster.
…Users false Cluster Bot 'launch 5.0.0-0.ci gcp,techpreview' [1]. Also asked for a release image that has my change via Cluster Bot 'build 5.0.0-0.ci,openshift#1388' [2]. Launch the update: $ oc adm release info registry.build10.ci.openshift.org/ci-ln-rmnm9yt/release:latest | grep Pull Pull From: registry.build10.ci.openshift.org/ci-ln-rmnm9yt/release@sha256:2cdcefbd1857a6f58a538e1ccc4460c89798318c35b9d51a2d2ff74d05e2fc1f $ oc adm upgrade --force --allow-explicit-upgrade --to-image registry.build10.ci.openshift.org/ci-ln-rmnm9yt/release@sha256:2cdcefbd1857a6f58a538e1ccc4460c89798318c35b9d51a2d2ff74d05e2fc1f --force because the CI image is unsigned, and --allow-explicit-upgrade because it is not recommended by an Update Service, neither one would be something I'd recommend outside of testing on a throw-away cluster. gather-extra artifacts in the run [3] show the Deployment struggling: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/release-openshift-origin-installer-launch-gcp-modern/2060016448293572608/artifacts/launch/gather-extra/artifacts/clusterversion.json | jq -r '.items[].status.conditions[] | select(.type == "Failing").message' deployment openshift-cluster-update-console-plugin/cluster-update-console-plugin has a replica failure FailedCreate: pods "cluster-update-console-plugin-547878cc4d-" is forbidden: unable to validate against any security context constraint: provider restricted-v3: .spec.hostUsers: Invalid value: null: Host Users must be set to false [1]: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/release-openshift-origin-installer-launch-gcp-modern/2060016448293572608 [2]: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/release-openshift-origin-installer-launch-aws-modern/2060017269097893888 [3]: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/release-openshift-origin-installer-launch-gcp-modern/2060016448293572608/artifacts/launch/gather-extra/artifacts/
…ra nodeSelector Infra Nodes are optional, but not required, and CI clusters don't have them, e.g. [1,2]: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2/2059320133616144384/artifacts/e2e-aws-ovn-serial/gather-extra/artifacts/nodes.json | jq -r '.items[].metadata.labels' | grep node-role | sort | uniq -c 3 "node-role.kubernetes.io/control-plane": "", 3 "node-role.kubernetes.io/master": "", 3 "node-role.kubernetes.io/worker": "", Without this change, the Pods fail to schedule on that kind of CI cluster: $ oc -n openshift-cluster-update-console-plugin get -o json pods | jq -r '.items[].status.conditions[].message' 0/6 nodes are available: 3 node(s) didn't match Pod's node affinity/selector, 3 node(s) had untolerated taint(s). no new claims to deallocate, preemption: 0/6 nodes are available: 6 Preemption is not helpful for scheduling. We'll probably need to have the CVO manage this Deployment more actively, so it can set useful nodeSelector on clusters which do have infra Nodes. But for now, just drop the selector. [1]: https://amd64.ocp.releases.ci.openshift.org/releasestream/5-dev-preview/release/5.0.0-ec.2 [2]: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2/2059320133616144384
The servicemonitor manifest contained both ServiceMonitor and PrometheusRule in a single file separated by `---`. The kube-apiserver manifest renderer cannot decode multi-document YAML, causing bootstrap to fail with "invalid character '-' after top-level value". Split into separate files and add the missing capability.openshift.io/name and release.openshift.io/feature-set annotations to match the other console-plugin manifests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds a new cluster-update-console-plugin, threads ImageStream refs into manifest rendering, creates namespace/network policy, Deployment and Service, adds Prometheus ServiceMonitor and alerting rules, and registers the plugin with the OpenShift console. ChangesCluster Update Console Plugin
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jhadvig The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
jhadvig
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
install/0000_50_cluster-update-console-plugin_81_prometheusrule.yaml (1)
44-51: 💤 Low valueConsider adding a
for:duration to prevent transient alerts.The
UpdateAvailablealert fires immediately whencluster_version_available_updates > 0. While this may be intentional for prompt notification, a shortfor:duration (e.g., 5m) would prevent transient fluctuations from generating alert noise.♻️ Suggested addition
expr: | sum by (channel, namespace, upstream) (cluster_version_available_updates) > 0 + for: 5m labels: severity: info🤖 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/0000_50_cluster-update-console-plugin_81_prometheusrule.yaml` around lines 44 - 51, The UpdateAvailable PrometheusRule alert currently fires immediately; open the alert definition for alert: UpdateAvailable and add a for: duration (e.g., for: 5m) directly under the alert metadata so the rule only transitions to firing after the condition (sum by (channel, namespace, upstream) (cluster_version_available_updates) > 0) holds for that sustained period; update the alert block (alert: UpdateAvailable, expr: ...) to include the new for: field.
🤖 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/0000_50_cluster-update-console-plugin_20_networkpolicy.yaml`:
- Around line 13-16: The NetworkPolicy currently uses podSelector: {} with
policyTypes: - Ingress and - Egress which creates a default-deny for all pods;
update the policy to either target specific pods instead of podSelector: {}
(e.g., use a labelSelector matching the console-plugin pods) or add explicit
allow rules for required traffic (ingress rules to permit backend/monitoring
sources and egress rules to permit upstream destinations) so the console plugin
service and monitoring can communicate; reference the podSelector and
policyTypes (Ingress, Egress) in the NetworkPolicy being modified and ensure the
new rules explicitly allow the necessary CIDRs/namespaceSelectors/port/protocols
for plugin traffic.
In `@install/0000_50_cluster-update-console-plugin_50_deployment.yaml`:
- Around line 37-46: The plugin container currently defines resources.requests
but no resources.limits and its securityContext lacks readOnlyRootFilesystem;
update the container spec for the container named "plugin" to add a
resources.limits block (set cpu and memory limits equal to or higher than the
existing requests, e.g., cpu and memory) and set
securityContext.readOnlyRootFilesystem: true; ensure you modify the container
entry that contains resources.requests and securityContext so both limits and
readOnlyRootFilesystem are present and compliant.
- Around line 30-37: The pod spec for the container named "plugin" is missing
liveness and readiness probes; add both probes under the container "plugin" that
use the existing port/name (https / containerPort 9001) — for example configure
an httpGet (or tcpSocket) probe hitting a health endpoint (e.g. /health or
/ready) on port 9001, set sensible initialDelaySeconds, periodSeconds and
failureThreshold values, and include identical or appropriately different probe
settings for liveness (to restart unhealthy containers) and readiness (to gate
service traffic) so the deployment meets the Liveness + readiness probes
requirement.
In `@install/0000_50_cluster-update-console-plugin_60_service.yaml`:
- Around line 6-11: The Service manifest for the cluster-update console plugin
is missing the serving-cert request annotation; add the annotation key
service.beta.openshift.io/serving-cert-secret-name with value
"cluster-update-console-plugin-cert" to the Service's metadata.annotations so
the cluster will provision the same secret that the Deployment mounts
(secretName: cluster-update-console-plugin-cert), ensuring the TLS secret is
created for the pods waiting on /var/cert.
In `@install/0000_50_cluster-update-console-plugin_80_servicemonitor.yaml`:
- Around line 1-27: The ServiceMonitor in this manifest uses the same
metadata.name and namespace as the cluster-version-operator ServiceMonitor,
causing the later 0000_90 manifest to overwrite annotations; fix by giving this
console plugin ServiceMonitor a unique metadata.name (e.g., append "-console" or
similar) and ensure any related references (labels/selectors like
metadata.labels k8s-app and selector.matchLabels) remain correct, or
alternatively merge the annotations (capability.openshift.io/name and
release.openshift.io/feature-set) into the later ServiceMonitor so they are not
lost when 0000_90 is applied.
In `@install/0000_50_cluster-update-console-plugin_81_prometheusrule.yaml`:
- Around line 1-13: This PrometheusRule duplicates an existing PrometheusRule
with metadata.name "cluster-version-operator" in namespace
"openshift-cluster-version" and lacks runbookUrl fields and proper
last_over_time wrapping; remove or rename this duplicate resource (or merge its
alerts into the existing PrometheusRule) so only one PrometheusRule with kind
PrometheusRule and name "cluster-version-operator" exists for that namespace,
and update each alert expression to wrap range queries with
last_over_time(...[5m]) and add appropriate runbookUrl annotations for each
alert to preserve Console/feature-set gating annotations (ensure the resource
that remains includes the Console gating annotations shown in
metadata.annotations).
In `@install/0000_50_cluster-update-console-plugin_90_consoleplugin.yaml`:
- Line 20: The manifest sets backend.service.port to the string "https" but
ConsolePluginService.port is an int32 and the actual Service listens on 9001;
change the backend.service.port value from "https" to the numeric 9001 so it
matches ConsolePluginService.port and the Service target port. Update the field
named backend.service.port in the ConsolePlugin YAML to the integer 9001 to
align with the Service configuration.
- Around line 18-19: The ConsolePlugin manifest sets spec.backend.service.name
to cluster-update-console-plugin which doesn't match the actual Service resource
name openshift-cluster-update-console-plugin; update the ConsolePlugin's
spec.backend.service.name to "openshift-cluster-update-console-plugin" (or
alternatively rename the Service to "cluster-update-console-plugin") so the
ConsolePlugin backend service name and the Service resource name match; check
the ConsolePlugin field spec.backend.service.port.targetPort and the Service
port too to ensure ports align after renaming.
---
Nitpick comments:
In `@install/0000_50_cluster-update-console-plugin_81_prometheusrule.yaml`:
- Around line 44-51: The UpdateAvailable PrometheusRule alert currently fires
immediately; open the alert definition for alert: UpdateAvailable and add a for:
duration (e.g., for: 5m) directly under the alert metadata so the rule only
transitions to firing after the condition (sum by (channel, namespace, upstream)
(cluster_version_available_updates) > 0) holds for that sustained period; update
the alert block (alert: UpdateAvailable, expr: ...) to include the new for:
field.
🪄 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: a03fc91b-d22b-41d8-8ed5-8cf949246c0e
📒 Files selected for processing (9)
install/0000_50_cluster-update-console-plugin_10_namespace.yamlinstall/0000_50_cluster-update-console-plugin_20_networkpolicy.yamlinstall/0000_50_cluster-update-console-plugin_50_deployment.yamlinstall/0000_50_cluster-update-console-plugin_60_service.yamlinstall/0000_50_cluster-update-console-plugin_80_servicemonitor.yamlinstall/0000_50_cluster-update-console-plugin_81_prometheusrule.yamlinstall/0000_50_cluster-update-console-plugin_90_consoleplugin.yamlinstall/image-referencespkg/payload/render_test.go
Comment on lines
+13
to
+16
| podSelector: {} | ||
| policyTypes: | ||
| - Ingress | ||
| - Egress |
There was a problem hiding this comment.
Default-deny policy blocks plugin traffic without companion allow policies.
podSelector: {} + Ingress/Egress with no rules denies all traffic. As-is, backend access to the plugin service (and monitoring ingress) will be blocked unless additional allow policies are added in this namespace.
🤖 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/0000_50_cluster-update-console-plugin_20_networkpolicy.yaml` around
lines 13 - 16, The NetworkPolicy currently uses podSelector: {} with
policyTypes: - Ingress and - Egress which creates a default-deny for all pods;
update the policy to either target specific pods instead of podSelector: {}
(e.g., use a labelSelector matching the console-plugin pods) or add explicit
allow rules for required traffic (ingress rules to permit backend/monitoring
sources and egress rules to permit upstream destinations) so the console plugin
service and monitoring can communicate; reference the podSelector and
policyTypes (Ingress, Egress) in the NetworkPolicy being modified and ensure the
new rules explicitly allow the necessary CIDRs/namespaceSelectors/port/protocols
for plugin traffic.
Comment on lines
+37
to
+46
| resources: | ||
| requests: | ||
| cpu: 20m | ||
| memory: 50Mi | ||
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: | ||
| - ALL | ||
| terminationMessagePolicy: FallbackToLogsOnError |
There was a problem hiding this comment.
Add container limits and read-only root filesystem.
Container plugin has requests but no limits, and readOnlyRootFilesystem is not set.
Suggested patch
resources:
requests:
cpu: 20m
memory: 50Mi
+ limits:
+ cpu: 100m
+ memory: 200Mi
securityContext:
allowPrivilegeEscalation: false
+ readOnlyRootFilesystem: true
capabilities:
drop:
- ALLAs per coding guidelines, **/*.{yaml,yml} requires readOnlyRootFilesystem and Resource limits (cpu, memory) on every container.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resources: | |
| requests: | |
| cpu: 20m | |
| memory: 50Mi | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| capabilities: | |
| drop: | |
| - ALL | |
| terminationMessagePolicy: FallbackToLogsOnError | |
| resources: | |
| requests: | |
| cpu: 20m | |
| memory: 50Mi | |
| limits: | |
| cpu: 100m | |
| memory: 200Mi | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| readOnlyRootFilesystem: true | |
| capabilities: | |
| drop: | |
| - ALL | |
| terminationMessagePolicy: FallbackToLogsOnError |
🤖 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/0000_50_cluster-update-console-plugin_50_deployment.yaml` around
lines 37 - 46, The plugin container currently defines resources.requests but no
resources.limits and its securityContext lacks readOnlyRootFilesystem; update
the container spec for the container named "plugin" to add a resources.limits
block (set cpu and memory limits equal to or higher than the existing requests,
e.g., cpu and memory) and set securityContext.readOnlyRootFilesystem: true;
ensure you modify the container entry that contains resources.requests and
securityContext so both limits and readOnlyRootFilesystem are present and
compliant.
Comment on lines
+1
to
+27
| apiVersion: monitoring.coreos.com/v1 | ||
| kind: ServiceMonitor | ||
| metadata: | ||
| labels: | ||
| k8s-app: cluster-version-operator | ||
| name: cluster-version-operator | ||
| namespace: openshift-cluster-version | ||
| annotations: | ||
| kubernetes.io/description: Configure Prometheus to monitor cluster-version operator metrics. | ||
| capability.openshift.io/name: Console | ||
| release.openshift.io/feature-set: TechPreviewNoUpgrade | ||
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| spec: | ||
| endpoints: | ||
| - interval: 30s | ||
| port: metrics | ||
| scheme: https | ||
| tlsConfig: | ||
| serverName: cluster-version-operator.openshift-cluster-version.svc | ||
| scrapeClass: tls-client-certificate-auth | ||
| namespaceSelector: | ||
| matchNames: | ||
| - openshift-cluster-version | ||
| selector: | ||
| matchLabels: | ||
| k8s-app: cluster-version-operator |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for ServiceMonitor resources with the same name/namespace
echo "=== ServiceMonitors named 'cluster-version-operator' ==="
rg -l "name: cluster-version-operator" --glob '*servicemonitor*.yaml' install/
echo ""
echo "=== Feature-set annotations in matching files ==="
for f in $(fd -e yaml servicemonitor install/); do
if rg -q "name: cluster-version-operator" "$f"; then
echo "--- $f ---"
rg "release.openshift.io/feature-set|name:|namespace:" "$f" | head -10
fi
doneRepository: openshift/cluster-version-operator
Length of output: 1174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
install/0000_50_cluster-update-console-plugin_80_servicemonitor.yaml
install/0000_90_cluster-version-operator_02_servicemonitor.yaml
)
for f in "${files[@]}"; do
echo "===== $f ====="
# Print only metadata/annotations area (first ~80 lines around 'annotations:')
if rg -n "annotations:" "$f" >/dev/null; then
start=$(rg -n "annotations:" "$f" | head -1 | cut -d: -f1)
# show 40 lines starting at annotations
sed -n "$start,$((start+60))p" "$f" | cat -n
else
echo "(no annotations key found)"
fi
echo
doneRepository: openshift/cluster-version-operator
Length of output: 5204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
install/0000_50_cluster-update-console-plugin_80_servicemonitor.yaml
install/0000_90_cluster-version-operator_02_servicemonitor.yaml
)
for f in "${files[@]}"; do
echo "===== $f ====="
if rg -n "annotations:" "$f" >/dev/null; then
start=$(rg -n "annotations:" "$f" | head -1 | cut -d: -f1)
sed -n "$start,$((start+80))p" "$f" | cat -n
else
echo "(no annotations key found)"
fi
echo
doneRepository: openshift/cluster-version-operator
Length of output: 6778
Fix duplicate ServiceMonitor name/namespace to avoid annotation overwrite
install/0000_50_cluster-update-console-plugin_80_servicemonitor.yaml and install/0000_90_cluster-version-operator_02_servicemonitor.yaml both define a ServiceMonitor named cluster-version-operator in openshift-cluster-version. Since 0000_90 is applied after 0000_50, it will replace the resource and drop capability.openshift.io/name: Console and release.openshift.io/feature-set: TechPreviewNoUpgrade from the console plugin manifest. Use a unique ServiceMonitor name for the console plugin (or merge the required annotations into the later manifest).
🤖 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/0000_50_cluster-update-console-plugin_80_servicemonitor.yaml` around
lines 1 - 27, The ServiceMonitor in this manifest uses the same metadata.name
and namespace as the cluster-version-operator ServiceMonitor, causing the later
0000_90 manifest to overwrite annotations; fix by giving this console plugin
ServiceMonitor a unique metadata.name (e.g., append "-console" or similar) and
ensure any related references (labels/selectors like metadata.labels k8s-app and
selector.matchLabels) remain correct, or alternatively merge the annotations
(capability.openshift.io/name and release.openshift.io/feature-set) into the
later ServiceMonitor so they are not lost when 0000_90 is applied.
The ConsolePlugin referenced service name "cluster-update-console-plugin" but the actual Service is named "openshift-cluster-update-console-plugin", preventing the console from reaching the plugin backend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add the service.beta.openshift.io/serving-cert-secret-name annotation to the Service so the service-ca operator provisions the TLS secret that the Deployment mounts at /var/cert. Without it the pod gets stuck waiting for the secret volume. Also fix the ConsolePlugin backend port from string "https" to numeric 9001, since ConsolePluginService.Port is int32. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CVO manifests in /manifests/ use Go template rendering but only had
access to ReleaseImage and ClusterProfile variables. Manifests that
need to reference other component images (like cluster-update-console-
plugin) had no way to resolve placeholder image URLs at deploy time.
Add an Images map to manifestRenderConfig, populated from the release
payload's /release-manifests/image-references ImageStream. CVO manifests
can now use {{index .Images "component-name"}} to reference any image
in the release.
Remove install/image-references since it was not processed from
/manifests/ by oc adm release new, and the image is already registered
in the release's image-references via CI/ART configuration.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/payload/render.go (1)
195-207: ⚡ Quick winConsider validating that image URIs are non-empty.
The function does not check whether
tag.From.Nameis non-empty before inserting into the map. If an ImageStream tag has an emptyDockerImagereference, templates using{{index .Images "tag-name"}}would render with an empty string, causing invalid manifest application.🛡️ Proposed validation
func imagesFromImageRef(imageRef *imagev1.ImageStream) map[string]string { images := make(map[string]string) if imageRef == nil { return images } for _, tag := range imageRef.Spec.Tags { - if tag.From != nil && tag.From.Kind == "DockerImage" { + if tag.From != nil && tag.From.Kind == "DockerImage" && tag.From.Name != "" { images[tag.Name] = tag.From.Name } } return images }🤖 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/payload/render.go` around lines 195 - 207, imagesFromImageRef currently inserts tag.From.Name into the images map without checking for emptiness; update imagesFromImageRef to skip tags where tag.From == nil or tag.From.Kind != "DockerImage" or tag.From.Name is empty (do not add entries with an empty URI), and optionally record/debug-log skipped tags if desired; reference the function imagesFromImageRef and the fields tag.From, tag.From.Kind and tag.From.Name when making the change.pkg/payload/render_test.go (1)
316-318: ⚡ Quick winConsider adding unit tests for
imagesFromImageRef.While
Test_cvoManifestsexercises the Images map indirectly through rendering, there's no direct unit test for theimagesFromImageRefhelper function. A table-driven test would document expected behavior and improve confidence for edge cases such as nil imageRef, empty tags, non-DockerImage kinds, and duplicate tag names.🤖 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/payload/render_test.go` around lines 316 - 318, Add a new table-driven unit test for the helper function imagesFromImageRef (e.g., Test_imagesFromImageRef) in pkg/payload/render_test.go to cover nil imageRef, empty tags, non-DockerImage kinds, and duplicate tag names; for each case construct an imageRef input, call imagesFromImageRef, and assert the returned map matches expected keys/values (including handling duplicates and empty tags), keeping Test_cvoManifests as-is but ensuring the new test directly verifies imagesFromImageRef behavior and edge cases.
🤖 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/payload/render.go`:
- Around line 42-47: The code currently silences errors from loadImageReferences
causing renderConfig.Images to be empty and templates that reference .Images
(e.g., install/0000_50_cluster-update-console-plugin_50_deployment.yaml) to
render invalid empty image fields; update the logic around loadImageReferences
in pkg/payload/render.go so that if loadImageReferences(releaseManifestsDir)
returns an error you propagate/return that error from the rendering path (or
explicitly gate and return an error when templates require .Images), rather than
only logging a warning — ensure renderConfig.Images is only left unset when that
is acceptable and otherwise call imagesFromImageRef(imageRef) and fail fast when
image refs cannot be loaded.
---
Nitpick comments:
In `@pkg/payload/render_test.go`:
- Around line 316-318: Add a new table-driven unit test for the helper function
imagesFromImageRef (e.g., Test_imagesFromImageRef) in pkg/payload/render_test.go
to cover nil imageRef, empty tags, non-DockerImage kinds, and duplicate tag
names; for each case construct an imageRef input, call imagesFromImageRef, and
assert the returned map matches expected keys/values (including handling
duplicates and empty tags), keeping Test_cvoManifests as-is but ensuring the new
test directly verifies imagesFromImageRef behavior and edge cases.
In `@pkg/payload/render.go`:
- Around line 195-207: imagesFromImageRef currently inserts tag.From.Name into
the images map without checking for emptiness; update imagesFromImageRef to skip
tags where tag.From == nil or tag.From.Kind != "DockerImage" or tag.From.Name is
empty (do not add entries with an empty URI), and optionally record/debug-log
skipped tags if desired; reference the function imagesFromImageRef and the
fields tag.From, tag.From.Kind and tag.From.Name when making the change.
🪄 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: b86d3e40-c869-4f98-bc00-cff997f23625
📒 Files selected for processing (4)
install/0000_50_cluster-update-console-plugin_50_deployment.yamlpkg/payload/payload.gopkg/payload/render.gopkg/payload/render_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- install/0000_50_cluster-update-console-plugin_50_deployment.yaml
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
The image-references file was incorrectly removed based on the assumption that cluster-update-console-plugin was already in the release's image-references via CI/ART configuration. It is not — the image exists in the CI ImageStream but is not yet wired into the release payload's image-references. Restore the file to preserve the declaration of intent while the release-side wiring is being resolved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jhadvig
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@jhadvig: This pull request references OTA-1956 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. |
- Remove duplicate ServiceMonitor and PrometheusRule that collided with existing CVO monitoring in 0000_90_cluster-version-operator_02_servicemonitor.yaml (same name/namespace, applied later and overwrites) - Add readiness and liveness probes to the plugin container - Add memory limits and readOnlyRootFilesystem - Fail hard when image-references cannot be loaded during rendering instead of silently producing empty image fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@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. |
Member
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the CI failures in #1388 caused by
0000_50_cluster-update-console-plugin_80_servicemonitor.yamlcontaining two YAML documents (ServiceMonitor + PrometheusRule) in a single file separated by---. The kube-apiserver manifest renderer cannot decode multi-document YAML, causing bootstrap to fail with:Changes:
_80_servicemonitor.yamland_81_prometheusrule.yaml)capability.openshift.io/name: Consoleandrelease.openshift.io/feature-set: TechPreviewNoUpgradeannotations to both resources, matching all other console-plugin manifests in this setTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores