)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f097ab23c3495fd78d04956dc17d3e49c6ca5ac5","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Block swap volume on volumes with \u003e1 rw attachment"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The only virt driver that supports swap volume is the"},{"line_number":10,"context_line":"libvirt driver and the blockRebase operation is not"},{"line_number":11,"context_line":"designed to handle multiattach volumes, so if we\u0027re"},{"line_number":12,"context_line":"swapping from a multiattach volume that has more than"},{"line_number":13,"context_line":"one read/write attachment, another server on the"},{"line_number":14,"context_line":"secondary attachment could be writing to the volume"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5f7c97a3_282138fd","line":11,"range":{"start_line":9,"start_character":1,"end_line":11,"end_character":38},"updated":"2018-06-07 10:04:51.000000000","message":"This limitation isn\u0027t specific to the libvirt driver; it\u0027s inherent in the contract of the swap_volume call. It would not be possible for any compute host to do any kind of copy here without coordination with other affected compute hosts, which aren\u0027t part of the call.\n\nFor example, if Xen implemented this call by pausing the VM and copying the underlying data with dd, it would also be broken in exactly the same way.\n\nThe limitation is fundamental to any copy operation. The libvirt driver and specifically blockRebase aren\u0027t relevant here.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"02ffe84044939cb04f0e5e876a08fbe74a2066d1","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Block swap volume on volumes with \u003e1 rw attachment"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The only virt driver that supports swap volume is the"},{"line_number":10,"context_line":"libvirt driver and the blockRebase operation is not"},{"line_number":11,"context_line":"designed to handle multiattach volumes, so if we\u0027re"},{"line_number":12,"context_line":"swapping from a multiattach volume that has more than"},{"line_number":13,"context_line":"one read/write attachment, another server on the"},{"line_number":14,"context_line":"secondary attachment could be writing to the volume"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5f7c97a3_eaa80fda","line":11,"range":{"start_line":9,"start_character":1,"end_line":11,"end_character":38},"in_reply_to":"5f7c97a3_282138fd","updated":"2018-06-07 12:47:28.000000000","message":"I can remove it.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f576ce80304c63471799198ce8f9de17756e5d91","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Block swap volume on volumes with \u003e1 rw attachment"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The only virt driver that supports swap volume is the"},{"line_number":10,"context_line":"libvirt driver and the blockRebase operation is not"},{"line_number":11,"context_line":"designed to handle multiattach volumes, so if we\u0027re"},{"line_number":12,"context_line":"swapping from a multiattach volume that has more than"},{"line_number":13,"context_line":"one read/write attachment, another server on the"},{"line_number":14,"context_line":"secondary attachment could be writing to the volume"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5f7c97a3_ea4fafbb","line":11,"range":{"start_line":9,"start_character":1,"end_line":11,"end_character":38},"in_reply_to":"5f7c97a3_eaa80fda","updated":"2018-06-07 13:27:06.000000000","message":"Thanks.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"212d234a52871896964498162b40cae9bffb1bc5","unresolved":false,"context_lines":[{"line_number":18,"context_line":"than one read/write attachment on the volume, the swap"},{"line_number":19,"context_line":"volume operation fails with a 400 BadRequest error."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Depends-On: https://review.openstack.org/573025/"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"Closes-Bug: #1775418"},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"5f7c97a3_ae41ab0c","line":21,"range":{"start_line":21,"start_character":12,"end_line":21,"end_character":48},"updated":"2018-06-08 15:54:43.000000000","message":"Shouldn\u0027t this be a change ID? Or did that change?","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"}],"nova/compute/api.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd74e30a1d7ea0811aa9f49434ea6aac83cb7055","unresolved":false,"context_lines":[{"line_number":4075,"context_line":"                                                instance\u003dinstance)"},{"line_number":4076,"context_line":""},{"line_number":4077,"context_line":"        # Disallow swapping with multiattach volumes."},{"line_number":4078,"context_line":"        # TODO(mriedem): We should be counting attachments here, and ideally"},{"line_number":4079,"context_line":"        # counting read/write attachments. We know old_volume has at least"},{"line_number":4080,"context_line":"        # one attachment (to this server) but if it has an attachment to"},{"line_number":4081,"context_line":"        # another server (in read/write mode) we should block it."},{"line_number":4082,"context_line":"        if old_volume[\u0027multiattach\u0027] or new_volume[\u0027multiattach\u0027]:"},{"line_number":4083,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4084,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_4626fb18","line":4081,"range":{"start_line":4078,"start_character":0,"end_line":4081,"end_character":65},"updated":"2018-06-06 15:17:38.000000000","message":"The reason I proposed just checking the multiattach flag, as you\u0027ve done here, rather than counting attachments is that the latter is inherently racy as I think you noted elsewhere. If we combined your change below with a hopefully fairly simple change to cinder, we can keep this functionality for users and be race free.\n\nSpecifically, modify cinder to allow us to change the multiattach flag on a volume as long as it has \u003c2 attachments.\n\nWith this change we wouldn\u0027t need to implement any locks anywhere. The user does:\n\n* Detach the volume from everywhere except at most 1 VM\n* Set multiattach\u003dFalse\n* Retype the volume\n* Set multiattach\u003dTrue\n* Re-attach the volume wherever\n\nOn the cinder side, it would also be more user friendly to deny retype for multiattach volumes immediately rather than wait for Nova to fail it, which you also suggested in IRC.","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6883531e55f472d7eda64f09050589d427ad5ecc","unresolved":false,"context_lines":[{"line_number":4078,"context_line":"        # TODO(mriedem): We should be counting attachments here, and ideally"},{"line_number":4079,"context_line":"        # counting read/write attachments. We know old_volume has at least"},{"line_number":4080,"context_line":"        # one attachment (to this server) but if it has an attachment to"},{"line_number":4081,"context_line":"        # another server (in read/write mode) we should block it."},{"line_number":4082,"context_line":"        if old_volume[\u0027multiattach\u0027] or new_volume[\u0027multiattach\u0027]:"},{"line_number":4083,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4084,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_66ca1747","line":4081,"updated":"2018-06-06 14:53:32.000000000","message":"Would it be possible to get a race here? swap-volume is requested with only one read-write attachment, while it\u0027s running another read-write attachment is added? Pretty unlikely, I know, but then all races are :)","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c15f7980f7a5e433fc28d9215f90cd4899791a96","unresolved":false,"context_lines":[{"line_number":4075,"context_line":"                                                instance\u003dinstance)"},{"line_number":4076,"context_line":""},{"line_number":4077,"context_line":"        # Disallow swapping with multiattach volumes."},{"line_number":4078,"context_line":"        # TODO(mriedem): We should be counting attachments here, and ideally"},{"line_number":4079,"context_line":"        # counting read/write attachments. We know old_volume has at least"},{"line_number":4080,"context_line":"        # one attachment (to this server) but if it has an attachment to"},{"line_number":4081,"context_line":"        # another server (in read/write mode) we should block it."},{"line_number":4082,"context_line":"        if old_volume[\u0027multiattach\u0027] or new_volume[\u0027multiattach\u0027]:"},{"line_number":4083,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4084,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_fcb6b83e","line":4081,"range":{"start_line":4078,"start_character":0,"end_line":4081,"end_character":65},"in_reply_to":"5f7c97a3_4626fb18","updated":"2018-06-06 15:47:04.000000000","message":"Hmm, my proposal above doesn\u0027t work. The issue is that the hypervisor uses the multiattach flag to alter the semantics of the disk, so if we changed it we\u0027d also have to coordinate with the hypervisor. You\u0027d probably need something like separate \u0027multiattachable\u0027 and \u0027multiattached\u0027 flags, and suddenly it\u0027s much less simple.\n\nPlease ignore this suggestion, I don\u0027t currently have an alternative.","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"08e0540b9e6d86d14ba8d5143f544b9f05d106f7","unresolved":false,"context_lines":[{"line_number":4078,"context_line":"        # TODO(mriedem): We should be counting attachments here, and ideally"},{"line_number":4079,"context_line":"        # counting read/write attachments. We know old_volume has at least"},{"line_number":4080,"context_line":"        # one attachment (to this server) but if it has an attachment to"},{"line_number":4081,"context_line":"        # another server (in read/write mode) we should block it."},{"line_number":4082,"context_line":"        if old_volume[\u0027multiattach\u0027] or new_volume[\u0027multiattach\u0027]:"},{"line_number":4083,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4084,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_dc073c81","line":4081,"in_reply_to":"5f7c97a3_66ca1747","updated":"2018-06-06 16:00:46.000000000","message":"\u003e Would it be possible to get a race here? swap-volume is requested\n \u003e with only one read-write attachment, while it\u0027s running another\n \u003e read-write attachment is added? Pretty unlikely, I know, but then\n \u003e all races are :)\n\nSure, but it\u0027s also bogus to deny a swap volume on a multiattach volume that is otherwise a normal volume if it\u0027s not attached to more than one server. I don\u0027t think there is an easy lock-free way to do that on the cinder side either FWIW, but I also don\u0027t think we should just completely disallow retype/volume migration for multiattach volumes if they are only attached to one server (or in the case of the volume we\u0027re swapping to, it doesn\u0027t have any attachments).","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"08e0540b9e6d86d14ba8d5143f544b9f05d106f7","unresolved":false,"context_lines":[{"line_number":4075,"context_line":"                                                instance\u003dinstance)"},{"line_number":4076,"context_line":""},{"line_number":4077,"context_line":"        # Disallow swapping with multiattach volumes."},{"line_number":4078,"context_line":"        # TODO(mriedem): We should be counting attachments here, and ideally"},{"line_number":4079,"context_line":"        # counting read/write attachments. We know old_volume has at least"},{"line_number":4080,"context_line":"        # one attachment (to this server) but if it has an attachment to"},{"line_number":4081,"context_line":"        # another server (in read/write mode) we should block it."},{"line_number":4082,"context_line":"        if old_volume[\u0027multiattach\u0027] or new_volume[\u0027multiattach\u0027]:"},{"line_number":4083,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4084,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_9cb304ea","line":4081,"range":{"start_line":4078,"start_character":0,"end_line":4081,"end_character":65},"in_reply_to":"5f7c97a3_fcb6b83e","updated":"2018-06-06 16:00:46.000000000","message":"\u003e Specifically, modify cinder to allow us to change the multiattach flag on a volume as long as it has \u003c2 attachments.\n\n\u003e Hmm, my proposal above doesn\u0027t work.\n\nYeah....glad you dropped that because I wasn\u0027t going to go down that rabbit hole.","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d2ba67af841085735f8eae09e243eaa40c28dee6","unresolved":false,"context_lines":[{"line_number":4116,"context_line":"        # attachment. We know the old_volume has at least one attachment since"},{"line_number":4117,"context_line":"        # it\u0027s attached to this server."},{"line_number":4118,"context_line":"        if (self._count_attachments_for_swap(context, old_volume) \u003e 1 or"},{"line_number":4119,"context_line":"                self._count_attachments_for_swap(context, new_volume) \u003e 1):"},{"line_number":4120,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4121,"context_line":""},{"line_number":4122,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_6b9f3765","line":4119,"range":{"start_line":4119,"start_character":16,"end_line":4119,"end_character":69},"updated":"2018-06-06 19:54:45.000000000","message":"I don\u0027t actually know if we need this for the target volume.","commit_id":"9788dc486b2cbd1e48a36530d02aa72e112b18ee"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"051ebdef74ff1e95b25173c311d4d51f231023c6","unresolved":false,"context_lines":[{"line_number":4055,"context_line":"        else:"},{"line_number":4056,"context_line":"            self._detach_volume(context, instance, volume)"},{"line_number":4057,"context_line":""},{"line_number":4058,"context_line":"    def _count_attachments_for_swap(self, ctxt, volume):"},{"line_number":4059,"context_line":"        \"\"\"Counts the number of attachments for a swap-related volume."},{"line_number":4060,"context_line":""},{"line_number":4061,"context_line":"        Attempts to only count read/write attachments if the volume attachment"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_917a06ee","line":4058,"updated":"2018-06-06 21:44:52.000000000","message":"Maybe just _count_attachments? I know we\u0027re doing it for swap, but they way the method is written it\u0027s completely generic, why not reflect that in the name?","commit_id":"91bca49490aefba77c2cad872f75eb687d2c7d83"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"97c6ff7285723b0c718c2e04166a959f75344af7","unresolved":false,"context_lines":[{"line_number":4055,"context_line":"        else:"},{"line_number":4056,"context_line":"            self._detach_volume(context, instance, volume)"},{"line_number":4057,"context_line":""},{"line_number":4058,"context_line":"    def _count_attachments_for_swap(self, ctxt, volume):"},{"line_number":4059,"context_line":"        \"\"\"Counts the number of attachments for a swap-related volume."},{"line_number":4060,"context_line":""},{"line_number":4061,"context_line":"        Attempts to only count read/write attachments if the volume attachment"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_1107d61f","line":4058,"in_reply_to":"5f7c97a3_917a06ee","updated":"2018-06-06 21:50:11.000000000","message":"Because swap only cares about read/write attachments.","commit_id":"91bca49490aefba77c2cad872f75eb687d2c7d83"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8630ad0d14ab661d15aa8924041df4673334d0e3","unresolved":false,"context_lines":[{"line_number":4068,"context_line":"        \"\"\""},{"line_number":4069,"context_line":"        # This is a dict, keyed by server ID, to a dict of attachment_id and"},{"line_number":4070,"context_line":"        # mountpoint."},{"line_number":4071,"context_line":"        attachments \u003d volume[\u0027attachments\u0027]"},{"line_number":4072,"context_line":"        # Multiattach volumes can have more than one attachment, so if there"},{"line_number":4073,"context_line":"        # is more than one attachment, attempt to count the read/write"},{"line_number":4074,"context_line":"        # attachments."}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_91bf665f","line":4071,"updated":"2018-06-06 21:39:42.000000000","message":"Hmm, this is blowing up:\n\nhttp://logs.openstack.org/90/572790/3/check/tempest-full/71a0cc6/controller/logs/screen-n-api.txt.gz?level\u003dTRACE#_Jun_06_20_24_13_846531\n\nI guess because the dict doesn\u0027t have the attachments key if there are no attachments.","commit_id":"91bca49490aefba77c2cad872f75eb687d2c7d83"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"97c6ff7285723b0c718c2e04166a959f75344af7","unresolved":false,"context_lines":[{"line_number":4103,"context_line":"        if not old_volume.get(\u0027attachments\u0027, {}).get(instance.uuid):"},{"line_number":4104,"context_line":"            msg \u003d _(\"Old volume is attached to a different instance.\")"},{"line_number":4105,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":4106,"context_line":"        if new_volume[\u0027attach_status\u0027] \u003d\u003d \u0027attached\u0027:"},{"line_number":4107,"context_line":"            msg \u003d _(\"New volume must be detached in order to swap.\")"},{"line_number":4108,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":4109,"context_line":"        if int(new_volume[\u0027size\u0027]) \u003c int(old_volume[\u0027size\u0027]):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_345520fb","line":4106,"range":{"start_line":4106,"start_character":8,"end_line":4106,"end_character":53},"updated":"2018-06-06 21:50:11.000000000","message":"I missed this, but you can\u0027t swap to a volume that is attached to anything already, regardless of read/write. So I can move the attachment count check for new_volume below.","commit_id":"91bca49490aefba77c2cad872f75eb687d2c7d83"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"051ebdef74ff1e95b25173c311d4d51f231023c6","unresolved":false,"context_lines":[{"line_number":4116,"context_line":"        # attachment. We know the old_volume has at least one attachment since"},{"line_number":4117,"context_line":"        # it\u0027s attached to this server."},{"line_number":4118,"context_line":"        if (self._count_attachments_for_swap(context, old_volume) \u003e 1 or"},{"line_number":4119,"context_line":"                self._count_attachments_for_swap(context, new_volume) \u003e 1):"},{"line_number":4120,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4121,"context_line":""},{"line_number":4122,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_f1749ad2","line":4119,"range":{"start_line":4119,"start_character":70,"end_line":4119,"end_character":71},"updated":"2018-06-06 21:44:52.000000000","message":"So then shouldn\u0027t this be \u003e\u003d1 or \u003e 0 - ie, old_volume has \u003e\u003d1, but if it has 1 (so first part of the or is false) and new_volume has 1 (so second part of the or is also false), we still have 2 rw attachments but this will fail the if. Why not just if count(old_volume) + count(new_volume) \u003e 1?","commit_id":"91bca49490aefba77c2cad872f75eb687d2c7d83"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"97c6ff7285723b0c718c2e04166a959f75344af7","unresolved":false,"context_lines":[{"line_number":4116,"context_line":"        # attachment. We know the old_volume has at least one attachment since"},{"line_number":4117,"context_line":"        # it\u0027s attached to this server."},{"line_number":4118,"context_line":"        if (self._count_attachments_for_swap(context, old_volume) \u003e 1 or"},{"line_number":4119,"context_line":"                self._count_attachments_for_swap(context, new_volume) \u003e 1):"},{"line_number":4120,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4121,"context_line":""},{"line_number":4122,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_3420c09b","line":4119,"range":{"start_line":4119,"start_character":70,"end_line":4119,"end_character":71},"in_reply_to":"5f7c97a3_f1749ad2","updated":"2018-06-06 21:50:11.000000000","message":"If the new volume has only read-only attachments, why would we care about swapping to it? But see above, I can probably drop the new volume check since you can\u0027t swap to a volume that is attached to anything.","commit_id":"91bca49490aefba77c2cad872f75eb687d2c7d83"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5a7d324d73971b7eb30749bd3ff9884eb86e68e4","unresolved":false,"context_lines":[{"line_number":4083,"context_line":"                try:"},{"line_number":4084,"context_line":"                    attachment_record \u003d self.volume_api.attachment_get("},{"line_number":4085,"context_line":"                        ctxt, attachment_id)"},{"line_number":4086,"context_line":"                    if attachment_record[\u0027attach_mode\u0027] \u003d\u003d \u0027rw\u0027:"},{"line_number":4087,"context_line":"                        count +\u003d 1"},{"line_number":4088,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":4089,"context_line":"                    # attachments are read/write by default so count it"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_a048065c","line":4086,"updated":"2018-06-07 02:32:34.000000000","message":"Hmm, apparently attach_mode isn\u0027t set?\n\nhttp://logs.openstack.org/90/572790/6/check/nova-multiattach/a1c6b26/logs/screen-n-api.txt.gz?level\u003dTRACE#_Jun_06_23_08_15_210710\n\nEven though the API says it should be:\n\nhttps://developer.openstack.org/api-ref/block-storage/v3/#show-attachment-details\n\nLooks like this was the raw response:\n\nhttp://logs.openstack.org/90/572790/6/check/nova-multiattach/a1c6b26/logs/screen-n-api.txt.gz#_Jun_06_23_08_15_201532\n\n{\n   \"attachment\":{\n      \"status\":\"attached\",\n      \"detached_at\":\"\",\n      \"connection_info\":{\n         \"auth_password\":\"jA5qkRgKkfEWXYSp\",\n         \"attachment_id\":\"18d5e902-f509-48ed-91fe-29cb4a893f7c\",\n         \"target_discovered\":false,\n         \"encrypted\":false,\n         \"driver_volume_type\":\"iscsi\",\n         \"qos_specs\":null,\n         \"target_iqn\":\"iqn.2010-10.org.openstack:volume-c430ddbc-aa1a-4e7d-8914-961e193bc42a\",\n         \"target_portal\":\"104.239.135.61:3260\",\n         \"volume_id\":\"c430ddbc-aa1a-4e7d-8914-961e193bc42a\",\n         \"target_lun\":1,\n         \"access_mode\":\"rw\",\n         \"auth_username\":\"Nt3XB76dNPTBrTHBaddF\",\n         \"auth_method\":\"CHAP\"\n      },\n      \"attached_at\":\"2018-06-06T23:08:14.000000\",\n      \"attach_mode\":\"ro\",\n      \"instance\":\"7e21dad7-3ca1-405b-b70b-1a2e1f80d83c\",\n      \"volume_id\":\"c430ddbc-aa1a-4e7d-8914-961e193bc42a\",\n      \"id\":\"18d5e902-f509-48ed-91fe-29cb4a893f7c\"\n   }\n}\n\nI guess this is getting translated and shoved into \u0027connection_info\u0027:\n\nhttps://github.com/openstack/nova/blob/master/nova/volume/cinder.py#L361","commit_id":"5adc7f3b34f1bea367e7b2e9cd19047ac5bfb12d"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f097ab23c3495fd78d04956dc17d3e49c6ca5ac5","unresolved":false,"context_lines":[{"line_number":4124,"context_line":"        # read/write attachment. We know the old_volume has at least one"},{"line_number":4125,"context_line":"        # attachment since it\u0027s attached to this server. The new_volume"},{"line_number":4126,"context_line":"        # can\u0027t have any attachments because of the attach_status check above."},{"line_number":4127,"context_line":"        if self._count_attachments_for_swap(context, old_volume) \u003e 1:"},{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_a32c8b01","line":4128,"range":{"start_line":4127,"start_character":0,"end_line":4128,"end_character":63},"updated":"2018-06-07 10:04:51.000000000","message":"This is obviously inherently racy. It\u0027s not just a regular race either, because this operation can take hours. If the volume is attached to another instance at any point during it, the result will probably be corruption.\n\nHow about...\n\nWe leave this here as a \u0027courtesy check\u0027, to make the api call fail early in the normal case. We then add the same check in compute manager\u0027s swap_volume, but wrap the whole thing in a cinder notification listener which will abort the operation if the volume is attached anywhere else before it completes.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"274012fe3b78fda952a59d8530c9261b5fb0c324","unresolved":false,"context_lines":[{"line_number":4124,"context_line":"        # read/write attachment. We know the old_volume has at least one"},{"line_number":4125,"context_line":"        # attachment since it\u0027s attached to this server. The new_volume"},{"line_number":4126,"context_line":"        # can\u0027t have any attachments because of the attach_status check above."},{"line_number":4127,"context_line":"        if self._count_attachments_for_swap(context, old_volume) \u003e 1:"},{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_ea3a4fc2","line":4128,"range":{"start_line":4127,"start_character":0,"end_line":4128,"end_character":63},"in_reply_to":"5f7c97a3_a32c8b01","updated":"2018-06-07 12:45:41.000000000","message":"\u003e but wrap the whole thing in a cinder notification listener which will abort the operation if the volume is attached anywhere else before it completes.\n\nThat\u0027s likely not something I\u0027m going to be writing up in a backportable change - nova doesn\u0027t listen for cinder notifications at all today, and would require operator intervention to even get that hooked up after the backport. We could just as easily use our cinder admin credentials, if configured in nova.conf, and lock the volume while it\u0027s being swapped (assuming cinder has some sort of lock API).","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8d328bc0763f68fb5d54e6c9b81db83dad7ec162","unresolved":false,"context_lines":[{"line_number":4124,"context_line":"        # read/write attachment. We know the old_volume has at least one"},{"line_number":4125,"context_line":"        # attachment since it\u0027s attached to this server. The new_volume"},{"line_number":4126,"context_line":"        # can\u0027t have any attachments because of the attach_status check above."},{"line_number":4127,"context_line":"        if self._count_attachments_for_swap(context, old_volume) \u003e 1:"},{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_2a24c7ae","line":4128,"range":{"start_line":4127,"start_character":0,"end_line":4128,"end_character":63},"in_reply_to":"5f7c97a3_a32c8b01","updated":"2018-06-07 12:43:29.000000000","message":"Again, I assume you\u0027re talking about a scenario where someone is calling the nova API directly without doing a volume retype or live migration, because if you were going through the Cinder API, the volume\u0027s status would be changed and you wouldn\u0027t/shouldn\u0027t be able to do things to that volume while it\u0027s undergoing the swap operation.\n\nSo if you\u0027re not going through cinder, and just nova directly, then you could do any number of dumb things to the volume since we don\u0027t have it locked, like detaching the volume while it\u0027s being swapped.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"02ffe84044939cb04f0e5e876a08fbe74a2066d1","unresolved":false,"context_lines":[{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4132,"context_line":"        except exception.InvalidInput as exc:"},{"line_number":4133,"context_line":"            raise exception.InvalidVolume(reason\u003dexc.format_message())"},{"line_number":4134,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_2aa7e70d","line":4131,"range":{"start_line":4131,"start_character":12,"end_line":4131,"end_character":70},"updated":"2018-06-07 12:47:28.000000000","message":"We change the old volume\u0027s attach status here, so I don\u0027t think once we hit this point that you can attach the volume to another server while we\u0027re swapping it.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4e1fbdca673ad3c9274b7996aa99075d224f0c5d","unresolved":false,"context_lines":[{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4132,"context_line":"        except exception.InvalidInput as exc:"},{"line_number":4133,"context_line":"            raise exception.InvalidVolume(reason\u003dexc.format_message())"},{"line_number":4134,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_aa4e9735","line":4131,"range":{"start_line":4131,"start_character":12,"end_line":4131,"end_character":70},"in_reply_to":"5f7c97a3_2aa7e70d","updated":"2018-06-07 12:50:17.000000000","message":"http://git.openstack.org/cgit/openstack/cinder/tree/cinder/volume/api.py#n723","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f576ce80304c63471799198ce8f9de17756e5d91","unresolved":false,"context_lines":[{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4132,"context_line":"        except exception.InvalidInput as exc:"},{"line_number":4133,"context_line":"            raise exception.InvalidVolume(reason\u003dexc.format_message())"},{"line_number":4134,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_752dc4fb","line":4131,"range":{"start_line":4131,"start_character":12,"end_line":4131,"end_character":70},"in_reply_to":"5f7c97a3_4ab7c3d9","updated":"2018-06-07 13:27:06.000000000","message":"Yes, I think this is the lock I was looking for. Nice! The user can still subvert it, but they have to do it explicitly, which is their own problem.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d43e09495fc54eececf420d859f453dbcb3b5e46","unresolved":false,"context_lines":[{"line_number":4128,"context_line":"            raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4129,"context_line":""},{"line_number":4130,"context_line":"        try:"},{"line_number":4131,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4132,"context_line":"        except exception.InvalidInput as exc:"},{"line_number":4133,"context_line":"            raise exception.InvalidVolume(reason\u003dexc.format_message())"},{"line_number":4134,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_4ab7c3d9","line":4131,"range":{"start_line":4131,"start_character":12,"end_line":4131,"end_character":70},"in_reply_to":"5f7c97a3_aa4e9735","updated":"2018-06-07 12:54:05.000000000","message":"Granted, to be safe, we should move the attachment count check to after this point when we\u0027ve got a lock on the volume. And we\u0027ll have to rollback the detaching status if counting attachments fails for some reason.","commit_id":"9342bd3040259ae95be038f480e978fd02c9961e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":4060,"context_line":""},{"line_number":4061,"context_line":"        Attempts to only count read/write attachments if the volume attachment"},{"line_number":4062,"context_line":"        records exist, otherwise simply just counts the number of attachments"},{"line_number":4063,"context_line":"        regardless of attach mode."},{"line_number":4064,"context_line":""},{"line_number":4065,"context_line":"        :param ctxt: nova.context.RequestContext - user request context"},{"line_number":4066,"context_line":"        :param volume: nova-translated volume dict from nova.volume.cinder."}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_4efca0b4","line":4063,"updated":"2018-06-08 11:10:18.000000000","message":"Suggestion: maybe point out that we have a \u0027lock\u0027 while we run this? The begin_detaching call isn\u0027t 100% obvious.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffcb66b636908b5c7bec7ac0b2f41a5b866951ef","unresolved":false,"context_lines":[{"line_number":4060,"context_line":""},{"line_number":4061,"context_line":"        Attempts to only count read/write attachments if the volume attachment"},{"line_number":4062,"context_line":"        records exist, otherwise simply just counts the number of attachments"},{"line_number":4063,"context_line":"        regardless of attach mode."},{"line_number":4064,"context_line":""},{"line_number":4065,"context_line":"        :param ctxt: nova.context.RequestContext - user request context"},{"line_number":4066,"context_line":"        :param volume: nova-translated volume dict from nova.volume.cinder."}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_f37ea268","line":4063,"in_reply_to":"5f7c97a3_4efca0b4","updated":"2018-06-09 14:00:02.000000000","message":"I don\u0027t think that\u0027s really appropriate from this method, which shouldn\u0027t assume what\u0027s going on when it\u0027s being called. If anything, it would just have a reminder in the docstring to say \u0027make sure the volume is locked so attachments can\u0027t change while counting\u0027.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":4088,"context_line":"                    # nova.volume.cinder code translates it and puts the"},{"line_number":4089,"context_line":"                    # attach_mode in the connection_info for some legacy"},{"line_number":4090,"context_line":"                    # reason..."},{"line_number":4091,"context_line":"                    if attachment_record.get("},{"line_number":4092,"context_line":"                            \u0027connection_info\u0027, {}).get("},{"line_number":4093,"context_line":"                                # attachments are read/write by default"},{"line_number":4094,"context_line":"                                \u0027attach_mode\u0027, \u0027rw\u0027) \u003d\u003d \u0027rw\u0027:"},{"line_number":4095,"context_line":"                        count +\u003d 1"},{"line_number":4096,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":4097,"context_line":"                    # attachments are read/write by default so count it"},{"line_number":4098,"context_line":"                    count +\u003d 1"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_acca1c9e","line":4095,"range":{"start_line":4091,"start_character":0,"end_line":4095,"end_character":34},"updated":"2018-06-08 11:10:18.000000000","message":"Eww. Is it worth avoiding looking under connection_info\u0027s skirt here by changing the pops in _translate_attachment_ref to gets, and using the cinder api as documented? It seems wrong that change I8156e4da broke the non-legacy use case.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"676e2c3f8fc0cc30e0c8f5d290a13f6e4fc331f6","unresolved":false,"context_lines":[{"line_number":4088,"context_line":"                    # nova.volume.cinder code translates it and puts the"},{"line_number":4089,"context_line":"                    # attach_mode in the connection_info for some legacy"},{"line_number":4090,"context_line":"                    # reason..."},{"line_number":4091,"context_line":"                    if attachment_record.get("},{"line_number":4092,"context_line":"                            \u0027connection_info\u0027, {}).get("},{"line_number":4093,"context_line":"                                # attachments are read/write by default"},{"line_number":4094,"context_line":"                                \u0027attach_mode\u0027, \u0027rw\u0027) \u003d\u003d \u0027rw\u0027:"},{"line_number":4095,"context_line":"                        count +\u003d 1"},{"line_number":4096,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":4097,"context_line":"                    # attachments are read/write by default so count it"},{"line_number":4098,"context_line":"                    count +\u003d 1"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_954cbe42","line":4095,"range":{"start_line":4091,"start_character":0,"end_line":4095,"end_character":34},"in_reply_to":"5f7c97a3_93972e91","updated":"2018-08-24 15:35:27.000000000","message":"Cleaned up in the follow up patch in the series:\n\nhttps://review.openstack.org/#/c/574413/1/nova/compute/api.py","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffcb66b636908b5c7bec7ac0b2f41a5b866951ef","unresolved":false,"context_lines":[{"line_number":4088,"context_line":"                    # nova.volume.cinder code translates it and puts the"},{"line_number":4089,"context_line":"                    # attach_mode in the connection_info for some legacy"},{"line_number":4090,"context_line":"                    # reason..."},{"line_number":4091,"context_line":"                    if attachment_record.get("},{"line_number":4092,"context_line":"                            \u0027connection_info\u0027, {}).get("},{"line_number":4093,"context_line":"                                # attachments are read/write by default"},{"line_number":4094,"context_line":"                                \u0027attach_mode\u0027, \u0027rw\u0027) \u003d\u003d \u0027rw\u0027:"},{"line_number":4095,"context_line":"                        count +\u003d 1"},{"line_number":4096,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":4097,"context_line":"                    # attachments are read/write by default so count it"},{"line_number":4098,"context_line":"                    count +\u003d 1"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_93972e91","line":4095,"range":{"start_line":4091,"start_character":0,"end_line":4095,"end_character":34},"in_reply_to":"5f7c97a3_acca1c9e","updated":"2018-06-09 14:00:02.000000000","message":"I don\u0027t know what fallout there will be from that. If I changed anything in _translate_attachment_ref specifically to this series it would just be keeping attach_mode as a top-level field on the attachment dict.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":4119,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":4120,"context_line":"        self.volume_api.check_availability_zone(context, new_volume,"},{"line_number":4121,"context_line":"                                                instance\u003dinstance)"},{"line_number":4122,"context_line":""},{"line_number":4123,"context_line":"        try:"},{"line_number":4124,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4125,"context_line":"        except exception.InvalidInput as exc:"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_fb045080","line":4122,"updated":"2018-06-08 11:10:18.000000000","message":"nit: whitespace","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b954d2899a7b827dda41f1755c57f36556973949","unresolved":false,"context_lines":[{"line_number":4125,"context_line":"        except exception.InvalidInput as exc:"},{"line_number":4126,"context_line":"            raise exception.InvalidVolume(reason\u003dexc.format_message())"},{"line_number":4127,"context_line":""},{"line_number":4128,"context_line":"        # Disallow swapping from multiattach volumes that have more than one"},{"line_number":4129,"context_line":"        # read/write attachment. We know the old_volume has at least one"},{"line_number":4130,"context_line":"        # attachment since it\u0027s attached to this server. The new_volume"},{"line_number":4131,"context_line":"        # can\u0027t have any attachments because of the attach_status check above."}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_1c8486d2","line":4128,"updated":"2018-06-11 00:52:32.000000000","message":"Could we move all this before the begin_detaching call on L4124, and thus dispense with the need for roll_detaching?","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"762758821d00203056011a31d5d93176ed421bd3","unresolved":false,"context_lines":[{"line_number":4125,"context_line":"        except exception.InvalidInput as exc:"},{"line_number":4126,"context_line":"            raise exception.InvalidVolume(reason\u003dexc.format_message())"},{"line_number":4127,"context_line":""},{"line_number":4128,"context_line":"        # Disallow swapping from multiattach volumes that have more than one"},{"line_number":4129,"context_line":"        # read/write attachment. We know the old_volume has at least one"},{"line_number":4130,"context_line":"        # attachment since it\u0027s attached to this server. The new_volume"},{"line_number":4131,"context_line":"        # can\u0027t have any attachments because of the attach_status check above."}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_9cc40062","line":4128,"in_reply_to":"5f7c97a3_1c8486d2","updated":"2018-06-11 19:01:29.000000000","message":"The entire point of moving it *after* begin _detaching is to change the status on the volume so it\u0027s essentially locked from adding new attachments while we\u0027re counting them.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":4137,"context_line":"        except Exception:  # This is generic to handle failures while counting"},{"line_number":4138,"context_line":"            # We need to reset the detaching status before raising."},{"line_number":4139,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4140,"context_line":"                self.volume_api.roll_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4141,"context_line":""},{"line_number":4142,"context_line":"        # Get the BDM for the attached (old) volume so we can tell if it was"},{"line_number":4143,"context_line":"        # attached with the new-style Cinder 3.44 API."}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_cc10b803","line":4140,"updated":"2018-06-08 11:10:18.000000000","message":"Not strictly a problem with this patch, but it looks like the scope of this try block should extend to immediately before swap_volume below.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":26458,"name":"Brin Zhang","email":"zhangbailin@inspur.com","username":"zhangbailin"},"change_message_id":"9f4f9f384442f15114b4d15c463a60c55c47adb2","unresolved":false,"context_lines":[{"line_number":4328,"context_line":"        try:"},{"line_number":4329,"context_line":"            if self._count_attachments_for_swap(context, old_volume) \u003e 1:"},{"line_number":4330,"context_line":"                raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4331,"context_line":"        except Exception:  # This is generic to handle failures while counting"},{"line_number":4332,"context_line":"            # We need to reset the detaching status before raising."},{"line_number":4333,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4334,"context_line":"                self.volume_api.roll_detaching(context, old_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_44918c4f","line":4331,"range":{"start_line":4331,"start_character":27,"end_line":4331,"end_character":78},"updated":"2018-11-09 01:58:43.000000000","message":"nit: This comment is for the except, the personal feeling is better on the except one line, and it feels awkward at the back.","commit_id":"b9ad51cadafc0009efe761298517f6562103cf47"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"85250c6bca7b0af9adbae406bf78d540daa486df","unresolved":false,"context_lines":[{"line_number":4328,"context_line":"        try:"},{"line_number":4329,"context_line":"            if self._count_attachments_for_swap(context, old_volume) \u003e 1:"},{"line_number":4330,"context_line":"                raise exception.MultiattachSwapVolumeNotSupported()"},{"line_number":4331,"context_line":"        except Exception:  # This is generic to handle failures while counting"},{"line_number":4332,"context_line":"            # We need to reset the detaching status before raising."},{"line_number":4333,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4334,"context_line":"                self.volume_api.roll_detaching(context, old_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_997f02bb","line":4331,"range":{"start_line":4331,"start_character":27,"end_line":4331,"end_character":78},"in_reply_to":"3f79a3b5_44918c4f","updated":"2018-11-09 14:21:13.000000000","message":"I\u0027m not sure I understand your comment. What is \"at the back\"?","commit_id":"b9ad51cadafc0009efe761298517f6562103cf47"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"c96f6b6701337a0affe5a6b1448f5f6e22b7258d","unresolved":false,"context_lines":[{"line_number":4446,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":4447,"context_line":"        self.volume_api.check_availability_zone(context, new_volume,"},{"line_number":4448,"context_line":"                                                instance\u003dinstance)"},{"line_number":4449,"context_line":""},{"line_number":4450,"context_line":"        try:"},{"line_number":4451,"context_line":"            self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":4452,"context_line":"        except exception.InvalidInput as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"5fc1f717_a9770dd4","line":4449,"updated":"2019-04-09 18:50:48.000000000","message":"nit - whitespace","commit_id":"0cc9772a51189196a72510817cac3784757c1911"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1cbcaa3b1a5f20652877e15641c2210b24724558","unresolved":false,"context_lines":[{"line_number":4395,"context_line":"        # Multiattach volumes can have more than one attachment, so if there"},{"line_number":4396,"context_line":"        # is more than one attachment, attempt to count the read/write"},{"line_number":4397,"context_line":"        # attachments."},{"line_number":4398,"context_line":"        if len(attachments) \u003e 1:"},{"line_number":4399,"context_line":"            count \u003d 0"},{"line_number":4400,"context_line":"            for attachment in attachments.values():"},{"line_number":4401,"context_line":"                attachment_id \u003d attachment[\u0027attachment_id\u0027]"}],"source_content_type":"text/x-python","patch_set":12,"id":"bfb3d3c7_5191ed98","line":4398,"range":{"start_line":4398,"start_character":0,"end_line":4398,"end_character":32},"updated":"2019-05-21 10:05:52.000000000","message":"nit:\n\n  if len(attachements) \u003c\u003d 1:\n      return len(attachments)\n\n  count \u003d 0\n  ...","commit_id":"d9367c1f6ad8f7cd17dcbd412f46846648df41db"}],"nova/exception.py":[{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"4d55802b3ebc33fc08724273b89c508c88d6188b","unresolved":false,"context_lines":[{"line_number":290,"context_line":""},{"line_number":291,"context_line":"class MultiattachSwapVolumeNotSupported(Invalid):"},{"line_number":292,"context_line":"    msg_fmt \u003d _(\u0027Swapping multi-attach volumes with more than one read/write \u0027"},{"line_number":293,"context_line":"                \u0027attachment is not supported.\u0027)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"class VolumeNotCreated(NovaException):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_06900395","line":293,"range":{"start_line":293,"start_character":31,"end_line":293,"end_character":44},"updated":"2018-06-06 14:40:27.000000000","message":"Instead of \"not supported\", might want to consider: \n\n    Swapping multi-attach volumes with more than one\n    read/write attachment can cause data corruption.\n\nOr something along those lines?  Because the core intention here seems to avoid the corruption of disks.","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd74e30a1d7ea0811aa9f49434ea6aac83cb7055","unresolved":false,"context_lines":[{"line_number":290,"context_line":""},{"line_number":291,"context_line":"class MultiattachSwapVolumeNotSupported(Invalid):"},{"line_number":292,"context_line":"    msg_fmt \u003d _(\u0027Swapping multi-attach volumes with more than one read/write \u0027"},{"line_number":293,"context_line":"                \u0027attachment is not supported.\u0027)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"class VolumeNotCreated(NovaException):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_e60627ce","line":293,"range":{"start_line":293,"start_character":31,"end_line":293,"end_character":44},"in_reply_to":"5f7c97a3_06900395","updated":"2018-06-06 15:17:38.000000000","message":"We\u0027re not apologising here, we\u0027re saying we\u0027re not going to do it. I think your message would be more appropriate to a warning if we were subsequently going to allow the operation to proceed. FWIW, I prefer \u0027not supported\u0027.","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"238493586cf3ba0f168da8416edc231d3d487c48","unresolved":false,"context_lines":[{"line_number":290,"context_line":""},{"line_number":291,"context_line":"class MultiattachSwapVolumeNotSupported(Invalid):"},{"line_number":292,"context_line":"    msg_fmt \u003d _(\u0027Swapping multi-attach volumes with more than one read/write \u0027"},{"line_number":293,"context_line":"                \u0027attachment is not supported.\u0027)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"class VolumeNotCreated(NovaException):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_8d82de51","line":293,"range":{"start_line":293,"start_character":31,"end_line":293,"end_character":44},"in_reply_to":"5f7c97a3_e60627ce","updated":"2018-06-07 07:51:19.000000000","message":"Yes, now that I re-read, \"not supported\" has a clearer \u0027imperative ring\u0027 to it, and better conveys the intention \"we are not going to do it\".\n\nMattR, please disregard my comment; it\u0027s good as-is.","commit_id":"50adeee68801a9b1f7b9bc0540ae43e9487ce9f5"}],"nova/tests/unit/api/openstack/compute/test_volumes.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9f96560227f513f7cbd76833c69c96fe97a8691e","unresolved":false,"context_lines":[{"line_number":977,"context_line":"    def test_swap_multiattach_multiple_readonly_attachments_fails("},{"line_number":978,"context_line":"            self, mock_get_instance):"},{"line_number":979,"context_line":"        \"\"\"Tests that trying to swap to/from a multiattach volume with"},{"line_number":980,"context_line":"        multiple read-only attachments will return an error."},{"line_number":981,"context_line":"        \"\"\""},{"line_number":982,"context_line":""},{"line_number":983,"context_line":"        def fake_volume_get(_context, volume_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_eb7b4726","line":980,"range":{"start_line":980,"start_character":22,"end_line":980,"end_character":26},"updated":"2018-06-06 19:56:51.000000000","message":"write","commit_id":"9788dc486b2cbd1e48a36530d02aa72e112b18ee"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":973,"context_line":""},{"line_number":974,"context_line":"class SwapVolumeMultiattachTestCase(test.NoDBTestCase):"},{"line_number":975,"context_line":""},{"line_number":976,"context_line":"    @mock.patch(\u0027nova.api.openstack.common.get_instance\u0027)"},{"line_number":977,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API.begin_detaching\u0027)"},{"line_number":978,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API.roll_detaching\u0027)"},{"line_number":979,"context_line":"    def test_swap_multiattach_multiple_readonly_attachments_fails("},{"line_number":980,"context_line":"            self, mock_roll_detaching, mock_begin_detaching,"},{"line_number":981,"context_line":"            mock_get_instance):"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_4cd4a8e9","line":978,"range":{"start_line":976,"start_character":0,"end_line":978,"end_character":56},"updated":"2018-06-08 11:10:18.000000000","message":"You put a TODO against CinderFixtureNewAttachFlow to refactor it and CinderFixture for better re-use. Time for that? Thinking mostly about centralising details of the volume and attachments dicts in tests.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffcb66b636908b5c7bec7ac0b2f41a5b866951ef","unresolved":false,"context_lines":[{"line_number":973,"context_line":""},{"line_number":974,"context_line":"class SwapVolumeMultiattachTestCase(test.NoDBTestCase):"},{"line_number":975,"context_line":""},{"line_number":976,"context_line":"    @mock.patch(\u0027nova.api.openstack.common.get_instance\u0027)"},{"line_number":977,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API.begin_detaching\u0027)"},{"line_number":978,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API.roll_detaching\u0027)"},{"line_number":979,"context_line":"    def test_swap_multiattach_multiple_readonly_attachments_fails("},{"line_number":980,"context_line":"            self, mock_roll_detaching, mock_begin_detaching,"},{"line_number":981,"context_line":"            mock_get_instance):"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_13d23e5d","line":978,"range":{"start_line":976,"start_character":0,"end_line":978,"end_character":56},"in_reply_to":"5f7c97a3_4cd4a8e9","updated":"2018-06-09 14:00:02.000000000","message":"I\u0027m not following - are you suggesting I change this test to use the CinderFixtureNewAttachFlow fixture? Those fixtures are mostly used in functional tests, but could be used in unit tests I guess, but I also need to mock the roll_detaching call so I can assert it\u0027s called.\n\nAlso, that\u0027s not really something that needs to be done to get this fix done is it? I mean, this is kind of how a single backportable fix turns into 10 patches and then dies.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":1009,"context_line":"            raise exception.VolumeNotFound(volume_id\u003dvolume_id)"},{"line_number":1010,"context_line":""},{"line_number":1011,"context_line":"        def fake_attachment_get(_context, attachment_id):"},{"line_number":1012,"context_line":"            return {\u0027connection_info\u0027: {\u0027attach_mode\u0027: \u0027rw\u0027}}"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":1015,"context_line":"        instance \u003d fake_instance.fake_instance_obj("}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_2c47ecbe","line":1012,"updated":"2018-06-08 11:10:18.000000000","message":"Hmm... we\u0027re duplicating remote cruftiness in a test.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffcb66b636908b5c7bec7ac0b2f41a5b866951ef","unresolved":false,"context_lines":[{"line_number":1009,"context_line":"            raise exception.VolumeNotFound(volume_id\u003dvolume_id)"},{"line_number":1010,"context_line":""},{"line_number":1011,"context_line":"        def fake_attachment_get(_context, attachment_id):"},{"line_number":1012,"context_line":"            return {\u0027connection_info\u0027: {\u0027attach_mode\u0027: \u0027rw\u0027}}"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":1015,"context_line":"        instance \u003d fake_instance.fake_instance_obj("}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_53bbd60d","line":1012,"in_reply_to":"5f7c97a3_2c47ecbe","updated":"2018-06-09 14:00:02.000000000","message":"? This is because the translate attachment ref thing.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"}],"nova/tests/unit/compute/test_compute_api.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":2764,"context_line":"            if attachment_id \u003d\u003d uuids.attachment1:"},{"line_number":2765,"context_line":"                raise exception.VolumeAttachmentNotFound("},{"line_number":2766,"context_line":"                    attachment_id\u003dattachment_id)"},{"line_number":2767,"context_line":"            return {\u0027connection_info\u0027: {\u0027attach_mode\u0027: \u0027ro\u0027}}"},{"line_number":2768,"context_line":""},{"line_number":2769,"context_line":"        with mock.patch.object(self.compute_api.volume_api, \u0027attachment_get\u0027,"},{"line_number":2770,"context_line":"                               side_effect\u003dfake_attachment_get) as mock_get:"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_c7f413f4","line":2767,"updated":"2018-06-08 11:10:18.000000000","message":"Wonky api assumptions again.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffcb66b636908b5c7bec7ac0b2f41a5b866951ef","unresolved":false,"context_lines":[{"line_number":2764,"context_line":"            if attachment_id \u003d\u003d uuids.attachment1:"},{"line_number":2765,"context_line":"                raise exception.VolumeAttachmentNotFound("},{"line_number":2766,"context_line":"                    attachment_id\u003dattachment_id)"},{"line_number":2767,"context_line":"            return {\u0027connection_info\u0027: {\u0027attach_mode\u0027: \u0027ro\u0027}}"},{"line_number":2768,"context_line":""},{"line_number":2769,"context_line":"        with mock.patch.object(self.compute_api.volume_api, \u0027attachment_get\u0027,"},{"line_number":2770,"context_line":"                               side_effect\u003dfake_attachment_get) as mock_get:"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_934ace24","line":2767,"in_reply_to":"5f7c97a3_c7f413f4","updated":"2018-06-09 14:00:02.000000000","message":"The API is behaving based on how the translate attachment ref stuff works - I don\u0027t think it would be appropriate for the unit tests to model a different behavior from how this code flow actually works. In fact, I changed this after realizing the code was broken because the unit tests were passing but tempest was failing.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cd37e73f323212a2163c516a217672bc035c6e3b","unresolved":false,"context_lines":[{"line_number":2769,"context_line":"        with mock.patch.object(self.compute_api.volume_api, \u0027attachment_get\u0027,"},{"line_number":2770,"context_line":"                               side_effect\u003dfake_attachment_get) as mock_get:"},{"line_number":2771,"context_line":"            self.assertEqual("},{"line_number":2772,"context_line":"                1, self.compute_api._count_attachments_for_swap(ctxt, volume))"},{"line_number":2773,"context_line":"        mock_get.assert_has_calls(["},{"line_number":2774,"context_line":"            mock.call(ctxt, uuids.attachment1),"},{"line_number":2775,"context_line":"            mock.call(ctxt, uuids.attachment2)], any_order\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_270147b0","line":2772,"updated":"2018-06-08 11:10:18.000000000","message":"Strictly speaking, this would also pass if we counted ro attachments but not missing ones, both of which would be wrong. Would be slightly better as 2 tests.","commit_id":"bcbbba2edba45e7f2a2802e17e0a2ea0e5a7dd72"}],"releasenotes/notes/bug-1775418-754fc50261f5d7c3.yaml":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cbce8b9802905d2851fe96c4afecab61c17d8d96","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    The `os-volume_attachments`_ update or commonly known as swap volume API"},{"line_number":5,"context_line":"    will now return `400` (BadRequest) error when attempting to swap from a"},{"line_number":6,"context_line":"    multi attached volume with more than one active read/write attachment"},{"line_number":7,"context_line":"    resolving `bug #1775418`_,"}],"source_content_type":"text/x-yaml","patch_set":13,"id":"bfb3d3c7_c8d51de7","line":4,"range":{"start_line":4,"start_character":4,"end_line":4,"end_character":76},"updated":"2019-05-22 09:01:56.000000000","message":"- The `os-volume_attachments`_ update API, commonly referred to as the swap volume API, ...\n\n- The `os-volume_attachments`_ updates, or \"swap volume\", API, ...","commit_id":"c611b43aebd82f79771bd4f64807369a77cf700c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cbce8b9802905d2851fe96c4afecab61c17d8d96","unresolved":false,"context_lines":[{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    The `os-volume_attachments`_ update or commonly known as swap volume API"},{"line_number":5,"context_line":"    will now return `400` (BadRequest) error when attempting to swap from a"},{"line_number":6,"context_line":"    multi attached volume with more than one active read/write attachment"},{"line_number":7,"context_line":"    resolving `bug #1775418`_,"},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":13,"id":"bfb3d3c7_68067189","line":5,"range":{"start_line":5,"start_character":20,"end_line":5,"end_character":25},"updated":"2019-05-22 09:01:56.000000000","message":"``400`` (otherwise it\u0027s italics instead of code)","commit_id":"c611b43aebd82f79771bd4f64807369a77cf700c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cbce8b9802905d2851fe96c4afecab61c17d8d96","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    The `os-volume_attachments`_ update or commonly known as swap volume API"},{"line_number":5,"context_line":"    will now return `400` (BadRequest) error when attempting to swap from a"},{"line_number":6,"context_line":"    multi attached volume with more than one active read/write attachment"},{"line_number":7,"context_line":"    resolving `bug #1775418`_,"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"    .. _os-volume_attachments: https://developer.openstack.org/api-ref/compute/?expanded\u003dupdate-a-volume-attachment-detail"},{"line_number":10,"context_line":"    .. _bug #1775418: https://launchpad.net/bugs/1775418"}],"source_content_type":"text/x-yaml","patch_set":13,"id":"bfb3d3c7_88096596","line":7,"range":{"start_line":7,"start_character":29,"end_line":7,"end_character":30},"updated":"2019-05-22 09:01:56.000000000","message":".","commit_id":"c611b43aebd82f79771bd4f64807369a77cf700c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d695fa40312b219e06290dea9260f30470dead8a","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    The `os-volume_attachments`_ update API, commonly reffered to as the swap"},{"line_number":5,"context_line":"    volume API will now return a ``400`` (BadRequest) error when attempting to"},{"line_number":6,"context_line":"    swap from a multi attached volume with more than one active read/write"},{"line_number":7,"context_line":"    attachment resolving `bug #1775418`_."}],"source_content_type":"text/x-yaml","patch_set":14,"id":"bfb3d3c7_28d159ce","line":4,"range":{"start_line":4,"start_character":54,"end_line":4,"end_character":62},"updated":"2019-05-22 09:08:48.000000000","message":"referred","commit_id":"3be1ff002fa7d8ef83f486582310c5e8fe65e794"}]}