)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"86d19ca56f2ef8a8991c34387793c16f664f4ab7","unresolved":false,"context_lines":[{"line_number":7,"context_line":"libvirt: Delete duplicate check when live-migrating"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"A year ago a useless check was added:"},{"line_number":10,"context_line":"https://review.openstack.org/#/c/195885"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"The above patch was aimed to enable live-migration when"},{"line_number":13,"context_line":"instance is booted from volume and has not local disk"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"1a95cdbc_7a496c30","line":10,"range":{"start_line":10,"start_character":0,"end_line":10,"end_character":39},"updated":"2016-10-10 14:58:42.000000000","message":"convention here would be to include the Change-Id or partial commit hash, e.g. \n\n    I7989128\n\nor\n\n    177e9ff7","commit_id":"e1ee17ce5be832ce8821ed5a61c4586a9feaf9e0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"418e349e33cfb77fbab526594b17839d2394f3d0","unresolved":false,"context_lines":[{"line_number":7,"context_line":"libvirt: Delete duplicate check when live-migrating"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"A year ago a useless check was added:"},{"line_number":10,"context_line":"https://review.openstack.org/#/c/195885"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"The above patch was aimed to enable live-migration when"},{"line_number":13,"context_line":"instance is booted from volume and has not local disk"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"7a77a97e_cde6aa14","line":10,"range":{"start_line":10,"start_character":0,"end_line":10,"end_character":39},"in_reply_to":"1a95cdbc_7a496c30","updated":"2016-11-17 05:58:05.000000000","message":"Done","commit_id":"e1ee17ce5be832ce8821ed5a61c4586a9feaf9e0"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"cfaea62f041f164fece22049efd72f9034f9ab44","unresolved":false,"context_lines":[{"line_number":7286,"context_line":"                          drvr.check_can_live_migrate_source,"},{"line_number":7287,"context_line":"                          self.context, instance, dest_check_data)"},{"line_number":7288,"context_line":""},{"line_number":7289,"context_line":"    def test_check_can_live_migrate_source_booted_from_volume(self):"},{"line_number":7290,"context_line":"        instance, dest_check_data, drvr \u003d self._mock_can_live_migrate_source("},{"line_number":7291,"context_line":"                is_booted_from_volume\u003dTrue,"},{"line_number":7292,"context_line":"                block_device_text\u003d\u0027[]\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_38242dc5","side":"PARENT","line":7289,"updated":"2016-08-24 13:35:22.000000000","message":"So removing one of those tests reduces coverage as we never make sure that we pass BDI to _is_shared_block_storage instead of calling it always with just None","commit_id":"b36f90572e606b3ee2ebe34d934842b7596e5dad"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"9ded491c36cb6e3d96d205de038715d557c99e64","unresolved":false,"context_lines":[{"line_number":7286,"context_line":"                          drvr.check_can_live_migrate_source,"},{"line_number":7287,"context_line":"                          self.context, instance, dest_check_data)"},{"line_number":7288,"context_line":""},{"line_number":7289,"context_line":"    def test_check_can_live_migrate_source_booted_from_volume(self):"},{"line_number":7290,"context_line":"        instance, dest_check_data, drvr \u003d self._mock_can_live_migrate_source("},{"line_number":7291,"context_line":"                is_booted_from_volume\u003dTrue,"},{"line_number":7292,"context_line":"                block_device_text\u003d\u0027[]\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_af85745a","side":"PARENT","line":7289,"in_reply_to":"1ac06dbe_38242dc5","updated":"2016-08-24 13:52:58.000000000","message":"These tests passed block_device_text rather than block_device_info.","commit_id":"b36f90572e606b3ee2ebe34d934842b7596e5dad"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"f0341b65a8643438626bae283ea5ab75d4e8c697","unresolved":false,"context_lines":[{"line_number":7286,"context_line":"                          drvr.check_can_live_migrate_source,"},{"line_number":7287,"context_line":"                          self.context, instance, dest_check_data)"},{"line_number":7288,"context_line":""},{"line_number":7289,"context_line":"    def test_check_can_live_migrate_source_booted_from_volume(self):"},{"line_number":7290,"context_line":"        instance, dest_check_data, drvr \u003d self._mock_can_live_migrate_source("},{"line_number":7291,"context_line":"                is_booted_from_volume\u003dTrue,"},{"line_number":7292,"context_line":"                block_device_text\u003d\u0027[]\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_af2f343d","side":"PARENT","line":7289,"in_reply_to":"1ac06dbe_af85745a","updated":"2016-08-24 13:55:18.000000000","message":"Ahh, you are right, there is too many things with similar name","commit_id":"b36f90572e606b3ee2ebe34d934842b7596e5dad"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"6dff8f01baac3e1c2532251a00d8ee76cb67d246","unresolved":false,"context_lines":[{"line_number":7171,"context_line":""},{"line_number":7172,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_is_shared_block_storage\u0027)"},{"line_number":7173,"context_line":"        drvr._is_shared_block_storage(instance, dest_check_data,"},{"line_number":7174,"context_line":"                None).AndReturn(is_shared_block_storage)"},{"line_number":7175,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_check_shared_storage_test_file\u0027)"},{"line_number":7176,"context_line":"        drvr._check_shared_storage_test_file(\u0027file\u0027).AndReturn("},{"line_number":7177,"context_line":"                is_shared_instance_path)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_9dbd6004","line":7174,"range":{"start_line":7174,"start_character":16,"end_line":7174,"end_character":20},"updated":"2016-08-24 09:54:33.000000000","message":"Why do you need to change it to None? Given that this parameter is optional in _is_shared_block_storage i think it should be validated, otherwise someone might just change it to None and tests will still pass.","commit_id":"8a440000e7a8f39bf23f59e5fd85ed0615c0bda0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"e10942eaa46eca5ded048d155e6212e6caa7fca9","unresolved":false,"context_lines":[{"line_number":7171,"context_line":""},{"line_number":7172,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_is_shared_block_storage\u0027)"},{"line_number":7173,"context_line":"        drvr._is_shared_block_storage(instance, dest_check_data,"},{"line_number":7174,"context_line":"                None).AndReturn(is_shared_block_storage)"},{"line_number":7175,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_check_shared_storage_test_file\u0027)"},{"line_number":7176,"context_line":"        drvr._check_shared_storage_test_file(\u0027file\u0027).AndReturn("},{"line_number":7177,"context_line":"                is_shared_instance_path)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_ce7600a3","line":7174,"range":{"start_line":7174,"start_character":16,"end_line":7174,"end_character":20},"in_reply_to":"1ac06dbe_9dbd6004","updated":"2016-08-24 10:44:05.000000000","message":"Since _is_shared_block_storage is called from check_can_live_migrate_source with block_device_info parameter (see L5497 on https://review.openstack.org/#/c/337056/3/nova/virt/libvirt/driver.py@5497), i have to specify an actual parameter here to mock. But here is no more block_device_info parameter, since i deleted it as not used in tests. Because that the only way to mock the call is to specify None here.","commit_id":"8a440000e7a8f39bf23f59e5fd85ed0615c0bda0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"3fc50effd57df6a1c2683de5109461335236a62c","unresolved":false,"context_lines":[{"line_number":7171,"context_line":""},{"line_number":7172,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_is_shared_block_storage\u0027)"},{"line_number":7173,"context_line":"        drvr._is_shared_block_storage(instance, dest_check_data,"},{"line_number":7174,"context_line":"                None).AndReturn(is_shared_block_storage)"},{"line_number":7175,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_check_shared_storage_test_file\u0027)"},{"line_number":7176,"context_line":"        drvr._check_shared_storage_test_file(\u0027file\u0027).AndReturn("},{"line_number":7177,"context_line":"                is_shared_instance_path)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_8c2feb3d","line":7174,"range":{"start_line":7174,"start_character":16,"end_line":7174,"end_character":20},"in_reply_to":"1ac06dbe_a3863a14","updated":"2016-08-24 12:37:27.000000000","message":"As it is said in the description, this fix simplifies related tests. This is a part of related tests, and it is simplified by removing unused function parameter - block_device_info is not used for calls of _mock_can_live_migrate_source. The actual value which is passed to _is_shared_block_storage from inside check_can_live_migrate_source for all tests is None. So that i just specify it instead of removed block_device_info. I do not change the way how it is called, this is not a call of _is_shared_block_storage, and this is not a redudant mock.\n\nCould you explain me explicitly, what i should to do remove block_device_info parameter from _mock_can_live_migrate_source?","commit_id":"8a440000e7a8f39bf23f59e5fd85ed0615c0bda0"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"846b33cb5272033b4f33a050dbfbf11dff6a919d","unresolved":false,"context_lines":[{"line_number":7171,"context_line":""},{"line_number":7172,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_is_shared_block_storage\u0027)"},{"line_number":7173,"context_line":"        drvr._is_shared_block_storage(instance, dest_check_data,"},{"line_number":7174,"context_line":"                None).AndReturn(is_shared_block_storage)"},{"line_number":7175,"context_line":"        self.mox.StubOutWithMock(drvr, \u0027_check_shared_storage_test_file\u0027)"},{"line_number":7176,"context_line":"        drvr._check_shared_storage_test_file(\u0027file\u0027).AndReturn("},{"line_number":7177,"context_line":"                is_shared_instance_path)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac06dbe_a3863a14","line":7174,"range":{"start_line":7174,"start_character":16,"end_line":7174,"end_character":20},"in_reply_to":"1ac06dbe_ce7600a3","updated":"2016-08-24 11:57:50.000000000","message":"My concern is that you are changing tests apart from functionality that you changed. You did not touch how _is_shared_block_storage is called, but you change the way how it is called in tests. I\u0027d expect that you just remove redundant mocks and that\u0027s all.","commit_id":"8a440000e7a8f39bf23f59e5fd85ed0615c0bda0"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0a10b8b797142bb0688898c189c44708f1bcae1d","unresolved":false,"context_lines":[{"line_number":5628,"context_line":"            # place block device files under the instance path"},{"line_number":5629,"context_line":"            return True"},{"line_number":5630,"context_line":""},{"line_number":5631,"context_line":"        if (dest_check_data.is_volume_backed and"},{"line_number":5632,"context_line":"                not bool(jsonutils.loads("},{"line_number":5633,"context_line":"                    self.get_instance_disk_info(instance,"},{"line_number":5634,"context_line":"                                                block_device_info)))):"},{"line_number":5635,"context_line":"            return True"},{"line_number":5636,"context_line":""},{"line_number":5637,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a74a57a_315ed245","line":5634,"range":{"start_line":5631,"start_character":8,"end_line":5634,"end_character":70},"updated":"2016-12-01 01:07:17.000000000","message":"This is the equivalent check that relies on dest_check_data.is_volume_backed being present and dest_check_data.is_volume_backed is set in nova/compute/manager.py before the call to check_can_live_migrate_source.","commit_id":"059fc14fba9ad1c8526f33209d05be9ccaf0272e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cc9284031991acc84266f8ea109fc81be77314cf","unresolved":false,"context_lines":[{"line_number":5664,"context_line":"            return True"},{"line_number":5665,"context_line":""},{"line_number":5666,"context_line":"        if (dest_check_data.is_volume_backed and"},{"line_number":5667,"context_line":"                not bool(jsonutils.loads("},{"line_number":5668,"context_line":"                    self.get_instance_disk_info(instance,"},{"line_number":5669,"context_line":"                                                block_device_info)))):"},{"line_number":5670,"context_line":"            return True"},{"line_number":5671,"context_line":""},{"line_number":5672,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a71b18c_069603f0","side":"PARENT","line":5669,"range":{"start_line":5667,"start_character":0,"end_line":5669,"end_character":70},"updated":"2016-12-02 10:58:15.000000000","message":"Probably, but eww....\n\nThis translates into: if the list of disks returned by get_instance_disk_info() is empty. get_instance_disk_info returns all disks attached to a libvirt domain which:\n\n* Have a path element (so not \u0027local\u0027 rbd disks)\n* Have a type of \u0027file\u0027 or \u0027block\u0027\n* Whose (stay with me here) target device attribute (or directory name in the case of a filesystem) corresponds to the notional device name assigned to the block device when it was created in compute manager.\n\nI\u0027m prepared to believe this corresponds to \u0027all local disks what we care about in this instance\u0027. For example, it won\u0027t return Rbd disks, but we already excluded those further because they return is_shared_block_storage()\u003dTrue. They\u0027re still local disks, though, and there\u0027s no comment explaining that the correlation isn\u0027t important here. I have just spent a few minutes tracing the code I haven\u0027t convinced myself there are no corner cases here. I could probably do it, but I\u0027d have to lie down afterwards. Also, I\u0027d need a whiteboard.","commit_id":"5d1b4c8ce653b86db45b4f53ae3573e0c81d5a83"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d5746d5e7a59aacc006e12125a75e6dd847609fc","unresolved":false,"context_lines":[{"line_number":5664,"context_line":"            return True"},{"line_number":5665,"context_line":""},{"line_number":5666,"context_line":"        if (dest_check_data.is_volume_backed and"},{"line_number":5667,"context_line":"                not bool(jsonutils.loads("},{"line_number":5668,"context_line":"                    self.get_instance_disk_info(instance,"},{"line_number":5669,"context_line":"                                                block_device_info)))):"},{"line_number":5670,"context_line":"            return True"},{"line_number":5671,"context_line":""},{"line_number":5672,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a71b18c_4ea482a5","side":"PARENT","line":5669,"range":{"start_line":5667,"start_character":0,"end_line":5669,"end_character":70},"in_reply_to":"3a71b18c_069603f0","updated":"2016-12-02 16:02:38.000000000","message":"Based on this, it sounds like it would be better to keep _has_local_disk and call it here instead. What do you think?","commit_id":"5d1b4c8ce653b86db45b4f53ae3573e0c81d5a83"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ec717740b001b1af7c543cabf2d811fed8df5e9c","unresolved":false,"context_lines":[{"line_number":5664,"context_line":"            return True"},{"line_number":5665,"context_line":""},{"line_number":5666,"context_line":"        if (dest_check_data.is_volume_backed and"},{"line_number":5667,"context_line":"                not bool(jsonutils.loads("},{"line_number":5668,"context_line":"                    self.get_instance_disk_info(instance,"},{"line_number":5669,"context_line":"                                                block_device_info)))):"},{"line_number":5670,"context_line":"            return True"},{"line_number":5671,"context_line":""},{"line_number":5672,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a71b18c_69df1f69","side":"PARENT","line":5669,"range":{"start_line":5667,"start_character":0,"end_line":5669,"end_character":70},"in_reply_to":"3a71b18c_23ce785a","updated":"2016-12-06 01:14:51.000000000","message":"Okay, thanks for that.","commit_id":"5d1b4c8ce653b86db45b4f53ae3573e0c81d5a83"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"6a933719ce6912b6cdb58c8179abd8920df4d2a5","unresolved":false,"context_lines":[{"line_number":5664,"context_line":"            return True"},{"line_number":5665,"context_line":""},{"line_number":5666,"context_line":"        if (dest_check_data.is_volume_backed and"},{"line_number":5667,"context_line":"                not bool(jsonutils.loads("},{"line_number":5668,"context_line":"                    self.get_instance_disk_info(instance,"},{"line_number":5669,"context_line":"                                                block_device_info)))):"},{"line_number":5670,"context_line":"            return True"},{"line_number":5671,"context_line":""},{"line_number":5672,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a71b18c_23ce785a","side":"PARENT","line":5669,"range":{"start_line":5667,"start_character":0,"end_line":5669,"end_character":70},"in_reply_to":"3a71b18c_4ea482a5","updated":"2016-12-02 17:41:41.000000000","message":"_has_local_disk works with the same result of get_instance_disk_info. If no disks are find here, _has_local_disk will never return True.\n\nMoreover, _has_local_disk works wrong, because it checks for disk.local, but disk.ephN is used in fact.","commit_id":"5d1b4c8ce653b86db45b4f53ae3573e0c81d5a83"}]}