CherryPicked: [4.21] Fix _sc_name_for_storage_api#5424
Conversation
Fix error when preference exists but no storage class defined in it Signed-off-by: Harel Meir <hmeir@redhat.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
hmeir
changed the title
|
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
Branch Management
Custom Commands
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/lgtm |
|
/approve |
|
/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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5424"
echo "== PR files =="
gh pr view "$pr" --repo "$repo" --json files,headRefName,baseRefName,title \
| jq '{title, headRefName, baseRefName, files: [.files[] | {path, additions, deletions}]}'
echo
echo "== First 220 lines of PR diff =="
gh pr diff "$pr" --repo "$repo" | sed -n '1,220p'
echo
echo "== Search for modified symbol in repo =="
rg -n "_sc_name_for_storage_api" . || true
echo
echo "== Candidate files by name =="
fd -i "storage.*py|conftest.py|storage.*yaml|storage.*json" . || trueLength of output: 6338 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== AST outline for utilities/virt.py near changed area =="
ast-grep outline utilities/virt.py --view expanded | sed -n '1,260p'
echo
echo "== utilities/virt.py lines 980-1045 =="
nl -ba utilities/virt.py | sed -n '980,1045p'
echo
echo "== Search for likely enclosing method names and key APIs =="
rg -n -C3 '_sc_name_for_storage_api|claimPropertySets|get_default_storage_class|accessModes|StorageProfile\(' utilities/virt.py tests conftest.py || true
echo
echo "== Search for callers of enclosing VM/storage APIs in tests and conftests =="
rg -n -C2 'get_access_modes|access_mode|storage_profile|StorageProfile|data_volume_template|vm_preference|storage_class' tests conftest.py || true
echo
echo "== Smoke-marked tests =="
rg -n '`@pytest`\.mark\.smoke' tests || true
echo
echo "== Gating-marked tests =="
rg -n '`@pytest`\.mark\.gating' tests || trueLength of output: 9067 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== utilities/virt.py lines 1004-1036 =="
sed -n '1004,1036p' utilities/virt.py
echo
echo "== All usages of get_storage_configuration() with context =="
rg -n -C3 '\bget_storage_configuration\s*\(' . || true
echo
echo "== Nearby storage config call chain in VirtualMachineForTests =="
rg -n -C4 'generate_body|update_vm_storage_configuration|get_storage_configuration|custom_service_enable' utilities/virt.py || true
echo
echo "== Search for transitive callers via VM object attributes commonly consuming storage config =="
rg -n -C2 'access_mode|volume_mode|storage_class|claimPropertySets|preferredStorageClassName' tests conftest.py utilities || true
echo
echo "== Smoke-marked tests under storage =="
rg -n '`@pytest`\.mark\.smoke' tests/storage tests/virt tests/install_upgrade_operators || true
echo
echo "== Gating-marked tests under storage =="
rg -n '`@pytest`\.mark\.gating' tests/storage tests/virt tests/install_upgrade_operators || trueLength of output: 193844 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== utilities/virt.py lines 1008-1034 with numbers =="
awk 'NR>=1008 && NR<=1034 {printf "%d:%s\n", NR, $0}' utilities/virt.py
echo
echo "== generate_body / get_storage_configuration same-file call chain =="
awk 'NR>=688 && NR<=735 {printf "%d:%s\n", NR, $0}' utilities/virt.py
echo
awk 'NR>=900 && NR<=1018 {printf "%d:%s\n", NR, $0}' utilities/virt.py
echo
echo "== Focused search in smoke/gating files most likely to create VMs via VirtualMachineForTests wrappers =="
rg -n -C3 'VirtualMachineForTests|VirtualMachineForTestsFromTemplate|create_rhel_vm|create_cirros_vm|create_vm|data_volume_template|get_data_volume_template_dict_with_default_storage_class' \
tests/storage/cdi_clone/test_clone.py \
tests/storage/cdi_clone/conftest.py \
tests/storage/cdi_upload/test_upload_virtctl.py \
tests/storage/cdi_upload/conftest.py \
tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py \
tests/virt/cluster/common_templates/conftest.py \
tests/virt/node/general/test_container_disk_vm.py \
tests/storage/cdi_import/test_import_http.py \
tests/storage/cdi_import/conftest.py \
tests/virt/cluster/common_templates/general/test_base_template.py \
tests/infrastructure/golden_images/test_common_templates_data_volumes.py \
tests/infrastructure/golden_images/conftest.py || trueLength of output: 33470 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== utilities/storage.py relevant helpers =="
rg -n -C4 'def data_volume_template_dict|def data_volume_template_with_source_ref_dict|def create_vm_from_dv' utilities/storage.py || true
echo
echo "== tests/storage/cdi_clone/test_clone.py around gating/smoke tests =="
awk 'NR>=90 && NR<=140 {printf "%d:%s\n", NR, $0}' tests/storage/cdi_clone/test_clone.py
echo
awk 'NR>=240 && NR<=295 {printf "%d:%s\n", NR, $0}' tests/storage/cdi_clone/test_clone.py
echo
echo "== tests/storage/cdi_upload/test_upload_virtctl.py around smoke/gating tests =="
awk 'NR>=155 && NR<=190 {printf "%d:%s\n", NR, $0}' tests/storage/cdi_upload/test_upload_virtctl.py
echo
awk 'NR>=235 && NR<=275 {printf "%d:%s\n", NR, $0}' tests/storage/cdi_upload/test_upload_virtctl.pyLength of output: 8886 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== utilities/storage.py lines 539-575 =="
awk 'NR>=539 && NR<=575 {printf "%d:%s\n", NR, $0}' utilities/storage.py
echo
echo "== utilities/storage.py lines 968-1015 =="
awk 'NR>=968 && NR<=1015 {printf "%d:%s\n", NR, $0}' utilities/storage.pyLength of output: 3381 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== vm_preference usages in tests/helpers with context =="
rg -n -C4 'vm_preference|preference' tests utilities | sed -n '1,260p'
echo
echo "== Focused VirtualMachineForTests + vm_preference call sites =="
rg -n -C5 'VirtualMachineForTests\(|VirtualMachineForTestsFromTemplate\(|create_rhel_vm\(|create_cirros_vm\(' tests utilities \
| rg -n 'vm_preference|preference|data_volume_template|source_ref|instance_type' || trueLength of output: 35476 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Candidate files with markers =="
rg -n '`@pytest`\.mark\.(smoke|gating)' \
tests/infrastructure/golden_images/update_boot_source/test_boot_sources_vm.py \
tests/infrastructure/golden_images/update_boot_source/test_ssp_data_import_crons.py \
tests/infrastructure/instance_types/supported_os/utils.py \
tests/infrastructure/instance_types/supported_os/conftest.py \
tests/virt/node/general/test_machinetype.py \
tests/virt/upgrade/conftest.py \
tests/virt/upgrade/test_upgrade_virt.py || true
echo
echo "== test_boot_sources_vm relevant slice =="
awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' tests/infrastructure/golden_images/update_boot_source/test_boot_sources_vm.py
echo
echo "== test_ssp_data_import_crons relevant slice =="
awk 'NR>=250 && NR<=320 {printf "%d:%s\n", NR, $0}' tests/infrastructure/golden_images/update_boot_source/test_ssp_data_import_crons.py
echo
echo "== update_boot_source conftest relevant slice =="
awk 'NR>=180 && NR<=220 {printf "%d:%s\n", NR, $0}' tests/infrastructure/golden_images/update_boot_source/conftest.py
echo
echo "== supported_os utils/conftest relevant slices =="
awk 'NR>=1 && NR<=40 {printf "%d:%s\n", NR, $0}' tests/infrastructure/instance_types/supported_os/utils.py
echo
awk 'NR>=118 && NR<=145 {printf "%d:%s\n", NR, $0}' tests/infrastructure/instance_types/supported_os/conftest.py
echo
echo "== machinetype relevant slice =="
awk 'NR>=40 && NR<=80 {printf "%d:%s\n", NR, $0}' tests/virt/node/general/test_machinetype.pyLength of output: 13540 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Usages of explicit vm_preference-based fixtures/helpers =="
rg -n -C3 'golden_image_windows_vm|rhel_vm_with_cluster_instance_type_and_preference|vm_preference_for_test|WINDOWS_2K22_PREFERENCE|vm_preference=' tests || true
echo
echo "== tests/conftest.py explicit preference fixtures around likely callers =="
awk 'NR>=2298 && NR<=2335 {printf "%d:%s\n", NR, $0}' tests/conftest.py
echo
awk 'NR>=2148 && NR<=2175 {printf "%d:%s\n", NR, $0}' tests/conftest.py
echo
echo "== supported_os tests using golden_image_windows_vm =="
rg -n -C3 'golden_image_windows_vm' tests/infrastructure/instance_types || true
echo
echo "== grep smoke/gating among those candidate test files =="
rg -n '`@pytest`\.mark\.(smoke|gating)' tests/infrastructure/instance_types tests/virt tests/storage tests/observability || trueLength of output: 37679 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== tests/virt/upgrade/conftest.py relevant slice =="
awk 'NR>=136 && NR<=160 {printf "%d:%s\n", NR, $0}' tests/virt/upgrade/conftest.py
echo
echo "== tests/virt/upgrade/test_upgrade_virt.py gating tests and fixture usage =="
awk 'NR>=60 && NR<=215 {printf "%d:%s\n", NR, $0}' tests/virt/upgrade/test_upgrade_virt.py
echo
echo "== supported_os windows template source =="
rg -n -C3 'windows_data_volume_template' tests/infrastructure/instance_types/supported_os || trueLength of output: 10310 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5424"
echo "== tests/virt/upgrade/conftest.py vms_for_upgrade mapping =="
awk 'NR>=100 && NR<=140 {printf "%d:%s\n", NR, $0}' tests/virt/upgrade/conftest.py
echo
awk 'NR>=160 && NR<=220 {printf "%d:%s\n", NR, $0}' tests/virt/upgrade/conftest.py
comment_body=$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — no smoke-marked test with a verified path to the changed branch was found.
- **Run gating tests: True** — verified path: `tests/virt/upgrade/test_upgrade_virt.py::{test_is_vm_running_before_upgrade,test_vm_console_before_upgrade,test_vm_ssh_before_upgrade,test_is_vm_running_after_upgrade,test_vm_console_after_upgrade,test_vm_ssh_after_upgrade}` → `vms_for_upgrade` → `tests/virt/upgrade/conftest.py::vm_with_instancetypes_for_upgrade` → `utilities/virt.py::VirtualMachineForTests.update_vm_storage_configuration()` → `utilities/virt.py::VirtualMachineForTests.get_storage_configuration()` → `_sc_name_for_storage_api()`.
**Affected tests to run**
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_is_vm_running_before_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_console_before_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_ssh_before_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_is_vm_running_after_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_console_after_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_ssh_after_upgrade` (gating)
**Real tests (cluster required)**
Error path (the fix):
`pytest tests/virt/upgrade/test_upgrade_virt.py -k 'test_is_vm_running_before_upgrade or test_vm_console_before_upgrade' --collect-only --default-storage-class=<cluster-default-sc>`
Expected: collection/setup reaches `vm_with_instancetypes_for_upgrade` without failing when the preference does not define `preferredStorageClassName`; no storage-profile lookup crash from `_sc_name_for_storage_api()`.
Happy path (regression):
`pytest tests/virt/upgrade/test_upgrade_virt.py -k 'test_vm_ssh_before_upgrade or test_vm_console_after_upgrade' --default-storage-class=<cluster-default-sc>`
Expected: the instance-type/preference VM is created and remains usable before/after upgrade; console and SSH checks still pass.
EOF
)
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
# Try inline PR comment on the first changed line.
if gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/comments" \
-f body="$comment_body" \
-f commit_id="$head_sha" \
-f path="utilities/virt.py" \
-F line=1019 \
-f side="RIGHT"
then
exit 0
fi
# Fallback: regular PR comment if inline posting fails.
gh pr comment "$pr" --repo "$repo" --body "$comment_body"Length of output: 7350 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5424"
echo "== Confirm vms_for_upgrade contents =="
rg -n -C4 'def vms_for_upgrade|vm_with_instancetypes_for_upgrade|yield vms_list|vms_list\.append' tests/virt/upgrade/conftest.py
echo
echo "== Confirm after-upgrade SSH test =="
awk 'NR>=205 && NR<=235 {printf "%d:%s\n", NR, $0}' tests/virt/upgrade/test_upgrade_virt.py
comment_body=$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — no smoke-marked test with a verified dependency path to the changed branch was found.
- **Run gating tests: True** — verified path: `tests/virt/upgrade/test_upgrade_virt.py::{test_is_vm_running_before_upgrade,test_vm_console_before_upgrade,test_vm_ssh_before_upgrade,test_is_vm_running_after_upgrade,test_vm_console_after_upgrade,test_vm_ssh_after_upgrade}` → `vms_for_upgrade` → `tests/virt/upgrade/conftest.py::vm_with_instancetypes_for_upgrade` → `utilities/virt.py::VirtualMachineForTests.update_vm_storage_configuration()` → `utilities/virt.py::VirtualMachineForTests.get_storage_configuration()` → `_sc_name_for_storage_api()`.
**Affected tests to run**
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_is_vm_running_before_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_console_before_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_ssh_before_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_is_vm_running_after_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_console_after_upgrade` (gating)
- `tests/virt/upgrade/test_upgrade_virt.py::TestUpgradeVirt::test_vm_ssh_after_upgrade` (gating)
**Real tests (cluster required)**
Error path (the fix):
`pytest tests/virt/upgrade/test_upgrade_virt.py -k 'test_is_vm_running_before_upgrade or test_vm_console_before_upgrade' --collect-only --default-storage-class=<cluster-default-sc>`
Expected: setup reaches the instance-type/preference VM without failing when the preference does not define `preferredStorageClassName`; no storage-profile lookup crash from `_sc_name_for_storage_api()`.
Happy path (regression):
`pytest tests/virt/upgrade/test_upgrade_virt.py -k 'test_vm_ssh_before_upgrade or test_vm_console_after_upgrade' --default-storage-class=<cluster-default-sc>`
Expected: the instance-type/preference VM is created and remains usable; console and SSH checks still pass across the upgrade flow.
EOF
)
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
echo
echo "== Try non-blocking PR review with one inline comment =="
set +e
review_resp=$(gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
-f body="" \
-f event="COMMENT" \
-f commit_id="$head_sha" \
-f comments[][path]="utilities/virt.py" \
-F comments[][line]=1019 \
-f comments[][side]="RIGHT" \
-f comments[][body]="$comment_body" 2>&1)
review_status=$?
set -e
printf '%s\n' "$review_resp"
if [ $review_status -eq 0 ]; then
exit 0
fi
echo
echo "== Fallback: regular PR comment via REST issues endpoint =="
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/issues/$pr/comments" \
-f body="$comment_body"Length of output: 7298
CRITICAL: I could not complete the required GitHub posting workflow because the integration lacks write permission on this PR. All three attempts failed with
Because the rule exists to keep the plan attached to the changed code, that permission failure blocks the final delivery step. Test Execution Plan
Affected tests to run
Real tests (cluster required) Happy path (regression): |
|
/lgtm |
|
/approve |
|
/cherry-pick cnv-4.20 cnv-4.19 |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:cnv-4.21 published |
|
/cherry-pick cnv-4.18 |
12 participants
What this PR does / why we need it:
Backport of #3416
Which issue(s) this PR fixes:
PR #5228 was merged recently, but its prerequisite fix PR #3416 (Fix _sc_name_for_storage_api) was not.
Causing upgrade failures in setup in multiple branches.
Special notes for reviewer:
jira-ticket:
NONE