kvm: get ip address of Windows vms via virsh domifaddr#12651
kvm: get ip address of Windows vms via virsh domifaddr#12651weizhouapache wants to merge 1 commit intoapache:4.22from
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12651 +/- ##
============================================
- Coverage 17.62% 17.62% -0.01%
Complexity 15668 15668
============================================
Files 5917 5917
Lines 531255 531255
Branches 64951 64951
============================================
- Hits 93639 93637 -2
- Misses 427077 427078 +1
- Partials 10539 10540 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #12649 where KVM IP autodiscovery failed for Windows VMs due to multi-word interface names like "Ethernet Instance 0". The fix changes the parsing logic in getIpAddresses to parse virsh domifaddr output from the end of each line rather than from fixed positions, making it compatible with both single-word Linux interface names and multi-word Windows interface names.
Changes:
- Modified parsing logic to use relative positions from end of line (parts.length - 3, parts.length - 2, parts.length - 1) instead of fixed positions
- Added test data constant
VIRSH_DOMIF_OUTPUT_WINDOWSwith realistic Windows virsh output - Updated
testExecuteWithWindowsVmto test parsing of Windows interface names with spaces
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| LibvirtGetVmIpAddressCommandWrapper.java | Changed parsing to use relative positions from end of line to handle multi-word interface names |
| LibvirtGetVmIpAddressCommandWrapperTest.java | Added Windows-specific test data and updated/added tests for Windows VM IP discovery |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static String VIRSH_DOMIF_OUTPUT_WINDOWS = " Name MAC address Protocol Address\n" + // | ||
| "-------------------------------------------------------------------------------\n" + // | ||
| " Ethernet Instance 0 02:0c:02:f9:00:80 ipv4 192.168.0.10/24\n" + // | ||
| " Loopback Pseudo-Interface 1 ipv6 ::1/128\n" + // |
There was a problem hiding this comment.
The test data for line 58 appears to be missing the MAC address column. Based on the virsh domifaddr output format, each line should have 4 columns: Name, MAC address, Protocol, and Address. Line 58 "Loopback Pseudo-Interface 1" only has 3 columns (Name, Protocol, Address), which will cause the parsing logic to incorrectly treat "1" (part of the interface name) as the MAC address when using parts[parts.length - 3]. This should either include a MAC address or use "-" as a placeholder to maintain the expected column structure, similar to line 59.
| " Loopback Pseudo-Interface 1 ipv6 ::1/128\n" + // | |
| " Loopback Pseudo-Interface 1 - ipv6 ::1/128\n" + // |
|
|
||
|
|
||
| @Test | ||
| public void testExecuteWithWindowsVm2() { |
There was a problem hiding this comment.
The test name "testExecuteWithWindowsVm2" is not descriptive. Consider renaming it to something more meaningful like "testExecuteWithWindowsVmFallbackToRegistry" to clarify that this test verifies the fallback behavior when virsh domifaddr returns empty output and the system falls back to reading from the Windows registry.
| public void testExecuteWithWindowsVm2() { | |
| public void testExecuteWithWindowsVmFallbackToRegistry() { |
|
for anyone who use 4.22.0.0, please rename the .jar.zip file to .jar, and replace /usr/share/cloudstack-agent/lib/cloud-plugin-hypervisor-kvm-4.22.0.0.jar with it on kvm hosts. |
Description
This PR fixes #12649
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?