[4.21][Storage] Manual Cherry-Pick: Add Velero backup hooks opt-out tests - #5924
Conversation
Signed-off-by: Emanuele Prella <eprella@redhat.com> Implements automated test coverage for the Velero backup hook opt-out feature (CNV-79727), which allows users to annotate VMs with `kubevirt.io/skip-backup-hooks` to prevent Velero from injecting freeze/unfreeze hooks on the virt-launcher pod during backup — intended for metadata-only workflows where a third-party solution handles data protection. **Deliverables:** - `test_backup_paused_vm_hooks_disabled` (CNV-16267) — verifies no Velero hook annotations are present on the virt-launcher pod when backing up a paused VM with the opt-out annotation set - `test_backup_running_vm_hooks_disabled` (CNV-16268) — same verification for a running VM **New fixtures (tests/data_protection/oadp/conftest.py):** - `namespace_for_hooks_backup` — dedicated namespace for hooks opt-out tests - `rhel_vm_with_hooks_opt_out` — RHEL VM with `kubevirt.io/skip-backup-hooks: "true"` annotation, verified at fixture setup time - `paused_rhel_vm_with_hooks_opt_out` — derives from the above, pauses the VM before yielding **New utility (`tests/data_protection/oadp/utils.py`):** - `assert_velero_backup_hooks_not_injected` — inspects the virt-launcher pod and fails if any of the six known Velero hook annotation keys are present **New constants (`utilities/constants/oadp.py`):** - `SKIP_BACKUP_HOOKS_ANNOTATION` — the VM-level opt-out annotation key - `VELERO_BACKUP_HOOK_ANNOTATIONS` — the five virt-launcher annotation keys Velero injects when hooks are enabled **Scope note:** The STD originally included `test_full_backup_restore_hooks_disabled`. Restore verification was intentionally dropped from this PR because the hooks opt-out feature is orthogonal to restore correctness — backup completing without hook injection is the meaningful signal. A follow-up can cover restore if deemed necessary. https://redhat.atlassian.net/browse/CNV-85597 Assysted by Quality Flow AI Tool <!-- 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". --> https://redhat.atlassian.net/browse/CNV-88655 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * Added coverage for virtual machines that opt out of Velero backup hooks. * Added scenarios for both running and paused virtual machines using namespace-scoped backups. * Added validation that Velero backup hook annotations are not injected when opt-out is enabled. * Improved test collection and logging for backup hook scenarios. <!-- 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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesVelero backup-hook opt-out
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
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
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
📌 Additional InformationCustom Commands:
For more information, please refer to the project documentation or contact the maintainers. |
|
/retest all |
|
/build-and-push-container |
|
Verification failed for PR #5924. Execution details |
|
/build-and-push-container |
|
Verification failed for PR #5924. Execution details |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5924 published |
|
@coderabbitai review |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5924 published |
|
/verified All tests passed for PR #5924. 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 metadata and changed files ==='
gh pr view 5924 --json baseRefName,headRefName,headRefOid,files --jq '{base: .baseRefName, head: .headRefName, sha: .headRefOid, files: [.files[].path]}'
echo '=== Diff with zero context ==='
git diff --unified=0 HEAD^ HEAD -- \
tests/data_protection/oadp/conftest.py \
tests/data_protection/oadp/test_velero_backup_hooks.py \
tests/data_protection/oadp/utils.py \
utilities/constants.py
echo '=== AST outlines ==='
ast-grep outline tests/data_protection/oadp/conftest.py --items all
ast-grep outline tests/data_protection/oadp/test_velero_backup_hooks.py --items all
ast-grep outline tests/data_protection/oadp/utils.py --items all
ast-grep outline utilities/constants.py --items all
echo '=== Modified utility function declarations ==='
git diff HEAD^ HEAD --unified=0 -- tests/data_protection/oadp/utils.py utilities/constants.py \
| grep -E '^[+-][[:space:]]*(async[[:space:]]+)?def[[:space:]]+' || true
echo '=== Direct references to changed helper and constants in tests ==='
rg -n -C 4 '\bassert_velero_backup_hooks_not_injected\b|\bSKIP_BACKUP_HOOKS_ANNOTATION\b|\bVELERO_BACKUP_HOOK_ANNOTATIONS\b' tests utilities
echo '=== Fixture and test references in OADP test subtree ==='
rg -n -C 5 '\b(namespace_for_hooks_backup|rhel_vm_with_hooks_opt_out|paused_rhel_vm_with_hooks_opt_out)\b' tests
echo '=== Smoke-marked test files ==='
rg -l '`@pytest`\.mark\.smoke' tests || true
echo '=== Gating-marked test files ==='
rg -l '`@pytest`\.mark\.gating' tests || true
echo '=== All OADP conftest files ==='
fd -a '^conftest\.py$' tests/data_protection tests 2>/dev/null | sortLength of output: 49336 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr=5924
echo '=== Full PR file patches ==='
gh api "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "FILE: \(.filename)\nSTATUS: \(.status)\nPATCH:\n\(.patch // "<no patch>")\n---"'
echo '=== Relevant source ranges ==='
sed -n '1,340p' tests/data_protection/oadp/conftest.py
sed -n '1,140p' tests/data_protection/oadp/test_velero_backup_hooks.py
sed -n '1,135p' tests/data_protection/oadp/utils.py
sed -n '985,1010p' utilities/constants.py
echo '=== All repository references to changed symbols ==='
rg -n -C 3 '\bassert_velero_backup_hooks_not_injected\s*\(|\bSKIP_BACKUP_HOOKS_ANNOTATION\b|\bVELERO_BACKUP_HOOK_ANNOTATIONS\b' .
echo '=== Same-file calls to the changed helper ==='
rg -n -C 3 '\bassert_velero_backup_hooks_not_injected\s*\(' tests/data_protection/oadp/utils.py
echo '=== Fixture wrappers and cleanup paths ==='
rg -n -U -C 4 '`@pytest`\.fixture|yield from|yield |with VirtualMachineForTests|def (namespace_for_hooks_backup|rhel_vm_with_hooks_opt_out|paused_rhel_vm_with_hooks_opt_out)\b' \
tests/data_protection/oadp/conftest.py
echo '=== Marker-specific direct references ==='
for marker in smoke gating; do
echo "--- ${marker} test files that reference changed symbols ---"
while IFS= read -r file; do
if rg -q '\bassert_velero_backup_hooks_not_injected\b|\bSKIP_BACKUP_HOOKS_ANNOTATION\b|\bVELERO_BACKUP_HOOK_ANNOTATIONS\b' "$file"; then
echo "$file"
fi
done < <(rg -l "`@pytest`\.mark\.${marker}" tests)
done
echo '=== Smoke and gating conftest hierarchy references ==='
python3 - <<'PY'
from pathlib import Path
import re
symbols = re.compile(r"\b(assert_velero_backup_hooks_not_injected|SKIP_BACKUP_HOOKS_ANNOTATION|VELERO_BACKUP_HOOK_ANNOTATIONS)\b")
for marker in ("smoke", "gating"):
print(f"--- {marker} ---")
marked = []
for test in Path("tests").rglob("test_*.py"):
if re.search(rf"`@pytest`\.mark\.{marker}\b", test.read_text(errors="replace")):
marked.append(test)
print(f"marked_files={len(marked)}")
matches = set()
for test in marked:
current = test.parent
while True:
conf = current / "conftest.py"
if conf.exists() and symbols.search(conf.read_text(errors="replace")):
matches.add(str(conf))
if current == Path("tests"):
break
current = current.parent
print("hierarchy_matches=" + (", ".join(sorted(matches)) if matches else "<none>"))
PY
echo '=== First changed line in first changed file ==='
gh api "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[0] | {filename, patch}'Length of output: 44433 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr=5924
symbols='\b(assert_velero_backup_hooks_not_injected|SKIP_BACKUP_HOOKS_ANNOTATION|VELERO_BACKUP_HOOK_ANNOTATIONS)\b'
for marker in smoke gating; do
echo "=== ${marker}: marked test files with direct changed-symbol references ==="
rg -l "`@pytest`\.mark\.${marker}" tests > "/tmp/${marker}-files.txt" || true
cat "/tmp/${marker}-files.txt"
found=0
while IFS= read -r file; do
[ -n "$file" ] || continue
if rg -q "$symbols" "$file"; then
echo "DIRECT: $file"
found=1
fi
done < "/tmp/${marker}-files.txt"
[ "$found" -eq 1 ] || echo '<none>'
echo "=== ${marker}: parent conftest files with changed-symbol references ==="
python3 - "$marker" "/tmp/${marker}-files.txt" <<'PY'
from pathlib import Path
import re
import sys
marker, listing = sys.argv[1:]
symbols = re.compile(r"\b(assert_velero_backup_hooks_not_injected|SKIP_BACKUP_HOOKS_ANNOTATION|VELERO_BACKUP_HOOK_ANNOTATIONS)\b")
matches = set()
for raw in Path(listing).read_text().splitlines():
test = Path(raw)
directory = test.parent
while True:
conftest = directory / "conftest.py"
if conftest.is_file() and symbols.search(conftest.read_text(errors="replace")):
matches.add(str(conftest))
if directory == Path("tests"):
break
directory = directory.parent
print("\n".join(sorted(matches)) if matches else "<none>")
PY
done
echo '=== Inline target validation ==='
gh pr view "$pr" --repo "$repo" --json headRefOid --jq .headRefOid
gh api "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[0] | {filename, patch: (.patch | split("\n")[:8] | join("\n"))}'Length of output: 5946 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr=5924
commit_id="7ee46b2e0ac8855b2e5a1c5305c350715db59cd8"
body=$(cat <<'EOF'
## Test Execution Plan
**Run smoke tests: False.** No smoke-marked test directly uses `assert_velero_backup_hooks_not_injected`, `SKIP_BACKUP_HOOKS_ANNOTATION`, or `VELERO_BACKUP_HOOK_ANNOTATIONS`. No parent `conftest.py` in a smoke test hierarchy references these symbols.
**Run gating tests: False.** No gating-marked test directly uses these symbols. No parent `conftest.py` in a gating test hierarchy references these symbols.
**Affected tests to run**
- `tests/data_protection/oadp/test_velero_backup_hooks.py::TestVeleroBackupHookOptOut::test_backup_paused_vm_hooks_disabled` (polarion: `CNV-16267`)
- `tests/data_protection/oadp/test_velero_backup_hooks.py::TestVeleroBackupHookOptOut::test_backup_running_vm_hooks_disabled` (polarion: `CNV-16268`)
The tests exercise the full dependency path: test → `paused_rhel_vm_with_hooks_opt_out` or `rhel_vm_with_hooks_opt_out` → `namespace_for_hooks_backup` → `VirtualMachineForTests` with `SKIP_BACKUP_HOOKS_ANNOTATION` → `assert_velero_backup_hooks_not_injected` → `VirtualMachineInstance.virt_launcher_pod`.
**Real tests (cluster required)**
Error path: paused VM with backup-hook opt-out.
```bash
pytest -s -o log_cli=true -m tier2 --jira tests/data_protection/oadp/test_velero_backup_hooks.py::TestVeleroBackupHookOptOut::test_backup_paused_vm_hooks_disabledExpected: the Velero backup completes and the Happy path: running VM with backup-hook opt-out. pytest -s -o log_cli=true -m tier2 --jira tests/data_protection/oadp/test_velero_backup_hooks.py::TestVeleroBackupHookOptOut::test_backup_running_vm_hooks_disabledExpected: the Velero backup completes and the gh api You are interacting with an AI system. |
|
/lgtm |
|
/lgtm |
|
/approve |
|
/approve |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5924. |
|
/cherry-pick cnv-4.20 |
|
Cherry-picked PR [4.21][Storage] Manual Cherry-Pick: Add Velero backup hooks opt-out tests into cnv-4.20: #5961 |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:cnv-4.21 published |
11 participants
What this PR does / why we need it:
This is a manual cherry-pick of #5478.
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-88655
Summary by CodeRabbit
New Features
Tests