[Main] Storage: Change Cirros images for RHEL from DataSource into upgrade tests#5003
[Main] Storage: Change Cirros images for RHEL from DataSource into upgrade tests#5003josemacassan wants to merge 4 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughReplace CirrOS-based snapshot-upgrade VMs with RHEL10 fixtures, add shared upgrade-file constants, refactor VM creation to accept data-source-backed volumes and inject files via SSH, update three snapshot/restore tests to use RHEL VMs and generic SSH-based command helpers, and remove the CirrOS console-based helper while adding disk-serial assertion. ChangesStorage Upgrade Test Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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
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. |
openshift-virtualization-qe-bot-3
requested review from
Ahmad-Hafe,
RoniKishner,
acinko-rh,
dshchedr,
hmeir,
jpeimer,
kgoldbla,
rlobillo and
vsibirsk
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5003 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.02%
==========================================
Files 25 25
Lines 2487 2461 -26
==========================================
- Hits 2454 2428 -26
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-3
requested review from
ema-aka-young,
kshvaika and
rnetser
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/storage/upgrade/utils.py (1)
20-27: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winHIGH: Add explicit type hints to updated public helpers.
Both context-manager helpers are public and changed in this PR, but they remain untyped. Please add full parameter and return annotations to keep these utilities strict and self-documenting.
💡 Proposed typing shape
from contextlib import contextmanager +from collections.abc import Generator +from typing import Any `@contextmanager` def create_vm_for_snapshot_upgrade_tests( - vm_name, - namespace, - client, - storage_class_for_snapshot, - cpu_model, - data_source, -): + vm_name: str, + namespace: str, + client: Any, + storage_class_for_snapshot: str, + cpu_model: str, + data_source: Any, +) -> Generator[VirtualMachineForTests]: @@ `@contextmanager` -def create_snapshot_for_upgrade(vm, client): +def create_snapshot_for_upgrade(vm: VirtualMachineForTests, client: Any) -> Generator[VirtualMachineSnapshot]:As per coding guidelines: "Type hints are MANDATORY."
Also applies to: 52-53
🤖 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/upgrade/utils.py` around lines 20 - 27, The public context-manager helpers (notably create_vm_for_snapshot_upgrade_tests and the other context-manager helper in this module) lack type hints; add explicit annotations for all params and returns using typing (e.g., import Any, Dict, Optional, Generator) and annotate parameters as vm_name: str, namespace: str, client: Any, storage_class_for_snapshot: str, cpu_model: str, data_source: Optional[Dict[str, Any]] (or a more precise dict type if known), and change the return type to a context-manager generator like Generator[Any, None, None] or Generator[Dict[str, Any], None, None] (use the precise VM type if available) and apply the same full-parameter/return annotations to the other helper mentioned.
🤖 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/upgrade/conftest.py`:
- Around line 59-61: The test fixture calls update_scratch_space_sc but that
symbol is not defined or imported, causing a NameError at runtime; fix by
importing update_scratch_space_sc from its module (or add a local definition)
and ensure the import is added to tests/storage/upgrade/conftest.py so the
with-statement using update_scratch_space_sc (and the variable
edited_cdi_config) resolves correctly at runtime.
- Line 27: Rename the verb-named fixtures to noun-style names: change
skip_if_less_than_two_storage_classes to two_storage_classes_required and
skip_if_not_override_cdiconfig_scratch_space to
cdiconfig_scratch_space_override_required (leave
storage_class_for_updating_cdiconfig_scratch as-is); update the fixture
definitions accordingly and then update every test, conftest import or parameter
that references skip_if_less_than_two_storage_classes or
skip_if_not_override_cdiconfig_scratch_space to use the new names
two_storage_classes_required and cdiconfig_scratch_space_override_required so
all usages match the renamed fixtures.
In `@tests/storage/upgrade/test_upgrade_storage.py`:
- Around line 55-61: Remove the duplicated client keyword argument passed to the
VirtualMachineRestore constructor calls; each invocation (e.g., the one creating
vm_restore with name=f"restore-snapshot-{rhel_vm_for_upgrade_a.name}" and
namespace=snapshots_for_upgrade_a.namespace and the other similar invocation
later) currently contains client=admin_client twice — keep a single
client=admin_client and remove the redundant one so the
VirtualMachineRestore(...) calls (and variables like vm_restore,
rhel_vm_for_upgrade_a, snapshots_for_upgrade_a) use only one client kwarg.
---
Outside diff comments:
In `@tests/storage/upgrade/utils.py`:
- Around line 20-27: The public context-manager helpers (notably
create_vm_for_snapshot_upgrade_tests and the other context-manager helper in
this module) lack type hints; add explicit annotations for all params and
returns using typing (e.g., import Any, Dict, Optional, Generator) and annotate
parameters as vm_name: str, namespace: str, client: Any,
storage_class_for_snapshot: str, cpu_model: str, data_source: Optional[Dict[str,
Any]] (or a more precise dict type if known), and change the return type to a
context-manager generator like Generator[Any, None, None] or Generator[Dict[str,
Any], None, None] (use the precise VM type if available) and apply the same
full-parameter/return annotations to the other helper mentioned.
🪄 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: 687c7ce5-f291-469c-852b-658c127810ca
📒 Files selected for processing (5)
tests/storage/upgrade/conftest.pytests/storage/upgrade/constants.pytests/storage/upgrade/test_upgrade_storage.pytests/storage/upgrade/utils.pyutilities/storage.py
💤 Files with no reviewable changes (1)
- utilities/storage.py
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5003 published |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4985. Overlapping filesutilities/storage.py |
josemacassan
force-pushed
the
improve-datasource-upgrade-test-4.20-main
branch
from
cbaa713 to
33a3a3f
Compare
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 (3)
tests/storage/upgrade/conftest.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore meaningful PR template content in “What this PR does / why we need it”.
The PR description still contains placeholder-only content for
##### What this PR does / why we need it:. This blocks reviewers from validating intent/scope and must be replaced with concrete rationale and expected behavior changes before merge.As per coding guidelines:
##### What this PR does / why we need it:must be present and have meaningful content; placeholder-only text is a HIGH-severity violation.🤖 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/upgrade/conftest.py` at line 1, Replace the placeholder under the PR heading "##### What this PR does / why we need it:" with a concise, concrete summary that states the change being introduced, the rationale, the scope (which modules/functions are affected), and the expected behavior or observable outcomes (including any test or migration impacts); ensure the section mentions any relevant functions/classes or test files affected (e.g., tests/storage/upgrade/conftest.py) so reviewers can validate intent and scope.tests/storage/upgrade/test_upgrade_storage.py (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template content before merge.
##### What this PR does / why we need it:is currently placeholder-only in the PR description. This blocks reviewability and must be meaningful content.As per coding guidelines: “
What this PR does / why we need it:— MUST be present AND have meaningful content.”🤖 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/upgrade/test_upgrade_storage.py` around lines 39 - 40, The PR description is missing meaningful content for the required template section; update the PR body so the "##### What this PR does / why we need it:" section contains a concise explanation of the change and its rationale (e.g., what TestUpgradeStorage covers and why the updated_default_storage_class_ocs_virt fixture is needed for this upgrade test), then push the corrected PR description before merging so the template requirement is satisfied.utilities/storage.py (1)
689-693: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winHIGH: New utility function is missing required typing and docstring contract.
assert_disk_serialis a new public helper inutilities/and should include type hints and a Google-style docstring.Proposed fix
-def assert_disk_serial(vm, command=_DEFAULT_DISK_SERIAL_COMMAND): +def assert_disk_serial( + vm: virt_util.VirtualMachineForTests, + command: list[str] = _DEFAULT_DISK_SERIAL_COMMAND, +) -> None: + """Assert that the VM exposes the expected hotplug disk serial in /dev/disk/by-id. + + Args: + vm: VM to inspect over SSH. + command: Command token list used to query disk identifiers. + """ assert ( HOTPLUG_DISK_SERIAL in run_ssh_commands(host=vm.ssh_exec, commands=command, wait_timeout=TIMEOUT_2MIN, sleep=TIMEOUT_5SEC)[0] ), f"hotplug disk serial id {HOTPLUG_DISK_SERIAL} is not in VM"As per coding guidelines: “Type hints are MANDATORY… all new public functions under utilities” and “Write Google-format docstrings for all public functions with non-obvious return values or side effects.”
🤖 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 `@utilities/storage.py` around lines 689 - 693, Add type hints and a Google-style docstring to the new public function assert_disk_serial: annotate parameters (vm: virtual machine type — e.g., VM or whatever class represents vm in this module — and command: str = _DEFAULT_DISK_SERIAL_COMMAND) and the return type as None; then add a Google-format docstring above the function that describes the purpose, parameters (vm, command), the side effect (raises AssertionError if HOTPLUG_DISK_SERIAL not present), and mentions it calls run_ssh_commands and checks HOTPLUG_DISK_SERIAL in the command output to help locate behavior; keep the existing assertion logic unchanged and reference HOTPLUG_DISK_SERIAL and run_ssh_commands in the docstring.
🤖 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 `@tests/storage/upgrade/conftest.py`:
- Line 1: Replace the placeholder under the PR heading "##### What this PR does
/ why we need it:" with a concise, concrete summary that states the change being
introduced, the rationale, the scope (which modules/functions are affected), and
the expected behavior or observable outcomes (including any test or migration
impacts); ensure the section mentions any relevant functions/classes or test
files affected (e.g., tests/storage/upgrade/conftest.py) so reviewers can
validate intent and scope.
In `@tests/storage/upgrade/test_upgrade_storage.py`:
- Around line 39-40: The PR description is missing meaningful content for the
required template section; update the PR body so the "##### What this PR does /
why we need it:" section contains a concise explanation of the change and its
rationale (e.g., what TestUpgradeStorage covers and why the
updated_default_storage_class_ocs_virt fixture is needed for this upgrade test),
then push the corrected PR description before merging so the template
requirement is satisfied.
In `@utilities/storage.py`:
- Around line 689-693: Add type hints and a Google-style docstring to the new
public function assert_disk_serial: annotate parameters (vm: virtual machine
type — e.g., VM or whatever class represents vm in this module — and command:
str = _DEFAULT_DISK_SERIAL_COMMAND) and the return type as None; then add a
Google-format docstring above the function that describes the purpose,
parameters (vm, command), the side effect (raises AssertionError if
HOTPLUG_DISK_SERIAL not present), and mentions it calls run_ssh_commands and
checks HOTPLUG_DISK_SERIAL in the command output to help locate behavior; keep
the existing assertion logic unchanged and reference HOTPLUG_DISK_SERIAL and
run_ssh_commands in the docstring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41eb2313-9769-4270-a656-c35eb83f69a8
📒 Files selected for processing (5)
tests/storage/upgrade/conftest.pytests/storage/upgrade/constants.pytests/storage/upgrade/test_upgrade_storage.pytests/storage/upgrade/utils.pyutilities/storage.py
|
/build-and-push-container |
1 similar comment
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5003 published |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5003 published |
|
Verification failed for PR #5003 (image: openshift-virtualization-tests:pr-5003). |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5003 published |
|
Verification failed for PR #5003 (image: openshift-virtualization-tests:pr-5003). |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5003 published |
|
Verification failed for PR #5003. Execution details |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5120. Overlapping filesutilities/storage.py |
|
/wip |
We run upgrades without HPP, so we can pass OCS only - to save some time and resources: |
|
/wip cancel |
…tests. (RedHatQE#2917) Change use of Cirros images coming from artifactory to RHEL coming from DataSource into upgrade tests. `tests/storage/upgrade`. We are refactoring tests by using RHEL images from DataSource, for this I have changed all the Cirros resources into upgrade tests as well as RHEL utilities and config functions I have added a function called `write_file_via_ssh` that writes a file using `run_ssh_commands` function. It has an internal import to avoid circular import errors I was facing. Programmatically I think is not a very good practice so I appreciate some comments specially in there (perhaps `utilities/storage.py` is not the best place to locate it, and other place might avoid those errors). https://issues.redhat.com/browse/CNV-69123 --------- Signed-off-by: Jose Manuel Castano <joscasta@redhat.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
Remove unused fixtures that reference non-existent update_scratch_space_sc function (removed in fbda8ef). Fix duplicate client keyword arguments in VirtualMachineRestore constructor calls. Signed-off-by: Jose Manuel Castano <joscasta@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes B008 ruff violation by using _DEFAULT_DISK_SERIAL_COMMAND instead of calling shlex.split() in function argument default. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Jose Manuel Castano <joscasta@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/storage.py (1)
111-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template sections before merge
The PR description is missing required template content (especially meaningful text under
##### What this PR does / why we need it:), which is a mandatory repo policy and should be fixed before approval.As per coding guidelines, required PR template sections must be present and “What this PR does / why we need it:” must have meaningful content.
🤖 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 `@utilities/storage.py` at line 111, Update the PR description by restoring all required template sections, particularly ensuring that the "What this PR does / why we need it:" section is present and contains meaningful, descriptive content that clearly explains the purpose and rationale for the changes being made. This is a mandatory repository policy that must be satisfied before the PR can be approved.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.
Inline comments:
In `@tests/storage/upgrade/test_upgrade_storage.py`:
- Line 1: The pull request template is incomplete and missing required sections
that must be filled before merge. Update the PR description to include all
mandatory template headers, and specifically ensure that the "What this PR does
/ why we need it:" section contains meaningful, substantive text that clearly
explains the purpose and rationale for the changes rather than leaving it blank
or with placeholder text. This is a required process step that must be completed
prior to merge approval.
---
Outside diff comments:
In `@utilities/storage.py`:
- Line 111: Update the PR description by restoring all required template
sections, particularly ensuring that the "What this PR does / why we need it:"
section is present and contains meaningful, descriptive content that clearly
explains the purpose and rationale for the changes being made. This is a
mandatory repository policy that must be satisfied before the PR can be
approved.
🪄 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: 071e1e9f-6ccc-4fe2-a7e5-d57a3d6204cc
📒 Files selected for processing (5)
tests/storage/upgrade/conftest.pytests/storage/upgrade/constants.pytests/storage/upgrade/test_upgrade_storage.pytests/storage/upgrade/utils.pyutilities/storage.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/storage.py (1)
111-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template sections before merge
The PR description is missing required template content (especially meaningful text under
##### What this PR does / why we need it:), which is a mandatory repo policy and should be fixed before approval.As per coding guidelines, required PR template sections must be present and “What this PR does / why we need it:” must have meaningful content.
🤖 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 `@utilities/storage.py` at line 111, Update the PR description by restoring all required template sections, particularly ensuring that the "What this PR does / why we need it:" section is present and contains meaningful, descriptive content that clearly explains the purpose and rationale for the changes being made. This is a mandatory repository policy that must be satisfied before the PR can be approved.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.
Inline comments:
In `@tests/storage/upgrade/test_upgrade_storage.py`:
- Line 1: The pull request template is incomplete and missing required sections
that must be filled before merge. Update the PR description to include all
mandatory template headers, and specifically ensure that the "What this PR does
/ why we need it:" section contains meaningful, substantive text that clearly
explains the purpose and rationale for the changes rather than leaving it blank
or with placeholder text. This is a required process step that must be completed
prior to merge approval.
---
Outside diff comments:
In `@utilities/storage.py`:
- Line 111: Update the PR description by restoring all required template
sections, particularly ensuring that the "What this PR does / why we need it:"
section is present and contains meaningful, descriptive content that clearly
explains the purpose and rationale for the changes being made. This is a
mandatory repository policy that must be satisfied before the PR can be
approved.
🪄 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: 071e1e9f-6ccc-4fe2-a7e5-d57a3d6204cc
📒 Files selected for processing (5)
tests/storage/upgrade/conftest.pytests/storage/upgrade/constants.pytests/storage/upgrade/test_upgrade_storage.pytests/storage/upgrade/utils.pyutilities/storage.py
📜 Review details
🔇 Additional comments (5)
utilities/storage.py (1)
145-148: LGTM!tests/storage/upgrade/constants.py (1)
1-9: LGTM!tests/storage/upgrade/utils.py (1)
3-16: LGTM!Also applies to: 26-47, 61-64
tests/storage/upgrade/conftest.py (1)
27-43: LGTM!Also applies to: 45-52, 54-71, 73-79
tests/storage/upgrade/test_upgrade_storage.py (1)
1-13: LGTM!Also applies to: 23-36, 60-86, 135-150, 164-191, 203-208, 218-218
🛑 Comments failed to post (1)
tests/storage/upgrade/test_upgrade_storage.py (1)
1-1:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: PR template sections are missing/empty and should be restored before merge.
The PR description must include all required template headers, and
What this PR does / why we need it:must contain meaningful text (not placeholders/blank).
As per coding guidelines, PR template sections are mandatory and this should be treated as a high-severity process blocker.🤖 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/upgrade/test_upgrade_storage.py` at line 1, The pull request template is incomplete and missing required sections that must be filled before merge. Update the PR description to include all mandatory template headers, and specifically ensure that the "What this PR does / why we need it:" section contains meaningful, substantive text that clearly explains the purpose and rationale for the changes rather than leaving it blank or with placeholder text. This is a required process step that must be completed prior to merge approval.Source: Coding guidelines
7 participants
Short description:
Forwardport: #2917
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Tests
Refactor