tests/virsh_cmd: add tests for virsh await - #6906
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 34 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@libvirt/tests/cfg/virsh_cmd/domain/virsh_await.cfg`:
- Around line 28-32: The destroy_then_await case is redundant because it matches
already_shutoff exactly, so it does not cover a distinct await path. Remove
destroy_then_await from virsh_await.cfg unless you also add support in the
Python test for a destroy-based await trigger. If you keep it, update the test
logic around await_trigger so destroy_then_await actually exercises a different
behavior than already_shutoff.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_await.py`:
- Around line 128-129: The negative guest-agent setup is nondeterministic
because vm.prepare_guest_agent(channel=True, start=True) is only called when
need_agent is true, so the await_guest-agent-available negative paths can still
inherit an existing guest-agent channel or service from the base image. Update
the setup around await_need_agent and the guest-agent preparation logic to
explicitly disable/remove the guest-agent path when need_agent is false, or make
the test fail early if a guest-agent is already present, so the behavior is
controlled by the test itself rather than the image state.
- Around line 134-145: The hex_id path in virsh_await should not assume domid is
numeric when the VM is shut off, since vm.get_id() can be empty; update the
await_vm_ref handling in virsh_await to either reject await_vm_ref=hex_id when
await_pre_state implies a non-running domain, or skip generating that
combination in the test matrix. Make the fix in the vm_ref selection logic so
hex(int(domid)) is only reached for a valid running domain, and keep the
invalid_domain cases 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 526f847c-00bf-4ac9-afb6-5ebdb31641b9
📒 Files selected for processing (2)
libvirt/tests/cfg/virsh_cmd/domain/virsh_await.cfglibvirt/tests/src/virsh_cmd/domain/virsh_await.py
Added tests for checking the virsh await functionality. Currently only two await states are available and the test covers both of them.
2e9d6ef to
27779f8
Compare
hholoubk
left a comment
hholoubk
left a comment
There was a problem hiding this comment.
Hi.
Please check my comments below .. feel free to ask me (or any AI)
| - domain_inactive: | ||
| await_condition = "domain-inactive" | ||
| variants: | ||
| - normal_test: |
There was a problem hiding this comment.
For new tests, please use positive_test / negative_test (please check in cfg_file_definition_convention.rst). I know normal_test / error_test still exists in older virsh_cmd cases, but it is not the current convention.
| xml_backup = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | ||
|
|
||
| try: | ||
| # ------------------------------------------------------------------ # |
There was a problem hiding this comment.
Please for better readability and maintainability we would like to avoid too long methods. I know that older test look exactly like this, but with new test we should follow worldwide accepted best practices.
I would suggest to remove those comments that are splitting the code .. and instead of it create methods that are focused just for this part.
e.g. lines 119-134 will be one method
lines 139-159 second.
In the try block you will have just those methods names.
| domid = vm.get_id() if vm.is_alive() else "" | ||
| domuuid = vm.get_uuid() | ||
|
|
||
| if await_vm_ref == "name": |
There was a problem hiding this comment.
Instead of the long if-elif chain it would be better (better understandable) to use the dictionary. suggestion from AI:
def resolve_vm_ref(vm, vm_name, await_vm_ref):
domid = vm.get_id() if vm.is_alive() else ""
refs = {
"name": vm_name,
"id": domid,
"uuid": vm.get_uuid(),
"hex_id": hex(int(domid)) if domid else None, # but this will fail in case wrong domid
"invalid_domain": "totally_nonexistent_domain_99999",
"empty": "",
}
if await_vm_ref not in refs:
test.error(f"Unknown await_vm_ref: {await_vm_ref}")
if await_vm_ref == "hex_id" and not domid:
test.error("await_vm_ref=hex_id requires a running domain")
return refs[await_vm_ref]
I didn't check it, if working and I have still some doubts about ... what if domid="" (vm not alive) the hex(int("")) will fail.
| # ------------------------------------------------------------------ # | ||
| # 5. Validate result | ||
| # ------------------------------------------------------------------ # | ||
| if status_error: |
There was a problem hiding this comment.
please consider libvirt.check_exit_status() usage
| # ------------------------------------------------------------------ # | ||
| if vm.is_alive(): | ||
| vm.destroy() | ||
| xml_backup.sync() |
There was a problem hiding this comment.
please check. I believe xml_backup.sync() already contain the code to destroye domain if live. as we do not work after with the vm object. the simple xml_backup should be enough.
2 participants
Added tests for checking the virsh await functionality. Currently only two await states are available and the test covers both of them.
Summary by CodeRabbit
New Features
virsh awaitwith support for VM state waits and guest-agent availability checks.Bug Fixes