)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":6167,"name":"Ken\u0027ichi Ohmichi","email":"ken1ohmichi@gmail.com","username":"oomichi"},"change_message_id":"37486b9f1a27cb0603d0957112d4883f73245eac","unresolved":false,"context_lines":[{"line_number":858,"context_line":"                self.driver.destroy(context, instance,"},{"line_number":859,"context_line":"                                    network_info,"},{"line_number":860,"context_line":"                                    bdi, destroy_disks)"},{"line_number":861,"context_line":"            except Exception as err:"},{"line_number":862,"context_line":"                LOG.warning(_LW(\"Failed to destroy instance %(id)s \""},{"line_number":863,"context_line":"                                \"as %(err)s.\"),"},{"line_number":864,"context_line":"                            {\u0027id\u0027: instance.uuid,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a5ae5dd_1ff1fbcd","line":861,"range":{"start_line":861,"start_character":19,"end_line":861,"end_character":28},"updated":"2016-02-03 02:36:54.000000000","message":"This exception is too wide catching.\nThis will make unexpected exceptions disappear.\n\nAs the launchpad report, the exception was exception.VolumeDeviceNotFound.\nWhy don\u0027t you catch exception.VolumeDeviceNotFound instead of Exception here?","commit_id":"37fb6de62ae6935fb92b0ac2607f443aeaaccb41"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"d45b71d1279d9d81e83c0e9fdf498a3e52c7ea62","unresolved":false,"context_lines":[{"line_number":858,"context_line":"                self.driver.destroy(context, instance,"},{"line_number":859,"context_line":"                                    network_info,"},{"line_number":860,"context_line":"                                    bdi, destroy_disks)"},{"line_number":861,"context_line":"            except Exception as err:"},{"line_number":862,"context_line":"                LOG.warning(_LW(\"Failed to destroy instance %(id)s \""},{"line_number":863,"context_line":"                                \"as %(err)s.\"),"},{"line_number":864,"context_line":"                            {\u0027id\u0027: instance.uuid,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a5ae5dd_1ce9fb1a","line":861,"range":{"start_line":861,"start_character":19,"end_line":861,"end_character":28},"in_reply_to":"5a5ae5dd_1ff1fbcd","updated":"2016-02-03 09:23:39.000000000","message":"I also think, that better place for catching such exception might be in the driver (in this case iscsi driver), just like in the nfs one…","commit_id":"37fb6de62ae6935fb92b0ac2607f443aeaaccb41"}],"nova/tests/unit/virt/libvirt/volume/test_iscsi.py":[{"author":{"_account_id":20223,"name":"zhaolihui","email":"zhaolh@awcloud.com","username":"zhaolihui"},"change_message_id":"bce074d5654793072766acee8dccdb6598611491","unresolved":false,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        libvirt_driver \u003d iscsi.LibvirtISCSIVolumeDriver(self.fake_conn)"},{"line_number":100,"context_line":"        libvirt_driver.connector.disconnect_volume \u003d mock.MagicMock("},{"line_number":101,"context_line":"            side_effect\u003dexception.VolumeNotFound(volume_id\u003d12345678))"},{"line_number":102,"context_line":"        libvirt_driver.disconnect_volume(connection_info, device_path)"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"        msg \u003d mock_LOG_debug.call_args_list[1]"}],"source_content_type":"text/x-python","patch_set":5,"id":"bae84128_e86e35c8","line":101,"range":{"start_line":101,"start_character":49,"end_line":101,"end_character":67},"updated":"2016-02-20 06:26:03.000000000","message":"the volume_id value should be from context,\nnot only 12345678","commit_id":"cc48203d18682bd8db930c75fb851829572369c2"},{"author":{"_account_id":17527,"name":"Felix Ma","email":"maqi@cmss.chinamobile.com","username":"felix23ma"},"change_message_id":"7def69663ee59f679611387b924c0bf52b2d329d","unresolved":false,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        libvirt_driver \u003d iscsi.LibvirtISCSIVolumeDriver(self.fake_conn)"},{"line_number":100,"context_line":"        libvirt_driver.connector.disconnect_volume \u003d mock.MagicMock("},{"line_number":101,"context_line":"            side_effect\u003dexception.VolumeNotFound(volume_id\u003d12345678))"},{"line_number":102,"context_line":"        libvirt_driver.disconnect_volume(connection_info, device_path)"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"        msg \u003d mock_LOG_debug.call_args_list[1]"}],"source_content_type":"text/x-python","patch_set":5,"id":"bae84128_48748134","line":101,"range":{"start_line":101,"start_character":49,"end_line":101,"end_character":67},"in_reply_to":"bae84128_e86e35c8","updated":"2016-02-20 07:01:37.000000000","message":"volume_id in the test case is in self.vol. I don\u0027t think it matters in this case as just VolumeNotFound is needed. Anyway I will upload a new patch. Thanks for your review!","commit_id":"cc48203d18682bd8db930c75fb851829572369c2"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"eda8167d93e28306f28a14c72caec87d74bf5590","unresolved":false,"context_lines":[{"line_number":98,"context_line":"        libvirt_driver.connector.disconnect_volume \u003d mock.MagicMock("},{"line_number":99,"context_line":"            side_effect\u003dexception.VolumeDeviceNotFound(device\u003ddevice_path))"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        self.assertRaises(exception.DiskNotFound,"},{"line_number":102,"context_line":"                          libvirt_driver.disconnect_volume,"},{"line_number":103,"context_line":"                          connection_info,"},{"line_number":104,"context_line":"                          device_path)"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_947af8ef","line":101,"range":{"start_line":101,"start_character":26,"end_line":101,"end_character":49},"updated":"2016-02-22 08:12:38.000000000","message":"I think it is confusing: first you mock disconnect_volume with raising one exception, and expect the other.","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":17527,"name":"Felix Ma","email":"maqi@cmss.chinamobile.com","username":"felix23ma"},"change_message_id":"69b459997a59463852d42ef020684d6414105387","unresolved":false,"context_lines":[{"line_number":98,"context_line":"        libvirt_driver.connector.disconnect_volume \u003d mock.MagicMock("},{"line_number":99,"context_line":"            side_effect\u003dexception.VolumeDeviceNotFound(device\u003ddevice_path))"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        self.assertRaises(exception.DiskNotFound,"},{"line_number":102,"context_line":"                          libvirt_driver.disconnect_volume,"},{"line_number":103,"context_line":"                          connection_info,"},{"line_number":104,"context_line":"                          device_path)"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_f708a62f","line":101,"range":{"start_line":101,"start_character":26,"end_line":101,"end_character":49},"in_reply_to":"bae84128_947af8ef","updated":"2016-02-22 08:45:03.000000000","message":"It\u0027s because I raise DiskNotFound exception from iscsi.py.\nI only mock connector.disconnect_volume, not disconnect_volume in iscsi.py","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"811d60897a2fb556e72eeb692fba5287d0a1820d","unresolved":false,"context_lines":[{"line_number":98,"context_line":"        libvirt_driver.connector.disconnect_volume \u003d mock.MagicMock("},{"line_number":99,"context_line":"            side_effect\u003dexception.VolumeDeviceNotFound(device\u003ddevice_path))"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        self.assertRaises(exception.DiskNotFound,"},{"line_number":102,"context_line":"                          libvirt_driver.disconnect_volume,"},{"line_number":103,"context_line":"                          connection_info,"},{"line_number":104,"context_line":"                          device_path)"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_878e0177","line":101,"range":{"start_line":101,"start_character":26,"end_line":101,"end_character":49},"in_reply_to":"bae84128_f708a62f","updated":"2016-02-23 14:53:05.000000000","message":"Yeah, I\u0027ve noticed. See my comment on the iscsi.py","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"}],"nova/virt/libvirt/volume/iscsi.py":[{"author":{"_account_id":6167,"name":"Ken\u0027ichi Ohmichi","email":"ken1ohmichi@gmail.com","username":"oomichi"},"change_message_id":"6dafad6354c567f6859e8f8d6bfb3432d866c2f0","unresolved":false,"context_lines":[{"line_number":16,"context_line":"from oslo_log import log as logging"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"from nova import utils"},{"line_number":19,"context_line":"from nova import exception"},{"line_number":20,"context_line":"from nova.virt.libvirt.volume import volume as libvirt_volume"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bae84128_923b5deb","line":19,"range":{"start_line":19,"start_character":0,"end_line":19,"end_character":26},"updated":"2016-02-19 23:14:42.000000000","message":"Please put this new line before the above like for passing pep8 check:\n\n 2016-02-17 23:07:12.571 | ./nova/virt/libvirt/volume/iscsi.py:19:1: H306  imports not in alphabetical order (nova.utils, nova.exception)","commit_id":"988114ffc61dece0e31a877d17fc36b3198d21e0"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"eda8167d93e28306f28a14c72caec87d74bf5590","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        LOG.debug(\"calling os-brick to detach iSCSI Volume\")"},{"line_number":95,"context_line":"        try:"},{"line_number":96,"context_line":"            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)"},{"line_number":97,"context_line":"        except exception.VolumeDeviceNotFound as exc:"},{"line_number":98,"context_line":"            LOG.debug(\u0027Ignoring VolumeDeviceNotFound: %s\u0027, exc)"},{"line_number":99,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":100,"context_line":"        LOG.debug(\"Disconnected iSCSI Volume %s\", disk_dev)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        super(LibvirtISCSIVolumeDriver,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_f4f42488","line":99,"range":{"start_line":97,"start_character":0,"end_line":99,"end_character":59},"updated":"2016-02-22 08:12:38.000000000","message":"I\u0027m wondering, if it is needed to elevate the exception, which would be catch in manager. Isn\u0027t it better to just log (with warning) it here and than simply pass, since manager will ignore the exception anyway?","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":17527,"name":"Felix Ma","email":"maqi@cmss.chinamobile.com","username":"felix23ma"},"change_message_id":"439ac3af6d24794f4e01a409b9f6337b885921ea","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        LOG.debug(\"calling os-brick to detach iSCSI Volume\")"},{"line_number":95,"context_line":"        try:"},{"line_number":96,"context_line":"            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)"},{"line_number":97,"context_line":"        except exception.VolumeDeviceNotFound as exc:"},{"line_number":98,"context_line":"            LOG.debug(\u0027Ignoring VolumeDeviceNotFound: %s\u0027, exc)"},{"line_number":99,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":100,"context_line":"        LOG.debug(\"Disconnected iSCSI Volume %s\", disk_dev)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        super(LibvirtISCSIVolumeDriver,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_7175bb96","line":99,"range":{"start_line":97,"start_character":0,"end_line":99,"end_character":59},"in_reply_to":"bae84128_08a08b8f","updated":"2016-02-22 11:54:21.000000000","message":"I tried a similar proposal in patch set 6. The difference was I excepted VolumeNotFound instead of VolumeDeviceNotFound. But the tempest verification was failed. I couldn\u0027t figure out what\u0027s going wrong. So I just raised another exception DiskNotFound to get around.\n\nIf I remove the debug logging in iscsi.py, do you think it\u0027s OK? Or should I catch VolumeDeviceNotFound into warning logging as you proposed?","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":17527,"name":"Felix Ma","email":"maqi@cmss.chinamobile.com","username":"felix23ma"},"change_message_id":"a8c3d7cf22a280eb87ec18f0eaeb50ec0cf91834","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        LOG.debug(\"calling os-brick to detach iSCSI Volume\")"},{"line_number":95,"context_line":"        try:"},{"line_number":96,"context_line":"            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)"},{"line_number":97,"context_line":"        except exception.VolumeDeviceNotFound as exc:"},{"line_number":98,"context_line":"            LOG.debug(\u0027Ignoring VolumeDeviceNotFound: %s\u0027, exc)"},{"line_number":99,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":100,"context_line":"        LOG.debug(\"Disconnected iSCSI Volume %s\", disk_dev)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        super(LibvirtISCSIVolumeDriver,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_03c3e589","line":99,"range":{"start_line":97,"start_character":0,"end_line":99,"end_character":59},"in_reply_to":"bae84128_13d077f5","updated":"2016-02-24 01:33:17.000000000","message":"Roman, thanks for your kind help! I will update the patch accordingly.","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"811d60897a2fb556e72eeb692fba5287d0a1820d","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        LOG.debug(\"calling os-brick to detach iSCSI Volume\")"},{"line_number":95,"context_line":"        try:"},{"line_number":96,"context_line":"            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)"},{"line_number":97,"context_line":"        except exception.VolumeDeviceNotFound as exc:"},{"line_number":98,"context_line":"            LOG.debug(\u0027Ignoring VolumeDeviceNotFound: %s\u0027, exc)"},{"line_number":99,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":100,"context_line":"        LOG.debug(\"Disconnected iSCSI Volume %s\", disk_dev)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        super(LibvirtISCSIVolumeDriver,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_13d077f5","line":99,"range":{"start_line":97,"start_character":0,"end_line":99,"end_character":59},"in_reply_to":"bae84128_7175bb96","updated":"2016-02-23 14:53:05.000000000","message":"There is a difference between the exception you was trying to catch and the one which was actually raised. As you can see, volume disconnect is performed on connector object, which origins from os-brick lib (os-brick.exception.VolumeDeviceNotFound) - maybe that was the reason.\n\nSo instead of providing another type of exception (which basically is the same as on the os_brick lib), i\u0027d propose the similar approach as before, like:\n\n    …\n    from os_brick import exception as os_brick_exception\n    …\n\n    def disconnect_volume(self, connection_info, disk_dev):\n        \"\"\"Detach the volume from instance_name.\"\"\"\n\n        LOG.debug(\"calling os-brick to detach iSCSI Volume\")\n        try:\n            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)\n        except os_brick_exception.VolumeDeviceNotFound as exc:\n            LOG.warning(_LW(\u0027Ignoring VolumeDeviceNotFound: %s\u0027), exc)\n            # NOTE: don\u0027t do anything further - we don\u0027t have volume device\n            # anymore\n            return\n\nAnd the corresponding test can look like this:\n\n    …\n    from os_brick import exception as os_brick_exception\n    …\n\n    def test_libvirt_iscsi_driver_disconnect_volume_with_devicenotfound(self):\n        device_path \u003d \u0027/dev/fake-dev\u0027\n        connection_info \u003d {\u0027data\u0027: {\u0027device_path\u0027: device_path}}\n\n        exception_class \u003d os_brick_exception.VolumeDeviceNotFound\n\n        libvirt_driver \u003d iscsi.LibvirtISCSIVolumeDriver(self.fake_conn)\n        libvirt_driver.connector.disconnect_volume \u003d mock.MagicMock(\n            side_effect\u003dexception_class(device\u003ddevice_path))\n\n        self.assertIsNone(libvirt_driver.disconnect_volume(connection_info,\n                                                           device_path))\n\nSo, for direct answer for your question - yes, i would opt for the warning message in case of missing device for the volume - it is not usual situation where the device is gone, and it deserves a warning message, and not cluttering the manager with catching the re-raised exceptions.\n\nUnless the entire case is more serious than something which we simply ignore, I\u0027m not aware what was the circumstances this situation emerged - the bug report doesn\u0027t shed any light on that.","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"363a19a1048164992b5c843a2a0bd8f3560e1ad0","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        LOG.debug(\"calling os-brick to detach iSCSI Volume\")"},{"line_number":95,"context_line":"        try:"},{"line_number":96,"context_line":"            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)"},{"line_number":97,"context_line":"        except exception.VolumeDeviceNotFound as exc:"},{"line_number":98,"context_line":"            LOG.debug(\u0027Ignoring VolumeDeviceNotFound: %s\u0027, exc)"},{"line_number":99,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":100,"context_line":"        LOG.debug(\"Disconnected iSCSI Volume %s\", disk_dev)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        super(LibvirtISCSIVolumeDriver,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_08a08b8f","line":99,"range":{"start_line":97,"start_character":0,"end_line":99,"end_character":59},"in_reply_to":"bae84128_d7a00246","updated":"2016-02-22 09:58:40.000000000","message":"What I\u0027ve suggested was more like:\n\n    def disconnect_volume(self, connection_info, disk_dev):\n        \"\"\"Detach the volume from instance_name.\"\"\"\n\n        LOG.debug(\"calling os-brick to detach iSCSI Volume\")\n        try:\n            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)\n        except exception.VolumeDeviceNotFound as exc:\n            LOG.warning(_LW(\u0027Ignoring VolumeDeviceNotFound: %s\u0027), exc)\n\nSo that it doesn\u0027t have to elevate it to the manager (through rising another exception) and the information about the missing volume device would appear in log.\n\nNote, that in your proposal, you are logging the fact about something bad happened twice - first here and the second one in the manager. What I\u0027m suggesting is to simplify things and be less confusing (throwing different exception from the on which is caught, just to log the situation might not be the thing I would expect).","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"},{"author":{"_account_id":17527,"name":"Felix Ma","email":"maqi@cmss.chinamobile.com","username":"felix23ma"},"change_message_id":"69b459997a59463852d42ef020684d6414105387","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        LOG.debug(\"calling os-brick to detach iSCSI Volume\")"},{"line_number":95,"context_line":"        try:"},{"line_number":96,"context_line":"            self.connector.disconnect_volume(connection_info[\u0027data\u0027], None)"},{"line_number":97,"context_line":"        except exception.VolumeDeviceNotFound as exc:"},{"line_number":98,"context_line":"            LOG.debug(\u0027Ignoring VolumeDeviceNotFound: %s\u0027, exc)"},{"line_number":99,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":100,"context_line":"        LOG.debug(\"Disconnected iSCSI Volume %s\", disk_dev)"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        super(LibvirtISCSIVolumeDriver,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bae84128_d7a00246","line":99,"range":{"start_line":97,"start_character":0,"end_line":99,"end_character":59},"in_reply_to":"bae84128_f4f42488","updated":"2016-02-22 08:45:03.000000000","message":"I had tried to do what you said but the manager won\u0027t ignore VolumeDeviceNotFound exception in nova/compute/manager.py:\ndef _driver_detach_volume(self, context, instance, bdm):\n    ...\n        except exception.DiskNotFound as err:\n            LOG.warning(_LW(\u0027Ignoring DiskNotFound exception while detaching \u0027\n                            \u0027volume %(volume_id)s from %(mp)s: %(err)s\u0027),\n                        {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp, \u0027err\u0027: err},\n                        instance\u003dinstance)\n        except Exception:\n            with excutils.save_and_reraise_exception():\n                LOG.exception(_LE(\u0027Failed to detach volume %(volume_id)s \u0027\n                                  \u0027from %(mp)s\u0027),\n                              {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},\n                              context\u003dcontext, instance\u003dinstance)\n                self.volume_api.roll_detaching(context, volume_id)","commit_id":"fd73305d69c865372a703e218ffda040e0b417b9"}]}