)]}'
{"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c8ae8a4b35025eaadb7855334c09d53e0f1174a9","unresolved":false,"context_lines":[{"line_number":14998,"context_line":"                srcfile, 1 * units.Gi / units.Ki)"},{"line_number":14999,"context_line":"            mock_define.assert_called_once_with(mock.sentinel.new_xml)"},{"line_number":15000,"context_line":""},{"line_number":15001,"context_line":"    def test_swap_volume_rebase_fail(self):"},{"line_number":15002,"context_line":"        # If the rebase fails, make sure we use the original domain XML to"},{"line_number":15003,"context_line":"        # redefine the domain."},{"line_number":15004,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f287b81_21be0665","line":15001,"updated":"2017-09-06 13:30:13.000000000","message":"This is screaming for code re-use. Apart from anything else it would help highlight the differences between the tests.","commit_id":"b1d7e5aed8381e9cc282ad7fe34ce25b7abbb1dc"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c8ae8a4b35025eaadb7855334c09d53e0f1174a9","unresolved":false,"context_lines":[{"line_number":15047,"context_line":"            srcfile \u003d \"/first/path\""},{"line_number":15048,"context_line":"            dstfile \u003d \"/second/path\""},{"line_number":15049,"context_line":""},{"line_number":15050,"context_line":"            mock_dom.XMLDesc.side_effect \u003d [mock.sentinel.orig_xml,"},{"line_number":15051,"context_line":"                                            mock.sentinel.new_xml]"},{"line_number":15052,"context_line":"            mock_dom.isPersistent.return_value \u003d True"},{"line_number":15053,"context_line":"            mock_is_job_complete.return_value \u003d True"},{"line_number":15054,"context_line":"            mock_dom.blockResize.side_effect \u003d test.TestingException()"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f287b81_c2b5fdef","line":15051,"range":{"start_line":15050,"start_character":0,"end_line":15051,"end_character":66},"updated":"2017-09-06 13:30:13.000000000","message":"I prefer your fake_rebase from above as it seems more robust to refactoring, but probably not worth changing.","commit_id":"b1d7e5aed8381e9cc282ad7fe34ce25b7abbb1dc"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d53a73d06bcde6a52105f7f0dff27daf2e28a269","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"        try:"},{"line_number":1280,"context_line":"            # NOTE (rmk): blockRebase cannot be executed on persistent"},{"line_number":1281,"context_line":"            #             domains, so we need to temporarily undefine it."},{"line_number":1282,"context_line":"            #             If any part of this block fails, the domain is"},{"line_number":1283,"context_line":"            #             re-defined regardless."},{"line_number":1284,"context_line":"            if guest.has_persistent_configuration():"},{"line_number":1285,"context_line":"                support_uefi \u003d self._has_uefi_support()"},{"line_number":1286,"context_line":"                guest.delete_configuration(support_uefi)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_762fdd87","line":1283,"range":{"start_line":1282,"start_character":26,"end_line":1283,"end_character":48},"updated":"2017-08-29 23:05:12.000000000","message":"Is this a problem now?","commit_id":"52d1e4f5044c762f94f41da29ce6616ba7b82c83"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"6fe5682c3045a0a4374cbb6478b6fc83b7def515","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"        try:"},{"line_number":1280,"context_line":"            # NOTE (rmk): blockRebase cannot be executed on persistent"},{"line_number":1281,"context_line":"            #             domains, so we need to temporarily undefine it."},{"line_number":1282,"context_line":"            #             If any part of this block fails, the domain is"},{"line_number":1283,"context_line":"            #             re-defined regardless."},{"line_number":1284,"context_line":"            if guest.has_persistent_configuration():"},{"line_number":1285,"context_line":"                support_uefi \u003d self._has_uefi_support()"},{"line_number":1286,"context_line":"                guest.delete_configuration(support_uefi)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_6097fa49","line":1283,"range":{"start_line":1282,"start_character":26,"end_line":1283,"end_character":48},"in_reply_to":"7f287b81_56bf41d0","updated":"2017-08-30 17:00:18.000000000","message":"First, yes, undefine / redefining the guest is mandatory with the current behavior of blockRebase() API.  And the part Matt R highlighted is still necessary in the event of the failure of blockRebase().\n\nSecond, the approach Melanie is taking sounds correct -- for the swapped volume to reflect, the live domain config ought to be used.  And as Melanie mentioned on IRC (#openstack-nova, Freenode), the original guest XML will be saved to handle the scenario where blockRebase() fails.\n\nWould be nice if Matthew Booth can confirm this, too.","commit_id":"52d1e4f5044c762f94f41da29ce6616ba7b82c83"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"932704177f052f15f3d0edd59b04e364a704adc7","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"        try:"},{"line_number":1280,"context_line":"            # NOTE (rmk): blockRebase cannot be executed on persistent"},{"line_number":1281,"context_line":"            #             domains, so we need to temporarily undefine it."},{"line_number":1282,"context_line":"            #             If any part of this block fails, the domain is"},{"line_number":1283,"context_line":"            #             re-defined regardless."},{"line_number":1284,"context_line":"            if guest.has_persistent_configuration():"},{"line_number":1285,"context_line":"                support_uefi \u003d self._has_uefi_support()"},{"line_number":1286,"context_line":"                guest.delete_configuration(support_uefi)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_76447d47","line":1283,"range":{"start_line":1282,"start_character":26,"end_line":1283,"end_character":48},"in_reply_to":"7f287b81_762fdd87","updated":"2017-08-29 23:06:04.000000000","message":"Meaning, if we remove the uefi configuration and then save that, aren\u0027t we hosed for that later?","commit_id":"52d1e4f5044c762f94f41da29ce6616ba7b82c83"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3f7a7d320d352075263c4370fedfdb3517d627bc","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"        try:"},{"line_number":1280,"context_line":"            # NOTE (rmk): blockRebase cannot be executed on persistent"},{"line_number":1281,"context_line":"            #             domains, so we need to temporarily undefine it."},{"line_number":1282,"context_line":"            #             If any part of this block fails, the domain is"},{"line_number":1283,"context_line":"            #             re-defined regardless."},{"line_number":1284,"context_line":"            if guest.has_persistent_configuration():"},{"line_number":1285,"context_line":"                support_uefi \u003d self._has_uefi_support()"},{"line_number":1286,"context_line":"                guest.delete_configuration(support_uefi)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_56bf41d0","line":1283,"range":{"start_line":1282,"start_character":26,"end_line":1283,"end_character":48},"in_reply_to":"7f287b81_76447d47","updated":"2017-08-29 23:20:37.000000000","message":"The guest.delete_configuration call is undefining the domain and support_uefi is a just flag for that. IIUC, undefining the domain removes the entire persistent config, which is separate from the live config from what I understand.\n\nI tested my patch with devstack and a persistent domain and things worked, so it did undefine the domain and then define it again (I changed this to read the live config instead of the persistent config). I intended to read the live config and write it back as the persistent config.\n\nI\u0027m going to consult with mdbooth and/or lyarwood to make sure this is the right way to do it.","commit_id":"52d1e4f5044c762f94f41da29ce6616ba7b82c83"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ffed5def5c7cecafe80d9f678469c528d646d0f9","unresolved":false,"context_lines":[{"line_number":1308,"context_line":"                dev.resize(resize_to * units.Gi / units.Ki)"},{"line_number":1309,"context_line":"        finally:"},{"line_number":1310,"context_line":"            # Make sure we define the updated instance in libvirt"},{"line_number":1311,"context_line":"            xml \u003d guest.get_xml_desc(dump_sensitive\u003dTrue)"},{"line_number":1312,"context_line":"            self._host.write_instance_config(xml)"},{"line_number":1313,"context_line":""},{"line_number":1314,"context_line":"    def swap_volume(self, old_connection_info,"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_206c529a","line":1311,"updated":"2017-08-30 16:52:19.000000000","message":"I was thinking maybe we should save the original XML at the beginning as was done previously and define the domain using that if we encounter any exceptions during the rebase/resize, to be extra safe. In the success scenario, it seems like using the live config to define the domain is the only way to reflect the new volume in the persistent config.","commit_id":"52d1e4f5044c762f94f41da29ce6616ba7b82c83"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"190c976c31230fb8c63a8a6f669a25e108a4b543","unresolved":false,"context_lines":[{"line_number":1315,"context_line":"            # configuration after the volume was swapped."},{"line_number":1316,"context_line":"            xml \u003d guest.get_xml_desc(dump_sensitive\u003dTrue)"},{"line_number":1317,"context_line":"        finally:"},{"line_number":1318,"context_line":"            self._host.write_instance_config(xml)"},{"line_number":1319,"context_line":""},{"line_number":1320,"context_line":"    def swap_volume(self, old_connection_info,"},{"line_number":1321,"context_line":"                    new_connection_info, instance, mountpoint, resize_to):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f287b81_69fe5306","line":1318,"updated":"2017-09-06 09:32:49.000000000","message":"Here\u0027s a fun observation. I don\u0027t see any task exclusion around swap_volume, i.e. it seems swap_volume can run concurrent with other operations like add/remove volume, and other invocations of swap_volume. If a failure occurred we\u0027d blow all other changes away and return to the domain xml prior to this swap_volume, which would be bad.\n\nHopefully I\u0027ve just missed where we\u0027re exluding other operations during swap_volume, but if I haven\u0027t we should either add it or fix how we do this.","commit_id":"b1d7e5aed8381e9cc282ad7fe34ce25b7abbb1dc"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"190c976c31230fb8c63a8a6f669a25e108a4b543","unresolved":false,"context_lines":[{"line_number":1313,"context_line":""},{"line_number":1314,"context_line":"            # Make sure we will redefine the domain using the updated"},{"line_number":1315,"context_line":"            # configuration after the volume was swapped."},{"line_number":1316,"context_line":"            xml \u003d guest.get_xml_desc(dump_sensitive\u003dTrue)"},{"line_number":1317,"context_line":"        finally:"},{"line_number":1318,"context_line":"            self._host.write_instance_config(xml)"},{"line_number":1319,"context_line":""},{"line_number":1320,"context_line":"    def swap_volume(self, old_connection_info,"},{"line_number":1321,"context_line":"                    new_connection_info, instance, mountpoint, resize_to):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f287b81_4978378e","line":1318,"range":{"start_line":1316,"start_character":0,"end_line":1318,"end_character":49},"updated":"2017-09-06 09:32:49.000000000","message":"This is a race unless we have locking around this whole operation, which I can\u0027t see. It\u0027s a smaller race than the existing mega-race, though, so I wouldn\u0027t block for this.","commit_id":"b1d7e5aed8381e9cc282ad7fe34ce25b7abbb1dc"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ffc03b3c1549a1808b08ffb5203d28bf4932f6a4","unresolved":false,"context_lines":[{"line_number":1318,"context_line":"            if resize_to:"},{"line_number":1319,"context_line":"                dev.resize(resize_to * units.Gi / units.Ki)"},{"line_number":1320,"context_line":""},{"line_number":1321,"context_line":"            # Make sure we will redefine the domain using the updated"},{"line_number":1322,"context_line":"            # configuration after the volume was swapped."},{"line_number":1323,"context_line":"            xml \u003d guest.get_xml_desc(dump_sensitive\u003dTrue)"},{"line_number":1324,"context_line":"        finally:"},{"line_number":1325,"context_line":"            self._host.write_instance_config(xml)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_bf240b5f","line":1322,"range":{"start_line":1321,"start_character":0,"end_line":1322,"end_character":57},"updated":"2017-10-25 16:33:50.000000000","message":"A note about why \u0027dump_inactive\u0027 isn\u0027t necessary here would be helpful:\n\n  \u003cmelwitt\u003e stephenfin: that controls whether we pull the\n  active (live) or inactive (persistent) config from the\n  domain. we want to pull the live config after the volume\n  was updated to write it back to the persistent config","commit_id":"8816606d02af2b9e46a901ee9d114a0e5574f21d"}]}