storage: storage migration cleanup stp#47
Closed
duyanyan wants to merge 1 commit intoRedHatQE:mainfrom
Closed
Conversation
|
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. |
|
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 OpenShift Virtualization storage migration cleanup QE test plan documenting a retentionPolicy feature for migration plans, test scope, acceptance criteria, test strategy, environment assumptions, traceability of scenarios, and sign-off templates. 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)
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
josemacassan,
jpeimer,
kgoldbla and
kshvaika
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
stps/sig-storage/storage_mig_cleanup.md (2)
203-211: Review whether all risk categories are truly N/A.All risk categories are marked "N/A" without justification in the mitigation strategy column. Most features have at least minimal timeline, coverage, or dependency risks. Verify whether this accurately reflects the feature's risk profile or if specific risks should be documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` around lines 203 - 211, The risk table titled "Risk Category | Specific Risk for This Feature | Mitigation Strategy | Status" currently lists every category as "N/A" with empty mitigation strategies; review each row (Timeline/Schedule, Test Coverage, Test Environment, Untestable Aspects, Resource Constraints, Dependencies, Other) and either provide a concise mitigation entry or a brief justification for why it is truly N/A, and update the "Status" checkbox accordingly (e.g., mark items that require action). Ensure the "Mitigation Strategy" cell for each non-N/A row contains a clear, actionable step (e.g., add contingency schedule, test plan notes, environment provisioning steps, dependency tracking) and that the "Dependencies" row explicitly lists any external services or repos if applicable.
164-173: Consider adding more specific environment configurations.All environment components reference "PSI cluster Standard cluster" without specific details. While this may be sufficient if PSI cluster is a well-defined internal standard, consider documenting specific requirements for storage, network, or other components that are relevant to storage migration testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` around lines 164 - 173, The table rows currently repeat the placeholder text "PSI cluster Standard cluster" (e.g., the "**Storage**", "**Network**", "**Required Operators**", "**Platform**", and "**Special Configurations**" cells) which lacks actionable environment detail; replace those placeholders with concrete configuration values for each relevant field (for example: storage backend type and version, CSI driver name and version, network MTU/VLAN info, required operator names and versions, platform specifics such as cloud provider or bare-metal config, and any special tunings) so readers can reproduce the storage migration tests; update the "**Storage**" and "**Network**" cells first and ensure "**Required Operators**" lists exact operator artifacts and versions referenced elsewhere in the doc.
🤖 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_cleanup.md`:
- Line 9: Update the table row under the "Enhancement(s)" column (the
"Enhancement(s)" field in storage_mig_cleanup.md) to replace the placeholder
"[Links to enhancement(s)]" with either the actual enhancement PR link(s), or if
no OpenShift enhancement PR exists, reference the HLD document (provide its
link/title), and if neither exists explicitly set the value to "N/A - No
enhancement document available."; ensure the table cell contains the final
chosen text or link.
- Line 151: Update the "Upgrade Testing" table row so the Applicable column
reflects that upgrade testing is not relevant: change the "Y" to "N" for the
"Upgrade Testing" entry and update the comment cell (the text currently "Not
Upgrade relevant") to include a brief justification (e.g., "N — not applicable:
no upgrade path or data migration required") so the table is consistent and
documents why it's not applicable.
- Line 153: Update the "Dependencies" table row in storage_mig_cleanup.md (the
"Dependencies" entry) to replace the vague note "CNV 4.22 is ready" with a clear
dependency statement that identifies the dependent component, the team
responsible, and what that team will test; e.g., state that the CNV 4.22 release
from the CNV team is required, list which CNV features or deliverables are
relied upon, and specify which tests the CNV team will perform versus which
tests the storage migration team will execute.
- Around line 237-241: The table rows listing retentionPolicy values (e.g.,
"retentionPolicy: keepSource", "retentionPolicy: deleteSource",
"retentionPolicy: None", and the two "Migration failed..." rows) currently have
Requirement IDs set to "TBD"; create or identify the corresponding Jira
requirement tickets for each of these five test cases and replace each "TBD"
with the actual Jira issue key (e.g., PROJ-1234) so the table entries link to
traceable requirements; ensure the Jira issues describe the same verification
objective as the row text and then update the table entries to use those Jira
IDs.
- Around line 237-241: Fix the typographical errors in the test scenario table
rows that reference retentionPolicy: replace "migtation" with "migration" in the
rows mentioning migration (both success and failure cases), change "cleanuped"
to "cleaned" in the retentionPolicy: deleteSource success row, and change
"beahvior" to "behavior" in the default retentionPolicy row; update the table
cells that contain these phrases (look for the rows containing "retentionPolicy:
keepSource", "retentionPolicy: deleteSource", "retentionPolicy: None", and the
"Migration failed with ..." rows) to correct the spelling.
- Around line 42-64: The STP review tables in storage_mig_cleanup.md have all
checklist items left unchecked; complete the "Review Requirements" and
"Technology and Design Review" tables by marking the Done column where items are
satisfied and filling the Details/Notes and Comments for each row (notably
"Review Requirements", "Understand Value", "Acceptance Criteria", and "Developer
Handoff/QE Kickoff", "Technology Challenges", "Test Environment Needs", "API
Extensions", "Topology Considerations") with concise summaries of coverage,
testability, environment needs, and any risks/blockers so the document meets
entry criteria for approval and QE kickoff.
- Around line 132-135: The out-of-scope table rows "Other storage migration
tests" and "Storage migration UI tests" currently lack stakeholder sign-off;
update the PM/Lead Agreement column for those rows by adding the responsible
PM/Lead name(s) and the sign-off date (e.g., "Jane Doe / 2026-03-09") so each
out-of-scope item has explicit owner/date agreement before finalizing the STP.
- Around line 250-255: Change the "Reviewers:" and "Approvers:" setext-style
headings to ATX-style headings (e.g., prefix with "### " or "#### ") so they
match the rest of the document; locate the literal lines containing "Reviewers:"
and "Approvers:" in the file and replace the underlined setext format with the
equivalent ATX header markers while preserving the list items below each
heading.
---
Nitpick comments:
In `@stps/sig-storage/storage_mig_cleanup.md`:
- Around line 203-211: The risk table titled "Risk Category | Specific Risk for
This Feature | Mitigation Strategy | Status" currently lists every category as
"N/A" with empty mitigation strategies; review each row (Timeline/Schedule, Test
Coverage, Test Environment, Untestable Aspects, Resource Constraints,
Dependencies, Other) and either provide a concise mitigation entry or a brief
justification for why it is truly N/A, and update the "Status" checkbox
accordingly (e.g., mark items that require action). Ensure the "Mitigation
Strategy" cell for each non-N/A row contains a clear, actionable step (e.g., add
contingency schedule, test plan notes, environment provisioning steps,
dependency tracking) and that the "Dependencies" row explicitly lists any
external services or repos if applicable.
- Around line 164-173: The table rows currently repeat the placeholder text "PSI
cluster Standard cluster" (e.g., the "**Storage**", "**Network**", "**Required
Operators**", "**Platform**", and "**Special Configurations**" cells) which
lacks actionable environment detail; replace those placeholders with concrete
configuration values for each relevant field (for example: storage backend type
and version, CSI driver name and version, network MTU/VLAN info, required
operator names and versions, platform specifics such as cloud provider or
bare-metal config, and any special tunings) so readers can reproduce the storage
migration tests; update the "**Storage**" and "**Network**" cells first and
ensure "**Required Operators**" lists exact operator artifacts and versions
referenced elsewhere in the doc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aef673f3-abab-437f-9b99-ab2a705c2cc3
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_cleanup.md
| | Regression Testing | Verifies that new changes do not break existing functionality | N | Ensure storage migration functionality will be affected by new implemented cleanup code | | ||
| | Upgrade Testing | Validates upgrade paths from previous versions, data migration, and configuration preservation | Y | Not Upgrade relevant | | ||
| | Backward Compatibility Testing | Ensures feature maintains compatibility with previous API versions and configurations | N | No Compatibility testing currently | | ||
| | Dependencies | Dependent on deliverables from other components/products? Identify what is tested by which team. | Y | CNV 4.22 is ready | |
There was a problem hiding this comment.
Clarify Dependencies entry.
The item is marked applicable ("Y") but the comment "CNV 4.22 is ready" doesn't describe the dependency relationship or identify which team tests what. Per the template instructions, dependencies should "Identify what is tested by which team."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-storage/storage_mig_cleanup.md` at line 153, Update the
"Dependencies" table row in storage_mig_cleanup.md (the "Dependencies" entry) to
replace the vague note "CNV 4.22 is ready" with a clear dependency statement
that identifies the dependent component, the team responsible, and what that
team will test; e.g., state that the CNV 4.22 release from the CNV team is
required, list which CNV features or deliverables are relied upon, and specify
which tests the CNV team will perform versus which tests the storage migration
team will execute.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
stps/sig-storage/storage_mig_cleanup.md (3)
151-151:⚠️ Potential issue | 🟡 MinorFix contradictory Upgrade Testing entry.
The Applicable column shows "Y" but the comment states "Not Upgrade relevant." If upgrade testing is not relevant, change the Applicable column to "N" to match the comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` at line 151, The table row for "Upgrade Testing" has a contradictory Applicable value ("Y") versus the comment "Not Upgrade relevant"; update that row so the Applicable column is "N" to match the comment. Locate the row containing the text "Upgrade Testing" and change the single-character cell currently set to "Y" to "N" (leave the rest of the row text unchanged). Ensure the Markdown table alignment and spacing remain intact after the edit.
238-238:⚠️ Potential issue | 🟡 MinorFix typographical error.
Line 238 contains "migtation" which should be "migration".
✍️ Proposed fix
-| TBD | retentionPolicy: deleteSource | Verify source DV/PVC will be cleaned up after migtation completed | Tier 2 | P0 | +| TBD | retentionPolicy: deleteSource | Verify source DV/PVC will be cleaned up after migration completed | Tier 2 | P0 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` at line 238, Replace the typo "migtation" with the correct word "migration" in the document string `Verify source DV/PVC will be cleaned up after migtation completed` (found in storage_mig_cleanup.md) so the line reads `Verify source DV/PVC will be cleaned up after migration completed`.
237-241:⚠️ Potential issue | 🟡 MinorFill in Requirement IDs before finalizing the STP.
All Requirement IDs remain "TBD". These must be linked to actual Jira issues for traceability before the test plan can be approved. Create or identify the appropriate Jira requirements and update this table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` around lines 237 - 241, Replace all "TBD" Requirement IDs in the table rows for retentionPolicy cases (the rows with "retentionPolicy: keepSource", "retentionPolicy: deleteSource", "retentionPolicy: None", "Migration failed with retentionPolicy keepSource", and "Migration failed with retentionPolicy deleteSource") with actual Jira requirement IDs; either link existing Jira tickets or create new ones and use their IDs, ensuring each table entry now contains a unique Jira ID for traceability and approval.
🧹 Nitpick comments (1)
stps/sig-storage/storage_mig_cleanup.md (1)
203-211: Complete risk assessment with justifications.All risk categories are marked "N/A" without providing justification in the Mitigation Strategy column. Per the template, "N/A" requires explicit justification. For each N/A entry, briefly explain why that risk category does not apply to this feature (e.g., "Timeline/Schedule - N/A: feature already complete and ready for testing").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` around lines 203 - 211, The risk table currently marks every Risk Category as "N/A" without justification; update the Mitigation Strategy column for each row (Timeline/Schedule, Test Coverage, Test Environment, Untestable Aspects, Resource Constraints, Dependencies, Other) with a short justification stating why that category does not apply (e.g., "Timeline/Schedule - N/A: feature already complete and ready for testing", "Test Coverage - N/A: no new code paths introduced", etc.), keeping each justification concise and specific so the reviewer can understand why N/A was chosen.
🤖 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_cleanup.md`:
- Around line 162-173: The environment specification table in
storage_mig_cleanup.md has empty Configuration cells and non-informative
Specification Examples (repeating "PSI cluster Standard cluster"); update each
row (e.g., **Cluster Topology**, **OCP & OpenShift Virtualization Version(s)**,
**CPU Virtualization**, **Compute Resources**, **Special Hardware**,
**Storage**, **Network**, **Required Operators**, **Platform**, **Special
Configurations**) to populate the Configuration column with concrete values
(node counts, CPU/memory per node, storage class/type and capacity, network
CIDR/VLAN/MTU, required operator versions) and replace the placeholder Spec
Examples with meaningful examples reflecting those configs, or explicitly mark a
cell "N/A" and add a one-line justification for why it is not applicable.
---
Duplicate comments:
In `@stps/sig-storage/storage_mig_cleanup.md`:
- Line 151: The table row for "Upgrade Testing" has a contradictory Applicable
value ("Y") versus the comment "Not Upgrade relevant"; update that row so the
Applicable column is "N" to match the comment. Locate the row containing the
text "Upgrade Testing" and change the single-character cell currently set to "Y"
to "N" (leave the rest of the row text unchanged). Ensure the Markdown table
alignment and spacing remain intact after the edit.
- Line 238: Replace the typo "migtation" with the correct word "migration" in
the document string `Verify source DV/PVC will be cleaned up after migtation
completed` (found in storage_mig_cleanup.md) so the line reads `Verify source
DV/PVC will be cleaned up after migration completed`.
- Around line 237-241: Replace all "TBD" Requirement IDs in the table rows for
retentionPolicy cases (the rows with "retentionPolicy: keepSource",
"retentionPolicy: deleteSource", "retentionPolicy: None", "Migration failed with
retentionPolicy keepSource", and "Migration failed with retentionPolicy
deleteSource") with actual Jira requirement IDs; either link existing Jira
tickets or create new ones and use their IDs, ensuring each table entry now
contains a unique Jira ID for traceability and approval.
---
Nitpick comments:
In `@stps/sig-storage/storage_mig_cleanup.md`:
- Around line 203-211: The risk table currently marks every Risk Category as
"N/A" without justification; update the Mitigation Strategy column for each row
(Timeline/Schedule, Test Coverage, Test Environment, Untestable Aspects,
Resource Constraints, Dependencies, Other) with a short justification stating
why that category does not apply (e.g., "Timeline/Schedule - N/A: feature
already complete and ready for testing", "Test Coverage - N/A: no new code paths
introduced", etc.), keeping each justification concise and specific so the
reviewer can understand why N/A was chosen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffc14f6e-2bc4-469a-9c5e-1a66fb4597f0
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_cleanup.md
openshift-virtualization-qe-bot-4
added
needs-rebase
and removed
commented-coderabbitai[bot]
labels
There was a problem hiding this comment.
♻️ Duplicate comments (4)
stps/sig-storage/storage_mig_cleanup.md (4)
247-273:⚠️ Potential issue | 🟡 MinorReplace
[TBD]requirement identifiers with traceable Jira mapping.Traceability is still incomplete with
[TBD]across all scenarios. At minimum, use the epic key in the first row and (optionally) leave subsequent rows blank when they map to the same epic.Based on learnings, in this repository it is acceptable to reuse one epic ID for related scenario rows and leave subsequent Requirement ID cells empty 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_cleanup.md` around lines 247 - 273, Replace the literal “[TBD]” requirement IDs in the table rows for VirtualMachineStorageMigrationPlan and MultiNamespaceVirtualMachineStorageMigrationPlan with a traceable Jira epic key (use the epic key once in the first related row); for subsequent rows that map to the same epic leave the Requirement ID cell empty per repo conventions; ensure every distinct scenario that belongs to a different epic gets its own Jira key and update the rows referencing retentionPolicy, namespace/spec combinations, and failure cases accordingly so no row retains “[TBD]”.
105-112:⚠️ Potential issue | 🟡 MinorFill PM/Lead agreement owners and dates for out-of-scope items.
Both out-of-scope entries still have placeholder agreement fields. Add explicit owner/date to avoid scope ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` around lines 105 - 112, Update the two out-of-scope checklist items that still show placeholder agreement fields by replacing "[ ] Name/Date" with the actual PM/Lead owner and date for each item; specifically edit the "Other storage migration tests" line and the "Storage migration UI tests" line to include the responsible person's name and the agreed date (e.g., "Alice Smith / 2026-03-24") so the markdown entries are no longer placeholders and clearly record the PM/Lead agreement.
252-252:⚠️ Potential issue | 🟡 MinorFix spelling typo:
remianed→remained.The test scenario text has repeated typos that reduce document quality and searchability.
✍️ Proposed text fix
- - *Test Scenario:* [Tier 2] Verify source DV/PVC will be remianed/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan + - *Test Scenario:* [Tier 2] Verify source DV/PVC will be remained/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan - - *Test Scenario:* [Tier 2] Verify source DV/PVC will be remianed/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan + - *Test Scenario:* [Tier 2] Verify source DV/PVC will be remained/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan - - *Test Scenario:* [Tier 2] Verify source DV/PVC will be remianed/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan + - *Test Scenario:* [Tier 2] Verify source DV/PVC will be remained/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlanAlso applies to: 256-256, 260-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` at line 252, Replace the misspelled word "remianed" with "remained" in the Test Scenario lines that mention "Verify source DV/PVC will be remianed/cleaned up after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan"; update all occurrences (the repeated instances around the same Test Scenario text) so the phrase reads "...will be remained/cleaned up..." corrected to "...will be remained/cleaned up..." — actually change "remianed" -> "remained" in each occurrence.
41-51:⚠️ Potential issue | 🟡 MinorComplete the remaining QE checklists before approval.
Line 41-51 and Line 69-77 still leave core planning gates unchecked (Acceptance Criteria, NFRs, API Extensions, Test Environment Needs, Topology Considerations). These should be marked and briefly filled before STP sign-off readiness.
Also applies to: 69-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` around lines 41 - 51, Several QE checklist sections are left unchecked; update the STP doc by completing the Acceptance Criteria and NFRs and filling the remaining planning gates (API Extensions, Test Environment Needs, Topology Considerations) referenced in the headings "Acceptance Criteria" and "Non-Functional Requirements (NFRs)" and the other checklist blocks around lines 41-51 and 69-77: explicitly list the missing acceptance items (e.g., retentionPolicy behaviors for VirtualMachineStorageMigrationPlan and MultiNamespaceVirtualMachineStorageMigrationPlan including namespace/spec/combined cases), specify NFR targets (documentation, performance, reliability) under "Non-Functional Requirements (NFRs)", and add brief entries for "API Extensions", "Test Environment Needs", and "Topology Considerations" describing required API changes, test cluster requirements, and topology impacts so the STP is fully filled before sign-off.
🤖 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_cleanup.md`:
- Around line 247-273: Replace the literal “[TBD]” requirement IDs in the table
rows for VirtualMachineStorageMigrationPlan and
MultiNamespaceVirtualMachineStorageMigrationPlan with a traceable Jira epic key
(use the epic key once in the first related row); for subsequent rows that map
to the same epic leave the Requirement ID cell empty per repo conventions;
ensure every distinct scenario that belongs to a different epic gets its own
Jira key and update the rows referencing retentionPolicy, namespace/spec
combinations, and failure cases accordingly so no row retains “[TBD]”.
- Around line 105-112: Update the two out-of-scope checklist items that still
show placeholder agreement fields by replacing "[ ] Name/Date" with the actual
PM/Lead owner and date for each item; specifically edit the "Other storage
migration tests" line and the "Storage migration UI tests" line to include the
responsible person's name and the agreed date (e.g., "Alice Smith / 2026-03-24")
so the markdown entries are no longer placeholders and clearly record the
PM/Lead agreement.
- Line 252: Replace the misspelled word "remianed" with "remained" in the Test
Scenario lines that mention "Verify source DV/PVC will be remianed/cleaned up
after migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan";
update all occurrences (the repeated instances around the same Test Scenario
text) so the phrase reads "...will be remained/cleaned up..." corrected to
"...will be remained/cleaned up..." — actually change "remianed" -> "remained"
in each occurrence.
- Around line 41-51: Several QE checklist sections are left unchecked; update
the STP doc by completing the Acceptance Criteria and NFRs and filling the
remaining planning gates (API Extensions, Test Environment Needs, Topology
Considerations) referenced in the headings "Acceptance Criteria" and
"Non-Functional Requirements (NFRs)" and the other checklist blocks around lines
41-51 and 69-77: explicitly list the missing acceptance items (e.g.,
retentionPolicy behaviors for VirtualMachineStorageMigrationPlan and
MultiNamespaceVirtualMachineStorageMigrationPlan including
namespace/spec/combined cases), specify NFR targets (documentation, performance,
reliability) under "Non-Functional Requirements (NFRs)", and add brief entries
for "API Extensions", "Test Environment Needs", and "Topology Considerations"
describing required API changes, test cluster requirements, and topology impacts
so the STP is fully filled before sign-off.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70251a7a-9f08-46ed-b293-d794368fc93f
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_cleanup.md
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
stps/sig-storage/storage_mig_cleanup.md (1)
96-96: Consider hyphenating compound modifiers.For improved readability, compound adjectives before nouns are typically hyphenated: "namespace-level and spec-level retentionPolicy".
📝 Proposed fix
- - combination of namespace level and spec level retentionPolicy + - combination of namespace-level and spec-level retentionPolicy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/storage_mig_cleanup.md` at line 96, Update the phrase "namespace level and spec level retentionPolicy" to hyphenate the compound modifiers so it reads "namespace-level and spec-level retentionPolicy" to improve readability; locate the exact string in storage_mig_cleanup.md and replace it accordingly.
🤖 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_cleanup.md`:
- Around line 154-155: Update the "Dependencies" section currently marked "No
Dependencies" to explicitly list CNV 4.22 with the migration controller as a
required dependency and clarify its status (e.g., "CNV 4.22 with migration
controller — assumed available in test env" or "CNV 4.22 with migration
controller — requires coordination with CNV team"); also add brief bullets
stating which parts the storage team will test (e.g., storage migration flows,
PV handling) versus which parts require CNV team verification (e.g., migration
controller behavior, cluster-level coordination) so readers know whether this is
blocking or non-blocking and who to contact for coordination.
- Around line 41-80: The checklist items currently show content but remain
unchecked: mark the boxes for "Acceptance Criteria", "Non-Functional
Requirements (NFRs)", "API Extensions", and "Topology Considerations" as
completed by changing their unchecked markers (- [ ]) to checked (- [x]) so the
document accurately reflects that the sections "Acceptance Criteria",
"Non-Functional Requirements", "API Extensions", and "Topology Considerations"
have been filled and reviewed.
- Line 258: Fix the typo in the test scenario sentence in storage_mig_cleanup.md
by replacing "remianed" with "remained" in the line that reads "*Test Scenario:*
[Tier 2] Verify source DV/PVC will be remianed/cleaned up after migration
completed in MultiNamespaceVirtualMachineStorageMigrationPlan" so the sentence
reads "...will be remained/cleaned up..." (or better "...will remain/cleaned
up..." if you prefer correct grammar).
- Line 254: Fix the typographical error in the Test Scenario line where the
phrase "remianed" is misspelled; locate the string "Verify source DV/PVC will be
remianed/cleaned up after migration completed in
MultiNamespaceVirtualMachineStorageMigrationPlan" and change "remianed" to
"remained" so it reads "Verify source DV/PVC will be remained/cleaned up after
migration completed in MultiNamespaceVirtualMachineStorageMigrationPlan".
- Line 262: Fix the typographical error in the Test Scenario string: locate the
line containing "*Test Scenario:* [Tier 2] Verify source DV/PVC will be
remianed/cleaned up after migration completed in
MultiNamespaceVirtualMachineStorageMigrationPlan" and correct "remianed" to
"remained" so the sentence reads "...will be remained/cleaned up..." (ensure the
exact phrase in the markdown is updated).
---
Nitpick comments:
In `@stps/sig-storage/storage_mig_cleanup.md`:
- Line 96: Update the phrase "namespace level and spec level retentionPolicy" to
hyphenate the compound modifiers so it reads "namespace-level and spec-level
retentionPolicy" to improve readability; locate the exact string in
storage_mig_cleanup.md and replace it accordingly.
🪄 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: b13ad7fd-7b2b-4d0f-9a1a-75692f06fcb4
📒 Files selected for processing (1)
stps/sig-storage/storage_mig_cleanup.md
Comment on lines
+154
to
+155
| - [ ] **Dependencies** — Blocked by deliverables from other components/products. Identify what we need from other teams before we can test. | ||
| - *Details:* No Dependencies |
There was a problem hiding this comment.
Clarify the Dependencies statement.
The document states "No Dependencies" but requires CNV 4.22 with migration controller (line 182). Consider clarifying whether:
- CNV 4.22 dependencies are already satisfied (non-blocking)
- Any coordination is needed with the CNV team for testing boundaries
- Which aspects are tested by the storage team vs. other teams
This helps prevent "I assumed you were testing that" issues during execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-storage/storage_mig_cleanup.md` around lines 154 - 155, Update the
"Dependencies" section currently marked "No Dependencies" to explicitly list CNV
4.22 with the migration controller as a required dependency and clarify its
status (e.g., "CNV 4.22 with migration controller — assumed available in test
env" or "CNV 4.22 with migration controller — requires coordination with CNV
team"); also add brief bullets stating which parts the storage team will test
(e.g., storage migration flows, PV handling) versus which parts require CNV team
verification (e.g., migration controller behavior, cluster-level coordination)
so readers know whether this is blocking or non-blocking and who to contact for
coordination.
jpeimer
reviewed
Mar 24, 2026
Contributor
|
/approve |
|
/lgtm |
rnetser
requested changes
Apr 5, 2026
|
|
||
| ### **Feature Overview** | ||
|
|
||
| The storage migration cleanup feature allows the system to optionally decommission migration plans and legacy PVCs upon successful completion. This feature introduces a `retentionPolicy` field to `VirtualMachineStorageMigrationPlan` and `MultiNamespaceVirtualMachineStorageMigrationPlan` resources, giving user control over whether to keep (`keepSource`) or delete (`deleteSource`) source DataVolumes/PVCs after a successful VM storage migration. |
Contributor
There was a problem hiding this comment.
please remove implemnetation details; explain the behavior of the feature.
something like "After storage migration, administrators can choose whether to keep or delete the original storage volumes."
Comment on lines
+107
to
+109
| - [ ] **Other storage migration tests** | ||
| - *Rationale:* covered in 4.21 already | ||
| - *PM/Lead Agreement:* [ ] Name/Date |
Contributor
There was a problem hiding this comment.
why is this out of scope? if something is already covered and tested than it should not be listed here
note that ANY out of scope item must have pm sign off
Comment on lines
+111
to
+113
| - [ ] **Storage migration UI tests** | ||
| - *Rationale:* covered in UI team | ||
| - *PM/Lead Agreement:* [ ] Name/Date |
Contributor
There was a problem hiding this comment.
covered in the Usability Testing section; please remove
| - *Details:* Not security relevant | ||
|
|
||
| - [x] **Usability Testing** — Validates user experience and accessibility requirements | ||
| - *Details:* Will be covered by UI team |
Contributor
There was a problem hiding this comment.
please add a link to the ui epic
|
|
||
| - **Special Hardware:** N/A | ||
|
|
||
| - **Storage:** Standard |
Contributor
There was a problem hiding this comment.
what does Standard mean in the context of storage? is this feature going to be tested with a specific sc? if so, please add
|
|
||
| - **Enhancement(s):** N/A | ||
| - **Feature Tracking:** [CNV-73509](https://issues.redhat.com/browse/CNV-73509) | ||
| - **Epic Tracking:** [CNV-81263](https://issues.redhat.com/browse/CNV-81263) |
Contributor
There was a problem hiding this comment.
please link to the epic (and not the stp task)
Comment on lines
+280
to
+285
| * **Reviewers:** | ||
| - [Name / @github-username] | ||
| - [Name / @github-username] | ||
| * **Approvers:** | ||
| - [Name / @github-username] | ||
| - [Name / @github-username] |
Comment on lines
+43
to
+48
| - RetentionPolicy works with VirtualMachineStorageMigrationPlan | ||
| - Namespace level retentionPolicy works with MultiNamespaceVirtualMachineStorageMigrationPlan | ||
| - Spec level retentionPolicy works with MultiNamespaceVirtualMachineStorageMigrationPlan | ||
| - Combination of namespace level and spec level retentionPolicy works with MultiNamespaceVirtualMachineStorageMigrationPlan | ||
| - Default retentionPolicy is keepSource | ||
| - RetentionPolicy will not clean up the volume when migration failed |
Contributor
There was a problem hiding this comment.
please do not include implementation details in the STP, e.g: "retentionPolicy works with VirtualMachineStorageMigrationPlan" — rephrase as user outcomes:
- "Source volumes are retained after successful migration (default behavior)"
- "Source volumes are deleted after successful migration when cleanup is requested"
- "Source volumes are preserved when migration fails, regardless of cleanup setting"
|
|
||
| **Testing Goals** | ||
|
|
||
| - **[P0]** Verify retentionPolicy (keepSource or deleteSource) works with VirtualMachineStorageMigrationPlan |
Contributor
There was a problem hiding this comment.
testing goals are too vague; testing goals must be neasurable, e.g: "Verify source volumes are deleted/retained per the cleanup policy after successful offline VM storage migration."
| - **[TBD]** — Migration failed with retentionPolicy keepSource/deleteSource | ||
| - *Test Scenario:* [Tier 2] Verify source DV/PVC will be affected when migration failed | ||
| - *Priority:* P2 | ||
|
|
Contributor
There was a problem hiding this comment.
what about a scenario where the source volume is still in use by another VM?
Author
There was a problem hiding this comment.
Do you mean the VM is running and the volume is in use? then it is covered by online VM migration
openshift-virtualization-qe-bot
requested review from
acinko-rh and
ema-aka-young
|
I have created new PR for taking over Yan's work on this one. See: #78 |
Contributor
|
closing, see #47 (comment) |
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
10 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:
https://issues.redhat.com/browse/CNV-73509
What this PR does
add storage migration cleanup stp
Special notes for your reviewer
Summary by CodeRabbit