)]}'
{"cinder/volume/drivers/lvm.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bf8e906fa7c5aaa21940a04d6548c1d384088eac","unresolved":false,"context_lines":[{"line_number":847,"context_line":""},{"line_number":848,"context_line":"        # NOTE(jdg): For the TGT driver this is a noop, for LIO this removes"},{"line_number":849,"context_line":"        # the initiator IQN from the targets access list, so we\u0027re good"},{"line_number":850,"context_line":"        if not has_shared_connections:"},{"line_number":851,"context_line":"            self.target_driver.terminate_connection(volume, connector,"},{"line_number":852,"context_line":"                                                    **kwargs)"},{"line_number":853,"context_line":"        return has_shared_connections"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_6389451c","line":850,"updated":"2018-11-07 15:09:58.000000000","message":"We shouldn\u0027t need this, right?  We should be removing the connection.  What did you see that required this?","commit_id":"4de3e3be259afd3f812399767dad85e6e1e4bf3e"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"2b3282f1cea778346864e65f6238856439c04090","unresolved":false,"context_lines":[{"line_number":847,"context_line":""},{"line_number":848,"context_line":"        # NOTE(jdg): For the TGT driver this is a noop, for LIO this removes"},{"line_number":849,"context_line":"        # the initiator IQN from the targets access list, so we\u0027re good"},{"line_number":850,"context_line":"        if not has_shared_connections:"},{"line_number":851,"context_line":"            self.target_driver.terminate_connection(volume, connector,"},{"line_number":852,"context_line":"                                                    **kwargs)"},{"line_number":853,"context_line":"        return has_shared_connections"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_031c1179","line":850,"in_reply_to":"3f79a3b5_6389451c","updated":"2018-11-07 15:28:18.000000000","message":"Apologies this was just a WIP hack at working around the issue as described in the bug. This isn\u0027t going to work with multiple hosts and would require an additional check to ensure that we only terminate_connection when down to a single active attachment per host.\n\nI\u0027ll take another swing at this now.","commit_id":"4de3e3be259afd3f812399767dad85e6e1e4bf3e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c4d2749e1a2683fd936a9752485eee13200f994e","unresolved":false,"context_lines":[{"line_number":841,"context_line":"        # we need to do here is check if there is more than one attachment for"},{"line_number":842,"context_line":"        # the volume, if there is; let the caller know that they should NOT"},{"line_number":843,"context_line":"        # remove the export."},{"line_number":844,"context_line":"        has_shared_connections \u003d False"},{"line_number":845,"context_line":"        attachments \u003d volume.volume_attachment"},{"line_number":846,"context_line":"        if len(attachments) \u003e 1:"},{"line_number":847,"context_line":"            has_shared_connections \u003d True"},{"line_number":848,"context_line":""},{"line_number":849,"context_line":"        # NOTE(jdg): For the TGT driver this is a noop, for LIO this removes"},{"line_number":850,"context_line":"        # the initiator IQN from the targets access list, so we\u0027re good"},{"line_number":851,"context_line":"        # NOTE(lyarwood): Given the above note we should only call"},{"line_number":852,"context_line":"        # terminate_connection for the lioadm driver when there is only one"},{"line_number":853,"context_line":"        # attachment left to remove, otherwise the ACL will be removed"},{"line_number":854,"context_line":"        # prematurely while other instances are still accessing the volume."},{"line_number":855,"context_line":"        if volume.multiattach and has_shared_connections:"},{"line_number":856,"context_line":"            if len([a for a in attachments if a.connector \u003d\u003d connector]) \u003e 1:"},{"line_number":857,"context_line":"                return has_shared_connections"},{"line_number":858,"context_line":""},{"line_number":859,"context_line":"        self.target_driver.terminate_connection(volume, connector, **kwargs)"},{"line_number":860,"context_line":"        return has_shared_connections"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_165658fa","line":860,"range":{"start_line":844,"start_character":0,"end_line":860,"end_character":37},"updated":"2018-11-09 18:42:31.000000000","message":"I could be wrong, but this looks more complicated than it could be, and there seems to be a case where this would leave the volume mapped, and probably wouldn\u0027t be able to get deleted.\n\nThe case were we would have a leftover mapping would be when we are doing a live migration (the volume is attached to two different nodes) but the volume doesn\u0027t support multiattach. \n This method would return True, which would prevent a call to `remove_export`.\n\nHaven\u0027t tested it, but I think this could work:\n\n    def terminate_connection(self, volume, connector, **kwargs):\n        if sum(1 for a in volume.volume_attachment\n               if a.connector \u003d\u003d connector) \u003e 1:\n            return True\n\n        self.target_driver.terminate_connection(volume, connector, **kwargs)\n        return False","commit_id":"af87069de3f89c13808cb51027b5b2cf7ac03af5"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"6d466d36121a9a03bb4e16468d0fca78a36f4ec1","unresolved":false,"context_lines":[{"line_number":841,"context_line":"        # we need to do here is check if there is more than one attachment for"},{"line_number":842,"context_line":"        # the volume, if there is; let the caller know that they should NOT"},{"line_number":843,"context_line":"        # remove the export."},{"line_number":844,"context_line":"        has_shared_connections \u003d False"},{"line_number":845,"context_line":"        attachments \u003d volume.volume_attachment"},{"line_number":846,"context_line":"        if len(attachments) \u003e 1:"},{"line_number":847,"context_line":"            has_shared_connections \u003d True"},{"line_number":848,"context_line":""},{"line_number":849,"context_line":"        # NOTE(jdg): For the TGT driver this is a noop, for LIO this removes"},{"line_number":850,"context_line":"        # the initiator IQN from the targets access list, so we\u0027re good"},{"line_number":851,"context_line":"        # NOTE(lyarwood): Given the above note we should only call"},{"line_number":852,"context_line":"        # terminate_connection for the lioadm driver when there is only one"},{"line_number":853,"context_line":"        # attachment left to remove, otherwise the ACL will be removed"},{"line_number":854,"context_line":"        # prematurely while other instances are still accessing the volume."},{"line_number":855,"context_line":"        if volume.multiattach and has_shared_connections:"},{"line_number":856,"context_line":"            if len([a for a in attachments if a.connector \u003d\u003d connector]) \u003e 1:"},{"line_number":857,"context_line":"                return has_shared_connections"},{"line_number":858,"context_line":""},{"line_number":859,"context_line":"        self.target_driver.terminate_connection(volume, connector, **kwargs)"},{"line_number":860,"context_line":"        return has_shared_connections"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_193d2bb8","line":860,"range":{"start_line":844,"start_character":0,"end_line":860,"end_character":37},"in_reply_to":"3f79a3b5_165658fa","updated":"2018-11-09 19:46:08.000000000","message":"\u003e I could be wrong, but this looks more complicated than it could be,\n\u003e and there seems to be a case where this would leave the volume\n\u003e mapped, and probably wouldn\u0027t be able to get deleted.\n\u003e \n\u003e The case were we would have a leftover mapping would be when we are\n\u003e doing a live migration (the volume is attached to two different\n\u003e nodes) but the volume doesn\u0027t support multiattach.\n\u003e This method would return True, which would prevent a call to\n\u003e `remove_export`.\n \nThis would still call terminate_connection during a live migration as the destination host attachment would have a different connector associated with it. I\u0027ve just noticed the comment doesn\u0027t really call out that this is checking per host and not overall so I\u0027ll respin to make that clear now.\n\n\u003e Haven\u0027t tested it, but I think this could work:\n\u003e \n\u003e def terminate_connection(self, volume, connector, **kwargs):\n\u003e if sum(1 for a in volume.volume_attachment\n\u003e if a.connector \u003d\u003d connector) \u003e 1:\n\u003e return True\n\u003e\n\u003e self.target_driver.terminate_connection(volume, connector,\n\u003e **kwargs)\n\u003e return False\n\nThat isn\u0027t going to work and would lead to `remove_export`being called prematurely when multiple hosts have a single attachment to the same volume.","commit_id":"af87069de3f89c13808cb51027b5b2cf7ac03af5"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"139e61a298d04dfa678fc96346fa7cbe8d5fc585","unresolved":false,"context_lines":[{"line_number":849,"context_line":"        # remove, otherwise the ACL will be removed prematurely while other"},{"line_number":850,"context_line":"        # attachments on the same host are still accessing the volume."},{"line_number":851,"context_line":"        attachments \u003d volume.volume_attachment"},{"line_number":852,"context_line":"        if volume.multiattach:"},{"line_number":853,"context_line":"            if sum(1 for a in attachments if a.connector \u003d\u003d connector) \u003e 1:"},{"line_number":854,"context_line":"                return True"},{"line_number":855,"context_line":""},{"line_number":856,"context_line":"        self.target_driver.terminate_connection(volume, connector, **kwargs)"},{"line_number":857,"context_line":"        return len(attachments) \u003e 1"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_24479e1d","line":854,"range":{"start_line":852,"start_character":0,"end_line":854,"end_character":27},"updated":"2018-11-09 21:25:11.000000000","message":"nit: why not use an `and`?","commit_id":"d0b59152ed413f39599ffd1554c0e4e0038431aa"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"c4917c2125ddcf90df42b6cc98692656240c7e26","unresolved":false,"context_lines":[{"line_number":849,"context_line":"        # remove, otherwise the ACL will be removed prematurely while other"},{"line_number":850,"context_line":"        # attachments on the same host are still accessing the volume."},{"line_number":851,"context_line":"        attachments \u003d volume.volume_attachment"},{"line_number":852,"context_line":"        if volume.multiattach:"},{"line_number":853,"context_line":"            if sum(1 for a in attachments if a.connector \u003d\u003d connector) \u003e 1:"},{"line_number":854,"context_line":"                return True"},{"line_number":855,"context_line":""},{"line_number":856,"context_line":"        self.target_driver.terminate_connection(volume, connector, **kwargs)"},{"line_number":857,"context_line":"        return len(attachments) \u003e 1"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_4da2faad","line":854,"range":{"start_line":852,"start_character":0,"end_line":854,"end_character":27},"in_reply_to":"3f79a3b5_24479e1d","updated":"2018-11-10 12:45:44.000000000","message":"Readability, to my eye the resulting conditional looks horrendous over two lines but I\u0027d be happy to respin if people feel the optimisation is required here.","commit_id":"d0b59152ed413f39599ffd1554c0e4e0038431aa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"4d29fe80cf5f7b6fc36b04e1e4b191f182a2bf6b","unresolved":false,"context_lines":[{"line_number":849,"context_line":"        # remove, otherwise the ACL will be removed prematurely while other"},{"line_number":850,"context_line":"        # attachments on the same host are still accessing the volume."},{"line_number":851,"context_line":"        attachments \u003d volume.volume_attachment"},{"line_number":852,"context_line":"        if volume.multiattach:"},{"line_number":853,"context_line":"            if sum(1 for a in attachments if a.connector \u003d\u003d connector) \u003e 1:"},{"line_number":854,"context_line":"                return True"},{"line_number":855,"context_line":""},{"line_number":856,"context_line":"        self.target_driver.terminate_connection(volume, connector, **kwargs)"},{"line_number":857,"context_line":"        return len(attachments) \u003e 1"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_12baa7b9","line":854,"range":{"start_line":852,"start_character":0,"end_line":854,"end_character":27},"in_reply_to":"3f79a3b5_4da2faad","updated":"2018-11-14 14:22:50.000000000","message":"To my eyes it is more readable the single `if` statement, but that\u0027s probably just me.  We should move this, so I\u0027ll change my vote.","commit_id":"d0b59152ed413f39599ffd1554c0e4e0038431aa"}]}