)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"909ffcffc692ecb16b7092f872806e8fbec28f3c","unresolved":false,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Closes-Bug: #1727260"},{"line_number":22,"context_line":"Change-Id: I4182642aab3fd2ffb1c97d2de9bdca58982289d8"},{"line_number":23,"context_line":"Signed-off-by: Sahid Orentino Ferdjaoui \u003csahid.ferdjaoui@redhat.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df87a7cf_2d7a6176","line":23,"range":{"start_line":23,"start_character":0,"end_line":23,"end_character":68},"updated":"2017-12-19 09:34:27.000000000","message":"supernit - Not required.","commit_id":"263dc394c236b91799743384246f6a33eb8d7715"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"88803bb9161aa542b411b3b30a3b9701db59d228","unresolved":false,"context_lines":[{"line_number":15,"context_line":"the process whether the device is not found in domain definition but"},{"line_number":16,"context_line":"try to disconnect the logical device from host."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This commit makes the process to attempt a disconnect volume even if"},{"line_number":19,"context_line":"the device is not attached to the guest."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Closes-Bug: #1727260"},{"line_number":22,"context_line":"Change-Id: I4182642aab3fd2ffb1c97d2de9bdca58982289d8"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9f91af0f_40869568","line":19,"range":{"start_line":18,"start_character":0,"end_line":19,"end_character":40},"updated":"2018-01-08 15:30:13.000000000","message":"nit - if you respin also include something about the warning message and encryptor detach that now also takes place.","commit_id":"2688ef88d0a0f6e734d8e6ca5dcc607a3dc34886"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"88803bb9161aa542b411b3b30a3b9701db59d228","unresolved":false,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Closes-Bug: #1727260"},{"line_number":22,"context_line":"Change-Id: I4182642aab3fd2ffb1c97d2de9bdca58982289d8"},{"line_number":23,"context_line":"Signed-off-by: Sahid Orentino Ferdjaoui \u003csahid.ferdjaoui@redhat.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9f91af0f_00a3ad01","line":23,"range":{"start_line":23,"start_character":0,"end_line":23,"end_character":68},"updated":"2018-01-08 15:30:13.000000000","message":"supernit - not required.","commit_id":"2688ef88d0a0f6e734d8e6ca5dcc607a3dc34886"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e59a5531928f9f5bec1037d94ff091ae3b25d577","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"libvirt: disconnect volume from host during detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under certain failure scenarios it may be that although the libvirt"},{"line_number":10,"context_line":"definition for the volume has been removed for the instance that the"},{"line_number":11,"context_line":"associated storage lun on the compute server may not have been fully"},{"line_number":12,"context_line":"cleaned up yet."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"3fa0c359_6cc1b554","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":31},"updated":"2018-02-06 16:40:56.000000000","message":"From https://bugs.launchpad.net/nova/+bug/1727260/comments/1 I guess one is that we suck at handling detach with multipath devices, and hit a type of \u0027in use\u0027 error and don\u0027t retry on that within the code.\n\nSo in this scenario:\n\n1. detach volume; updates the guest xml so the device is no longer there\n2. disconnect the volume using os-brick - this fails b/c the path is in use\n3. user tries to detach the volume again, which now fails because we said the disk wasn\u0027t found in the guest because it was removed in #1\n\nRight?\n\n#3 could still fail again even with this fix if we still hit the same \u0027in use\u0027 error when disconnecting the volume, so we\u0027re just sort of papering over the actual root problem and letting the user have another chance at removing the volume. I guess we at least store a fault record on the instance in ComputeManager.detach_volume so the user/admin can see that the default failed and maybe why it failed (only the admin would see the traceback details though in the fault).","commit_id":"326aa1b3c1b547154e6715a422cfcfb077bf176c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"17b3b6b4a1978359e0ef2764886c379e9a3e3c9b","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"libvirt: disconnect volume from host during detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under certain failure scenarios it may be that although the libvirt"},{"line_number":10,"context_line":"definition for the volume has been removed for the instance that the"},{"line_number":11,"context_line":"associated storage lun on the compute server may not have been fully"},{"line_number":12,"context_line":"cleaned up yet."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"3fa0c359_68c99a76","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":31},"in_reply_to":"3fa0c359_6cc1b554","updated":"2018-02-06 22:19:34.000000000","message":"No, step 3 succeeds for the user because the libvirt driver thinks \"already detached, nothing to do\" when the device was never detached from the compute host. The device was only detached from the guest. So the device is *still attached* to the compute host when we terminate the connection in cinder, which is wrong, and to make things worse, we report it as \"success\" to the user.\n\nThere are a few steps in the detach: detach from guest, detach from compute host, terminate in cinder. We need to make sure we don\u0027t miss a step in that process.\n\nThe failure to detach from the compute host \"map in use\" occurred as an expected behavior when using LVM volumes while also using multipath. Not an expert on this, but from what I understand, what happened downstream was that during an LVM scan, a volume detach was tried, and so the detach failed because \"map in use\". This meant the detach just needed to be tried again later, when the volume was no longer being scanned by LVM.\n\nSo what we\u0027re trying to do here is enable each step in the detach process to be retried. If detach from the guest succeeded but detach from the compute host failed, we want a second attempt to detach to try the detach from the compute host again, and when all steps succeed, that is when we signal success to the user.","commit_id":"326aa1b3c1b547154e6715a422cfcfb077bf176c"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3091f046289ef8b4420e253deb1c58ba802aea82","unresolved":false,"context_lines":[{"line_number":6666,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"},{"line_number":6667,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6668,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host._get_domain\u0027)"},{"line_number":6669,"context_line":"    def test_detach_volume_disk_not_found_encrypton(self, mock_get_domain,"},{"line_number":6670,"context_line":"                                                    mock_disconnect_volume,"},{"line_number":6671,"context_line":"                                                    mock_get_encryptor):"},{"line_number":6672,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f4b6375_618c2a35","line":6669,"range":{"start_line":6669,"start_character":42,"end_line":6669,"end_character":51},"updated":"2017-10-31 17:41:05.000000000","message":"typo \"encryption\" and a better name might be \"test_detach_volume_disk_not_found_encryption_ignores_wrong_device\" being that we also need another test for the case where ProcessExecutionError should be reraised (exit code !\u003d 4).","commit_id":"7bec92ae1b98fba4bb23ffb2e721b0391534dea9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3091f046289ef8b4420e253deb1c58ba802aea82","unresolved":false,"context_lines":[{"line_number":6678,"context_line":"        mock_dom \u003d mock.MagicMock(return_value\u003dmock_xml_without_disk)"},{"line_number":6679,"context_line":"        mock_encryptor \u003d mock.MagicMock()"},{"line_number":6680,"context_line":"        mock_encryptor.detach_volume \u003d mock.MagicMock("},{"line_number":6681,"context_line":"            side_effect\u003dprocessutils.ProcessExecutionError)"},{"line_number":6682,"context_line":"        mock_get_encryptor.return_value \u003d mock_encryptor"},{"line_number":6683,"context_line":""},{"line_number":6684,"context_line":"        connection_info \u003d {\"driver_volume_type\": \"fake\","}],"source_content_type":"text/x-python","patch_set":3,"id":"3f4b6375_c4208cfa","line":6681,"range":{"start_line":6681,"start_character":37,"end_line":6681,"end_character":58},"updated":"2017-10-31 17:41:05.000000000","message":"You can do ProcessExecutionError(exit_code\u003dN) to set the exit code. I think you\u0027ll need to do that for the new test that verifies ProcessExecutionError is reraised if exit_code !\u003d 4.","commit_id":"7bec92ae1b98fba4bb23ffb2e721b0391534dea9"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"b3ae2661de86727f69d7514e045ef39864e3afb7","unresolved":false,"context_lines":[{"line_number":6678,"context_line":"        mock_dom \u003d mock.MagicMock(return_value\u003dmock_xml_without_disk)"},{"line_number":6679,"context_line":"        mock_encryptor \u003d mock.MagicMock()"},{"line_number":6680,"context_line":"        mock_encryptor.detach_volume \u003d mock.MagicMock("},{"line_number":6681,"context_line":"            side_effect\u003dprocessutils.ProcessExecutionError)"},{"line_number":6682,"context_line":"        mock_get_encryptor.return_value \u003d mock_encryptor"},{"line_number":6683,"context_line":""},{"line_number":6684,"context_line":"        connection_info \u003d {\"driver_volume_type\": \"fake\","}],"source_content_type":"text/x-python","patch_set":3,"id":"1f485f77_76b88329","line":6681,"range":{"start_line":6681,"start_character":37,"end_line":6681,"end_character":58},"in_reply_to":"3f4b6375_c4208cfa","updated":"2017-11-13 11:36:30.000000000","message":"I think we want it to continue anyway for all cases","commit_id":"7bec92ae1b98fba4bb23ffb2e721b0391534dea9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3091f046289ef8b4420e253deb1c58ba802aea82","unresolved":false,"context_lines":[{"line_number":6692,"context_line":"        drvr.detach_volume(connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6693,"context_line":"                           mock_encryptor)"},{"line_number":6694,"context_line":""},{"line_number":6695,"context_line":"        mock_get_domain.assert_called_once_with(instance)"},{"line_number":6696,"context_line":"        mock_disconnect_volume.assert_called_once_with("},{"line_number":6697,"context_line":"            connection_info, \"vdc\", instance)"},{"line_number":6698,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3f4b6375_842734b7","line":6695,"updated":"2017-10-31 17:41:05.000000000","message":"I think we should also assert that mock_get_encryptor and mock_encryptor.detach_volume were called since they are guarded by \"if encryption\". Could be self.assertTrue(mock_get_encryptor.called) and self.assertTrue(mock_encryptor.detach_volume.called) for example.","commit_id":"7bec92ae1b98fba4bb23ffb2e721b0391534dea9"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"77f9be334f3bd765329ac2f88ed5a51e0c44dd52","unresolved":false,"context_lines":[{"line_number":6694,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"},{"line_number":6695,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6696,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host._get_domain\u0027)"},{"line_number":6697,"context_line":"    def test_detach_volume_disk_not_found_encrypton(self, mock_get_domain,"},{"line_number":6698,"context_line":"                                                    mock_disconnect_volume,"},{"line_number":6699,"context_line":"                                                    mock_get_encryptor):"},{"line_number":6700,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_780c3794","line":6697,"range":{"start_line":6697,"start_character":42,"end_line":6697,"end_character":51},"updated":"2017-12-13 11:09:29.000000000","message":"encryption","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"77f9be334f3bd765329ac2f88ed5a51e0c44dd52","unresolved":false,"context_lines":[{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_38546f54","line":6723,"range":{"start_line":6720,"start_character":0,"end_line":6723,"end_character":27},"updated":"2017-12-13 11:09:29.000000000","message":"This is rather useless. We\u0027re mostly checking the normal flow of the code and then checking that we raise the exception which we have explicitly raised above. This test would make sense if we were actually doing somethig with the exception (i.e. handling it). As things stand though, I\u0027d rather just keep the check for \u0027detach_volume\u0027 above and add a check for the logging","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ab4d05d31a408ceed279ffe472522268f0496666","unresolved":false,"context_lines":[{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_87f18f04","line":6723,"range":{"start_line":6720,"start_character":0,"end_line":6723,"end_character":27},"in_reply_to":"df87a7cf_38546f54","updated":"2017-12-13 16:50:59.000000000","message":"Hm, yeah. I guess really what this test should be doing is verifying that we continue on to do the encryptor disconnect even if we get DeviceNotFound during the guest detach.\n\nSo I think we should remove the ProcessExecutionError from this test and then assert that mock_encryptor.detach_volume was called and mock_disconnect_volume was called. What do you think, Stephen?","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"7c1491bb48293cb41b61d730cacad5049ae99eea","unresolved":false,"context_lines":[{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_dab704f5","line":6723,"range":{"start_line":6720,"start_character":0,"end_line":6723,"end_character":27},"in_reply_to":"df87a7cf_87974f3b","updated":"2017-12-18 12:58:13.000000000","message":"This assertion is not useless at all, it\u0027s assert the process which is basically what we want with an unit test. It\u0027s important to assert the function driver.detach_volume is well re-reasing that exception to the manager and so does not hide it.","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3c806db78094605c1876b3e292cfe28e66961e76","unresolved":false,"context_lines":[{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_c73557aa","line":6723,"range":{"start_line":6720,"start_character":0,"end_line":6723,"end_character":27},"in_reply_to":"df87a7cf_87f18f04","updated":"2017-12-13 16:52:48.000000000","message":"I had meant to mention in my previous comment that this test *is* indirectly already doing what I suggested, it\u0027s verifying that we do the encryptor detach even after DeviceNotFound, but I agree that it would be better to remove the unnecessary ProcessExecutionError stuff.","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ce5ae7663daa2b6890228547bbcb0af5c5be7a92","unresolved":false,"context_lines":[{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_87974f3b","line":6723,"range":{"start_line":6720,"start_character":0,"end_line":6723,"end_character":27},"in_reply_to":"df87a7cf_c73557aa","updated":"2017-12-13 17:03:48.000000000","message":"Indeed. I actually thought we were doing this already on L6688, but that\u0027s a different \u0027detach_volume\u0027 :) We could fold in the assertion to the above function but, just changing this test to assert that this was called also sounds good to me","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"909ffcffc692ecb16b7092f872806e8fbec28f3c","unresolved":false,"context_lines":[{"line_number":6694,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"},{"line_number":6695,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6696,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host._get_domain\u0027)"},{"line_number":6697,"context_line":"    def test_detach_volume_disk_not_found_encrypton(self, mock_get_domain,"},{"line_number":6698,"context_line":"                                                    mock_disconnect_volume,"},{"line_number":6699,"context_line":"                                                    mock_get_encryptor):"},{"line_number":6700,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"}],"source_content_type":"text/x-python","patch_set":6,"id":"df87a7cf_ed69daf7","line":6697,"range":{"start_line":6697,"start_character":42,"end_line":6697,"end_character":51},"updated":"2017-12-19 09:34:27.000000000","message":"nit - encryption","commit_id":"263dc394c236b91799743384246f6a33eb8d7715"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"909ffcffc692ecb16b7092f872806e8fbec28f3c","unresolved":false,"context_lines":[{"line_number":6703,"context_line":"  \u003cdevices\u003e"},{"line_number":6704,"context_line":"  \u003c/devices\u003e"},{"line_number":6705,"context_line":"\u003c/domain\u003e\"\"\""},{"line_number":6706,"context_line":"        mock_dom \u003d mock.MagicMock(return_value\u003dmock_xml_without_disk)"},{"line_number":6707,"context_line":"        mock_encryptor \u003d mock.MagicMock()"},{"line_number":6708,"context_line":"        mock_encryptor.detach_volume \u003d mock.MagicMock("},{"line_number":6709,"context_line":"            side_effect\u003dprocessutils.ProcessExecutionError)"},{"line_number":6710,"context_line":"        mock_get_encryptor.return_value \u003d mock_encryptor"},{"line_number":6711,"context_line":""},{"line_number":6712,"context_line":"        connection_info \u003d {\"driver_volume_type\": \"fake\","},{"line_number":6713,"context_line":"                           \"data\": {\"device_path\": \"/fake\","},{"line_number":6714,"context_line":"                                    \"access_mode\": \"rw\"}}"},{"line_number":6715,"context_line":""},{"line_number":6716,"context_line":"        mock_dom.info.return_value \u003d [power_state.RUNNING, 512, 512, 2, 1234,"},{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"df87a7cf_cd5c151b","line":6723,"range":{"start_line":6706,"start_character":0,"end_line":6723,"end_character":27},"updated":"2017-12-19 09:34:27.000000000","message":"Given the commit message I don\u0027t see the need to test this, shouldn\u0027t we just be asserting that encryptor.detach_volume is called even when there is no disk attached to the domain and an encryption dict is provided?","commit_id":"263dc394c236b91799743384246f6a33eb8d7715"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f0af19c09a199ccf11d2ba5b0f460dceb83043bc","unresolved":false,"context_lines":[{"line_number":6703,"context_line":"  \u003cdevices\u003e"},{"line_number":6704,"context_line":"  \u003c/devices\u003e"},{"line_number":6705,"context_line":"\u003c/domain\u003e\"\"\""},{"line_number":6706,"context_line":"        mock_dom \u003d mock.MagicMock(return_value\u003dmock_xml_without_disk)"},{"line_number":6707,"context_line":"        mock_encryptor \u003d mock.MagicMock()"},{"line_number":6708,"context_line":"        mock_encryptor.detach_volume \u003d mock.MagicMock("},{"line_number":6709,"context_line":"            side_effect\u003dprocessutils.ProcessExecutionError)"},{"line_number":6710,"context_line":"        mock_get_encryptor.return_value \u003d mock_encryptor"},{"line_number":6711,"context_line":""},{"line_number":6712,"context_line":"        connection_info \u003d {\"driver_volume_type\": \"fake\","},{"line_number":6713,"context_line":"                           \"data\": {\"device_path\": \"/fake\","},{"line_number":6714,"context_line":"                                    \"access_mode\": \"rw\"}}"},{"line_number":6715,"context_line":""},{"line_number":6716,"context_line":"        mock_dom.info.return_value \u003d [power_state.RUNNING, 512, 512, 2, 1234,"},{"line_number":6717,"context_line":"                                      5678]"},{"line_number":6718,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6719,"context_line":""},{"line_number":6720,"context_line":"        self.assertRaises("},{"line_number":6721,"context_line":"            processutils.ProcessExecutionError,"},{"line_number":6722,"context_line":"            drvr.detach_volume, connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6723,"context_line":"            mock_encryptor)"},{"line_number":6724,"context_line":""},{"line_number":6725,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6726,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"df87a7cf_d9037241","line":6723,"range":{"start_line":6706,"start_character":0,"end_line":6723,"end_character":27},"in_reply_to":"df87a7cf_cd5c151b","updated":"2017-12-19 16:18:30.000000000","message":"Agreed, I commented on PS5, that this should remove the ProcessExecutionError stuff and instead assert that mock_encryptor.detach_volume and mock_disconnect_volume were called. The ProcessExecutionError assertion adds unnecessary complexity, IMHO. We just want to test that encryptor detach is attempted even when we get DeviceNotFound.\n\nGiven there\u0027s no change to the exception handling of the encryptor piece, I don\u0027t think we need to verify that exceptions will propagate up. I think we can assume they do as there\u0027s no try-except around it.","commit_id":"263dc394c236b91799743384246f6a33eb8d7715"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"88803bb9161aa542b411b3b30a3b9701db59d228","unresolved":false,"context_lines":[{"line_number":6706,"context_line":"        mock_dom.info.return_value \u003d [power_state.RUNNING, 512, 512, 2, 1234,"},{"line_number":6707,"context_line":"                                      5678]"},{"line_number":6708,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6709,"context_line":""},{"line_number":6710,"context_line":"        drvr.detach_volume(connection_info, instance, \u0027/dev/vdc\u0027)"},{"line_number":6711,"context_line":""},{"line_number":6712,"context_line":"        mock_get_domain.assert_called_once_with(instance)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f91af0f_2d490a8a","line":6709,"updated":"2018-01-08 15:30:13.000000000","message":"supernit - whitespace","commit_id":"2688ef88d0a0f6e734d8e6ca5dcc607a3dc34886"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"88803bb9161aa542b411b3b30a3b9701db59d228","unresolved":false,"context_lines":[{"line_number":6740,"context_line":""},{"line_number":6741,"context_line":"        drvr.detach_volume(connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6742,"context_line":"                           encryption)"},{"line_number":6743,"context_line":"        mock_encryptor.detach_volume.assert_called_once_with(**encryption)"},{"line_number":6744,"context_line":""},{"line_number":6745,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume\u0027)"},{"line_number":6746,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f91af0f_00208d9a","line":6743,"range":{"start_line":6743,"start_character":0,"end_line":6743,"end_character":74},"updated":"2018-01-08 15:30:13.000000000","message":"nit - we could mock and assert that disconnect_volume is also called here.","commit_id":"2688ef88d0a0f6e734d8e6ca5dcc607a3dc34886"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"6b1a947241b39763082eaad9a2471a5fb7e80854","unresolved":false,"context_lines":[{"line_number":6962,"context_line":"                                      5678]"},{"line_number":6963,"context_line":"        mock_get_domain.return_value \u003d mock_dom"},{"line_number":6964,"context_line":""},{"line_number":6965,"context_line":"        drvr.detach_volume(connection_info, instance, \u0027/dev/vdc\u0027,"},{"line_number":6966,"context_line":"                           encryption)"},{"line_number":6967,"context_line":""},{"line_number":6968,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_driver\u0027)"},{"line_number":6969,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor\u0027)"}],"source_content_type":"text/x-python","patch_set":11,"id":"3fa0c359_f8ac11a0","line":6966,"range":{"start_line":6965,"start_character":0,"end_line":6966,"end_character":38},"updated":"2018-02-12 16:08:03.000000000","message":"We should still assert that _disconnect_volume is called here here using encryption\u003dencryption.","commit_id":"e7ca08507875ab9f31bea9907b1cf60eb5eb015c"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"8df8fc456d5191efee7b4eddc6e29b54b0c4382c","unresolved":false,"context_lines":[{"line_number":1414,"context_line":"        if encryption:"},{"line_number":1415,"context_line":"            encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1416,"context_line":"                                                       encryption)"},{"line_number":1417,"context_line":"            encryptor.detach_volume(**encryption)"},{"line_number":1418,"context_line":"        self._disconnect_volume(connection_info, disk_dev, instance)"},{"line_number":1419,"context_line":""},{"line_number":1420,"context_line":"    def extend_volume(self, connection_info, instance):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_c24dae3e","line":1417,"updated":"2017-10-25 17:14:02.000000000","message":"Hm, I\u0027m not sure we should move this code block as part of this change. At its existing location, it would be called once in the bug scenario (detach from guest succeeds and detach from compute host L1418 fails) but if we move it, it will be called at least twice in the bug scenario (it would be called more times if an end user kept calling detach and L1418 kept failing).\n\nI\u0027m not entirely sure of the behavior of os-brick detach_volume [1] in this case as far as how it would behave if called repeatedly. The code comment says it will ignore exit code 4 but that\u0027s not true as the call is being made with check_exit_code\u003dTrue which will raise ProcessExecutionError for any non-zero exit code.\n\nSo I\u0027m thinking we should leave the code block where it was. What do you think?\n\n[1] https://github.com/openstack/os-brick/blob/master/os_brick/encryptors/cryptsetup.py#L172-L177","commit_id":"fd59cac954284b7f33a37fe318b8de7c289874dc"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"fda9a2cee8c2e18edc50feee2f5389020727b70f","unresolved":false,"context_lines":[{"line_number":1414,"context_line":"        if encryption:"},{"line_number":1415,"context_line":"            encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1416,"context_line":"                                                       encryption)"},{"line_number":1417,"context_line":"            encryptor.detach_volume(**encryption)"},{"line_number":1418,"context_line":"        self._disconnect_volume(connection_info, disk_dev, instance)"},{"line_number":1419,"context_line":""},{"line_number":1420,"context_line":"    def extend_volume(self, connection_info, instance):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_e8a079f6","line":1417,"in_reply_to":"3f4b6375_c24dae3e","updated":"2017-10-26 08:36:50.000000000","message":"Yes makes sense but in that case we could be in a situation where it will be never called.\n\nWhat about to add a \"try..catch\" around that part and log any error as warning. Then I will also add comment pointing that the os-brick code should ignore the fact that we try to destroy a non-active dm-crypt device.\n\nSounds reasonable?","commit_id":"fd59cac954284b7f33a37fe318b8de7c289874dc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d6b9f54d04a0ba2dbe790420a33fa1b1ac9d1f3e","unresolved":false,"context_lines":[{"line_number":1414,"context_line":"        if encryption:"},{"line_number":1415,"context_line":"            encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1416,"context_line":"                                                       encryption)"},{"line_number":1417,"context_line":"            encryptor.detach_volume(**encryption)"},{"line_number":1418,"context_line":"        self._disconnect_volume(connection_info, disk_dev, instance)"},{"line_number":1419,"context_line":""},{"line_number":1420,"context_line":"    def extend_volume(self, connection_info, instance):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_74433443","line":1417,"in_reply_to":"3f4b6375_e8a079f6","updated":"2017-10-26 16:44:23.000000000","message":"Good point -- I had a feeling I was missing something. Yeah, I agree try-catch with comment around this part and log a warning is the way to go. This is how the old nova code was handling the exit code 4, for reference [1].\n\n[1] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/storage/dmcrypt.py#L72-L84","commit_id":"fd59cac954284b7f33a37fe318b8de7c289874dc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3091f046289ef8b4420e253deb1c58ba802aea82","unresolved":false,"context_lines":[{"line_number":1419,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":1420,"context_line":"            if e.exit_code \u003d\u003d 4:"},{"line_number":1421,"context_line":"                LOG.debug(\"Ignoring exit code 4, volume already destroyed\")"},{"line_number":1422,"context_line":"            else:"},{"line_number":1423,"context_line":"                LOG.warning(\"Could not disconnect encrypted volume \""},{"line_number":1424,"context_line":"                            \"%(volume)s. If dm-crypt device is still active \""},{"line_number":1425,"context_line":"                            \"it will have to be destroyed manually for \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3f4b6375_e46388dc","line":1422,"updated":"2017-10-31 17:41:05.000000000","message":"We need to reraise here using excutils.save_and_reraise_exception() and LOG.error because we only want to ignore exit code 4 (wrong device specified). If it fails for any other reason, we don\u0027t want to proceed further.","commit_id":"7bec92ae1b98fba4bb23ffb2e721b0391534dea9"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"59d2c8b51f21cc1e01261df224ba406521c1bd0a","unresolved":false,"context_lines":[{"line_number":1419,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":1420,"context_line":"            if e.exit_code \u003d\u003d 4:"},{"line_number":1421,"context_line":"                LOG.debug(\"Ignoring exit code 4, volume already destroyed\")"},{"line_number":1422,"context_line":"            else:"},{"line_number":1423,"context_line":"                LOG.warning(\"Could not disconnect encrypted volume \""},{"line_number":1424,"context_line":"                            \"%(volume)s. If dm-crypt device is still active \""},{"line_number":1425,"context_line":"                            \"it will have to be destroyed manually for \""}],"source_content_type":"text/x-python","patch_set":3,"id":"1f485f77_82b76fd9","line":1422,"in_reply_to":"3f4b6375_e46388dc","updated":"2017-11-09 16:14:48.000000000","message":"\u003e We need to reraise here using excutils.save_and_reraise_exception()\n \u003e and LOG.error because we only want to ignore exit code 4 (wrong\n \u003e device specified). If it fails for any other reason, we don\u0027t want\n \u003e to proceed further.\n\nWell Actually that needs to be discussed, because if we quit early and so before to call disconnect_volume, we will be in the exact situation of this bug. That is why I prefered to log and continue.","commit_id":"7bec92ae1b98fba4bb23ffb2e721b0391534dea9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d7ddd6c7f0c6c25b8f5d941a9eb6ad14c3919688","unresolved":false,"context_lines":[{"line_number":1422,"context_line":"                encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1423,"context_line":"                                                       encryption)"},{"line_number":1424,"context_line":"                encryptor.detach_volume(**encryption)"},{"line_number":1425,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":1426,"context_line":"            if e.exit_code \u003d\u003d 4:"},{"line_number":1427,"context_line":"                LOG.debug(\"Ignoring exit code 4, volume already destroyed\")"},{"line_number":1428,"context_line":"            else:"},{"line_number":1429,"context_line":"                LOG.warning(\"Could not disconnect encrypted volume \""},{"line_number":1430,"context_line":"                            \"%(volume)s. If dm-crypt device is still active \""},{"line_number":1431,"context_line":"                            \"it will have to be destroyed manually for \""},{"line_number":1432,"context_line":"                            \"cleanup to succeed.\", {\u0027volume\u0027: disk_dev})"},{"line_number":1433,"context_line":""},{"line_number":1434,"context_line":"        self._disconnect_volume(connection_info, disk_dev, instance)"},{"line_number":1435,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_bbd45d21","line":1432,"range":{"start_line":1425,"start_character":8,"end_line":1432,"end_character":72},"updated":"2017-12-08 15:53:37.000000000","message":"As of os-brick version 2.1.1 released recently, which we have in requirements.txt [1], we have the ignore of exit code 4 built into os-brick [2][3]. So we no longer need the try-except block here.\n\n[1] https://github.com/openstack/nova/blob/master/requirements.txt#L55\n[2] https://review.openstack.org/521809\n[3] https://review.openstack.org/522263","commit_id":"6d89ff9321f4e42c78d8e156d4c43268c1ff7688"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"77f9be334f3bd765329ac2f88ed5a51e0c44dd52","unresolved":false,"context_lines":[{"line_number":1417,"context_line":"            LOG.warning(\"During detach_volume, instance disappeared.\","},{"line_number":1418,"context_line":"                        instance\u003dinstance)"},{"line_number":1419,"context_line":"        except exception.DeviceNotFound:"},{"line_number":1420,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":1421,"context_line":"        except libvirt.libvirtError as ex:"},{"line_number":1422,"context_line":"            # NOTE(vish): This is called to cleanup volumes after live"},{"line_number":1423,"context_line":"            #             migration, so we should still disconnect even if"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_58f5b382","side":"PARENT","line":1420,"range":{"start_line":1420,"start_character":12,"end_line":1420,"end_character":59},"updated":"2017-12-13 11:09:29.000000000","message":"This is still used all over the place, so no need to remove","commit_id":"3cec0cb584ac82a5a57400b94d25f8ac73bb0950"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ab4d05d31a408ceed279ffe472522268f0496666","unresolved":false,"context_lines":[{"line_number":1417,"context_line":"            LOG.warning(\"During detach_volume, instance disappeared.\","},{"line_number":1418,"context_line":"                        instance\u003dinstance)"},{"line_number":1419,"context_line":"        except exception.DeviceNotFound:"},{"line_number":1420,"context_line":"            raise exception.DiskNotFound(location\u003ddisk_dev)"},{"line_number":1421,"context_line":"        except libvirt.libvirtError as ex:"},{"line_number":1422,"context_line":"            # NOTE(vish): This is called to cleanup volumes after live"},{"line_number":1423,"context_line":"            #             migration, so we should still disconnect even if"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_ac5fd870","side":"PARENT","line":1420,"range":{"start_line":1420,"start_character":12,"end_line":1420,"end_character":59},"in_reply_to":"df87a7cf_58f5b382","updated":"2017-12-13 16:50:59.000000000","message":"This is the point of the patch, to remove this, because what we\u0027re seeing in a bug downstream is that we\u0027re assuming if the device has already been detached from the guest, it\u0027s also been detached from the compute host. But this isn\u0027t necessarily true because if guest detach succeeds and compute host detach fails on a first attempt, a second attempt will raise DiskNotFound and then the compute manager will terminate the connection with cinder even though the device has *never* been detached from the compute host, which causes problems with failed multipaths.\n\nSo if we detect that the device is already detached from the guest, we should go ahead and try to detach from the compute host (which will also ignore not found).","commit_id":"3cec0cb584ac82a5a57400b94d25f8ac73bb0950"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"77f9be334f3bd765329ac2f88ed5a51e0c44dd52","unresolved":false,"context_lines":[{"line_number":1430,"context_line":""},{"line_number":1431,"context_line":"        if encryption:"},{"line_number":1432,"context_line":"            encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1433,"context_line":"                                                       encryption)"},{"line_number":1434,"context_line":"            encryptor.detach_volume(**encryption)"},{"line_number":1435,"context_line":""},{"line_number":1436,"context_line":"        self._disconnect_volume(connection_info, disk_dev, instance)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df87a7cf_583a9334","line":1433,"range":{"start_line":1433,"start_character":51,"end_line":1433,"end_character":55},"updated":"2017-12-13 11:09:29.000000000","message":"nit: alignment","commit_id":"3bc4f745a15ff803b66cac5ce368e18679e9aa2d"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"88803bb9161aa542b411b3b30a3b9701db59d228","unresolved":false,"context_lines":[{"line_number":1426,"context_line":"            else:"},{"line_number":1427,"context_line":"                raise"},{"line_number":1428,"context_line":""},{"line_number":1429,"context_line":"        if encryption:"},{"line_number":1430,"context_line":"            encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1431,"context_line":"                                                   encryption)"},{"line_number":1432,"context_line":"            encryptor.detach_volume(**encryption)"},{"line_number":1433,"context_line":""},{"line_number":1434,"context_line":"        self._disconnect_volume(connection_info, instance)"},{"line_number":1435,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9f91af0f_80481d5c","line":1432,"range":{"start_line":1429,"start_character":0,"end_line":1432,"end_character":49},"updated":"2018-01-08 15:30:13.000000000","message":"FYI - Ica323b87fa85a454fca9d46ada3677f18fe50022 is looking to refactor the above into a single call. If it lands before this change you should respin to use the new _detach_encryptor method.","commit_id":"2688ef88d0a0f6e734d8e6ca5dcc607a3dc34886"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f02e212e5602aaecb6b15b36837b10843090d19b","unresolved":false,"context_lines":[{"line_number":1511,"context_line":"            else:"},{"line_number":1512,"context_line":"                raise"},{"line_number":1513,"context_line":""},{"line_number":1514,"context_line":"        if encryption:"},{"line_number":1515,"context_line":"            encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":1516,"context_line":"                                                   encryption)"},{"line_number":1517,"context_line":"            encryptor.detach_volume(**encryption)"},{"line_number":1518,"context_line":""},{"line_number":1519,"context_line":"        # NOTE(lyarwood): We can provide None as the request context here as we"},{"line_number":1520,"context_line":"        # already have the encryption metadata dict from the compute layer."}],"source_content_type":"text/x-python","patch_set":9,"id":"5f93b717_d5d0a0cc","line":1517,"range":{"start_line":1514,"start_character":0,"end_line":1517,"end_character":49},"updated":"2018-01-25 14:35:32.000000000","message":"This is no longer a thing. We do it in _disconnect_volume.","commit_id":"fb371414f64214caf6dc434370fbcdd8064ddd2d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e59a5531928f9f5bec1037d94ff091ae3b25d577","unresolved":false,"context_lines":[{"line_number":1227,"context_line":"        vol_driver.connect_volume(connection_info, instance)"},{"line_number":1228,"context_line":"        self._attach_encryptor(context, connection_info, encryption\u003dencryption)"},{"line_number":1229,"context_line":""},{"line_number":1230,"context_line":"    def _disconnect_volume(self, context, connection_info, instance,"},{"line_number":1231,"context_line":"                           encryption\u003dNone):"},{"line_number":1232,"context_line":"        self._detach_encryptor(context, connection_info, encryption\u003dencryption)"},{"line_number":1233,"context_line":"        vol_driver \u003d self._get_volume_driver(connection_info)"},{"line_number":1234,"context_line":"        vol_driver.disconnect_volume(connection_info, instance)"},{"line_number":1235,"context_line":""},{"line_number":1236,"context_line":"    def _extend_volume(self, connection_info, instance):"},{"line_number":1237,"context_line":"        vol_driver \u003d self._get_volume_driver(connection_info)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3fa0c359_8c5441cc","line":1234,"range":{"start_line":1230,"start_character":4,"end_line":1234,"end_character":63},"updated":"2018-02-06 16:40:56.000000000","message":"I assume everything in here is idempotent if it\u0027s already disconnected?","commit_id":"326aa1b3c1b547154e6715a422cfcfb077bf176c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"17b3b6b4a1978359e0ef2764886c379e9a3e3c9b","unresolved":false,"context_lines":[{"line_number":1227,"context_line":"        vol_driver.connect_volume(connection_info, instance)"},{"line_number":1228,"context_line":"        self._attach_encryptor(context, connection_info, encryption\u003dencryption)"},{"line_number":1229,"context_line":""},{"line_number":1230,"context_line":"    def _disconnect_volume(self, context, connection_info, instance,"},{"line_number":1231,"context_line":"                           encryption\u003dNone):"},{"line_number":1232,"context_line":"        self._detach_encryptor(context, connection_info, encryption\u003dencryption)"},{"line_number":1233,"context_line":"        vol_driver \u003d self._get_volume_driver(connection_info)"},{"line_number":1234,"context_line":"        vol_driver.disconnect_volume(connection_info, instance)"},{"line_number":1235,"context_line":""},{"line_number":1236,"context_line":"    def _extend_volume(self, connection_info, instance):"},{"line_number":1237,"context_line":"        vol_driver \u003d self._get_volume_driver(connection_info)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3fa0c359_c87a2e23","line":1234,"range":{"start_line":1230,"start_character":4,"end_line":1234,"end_character":63},"in_reply_to":"3fa0c359_8c5441cc","updated":"2018-02-06 22:19:34.000000000","message":"Yes, it\u0027s been a pattern that \"already detached\" is not treated as an error.","commit_id":"326aa1b3c1b547154e6715a422cfcfb077bf176c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e59a5531928f9f5bec1037d94ff091ae3b25d577","unresolved":false,"context_lines":[{"line_number":1498,"context_line":"            # We should still try to disconnect logical device from"},{"line_number":1499,"context_line":"            # host, an error might have happened during a previous"},{"line_number":1500,"context_line":"            # call."},{"line_number":1501,"context_line":"            LOG.warning(\"Device %s not found in instance.\","},{"line_number":1502,"context_line":"                        disk_dev, instance\u003dinstance)"},{"line_number":1503,"context_line":"        except libvirt.libvirtError as ex:"},{"line_number":1504,"context_line":"            # NOTE(vish): This is called to cleanup volumes after live"}],"source_content_type":"text/x-python","patch_set":10,"id":"3fa0c359_ccc56990","line":1501,"range":{"start_line":1501,"start_character":16,"end_line":1501,"end_character":23},"updated":"2018-02-06 16:40:56.000000000","message":"Warning seems a bit excessive here, is it going to be normal to hit this? If so, it seems like this should be INFO at best.","commit_id":"326aa1b3c1b547154e6715a422cfcfb077bf176c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"17b3b6b4a1978359e0ef2764886c379e9a3e3c9b","unresolved":false,"context_lines":[{"line_number":1498,"context_line":"            # We should still try to disconnect logical device from"},{"line_number":1499,"context_line":"            # host, an error might have happened during a previous"},{"line_number":1500,"context_line":"            # call."},{"line_number":1501,"context_line":"            LOG.warning(\"Device %s not found in instance.\","},{"line_number":1502,"context_line":"                        disk_dev, instance\u003dinstance)"},{"line_number":1503,"context_line":"        except libvirt.libvirtError as ex:"},{"line_number":1504,"context_line":"            # NOTE(vish): This is called to cleanup volumes after live"}],"source_content_type":"text/x-python","patch_set":10,"id":"3fa0c359_089ed655","line":1501,"range":{"start_line":1501,"start_character":16,"end_line":1501,"end_character":23},"in_reply_to":"3fa0c359_ccc56990","updated":"2018-02-06 22:19:34.000000000","message":"I think it\u0027s normal but it shouldn\u0027t be very common.","commit_id":"326aa1b3c1b547154e6715a422cfcfb077bf176c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a4646439b3575e58948dd594659e08c2104e090b","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"        # already have the encryption metadata dict from the compute layer."},{"line_number":1590,"context_line":"        # This avoids the need to add the request context to the signature of"},{"line_number":1591,"context_line":"        # detach_volume requiring changes across all drivers."},{"line_number":1592,"context_line":"        self._disconnect_volume(None, connection_info, instance,"},{"line_number":1593,"context_line":"                                encryption\u003dencryption)"},{"line_number":1594,"context_line":""},{"line_number":1595,"context_line":"    def extend_volume(self, connection_info, instance):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa0c359_c87732d4","line":1592,"updated":"2018-02-21 14:38:23.000000000","message":"Why don\u0027t we do this first, if this is the part that\u0027s likely to fail more often than cleaning up the guest? In other words, if the guest doesn\u0027t think it has a block device disk config, that\u0027s probably less problematic than orphaning the volume on the host, right?","commit_id":"ce531dd1b763704b9043ddde8e80ac99cd193660"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e86aec4aea6866cd191ff90a8b984013b0697c87","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"        # already have the encryption metadata dict from the compute layer."},{"line_number":1590,"context_line":"        # This avoids the need to add the request context to the signature of"},{"line_number":1591,"context_line":"        # detach_volume requiring changes across all drivers."},{"line_number":1592,"context_line":"        self._disconnect_volume(None, connection_info, instance,"},{"line_number":1593,"context_line":"                                encryption\u003dencryption)"},{"line_number":1594,"context_line":""},{"line_number":1595,"context_line":"    def extend_volume(self, connection_info, instance):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa0c359_dbda9a76","line":1592,"in_reply_to":"3fa0c359_c87732d4","updated":"2018-02-21 14:51:02.000000000","message":"We can\u0027t do this first, AFAIK. This is the bit that removes the local side of the volume back to cinder and ultimately allows it to detach right?\n\nIf we did this first it would never complete normally because the device would be in use.","commit_id":"ce531dd1b763704b9043ddde8e80ac99cd193660"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b02f95f0dd1362bd7890a8492604805622334e97","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"        # already have the encryption metadata dict from the compute layer."},{"line_number":1590,"context_line":"        # This avoids the need to add the request context to the signature of"},{"line_number":1591,"context_line":"        # detach_volume requiring changes across all drivers."},{"line_number":1592,"context_line":"        self._disconnect_volume(None, connection_info, instance,"},{"line_number":1593,"context_line":"                                encryption\u003dencryption)"},{"line_number":1594,"context_line":""},{"line_number":1595,"context_line":"    def extend_volume(self, connection_info, instance):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa0c359_bb4b0e29","line":1592,"in_reply_to":"3fa0c359_dbda9a76","updated":"2018-02-21 14:54:08.000000000","message":"Yeah good point.","commit_id":"ce531dd1b763704b9043ddde8e80ac99cd193660"}]}