WIP: [Storage] Add Velero backup hooks opt-out tests#5478
WIP: [Storage] Add Velero backup hooks opt-out tests#5478ema-aka-young wants to merge 7 commits into
Conversation
Signed-off-by: Emanuele Prella <eprella@redhat.com>
GitHub Actions
Waiting
📝 WalkthroughWalkthroughAdds OADP/Velero coverage for VMs annotated to skip backup hooks, including VM setup, backup and restore fixtures, Velero log retrieval, and assertions that freeze hooks are absent. ChangesVelero hooks opt-out feature
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ 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 |
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
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. |
|
/wip |
openshift-virtualization-qe-bot-3
added
commented-ema-aka-young
and removed
changes-requested-coderabbitai[bot]
labels
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5478 published |
|
/verified All tests passed for PR #5478. 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: |
|
Tip For best results, initiate chat on the files or code changes.
Test Execution Plan
Both tests already passed in job (this is my third identical pass on this thread — happy to re-run if new commits land) |
| with create_rhel_vm( | ||
| storage_class=snapshot_storage_class_name_scope_module, | ||
| namespace=namespace_for_backup.name, | ||
| dv_name="dv-hooks-opt-out", | ||
| vm_name="vm-hooks-opt-out", | ||
| rhel_image=Images.Rhel.LATEST_RELEASE_STR, | ||
| client=admin_client, | ||
| wait_running=True, | ||
| annotations={SKIP_BACKUP_HOOKS_ANNOTATION: "true"}, | ||
| ) as vm: |
There was a problem hiding this comment.
Instead of adding more tests that use artifactory, let's create a VM from the DataSource.
You can probably reuse the rhel_vm_for_snapshot fixture (but pay attention, it's a function scope)
Please check the scope - can we use the same VM for both tests in the class, or do we need a new VM for each test?
There was a problem hiding this comment.
It makes total sense to use Datasource, will update it.
| wait_for_running_vm( | ||
| vm=rhel_vm_with_hooks_opt_out, | ||
| wait_until_running_timeout=TIMEOUT_15MIN, | ||
| wait_for_interfaces=False, | ||
| check_ssh_connectivity=False, | ||
| ) |
There was a problem hiding this comment.
If VM was stopped before, should we make sure to start it?
Maybe running_vm(vm=rhel_vm_with_hooks_opt_out) would be a better fit here?
There was a problem hiding this comment.
It's now updated. We struggled to ssh connect to the restored VM, but we investigated the reasons and now should be good (we are running sync so that the ssh host keys are present on the restored VM).
There was a problem hiding this comment.
Back on this: it just seems we can't reliably check the post restore, it's by design difficult to guarantee it without fsfreeze. So here we just ensure no pre and post backups hooks are run and ignore the post restore check.
There was a problem hiding this comment.
Yes, we've discussed this extensively offline and decided it's better to simply verify that the pre- and post-hooks are removed and leaving the post-restore checks out of the test since we can't guarantee the VM will be bootable without an fsfreeze.
There was a problem hiding this comment.
Since we are facing this limitation only now, I think we should:
- open a bug
- verify with PM that it's acceptable behaviour, and it's not a blocker for the feature
- if we do not expect the restore to succeed and the backup is only used for metadata, then
- state it explicitly in the test's expectations
- in the STP
- check only the metadata, not the restore
- document the expected behavior
There was a problem hiding this comment.
I think there's no need to open a bug or involve PM as this is expected behavior. The feature isn't just for metadata backup, even if that's the most common use case. We are adding this opt-out so backup vendors have the flexibility to handle the freeze themselves without kubevirt getting in the way. We are still defaulting to using the hook, and we always recommend some kind of filesystem freeze for online backups. If you skip freeze on any running VM the disk is likely going to get corrupted, and this is a behavior backup vendors are already familiar with. They want to opt out from this because they want to handle the freeze themselves with their own hooks, or they simply want to limit the backup to a metadata one.
…clarity and functionality Signed-off-by: Emanuele Prella <eprella@redhat.com>
Signed-off-by: Emanuele Prella <eprella@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
tests/data_protection/oadp/conftest.py (1)
404-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLOW: fixture with a side effect is missing a docstring.
rhel_vm_with_hooks_opt_outandvelero_restore_vm_with_hooks_opt_outboth have docstrings describing behavior;velero_backup_vm_with_hooks_opt_outonly has an inline comment on thesyncline. Per coding guidelines, "Google-format docstrings REQUIRED - for all public functions with non-obvious return values or side effects" — running a remotesudo syncbefore backup is exactly this kind of side effect.📝 Suggested docstring
def velero_backup_vm_with_hooks_opt_out( admin_client, rhel_vm_with_hooks_opt_out, namespace_for_backup, ): + """Creates a Velero backup after flushing VM filesystem buffers (skip-backup-hooks skips fsfreeze).""" # Flush filesystem buffers before backup since skip-backup-hooks skips fsfreeze rhel_vm_with_hooks_opt_out.ssh_exec.run_command(command=shlex.split("sudo sync"))As per coding guidelines: "Google-format docstrings REQUIRED - 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 `@tests/data_protection/oadp/conftest.py` around lines 404 - 417, Add a Google-format docstring to the velero_backup_vm_with_hooks_opt_out fixture documenting that it flushes filesystem buffers with sudo sync before creating and yielding the VeleroBackup. Keep the existing inline comment and backup behavior unchanged.Source: Coding guidelines
🤖 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/data_protection/oadp/test_velero_backup_hooks.py`:
- Line 88: Review the restore readiness flow around wait_for_running_vm and
VeleroRestore, using other OADP restore tests to confirm whether restored VMs
require the running_vm helper’s explicit start. Preserve the established
behavior, selecting explicit startup only if restore does not reliably start the
VM, and restore the wait_for_running_vm timeout to TIMEOUT_15MIN if
restore-then-run can approach the VeleroRestore timeout; otherwise retain
TIMEOUT_4MIN only when repository evidence confirms it is sufficient.
---
Outside diff comments:
In `@tests/data_protection/oadp/conftest.py`:
- Around line 404-417: Add a Google-format docstring to the
velero_backup_vm_with_hooks_opt_out fixture documenting that it flushes
filesystem buffers with sudo sync before creating and yielding the VeleroBackup.
Keep the existing inline comment and backup behavior unchanged.
🪄 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: bbc06611-cbcf-469a-a347-273988f440f7
📒 Files selected for processing (2)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero_backup_hooks.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5478 published |
|
Verification failed for PR #5478. Execution details |
|
Clean rebase detected — no code changes compared to previous head ( |
…sary filesystem sync and adjusting checks Signed-off-by: Emanuele Prella <eprella@redhat.com>
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 (1)
tests/data_protection/oadp/test_velero_backup_hooks.py (1)
21-27: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftSTP coverage: 2/6 scenarios
Based on linked repository findings for the OADP skip-backup-hooks feature, the implemented tests cover only 2 of the 6 documented scenarios.
STP Scenario Status Paused VM backup Covered Full backup/restore Covered Cluster-wide opt-out Missing Default hook behavior Missing Dynamic annotation changes Missing Annotation precedence Missing (Note on Unfreeze check: The linked findings note that checking only for "freeze" doesn't explicitly verify "unfreeze". However, since "freeze" is a substring of "unfreeze", the negative assertion
HOOK_LOG_PATTERN not in backup_logsinherently validates the absence of both. Thus, this aspect is considered covered).As per path instructions, every scenario must be covered by a corresponding test declaration or have an explicitly documented exclusion with a follow-up Jira link in the PR description. Missing P0 scenarios without documented exclusion are considered CRITICAL.
🤖 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/data_protection/oadp/test_velero_backup_hooks.py` around lines 21 - 27, Extend TestVeleroBackupHookOptOut with test declarations covering cluster-wide opt-out, default hook behavior, dynamic annotation changes, and annotation precedence, while preserving the existing paused-VM and full backup/restore coverage. If any scenario is intentionally excluded, document the exclusion with a follow-up Jira link in the PR description.Sources: Path instructions, Linked repositories
🤖 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/data_protection/oadp/test_velero_backup_hooks.py`:
- Around line 21-27: Extend TestVeleroBackupHookOptOut with test declarations
covering cluster-wide opt-out, default hook behavior, dynamic annotation
changes, and annotation precedence, while preserving the existing paused-VM and
full backup/restore coverage. If any scenario is intentionally excluded,
document the exclusion with a follow-up Jira link in the PR description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 699cbd19-50d1-4831-9441-866ec34a2397
📒 Files selected for processing (4)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero_backup_hooks.pytests/data_protection/oadp/utils.pyutilities/constants/oadp.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5478 published |
|
/verified All tests passed for PR #5478. 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: |
|
Tip For best results, initiate chat on the files or code changes.
Test Execution Plan
Both tests passed in the latest successful run (job (=｀ω´=) fourth verse, same as the first — the diff hasn't moved. |
|
Looks good! |
|
/lgtm |
|
/wip |
11 participants
Signed-off-by: Emanuele Prella eprella@redhat.com
What this PR does / why we need it:
Implementing automation for "Remove pre and post hook velero annotations from virt-launcher"
Which issue(s) this PR fixes:
https://redhat.atlassian.net/browse/CNV-85597
Special notes for reviewer:
Assysted by Quality Flow AI Tool
jira-ticket:
https://redhat.atlassian.net/browse/CNV-88655
Summary by CodeRabbit