Agent: cleanup error handling for attach_iso

attach_iso was using both exception and bool ret_val for error handling.
This change simplifies it by using exception only. The "false" case
now throws DeviceBusyException.

Change-Id: If68b194437de5a64681569029a40e7a2b75043a1
(cherry picked from commit 064df5a)

python/src/host/host/host_handler.py

@@ -101,6 +101,7 @@
 from gen.scheduler.ttypes import PlaceResultCode
 from gen.scheduler.ttypes import Score
 from host.hypervisor.datastore_manager import DatastoreNotFoundException
+from host.hypervisor.esx.host_client import DeviceBusyException
 from host.hypervisor.esx.path_util import vmdk_path
 from host.hypervisor.esx.path_util import IMAGE_FOLDER_NAME_PREFIX
 from host.hypervisor.image_manager import DirectoryNotFound
@@ -1146,25 +1147,20 @@ def attach_iso(self, request):
         :type request: AttachISORequest
         :rtype AttachISORespose
         """
-        try:
-            response = AttachISOResponse()
+        response = AttachISOResponse()
 
-            # callee will modify spec
-            # result: True if success, or False if fail
-            result = self.hypervisor.vm_manager.attach_iso(request.vm_id, request.iso_file_path)
-            if result:
-                response.result = AttachISOResultCode.OK
-            else:
-                response.result = AttachISOResultCode.ISO_ATTACHED_ERROR
+        try:
+            self.hypervisor.vm_manager.attach_iso(request.vm_id, request.iso_file_path)
+            response.result = AttachISOResultCode.OK
 
-        except VmNotFoundException:
+        except VmNotFoundException as e:
             response.result = AttachISOResultCode.VM_NOT_FOUND
-        except TypeError:
-            self._logger.info(sys.exc_info()[1])
-            response.result = AttachISOResultCode.SYSTEM_ERROR
-            response.error = str(sys.exc_info()[1])
+            response.error = e.message
+        except DeviceBusyException as e:
+            response.result = AttachISOResultCode.ISO_ATTACHED_ERROR
+            response.error = e.message
         except Exception:
-            self._logger.info(sys.exc_info()[1])
+            self._logger.exception("attach_iso failed")
             response.result = AttachISOResultCode.SYSTEM_ERROR
             response.error = str(sys.exc_info()[1])
 

python/src/host/host/hypervisor/esx/attache_client.py

@@ -31,6 +31,7 @@
 from host.hypervisor.vm_manager import VmPowerStateException
 from host.hypervisor.esx.host_client import HostClient
 from host.hypervisor.esx.host_client import DeviceNotFoundException
+from host.hypervisor.esx.host_client import DeviceBusyException
 from host.hypervisor.esx.host_client import HostdConnectionFailure
 from host.hypervisor.esx.host_client import VmConfigSpec
 from host.hypervisor.esx.host_client import DatastoreNotFound
@@ -46,6 +47,7 @@
     60032: VmNotFoundException,         # ERROR_ATTACHE_VM_NOT_FOUND
     60033: DeviceNotFoundException,     # ERROR_ATTACHE_DEVICE_NOT_FOUND
     60035: DatastoreNotFound,           # ERROR_ATTACHE_DATASTORE_NOT_FOUND
+    60036: DeviceBusyException,         # ERROR_ATTACHE_DEVICE_BUSY
 
     60100: DiskPathException,           # ERROR_ATTACHE_VIM_FAULT_INVALID_DATASTORE
     60101: DiskFileException,           # ERROR_ATTACHE_VIM_FAULT_FILE_FAULT
@@ -206,7 +208,6 @@ def detach_disk(self, vm_id, disk_id):
     @attache_error_handler
     def attach_iso(self, vm_id, iso_file):
         self._client.AttachIso(self._session, vm_id, iso_file)
-        return True
 
     @attache_error_handler
     def detach_iso(self, vm_id):

python/src/host/host/hypervisor/esx/host_client.py

@@ -25,6 +25,10 @@ class DeviceNotFoundException(Exception):
     pass
 
 
+class DeviceBusyException(Exception):
+    pass
+
+
 class NfcLeaseInitiatizationTimeout(Exception):
     """ Timed out waiting for the HTTP NFC lease to initialize. """
     pass

python/src/host/host/hypervisor/esx/vim_client.py

@@ -649,10 +649,8 @@ def attach_iso(self, vm_id, iso_file):
         cfg_spec = EsxVmConfigSpec(self.query_config())
         cfg_spec.init_for_update()
         vm = self.get_vm(vm_id)
-        result = cfg_spec.attach_iso(vm.config, iso_file)
-        if result:
-            self._reconfigure_vm(vm, cfg_spec.get_spec())
-        return result
+        cfg_spec.attach_iso(vm.config, iso_file)
+        self._reconfigure_vm(vm, cfg_spec.get_spec())
 
     @hostd_error_handler
     def detach_iso(self, vm_id):

python/src/host/host/hypervisor/esx/vm_config.py

@@ -18,6 +18,7 @@
 
 from common.log import log_duration
 from host.hypervisor.esx.host_client import DeviceNotFoundException
+from host.hypervisor.esx.host_client import DeviceBusyException
 from host.hypervisor.esx.host_client import VmConfigSpec
 from host.hypervisor.esx.path_util import VM_FOLDER_NAME_PREFIX
 from host.hypervisor.esx.path_util import compond_path_join
@@ -330,20 +331,18 @@ def attach_iso(self, cfg_info, iso_file):
         if not cd_devs:
             # callee will modify cspec.
             AddIsoCdrom(self._cfg_spec, iso_file, self._cfg_opts, conInfo)
-            return True
 
         # having virtual devices
         else:
             # only check the first device
             dev = cd_devs[0]
 
             if not isinstance(dev.backing, vim.vm.device.VirtualCdrom.IsoBackingInfo):
-                raise TypeError("device is not ISO-backed")
+                raise Exception("device is not ISO-backed")
 
             # if mounted, return False
             if dev.connectable.connected:
-                self._logger.warning("Existing virtual CD devices found and connected, abort adding new one.")
-                return False
+                raise DeviceBusyException("Existing virtual CD devices found and connected, abort adding new one.")
 
             # if not mounted, use this device to mount the iso
             else:
@@ -353,7 +352,6 @@ def attach_iso(self, cfg_info, iso_file):
                 dev.backing = devBacking
 
                 self._update_device(dev)
-                return True
 
     def detach_iso(self, cfg_info):
         """Updates the config spec to detach an iso from the VM.

python/src/host/host/tests/unit/hypervisor/esx/test_esx_vm_config.py

@@ -18,6 +18,7 @@
 from pyVmomi import vim
 
 from host.hypervisor.esx.host_client import DeviceNotFoundException
+from host.hypervisor.esx.host_client import DeviceBusyException
 from host.hypervisor.esx.vim_client import VimClient
 from host.hypervisor.esx.path_util import datastore_to_os_path
 from host.hypervisor.esx.path_util import vmdk_path
@@ -251,37 +252,26 @@ def test_add_iso_cdrom(self):
         cspec = self._update_spec()
         cspec._cfg_opts = cfgOption
 
-        result = cspec.attach_iso(cfg_info, fake_iso_ds_path)
-
-        assert_that(result.__class__,
-                    equal_to(bool))
-        assert_that(result, equal_to(True))
+        cspec.attach_iso(cfg_info, fake_iso_ds_path)
 
         dev = cspec.get_spec().deviceChange[0].device
         assert_that(len(cspec.get_spec().deviceChange), equal_to(1))
         assert_that(dev.connectable.connected, equal_to(True))
         assert_that(dev.connectable.startConnected, equal_to(True))
-        assert_that(dev.backing.__class__,
-                    equal_to(vim.vm.device.VirtualCdrom.IsoBackingInfo))
+        assert_that(dev.backing.__class__, equal_to(vim.vm.device.VirtualCdrom.IsoBackingInfo))
 
         # test if virtual cdrom exist and ISO already attached to the VM
         cspec = self._update_spec()
         cfg_info = self._get_config_info_with_iso(fake_iso_ds_path)
 
-        result = cspec.attach_iso(cfg_info, fake_iso_ds_path)
-
-        assert_that(result.__class__, equal_to(bool))
-        assert_that(result, equal_to(False))
+        self.assertRaises(DeviceBusyException, cspec.attach_iso, cfg_info, fake_iso_ds_path)
 
         # test if virtual cdrom exist and it's iso_backing
         # and ISO is not attached to the VM
         cspec = self._update_spec()
         cfg_info = self._get_config_info_without_connected(is_iso_backing=True)
 
-        result = cspec.attach_iso(cfg_info, fake_iso_ds_path)
-
-        assert_that(result.__class__, equal_to(bool))
-        assert_that(result, equal_to(True))
+        cspec.attach_iso(cfg_info, fake_iso_ds_path)
 
         dev = cspec.get_spec().deviceChange[0].device
         assert_that(len(cspec.get_spec().deviceChange), equal_to(1))
@@ -294,7 +284,7 @@ def test_add_iso_cdrom(self):
         cspec = self._update_spec()
         cfg_info = self._get_config_info_without_connected(is_iso_backing=False)
 
-        self.assertRaises(TypeError, cspec.attach_iso, cfg_info, fake_iso_ds_path)
+        self.assertRaises(Exception, cspec.attach_iso, cfg_info, fake_iso_ds_path)
 
     def test_disconnect_iso(self):
         # on vm config with no cdrom devices