storage: Add STP for offline storage migration#63
Open
duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
Open
storage: Add STP for offline storage migration#63duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
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 Software Test Plan "Offline Storage Migration - Quality Engineering Plan" for OpenShift Virtualization (CNV-73509 / epic CNV-73500). Documents feature metadata, QE owner, review checklist, prioritized testing goals, scope/strategy, environment/topology, known limitations, and requirement-to-test traceability. (+421 lines) Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
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
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
AI Features
💡 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: 4
🧹 Nitpick comments (2)
stps/sig-storage/storage_mig_offline.md (2)
232-237: Usability classification contradicts stated scope ownership.
Usability Testingis checked, but details say UI testing is owned by UI team and elsewhere UI tests are out of scope. Either uncheck this item or scope it strictly to non-UI UX checks (CLI/status/events).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 232 - 237, Update the checklist so the "Usability Testing" item is consistent with the stated scope: either remove the checked mark (uncheck "Usability Testing") or rewrite that checklist line to explicitly limit it to non-UI aspects (CLI/status/events) and replace the existing "*Details:* UI testing will be covered by the UI team" with a clarifying note that UI testing is out of scope and owned by the UI team; ensure the change targets the "Usability Testing" checklist entry and the accompanying "*Details:* UI testing will be covered by the UI team" line.
7-7: Prefer linking HLD inEnhancement(s)instead ofN/Awhen no enhancement PR exists.Using an HLD reference preserves traceability while still aligning with repo practice when no enhancement PR is available.
Based on learnings: In this repository, if no enhancement PR exists, it is acceptable to reference only the HLD document in the Enhancement(s) field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` at line 7, Update the markdown front-matter entry "Enhancement(s):" to reference the HLD document instead of "N/A": replace the literal "N/A" with a short link or citation to the relevant HLD (e.g., HLD title and URL or internal doc reference) so the enhancement field preserves traceability; edit the line showing **Enhancement(s):** in storage_mig_offline.md to include that HLD reference.
🤖 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-storage/storage_mig_offline.md`:
- Around line 174-175: The document has inconsistent priority labels for the
same scenario: the heading/entry "Verify retentionPolicy for offline VM storage
migration" is marked as **[P1]** in Section II but as **[P0]** in Section III
(also duplicated around lines 395-397); pick the correct priority (P0 or P1) and
make both occurrences consistent by updating the priority tag(s) so the scenario
uses the same label in "Verify retentionPolicy for offline VM storage migration"
across Section II and Section III (and the duplicate at 395-397).
- Around line 191-197: The checklist items "Other storage migration tests" and
"Storage migration UI tests" are checked but their "PM/Lead Agreement" fields
are empty; update the document so the state is consistent by either filling the
PM/Lead Agreement lines with the approver name and date for each item ("Other
storage migration tests" and "Storage migration UI tests") or change the
top-level checkboxes to an unchecked/pending state (or add a clear "Pending"
note) to avoid indicating false completion.
- Around line 401-410: Replace the placeholder reviewers/approvers in the "IV.
Sign-off and Approval" section with real names and GitHub handles and add a
final sign-off checklist that explicitly lists required role approvals (QE Lead,
PM, Development Lead) and checklist items (e.g., "Test cases executed",
"Regression run status", "Release readiness", "Risk acceptance", "Deployment
window approved"); update the heading "IV. Sign-off and Approval" so the block
contains concrete approver entries and checkboxes for each required sign-off
step to ensure traceable approvals.
- Around line 311-318: Add a mandatory "Exit Criteria" subsection immediately
after the "Entry Criteria" header in Section II that lists clear,
test-completion and readiness gating items for sign-off (e.g., all test cases
executed/pass rates, critical/high defects closed or mitigated, documentation
updated/merged, environment teardown steps, and stakeholder approval). Ensure
the new "Exit Criteria" markdown header mirrors the style of "Entry Criteria"
and contains a checked/unchecked checklist template so reviewers can mark items
off during sign-off; reference the existing "Entry Criteria" text to copy
formatting and checklist style.
---
Nitpick comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Around line 232-237: Update the checklist so the "Usability Testing" item is
consistent with the stated scope: either remove the checked mark (uncheck
"Usability Testing") or rewrite that checklist line to explicitly limit it to
non-UI aspects (CLI/status/events) and replace the existing "*Details:* UI
testing will be covered by the UI team" with a clarifying note that UI testing
is out of scope and owned by the UI team; ensure the change targets the
"Usability Testing" checklist entry and the accompanying "*Details:* UI testing
will be covered by the UI team" line.
- Line 7: Update the markdown front-matter entry "Enhancement(s):" to reference
the HLD document instead of "N/A": replace the literal "N/A" with a short link
or citation to the relevant HLD (e.g., HLD title and URL or internal doc
reference) so the enhancement field preserves traceability; edit the line
showing **Enhancement(s):** in storage_mig_offline.md to include that HLD
reference.
🪄 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: f7b2c049-541d-4f3c-82da-c8338c8c7b36
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_offline.md
awels
approved these changes
Apr 3, 2026
rnetser
requested changes
Apr 3, 2026
Comment on lines
+8
to
+9
| - **Feature Tracking:** [CNV-82430](https://issues.redhat.com/browse/CNV-82430) | ||
| - **Epic Tracking:** [CNV-73500](https://issues.redhat.com/browse/CNV-73500) |
Contributor
There was a problem hiding this comment.
please update with:
- cloud jira urls
- the feature id
Author
There was a problem hiding this comment.
Update with could jira urls. but not sure what is feature id...
Comment on lines
+15
to
+19
| - **VM** - Virtual Machine | ||
| - **CNV** - Container-native Virtualization (OpenShift Virtualization) | ||
| - **CDI** - Containerized Data Importer | ||
| - **PVC** - Persistent Volume Claim | ||
| - **DV** - DataVolume |
Contributor
There was a problem hiding this comment.
these should be removed.
please include only items which are:
- directly relevant to the feature
- are not standard terms in openshift virtualization
|
|
||
| ### **Metadata & Tracking** | ||
|
|
||
| - **Enhancement(s):** N/A |
Contributor
There was a problem hiding this comment.
please add a link to the vep; if an enhancement does not exist, please add a link to the implementation PR
| - [x] **Non-Functional Requirements (NFRs)** | ||
| <!-- Confirmed coverage for NFRs, including Performance, Security, Usability, Downtime, Connectivity, Monitoring (alerts/metrics), Scalability, Portability (e.g., cloud support), and Docs.--> | ||
| - *List applicable NFRs and their targets:* Documentation updates to reflect offline VM storage migration support | ||
| - *Note any NFRs not covered and why:* Performance and scale testing are not required for this feature |
Contributor
There was a problem hiding this comment.
https://redhat.atlassian.net/browse/CNV-82430 states Performance testing for bulk offline migrations
Author
There was a problem hiding this comment.
we are not going to cover performance testing in the test plan currently
| **Note:** Replace examples with your actual out-of-scope items. If there are no items; delete the checklist and add `None`--> | ||
|
|
||
| - [x] **Other storage migration tests** | ||
| - *Rationale:* Existing storage migration tests already covered in 4.21 release |
Contributor
There was a problem hiding this comment.
out os scope means that x is part of the feature but qe decided not to test it because of y.
so in this context Existing storage migration tests already covered in 4.21 release is not out of scope as they will be tested as part of regression
| - **[P0]** Verify storage migration for offline VM | ||
| - **[P0]** Verify storage migration for offline VM and running VM in one migration plan | ||
| - **[P1]** Verify storage migration for offline VM with hotplug disk | ||
| - **[P1]** Verify retentionPolicy for offline VM storage migration |
Contributor
There was a problem hiding this comment.
should be udpated to p0, right?
| - **[P1]** Test integration with OpenShift monitoring stack: metrics appear in Prometheus, | ||
| alerts fire correctly in Alertmanager --> | ||
|
|
||
| - **[P0]** Verify storage migration for offline VM |
Contributor
There was a problem hiding this comment.
This is too generic; what does success look like?
A testing goal must be measurable. Please check the above hints for examples and reference..
In this case, it could be something like "Verify offline VM storage migration completes and VM boots successfully"
Please update the items in this section
|
|
||
| **Requirement Summary:** Brief description from the Jira issue (user story format preferred) --> | ||
|
|
||
| - **[CNV-82430]** — Storage migration support for offline VMs |
Contributor
There was a problem hiding this comment.
please see the hints on the expected format.
here, for example, it would be something like
"As a cluster admin, I want to migrate storage for offline VMs without starting them"
please update all items to reflect the user stories that are covered in each one of the test scenarios
|
|
||
| - [x] **Technology Challenges** | ||
| <!-- Identified potential testing challenges related to the underlying technology.--> | ||
| - *List identified challenges:* Offline VMs initiate migration through CDI clone of the source PVC |
Contributor
There was a problem hiding this comment.
this is an implementation detail; should be rephrased from testing perspective
in this case, does offline migration follow a different path from online migration?
if so, it would be something like "Offline migration uses a different mechanism than online migration, requiring separate test coverage."
| <!-- **Requirement ID:** | ||
| - Use Jira issue key (e.g., CNV-12345) | ||
|
|
||
| **Requirement Summary:** Brief description from the Jira issue (user story format preferred) --> |
Contributor
There was a problem hiding this comment.
what about edge cases, are they covered in tier1?
for example - what happens if migration fails half way? what happens if a vm is started during offline migration? multiple disks?
not sure if relevant - migration with vm snapshots?
Author
There was a problem hiding this comment.
Added tests for migration failed and start VM during miration.
for multiple disk - can be covered by hotplug vm
VM snapshot - seems not relevant I think
openshift-virtualization-qe-bot-4
requested review from
acinko-rh and
ema-aka-young
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
stps/sig-storage/storage_mig_offline.md (3)
188-190:⚠️ Potential issue | 🟠 MajorReplace template placeholder in Out-of-Scope with real content or
None.Line 188–Line 190 still contains template/example text, which leaves scope boundaries ambiguous.
Suggested edit
-- [ ] **[e.g., Testing of deprecated features]** - - *Rationale:* [Why this is excluded] - - *PM/Lead Agreement:* [Name/Date] +None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 188 - 190, The Out-of-Scope section contains a template checklist item "[e.g., Testing of deprecated features]" and placeholder fields; update that checklist entry under the "Out-of-Scope" heading in storage_mig_offline.md to either a concrete exclusion with a brief rationale and PM/Lead agreement (populate the "*Rationale:*" and "*PM/Lead Agreement:*" fields) or replace the entire placeholder block with "None" if nothing is out-of-scope, ensuring the checklist no longer contains example/template text.
216-220:⚠️ Potential issue | 🟠 MajorReconcile performance/scale “Not applicable” with stated requirement inputs.
Line 216–Line 220 marks performance and scale as not applicable, but earlier scope references include CNV-82430 in requirements review (Line 37), which can imply performance-oriented coverage expectations. Please align these sections to avoid planning drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 216 - 220, The "Performance Testing" and "Scale Testing" checkboxes are marked "Not applicable" but the requirements review references CNV-82430 which implies performance/scale considerations; update the "Performance Testing" and "Scale Testing" entries to reconcile this by either (a) removing "Not applicable" and adding specific performance/scale acceptance criteria tied to CNV-82430, or (b) keeping "Not applicable" but adding a concise justification referencing the "requirements review" and CNV-82430 to explain why performance/scale tests are out of scope; edit the "Performance Testing" and "Scale Testing" checklist items and the earlier "requirements review" mention so they consistently reflect the chosen approach.
304-311:⚠️ Potential issue | 🟠 MajorAdd an explicit Exit Criteria section after Entry Criteria.
Line 304–Line 311 defines entry checks only; an STP sign-off path should include explicit exit gates.
Suggested section structure
#### **4. Entry Criteria** @@ - [x] Requirements and design documents are **approved and merged** - [x] Test environment can be **set up and configured** (see Section II.3 - Test Environment) + +#### **5. Exit Criteria** + +- [ ] All P0/P1 planned scenarios executed +- [ ] Blocking/high defects resolved or formally accepted +- [ ] Automation merged and included in target lanes +- [ ] Stakeholder sign-off completed -#### **5. Risks** +#### **6. Risks**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 304 - 311, Add an explicit "Exit Criteria" section immediately after the existing "#### **4. Entry Criteria**" block and before "#### **5. Risks**"; in that new section list concrete exit gates such as verification steps completed, required test cases passed (with thresholds), mandatory deliverables (logs, results, migration artifacts), responsible approvers for sign-off, and rollback/contingency conditions so the STP has clear sign-off criteria and owners; reference the "Entry Criteria" and "5. Risks" headings to locate where to insert the section and update any table-of-contents or cross-references if present.
🤖 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-storage/storage_mig_offline.md`:
- Line 170: Replace the misspelled term "retetionPolicy" with the correct
"retentionPolicy" in the documentation sentence that reads "**[P0]** Verify
source volumes can be retained/deleted for offline VM storage migration when
retetionPolicy is set" so the policy name is spelled correctly wherever that
token appears in the file.
- Line 405: The QE member list line "QE Members (OCP-V): Jenia Peimer
(jpeimer@), Kate Shvaika (kshvaika@), Jose Manuel Castano(joscasta@)" uses
malformed handles; update that line so each reviewer is represented with a
consistent, valid `@username` handle (e.g., replace "jpeimer@", "kshvaika@", and
"joscasta@" with their proper `@handles`) while keeping the "QE Members (OCP-V):"
prefix unchanged to ensure traceable approvals.
---
Duplicate comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Around line 188-190: The Out-of-Scope section contains a template checklist
item "[e.g., Testing of deprecated features]" and placeholder fields; update
that checklist entry under the "Out-of-Scope" heading in storage_mig_offline.md
to either a concrete exclusion with a brief rationale and PM/Lead agreement
(populate the "*Rationale:*" and "*PM/Lead Agreement:*" fields) or replace the
entire placeholder block with "None" if nothing is out-of-scope, ensuring the
checklist no longer contains example/template text.
- Around line 216-220: The "Performance Testing" and "Scale Testing" checkboxes
are marked "Not applicable" but the requirements review references CNV-82430
which implies performance/scale considerations; update the "Performance Testing"
and "Scale Testing" entries to reconcile this by either (a) removing "Not
applicable" and adding specific performance/scale acceptance criteria tied to
CNV-82430, or (b) keeping "Not applicable" but adding a concise justification
referencing the "requirements review" and CNV-82430 to explain why
performance/scale tests are out of scope; edit the "Performance Testing" and
"Scale Testing" checklist items and the earlier "requirements review" mention so
they consistently reflect the chosen approach.
- Around line 304-311: Add an explicit "Exit Criteria" section immediately after
the existing "#### **4. Entry Criteria**" block and before "#### **5. Risks**";
in that new section list concrete exit gates such as verification steps
completed, required test cases passed (with thresholds), mandatory deliverables
(logs, results, migration artifacts), responsible approvers for sign-off, and
rollback/contingency conditions so the STP has clear sign-off criteria and
owners; reference the "Entry Criteria" and "5. Risks" headings to locate where
to insert the section and update any table-of-contents or cross-references if
present.
🪄 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: 0ce74b5c-96e0-4cfb-ac42-0ca50dc883fc
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_offline.md
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
stps/sig-storage/storage_mig_offline.md (2)
413-413:⚠️ Potential issue | 🟡 MinorUse valid, consistent GitHub handle format in reviewer list.
jpeimer@,kshvaika@,joscasta@should be written as@jpeimer,@kshvaika,@joscastafor traceable approvals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` at line 413, Update the reviewer list string "QE Members (OCP-V): Jenia Peimer (jpeimer@), Kate Shvaika (kshvaika@), Jose Manuel Castano(joscasta@)" to use valid GitHub handle format by replacing "jpeimer@", "kshvaika@" and "joscasta@" with "@jpeimer", "@kshvaika" and "@joscasta" respectively so the entry reads e.g. "Jenia Peimer (`@jpeimer`), Kate Shvaika (`@kshvaika`), Jose Manuel Castano (`@joscasta`)".
171-171:⚠️ Potential issue | 🟡 MinorFix
retentionPolicyspelling to avoid terminology drift.
retetionPolicyshould beretentionPolicyfor consistency with API/feature wording.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` at line 171, The documentation contains a misspelling: replace the incorrect identifier "retetionPolicy" with the correct "retentionPolicy" wherever it appears (e.g., the bullet line "**[P0]** Verify source volumes can be retained/deleted for offline VM storage migration when retetionPolicy is set"); update all occurrences to maintain consistency with the API/feature naming and run a quick search to ensure no other references remain.
🧹 Nitpick comments (2)
stps/sig-storage/storage_mig_offline.md (2)
384-402: Epic ID repetition is acceptable; optional cleanup available.This is valid as-is. If you want to reduce visual noise, keep the first
CNV-73500and leave subsequent requirement ID cells/lines empty in the same table/list block.Based on learnings: in this repository, repeating the epic ID is acceptable, while leaving subsequent rows empty is preferred to avoid redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 384 - 402, The repeated epic ID "CNV-73500" in the storage_mig_offline requirement list is redundant; to reduce visual noise, keep the first "CNV-73500" entry and clear (leave empty) the epic ID cells/lines for the subsequent list items within the same block so the requirement text remains unchanged but the repeated ID is omitted—locate the block containing the four subsequent bullet items (each starting with "- **[CNV-73500]**") and remove the duplicate ID text from the later bullets, leaving their descriptions and test scenarios intact.
189-191: Replace placeholder out-of-scope item with real content orNone.The current example template entry is still present. For a finalized STP, either provide real exclusions with PM/Lead agreement or remove the checklist and state
None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 189 - 191, The template's placeholder out-of-scope checklist item "[e.g., Testing of deprecated features]" must be replaced with either a concrete exclusion (fill in the item, rationale, and PM/Lead Agreement fields) or removed and replaced with the single word "None"; update the section in storage_mig_offline.md where that checklist appears (look for the bullet "- [ ] **[e.g., Testing of deprecated features]**") and ensure the PM/Lead Agreement line includes a name and date if you add a real exclusion.
🤖 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-storage/storage_mig_offline.md`:
- Line 397: Update the test scenario line in storage_mig_offline.md to replace
the grammatically unclear phrase "will be remained/cleaned up" with a clear,
direct wording such as "is retained or cleaned up" (or "is retained or removed")
so the expected behavior in the test scenario is unambiguous; locate and edit
the line containing the Test Scenario text for the Tier 2 offline VM
retentionPolicy in the document and apply the new phrasing.
- Line 400: The user story line for **[CNV-73500]** is grammatically malformed;
rewrite it to a clear, idiomatic user-story sentence. Replace "As a VM owner, I
want to offline VM still points to origin volume when migration is failed" with
a corrected version such as "As a VM owner, I want an offline VM to still point
to the original volume when migration fails" (or similar) and ensure wording
uses "offline VM" vs "an offline VM", "original" instead of "origin", and the
verb tense "when migration fails".
---
Duplicate comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Line 413: Update the reviewer list string "QE Members (OCP-V): Jenia Peimer
(jpeimer@), Kate Shvaika (kshvaika@), Jose Manuel Castano(joscasta@)" to use
valid GitHub handle format by replacing "jpeimer@", "kshvaika@" and "joscasta@"
with "@jpeimer", "@kshvaika" and "@joscasta" respectively so the entry reads
e.g. "Jenia Peimer (`@jpeimer`), Kate Shvaika (`@kshvaika`), Jose Manuel Castano
(`@joscasta`)".
- Line 171: The documentation contains a misspelling: replace the incorrect
identifier "retetionPolicy" with the correct "retentionPolicy" wherever it
appears (e.g., the bullet line "**[P0]** Verify source volumes can be
retained/deleted for offline VM storage migration when retetionPolicy is set");
update all occurrences to maintain consistency with the API/feature naming and
run a quick search to ensure no other references remain.
---
Nitpick comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Around line 384-402: The repeated epic ID "CNV-73500" in the
storage_mig_offline requirement list is redundant; to reduce visual noise, keep
the first "CNV-73500" entry and clear (leave empty) the epic ID cells/lines for
the subsequent list items within the same block so the requirement text remains
unchanged but the repeated ID is omitted—locate the block containing the four
subsequent bullet items (each starting with "- **[CNV-73500]**") and remove the
duplicate ID text from the later bullets, leaving their descriptions and test
scenarios intact.
- Around line 189-191: The template's placeholder out-of-scope checklist item
"[e.g., Testing of deprecated features]" must be replaced with either a concrete
exclusion (fill in the item, rationale, and PM/Lead Agreement fields) or removed
and replaced with the single word "None"; update the section in
storage_mig_offline.md where that checklist appears (look for the bullet "- [ ]
**[e.g., Testing of deprecated features]**") and ensure the PM/Lead Agreement
line includes a name and date if you add a real exclusion.
🪄 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: b8362740-c8ee-43e9-90cd-b42637eda146
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_offline.md
duyanyan
force-pushed
the
offline
branch
2 times, most recently
from
4fd3f68 to
cc81e7f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
stps/sig-storage/storage_mig_offline.md (4)
191-194:⚠️ Potential issue | 🟠 MajorOut-of-scope section is still a placeholder and not actionable.
Lines 191-194 keep template/example text instead of real exclusions (or
None), and PM/Lead agreement is not filled.Suggested fix
-- [ ] **[e.g., Testing of deprecated features]** - - *Rationale:* [Why this is excluded] - - *PM/Lead Agreement:* [Name/Date] +None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 191 - 194, The "Out-of-scope" subsection currently contains template placeholders (e.g., "**[e.g., Testing of deprecated features]**", "*Rationale:*", "*PM/Lead Agreement:*")—replace these with concrete exclusions or the explicit value "None" and provide the PM/Lead sign-off (name and date) where "*PM/Lead Agreement:*" appears; update the section header or checklist item text to be actionable and specific so it no longer reads like a template.
420-420:⚠️ Potential issue | 🟡 MinorUse consistent GitHub handle format for reviewer traceability.
Line 420 mixes malformed handles (
jpeimer@,kshvaika@,joscasta@) instead of@username.Suggested fix
- - QE Members (OCP-V): Jenia Peimer (jpeimer@), Kate Shvaika (kshvaika@), Jose Manuel Castano(joscasta@) + - QE Members (OCP-V): Jenia Peimer (`@jpeimer`), Kate Shvaika (`@kshvaika`), Jose Manuel Castano (`@joscasta`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` at line 420, Replace the malformed reviewer handles in the "QE Members (OCP-V): Jenia Peimer (jpeimer@), Kate Shvaika (kshvaika@), Jose Manuel Castano(joscasta@)" line with proper GitHub-style handles (e.g., `@jpeimer`, `@kshvaika`, `@joscasta`) so each reviewer is traceable; update the string exactly where "QE Members (OCP-V):" and the three names/handles appear to use the `@username` format consistently.
400-406:⚠️ Potential issue | 🟡 MinorUser story and scenario wording is grammatically unclear.
Lines 401-405 use non-idiomatic phrasing (“will be remained”, “I want to offline VM…”), which makes pass/fail intent harder to read.
Suggested fix
-- **[CNV-73500]** — As a VM owner, I want to retain/delete the source volume for offline VM - - *Test Scenario:* [Tier 2] Verify source volume will be remained/cleaned up for offline VM with retentionPolicy set in Plan +- **[CNV-73500]** — As a VM owner, I want to retain or delete the source volume for an offline VM + - *Test Scenario:* [Tier 2] Verify source volume is retained or cleaned up for an offline VM when retentionPolicy is set in the Migration Plan - *Priority:* P0 -- **[CNV-73500]** — As a VM owner, I want to offline VM still points to origin volume when migration is failed - - *Test Scenario:* [Tier 2] Verify offline VM points to origin volume when migration failed +- **[CNV-73500]** — As a VM owner, I want an offline VM to still point to the original volume when migration fails + - *Test Scenario:* [Tier 2] Verify an offline VM still points to the original volume when migration fails - *Priority:* P1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 400 - 406, Rewrite the two CNV-73500 bullet entries to use clear, idiomatic English: change "As a VM owner, I want to retain/delete the source volume for offline VM" to "As a VM owner, I want to retain or delete the source volume for an offline VM" and update the test scenario to "Verify that the source volume is retained or cleaned up for an offline VM when retentionPolicy is set in the Plan"; change "As a VM owner, I want to offline VM still points to origin volume when migration is failed" to "As a VM owner, I want an offline VM to still point to the original volume if migration fails" and update the test scenario to "Verify that an offline VM points to the original volume when migration fails"; keep the CNV-73500 IDs and priorities intact.
172-172:⚠️ Potential issue | 🟡 MinorFix typo in policy name.
Line 172 has
retetionPolicy; this should beretentionPolicy.Suggested fix
-- **[P0]** Verify source volumes can be retained/deleted for offline VM storage migration when retetionPolicy is set +- **[P0]** Verify source volumes can be retained/deleted for offline VM storage migration when retentionPolicy is set🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` at line 172, The document contains a typo: the token "retetionPolicy" should be corrected to "retentionPolicy"; locate the occurrence of "retetionPolicy" in the markdown (line with the bullet "**[P0]** Verify source volumes...") and replace it with the correct spelling "retentionPolicy" so the policy name is accurate.
🧹 Nitpick comments (1)
stps/sig-storage/storage_mig_offline.md (1)
388-410: Consider de-duplicating Requirement ID cells in traceability rows.For rows under the same epic, you can keep the first
CNV-73500and leave subsequent Requirement ID cells empty to reduce redundancy.Based on learnings: In this repository’s STP traceability tables, leaving Requirement ID empty after the first row for the same epic is acceptable and preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` around lines 388 - 410, The Requirement ID CNV-73500 is repeated for every traceability row; update the block so only the first row retains "CNV-73500" and clear the Requirement ID cells for subsequent rows under the same epic (the four following test rows: mixed VM states, hotplug disk, retain/delete source volume, migration failure and start-during-migration scenarios) to de-duplicate the traceability table entries while leaving all other text unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Around line 191-194: The "Out-of-scope" subsection currently contains template
placeholders (e.g., "**[e.g., Testing of deprecated features]**",
"*Rationale:*", "*PM/Lead Agreement:*")—replace these with concrete exclusions
or the explicit value "None" and provide the PM/Lead sign-off (name and date)
where "*PM/Lead Agreement:*" appears; update the section header or checklist
item text to be actionable and specific so it no longer reads like a template.
- Line 420: Replace the malformed reviewer handles in the "QE Members (OCP-V):
Jenia Peimer (jpeimer@), Kate Shvaika (kshvaika@), Jose Manuel
Castano(joscasta@)" line with proper GitHub-style handles (e.g., `@jpeimer`,
`@kshvaika`, `@joscasta`) so each reviewer is traceable; update the string exactly
where "QE Members (OCP-V):" and the three names/handles appear to use the
`@username` format consistently.
- Around line 400-406: Rewrite the two CNV-73500 bullet entries to use clear,
idiomatic English: change "As a VM owner, I want to retain/delete the source
volume for offline VM" to "As a VM owner, I want to retain or delete the source
volume for an offline VM" and update the test scenario to "Verify that the
source volume is retained or cleaned up for an offline VM when retentionPolicy
is set in the Plan"; change "As a VM owner, I want to offline VM still points to
origin volume when migration is failed" to "As a VM owner, I want an offline VM
to still point to the original volume if migration fails" and update the test
scenario to "Verify that an offline VM points to the original volume when
migration fails"; keep the CNV-73500 IDs and priorities intact.
- Line 172: The document contains a typo: the token "retetionPolicy" should be
corrected to "retentionPolicy"; locate the occurrence of "retetionPolicy" in the
markdown (line with the bullet "**[P0]** Verify source volumes...") and replace
it with the correct spelling "retentionPolicy" so the policy name is accurate.
---
Nitpick comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Around line 388-410: The Requirement ID CNV-73500 is repeated for every
traceability row; update the block so only the first row retains "CNV-73500" and
clear the Requirement ID cells for subsequent rows under the same epic (the four
following test rows: mixed VM states, hotplug disk, retain/delete source volume,
migration failure and start-during-migration scenarios) to de-duplicate the
traceability table entries while leaving all other text unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 368bf1f6-2fb8-4701-bb2b-493eabbad019
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_offline.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
stps/sig-storage/storage_mig_offline.md (1)
37-37:⚠️ Potential issue | 🟠 MajorRequirement traceability is incomplete for CNV-82430.
Line 37 says CNV-82430 requirements were reviewed, but Section III maps only CNV-73500 scenarios and Section II marks performance as not applicable. Please either add CNV-82430 traceable scenario(s) (or an explicit out-of-scope entry with stakeholder sign-off), or remove CNV-82430 from reviewed requirements to keep the plan internally consistent.
Also applies to: 217-221, 386-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_offline.md` at line 37, The doc claims CNV-82430 was reviewed (Line 37) but no traceability exists in Section III and Section II marks performance N/A; either add CNV-82430-specific mapped scenarios into Section III with links to the exact requirements and update Section II to include any relevant performance considerations and stakeholder sign-off, or explicitly mark CNV-82430 as out-of-scope with stakeholder sign-off in Section II and remove references to CNV-82430 (Line 37 and the other occurrences around lines 217-221 and 386-408); ensure consistency by updating the reviewed-requirements list to match the Section II/III entries.
🤖 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-storage/storage_mig_offline.md`:
- Around line 169-170: The test environment section currently only lists one
StorageClass but the P0 checklist requires validating migration between ODF and
HPP; update the environment block to include both StorageClasses with their
concrete names (e.g., the ODF StorageClass name and the HPP StorageClass name)
so the plan can run as written, and make the same addition for the repeated
section referenced around lines 264-277; ensure the StorageClass names are
explicit and clearly labeled (ODF vs HPP) next to the P0 checklist items so
readers know which SC each test uses.
---
Duplicate comments:
In `@stps/sig-storage/storage_mig_offline.md`:
- Line 37: The doc claims CNV-82430 was reviewed (Line 37) but no traceability
exists in Section III and Section II marks performance N/A; either add
CNV-82430-specific mapped scenarios into Section III with links to the exact
requirements and update Section II to include any relevant performance
considerations and stakeholder sign-off, or explicitly mark CNV-82430 as
out-of-scope with stakeholder sign-off in Section II and remove references to
CNV-82430 (Line 37 and the other occurrences around lines 217-221 and 386-408);
ensure consistency by updating the reviewed-requirements list to match the
Section II/III entries.
🪄 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: 5627398e-cdeb-4e89-ac63-93a7aff362ad
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_offline.md
Signed-off-by: Yan Du <yadu@redhat.com>
|
Clean rebase detected — no code changes compared to previous head ( |
|
Created new PR after Yan's leave. See: #76 |
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
9 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.
STP Metadata
VEP issue:
What this PR does
Special notes for your reviewer
Summary by CodeRabbit