Conversation
- Metadata & tracking (enhancement, Jira, QE owner, status) - Document conventions table (CBT, Push mode, Pull mode) - Feature overview and motivation - Section I/II placeholders to be filled Co-authored-by: Cursor <cursoragent@cursor.com>
- Motivation moved to Section I (Motivation and Requirements Review) - Section I checklists: Done [x] for all rows; Entry Criteria, Out of Scope, Risks Status [x] - Testing Goals: P0/P1/P2 goals (CBT, push/pull backup, Windows VM, OCP 4.21, ForceFullBackup, full PVC, etc.); sorted by priority - Out of Scope: restore as feature, offline, performance; removed pull-mode (now P2 goal) - Removed/replaced semicolons; simplified goal wording; removed VM restart goal, VM state PVC/hot-plug goal - Section II.2 Test Strategy: Applicable (Y/N/N/A) and Comments filled for all rows - Performance: no testing planned currently, may be in future - Dependencies: OpenShift and CNV; OCP 4.21 updated KubeVirt images - Compatibility: CBT storage-agnostic; OCP 4.21, Windows VM Co-authored-by: Cursor <cursoragent@cursor.com>
…Section III - Test Environment: uniform 'Standard' where applicable; OCP 4.21, Storage, Special Configurations (KubeVirt images for 4.21) - Entry Criteria: add IncrementalBackup feature gate - Risks: fill all rows (timeline, coverage, env, dependencies, T1 quarantined; start with push backup) - Known Limitations: testing can start with current scope; T1 quarantined; feature in progress, storage providers unblocked - Section III: note to fill from Jira (CNV-61530), single TBD row; Section IV sign-off left as placeholder Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.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:
WalkthroughNew OpenShift Virtualization Changed Block Tracking (CBT) Software Test Plan added: metadata (VEP/Jira/QE owner/SIG/status), CBT terminology and push/pull modes, feature overview, QE and tech review checklists, prioritized test scenarios (P0–P2), environment assumptions, risks/limitations, traceability, and sign-off placeholders. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-4
requested review from
kgoldbla and
kshvaika
ema-aka-young
left a comment
ema-aka-young
left a comment
There was a problem hiding this comment.
Leaving a couple of questions, mainly for understanding.
| | Functional Testing | Validates that the feature works according to specified requirements and user stories | Y | Core scope: CBT enable/disable, full and incremental backup, push/pull, Windows VM, OCP 4.22. | | ||
| | Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | Test cases to be automated for CI and regression. | | ||
| | Performance Testing | Validates feature performance meets requirements (latency, throughput, resource usage) | N/A | No performance testing planned currently. May be in scope in the future. | | ||
| | Security Testing | Verifies security requirements, RBAC, authentication, authorization, and vulnerability scanning | N | Not planned as part of scope of testing. | |
There was a problem hiding this comment.
with pull mode we might do need to verify security? is there security testing for vmexport for example? its pretty much the same
There was a problem hiding this comment.
I'd say it's pretty much the same (it's the exact same API, nothing about the auth path is different). The only addition is that a backup export allows HTTP CONNECT to enable the virt-exportserver <-> virt-launcher tunnel, so it might be a good idea to add a test that ensures one can't establish a connection that way without the required client certificate (should be as simple as just issuing an HTTP CONNECT without any certificate).
There was a problem hiding this comment.
Per offline conversation with @Acedus: Since we are using an external API, security checks are handled at the API level. We’ve determined that redundant testing on our side isn’t necessary.
There was a problem hiding this comment.
@dalia-frank unresolved this - as this info only appears as a review comment, I think it is important to add this item under the out of socpe - not testing because....
- Add testing goals: hotplugged disks, incremental after migration, overlay RWO/RWX - RWO issues: incremental backup after migration (not pull mode) in Risks/Known Limits - Dependencies: add libvirt; Test Env: libvirt RPMs for 4.22 - Pull-mode: P1, merged for 4.22; Windows VM: P1 - Checkpoint redefinition: add after-migration RWO note (libvirt fix pending) - Jira Tracking: STP creation task CNV-61552 - Replace semicolons with commas in cbt.md Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stps/sig-storage/cbt.md (1)
124-126: Fill empty tool entries or mark asN/Ato avoid ambiguity.Line 124-Line 126 currently leave framework/tool fields blank. Prefer explicit
N/Aor planned tool names so reviewers can distinguish “intentionally none” vs “TBD.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/cbt.md` around lines 124 - 126, The table rows labeled "Test Framework", "CI/CD", and "Other Tools" currently have empty cells; update the cbt.md table by replacing each blank cell under those headings with either "N/A" if no tool is to be used or the planned tool name(s) (e.g., "GitHub Actions", "Jest", "Docker") so readers can tell if it’s intentionally none versus TBD; ensure you edit the lines containing the literal strings "Test Framework", "CI/CD", and "Other Tools" to populate their corresponding table cells consistently.
🤖 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/cbt.md`:
- Line 85: Replace the fragment-style rationale/comments entries that read "Can
be separate effort." and "May be in scope in the future." with full sentences
(e.g., "This can be handled as a separate effort." and "This may be in scope in
the future.") in the markdown table rows containing "Performance/benchmarking of
backup duration or throughput" and the other row that currently uses "May be in
scope in the future."; ensure capitalization and punctuation are consistent
across those rationale/comments cells.
---
Nitpick comments:
In `@stps/sig-storage/cbt.md`:
- Around line 124-126: The table rows labeled "Test Framework", "CI/CD", and
"Other Tools" currently have empty cells; update the cbt.md table by replacing
each blank cell under those headings with either "N/A" if no tool is to be used
or the planned tool name(s) (e.g., "GitHub Actions", "Jest", "Docker") so
readers can tell if it’s intentionally none versus TBD; ensure you edit the
lines containing the literal strings "Test Framework", "CI/CD", and "Other
Tools" to populate their corresponding table cells consistently.
🪄 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: 487b20bb-6d00-4ac2-ad7c-6e7ce865338b
📒 Files selected for processing (1)
stps/sig-storage/cbt.md
- Out of scope: performance rationale as one formal sentence - Performance testing: replace fragment phrasing with two clear sentences - Automation testing: expected automation for CI/regression Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
stps/sig-storage/cbt.md (1)
180-189:⚠️ Potential issue | 🟡 MinorReplace sign-off placeholders before merge.
The Reviewers and Approvers sections still contain placeholder entries
[Name /@github-username]. While acceptable during draft/review, these should be replaced with actual names/handles before final approval, or explicitly marked as "Pending" with a tracking reference if approvals are not yet complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/cbt.md` around lines 180 - 189, The "IV. Sign-off and Approval" section contains placeholder entries for Reviewers and Approvers; update the list items under the "Reviewers:" and "Approvers:" headings (the bullet lines with "[Name / `@github-username`]") to either the actual reviewer/approver names and GitHub handles or replace each placeholder with "Pending — track: <issue/PR # or ticket>" so the document reflects real assignees or a traceable pending state before merge.
🧹 Nitpick comments (1)
stps/sig-storage/cbt.md (1)
120-127: Consider populating the Testing Tools & Frameworks table.The table is currently empty for Test Framework, CI/CD, and Other Tools rows. If the testing uses standard tools/frameworks that don't need special mention, this is acceptable. However, documenting the actual tools (e.g., pytest, pytest-cnv, OpenShift CI, etc.) would make the STP more complete and useful as a reference document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-storage/cbt.md` around lines 120 - 127, The "3.1. Testing Tools & Frameworks" table is empty; populate the Test Framework, CI/CD, and Other Tools rows with the actual tools used (or explicitly state "None / N/A" if none). Edit the section header "3.1. Testing Tools & Frameworks" and update the table rows for "Test Framework", "CI/CD", and "Other Tools" to include specific entries such as pytest / pytest-cnv (or your project’s test runner), the CI system (e.g., OpenShift CI, GitHub Actions, Jenkins), and any other utilities (e.g., testcontainers, coverage, linters), so readers can find which tools are used.
🤖 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/cbt.md`:
- Around line 180-189: The "IV. Sign-off and Approval" section contains
placeholder entries for Reviewers and Approvers; update the list items under the
"Reviewers:" and "Approvers:" headings (the bullet lines with "[Name /
`@github-username`]") to either the actual reviewer/approver names and GitHub
handles or replace each placeholder with "Pending — track: <issue/PR # or
ticket>" so the document reflects real assignees or a traceable pending state
before merge.
---
Nitpick comments:
In `@stps/sig-storage/cbt.md`:
- Around line 120-127: The "3.1. Testing Tools & Frameworks" table is empty;
populate the Test Framework, CI/CD, and Other Tools rows with the actual tools
used (or explicitly state "None / N/A" if none). Edit the section header "3.1.
Testing Tools & Frameworks" and update the table rows for "Test Framework",
"CI/CD", and "Other Tools" to include specific entries such as pytest /
pytest-cnv (or your project’s test runner), the CI system (e.g., OpenShift CI,
GitHub Actions, Jenkins), and any other utilities (e.g., testcontainers,
coverage, linters), so readers can find which tools are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e8b24c2-92ce-4250-9bc2-fd9985c1062b
📒 Files selected for processing (1)
stps/sig-storage/cbt.md
- Feature overview and Pull mode: VMExport API HTTP shim over NBD - Dependencies and cross integrations: VMExport for pull-mode endpoints - Pull-mode traceability: verify via VMExport API - Risks/limitations/scenarios: general CBT/backup wording for RWO overlay - T1 quarantine: CNV-78846 and kubevirt#17297 (KubeVirt disk source), separate from libvirt overlay issue - Remove misleading libvirt-as-primary flaky-backup bullet Made-with: Cursor
Remove VMExport/HTTP shim from Document Conventions; Feature Overview already describes KubeVirt pull-mode access (addresses review feedback). Made-with: Cursor
|
/lgtm |
|
/lgtm |
|
/lgtm |
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
added some comments
looks good:
- Testing Goals are well-prioritized (P0/P1/P2) and well-ordered
- Risk analysis is detailed with specific Jira links and upstream bug tracking
- Test scenarios have good coverage and proper traceability to CNV-61530
- Out of Scope items have solid rationale (just missing the sign-offs)
- Document Conventions section defines genuinely feature-specific terms
- Negative/edge cases are considered (PVC full, crash/corruption, mutually exclusive operations)
| | Out-of-Scope Item | Rationale | PM/ Lead Agreement | | ||
| |:---------------------------------------------------------------------|:-----------------------|:-------------------| | ||
| | Restore as a feature (restore API, restore workflows, restore UX) | Restore is used only to validate that backup was done properly (backup integrity). We do not test restore as a product feature. | [ ] Name/Date | | ||
| | Offline backup | Only online backup is supported in initial implementation per VEP. | [ ] Name/Date | |
There was a problem hiding this comment.
this is a limitation (i.e we cannot test, as opposed to ' we can test but chose not to because....')
|
|
||
| | Out-of-Scope Item | Rationale | PM/ Lead Agreement | | ||
| |:---------------------------------------------------------------------|:-----------------------|:-------------------| | ||
| | Restore as a feature (restore API, restore workflows, restore UX) | Restore is used only to validate that backup was done properly (backup integrity). We do not test restore as a product feature. | [ ] Name/Date | |
|
|
||
| ### **Feature Overview** | ||
|
|
||
| CBT (Changed Block Tracking) enables storage-agnostic incremental backup of VMs using QEMU, saving only changed blocks since the last backup. The main consumers are backup vendors and cluster admins. Backups can run in push mode, where the user provides a filesystem-based PVC to store backup data, or in pull mode, where the user provides a PVC for scratch space and libvirt exposes an NBD export so external components can pull backup data or query the dirty bitmap. In KubeVirt, pull-mode access to that NBD export is further abstracted through an HTTP shim layer using the **VMExport API**. |
There was a problem hiding this comment.
please avoid using implementatio in the stp; this is a high level doc.
proposed:
CBT (Changed Block Tracking) enables storage-agnostic incremental backup of VMs, saving only changed blocks since the last backup. The main consumers are backup vendors and cluster admins. Backups can run in push mode, where the user
provides a PVC to store backup data, or in pull mode, where the user provides a PVC for scratch space and backup data is exposed via an export endpoint that external backup tools can connect to and pull data from.
| |:---------------------------------------------------------------------|:-----------------------|:-------------------| | ||
| | Restore as a feature (restore API, restore workflows, restore UX) | Restore is used only to validate that backup was done properly (backup integrity). We do not test restore as a product feature. | [ ] Name/Date | | ||
| | Offline backup | Only online backup is supported in initial implementation per VEP. | [ ] Name/Date | | ||
| | Performance/benchmarking of backup duration or throughput | This is not a functional requirement for this STP and can be tracked as a separate effort. | [ ] Name/Date | |
There was a problem hiding this comment.
This is already covered under Test Strategy, so can be removed from here.
Is it clear to PMs that there will be no performance testing for this feature?
| | Functional Testing | Validates that the feature works according to specified requirements and user stories | Y | Core scope: CBT enable/disable, full and incremental backup, push/pull, Windows VM, OCP 4.22. | | ||
| | Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | Test cases to be automated for CI and regression. | | ||
| | Performance Testing | Validates feature performance meets requirements (latency, throughput, resource usage) | N/A | No performance testing planned currently. May be in scope in the future. | | ||
| | Security Testing | Verifies security requirements, RBAC, authentication, authorization, and vulnerability scanning | N | Not planned as part of scope of testing. | |
There was a problem hiding this comment.
@dalia-frank unresolved this - as this info only appears as a review comment, I think it is important to add this item under the out of socpe - not testing because....
| #### **6. Known Limitations** | ||
|
|
||
| - Testing can start with current scope (e.g. upstream, P0/P1 focus). Not all tests are active yet: Tier 1 tests are quarantined downstream until failures are debugged. Downstream instability is tracked in [CNV-78846](https://redhat.atlassian.net/browse/CNV-78846) and tied to faulty **disk source resolution in KubeVirt** ([kubevirt/kubevirt#17297](https://github.com/kubevirt/kubevirt/issues/17297)), which is **for the most part unrelated** to a Libvirt/QEMU regression. | ||
| - Feature code and API are still under development. Testing will start on OCP 4.22 with HCO feature gate for CBT enabled. |
There was a problem hiding this comment.
this is already mentioned under risks, as it is indeed a risk, please remove from here
| | Dependencies | Feature code and API still in progress. Libvirt/QEMU bug requires fix for consistent backup overall and for **qcow2 overlay migration** with **RWO** backend (bitmap behavior). This is a **general CBT/backup** limitation for that storage layout. It is not specific to push vs pull backup mode. | Track dev deliverables. OCP 4.22. Blocked on libvirt/QEMU fix for incremental-after-overlay-migration coverage with RWO. Use RWX or defer scenario until fix. | [x] | | ||
| | Other | T1 tests quarantined downstream. Current flakes (as of this STP) align with [CNV-78846](https://redhat.atlassian.net/browse/CNV-78846): faulty **disk source resolution in KubeVirt** ([kubevirt/kubevirt#17297](https://github.com/kubevirt/kubevirt/issues/17297)), mostly **not** a Libvirt/QEMU regression. Separately, incremental backup after **qcow2 overlay migration** with **RWO** (bitmap migration) is a **general CBT/backup** risk tracked under libvirt fixes and Known Limitations. | T1: debug and re-enable against CNV-78846 and KubeVirt upstream. For overlay migration with RWO, use RWX or defer until libvirt fix. Track upstream fixes for both tracks. | [x] | | ||
|
|
||
| #### **6. Known Limitations** |
There was a problem hiding this comment.
please add sg=ingoffs here as well (the newer template added that), thanks
12 participants
STP Metadata
VEP issue: https://github.com/kubevirt/enhancements/blob/main/veps/sig-storage/incremental-backup.md
What this PR does
Adds the CBT (Changed Block Tracking) STP for CNV-61530
Special notes for your reviewer
Summary by CodeRabbit