Consume HttpNotFound

A recent pypowervm commit [1] in release 1.1.4 (now the minimum
requirement for nova-powervm) added a more granular HttpError for 404
called HttpNotFound.  This allows consumers to change goofy logic like:

 try:
     ...
 except HttpError as e:
     if e.response.status == 404:
         do_one_thing()
     else:
         do_another_thing()

...to the prettier:

 try:
     ...
 except HttpNotFound:
     do_one_thing()
 except HttpError:
     do_another_thing()

This change set effects such improvements across nova-powervm where
appropriate.

[1] powervm/pypowervm@9baf10d

Change-Id: Iaadca830b11318787868a42e00fc814241d18081

nova_powervm/tests/virt/powervm/nvram/test_manager.py

@@ -39,32 +39,18 @@ def setUp(self):
         self.mock_remove = self.useFixture(
             fixtures.MockPatchObject(self.fake_store, 'delete')).mock
 
-    @mock.patch('nova_powervm.virt.powervm.nvram.manager.LOG.warning')
+    @mock.patch('nova_powervm.virt.powervm.nvram.manager.LOG.exception')
     @mock.patch.object(vm, 'get_instance_wrapper')
     def test_store_with_exception(self, mock_get_inst, mock_log):
-        mock_resp = mock.Mock()
-        mock_resp.status = 410
-        mock_resp.reqpath = (
-            '/rest/api/uom/ManagedSystem/c5d782c7-44e4-3086-ad15-'
-            'b16fb039d63b/LogicalPartition/1B5FB633-16D1-4E10-A14'
-            '5-E6FB905161A3?group=None')
-        mock_get_inst.side_effect = pvm_exc.HttpError(mock_resp)
+        mock_get_inst.side_effect = pvm_exc.HttpError(mock.Mock())
         mgr = manager.NvramManager(self.fake_store, mock.Mock(), mock.Mock())
         mgr.store(powervm.TEST_INST1)
-        mock_log.assert_called_once_with(u'Unable to store the NVRAM for '
-                                         u'instance: %s',
-                                         powervm.TEST_INST1.name)
+        self.assertEqual(1, mock_log.call_count)
 
     @mock.patch('nova_powervm.virt.powervm.nvram.manager.LOG.warning')
     @mock.patch.object(vm, 'get_instance_wrapper')
     def test_store_with_not_found_exc(self, mock_get_inst, mock_log):
-        mock_resp = mock.Mock()
-        mock_resp.status = 404
-        mock_resp.reqpath = (
-            '/rest/api/uom/ManagedSystem/c5d782c7-44e4-3086-ad15-'
-            'b16fb039d63b/LogicalPartition/1B5FB633-16D1-4E10-A14'
-            '5-E6FB905161A3?group=None')
-        mock_get_inst.side_effect = pvm_exc.HttpError(mock_resp)
+        mock_get_inst.side_effect = pvm_exc.HttpNotFound(mock.Mock())
         mgr = manager.NvramManager(self.fake_store, mock.Mock(), mock.Mock())
         mgr.store(powervm.TEST_INST1)
         mock_log.assert_not_called()

nova_powervm/tests/virt/powervm/tasks/test_vm.py

@@ -37,7 +37,7 @@ def setUp(self):
     @mock.patch('pypowervm.utils.transaction.FeedTask')
     @mock.patch('pypowervm.tasks.partition.build_active_vio_feed_task')
     @mock.patch('pypowervm.tasks.storage.add_lpar_storage_scrub_tasks')
-    @mock.patch('nova_powervm.virt.powervm.vm.crt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.create_lpar')
     def test_create(self, mock_vm_crt, mock_stg, mock_bld, mock_ftsk):
         nvram_mgr = mock.Mock()
         nvram_mgr.fetch.return_value = 'data'
@@ -71,7 +71,7 @@ def test_create(self, mock_vm_crt, mock_stg, mock_bld, mock_ftsk):
 
     @mock.patch('nova_powervm.virt.powervm.vm.get_pvm_uuid')
     @mock.patch('nova_powervm.virt.powervm.tasks.vm.Create.execute')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     def test_create_revert(self, mock_vm_dlt, mock_crt_exc,
                            mock_get_pvm_uuid):
 
@@ -142,13 +142,11 @@ def test_power_off(self, mock_pwroff):
         mock_pwroff.assert_called_once_with(self.apt, self.instance,
                                             force_immediate=True)
 
-    @mock.patch('nova_powervm.virt.powervm.vm.get_pvm_uuid')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
-    def test_delete(self, mock_dlt, mock_uuid):
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
+    def test_delete(self, mock_dlt):
         delete = tf_vm.Delete(self.apt, self.instance)
         delete.execute()
-        mock_dlt.assert_called_once_with(self.apt, mock_uuid.return_value)
-        mock_uuid.assert_called_once_with(self.instance)
+        mock_dlt.assert_called_once_with(self.apt, self.instance)
 
     @mock.patch('nova_powervm.virt.powervm.vm.update')
     def test_resize(self, mock_vm_update):

nova_powervm/tests/virt/powervm/test_driver.py

@@ -107,7 +107,7 @@ def setUp(self):
         self.iscsi_vol_drv = self.iscsi_vol_fix.drv
 
         self.crt_lpar = self.useFixture(fixtures.MockPatch(
-            'nova_powervm.virt.powervm.vm.crt_lpar')).mock
+            'nova_powervm.virt.powervm.vm.create_lpar')).mock
 
         self.get_inst_wrap = self.useFixture(fixtures.MockPatch(
             'nova_powervm.virt.powervm.vm.get_instance_wrapper')).mock
@@ -611,7 +611,7 @@ def test_spawn_recreate(self, mock_tf_run, mock_flow, mock_build_slot_mgr,
     @mock.patch('nova.virt.block_device.DriverVolumeBlockDevice.save')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.PlugMgmtVif.execute')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.PlugVifs.execute')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     @mock.patch('nova.virt.configdrive.required_by')
     @mock.patch('nova_powervm.virt.powervm.vm.power_on')
     @mock.patch('nova_powervm.virt.powervm.vm.power_off')
@@ -749,7 +749,7 @@ def test_spawn_ibmi_without_bdms(self, mock_pwron, mock_boot_conn_type,
                 'execute')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.PlugMgmtVif.execute')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.PlugVifs.execute')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     @mock.patch('nova.virt.configdrive.required_by')
     def test_spawn_ops_rollback_disk(self, mock_cfg_drv, mock_dlt,
                                      mock_plug_vifs, mock_plug_mgmt_vifs,
@@ -778,7 +778,7 @@ def test_spawn_ops_rollback_disk(self, mock_cfg_drv, mock_dlt,
     @mock.patch('nova.virt.block_device.DriverVolumeBlockDevice.save')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.PlugMgmtVif.execute')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.PlugVifs.execute')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     @mock.patch('nova.virt.configdrive.required_by')
     @mock.patch('pypowervm.tasks.power.power_on')
     @mock.patch('pypowervm.tasks.power.power_off')
@@ -918,7 +918,7 @@ def test_add_vol_disconn_task(self, mock_disconn_vol):
     @mock.patch('nova_powervm.virt.powervm.tasks.network.UnplugVifs.execute')
     @mock.patch('nova.virt.powervm_ext.driver.PowerVMDriver.'
                 '_is_booted_from_volume')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     @mock.patch('nova_powervm.virt.powervm.vm.power_off')
     @mock.patch('nova_powervm.virt.powervm.media.ConfigDrivePowerVM.'
                 'dlt_vopt')
@@ -1056,7 +1056,7 @@ def assert_not_called():
     @mock.patch('nova_powervm.virt.powervm.tasks.network.UnplugVifs.execute')
     @mock.patch('nova.virt.powervm_ext.driver.PowerVMDriver.'
                 '_is_booted_from_volume')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     @mock.patch('nova_powervm.virt.powervm.vm.power_off')
     @mock.patch('nova_powervm.virt.powervm.media.ConfigDrivePowerVM.'
                 'dlt_vopt')
@@ -1215,7 +1215,7 @@ def test_detach_volume(self, mock_bld_slot_mgr, mock_inst_exists):
 
     @mock.patch('pypowervm.tasks.scsi_mapper.remove_maps')
     @mock.patch('nova_powervm.virt.powervm.tasks.network.UnplugVifs.execute')
-    @mock.patch('nova_powervm.virt.powervm.vm.dlt_lpar')
+    @mock.patch('nova_powervm.virt.powervm.vm.delete_lpar')
     @mock.patch('nova_powervm.virt.powervm.vm.power_off')
     @mock.patch('nova_powervm.virt.powervm.media.ConfigDrivePowerVM.'
                 'dlt_vopt')
@@ -1673,8 +1673,7 @@ def test_get_vnc_console(self, mock_vterm):
                           mock.ANY, self.inst)
 
         # 404
-        mock_resp = mock.Mock(status=404)
-        mock_vterm.side_effect = pvm_exc.HttpError(mock_resp)
+        mock_vterm.side_effect = pvm_exc.HttpNotFound(mock.Mock(status=404))
         self.assertRaises(exc.InstanceNotFound, self.drv.get_vnc_console,
                           mock.ANY, self.inst)
 

nova_powervm/tests/virt/powervm/test_vm.py

@@ -291,7 +291,7 @@ def test_instance_info(self):
         self.assertEqual(inst_info.cpu_time_ns, 0)
 
         # Check that we raise an exception if the instance is gone.
-        self.apt.read.side_effect = pvm_exc.HttpError(
+        self.apt.read.side_effect = pvm_exc.HttpNotFound(
             mock.MagicMock(status=404))
         self.assertRaises(exception.InstanceNotFound,
                           inst_info.__getattribute__, 'state')
@@ -303,12 +303,8 @@ class FakeResp2(object):
             def __init__(self, body):
                 self.body = '"%s"' % body
 
-        resp = FakeResp2('running')
-
-        def return_resp(*args, **kwds):
-            return resp
-
-        self.apt.read.side_effect = return_resp
+        self.apt.read.side_effect = None
+        self.apt.read.return_value = FakeResp2('running')
         self.assertEqual(inst_info.state, power_state.RUNNING)
 
         # Check the __eq__ method
@@ -336,68 +332,85 @@ def test_get_lpar_names(self):
         self.assertEqual(lpar_list[0], 'z3-9-5-126-208-000001f0')
         self.assertEqual(len(lpar_list), 20)
 
+    @mock.patch('nova_powervm.virt.powervm.vm.get_pvm_uuid')
     @mock.patch('pypowervm.tasks.vterm.close_vterm')
-    def test_dlt_lpar(self, mock_vterm):
+    def test_dlt_lpar(self, mock_vterm, mock_pvm_uuid):
         """Performs a delete LPAR test."""
-        vm.dlt_lpar(self.apt, '12345')
-        self.assertEqual(1, self.apt.delete.call_count)
-        self.assertEqual(1, mock_vterm.call_count)
+        mock_pvm_uuid.return_value = 'pvm_uuid'
+
+        vm.delete_lpar(self.apt, 'inst')
+        mock_pvm_uuid.assert_called_once_with('inst')
+        mock_vterm.assert_called_once_with(self.apt, 'pvm_uuid')
+        self.apt.delete.assert_called_once_with('LogicalPartition',
+                                                root_id='pvm_uuid')
 
         # Test Failure Path
         # build a mock response body with the expected HSCL msg
-        resp = mock.Mock()
-        resp.body = 'error msg: HSCL151B more text'
+        resp = mock.Mock(body='error msg: HSCL151B more text')
         self.apt.delete.side_effect = pvm_exc.Error(
             'Mock Error Message', response=resp)
 
         # Reset counters
+        mock_pvm_uuid.reset_mock()
         self.apt.reset_mock()
         mock_vterm.reset_mock()
 
         self.assertRaises(pvm_exc.Error,
-                          vm.dlt_lpar, self.apt, '12345')
-        self.assertEqual(1, mock_vterm.call_count)
-        self.assertEqual(1, self.apt.delete.call_count)
-
-        # Test HttpError 404
+                          vm.delete_lpar, self.apt, 'inst')
+        mock_pvm_uuid.assert_called_once_with('inst')
+        mock_vterm.assert_called_once_with(self.apt, 'pvm_uuid')
+        self.apt.delete.assert_called_once_with('LogicalPartition',
+                                                root_id='pvm_uuid')
+
+        # Test HttpNotFound - exception not raised
+        mock_pvm_uuid.reset_mock()
         self.apt.reset_mock()
         mock_vterm.reset_mock()
 
         resp.status = 404
-        self.apt.delete.side_effect = pvm_exc.HttpError(resp=resp)
-        vm.dlt_lpar(self.apt, '54321')
-        self.assertEqual(1, mock_vterm.call_count)
-        self.assertEqual(1, self.apt.delete.call_count)
+        self.apt.delete.side_effect = pvm_exc.HttpNotFound(resp=resp)
+        vm.delete_lpar(self.apt, 'inst')
+        mock_pvm_uuid.assert_called_once_with('inst')
+        mock_vterm.assert_called_once_with(self.apt, 'pvm_uuid')
+        self.apt.delete.assert_called_once_with('LogicalPartition',
+                                                root_id='pvm_uuid')
 
         # Test Other HttpError
+        mock_pvm_uuid.reset_mock()
         self.apt.reset_mock()
         mock_vterm.reset_mock()
 
         resp.status = 111
         self.apt.delete.side_effect = pvm_exc.HttpError(resp=resp)
-        self.assertRaises(pvm_exc.HttpError, vm.dlt_lpar, self.apt, '11111')
-        self.assertEqual(1, mock_vterm.call_count)
-        self.assertEqual(1, self.apt.delete.call_count)
-
-        # Test HttpError 404 closing vterm
+        self.assertRaises(pvm_exc.HttpError, vm.delete_lpar, self.apt, 'inst')
+        mock_pvm_uuid.assert_called_once_with('inst')
+        mock_vterm.assert_called_once_with(self.apt, 'pvm_uuid')
+        self.apt.delete.assert_called_once_with('LogicalPartition',
+                                                root_id='pvm_uuid')
+
+        # Test HttpNotFound closing vterm
+        mock_pvm_uuid.reset_mock()
         self.apt.reset_mock()
         mock_vterm.reset_mock()
 
         resp.status = 404
-        mock_vterm.side_effect = pvm_exc.HttpError(resp=resp)
-        vm.dlt_lpar(self.apt, '55555')
-        self.assertEqual(1, mock_vterm.call_count)
-        self.assertEqual(0, self.apt.delete.call_count)
+        mock_vterm.side_effect = pvm_exc.HttpNotFound(resp=resp)
+        vm.delete_lpar(self.apt, 'inst')
+        mock_pvm_uuid.assert_called_once_with('inst')
+        mock_vterm.assert_called_once_with(self.apt, 'pvm_uuid')
+        self.apt.delete.assert_not_called()
 
         # Test Other HttpError closing vterm
+        mock_pvm_uuid.reset_mock()
         self.apt.reset_mock()
         mock_vterm.reset_mock()
 
         resp.status = 111
         mock_vterm.side_effect = pvm_exc.HttpError(resp=resp)
-        self.assertRaises(pvm_exc.HttpError, vm.dlt_lpar, self.apt, '33333')
-        self.assertEqual(1, mock_vterm.call_count)
-        self.assertEqual(0, self.apt.delete.call_count)
+        self.assertRaises(pvm_exc.HttpError, vm.delete_lpar, self.apt, 'inst')
+        mock_pvm_uuid.assert_called_once_with('inst')
+        mock_vterm.assert_called_once_with(self.apt, 'pvm_uuid')
+        self.apt.delete.assert_not_called()
 
     @mock.patch('nova_powervm.virt.powervm.vm.VMBuilder._add_IBMi_attrs')
     @mock.patch('pypowervm.utils.lpar_builder.DefaultStandardize')
@@ -412,7 +425,7 @@ def test_crt_lpar(self, mock_vld_all, mock_bld, mock_stdz, mock_ibmi):
         lparw = pvm_lpar.LPAR.wrap(self.resp.feed.entries[0])
         mock_bld.return_value = lparw
         self.apt.create.return_value = lparw.entry
-        vm.crt_lpar(self.apt, host_wrapper, instance, nvram='data')
+        vm.create_lpar(self.apt, host_wrapper, instance, nvram='data')
         self.apt.create.assert_called_once_with(
             lparw, host_wrapper.schema_type, child_type='LogicalPartition',
             root_id=host_wrapper.uuid, service='uom', timeout=-1)
@@ -429,7 +442,8 @@ def test_crt_lpar(self, mock_vld_all, mock_bld, mock_stdz, mock_ibmi):
         self.apt.create.return_value = lparw.entry
         mock_slot_mgr = mock.Mock(build_map=mock.Mock(
             get_max_vslots=mock.Mock(return_value=123)))
-        vm.crt_lpar(self.apt, host_wrapper, instance, slot_mgr=mock_slot_mgr)
+        vm.create_lpar(self.apt, host_wrapper, instance,
+                       slot_mgr=mock_slot_mgr)
         self.assertTrue(self.apt.create.called)
         self.assertTrue(mock_vld_all.called)
         self.assertTrue(lparw.srr_enabled)
@@ -443,26 +457,25 @@ def test_crt_lpar(self, mock_vld_all, mock_bld, mock_stdz, mock_ibmi):
         # Test to verify the LPAR Creation with invalid name specification
         mock_bld.side_effect = lpar_bld.LPARBuilderException("Invalid Name")
         host_wrapper = mock.Mock()
-        self.assertRaises(exception.BuildAbortException, vm.crt_lpar,
+        self.assertRaises(exception.BuildAbortException, vm.create_lpar,
                           self.apt, host_wrapper, instance)
 
         resp = mock.Mock(status=202, method='fake', path='/dev/',
                          reason='Failure')
         mock_bld.side_effect = pvm_exc.HttpError(resp)
         try:
-            vm.crt_lpar(self.apt, host_wrapper, instance)
+            vm.create_lpar(self.apt, host_wrapper, instance)
         except nvex.PowerVMAPIFailed as e:
             self.assertEqual(e.kwargs['inst_name'], instance.name)
             self.assertEqual(e.kwargs['reason'], mock_bld.side_effect)
         flavor.extra_specs = {'powervm:BADATTR': 'true'}
         host_wrapper = mock.Mock()
-        self.assertRaises(exception.InvalidAttribute, vm.crt_lpar,
+        self.assertRaises(exception.InvalidAttribute, vm.create_lpar,
                           self.apt, host_wrapper, instance)
 
     @mock.patch('pypowervm.wrappers.logical_partition.LPAR.get')
     def test_get_instance_wrapper(self, mock_get):
-        resp = mock.Mock(status=404)
-        mock_get.side_effect = pvm_exc.Error('message', response=resp)
+        mock_get.side_effect = pvm_exc.HttpNotFound(resp=mock.Mock(status=404))
         instance = objects.Instance(**powervm.TEST_INSTANCE)
         # vm.get_instance_wrapper(self.apt, instance, 'lpar_uuid')
         self.assertRaises(exception.InstanceNotFound, vm.get_instance_wrapper,
@@ -735,7 +748,7 @@ def adpt_read_no_log(*args, **kwds):
 
         resp = mock.MagicMock()
         resp.status = 404
-        self.apt.read.side_effect = pvm_exc.HttpError(resp)
+        self.apt.read.side_effect = pvm_exc.HttpNotFound(resp)
         self.assertRaises(exception.InstanceNotFound, vm.get_vm_qp, self.apt,
                           'lpar_uuid', log_errors=False)
 

nova_powervm/virt/powervm/driver.py

@@ -1732,14 +1732,12 @@ def get_vnc_console(self, context, instance):
                 self.adapter, lpar_uuid, host, vnc_path=lpar_uuid,
                 use_x509_auth=use_x509_auth, ca_certs=ca_certs,
                 server_cert=server_cert, server_key=server_key)
-        except Exception as err:
-            # If the LPAR was not found, then give a more descriptive message
-            if isinstance(err, pvm_exc.HttpError):
-                if err.response.status == 404:
-                    raise exception.InstanceNotFound(instance_id=instance.uuid)
+        except pvm_exc.HttpNotFound:
+            raise exception.InstanceNotFound(instance_id=instance.uuid)
+        except pvm_exc.Error:
             # Otherwise wrapper the error in an exception that can be handled
-            raise exception.InternalError(
-                err=_("Unable to open console.  Error is: %s") % err)
+            LOG.exception("Unable to open console.", instance=instance)
+            raise exception.InternalError(err=_("Unable to open console."))
 
         # Note that the VNC viewer will wrap the internal_access_path with
         # the HTTP content.