[Storage] STP: File-Level Restore#121
Conversation
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
|
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:
📝 WalkthroughWalkthroughAdded a new Software Test Plan for VIRTSTRAT-480 file-level restore. It defines feature metadata, review checkpoints, scope and strategy, risks, traceability to linked stories, and sign-off. The document also records restore modes, limitations, infrastructure needs, and entry criteria. ChangesFile-level restore STP
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-2
requested review from
Acedus,
Ahmad-Hafe,
akalenyu,
alromeros,
arnongilboa,
awels,
dalia-frank,
dsanatar,
josemacassan,
kgoldbla and
noamasu
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stps/sig-storage/VIRTSTRAT-480_file_level_restore.md`:
- Around line 68-81: The acceptance criteria in VIRTSTRAT-480 need a verifiable
non-disruption check instead of only stating that restore happens “without
interrupting the VM’s availability.” Update the acceptance criteria section to
add a condition tied to continuous guest accessibility during the restore, such
as uninterrupted VM network connectivity and guest responsiveness throughout the
operation, so the check would fail if the implementation stopped and restarted
the VM. Keep the wording aligned with the existing seamless restore items and
ensure the new criterion is explicit enough to validate in tests.
- Around line 83-93: The NFRs section is missing required categories, so update
the checklist in the restore spec to explicitly cover UI, Documentation, and
Observability alongside the existing Security/Usability/Monitoring items. In the
NFRs block, use the relevant spec symbols/section headings to add a clear
PM/UX-backed statement for UI (even if no UI changes are planned), note whether
any user-facing docs or configuration guidance will be validated under
Documentation, and make Observability explicit if it is being treated as part of
Monitoring; keep Performance and Scalability as justified deferrals.
- Around line 94-113: Replace every approval-blocking placeholder in the STP
with real sign-offs and names, including the Sign-off entries in Known
Limitations, Test Limitations, and Risks, the PM/Lead Agreement entries in Out
of Scope, and the Approval section placeholders. Update the affected sections in
the document by using the existing labels like Known Limitations, Out of Scope,
Test Limitations, Risks, and Approval, and populate each `[TBD]` or `TBD` field
with the actual reviewer name and date/handle as required.
- Around line 157-189: Update the P2 testing goal about path formatting
variations so it names the missing configuration dimensions needed to make the
test actionable. In the testing goals list, revise the entry for the restore
path-variation case to explicitly call out the guest OS and filesystem contexts,
using the existing restore test items as the reference point for consistency.
Keep the scope focused on the restore workflow, but make sure the goal clearly
states which Linux/Windows and filesystem combinations it applies to.
- Around line 416-423: The file restore scenarios are incorrectly marked as Tier
3 even though the STP guidelines only allow Tier 1 or Tier 2. Update the
affected scenario entries for Windows VM backup PVC restore, Windows VM volume
snapshot restore, Windows VM manual file browsing, and concurrent restore on
different VMs to use Tier 2, or add explicit repository-level justification if
Tier 3 must remain. Keep the scenario wording intact and only change the tier
classification in the relevant bullet items.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 857d770d-6168-49c1-8978-f8f633f8a6c0
📒 Files selected for processing (1)
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
Code Review
Found 8 issue(s) in this PR:
💡 Suggestions (8)
| File | Line | Issue |
|---|---|---|
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
73 | [CRITICAL] Non-disruptive restore AC lacks falsifiable verification. |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
17 | [HIGH] Feature Maturity uses "TBD" for TP and GA — not an allowed value. |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
337 | [HIGH] Test Environment risk is missing the required supplemental field. |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
343 | [HIGH] Untestable Aspects risk is missing the required supplemental field. |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
240 | [HIGH] Automation Testing strategy references non-existent "Tier 3". |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
188 | [HIGH] Testing Goal P0 #9 overlaps with P0 #1 and #2. |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
167 | [MEDIUM] P0 priority overuse — all 9 testing goals are P0, which dilutes "bl |
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md |
193 | [MEDIUM] Compound testing goal is a grab-bag of 5+ distinct behaviors. |
Review generated by pi
| - Data protection partners can restore single files or a subset of user data into a VM | ||
| - Users do not need to restore an entire VM to recover a small set of files | ||
| - Backup vendor can trigger file-level restore from a backup PVC | ||
| - VM Admin/User can restore specific files/directories from a VolumeSnapshot into a running VM without interrupting the VM's availability — the VM remains network-reachable and guest-responsive throughout the restore operation (no reboot, no pause) |
There was a problem hiding this comment.
[CRITICAL] Non-disruptive restore AC lacks falsifiable verification.
The AC says "VM remains network-reachable and guest-responsive throughout the restore operation (no reboot, no pause)" — but the matching test scenario (line 381) just says "without interrupting VM availability."
A simple end-state check (VM is running after restore) would pass even after a disruptive stop-and-restart. The AC and its matching scenario need to specify the continuous verification mechanism.
Suggested rewrite for the AC:
VM Admin/User can restore specific files/directories from a VolumeSnapshot into a running VM — a continuous connectivity check (ping or TCP connection) shows zero interruption during restore, and a long-running guest process completes without restart
And for the scenario (line 381):
Verify files and directories are restored from a volume snapshot while a continuous connectivity check confirms zero interruption to the running VM
There was a problem hiding this comment.
I don't see why we would need to rewrite the AC, but I'll proceed with the update for the test scenario.
| - [CNV-89229](https://issues.redhat.com/browse/CNV-89229) - File-Level Restore: API extensions and support encryption | ||
| - **Feature Maturity:** | ||
| - DP: CNV 5.0.0 | ||
| - TP: TBD |
There was a problem hiding this comment.
[HIGH] Feature Maturity uses "TBD" for TP and GA — not an allowed value.
AGENTS.md requires: TP: [version or N/A], GA: [version]. Use actual target versions or N/A if not yet determined.
| - TP: TBD | |
| - TP: N/A | |
| - GA: N/A |
There was a problem hiding this comment.
ACK - we should enforce this through Quality Flow.
|
|
||
| - **Risk:** Windows guest testing may have limited coverage due to image availability and configuration complexity | ||
| - **Mitigation:** Prioritize Linux guest testing for Dev Preview. Establish Windows test VM image with SSH pre-configured. | ||
| - *Areas with reduced coverage:* Windows guest restore |
There was a problem hiding this comment.
[HIGH] Test Environment risk is missing the required supplemental field.
Per AGENTS.md II.5: when a risk exists, the category-specific supplemental field is required. For Test Environment, add:
*Missing or unavailable environments:* LVM-backed storage provisioner in standard CI clusters
There was a problem hiding this comment.
This is my bad - I thought Quality Flow was diverging from the templates. I'll update in the next iteration.
| **Test Environment** | ||
|
|
||
| - **Risk:** LVM-backed storage for UUID collision testing may not be available in standard CI environments | ||
| - **Mitigation:** Use dedicated test environment with LVM provisioner or mock the UUID collision scenario at the operator level. |
There was a problem hiding this comment.
[HIGH] Untestable Aspects risk is missing the required supplemental field.
Per AGENTS.md II.5: when a risk exists, the category-specific supplemental field is required. For Untestable Aspects, add:
*Reason untestable and mitigation approach:* Third-party vendor backup formats are proprietary; tested via CRD API surface and PVC-based restore path
| - [P1] Verify the file restore operator deploys and operates correctly as an HCO-managed component | ||
| - [P2] Verify cross-namespace restore from a volume in a different namespace completes with temporary resources cleaned up after completion | ||
| - [P2] Verify restore succeeds when the source volume's storage mode differs from the cluster default | ||
| - [P2] Verify restore from an LVM-based snapshot handles volume identifier collisions without mount failures |
There was a problem hiding this comment.
[HIGH] Testing Goal P0 #9 overlaps with P0 #1 and #2.
"Verify end-to-end restore workflow from restore request creation to file verification using the file restore API" appears to be a summary of P0 #1 (restore from backup volume) and P0 #2 (restore from volume snapshot).
If it tests the same flows, it's redundant — remove it. If it tests something distinct (e.g., a specific multi-step API-driven workflow not covered by #1/#2), clarify what distinguishes it.
| - [P0] Verify a backup vendor can restore files from a backup volume into a running Linux VM with file integrity preserved (size, ownership, permissions) | ||
| - [P0] Verify a VM user can restore files from a volume snapshot into a running VM without interrupting the VM's availability | ||
| - [P0] Verify manual restore mode provides read-only access to backup contents and the system cleans up when the restore request is removed | ||
| - [P0] Verify the system correctly detects whether a VM runs Linux or Windows and selects the appropriate restore method |
There was a problem hiding this comment.
[MEDIUM] P0 priority overuse — all 9 testing goals are P0, which dilutes "blocks GA" meaning.
5 of the 9 P0 goals are error-handling/validation scenarios (clear errors at each stage, reject invalid requests, cleanup, security). Error handling is important but having ALL goals as P0 means nothing is differentiated.
Consider keeping core happy-path goals (restore from backup, restore from snapshot, manual mode, OS detection) as P0 and moving error-handling scenarios to P1.
There was a problem hiding this comment.
Not sure I understand this specific goal
There was a problem hiding this comment.
@rnetser I will re-evaluate them, although if P0 equals to "blocks GA" then I'd say error validation does it from my perspective.
There was a problem hiding this comment.
@arnongilboa I'll rephrase the "selects the appropriate restore method" as per Slack conversation
| - [P2] Verify paths with formatting variations (trailing slashes, double slashes) are normalized and restore succeeds on Linux (ext4, XFS) and Windows (NTFS) guests | ||
| - [P2] Verify the system handles guest connection loss during file transfer gracefully with partial completion status | ||
| - [P2] Verify operator upgrade preserves existing restore resources and their status | ||
| - [P2] Verify restore operations handle edge cases including large files, root disk snapshots, cross-filesystem constraints, concurrent multi-VM operations, and API server rate-limiting resilience |
There was a problem hiding this comment.
[MEDIUM] Compound testing goal is a grab-bag of 5+ distinct behaviors.
"Verify restore operations handle edge cases including large files, root disk snapshots, cross-filesystem constraints, concurrent multi-VM operations, and API server rate-limiting resilience" — each of these can independently pass or fail.
Per AGENTS.md: "if one scenario can fail while another passes, they are separate items." Split into individual goals or remove this meta-goal since each behavior already has its own dedicated goal/scenario elsewhere in the STP.
There was a problem hiding this comment.
I'm removing it; the corresponding scenarios can trace back to other existing goals.
| - [x] **Non-Functional Requirements (NFRs)** | ||
| - *List applicable NFRs and their targets:* | ||
| - Security: Restore operations use authenticated, restricted guest access with dedicated user and RBAC roles (admin/editor/viewer) | ||
| - Security: Credentials used for guest access are generated per-operation and cleaned up after completion |
There was a problem hiding this comment.
Currently it's a single key pair (not per operation), but we will improve it to SSH certificates with TTL.
| - **Guest helper script must be pre-installed in the VM; the operator does not install it automatically** | ||
| - *PM Sign-off:* [TBD] | ||
|
|
||
| - **SSH access must be configured on the VM with the `filerestore` user; the operator does not configure guest SSH automatically** | ||
| - *PM Sign-off:* [TBD] |
There was a problem hiding this comment.
Maybe worth mentioning we provide basic setup scripts (U/S) covering both of the above.
| - XFS snapshot mounting requires special options to avoid filesystem identifier collisions | ||
| - LVM-based volume snapshots have identifier collision issues that need explicit handling | ||
| - Storage mode mismatches between snapshot sources and cluster defaults can cause restore failures, requiring tests across different storage configurations | ||
| - Windows guest connectivity support for guest helper execution |
There was a problem hiding this comment.
More explicitly - OpenSSH Server (any alternative?)
There was a problem hiding this comment.
Will mention this
| - [x] **API Extensions** | ||
| - *List new or modified APIs:* | ||
| - New file restore API — users create restore requests specifying a target VM, backup source (PVC or VolumeSnapshot), and file paths. Status provides phase tracking and completion information. | ||
| - *Testing impact:* All create, read, update, and delete operations on the file restore API must be tested. Restore phase transitions must be validated. Status must be verified for both success and failure paths. |
| - *See environment requirements in Section II.3 and testing tools in Section II.3.1* | ||
|
|
||
| - [x] **Topology Considerations** | ||
| - *Describe topology requirements:* Multi-node cluster required for hotplug volume attachment. Cross-namespace PVC restore requires namespace-level RBAC verification. |
There was a problem hiding this comment.
Multi-node cluster required for hotplug volume attachment?
There was a problem hiding this comment.
Possible hallucination :)
| #### **1. Scope of Testing** | ||
|
|
||
| This STP covers the Dev Preview phase of the File-Level Restore feature for CNV 5.0. | ||
| Testing validates automatic and manual restore modes, Linux and Windows guest support, PVC and |
There was a problem hiding this comment.
Note automatic mode should have the main focus.
| - *Details:* Validate compatibility with KubeVirt DeclarativeHotplugVolumes feature gate. Verify file restore API backward compatibility. | ||
|
|
||
| - [x] **Upgrade Testing** — Validates upgrade paths from previous versions, data migration, and configuration preservation | ||
| - *Details:* Verify operator upgrade preserves existing file restore resources and their status. First release; no prior version to upgrade from, but HCO integration upgrade path must be validated. |
| - *Details:* Verify operator upgrade preserves existing file restore resources and their status. First release; no prior version to upgrade from, but HCO integration upgrade path must be validated. | ||
|
|
||
| - [x] **Dependencies** — Blocked by deliverables from other components/products | ||
| - *Details:* Depends on HCO team to add file restore operator as an HCO-managed component for downstream delivery (CNV-89642). HCO integration includes operator lifecycle management and certificate rotation. |
| #### **3.1. Testing Tools & Frameworks** | ||
|
|
||
| - **Test Framework:** | ||
| - Upstream: Go/Ginkgo e2e tests in kubevirt/vm-file-restore-operator repository using kubevirtci for cluster management. |
There was a problem hiding this comment.
The U/S e2e test is currently minimal. Should we suggest coverage extension here?
There was a problem hiding this comment.
That's what Tier 1 tests will cover
| - [x] DeclarativeHotplugVolumes feature gate is enabled in KubeVirt CR | ||
| - [x] Guest helper scripts are available for installation on test VMs (Linux and Windows) | ||
| - [x] VolumeSnapshot-capable StorageClass is configured and functional | ||
| - [ ] HCO integration for downstream delivery is available (CNV-89642) |
There was a problem hiding this comment.
Initially, I thought we would need it to have a proper deploy of the operator on the test cluster, but I think we have work around now ?
Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
peterclauterbach
left a comment
peterclauterbach
left a comment
There was a problem hiding this comment.
In general, there is missing failure testing, I recommend calling out common us cases like:
- Target disk capacity fills up during restore.
- File or directory permissions incorrect during the restore.
- other failures like FLR source or target volume is unplugged during restore.
Thanks @peterclauterbach for raising these use cases. I'll add more coverage for the disk filling up and volumes unplug.
Regarding this one, considering the guest helper will be running as root (write shouldn't be prevented) and we cover file ownership and permissions with "Verify files are restored to a running Linux VM from a backup volume with file integrity preserved (size, ownership, permissions", I wouldn't add a new one. |
…tions Signed-off-by: Emanuele Prella <eprella@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md (2)
204-223: 📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick winFill the PM/Lead agreements before merging.
Each Out of Scope item still has a
[TBD]agreement, so the required sign-off is missing and the exclusions are not approvable yet. As per coding guidelines: Out of Scope items need Rationale and PM/Lead Agreement with name/date.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stps/sig-storage/VIRTSTRAT-480_file_level_restore.md` around lines 204 - 223, The Out of Scope section still has placeholder PM/Lead agreements, so complete the sign-off entries before merge. Update each affected bullet in the out-of-scope list to replace the [TBD] agreement with the required PM/Lead name and date, keeping the existing rationale text intact. Use the Out of Scope items block in the VIRTSTRAT-480 file-level restore document to locate the entries quickly.Source: Coding guidelines
19-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a contact for the QE owner.
QE Owner(s)is missing the required contact/handle, so this metadata is not actionable for follow-up. As per coding guidelines: QE Owner(s) listed with name and contact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stps/sig-storage/VIRTSTRAT-480_file_level_restore.md` at line 19, The QE Owner(s) metadata is incomplete because it lists only the owner name without a contact/handle. Update the “QE Owner(s)” entry in the file so it includes both the person’s name and their contact information in the same metadata line, following the existing coding guidelines for owner attribution.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@stps/sig-storage/VIRTSTRAT-480_file_level_restore.md`:
- Around line 204-223: The Out of Scope section still has placeholder PM/Lead
agreements, so complete the sign-off entries before merge. Update each affected
bullet in the out-of-scope list to replace the [TBD] agreement with the required
PM/Lead name and date, keeping the existing rationale text intact. Use the Out
of Scope items block in the VIRTSTRAT-480 file-level restore document to locate
the entries quickly.
- Line 19: The QE Owner(s) metadata is incomplete because it lists only the
owner name without a contact/handle. Update the “QE Owner(s)” entry in the file
so it includes both the person’s name and their contact information in the same
metadata line, following the existing coding guidelines for owner attribution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94dc62a2-3f1e-4d9b-aebc-ade7a2315e52
📒 Files selected for processing (1)
stps/sig-storage/VIRTSTRAT-480_file_level_restore.md
10 participants
STP Metadata
VEP issue:
kubevirt/enhancements#170
What this PR does
Add STP for File-Level Restore
Special notes for your reviewer
Assisted by Quality Flow and Claude Code
Summary by CodeRabbit
Summary by CodeRabbit