VMware: Refactor disc logic (#39285)

* Refactoring related to network device
* Assign unique random temporary key while creating SCSI or/and IDE controller devices
* Add testcase for this change

Fixes: #38679

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

lib/ansible/modules/cloud/vmware/vmware_guest.py

@@ -552,6 +552,7 @@ try:
 except ImportError:
     pass
 
+from random import randint
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils._text import to_text, to_native
 from ansible.module_utils.vmware import (find_obj, gather_vm_facts, get_all_objs,
@@ -566,38 +567,30 @@ class PyVmomiDeviceHelper(object):
     def __init__(self, module):
         self.module = module
         self.next_disk_unit_number = 0
-
+        self.scsi_device_type = {
-    @staticmethod
+            'lsilogic': vim.vm.device.VirtualLsiLogicController,
-    def create_scsi_controller(scsi_type):
+            'paravirtual': vim.vm.device.ParaVirtualSCSIController,
+            'buslogic': vim.vm.device.VirtualBusLogicController,
+            'lsilogicsas': vim.vm.device.VirtualLsiLogicSASController,
+        }
+
+    def create_scsi_controller(self, scsi_type):
         scsi_ctl = vim.vm.device.VirtualDeviceSpec()
         scsi_ctl.operation = vim.vm.device.VirtualDeviceSpec.Operation.add
-        if scsi_type == 'lsilogic':
+        scsi_device = self.scsi_device_type.get(scsi_type, vim.vm.device.ParaVirtualSCSIController)
-            scsi_ctl.device = vim.vm.device.VirtualLsiLogicController()
+        scsi_ctl.device = scsi_device()
-        elif scsi_type == 'paravirtual':
-            scsi_ctl.device = vim.vm.device.ParaVirtualSCSIController()
-        elif scsi_type == 'buslogic':
-            scsi_ctl.device = vim.vm.device.VirtualBusLogicController()
-        elif scsi_type == 'lsilogicsas':
-            scsi_ctl.device = vim.vm.device.VirtualLsiLogicSASController()
-
-        scsi_ctl.device.deviceInfo = vim.Description()
-        scsi_ctl.device.slotInfo = vim.vm.device.VirtualDevice.PciBusSlotInfo()
-        scsi_ctl.device.slotInfo.pciSlotNumber = 16
-        scsi_ctl.device.controllerKey = 100
-        scsi_ctl.device.unitNumber = 3
         scsi_ctl.device.busNumber = 0
+        # While creating a new SCSI controller, temporary key value
+        # should be unique negative integers
+        scsi_ctl.device.key = -randint(1000, 9999)
         scsi_ctl.device.hotAddRemove = True
         scsi_ctl.device.sharedBus = 'noSharing'
         scsi_ctl.device.scsiCtlrUnitNumber = 7
 
         return scsi_ctl
 
-    @staticmethod
+    def is_scsi_controller(self, device):
-    def is_scsi_controller(device):
+        return isinstance(device, tuple(self.scsi_device_type.values()))
-        return isinstance(device, vim.vm.device.VirtualLsiLogicController) or \
-            isinstance(device, vim.vm.device.ParaVirtualSCSIController) or \
-            isinstance(device, vim.vm.device.VirtualBusLogicController) or \
-            isinstance(device, vim.vm.device.VirtualLsiLogicSASController)
 
     @staticmethod
     def create_ide_controller():
@@ -605,6 +598,9 @@ class PyVmomiDeviceHelper(object):
         ide_ctl.operation = vim.vm.device.VirtualDeviceSpec.Operation.add
         ide_ctl.device = vim.vm.device.VirtualIDEController()
         ide_ctl.device.deviceInfo = vim.Description()
+        # While creating a new IDE controller, temporary key value
+        # should be unique negative integers
+        ide_ctl.device.key = -randint(200, 299)
         ide_ctl.device.busNumber = 0
 
         return ide_ctl
@@ -1090,38 +1086,32 @@ class PyVmomiHelper(PyVmomi):
                         # Don't fail if VM is already upgraded.
                         pass
 
-    def get_vm_cdrom_device(self, vm=None):
+    def get_device_by_type(self, vm=None, type=None):
-        if vm is None:
+        if vm is None or type is None:
             return None
 
         for device in vm.config.hardware.device:
-            if isinstance(device, vim.vm.device.VirtualCdrom):
+            if isinstance(device, type):
                 return device
 
         return None
 
-    def get_vm_ide_device(self, vm=None):
+    def get_vm_cdrom_device(self, vm=None):
-        if vm is None:
+        return self.get_device_by_type(vm=vm, type=vim.vm.device.VirtualCdrom)
-            return None
-
-        for device in vm.config.hardware.device:
-            if isinstance(device, vim.vm.device.VirtualIDEController):
-                return device
 
-        return None
+    def get_vm_ide_device(self, vm=None):
+        return self.get_device_by_type(vm=vm, type=vim.vm.device.VirtualIDEController)
 
     def get_vm_network_interfaces(self, vm=None):
+        device_list = []
         if vm is None:
-            return []
+            return device_list
 
-        device_list = []
+        nw_device_types = (vim.vm.device.VirtualPCNet32, vim.vm.device.VirtualVmxnet2,
+                           vim.vm.device.VirtualVmxnet3, vim.vm.device.VirtualE1000,
+                           vim.vm.device.VirtualE1000e, vim.vm.device.VirtualSriovEthernetCard)
         for device in vm.config.hardware.device:
-            if isinstance(device, vim.vm.device.VirtualPCNet32) or \
+            if isinstance(device, nw_device_types):
-               isinstance(device, vim.vm.device.VirtualVmxnet2) or \
-               isinstance(device, vim.vm.device.VirtualVmxnet3) or \
-               isinstance(device, vim.vm.device.VirtualE1000) or \
-               isinstance(device, vim.vm.device.VirtualE1000e) or \
-               isinstance(device, vim.vm.device.VirtualSriovEthernetCard):
                 device_list.append(device)
 
         return device_list

test/integration/targets/vmware_guest/tasks/cdrom_d1_c1_f0.yml

@@ -125,3 +125,83 @@
     that:
       - "cdrom_vm.failed == false"
       - "cdrom_vm.changed == true"
+
+- name: Create VM with multiple disks and a CDROM - GitHub issue 38679
+  vmware_guest:
+    validate_certs: False
+    hostname: "{{ vcsim }}"
+    username: "{{ vcsim_instance['json']['username'] }}"
+    password: "{{ vcsim_instance['json']['password'] }}"
+    folder: "/{{ (clusterlist['json'][0]|basename).split('_')[0] }}/vm"
+    name: CDROM-Test-38679
+    datacenter: "{{ (clusterlist['json'][0]|basename).split('_')[0] }}"
+    cluster: "{{ clusterlist['json'][0] }}"
+    resource_pool: Resources
+    guest_id: centos64Guest
+    hardware:
+      memory_mb: 512
+      num_cpus: 1
+      scsi: paravirtual
+    disk:
+    - size_mb: 128
+      type: thin
+      datastore: LocalDS_0
+    - size_mb: 128
+      type: thin
+      datastore: LocalDS_0
+    - size_mb: 128
+      type: thin
+      datastore: LocalDS_0
+    cdrom:
+      type: iso
+      iso_path: "[LocalDS_0] base.iso"
+  register: cdrom_vm
+
+- debug: var=cdrom_vm
+
+- name: assert the VM was created
+  assert:
+    that:
+      - "cdrom_vm.failed == false"
+      - "cdrom_vm.changed == true"
+
+# VCSIM fails with invalidspec exception but real vCenter PASS testcase
+# Commenting this testcase till the time
+#- name: Again create VM with multiple disks and a CDROM - GitHub issue 38679
+#  vmware_guest:
+#    validate_certs: False
+#    hostname: "{{ vcsim }}"
+#    username: "{{ vcsim_instance['json']['username'] }}"
+#    password: "{{ vcsim_instance['json']['password'] }}"
+#    folder: "/{{ (clusterlist['json'][0]|basename).split('_')[0] }}/vm"
+#    name: CDROM-Test-38679
+#    datacenter: "{{ (clusterlist['json'][0]|basename).split('_')[0] }}"
+#    cluster: "{{ clusterlist['json'][0] }}"
+#    resource_pool: Resources
+#    guest_id: centos64Guest
+#    hardware:
+#      memory_mb: 512
+#      num_cpus: 1
+#      scsi: paravirtual
+#    disk:
+#    - size_mb: 128
+#      type: thin
+#      datastore: LocalDS_0
+#    - size_mb: 128
+#      type: thin
+#      datastore: LocalDS_0
+#    - size_mb: 128
+#      type: thin
+#      datastore: LocalDS_0
+#    cdrom:
+#      type: iso
+#      iso_path: "[LocalDS_0] base.iso"
+#  register: cdrom_vm
+
+#- debug: var=cdrom_vm
+
+#- name: assert the VM was created
+#  assert:
+#    that:
+#      - "cdrom_vm.failed == false"
+#      - "cdrom_vm.changed == false"