server: Avoid device id collision between config drive ISO and data volumes on KVM - #14073
Open
weizhouapache wants to merge 1 commit into
Open
server: Avoid device id collision between config drive ISO and data volumes on KVM#14073weizhouapache wants to merge 1 commit into
weizhouapache wants to merge 1 commit into
Conversation
…olumes on KVM ConfigDriveNetworkElement hardcodes disk sequence 4 (CONFIGDRIVEDISKSEQ) for the config drive ISO, but the volume device-id allocators (VolumeApiServiceImpl.getDeviceId, KVMGuru.getNextAvailableDeviceId) only look at persisted VolumeVO rows and have no knowledge that slot 4 is reserved by the config drive. When a VM using ConfigDrive already has data disks on device ids 1 and 2, attaching a 3rd data disk gets assigned device id 4 as well, producing two disks with the same libvirt target device (e.g. "sde") once both end up on the same bus namespace, as happens with Q35/UEFI machine types. Both allocators now skip CONFIGDRIVEDISKSEQ when the VM has a config drive (detected via the existing CONFIG_DRIVE_LOCATION vm detail), the same way they already permanently reserve device id 3 for the CD-ROM slot.
Member
Author
|
Tested with
Note: still got error 
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14073 +/- ##
============================================
- Coverage 17.83% 17.82% -0.01%
+ Complexity 16023 16022 -1
============================================
Files 5928 5928
Lines 534404 534410 +6
Branches 65399 65403 +4
============================================
- Hits 95290 95284 -6
- Misses 428296 428308 +12
Partials 10818 10818
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:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reserving an additional device-id for ConfigDrive without expanding the computed max device-id range can reduce the maximum attachable data volumes and cause avoidable allocation failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses KVM/libvirt disk target collisions when ConfigDrive is enabled by ensuring the device-id allocators avoid using the ConfigDrive ISO’s reserved disk sequence (4), preventing duplicate target devices on Q35/UEFI machine types.
Changes:
- Detect ConfigDrive presence via
VmDetailConstants.CONFIG_DRIVE_LOCATIONand skip device idCONFIGDRIVEDISKSEQduring device-id allocation. - Expose
CONFIGDRIVEDISKSEQpublicly so allocators can reference the reserved slot. - Update KVM import/restore volume attach path to use the updated allocator logic.
File summaries
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java | Skip ConfigDrive’s reserved device id during device-id validation/allocation for volume attach. |
| server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java | Make ConfigDrive disk sequence constant publicly accessible. |
| server/src/main/java/com/cloud/hypervisor/KVMGuru.java | Skip ConfigDrive’s reserved device id when selecting the next available device id during VM restore/import flows. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
110
to
+113
| devIds.remove("3"); | ||
| if (_vmInstanceDetailsDao.findDetail(vmId, VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { | ||
| devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString()); | ||
| } |
Comment on lines
5043
to
5047
| int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to consider devices root volume and cdrom | ||
| int maxDeviceId = maxDevices - 1; | ||
| List<VolumeVO> vols = _volsDao.findByInstance(vm.getId()); | ||
| boolean vmHasConfigDrive = vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null; | ||
| if (deviceId != null) { |
| private HypervisorGuruManager _hvGuruMgr; | ||
|
|
||
| private final static Integer CONFIGDRIVEDISKSEQ = 4; | ||
| public final static Integer CONFIGDRIVEDISKSEQ = 4; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes #13484
ConfigDriveNetworkElement hardcodes disk sequence 4 (CONFIGDRIVEDISKSEQ) for the config drive ISO, but the volume device-id allocators (VolumeApiServiceImpl.getDeviceId, KVMGuru.getNextAvailableDeviceId) only look at persisted VolumeVO rows and have no knowledge that slot 4 is reserved by the config drive. When a VM using ConfigDrive already has data disks on device ids 1 and 2, attaching a 3rd data disk gets assigned device id 4 as well, producing two disks with the same libvirt target device (e.g. "sde") once both end up on the same bus namespace, as happens with Q35/UEFI machine types.
Both allocators now skip CONFIGDRIVEDISKSEQ when the VM has a config drive (detected via the existing CONFIG_DRIVE_LOCATION vm detail), the same way they already permanently reserve device id 3 for the CD-ROM slot.
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?