)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"52c156ac47b4a2208c519778aaf2f774fa6d0191","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Revert attachment creation on failure"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"when creating a volume attachment with a already specified connector and"},{"line_number":10,"context_line":"the call to cinder-volume fails we leave a volume attachment in the"},{"line_number":11,"context_line":"state \"reserved\" behind and have the volume also in the state"},{"line_number":12,"context_line":"\"reserved\"."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1aa629a5_dbb2ddc1","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":4},"updated":"2021-06-10 10:32:46.000000000","message":"nit: When","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"52c156ac47b4a2208c519778aaf2f774fa6d0191","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Revert attachment creation on failure"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"when creating a volume attachment with a already specified connector and"},{"line_number":10,"context_line":"the call to cinder-volume fails we leave a volume attachment in the"},{"line_number":11,"context_line":"state \"reserved\" behind and have the volume also in the state"},{"line_number":12,"context_line":"\"reserved\"."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"6609b23b_98414752","line":9,"range":{"start_line":9,"start_character":39,"end_line":9,"end_character":40},"updated":"2021-06-10 10:32:46.000000000","message":"nit: an","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"52c156ac47b4a2208c519778aaf2f774fa6d0191","unresolved":true,"context_lines":[{"line_number":12,"context_line":"\"reserved\"."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"If same call succeeds afterwards the volume has two attachments. One in"},{"line_number":15,"context_line":"\"in-use\" and one in \"reserved\". If the volume is later detached the one in"},{"line_number":16,"context_line":"\"reserved\" will still exist and cause the volume to be stuck in this"},{"line_number":17,"context_line":"state."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a693ba5_063ee3b9","line":15,"range":{"start_line":15,"start_character":72,"end_line":15,"end_character":74},"updated":"2021-06-10 10:32:46.000000000","message":"nit: line too long, please move \"in\" to the next line","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"}],"cinder/volume/api.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"52c156ac47b4a2208c519778aaf2f774fa6d0191","unresolved":true,"context_lines":[{"line_number":2168,"context_line":"        if connector:"},{"line_number":2169,"context_line":"            try:"},{"line_number":2170,"context_line":"                connection_info \u003d ("},{"line_number":2171,"context_line":"                    self.volume_rpcapi.attachment_update("},{"line_number":2172,"context_line":"                        ctxt,"},{"line_number":2173,"context_line":"                        volume_ref,"},{"line_number":2174,"context_line":"                        connector,"},{"line_number":2175,"context_line":"                        attachment_ref.id))"},{"line_number":2176,"context_line":"            except Exception:"},{"line_number":2177,"context_line":"                # If the connection fails to update it would leave a stuck"},{"line_number":2178,"context_line":"                # \"reserved\" attachment behind. As the caller would retry this"}],"source_content_type":"text/x-python","patch_set":1,"id":"019f956a_dcb12f11","line":2175,"range":{"start_line":2171,"start_character":0,"end_line":2175,"end_character":43},"updated":"2021-06-10 10:32:46.000000000","message":"-1: Unnecessary change","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":29074,"name":"Felix Huettner","email":"felix.huettner@digits.schwarz","username":"felix.huettner"},"change_message_id":"28c1c20e69aac62726f3b528ebfc5e95cb3b9e54","unresolved":false,"context_lines":[{"line_number":2168,"context_line":"        if connector:"},{"line_number":2169,"context_line":"            try:"},{"line_number":2170,"context_line":"                connection_info \u003d ("},{"line_number":2171,"context_line":"                    self.volume_rpcapi.attachment_update("},{"line_number":2172,"context_line":"                        ctxt,"},{"line_number":2173,"context_line":"                        volume_ref,"},{"line_number":2174,"context_line":"                        connector,"},{"line_number":2175,"context_line":"                        attachment_ref.id))"},{"line_number":2176,"context_line":"            except Exception:"},{"line_number":2177,"context_line":"                # If the connection fails to update it would leave a stuck"},{"line_number":2178,"context_line":"                # \"reserved\" attachment behind. As the caller would retry this"}],"source_content_type":"text/x-python","patch_set":1,"id":"eb6091bb_1ade06f6","line":2175,"range":{"start_line":2171,"start_character":0,"end_line":2175,"end_character":43},"in_reply_to":"019f956a_dcb12f11","updated":"2021-06-29 09:18:29.000000000","message":"Done","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"52c156ac47b4a2208c519778aaf2f774fa6d0191","unresolved":true,"context_lines":[{"line_number":2177,"context_line":"                # If the connection fails to update it would leave a stuck"},{"line_number":2178,"context_line":"                # \"reserved\" attachment behind. As the caller would retry this"},{"line_number":2179,"context_line":"                # call on an error we would pile up \"reserved\" attachments"},{"line_number":2180,"context_line":"                # that are never removed. We therefor remove the failed"},{"line_number":2181,"context_line":"                # attachment completely here to avoid this situation."},{"line_number":2182,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2183,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"}],"source_content_type":"text/x-python","patch_set":1,"id":"1c950f77_f12b6390","line":2180,"range":{"start_line":2180,"start_character":18,"end_line":2180,"end_character":41},"updated":"2021-06-10 10:32:46.000000000","message":"Users can remove them manually, since the default policy for deleting an attachment is admin or owner.","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":29074,"name":"Felix Huettner","email":"felix.huettner@digits.schwarz","username":"felix.huettner"},"change_message_id":"28c1c20e69aac62726f3b528ebfc5e95cb3b9e54","unresolved":false,"context_lines":[{"line_number":2177,"context_line":"                # If the connection fails to update it would leave a stuck"},{"line_number":2178,"context_line":"                # \"reserved\" attachment behind. As the caller would retry this"},{"line_number":2179,"context_line":"                # call on an error we would pile up \"reserved\" attachments"},{"line_number":2180,"context_line":"                # that are never removed. We therefor remove the failed"},{"line_number":2181,"context_line":"                # attachment completely here to avoid this situation."},{"line_number":2182,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2183,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"}],"source_content_type":"text/x-python","patch_set":1,"id":"688261c8_532d81ff","line":2180,"range":{"start_line":2180,"start_character":18,"end_line":2180,"end_character":41},"in_reply_to":"1c950f77_f12b6390","updated":"2021-06-29 09:18:29.000000000","message":"Done","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"52c156ac47b4a2208c519778aaf2f774fa6d0191","unresolved":true,"context_lines":[{"line_number":2182,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2183,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"},{"line_number":2184,"context_line":"                                            volume_ref.id,"},{"line_number":2185,"context_line":"                                            attachment_ref.id)"},{"line_number":2186,"context_line":""},{"line_number":2187,"context_line":"        attachment_ref.connection_info \u003d connection_info"},{"line_number":2188,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"896862ee_c164a38e","line":2185,"updated":"2021-06-10 10:32:46.000000000","message":"-1: I believe it\u0027s not that simple, since we could get an RPC timeout error here, yet the volume service may still be processing the export/mapping of the volume, which means that removing the attachment here would make the call to `self.db.volume_attached` in volume manager\u0027s `attachment_update` method fail, leaving the volume still mapped in the backend, and some drivers then are unable to delete a volume that is mapped.\n\nIn my opinion we cannot just delete the attachment from the DB unless we also ensure that the volume is properly detached on the manager when the attachment is removed.\n\nThe current behavior is not great, but at least on an RPC timeout the volume would transition to in-use, and we could manually delete the attachment which would properly unmap the volume.\n\nI\u0027m ok to do the volume_detached call here on non RPC timeout cases or to leave this as it is and in the manager to the detach if the attachment has been removed.","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"8db011c40b3c6e87f8f34847dff85075c9ca2f69","unresolved":true,"context_lines":[{"line_number":2182,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2183,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"},{"line_number":2184,"context_line":"                                            volume_ref.id,"},{"line_number":2185,"context_line":"                                            attachment_ref.id)"},{"line_number":2186,"context_line":""},{"line_number":2187,"context_line":"        attachment_ref.connection_info \u003d connection_info"},{"line_number":2188,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ceb1e41c_01ecd8e8","line":2185,"in_reply_to":"896862ee_c164a38e","updated":"2021-06-28 10:51:51.000000000","message":"\u003e Felix Huettner said:\n\u003e We can actually be sure that the volume service will never process the attachment request. \n\u003e When setting the volume to detached the volume_attachment is marked as deleted. The volume \n\u003e manager is calling `objects.VolumeAttachment.get_by_id` at its start which whill raise a \n\u003e `VolumeAttachmentNotFound` as the attachment has been deleted.\n\n-1: That is not always the case, specifically in the case I described in my comment that will not be true.\n\nThis is the sequence of events that I was talking about:\n\n[cinder-api]\n- arrives to L2171 and makes the synchronous RPC call to the volume manager\u0027s `attachment_update` method and waits for a response.\n\n[cinder-volume]\n- Starts processing the request and arrives at the `self.driver.attach_volume` call, which is slow because the backend is saturated.\n\n[cinder-api]\n- the RPC call times out, because the call in the cinder-volume service is still ongoing.\n- remove the attachment DB record because there has been an exception to the volume service call\n- return an error to the REST API caller\n\n[cinder-volume]\n- the driver finishes the mapping operation\n- manager code makes the call to `self.db.volume_attached`\n- DB method fails because the attachment was deleted by the API\n- The volume is left mapped in the storage array since there is no call to cleanup the mapping in the manager\n\n\nIf user tries to delete the volume now, depending on the driver, the operation may fail because the volume is mapped, and there is no attachment record in Cinder\u0027s DB to request a cleanup or anything like that, so an admin will have to go into the storage array\u0027s console to clean things up.","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":29074,"name":"Felix Huettner","email":"felix.huettner@digits.schwarz","username":"felix.huettner"},"change_message_id":"28c1c20e69aac62726f3b528ebfc5e95cb3b9e54","unresolved":true,"context_lines":[{"line_number":2182,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2183,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"},{"line_number":2184,"context_line":"                                            volume_ref.id,"},{"line_number":2185,"context_line":"                                            attachment_ref.id)"},{"line_number":2186,"context_line":""},{"line_number":2187,"context_line":"        attachment_ref.connection_info \u003d connection_info"},{"line_number":2188,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ffb1b2b6_de978af8","line":2185,"in_reply_to":"ceb1e41c_01ecd8e8","updated":"2021-06-29 09:18:29.000000000","message":"For me it seem like that we unfortunately can not determine if cinder-volume has started working on the volume already. I was thinking about checking if the cinder-volume service has ack\u0027ed the rpc message but it seems like oslo_messaging is not exposing that information. I think this makes it impossible to actually rely on the outcome of `self.volume_rpcapi.attachment_update` to solve the bug.\n\nAs alternaitves we could prevent the duplication attachments from happening earlier in the code or clean them up when we detach the volume.\n\nHowever for detaching a volume nova just removes the individual volume attachment that is in use. As we can not differenciate between removing an individual attachment and detaching the whole volume we can not use this as a trigger to clean up the additional attachments.\n\nThe other option is to improve `_attachment_reserve` to handle an additional case. (https://review.opendev.org/c/openstack/cinder/+/793136/1/cinder/volume/api.py#2128)\nThe idea would be that if `attachment.instance_uuid \u003d\u003d instance_uuid` and the existing attachment is \u0027reserved\u0027 then we could just return this existing attachment. This would however require that `attachment_update` and `self.driver.attach_volume` are idempotent and i am unsure if this is the case.\nAlso this changes the API contract there can be no longer two VolumeAttachments in reserved state for the same instance. Here i am also not sure if this is intended.","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"b0ce1881a45e61b6cf74fefc831d0cfe1418ecc5","unresolved":true,"context_lines":[{"line_number":2182,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2183,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"},{"line_number":2184,"context_line":"                                            volume_ref.id,"},{"line_number":2185,"context_line":"                                            attachment_ref.id)"},{"line_number":2186,"context_line":""},{"line_number":2187,"context_line":"        attachment_ref.connection_info \u003d connection_info"},{"line_number":2188,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"28b16810_d517d123","line":2185,"in_reply_to":"ffb1b2b6_de978af8","updated":"2021-06-30 10:43:26.000000000","message":"If I remember correctly Nova doesn\u0027t use the attachment_create with a connector, it always creates an empty attachment first to reserve the volume and then calls the attachment_update method on line 2210 with the connector properties.  So Nova has the attachment id when the update fails and can remove the attachment in that case.  Or am I missing a case where Nova calls the create with the connector (migration or something like that)?\n\nI think the `_attachment_reserve` change you are proposing is a good path to explore, though the `attachment_update` is not idempotent, and the `self.driver.attach_volume` call is a callback to let the driver know that the volume attachment has completed, it doesn\u0027t actually do the mapping and exporting, so it\u0027s driver dependent (though not many drivers implement it).","commit_id":"804fb6c70441331c891f5213b6b5343e676e0aab"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"8db011c40b3c6e87f8f34847dff85075c9ca2f69","unresolved":true,"context_lines":[{"line_number":2172,"context_line":"                # If the connection fails to update it would leave a stuck"},{"line_number":2173,"context_line":"                # \"reserved\" attachment behind. As the caller would retry this"},{"line_number":2174,"context_line":"                # call on an error we would pile up \"reserved\" attachments"},{"line_number":2175,"context_line":"                # that require manual removal. We therefor remove the failed"},{"line_number":2176,"context_line":"                # attachment completely here to avoid this situation."},{"line_number":2177,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":2178,"context_line":"                    self.db.volume_detached(ctxt.elevated(),"}],"source_content_type":"text/x-python","patch_set":2,"id":"51a2f598_e68c8f6a","line":2175,"range":{"start_line":2175,"start_character":50,"end_line":2175,"end_character":58},"updated":"2021-06-28 10:51:51.000000000","message":"nit: therefore","commit_id":"f714a5a7655cb09ed3cf1772348c41b3c22d375e"}]}