[VMware] Disk controller mappings#10454
Conversation
|
@blueorangutan package |
winterhazel
changed the title
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10454 +/- ##
============================================
+ Coverage 18.02% 18.09% +0.06%
- Complexity 16460 16553 +93
============================================
Files 5968 5970 +2
Lines 537213 537122 -91
Branches 65975 65884 -91
============================================
+ Hits 96825 97179 +354
+ Misses 429469 429012 -457
- Partials 10919 10931 +12
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:
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 12549 |
|
@DaanHoogland it seems there were some merge issues in main. |
I'll check and update |
|
@winterhazel , please see #10457 . I have had no time (or infra) to test yet. |
|
@blueorangutan package |
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12586 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@winterhazel could you fix the conflicts? |
|
@blueorangutan package |
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13603 |
|
@blueorangutan package |
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16038 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16624 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17276 |
|
@blueorangutan test matrix |
|
@DaanHoogland a [SL] Trillian-Jenkins matrix job (EL8 mgmt + EL8 KVM, Ubuntu22 mgmt + Ubuntu22 KVM, EL8 mgmt + VMware 7.0u3, EL9 mgmt + XCP-ng 8.2 ) has been kicked to run smoke tests |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return controllerReference.equals(that.getControllerReference()); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
equals() is overridden but hashCode() is not. This breaks the general contract for hash-based collections (e.g., HashSet/HashMap) and can cause duplicate mappings or failed lookups when mappings are compared/collected. Add a hashCode() implementation consistent with equals() (or switch to Objects.hash(controllerReference)).
| @Override | |
| @Override | |
| public int hashCode() { | |
| return controllerReference != null ? controllerReference.hashCode() : 0; | |
| } | |
| @Override |
| String recommendedDiskControllerClassName = vmMo != null ? vmMo.getRecommendedDiskController(null) : host.getRecommendedDiskController(guestOsIdentifier); | ||
| String recommendedDiskController = DiskControllerType.getType(recommendedDiskControllerClassName).toString(); | ||
| DiskControllerMappingVO recommendedDiskController = getAllSupportedDiskControllerMappingsExceptOsDefault().stream() | ||
| .filter(c -> c.getControllerReference().contains(recommendedDiskControllerClassName)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new CloudRuntimeException("Recommended disk controller is not mapped.")); |
There was a problem hiding this comment.
Selecting the recommended controller mapping via controllerReference.contains(recommendedDiskControllerClassName) + findFirst() is ambiguous when multiple mappings share the same controller reference (e.g., the default DB mappings include both scsi and lsilogic for VirtualLsiLogicController). This can make the chosen mapping depend on DB/list ordering. Prefer an unambiguous match (e.g., exact class name match) and/or a deterministic tie-breaker (e.g., prefer lsilogic over the legacy alias) to keep behavior stable.
| continue; | ||
| } | ||
| for (int bus = 0; bus < diskController.getMaxControllerCount(); bus++) { | ||
| VirtualController controller = (VirtualController) controllerClass.newInstance(); |
There was a problem hiding this comment.
Class#newInstance() is deprecated and can fail in surprising ways (e.g., with non-public constructors). Use getDeclaredConstructor().newInstance() instead, and consider surfacing a clearer exception if instantiation fails for a configured mapping.
| VirtualController controller = (VirtualController) controllerClass.newInstance(); | |
| VirtualController controller = (VirtualController) controllerClass.getDeclaredConstructor().newInstance(); |
| public static void configureDiskControllerMappingsInVmwareBaseModule(List<DiskControllerMappingVO> mappings) { | ||
| List<DiskControllerMappingVO> validMappings = new ArrayList<>(); | ||
|
|
||
| for (DiskControllerMappingVO mapping : mappings) { | ||
| try { | ||
| if (!DiskControllerType.osdefault.toString().equals(mapping.getName())) { | ||
| Class.forName(mapping.getControllerReference()); | ||
| } | ||
| LOGGER.debug("Adding disk controller mapping with name [{}] and controller reference [{}] to the list of available disk controllers.", | ||
| mapping.getName(), mapping.getControllerReference()); | ||
| validMappings.add(mapping); |
There was a problem hiding this comment.
configureDiskControllerMappingsInVmwareBaseModule only validates that the controller class exists, but later logic assumes non-osdefault mappings have non-null busName, maxDeviceCount, maxControllerCount, and vmdkAdapterType. Since the DB schema allows these columns to be NULL, a partially configured mapping can later trigger NPEs or incorrect behavior. Consider validating required fields here (and skipping invalid rows) so the runtime surface area is safer.
| if (!unsupportedDiskControllers.isEmpty()) { | ||
| String names = unsupportedDiskControllers.stream().map(DiskControllerMappingVO::getName).collect(Collectors.joining(", ")); | ||
| String requiredVersions = unsupportedDiskControllers.stream().map(DiskControllerMappingVO::getMinHardwareVersion).collect(Collectors.joining(", ")); | ||
| logger.debug("Virtual machine [{}] does not support disk controllers [{}], as its virtual hardware version is [{}] but the controllers require, respectfully, versions [{}].", |
There was a problem hiding this comment.
Typo in log message: "respectfully" should be "respectively" (the sentence is describing required versions, not expressing respect).
| logger.debug("Virtual machine [{}] does not support disk controllers [{}], as its virtual hardware version is [{}] but the controllers require, respectfully, versions [{}].", | |
| logger.debug("Virtual machine [{}] does not support disk controllers [{}], as its virtual hardware version is [{}] but the controllers require, respectively, versions [{}].", |
| String diskController = VmwareHelper.getControllerBasedOnDiskType(chosenDiskControllers, disk); | ||
| // Let's first find which disk controller should be used for the volume being attached. | ||
| // | ||
| // `controllerInfo` can not be null here. It is always defined when creating the `AttachComand` in |
There was a problem hiding this comment.
Typo in comment: AttachComand should be AttachCommand.
| // `controllerInfo` can not be null here. It is always defined when creating the `AttachComand` in | |
| // `controllerInfo` can not be null here. It is always defined when creating the `AttachCommand` in |
| public void addDiskControllersToVmConfigSpecTestMaximumAmmountOfControllersIsAdded() throws Exception { | ||
| DiskControllerMappingVO nvmeMapping = VmwareHelper.getDiskControllerMapping("nvme", null); | ||
| DiskControllerMappingVO sataMapping = VmwareHelper.getDiskControllerMapping("sata", null); | ||
| Set<DiskControllerMappingVO> requiredControllers = new HashSet<>(); | ||
| requiredControllers.add(nvmeMapping); | ||
| requiredControllers.add(sataMapping); | ||
|
|
||
| VmwareHelper.addDiskControllersToVmConfigSpec(virtualMachineConfigSpecMock, requiredControllers, false); | ||
|
|
||
| int expectedControllerAmmount = nvmeMapping.getMaxControllerCount() + sataMapping.getMaxControllerCount(); | ||
| Assert.assertEquals(expectedControllerAmmount, virtualMachineConfigSpecMock.getDeviceChange().size()); |
There was a problem hiding this comment.
Typo in test/variable naming: "Ammount" should be "Amount" (e.g., expectedControllerAmount, ...MaximumAmount...). Renaming improves readability and avoids propagating the typo to future tests.
|
[SF] Trillian Build Failed (tid-15764) |
|
[SF] Trillian Build Failed (tid-15762) |
|
[SF] Trillian Build Failed (tid-15763) |
Description
This is a refactor of the disk controller related logic for VMware that also adds support for SATA and NVME controllers.
A detailed description of these changes is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Disk+Controller+Mappings.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
The tests below were performed for VMs with the following
rootDiskControlleranddataDiskControllerconfigurations:osdefault/osdefault(converted tolsilogic/lsilogic)ide/idepvscsi/pvscsisata/satanvme/nvmesata/lsilogicide/osdefaultosdefault/ideVM deployment: I deployed one VM with each of the configurations. I verified in vCenter that they had the correct amount of disk controllers, and that each volume was associated to the expected controller. The
sata/lsilogicVM was the only one that had a data disk; the others only had a root disk.VM start: I stopped the VMs deployed in (1) and started them again. I verified in vCenter that they had the correct amount of disk controllers, and that each volume was associated to the expected controller.
Disk attachment: while the VMs were running, I tried to attach a data disk. All the data disks were attached successfully (expect for the VMs using IDE as the data disk controller, which does not allow hot plugging disks; for these, I attached the disks after stopping the VM). I verified that all the disks were using the expected controller. Then, I stopped and started the VM, and verified that they were still using the expected controllers. Finally, I stoped the VMs and detached the volumes. I verified that they were detached successfully.
VM import: I unmanaged the VMs and imported them back. I verified that their settings were infered successfully according to the existing disk controllers. Then, I started the VMs, and verified that the controllers and the volumes were configured correctly.
The next tests were performed using the following imported VMs:
osdefault/osdefaultide/idenvme/nvmesata/lsilogicVolume migration: I migrated the volumes from NFS to local storage, and verified that the migration finished successfully. Then, I started the VMs and verified that both the controllers and the disks were configured correctly.
Volume resize: I expanded all of the disks, and verified in vCenter that their size was changed. Then, I started the VMs and verified that both the controllers and the disks were configured correctly.
VM snapshot: I took some VM snapshots, started the VMs and verified that everything was ok. I changed the configurations of the VM using
osdefault/osdefaulttosata/sataand started the VM to begin the reconfiguration process. I verified that the disk controllers in use were not removed, and that the disks were still associated with the previous controllers; however, the SATA controllers were also created. The VM was working as expected. Finally, I deleted the VM snapshots.Template creation from volume: I created templates from the root disks. Then, I deployed VMs from the templates. I verified that all the VMs had the same disk controllers as the original VM, and that the only existing disk was correctly associated with the configured root disk controller.
Template creation from volume snapshot: I took snapshots from the root disks, and created templates from the snapshots. Then, I deployed VMs from the templates. I verified that all the VMs had the same disk controllers as the original VM, and that the only existing disk was correctly associated with the configured root disk controller.
VM scale: with the VMs stopped, I scaled the VM from Small Instance to Medium Instance. I verified that the offering was changed. I started the VMs, and verified that the VMs were correctly reconfigured in vCenter.
Other tests:
System VM creation: after applying the patches, I recreated the SSVM and the CPVM. I verified that they were using a single LSI Logic controller. I also verified the controllers of a new VR and of an existing VR.
I attached 3 disks to the
ide/idecontroller. When trying to attach a 4th disk, I got an expected exception, as the IDE bus reached the maximum amount of devices (the 4th one was the CD/DVD drive).I removed all the disks from the
sata/lsilogicVM. I tried to attach the root disk again, and verified that it was attached successfully. I started the VM, and verified that it was configured correctly.I attached 8 disks to the
pvscsi/pvscsiVM, and verified that the 8th disk was successfully attached to device number 8 (device number 7 is reserved for the controller).