)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"bc5e841a40797c6da044acfa9ed1ab68f175a6ec","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"As Cinder provides the necessary checks before detaching a volume"},{"line_number":10,"context_line":"it is unnecessary to track the volume state on the Nova side. This"},{"line_number":11,"context_line":"patch removes the unnecessary checks."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: If63dbd236ce0970ea245707cacb2664e3fdbdfd1"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"dfeb2761_01d0b41e","line":11,"updated":"2017-04-03 11:59:39.000000000","message":"I think we need at least a small discussion on why these checks are not required. Also the intent here is to help with multi-attach, thats not listed here.\n\nI have had a dig a bit further, and I am not sure this isn\u0027t a breaking API change.\n\nI quite like the idea of if a user does force-detach, Nova is still able to detach the volume. But I am not sure that works with the new API, as the connector would be lost, etc.\n\nThere seems to be quite a few subtle things being suggested here, we should take care to list them out here.\n\nPS\nI really think this needs a blueprint or a spec, just to track what the intent is here.","commit_id":"8ed18aea8e6bc23efdd15704fcad629af459aa47"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"836bae14a503d8625f9d0ae764da13794a6fbdf8","unresolved":false,"context_lines":[{"line_number":17,"context_line":"this as an opportunity to clean up the code and remove the parts we"},{"line_number":18,"context_line":"don\u0027t need anymore. As part of this activity we are working on to"},{"line_number":19,"context_line":"be able to support volume multi-attach, which will be easier if each"},{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_31bea9ff","line":20,"range":{"start_line":20,"start_character":0,"end_line":20,"end_character":6},"updated":"2017-07-20 16:39:47.000000000","message":"by module here you mean nova and cinder, right? might be better to say \u0027service\u0027 instead of module.","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":17,"context_line":"this as an opportunity to clean up the code and remove the parts we"},{"line_number":18,"context_line":"don\u0027t need anymore. As part of this activity we are working on to"},{"line_number":19,"context_line":"be able to support volume multi-attach, which will be easier if each"},{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_423cb5be","line":20,"range":{"start_line":20,"start_character":0,"end_line":20,"end_character":6},"in_reply_to":"1f1a1f67_31bea9ff","updated":"2017-07-20 18:09:13.000000000","message":"Done","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"836bae14a503d8625f9d0ae764da13794a6fbdf8","unresolved":false,"context_lines":[{"line_number":19,"context_line":"be able to support volume multi-attach, which will be easier if each"},{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"},{"line_number":24,"context_line":"error handling."},{"line_number":25,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_5146c523","line":22,"range":{"start_line":22,"start_character":11,"end_line":22,"end_character":20},"updated":"2017-07-20 16:39:47.000000000","message":"also","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":19,"context_line":"be able to support volume multi-attach, which will be easier if each"},{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"},{"line_number":24,"context_line":"error handling."},{"line_number":25,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_c279e593","line":22,"range":{"start_line":22,"start_character":11,"end_line":22,"end_character":20},"in_reply_to":"1f1a1f67_5146c523","updated":"2017-07-20 18:09:13.000000000","message":"Done","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"836bae14a503d8625f9d0ae764da13794a6fbdf8","unresolved":false,"context_lines":[{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"},{"line_number":24,"context_line":"error handling."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Change-Id: If63dbd236ce0970ea245707cacb2664e3fdbdfd1"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_b1263968","line":23,"range":{"start_line":23,"start_character":35,"end_line":23,"end_character":48},"updated":"2017-07-20 16:39:47.000000000","message":"check_detach could also raise VolumeUnattached - are we handling that? Because VolumeUnattached is handled in the REST API controller code as a noop, but InvalidVolume is handled as a 400 response.\n\nDoes Cinder noop if you ask to begin_detaching an already unattached volume? It looks like it would fail on an unexpected status:\n\nhttps://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L690\n\nAlthough it looks like the VolumeUnattached check was added to check_detach specifically as groundwork for multi-attach support, so maybe we should just remove that exception and sort it out later when we actually support multi-attach? We should make a note of it either way.","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e83961c4335d11199147c9811fcddd9299f4ec01","unresolved":false,"context_lines":[{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"},{"line_number":24,"context_line":"error handling."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Change-Id: If63dbd236ce0970ea245707cacb2664e3fdbdfd1"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_e4b631dd","line":23,"range":{"start_line":23,"start_character":35,"end_line":23,"end_character":48},"in_reply_to":"1f1a1f67_b1263968","updated":"2017-07-20 17:00:49.000000000","message":"Sorry, I realize more here now. So VolumeUnattached would be handled as a 404 response in the detach volume and swap volume operations:\n\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/compute/volumes.py#L414\n\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/compute/volumes.py#L470\n\nSo that \u0027found\u0027 variable will be False and result in a 404. So maybe leaking this up as a 400 now would be OK...","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":20,"context_line":"module has their own set of responsibilities."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"This patch also also handles error translation by catching"},{"line_number":23,"context_line":"InvalidInput exception and raising InvalidVolume to match the former"},{"line_number":24,"context_line":"error handling."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Change-Id: If63dbd236ce0970ea245707cacb2664e3fdbdfd1"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"1f1a1f67_e2606138","line":23,"range":{"start_line":23,"start_character":35,"end_line":23,"end_character":48},"in_reply_to":"1f1a1f67_e4b631dd","updated":"2017-07-20 18:09:13.000000000","message":"Added a short paragraph about VolumeUnattached here.","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"}],"nova/compute/api.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e83961c4335d11199147c9811fcddd9299f4ec01","unresolved":false,"context_lines":[{"line_number":3658,"context_line":"                                   disk_bus, device_type, tag\u003dtag)"},{"line_number":3659,"context_line":""},{"line_number":3660,"context_line":"    def _check_and_begin_detach(self, context, volume, instance):"},{"line_number":3661,"context_line":"        self.volume_api.check_detach(context, volume, instance\u003dinstance)"},{"line_number":3662,"context_line":"        self.volume_api.begin_detaching(context, volume[\u0027id\u0027])"},{"line_number":3663,"context_line":""},{"line_number":3664,"context_line":"    def _detach_volume(self, context, instance, volume):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_244789bf","side":"PARENT","line":3661,"range":{"start_line":3661,"start_character":8,"end_line":3661,"end_character":72},"updated":"2017-07-20 17:00:49.000000000","message":"If we wanted to, we could move this here:\n\nhttps://github.com/openstack/nova/blob/master/nova/volume/cinder.py#L350-L356\n\nFor the VolumeUnattached scenario that I\u0027m asking about.","commit_id":"c18d5c6325616b025fbe6b1086fe5eabe557209f"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":3658,"context_line":"                                   disk_bus, device_type, tag\u003dtag)"},{"line_number":3659,"context_line":""},{"line_number":3660,"context_line":"    def _check_and_begin_detach(self, context, volume, instance):"},{"line_number":3661,"context_line":"        self.volume_api.check_detach(context, volume, instance\u003dinstance)"},{"line_number":3662,"context_line":"        self.volume_api.begin_detaching(context, volume[\u0027id\u0027])"},{"line_number":3663,"context_line":""},{"line_number":3664,"context_line":"    def _detach_volume(self, context, instance, volume):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_c7ec572b","side":"PARENT","line":3661,"range":{"start_line":3661,"start_character":8,"end_line":3661,"end_character":72},"in_reply_to":"1f1a1f67_244789bf","updated":"2017-07-20 18:09:13.000000000","message":"For swap_volume check_detach was kind of a duplication, so I wouldn\u0027t move that multi-attach related change here now, rather fix it if needed, when we get to actual multi-attach.","commit_id":"c18d5c6325616b025fbe6b1086fe5eabe557209f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e0dd3a7758a36bfda78bd1a833b7e6b9013c61a9","unresolved":false,"context_lines":[{"line_number":3709,"context_line":"                                    vm_states.RESIZED, vm_states.SOFT_DELETED])"},{"line_number":3710,"context_line":"    def swap_volume(self, context, instance, old_volume, new_volume):"},{"line_number":3711,"context_line":"        \"\"\"Swap volume attached to an instance.\"\"\""},{"line_number":3712,"context_line":"        if old_volume[\u0027attach_status\u0027] \u003d\u003d \u0027detached\u0027:"},{"line_number":3713,"context_line":"            raise exception.VolumeUnattached(volume_id\u003dold_volume[\u0027id\u0027])"},{"line_number":3714,"context_line":"        # The caller likely got the instance from volume[\u0027attachments\u0027]"},{"line_number":3715,"context_line":"        # in the first place, but let\u0027s sanity check."},{"line_number":3716,"context_line":"        if not old_volume.get(\u0027attachments\u0027, {}).get(instance.uuid):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_440e0533","line":3713,"range":{"start_line":3712,"start_character":8,"end_line":3713,"end_character":72},"updated":"2017-07-20 16:42:43.000000000","message":"Interesting - I just noticed this, which is handled in the REST API controller code as a noop, so it wouldn\u0027t fail the request, but it wouldn\u0027t swap the volume and attach to new_volume either. That seems like a bug we should fix separately.\n\nLooks like this was intentional:\n\nI661b97762af70765ab8111313d24cf1a175f7e6c\n\n4 years ago...","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e83961c4335d11199147c9811fcddd9299f4ec01","unresolved":false,"context_lines":[{"line_number":3709,"context_line":"                                    vm_states.RESIZED, vm_states.SOFT_DELETED])"},{"line_number":3710,"context_line":"    def swap_volume(self, context, instance, old_volume, new_volume):"},{"line_number":3711,"context_line":"        \"\"\"Swap volume attached to an instance.\"\"\""},{"line_number":3712,"context_line":"        if old_volume[\u0027attach_status\u0027] \u003d\u003d \u0027detached\u0027:"},{"line_number":3713,"context_line":"            raise exception.VolumeUnattached(volume_id\u003dold_volume[\u0027id\u0027])"},{"line_number":3714,"context_line":"        # The caller likely got the instance from volume[\u0027attachments\u0027]"},{"line_number":3715,"context_line":"        # in the first place, but let\u0027s sanity check."},{"line_number":3716,"context_line":"        if not old_volume.get(\u0027attachments\u0027, {}).get(instance.uuid):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_64ca2150","line":3713,"range":{"start_line":3712,"start_character":8,"end_line":3713,"end_character":72},"in_reply_to":"1f1a1f67_440e0533","updated":"2017-07-20 17:00:49.000000000","message":"Correction, it\u0027s handled as a 404 in the API.","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":3709,"context_line":"                                    vm_states.RESIZED, vm_states.SOFT_DELETED])"},{"line_number":3710,"context_line":"    def swap_volume(self, context, instance, old_volume, new_volume):"},{"line_number":3711,"context_line":"        \"\"\"Swap volume attached to an instance.\"\"\""},{"line_number":3712,"context_line":"        if old_volume[\u0027attach_status\u0027] \u003d\u003d \u0027detached\u0027:"},{"line_number":3713,"context_line":"            raise exception.VolumeUnattached(volume_id\u003dold_volume[\u0027id\u0027])"},{"line_number":3714,"context_line":"        # The caller likely got the instance from volume[\u0027attachments\u0027]"},{"line_number":3715,"context_line":"        # in the first place, but let\u0027s sanity check."},{"line_number":3716,"context_line":"        if not old_volume.get(\u0027attachments\u0027, {}).get(instance.uuid):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_87f65ff9","line":3713,"range":{"start_line":3712,"start_character":8,"end_line":3713,"end_character":72},"in_reply_to":"1f1a1f67_64ca2150","updated":"2017-07-20 18:09:13.000000000","message":"+1","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c65cba0699e09c39dc19929dd37a0ccdadb5925f","unresolved":false,"context_lines":[{"line_number":3658,"context_line":"                                   disk_bus, device_type, tag\u003dtag)"},{"line_number":3659,"context_line":""},{"line_number":3660,"context_line":"    def _check_and_begin_detach(self, context, volume, instance):"},{"line_number":3661,"context_line":"        self.volume_api.check_detach(context, volume, instance\u003dinstance)"},{"line_number":3662,"context_line":"        self.volume_api.begin_detaching(context, volume[\u0027id\u0027])"},{"line_number":3663,"context_line":""},{"line_number":3664,"context_line":"    def _detach_volume(self, context, instance, volume):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1f1a1f67_3851e663","side":"PARENT","line":3661,"range":{"start_line":3661,"start_character":8,"end_line":3661,"end_character":72},"updated":"2017-07-20 19:08:41.000000000","message":"If this isn\u0027t called for detach volume, then VolumeUnattached won\u0027t be raised in this flow, which means we can remove this handling in the REST API controller code:\n\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/compute/volumes.py#L470\n\nSo let\u0027s at least do that.","commit_id":"c18d5c6325616b025fbe6b1086fe5eabe557209f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c65cba0699e09c39dc19929dd37a0ccdadb5925f","unresolved":false,"context_lines":[{"line_number":3709,"context_line":"                                    vm_states.RESIZED, vm_states.SOFT_DELETED])"},{"line_number":3710,"context_line":"    def swap_volume(self, context, instance, old_volume, new_volume):"},{"line_number":3711,"context_line":"        \"\"\"Swap volume attached to an instance.\"\"\""},{"line_number":3712,"context_line":"        if old_volume[\u0027attach_status\u0027] \u003d\u003d \u0027detached\u0027:"},{"line_number":3713,"context_line":"            raise exception.VolumeUnattached(volume_id\u003dold_volume[\u0027id\u0027])"},{"line_number":3714,"context_line":"        # The caller likely got the instance from volume[\u0027attachments\u0027]"},{"line_number":3715,"context_line":"        # in the first place, but let\u0027s sanity check."},{"line_number":3716,"context_line":"        if not old_volume.get(\u0027attachments\u0027, {}).get(instance.uuid):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1f1a1f67_b898561b","line":3713,"range":{"start_line":3712,"start_character":8,"end_line":3713,"end_character":72},"updated":"2017-07-20 19:08:41.000000000","message":"Can/should we also remove this? This was basically duplication with check_detach, but wasn\u0027t using check_detach for the old_volume. And if the old_volume isn\u0027t attached, the next check below should catch that and raise InvalidVolume, or the begin_detaching call should catch it and fail. So it seems pointless to leave this in here too if the goal of the change is to remove the state checking in nova.\n\nNote that if you remove this here, you should remove it\u0027s handling in the REST API controller code here:\n\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/compute/volumes.py#L414","commit_id":"2d6e43afff1845c28d97c8250158c0d12121a36b"}],"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"836bae14a503d8625f9d0ae764da13794a6fbdf8","unresolved":false,"context_lines":[{"line_number":2935,"context_line":"            action\u003dfields.NotificationAction.REBUILD,"},{"line_number":2936,"context_line":"            phase\u003dfields.NotificationPhase.END)"},{"line_number":2937,"context_line":""},{"line_number":2938,"context_line":"    def _handle_bad_volumes_detached(self, context, instance, bad_devices,"},{"line_number":2939,"context_line":"                                     block_device_info):"},{"line_number":2940,"context_line":"        \"\"\"Handle cases where the virt-layer had to detach non-working volumes"},{"line_number":2941,"context_line":"        in order to complete an operation."}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_715e6146","line":2938,"updated":"2017-07-20 16:39:47.000000000","message":"OK so this is only used in the reboot operation, and only used in the xenapi driver, so I think this is OK, it doesn\u0027t have any bearing on the API or assumptions that the API would make about what\u0027s happening in the computes (which is always a concern for mixed-version compute support).","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":2935,"context_line":"            action\u003dfields.NotificationAction.REBUILD,"},{"line_number":2936,"context_line":"            phase\u003dfields.NotificationPhase.END)"},{"line_number":2937,"context_line":""},{"line_number":2938,"context_line":"    def _handle_bad_volumes_detached(self, context, instance, bad_devices,"},{"line_number":2939,"context_line":"                                     block_device_info):"},{"line_number":2940,"context_line":"        \"\"\"Handle cases where the virt-layer had to detach non-working volumes"},{"line_number":2941,"context_line":"        in order to complete an operation."}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_82ed2d8d","line":2938,"in_reply_to":"1f1a1f67_715e6146","updated":"2017-07-20 18:09:13.000000000","message":"+1","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"836bae14a503d8625f9d0ae764da13794a6fbdf8","unresolved":false,"context_lines":[{"line_number":2947,"context_line":"                except KeyError:"},{"line_number":2948,"context_line":"                    continue"},{"line_number":2949,"context_line":""},{"line_number":2950,"context_line":"                # NOTE(sirp): ideally we\u0027d just call"},{"line_number":2951,"context_line":"                # `compute_api.detach_volume` here but since that hits the"},{"line_number":2952,"context_line":"                # DB directly, that\u0027s off limits from within the"},{"line_number":2953,"context_line":"                # compute-manager."},{"line_number":2954,"context_line":"                #"},{"line_number":2955,"context_line":"                # API-detach"},{"line_number":2956,"context_line":"                LOG.info(\"Detaching from volume api: %s\", volume_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_b1b6398a","line":2953,"range":{"start_line":2950,"start_character":16,"end_line":2953,"end_character":34},"updated":"2017-07-20 16:39:47.000000000","message":"Good lord this is an old comment. Unrelated. Just makes me laugh.","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"004b3b35ad8fc732b13e459278ac0166cd787d66","unresolved":false,"context_lines":[{"line_number":2947,"context_line":"                except KeyError:"},{"line_number":2948,"context_line":"                    continue"},{"line_number":2949,"context_line":""},{"line_number":2950,"context_line":"                # NOTE(sirp): ideally we\u0027d just call"},{"line_number":2951,"context_line":"                # `compute_api.detach_volume` here but since that hits the"},{"line_number":2952,"context_line":"                # DB directly, that\u0027s off limits from within the"},{"line_number":2953,"context_line":"                # compute-manager."},{"line_number":2954,"context_line":"                #"},{"line_number":2955,"context_line":"                # API-detach"},{"line_number":2956,"context_line":"                LOG.info(\"Detaching from volume api: %s\", volume_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"1f1a1f67_024b5db0","line":2953,"range":{"start_line":2950,"start_character":16,"end_line":2953,"end_character":34},"in_reply_to":"1f1a1f67_b1b6398a","updated":"2017-07-20 18:09:13.000000000","message":":)","commit_id":"83a25b62bf617a40b56c695dd75adc3f6da8465a"}],"nova/volume/cinder.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1b9fdb61d7cc61e7dfffc63155903d92c08ccc7a","unresolved":false,"context_lines":[{"line_number":292,"context_line":"        if instance is not None and instance.uuid not in volume[\u0027attachments\u0027]:"},{"line_number":293,"context_line":"            # TODO(ildikov): change it to a better exception, when enable"},{"line_number":294,"context_line":"            # multi-attach."},{"line_number":295,"context_line":"            raise exception.VolumeUnattached(volume_id\u003dvolume[\u0027id\u0027])"},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"    @translate_volume_exception"},{"line_number":298,"context_line":"    def reserve_volume(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":1,"id":"dfeb2761_7b95f1e3","side":"PARENT","line":295,"updated":"2017-04-03 11:11:38.000000000","message":"So lets think about the role we want this code to play...\n\nA user accidentally gets the volume-ids they are using when calling detach mixed up. Its attached to some other server, or not attached to any server, or they already called detach and we are half way through the process.\n\nWhat the user wants is the API to quickly return with an error message warning them about whats wrong.\n\nIf we remove this, I believe we would not actually find the instance-uuid is bad until we notice there is nothing to detach on the instance, we might even incorrect detach a volume that is actually attached on another instance somewhere? I think.\n\nAm I overthinking that?","commit_id":"d83386524b07cf9da038dae4d487f6a234918e6c"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"bc5e841a40797c6da044acfa9ed1ab68f175a6ec","unresolved":false,"context_lines":[{"line_number":292,"context_line":"        if instance is not None and instance.uuid not in volume[\u0027attachments\u0027]:"},{"line_number":293,"context_line":"            # TODO(ildikov): change it to a better exception, when enable"},{"line_number":294,"context_line":"            # multi-attach."},{"line_number":295,"context_line":"            raise exception.VolumeUnattached(volume_id\u003dvolume[\u0027id\u0027])"},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"    @translate_volume_exception"},{"line_number":298,"context_line":"    def reserve_volume(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":1,"id":"dfeb2761_415a3cf3","side":"PARENT","line":295,"in_reply_to":"dfeb2761_7b95f1e3","updated":"2017-04-03 11:59:39.000000000","message":"So I missed all the logic in here doing checks against the BDM, it shouldn\u0027t be in there, but it is:\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/compute/volumes.py#L431\n\nThat probably means the worst of the cases I mention are protected here.\n\nBut I still worry about when you do force-detach in Cinder. Although if Nova still thinks its attached. Maybe thats best.","commit_id":"d83386524b07cf9da038dae4d487f6a234918e6c"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"5bf3c4626f044790d5381215b4be1498cf6a9438","unresolved":false,"context_lines":[{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    @translate_volume_exception"},{"line_number":288,"context_line":"    def begin_detaching(self, context, volume_id):"},{"line_number":289,"context_line":"        cinderclient(context).volumes.begin_detaching(volume_id)"},{"line_number":290,"context_line":""},{"line_number":291,"context_line":"    @translate_volume_exception"},{"line_number":292,"context_line":"    def roll_detaching(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":1,"id":"dfeb2761_3b6d2904","line":289,"updated":"2017-04-03 11:12:22.000000000","message":"Its unclear to me how this could check the attachments have a matching instance.uuid.","commit_id":"8ed18aea8e6bc23efdd15704fcad629af459aa47"}]}