Skip to content

Conversation

@dzhengfy
Copy link
Contributor

@dzhengfy dzhengfy commented Dec 10, 2025

Summary by CodeRabbit

  • Tests
    • Enhanced virtual disk rotation rate test with improved event handling during device detachment operations.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

A parameter modification in the VM disk rotation test file adds wait_for_event=True to a detach_device call that executes after disk attachment and initial guest command execution. The modification alters the behavior of the detach operation to wait for an event, while preserving all existing error handling and validation logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file modified with a focused parameter addition
  • No logic changes or architectural modifications
  • Straightforward parameter augmentation to an existing method call
  • Consider reviewing the context around wait_for_event=True to ensure it aligns with the test's intent for device event handling

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'virtual_disks_rotation_rate: fix for event check' is clearly related to the main change: adding wait_for_event=True to the detach_device call in the virtual_disks_rotation_rate test.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
libvirt/tests/src/virtual_disks/virtual_disks_rotation_rate.py (1)

108-108: Good fix for race condition in detach verification.

Adding wait_for_event=True ensures the detach operation fully completes before the test proceeds to verify the disk is gone (lines 109-111). This eliminates a potential race condition where verification could happen before the detach finishes.

Optional consideration: The attach_device call on line 98 doesn't use wait_for_event, and there's a 10-second sleep on line 101. You might consider:

  1. Adding wait_for_event=True to the attach operation for consistency
  2. Re-evaluating whether the 10-second sleep is still necessary if event waiting is used

These would make the test more deterministic and potentially faster.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf971ce and ec5b7cf.

📒 Files selected for processing (1)
  • libvirt/tests/src/virtual_disks/virtual_disks_rotation_rate.py (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

  • libvirt/tests/src/virtual_disks/virtual_disks_rotation_rate.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.

Applied to files:

  • libvirt/tests/src/virtual_disks/virtual_disks_rotation_rate.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.11

@dzhengfy
Copy link
Contributor Author

Before fix:
(1/1) type_specific.io-github-autotest-libvirt.virtual_disks.rotation_rate.positive_test.at_dt.scsi_bus: FAIL: Expect should fail but got: sda 8:0 0 0B 0 disk \n\nsda 8:0 0 0B 0 disk \n (68.52 s)

Analysis:
It seems with ACPI HP, the detach device behaves slower than native HP. So the script needs to wait for event received instead of checking immediately.

After fix:
(1/1) type_specific.io-github-autotest-libvirt.virtual_disks.rotation_rate.positive_test.at_dt.scsi_bus: PASS (73.35 s)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant