)]}'
{"cinder/objects/volume.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"62d87f344f8524b540047c3f72b6fffb986f087b","unresolved":false,"context_lines":[{"line_number":463,"context_line":"            setattr(self, key, value)"},{"line_number":464,"context_line":"            setattr(dest_volume, key, value_to_dst)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"        self.save()"},{"line_number":467,"context_line":"        dest_volume.save()"},{"line_number":468,"context_line":"        return dest_volume"},{"line_number":469,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_0e2afa8f","line":466,"updated":"2016-08-18 09:25:09.000000000","message":"-1: This should not be necessary","commit_id":"e5931cc84034fc3625e441fb014f7ec7774259f6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"ebfabac2ff300ba24392245aa906c14eab1cf0f1","unresolved":false,"context_lines":[{"line_number":463,"context_line":"            setattr(self, key, value)"},{"line_number":464,"context_line":"            setattr(dest_volume, key, value_to_dst)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"        self.save()"},{"line_number":467,"context_line":"        dest_volume.save()"},{"line_number":468,"context_line":"        return dest_volume"},{"line_number":469,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_360e198c","line":466,"in_reply_to":"3ac371cc_0e2afa8f","updated":"2016-08-18 13:24:13.000000000","message":"We must call save here. As here attach_status of volume is set as detached, and later call attach_volume will set attach_status as attached in db. But call volume.save after rpcapi.volume_attach will reset attach_status to detached again. This is wrong.","commit_id":"e5931cc84034fc3625e441fb014f7ec7774259f6"}],"cinder/tests/unit/objects/test_volume.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f58d28d8915a90632d2395b4fc86c5735641e524","unresolved":false,"context_lines":[{"line_number":381,"context_line":"        self.assertEqual(src_volume.id, updated_dest_volume._name_id)"},{"line_number":382,"context_line":"        self.assertTrue(volume_update.called)"},{"line_number":383,"context_line":"        volume_update.assert_has_calls(["},{"line_number":384,"context_line":"            mock.call(self.context, src_volume.id, mock.ANY),"},{"line_number":385,"context_line":"            mock.call(self.context, dest_volume.id, mock.ANY)])"},{"line_number":386,"context_line":"        ctxt, vol_id, updates \u003d volume_update.call_args[0]"},{"line_number":387,"context_line":"        self.assertNotIn(\u0027volume_type\u0027, updates)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_d7014a3b","line":384,"updated":"2016-08-19 08:50:12.000000000","message":"nit: Shouldn\u0027t we check the data we are actually updating?","commit_id":"e28c1dcabae2e8241ecbb744560b20514820e4a8"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"ecf2f54008dfb10a5f2fc4c673cd7ddd3f67e590","unresolved":false,"context_lines":[{"line_number":381,"context_line":"        self.assertEqual(src_volume.id, updated_dest_volume._name_id)"},{"line_number":382,"context_line":"        self.assertTrue(volume_update.called)"},{"line_number":383,"context_line":"        volume_update.assert_has_calls(["},{"line_number":384,"context_line":"            mock.call(self.context, src_volume.id, mock.ANY),"},{"line_number":385,"context_line":"            mock.call(self.context, dest_volume.id, mock.ANY)])"},{"line_number":386,"context_line":"        ctxt, vol_id, updates \u003d volume_update.call_args[0]"},{"line_number":387,"context_line":"        self.assertNotIn(\u0027volume_type\u0027, updates)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1ac06dbe_bd2605b8","line":384,"in_reply_to":"3ac371cc_d7014a3b","updated":"2016-08-22 02:43:34.000000000","message":"I think we don\u0027t need to check changed date here as it calls cinder_obj_changes() to get the changed fields which has been tested in other ut. \n\nMeanwhile Line 378 - 381, and Line 387 - line 402 tested data in volume objects. I think that is enough.","commit_id":"e28c1dcabae2e8241ecbb744560b20514820e4a8"}],"cinder/volume/manager.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"62d87f344f8524b540047c3f72b6fffb986f087b","unresolved":false,"context_lines":[{"line_number":1793,"context_line":"        volume.save()"},{"line_number":1794,"context_line":""},{"line_number":1795,"context_line":"        # Detach the source volume (if it fails, don\u0027t fail the migration)"},{"line_number":1796,"context_line":"        # As after detach and refresh, volume_attchments will be None. Here"},{"line_number":1797,"context_line":"        # is to keep volume_attachment for later attach."},{"line_number":1798,"context_line":"        attachments \u003d None"},{"line_number":1799,"context_line":"        try:"},{"line_number":1800,"context_line":"            if orig_volume_status \u003d\u003d \u0027in-use\u0027:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_092f9488","line":1797,"range":{"start_line":1796,"start_character":71,"end_line":1797,"end_character":16},"updated":"2016-08-18 09:25:09.000000000","message":"nit: Replace with \"We\"","commit_id":"e5931cc84034fc3625e441fb014f7ec7774259f6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"ebfabac2ff300ba24392245aa906c14eab1cf0f1","unresolved":false,"context_lines":[{"line_number":1793,"context_line":"        volume.save()"},{"line_number":1794,"context_line":""},{"line_number":1795,"context_line":"        # Detach the source volume (if it fails, don\u0027t fail the migration)"},{"line_number":1796,"context_line":"        # As after detach and refresh, volume_attchments will be None. Here"},{"line_number":1797,"context_line":"        # is to keep volume_attachment for later attach."},{"line_number":1798,"context_line":"        attachments \u003d None"},{"line_number":1799,"context_line":"        try:"},{"line_number":1800,"context_line":"            if orig_volume_status \u003d\u003d \u0027in-use\u0027:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_369bb967","line":1797,"range":{"start_line":1796,"start_character":71,"end_line":1797,"end_character":16},"in_reply_to":"3ac371cc_092f9488","updated":"2016-08-18 13:24:13.000000000","message":"Done","commit_id":"e5931cc84034fc3625e441fb014f7ec7774259f6"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"62d87f344f8524b540047c3f72b6fffb986f087b","unresolved":false,"context_lines":[{"line_number":1795,"context_line":"        # Detach the source volume (if it fails, don\u0027t fail the migration)"},{"line_number":1796,"context_line":"        # As after detach and refresh, volume_attchments will be None. Here"},{"line_number":1797,"context_line":"        # is to keep volume_attachment for later attach."},{"line_number":1798,"context_line":"        attachments \u003d None"},{"line_number":1799,"context_line":"        try:"},{"line_number":1800,"context_line":"            if orig_volume_status \u003d\u003d \u0027in-use\u0027:"},{"line_number":1801,"context_line":"                attachments \u003d volume.volume_attachment"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_e96b18b5","line":1798,"updated":"2016-08-18 09:25:09.000000000","message":"nit: This would probably go on L1804 in an else clause:\n\n else:\n     attachments \u003d None","commit_id":"e5931cc84034fc3625e441fb014f7ec7774259f6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"ebfabac2ff300ba24392245aa906c14eab1cf0f1","unresolved":false,"context_lines":[{"line_number":1795,"context_line":"        # Detach the source volume (if it fails, don\u0027t fail the migration)"},{"line_number":1796,"context_line":"        # As after detach and refresh, volume_attchments will be None. Here"},{"line_number":1797,"context_line":"        # is to keep volume_attachment for later attach."},{"line_number":1798,"context_line":"        attachments \u003d None"},{"line_number":1799,"context_line":"        try:"},{"line_number":1800,"context_line":"            if orig_volume_status \u003d\u003d \u0027in-use\u0027:"},{"line_number":1801,"context_line":"                attachments \u003d volume.volume_attachment"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_96a6edab","line":1798,"in_reply_to":"3ac371cc_e96b18b5","updated":"2016-08-18 13:24:13.000000000","message":"Done","commit_id":"e5931cc84034fc3625e441fb014f7ec7774259f6"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f58d28d8915a90632d2395b4fc86c5735641e524","unresolved":false,"context_lines":[{"line_number":1795,"context_line":"        # Detach the source volume (if it fails, don\u0027t fail the migration)"},{"line_number":1796,"context_line":"        # As after detach and refresh, volume_attchments will be None."},{"line_number":1797,"context_line":"        # We keep volume_attachment for later attach."},{"line_number":1798,"context_line":"        try:"},{"line_number":1799,"context_line":"            if orig_volume_status \u003d\u003d \u0027in-use\u0027:"},{"line_number":1800,"context_line":"                attachments \u003d volume.volume_attachment"},{"line_number":1801,"context_line":"                for attachment in attachments:"},{"line_number":1802,"context_line":"                    self.detach_volume(ctxt, volume.id, attachment[\u0027id\u0027])"},{"line_number":1803,"context_line":"            else:"},{"line_number":1804,"context_line":"                attachments \u003d None"},{"line_number":1805,"context_line":"        except Exception as ex:"},{"line_number":1806,"context_line":"            LOG.error(_LE(\"Detach migration source volume failed:  %(err)s\"),"},{"line_number":1807,"context_line":"                      {\u0027err\u0027: ex}, resource\u003dvolume)"},{"line_number":1808,"context_line":""},{"line_number":1809,"context_line":"        # Give driver (new_volume) a chance to update things as needed"},{"line_number":1810,"context_line":"        # after a successful migration."}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_7787b6d7","line":1807,"range":{"start_line":1798,"start_character":0,"end_line":1807,"end_character":51},"updated":"2016-08-19 08:50:12.000000000","message":"Having another look at this, the whole thing looks wrong to me, this should be:\n\n if orig_volume_status \u003d\u003d \u0027in-use\u0027:\n     attachments \u003d volume.volume_attachment\n else:\n     attachments \u003d None\n\n try:\n     for attachment in attachments:\n         self.detach_volume(ctxt, volume.id, attachment[\u0027id\u0027])\n except Exception as ex:\n     LOG.error(_LE(\"Detach migration source volume failed:  %(err)s\"),\n               {\u0027err\u0027: ex}, resource\u003dvolume)\n\nBecause an error reading from the DB should stop the operation and this will fail on L1815 if the DB hasn\u0027t recovered and on L1826 if it has recovered because `attachments` variable doesn\u0027t exist.\n\nI know with the code you had in patch 4 it wouldn\u0027t fail on L1826, because the variable would exist, but I think it should fail if reading the attachments is not possible.\n\nSorry for driving you crazy with this, but we should leave this as you had it before or change it to something similar to above code.","commit_id":"e28c1dcabae2e8241ecbb744560b20514820e4a8"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"ecf2f54008dfb10a5f2fc4c673cd7ddd3f67e590","unresolved":false,"context_lines":[{"line_number":1795,"context_line":"        # Detach the source volume (if it fails, don\u0027t fail the migration)"},{"line_number":1796,"context_line":"        # As after detach and refresh, volume_attchments will be None."},{"line_number":1797,"context_line":"        # We keep volume_attachment for later attach."},{"line_number":1798,"context_line":"        try:"},{"line_number":1799,"context_line":"            if orig_volume_status \u003d\u003d \u0027in-use\u0027:"},{"line_number":1800,"context_line":"                attachments \u003d volume.volume_attachment"},{"line_number":1801,"context_line":"                for attachment in attachments:"},{"line_number":1802,"context_line":"                    self.detach_volume(ctxt, volume.id, attachment[\u0027id\u0027])"},{"line_number":1803,"context_line":"            else:"},{"line_number":1804,"context_line":"                attachments \u003d None"},{"line_number":1805,"context_line":"        except Exception as ex:"},{"line_number":1806,"context_line":"            LOG.error(_LE(\"Detach migration source volume failed:  %(err)s\"),"},{"line_number":1807,"context_line":"                      {\u0027err\u0027: ex}, resource\u003dvolume)"},{"line_number":1808,"context_line":""},{"line_number":1809,"context_line":"        # Give driver (new_volume) a chance to update things as needed"},{"line_number":1810,"context_line":"        # after a successful migration."}],"source_content_type":"text/x-python","patch_set":5,"id":"1ac06dbe_f98b721c","line":1807,"range":{"start_line":1798,"start_character":0,"end_line":1807,"end_character":51},"in_reply_to":"3ac371cc_7787b6d7","updated":"2016-08-22 02:43:34.000000000","message":"You consider so carefully!\nMoved volume.volume_attachment out of try...except, it stops when reading from db fails. This is reasonable.","commit_id":"e28c1dcabae2e8241ecbb744560b20514820e4a8"}]}