Unnecessary bdm entry is created if same volume is attached twice to an instance

If we take a look at following code, line 3023,3024 can be moved to previous line before reserve bdm
because it's only a status check , I will try to submit a patch and see whether they are some traps of
concurrency issue there

3007 def _attach_volume(self, context, instance, volume_id, device,
3008 disk_bus, device_type):
3009 """Attach an existing volume to an existing instance.
3010
3011 This method is separated to make it possible for cells version
3012 to override it.
3013 """
3014 # NOTE(vish): This is done on the compute host because we want
3015 # to avoid a race where two devices are requested at
3016 # the same time. When db access is removed from
3017 # compute, the bdm will be created here and we will
3018 # have to make sure that they are assigned atomically.
3019 volume_bdm = self.compute_rpcapi.reserve_block_device_name(
3020 context, instance, device, volume_id, disk_bus=disk_bus,
3021 device_type=device_type)
3022 try:
3023 volume = self.volume_api.get(context, volume_id)
3024 self.volume_api.check_attach(context, volume, instance=instance)
3025 self.volume_api.reserve_volume(context, volume_id)
3026 self.compute_rpcapi.attach_volume(context, instance=instance,
3027 volume_id=volume_id, mountpoint=device, bdm=volume_bdm)
3028 except Exception:
3029 with excutils.save_and_reraise_exception():
3030 volume_bdm.destroy()
3031

Fix proposed to branch: master
Review: https://review.openstack.org/163177

Change abandoned by jichenjc (<email address hidden>) on branch: master
Review: https://review.openstack.org/163177
Reason: seems an old bug exists and no better way now..

So this behavior can have pretty bad consequences, if we fail to delete the created BDM.

If in normal operation once the BDM record is created reserving the device name on the compute host, we will attempt to mark it as 'attaching' in Cinder. This fails if the volume is already attached, and as part of the cleanup we delete the BDM record we created.

If something goes wrong with the reply message however (there is a problem with the network, or for some reason the API worker that accepted the attach request goes away in the meantime are some of the problems I can think of), the cleanup code never runs and we are stuck with an invalid BDM, which we have to clean up manually. This is not ideal obviously but is made worse, by the fact that when looking for the BDM record, we commonly select records only by instance and volume IDs which will not be unique in this case, and Nova may attempt to use the wrong one by chance which causes things like detach, migrate etc. to fail until manual cleanup

Fix proposed to branch: master
Review: https://review.openstack.org/290793

Per comment 4, we had some patches from Dan Smith at one point to create a uuid column in the block_device_mappings table and then use that in the objects, so we could avoid issues with bdm_update_or_create in the DB API. Those never landed because they were written for a latent boot from volume race with cells v1 and we decided to just not fix cells v1, but could still be useful for situations like this.

This is Dan's BDM object change to use a uuid:

https://review.openstack.org/#/c/242603/

https://review.openstack.org/#/c/290793 is updated according to changes in volume API (reserve call is added).
Order of creating BDM and volume reserving is reversed due guess that remote HTTP call can fail more frequently than DB request.

Hi Lee Yarwood, any updates on this bug? If not, I would like to work on it.

I'm going to revisit the following stack of patches in Pike and hopefully close this out finally :

https://review.openstack.org/#/q/topic:bug/1427060+status:open

Please feel free to review and rebase these if you have time but I'd like to retain ownership of the bug as a reminder to revisit this when the next cycle opens up.

Related fix proposed to branch: master
Review: https://review.openstack.org/453451

Change abandoned by Zhang Hua (<email address hidden>) on branch: master
Review: https://review.openstack.org/453451
Reason: sorry, this patch can't stop creating new dirty BDM record when happen to fail to run 'volume_bdm.destroy()' - https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3715, so abandon it

Change abandoned by Dan Smith (<email address hidden>) on branch: master
Review: https://review.openstack.org/290793
Reason: So I think Lee's comment means this should be abandoned. Probably makes sense so current owners can "own" the resulting patch. Let me know if this isn't right and I can un-abandon this.

Hi,

Is anybody working on it? Can I reassign it to me?

Thanks

I request the bug reporter to close the bug as this bug is already fixed in latest(Rock). Here is my analysis.

nova volume-attach d709f9d8-e9ea-4c07-a83c-350689d27919 54bc2524-a0c5-4908-9ea2-726708667702
+----------+--------------------------------------+
| Property | Value |
+----------+--------------------------------------+
| device | /dev/vdb |
| id | 54bc2524-a0c5-4908-9ea2-726708667702 |
| serverId | d709f9d8-e9ea-4c07-a83c-350689d27919 |
| volumeId | 54bc2524-a0c5-4908-9ea2-726708667702 |
+----------+--------------------------------------+

 select * from block_device_mapping where instance_uuid='d709f9d8-e9ea-4c07-a83c-350689d27919';
+---------------------+---------------------+------------+----+-------------+-----------------------+-------------+--------------------------------------+-------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------+---------+-------------+------------------+--------------+-------------+----------+------------+----------+------+--------------------------------------+--------------------------------------+
| created_at | updated_at | deleted_at | id | device_name | delete_on_termination | snapshot_id | volume_id | volume_size | no_device | connection_info | instance_uuid | deleted | source_type | destination_type | guest_format | device_type | disk_bus | boot_index | image_id | tag | attachment_id | uuid |
+---------------------+---------------------+------------+----+-------------+-----------------------+-------------+--------------------------------------+-------------+-----------+------------------------------------------------------------------------------------------------------...

Repeat this process, there is no such problem in the current branch, invalid bug.

[root@localhost ~]# cinder list
+--------------------------------------+--------+------+------+-------------+----------+--------------------------------------+
| ID | Status | Name | Size | Volume Type | Bootable | Attached to |
+--------------------------------------+--------+------+------+-------------+----------+--------------------------------------+
| df191675-0ac1-4101-a590-7ad2f7607e89 | in-use | | 1 | lvmdriver-1 | true | ec939a46-9e1e-4126-8214-6ceb3b95676a |
+--------------------------------------+--------+------+------+-------------+----------+--------------------------------------+

[root@localhost ~]# nova volume-attach ec939a46-9e1e-4126-8214-6ceb3b95676a df191675-0ac1-4101-a590-7ad2f7607e89
ERROR (BadRequest): Invalid volume: volume df191675-0ac1-4101-a590-7ad2f7607e89 already attached (HTTP 400) (Request-ID: req-cb0d0d98-8035-4a9e-b055-ef515235e8aa)

MariaDB [nova_cell1]> select volume_id,volume_size,device_name,attachment_id,deleted_at from block_device_mapping where instance_uuid='ec939a46-9e1e-4126-8214-6ceb3b95676a';
+--------------------------------------+-------------+-------------+---------------+------------+
| volume_id | volume_size | device_name | attachment_id | deleted_at |
+--------------------------------------+-------------+-------------+---------------+------------+
| df191675-0ac1-4101-a590-7ad2f7607e89 | 1 | /dev/vda | NULL | NULL |
+--------------------------------------+-------------+-------------+---------------+------------+
1 row in set (0.00 sec)

Did the Rocky version solve the problem of the drive letter display?

Related fix proposed to branch: master
Review: https://review.opendev.org/692940

Change abandoned by Matt Riedemann (<email address hidden>) on branch: master
Review: https://review.opendev.org/692940