[Storage][4.21] Remove artifactory usage from disk preallocation tests#5391
Conversation
…group them into a feature-level folder (RedHatQE#5265) Refactors CDI disk preallocation tests from a single flat file into a proper feature subdirectory (tests/storage/disk_preallocation/), replacing Artifactory-hosted images with Quay registry images. Fix flakiness related to Artifactory-hosted images on gating tests. DataSource (golden images) was evaluated but is not suitable because on storage classes with smart cloning (e.g., ocs-storagecluster-ceph-rbd-virtualization), CDI skips its own preallocation step during clone operations. Assisted by Claude Code <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> CNV-90355 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Tests** * Reorganized the disk preallocation test suite into a dedicated post-upgrade module with dedicated fixtures and shared utilities. * Added new test cases validating disk preallocation behavior for DataVolumes with per-DV requests, CDI-level defaults, explicit `false` overrides, and “blank” DataVolumes. * Removed the previous all-in-one disk preallocation test module to streamline coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Emanuele Prella <eprella@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 |
|
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-4
requested review from
dalia-frank and
josemacassan
openshift-virtualization-qe-bot-4
requested review from
Ahmad-Hafe,
acinko-rh and
kgoldbla
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5391 published |
|
/verified All tests passed for PR #5391. Execution details |
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
echo "== PR files =="
gh pr diff 5391 --repo RedHatQE/openshift-virtualization-tests --name-only
echo
echo "== PR unified diff (first 260 lines) =="
gh pr diff 5391 --repo RedHatQE/openshift-virtualization-tests | sed -n '1,260p'
echo
echo "== Git status/branch =="
git status --short
git branch --show-current
echo
echo "== Locate changed files in repo =="
while IFS= read -r f; do
[ -n "$f" ] || continue
echo "-- $f"
fd --full-path "^${f}$" .
done < <(gh pr diff 5391 --repo RedHatQE/openshift-virtualization-tests --name-only)
echo
echo "== First changed file and first added line in diff =="
python - <<'PY'
import subprocess, re
diff = subprocess.check_output(
["gh", "pr", "diff", "5391", "--repo", "RedHatQE/openshift-virtualization-tests"],
text=True,
)
current_file = None
new_line = None
for line in diff.splitlines():
if line.startswith("diff --git "):
m = re.match(r"diff --git a/(.*?) b/(.*)", line)
if m:
current_file = m.group(2)
new_line = None
elif line.startswith("@@ "):
m = re.match(r"@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", line)
if m:
new_line = int(m.group(1))
elif current_file and new_line is not None:
if line.startswith("+") and not line.startswith("+++"):
print(f"{current_file}:{new_line}")
raise SystemExit(0)
elif line.startswith("-") and not line.startswith("---"):
pass
else:
new_line += 1
print("NO_ADDED_LINE_FOUND")
PYLength of output: 11444 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5391"
echo "== Base and head refs =="
gh pr view "$pr" --repo "$repo" --json baseRefName,headRefName,headRefOid --jq '{base:.baseRefName, head:.headRefName, sha:.headRefOid}'
echo
echo "== Full deleted file from base branch =="
git show "origin/$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq -r '.baseRefName'):tests/storage/test_disk_preallocation.py" | sed -n '1,260p'
echo
echo "== Outline: new test file =="
ast-grep outline tests/storage/disk_preallocation/test_disk_preallocation.py --view expanded || true
echo
echo "== Outline: new local conftest =="
ast-grep outline tests/storage/disk_preallocation/conftest.py --view expanded || true
echo
echo "== Parent conftest files near storage tests =="
fd 'conftest\.py' tests | sort | sed -n '1,120p'
echo
echo "== tests/storage/conftest.py relevant section =="
sed -n '1,260p' tests/storage/conftest.py
echo
echo "== tests/conftest.py imports of storage or preallocation symbols =="
rg -n -C2 'disk_preallocation|preallocation|test_disk_preallocation|create_dv|ResourceEditorValidateHCOReconcile' tests/conftest.py tests/storage/conftest.py tests/storage/disk_preallocation/conftest.py tests/storage/disk_preallocation/test_disk_preallocation.py tests/storage/disk_preallocation/utils.py
echo
echo "== Smoke-marked files =="
rg -n --glob 'tests/**/*.py' '`@pytest`\.mark\.smoke'
echo
echo "== Gating-marked files under new directory =="
rg -n --glob 'tests/storage/disk_preallocation/**/*.py' '`@pytest`\.mark\.gating'
echo
echo "== Uses of new local fixtures outside new test file =="
rg -n -C2 'cdi_preallocation_enabled|registry_dv_with_preallocation|registry_dv_no_preallocation_spec|registry_dv_with_preallocation_false|blank_dv_with_preallocation' tests
echo
echo "== Uses of new local helper functions outside new test file =="
rg -n -C2 'assert_preallocation_requested_annotation|assert_preallocation_annotation|wait_for_cdi_preallocation_enabled' tests
echo
echo "== Diffstat and full diff for changed storage files =="
git diff --stat "origin/$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq -r '.baseRefName')"...HEAD -- tests/storage/test_disk_preallocation.py tests/storage/disk_preallocation/conftest.py tests/storage/disk_preallocation/test_disk_preallocation.py tests/storage/disk_preallocation/utils.py
echo
git diff "origin/$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq -r '.baseRefName')"...HEAD -- tests/storage/test_disk_preallocation.py tests/storage/disk_preallocation/conftest.py tests/storage/disk_preallocation/test_disk_preallocation.py tests/storage/disk_preallocation/utils.py | sed -n '1,320p'Length of output: 3304 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5391"
path="tests/storage/disk_preallocation/conftest.py"
line="1"
sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq -r '.headRefOid')"
# Lightweight verification used to support the plan text.
echo "== Smoke-marked tests =="
rg -n --glob 'tests/**/*.py' '`@pytest`\.mark\.smoke' tests || true
echo
echo "== Gating-marked tests in affected area =="
rg -n --glob 'tests/storage/disk_preallocation/**/*.py' '`@pytest`\.mark\.gating' tests/storage/disk_preallocation || true
echo
echo "== Parent/local conftest and helper references =="
rg -n -C1 'disk_preallocation|assert_preallocation_|wait_for_cdi_preallocation_enabled|registry_dv_|blank_dv_with_preallocation|cdi_preallocation_enabled' \
tests/conftest.py tests/storage/conftest.py tests/storage/disk_preallocation/conftest.py tests/storage/disk_preallocation/test_disk_preallocation.py tests/storage/disk_preallocation/utils.py || true
body="$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — Verified by searching smoke-marked tests under `tests/`; no smoke-marked test is in `tests/storage/disk_preallocation/`. The new `tests/storage/disk_preallocation/conftest.py` is package-local, and there is no parent `tests/conftest.py` or `tests/storage/conftest.py` dependency path into these new fixtures/helpers that would widen impact to smoke suites.
- **Run gating tests: True** — Direct dependency paths:
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` → `registry_dv_with_preallocation` fixture in `tests/storage/disk_preallocation/conftest.py` → `create_dv(source=REGISTRY_STR, url=QUAY_FEDORA_CONTAINER_IMAGE, preallocation=True)`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_for_blank_dv` → `blank_dv_with_preallocation` fixture in `tests/storage/disk_preallocation/conftest.py` → `create_dv(source="blank", preallocation=True)`
**Affected tests to run**
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_for_blank_dv` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_without_preallocation`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_with_preallocation_false`
**Real tests (cluster required)**
Error path (the fix):
`pytest -s -o log_cli=true tests/storage/disk_preallocation/test_disk_preallocation.py -k 'test_preallocation_dv or test_preallocation_globally_dv_spec_without_preallocation'`
Expected: registry-backed DataVolumes import successfully using `QUAY_FEDORA_CONTAINER_IMAGE`, and the PVC annotations match the expected preallocation state.
Happy path (regression):
`pytest -s -o log_cli=true tests/storage/disk_preallocation/test_disk_preallocation.py -k 'test_preallocation_for_blank_dv or test_preallocation_globally_dv_spec_with_preallocation_false'`
Expected: blank DataVolume preallocation still succeeds, and explicit `preallocation=False` still overrides the global CDI preallocation setting.
EOF
)"
post_inline() {
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"repos/$repo/pulls/$pr/comments" \
-f body="$body" \
-f commit_id="$sha" \
-f path="$path" \
-F side='RIGHT' \
-F line="$line"
}
# Retry inline comment twice, then fall back to a regular PR comment.
if ! post_inline; then
sleep 2
if ! post_inline; then
gh pr comment "$pr" --repo "$repo" --body "$body"
fi
fiLength of output: 4034 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5391"
path="tests/storage/disk_preallocation/conftest.py"
line=1
sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
body="$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — Verified by searching smoke-marked tests under `tests/`; no smoke-marked test is in `tests/storage/disk_preallocation/`. The new `tests/storage/disk_preallocation/conftest.py` is package-local, and there is no parent `tests/conftest.py` or `tests/storage/conftest.py` dependency path into these new fixtures/helpers that would widen impact to smoke suites.
- **Run gating tests: True** — Direct dependency paths:
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` → `registry_dv_with_preallocation` fixture in `tests/storage/disk_preallocation/conftest.py` → `create_dv(source=REGISTRY_STR, url=QUAY_FEDORA_CONTAINER_IMAGE, preallocation=True)`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_for_blank_dv` → `blank_dv_with_preallocation` fixture in `tests/storage/disk_preallocation/conftest.py` → `create_dv(source="blank", preallocation=True)`
**Affected tests to run**
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_for_blank_dv` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_without_preallocation`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_with_preallocation_false`
**Real tests (cluster required)**
Error path (the fix):
`pytest -s -o log_cli=true tests/storage/disk_preallocation/test_disk_preallocation.py -k 'test_preallocation_dv or test_preallocation_globally_dv_spec_without_preallocation'`
Expected: registry-backed DataVolumes import successfully using `QUAY_FEDORA_CONTAINER_IMAGE`, and the PVC annotations match the expected preallocation state.
Happy path (regression):
`pytest -s -o log_cli=true tests/storage/disk_preallocation/test_disk_preallocation.py -k 'test_preallocation_for_blank_dv or test_preallocation_globally_dv_spec_with_preallocation_false'`
Expected: blank DataVolume preallocation still succeeds, and explicit `preallocation=False` still overrides the global CDI preallocation setting.
EOF
)"
post_inline() {
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/comments" \
-f body="$body" \
-f commit_id="$sha" \
-f path="$path" \
-F side=RIGHT \
-F line="$line"
}
if ! post_inline; then
sleep 2
if ! post_inline; then
gh pr comment "$pr" --repo "$repo" --body "$body"
fi
fiLength of output: 7736 |
6 participants
What this PR does / why we need it:
Refactors CDI disk preallocation tests from a single flat file into a proper feature subdirectory (tests/storage/disk_preallocation/), replacing Artifactory-hosted images with Quay registry images.
Which issue(s) this PR fixes:
Fix flakiness related to Artifactory-hosted images on gating tests.
Special notes for reviewer:
Manual backport of #5265
jira-ticket:
CNV-90355