cnv/virt-cluster-validate: Install dependency operators and Azure RWX storage - #82726
cnv/virt-cluster-validate: Install dependency operators and Azure RWX storage#82726tiraboschi wants to merge 1 commit into
Conversation
WalkthroughAzure and GCP validation workflows now install prerequisite operators from a v4.22 catalog. Azure provisions Azure Files NFS storage and enables host-network and live-migration checks. GCP enables host-network checks. Both workflows archive JUnit results. ChangesCloud validation workflows
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tiraboschi 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml`:
- Around line 39-45: Remove the skip_checking property from every operator entry
in the OPERATORS definitions at
ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml
lines 39-45 and lines 153-159, including the Azure and GCP operator entries, so
installation failures stop validation.
🪄 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: 223d21c4-bf74-45b4-a12c-e5ededdb00b4
📒 Files selected for processing (1)
ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml
6e2d012 to
b607717
Compare
|
/pj-rehearse |
|
@tiraboschi: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
… storage Pre-install nmstate, OADP, NHC, and SNR operators via the install-operators step-registry ref on both e2e-azure and e2e-gcp lanes. This reduces test skips (OADP/backup) and warnings (node remediation) that previously fired because these operators were absent. Since OCP 5.0 nightlies may not have all operators in the default redhat-operators catalog, a custom CatalogSource is created from the v4.22 redhat-operator-index (the last released OCP version). On Azure, create an azurefile-csi-nfs StorageClass annotated as the default virt class (storageclass.kubevirt.io/is-default-virt-class), enabling the live-migration check that was previously skipped due to RWO-only storage. Also surface junit-results.xml in CI artifacts for Spyglass rendering. GCP RWX storage is left for a follow-up: it requires the gcp-filestore-csi-driver-operator subscription, a ClusterCSIDriver object, and a storage-destroy-csi-gcp-filestore post step for cleanup. Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
b607717 to
33f621c
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
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
`@ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml`:
- Line 115: Remove the trailing `|| true` from the storage-class clearing
command so failures from `oc get` or `oc patch` propagate and stop validation.
Update the command around the `oc get storageclass` pipeline while preserving
the existing patch operation and annotation target.
🪄 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: 8f12653f-2324-4458-9308-4965de11272b
📒 Files selected for processing (1)
ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml
| cli: latest | ||
| commands: | | ||
| set -euo pipefail | ||
| oc get storageclass -o name | xargs -I{} oc patch {} -p '{"metadata":{"annotations":{"storageclass.kubevirt.io/is-default-virt-class":"false"}}}' --type=merge 2>/dev/null || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not ignore failures while clearing default virtualization classes.
|| true suppresses failures from both oc get and oc patch. If a patch fails, an existing class can remain the default virtualization class while azurefile-csi-nfs is also marked default. The live-migration validation can then select an unintended RWO class.
Proposed fix
- oc get storageclass -o name | xargs -I{} oc patch {} -p '{"metadata":{"annotations":{"storageclass.kubevirt.io/is-default-virt-class":"false"}}}' --type=merge 2>/dev/null || true
+ oc get storageclass -o name | while IFS= read -r storage_class; do
+ oc patch "${storage_class}" \
+ -p '{"metadata":{"annotations":{"storageclass.kubevirt.io/is-default-virt-class":"false"}}}' \
+ --type=merge
+ done📝 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.
| oc get storageclass -o name | xargs -I{} oc patch {} -p '{"metadata":{"annotations":{"storageclass.kubevirt.io/is-default-virt-class":"false"}}}' --type=merge 2>/dev/null || true | |
| oc get storageclass -o name | while IFS= read -r storage_class; do | |
| oc patch "${storage_class}" \ | |
| -p '{"metadata":{"annotations":{"storageclass.kubevirt.io/is-default-virt-class":"false"}}}' \ | |
| --type=merge | |
| done |
🤖 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
`@ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml`
at line 115, Remove the trailing `|| true` from the storage-class clearing
command so failures from `oc get` or `oc patch` propagate and stop validation.
Update the command around the `oc get storageclass` pipeline while preserving
the existing patch operation and annotation target.
There was a problem hiding this comment.
The || true is intentional here: this is a best-effort cleanup that clears the is-default-virt-class annotation from any existing StorageClasses before creating the new one. If no StorageClasses have this annotation yet (a possible case on a fresh CI cluster), oc patch would otherwise fail with a no-op error. The script already has set -euo pipefail, so the critical commands after this line (the oc create) will still fail the step if something is actually wrong. Removing || true would cause spurious failures on clean clusters.
There was a problem hiding this comment.
@tiraboschi, thanks for the clarification.
The absence of storageclass.kubevirt.io/is-default-virt-class does not cause this merge patch to fail. oc patch --type=merge adds the annotation when it is absent.
The concern is instead that || true also hides a failed oc get storageclass or a failed patch of an existing StorageClass. In that case, an existing class can remain the default virtualization class together with azurefile-csi-nfs.
The cleanup can remain best effort if multiple default virtualization classes are safe for the validation. Otherwise, the command should ignore only the expected empty-input case and propagate API and patch failures. I will keep the finding open.
✏️ Learnings added
Learnt from: tiraboschi
Repo: openshift/release PR: 82726
File: ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml:115-115
Timestamp: 2026-07-31T15:22:59.275Z
Learning: In `ci-operator/config/openshift-cnv/virt-cluster-validate/openshift-cnv-virt-cluster-validate-main.yaml`, the `setup-rwx-storage` step intentionally performs best-effort cleanup of the `storageclass.kubevirt.io/is-default-virt-class` annotation before it creates the `azurefile-csi-nfs` StorageClass.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
/pj-rehearse |
|
@tiraboschi: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@tiraboschi: The following test 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. |
1 participant
Pre-install nmstate, OADP, NHC, and SNR operators via the install-operators step-registry ref on both e2e-azure and e2e-gcp lanes. This reduces test skips (OADP/backup) and warnings (node remediation) that previously fired because these operators were absent.
On Azure, create an azurefile-csi-nfs StorageClass annotated as the default virt class (storageclass.kubevirt.io/is-default-virt-class), enabling the live-migration check that was previously skipped due to RWO-only storage.
Also surface junit-results.xml in CI artifacts for Spyglass rendering.
GCP RWX storage is left for a follow-up: it requires the gcp-filestore-csi-driver-operator subscription, a ClusterCSIDriver object, and a storage-destroy-csi-gcp-filestore post step for cleanup.
Summary by CodeRabbit
OPERATORSenvironment variable.setup-rwx-storagestep that creates anazurefile-csi-nfsStorageClass and marks it as the default virtualization class to enable live-migration validation checks.11-host-networkfrom the SKIP_CHECKS list.70-live-migrationfrom the SKIP_CHECKS list; the GCP workflow continues to skip this check pending RWX storage setup in a follow-up change.junit-results.xmlto CI artifacts in both workflows for Spyglass rendering.