edit: clone conformance tests with 4-disk VM, no registry dependency (CNV-5607 & CNV-5608)#5624
edit: clone conformance tests with 4-disk VM, no registry dependency (CNV-5607 & CNV-5608)#5624Ahmad-Hafe wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe CDI clone tests derive Fedora DataVolume source metadata and sizes from scoped data sources, centralize multi-disk VM clone setup in a shared helper, mark two tests as conformance tests, and parse source sizes with ChangesCDI clone refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Ahmad-Hafe
force-pushed
the
feat/clone-4-disks-conformance
branch
from
87cb959 to
7188fdc
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:failed
Ahmad-Hafe
force-pushed
the
feat/clone-4-disks-conformance
branch
from
7188fdc to
f74b1b7
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:failed
Ahmad-Hafe
force-pushed
the
feat/clone-4-disks-conformance
branch
from
f74b1b7 to
65b0521
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Ahmad-Hafe
force-pushed
the
feat/clone-4-disks-conformance
branch
from
65b0521 to
9d6e7a8
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
1 similar comment
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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
removed
the
tox:verify-tc-requirement-polarion:running
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "--- markers in cdi_clone dir ---"
rg -n '`@pytest.mark`' tests/storage/cdi_clone/
echo "--- usages of fedora_dv_with_filesystem_volume_mode / fedora_dv_with_block_volume_mode ---"
rg -n 'fedora_dv_with_filesystem_volume_mode|fedora_dv_with_block_volume_mode' tests/storage/cdi_clone/
echo "--- usages of create_vm_from_clone_dv_template outside utils.py ---"
rg -rn 'create_vm_from_clone_dv_template' tests/
echo "--- usages of fedora_data_source_scope_module fixture ---"
rg -n 'fedora_data_source_scope_module' tests/storage/cdi_clone/ -A2 -B2
echo "--- confirm no smoke/gating markers exist in this dir before change (git diff) ---"
git diff origin/main...HEAD -- tests/storage/cdi_clone/ | head -200Length of output: 6584 🏁 Script executed: #!/bin/bash
sed -n '210,265p' tests/storage/cdi_clone/test_clone.py
echo "--- proper search for create_vm_from_clone_dv_template ---"
rg -n "create_vm_from_clone_dv_template" tests/storage/cdi_clone/Length of output: 2579
Test Execution Plan
Affected tests to run
Both tests exercise the new Real tests (cluster required) Happy path — FS→Block clone with 4 disks (gating): Happy path — Block→FS clone with 4 disks + volume-mode overhead sizing (smoke): Expected in both cases: source DV is created from the golden-image |
|
/wip cancel |
|
/wip |
|
Clean rebase detected — no code changes compared to previous head ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/wip cancel |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes.
Test Execution Plan
Affected tests to run
Real tests (cluster required) Happy path — FS→Block clone with 4 disks (gating): Happy path — Block→FS clone with 4 disks + volume-mode overhead sizing (smoke): Note: I don't have a capability to post a standalone inline review comment independent of this reply — this plan is provided here in the conversation thread. |
|
/rerun-smoke |
|
/lgtm |
|
/rerun-smoke |
|
/rerun-smoke |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4571. Overlapping filestests/storage/cdi_clone/conftest.py |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/storage/cdi_clone/test_clone.py`:
- Around line 241-244: Remove the int() cast from the image_size argument in the
overhead_size_for_dv call, passing the parsed GiB value as a float so fractional
image sizes are preserved and rounded by the existing helper.
🪄 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: db3d8235-4124-439f-b3eb-780de28b5f88
📒 Files selected for processing (3)
tests/storage/cdi_clone/conftest.pytests/storage/cdi_clone/test_clone.pytests/storage/cdi_clone/utils.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/retest all |
|
reported a bug for Devops engineering team, regard faild/ stic cnv-smoke/validation currentley it blocked me |
Switch CDI clone tests (CNV-5607, CNV-5608) to use golden image DataSource instead of quay.io registry, making them self-validating for disconnected environments. Add conformance marker and create VMs with 4 cloned disks to cover the "Clone a VM with 4 disks" requirement using host-assisted cloning (FS↔block volume mode conversion). Signed-off-by: Ahmad Hafe <ahafe@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Clean rebase detected — no code changes compared to previous head ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/storage/cdi_clone/utils.py`:
- Around line 11-24: Expand the docstring for create_vm_from_clone_dv_template
to Google format by adding explicit Args: entries for each parameter and a Side
effects: section documenting VM/DataVolume creation, guest OS boot, and
disk-count verification. Preserve the existing description and clarify the
function’s return behavior if applicable.
🪄 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: 0b4e4d91-a757-42ce-afaa-cda93ff5cc79
📒 Files selected for processing (3)
tests/storage/cdi_clone/conftest.pytests/storage/cdi_clone/test_clone.pytests/storage/cdi_clone/utils.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
| def create_vm_from_clone_dv_template( | ||
| vm_name, | ||
| dv_name, | ||
| namespace_name, | ||
| source_dv, | ||
| client, | ||
| volume_mode, | ||
| storage_class, | ||
| size=None, | ||
| ): | ||
| """Create a VM with NUM_CLONE_DISKS cloned disks from source_dv and verify all disks are visible. | ||
|
|
||
| DV names are derived as {dv_name}-0, {dv_name}-1, etc. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
MEDIUM: Use Google-format docstrings.
Public functions with non-obvious side effects must use Google-format docstrings. This function has significant side effects—it creates a VirtualMachine, adds multiple DataVolumes, boots the guest OS, and asserts the disk count.
Please expand the docstring to include explicit Args: and Side effects: sections. As per coding guidelines, use Google-format docstrings for public functions with non-obvious return values or side effects.
🛠️ Proposed fix
def create_vm_from_clone_dv_template(
vm_name,
dv_name,
namespace_name,
source_dv,
client,
volume_mode,
storage_class,
size=None,
):
- """Create a VM with NUM_CLONE_DISKS cloned disks from source_dv and verify all disks are visible.
-
- DV names are derived as {dv_name}-0, {dv_name}-1, etc.
- """
+ """
+ Create a VM with NUM_CLONE_DISKS cloned disks from source_dv and verify all disks are visible.
+
+ DV names are derived as {dv_name}-0, {dv_name}-1, etc.
+
+ Args:
+ vm_name (str): The name of the VM to create.
+ dv_name (str): The base name for the data volumes.
+ namespace_name (str): The namespace for the VM and data volumes.
+ source_dv (DataVolume): The source data volume to clone.
+ client (DynamicClient): The Kubernetes dynamic client.
+ volume_mode (str): The volume mode for the cloned disks.
+ storage_class (str): The storage class for the cloned disks.
+ size (str, optional): An optional size override for the cloned disks.
+
+ Side effects:
+ Creates a VirtualMachine, adds multiple DataVolumes, starts the VM,
+ and asserts that the expected number of disks are visible in the guest OS.
+ """📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def create_vm_from_clone_dv_template( | |
| vm_name, | |
| dv_name, | |
| namespace_name, | |
| source_dv, | |
| client, | |
| volume_mode, | |
| storage_class, | |
| size=None, | |
| ): | |
| """Create a VM with NUM_CLONE_DISKS cloned disks from source_dv and verify all disks are visible. | |
| DV names are derived as {dv_name}-0, {dv_name}-1, etc. | |
| """ | |
| def create_vm_from_clone_dv_template( | |
| vm_name, | |
| dv_name, | |
| namespace_name, | |
| source_dv, | |
| client, | |
| volume_mode, | |
| storage_class, | |
| size=None, | |
| ): | |
| """ | |
| Create a VM with NUM_CLONE_DISKS cloned disks from source_dv and verify all disks are visible. | |
| DV names are derived as {dv_name}-0, {dv_name}-1, etc. | |
| Args: | |
| vm_name (str): The name of the VM to create. | |
| dv_name (str): The base name for the data volumes. | |
| namespace_name (str): The namespace for the VM and data volumes. | |
| source_dv (DataVolume): The source data volume to clone. | |
| client (DynamicClient): The Kubernetes dynamic client. | |
| volume_mode (str): The volume mode for the cloned disks. | |
| storage_class (str): The storage class for the cloned disks. | |
| size (str, optional): An optional size override for the cloned disks. | |
| Side effects: | |
| Creates a VirtualMachine, adds multiple DataVolumes, starts the VM, | |
| and asserts that the expected number of disks are visible in the guest OS. | |
| """ |
🤖 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 `@tests/storage/cdi_clone/utils.py` around lines 11 - 24, Expand the docstring
for create_vm_from_clone_dv_template to Google format by adding explicit Args:
entries for each parameter and a Side effects: section documenting VM/DataVolume
creation, guest OS boot, and disk-count verification. Preserve the existing
description and clarify the function’s return behavior if applicable.
Source: Coding guidelines
10 participants
What this PR does / why we need it:
adjust test_clone_from_block_to_fs_using_dv_template & test_clone_from_block_to_fs_using_dv_template
to meet the new conformance requirement "Clone a VM with 4 disks" + host-assisted cloning" + remove quay external registry dependency foor booting the disk to use goldenimage instead)
conformancemarkerconformancemarkerdataVolumeTemplate(1 boot + 3 additional viaadd_dv_to_vm)Which issue(s) this PR fixes:
CNV-88909
Special notes for reviewer:
Tests executed on GCP cluster with
sp-balanced-storageSCand on sc ocs-storagecluster-ceph-rbd-virtualization
jira-ticket:
CNV-88909
Summary by CodeRabbit
Summary of changes