[Storage] add vTPM for vm from memory dump test#4685
Open
acinko-rh wants to merge 10 commits into
Open
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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-5
requested review from
RoniKishner,
ema-aka-young,
josemacassan and
rnetser
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4685 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.03%
==========================================
Files 25 25
Lines 2487 2449 -38
==========================================
- Hits 2454 2416 -38
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot-5
requested review from
Ahmad-Hafe,
dalia-frank,
dshchedr,
geetikakay and
kgoldbla
Contributor
|
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
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Contributor
Author
|
/wip |
openshift-virtualization-qe-bot-5
changed the title
Contributor
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Signed-off-by: Adam Cinko <acinko@redhat.com>
Signed-off-by: Adam Cinko <acinko@redhat.com>
Signed-off-by: Adam Cinko <acinko@redhat.com>
Signed-off-by: Adam Cinko <acinko@redhat.com>
Contributor
Author
|
/wip cancel |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filestests/utils.py |
|
Clean rebase detected — no code changes compared to previous head ( |
Contributor
|
/lgtm |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4916. Overlapping filestests/utils.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5135. Overlapping filestests/utils.py |
Contributor
|
Clean rebase detected — no code changes compared to previous head ( |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4994. Overlapping filestests/storage/test_hotplug.py |
Contributor
|
/lgtm |
dshchedr
previously approved these changes
Jun 26, 2026
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5320. Overlapping filestests/storage/memory_dump/conftest.py |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/storage/test_hotplug.py (1)
87-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix the renamed helper references in this module.
Ruff still resolves both names as undefined at Line 87 and Line 104. Until the import block brings
create_windows2022_dv_template_from_registryandcreate_windows2022_vm_with_vtpminto scope under those names, this test module will not import/collect.🤖 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/test_hotplug.py` around lines 87 - 105, The test module is still referencing renamed helpers that are not in scope, so update the imports in this file to bring create_windows2022_dv_template_from_registry and create_windows2022_vm_with_vtpm in under the exact names used by the fixtures. Check the fixture definitions that call create_windows2022_dv_template_from_registry and vm_instance_from_template_multi_storage_scope_class, and make the import block match those symbols so Ruff can resolve them during collection.Source: Linters/SAST tools
🤖 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/memory_dump/conftest.py`:
- Around line 23-35: The memory-dump conftest is still importing outdated
Windows helper names, so test collection fails before the fixture can run.
Update the `tests.utils` import used by `conftest.py` to the renamed helpers
that match `create_windows2022_dv_template_from_registry` and
`create_windows2022_vm_with_vtpm`, and make sure the fixture setup continues to
reference those same symbols consistently.
In `@tests/utils.py`:
- Around line 708-715: The PR template content is incomplete and the required
sections are missing meaningful text, especially “What this PR does / why we
need it:”. Restore the full template structure and add a substantive explanation
for the vTPM/helper rewire so all required PR sections are present. Use the
surrounding helper context in create_windows2022_dv_template_from_registry as
the place to ensure the template-driven output preserves the expected sections
before merge.
---
Outside diff comments:
In `@tests/storage/test_hotplug.py`:
- Around line 87-105: The test module is still referencing renamed helpers that
are not in scope, so update the imports in this file to bring
create_windows2022_dv_template_from_registry and create_windows2022_vm_with_vtpm
in under the exact names used by the fixtures. Check the fixture definitions
that call create_windows2022_dv_template_from_registry and
vm_instance_from_template_multi_storage_scope_class, and make the import block
match those symbols so Ruff can resolve them during collection.
🪄 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: b0598307-44c3-4e85-ad25-5968a8cff032
📒 Files selected for processing (4)
tests/storage/memory_dump/conftest.pytests/storage/memory_dump/test_memory_dump.pytests/storage/test_hotplug.pytests/utils.py
Comment on lines
+23
to
+35
| create_windows2022_dv_template_from_registry( | ||
| dv_name="windows-2022-dv", | ||
| namespace=namespace.name, | ||
| client=unprivileged_client, | ||
| storage_class=py_config["default_storage_class"], | ||
| ) as dv_template, | ||
| create_windows2022_vm_with_vtpm( | ||
| vm_name="windows-2022-vm", | ||
| namespace=namespace.name, | ||
| client=unprivileged_client, | ||
| dv_template=dv_template, | ||
| cpu_model=modern_cpu_for_migration, | ||
| ) as vm, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Import the renamed Windows helpers.
create_windows2022_dv_template_from_registry and create_windows2022_vm_with_vtpm are unresolved at Line 23 and Line 29, so this module will fail during collection before any memory-dump fixture runs. Update the tests.utils import to match the renamed helpers.
Suggested import update
-from tests.utils import create_windows2022_dv_from_registry, create_windows2022_vm_with_vtpm_from_registry
+from tests.utils import create_windows2022_dv_template_from_registry, create_windows2022_vm_with_vtpm📝 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.
Suggested change
| create_windows2022_dv_template_from_registry( | |
| dv_name="windows-2022-dv", | |
| namespace=namespace.name, | |
| client=unprivileged_client, | |
| storage_class=py_config["default_storage_class"], | |
| ) as dv_template, | |
| create_windows2022_vm_with_vtpm( | |
| vm_name="windows-2022-vm", | |
| namespace=namespace.name, | |
| client=unprivileged_client, | |
| dv_template=dv_template, | |
| cpu_model=modern_cpu_for_migration, | |
| ) as vm, | |
| from tests.utils import create_windows2022_dv_template_from_registry, create_windows2022_vm_with_vtpm |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 23-23: Undefined name create_windows2022_dv_template_from_registry
(F821)
[error] 29-29: Undefined name create_windows2022_vm_with_vtpm
(F821)
🤖 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/memory_dump/conftest.py` around lines 23 - 35, The memory-dump
conftest is still importing outdated Windows helper names, so test collection
fails before the fixture can run. Update the `tests.utils` import used by
`conftest.py` to the renamed helpers that match
`create_windows2022_dv_template_from_registry` and
`create_windows2022_vm_with_vtpm`, and make sure the fixture setup continues to
reference those same symbols consistently.
Source: Linters/SAST tools
Comment on lines
+708
to
+715
| def create_windows2022_dv_template_from_registry( | ||
| dv_name: str, | ||
| namespace: str, | ||
| client: DynamicClient, | ||
| storage_class: str, | ||
| ) -> Generator[dict]: | ||
| """ | ||
| Creates a Windows Server 2022 DataVolume from registry container disk. | ||
| Creates a Windows Server 2022 DataVolume template from registry container disk. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore the required PR template sections.
The PR summary says the description still lacks meaningful content for ##### What this PR does / why we need it: and does not preserve the required template sections. Please restore the template and explain why this vTPM/helper rewire is needed before merge. As per path instructions, What this PR does / why we need it: must have meaningful content and all required sections must be present.
🤖 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/utils.py` around lines 708 - 715, The PR template content is incomplete
and the required sections are missing meaningful text, especially “What this PR
does / why we need it:”. Restore the full template structure and add a
substantive explanation for the vTPM/helper rewire so all required PR sections
are present. Use the surrounding helper context in
create_windows2022_dv_template_from_registry as the place to ensure the
template-driven output preserves the expected sections before merge.
Source: Path instructions
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
11 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.
Short description:
Adding Win 2022 and vTPM using instance types and preferences into
tests/storage/memory_dumpmoduleMore details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Co-authored: Claude Code
https://redhat.atlassian.net/browse/CNV-51351
jira-ticket:
Summary by CodeRabbit
New Features
Bug Fixes