)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Matthew Booth \u003cmbooth@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2018-10-24 09:44:19 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Don\u0027t delete disks on shared storage during evacuate"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When evacuating an instance between compute hosts on shared storage,"},{"line_number":10,"context_line":"during the rebuild operation we call spawn() on the destination"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":17,"id":"3f79a3b5_193532a7","line":7,"updated":"2018-11-27 16:09:52.000000000","message":"nit: prefix with \"libvirt: \" since this is a libvirt-specific fix.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When evacuating an instance between compute hosts on shared storage,"},{"line_number":10,"context_line":"during the rebuild operation we call spawn() on the destination"},{"line_number":11,"context_line":"compute.  spawn() currently assumes that it should cleanup all"},{"line_number":12,"context_line":"resources on failure, which results in user data being deleted in the"},{"line_number":13,"context_line":"evacuate case."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This change modifies spawn in the libvirt driver such that it only"},{"line_number":16,"context_line":"cleans up resources it created."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":17,"id":"3f79a3b5_991702f0","line":13,"range":{"start_line":11,"start_character":10,"end_line":13,"end_character":14},"updated":"2018-11-27 16:09:52.000000000","message":"As in the libvirt driver spawn() method passes destroy_disks_on_failure\u003dTrue unconditionally:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/virt/libvirt/driver.py#L3075\n\nWhich gets passed to this:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/virt/libvirt/driver.py#L5642\n\nand then here:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/virt/libvirt/driver.py#L5578\n\nand then cleanup() destroys various disks regardless of shared storage because the driver told it to.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"49f09a8a65577a0fabdbd54621dbbb55fe83964c","unresolved":false,"context_lines":[{"line_number":18,"context_line":"Co-Authored-By: Lee Yarwood \u003clyarwood@redhat.com\u003e"},{"line_number":19,"context_line":"Closes-Bug: #1550919"},{"line_number":20,"context_line":"Change-Id: I764481966c96a67d993da6e902dc9fc3ad29ee36"},{"line_number":21,"context_line":"(cherry picked from commit 083df01a4d90e155c1b4ef9e954d8be14a02d830)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":33,"id":"ff570b3c_6a3903d7","line":21,"range":{"start_line":21,"start_character":0,"end_line":21,"end_character":68},"updated":"2020-06-09 14:28:33.000000000","message":"Noted in the backports, but this looks like a mistake introduced in PS30","commit_id":"497360b0ea970f1e68912be8229ef8c3f5454e9e"}],"nova/compute/manager.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"041160d0e286fe83ca73eb65ae891e73492cd22a","unresolved":false,"context_lines":[{"line_number":682,"context_line":"                    context, instance)"},{"line_number":683,"context_line":"                bdi \u003d self._get_instance_block_device_info(context,"},{"line_number":684,"context_line":"                                                           instance)"},{"line_number":685,"context_line":"                destroy_disks \u003d not (self._is_instance_storage_shared("},{"line_number":686,"context_line":"                    context, instance))"},{"line_number":687,"context_line":"            except exception.InstanceNotFound:"},{"line_number":688,"context_line":"                network_info \u003d network_model.NetworkInfo()"},{"line_number":689,"context_line":"                bdi \u003d {}"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_085e4278","line":686,"range":{"start_line":685,"start_character":0,"end_line":686,"end_character":39},"updated":"2018-11-09 11:05:00.000000000","message":"In my use case destroy_disks is set to False when the rbd imagebackend is used here.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"041160d0e286fe83ca73eb65ae891e73492cd22a","unresolved":false,"context_lines":[{"line_number":692,"context_line":"                         instance\u003dinstance)"},{"line_number":693,"context_line":"                # always destroy disks if the instance was deleted"},{"line_number":694,"context_line":"                destroy_disks \u003d True"},{"line_number":695,"context_line":"            self.driver.destroy(context, instance,"},{"line_number":696,"context_line":"                                network_info,"},{"line_number":697,"context_line":"                                bdi, destroy_disks)"},{"line_number":698,"context_line":""},{"line_number":699,"context_line":"            # delete the allocation of the evacuated instance from this host"},{"line_number":700,"context_line":"            if migration.source_node not in compute_nodes:"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_c8536a6d","line":697,"range":{"start_line":695,"start_character":0,"end_line":697,"end_character":51},"updated":"2018-11-09 11:05:00.000000000","message":"destroy_disks is then passed to destroy and then cleanup here.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"}],"nova/tests/functional/libvirt/test_evacuate.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"51f9021e0f47016028d3991614f1906c52268faf","unresolved":false,"context_lines":[{"line_number":142,"context_line":"            self.source_instance_path(server), disk)"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        self.assertTrue(os.path.exists(source_root_disk),"},{"line_number":145,"context_line":"                        \"Source root disk %s for server %s does not exist\" %"},{"line_number":146,"context_line":"                        (source_root_disk, name))"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":""}],"source_content_type":"text/x-python","patch_set":32,"id":"ff570b3c_fc618cd4","line":145,"range":{"start_line":145,"start_character":59,"end_line":145,"end_character":73},"updated":"2020-05-21 16:32:55.000000000","message":"should not have been deleted ?\n\nor just put a comment above this explaining why this check is here, like you did in the last modified check in the file","commit_id":"ee1447ed440e6ea3a300489caec7af240524adb1"}],"nova/tests/functional/regressions/test_bug_1550919.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                                        \u0027disk\u0027)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.assertTrue(os.path.exists(source_root_disk),"},{"line_number":70,"context_line":"                        \"Source root disk %s exists\" % source_root_disk)"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"# NOTE(mdbooth): The Rbd tests also need to mock the flat backend, because the"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_bc60490e","line":70,"range":{"start_line":70,"start_character":23,"end_line":70,"end_character":52},"updated":"2018-10-19 14:33:47.000000000","message":"negate it as it is printed when the path does not exists","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"aa6a2c2d1947c2c1ab8e5f76e5f7da54441596e0","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                                        \u0027disk\u0027)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.assertTrue(os.path.exists(source_root_disk),"},{"line_number":70,"context_line":"                        \"Source root disk %s exists\" % source_root_disk)"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"# NOTE(mdbooth): The Rbd tests also need to mock the flat backend, because the"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_9ac0c92d","line":70,"range":{"start_line":70,"start_character":23,"end_line":70,"end_character":52},"in_reply_to":"3f79a3b5_bc60490e","updated":"2018-10-23 14:33:10.000000000","message":"Done","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        # Check that the instance directory still exists"},{"line_number":301,"context_line":"        self.assertTrue(os.path.exists(shared_instance_path),"},{"line_number":302,"context_line":"                        \"Shared instance directory %s exists\" %"},{"line_number":303,"context_line":"                        shared_instance_path)"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"        self.assert_disks_shared_instancedir(server)"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_7c6ab1ec","line":302,"range":{"start_line":302,"start_character":24,"end_line":302,"end_character":62},"updated":"2018-10-19 14:33:47.000000000","message":"negate it","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"aa6a2c2d1947c2c1ab8e5f76e5f7da54441596e0","unresolved":false,"context_lines":[{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        # Check that the instance directory still exists"},{"line_number":301,"context_line":"        self.assertTrue(os.path.exists(shared_instance_path),"},{"line_number":302,"context_line":"                        \"Shared instance directory %s exists\" %"},{"line_number":303,"context_line":"                        shared_instance_path)"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"        self.assert_disks_shared_instancedir(server)"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_bad025f6","line":302,"range":{"start_line":302,"start_character":24,"end_line":302,"end_character":62},"in_reply_to":"3f79a3b5_7c6ab1ec","updated":"2018-10-23 14:33:10.000000000","message":"Done","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":111,"context_line":"        # Check that we created a root disk and haven\u0027t called _cleanup_rbd at"},{"line_number":112,"context_line":"        # all"},{"line_number":113,"context_line":"        self.assertIn(\"%s_disk\" % server[\u0027id\u0027], self.created)"},{"line_number":114,"context_line":"        self.assertEqual(0, self.mock_cleanup_rbd.call_count)"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"    # We never want to cleanup rbd disks during evacuate, regardless of"},{"line_number":117,"context_line":"    # instance shared storage"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_59bbaadc","line":114,"range":{"start_line":114,"start_character":8,"end_line":114,"end_character":61},"updated":"2018-11-27 16:09:52.000000000","message":"nit: I prefer self.mock_cleanup_rbd.assert_not_called()","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":111,"context_line":"        # Check that we created a root disk and haven\u0027t called _cleanup_rbd at"},{"line_number":112,"context_line":"        # all"},{"line_number":113,"context_line":"        self.assertIn(\"%s_disk\" % server[\u0027id\u0027], self.created)"},{"line_number":114,"context_line":"        self.assertEqual(0, self.mock_cleanup_rbd.call_count)"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"    # We never want to cleanup rbd disks during evacuate, regardless of"},{"line_number":117,"context_line":"    # instance shared storage"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_4f8f2e21","line":114,"range":{"start_line":114,"start_character":8,"end_line":114,"end_character":61},"in_reply_to":"3f79a3b5_59bbaadc","updated":"2018-11-29 14:17:27.000000000","message":"Done","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"2dbbfd4f271dd8ead287f913391995627234ed00","unresolved":false,"context_lines":[{"line_number":16983,"context_line":"                                                 mock.sentinel.cleanup)"},{"line_number":16984,"context_line":"            # destroy_disks_on_failure\u003dTrue, used only by spawn()"},{"line_number":16985,"context_line":"            mock_cleanup.reset_mock()"},{"line_number":16986,"context_line":"            self.assertRaises(test.TestingException,"},{"line_number":16987,"context_line":"                              drvr._create_domain_and_network,"},{"line_number":16988,"context_line":"                              self.context, \u0027xml\u0027, instance, [], None,"},{"line_number":16989,"context_line":"                              cleanup_on_failure\u003dmock.sentinel.cleanup)"},{"line_number":16990,"context_line":"            mock_cleanup.assert_called_once_with(self.context, instance,"},{"line_number":16991,"context_line":"                                                 [], None, None,"},{"line_number":16992,"context_line":"                                                 mock.sentinel.cleanup)"},{"line_number":16993,"context_line":""},{"line_number":16994,"context_line":"        the_test()"},{"line_number":16995,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_5405ddc0","line":16992,"range":{"start_line":16986,"start_character":1,"end_line":16992,"end_character":71},"updated":"2018-08-23 21:15:22.000000000","message":"Am I missing something?  As far as I can tell this is exactly the same as the previous test within the same function.\n\nThe previous version of the test was ensuring that _cleanup_failed_start() would be called with \"destroy_disks\u003dTrue\" if _create_domain_and_network() was called with destroy_disks_on_failure\u003dTrue.  I think the equivalent test would be","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ab96766cb7b250ff2d38df41b10bdf2e73ac5aef","unresolved":false,"context_lines":[{"line_number":16983,"context_line":"                                                 mock.sentinel.cleanup)"},{"line_number":16984,"context_line":"            # destroy_disks_on_failure\u003dTrue, used only by spawn()"},{"line_number":16985,"context_line":"            mock_cleanup.reset_mock()"},{"line_number":16986,"context_line":"            self.assertRaises(test.TestingException,"},{"line_number":16987,"context_line":"                              drvr._create_domain_and_network,"},{"line_number":16988,"context_line":"                              self.context, \u0027xml\u0027, instance, [], None,"},{"line_number":16989,"context_line":"                              cleanup_on_failure\u003dmock.sentinel.cleanup)"},{"line_number":16990,"context_line":"            mock_cleanup.assert_called_once_with(self.context, instance,"},{"line_number":16991,"context_line":"                                                 [], None, None,"},{"line_number":16992,"context_line":"                                                 mock.sentinel.cleanup)"},{"line_number":16993,"context_line":""},{"line_number":16994,"context_line":"        the_test()"},{"line_number":16995,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_f51419eb","line":16992,"range":{"start_line":16986,"start_character":1,"end_line":16992,"end_character":71},"in_reply_to":"3f79a3b5_5405ddc0","updated":"2018-08-24 10:51:30.000000000","message":"Good spot, the duplication isn\u0027t relevant. FWIW, it could equally have been a sentinel before. I\u0027ll clean this up when I inevitably respin it.","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"2d28cea53654208539d1a172090d5cd1c7786054","unresolved":false,"context_lines":[{"line_number":18843,"context_line":"                              self.context, \u0027xml\u0027, instance, [], None)"},{"line_number":18844,"context_line":"            mock_cleanup.assert_called_once_with(self.context, instance,"},{"line_number":18845,"context_line":"                                                 [], None, None, False)"},{"line_number":18846,"context_line":"            # destroy_disks_on_failure\u003dTrue, used only by spawn()"},{"line_number":18847,"context_line":"            mock_cleanup.reset_mock()"},{"line_number":18848,"context_line":"            self.assertRaises(test.TestingException,"},{"line_number":18849,"context_line":"                              drvr._create_domain_and_network,"},{"line_number":18850,"context_line":"                              self.context, \u0027xml\u0027, instance, [], None,"},{"line_number":18851,"context_line":"                              destroy_disks_on_failure\u003dTrue)"},{"line_number":18852,"context_line":"            mock_cleanup.assert_called_once_with(self.context, instance,"},{"line_number":18853,"context_line":"                                                 [], None, None, True)"},{"line_number":18854,"context_line":""},{"line_number":18855,"context_line":"        the_test()"},{"line_number":18856,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"ff570b3c_e5057647","side":"PARENT","line":18853,"range":{"start_line":18846,"start_character":0,"end_line":18853,"end_character":70},"updated":"2020-05-21 11:26:03.000000000","message":"We\u0027ve removed destroy_disks_on_failure so we can remove this test also.","commit_id":"cbbd36aeacb19cab20c55ca3f727f452b4e0565e"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"2d28cea53654208539d1a172090d5cd1c7786054","unresolved":false,"context_lines":[{"line_number":18847,"context_line":"                self.context, instance, [], None, None,"},{"line_number":18848,"context_line":"                cleanup_instance_dir\u003dmock.sentinel.cleanup_instance_dir,"},{"line_number":18849,"context_line":"                cleanup_instance_disks\u003dmock.sentinel.cleanup_instance_disks)"},{"line_number":18850,"context_line":"            # destroy_disks_on_failure\u003dTrue, used only by spawn()"},{"line_number":18851,"context_line":"            mock_cleanup.reset_mock()"},{"line_number":18852,"context_line":"            self.assertRaises("},{"line_number":18853,"context_line":"                test.TestingException, drvr._create_domain_and_network,"},{"line_number":18854,"context_line":"                self.context, \u0027xml\u0027, instance, [], None,"},{"line_number":18855,"context_line":"                cleanup_instance_dir\u003dmock.sentinel.cleanup_instance_dir,"},{"line_number":18856,"context_line":"                cleanup_instance_disks\u003dmock.sentinel.cleanup_instance_disks)"},{"line_number":18857,"context_line":"            mock_cleanup.assert_called_once_with("},{"line_number":18858,"context_line":"                self.context, instance, [], None, None,"},{"line_number":18859,"context_line":"                cleanup_instance_dir\u003dmock.sentinel.cleanup_instance_dir,"},{"line_number":18860,"context_line":"                cleanup_instance_disks\u003dmock.sentinel.cleanup_instance_disks)"},{"line_number":18861,"context_line":""},{"line_number":18862,"context_line":"        the_test()"},{"line_number":18863,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"ff570b3c_25fc8e3c","line":18860,"range":{"start_line":18850,"start_character":0,"end_line":18860,"end_character":76},"updated":"2020-05-21 11:26:03.000000000","message":"This is just a duplicate.","commit_id":"a3b9fdaa3abe8ddcecd3c4f769480afa065e7172"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"a7c50bc769d31915d1ea45718fb28e29c22b99c5","unresolved":false,"context_lines":[{"line_number":3046,"context_line":"        # NOTE(mdbooth): This code was cut/paste from"},{"line_number":3047,"context_line":"        # ComputeManager._rebuild_default_impl. If changing this code, please"},{"line_number":3048,"context_line":"        # consider if the changes would apply to other drivers."},{"line_number":3049,"context_line":"        if preserve_ephemeral:"},{"line_number":3050,"context_line":"            raise exception.PreserveEphemeralNotSupported()"},{"line_number":3051,"context_line":""},{"line_number":3052,"context_line":"        destroy_disks_on_failure \u003d True"},{"line_number":3053,"context_line":"        if recreate:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_a26fe0f8","line":3050,"range":{"start_line":3049,"start_character":8,"end_line":3050,"end_character":59},"updated":"2018-06-28 17:17:02.000000000","message":"in the case of remote storage (nfs, ceph) couldn\u0027t we actually preserve the ephemeral devices?","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"130b4e0200e3c6343f2e424be7e36454105dcff2","unresolved":false,"context_lines":[{"line_number":3046,"context_line":"        # NOTE(mdbooth): This code was cut/paste from"},{"line_number":3047,"context_line":"        # ComputeManager._rebuild_default_impl. If changing this code, please"},{"line_number":3048,"context_line":"        # consider if the changes would apply to other drivers."},{"line_number":3049,"context_line":"        if preserve_ephemeral:"},{"line_number":3050,"context_line":"            raise exception.PreserveEphemeralNotSupported()"},{"line_number":3051,"context_line":""},{"line_number":3052,"context_line":"        destroy_disks_on_failure \u003d True"},{"line_number":3053,"context_line":"        if recreate:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_7bf9654c","line":3050,"range":{"start_line":3049,"start_character":8,"end_line":3050,"end_character":59},"in_reply_to":"5f7c97a3_a26fe0f8","updated":"2018-06-29 09:05:23.000000000","message":"Yes, and we probably have enough information here to do it.\n\nNot going to tackle it here, though, or even in an immediate follow up. Priority will be to kill the code duplication.","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"98317d9e5d0dae8c9c090f18560c3e76781b1ddb","unresolved":false,"context_lines":[{"line_number":3070,"context_line":"            # the instance directory we see is the one owned by the compute"},{"line_number":3071,"context_line":"            # host we\u0027re evacuating from, which would allow us to be a bit more"},{"line_number":3072,"context_line":"            # robust here."},{"line_number":3073,"context_line":"            if self.instance_on_disk(instance):"},{"line_number":3074,"context_line":"                destroy_disks_on_failure \u003d False"},{"line_number":3075,"context_line":"        else:"},{"line_number":3076,"context_line":"            timeout, retry_interval \u003d compute_utils.get_power_off_values("}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_f1a436a6","line":3073,"range":{"start_line":3073,"start_character":15,"end_line":3073,"end_character":46},"updated":"2018-07-18 17:19:04.000000000","message":"Note: as mentioned on a similar change around rebuild [1], this method will return True even in the case of non-shared if it\u0027s a rebuild (same host).\n\n[1] https://review.openstack.org/#/c/288109/1/nova/virt/libvirt/driver.py@4898","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"61fb79346f127a3e8d56a24133a7b3fdb7f82575","unresolved":false,"context_lines":[{"line_number":3070,"context_line":"            # the instance directory we see is the one owned by the compute"},{"line_number":3071,"context_line":"            # host we\u0027re evacuating from, which would allow us to be a bit more"},{"line_number":3072,"context_line":"            # robust here."},{"line_number":3073,"context_line":"            if self.instance_on_disk(instance):"},{"line_number":3074,"context_line":"                destroy_disks_on_failure \u003d False"},{"line_number":3075,"context_line":"        else:"},{"line_number":3076,"context_line":"            timeout, retry_interval \u003d compute_utils.get_power_off_values("}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_a215f7e6","line":3073,"range":{"start_line":3073,"start_character":15,"end_line":3073,"end_character":46},"in_reply_to":"3f79a3b5_227247ae","updated":"2018-10-19 15:37:35.000000000","message":"OK, looks like I was led astray by the word \"recreate\" on L3053. I see now after rebasing that it\u0027s been changed to be more obvious: \"evacuate\".\n\nAnd OK,","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"9089daf44644ce98da27757b876b25bc26b68809","unresolved":false,"context_lines":[{"line_number":3070,"context_line":"            # the instance directory we see is the one owned by the compute"},{"line_number":3071,"context_line":"            # host we\u0027re evacuating from, which would allow us to be a bit more"},{"line_number":3072,"context_line":"            # robust here."},{"line_number":3073,"context_line":"            if self.instance_on_disk(instance):"},{"line_number":3074,"context_line":"                destroy_disks_on_failure \u003d False"},{"line_number":3075,"context_line":"        else:"},{"line_number":3076,"context_line":"            timeout, retry_interval \u003d compute_utils.get_power_off_values("}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_227247ae","line":3073,"range":{"start_line":3073,"start_character":15,"end_line":3073,"end_character":46},"in_reply_to":"5f7c97a3_f1a436a6","updated":"2018-08-14 16:20:38.000000000","message":"This case is only for evacuate (it\u0027s guarded on line 3053), which is never same host.\n\nTo answer the question on the other patch about if we should ever cleanup a failed rebuild: absolutely yes. This causes problems for future migrations which get scheduled here.","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"40e422bf189e381fb950a85b5610a48a8e1299ab","unresolved":false,"context_lines":[{"line_number":3099,"context_line":""},{"line_number":3100,"context_line":"    def spawn(self, context, instance, image_meta, injected_files,"},{"line_number":3101,"context_line":"              admin_password, allocations, network_info\u003dNone,"},{"line_number":3102,"context_line":"              block_device_info\u003dNone):"},{"line_number":3103,"context_line":"        return self._spawn(context, instance, image_meta, injected_files,"},{"line_number":3104,"context_line":"                  admin_password, allocations, network_info\u003dnetwork_info,"},{"line_number":3105,"context_line":"                  block_device_info\u003dblock_device_info)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_64026b0a","line":3102,"updated":"2018-07-20 14:16:57.000000000","message":"I also wanted to note, also from the IRC discussion from the other day, if we were to add a destroy_disks_on_failure kwarg to the spawn method, we\u0027d do that only on master as a follow up change and only the task_state-based change would be backported.","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"98317d9e5d0dae8c9c090f18560c3e76781b1ddb","unresolved":false,"context_lines":[{"line_number":3135,"context_line":"            context, xml, instance, network_info,"},{"line_number":3136,"context_line":"            block_device_info\u003dblock_device_info,"},{"line_number":3137,"context_line":"            post_xml_callback\u003dgen_confdrive,"},{"line_number":3138,"context_line":"            destroy_disks_on_failure\u003ddestroy_disks_on_failure)"},{"line_number":3139,"context_line":"        LOG.debug(\"Guest created on hypervisor\", instance\u003dinstance)"},{"line_number":3140,"context_line":""},{"line_number":3141,"context_line":"        def _wait_for_boot():"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_91b5c27f","line":3138,"updated":"2018-07-18 17:19:04.000000000","message":"As discussed on IRC today, we could use the instance.task_state to know whether we are doing a rebuild and set destroy_disks_on_failure accordingly. It seems to me though, that we shouldn\u0027t destroy the disk if rebuild fails, shared or not. If rebuild didn\u0027t complete, shouldn\u0027t the disk remain?\n\nSo, I\u0027m wondering if in spawn(), we should just have:\n\n destroy_disks_on_failure \u003d instance.task_state !\u003d task_states.REBUILD_SPAWNING","commit_id":"c9d7f7d8a3fedaf60d52a35d5c8034ec4129ae49"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"92cb1ada2b00886165c6ebda473c98f43216bf9e","unresolved":false,"context_lines":[{"line_number":1006,"context_line":"        else:"},{"line_number":1007,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1008,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1009,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1010,"context_line":"            # this is wrong."},{"line_number":1011,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1012,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1013,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_a7da4de6","line":1010,"range":{"start_line":1009,"start_character":60,"end_line":1010,"end_character":28},"updated":"2018-08-21 19:03:42.000000000","message":"what do you think is wrong about it?  If you\u0027ve got rbd instance disks but a local directory (for the xml and console log for example) don\u0027t you need to delete the directory?","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"100de5bdbd246f05ce0e1f3782f7299ea9315974","unresolved":false,"context_lines":[{"line_number":1006,"context_line":"        else:"},{"line_number":1007,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1008,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1009,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1010,"context_line":"            # this is wrong."},{"line_number":1011,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1012,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1013,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_c2737a4d","line":1010,"range":{"start_line":1009,"start_character":60,"end_line":1010,"end_character":28},"in_reply_to":"3f79a3b5_a7da4de6","updated":"2018-08-23 14:06:21.000000000","message":"I think it\u0027s wrong because we might have both shared block and instance storage. It\u0027s weird that we\u0027re using \u0027not is_shared_block_storage\u0027 as a proxy when we actually have \u0027is_shared_instance_storage\u0027 available.\n\nI decided not to change it here, though, to minimise this change. This is just code motion.","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"03a2698ba0807b854e168e7cbf14638ef05d210b","unresolved":false,"context_lines":[{"line_number":1071,"context_line":"                self._disconnect_volume(context, connection_info, instance)"},{"line_number":1072,"context_line":"            except Exception as exc:"},{"line_number":1073,"context_line":"                with excutils.save_and_reraise_exception() as ctxt:"},{"line_number":1074,"context_line":"                    if cleanup_resources and cleanup_resources.disks:"},{"line_number":1075,"context_line":"                        # Don\u0027t block on Volume errors if we\u0027re trying to"},{"line_number":1076,"context_line":"                        # delete the instance as we may be partially created"},{"line_number":1077,"context_line":"                        # or deleted"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_7ca8948b","line":1074,"updated":"2018-08-29 14:39:57.000000000","message":"The original call was cleanup(destroy_disks\u003dfoo).\n\nif destroy_disks\u003dTrue:\n\nOn (new) line 1005 we set cleanup_resources.disks to True in this case, so this is True -\u003e behaviour is the same\n\nif destroy_disks\u003dFalse:\n\nOn (new) line 1015 we set cleanup_resources.disks to False, so this is False -\u003e behaviour is the same.","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"03a2698ba0807b854e168e7cbf14638ef05d210b","unresolved":false,"context_lines":[{"line_number":1083,"context_line":"                             \u0027exc\u0027: encodeutils.exception_to_unicode(exc)},"},{"line_number":1084,"context_line":"                            instance\u003dinstance)"},{"line_number":1085,"context_line":""},{"line_number":1086,"context_line":"        if cleanup_resources and cleanup_resources.disks:"},{"line_number":1087,"context_line":"            # NOTE(haomai): destroy volumes if needed"},{"line_number":1088,"context_line":"            if CONF.libvirt.images_type \u003d\u003d \u0027lvm\u0027:"},{"line_number":1089,"context_line":"                self._cleanup_lvm(instance, block_device_info)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_5c8fd82d","line":1086,"updated":"2018-08-29 14:39:57.000000000","message":"See above: destroy_disks is maintained.","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"03a2698ba0807b854e168e7cbf14638ef05d210b","unresolved":false,"context_lines":[{"line_number":1090,"context_line":"            if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":1091,"context_line":"                self._cleanup_rbd(instance)"},{"line_number":1092,"context_line":""},{"line_number":1093,"context_line":"        if cleanup_resources and cleanup_resources.instance_dir:"},{"line_number":1094,"context_line":"            attempts \u003d int(instance.system_metadata.get(\u0027clean_attempts\u0027,"},{"line_number":1095,"context_line":"                                                        \u00270\u0027))"},{"line_number":1096,"context_line":"            success \u003d self.delete_instance_files(instance)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_9c02506f","line":1093,"updated":"2018-08-29 14:39:57.000000000","message":"Original call was cleanup(destroy_disks\u003dfoo, migrate_data\u003dbar).\n\nif destroy_disks\u003dTrue:\n\nOn (new) line 1005 we set cleanup_resources.instance_dir to True -\u003e behaviour is the same\n\nif destroy_disks\u003dFalse:\n\nOn (new) line 1012-1013 we calculate cleanup_resources.instance_dir using the code moved from (old) line 1067-1068 -\u003e behaviour is the same.","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"2dbbfd4f271dd8ead287f913391995627234ed00","unresolved":false,"context_lines":[{"line_number":3470,"context_line":"            nova.privsep.path.chown(image(\u0027disk\u0027).path, uid\u003duid)"},{"line_number":3471,"context_line":""},{"line_number":3472,"context_line":"        created_disks \u003d self._create_and_inject_local_root("},{"line_number":3473,"context_line":"                context, instance, booted_from_volume, created_disks,"},{"line_number":3474,"context_line":"                suffix, disk_images, injection_info, fallback_from_host)"},{"line_number":3475,"context_line":""},{"line_number":3476,"context_line":"        # Lookup the filesystem type if required"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_f4f6698f","line":3473,"range":{"start_line":3473,"start_character":55,"end_line":3473,"end_character":68},"updated":"2018-08-23 21:15:22.000000000","message":"_create_and_inject_local_root() doesn\u0027t actually need the previous value of \"created_disks\" so I think it\u0027d be cleaner if we didn\u0027t pass it in.  Instead, we could handle the logic here:\n\ntmp_created_disks \u003d self._create_and_inject_local_root(....)\ncreated_disks \u003d created_disks or tmp_created_disks","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ab96766cb7b250ff2d38df41b10bdf2e73ac5aef","unresolved":false,"context_lines":[{"line_number":3470,"context_line":"            nova.privsep.path.chown(image(\u0027disk\u0027).path, uid\u003duid)"},{"line_number":3471,"context_line":""},{"line_number":3472,"context_line":"        created_disks \u003d self._create_and_inject_local_root("},{"line_number":3473,"context_line":"                context, instance, booted_from_volume, created_disks,"},{"line_number":3474,"context_line":"                suffix, disk_images, injection_info, fallback_from_host)"},{"line_number":3475,"context_line":""},{"line_number":3476,"context_line":"        # Lookup the filesystem type if required"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_958a8518","line":3473,"range":{"start_line":3473,"start_character":55,"end_line":3473,"end_character":68},"in_reply_to":"3f79a3b5_f4f6698f","updated":"2018-08-24 10:51:30.000000000","message":"It\u0027s actually a small optimisation to pass created_disks here, because we stop checking for disk existence after we found the first created disk. While the cost of disk.exists() isn\u0027t high, it\u0027s also not free so I erred on the side of not doing it when we already know what we\u0027re going to return.\n\nSpecifically, if an image defined kernel or ramdisk, and we created them, by passing created_disk here we can void the subsequent redundant exists() check for the root disk.\n\nThe other reason to do it is consistency. We do it everywhere else, so we do it here.","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"03a2698ba0807b854e168e7cbf14638ef05d210b","unresolved":false,"context_lines":[{"line_number":5647,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"},{"line_number":5648,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5649,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5650,"context_line":"                                   cleanup_on_failure\u003dNone):"},{"line_number":5651,"context_line":""},{"line_number":5652,"context_line":"        \"\"\"Do required network setup and create domain.\"\"\""},{"line_number":5653,"context_line":"        timeout \u003d CONF.vif_plugging_timeout"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_3739d5d7","line":5650,"updated":"2018-08-29 14:39:57.000000000","message":"_create_domain_and_network previously had only 1 caller which set destroy_disks_on_failure\u003dTrue: spawn. That\u0027s the subject of this fix.\n\nEverything else previous got destroy_disks_on_failure\u003dFalse, which now becomes cleanup_on_failure\u003dNone. As we pass this directly to _cleanup, this would previously have been cleanup(destroy_disks\u003dFalse, migrate_data\u003dNone), which we now causes us to delete nothing (instance dir or disks).","commit_id":"6609ba4156aa52c8ab207db90d5fc0ae346456cb"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"107152060c3fe0d73cf911396b1969ccba610ed5","unresolved":false,"context_lines":[{"line_number":3568,"context_line":""},{"line_number":3569,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3570,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3571,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3572,"context_line":"                created_disks \u003d True"},{"line_number":3573,"context_line":""},{"line_number":3574,"context_line":"            if instance.task_state \u003d\u003d task_states.RESIZE_FINISH:"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_537ae987","line":3571,"range":{"start_line":3571,"start_character":15,"end_line":3571,"end_character":37},"updated":"2018-09-17 22:38:32.000000000","message":"nit: I think this check is unnecessary since it\u0027ll always resolve to True","commit_id":"d0a6f424c963220740faa832aeaf55d2d301d199"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"61fb79346f127a3e8d56a24133a7b3fdb7f82575","unresolved":false,"context_lines":[{"line_number":166,"context_line":"    def __repr__(self):"},{"line_number":167,"context_line":"        return (\u0027InjectionInfo(network_info\u003d%r, files\u003d%r, \u0027"},{"line_number":168,"context_line":"                \u0027admin_pass\u003d\u003cSANITIZED\u003e)\u0027) % (self.network_info, self.files)"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"libvirt_volume_drivers \u003d ["},{"line_number":172,"context_line":"    \u0027iscsi\u003dnova.virt.libvirt.volume.iscsi.LibvirtISCSIVolumeDriver\u0027,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_42eeeefe","line":169,"updated":"2018-10-19 15:37:35.000000000","message":"Unrelated change","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"aa6a2c2d1947c2c1ab8e5f76e5f7da54441596e0","unresolved":false,"context_lines":[{"line_number":166,"context_line":"    def __repr__(self):"},{"line_number":167,"context_line":"        return (\u0027InjectionInfo(network_info\u003d%r, files\u003d%r, \u0027"},{"line_number":168,"context_line":"                \u0027admin_pass\u003d\u003cSANITIZED\u003e)\u0027) % (self.network_info, self.files)"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"libvirt_volume_drivers \u003d ["},{"line_number":172,"context_line":"    \u0027iscsi\u003dnova.virt.libvirt.volume.iscsi.LibvirtISCSIVolumeDriver\u0027,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_da1f216f","line":169,"in_reply_to":"3f79a3b5_42eeeefe","updated":"2018-10-23 14:33:10.000000000","message":"Done","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":1019,"context_line":"                cleanup_resources\u003dcleanup_resources)"},{"line_number":1020,"context_line":""},{"line_number":1021,"context_line":"    def _cleanup(self, context, instance, network_info, block_device_info\u003dNone,"},{"line_number":1022,"context_line":"                 cleanup_resources\u003dNone):"},{"line_number":1023,"context_line":"        if cleanup_resources and cleanup_resources.vifs:"},{"line_number":1024,"context_line":"            self._unplug_vifs(instance, network_info, True)"},{"line_number":1025,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_9f953f97","line":1022,"range":{"start_line":1022,"start_character":17,"end_line":1022,"end_character":39},"updated":"2018-10-19 14:33:47.000000000","message":"as far as I see every direct caller provides the cleanup_resource param so this does not need to be defaulted to None (but need to move before blok_device_info). This way the conditionals below can be simplified.\n--later--\nthere are indirect callers passing over None, I left suggestion there how to still avoid using a None value.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3396,"context_line":""},{"line_number":3397,"context_line":"        created_instance_dir \u003d True"},{"line_number":3398,"context_line":""},{"line_number":3399,"context_line":"        # ensure directories exist and are writable"},{"line_number":3400,"context_line":"        instance_dir \u003d libvirt_utils.get_instance_path(instance)"},{"line_number":3401,"context_line":"        if os.path.exists(instance_dir):"},{"line_number":3402,"context_line":"            LOG.debug(\"Instance directory exists: not creating\")"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_3fa42b05","line":3399,"range":{"start_line":3399,"start_character":8,"end_line":3399,"end_character":51},"updated":"2018-10-19 14:33:47.000000000","message":"The below code does not ensure that the existing directory is writable. I\u0027m not sure that the previous fileutils.ensure_tree call did made an existing but not writable path writable though. :/","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3396,"context_line":""},{"line_number":3397,"context_line":"        created_instance_dir \u003d True"},{"line_number":3398,"context_line":""},{"line_number":3399,"context_line":"        # ensure directories exist and are writable"},{"line_number":3400,"context_line":"        instance_dir \u003d libvirt_utils.get_instance_path(instance)"},{"line_number":3401,"context_line":"        if os.path.exists(instance_dir):"},{"line_number":3402,"context_line":"            LOG.debug(\"Instance directory exists: not creating\")"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_ce6772b8","line":3399,"range":{"start_line":3399,"start_character":8,"end_line":3399,"end_character":51},"in_reply_to":"3f79a3b5_3fa42b05","updated":"2018-10-22 08:56:48.000000000","message":"I don\u0027t want to change how resources are created; I\u0027m going for no regressions!","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8e1b70dcad64800c7b5ecca6c0dd805399742455","unresolved":false,"context_lines":[{"line_number":3396,"context_line":""},{"line_number":3397,"context_line":"        created_instance_dir \u003d True"},{"line_number":3398,"context_line":""},{"line_number":3399,"context_line":"        # ensure directories exist and are writable"},{"line_number":3400,"context_line":"        instance_dir \u003d libvirt_utils.get_instance_path(instance)"},{"line_number":3401,"context_line":"        if os.path.exists(instance_dir):"},{"line_number":3402,"context_line":"            LOG.debug(\"Instance directory exists: not creating\")"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_1773afe9","line":3399,"range":{"start_line":3399,"start_character":8,"end_line":3399,"end_character":51},"in_reply_to":"3f79a3b5_ce6772b8","updated":"2018-10-29 09:48:59.000000000","message":"Fair enough.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3451,"context_line":"        # which may not be using the instance directory."},{"line_number":3452,"context_line":"        if disk_images[\u0027kernel_id\u0027]:"},{"line_number":3453,"context_line":"            fname \u003d imagecache.get_cache_fname(disk_images[\u0027kernel_id\u0027])"},{"line_number":3454,"context_line":"            kernel \u003d raw(\u0027kernel\u0027)"},{"line_number":3455,"context_line":"            kernel.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3456,"context_line":"                         context\u003dcontext, filename\u003dfname,"},{"line_number":3457,"context_line":"                         image_id\u003ddisk_images[\u0027kernel_id\u0027])"},{"line_number":3458,"context_line":"            if disk_images[\u0027ramdisk_id\u0027]:"},{"line_number":3459,"context_line":"                fname \u003d imagecache.get_cache_fname(disk_images[\u0027ramdisk_id\u0027])"},{"line_number":3460,"context_line":"                ramdisk \u003d raw(\u0027ramdisk\u0027)"},{"line_number":3461,"context_line":"                ramdisk.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3462,"context_line":"                              context\u003dcontext, filename\u003dfname,"},{"line_number":3463,"context_line":"                              image_id\u003ddisk_images[\u0027ramdisk_id\u0027])"},{"line_number":3464,"context_line":""},{"line_number":3465,"context_line":"        if CONF.libvirt.virt_type \u003d\u003d \u0027uml\u0027:"},{"line_number":3466,"context_line":"            # PONDERING(mikal): can I assume that root is UID zero in every"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_9f31bfbe","line":3463,"range":{"start_line":3454,"start_character":0,"end_line":3463,"end_character":65},"updated":"2018-10-19 14:33:47.000000000","message":"is this just pure refactoring? if yes then I would do it separately from the bugfix","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"aa6a2c2d1947c2c1ab8e5f76e5f7da54441596e0","unresolved":false,"context_lines":[{"line_number":3451,"context_line":"        # which may not be using the instance directory."},{"line_number":3452,"context_line":"        if disk_images[\u0027kernel_id\u0027]:"},{"line_number":3453,"context_line":"            fname \u003d imagecache.get_cache_fname(disk_images[\u0027kernel_id\u0027])"},{"line_number":3454,"context_line":"            kernel \u003d raw(\u0027kernel\u0027)"},{"line_number":3455,"context_line":"            kernel.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3456,"context_line":"                         context\u003dcontext, filename\u003dfname,"},{"line_number":3457,"context_line":"                         image_id\u003ddisk_images[\u0027kernel_id\u0027])"},{"line_number":3458,"context_line":"            if disk_images[\u0027ramdisk_id\u0027]:"},{"line_number":3459,"context_line":"                fname \u003d imagecache.get_cache_fname(disk_images[\u0027ramdisk_id\u0027])"},{"line_number":3460,"context_line":"                ramdisk \u003d raw(\u0027ramdisk\u0027)"},{"line_number":3461,"context_line":"                ramdisk.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3462,"context_line":"                              context\u003dcontext, filename\u003dfname,"},{"line_number":3463,"context_line":"                              image_id\u003ddisk_images[\u0027ramdisk_id\u0027])"},{"line_number":3464,"context_line":""},{"line_number":3465,"context_line":"        if CONF.libvirt.virt_type \u003d\u003d \u0027uml\u0027:"},{"line_number":3466,"context_line":"            # PONDERING(mikal): can I assume that root is UID zero in every"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_1a36f903","line":3463,"range":{"start_line":3454,"start_character":0,"end_line":3463,"end_character":65},"in_reply_to":"3f79a3b5_2eee26fe","updated":"2018-10-23 14:33:10.000000000","message":"Done","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"61fb79346f127a3e8d56a24133a7b3fdb7f82575","unresolved":false,"context_lines":[{"line_number":3451,"context_line":"        # which may not be using the instance directory."},{"line_number":3452,"context_line":"        if disk_images[\u0027kernel_id\u0027]:"},{"line_number":3453,"context_line":"            fname \u003d imagecache.get_cache_fname(disk_images[\u0027kernel_id\u0027])"},{"line_number":3454,"context_line":"            kernel \u003d raw(\u0027kernel\u0027)"},{"line_number":3455,"context_line":"            kernel.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3456,"context_line":"                         context\u003dcontext, filename\u003dfname,"},{"line_number":3457,"context_line":"                         image_id\u003ddisk_images[\u0027kernel_id\u0027])"},{"line_number":3458,"context_line":"            if disk_images[\u0027ramdisk_id\u0027]:"},{"line_number":3459,"context_line":"                fname \u003d imagecache.get_cache_fname(disk_images[\u0027ramdisk_id\u0027])"},{"line_number":3460,"context_line":"                ramdisk \u003d raw(\u0027ramdisk\u0027)"},{"line_number":3461,"context_line":"                ramdisk.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3462,"context_line":"                              context\u003dcontext, filename\u003dfname,"},{"line_number":3463,"context_line":"                              image_id\u003ddisk_images[\u0027ramdisk_id\u0027])"},{"line_number":3464,"context_line":""},{"line_number":3465,"context_line":"        if CONF.libvirt.virt_type \u003d\u003d \u0027uml\u0027:"},{"line_number":3466,"context_line":"            # PONDERING(mikal): can I assume that root is UID zero in every"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_a2e3423a","line":3463,"range":{"start_line":3454,"start_character":0,"end_line":3463,"end_character":65},"in_reply_to":"3f79a3b5_9f31bfbe","updated":"2018-10-19 15:37:35.000000000","message":"Agreed, let\u0027s keep refactoring separate. This is a complex area of the code and it would help to keep things as simple as possible.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3451,"context_line":"        # which may not be using the instance directory."},{"line_number":3452,"context_line":"        if disk_images[\u0027kernel_id\u0027]:"},{"line_number":3453,"context_line":"            fname \u003d imagecache.get_cache_fname(disk_images[\u0027kernel_id\u0027])"},{"line_number":3454,"context_line":"            kernel \u003d raw(\u0027kernel\u0027)"},{"line_number":3455,"context_line":"            kernel.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3456,"context_line":"                         context\u003dcontext, filename\u003dfname,"},{"line_number":3457,"context_line":"                         image_id\u003ddisk_images[\u0027kernel_id\u0027])"},{"line_number":3458,"context_line":"            if disk_images[\u0027ramdisk_id\u0027]:"},{"line_number":3459,"context_line":"                fname \u003d imagecache.get_cache_fname(disk_images[\u0027ramdisk_id\u0027])"},{"line_number":3460,"context_line":"                ramdisk \u003d raw(\u0027ramdisk\u0027)"},{"line_number":3461,"context_line":"                ramdisk.cache(fetch_func\u003dlibvirt_utils.fetch_raw_image,"},{"line_number":3462,"context_line":"                              context\u003dcontext, filename\u003dfname,"},{"line_number":3463,"context_line":"                              image_id\u003ddisk_images[\u0027ramdisk_id\u0027])"},{"line_number":3464,"context_line":""},{"line_number":3465,"context_line":"        if CONF.libvirt.virt_type \u003d\u003d \u0027uml\u0027:"},{"line_number":3466,"context_line":"            # PONDERING(mikal): can I assume that root is UID zero in every"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_2eee26fe","line":3463,"range":{"start_line":3454,"start_character":0,"end_line":3463,"end_character":65},"in_reply_to":"3f79a3b5_a2e3423a","updated":"2018-10-22 08:56:48.000000000","message":"This code has been through a bunch of iterations, and an earlier version did a separate check on kernel and ramdisk until I realise that was broken. I\u0027ve forgotten to \u0027un-refactor\u0027 it.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3468,"context_line":"            uid \u003d pwd.getpwnam(\u0027root\u0027).pw_uid"},{"line_number":3469,"context_line":"            nova.privsep.path.chown(image(\u0027disk\u0027).path, uid\u003duid)"},{"line_number":3470,"context_line":""},{"line_number":3471,"context_line":"        created_disks \u003d self._create_and_inject_local_root("},{"line_number":3472,"context_line":"                context, instance, booted_from_volume, suffix, disk_images,"},{"line_number":3473,"context_line":"                injection_info, fallback_from_host)"},{"line_number":3474,"context_line":""},{"line_number":3475,"context_line":"        # Lookup the filesystem type if required"},{"line_number":3476,"context_line":"        os_type_with_default \u003d nova.privsep.fs.get_fs_type_for_os_type("}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_bf34bbac","line":3473,"range":{"start_line":3471,"start_character":0,"end_line":3473,"end_character":51},"updated":"2018-10-19 14:33:47.000000000","message":"ditto","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3468,"context_line":"            uid \u003d pwd.getpwnam(\u0027root\u0027).pw_uid"},{"line_number":3469,"context_line":"            nova.privsep.path.chown(image(\u0027disk\u0027).path, uid\u003duid)"},{"line_number":3470,"context_line":""},{"line_number":3471,"context_line":"        created_disks \u003d self._create_and_inject_local_root("},{"line_number":3472,"context_line":"                context, instance, booted_from_volume, suffix, disk_images,"},{"line_number":3473,"context_line":"                injection_info, fallback_from_host)"},{"line_number":3474,"context_line":""},{"line_number":3475,"context_line":"        # Lookup the filesystem type if required"},{"line_number":3476,"context_line":"        os_type_with_default \u003d nova.privsep.fs.get_fs_type_for_os_type("}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_eef7ced0","line":3473,"range":{"start_line":3471,"start_character":0,"end_line":3473,"end_character":51},"in_reply_to":"3f79a3b5_bf34bbac","updated":"2018-10-22 08:56:48.000000000","message":"This isn\u0027t refactoring. We\u0027re now capturing whether _create_and_inject_local_root created the root disk or not, where previously we didn\u0027t.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3484,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3485,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3486,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3487,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3488,"context_line":"                created_disks \u003d True"},{"line_number":3489,"context_line":""},{"line_number":3490,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_3f698b8c","line":3487,"range":{"start_line":3487,"start_character":15,"end_line":3487,"end_character":36},"updated":"2018-10-19 14:33:47.000000000","message":"why do we need this condition?","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"61fb79346f127a3e8d56a24133a7b3fdb7f82575","unresolved":false,"context_lines":[{"line_number":3484,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3485,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3486,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3487,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3488,"context_line":"                created_disks \u003d True"},{"line_number":3489,"context_line":""},{"line_number":3490,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_62b20a99","line":3487,"range":{"start_line":3487,"start_character":15,"end_line":3487,"end_character":36},"in_reply_to":"3f79a3b5_3f698b8c","updated":"2018-10-19 15:37:35.000000000","message":"Is this just to avoid setting created_disks \u003d True more than once? If so, I don\u0027t think we need to guard on it and it would make the code simpler to understand to omit it.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3484,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3485,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3486,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3487,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3488,"context_line":"                created_disks \u003d True"},{"line_number":3489,"context_line":""},{"line_number":3490,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_8ecdfa93","line":3487,"range":{"start_line":3487,"start_character":15,"end_line":3487,"end_character":36},"in_reply_to":"3f79a3b5_62b20a99","updated":"2018-10-22 08:56:48.000000000","message":"It\u0027s because the exists() test isn\u0027t free. If we already know we created resources we shortcut the test.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8e1b70dcad64800c7b5ecca6c0dd805399742455","unresolved":false,"context_lines":[{"line_number":3484,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3485,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3486,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3487,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3488,"context_line":"                created_disks \u003d True"},{"line_number":3489,"context_line":""},{"line_number":3490,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_b74bdb83","line":3487,"range":{"start_line":3487,"start_character":15,"end_line":3487,"end_character":36},"in_reply_to":"3f79a3b5_8ecdfa93","updated":"2018-10-29 09:48:59.000000000","message":"Ohh. OK, then it make sense.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3503,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3504,"context_line":"                block_device_info)):"},{"line_number":3505,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3506,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3507,"context_line":"                created_disks \u003d True"},{"line_number":3508,"context_line":""},{"line_number":3509,"context_line":"            specified_fs \u003d eph.get(\u0027guest_format\u0027)"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_df6737b8","line":3506,"range":{"start_line":3506,"start_character":15,"end_line":3506,"end_character":36},"updated":"2018-10-19 14:33:47.000000000","message":"ditto","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3503,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3504,"context_line":"                block_device_info)):"},{"line_number":3505,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3506,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3507,"context_line":"                created_disks \u003d True"},{"line_number":3508,"context_line":""},{"line_number":3509,"context_line":"            specified_fs \u003d eph.get(\u0027guest_format\u0027)"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_aed0f62b","line":3506,"range":{"start_line":3506,"start_character":15,"end_line":3506,"end_character":36},"in_reply_to":"3f79a3b5_df6737b8","updated":"2018-10-22 08:56:48.000000000","message":"ditto ;)","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3528,"context_line":"        if swap_mb \u003e 0:"},{"line_number":3529,"context_line":"            size \u003d swap_mb * units.Mi"},{"line_number":3530,"context_line":"            swap \u003d image(\u0027disk.swap\u0027)"},{"line_number":3531,"context_line":"            if not created_disks and not swap.exists():"},{"line_number":3532,"context_line":"                created_disks \u003d True"},{"line_number":3533,"context_line":"            swap.cache(fetch_func\u003dself._create_swap, context\u003dcontext,"},{"line_number":3534,"context_line":"                       filename\u003d\"swap_%s\" % swap_mb,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_7f0ce3ea","line":3531,"range":{"start_line":3531,"start_character":15,"end_line":3531,"end_character":36},"updated":"2018-10-19 14:33:47.000000000","message":"why we need this?","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3528,"context_line":"        if swap_mb \u003e 0:"},{"line_number":3529,"context_line":"            size \u003d swap_mb * units.Mi"},{"line_number":3530,"context_line":"            swap \u003d image(\u0027disk.swap\u0027)"},{"line_number":3531,"context_line":"            if not created_disks and not swap.exists():"},{"line_number":3532,"context_line":"                created_disks \u003d True"},{"line_number":3533,"context_line":"            swap.cache(fetch_func\u003dself._create_swap, context\u003dcontext,"},{"line_number":3534,"context_line":"                       filename\u003d\"swap_%s\" % swap_mb,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_4ed7e223","line":3531,"range":{"start_line":3531,"start_character":15,"end_line":3531,"end_character":36},"in_reply_to":"3f79a3b5_7f0ce3ea","updated":"2018-10-22 08:56:48.000000000","message":"See above.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3561,"context_line":"            if size \u003d\u003d 0 or suffix \u003d\u003d \u0027.rescue\u0027:"},{"line_number":3562,"context_line":"                size \u003d None"},{"line_number":3563,"context_line":""},{"line_number":3564,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3565,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3566,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3567,"context_line":"                created_disks \u003d True"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_ff10f3f6","line":3564,"range":{"start_line":3564,"start_character":12,"end_line":3564,"end_character":21},"updated":"2018-10-19 14:33:47.000000000","message":"As far as I see this is pure rename refactoring. Please keep the refactoring in a separate patch to help the reviewer.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"aa6a2c2d1947c2c1ab8e5f76e5f7da54441596e0","unresolved":false,"context_lines":[{"line_number":3561,"context_line":"            if size \u003d\u003d 0 or suffix \u003d\u003d \u0027.rescue\u0027:"},{"line_number":3562,"context_line":"                size \u003d None"},{"line_number":3563,"context_line":""},{"line_number":3564,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3565,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3566,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3567,"context_line":"                created_disks \u003d True"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_ad08dc7f","line":3564,"range":{"start_line":3564,"start_character":12,"end_line":3564,"end_character":21},"in_reply_to":"3f79a3b5_eee58e08","updated":"2018-10-23 14:33:10.000000000","message":"Ah, as I come to take it out I see now why I put it in: it\u0027s because backend.exists() is confusing. Meh, taken it back out again anyway.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3561,"context_line":"            if size \u003d\u003d 0 or suffix \u003d\u003d \u0027.rescue\u0027:"},{"line_number":3562,"context_line":"                size \u003d None"},{"line_number":3563,"context_line":""},{"line_number":3564,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3565,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3566,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3567,"context_line":"                created_disks \u003d True"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_eee58e08","line":3564,"range":{"start_line":3564,"start_character":12,"end_line":3564,"end_character":21},"in_reply_to":"3f79a3b5_ff10f3f6","updated":"2018-10-22 08:56:48.000000000","message":"Ok, not sure how this slipped in.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":3563,"context_line":""},{"line_number":3564,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3565,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3566,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3567,"context_line":"                created_disks \u003d True"},{"line_number":3568,"context_line":""},{"line_number":3569,"context_line":"            if instance.task_state \u003d\u003d task_states.RESIZE_FINISH:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_3f174bf2","line":3566,"range":{"start_line":3566,"start_character":15,"end_line":3566,"end_character":36},"updated":"2018-10-19 14:33:47.000000000","message":"ditto","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"61fb79346f127a3e8d56a24133a7b3fdb7f82575","unresolved":false,"context_lines":[{"line_number":3563,"context_line":""},{"line_number":3564,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3565,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3566,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3567,"context_line":"                created_disks \u003d True"},{"line_number":3568,"context_line":""},{"line_number":3569,"context_line":"            if instance.task_state \u003d\u003d task_states.RESIZE_FINISH:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_62cbea1c","line":3566,"range":{"start_line":3566,"start_character":15,"end_line":3566,"end_character":36},"in_reply_to":"3f79a3b5_3f174bf2","updated":"2018-10-19 15:37:35.000000000","message":"Yeah, this condition is unnecessary.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":3563,"context_line":""},{"line_number":3564,"context_line":"            root_disk \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3565,"context_line":"                                                   CONF.libvirt.images_type)"},{"line_number":3566,"context_line":"            if not created_disks and not root_disk.exists():"},{"line_number":3567,"context_line":"                created_disks \u003d True"},{"line_number":3568,"context_line":""},{"line_number":3569,"context_line":"            if instance.task_state \u003d\u003d task_states.RESIZE_FINISH:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_0ee1eafd","line":3566,"range":{"start_line":3566,"start_character":15,"end_line":3566,"end_character":36},"in_reply_to":"3f79a3b5_62cbea1c","updated":"2018-10-22 08:56:48.000000000","message":"Yeah, this is from an earlier version when we tested kernel and ramdisk, and passed that in here. It can be removed now.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c95dd702b4bf78b7ac7dbcf8d800562c3d054d79","unresolved":false,"context_lines":[{"line_number":5637,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"},{"line_number":5638,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5639,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5640,"context_line":"                                   cleanup_on_failure\u003dNone):"},{"line_number":5641,"context_line":""},{"line_number":5642,"context_line":"        \"\"\"Do required network setup and create domain.\"\"\""},{"line_number":5643,"context_line":"        timeout \u003d CONF.vif_plugging_timeout"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_bfb91bec","line":5640,"range":{"start_line":5640,"start_character":54,"end_line":5640,"end_character":58},"updated":"2018-10-19 14:33:47.000000000","message":"tHis could be a CleanupResources(False, False, False) which can be move the a constant NOTHING_TO_CLEANUP. This way the reader does not need to look up what None means for the subseqent indirect call.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"aa6a2c2d1947c2c1ab8e5f76e5f7da54441596e0","unresolved":false,"context_lines":[{"line_number":5637,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"},{"line_number":5638,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5639,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5640,"context_line":"                                   cleanup_on_failure\u003dNone):"},{"line_number":5641,"context_line":""},{"line_number":5642,"context_line":"        \"\"\"Do required network setup and create domain.\"\"\""},{"line_number":5643,"context_line":"        timeout \u003d CONF.vif_plugging_timeout"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_f928da36","line":5640,"range":{"start_line":5640,"start_character":54,"end_line":5640,"end_character":58},"in_reply_to":"3f79a3b5_4ea5a2b3","updated":"2018-10-23 14:33:10.000000000","message":"There was a bug here: with cleanup_on_failure\u003dNone we were defaulting to not cleaning up vifs, whereas previously cleanup() had destroy_vifs\u003dTrue.\n\nI\u0027ve taken destroy_vifs out of CleanupResources in the new patch, and consequently renamed CleanupResources to CleanupData. I think this is clearer.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3e2445575538c4b3976a004a0143444d2d6071a2","unresolved":false,"context_lines":[{"line_number":5637,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"},{"line_number":5638,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5639,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5640,"context_line":"                                   cleanup_on_failure\u003dNone):"},{"line_number":5641,"context_line":""},{"line_number":5642,"context_line":"        \"\"\"Do required network setup and create domain.\"\"\""},{"line_number":5643,"context_line":"        timeout \u003d CONF.vif_plugging_timeout"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_4ea5a2b3","line":5640,"range":{"start_line":5640,"start_character":54,"end_line":5640,"end_character":58},"in_reply_to":"3f79a3b5_823e4688","updated":"2018-10-22 08:56:48.000000000","message":"I\u0027ll look into that.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"61fb79346f127a3e8d56a24133a7b3fdb7f82575","unresolved":false,"context_lines":[{"line_number":5637,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"},{"line_number":5638,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5639,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5640,"context_line":"                                   cleanup_on_failure\u003dNone):"},{"line_number":5641,"context_line":""},{"line_number":5642,"context_line":"        \"\"\"Do required network setup and create domain.\"\"\""},{"line_number":5643,"context_line":"        timeout \u003d CONF.vif_plugging_timeout"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f79a3b5_823e4688","line":5640,"range":{"start_line":5640,"start_character":54,"end_line":5640,"end_character":58},"in_reply_to":"3f79a3b5_bfb91bec","updated":"2018-10-19 15:37:35.000000000","message":"Another benefit to doing that is no more having to check \"if cleanup_resources\" before every condition.","commit_id":"a7e8a154f869e4b2ac286457bd42e6a69572cb99"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":155,"context_line":"GuestNumaConfig \u003d collections.namedtuple("},{"line_number":156,"context_line":"    \u0027GuestNumaConfig\u0027, [\u0027cpuset\u0027, \u0027cputune\u0027, \u0027numaconfig\u0027, \u0027numatune\u0027])"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"CleanupData \u003d collections.namedtuple("},{"line_number":159,"context_line":"    \u0027CleanupData\u0027, [\u0027instance_dir\u0027, \u0027disks\u0027])"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"CLEANUP_DATA_NONE \u003d CleanupData(False, False)"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_3977ced4","line":158,"updated":"2018-11-27 16:09:52.000000000","message":"A comment with this describing the attributes would probably be useful, i.e. this is used to track, during spawn/migrate, which local (data/console/config, root, ephemeral or swap) disks were created so they can be cleaned up on failure.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":155,"context_line":"GuestNumaConfig \u003d collections.namedtuple("},{"line_number":156,"context_line":"    \u0027GuestNumaConfig\u0027, [\u0027cpuset\u0027, \u0027cputune\u0027, \u0027numaconfig\u0027, \u0027numatune\u0027])"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"CleanupData \u003d collections.namedtuple("},{"line_number":159,"context_line":"    \u0027CleanupData\u0027, [\u0027instance_dir\u0027, \u0027disks\u0027])"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"CLEANUP_DATA_NONE \u003d CleanupData(False, False)"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_8fd30609","line":158,"in_reply_to":"3f79a3b5_3977ced4","updated":"2018-11-29 14:17:27.000000000","message":"Done","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"041160d0e286fe83ca73eb65ae891e73492cd22a","unresolved":false,"context_lines":[{"line_number":1003,"context_line":"        if destroy_disks:"},{"line_number":1004,"context_line":"            cleanup_data \u003d CleanupData(True, True)"},{"line_number":1005,"context_line":"        else:"},{"line_number":1006,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1007,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1008,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1009,"context_line":"            # this is wrong."},{"line_number":1010,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1011,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1012,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"            cleanup_data \u003d CleanupData(cleanup_instance_dir, False)"},{"line_number":1015,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_284bfe85","line":1012,"range":{"start_line":1006,"start_character":0,"end_line":1012,"end_character":75},"updated":"2018-11-09 11:05:00.000000000","message":"I think we can simplify this to the following that\u0027s also used in check_instance_shared_storage_local:\n\n\n    cleanup_instance_dir \u003d self.image_backend.backend().is_shared_block_storage()\n\n\nThat way callers to cleanup don\u0027t have provide migrate_data to ensure the instance directory is removed when destroy_disks is set to False, such as when the src n-cpu restarts after an evacuation when the rbd imagebackend is being used.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a3675d885798c168e2bb6bdf2402bc8036f755fb","unresolved":false,"context_lines":[{"line_number":1003,"context_line":"        if destroy_disks:"},{"line_number":1004,"context_line":"            cleanup_data \u003d CleanupData(True, True)"},{"line_number":1005,"context_line":"        else:"},{"line_number":1006,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1007,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1008,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1009,"context_line":"            # this is wrong."},{"line_number":1010,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1011,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1012,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"            cleanup_data \u003d CleanupData(cleanup_instance_dir, False)"},{"line_number":1015,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_bb2c0895","line":1012,"range":{"start_line":1006,"start_character":0,"end_line":1012,"end_character":75},"in_reply_to":"3f79a3b5_284bfe85","updated":"2018-11-13 10:45:31.000000000","message":"I didn\u0027t want to change this behaviour in this patch, which is why I wrote the comment about the existing behaviour, which I think is wrong. This is really just supposed to be code motion. If I\u0027ve changed any existing behaviour (other than intentionally) I\u0027d consider that a bug in this patch, but I think this code preserves the existing behaviour.\n\nAs noted in the comment, though, I do agree the existing behaviour is wrong, but I\u0027d prefer to fix it in a follow-up. I want to keep it separate because there\u0027s a long history of seemingly innocuous changes like this causing regressions due to long-forgotten legacy behaviour and/or latent bugs.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"e6bb4d69d546b90da5735719aba9e4fb4b827b0e","unresolved":false,"context_lines":[{"line_number":1003,"context_line":"        if destroy_disks:"},{"line_number":1004,"context_line":"            cleanup_data \u003d CleanupData(True, True)"},{"line_number":1005,"context_line":"        else:"},{"line_number":1006,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1007,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1008,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1009,"context_line":"            # this is wrong."},{"line_number":1010,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1011,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1012,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"            cleanup_data \u003d CleanupData(cleanup_instance_dir, False)"},{"line_number":1015,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_bbaf28e0","line":1012,"range":{"start_line":1006,"start_character":0,"end_line":1012,"end_character":75},"in_reply_to":"3f79a3b5_bb2c0895","updated":"2018-11-13 10:48:41.000000000","message":"ACK that\u0027s fair, I\u0027ll remove my -1 and work on changing this in an additional patch.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":1003,"context_line":"        if destroy_disks:"},{"line_number":1004,"context_line":"            cleanup_data \u003d CleanupData(True, True)"},{"line_number":1005,"context_line":"        else:"},{"line_number":1006,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1007,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1008,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1009,"context_line":"            # this is wrong."},{"line_number":1010,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1011,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1012,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"            cleanup_data \u003d CleanupData(cleanup_instance_dir, False)"},{"line_number":1015,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_74454b15","line":1012,"range":{"start_line":1006,"start_character":0,"end_line":1012,"end_character":75},"in_reply_to":"3f79a3b5_bbaf28e0","updated":"2018-11-27 16:09:52.000000000","message":"I think the only place outside of the driver itself that calls cleanup is in the compute manager during post live migration:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/compute/manager.py#L6623\n\nThat would be running from the source host after the guest is successfully live migrated to the dest host.\n\nIt will only call driver.cleanup() if the migrate_data says there isn\u0027t a shared instance_path:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/compute/manager.py#L6464\n\nand destroy_disks will only be True if migrate_data says it\u0027s not using shared block storage:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/compute/manager.py#L6465\n\nSo the logic here seems to align with that, and with what cleanup() was doing before:\n\nhttps://review.openstack.org/#/c/578846/17/nova/virt/libvirt/driver.py@a1065","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3398,"context_line":"        # ensure directories exist and are writable"},{"line_number":3399,"context_line":"        instance_dir \u003d libvirt_utils.get_instance_path(instance)"},{"line_number":3400,"context_line":"        if os.path.exists(instance_dir):"},{"line_number":3401,"context_line":"            LOG.debug(\"Instance directory exists: not creating\")"},{"line_number":3402,"context_line":"            created_instance_dir \u003d False"},{"line_number":3403,"context_line":"        else:"},{"line_number":3404,"context_line":"            LOG.debug(\"Creating instance directory\")"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_7978e62a","line":3401,"range":{"start_line":3401,"start_character":12,"end_line":3401,"end_character":64},"updated":"2018-11-27 16:09:52.000000000","message":"Include the instance_dir in the debug message for context.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":3398,"context_line":"        # ensure directories exist and are writable"},{"line_number":3399,"context_line":"        instance_dir \u003d libvirt_utils.get_instance_path(instance)"},{"line_number":3400,"context_line":"        if os.path.exists(instance_dir):"},{"line_number":3401,"context_line":"            LOG.debug(\"Instance directory exists: not creating\")"},{"line_number":3402,"context_line":"            created_instance_dir \u003d False"},{"line_number":3403,"context_line":"        else:"},{"line_number":3404,"context_line":"            LOG.debug(\"Creating instance directory\")"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_0f4ba45d","line":3401,"range":{"start_line":3401,"start_character":12,"end_line":3401,"end_character":64},"in_reply_to":"3f79a3b5_7978e62a","updated":"2018-11-29 14:17:27.000000000","message":"Ack, and throughout.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3401,"context_line":"            LOG.debug(\"Instance directory exists: not creating\")"},{"line_number":3402,"context_line":"            created_instance_dir \u003d False"},{"line_number":3403,"context_line":"        else:"},{"line_number":3404,"context_line":"            LOG.debug(\"Creating instance directory\")"},{"line_number":3405,"context_line":"            fileutils.ensure_tree(libvirt_utils.get_instance_path(instance))"},{"line_number":3406,"context_line":""},{"line_number":3407,"context_line":"        LOG.info(\u0027Creating image\u0027, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_396eee64","line":3404,"updated":"2018-11-27 16:09:52.000000000","message":"same","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3483,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3484,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3485,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3486,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3487,"context_line":"                created_disks \u003d True"},{"line_number":3488,"context_line":""},{"line_number":3489,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"},{"line_number":3490,"context_line":"                                   fs_label\u003d\u0027ephemeral0\u0027,"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_599e4a52","line":3487,"range":{"start_line":3486,"start_character":12,"end_line":3487,"end_character":36},"updated":"2018-11-27 16:09:52.000000000","message":"A code comment here would be useful.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"97ac575ba836c352f4356d7dfd330e3a79b71e53","unresolved":false,"context_lines":[{"line_number":3483,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3484,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3485,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3486,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3487,"context_line":"                created_disks \u003d True"},{"line_number":3488,"context_line":""},{"line_number":3489,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"},{"line_number":3490,"context_line":"                                   fs_label\u003d\u0027ephemeral0\u0027,"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_46715446","line":3487,"range":{"start_line":3486,"start_character":12,"end_line":3487,"end_character":36},"in_reply_to":"3f79a3b5_4fc72e12","updated":"2018-11-29 17:10:56.000000000","message":"Well, I assume the \"not created_disks\" is in here to avoid the exists() call because if we created the root disk above and set created_disks\u003dTrue, we can save ourselves some time. That\u0027s fine. I\u0027m mostly confused about disk_image.exists(), which I guess is an imagebackend object (Flat/Rbd/etc) but there is clearly some tight coupling because in this code we don\u0027t create the disk, that would happen in _create_ephemeral via the image cache call below, yes? But I don\u0027t see anything in _create_ephemeral checking to see if it exists first.\n\nAnyway, maybe it\u0027s not worth it. There is just a lot of stuff going on in this method overall and very little of it is documented, and lots of stuff is compressed into these little conditional and looping blocks.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":3483,"context_line":"        ephemeral_gb \u003d instance.flavor.ephemeral_gb"},{"line_number":3484,"context_line":"        if \u0027disk.local\u0027 in disk_mapping:"},{"line_number":3485,"context_line":"            disk_image \u003d image(\u0027disk.local\u0027)"},{"line_number":3486,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3487,"context_line":"                created_disks \u003d True"},{"line_number":3488,"context_line":""},{"line_number":3489,"context_line":"            fn \u003d functools.partial(self._create_ephemeral,"},{"line_number":3490,"context_line":"                                   fs_label\u003d\u0027ephemeral0\u0027,"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_4fc72e12","line":3487,"range":{"start_line":3486,"start_character":12,"end_line":3487,"end_character":36},"in_reply_to":"3f79a3b5_599e4a52","updated":"2018-11-29 14:17:27.000000000","message":"I obviously have \u0027my own code\u0027 blindness. What would you comment here?","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3499,"context_line":"                             size\u003dsize,"},{"line_number":3500,"context_line":"                             ephemeral_size\u003dephemeral_gb)"},{"line_number":3501,"context_line":""},{"line_number":3502,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3503,"context_line":"                block_device_info)):"},{"line_number":3504,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3505,"context_line":"            if not created_disks and not disk_image.exists():"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_f4cddb91","line":3502,"updated":"2018-11-27 16:09:52.000000000","message":"It would be good to add an ephemeral and swap disk to the flavor used in the functional test, to make sure those are also cleaned up, yeah? I\u0027m not sure how much harder that is though, since the functional test is really just focused on the instance dir and local disk. If it would explode the complexity if the functional test, then unit test coverage might be sufficient. However, I don\u0027t see the ephemeral/swap disk usage setting the flag is asserted anywhere in the unit tests - at least not obviously. So -1 for that.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":3499,"context_line":"                             size\u003dsize,"},{"line_number":3500,"context_line":"                             ephemeral_size\u003dephemeral_gb)"},{"line_number":3501,"context_line":""},{"line_number":3502,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3503,"context_line":"                block_device_info)):"},{"line_number":3504,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3505,"context_line":"            if not created_disks and not disk_image.exists():"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_1493b700","line":3502,"in_reply_to":"3f79a3b5_d4abff6c","updated":"2018-11-29 14:17:27.000000000","message":"I expanded the regression test to cover this, hopefully thoroughly:\n\nhttps://review.openstack.org/620917","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"bc00340fe47f4c3b9e756d981561a38a3eab4c23","unresolved":false,"context_lines":[{"line_number":3499,"context_line":"                             size\u003dsize,"},{"line_number":3500,"context_line":"                             ephemeral_size\u003dephemeral_gb)"},{"line_number":3501,"context_line":""},{"line_number":3502,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3503,"context_line":"                block_device_info)):"},{"line_number":3504,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3505,"context_line":"            if not created_disks and not disk_image.exists():"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_d4abff6c","line":3502,"in_reply_to":"3f79a3b5_f4cddb91","updated":"2018-11-27 16:13:14.000000000","message":"Another idea is write a functional test variant that uses a volume-backed server which has ephemeral/swap disks on it, to make sure everything is handled properly. That might be asking a lot for this change though - so maybe a follow up if we can get sufficient unit test coverage for the ephemeral/swap disk case already noted here.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3502,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3503,"context_line":"                block_device_info)):"},{"line_number":3504,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3505,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3506,"context_line":"                created_disks \u003d True"},{"line_number":3507,"context_line":""},{"line_number":3508,"context_line":"            specified_fs \u003d eph.get(\u0027guest_format\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_f9b2b6e7","line":3505,"updated":"2018-11-27 16:09:52.000000000","message":"same","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3534,"context_line":"                       size\u003dsize, swap_mb\u003dswap_mb)"},{"line_number":3535,"context_line":""},{"line_number":3536,"context_line":"        if created_disks:"},{"line_number":3537,"context_line":"            LOG.debug(\u0027Created ephemeral disks\u0027)"},{"line_number":3538,"context_line":"        else:"},{"line_number":3539,"context_line":"            LOG.debug(\u0027Did not create ephemeral disks\u0027)"},{"line_number":3540,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_b9799efa","line":3537,"range":{"start_line":3537,"start_character":12,"end_line":3537,"end_character":48},"updated":"2018-11-27 16:09:52.000000000","message":"nit: include the instance in the message via the instance\u003dinstance kwarg.\n\nAlso, the message is a tad bit misleading in that it could be ephemeral or swap disks. Maybe \"ephemeral\" here just means both/either since they don\u0027t persist, but \"ephemeral\" is also the non-swap local disk name we use in the API so it could be confused if someone was debugging this and thinks that ephemeral disks were created when they expected just a root disk and a swap disk, for example. Maybe just saying \"Created ephemeral and/or swap disks\" would clarify?","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":3534,"context_line":"                       size\u003dsize, swap_mb\u003dswap_mb)"},{"line_number":3535,"context_line":""},{"line_number":3536,"context_line":"        if created_disks:"},{"line_number":3537,"context_line":"            LOG.debug(\u0027Created ephemeral disks\u0027)"},{"line_number":3538,"context_line":"        else:"},{"line_number":3539,"context_line":"            LOG.debug(\u0027Did not create ephemeral disks\u0027)"},{"line_number":3540,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_7408eb2c","line":3537,"range":{"start_line":3537,"start_character":12,"end_line":3537,"end_character":48},"in_reply_to":"3f79a3b5_b9799efa","updated":"2018-11-29 14:17:27.000000000","message":"I should probably have said \u0027local disks\u0027 here, although this is confusing too since rbd disks aren\u0027t local. \u0027Nova-managed\u0027? I think I\u0027ve used local in other contexts, so I\u0027ll go with that.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3536,"context_line":"        if created_disks:"},{"line_number":3537,"context_line":"            LOG.debug(\u0027Created ephemeral disks\u0027)"},{"line_number":3538,"context_line":"        else:"},{"line_number":3539,"context_line":"            LOG.debug(\u0027Did not create ephemeral disks\u0027)"},{"line_number":3540,"context_line":""},{"line_number":3541,"context_line":"        return CleanupData(created_instance_dir, created_disks)"},{"line_number":3542,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_d97c9aea","line":3539,"updated":"2018-11-27 16:09:52.000000000","message":"same","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3543,"context_line":"    def _create_and_inject_local_root(self, context, instance,"},{"line_number":3544,"context_line":"                                      booted_from_volume, suffix, disk_images,"},{"line_number":3545,"context_line":"                                      injection_info, fallback_from_host):"},{"line_number":3546,"context_line":"        created_disks \u003d False"},{"line_number":3547,"context_line":""},{"line_number":3548,"context_line":"        # File injection only if needed"},{"line_number":3549,"context_line":"        need_inject \u003d (not configdrive.required_by(instance) and"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_f9b87678","line":3546,"range":{"start_line":3546,"start_character":20,"end_line":3546,"end_character":21},"updated":"2018-11-27 16:09:52.000000000","message":"Would created_disk (singular) be more appropriate here? There is either 0 or 1 \"local\" root disk created in this method, right?","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":3543,"context_line":"    def _create_and_inject_local_root(self, context, instance,"},{"line_number":3544,"context_line":"                                      booted_from_volume, suffix, disk_images,"},{"line_number":3545,"context_line":"                                      injection_info, fallback_from_host):"},{"line_number":3546,"context_line":"        created_disks \u003d False"},{"line_number":3547,"context_line":""},{"line_number":3548,"context_line":"        # File injection only if needed"},{"line_number":3549,"context_line":"        need_inject \u003d (not configdrive.required_by(instance) and"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_f4b87b78","line":3546,"range":{"start_line":3546,"start_character":20,"end_line":3546,"end_character":21},"in_reply_to":"3f79a3b5_f9b87678","updated":"2018-11-29 14:17:27.000000000","message":"Yeah, but a bit less greppable as it\u0027s plural everywhere else.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":3562,"context_line":""},{"line_number":3563,"context_line":"            backend \u003d self.image_backend.by_name(instance, \u0027disk\u0027 + suffix,"},{"line_number":3564,"context_line":"                                                 CONF.libvirt.images_type)"},{"line_number":3565,"context_line":"            if not backend.exists():"},{"line_number":3566,"context_line":"                created_disks \u003d True"},{"line_number":3567,"context_line":""},{"line_number":3568,"context_line":"            if instance.task_state \u003d\u003d task_states.RESIZE_FINISH:"},{"line_number":3569,"context_line":"                backend.create_snap(libvirt_utils.RESIZE_SNAPSHOT_NAME)"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_b9951ee4","line":3566,"range":{"start_line":3565,"start_character":13,"end_line":3566,"end_character":36},"updated":"2018-11-27 16:09:52.000000000","message":"A comment here would be good. Also, this could just be:\n\ncreated_disks \u003d not backend.exists()","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"59054bc9dce5378baa3afd7048e7df59a6537267","unresolved":false,"context_lines":[{"line_number":5629,"context_line":"                guest.poweroff()"},{"line_number":5630,"context_line":"        finally:"},{"line_number":5631,"context_line":"            self._cleanup(context, instance, network_info, block_device_info,"},{"line_number":5632,"context_line":"                          cleanup_data, True)"},{"line_number":5633,"context_line":""},{"line_number":5634,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5635,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_14fdf705","line":5632,"range":{"start_line":5632,"start_character":40,"end_line":5632,"end_character":44},"updated":"2018-11-27 16:09:52.000000000","message":"Please use a named kwarg here, destroy_vifs\u003dTrue. Otherwise I have to look at the function signature to know what this True is for.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fb5441d3fbcea97c709a730895dcd30f903bf4cb","unresolved":false,"context_lines":[{"line_number":5629,"context_line":"                guest.poweroff()"},{"line_number":5630,"context_line":"        finally:"},{"line_number":5631,"context_line":"            self._cleanup(context, instance, network_info, block_device_info,"},{"line_number":5632,"context_line":"                          cleanup_data, True)"},{"line_number":5633,"context_line":""},{"line_number":5634,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5635,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":17,"id":"3f79a3b5_34a27341","line":5632,"range":{"start_line":5632,"start_character":40,"end_line":5632,"end_character":44},"in_reply_to":"3f79a3b5_14fdf705","updated":"2018-11-29 14:17:27.000000000","message":"Ack.","commit_id":"58b1415f7f349a27e36b384761b8035f26b229e9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd186bc62e8558b3b60fbca0c1df902808ef9bec","unresolved":false,"context_lines":[{"line_number":3401,"context_line":"        # ensure directories exist and are writable"},{"line_number":3402,"context_line":"        instance_dir \u003d libvirt_utils.get_instance_path(instance)"},{"line_number":3403,"context_line":"        if os.path.exists(instance_dir):"},{"line_number":3404,"context_line":"            LOG.debug(\"Instance directory exists: not creating\","},{"line_number":3405,"context_line":"                      instance\u003dinstance)"},{"line_number":3406,"context_line":"            created_instance_dir \u003d False"},{"line_number":3407,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":19,"id":"3f79a3b5_46edd4b7","line":3404,"range":{"start_line":3404,"start_character":32,"end_line":3404,"end_character":41},"updated":"2018-11-29 17:14:31.000000000","message":"nit: I meant more like this:\n\n(\"Instance directory %s exists: not creating\", instance_dir, intsance\u003dinstance)","commit_id":"b8d7a522a9886bd1097e8e15912e56a9ccaab680"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd186bc62e8558b3b60fbca0c1df902808ef9bec","unresolved":false,"context_lines":[{"line_number":3405,"context_line":"                      instance\u003dinstance)"},{"line_number":3406,"context_line":"            created_instance_dir \u003d False"},{"line_number":3407,"context_line":"        else:"},{"line_number":3408,"context_line":"            LOG.debug(\"Creating instance directory\", instance\u003dinstance)"},{"line_number":3409,"context_line":"            fileutils.ensure_tree(libvirt_utils.get_instance_path(instance))"},{"line_number":3410,"context_line":""},{"line_number":3411,"context_line":"        LOG.info(\u0027Creating image\u0027, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3f79a3b5_e6e980ac","line":3408,"range":{"start_line":3408,"start_character":41,"end_line":3408,"end_character":50},"updated":"2018-11-29 17:14:31.000000000","message":"same","commit_id":"b8d7a522a9886bd1097e8e15912e56a9ccaab680"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"34e180380e84c01c224061fddd35fa344cccd179","unresolved":false,"context_lines":[{"line_number":1008,"context_line":"        else:"},{"line_number":1009,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1010,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1011,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1012,"context_line":"            # this is wrong."},{"line_number":1013,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1014,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1015,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_302acd78","line":1012,"range":{"start_line":1011,"start_character":60,"end_line":1012,"end_character":28},"updated":"2018-12-05 20:16:28.000000000","message":"What do you mean here? That it\u0027s possible that the instance _is_ shared but it \"usually\" isn\u0027t?","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e3015d90a76b52bcadde4b63071c3fabf9d196a5","unresolved":false,"context_lines":[{"line_number":1008,"context_line":"        else:"},{"line_number":1009,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1010,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1011,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1012,"context_line":"            # this is wrong."},{"line_number":1013,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1014,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1015,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_6bca89a3","line":1012,"range":{"start_line":1011,"start_character":60,"end_line":1012,"end_character":28},"in_reply_to":"3f79a3b5_302acd78","updated":"2018-12-06 15:39:53.000000000","message":"Yes. The assumption that the instance directory is not shared because the block storage is shared is fundamentally unsound, but probably usually correct in practise.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"bd682ee6b2b917a811133a28547fe9784aa1537b","unresolved":false,"context_lines":[{"line_number":3471,"context_line":"            uid \u003d pwd.getpwnam(\u0027root\u0027).pw_uid"},{"line_number":3472,"context_line":"            nova.privsep.path.chown(image(\u0027disk\u0027).path, uid\u003duid)"},{"line_number":3473,"context_line":""},{"line_number":3474,"context_line":"        created_disks \u003d self._create_and_inject_local_root("},{"line_number":3475,"context_line":"                context, instance, booted_from_volume, suffix, disk_images,"},{"line_number":3476,"context_line":"                injection_info, fallback_from_host)"},{"line_number":3477,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_c95b37f6","line":3474,"range":{"start_line":3474,"start_character":8,"end_line":3474,"end_character":21},"updated":"2018-11-29 17:38:23.000000000","message":"If I immediately set this to False after this assignment the functional tests still don\u0027t fail.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"90a1c49fec77d67ef1e8d35218874e92f0387b5f","unresolved":false,"context_lines":[{"line_number":3471,"context_line":"            uid \u003d pwd.getpwnam(\u0027root\u0027).pw_uid"},{"line_number":3472,"context_line":"            nova.privsep.path.chown(image(\u0027disk\u0027).path, uid\u003duid)"},{"line_number":3473,"context_line":""},{"line_number":3474,"context_line":"        created_disks \u003d self._create_and_inject_local_root("},{"line_number":3475,"context_line":"                context, instance, booted_from_volume, suffix, disk_images,"},{"line_number":3476,"context_line":"                injection_info, fallback_from_host)"},{"line_number":3477,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_bf53a208","line":3474,"range":{"start_line":3474,"start_character":8,"end_line":3474,"end_character":21},"in_reply_to":"3f79a3b5_c95b37f6","updated":"2018-12-11 15:08:34.000000000","message":"That\u0027s because \u0027False\u0027 means: we didn\u0027t create a local root disk, so don\u0027t delete it. In the evacuate case on Rbd and for Flat/shared we\u0027d expect this to be False anyway, because the disk already exists. For Flat/nonshared the test doesn\u0027t fail because the flat backend only cleans up disks by deleting the instance directory, which we don\u0027t do because we correctly detected that we didn\u0027t create the instance directory.\n\nSetting this to False would have caused Rbd disks to not be cleaned up if there was an error during the initial spawn, but we\u0027re only injecting a failure into the evacuate here, not the initial spawn.\n\nSetting this to True causes the Rbd evacuate tests to fail as you\u0027d expect, because we end up deleting shared disks during the failed evacuation.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"389b562818608d4f753af4e79aa5b47be4116d10","unresolved":false,"context_lines":[{"line_number":3506,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3507,"context_line":"                block_device_info)):"},{"line_number":3508,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3509,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3510,"context_line":"                created_disks \u003d True"},{"line_number":3511,"context_line":""},{"line_number":3512,"context_line":"            specified_fs \u003d eph.get(\u0027guest_format\u0027)"},{"line_number":3513,"context_line":"            if specified_fs and not self.is_supported_fs_format(specified_fs):"}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_8979bf82","line":3510,"range":{"start_line":3509,"start_character":12,"end_line":3510,"end_character":36},"updated":"2018-11-29 17:34:33.000000000","message":"Hmm, I commented this out and re-ran the functional tests and nothing failed.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cdbb7dda1daf3840d2a0ee39ec8f2f6e052b2edc","unresolved":false,"context_lines":[{"line_number":3506,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3507,"context_line":"                block_device_info)):"},{"line_number":3508,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3509,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3510,"context_line":"                created_disks \u003d True"},{"line_number":3511,"context_line":""},{"line_number":3512,"context_line":"            specified_fs \u003d eph.get(\u0027guest_format\u0027)"},{"line_number":3513,"context_line":"            if specified_fs and not self.is_supported_fs_format(specified_fs):"}],"source_content_type":"text/x-python","patch_set":20,"id":"bfb3d3c7_46dc60e3","line":3510,"range":{"start_line":3509,"start_character":12,"end_line":3510,"end_character":36},"in_reply_to":"3f79a3b5_1f8e5624","updated":"2019-05-28 15:19:10.000000000","message":"This is addressed by the latest functional test.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"90a1c49fec77d67ef1e8d35218874e92f0387b5f","unresolved":false,"context_lines":[{"line_number":3506,"context_line":"        for idx, eph in enumerate(driver.block_device_info_get_ephemerals("},{"line_number":3507,"context_line":"                block_device_info)):"},{"line_number":3508,"context_line":"            disk_image \u003d image(blockinfo.get_eph_disk(idx))"},{"line_number":3509,"context_line":"            if not created_disks and not disk_image.exists():"},{"line_number":3510,"context_line":"                created_disks \u003d True"},{"line_number":3511,"context_line":""},{"line_number":3512,"context_line":"            specified_fs \u003d eph.get(\u0027guest_format\u0027)"},{"line_number":3513,"context_line":"            if specified_fs and not self.is_supported_fs_format(specified_fs):"}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_1f8e5624","line":3510,"range":{"start_line":3509,"start_character":12,"end_line":3510,"end_character":36},"in_reply_to":"3f79a3b5_8979bf82","updated":"2018-12-11 15:08:34.000000000","message":"That\u0027s an interesting quirk. Commenting this out means that we\u0027re leaving something False which should have been True. However, as noted above, False doesn\u0027t cause an interesting failure in the evacuate case. This code is critical during the initial spawn.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"389b562818608d4f753af4e79aa5b47be4116d10","unresolved":false,"context_lines":[{"line_number":3531,"context_line":"        if swap_mb \u003e 0:"},{"line_number":3532,"context_line":"            size \u003d swap_mb * units.Mi"},{"line_number":3533,"context_line":"            swap \u003d image(\u0027disk.swap\u0027)"},{"line_number":3534,"context_line":"            if not created_disks and not swap.exists():"},{"line_number":3535,"context_line":"                created_disks \u003d True"},{"line_number":3536,"context_line":"            swap.cache(fetch_func\u003dself._create_swap, context\u003dcontext,"},{"line_number":3537,"context_line":"                       filename\u003d\"swap_%s\" % swap_mb,"},{"line_number":3538,"context_line":"                       size\u003dsize, swap_mb\u003dswap_mb)"}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_29d3cb66","line":3535,"range":{"start_line":3534,"start_character":12,"end_line":3535,"end_character":36},"updated":"2018-11-29 17:34:33.000000000","message":"same, removing this doesn\u0027t make the functional test fail","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"90a1c49fec77d67ef1e8d35218874e92f0387b5f","unresolved":false,"context_lines":[{"line_number":3531,"context_line":"        if swap_mb \u003e 0:"},{"line_number":3532,"context_line":"            size \u003d swap_mb * units.Mi"},{"line_number":3533,"context_line":"            swap \u003d image(\u0027disk.swap\u0027)"},{"line_number":3534,"context_line":"            if not created_disks and not swap.exists():"},{"line_number":3535,"context_line":"                created_disks \u003d True"},{"line_number":3536,"context_line":"            swap.cache(fetch_func\u003dself._create_swap, context\u003dcontext,"},{"line_number":3537,"context_line":"                       filename\u003d\"swap_%s\" % swap_mb,"},{"line_number":3538,"context_line":"                       size\u003dsize, swap_mb\u003dswap_mb)"}],"source_content_type":"text/x-python","patch_set":20,"id":"3f79a3b5_9fa10696","line":3535,"range":{"start_line":3534,"start_character":12,"end_line":3535,"end_character":36},"in_reply_to":"3f79a3b5_29d3cb66","updated":"2018-12-11 15:08:34.000000000","message":"And again.","commit_id":"879b4344ba82a315d1cec1c1d1b97a8c02a9a3e2"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"61ee32b207216f1eb723829a5228fad83ddcebe3","unresolved":false,"context_lines":[{"line_number":1132,"context_line":"        if destroy_disks:"},{"line_number":1133,"context_line":"            cleanup_data \u003d CleanupData(True, True)"},{"line_number":1134,"context_line":"        else:"},{"line_number":1135,"context_line":"            # NOTE(mdbooth): I think the theory here was that if this is a"},{"line_number":1136,"context_line":"            # migration with shared block storage then we need to delete the"},{"line_number":1137,"context_line":"            # instance directory because that\u0027s not shared. I\u0027m pretty sure"},{"line_number":1138,"context_line":"            # this is wrong."},{"line_number":1139,"context_line":"            cleanup_instance_dir \u003d False"},{"line_number":1140,"context_line":"            if migrate_data and \u0027is_shared_block_storage\u0027 in migrate_data:"},{"line_number":1141,"context_line":"                cleanup_instance_dir \u003d migrate_data.is_shared_block_storage"}],"source_content_type":"text/x-python","patch_set":24,"id":"9fb8cfa7_5de3e934","line":1138,"range":{"start_line":1135,"start_character":0,"end_line":1138,"end_character":28},"updated":"2019-06-25 16:55:32.000000000","message":"Yeah, it looks like an undocumented assumption when using the Rbd imagebackend. Worthy of a followup bug and/or change to clear this up either in the code or docs?","commit_id":"d0ca72a3839eecd53fa39f51b33658e49e6ce7fb"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"61ee32b207216f1eb723829a5228fad83ddcebe3","unresolved":false,"context_lines":[{"line_number":1154,"context_line":""},{"line_number":1155,"context_line":"            cleanup_data \u003d CleanupData(cleanup_instance_dir, False)"},{"line_number":1156,"context_line":""},{"line_number":1157,"context_line":"        return self._cleanup("},{"line_number":1158,"context_line":"                context, instance, network_info, block_device_info,"},{"line_number":1159,"context_line":"                cleanup_data, destroy_vifs)"},{"line_number":1160,"context_line":""},{"line_number":1161,"context_line":"    def _cleanup(self, context, instance, network_info, block_device_info,"},{"line_number":1162,"context_line":"                 cleanup_data, destroy_vifs):"}],"source_content_type":"text/x-python","patch_set":24,"id":"9fb8cfa7_5d744936","line":1159,"range":{"start_line":1157,"start_character":0,"end_line":1159,"end_character":43},"updated":"2019-06-25 16:55:32.000000000","message":"While the logic above this call is well tested by existing unit tests I don\u0027t think there\u0027s anything asserting the various values of cleanup_data we end up calling _cleanup with right?\n\nI appreciate that it\u0027s pretty mechanical given the extensive functional testing but still warranted IMHO just to avoid a bug sneaking in silently in the future.","commit_id":"d0ca72a3839eecd53fa39f51b33658e49e6ce7fb"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0804c92ece31fc0ae86561e0a31e879a316bd23d","unresolved":false,"context_lines":[{"line_number":1379,"context_line":"                cleanup_data, destroy_vifs)"},{"line_number":1380,"context_line":""},{"line_number":1381,"context_line":"    def _cleanup(self, context, instance, network_info, block_device_info,"},{"line_number":1382,"context_line":"                 cleanup_data, destroy_vifs):"},{"line_number":1383,"context_line":"        \"\"\"Cleanup the domain and any attached resources from the host."},{"line_number":1384,"context_line":""},{"line_number":1385,"context_line":"        This method cleans up any pmem devices, unplugs VIFs, disconnects"}],"source_content_type":"text/x-python","patch_set":28,"id":"ff570b3c_fbb804c9","line":1382,"range":{"start_line":1382,"start_character":17,"end_line":1382,"end_character":29},"updated":"2020-05-20 17:01:47.000000000","message":"Any reason not to just provide two boolean variables, such as \u0027clean_instance_dir\u0027 and \u0027clean_disks\u0027?","commit_id":"7a7b6f74ebccc57e92fc6e8e658923c6c4be9e0a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"51f9021e0f47016028d3991614f1906c52268faf","unresolved":false,"context_lines":[{"line_number":3629,"context_line":"                                          context, instance,"},{"line_number":3630,"context_line":"                                          injection_info)"},{"line_number":3631,"context_line":"        cleanup_data \u003d self._create_image("},{"line_number":3632,"context_line":"                context, instance, disk_info[\u0027mapping\u0027],"},{"line_number":3633,"context_line":"                injection_info\u003dinjection_info,"},{"line_number":3634,"context_line":"                block_device_info\u003dblock_device_info)"},{"line_number":3635,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"ff570b3c_14b4307f","line":3632,"range":{"start_line":3632,"start_character":12,"end_line":3632,"end_character":16},"updated":"2020-05-21 16:32:55.000000000","message":"nit","commit_id":"083df01a4d90e155c1b4ef9e954d8be14a02d830"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0804c92ece31fc0ae86561e0a31e879a316bd23d","unresolved":false,"context_lines":[{"line_number":6478,"context_line":"                guest.poweroff()"},{"line_number":6479,"context_line":"        finally:"},{"line_number":6480,"context_line":"            self._cleanup(context, instance, network_info, block_device_info,"},{"line_number":6481,"context_line":"                          cleanup_data, True)"},{"line_number":6482,"context_line":""},{"line_number":6483,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":6484,"context_line":"                                   block_device_info\u003dNone, power_on\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":29,"id":"ff570b3c_d49d980a","line":6481,"updated":"2020-05-20 17:01:47.000000000","message":"personally, I\u0027d rather we had named keywords here since there are a lot of options here now","commit_id":"083df01a4d90e155c1b4ef9e954d8be14a02d830"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0804c92ece31fc0ae86561e0a31e879a316bd23d","unresolved":false,"context_lines":[{"line_number":6485,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":6486,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":6487,"context_line":"                                   external_events\u003dNone,"},{"line_number":6488,"context_line":"                                   cleanup_data\u003dCLEANUP_DATA_NONE):"},{"line_number":6489,"context_line":""},{"line_number":6490,"context_line":"        \"\"\"Do required network setup and create domain.\"\"\""},{"line_number":6491,"context_line":"        timeout \u003d CONF.vif_plugging_timeout"}],"source_content_type":"text/x-python","patch_set":29,"id":"ff570b3c_542068f7","line":6488,"range":{"start_line":6488,"start_character":35,"end_line":6488,"end_character":65},"updated":"2020-05-20 17:01:47.000000000","message":"This also applies much higher up, but what\u0027s the rationale for nested this instead of providing two boolean variables, e.g. \u0027clean_instance_dir\u0027 and \u0027clean_disks\u0027 (possibly with a \u0027_on_failure\u0027 suffix though that\u0027s getting real wordy)?","commit_id":"083df01a4d90e155c1b4ef9e954d8be14a02d830"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"2d28cea53654208539d1a172090d5cd1c7786054","unresolved":false,"context_lines":[{"line_number":3592,"context_line":"            context, xml, instance, network_info,"},{"line_number":3593,"context_line":"            block_device_info\u003dblock_device_info,"},{"line_number":3594,"context_line":"            post_xml_callback\u003dgen_confdrive,"},{"line_number":3595,"context_line":"            destroy_disks_on_failure\u003dTrue,"},{"line_number":3596,"context_line":"            power_on\u003dpower_on)"},{"line_number":3597,"context_line":"        LOG.debug(\"Guest created on hypervisor\", instance\u003dinstance)"},{"line_number":3598,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"ff570b3c_459a6213","side":"PARENT","line":3595,"range":{"start_line":3595,"start_character":0,"end_line":3595,"end_character":42},"updated":"2020-05-21 11:26:03.000000000","message":"destroy_disks_on_failure is removed here.","commit_id":"cbbd36aeacb19cab20c55ca3f727f452b4e0565e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"51f9021e0f47016028d3991614f1906c52268faf","unresolved":false,"context_lines":[{"line_number":6477,"context_line":"            if guest and guest.is_active():"},{"line_number":6478,"context_line":"                guest.poweroff()"},{"line_number":6479,"context_line":"        finally:"},{"line_number":6480,"context_line":"            self._cleanup(context, instance, network_info, block_device_info,"},{"line_number":6481,"context_line":"                          True, cleanup_instance_dir\u003dcleanup_instance_dir,"},{"line_number":6482,"context_line":"                          cleanup_instance_disks\u003dcleanup_instance_disks)"},{"line_number":6483,"context_line":""},{"line_number":6484,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"}],"source_content_type":"text/x-python","patch_set":32,"id":"ff570b3c_9c705074","line":6481,"range":{"start_line":6480,"start_character":46,"end_line":6481,"end_character":32},"updated":"2020-05-21 16:32:55.000000000","message":"You still seem to have dropped the keywords here. Any reason we have to do that? There\u0027s a lot going on here","commit_id":"ee1447ed440e6ea3a300489caec7af240524adb1"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"95fa003bfc1a910b0486ccca8b89e9eb7c130acb","unresolved":false,"context_lines":[{"line_number":6477,"context_line":"            if guest and guest.is_active():"},{"line_number":6478,"context_line":"                guest.poweroff()"},{"line_number":6479,"context_line":"        finally:"},{"line_number":6480,"context_line":"            self._cleanup(context, instance, network_info, block_device_info,"},{"line_number":6481,"context_line":"                          True, cleanup_instance_dir\u003dcleanup_instance_dir,"},{"line_number":6482,"context_line":"                          cleanup_instance_disks\u003dcleanup_instance_disks)"},{"line_number":6483,"context_line":""},{"line_number":6484,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"}],"source_content_type":"text/x-python","patch_set":32,"id":"ff570b3c_8751e265","line":6481,"range":{"start_line":6480,"start_character":46,"end_line":6481,"end_character":32},"in_reply_to":"ff570b3c_9c705074","updated":"2020-05-26 09:37:05.000000000","message":"Looks like Matt did this when moving between the public cleanup and private _cleanup methods. I\u0027ll respin and keep kwargs the same across both.","commit_id":"ee1447ed440e6ea3a300489caec7af240524adb1"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"51f9021e0f47016028d3991614f1906c52268faf","unresolved":false,"context_lines":[{"line_number":6518,"context_line":"                self._cleanup_failed_start("},{"line_number":6519,"context_line":"                    context, instance, network_info, block_device_info, guest,"},{"line_number":6520,"context_line":"                    cleanup_instance_dir\u003dcleanup_instance_dir,"},{"line_number":6521,"context_line":"                    cleanup_instance_disks\u003dcleanup_instance_disks)"},{"line_number":6522,"context_line":"        except eventlet.timeout.Timeout:"},{"line_number":6523,"context_line":"            # We never heard from Neutron"},{"line_number":6524,"context_line":"            LOG.warning(\u0027Timeout waiting for %(events)s for \u0027"}],"source_content_type":"text/x-python","patch_set":32,"id":"ff570b3c_9cf5f0cd","line":6521,"updated":"2020-05-21 16:32:55.000000000","message":"there were never keywords here, though I sure wish there were","commit_id":"ee1447ed440e6ea3a300489caec7af240524adb1"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b941d08408de6f30e6907b47c4e72b2c1604e442","unresolved":false,"context_lines":[{"line_number":1343,"context_line":"                    cleanup_instance_dir \u003d CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027"},{"line_number":1344,"context_line":""},{"line_number":1345,"context_line":"        return self._cleanup("},{"line_number":1346,"context_line":"                context, instance, network_info,"},{"line_number":1347,"context_line":"                block_device_info\u003dblock_device_info,"},{"line_number":1348,"context_line":"                destroy_vifs\u003ddestroy_vifs,"},{"line_number":1349,"context_line":"                cleanup_instance_dir\u003dcleanup_instance_dir,"}],"source_content_type":"text/x-python","patch_set":33,"id":"ff570b3c_199a55c4","line":1346,"range":{"start_line":1346,"start_character":12,"end_line":1346,"end_character":16},"updated":"2020-05-26 13:48:11.000000000","message":"nit","commit_id":"497360b0ea970f1e68912be8229ef8c3f5454e9e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b941d08408de6f30e6907b47c4e72b2c1604e442","unresolved":false,"context_lines":[{"line_number":3603,"context_line":"                                          context, instance,"},{"line_number":3604,"context_line":"                                          injection_info)"},{"line_number":3605,"context_line":"        created_instance_dir, created_disks \u003d self._create_image("},{"line_number":3606,"context_line":"                context, instance, disk_info[\u0027mapping\u0027],"},{"line_number":3607,"context_line":"                injection_info\u003dinjection_info,"},{"line_number":3608,"context_line":"                block_device_info\u003dblock_device_info)"},{"line_number":3609,"context_line":""}],"source_content_type":"text/x-python","patch_set":33,"id":"ff570b3c_59a4cd08","line":3606,"range":{"start_line":3606,"start_character":12,"end_line":3606,"end_character":16},"updated":"2020-05-26 13:48:11.000000000","message":"nit","commit_id":"497360b0ea970f1e68912be8229ef8c3f5454e9e"}]}