)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"e5ce8fc363416b2bfee26825bc16eeeee6e70964","unresolved":false,"context_lines":[{"line_number":6720,"context_line":"        timeout \u003d CONF.running_deleted_instance_timeout"},{"line_number":6721,"context_line":"        filters \u003d {\u0027deleted\u0027: True,"},{"line_number":6722,"context_line":"                   \u0027soft_deleted\u0027: False,"},{"line_number":6723,"context_line":"                   \u0027host\u0027: self.host}"},{"line_number":6724,"context_line":"        instances \u003d self._get_instances_on_driver(context, filters)"},{"line_number":6725,"context_line":"        return [i for i in instances if self._deleted_old_enough(i, timeout)]"},{"line_number":6726,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9f436f4f_c4dc521a","side":"PARENT","line":6723,"updated":"2017-08-09 07:02:27.000000000","message":"This will not receive all instances, because _get_instance_on_driver will apply instance_uuids filter on it.","commit_id":"2765f041b990a8eb6d1de309d62deca698e67705"},{"author":{"_account_id":2750,"name":"Sean Dague","email":"sean@dague.net","username":"sdague"},"change_message_id":"f4b9aeea8be245225aff5b46823fc5bdaf8fd31f","unresolved":false,"context_lines":[{"line_number":6720,"context_line":"        timeout \u003d CONF.running_deleted_instance_timeout"},{"line_number":6721,"context_line":"        filters \u003d {\u0027deleted\u0027: True,"},{"line_number":6722,"context_line":"                   \u0027soft_deleted\u0027: False}"},{"line_number":6723,"context_line":"        instances \u003d self._get_instances_on_driver(context, filters)"},{"line_number":6724,"context_line":"        return [i for i in instances if self._deleted_old_enough(i, timeout)]"},{"line_number":6725,"context_line":""},{"line_number":6726,"context_line":"    def _deleted_old_enough(self, instance, timeout):"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f436f4f_dccaa76c","line":6723,"updated":"2017-08-10 11:49:25.000000000","message":"ok, _get_instances_on_driver should be retaining this as a shared nothing sharded problem, as the virt driver returns only instances it thinks it owns.","commit_id":"46cef77eb82e544ad1e4e6126579f1066ab1311a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"52918b71ac264f2da2c9f4b8be98467c9db1f11e","unresolved":false,"context_lines":[{"line_number":6720,"context_line":"        timeout \u003d CONF.running_deleted_instance_timeout"},{"line_number":6721,"context_line":"        filters \u003d {\u0027deleted\u0027: True,"},{"line_number":6722,"context_line":"                   \u0027soft_deleted\u0027: False,"},{"line_number":6723,"context_line":"                   \u0027host\u0027: self.host}"},{"line_number":6724,"context_line":"        instances \u003d self._get_instances_on_driver(context, filters)"},{"line_number":6725,"context_line":"        return [i for i in instances if self._deleted_old_enough(i, timeout)]"},{"line_number":6726,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9f436f4f_4bc30658","side":"PARENT","line":6723,"updated":"2017-08-16 08:09:11.000000000","message":"OK, so I looked deeper into this...\n\nThere are apparently virt drivers that do not support listing instances by UUID. For these drivers, we still need to filter the InstanceList.get_by_filters() to the host.\n\nSo, my recommendation is to add the host filter *back* to the filters here:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L612\n\nfor those drivers that do not support listing by instance UUID.","commit_id":"2765f041b990a8eb6d1de309d62deca698e67705"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ad001d95fb4c65b84b0f85ce834cde96252ade31","unresolved":false,"context_lines":[{"line_number":611,"context_line":"        # to brute force."},{"line_number":612,"context_line":"        driver_instances \u003d self.driver.list_instances()"},{"line_number":613,"context_line":"        # NOTE(mjozefcz): In this case we need to apply host filter."},{"line_number":614,"context_line":"        # Without this all instance data would be get from db."},{"line_number":615,"context_line":"        filters[\u0027host\u0027] \u003d self.host"},{"line_number":616,"context_line":"        instances \u003d objects.InstanceList.get_by_filters(context, filters,"},{"line_number":617,"context_line":"                                                        use_slave\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9f436f4f_7cb329ea","line":614,"range":{"start_line":614,"start_character":47,"end_line":614,"end_character":62},"updated":"2017-08-16 13:40:52.000000000","message":"s/be get from db/be fetched from db/","commit_id":"739c224426c362ee6beb9941241b85669c30ce7c"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e7540cfadc898d6862b0f3fe68dc5263d9de68dc","unresolved":false,"context_lines":[{"line_number":6678,"context_line":"    def test_running_deleted_instances_instance_from_other_host("},{"line_number":6679,"context_line":"            self, mock_is_older, mock_get):"},{"line_number":6680,"context_line":"        \"\"\"This test covers situation when orphaned instance is running on"},{"line_number":6681,"context_line":"           host A but it is deleted in nova with database adnotation that has"},{"line_number":6682,"context_line":"           resided on host B. It could happen when instance will be deleted"},{"line_number":6683,"context_line":"           during its migration to other host (bug #1285000)."},{"line_number":6684,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"9f436f4f_a61c926a","line":6681,"updated":"2017-08-09 13:12:45.000000000","message":"annotation?","commit_id":"69001875ff39a4d222c16c262afdab40837bd358"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e7540cfadc898d6862b0f3fe68dc5263d9de68dc","unresolved":false,"context_lines":[{"line_number":6680,"context_line":"        \"\"\"This test covers situation when orphaned instance is running on"},{"line_number":6681,"context_line":"           host A but it is deleted in nova with database adnotation that has"},{"line_number":6682,"context_line":"           resided on host B. It could happen when instance will be deleted"},{"line_number":6683,"context_line":"           during its migration to other host (bug #1285000)."},{"line_number":6684,"context_line":"        \"\"\""},{"line_number":6685,"context_line":"        admin_context \u003d context.get_admin_context()"},{"line_number":6686,"context_line":"        self.compute.host \u003d \u0027host\u0027"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f436f4f_8304e018","line":6683,"updated":"2017-08-09 13:12:45.000000000","message":"pep8 will likely complain about this formatting","commit_id":"69001875ff39a4d222c16c262afdab40837bd358"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e7540cfadc898d6862b0f3fe68dc5263d9de68dc","unresolved":false,"context_lines":[{"line_number":6687,"context_line":"        instance \u003d self._create_fake_instance_obj()"},{"line_number":6688,"context_line":"        instance.deleted \u003d True"},{"line_number":6689,"context_line":"        # NOTE(mjozefcz): Instance hypervisor from db side should be"},{"line_number":6690,"context_line":"        # diffrent than host from which method running_deleted_instances"},{"line_number":6691,"context_line":"        # is triggered."},{"line_number":6692,"context_line":"        instance.host \u003d \"other_host\""},{"line_number":6693,"context_line":"        now \u003d timeutils.utcnow()"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f436f4f_2622e2a5","line":6690,"updated":"2017-08-09 13:12:45.000000000","message":"different","commit_id":"69001875ff39a4d222c16c262afdab40837bd358"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"b88bcf50d29ea94b5d87eb030af3c31ad2fe4d77","unresolved":false,"context_lines":[{"line_number":6686,"context_line":"        self.compute.host \u003d \u0027host\u0027"},{"line_number":6687,"context_line":"        instance \u003d self._create_fake_instance_obj()"},{"line_number":6688,"context_line":"        instance.deleted \u003d True"},{"line_number":6689,"context_line":"        # NOTE(mjozefcz): Instance hypervisor from db side should be"},{"line_number":6690,"context_line":"        # different than host from which method running_deleted_instances"},{"line_number":6691,"context_line":"        # is triggered."},{"line_number":6692,"context_line":"        instance.host \u003d \"other_host\""},{"line_number":6693,"context_line":"        now \u003d timeutils.utcnow()"},{"line_number":6694,"context_line":"        instance.deleted_at \u003d now"},{"line_number":6695,"context_line":"        mock_get.return_value \u003d [instance]"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f436f4f_38cf76b1","line":6692,"range":{"start_line":6689,"start_character":0,"end_line":6692,"end_character":36},"updated":"2017-08-10 16:23:22.000000000","message":"You can remove all of this and the unit test would still succeed. This is because you\u0027re mocking out the return from _get_instances_on_driver() and _deleted_old_enough() doesn\u0027t look at the instance\u0027s host attribute at all.\n\nTherefore, you can remove this entire unit test because it isn\u0027t stressing any different code paths that the existing unit tests already are.","commit_id":"46cef77eb82e544ad1e4e6126579f1066ab1311a"},{"author":{"_account_id":20676,"name":"daniel.pawlik","display_name":"Daniel Pawlik","email":"dpawlik@redhat.com","username":"daniel.pawlik"},"change_message_id":"f76ee55c25250bc3a8e8aa7ad9bc9d4d64b7a71a","unresolved":false,"context_lines":[{"line_number":6686,"context_line":"        self.compute.host \u003d \u0027host\u0027"},{"line_number":6687,"context_line":"        instance \u003d self._create_fake_instance_obj()"},{"line_number":6688,"context_line":"        instance.deleted \u003d True"},{"line_number":6689,"context_line":"        # NOTE(mjozefcz): Instance hypervisor from db side should be"},{"line_number":6690,"context_line":"        # different than host from which method running_deleted_instances"},{"line_number":6691,"context_line":"        # is triggered."},{"line_number":6692,"context_line":"        instance.host \u003d \"other_host\""},{"line_number":6693,"context_line":"        now \u003d timeutils.utcnow()"},{"line_number":6694,"context_line":"        instance.deleted_at \u003d now"},{"line_number":6695,"context_line":"        mock_get.return_value \u003d [instance]"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f436f4f_f6e7ade3","line":6692,"range":{"start_line":6689,"start_character":0,"end_line":6692,"end_character":36},"in_reply_to":"9f436f4f_38cf76b1","updated":"2017-08-11 07:51:12.000000000","message":"According to comment made by Chris Dent, some test(s) should be done so PS owner has made such test. \nTBH I agree with you: multiple tests are testing the same part of code. \n\nSo what should be done here: remove such test or leave? :)","commit_id":"46cef77eb82e544ad1e4e6126579f1066ab1311a"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"db6b49aa9f1404175b43c47633f59504fc39f9c5","unresolved":false,"context_lines":[{"line_number":6689,"context_line":"        # NOTE(mjozefcz): Instance hypervisor from db side should be"},{"line_number":6690,"context_line":"        # different than host from which method running_deleted_instances"},{"line_number":6691,"context_line":"        # is triggered."},{"line_number":6692,"context_line":"        instance.host \u003d \"other_host\""},{"line_number":6693,"context_line":"        now \u003d timeutils.utcnow()"},{"line_number":6694,"context_line":"        instance.deleted_at \u003d now"},{"line_number":6695,"context_line":"        mock_get.return_value \u003d [instance]"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f436f4f_bbaea995","line":6692,"in_reply_to":"9f436f4f_f6e7ade3","updated":"2017-08-11 10:06:52.000000000","message":"In a conflict between what I suggest and what Jay suggests, it\u0027s usually better to go with what Jay says, at least if it is about nova. He is wrong about candy bars.\n\nHowever, in this case the problem is not that there shouldn\u0027t be an additional test, but rather that this new test does not test what needs to be tested. My advice was to make sure the change was covered.\n\nThis test doesn\u0027t add any coverage to the thing that was actually changed, it just confirms that the changed filter is getting used, by mocking everything.\n\nUnfortunately the other test doesn\u0027t even touch this code, so neither test actually covers the situation.\n\nI\u0027d argue that what this really needs is a functional test of some kind that has some real data behind the eventual call to objects.InstanceList.get_by_filters so that the bug that is being fixed is actually modelled.\n\nIf that\u0027s not going to be possible, then I would say remove both unit tests and see whether other reviewers are happy enough with the change by inspection.","commit_id":"46cef77eb82e544ad1e4e6126579f1066ab1311a"}],"nova/tests/unit/compute/test_compute_mgr.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e7540cfadc898d6862b0f3fe68dc5263d9de68dc","unresolved":false,"context_lines":[{"line_number":1434,"context_line":"            self, mock_instance_list):"},{"line_number":1435,"context_line":"        \"\"\"This test covers situation when some instances are running on host"},{"line_number":1436,"context_line":"           A but db shows that they were running on host B and they are"},{"line_number":1437,"context_line":"           deleted. More details - see bug: #1285000"},{"line_number":1438,"context_line":"        \"\"\""},{"line_number":1439,"context_line":"        self.compute.host \u003d \u0027host\u0027"},{"line_number":1440,"context_line":"        filters \u003d {\u0027deleted\u0027: True,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f436f4f_a635b2dc","line":1437,"updated":"2017-08-09 13:12:45.000000000","message":"same pep8 concern","commit_id":"69001875ff39a4d222c16c262afdab40837bd358"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"db6b49aa9f1404175b43c47633f59504fc39f9c5","unresolved":false,"context_lines":[{"line_number":1430,"context_line":"                         [x[\u0027uuid\u0027] for x in result])"},{"line_number":1431,"context_line":""},{"line_number":1432,"context_line":"    @mock.patch(\u0027nova.objects.InstanceList.get_by_filters\u0027)"},{"line_number":1433,"context_line":"    def test_get_instances_on_driver_deleted_instances_from_other_host("},{"line_number":1434,"context_line":"            self, mock_instance_list):"},{"line_number":1435,"context_line":"        \"\"\"This test covers situation when some instances are running on host"},{"line_number":1436,"context_line":"           A but db shows that they were running on host B and they are"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f436f4f_be5a1727","line":1433,"updated":"2017-08-11 10:06:52.000000000","message":"this test isn\u0027t really relevant to the fix made because it doesn\u0027t touch the changed code, as far as I can tell, so should be removed.","commit_id":"46cef77eb82e544ad1e4e6126579f1066ab1311a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"55657e50da7249f3fc7fd1595012cc76ef368f2d","unresolved":false,"context_lines":[{"line_number":1479,"context_line":"            mock_driver_uuids.side_effect \u003d NotImplementedError()"},{"line_number":1480,"context_line":"            mock_driver_instances.return_value \u003d driver_instance_names"},{"line_number":1481,"context_line":"            result \u003d self.compute._get_instances_on_driver(self.context,"},{"line_number":1482,"context_line":"                                                           filters)"},{"line_number":1483,"context_line":""},{"line_number":1484,"context_line":"        self.assertEqual([x[\u0027uuid\u0027] for x in driver_instances],"},{"line_number":1485,"context_line":"                         [x[\u0027uuid\u0027] for x in result])"}],"source_content_type":"text/x-python","patch_set":10,"id":"9f436f4f_03493e31","line":1482,"range":{"start_line":1482,"start_character":59,"end_line":1482,"end_character":66},"updated":"2017-08-17 14:43:26.000000000","message":"Here you\u0027re passing filters that already contains host\u003dhost, but you expect this method to _add_ it.","commit_id":"a6cd2460d3ec6c32aee4621edbd57a6682f23e03"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bab2c8611d8c293ccafd5ef5850f1d0db0bcfab4","unresolved":false,"context_lines":[{"line_number":1484,"context_line":"        self.assertEqual([x[\u0027uuid\u0027] for x in driver_instances],"},{"line_number":1485,"context_line":"                         [x[\u0027uuid\u0027] for x in result])"},{"line_number":1486,"context_line":"        mock_instance_list.assert_called_with(self.context, filters,"},{"line_number":1487,"context_line":"                                              use_slave\u003dTrue)"},{"line_number":1488,"context_line":""},{"line_number":1489,"context_line":"    def test_instance_usage_audit(self):"},{"line_number":1490,"context_line":"        instances \u003d [objects.Instance(uuid\u003duuids.instance)]"}],"source_content_type":"text/x-python","patch_set":10,"id":"9f436f4f_d5f40c8a","line":1487,"updated":"2017-08-17 13:51:15.000000000","message":"I could be wrong, but I don\u0027t see anywhere in here that you\u0027re asserting that filters looks like it should. Meaning, I don\u0027t see that you check that InstanceList.get_by_filters() is passed all three things, to make sure that your code to add it back in before this call gets run properly.","commit_id":"a6cd2460d3ec6c32aee4621edbd57a6682f23e03"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f6b51984eef494608185a3be503c49494a82eb20","unresolved":false,"context_lines":[{"line_number":1449,"context_line":"        # Test getting instances when driver doesn\u0027t support"},{"line_number":1450,"context_line":"        # \u0027list_instance_uuids\u0027"},{"line_number":1451,"context_line":"        self.compute.host \u003d \u0027host\u0027"},{"line_number":1452,"context_line":"        filters \u003d {}"},{"line_number":1453,"context_line":""},{"line_number":1454,"context_line":"        self.flags(instance_name_template\u003d\u0027inst-%i\u0027)"},{"line_number":1455,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"9f436f4f_5ed6f929","line":1452,"updated":"2017-08-17 15:20:39.000000000","message":"Probably better to just pass {} below instead of this intermediate variable, but not a big deal.","commit_id":"aba3f649323d328f8122d372dea38256dfed6a52"}]}