Revert "CherryPicked: [cnv-4.19] CherryPicked: [4.21] Fix _sc_name_for_storage_api"#5488
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL 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 |
|
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
|
/security-override |
|
Security checks overridden by @vsibirsk. Security check runs set to pass. |
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 5488 --repo RedHatQE/openshift-virtualization-tests > /tmp/pr.diff
echo "---FILES---"
gh pr view 5488 --repo RedHatQE/openshift-virtualization-tests --json files --jq '.files[].path'
echo "---DIFF---"
cat /tmp/pr.diffLength of output: 1220 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "def get_default_storage_class" -A 15 utilities/ libs/ 2>/dev/null
echo "---callers---"
rg -n "_sc_name_for_storage_api|get_default_storage_class" -g '*.py' 2>/dev/null
echo "---class context---"
sed -n '960,1030p' utilities/virt.pyLength of output: 1684 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---callers of get_default_storage_class---"
rg -n "get_default_storage_class" --type=py
echo "---callers of _sc_name_for_storage_api---"
rg -n "_sc_name_for_storage_api" --type=py -C3
echo "---smoke marker files---"
rg -l '`@pytest.mark.smoke`' --type=py | head -50
echo "---gating marker files---"
rg -l '`@pytest.mark.gating`' --type=py | head -50Length of output: 732 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---callers of get_default_storage_class---"
rg -n "get_default_storage_class" -g '*.py'
echo "---callers of _sc_name_for_storage_api---"
rg -n "_sc_name_for_storage_api" -g '*.py' -C3
echo "---smoke marker files count---"
rg -l '`@pytest.mark.smoke`' -g '*.py' | wc -l
echo "---gating marker files count---"
rg -l '`@pytest.mark.gating`' -g '*.py' | wc -lLength of output: 738 🏁 Script executed: #!/bin/bash
set +e
echo "---callers of get_default_storage_class---"
rg -n "get_default_storage_class" -g '*.py'
echo "exit:$?"
echo "---callers of _sc_name_for_storage_api---"
rg -n "_sc_name_for_storage_api" -g '*.py' -C3
echo "exit:$?"
echo "---smoke marker files count---"
rg -l '`@pytest.mark.smoke`' -g '*.py' | wc -l
echo "---gating marker files count---"
rg -l '`@pytest.mark.gating`' -g '*.py' | wc -l
pwd
lsLength of output: 764 🏁 Script executed: echo "hello"
pwd
ls -laLength of output: 1913 🏁 Script executed: echo "---callers of get_default_storage_class---"
rg -n "get_default_storage_class" -g '*.py'
echo "---callers of _sc_name_for_storage_api---"
rg -n "_sc_name_for_storage_api" -g '*.py' -C3
echo "---smoke marker count---"
rg -l '`@pytest.mark.smoke`' -g '*.py' | wc -l
echo "---gating marker count---"
rg -l '`@pytest.mark.gating`' -g '*.py' | wc -lLength of output: 696 🏁 Script executed: echo "---grep get_default_storage_class---"
grep -rn "get_default_storage_class" --include="*.py" . || echo "grep exit $?"
echo "---grep _sc_name_for_storage_api---"
grep -rn "_sc_name_for_storage_api" --include="*.py" . || echo "grep exit $?"Length of output: 1585 🏁 Script executed: sed -n '1250,1275p' tests/conftest.py
echo "---"
sed -n '1290,1320p' utilities/virt.py
echo "---"
sed -n '1000,1025p' utilities/virt.py
echo "---find fixture name for line 1266---"
awk 'NR==1266{print}' tests/conftest.py
grep -n "^def \|`@pytest.fixture`" tests/conftest.py | awk -F: '$1<1266' | tail -5
echo "---find callers of that fixture---"
sed -n '1240,1270p' tests/conftest.pyLength of output: 5274 🏁 Script executed: echo "---get_storage_configuration callers---"
grep -rn "get_storage_configuration" --include="*.py" .
echo "---class context---"
grep -n "^class " utilities/virt.py | awk -F: '$1<1004' | tail -3Length of output: 666 🏁 Script executed: sed -n '890,920p' utilities/virt.py
echo "---find calling method name---"
awk 'NR>=850 && NR<=915 && /def /' utilities/virt.pyLength of output: 2019 🏁 Script executed: rg -l '`@pytest.mark.smoke`' -g '*.py' 2>&1 | head -20
echo "count smoke:"
rg -l '`@pytest.mark.smoke`' -g '*.py' 2>&1 | wc -l
echo "count gating:"
rg -l '`@pytest.mark.gating`' -g '*.py' 2>&1 | wc -lLength of output: 341 🏁 Script executed: echo "smoke files:"
grep -rl "pytest.mark.smoke" --include="*.py" tests | wc -l
grep -rl "pytest.mark.smoke" --include="*.py" tests | head -10
echo "gating files:"
grep -rl "pytest.mark.gating" --include="*.py" tests | wc -l
grep -rl "pytest.mark.gating" --include="*.py" tests | head -10Length of output: 1626 🏁 Script executed: grep -n "VirtualMachineForTests\|data_volume_template" tests/storage/cdi_clone/test_clone.py | head -10
echo "---"
grep -n "VirtualMachineForTests\|data_volume_template" tests/virt/node/general/test_container_disk_vm.py | head -10Length of output: 649 🏁 Script executed: gh pr comment 5488 --repo RedHatQE/openshift-virtualization-tests --body "placeholder" --help 2>&1 | head -1
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5488/files --jq '.[0].filename, .[0].patch' | head -20Length of output: 1066 🏁 Script executed: HEAD_SHA=$(gh pr view 5488 --repo RedHatQE/openshift-virtualization-tests --json headRefOid --jq '.headRefOid')
echo "$HEAD_SHA"Length of output: 308 🏁 Script executed: grep -n "get_storage_configuration\|_sc_name_for_storage_api\|else:\|return get_default_storage_class" utilities/virt.py | sed -n '1,10p'
sed -n '1000,1020p' utilities/virt.py | cat -nLength of output: 1699 🏁 Script executed: HEAD_SHA="cf5027faacf90c0b070ca23551ac8d4fa64ef77a"
BODY=$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — No traced path from `get_default_storage_class()` (utilities/storage.py) to a smoke-marked test through fixture/utility chains verified below.
- **Run gating tests: True** — `get_storage_configuration()` (utilities/virt.py, called from `update_vm_storage_configuration` → `VirtualMachineForTests.__init__`) is exercised whenever a VM is created with `data_volume_template`/`data_volume`/`pvc` and `is_vm_from_template=False`. This is the exact path that was raising `TypeError: get_default_storage_class() got an unexpected keyword argument 'client'` before this revert. Many gating tests instantiate `VirtualMachineForTests` this way, e.g.:
- `tests/storage/cdi_clone/test_clone.py` (uses `VirtualMachineForTests` with `data_volume_template`)
- `tests/virt/upgrade/utils.py::*` (calls `vm.get_storage_configuration()` directly)
- broadly, `-m gating` tests under `tests/storage/` and `tests/virt/` that create VMs from a `DataVolumeTemplate`/PVC without a preference/template.
- **Affected tests to run**
- `tests/storage/cdi_clone/test_clone.py` (gating)
- `tests/virt/upgrade/utils.py` usages (gating) — verify via `tests/virt/upgrade/` suite
- `-m gating` subset touching `utilities/virt.py::VirtualMachineForTests` and `utilities/storage.py::get_default_storage_class` (broad sanity pass recommended given prior widespread setup failures across gating lanes)
**Real tests (cluster required)**
Error path (the fix — confirms `TypeError` no longer raised):
`pytest tests/storage/cdi_clone/test_clone.py --collect-only`
Expected: collection and setup succeed; no `TypeError: get_default_storage_class() got an unexpected keyword argument 'client'`
Happy path (regression — default SC resolution still works when no client kwarg is passed):
`pytest tests/storage/cdi_clone/test_clone.py -k test_clone -m gating`
Expected: VM setup resolves the default storage class correctly and test passes.
EOF
)
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5488/comments \
-f commit_id="$HEAD_SHA" \
-f path="utilities/virt.py" \
-F line=1012 \
-f side="RIGHT" \
-f body="$BODY"Length of output: 7304 The inline test execution plan comment has been posted successfully on |
|
/retest dco |
|
No dco configured for this repository |
…r_storag…" This reverts commit 02ce5c1. Signed-off-by: vsibirsk <vsibirsk@redhat.com>
vsibirsk
force-pushed
the
revert-5438-CherryPicked-4.21-fix-storage-config-FzBKz
branch
from
cf5027f to
be4c297
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/rerun-smoke |
vsibirsk
deleted the
revert-5438-CherryPicked-4.21-fix-storage-config-FzBKz
branch
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:cnv-4.19 published |
5 participants
Reverts #5438
breaks gating lanes
failed on setup with "TypeError: get_default_storage_class() got an unexpected keyword argument 'client'"