)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":22,"context_line":"destroy_bdm\u003dTrue was provided the BDM would always be destroyed first"},{"line_number":23,"context_line":"before any detach call was made to cinder. This could result in the"},{"line_number":24,"context_line":"destruction of a BDM while the volume remained marked as attached in"},{"line_number":25,"context_line":"Cinder if errors were encountered by the detach call."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"This has now changed so the BDM is only destroyed after a successful"},{"line_number":28,"context_line":"detach call has been made to cinder. In the event of a failure during"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ba2be162_ac7184e2","line":25,"updated":"2017-03-02 12:06:00.000000000","message":"That sounds more correct to me.\n\nWould it be possible to do that as a separate change, before this patch?","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":30,"context_line":"detach the volume via Nova as the BDM is no longer destroyed."},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"TODO"},{"line_number":33,"context_line":"- unittest for new bdm destroy behaviour"},{"line_number":34,"context_line":"- add TODOs to move tests from compute/*.py into virt/test_block_device.py?"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"Change-Id: I7a53e08f3fad6abb27a1d8ad425b4f916341cab3"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ba2be162_2c03d410","line":33,"updated":"2017-03-02 12:06:00.000000000","message":"+1 :)","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"TODO"},{"line_number":33,"context_line":"- unittest for new bdm destroy behaviour"},{"line_number":34,"context_line":"- add TODOs to move tests from compute/*.py into virt/test_block_device.py?"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"Change-Id: I7a53e08f3fad6abb27a1d8ad425b4f916341cab3"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ba2be162_6c1cec77","line":34,"updated":"2017-03-02 12:06:00.000000000","message":"I would do that in a separate patch. Oh right, add TODOs is probably best.","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"}],"nova/compute/manager.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":4841,"context_line":""},{"line_number":4842,"context_line":"        \"\"\""},{"line_number":4843,"context_line":"        bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":4844,"context_line":"                context, volume_id, instance.uuid)"},{"line_number":4845,"context_line":"        driver_bdm \u003d driver_block_device.convert_volume(bdm)"},{"line_number":4846,"context_line":""},{"line_number":4847,"context_line":"        self._notify_volume_usage_detach(context, instance, bdm, volume_id)"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_a7ffdd65","line":4844,"updated":"2017-03-02 12:06:00.000000000","message":"We should do the refactor where we get BDMs from the instance, rather than the reverse. like instance.bdms[volume_id] does the correct lazy load, only if needed. That would stop us having to refetch the bdm everywhere.\n\nI suspect thats already on your TODO list somewhere, just wanted to say that here, to make sure we are agreeing on the general direction here.\n\nRight now we refetch the BDM lots, and I think that fetches a full copy of the instance too, which is clearly wasteful:\nhttps://github.com/openstack/nova/blob/master/nova/objects/block_device.py#L70\n\nWe might want to at least not fetch that by default anymore, or something?","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":4850,"context_line":"                 {\u0027volume_id\u0027: volume_id}, instance\u003dinstance)"},{"line_number":4851,"context_line":""},{"line_number":4852,"context_line":"        driver_bdm.detach(context, instance, self.volume_api, self.driver,"},{"line_number":4853,"context_line":"                          attachment_id\u003dattachment_id, destroy_bdm\u003ddestroy_bdm)"},{"line_number":4854,"context_line":""},{"line_number":4855,"context_line":"        if destroy_bdm:"},{"line_number":4856,"context_line":"            bdm.destroy()"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_47e4613a","line":4853,"range":{"start_line":4853,"start_character":55,"end_line":4853,"end_character":78},"updated":"2017-03-02 12:06:00.000000000","message":"it seems odd passing this in here, then also doing the destroy below.","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":4808,"context_line":"        try:"},{"line_number":4809,"context_line":"            vol_stats \u003d self.driver.block_stats(instance, mp)"},{"line_number":4810,"context_line":"        except NotImplementedError:"},{"line_number":4811,"context_line":"            pass"},{"line_number":4812,"context_line":""},{"line_number":4813,"context_line":"        if vol_stats:"},{"line_number":4814,"context_line":"            LOG.debug(\"Updating volume usage cache with totals\","}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_991c12c3","line":4811,"updated":"2017-03-16 12:12:25.000000000","message":"nit: you could return here and ditch a level of indent below. The previous code couldn\u0027t do that because it wasn\u0027t in a separate function. You would also be able to ditch line 4803.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":4808,"context_line":"        try:"},{"line_number":4809,"context_line":"            vol_stats \u003d self.driver.block_stats(instance, mp)"},{"line_number":4810,"context_line":"        except NotImplementedError:"},{"line_number":4811,"context_line":"            pass"},{"line_number":4812,"context_line":""},{"line_number":4813,"context_line":"        if vol_stats:"},{"line_number":4814,"context_line":"            LOG.debug(\"Updating volume usage cache with totals\","}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_b3456671","line":4811,"in_reply_to":"3a1ff146_991c12c3","updated":"2017-03-16 13:32:40.000000000","message":"Done","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":4842,"context_line":"        \"\"\""},{"line_number":4843,"context_line":"        bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":4844,"context_line":"                context, volume_id, instance.uuid)"},{"line_number":4845,"context_line":"        driver_bdm \u003d driver_block_device.convert_volume(bdm)"},{"line_number":4846,"context_line":""},{"line_number":4847,"context_line":"        self._notify_volume_usage_detach(context, instance, bdm, volume_id)"},{"line_number":4848,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_f4098b2d","line":4845,"updated":"2017-03-16 12:12:25.000000000","message":"nit: this could be created just before its usage below, which would reduce its scope.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":4842,"context_line":"        \"\"\""},{"line_number":4843,"context_line":"        bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":4844,"context_line":"                context, volume_id, instance.uuid)"},{"line_number":4845,"context_line":"        driver_bdm \u003d driver_block_device.convert_volume(bdm)"},{"line_number":4846,"context_line":""},{"line_number":4847,"context_line":"        self._notify_volume_usage_detach(context, instance, bdm, volume_id)"},{"line_number":4848,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_3383369a","line":4845,"in_reply_to":"3a1ff146_f4098b2d","updated":"2017-03-16 13:32:40.000000000","message":"Done","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":4857,"context_line":""},{"line_number":4858,"context_line":"        info \u003d dict(volume_id\u003dvolume_id)"},{"line_number":4859,"context_line":"        self._notify_about_instance_usage("},{"line_number":4860,"context_line":"            context, instance, \"volume.detach\", extra_usage_info\u003dinfo)"},{"line_number":4861,"context_line":""},{"line_number":4862,"context_line":"    @wrap_exception()"},{"line_number":4863,"context_line":"    @wrap_instance_fault"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_d4ec8f2b","line":4860,"updated":"2017-03-16 12:12:25.000000000","message":"As in the previous patch, this should live before the call to detach_volume above.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":4857,"context_line":""},{"line_number":4858,"context_line":"        info \u003d dict(volume_id\u003dvolume_id)"},{"line_number":4859,"context_line":"        self._notify_about_instance_usage("},{"line_number":4860,"context_line":"            context, instance, \"volume.detach\", extra_usage_info\u003dinfo)"},{"line_number":4861,"context_line":""},{"line_number":4862,"context_line":"    @wrap_exception()"},{"line_number":4863,"context_line":"    @wrap_instance_fault"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_d6ebb0ae","line":4860,"in_reply_to":"3a1ff146_d4ec8f2b","updated":"2017-03-16 13:32:40.000000000","message":"Done","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":5028,"context_line":"            old_volume_id, new_volume_id)"},{"line_number":5029,"context_line":""},{"line_number":5030,"context_line":"    @wrap_exception()"},{"line_number":5031,"context_line":"    def remove_volume_connection(self, context, volume_id, instance):"},{"line_number":5032,"context_line":"        \"\"\"Remove a volume connection using the volume api.\"\"\""},{"line_number":5033,"context_line":"        # NOTE(vish): We don\u0027t want to actually mark the volume"},{"line_number":5034,"context_line":"        #             detached, or delete the bdm, just remove the"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_1448e786","line":5031,"updated":"2017-03-16 12:12:25.000000000","message":"Aside: this method shouldn\u0027t exist. It should be rolled into rollback_live_migration_at_destination.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":5028,"context_line":"            old_volume_id, new_volume_id)"},{"line_number":5029,"context_line":""},{"line_number":5030,"context_line":"    @wrap_exception()"},{"line_number":5031,"context_line":"    def remove_volume_connection(self, context, volume_id, instance):"},{"line_number":5032,"context_line":"        \"\"\"Remove a volume connection using the volume api.\"\"\""},{"line_number":5033,"context_line":"        # NOTE(vish): We don\u0027t want to actually mark the volume"},{"line_number":5034,"context_line":"        #             detached, or delete the bdm, just remove the"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_16f54882","line":5031,"in_reply_to":"3a1ff146_1448e786","updated":"2017-03-16 13:32:40.000000000","message":"Agreed","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3cccafa25375de06947c3dfc8c192936722a8e46","unresolved":false,"context_lines":[{"line_number":4797,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4798,"context_line":""},{"line_number":4799,"context_line":"    def _notify_volume_usage_detach(self, context, instance, bdm, volume_id):"},{"line_number":4800,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4801,"context_line":"            return"},{"line_number":4802,"context_line":""},{"line_number":4803,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_f202d66e","line":4800,"updated":"2017-03-16 18:25:57.000000000","message":"\u003c\u003d 0 maybe?\n\nhttps://github.com/openstack/nova/blob/b8f18c7460c66c80afba14975e29541c5f0e90d7/nova/conf/compute.py#L724","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3cccafa25375de06947c3dfc8c192936722a8e46","unresolved":false,"context_lines":[{"line_number":4847,"context_line":"                 {\u0027volume_id\u0027: volume_id}, instance\u003dinstance)"},{"line_number":4848,"context_line":""},{"line_number":4849,"context_line":"        driver_bdm \u003d driver_block_device.convert_volume(bdm)"},{"line_number":4850,"context_line":"        driver_bdm.detach(context, instance, self.volume_api, self.driver,"},{"line_number":4851,"context_line":"                          attachment_id\u003dattachment_id, destroy_bdm\u003ddestroy_bdm)"},{"line_number":4852,"context_line":"        info \u003d dict(volume_id\u003dvolume_id)"},{"line_number":4853,"context_line":"        self._notify_about_instance_usage("}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_b21cde78","line":4850,"updated":"2017-03-16 18:25:57.000000000","message":"a bit of me wants to check for NotImplementedError. But thats really stupid really.","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3cccafa25375de06947c3dfc8c192936722a8e46","unresolved":false,"context_lines":[{"line_number":4860,"context_line":"    @wrap_instance_fault"},{"line_number":4861,"context_line":"    def detach_volume(self, context, volume_id, instance, attachment_id\u003dNone):"},{"line_number":4862,"context_line":"        \"\"\"Detach a volume from an instance.\"\"\""},{"line_number":4863,"context_line":"        @utils.synchronized(instance.uuid)"},{"line_number":4864,"context_line":"        def do_detach_volume(context, volume_id, instance, attachment_id):"},{"line_number":4865,"context_line":"            try:"},{"line_number":4866,"context_line":"                self._detach_volume(context, volume_id, instance,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_12dbf2ee","line":4863,"updated":"2017-03-16 18:25:57.000000000","message":"While correct, this seems new and not mentioned in the git commit message. Maybe do this as a follow on patch, just to be clean?","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f8acaff19922a1631381bf0bec29aefc35f5365c","unresolved":false,"context_lines":[{"line_number":4860,"context_line":"    @wrap_instance_fault"},{"line_number":4861,"context_line":"    def detach_volume(self, context, volume_id, instance, attachment_id\u003dNone):"},{"line_number":4862,"context_line":"        \"\"\"Detach a volume from an instance.\"\"\""},{"line_number":4863,"context_line":"        @utils.synchronized(instance.uuid)"},{"line_number":4864,"context_line":"        def do_detach_volume(context, volume_id, instance, attachment_id):"},{"line_number":4865,"context_line":"            try:"},{"line_number":4866,"context_line":"                self._detach_volume(context, volume_id, instance,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_23832bce","line":4863,"in_reply_to":"1a1ced50_12dbf2ee","updated":"2017-03-17 11:07:57.000000000","message":"I\u0027m ambivalent, tbh. We\u0027re following the pattern of attach, and attach does this.","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3cccafa25375de06947c3dfc8c192936722a8e46","unresolved":false,"context_lines":[{"line_number":5029,"context_line":"        \"\"\"Remove a volume connection using the volume api.\"\"\""},{"line_number":5030,"context_line":"        # NOTE(vish): We don\u0027t want to actually mark the volume"},{"line_number":5031,"context_line":"        #             detached, or delete the bdm, just remove the"},{"line_number":5032,"context_line":"        #             connection from this host."},{"line_number":5033,"context_line":""},{"line_number":5034,"context_line":"        try:"},{"line_number":5035,"context_line":"            bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_fd5b872c","line":5032,"updated":"2017-03-16 18:25:57.000000000","message":"Its tempting to mention this is used when rolling back a live-migration, it disconnects from the destination host as that connection is no longer required.\n\nWe really need to specify the attachment_id here ASAP :( But thats a different patch.","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ea8af3a53afce263e1e3ad8510b3a7d18a94aa02","unresolved":false,"context_lines":[{"line_number":4825,"context_line":"        self._notify_about_instance_usage("},{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, volume_id, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_6ca00b43","line":4828,"range":{"start_line":4828,"start_character":51,"end_line":4828,"end_character":60},"updated":"2017-03-29 15:59:46.000000000","message":"This is not used.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"54c8861cbf0d8d05d8a3f2875b854972cbe07677","unresolved":false,"context_lines":[{"line_number":4825,"context_line":"        self._notify_about_instance_usage("},{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, volume_id, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_cc339713","line":4828,"range":{"start_line":4828,"start_character":51,"end_line":4828,"end_character":60},"in_reply_to":"ffe62b97_6ca00b43","updated":"2017-03-29 16:01:33.000000000","message":"hmm, I guess the bdm is always enough info here, even with multi-attach.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fcc117714b68697804576f1861cb3daa6bce3fd5","unresolved":false,"context_lines":[{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, volume_id, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""},{"line_number":4832,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_ec489bb3","line":4829,"range":{"start_line":4829,"start_character":8,"end_line":4829,"end_character":48},"updated":"2017-03-29 15:57:42.000000000","message":"This is a change in behavior, it used to be:\n\nif CONF.volume_usage_poll_interval \u003e 0:\n\nSo shouldn\u0027t we check \u003c\u003d 0 to return? Especially since we don\u0027t set a min value on the option:\n\nhttp://git.openstack.org/cgit/openstack/nova/tree/nova/conf/compute.py#n712","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"60ef0aa96a09a27cd968f27c2c9ce88b6c4df280","unresolved":false,"context_lines":[{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, volume_id, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""},{"line_number":4832,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_80a66e0e","line":4829,"range":{"start_line":4829,"start_character":8,"end_line":4829,"end_character":48},"in_reply_to":"ffe62b97_4c62e70b","updated":"2017-03-29 17:05:43.000000000","message":"Yeah, we should fix this.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c27fc4d2d14030c7096b528140aaecbf4b4cd504","unresolved":false,"context_lines":[{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, volume_id, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""},{"line_number":4832,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_e07bfaf1","line":4829,"range":{"start_line":4829,"start_character":8,"end_line":4829,"end_character":48},"in_reply_to":"ffe62b97_80a66e0e","updated":"2017-03-29 17:15:41.000000000","message":"I take that back:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6278-L6280\n\nThis usage is correct, as that\u0027s the same test as used by the job. We should update both things, or the docs.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"54c8861cbf0d8d05d8a3f2875b854972cbe07677","unresolved":false,"context_lines":[{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, volume_id, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""},{"line_number":4832,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_4c62e70b","line":4829,"range":{"start_line":4829,"start_character":8,"end_line":4829,"end_character":48},"in_reply_to":"ffe62b97_ec489bb3","updated":"2017-03-29 16:01:33.000000000","message":"dam it, I managed to spot that once, but not the second time:\nhttps://review.openstack.org/#/c/439520/10/nova/compute/manager.py@4800","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":4890,"context_line":"    @wrap_instance_fault"},{"line_number":4891,"context_line":"    def detach_volume(self, context, volume_id, instance, attachment_id\u003dNone):"},{"line_number":4892,"context_line":"        \"\"\"Detach a volume from an instance.\"\"\""},{"line_number":4893,"context_line":"        @utils.synchronized(instance.uuid)"},{"line_number":4894,"context_line":"        def do_detach_volume(context, volume_id, instance, attachment_id):"},{"line_number":4895,"context_line":"            try:"},{"line_number":4896,"context_line":"                self._detach_volume(context, volume_id, instance,"},{"line_number":4897,"context_line":"                                    attachment_id\u003dattachment_id)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_5aa5f9a0","line":4894,"range":{"start_line":4893,"start_character":8,"end_line":4894,"end_character":74},"updated":"2017-03-29 16:41:03.000000000","message":"Isn\u0027t this locking all new so not really a refactor? I\u0027m assuming this is a new thing mdbooth was pushing for in this series? IMO it\u0027s not appropriate to hook this in here though since it\u0027s new. Do it separately.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c97ece182d8d862c151438426ab3e2cf15e9386b","unresolved":false,"context_lines":[{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""},{"line_number":4832,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":14,"id":"ffe62b97_5fe8247b","line":4829,"updated":"2017-03-30 12:33:25.000000000","message":"With sincere apologies to you and mriedem:\n\nI was just writing up a bug for this to remind us to update the docs when I noticed the behaviour of periodic_task.periodic_task is:\n\n* Disable any job with spacing\u003c0\n* Set job with spacing\u003d\u003d0 to default interval\n\n_poll_volume_usage explicitly aborts the job if volume_usage_poll_interval is zero, so:\n\n* zero, we run it every 60 seconds and do nothing\n* less than zero, we don\u0027t run it at all\n\nSo... mriedem was right first time: this should be \u003c\u003d 0. Sorry!\n\nAside: for the sake of the planet, we should probably default this to -1 :)","commit_id":"5af7fb05ce16b40ccc42025b73c3160d4f5ace41"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"b56b5d7504ae1c5b43f577a5484e7eb144fa123c","unresolved":false,"context_lines":[{"line_number":4826,"context_line":"            context, instance, \"volume.attach\", extra_usage_info\u003dinfo)"},{"line_number":4827,"context_line":""},{"line_number":4828,"context_line":"    def _notify_volume_usage_detach(self, context, instance, bdm):"},{"line_number":4829,"context_line":"        if CONF.volume_usage_poll_interval \u003d\u003d 0:"},{"line_number":4830,"context_line":"            return"},{"line_number":4831,"context_line":""},{"line_number":4832,"context_line":"        vol_stats \u003d []"}],"source_content_type":"text/x-python","patch_set":14,"id":"dfeb2761_812d3a48","line":4829,"in_reply_to":"ffe62b97_5fe8247b","updated":"2017-03-30 17:10:34.000000000","message":"We should probably fix that up at some point like we did for the others.\n\n0 generally does mean default these days.","commit_id":"5af7fb05ce16b40ccc42025b73c3160d4f5ace41"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"56cce8371520d861500e685b884916d56b77795e","unresolved":false,"context_lines":[{"line_number":4891,"context_line":"    def detach_volume(self, context, volume_id, instance, attachment_id\u003dNone):"},{"line_number":4892,"context_line":"        \"\"\"Detach a volume from an instance.\"\"\""},{"line_number":4893,"context_line":""},{"line_number":4894,"context_line":"        self._detach_volume(context, volume_id, instance,"},{"line_number":4895,"context_line":"                            attachment_id\u003dattachment_id)"},{"line_number":4896,"context_line":""},{"line_number":4897,"context_line":"    def _init_volume_connection(self, context, new_volume_id,"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_fbcb8f6f","line":4894,"range":{"start_line":4894,"start_character":13,"end_line":4894,"end_character":27},"updated":"2017-03-30 20:24:52.000000000","message":"Since this is a complete pass through now, it seems we could have just avoided _detach_volume and had everything that needs to call it just call self.detach_volume since it\u0027s the same thing. That could be touched up later.\n\nActually nothing else even calls _detach_volume in here, so it\u0027s pointless to have the separate method, unless you plan on adding that lock back in later.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"a584b6a6624477a17cee1dd271f68ea0e2cb9e20","unresolved":false,"context_lines":[{"line_number":4891,"context_line":"    def detach_volume(self, context, volume_id, instance, attachment_id\u003dNone):"},{"line_number":4892,"context_line":"        \"\"\"Detach a volume from an instance.\"\"\""},{"line_number":4893,"context_line":""},{"line_number":4894,"context_line":"        self._detach_volume(context, volume_id, instance,"},{"line_number":4895,"context_line":"                            attachment_id\u003dattachment_id)"},{"line_number":4896,"context_line":""},{"line_number":4897,"context_line":"    def _init_volume_connection(self, context, new_volume_id,"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_fb8edc25","line":4894,"range":{"start_line":4894,"start_character":13,"end_line":4894,"end_character":27},"in_reply_to":"dfeb2761_fbcb8f6f","updated":"2017-03-31 09:20:19.000000000","message":"Oh, I would like to see that lock added in a later patch, this makes that easier I guess.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1d5fd7aacd7229eff9f3af43727cdf7431eb23d5","unresolved":false,"context_lines":[{"line_number":5058,"context_line":"                    context, volume_id, instance.uuid)"},{"line_number":5059,"context_line":"            connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":5060,"context_line":"            driver_bdm \u003d driver_block_device.convert_volume(bdm)"},{"line_number":5061,"context_line":"            driver_bdm.driver_detach(context, instance, connector,"},{"line_number":5062,"context_line":"                                     self.volume_api, self.driver)"},{"line_number":5063,"context_line":"            self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":5064,"context_line":"        except exception.NotFound:"}],"source_content_type":"text/x-python","patch_set":16,"id":"df140735_e6d4abbf","line":5061,"range":{"start_line":5061,"start_character":56,"end_line":5061,"end_character":65},"updated":"2017-06-01 03:03:20.000000000","message":"This isn\u0027t in the driver_detach method signature:\n\nhttps://review.openstack.org/#/c/439520/16/nova/virt/block_device.py@263","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"}],"nova/tests/unit/compute/test_compute_mgr.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"56cce8371520d861500e685b884916d56b77795e","unresolved":false,"context_lines":[{"line_number":2442,"context_line":"                    self.context, uuids.volume_id, connector)"},{"line_number":2443,"context_line":""},{"line_number":2444,"context_line":"    def test_detach_volume(self):"},{"line_number":2445,"context_line":"        # TODO(lyarwood): Move into ../virt/test_block_device.py"},{"line_number":2446,"context_line":"        self._test_detach_volume()"},{"line_number":2447,"context_line":""},{"line_number":2448,"context_line":"    def test_detach_volume_not_destroy_bdm(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_db4e8bc0","line":2445,"updated":"2017-03-30 20:24:52.000000000","message":"I don\u0027t really agree with moving these, at least all of them, since these tests are still hitting the compute manager code, which we still want coverage on.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"}],"nova/virt/block_device.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":260,"context_line":"        virt_driver.detach_volume(connection_info, instance,"},{"line_number":261,"context_line":"                self[\u0027mount_device\u0027], encryption\u003dencryption)"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    def detach(self, context, instance, volume_api, virt_driver,"},{"line_number":264,"context_line":"               attachment_id\u003dNone, do_driver_detach\u003dTrue,"},{"line_number":265,"context_line":"               do_volume_detach\u003dTrue, destroy_bdm\u003dFalse):"},{"line_number":266,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_2c4bd4b1","line":263,"updated":"2017-03-02 12:06:00.000000000","message":"+1 moving the logic into here.","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"786e29043730a16df61885a527b2896626dbd1ed","unresolved":false,"context_lines":[{"line_number":291,"context_line":"                                      {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},"},{"line_number":292,"context_line":"                                      instance\u003dinstance)"},{"line_number":293,"context_line":"                        volume_api.roll_detaching(context, volume_id)"},{"line_number":294,"context_line":"            elif not destroy_bdm:"},{"line_number":295,"context_line":"                LOG.debug(\"Skipping _driver_detach_volume during remote \""},{"line_number":296,"context_line":"                          \"rebuild.\", instance\u003dinstance)"},{"line_number":297,"context_line":"            elif destroy_bdm:"},{"line_number":298,"context_line":"                LOG.error(_LE(\"Unable to call for a driver detach of volume \""},{"line_number":299,"context_line":"                              \"%(vol_id)s due to the instance being \""},{"line_number":300,"context_line":"                              \"registered to the remote host %(inst_host)s.\"),"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_dd3bc0cf","line":297,"range":{"start_line":294,"start_character":12,"end_line":297,"end_character":29},"updated":"2017-03-02 12:06:00.000000000","message":"I wonder if what we should have here is:\n\n  if is_evacuate:\n      ... # skip detach...\n  else:\n      ... # oh my we are all messed up here, put instance into ERROR state","commit_id":"ffc8b99823daecc89ee1be5039f5c5e382d965fd"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":266,"context_line":"                self[\u0027mount_device\u0027], encryption\u003dencryption)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def detach(self, context, instance, volume_api, virt_driver,"},{"line_number":269,"context_line":"               attachment_id\u003dNone, do_driver_detach\u003dTrue,"},{"line_number":270,"context_line":"               do_volume_detach\u003dTrue, destroy_bdm\u003dFalse):"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_4fc64c4a","line":269,"range":{"start_line":269,"start_character":35,"end_line":269,"end_character":51},"updated":"2017-03-16 12:12:25.000000000","message":"Why do we need this?","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":266,"context_line":"                self[\u0027mount_device\u0027], encryption\u003dencryption)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def detach(self, context, instance, volume_api, virt_driver,"},{"line_number":269,"context_line":"               attachment_id\u003dNone, do_driver_detach\u003dTrue,"},{"line_number":270,"context_line":"               do_volume_detach\u003dTrue, destroy_bdm\u003dFalse):"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_5346f2bf","line":269,"range":{"start_line":269,"start_character":35,"end_line":269,"end_character":51},"in_reply_to":"3a1ff146_4fc64c4a","updated":"2017-03-16 13:32:40.000000000","message":"We don\u0027t for this change, I think I had the evacuate flow in mind where we don\u0027t want to even try a driver detach on the remote. However it\u0027s out of scope for this refactor and I have just amended it into the commit by mistake, apologies.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":277,"context_line":"        # Only attempt to detach and disconnect from the volume if the instance"},{"line_number":278,"context_line":"        # is currently associated with the local compute host."},{"line_number":279,"context_line":"        if do_driver_detach:"},{"line_number":280,"context_line":"            if CONF.host \u003d\u003d instance.host:"},{"line_number":281,"context_line":"                # Only attempt to detach and disconnect from the volume if the"},{"line_number":282,"context_line":"                # instance is currently associated with the local compute host."},{"line_number":283,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_af18a085","line":280,"updated":"2017-03-16 12:12:25.000000000","message":"This is an error in the remove_volume_connection() flow when it\u0027s called during live migration rollback. In this case instance.host will be the *source*, not this host, but we\u0027re still attached locally and we still need to bring down the local connection.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":277,"context_line":"        # Only attempt to detach and disconnect from the volume if the instance"},{"line_number":278,"context_line":"        # is currently associated with the local compute host."},{"line_number":279,"context_line":"        if do_driver_detach:"},{"line_number":280,"context_line":"            if CONF.host \u003d\u003d instance.host:"},{"line_number":281,"context_line":"                # Only attempt to detach and disconnect from the volume if the"},{"line_number":282,"context_line":"                # instance is currently associated with the local compute host."},{"line_number":283,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_960938e0","line":280,"in_reply_to":"3a1ff146_af18a085","updated":"2017-03-16 13:32:40.000000000","message":"That\u0027s an existing wrinkle with the current code that appears to be hidden for Libvirt by the destroy() we call for in rollback_live_migration_at_destination that nukes the domain.\n\nI guess we should always attempt _driver_detach and just handle InstanceNotFound or something like it right? IMHO that\u0027s outside of this refactor but I can add a TODO if you want.\n\nAlso, regarding instance.host is that only updated at the end of a successful migration?","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"27c5d33135bfdafd76e5d14967602e0de7a050ba","unresolved":false,"context_lines":[{"line_number":307,"context_line":"                           \u0027inst_host\u0027: instance.host}, instance\u003dinstance)"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        if connection_info and not destroy_bdm and ("},{"line_number":310,"context_line":"           connector.get(\u0027host\u0027) !\u003d instance.host):"},{"line_number":311,"context_line":"            # If the volume is attached to another host (evacuate) then"},{"line_number":312,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":313,"context_line":"            # was stored in connection_info instead (if we have one, and it"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_ca078a7f","line":310,"updated":"2017-03-16 12:12:25.000000000","message":"This is a change in behaviour for the remove_volume_connection() flow, and I suspect it\u0027s wrong.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d1ff2f51a73512dd9c60055abc180c5a29b18117","unresolved":false,"context_lines":[{"line_number":307,"context_line":"                           \u0027inst_host\u0027: instance.host}, instance\u003dinstance)"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        if connection_info and not destroy_bdm and ("},{"line_number":310,"context_line":"           connector.get(\u0027host\u0027) !\u003d instance.host):"},{"line_number":311,"context_line":"            # If the volume is attached to another host (evacuate) then"},{"line_number":312,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":313,"context_line":"            # was stored in connection_info instead (if we have one, and it"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a1ff146_b69ab461","line":310,"in_reply_to":"3a1ff146_ca078a7f","updated":"2017-03-16 13:32:40.000000000","message":"While it is a change in behaviour to look for the stashed connector during a LM rollback this will still provide the same connector to terminate_connection in that flow.","commit_id":"00a88841262875bcb7585aa755190c789194b2c6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f8acaff19922a1631381bf0bec29aefc35f5365c","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"        # Only attempt to detach and disconnect from the volume if the instance"},{"line_number":277,"context_line":"        # is currently associated with the local compute host."},{"line_number":278,"context_line":"        if CONF.host \u003d\u003d instance.host:"},{"line_number":279,"context_line":"            # Only attempt to detach and disconnect from the volume if the"},{"line_number":280,"context_line":"            # instance is currently associated with the local compute host."},{"line_number":281,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_e31113b1","line":278,"updated":"2017-03-17 11:07:57.000000000","message":"This is still going to be a change of behaviour when called on the destination during rollback live migration. Previously we\u0027d have called _driver_detach(), but with this change we won\u0027t.\n\nIf we call it again subsequently during instance cleanup on the destination, I\u0027d like to see that addressed directly by this patch. For example, are we sure all drivers will work this way? Is remove_volume_connection() already redundant? Do we have test coverage which proves connections are still cleaned up even if remove_volume_connection() no longer works?\n\nI\u0027m confident that this patch breaks remove_volume_connection(). That may be ok, but I\u0027d like to see it addressed directly by either fixing or removing it.","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3cccafa25375de06947c3dfc8c192936722a8e46","unresolved":false,"context_lines":[{"line_number":309,"context_line":"            # If the volume is attached to another host (evacuate) then"},{"line_number":310,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":311,"context_line":"            # was stored in connection_info instead (if we have one, and it"},{"line_number":312,"context_line":"            # is for the expected host)."},{"line_number":313,"context_line":"            stashed_connector \u003d connection_info.get(\u0027connector\u0027)"},{"line_number":314,"context_line":"            if not stashed_connector:"},{"line_number":315,"context_line":"                # Volume was attached before we began stashing connectors"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a1ced50_3d6fdf93","line":312,"updated":"2017-03-16 18:25:57.000000000","message":"This is probably also for that live-migrate rollback case I guess?","commit_id":"d61e6e5db6b87efabb47656acab68834b6336056"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"bdf0fffc877eebf1e40fd380b1f6e27b7d7fe107","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        \"\"\""},{"line_number":154,"context_line":"        raise NotImplementedError()"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"    def detach(self, **kwargs):"},{"line_number":157,"context_line":"        \"\"\"Detach the device from an instance."},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"        To be overridden in subclasses with the detaching logic for"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a1ced50_81d344d4","line":156,"range":{"start_line":156,"start_character":8,"end_line":156,"end_character":14},"updated":"2017-03-22 18:26:58.000000000","message":"We need another for driver_detach I think.","commit_id":"d0e5a303f158f594b54572f168dbfa0188651868"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        \"\"\""},{"line_number":154,"context_line":"        raise NotImplementedError()"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"    def detach(self, **kwargs):"},{"line_number":157,"context_line":"        \"\"\"Detach the device from an instance and detach in Cinder."},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"        Note: driver_detach is called as part of this method."}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_9a6ee1c6","line":156,"range":{"start_line":156,"start_character":23,"end_line":156,"end_character":29},"updated":"2017-03-29 16:41:03.000000000","message":"Why don\u0027t we call out the actual arguments? I\u0027m not a fan of just kwargs here because then we can\u0027t document them. For example, destroy_bdm is tightly coupled to the operation in the compute manager like rebuild.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"60ef0aa96a09a27cd968f27c2c9ce88b6c4df280","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        \"\"\""},{"line_number":154,"context_line":"        raise NotImplementedError()"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"    def detach(self, **kwargs):"},{"line_number":157,"context_line":"        \"\"\"Detach the device from an instance and detach in Cinder."},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"        Note: driver_detach is called as part of this method."}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_d5d43a4a","line":156,"range":{"start_line":156,"start_character":23,"end_line":156,"end_character":29},"in_reply_to":"ffe62b97_9a6ee1c6","updated":"2017-03-29 17:05:43.000000000","message":"+1, but... it\u0027s consistent with attach(). Fix it all, or be consistent. For now I personally prefer consistency. We could follow this one up.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"39e2366c6a7469ddefd236806b7bdd04ecf3e28e","unresolved":false,"context_lines":[{"line_number":260,"context_line":"                LOG.info(_LI(\u0027preserve multipath_id %s\u0027),"},{"line_number":261,"context_line":"                         connection_info[\u0027data\u0027][\u0027multipath_id\u0027])"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    def driver_detach(self, context, instance, connector, volume_api,"},{"line_number":264,"context_line":"                      virt_driver):"},{"line_number":265,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":266,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_f0d27734","line":263,"updated":"2017-03-24 10:41:10.000000000","message":"This could do with a better name, but ideally it can go away entirely along the lines of your original patch when we sort out remove_volume_connection.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":263,"context_line":"    def driver_detach(self, context, instance, connector, volume_api,"},{"line_number":264,"context_line":"                      virt_driver):"},{"line_number":265,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":266,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"},{"line_number":267,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_fa796dc4","line":266,"range":{"start_line":266,"start_character":13,"end_line":266,"end_character":33},"updated":"2017-03-29 16:41:03.000000000","message":"Are we sure this is going to be set? For certain types of BDMs like swap and ephemeral I don\u0027t see this transformed. I\u0027m assuming we don\u0027t do a good job of testing attach/detach for those types of devices either.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"249b5287eb9932d95233d4db669c11eaabd1772a","unresolved":false,"context_lines":[{"line_number":263,"context_line":"    def driver_detach(self, context, instance, connector, volume_api,"},{"line_number":264,"context_line":"                      virt_driver):"},{"line_number":265,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":266,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"},{"line_number":267,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_4022a6ba","line":266,"range":{"start_line":266,"start_character":13,"end_line":266,"end_character":33},"in_reply_to":"ffe62b97_40a06690","updated":"2017-03-29 17:21:47.000000000","message":"Now that I think about it, it\u0027s simpler than that: this is DriverVolumeBlockDevice.driver_detach(), and DriverVolumeBlockDevice explicitly defines mount_device as a legacy field. This is always good, no additional context required.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"cdeaf2be695b370573d16bc45e7e8d53c77efe03","unresolved":false,"context_lines":[{"line_number":263,"context_line":"    def driver_detach(self, context, instance, connector, volume_api,"},{"line_number":264,"context_line":"                      virt_driver):"},{"line_number":265,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":266,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"},{"line_number":267,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_40a06690","line":266,"range":{"start_line":266,"start_character":13,"end_line":266,"end_character":33},"in_reply_to":"ffe62b97_b5c1b656","updated":"2017-03-29 17:10:03.000000000","message":"What do you mean they can\u0027t be attached? I guess you mean via a post-server create volume attach operation, so they can only be attached during server create. In that case then yeah I agree.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"35b38196650a3998bc20f992661a4b4c96c2c222","unresolved":false,"context_lines":[{"line_number":263,"context_line":"    def driver_detach(self, context, instance, connector, volume_api,"},{"line_number":264,"context_line":"                      virt_driver):"},{"line_number":265,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":266,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"},{"line_number":267,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_75b7aef6","line":266,"range":{"start_line":266,"start_character":13,"end_line":266,"end_character":33},"in_reply_to":"ffe62b97_fa796dc4","updated":"2017-03-29 16:51:58.000000000","message":"Nevermind, I guess it will always be set as long as the transformation goes through the DriverVolumeBlockDevice which is the parent class for the swap and ephemeral types:\n\nhttps://review.openstack.org/#/c/439520/13/nova/virt/block_device.py@248\n\nThe special sauce is in the variable overrides in the subclasses.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"60ef0aa96a09a27cd968f27c2c9ce88b6c4df280","unresolved":false,"context_lines":[{"line_number":263,"context_line":"    def driver_detach(self, context, instance, connector, volume_api,"},{"line_number":264,"context_line":"                      virt_driver):"},{"line_number":265,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":266,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"},{"line_number":267,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_b5c1b656","line":266,"range":{"start_line":266,"start_character":13,"end_line":266,"end_character":33},"in_reply_to":"ffe62b97_fa796dc4","updated":"2017-03-29 17:05:43.000000000","message":"They can\u0027t be attached/detached. mount_device will always be set for any subclass of DriverVolumeBlockDevice, so I think we\u0027re good here.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":267,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"},{"line_number":270,"context_line":"                  \u0027 mountpoint %(mp)s\u0027), {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},"},{"line_number":271,"context_line":"                  instance\u003dinstance)"},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"        try:"},{"line_number":274,"context_line":"            if not virt_driver.instance_exists(instance):"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_5a2a99da","line":271,"range":{"start_line":270,"start_character":18,"end_line":271,"end_character":36},"updated":"2017-03-29 16:41:03.000000000","message":"nit: the alignment here is messed up.","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":273,"context_line":"        try:"},{"line_number":274,"context_line":"            if not virt_driver.instance_exists(instance):"},{"line_number":275,"context_line":"                LOG.warning(_LW(\u0027Detaching volume from unknown instance\u0027),"},{"line_number":276,"context_line":"                             instance\u003dinstance)"},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"            encryption \u003d encryptors.get_encryption_metadata(context,"},{"line_number":279,"context_line":"                    volume_api, volume_id, connection_info)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_9a8141c6","line":276,"updated":"2017-03-29 16:41:03.000000000","message":"alignment","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":275,"context_line":"                LOG.warning(_LW(\u0027Detaching volume from unknown instance\u0027),"},{"line_number":276,"context_line":"                             instance\u003dinstance)"},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"            encryption \u003d encryptors.get_encryption_metadata(context,"},{"line_number":279,"context_line":"                    volume_api, volume_id, connection_info)"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"            virt_driver.detach_volume(connection_info, instance, mp,"}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_fa6f2d25","line":278,"range":{"start_line":278,"start_character":60,"end_line":278,"end_character":67},"updated":"2017-03-29 16:41:03.000000000","message":"Can we drop this to the next line too please?","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":298,"context_line":"               attachment_id\u003dNone, destroy_bdm\u003dFalse):"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        connector \u003d virt_driver.get_volume_connector(instance)"},{"line_number":301,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":302,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        if connection_info and not destroy_bdm and ("}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_ba89a575","line":301,"range":{"start_line":301,"start_character":8,"end_line":301,"end_character":49},"updated":"2017-03-29 16:41:03.000000000","message":"Why don\u0027t we have to deserialize this? In the old code we did:\n\nconnection_info \u003d jsonutils.loads(bdm.connection_info)","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"60ef0aa96a09a27cd968f27c2c9ce88b6c4df280","unresolved":false,"context_lines":[{"line_number":298,"context_line":"               attachment_id\u003dNone, destroy_bdm\u003dFalse):"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        connector \u003d virt_driver.get_volume_connector(instance)"},{"line_number":301,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":302,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        if connection_info and not destroy_bdm and ("}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_55970a49","line":301,"range":{"start_line":301,"start_character":8,"end_line":301,"end_character":49},"in_reply_to":"ffe62b97_ba89a575","updated":"2017-03-29 17:05:43.000000000","message":"It was already deserialized in DriverVolumeBlockDevice._transform()","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"35b38196650a3998bc20f992661a4b4c96c2c222","unresolved":false,"context_lines":[{"line_number":298,"context_line":"               attachment_id\u003dNone, destroy_bdm\u003dFalse):"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        connector \u003d virt_driver.get_volume_connector(instance)"},{"line_number":301,"context_line":"        connection_info \u003d self[\u0027connection_info\u0027]"},{"line_number":302,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        if connection_info and not destroy_bdm and ("}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_d5079ac9","line":301,"range":{"start_line":301,"start_character":8,"end_line":301,"end_character":49},"in_reply_to":"ffe62b97_ba89a575","updated":"2017-03-29 16:51:58.000000000","message":"Nevermind I see we do it during the transformation:\n\nhttps://review.openstack.org/#/c/439520/13/nova/virt/block_device.py@250","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":338,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":339,"context_line":"                connector \u003d stashed_connector"},{"line_number":340,"context_line":""},{"line_number":341,"context_line":"        # Only attempt to detach and disconnect from the volume if the instance"},{"line_number":342,"context_line":"        # is currently associated with the local compute host."},{"line_number":343,"context_line":"        if CONF.host \u003d\u003d instance.host:"},{"line_number":344,"context_line":"            # Only attempt to detach and disconnect from the volume if the"},{"line_number":345,"context_line":"            # instance is currently associated with the local compute host."}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_1a6c119a","line":342,"range":{"start_line":341,"start_character":8,"end_line":342,"end_character":62},"updated":"2017-03-29 16:41:03.000000000","message":"This is already commented in the if block below, as it was in the original code:\n\nhttps://review.openstack.org/#/c/439520/13/nova/compute/manager.py@a4909\n\nAlso, why is this block all coming after the above conditionals, which is opposite from the original code?","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1f09bcb4928f06e3da274d2f5d0e2330bcf650f1","unresolved":false,"context_lines":[{"line_number":346,"context_line":"            self.driver_detach(context, instance, connector, volume_api,"},{"line_number":347,"context_line":"                               virt_driver)"},{"line_number":348,"context_line":"        elif not destroy_bdm:"},{"line_number":349,"context_line":"            LOG.debug(\"Skipping _driver_detach_volume during remote \""},{"line_number":350,"context_line":"                      \"rebuild.\", instance\u003dinstance)"},{"line_number":351,"context_line":"        elif destroy_bdm:"},{"line_number":352,"context_line":"            LOG.error(_LE(\"Unable to call for a driver detach of volume \""}],"source_content_type":"text/x-python","patch_set":13,"id":"ffe62b97_7a10bd00","line":349,"range":{"start_line":349,"start_character":32,"end_line":349,"end_character":53},"updated":"2017-03-29 16:41:03.000000000","message":"This is old now right?","commit_id":"29b26fb17091f0f01f08429ab197c232f6c37ff5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"56cce8371520d861500e685b884916d56b77795e","unresolved":false,"context_lines":[{"line_number":265,"context_line":"        mp \u003d self[\u0027mount_device\u0027]"},{"line_number":266,"context_line":"        volume_id \u003d self.volume_id"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"},{"line_number":269,"context_line":"                     \u0027 mountpoint %(mp)s\u0027), {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},"},{"line_number":270,"context_line":"                     instance\u003dinstance)"},{"line_number":271,"context_line":"        try:"},{"line_number":272,"context_line":"            if not virt_driver.instance_exists(instance):"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_5b839b05","line":269,"range":{"start_line":268,"start_character":75,"end_line":269,"end_character":23},"updated":"2017-03-30 20:24:52.000000000","message":"We have one too many spaces here.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"56cce8371520d861500e685b884916d56b77795e","unresolved":false,"context_lines":[{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"},{"line_number":269,"context_line":"                     \u0027 mountpoint %(mp)s\u0027), {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},"},{"line_number":270,"context_line":"                     instance\u003dinstance)"},{"line_number":271,"context_line":"        try:"},{"line_number":272,"context_line":"            if not virt_driver.instance_exists(instance):"},{"line_number":273,"context_line":"                LOG.warning(_LW(\u0027Detaching volume from unknown instance\u0027),"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_7b93ff50","line":270,"updated":"2017-03-30 20:24:52.000000000","message":"instance should be aligned with _LI","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c329ea8950970d40a48a26280cb966d1aec7b11b","unresolved":false,"context_lines":[{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"},{"line_number":269,"context_line":"                     \u0027 mountpoint %(mp)s\u0027), {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},"},{"line_number":270,"context_line":"                     instance\u003dinstance)"},{"line_number":271,"context_line":"        try:"},{"line_number":272,"context_line":"            if not virt_driver.instance_exists(instance):"},{"line_number":273,"context_line":"                LOG.warning(_LW(\u0027Detaching volume from unknown instance\u0027),"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_92e32606","line":270,"in_reply_to":"dfeb2761_1ba9807a","updated":"2017-06-01 03:01:22.000000000","message":"Yeah I\u0027d rather just clean that up later in separate change that handles the entire module.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"a584b6a6624477a17cee1dd271f68ea0e2cb9e20","unresolved":false,"context_lines":[{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        LOG.info(_LI(\u0027Attempting to driver detach volume %(volume_id)s from \u0027"},{"line_number":269,"context_line":"                     \u0027 mountpoint %(mp)s\u0027), {\u0027volume_id\u0027: volume_id, \u0027mp\u0027: mp},"},{"line_number":270,"context_line":"                     instance\u003dinstance)"},{"line_number":271,"context_line":"        try:"},{"line_number":272,"context_line":"            if not virt_driver.instance_exists(instance):"},{"line_number":273,"context_line":"                LOG.warning(_LW(\u0027Detaching volume from unknown instance\u0027),"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_1ba9807a","line":270,"in_reply_to":"dfeb2761_7b93ff50","updated":"2017-03-31 09:20:19.000000000","message":"Are we not also removing _LI now too, but meh.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"56cce8371520d861500e685b884916d56b77795e","unresolved":false,"context_lines":[{"line_number":271,"context_line":"        try:"},{"line_number":272,"context_line":"            if not virt_driver.instance_exists(instance):"},{"line_number":273,"context_line":"                LOG.warning(_LW(\u0027Detaching volume from unknown instance\u0027),"},{"line_number":274,"context_line":"                                instance\u003dinstance)"},{"line_number":275,"context_line":""},{"line_number":276,"context_line":"            encryption \u003d encryptors.get_encryption_metadata(context,"},{"line_number":277,"context_line":"                    volume_api, volume_id, connection_info)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_bb73c74d","line":274,"updated":"2017-03-30 20:24:52.000000000","message":"The instance kwarg should actually be lined up with _LW.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"56cce8371520d861500e685b884916d56b77795e","unresolved":false,"context_lines":[{"line_number":303,"context_line":"        if CONF.host \u003d\u003d instance.host:"},{"line_number":304,"context_line":"            self.driver_detach(context, instance, volume_api, virt_driver)"},{"line_number":305,"context_line":"        elif not destroy_bdm:"},{"line_number":306,"context_line":"            LOG.debug(\"Skipping _driver_detach during remote rebuild.\","},{"line_number":307,"context_line":"                      instance\u003dinstance)"},{"line_number":308,"context_line":"        elif destroy_bdm:"},{"line_number":309,"context_line":"            LOG.error(_LE(\"Unable to call for a driver detach of volume \""}],"source_content_type":"text/x-python","patch_set":16,"id":"dfeb2761_bbc18731","line":306,"range":{"start_line":306,"start_character":32,"end_line":306,"end_character":46},"updated":"2017-03-30 20:24:52.000000000","message":"Just driver_detach.","commit_id":"be6d5fa4e630b428fad74f2a6540e920d810c477"}]}