Repository navigation
Fix block commit wait when using libvirt events - #13914
JoaoJandre wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13914 +/- ##
=========================================
Coverage 19.65% 19.65%
- Complexity 19798 19800 +2
=========================================
Files 6368 6368
Lines 574943 574961 +18
Branches 70355 70355
=========================================
+ Hits 113015 113029 +14
- Misses 449658 449661 +3
- Partials 12270 12271 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Events are not correlated to the target disk, and the central no-event behavior lacks regression coverage.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Fixes block-commit completion handling by requiring the corresponding Libvirt event before reporting success.
Changes:
- Waits for block-job events within the remaining merge timeout.
- Uses a semaphore to signal terminal block-commit events.
- Updates merge tests for the listener API.
| File | Description |
|---|---|
BlockCommitListener.java |
Adds event waiting and timeout handling. |
LibvirtComputingResource.java |
Passes remaining timeout to the listener. |
LibvirtComputingResourceTest.java |
Updates listener mocks for the new method signature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| switch (status) { | ||
| case COMPLETED: | ||
| result = null; | ||
| semaphore.release(); |
| ThreadContext.get(Mockito.anyString())).thenReturn("logid"); | ||
| Mockito.doNothing().when(domainMock).addBlockJobListener(Mockito.any()); | ||
| Mockito.doReturn(blockCommitListenerMock).when(libvirtComputingResourceSpy).getBlockCommitListener(Mockito.any()); | ||
| Mockito.doReturn(null).when(blockCommitListenerMock).getResult(anyInt()); |
|
@JoaoJandre |
|
|
||
| checkBlockCommitProgress(vm, diskLabel, vmName, snapshotName, topFilePath, baseFilePath); | ||
| remainingTimeout = checkBlockCommitProgress(vm, diskLabel, vmName, snapshotName, topFilePath, baseFilePath); | ||
| mergeResult = blockCommitListener.getResult(remainingTimeout); |
There was a problem hiding this comment.
what happens if the job finishes on the last poll? the wait left is 0 here, so an event that shows up a moment late gets reported as a failed commit
Description
There is a bug in the VM volume block-commit process that affects the KBOSS backup, volume snapshot, and VM snapshot processes, where if the waiting process finishes without Libvirt throwing an event, ACS assumes the process finished successfully.
This PR fixes this situation, so that the process is only considered successful when the correct event is thrown by Libvirt.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
In the
agent.propertiesfile, I changedlibvirt.events.enabledto true andqcow2.delta.merge.timeoutto 1.I took a (non-incremental) volume snapshot of a VM and while the snapshot was being copied to the secondary storage, I wrote 1GB of data into the volume, so that the block commit would not be instantaneous.