)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"01a5df230cdfdcb6be72c112781a2d1318530fb7","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Reserving a device means that we have to create a BDM record on the"},{"line_number":10,"context_line":"compute node, as part of an RPC call. If we can\u0027t actually attach the"},{"line_number":11,"context_line":"volume - it\u0027s pointless to create a BDM record which we will just delete"},{"line_number":12,"context_line":"if the reservation in Cinder fails, but this also opens us up to leaking"},{"line_number":13,"context_line":"BDM records, as there is a window where the API worker dying or RPC"},{"line_number":14,"context_line":"timing out leaves us with a leaked BDM."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1ac06dbe_1499c71f","line":12,"range":{"start_line":11,"start_character":9,"end_line":12,"end_character":34},"updated":"2016-08-22 17:22:09.000000000","message":"Don\u0027t we want to add a test case for this? Looks like the existing one tests the behaviour of the context manager (i.e. that it calls unreserve() if attach() fails later), but not the fact that we don\u0027t actually create a BDM entry of reserve() fails","commit_id":"90961da2e08e8a89a31a3755bba395684f7b80b1"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"63f6aa59c2328de601acfda805801b7531197a5b","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Reserving a device means that we have to create a BDM record on the"},{"line_number":10,"context_line":"compute node, as part of an RPC call. If we can\u0027t actually attach the"},{"line_number":11,"context_line":"volume - it\u0027s pointless to create a BDM record which we will just delete"},{"line_number":12,"context_line":"if the reservation in Cinder fails, but this also opens us up to leaking"},{"line_number":13,"context_line":"BDM records, as there is a window where the API worker dying or RPC"},{"line_number":14,"context_line":"timing out leaves us with a leaked BDM."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1ac06dbe_715ebb13","line":12,"range":{"start_line":11,"start_character":9,"end_line":12,"end_character":34},"in_reply_to":"1ac06dbe_1499c71f","updated":"2016-08-24 15:27:34.000000000","message":"I don\u0027t see the simple way to test that BDM is actually created\nor at least some sqlalchemy API is called\nbecause in most cases it\u0027s done via RPC call to compute like:\ncompute_rpcapi.reserve_block_device_name.\nIndirectly it\u0027s tested in _ComputeAPIUnitTestMixIn.test_attach_volume\nin which reserve_block_device_name mock is called and fake BDM is returned.","commit_id":"90961da2e08e8a89a31a3755bba395684f7b80b1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0a7253530f5971ac2fd51e930dfd73f80b98781e","unresolved":false,"context_lines":[{"line_number":14,"context_line":"timing out leaves us with a leaked BDM."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I0a7eacd0751d604b932e65a2671e2b88010b3560"},{"line_number":17,"context_line":"Closes-bug: 1427060"},{"line_number":18,"context_line":"Co-Authored-By: Andrey Volkov \u003cavolkov@mirantis.com\u003e"},{"line_number":19,"context_line":"Co-Authored-By: Lee Yarwood \u003clyarwood@redhat.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"3a98d1f4_51f582ea","line":17,"updated":"2016-10-03 18:46:19.000000000","message":"Might want to also mark this as closes-bug 1629952 because of the shelved_offloaded stuff.","commit_id":"fe3a9409b79f5b96da4e07e4f46154169e2d217e"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"7da301211534869ee7f3fe60fa54d75deb3e3e4c","unresolved":false,"context_lines":[{"line_number":15,"context_line":"To correct this the bdm is now only created after the volume is fully"},{"line_number":16,"context_line":"reserved in Cinder. Failures to then create the bdm or to attach the"},{"line_number":17,"context_line":"volume now also result in the volume unreserved."},{"line_number":18,"context_line":"And remained bdm will be sweeped if the volume is re-attached to the"},{"line_number":19,"context_line":"same instance."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I0a7eacd0751d604b932e65a2671e2b88010b3560"},{"line_number":22,"context_line":"Closes-bug: 1427060"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"1f1a1f67_d2f2e59a","line":19,"range":{"start_line":18,"start_character":0,"end_line":19,"end_character":14},"updated":"2017-07-12 12:54:17.000000000","message":"nit - I would replace this with the following, without the newline :\n\nAny bdm records also created during the failure for the associated instance and volume will be removed during the next attempt to attach.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"788e74d548ce2ba5c0bfed138a703d880841393a","unresolved":false,"context_lines":[{"line_number":15,"context_line":"To correct this the bdm is now only created after the volume is fully"},{"line_number":16,"context_line":"reserved in Cinder. Failures to then create the bdm or to attach the"},{"line_number":17,"context_line":"volume now also result in the volume unreserved."},{"line_number":18,"context_line":"And remained bdm will be sweeped if the volume is re-attached to the"},{"line_number":19,"context_line":"same instance."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I0a7eacd0751d604b932e65a2671e2b88010b3560"},{"line_number":22,"context_line":"Closes-bug: 1427060"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"1f1a1f67_e57df9e7","line":19,"range":{"start_line":18,"start_character":0,"end_line":19,"end_character":14},"in_reply_to":"1f1a1f67_d2f2e59a","updated":"2017-07-13 09:55:25.000000000","message":"Thank you for suggesting!\nI\u0027ll fix it!","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5c1696f2a5ae78832473e6be09835b056184dc74","unresolved":false,"context_lines":[{"line_number":12,"context_line":"correctly cleaned up in the event of an API or RPC failure later in the"},{"line_number":13,"context_line":"attach process."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"To correct this, remained bdm will be sweeped if the volume is"},{"line_number":16,"context_line":"re-attached to the same instance."},{"line_number":17,"context_line":"Bdm record of a specific volume should be only one by instances even"},{"line_number":18,"context_line":"if volume multi-attach is used."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"This fix doesn\u0027t cause side effects to volume multi attach."},{"line_number":21,"context_line":"Because volume multi attach doesn\u0027t allow multi attaching one volume"},{"line_number":22,"context_line":"to one instance."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Change-Id: I0a7eacd0751d604b932e65a2671e2b88010b3560"},{"line_number":25,"context_line":"Closes-bug: 1427060"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":22,"id":"9f436f4f_56ed8289","line":22,"range":{"start_line":15,"start_character":0,"end_line":22,"end_character":16},"updated":"2017-08-10 02:23:55.000000000","message":"What\u0027s all the multi-attach talk about? Nova doesn\u0027t yet support volume multi-attach so that\u0027s just confusing here. I know a lot of people have forked Nova to support multi-attach out of tree, but that shouldn\u0027t confuse this.","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"}],"nova/compute/api.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"01a5403add4314a07997b227d04a47df52e2b956","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"9abb7d3a_d1120058","line":3074,"updated":"2016-06-02 16:33:59.000000000","message":"@Walter isn\u0027t that something for the cinder side to address by tracking the actual reservation requests instead of simply setting a volume to `attaching`?","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e293a178394fc9dbbf85e4c83c973b370c98aceb","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_7aacbced","line":3074,"updated":"2016-03-11 20:57:36.000000000","message":"OK, so by the time we get here, we\u0027ve checked that we can attach the volume (it\u0027s status in cinder is \u0027available\u0027 and we try to reserve it (put it\u0027s status in \u0027in-use\u0027). Then we yield to create the BDM in the nova database.\n\nIf something with that explodes, we at least unreserve the volume in cinder so it\u0027s back to being \u0027available\u0027.","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"79a331cf7e00bcdb0918e8800898dcd76d54b7b5","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_81a64351","line":3074,"in_reply_to":"3afc51ec_3a783fc4","updated":"2016-03-14 14:27:16.000000000","message":"And note that unreserve will move the volume back to \u0027available\u0027 if there are no attachments, else it puts it in \u0027in-use\u0027:\n\nhttps://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L584","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"8b01ae1bc55cc5c4d4eee594505fb22a47964875","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_3a783fc4","line":3074,"in_reply_to":"3afc51ec_60740e9c","updated":"2016-03-14 12:35:00.000000000","message":"\u003e And when we unreserve the volume, we check for state \"attaching\" and then put in state \"in use\".\n\nThis is not accurate. AFAIC this will be put to in_use only if we have succeeded in attaching the volume already (since Ciner looks for \"attach_status\" to be \"attaching\". This can never happen unless the volumes have been tampered with by hand, as all Nova calls will do reserve_volume first which fails it if it\u0027s been attached before.\n\n\u003e So we have a race. We check_attach, and everything is OK, volume is available (or in-use for multi-attach) If the state changes before we call reserve_volume, then reserve volume will fail.\n\nThis is exactly what we want to happen and why we call reserve_volume. Check_attach is wrong but it does check for AZs which don\u0027t change that often so it\u0027s still a useful check to have.\n\n\u003e But when we call unreserve_volume, that will also fail if the state is not \u0027attaching\u0027. this could happen if the call we\u0027ve competed with finishes (state is now in-use) or if a multi-attach detaching was or still is in progress (state is detaching or available).\n\nunreserve will not be called unless reserve succeeded (and something else failed) in which case the other attach call would have failed due to the volume being reserved already. Reserve volume is the closest we have to an atomic test-and-set and this is exactly the correct way to use it.","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":7173,"name":"Scott DAngelo","email":"scott.dangelo@gmail.com","username":"scottda"},"change_message_id":"a26eed8b35f651082cc2b688d0570c8e4369991c","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_60740e9c","line":3074,"in_reply_to":"3afc51ec_7aacbced","updated":"2016-03-11 21:16:57.000000000","message":"Actually, when we reserve the volume, we put it in \"attaching\".\nAnd when we unreserve the volume, we check for state \"attaching\" and then put in state \"in use\".\nSo we have a race. We check_attach, and everything is OK, volume is available (or in-use for multi-attach)\nIf the state changes before we call reserve_volume, then reserve volume will fail.\nBut when we call unreserve_volume, that will also fail if the state is not \u0027attaching\u0027. this could happen if the call we\u0027ve competed with finishes (state is now in-use) or if a multi-attach detaching was or still is in progress (state is detaching or available).\n\nSo L#3079 would fail in this case.","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":5997,"name":"Walt","display_name":"Hemna","email":"waboring@hemna.com","username":"walter-boring","status":"SAP"},"change_message_id":"26d942a3a0a34a0667402d0f923305525345680d","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"dab17558_0c30e69d","line":3074,"in_reply_to":"3afc51ec_81a64351","updated":"2016-05-12 19:56:40.000000000","message":"If the state of the volume changes between the volume get and reserve_volume, you don\u0027t want to call unreserve.  That is bad.  Someone else could be in the middle of attaching the volume already.","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"d191fcc9110711a0e1534c5d033bdc2e49954deb","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"7aa08908_cc79c731","line":3074,"in_reply_to":"9abb7d3a_6cbfbb60","updated":"2016-06-06 09:56:29.000000000","message":"@lee AFAIK the race condition is there since a long time, there is a nova bug https://bugs.launchpad.net/nova/+bug/1275144 and as you can see from the comments there is not a clear decision about if we want to serialize the attach/detach operation or if we want to find a different way to fix the issue. Unfortunately it seems that the discussion is a bit stale at the moment.","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":5997,"name":"Walt","display_name":"Hemna","email":"waboring@hemna.com","username":"walter-boring","status":"SAP"},"change_message_id":"eac81446f016e9204415fa62cc5626709d20c333","unresolved":false,"context_lines":[{"line_number":3071,"context_line":"        self.volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":3072,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3073,"context_line":"        try:"},{"line_number":3074,"context_line":"            yield"},{"line_number":3075,"context_line":"        except Exception:"},{"line_number":3076,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3077,"context_line":"                LOG.info(_LI(\"Unreserving a reserved volume %(vol)s, due to \""}],"source_content_type":"text/x-python","patch_set":3,"id":"9abb7d3a_6cbfbb60","line":3074,"in_reply_to":"9abb7d3a_d1120058","updated":"2016-06-03 15:34:19.000000000","message":"This is a race condition that has nothing to do with cinder tracking.   When you do a volume get, and return the value and then run check_attach, the volume state can change.  This patch can unreserve the volume when someone else can be in the middle of attaching it.  :(\n\nI\u0027m actively working on removing the call to check_attach entirely, due to this race condition.  Have a look at this:\nhttps://review.openstack.org/#/c/315789/2/nova/compute/api.py","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"01a5403add4314a07997b227d04a47df52e2b956","unresolved":false,"context_lines":[{"line_number":3089,"context_line":"                context, volume_id, instance):"},{"line_number":3090,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3091,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3092,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3093,"context_line":"        try:"},{"line_number":3094,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3095,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"9abb7d3a_b10f7c2d","line":3092,"updated":"2016-06-02 16:33:59.000000000","message":"@Matt maybe I\u0027m missing something here but we already have instance_uuid and volume_id columns to use for each bdm. Wouldn\u0027t it be enough to fetch all instance bdms, filter by volume_id and destroy these in the case where volume_bdm isn\u0027t set? Is it valid to have multiple undeleted bdms for an instance_uuid and volume_id combination?","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e293a178394fc9dbbf85e4c83c973b370c98aceb","unresolved":false,"context_lines":[{"line_number":3087,"context_line":"        \"\"\""},{"line_number":3088,"context_line":"        with self._check_attach_and_reserve_volume("},{"line_number":3089,"context_line":"                context, volume_id, instance):"},{"line_number":3090,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3091,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3092,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3093,"context_line":"        try:"},{"line_number":3094,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3095,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_ba8da47d","line":3092,"range":{"start_line":3090,"start_character":25,"end_line":3092,"end_character":40},"updated":"2016-03-11 20:57:36.000000000","message":"So this is going to cast off to the compute to reserve the device name, but if that fails, the exception handling in the context manager (_check_attach_and_reserve_volume) is only going to unreserve the volume. Since we\u0027re outside the try/except in this method, we don\u0027t delete the BDM, right? Isn\u0027t that what we were trying to fix?\n\nOh, well, the call to reserve the device name, which also creates the BDM:\n\nhttps://github.com/openstack/nova/blob/fb6f37f7f91c47aced8b19e527e2771743911d0a/nova/compute/manager.py#L4699\n\nIf that fails then we might not have a volume_bdm to delete. Although the compute could create the BDM in the database and the response from the rpc call could fail, right? In which case we\u0027re still leaking the BDM in the database.\n\nWouldn\u0027t it be best to move this into the try/except and if volume_bdm is None, we query the database for BDMs by volume_id and instance and destroy() if we find one?","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"37922482bc5d4b79cc027b6d2a20cbcaa2bea75b","unresolved":false,"context_lines":[{"line_number":3087,"context_line":"        \"\"\""},{"line_number":3088,"context_line":"        with self._check_attach_and_reserve_volume("},{"line_number":3089,"context_line":"                context, volume_id, instance):"},{"line_number":3090,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3091,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3092,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3093,"context_line":"        try:"},{"line_number":3094,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3095,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_c1e04bcf","line":3092,"range":{"start_line":3090,"start_character":25,"end_line":3092,"end_character":40},"in_reply_to":"3afc51ec_14ef2188","updated":"2016-03-14 14:21:36.000000000","message":"We could also move forward with Dan\u0027s series to add a uuid column to the BDM table:\n\nhttps://review.openstack.org/#/c/242602/","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"31035af96fb46bd91b9c4da388ec1f088e353c9d","unresolved":false,"context_lines":[{"line_number":3087,"context_line":"        \"\"\""},{"line_number":3088,"context_line":"        with self._check_attach_and_reserve_volume("},{"line_number":3089,"context_line":"                context, volume_id, instance):"},{"line_number":3090,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3091,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3092,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3093,"context_line":"        try:"},{"line_number":3094,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3095,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_676ea096","line":3092,"range":{"start_line":3090,"start_character":25,"end_line":3092,"end_character":40},"in_reply_to":"3afc51ec_585709ce","updated":"2016-03-14 16:45:27.000000000","message":"can we get more info back from cinder and filter by attaching status? If there are more than one but just one is in attaching status we know what to delete.\nIf even this additional info is not enough then we log the warning","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"8b01ae1bc55cc5c4d4eee594505fb22a47964875","unresolved":false,"context_lines":[{"line_number":3087,"context_line":"        \"\"\""},{"line_number":3088,"context_line":"        with self._check_attach_and_reserve_volume("},{"line_number":3089,"context_line":"                context, volume_id, instance):"},{"line_number":3090,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3091,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3092,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3093,"context_line":"        try:"},{"line_number":3094,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3095,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_14ef2188","line":3092,"range":{"start_line":3090,"start_character":25,"end_line":3092,"end_character":40},"in_reply_to":"3afc51ec_ba8da47d","updated":"2016-03-14 12:35:00.000000000","message":"So you make a very good point and get to a bigger issue. \n\nThere is no way to actually know which BDM to delete at that point if there is more than one other than use a leaky heuristic in the where clause (create time and maybe connection info all of which could in theory be inconclusive).\n\nThe way to solve this would be to have a unique constraint on (volume_id, instance_id, deleted), which should take care of this issue ever happening again, but that is not as straightforward as it seems, as we would have to actually do a data migration before we can add it.","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5e115eaee643928a250f24c228d5664ff0fe37f4","unresolved":false,"context_lines":[{"line_number":3087,"context_line":"        \"\"\""},{"line_number":3088,"context_line":"        with self._check_attach_and_reserve_volume("},{"line_number":3089,"context_line":"                context, volume_id, instance):"},{"line_number":3090,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3091,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3092,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3093,"context_line":"        try:"},{"line_number":3094,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3095,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3afc51ec_585709ce","line":3092,"range":{"start_line":3090,"start_character":25,"end_line":3092,"end_character":40},"in_reply_to":"3afc51ec_c1e04bcf","updated":"2016-03-14 15:08:17.000000000","message":"Per \"There is no way to actually know which BDM to delete at that point if there is more than one\" - so it\u0027s not great, but I think still think checking the bdm list in this case is at least worth a try for cleaning up, if you only get one back, we delete that one, if we get more than one, we log a warning and we have a TODO to make that better with some type of unique constraint in the bdm table (be that uuid or volume_id/instance_uuid columns).","commit_id":"51e064b3bdbd6cb00796f71418933d30ec6b6e02"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"01a5df230cdfdcb6be72c112781a2d1318530fb7","unresolved":false,"context_lines":[{"line_number":3152,"context_line":"        return volume_bdm"},{"line_number":3153,"context_line":""},{"line_number":3154,"context_line":"    @contextlib.contextmanager"},{"line_number":3155,"context_line":"    def _check_reserve_volume(self, context, volume_id, instance):"},{"line_number":3156,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3157,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3158,"context_line":"                                                instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_e34557b3","line":3155,"updated":"2016-08-22 17:22:09.000000000","message":"nit: not sure if we need to change the name here at all","commit_id":"90961da2e08e8a89a31a3755bba395684f7b80b1"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"63f6aa59c2328de601acfda805801b7531197a5b","unresolved":false,"context_lines":[{"line_number":3152,"context_line":"        return volume_bdm"},{"line_number":3153,"context_line":""},{"line_number":3154,"context_line":"    @contextlib.contextmanager"},{"line_number":3155,"context_line":"    def _check_reserve_volume(self, context, volume_id, instance):"},{"line_number":3156,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3157,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3158,"context_line":"                                                instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_71723b4b","line":3155,"in_reply_to":"1ac06dbe_e34557b3","updated":"2016-08-24 15:27:34.000000000","message":"The main concern here I think is that the function code also checks availability zone. That part should be removed after check availability goes to Cinder. I removed \"attach_and\" because the method doesn\u0027t volume state now.","commit_id":"90961da2e08e8a89a31a3755bba395684f7b80b1"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"a3110781ff0c1f1a2fd8d34f5130b36c3b8bdd96","unresolved":false,"context_lines":[{"line_number":3150,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3151,"context_line":"        return volume_bdm"},{"line_number":3152,"context_line":""},{"line_number":3153,"context_line":"    def _check_attach_and_reserve_volume(self, context, volume_id, instance):"},{"line_number":3154,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3155,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3156,"context_line":"                                                instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_d0be1aad","side":"PARENT","line":3153,"updated":"2016-09-02 15:06:53.000000000","message":"nit:do we really need to rename this?","commit_id":"9a1f48403defac2290570ca40fc20955ec081164"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"1cbf1a3a45f936b606e93501daa977a9f9147b51","unresolved":false,"context_lines":[{"line_number":3150,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3151,"context_line":"        return volume_bdm"},{"line_number":3152,"context_line":""},{"line_number":3153,"context_line":"    def _check_attach_and_reserve_volume(self, context, volume_id, instance):"},{"line_number":3154,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3155,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3156,"context_line":"                                                instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_c9519c64","side":"PARENT","line":3153,"in_reply_to":"fa7ab95a_d0be1aad","updated":"2016-09-05 11:06:29.000000000","message":"The original name was returned back.","commit_id":"9a1f48403defac2290570ca40fc20955ec081164"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"cc27b525660e578f82a02a3e790132ba64f631c7","unresolved":false,"context_lines":[{"line_number":3164,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3165,"context_line":"        try:"},{"line_number":3166,"context_line":"            yield"},{"line_number":3167,"context_line":"        except Exception:"},{"line_number":3168,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3169,"context_line":"                self._unreserve_volume(context, volume_id)"},{"line_number":3170,"context_line":""},{"line_number":3171,"context_line":"    def _attach_volume(self, context, instance, volume_id, device,"},{"line_number":3172,"context_line":"                       disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ac06dbe_cd126158","line":3169,"range":{"start_line":3167,"start_character":0,"end_line":3169,"end_character":58},"updated":"2016-08-24 20:15:33.000000000","message":"Do we still want to search for and remove the bdm *if* it was created by the compute but we failed during the reply? IIRC that was one of the issues Nikola was attempting to address here.","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"a3110781ff0c1f1a2fd8d34f5130b36c3b8bdd96","unresolved":false,"context_lines":[{"line_number":3164,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3165,"context_line":"        try:"},{"line_number":3166,"context_line":"            yield"},{"line_number":3167,"context_line":"        except Exception:"},{"line_number":3168,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3169,"context_line":"                self._unreserve_volume(context, volume_id)"},{"line_number":3170,"context_line":""},{"line_number":3171,"context_line":"    def _attach_volume(self, context, instance, volume_id, device,"},{"line_number":3172,"context_line":"                       disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_90a1f296","line":3169,"range":{"start_line":3167,"start_character":0,"end_line":3169,"end_character":58},"in_reply_to":"1ac06dbe_cd126158","updated":"2016-09-02 15:06:53.000000000","message":"I don\u0027t think we need to pursue that goal here. IMHO, this patch is ok as it is. Dealing of RPC replies failures would be nice, but we could see that as enhancement for the future (as it requires a DB schema migration).","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"a3110781ff0c1f1a2fd8d34f5130b36c3b8bdd96","unresolved":false,"context_lines":[{"line_number":3184,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3185,"context_line":"        except Exception:"},{"line_number":3186,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3187,"context_line":"                self._unreserve_volume(context, volume_id)"},{"line_number":3188,"context_line":"                volume_bdm.destroy()"},{"line_number":3189,"context_line":""},{"line_number":3190,"context_line":"        return volume_bdm.device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_30a2e6fc","line":3187,"updated":"2016-09-02 15:06:53.000000000","message":"nit: why not execute the try/except block in the with block above, so that we can rely on _unreserve_volume() be executed on line 3169 in case of errors?","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"1cbf1a3a45f936b606e93501daa977a9f9147b51","unresolved":false,"context_lines":[{"line_number":3184,"context_line":"            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)"},{"line_number":3185,"context_line":"        except Exception:"},{"line_number":3186,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3187,"context_line":"                self._unreserve_volume(context, volume_id)"},{"line_number":3188,"context_line":"                volume_bdm.destroy()"},{"line_number":3189,"context_line":""},{"line_number":3190,"context_line":"        return volume_bdm.device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_699fb003","line":3187,"in_reply_to":"fa7ab95a_30a2e6fc","updated":"2016-09-05 11:06:29.000000000","message":"Done","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"a3110781ff0c1f1a2fd8d34f5130b36c3b8bdd96","unresolved":false,"context_lines":[{"line_number":3214,"context_line":"                                   device)"},{"line_number":3215,"context_line":"        except Exception:"},{"line_number":3216,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3217,"context_line":"                self._unreserve_volume(context, volume_id)"},{"line_number":3218,"context_line":"                volume_bdm.destroy()"},{"line_number":3219,"context_line":""},{"line_number":3220,"context_line":"        return volume_bdm.device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_30ad8684","line":3217,"updated":"2016-09-02 15:06:53.000000000","message":"nit: ditto","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"1cbf1a3a45f936b606e93501daa977a9f9147b51","unresolved":false,"context_lines":[{"line_number":3214,"context_line":"                                   device)"},{"line_number":3215,"context_line":"        except Exception:"},{"line_number":3216,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3217,"context_line":"                self._unreserve_volume(context, volume_id)"},{"line_number":3218,"context_line":"                volume_bdm.destroy()"},{"line_number":3219,"context_line":""},{"line_number":3220,"context_line":"        return volume_bdm.device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_29bcc86b","line":3217,"in_reply_to":"fa7ab95a_30ad8684","updated":"2016-09-05 11:06:29.000000000","message":"Done","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"157a119ad94cd6c2fa1bd983642b2f9d07f08c33","unresolved":false,"context_lines":[{"line_number":3316,"context_line":"        \"\"\""},{"line_number":3317,"context_line":"        with self._check_attach_and_reserve_volume("},{"line_number":3318,"context_line":"                context, volume_id, instance):"},{"line_number":3319,"context_line":"            volume_bdm \u003d self._create_volume_bdm("},{"line_number":3320,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3321,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3322,"context_line":"            try:"},{"line_number":3323,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3324,"context_line":"                    context, instance, volume_bdm)"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a8ec9b2_cc3c5432","line":3321,"range":{"start_line":3319,"start_character":0,"end_line":3321,"end_character":40},"updated":"2016-09-14 11:19:15.000000000","message":"This should be in the try block below to catch RPC reply errors from the reserve_block_device_name call we can make to the compute.","commit_id":"8417f410a4b5abef44e0b9cdb9c17b93d66d9b3f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"97890861a1e9348de05dc5c543e3831be63418f2","unresolved":false,"context_lines":[{"line_number":3320,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3321,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3322,"context_line":"            try:"},{"line_number":3323,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3324,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3325,"context_line":"            except Exception:"},{"line_number":3326,"context_line":"                with excutils.save_and_reraise_exception():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a89bdaa_829a06c4","line":3323,"updated":"2016-09-09 16:17:32.000000000","message":"Attach is a cast, which means we\u0027ll never see any error bubble up from here.","commit_id":"8417f410a4b5abef44e0b9cdb9c17b93d66d9b3f"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"8fe9b5f686f54b67f48481798d8dca63f589227d","unresolved":false,"context_lines":[{"line_number":3320,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3321,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3322,"context_line":"            try:"},{"line_number":3323,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3324,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3325,"context_line":"            except Exception:"},{"line_number":3326,"context_line":"                with excutils.save_and_reraise_exception():"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a8ec9b2_a2af0fe2","line":3323,"in_reply_to":"9a89bdaa_829a06c4","updated":"2016-09-14 11:27:15.000000000","message":"Also, is there really zero chance of anything bubbling up from a cast call here, even when initially sending it?","commit_id":"8417f410a4b5abef44e0b9cdb9c17b93d66d9b3f"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"e98eea2631ee93ff8284fceee5d6d5bc5258bfed","unresolved":false,"context_lines":[{"line_number":3320,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3321,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3322,"context_line":"            try:"},{"line_number":3323,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3324,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3325,"context_line":"            except Exception:"},{"line_number":3326,"context_line":"                with excutils.save_and_reraise_exception():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a89bdaa_6dc0652b","line":3323,"in_reply_to":"9a89bdaa_829a06c4","updated":"2016-09-12 07:58:40.000000000","message":"Do you mean that we shouldn\u0027t destroy BDM in the case of network error?\nIs that reason for -2 or you have some other thoughts?","commit_id":"8417f410a4b5abef44e0b9cdb9c17b93d66d9b3f"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"157a119ad94cd6c2fa1bd983642b2f9d07f08c33","unresolved":false,"context_lines":[{"line_number":3320,"context_line":"                context, instance, device, volume_id, disk_bus\u003ddisk_bus,"},{"line_number":3321,"context_line":"                device_type\u003ddevice_type)"},{"line_number":3322,"context_line":"            try:"},{"line_number":3323,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3324,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3325,"context_line":"            except Exception:"},{"line_number":3326,"context_line":"                with excutils.save_and_reraise_exception():"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a8ec9b2_9f13505c","line":3323,"in_reply_to":"9a89bdaa_829a06c4","updated":"2016-09-14 11:19:15.000000000","message":"The RPC errors Nikola was talking about in the bug [1] are from _create_volume_bdm above and the RPC call to reserve_block_device_name that it makes. IMHO this needs to drop down into this try block so we can then clean up in the event of a reply error etc. \n\nThis should be easier to do now that we are reserving on the cinder side before looking to create the bdm. As a result any bdm for the instance_uuid and volume_id that we find should be fair game to be removed AFAICT.\n\n[1] https://bugs.launchpad.net/nova/+bug/1427060/comments/4","commit_id":"8417f410a4b5abef44e0b9cdb9c17b93d66d9b3f"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"7c8fe8e0e4b8c8c0b7df7fb4fa4c63c2ea41fc3e","unresolved":false,"context_lines":[{"line_number":3402,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3403,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3404,"context_line":"            except Exception:"},{"line_number":3405,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3406,"context_line":"                    # NOTE(lyarwood): Ensure that the volume_bdm has not been"},{"line_number":3407,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3408,"context_line":"                    if not volume_bdm:"},{"line_number":3409,"context_line":"                        try:"},{"line_number":3410,"context_line":"                            volume_bdm \u003d \\"},{"line_number":3411,"context_line":"                                objects.BlockDeviceMapping.get_by_volume_and_instance(  # noqa"},{"line_number":3412,"context_line":"                                            context, volume_id, instance.uuid)"},{"line_number":3413,"context_line":"                        except exception.VolumeBDMNotFound:"},{"line_number":3414,"context_line":"                            LOG.warning(_LW(\"Unable to find block device \""},{"line_number":3415,"context_line":"                                            \"mapping for removal.\"))"},{"line_number":3416,"context_line":"                    if volume_bdm:"},{"line_number":3417,"context_line":"                        volume_bdm.destroy()"},{"line_number":3418,"context_line":"        return volume_bdm.device_name"},{"line_number":3419,"context_line":""},{"line_number":3420,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_1395c231","line":3417,"range":{"start_line":3405,"start_character":0,"end_line":3417,"end_character":44},"updated":"2016-09-18 15:22:01.000000000","message":"Hey Andrey, sorry for pushing this over the weekend but I had a few minutes and wanted to get this moving again. If you agree with this approach for dealing with failures during bdm creation I\u0027ll add tests in the morning.","commit_id":"fe3a9409b79f5b96da4e07e4f46154169e2d217e"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"744fa818fa8617cad0b8884a38c525d7d81d419c","unresolved":false,"context_lines":[{"line_number":3402,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3403,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3404,"context_line":"            except Exception:"},{"line_number":3405,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3406,"context_line":"                    # NOTE(lyarwood): Ensure that the volume_bdm has not been"},{"line_number":3407,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3408,"context_line":"                    if not volume_bdm:"},{"line_number":3409,"context_line":"                        try:"},{"line_number":3410,"context_line":"                            volume_bdm \u003d \\"},{"line_number":3411,"context_line":"                                objects.BlockDeviceMapping.get_by_volume_and_instance(  # noqa"},{"line_number":3412,"context_line":"                                            context, volume_id, instance.uuid)"},{"line_number":3413,"context_line":"                        except exception.VolumeBDMNotFound:"},{"line_number":3414,"context_line":"                            LOG.warning(_LW(\"Unable to find block device \""},{"line_number":3415,"context_line":"                                            \"mapping for removal.\"))"},{"line_number":3416,"context_line":"                    if volume_bdm:"},{"line_number":3417,"context_line":"                        volume_bdm.destroy()"},{"line_number":3418,"context_line":"        return volume_bdm.device_name"},{"line_number":3419,"context_line":""},{"line_number":3420,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_56a64dae","line":3417,"range":{"start_line":3405,"start_character":0,"end_line":3417,"end_character":44},"in_reply_to":"7a8ec9b2_1395c231","updated":"2016-09-19 08:12:20.000000000","message":"Hi. After talking with you I thought about the same approach.\nOnly one thing I\u0027m concerned is multiple failed bdms (as there is no instance volume_id unique index yet) but maybe it\u0027s okay.\nPlease, go ahead.","commit_id":"fe3a9409b79f5b96da4e07e4f46154169e2d217e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3488a75c88ff0b0691bf53ac0cfe3e2a0ccb95a3","unresolved":false,"context_lines":[{"line_number":3402,"context_line":"                self.compute_rpcapi.attach_volume("},{"line_number":3403,"context_line":"                    context, instance, volume_bdm)"},{"line_number":3404,"context_line":"            except Exception:"},{"line_number":3405,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3406,"context_line":"                    # NOTE(lyarwood): Ensure that the volume_bdm has not been"},{"line_number":3407,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3408,"context_line":"                    if not volume_bdm:"},{"line_number":3409,"context_line":"                        try:"},{"line_number":3410,"context_line":"                            volume_bdm \u003d \\"},{"line_number":3411,"context_line":"                                objects.BlockDeviceMapping.get_by_volume_and_instance(  # noqa"},{"line_number":3412,"context_line":"                                            context, volume_id, instance.uuid)"},{"line_number":3413,"context_line":"                        except exception.VolumeBDMNotFound:"},{"line_number":3414,"context_line":"                            LOG.warning(_LW(\"Unable to find block device \""},{"line_number":3415,"context_line":"                                            \"mapping for removal.\"))"},{"line_number":3416,"context_line":"                    if volume_bdm:"},{"line_number":3417,"context_line":"                        volume_bdm.destroy()"},{"line_number":3418,"context_line":"        return volume_bdm.device_name"},{"line_number":3419,"context_line":""},{"line_number":3420,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"}],"source_content_type":"text/x-python","patch_set":11,"id":"3a98d1f4_b10246b2","line":3417,"range":{"start_line":3405,"start_character":0,"end_line":3417,"end_character":44},"in_reply_to":"7a8ec9b2_56a64dae","updated":"2016-10-03 18:49:46.000000000","message":"We\u0027ve talked about adding a uuid field to bdms for a long time now, and the os-diagnostics API redo is bringing it up again, so we might want to just add that if it\u0027s also useful here.","commit_id":"fe3a9409b79f5b96da4e07e4f46154169e2d217e"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"dc125479740e27b9c01071d91227944fac6fa6dd","unresolved":false,"context_lines":[{"line_number":3407,"context_line":"            except Exception:"},{"line_number":3408,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3409,"context_line":"                    # NOTE(lyarwood): Ensure that volume_bdm has not been"},{"line_number":3410,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3411,"context_line":"                    if not volume_bdm:"},{"line_number":3412,"context_line":"                        try:"},{"line_number":3413,"context_line":"                            volume_bdm \u003d \\"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_22186784","line":3410,"updated":"2016-10-11 17:23:17.000000000","message":"Agreed, I\u0027ll look into cleaning that up in a follow up change.","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"46f69e164fb21c93453be95eac74a7bd5488b6b7","unresolved":false,"context_lines":[{"line_number":3407,"context_line":"            except Exception:"},{"line_number":3408,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3409,"context_line":"                    # NOTE(lyarwood): Ensure that volume_bdm has not been"},{"line_number":3410,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3411,"context_line":"                    if not volume_bdm:"},{"line_number":3412,"context_line":"                        try:"},{"line_number":3413,"context_line":"                            volume_bdm \u003d \\"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_716dd303","line":3410,"updated":"2016-10-11 16:06:52.000000000","message":"I don\u0027t think this can ever happen. We create volume_bdm before we make the rpc call to the compute. The none-ness of this is determined before we ever ask the compute to do anything, so I don\u0027t see how it could ever need to be looked up again. I would expect we just do L3419-3420 and call it good, right?","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"26d45c1ee73bddc59f2687c52b0ba1de3258c25d","unresolved":false,"context_lines":[{"line_number":3407,"context_line":"            except Exception:"},{"line_number":3408,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3409,"context_line":"                    # NOTE(lyarwood): Ensure that volume_bdm has not been"},{"line_number":3410,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3411,"context_line":"                    if not volume_bdm:"},{"line_number":3412,"context_line":"                        try:"},{"line_number":3413,"context_line":"                            volume_bdm \u003d \\"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_ec9f485d","line":3410,"in_reply_to":"fa6399be_716dd303","updated":"2016-10-11 16:19:57.000000000","message":"Check L33480-L3373, _create_volume_bdm defaults to calling self.compute_rpcapi.reserve_block_device_name to create the bdm remotely. That\u0027s the RPC call and potential failure case that I wanted to address with this additional try/except. Hopefully I\u0027ve not got this completely wrong!","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"92624b27580e06a760d818b0102d03833318fb9d","unresolved":false,"context_lines":[{"line_number":3407,"context_line":"            except Exception:"},{"line_number":3408,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3409,"context_line":"                    # NOTE(lyarwood): Ensure that volume_bdm has not been"},{"line_number":3410,"context_line":"                    # created on the remote compute before the failure."},{"line_number":3411,"context_line":"                    if not volume_bdm:"},{"line_number":3412,"context_line":"                        try:"},{"line_number":3413,"context_line":"                            volume_bdm \u003d \\"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_875f8571","line":3410,"in_reply_to":"fa6399be_ec9f485d","updated":"2016-10-11 17:01:25.000000000","message":"Ah, I see, you\u0027re saying if we called _create_volume_bdm(), it actually created the BDM, but the reply didn\u0027t come back... I think this is the second time I\u0027ve been confused by _create_volume_bdm() being either local or remote for seemingly no reason.\n\nLooking at the history of when that was added, I think that was ill-considered. A method that calls one or the other single statements based on a defaulted local_or_not flag is just silly.","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"92624b27580e06a760d818b0102d03833318fb9d","unresolved":false,"context_lines":[{"line_number":3411,"context_line":"                    if not volume_bdm:"},{"line_number":3412,"context_line":"                        try:"},{"line_number":3413,"context_line":"                            volume_bdm \u003d \\"},{"line_number":3414,"context_line":"                                objects.BlockDeviceMapping.get_by_volume_and_instance(  # noqa"},{"line_number":3415,"context_line":"                                            context, volume_id, instance.uuid)"},{"line_number":3416,"context_line":"                        except exception.VolumeBDMNotFound:"},{"line_number":3417,"context_line":"                            LOG.warning(_LW(\"Unable to find block device \""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_a75e4943","line":3414,"range":{"start_line":3414,"start_character":88,"end_line":3414,"end_character":94},"updated":"2016-10-11 17:01:25.000000000","message":"Don\u0027t do this. The fact that you think you need to means that the nesting here is too deep and should be broken up. Obviously this is a very complex path (hence all the confusion) so the indent level is helping indicate that :)\n\nI would put basically all of this in a helper method that cleans up after this mess and just call it from the top-level except clause.","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"dc125479740e27b9c01071d91227944fac6fa6dd","unresolved":false,"context_lines":[{"line_number":3411,"context_line":"                    if not volume_bdm:"},{"line_number":3412,"context_line":"                        try:"},{"line_number":3413,"context_line":"                            volume_bdm \u003d \\"},{"line_number":3414,"context_line":"                                objects.BlockDeviceMapping.get_by_volume_and_instance(  # noqa"},{"line_number":3415,"context_line":"                                            context, volume_id, instance.uuid)"},{"line_number":3416,"context_line":"                        except exception.VolumeBDMNotFound:"},{"line_number":3417,"context_line":"                            LOG.warning(_LW(\"Unable to find block device \""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_421d5b72","line":3414,"updated":"2016-10-11 17:23:17.000000000","message":"Yeah sorry about that, this was just a placeholder while I looked into what additional logic we would need to add here. For example cases where we have already leaked BDMs etc (and thus need your bdm.uuid change). I\u0027ll throw this into a helper method in the next PS.","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"92624b27580e06a760d818b0102d03833318fb9d","unresolved":false,"context_lines":[{"line_number":3444,"context_line":"                                       device)"},{"line_number":3445,"context_line":"            except Exception:"},{"line_number":3446,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3447,"context_line":"                    volume_bdm.destroy()"},{"line_number":3448,"context_line":""},{"line_number":3449,"context_line":"        return volume_bdm.device_name"},{"line_number":3450,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_62a5ff02","line":3447,"updated":"2016-10-11 17:01:25.000000000","message":"Am I missing where this is covered?","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"dc125479740e27b9c01071d91227944fac6fa6dd","unresolved":false,"context_lines":[{"line_number":3444,"context_line":"                                       device)"},{"line_number":3445,"context_line":"            except Exception:"},{"line_number":3446,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3447,"context_line":"                    volume_bdm.destroy()"},{"line_number":3448,"context_line":""},{"line_number":3449,"context_line":"        return volume_bdm.device_name"},{"line_number":3450,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_62125f64","line":3447,"updated":"2016-10-11 17:23:17.000000000","message":"This isn\u0027t a remote creation call so I\u0027m not sure if we need to rollback in the same way here.","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"8161d43991b024c571bca5948ebd948cc4c20400","unresolved":false,"context_lines":[{"line_number":3444,"context_line":"                                       device)"},{"line_number":3445,"context_line":"            except Exception:"},{"line_number":3446,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3447,"context_line":"                    volume_bdm.destroy()"},{"line_number":3448,"context_line":""},{"line_number":3449,"context_line":"        return volume_bdm.device_name"},{"line_number":3450,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9a629dbe_3cbda768","line":3447,"in_reply_to":"fa6399be_245cf44a","updated":"2016-11-09 12:00:56.000000000","message":"test_attach_volume_shelved_unreserve_all_if_attach_fails","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"35a6976bf6c2094cd1d8d5b45a5e8568876d9b96","unresolved":false,"context_lines":[{"line_number":3444,"context_line":"                                       device)"},{"line_number":3445,"context_line":"            except Exception:"},{"line_number":3446,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3447,"context_line":"                    volume_bdm.destroy()"},{"line_number":3448,"context_line":""},{"line_number":3449,"context_line":"        return volume_bdm.device_name"},{"line_number":3450,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"fa6399be_245cf44a","line":3447,"in_reply_to":"fa6399be_62125f64","updated":"2016-10-11 17:47:43.000000000","message":"I mean in the tests.","commit_id":"d60d27fdcebeaa449b44f2e027169936ee804d16"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"bacbbec2ebc70bf623b552514d64ad1829405290","unresolved":false,"context_lines":[{"line_number":3403,"context_line":"        return volume_bdm"},{"line_number":3404,"context_line":""},{"line_number":3405,"context_line":"    def _cleanup_volume_bdm(self, context, instance, volume_id, bdm):"},{"line_number":3406,"context_line":"        if not bdm:"},{"line_number":3407,"context_line":"            try:"},{"line_number":3408,"context_line":"                bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":3409,"context_line":"                        context, volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a629dbe_39a214af","line":3406,"range":{"start_line":3406,"start_character":8,"end_line":3406,"end_character":19},"updated":"2016-11-09 13:24:13.000000000","message":"if bdm is None","commit_id":"d0a908003d69ca81e750e89d053da958c5be6f67"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"bacbbec2ebc70bf623b552514d64ad1829405290","unresolved":false,"context_lines":[{"line_number":3405,"context_line":"    def _cleanup_volume_bdm(self, context, instance, volume_id, bdm):"},{"line_number":3406,"context_line":"        if not bdm:"},{"line_number":3407,"context_line":"            try:"},{"line_number":3408,"context_line":"                bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":3409,"context_line":"                        context, volume_id, instance.uuid)"},{"line_number":3410,"context_line":"            except exception.VolumeBDMNotFound:"},{"line_number":3411,"context_line":"                LOG.warning(_LW(\"Unable to find block device mapping for \""},{"line_number":3412,"context_line":"                                \"removal.\"))"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a629dbe_39b774d9","line":3409,"range":{"start_line":3408,"start_character":0,"end_line":3409,"end_character":58},"updated":"2016-11-09 13:24:13.000000000","message":"An earlier PS did reference the use of bdm.uuid here to make this safer, I really can\u0027t recall why so happy to rethink this if required.","commit_id":"d0a908003d69ca81e750e89d053da958c5be6f67"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"6b6704f9ddd6b4cf4d50469e6dc92ceaba6f9ed4","unresolved":false,"context_lines":[{"line_number":3405,"context_line":"    def _cleanup_volume_bdm(self, context, instance, volume_id, bdm):"},{"line_number":3406,"context_line":"        if bdm is None:"},{"line_number":3407,"context_line":"            try:"},{"line_number":3408,"context_line":"                bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":3409,"context_line":"                        context, volume_id, instance.uuid)"},{"line_number":3410,"context_line":"            except exception.VolumeBDMNotFound:"},{"line_number":3411,"context_line":"                LOG.warning(_LW(\"Unable to find block device mapping for \""},{"line_number":3412,"context_line":"                                \"removal.\"))"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a629dbe_b6a2a697","line":3409,"range":{"start_line":3408,"start_character":0,"end_line":3409,"end_character":58},"updated":"2016-11-09 17:12:00.000000000","message":"The main outstanding question that I have is around the need for bdm.uuid here to somehow handle the case where we end up with multiple bdms in the DB. Is that still required?","commit_id":"e784edc56fca5396d67b399cb60c9862474dcf65"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1fa8a8cfbb937dcfd9f2e85a138c228b73d1e40","unresolved":false,"context_lines":[{"line_number":3405,"context_line":"    def _cleanup_volume_bdm(self, context, instance, volume_id, bdm):"},{"line_number":3406,"context_line":"        if bdm is None:"},{"line_number":3407,"context_line":"            try:"},{"line_number":3408,"context_line":"                bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":3409,"context_line":"                        context, volume_id, instance.uuid)"},{"line_number":3410,"context_line":"            except exception.VolumeBDMNotFound:"},{"line_number":3411,"context_line":"                LOG.warning(_LW(\"Unable to find block device mapping for \""},{"line_number":3412,"context_line":"                                \"removal.\"))"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a629dbe_c1959687","line":3409,"range":{"start_line":3408,"start_character":0,"end_line":3409,"end_character":58},"in_reply_to":"9a629dbe_b6a2a697","updated":"2016-11-09 17:26:20.000000000","message":"If you want to update that series, it\u0027s restored here:\n\nhttps://review.openstack.org/#/c/242603/","commit_id":"e784edc56fca5396d67b399cb60c9862474dcf65"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"dc45282f898d72535efd1f2c412030a8fcc72891","unresolved":false,"context_lines":[{"line_number":3437,"context_line":"        if bdm is None:"},{"line_number":3438,"context_line":"            try:"},{"line_number":3439,"context_line":"                bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("},{"line_number":3440,"context_line":"                        context, volume_id, instance.uuid)"},{"line_number":3441,"context_line":"            except exception.VolumeBDMNotFound:"},{"line_number":3442,"context_line":"                LOG.warning(_LW(\"Unable to find block device mapping for \""},{"line_number":3443,"context_line":"                                \"removal.\"))"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_388a28c0","line":3440,"updated":"2016-11-16 13:23:45.000000000","message":"I forgot to note that we\u0027ve seen failure cases where we end up with multiple bdm records using the same instance_uuid and volume_id in the past. To make these go away I assume we would need to make volume_id and instance_uuid a unique constraint in the db or should we just fail if we find \u003e1 and ask the operator to manually clean up?","commit_id":"99bbc178aa295930838518991706db8b5d4b2b48"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"f5c3c49ef71bd59cdbe9f1c77d20af5bcbebff9e","unresolved":false,"context_lines":[{"line_number":3776,"context_line":"        This method is separated to make it possible for cells version"},{"line_number":3777,"context_line":"        to override it."},{"line_number":3778,"context_line":"        \"\"\""},{"line_number":3779,"context_line":"        self._check_attach_and_reserve_volume(context, volume_id, instance)"},{"line_number":3780,"context_line":"        volume_bdm \u003d None"},{"line_number":3781,"context_line":"        with self._unreserve_attaching_failed_volume(context, volume_id):"},{"line_number":3782,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_e55f580c","line":3779,"range":{"start_line":3779,"start_character":13,"end_line":3779,"end_character":45},"updated":"2017-07-12 15:17:34.000000000","message":"We are implementing a new attach/detach flow using the new Cinder API calls introduced in microversion 3.27. In that flow _check_attach_and_reserve_volume needs the BDM to store the attachment_id received from Cinder. See in line #3754 and #3794 here: https://review.openstack.org/#/c/330285/91/nova/compute/api.py","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"bd69bf6a7c9df9a847d2a5d8f153e15b4662b4e0","unresolved":false,"context_lines":[{"line_number":3776,"context_line":"        This method is separated to make it possible for cells version"},{"line_number":3777,"context_line":"        to override it."},{"line_number":3778,"context_line":"        \"\"\""},{"line_number":3779,"context_line":"        self._check_attach_and_reserve_volume(context, volume_id, instance)"},{"line_number":3780,"context_line":"        volume_bdm \u003d None"},{"line_number":3781,"context_line":"        with self._unreserve_attaching_failed_volume(context, volume_id):"},{"line_number":3782,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_2c83d9ce","line":3779,"range":{"start_line":3779,"start_character":13,"end_line":3779,"end_character":45},"in_reply_to":"1f1a1f67_e50979dd","updated":"2017-07-13 10:45:03.000000000","message":"Yes, I wanted to point out the conflict as the direction we are going towards is enabling the new Cinder attach/detach API.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"788e74d548ce2ba5c0bfed138a703d880841393a","unresolved":false,"context_lines":[{"line_number":3776,"context_line":"        This method is separated to make it possible for cells version"},{"line_number":3777,"context_line":"        to override it."},{"line_number":3778,"context_line":"        \"\"\""},{"line_number":3779,"context_line":"        self._check_attach_and_reserve_volume(context, volume_id, instance)"},{"line_number":3780,"context_line":"        volume_bdm \u003d None"},{"line_number":3781,"context_line":"        with self._unreserve_attaching_failed_volume(context, volume_id):"},{"line_number":3782,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_e50979dd","line":3779,"range":{"start_line":3779,"start_character":13,"end_line":3779,"end_character":45},"in_reply_to":"1f1a1f67_e55f580c","updated":"2017-07-13 09:55:25.000000000","message":"Thank you for suggesting.\nYou want to say that this patch and that patch are conflicted, don\u0027t you?\nThis will be a difficult problem...","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"f5c3c49ef71bd59cdbe9f1c77d20af5bcbebff9e","unresolved":false,"context_lines":[{"line_number":3788,"context_line":"                return volume_bdm.device_name"},{"line_number":3789,"context_line":"            except Exception:"},{"line_number":3790,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3791,"context_line":"                    if volume_bdm:"},{"line_number":3792,"context_line":"                        volume_bdm.destroy()"},{"line_number":3793,"context_line":""},{"line_number":3794,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"},{"line_number":3795,"context_line":"                                         device, disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_859e6405","line":3792,"range":{"start_line":3791,"start_character":20,"end_line":3792,"end_character":44},"updated":"2017-07-12 15:17:34.000000000","message":"The bug report talks about an extra BDM record, which I guess is only removed by the sweep in the DB layer, which only works until multi-attach is not introduced. So I wonder what triggered all this change that\u0027s in this file?","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"788e74d548ce2ba5c0bfed138a703d880841393a","unresolved":false,"context_lines":[{"line_number":3788,"context_line":"                return volume_bdm.device_name"},{"line_number":3789,"context_line":"            except Exception:"},{"line_number":3790,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3791,"context_line":"                    if volume_bdm:"},{"line_number":3792,"context_line":"                        volume_bdm.destroy()"},{"line_number":3793,"context_line":""},{"line_number":3794,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"},{"line_number":3795,"context_line":"                                         device, disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_e5bcd9bc","line":3792,"range":{"start_line":3791,"start_character":20,"end_line":3792,"end_character":44},"in_reply_to":"1f1a1f67_859e6405","updated":"2017-07-13 09:55:25.000000000","message":"I think that changing points of this file don\u0027t cause side effects for multi-attach.\nBecause these will sweep only block-device-mapping created in current API process.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"90c5ec6b2d8253724056e818fd113fd03dbdf72f","unresolved":false,"context_lines":[{"line_number":3788,"context_line":"                return volume_bdm.device_name"},{"line_number":3789,"context_line":"            except Exception:"},{"line_number":3790,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3791,"context_line":"                    if volume_bdm:"},{"line_number":3792,"context_line":"                        volume_bdm.destroy()"},{"line_number":3793,"context_line":""},{"line_number":3794,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"},{"line_number":3795,"context_line":"                                         device, disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_ccf6a590","line":3792,"range":{"start_line":3791,"start_character":20,"end_line":3792,"end_character":44},"in_reply_to":"1f1a1f67_859e6405","updated":"2017-07-13 10:40:12.000000000","message":"I\u0027m pretty sure the DB changes will work with multi-attached volumes, unless I\u0027m forgetting something we are not allowing multiple attachments of the same volume to the same instance, thus creating multiple BDMs with the same instance and volume UUIDs.\n\nThe changes in this file were to ensure BDMs are removed if we encountered a failure reserving the volume in Cinder, given the new flow I assume that\u0027s being taken care of elsewhere now?\n\nIMHO we should revert the changes to this file from this change and just focus on the DB cleanup assuming everyone is happy that it will still work with multi-attach volumes and doesn\u0027t impact any other BDM creation flows.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"c8c965eb102223b04b004257e38a356ccbcb404b","unresolved":false,"context_lines":[{"line_number":3788,"context_line":"                return volume_bdm.device_name"},{"line_number":3789,"context_line":"            except Exception:"},{"line_number":3790,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3791,"context_line":"                    if volume_bdm:"},{"line_number":3792,"context_line":"                        volume_bdm.destroy()"},{"line_number":3793,"context_line":""},{"line_number":3794,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"},{"line_number":3795,"context_line":"                                         device, disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_8e5ac627","line":3792,"range":{"start_line":3791,"start_character":20,"end_line":3792,"end_character":44},"in_reply_to":"1f1a1f67_ac4809cc","updated":"2017-07-14 02:30:35.000000000","message":"Hi Lee, Ildiko,\n\nThank you for pointing out and suggesting.\n\nI rethought about the necessity of changing this file.\nAs a result, I think that it is unnecessary to solve this issue.\n\nI will make a new patch according to above idea, and I will test it.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"bd69bf6a7c9df9a847d2a5d8f153e15b4662b4e0","unresolved":false,"context_lines":[{"line_number":3788,"context_line":"                return volume_bdm.device_name"},{"line_number":3789,"context_line":"            except Exception:"},{"line_number":3790,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":3791,"context_line":"                    if volume_bdm:"},{"line_number":3792,"context_line":"                        volume_bdm.destroy()"},{"line_number":3793,"context_line":""},{"line_number":3794,"context_line":"    def _attach_volume_shelved_offloaded(self, context, instance, volume_id,"},{"line_number":3795,"context_line":"                                         device, disk_bus, device_type):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_ac4809cc","line":3792,"range":{"start_line":3791,"start_character":20,"end_line":3792,"end_character":44},"in_reply_to":"1f1a1f67_e5bcd9bc","updated":"2017-07-13 10:45:03.000000000","message":"My question here wasn\u0027t related to multi-attach. I wonder how the change that you are suggesting here addresses the problem in the bug report exactly, which is about duplicated BDM\u0027s?\n\nAs if the solution is the sweep itself, then we don\u0027t need all these changes here.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"f5c3c49ef71bd59cdbe9f1c77d20af5bcbebff9e","unresolved":false,"context_lines":[{"line_number":3804,"context_line":"        instance will be unshelved."},{"line_number":3805,"context_line":"        \"\"\""},{"line_number":3806,"context_line":""},{"line_number":3807,"context_line":"        self._check_attach_and_reserve_volume(context, volume_id, instance)"},{"line_number":3808,"context_line":"        volume_bdm \u003d None"},{"line_number":3809,"context_line":"        with self._unreserve_attaching_failed_volume(context, volume_id):"},{"line_number":3810,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_85cb04a4","line":3807,"range":{"start_line":3807,"start_character":13,"end_line":3807,"end_character":45},"updated":"2017-07-12 15:17:34.000000000","message":"Same as above.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"}],"nova/db/sqlalchemy/api.py":[{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"f5c3c49ef71bd59cdbe9f1c77d20af5bcbebff9e","unresolved":false,"context_lines":[{"line_number":4148,"context_line":"    values \u003d _from_legacy_values(values, legacy)"},{"line_number":4149,"context_line":"    convert_objects_related_datetimes(values)"},{"line_number":4150,"context_line":""},{"line_number":4151,"context_line":"    # Sweep unnecessary records. Volumes are\u0027t multi-attched to same instance."},{"line_number":4152,"context_line":"    if values.get(\u0027volume_id\u0027):"},{"line_number":4153,"context_line":"        query \u003d _block_device_mapping_get_query(context).\\"},{"line_number":4154,"context_line":"                filter_by(instance_uuid\u003dvalues[\u0027instance_uuid\u0027]).\\"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_652c6847","line":4151,"range":{"start_line":4151,"start_character":33,"end_line":4151,"end_character":78},"updated":"2017-07-12 15:17:34.000000000","message":"I would say not yet, but it is work in progress. Does it happen and cause problems often?","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"788e74d548ce2ba5c0bfed138a703d880841393a","unresolved":false,"context_lines":[{"line_number":4148,"context_line":"    values \u003d _from_legacy_values(values, legacy)"},{"line_number":4149,"context_line":"    convert_objects_related_datetimes(values)"},{"line_number":4150,"context_line":""},{"line_number":4151,"context_line":"    # Sweep unnecessary records. Volumes are\u0027t multi-attched to same instance."},{"line_number":4152,"context_line":"    if values.get(\u0027volume_id\u0027):"},{"line_number":4153,"context_line":"        query \u003d _block_device_mapping_get_query(context).\\"},{"line_number":4154,"context_line":"                filter_by(instance_uuid\u003dvalues[\u0027instance_uuid\u0027]).\\"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_85276513","line":4151,"range":{"start_line":4151,"start_character":33,"end_line":4151,"end_character":78},"in_reply_to":"1f1a1f67_652c6847","updated":"2017-07-13 09:55:25.000000000","message":"Corrected record will be also deleted if multi-attach allows to attach a volume as different devices of one instance.\nThis is too bad.\n\nBut, in my understanding, multi-attach doesn\u0027t propose it.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"c8c965eb102223b04b004257e38a356ccbcb404b","unresolved":false,"context_lines":[{"line_number":4148,"context_line":"    values \u003d _from_legacy_values(values, legacy)"},{"line_number":4149,"context_line":"    convert_objects_related_datetimes(values)"},{"line_number":4150,"context_line":""},{"line_number":4151,"context_line":"    # Sweep unnecessary records. Volumes are\u0027t multi-attched to same instance."},{"line_number":4152,"context_line":"    if values.get(\u0027volume_id\u0027):"},{"line_number":4153,"context_line":"        query \u003d _block_device_mapping_get_query(context).\\"},{"line_number":4154,"context_line":"                filter_by(instance_uuid\u003dvalues[\u0027instance_uuid\u0027]).\\"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_f362433e","line":4151,"range":{"start_line":4151,"start_character":33,"end_line":4151,"end_character":78},"in_reply_to":"1f1a1f67_6cc79114","updated":"2017-07-14 02:30:35.000000000","message":"OK, I improve this comment.\nCurrent comment is hard to understand as you say.\nSorry.","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"bd69bf6a7c9df9a847d2a5d8f153e15b4662b4e0","unresolved":false,"context_lines":[{"line_number":4148,"context_line":"    values \u003d _from_legacy_values(values, legacy)"},{"line_number":4149,"context_line":"    convert_objects_related_datetimes(values)"},{"line_number":4150,"context_line":""},{"line_number":4151,"context_line":"    # Sweep unnecessary records. Volumes are\u0027t multi-attched to same instance."},{"line_number":4152,"context_line":"    if values.get(\u0027volume_id\u0027):"},{"line_number":4153,"context_line":"        query \u003d _block_device_mapping_get_query(context).\\"},{"line_number":4154,"context_line":"                filter_by(instance_uuid\u003dvalues[\u0027instance_uuid\u0027]).\\"}],"source_content_type":"text/x-python","patch_set":20,"id":"1f1a1f67_6cc79114","line":4151,"range":{"start_line":4151,"start_character":33,"end_line":4151,"end_character":78},"in_reply_to":"1f1a1f67_85276513","updated":"2017-07-13 10:45:03.000000000","message":"We only have this case for live migrate, where the volume technically gets multi-attached to the same instance, but we do not create a new BDM in that case, so it should be fine.\n\nI just confused myself with the multi-attach note here without processing the whole sentence...","commit_id":"a40aa68cb479aadd8a5890575a2315071df84934"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1b89bcb4225ec44ffc72c4df5c13c6630887e836","unresolved":false,"context_lines":[{"line_number":4153,"context_line":"    values \u003d _from_legacy_values(values, legacy)"},{"line_number":4154,"context_line":"    convert_objects_related_datetimes(values)"},{"line_number":4155,"context_line":""},{"line_number":4156,"context_line":"    # Sweep duplicated records."},{"line_number":4157,"context_line":"    # Bdm record of a specific volume should be only one by instances"},{"line_number":4158,"context_line":"    # even if volume multi-attach is used."},{"line_number":4159,"context_line":"    if values.get(\u0027volume_id\u0027):"},{"line_number":4160,"context_line":"        query \u003d _block_device_mapping_get_query(context).\\"},{"line_number":4161,"context_line":"                filter_by(instance_uuid\u003dvalues[\u0027instance_uuid\u0027]).\\"},{"line_number":4162,"context_line":"                filter_by(volume_id\u003dvalues[\u0027volume_id\u0027])"},{"line_number":4163,"context_line":"        query.soft_delete()"},{"line_number":4164,"context_line":""},{"line_number":4165,"context_line":"    bdm_ref \u003d models.BlockDeviceMapping()"},{"line_number":4166,"context_line":"    bdm_ref.update(values)"}],"source_content_type":"text/x-python","patch_set":23,"id":"5f4e5783_56a8eb1d","line":4163,"range":{"start_line":4156,"start_character":0,"end_line":4163,"end_character":27},"updated":"2017-10-11 12:52:52.000000000","message":"This could probably be dragged into its own function, a la \u0027convert_objects_related_datetimes\u0027","commit_id":"1ed082bc2f5823a9862958d177f3aca8be1407a9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ddad5ce9fb794512766bbf11b43473aaa66187a4","unresolved":false,"context_lines":[{"line_number":4193,"context_line":"        query \u003d _block_device_mapping_get_query(context).\\"},{"line_number":4194,"context_line":"                filter_by(instance_uuid\u003dvalues[\u0027instance_uuid\u0027]).\\"},{"line_number":4195,"context_line":"                filter_by(volume_id\u003dvalues[\u0027volume_id\u0027])"},{"line_number":4196,"context_line":"        query.soft_delete()"},{"line_number":4197,"context_line":""},{"line_number":4198,"context_line":"    # NOTE(xqueralt): Only update a BDM when device_name was provided. We"},{"line_number":4199,"context_line":"    # allow empty device names so they will be set later by the manager."}],"source_content_type":"text/x-python","patch_set":23,"id":"5f4e5783_ef6ff6c2","line":4196,"updated":"2017-10-11 14:01:35.000000000","message":"At first glance this is wrong, because if values contains volume_id then we\u0027re always going to delete the previous BDM rather than update it. However, this only has a single caller: BlockDeviceMapping.update_or_create(). This has populated values with self.obj_get_changes(). I can\u0027t think of any reason that an existing BDM object would have its volume_id changed, so it\u0027s probably ok.\n\nThis chain of reasoning is pretty long, though, with plenty of scope for current or future error. Can\u0027t say I\u0027m a fan.","commit_id":"1ed082bc2f5823a9862958d177f3aca8be1407a9"}],"nova/tests/unit/compute/test_compute_api.py":[{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"a3110781ff0c1f1a2fd8d34f5130b36c3b8bdd96","unresolved":false,"context_lines":[{"line_number":412,"context_line":"            self.assertEqual(0, mock_reserve.call_count)"},{"line_number":413,"context_line":"            self.assertEqual(0, mock_attach.call_count)"},{"line_number":414,"context_line":""},{"line_number":415,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027reserve_block_device_name\u0027)"},{"line_number":416,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027attach_volume\u0027)"},{"line_number":417,"context_line":"    def test_attach_volume_reserve_device_fails(self,"},{"line_number":418,"context_line":"                                                mock_attach, mock_reserve):"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_91c81457","line":415,"updated":"2016-09-02 15:06:53.000000000","message":"Didn\u0027t we want to specifically test the case when reserve() fails on Cinder side, not on Nova side (i.e. we would need to mock volume_api.reserve_volume())?\n\nI think an additional test case would be beneficial.","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"1cbf1a3a45f936b606e93501daa977a9f9147b51","unresolved":false,"context_lines":[{"line_number":412,"context_line":"            self.assertEqual(0, mock_reserve.call_count)"},{"line_number":413,"context_line":"            self.assertEqual(0, mock_attach.call_count)"},{"line_number":414,"context_line":""},{"line_number":415,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027reserve_block_device_name\u0027)"},{"line_number":416,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027attach_volume\u0027)"},{"line_number":417,"context_line":"    def test_attach_volume_reserve_device_fails(self,"},{"line_number":418,"context_line":"                                                mock_attach, mock_reserve):"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_ad046315","line":415,"in_reply_to":"fa7ab95a_91c81457","updated":"2016-09-05 11:06:29.000000000","message":"I\u0027ve added test_attach_volume_reserve_device_fails_cinder_side\nto check cinder client methods were called.","commit_id":"5aa3257e97e798024298cf48ea6988193fed17d3"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"157a119ad94cd6c2fa1bd983642b2f9d07f08c33","unresolved":false,"context_lines":[{"line_number":435,"context_line":"        client.volumes.get.called_once_with(volume[\u0027id\u0027])"},{"line_number":436,"context_line":"        client.volumes.unreserve.called_once_with(volume[\u0027id\u0027])"},{"line_number":437,"context_line":""},{"line_number":438,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027reserve_block_device_name\u0027)"},{"line_number":439,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027attach_volume\u0027)"},{"line_number":440,"context_line":"    def test_attach_volume_reserve_device_fails(self,"},{"line_number":441,"context_line":"                                                mock_attach, mock_reserve):"},{"line_number":442,"context_line":"        \"\"\"Cinder volume should be unreserved if bdm is not created.\"\"\""},{"line_number":443,"context_line":"        instance \u003d self._create_instance_obj()"},{"line_number":444,"context_line":"        volume \u003d fake_volume.fake_volume(1, \u0027test-vol\u0027, \u0027test-vol\u0027,"},{"line_number":445,"context_line":"                                         None, None, None, None, None)"},{"line_number":446,"context_line":"        mock_reserve.side_effect \u003d test.TestingException()"},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"        mock_volume_api \u003d mock.patch.object(self.compute_api, \u0027volume_api\u0027,"},{"line_number":449,"context_line":"                                            mock.MagicMock(spec\u003dcinder.API))"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        with mock_volume_api as mock_v_api:"},{"line_number":452,"context_line":"            mock_v_api.get.return_value \u003d volume"},{"line_number":453,"context_line":"            self.assertRaises(test.TestingException,"},{"line_number":454,"context_line":"                              self.compute_api.attach_volume,"},{"line_number":455,"context_line":"                              self.context, instance, volume[\u0027id\u0027])"},{"line_number":456,"context_line":"            mock_v_api.check_availability_zone.assert_called_once_with("},{"line_number":457,"context_line":"                self.context,"},{"line_number":458,"context_line":"                volume,"},{"line_number":459,"context_line":"                instance\u003dinstance)"},{"line_number":460,"context_line":"            mock_v_api.reserve_volume.assert_called_once_with(self.context,"},{"line_number":461,"context_line":"                                                              volume[\u0027id\u0027])"},{"line_number":462,"context_line":"            mock_v_api.unreserve_volume.assert_called_once_with(self.context,"},{"line_number":463,"context_line":"                                                                volume[\u0027id\u0027])"},{"line_number":464,"context_line":"            self.assertEqual(0, mock_attach.call_count)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027reserve_block_device_name\u0027)"},{"line_number":467,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027attach_volume\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a8ec9b2_7f73e4b1","line":464,"range":{"start_line":438,"start_character":0,"end_line":464,"end_character":55},"updated":"2016-09-14 11:19:15.000000000","message":"This test should be extended to ensure that any bdm created by the compute is also cleaned up.","commit_id":"8417f410a4b5abef44e0b9cdb9c17b93d66d9b3f"}],"nova/tests/unit/db/test_db_api.py":[{"author":{"_account_id":24711,"name":"Jackie Truong","email":"jacklyn.truong@jhuapl.edu","username":"jackie-truong"},"change_message_id":"4bff798c086a5b3d88d71ef9803e5c57f05e98bb","unresolved":false,"context_lines":[{"line_number":6409,"context_line":"        uuid \u003d values[\u0027instance_uuid\u0027]"},{"line_number":6410,"context_line":"        bdm_real \u003d db.block_device_mapping_get_all_by_instance(self.ctxt, uuid)"},{"line_number":6411,"context_line":"        self.assertEqual(len(bdm_real), 1)"},{"line_number":6412,"context_line":"        self.assertEqual(bdm_real[0][\u0027device_name\u0027], \u0027fake_name\u0027)"},{"line_number":6413,"context_line":""},{"line_number":6414,"context_line":"    def test_block_device_mapping_update(self):"},{"line_number":6415,"context_line":"        bdm \u003d self._create_bdm({})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_1e78ec8d","line":6412,"updated":"2017-08-12 00:07:58.000000000","message":"Wouldn\u0027t it make more sense to check the volume_id instead of the device_name? Since that\u0027s what we\u0027re using to determine duplicate records","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"456e8677ae7024ce51117917bbf01100e16962e5","unresolved":false,"context_lines":[{"line_number":6409,"context_line":"        uuid \u003d values[\u0027instance_uuid\u0027]"},{"line_number":6410,"context_line":"        bdm_real \u003d db.block_device_mapping_get_all_by_instance(self.ctxt, uuid)"},{"line_number":6411,"context_line":"        self.assertEqual(len(bdm_real), 1)"},{"line_number":6412,"context_line":"        self.assertEqual(bdm_real[0][\u0027device_name\u0027], \u0027fake_name\u0027)"},{"line_number":6413,"context_line":""},{"line_number":6414,"context_line":"    def test_block_device_mapping_update(self):"},{"line_number":6415,"context_line":"        bdm \u003d self._create_bdm({})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_fddc00e5","line":6412,"in_reply_to":"9f436f4f_1e78ec8d","updated":"2017-08-14 05:41:39.000000000","message":"Thanks!\nI think that your comment is right.\nI fix this assertion.","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"69b1a151718e6aabc1f4b5e03504c975398981ef","unresolved":false,"context_lines":[{"line_number":6409,"context_line":"        uuid \u003d values[\u0027instance_uuid\u0027]"},{"line_number":6410,"context_line":"        bdm_real \u003d db.block_device_mapping_get_all_by_instance(self.ctxt, uuid)"},{"line_number":6411,"context_line":"        self.assertEqual(len(bdm_real), 1)"},{"line_number":6412,"context_line":"        self.assertEqual(bdm_real[0][\u0027device_name\u0027], \u0027fake_name\u0027)"},{"line_number":6413,"context_line":""},{"line_number":6414,"context_line":"    def test_block_device_mapping_update(self):"},{"line_number":6415,"context_line":"        bdm \u003d self._create_bdm({})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_31b375eb","line":6412,"in_reply_to":"9f436f4f_fddc00e5","updated":"2017-08-17 07:50:36.000000000","message":"Done","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"},{"author":{"_account_id":24711,"name":"Jackie Truong","email":"jacklyn.truong@jhuapl.edu","username":"jackie-truong"},"change_message_id":"4bff798c086a5b3d88d71ef9803e5c57f05e98bb","unresolved":false,"context_lines":[{"line_number":6628,"context_line":"        self.assertEqual(len(bdms), 1)"},{"line_number":6629,"context_line":"        self.assertEqual(bdms[0][\u0027device_name\u0027], \u0027/dev/vda\u0027)"},{"line_number":6630,"context_line":""},{"line_number":6631,"context_line":"    def test_block_device_mapping_get_all_by_volume_id(self):"},{"line_number":6632,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id1\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6633,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id2\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6634,"context_line":"        bdms \u003d db.block_device_mapping_get_all_by_volume_id(self.ctxt,"},{"line_number":6635,"context_line":"                                                            \u0027fake_id\u0027)"},{"line_number":6636,"context_line":"        self.assertEqual(bdms[0][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6637,"context_line":"        self.assertEqual(bdms[1][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6638,"context_line":"        self.assertEqual(2, len(bdms))"},{"line_number":6639,"context_line":""},{"line_number":6640,"context_line":"    def test_block_device_mapping_get_all_by_volume_id_join_instance(self):"},{"line_number":6641,"context_line":"        self._create_bdm({\u0027volume_id\u0027: \u0027fake_id\u0027})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_de7b8478","line":6638,"range":{"start_line":6631,"start_character":0,"end_line":6638,"end_character":38},"updated":"2017-08-12 00:07:58.000000000","message":"If we\u0027re adding a specified instance_uuid field when creating the bdm records, shouldn\u0027t we assert that the instance_uuid field is what we expect it to be?\n\nAlso, with the updates to block_device_mapping_create and block_device_mapping_update_or_create, shouldn\u0027t we expect to only retrieve one bdm record here when calling block_device_mapping_get_all_by_volume_id?","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"69b1a151718e6aabc1f4b5e03504c975398981ef","unresolved":false,"context_lines":[{"line_number":6628,"context_line":"        self.assertEqual(len(bdms), 1)"},{"line_number":6629,"context_line":"        self.assertEqual(bdms[0][\u0027device_name\u0027], \u0027/dev/vda\u0027)"},{"line_number":6630,"context_line":""},{"line_number":6631,"context_line":"    def test_block_device_mapping_get_all_by_volume_id(self):"},{"line_number":6632,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id1\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6633,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id2\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6634,"context_line":"        bdms \u003d db.block_device_mapping_get_all_by_volume_id(self.ctxt,"},{"line_number":6635,"context_line":"                                                            \u0027fake_id\u0027)"},{"line_number":6636,"context_line":"        self.assertEqual(bdms[0][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6637,"context_line":"        self.assertEqual(bdms[1][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6638,"context_line":"        self.assertEqual(2, len(bdms))"},{"line_number":6639,"context_line":""},{"line_number":6640,"context_line":"    def test_block_device_mapping_get_all_by_volume_id_join_instance(self):"},{"line_number":6641,"context_line":"        self._create_bdm({\u0027volume_id\u0027: \u0027fake_id\u0027})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_91dfc10b","line":6638,"range":{"start_line":6631,"start_character":0,"end_line":6638,"end_character":38},"in_reply_to":"9f436f4f_7dc93024","updated":"2017-08-17 07:50:36.000000000","message":"Done","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"34f2fa7919826b6845c2bed43fd57b9a55024b89","unresolved":false,"context_lines":[{"line_number":6628,"context_line":"        self.assertEqual(len(bdms), 1)"},{"line_number":6629,"context_line":"        self.assertEqual(bdms[0][\u0027device_name\u0027], \u0027/dev/vda\u0027)"},{"line_number":6630,"context_line":""},{"line_number":6631,"context_line":"    def test_block_device_mapping_get_all_by_volume_id(self):"},{"line_number":6632,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id1\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6633,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id2\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6634,"context_line":"        bdms \u003d db.block_device_mapping_get_all_by_volume_id(self.ctxt,"},{"line_number":6635,"context_line":"                                                            \u0027fake_id\u0027)"},{"line_number":6636,"context_line":"        self.assertEqual(bdms[0][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6637,"context_line":"        self.assertEqual(bdms[1][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6638,"context_line":"        self.assertEqual(2, len(bdms))"},{"line_number":6639,"context_line":""},{"line_number":6640,"context_line":"    def test_block_device_mapping_get_all_by_volume_id_join_instance(self):"},{"line_number":6641,"context_line":"        self._create_bdm({\u0027volume_id\u0027: \u0027fake_id\u0027})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_7d65b01d","line":6638,"range":{"start_line":6631,"start_character":0,"end_line":6638,"end_character":38},"in_reply_to":"9f436f4f_7dc93024","updated":"2017-08-14 05:45:49.000000000","message":"P.S.\n\nI created a test case which expects that duplicated record will be removed.\nThat is a below test case:\n\ntest_block_device_mapping_update_or_create_duplicated_record","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"},{"author":{"_account_id":8988,"name":"Rikimaru Honjo","email":"honjo.rikimaru@ntt-tx.co.jp","username":"rikimaru.honjo","inactive":true},"change_message_id":"456e8677ae7024ce51117917bbf01100e16962e5","unresolved":false,"context_lines":[{"line_number":6628,"context_line":"        self.assertEqual(len(bdms), 1)"},{"line_number":6629,"context_line":"        self.assertEqual(bdms[0][\u0027device_name\u0027], \u0027/dev/vda\u0027)"},{"line_number":6630,"context_line":""},{"line_number":6631,"context_line":"    def test_block_device_mapping_get_all_by_volume_id(self):"},{"line_number":6632,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id1\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6633,"context_line":"        self._create_bdm({\u0027instance_uuid\u0027: \u0027fake_id2\u0027, \u0027volume_id\u0027: \u0027fake_id\u0027})"},{"line_number":6634,"context_line":"        bdms \u003d db.block_device_mapping_get_all_by_volume_id(self.ctxt,"},{"line_number":6635,"context_line":"                                                            \u0027fake_id\u0027)"},{"line_number":6636,"context_line":"        self.assertEqual(bdms[0][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6637,"context_line":"        self.assertEqual(bdms[1][\u0027volume_id\u0027], \u0027fake_id\u0027)"},{"line_number":6638,"context_line":"        self.assertEqual(2, len(bdms))"},{"line_number":6639,"context_line":""},{"line_number":6640,"context_line":"    def test_block_device_mapping_get_all_by_volume_id_join_instance(self):"},{"line_number":6641,"context_line":"        self._create_bdm({\u0027volume_id\u0027: \u0027fake_id\u0027})"}],"source_content_type":"text/x-python","patch_set":22,"id":"9f436f4f_7dc93024","line":6638,"range":{"start_line":6631,"start_character":0,"end_line":6638,"end_character":38},"in_reply_to":"9f436f4f_de7b8478","updated":"2017-08-14 05:41:39.000000000","message":"\u003e If we\u0027re adding a specified instance_uuid field when creating the bdm records, shouldn\u0027t we assert that the instance_uuid field is what we expect it to be?\n\nOK, I add such assertions.\n\n\u003e Also, with the updates to block_device_mapping_create and block_device_mapping_update_or_create, shouldn\u0027t we expect to only retrieve one bdm record here when calling block_device_mapping_get_all_by_volume_id?\n\nI don\u0027t add such tests.\nBecause block_device_mapping_get_all_by_volume_id will get multiple bdm records when we use volume-multi-attach.","commit_id":"9b80f2fe894cfebee01585b78aaa65f07602ff8a"}]}