Conversation
|
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 QE Software Test Plan for OpenShift Virtualization that specifies multi-architecture (amd64/arm64) VM creation and same-architecture live migration testing on heterogeneous clusters, including scope, topology, environment, tooling, entry/exit criteria, risks, limitations, traceability, and sign-off. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
SamAlber,
jerry7z,
kbidarkar and
vsibirsk
|
/wip |
openshift-virtualization-qe-bot-3
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: 7
🤖 Fix all issues with AI agents
In @stps/sig-virt/multiarch_support.md:
- Around line 38-46: The "Technology and Design Review" table marks all checks
as done but contains generic templated notes; update each row (Developer
Handoff/QE Kickoff, Technology Challenges, Test Environment Needs, API
Extensions, Topology Considerations) with concrete, multi-arch-specific details
(e.g., meeting attendees/outcomes for Developer Handoff/QE Kickoff; specific
architecture challenges and mitigations for Technology Challenges; required
emulator/hardware/configuration and test matrix for Test Environment Needs;
precise API changes, backward-compat concerns and test cases for API Extensions;
cluster/network impact and validation strategy for Topology Considerations), or
if any of those were not actually completed, change the corresponding Done
checkbox from [V] to unchecked to reflect incomplete review.
- Line 109: Update the American English abbreviation style by adding periods to
"etc" in the table entries: change the text "e.g., Specific NICs for SR-IOV, GPU
etc" (and the other occurrence on line 113) to "e.g., Specific NICs for SR-IOV,
GPU etc." so both table cells use "etc." consistently.
- Around line 57-68: Fix the typo "thier" → "their" in the "In Scope" bullet
("Create VMs for both AMD and ARM and confirm the cluster automatically places
them on their respective architecture node.") and remove the placeholder note
under the "Out of Scope (Testing Scope Exclusions)" header; then replace the
example placeholder table row(s) with real out-of-scope items relevant to
multi-arch VM testing (e.g., "Testing with container disk VM" with rationale and
PM/Lead agreement), ensuring the table entries are complete and not left as
instructional placeholders.
- Line 3: Fix the title line by correcting the typo "craetion" to "creation",
change "multi arch" to "multi-arch", and remove or fix the stray trailing
asterisks so the header is valid Markdown; e.g. update the header string "## VM
craetion and Live Migration on a multi arch cluster - Quality Engineering
Plan**" to a properly formatted header such as "## VM creation and Live
Migration on a multi-arch cluster - Quality Engineering Plan".
- Around line 159-171: The Requirement Summary column in the "III. Test
Scenarios & Traceability" table is empty for the listed CNV Jira IDs; fill each
row's Requirement Summary with a concise one-line description of the requirement
for CNV-72102, CNV-74480, CNV-75737, CNV-74481, and CNV-33896 so the
traceability matrix links requirement to test coverage (e.g., CNV-72102:
"Support deployment and validation of multi-arch (ARM64/AMD64) clusters on
4.21", CNV-74480: "Allow amd64 cpuModel updates in HCO while ensuring VMs
schedule/run on ARM nodes", CNV-75737: "Execute Tier2 automated tests across
ARM64 and AMD64 nodes", CNV-74481: "Extend Tier2 automation to detect and handle
multi-arch node differences", CNV-33896: "Run conformance test suite on
multi-arch clusters to verify platform compatibility"). Ensure each summary is
short, specific, and directly maps to the existing Test Scenario for reviewers.
- Around line 128-135: Update the "Entry Criteria" checklist by adding an
explicit tracking reference for the "Multi-CPU architecture support enabled in
openshift-virtualization repo" item: change that bullet to include the relevant
Jira/enhancement link (e.g., reference
[CNV-26818](https://issues.redhat.com/browse/CNV-26818) or the enhancement
document) so the completion of this criterion is clearly tracked; ensure the
text uses the same bullet label ("Multi-CPU architecture support enabled in
openshift-virtualization repo") and simply appends the tracking URL or issue key
in parentheses or as a markdown link.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stps/sig-virt/multiarch_support.md
🧰 Additional context used
🪛 LanguageTool
stps/sig-virt/multiarch_support.md
[grammar] ~3-~3: Ensure spelling is correct
Context: ...t-virtualization-tests Test plan ## VM craetion and Live Migration on a multi arch clus...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...M craetion and Live Migration on a multi arch cluster - Quality Engineering Plan*...
(QB_NEW_EN_HYPHEN)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...he cluster automatically places them on thier respective architecture node. - Test VM...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~109-~109: In American English, abbreviations like “etc.” require a period.
Context: ... | [e.g., Specific NICs for SR-IOV, GPU etc] ...
(ETC_PERIOD)
[style] ~113-~113: In American English, abbreviations like “etc.” require a period.
Context: ... | [e.g., Bare metal, AWS, Azure, GCP etc] ...
(ETC_PERIOD)
[grammar] ~168-~168: Ensure spelling is correct
Context: ...V-75737] | | Run Tier2 Tests on Multi-Arch Clusters (ARM64 and...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~169-~169: Ensure spelling is correct
Context: ...4481] | | Update Tier2 automation to handle Multi-Arch scenari...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (1)
stps/sig-virt/multiarch_support.md (1)
116-126: No action needed—the Testing Tools & Frameworks section is correctly empty per documented instructions.The section explicitly states to "Leave empty if using standard tools" (lines 119-120). Based on Section 3 (Test Strategy), this feature uses standard functional and automation testing approaches. The empty table correctly reflects this and aligns with the template's guidance.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @stps/sig-virt/multiarch_support.md:
- Around line 116-126: Populate the "4.1. Testing Tools & Frameworks" table by
stating whether standard testing infra is sufficient or listing any additional
tools required for multi-arch validation; specifically either add the sentence
"Standard openshift-virtualization testing infrastructure used; no additional
tools required." or, if extra tooling is needed, list items such as "ARM64 test
runners (qemu-user-static or hardware runners), cross-architecture CI jobs,
platform-specific debuggers" under the appropriate table rows (**Test
Framework**, **CI/CD**, **Other Tools**) so the section is no longer empty.
- Around line 159-170: The Requirement Summary column in the traceability matrix
is empty for the listed tickets; update the table rows for [CNV-72102],
[CNV-74480], [CNV-75737], [CNV-74481], and [CNV-33896] by adding a one-line
summary for each JIRA (e.g., CNV-72102: "Deploy and validate multi-arch cluster
on 4.21", CNV-74480: "Validate amd64 cpuModel update and arm64 VM scheduling",
CNV-75737: "Execute Tier2 tests across ARM64 and AMD64", CNV-74481: "Extend
Tier2 automation for multi-arch scenarios", CNV-33896: "Run conformance tests on
ARM64 and AMD64"); ensure each summary is concise, matches the ticket intent,
and placed in the Requirement Summary column for easy reviewer traceability.
- Around line 136-152: The table has placeholder text in the "Test Environment",
"Untestable Aspects", "Resource Constraints", and "Dependencies" rows—replace
each placeholder cell with concrete risk descriptions and matching mitigation
steps (e.g., for "Test Environment" describe required hardware or cluster
constraints and mitigation like reserving nodes or emulating; for "Untestable
Aspects" list scenarios that cannot be validated and mitigation like scale
extrapolation and monitoring; for "Resource Constraints" name people/CI limits
and mitigation such as prioritization or cross-team support; for "Dependencies"
list external teams/PRs and mitigation like sync points or fallback plans) and
populate the "Status" checkbox for each row after stakeholder review/approval;
use the "Code Freeze on Jan 12th" and the PR link example as models for
specificity and include any relevant links or owners.
- Around line 83-97: The table rows labeled "Dependencies" and "Cross
Integrations" currently contain placeholder text; replace them with concrete
assessments naming the specific teams and components (e.g., for Dependencies:
"Depends on CNV storage team for DataSource support; requires multi-arch cluster
provisioning from infra team"; for Cross Integrations: "Storage team tests
DataSource provisioning; Network team validates multi-arch networking; CI team
verifies multi-arch image build and deployment"). Ensure each entry is concise,
actionable, and maps the test responsibility to the owning team or component so
the test strategy clearly shows who validates each dependency and integration.
🧹 Nitpick comments (4)
stps/sig-virt/multiarch_support.md (4)
1-3: Fix formatting issues in title.Line 3 has an extra closing asterisk, and "multi arch" should be hyphenated as "multi-arch" per markdown style conventions.
- ## VM creation and Live Migration on a multi arch cluster - Quality Engineering Plan** + ## VM creation and Live Migration on a multi-arch cluster - Quality Engineering Plan
128-134: Clarify Entry Criteria completion timeline.Lines 132 and 134 have unchecked criteria, which is appropriate for a WIP document. However, the STP should include a note indicating the planned date or milestone for completing these entry criteria before formal testing begins.
99-114: Clean up placeholder text in N/A test environment entries.Lines 108 and 111-114 have "N/A" entries with bracketed example text (e.g., "[e.g., Minimum per worker node: 8 vCPUs, 32GB RAM]"). Remove the bracket notation or replace with actual values if applicable. The instructions at line 101 state "N/A means explicitly not applicable"—the examples should not remain in the final document.
154-157: Consider expanding Known Limitations with workarounds or resolution timeline.The Known Limitations section is concise and clear. For completeness, consider adding:
- Workaround: "Manually specify
spec.template.spec.architecturein VM YAML to override auto-scheduling"- Expected resolution: Reference to related GitHub/JIRA issue tracking this limitation
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stps/sig-virt/multiarch_support.md
🧰 Additional context used
🪛 LanguageTool
stps/sig-virt/multiarch_support.md
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...M creation and Live Migration on a multi arch cluster - Quality Engineering Plan*...
(QB_NEW_EN_HYPHEN)
[grammar] ~168-~168: Ensure spelling is correct
Context: ...V-75737] | | Run Tier2 Tests on Multi-Arch Clusters (ARM64 and...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~169-~169: Ensure spelling is correct
Context: ...4481] | | Update Tier2 automation to handle Multi-Arch scenari...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: can-be-merged
- GitHub Check: tox
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: tox
- GitHub Check: can-be-merged
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@stps/sig-virt/multiarch_support.md`:
- Line 113: Update the table row labeled "Platform" in multiarch_support.md to
replace the "N/A" value with "AWS" so it correctly reflects that AWS is the
provider for all ARM nodes in the multi-arch test environment; locate the row
containing the "**Platform**" header and change its second column from "N/A" to
"AWS".
- Line 133: Unmark the "[V] Test environment can be **set up and configured**
(see Section II.4 - Test Environment)" checkpoint until the second arm64 node is
actually added, or alternatively update Section II.4 (the "Test Environment"
description that currently states "2 amd64 workers, and 2 arm64 workers") to
reflect the current single arm64 node and adjust test scope accordingly; locate
the checklist entry and the Section II.4 text in multiarch_support.md and either
remove the check mark and add a TODO noting the pending manual ARM node
addition, or change the environment description and any dependent test
coverage/acceptance criteria to match one arm64 worker.
♻️ Duplicate comments (7)
stps/sig-virt/multiarch_support.md (7)
3-3: Fix title formatting.The title still has "multi arch" instead of "multi-arch" and has stray trailing asterisks. This was flagged in previous reviews.
38-46: Complete the Technology and Design Review with specific details.All items are marked as done, but the "Details/Notes" contain generic template text (e.g., line 42: "Critical for identifying untestable aspects early"). This was flagged in previous reviews and remains unresolved. Either provide concrete multi-arch-specific details for each row or uncheck items that weren't fully reviewed.
83-97: Fix Test Strategy table formatting.The table structure is incorrect: values like "Yes" and "N/A" are placed in the "Description" column instead of the "Applicable (Y/N or N/A)" column. This was explicitly flagged by vsibirsk in previous reviews. Additionally, the "Dependencies" and "Cross Integrations" rows still contain placeholder/incomplete descriptions.
Based on learnings, vsibirsk noted wrong table filling with Y/N/A values in wrong column.
🔧 Example fix for table structure
-| Functional Testing | Yes | | | +| Functional Testing | Core VM scheduling and migration validation on multi-arch cluster | Y | | -| Automation Testing | Yes | | | +| Automation Testing | Automated test suite covering VM creation, scheduling, and migration scenarios | Y | | -| Dependencies | Dependent on deliverables from other components/products? Identify what is tested by which team. | | | +| Dependencies | Multi-arch cluster provisioning (infra team); ARM64 node availability (CI team); DataSource support (storage team) | Y | Coordinate with infra/CI for environment |
116-127: Document testing tools or clarify standard tooling use.The Testing Tools & Frameworks section is completely empty. This was flagged in previous reviews. Either document any multi-arch-specific tools required, or explicitly state that standard OpenShift virtualization testing infrastructure is sufficient.
148-151: Complete risk assessments with actual project risks.Rows for "Test Environment", "Untestable Aspects", "Resource Constraints", and "Dependencies" still contain placeholder instructional text. These must be replaced with actual risk assessments and mitigation strategies specific to multi-arch testing. For example, vsibirsk noted the test environment currently lacks sufficient ARM nodes for migration testing—this should be documented as a Test Environment risk.
Based on learnings, vsibirsk flagged the lack of additional ARM nodes as an environment concern.
💡 Example risk entries
-| Test Environment | [Describe environment risks, e.g., "Requires GPU hardware, limited availability"] | [Your mitigation, e.g., "Reserve GPU nodes early, schedule tests in advance"] | [ ] | +| Test Environment | Current setup has only 1 ARM node; requires manual addition of 2nd ARM node for migration testing | Automate ARM node provisioning in CI; coordinate with infra team for additional resources | [ ] |
164-170: Populate the Requirement Summary column.The "Requirement Summary" column is completely empty for all CNV Jira IDs. This was flagged multiple times in previous reviews as a critical issue. The section explicitly states its purpose is to "link requirements to test coverage, enabling reviewers to verify all requirements are tested"—this is impossible without requirement summaries. Add concise one-line descriptions for each Jira ticket.
58-60: Standardize architecture terminology.The document inconsistently refers to architectures as "AMD64 and ARM64" (line 58) versus "
amd64" and "arm64" (line 60). Per vsibirsk's previous feedback, use the lowercase forms (amd64andarm64) consistently throughout to match KubeVirt conventions.Based on learnings, vsibirsk requested consistent arch naming using lowercase forms.
📝 Proposed consistency fix
-- Create VMs for both AMD64 and ARM64 arch types and confirm the cluster automatically schedules them on respective architecture node. +- Create VMs for both `amd64` and `arm64` architectures and confirm the cluster automatically schedules them on respective architecture node. -- Test VM creation using both golden image DataSources and custom Qcow2 images and ensure they always land on the right nodes. +- Test VM creation using both golden image DataSources and custom Qcow2 images for each architecture and ensure they always land on the right nodes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stps/sig-virt/multiarch_support.md
🧰 Additional context used
🪛 LanguageTool
stps/sig-virt/multiarch_support.md
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...M creation and Live Migration on a multi arch cluster - Quality Engineering Plan*...
(QB_NEW_EN_HYPHEN)
[grammar] ~168-~168: Ensure spelling is correct
Context: ...V-75737] | | Run Tier2 Tests on Multi-Arch Clusters (ARM64 and...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~169-~169: Ensure spelling is correct
Context: ...4481] | | Update Tier2 automation to handle Multi-Arch scenari...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
stps/sig-virt/multiarch_support.md (1)
86-89: Complete PM/Lead Agreement sign-offs before final approval.The Out of Scope table contains unchecked PM/Lead Agreement checkboxes. While this is acceptable for a Draft document, these must be completed with actual names and dates before the STP moves to final approval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-virt/multiarch_support.md` around lines 86 - 89, The "Out-of-Scope Item" table rows for "Testing with container disk VM" and "Cross-arch live migration" currently have unchecked PM/ Lead Agreement boxes; update those two entries in multiarch_support.md by replacing the "[ ] Name/Date" placeholders with the actual approver names and dates (and change to a checked state if your policy requires "[x]" or similar), ensuring both rows include completed PM/Lead sign-offs before moving the STP to final approval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stps/sig-virt/multiarch_support.md`:
- Line 120: The table row for "Backward Compatibility Testing" contains a
concatenated token "test plansig-iuo"; update the cell text to separate the
words (e.g., "test plan sig-iuo" or "test plan. sig-iuo") or remove the stray
fragment so the sentence reads correctly; locate the row by the header "Backward
Compatibility Testing" in multiarch_support.md and edit the third/fourth column
cell to insert the missing space or punctuation.
- Line 161: The table's "Timeline/Schedule" row is missing a Specific Risk
entry; update the "Specific Risk for This Feature" cell for the
Timeline/Schedule row (the table row that begins with "Timeline/Schedule") by
either adding a concise risk description (e.g., "Feature freeze date
approaching", "Limited time for test execution", or "Dependencies not ready by
release milestone") or explicitly mark the cell "N/A" and add one-line
justification for why schedule risk does not apply; ensure the change is applied
in the same markdown table row so the table remains aligned.
- Line 100: Update the architecture names in the checklist entry "**[P0]**
Verify ARM64 and AMD64 VMs are migrated to same-architecture nodes during
upgrades and correct placement is preserved." to use kubevirt/convention
lowercase identifiers "arm64" and "amd64" (i.e., change "ARM64 and AMD64" to
"arm64 and amd64") and scan the document for any other uppercase occurrences to
normalize them to lowercase for consistency with lines like 78, 80, and 132.
- Around line 183-185: Update the architecture names in the traceability matrix
entries that reference "ARM64 and AMD64" to use lowercase per KubeVirt
convention; specifically modify the rows for CNV-26818, CNV-75737 and CNV-33896
so the description text reads "arm64 and amd64" (or "amd64 and arm64" to match
surrounding ordering) to match the rest of the document.
---
Nitpick comments:
In `@stps/sig-virt/multiarch_support.md`:
- Around line 86-89: The "Out-of-Scope Item" table rows for "Testing with
container disk VM" and "Cross-arch live migration" currently have unchecked PM/
Lead Agreement boxes; update those two entries in multiarch_support.md by
replacing the "[ ] Name/Date" placeholders with the actual approver names and
dates (and change to a checked state if your policy requires "[x]" or similar),
ensuring both rows include completed PM/Lead sign-offs before moving the STP to
final approval.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2aea91d2-90e6-4e40-99dd-d3db9d1c391e
📒 Files selected for processing (1)
stps/sig-virt/multiarch_support.md
Signed-off-by: akri3i <guptaakriti70@gmail.com>
for more information, see https://pre-commit.ci
Co-authored-by: vsibirsk <57763370+vsibirsk@users.noreply.github.com>
| - *Test Scenario:* [Tier 2] Verify VMs created from arch-specific golden image DataSources run on matching architecture nodes | ||
| - *Priority:* P0 | ||
|
|
||
| - **[CNV-26818](https://issues.redhat.com/browse/CNV-26818)** — VM creation with custom Qcow2 images on correct arch |
There was a problem hiding this comment.
Is it custom template or a just a plain VM?
will the have the architecture field?
| - *Test Scenario:* [Tier 2] Verify arm64 and amd64 VMs are migrated to same-architecture nodes during upgrades and placement preserved | ||
| - *Priority:* P0 | ||
|
|
||
| - **[CNV-75737](https://issues.redhat.com/browse/CNV-75737)** — Regression: Run T1+T2 on multiarch clusters |
There was a problem hiding this comment.
I dont think that we need those here, those are not new tests
| - *Test Scenario:* [Tier 2] Run Tier 1 and Tier 2 test suites on multiarch clusters with both CPU architectures | ||
| - *Priority:* P0 | ||
|
|
||
| - **[CNV-33896](https://issues.redhat.com/browse/CNV-33896)** — Conformance tests on multiarch cluster |
| - *Test Scenario:* [Tier 1] Run conformance tests on multi-arch cluster (arm64 and amd64) | ||
| - *Priority:* P1 | ||
|
|
||
| --- |
There was a problem hiding this comment.
btw, what about creating vm from the cli? is it supported?
And what about defaultCPUs?
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
while acceptance criteria covers what needs to be verified, most of it is already covered by regression tests. please make sure you only add (if needed) scenarios that are not already covered. please also make sure that the stp maintains only high level content and does not include implementation details.
your stp are an extension to the main one (and based on size could potentially be incorporated in the main one); please remove any redundant info
11 participants
STP Metadata
VEP issue:
What this PR does
Special notes for your reviewer
Summary by CodeRabbit