Conversation
Adding storage multi-arch stp
for more information, see https://pre-commit.ci
|
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 Software Test Plan (STP) document defining CDI testing for heterogeneous multi-architecture OpenShift virtualization clusters, covering metadata, conventions, scope, goals, test strategy, environment/tooling requirements, detailed test scenarios with traceability, cross-integration touchpoints, risks, limitations, and sign-off. (34 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
openshift-virtualization-qe-bot
requested review from
dalia-frank,
josemacassan and
kshvaika
|
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
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@stps/sig-storage/storage_multiarch_stp.md`:
- Around line 219-224: Populate the empty "Requirement ID" column for each table
row by inserting the corresponding Jira issue keys (e.g., CNV-12345) that map
each test scenario to its requirement; update the four rows whose summaries
include "Pull multi-arch image...", "Pull failed when multi-arch...", "node
selector for multi-arch...", and "DataSource pointing to another DataSource" so
the "Requirement ID" cells contain the appropriate Jira keys, ensuring each
entry is a unique, traceable identifier referenced in your test plan.
- Line 108: Fix the spelling in the bullet text "[P0] Validata DataSource
pointing to another DataSource" by changing "Validata" to "Validate" so it reads
"[P0] Validate DataSource pointing to another DataSource".
- Line 3: The document title line "CDI support for heterogeneous multi-arch
clusters - Quality Engineering Pla" is truncated; update that heading to read
"CDI support for heterogeneous multi-arch clusters - Quality Engineering Plan"
by replacing "Pla" with "Plan" in the title string.
- Line 38: There is a missing space in the markdown table cell containing
"andunambiguous"; edit storage_multiarch_stp.md and change the text in the
Testability row (the cell with "**Testability** | [x] | Requirements are
testable andunambiguous") to insert a space so it reads "Requirements are
testable and unambiguous".
- Around line 110-117: The "Out of Scope (Testing Scope Exclusions)" header is
indented as if it were a child of the preceding P0 goal; move the "Out of Scope
(Testing Scope Exclusions)" section out to top-level within the "Scope of
Testing" section by removing the extra indentation/leading spaces before the
header and its table so the header and its table align with other top-level
sections (e.g., the P0 goal and "Scope of Testing") ensuring "Out of Scope
(Testing Scope Exclusions)" appears as a sibling section rather than a nested
sub-item.
🧹 Nitpick comments (1)
stps/sig-storage/storage_multiarch_stp.md (1)
232-237: Replace placeholder values in sign-off section.The reviewers and approvers sections contain placeholder values. Please update these with actual names and GitHub usernames before moving the document out of draft status.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stps/sig-storage/storage_multiarch_stp.md
🧰 Additional context used
🪛 LanguageTool
stps/sig-storage/storage_multiarch_stp.md
[grammar] ~108-~108: Ensure spelling is correct
Context: ...cture with pull method Node - [P0] Validata DataSource pointing to another DataSour...
(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: can-be-merged
- GitHub Check: tox
- GitHub Check: tox
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
✏️ 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: 3
🤖 Fix all issues with AI agents
In `@stps/sig-storage/storage_multiarch_stp.md`:
- Line 119: The markdown headings have leading whitespace; remove the leading
spaces so each heading token starts at column 0 (e.g., change " #### **2. Test
Strategy**" to "#### **2. Test Strategy**") and apply the same fix to the other
affected headings ("#### **3. Test Cases**", "#### **4. Test Environment**",
"#### **5. Test Results**" or whatever their exact text is at lines 140, 157,
and 169) so all headings begin at the start of the line to comply with Markdown
syntax.
- Line 19: Replace the placeholder under the "Document Conventions (if
applicable):" heading by either listing the specific acronyms/terms used in this
document (for example "CDI: Containerized Data Importer", "DIC: DataImportCron")
or explicitly state "None" if no special conventions apply; update the “Document
Conventions” line in storage_multiarch_stp.md accordingly so it is no longer a
placeholder.
- Around line 112-117: Update the "PM/ Lead Agreement" column in the Non-Goal
table by replacing each "[ ] Name/Date" placeholder with the actual approver's
name and date for the corresponding row (Performance Testing, Security Testing,
Usability testing, Backward Compatibility Testing); in particular, obtain and
record the PM/Lead sign-off for the "Backward Compatibility Testing" row that
references the SSP test plan so it clearly shows stakeholder agreement before
finalizing the STP.
♻️ Duplicate comments (2)
stps/sig-storage/storage_multiarch_stp.md (2)
190-198: Complete the risks table mitigation strategies.Several risk entries are incomplete:
- Timeline/Schedule (line 192): Has "Code-Freeze" risk but no mitigation
- Test Coverage (line 193): Both "Specific Risk" and "Mitigation Strategy" are empty
- Test Environment (line 194): Has Jira reference but no mitigation strategy
- Known Bugs (line 198): Has Jira reference but no mitigation strategy
Each risk should document how it will be managed or mitigated. For Jira-tracked issues, briefly describe the mitigation approach (e.g., "Tracked in CNV-76482, workaround documented in test setup").
219-224: Populate requirement IDs for traceability.The "Requirement ID" column remains empty for all test scenarios. Per the instructions (lines 214-217), each scenario should reference a specific Jira issue key (e.g., CNV-xxxxx) to enable requirement-to-test traceability.
This is essential for:
- Verifying complete requirement coverage
- Tracking test results back to requirements
- Impact analysis when requirements change
🧹 Nitpick comments (3)
stps/sig-storage/storage_multiarch_stp.md (3)
163-167: Clarify or remove "MultiArch cluster" from Test Framework."MultiArch cluster" is listed as a Test Framework, but it's actually an environment requirement (already documented in section 3). If using standard testing frameworks (pytest, unittest, etc.), either list them explicitly or remove this entry per the section instructions.
♻️ Proposed fix
If using standard frameworks:
-| **Test Framework** | MultiArch cluster | -| **CI/CD** | | -| **Other Tools** | | +| **Test Framework** | Standard Python pytest framework | +| **CI/CD** | Standard CI (no additional tools) | +| **Other Tools** | None |Or if truly no special tools:
-| Category | Tools/Frameworks | -|:------------------ |:----------------- | -| **Test Framework** | MultiArch cluster | -| **CI/CD** | | -| **Other Tools** | | +| Category | Tools/Frameworks | +|:------------------ |:----------------- | +| **Test Framework** | Standard framework (pytest) | +| **CI/CD** | N/A - using standard CI | +| **Other Tools** | N/A |
200-207: Document known limitations or state "None".The section contains only an example comment. Based on the test environment requirements (AWS with arm64 workers only), consider documenting actual limitations such as:
- s390x architecture not tested (hardware availability)
- Limited to AWS platform (arm64 worker availability)
- 12-hour MultiArch cluster availability window
Or state explicitly if no limitations exist.
232-237: Consider adding assigned reviewers to sign-off section.The PR metadata shows specific reviewers assigned (kgoldbla, dalia-frank, josemacassan, jpeimer, Ahmad-Hafe, kshvaika). Consider populating the placeholder names with these reviewers to clarify approval workflow.
📝 Example
* **Reviewers:** - `@kgoldbla` - `@dalia-frank` - `@josemacassan` - `@kshvaika` * **Approvers:** - `@jpeimer` - `@Ahmad-Hafe`
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stps/sig-storage/storage_multiarch_stp.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
stps/sig-storage/storage_multiarch_stp.md
119-119: Headings must start at the beginning of the line
(MD023, heading-start-left)
140-140: Headings must start at the beginning of the line
(MD023, heading-start-left)
157-157: Headings must start at the beginning of the line
(MD023, heading-start-left)
169-169: Headings must start at the beginning of the line
(MD023, heading-start-left)
⏰ 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). (4)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: tox
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| | Non-Goal | Rationale | PM/ Lead Agreement | | ||
| |:------------------------------ |:-------------------------------------------------------------------------------------------------- |:------------------ | | ||
| | Performance Testing | Out of scope for this test plan | [ ] Name/Date | | ||
| | Security Testing | Out of scope for this test plan | [ ] Name/Date | | ||
| | Usability testing | Should be done by UI team | [ ] Name/Date | | ||
| | Backward Compatibility Testing | VM creation using architecture-specific DataSources and legacy DataSource covered by SSP test plan | [ ] Name/Date | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "storage_multiarch_stp.md" -type fRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 138
🏁 Script executed:
head -120 stps/sig-storage/storage_multiarch_stp.md | tail -30Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 2021
🏁 Script executed:
head -50 stps/sig-storage/storage_multiarch_stp.mdRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 6116
🏁 Script executed:
grep -i "status\|draft\|version\|approval" stps/sig-storage/storage_multiarch_stp.md | head -20Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 3840
Complete PM/Lead agreements for out-of-scope items.
The "PM/ Lead Agreement" column shows placeholder values "[ ] Name/Date" and should be filled with actual names and dates confirming stakeholder alignment on scope exclusions. This is particularly important for items like "Backward Compatibility Testing" which references SSP test plan coverage.
Since this is a draft STP, these can be completed during the review process. Ensure sign-offs are obtained before final approval.
🤖 Prompt for AI Agents
In `@stps/sig-storage/storage_multiarch_stp.md` around lines 112 - 117, Update the
"PM/ Lead Agreement" column in the Non-Goal table by replacing each "[ ]
Name/Date" placeholder with the actual approver's name and date for the
corresponding row (Performance Testing, Security Testing, Usability testing,
Backward Compatibility Testing); in particular, obtain and record the PM/Lead
sign-off for the "Backward Compatibility Testing" row that references the SSP
test plan so it clearly shows stakeholder agreement before finalizing the STP.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@stps/sig-storage/storage_multiarch_stp.md`:
- Around line 207-214: The "Known Limitations" section currently mixes example
text with potential real limitations; clarify by removing the example markers
and either (a) if s390x is genuinely untestable for this feature, move the line
"s390x is not testable due to hardware availability" out of the example/comment
block into the visible list under the "#### **6. Known Limitations**" heading
and add any other real constraints, or (b) if there are no real limitations,
replace the example content with a single line "None" to make the section
explicit; update the storage_multiarch_stp.md Known Limitations block
accordingly.
- Around line 221-233: The Requirement ID column currently contains
TC-01/TC-02/... entries (rows for TC-01 through TC-05) but the header text above
the table requires Jira issue keys (e.g., CNV-12345) for traceability; either
replace each TC-## value with the corresponding Jira issue key that maps to that
requirement (update the cells under "Requirement ID" for the entries currently
labeled TC-01..TC-05) or change the header/instruction text to explicitly allow
the TC-## convention (edit the comments at the top that state "Use Jira issue
key (e.g., CNV-12345)") so it matches the table; ensure consistency between the
header instruction and the table entries so Requirement ID and traceability are
aligned.
♻️ Duplicate comments (6)
stps/sig-storage/storage_multiarch_stp.md (6)
119-124: Fix table indentation.The table rows have leading whitespace that should be removed for proper markdown formatting. This was flagged in previous reviews but remains unaddressed.
📝 Proposed fix
- | Non-Goal | Rationale | PM/ Lead Agreement | - |:------------------------------ |:-------------------------------------------------------------------------------------------------- |:------------------ | - | Performance Testing | Out of scope for this test plan | [ ] Name/Date | - | Security Testing | Out of scope for this test plan | [ ] Name/Date | - | Usability testing | Should be done by UI team | [ ] Name/Date | - | Backward Compatibility Testing | VM creation using architecture-specific DataSources and legacy DataSource covered by SSP test plan | [ ] Name/Date | +| Non-Goal | Rationale | PM/ Lead Agreement | +|:------------------------------ |:-------------------------------------------------------------------------------------------------- |:------------------ | +| Performance Testing | Out of scope for this test plan | [ ] Name/Date | +| Security Testing | Out of scope for this test plan | [ ] Name/Date | +| Usability testing | Should be done by UI team | [ ] Name/Date | +| Backward Compatibility Testing | VM creation using architecture-specific DataSources and legacy DataSource covered by SSP test plan | [ ] Name/Date |
126-126: Fix heading indentation.The heading has leading whitespace which violates markdown syntax. Headings must start at the beginning of the line.
📝 Proposed fix
- #### **2. Test Strategy** +#### **2. Test Strategy**
147-147: Fix heading indentation.The heading has leading whitespace which violates markdown syntax.
📝 Proposed fix
- #### **3. Test Environment** +#### **3. Test Environment**
164-164: Fix heading indentation.The heading has leading whitespace which violates markdown syntax.
📝 Proposed fix
- #### **3.1. Testing Tools & Frameworks** +#### **3.1. Testing Tools & Frameworks**
176-176: Fix heading indentation.The heading has leading whitespace which violates markdown syntax.
📝 Proposed fix
- #### **4. Entry Criteria** +#### **4. Entry Criteria**
197-206: Complete the risks table.Several entries remain incomplete from previous reviews:
- Timeline/Schedule (line 199): Has "Code-Freeze" risk but no mitigation strategy
- Test Coverage (line 200): Empty "Specific Risk" cell needs to be filled or marked "N/A"
- Test Environment (line 201): Has Jira reference but no mitigation strategy
- Known Bugs (line 205): Has Jira reference but no mitigation strategy
For each risk, either provide the mitigation strategy or explain why mitigation is not applicable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@stps/sig-storage/storage_multiarch_stp.md`:
- Around line 209-214: The "Known Limitations" HTML comment opened with "<!--"
in the storage_multiarch_stp.md document is never closed, causing subsequent
content to be hidden; locate the unclosed comment in the "Known Limitations"
section and add the closing token "-->" immediately after the intended comment
text so the rest of the Markdown renders normally (ensure the inserted "-->"
corresponds to the existing "<!--" and does not alter adjacent content).
♻️ Duplicate comments (5)
stps/sig-storage/storage_multiarch_stp.md (5)
19-19: Remove the placeholder label from “Document Conventions”.Line 19 still contains placeholder text even though actual conventions are listed below. Please delete the bracketed placeholder text.
117-124: Unindent the “Out of Scope” table so it renders as a table.The leading spaces turn the table into a code block in Markdown.
📝 Proposed fix
- | Non-Goal | Rationale | PM/ Lead Agreement | - |:------------------------------ |:-------------------------------------------------------------------------------------------------- |:------------------ | - | Performance Testing | Out of scope for this test plan | [ ] Name/Date | - | Security Testing | Out of scope for this test plan | [ ] Name/Date | - | Usability testing | Should be done by UI team | [ ] Name/Date | - | Backward Compatibility Testing | VM creation using architecture-specific DataSources and legacy DataSource covered by SSP test plan | [ ] Name/Date | +| Non-Goal | Rationale | PM/ Lead Agreement | +|:------------------------------ |:-------------------------------------------------------------------------------------------------- |:------------------ | +| Performance Testing | Out of scope for this test plan | [ ] Name/Date | +| Security Testing | Out of scope for this test plan | [ ] Name/Date | +| Usability testing | Should be done by UI team | [ ] Name/Date | +| Backward Compatibility Testing | VM creation using architecture-specific DataSources and legacy DataSource covered by SSP test plan | [ ] Name/Date |
126-207: Fix heading indentation and add a space after the hash marks.Headings like
####**2. Test Strategy**are indented and missing a space, which breaks Markdown rendering. Please apply to all affected headings in this block (Lines 126, 147, 164, 176, 190, 207).📝 Proposed fix (apply to all affected headings)
- ####**2. Test Strategy** +#### **2. Test Strategy**
197-205: Fill in missing risk details or mark as N/A.Several rows are empty (e.g., Timeline/Schedule mitigation, Test Coverage risk). Please complete or mark N/A with justification.
221-233: Align “Requirement ID” with the Jira-key convention.The table uses TC-## values, but the instructions require Jira issue keys for traceability.
openshift-virtualization-qe-bot-2
added
size/XL
and removed
commented-duyanyan
size/L
labels
| - **Epic Tracking:** [CNV-73892](https://issues.redhat.com/browse/CNV-73892) | ||
| - **QE Owner(s):** Yan Du | ||
| - **Owning SIG:** sig-storage | ||
| - **Participating SIGs:** sig-infra, sig-storage, sig-virt |
There was a problem hiding this comment.
I'm not sure what should be here, maybe
| - **Participating SIGs:** sig-infra, sig-storage, sig-virt | |
| - **Participating SIGs:** sig-iuo, sig-infra, sig-storage, sig-virt, sig-network, sig-ui |
| - `spec.registry.platform.architecture` field for specifying target architecture | ||
| - `DataSourceSource.dataSource.namespace` (string) - Pointer DataSource namespace | ||
| - `DataSourceSource.dataSource.name` (string) - Pointer DataSource name |
There was a problem hiding this comment.
It may be better to add the full api, so that it'll be easier to see which resource it's about, i.e.:
datasource.spec.source.dataSource
datasource.spec.source.dataSource.namespace
datasource.spec.source.dataSource.name
| **Functional Goals**: | ||
|
|
||
| - **[P0]** Verify multi-arch image matching architecture with pull method Pod | ||
| - Import succeeds when using `spec.registry.pullMethod: pod` with `spec.registry.platform.architecture: arm64` |
There was a problem hiding this comment.
| - Import succeeds when using `spec.registry.pullMethod: pod` with `spec.registry.platform.architecture: arm64` | |
| - Import succeeds when using `dv.spec.source.registry.pullMethod: pod` with `dv.spec.source.registry.platform.architecture: arm64` |
| - Correct architecture-specific image layers are pulled from multi-arch registry | ||
|
|
||
| - **[P0]** Verify multi-arch image matching architecture with pull method Node | ||
| - Import succeeds when using `spec.registry.pullMethod: node` with correct architecture |
There was a problem hiding this comment.
| - Import succeeds when using `spec.registry.pullMethod: node` with correct architecture | |
| - Import succeeds when using `dv.spec.source.registry.pullMethod: node` with correct architecture |
| ### **III. Test Scenarios & Traceability** | ||
|
|
||
| - **[TBD]** — Pull multi-arch image matching architecture with pull method Pod | ||
| - *Test Scenario:* [Tier 1] Verify the import succeeded with spec.registry.pullMethod: pod and spec.registry.platform.architecture: arm64 |
There was a problem hiding this comment.
| - *Test Scenario:* [Tier 1] Verify the import succeeded with spec.registry.pullMethod: pod and spec.registry.platform.architecture: arm64 | |
| - *Test Scenario:* [Tier 1] Verify the import succeeded with dv.spec.source.registry.pullMethod: pod and dv.spec.source.registry.platform.architecture: arm64 |
| - *Priority:* P0 | ||
|
|
||
| - **[TBD]** — Pull failed when multi-arch image with absent architecture with pull method Pod | ||
| - *Test Scenario:* [Tier 1] Verify the import failed with spec.registry.pullMethod: pod and spec.registry.platform.architecture: absent (architecture not in image) |
| - *Priority:* P1 | ||
|
|
||
| - **[TBD]** — Node selector for multi-arch image architecture with pull method Node | ||
| - *Test Scenario:* [Tier 1] Verify the import pod has "Unschedulable" condition with spec.registry.pullMethod: node and spec.registry.platform.architecture: absent (no matching nodes) |
| - *Priority:* P0 | ||
|
|
||
| - **[TBD]** — Cross-architecture VM cloning | ||
| - *Test Scenario:* [Tier 2] Clone AMD64 VM, verify clone uses AMD64 DataSource. Attempt cross-arch clone, verify appropriate error message |
There was a problem hiding this comment.
I expect that the happy scenario will be covered by the existing Virt tests: https://github.com/RedHatQE/openshift-virtualization-tests/blob/main/tests/virt/cluster/vm_cloning/test_vm_cloning.py
Let's ask the Virt team if we need the negative test for this.
There was a problem hiding this comment.
Hi, @akri3i Do you think we need the negative test for this?
for more information, see https://pre-commit.ci
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 is an extension to the main one (and based on size could potentially be incorporated in the main one); please remove any redundant info
9 participants
Adding storage multi-arch stp
STP Metadata
VEP issue:
What this PR does
Special notes for your reviewer
Summary by CodeRabbit