[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
📝 WalkthroughWalkthroughReplace CirrOS-based snapshot-upgrade VMs with RHEL10 fixtures, add shared upgrade-file constants, change VM creation to accept data-source-backed volumes and inject files via SSH, update three snapshot/restore tests to use RHEL VMs and generic VM command helpers, and remove the CirrOS-specific helper. ChangesStorage Upgrade Test Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (3 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
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 (4)
tests/storage/upgrade/conftest.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore meaningful PR template content before merge.
The PR description still uses placeholders and does not provide meaningful content under
##### What this PR does / why we need it:. This is a required merge-gate section and must be filled with concrete rationale and scope.As per coding guidelines,
##### What this PR does / why we need it:“MUST be present AND have meaningful content” and placeholder-only content must be flagged as HIGH.🤖 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, PR template still contains placeholder text for the required section "##### What this PR does / why we need it:" — update the PR description to replace the placeholder with a concrete summary that states the change set, scope, and rationale (e.g., what behaviors or tests in this change affect, why the upgrade tests/fixtures in this PR are needed, and any rollout/compatibility considerations). Ensure the header "##### What this PR does / why we need it:" is present and populated with meaningful content before merge.tests/storage/upgrade/test_upgrade_storage.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template content before merge.
##### What this PR does / why we need it:is currently placeholder-level/empty per the provided PR metadata. Please add meaningful, concrete content and ensure all required template sections remain present.As per coding guidelines: “
##### What this PR does / why we need it:— MUST be present AND have meaningful content… Flag as HIGH if the section is missing, empty… or contains only placeholder tokens.”🤖 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, Restore and populate the PR template section titled "##### What this PR does / why we need it:" with concrete, meaningful content (not placeholders) that summarizes the change, the motivation, and any user/behavioral impact; replace any placeholder tokens like "TBD" or empty lines with a short paragraph describing the purpose of this PR, intended behavior, and why it’s needed, and ensure the other required template sections remain present and unchanged so CI/QA checks recognize the template.utilities/storage.py (2)
690-694:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Add mandatory type hints to new public utility function
Line 690 introduces a new public function in
utilities/without type hints. Please annotatevm,command, and return type explicitly.Proposed fix
-def assert_disk_serial(vm, command=shlex.split("sudo ls /dev/disk/by-id")): +def assert_disk_serial( + vm: virt_util.VirtualMachineForTests, + command: list[str] | None = None, +) -> None: + effective_command = command or _DEFAULT_DISK_SERIAL_COMMAND assert ( HOTPLUG_DISK_SERIAL - in run_ssh_commands(host=vm.ssh_exec, commands=command, wait_timeout=TIMEOUT_2MIN, sleep=TIMEOUT_5SEC)[0] + in run_ssh_commands( + host=vm.ssh_exec, + commands=effective_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.”
🤖 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 690 - 694, The function assert_disk_serial is a new public utility and must have explicit type hints: annotate the vm parameter (e.g., vm: Any or the concrete VM class used across utilities), annotate command as a list of strings (e.g., command: list[str] or List[str]) and set the return type to None (-> None). Update the signature of assert_disk_serial(vm, command=shlex.split(...)) accordingly, and add the necessary typing imports (Any or the VM class and List from typing) if they are not already present; keep the body using run_ssh_commands and HOTPLUG_DISK_SERIAL unchanged.
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: PR template content is incomplete and blocks review quality
The PR description is missing meaningful content in
##### What this PR does / why we need it:(currently placeholder-level). Please restore substantive rationale in that section before merge.As per coding guidelines: “
What this PR does / why we need it:MUST be present AND have meaningful content… flag as HIGH if placeholder-only.”🤖 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 1, The PR description currently contains only placeholder content for the header "##### What this PR does / why we need it:" — update that section to include a concise summary of changes, the reason these changes are required, the impact/risks, and any verification steps; specifically replace the placeholder text under "##### What this PR does / why we need it:" with meaningful content (summary, motivation, risk/impact, and testing notes) so the reviewer can evaluate the change.
🤖 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 `@utilities/storage.py`:
- Around line 69-70: Replace the inline shlex.split default in
assert_disk_serial with the module-level constant and avoid a shared mutable
default by changing the function signature to command=None; inside
assert_disk_serial set command = _DEFAULT_DISK_SERIAL_COMMAND when command is
None, then call run_ssh_commands(host=vm.ssh_exec, commands=command,
wait_timeout=TIMEOUT_2MIN, sleep=TIMEOUT_5SEC) and assert HOTPLUG_DISK_SERIAL is
present; also remove any unused duplicate shlex.split declarations so
_DEFAULT_DISK_SERIAL_COMMAND is the single source of truth.
---
Outside diff comments:
In `@tests/storage/upgrade/conftest.py`:
- Line 1: PR template still contains placeholder text for the required section
"##### What this PR does / why we need it:" — update the PR description to
replace the placeholder with a concrete summary that states the change set,
scope, and rationale (e.g., what behaviors or tests in this change affect, why
the upgrade tests/fixtures in this PR are needed, and any rollout/compatibility
considerations). Ensure the header "##### What this PR does / why we need it:"
is present and populated with meaningful content before merge.
In `@tests/storage/upgrade/test_upgrade_storage.py`:
- Line 1: Restore and populate the PR template section titled "##### What this
PR does / why we need it:" with concrete, meaningful content (not placeholders)
that summarizes the change, the motivation, and any user/behavioral impact;
replace any placeholder tokens like "TBD" or empty lines with a short paragraph
describing the purpose of this PR, intended behavior, and why it’s needed, and
ensure the other required template sections remain present and unchanged so
CI/QA checks recognize the template.
In `@utilities/storage.py`:
- Around line 690-694: The function assert_disk_serial is a new public utility
and must have explicit type hints: annotate the vm parameter (e.g., vm: Any or
the concrete VM class used across utilities), annotate command as a list of
strings (e.g., command: list[str] or List[str]) and set the return type to None
(-> None). Update the signature of assert_disk_serial(vm,
command=shlex.split(...)) accordingly, and add the necessary typing imports (Any
or the VM class and List from typing) if they are not already present; keep the
body using run_ssh_commands and HOTPLUG_DISK_SERIAL unchanged.
- Line 1: The PR description currently contains only placeholder content for the
header "##### What this PR does / why we need it:" — update that section to
include a concise summary of changes, the reason these changes are required, the
impact/risks, and any verification steps; specifically replace the placeholder
text under "##### What this PR does / why we need it:" with meaningful content
(summary, motivation, risk/impact, and testing notes) so the reviewer can
evaluate the change.
🪄 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: 55c31e4c-1507-4873-8cc1-d35bbacbc500
📒 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 (4)
tests/storage/upgrade/conftest.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore meaningful PR template content before merge.
The PR description still uses placeholders and does not provide meaningful content under
##### What this PR does / why we need it:. This is a required merge-gate section and must be filled with concrete rationale and scope.As per coding guidelines,
##### What this PR does / why we need it:“MUST be present AND have meaningful content” and placeholder-only content must be flagged as HIGH.🤖 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, PR template still contains placeholder text for the required section "##### What this PR does / why we need it:" — update the PR description to replace the placeholder with a concrete summary that states the change set, scope, and rationale (e.g., what behaviors or tests in this change affect, why the upgrade tests/fixtures in this PR are needed, and any rollout/compatibility considerations). Ensure the header "##### What this PR does / why we need it:" is present and populated with meaningful content before merge.tests/storage/upgrade/test_upgrade_storage.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template content before merge.
##### What this PR does / why we need it:is currently placeholder-level/empty per the provided PR metadata. Please add meaningful, concrete content and ensure all required template sections remain present.As per coding guidelines: “
##### What this PR does / why we need it:— MUST be present AND have meaningful content… Flag as HIGH if the section is missing, empty… or contains only placeholder tokens.”🤖 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, Restore and populate the PR template section titled "##### What this PR does / why we need it:" with concrete, meaningful content (not placeholders) that summarizes the change, the motivation, and any user/behavioral impact; replace any placeholder tokens like "TBD" or empty lines with a short paragraph describing the purpose of this PR, intended behavior, and why it’s needed, and ensure the other required template sections remain present and unchanged so CI/QA checks recognize the template.utilities/storage.py (2)
690-694:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Add mandatory type hints to new public utility function
Line 690 introduces a new public function in
utilities/without type hints. Please annotatevm,command, and return type explicitly.Proposed fix
-def assert_disk_serial(vm, command=shlex.split("sudo ls /dev/disk/by-id")): +def assert_disk_serial( + vm: virt_util.VirtualMachineForTests, + command: list[str] | None = None, +) -> None: + effective_command = command or _DEFAULT_DISK_SERIAL_COMMAND assert ( HOTPLUG_DISK_SERIAL - in run_ssh_commands(host=vm.ssh_exec, commands=command, wait_timeout=TIMEOUT_2MIN, sleep=TIMEOUT_5SEC)[0] + in run_ssh_commands( + host=vm.ssh_exec, + commands=effective_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.”
🤖 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 690 - 694, The function assert_disk_serial is a new public utility and must have explicit type hints: annotate the vm parameter (e.g., vm: Any or the concrete VM class used across utilities), annotate command as a list of strings (e.g., command: list[str] or List[str]) and set the return type to None (-> None). Update the signature of assert_disk_serial(vm, command=shlex.split(...)) accordingly, and add the necessary typing imports (Any or the VM class and List from typing) if they are not already present; keep the body using run_ssh_commands and HOTPLUG_DISK_SERIAL unchanged.
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: PR template content is incomplete and blocks review quality
The PR description is missing meaningful content in
##### What this PR does / why we need it:(currently placeholder-level). Please restore substantive rationale in that section before merge.As per coding guidelines: “
What this PR does / why we need it:MUST be present AND have meaningful content… flag as HIGH if placeholder-only.”🤖 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 1, The PR description currently contains only placeholder content for the header "##### What this PR does / why we need it:" — update that section to include a concise summary of changes, the reason these changes are required, the impact/risks, and any verification steps; specifically replace the placeholder text under "##### What this PR does / why we need it:" with meaningful content (summary, motivation, risk/impact, and testing notes) so the reviewer can evaluate the change.
🤖 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 `@utilities/storage.py`:
- Around line 69-70: Replace the inline shlex.split default in
assert_disk_serial with the module-level constant and avoid a shared mutable
default by changing the function signature to command=None; inside
assert_disk_serial set command = _DEFAULT_DISK_SERIAL_COMMAND when command is
None, then call run_ssh_commands(host=vm.ssh_exec, commands=command,
wait_timeout=TIMEOUT_2MIN, sleep=TIMEOUT_5SEC) and assert HOTPLUG_DISK_SERIAL is
present; also remove any unused duplicate shlex.split declarations so
_DEFAULT_DISK_SERIAL_COMMAND is the single source of truth.
---
Outside diff comments:
In `@tests/storage/upgrade/conftest.py`:
- Line 1: PR template still contains placeholder text for the required section
"##### What this PR does / why we need it:" — update the PR description to
replace the placeholder with a concrete summary that states the change set,
scope, and rationale (e.g., what behaviors or tests in this change affect, why
the upgrade tests/fixtures in this PR are needed, and any rollout/compatibility
considerations). Ensure the header "##### What this PR does / why we need it:"
is present and populated with meaningful content before merge.
In `@tests/storage/upgrade/test_upgrade_storage.py`:
- Line 1: Restore and populate the PR template section titled "##### What this
PR does / why we need it:" with concrete, meaningful content (not placeholders)
that summarizes the change, the motivation, and any user/behavioral impact;
replace any placeholder tokens like "TBD" or empty lines with a short paragraph
describing the purpose of this PR, intended behavior, and why it’s needed, and
ensure the other required template sections remain present and unchanged so
CI/QA checks recognize the template.
In `@utilities/storage.py`:
- Around line 690-694: The function assert_disk_serial is a new public utility
and must have explicit type hints: annotate the vm parameter (e.g., vm: Any or
the concrete VM class used across utilities), annotate command as a list of
strings (e.g., command: list[str] or List[str]) and set the return type to None
(-> None). Update the signature of assert_disk_serial(vm,
command=shlex.split(...)) accordingly, and add the necessary typing imports (Any
or the VM class and List from typing) if they are not already present; keep the
body using run_ssh_commands and HOTPLUG_DISK_SERIAL unchanged.
- Line 1: The PR description currently contains only placeholder content for the
header "##### What this PR does / why we need it:" — update that section to
include a concise summary of changes, the reason these changes are required, the
impact/risks, and any verification steps; specifically replace the placeholder
text under "##### What this PR does / why we need it:" with meaningful content
(summary, motivation, risk/impact, and testing notes) so the reviewer can
evaluate the change.
🪄 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: 55c31e4c-1507-4873-8cc1-d35bbacbc500
📒 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 (7)
tests/storage/upgrade/conftest.py (1)
27-41: LGTM!Also applies to: 45-51, 54-70, 73-79
tests/storage/upgrade/test_upgrade_storage.py (1)
6-10: LGTM!Also applies to: 20-20, 24-24, 27-27, 51-77, 126-141, 155-182
tests/storage/upgrade/constants.py (1)
5-9: LGTM!tests/storage/upgrade/utils.py (3)
3-16: LGTM!
20-48: LGTM!
51-66: LGTM!utilities/storage.py (1)
309-309: LGTM!
🛑 Comments failed to post (1)
utilities/storage.py (1)
69-70:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="utilities/storage.py" echo "== Grep for _DEFAULT_DISK_SERIAL_COMMAND ==" rg -n "_DEFAULT_DISK_SERIAL_COMMAND" "$FILE" || true echo "== Grep for by-id ==" rg -n "disk/by-id" "$FILE" || true echo "== Grep for shlex.split in file ==" rg -n "shlex\.split\(" "$FILE" || true echo "== Show lines 50-90 ==" nl -ba "$FILE" | sed -n '50,95p' echo "== Show lines 660-720 ==" nl -ba "$FILE" | sed -n '660,720p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 265
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="utilities/storage.py" echo "== Grep for _DEFAULT_DISK_SERIAL_COMMAND ==" rg -n "_DEFAULT_DISK_SERIAL_COMMAND" "$FILE" || true echo "== Grep for disk/by-id ==" rg -n "disk/by-id" "$FILE" || true echo "== Grep for shlex.split( in file ==" rg -n "shlex\.split\(" "$FILE" || true echo "== Show lines 50-95 ==" nl -ba "$FILE" | sed -n '50,95p' echo "== Show lines 660-720 ==" nl -ba "$FILE" | sed -n '660,720p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1117
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="utilities/storage.py" echo "== Lines 55-80 ==" awk 'NR>=55 && NR<=80 {printf "%d:%s\n", NR, $0}' "$FILE" echo "== Lines 675-705 ==" awk 'NR>=675 && NR<=705 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2283
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="utilities/storage.py" rg -n "_DEFAULT_DISK_SERIAL_COMMAND" . -S || true rg -n "assert_disk_serial\(" . -S || trueRepository: RedHatQE/openshift-virtualization-tests
Length of output: 665
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="utilities/storage.py" # Find definition of run_ssh_commands (prefer local definition, but repo search is fine) rg -n "def run_ssh_commands" "$FILE" || true rg -n "run_ssh_commands" "$FILE" | head -n 50 # Repo-wide locate of run_ssh_commands definition rg -n "def run_ssh_commands" . -S # Show assert_disk_serial callers context (lines around matches) FILE1="tests/storage/test_hotplug.py" FILE2="tests/storage/upgrade/test_upgrade_storage.py" awk 'NR>=200 && NR<=330 {print NR ":" $0}' "$FILE1" | head -n 140 awk 'NR>=70 && NR<=140 {print NR ":" $0}' "$FILE2" awk 'NR>=160 && NR<=240 {print NR ":" $0}' "$FILE2"Repository: RedHatQE/openshift-virtualization-tests
Length of output: 653
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search entire repo for run_ssh_commands implementation or any mention rg -n "def run_ssh_commands" . -S || true rg -n "run_ssh_commands\(" . -S | head -n 200 # Also check if pyhelper_utils is vendored in-repo fd -a "pyhelper_utils" . || true find . -maxdepth 5 -type f -name "*run_ssh_commands*" -o -name "shell.py" | head -n 200 || trueRepository: RedHatQE/openshift-virtualization-tests
Length of output: 10298
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import inspect try: import pyhelper_utils.shell as sh except Exception as e: print("IMPORT_ERROR:", repr(e)) raise SystemExit(0) print("run_ssh_commands module file:", getattr(sh, "__file__", None)) print("run_ssh_commands signature:", inspect.signature(sh.run_ssh_commands)) try: src = inspect.getsource(sh.run_ssh_commands) print("\n--- run_ssh_commands source (truncated to 250 lines) ---") lines = src.splitlines() for i, line in enumerate(lines[:250], start=1): print(f"{i:03d}:{line}") if len(lines) > 250: print(f"... ({len(lines)} total lines)") except OSError as e: print("SOURCE_ERROR:", repr(e)) PYRepository: RedHatQE/openshift-virtualization-tests
Length of output: 153
🌐 Web query:
pyhelper_utils.shell run_ssh_commands commands parameter mutates list💡 Result:
The pyhelper-utils library provides utility functions, including an SSH command executor, but there is no evidence in its official documentation or repository suggesting that the commands parameter in its run_ssh_command function mutates the provided list [1][2]. In Python, passing a list to a function allows the function to access and modify the original list object if it performs in-place operations (such as.append,.extend, or item assignment) [1]. However, official usage examples for pyhelper-utils typically demonstrate passing the output of shlex.split directly to the commands parameter, as shown below: from rrmngmnt import Host, UserWithPKey host = Host("1.1.1.1") user = UserWithPKey('user', '/path/to/pkey') host.executor_user = user run_ssh_command(host=host, commands=shlex.split("ls -l")) If you are concerned about list mutation, you can pass a copy of your list to the function to ensure the original remains unchanged: run_ssh_command(host=host, commands=my_command_list[:]) There is no specialized or documented behavior in pyhelper-utils that treats the commands parameter in a way that causes unexpected side effects or mutations to external variables [1][2].
Citations:
- 1: https://pypi.org/project/pyhelper-utils/1.0.17/
- 2: https://pypi.org/project/pyhelper-utils/2.0.2/
Fix unused
_DEFAULT_DISK_SERIAL_COMMANDand remove the shared mutable default list
utilities/storage.py:69defines_DEFAULT_DISK_SERIAL_COMMAND = shlex.split("sudo ls /dev/disk/by-id"), butassert_disk_serialuses its own inlineshlex.split(...)default atutilities/storage.py:690, leaving_DEFAULT_DISK_SERIAL_COMMANDas dead code.utilities/storage.py:690usesshlex.split(...)as a default argument; that list is created once at import time and then shared across calls. Default args should avoid shared mutable state.Example fix:
_DEFAULT_DISK_SERIAL_COMMAND = shlex.split("sudo ls /dev/disk/by-id") def assert_disk_serial(vm, command=None): if command is None: command = _DEFAULT_DISK_SERIAL_COMMAND 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"🤖 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 69 - 70, Replace the inline shlex.split default in assert_disk_serial with the module-level constant and avoid a shared mutable default by changing the function signature to command=None; inside assert_disk_serial set command = _DEFAULT_DISK_SERIAL_COMMAND when command is None, then call run_ssh_commands(host=vm.ssh_exec, commands=command, wait_timeout=TIMEOUT_2MIN, sleep=TIMEOUT_5SEC) and assert HOTPLUG_DISK_SERIAL is present; also remove any unused duplicate shlex.split declarations so _DEFAULT_DISK_SERIAL_COMMAND is the single source of truth.
|
/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 |
…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>
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 |
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