Race condition in quickly attaching / deleting volumes

Bug #1335889 reported by Joseph Lanoux
82
This bug affects 13 people
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
High
Michael Dovgal
OpenStack Compute (nova)
Expired
Undecided
Unassigned
tempest
Invalid
Undecided
Unassigned

Bug Description

It seems that there is a race condition in the stress volume_attach_delete test. It creates VMs and volumes, attaches volumes and deletes everything.

The test is waiting for volumes to be in 'in-use' state before deleting VMs. It seems that Nova/Cinder don't have time to register volumes as attached in their databases before VMs get deleted. Volumes are then left attached to deleted VMs and unable to be deleted.

description: updated
Revision history for this message
David Kranz (david-kranz) wrote :

I am not seeing how this is a bug in tempest. Tempest is deleting the vm only after nova reports that the volume is 'in-use' which seems fine. It would be nice if there was a backtrace, log, or something associated with this ticket. This might be a cinder issue but more likely nova and the test is making a nova call.

Changed in tempest:
status: New → Invalid
Revision history for this message
Sean Dague (sdague) wrote :

So I believe this is the same basic issue that exists between cinder and nova in the fact that it is assumed the event will happen within a reasonable amount of time, and if it doesn't... sadness.

Changed in nova:
status: New → Confirmed
importance: Undecided → High
summary: - Race condition in the stress volume_attach_delete test
+ Race condition in quickly attaching / deleting volumes
Changed in nova:
assignee: nobody → Trung Trinh (trung-t-trinh)
Changed in cinder:
status: New → Confirmed
importance: Undecided → High
Revision history for this message
Trung Trinh (trung-t-trinh) wrote :

Initial analysis of Nova code is as follows.

The operation of deleting VM doesn't check if the VM is being attached with volume or not.
The operation of attaching volume to VM can't receive any status response from Cinder .

There is no sync between Nova and Cinder under this scenario.
Nova has no mechanism of pending an operation. Indeed, it just raises some Exception if invalid state is detected.
Cinder has no mechanism of responding the operation status.

Therefore, it's nearly impossible to resolve this problem completely. So, we ought to live with this problem.

A work-around is that Cinder should allow to delete an orphaned volume (i.e. the volume attached to an already-deleted VM).

Any feedback/comment is always appreciated

Changed in cinder:
assignee: nobody → Trung Trinh (trung-t-trinh)
Revision history for this message
Trung Trinh (trung-t-trinh) wrote :

Proposed work-around of Cinder source code to allow deleting of volume attached to the already-deleted VM:

In the function cinder.volume.api.API.delete(), there exists an if-clause as follows:

        if volume['attach_status'] == "attached":
            MY_LOG.debug("%s - API::delete - Volume is still attached, "\
                         "need to detach first" % __name__)

            # Volume is still attached, need to detach first
            raise exception.VolumeAttached(volume_id=volume_id)

Based on the above if-clause, it's possible to add further code to check if volume['instance_uuid'] results in a non-existed instance, then the deleting operation is still passed through.
The question is how to check if some instance_uuid exists or not from Cinder side?

Revision history for this message
Trung Trinh (trung-t-trinh) wrote :

Based on debugging, it can be seen that Nova's deleting of a VM always triggers Cinder's detaching of the associated volume.

Revision history for this message
Trung Trinh (trung-t-trinh) wrote :

The work-around of allowing to delete volume attached to an already-deleted VM is successful as below:

xtrutri@ubuntu:~/Work/devstack-stable-juno$ cinder list
+--------------------------------------+--------+--------------+------+-------------+----------+--------------------------------------+
| ID | Status | Display Name | Size | Volume Type | Bootable | Attached to |
+--------------------------------------+--------+--------------+------+-------------+----------+--------------------------------------+
| 5fc4746e-bc31-4764-8877-cf32de9b4e22 | in-use | new_vol | 1 | lvmdriver-1 | false | 8e6033c7-1450-4526-95c1-84879a51af5e |
+--------------------------------------+--------+--------------+------+-------------+----------+--------------------------------------+

xtrutri@ubuntu:~/Work/devstack-stable-juno$ cinder delete 5fc4746e-bc31-4764-8877-cf32de9b4e22

We can see that now it's possible to delete the volume whose status is still "in-use"

Please be noted the Cinder's detaching of the associated volume, which is triggered by Nova's deleting of a VM, is temporarily disabled for having the scenario that the volume is attached to an already-deleted VM

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

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

Changed in cinder:
status: Confirmed → In Progress
Mike Perez (thingee)
Changed in cinder:
status: In Progress → Confirmed
assignee: Trung Trinh (trung-t-trinh) → nobody
Changed in nova:
assignee: Trung Trinh (trung-t-trinh) → nobody
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on cinder (master)

Change abandoned by Mike Perez (<email address hidden>) on branch: master
Review: https://review.openstack.org/142006
Reason: Over a month with no update.

tags: added: race-condition testing volumes
Revision history for this message
Matt Riedemann (mriedem) wrote :

There is a change here https://review.openstack.org/#/c/184537/ that adds a nova-manage command to force-delete a volume attachment from a server instance.

Revision history for this message
Walt Boring (walter-boring) wrote :

I think changing the cinder API to just allow a volume to be deleted, without a force flag, when it's 'in-use' is a dangerous and wrong to do. We'll get users who do it by mistake and then nuke their volumes attached to live VMs.

So, it seems the issue is related to the fact that in Nova's code we nuke the VM first and then try and delete the volumes.
https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2365-L2375

Shouldn't nova ensure that it detaches the volume first before deleting it?
https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2307-L2315

Or check the exception that comes back from the volume_api.delete call and see if it complains because it's attached, then call terminate_connection, then delete.

Revision history for this message
vishal yadav (vishalcdac07) wrote :

>> So, it seems the issue is related to the fact that in Nova's code we nuke the VM first and then try and delete the volumes.
>> Shouldn't nova ensure that it detaches the volume first before deleting it?

I think above behavior likely to be changed in nova considering volume muti-attach feature. So nova should detach the volume while nuking the VM and should only delete the volume if it is not attached to other live VM/s.

>> Or check the exception that comes back from the volume_api.delete call and see if it complains because it's attached, then call terminate_connection, then delete.
This lazy verification might be used to avoid some race-conditions but can have performance penalty for VM terminate operation.

Changed in cinder:
assignee: nobody → Gaurav Singla (gsingla294)
Revision history for this message
Gourav Singla (gsingla294) wrote :

I opine that we should detach the volume first in nova code and then delete the VM. if we get an error while detaching volume then don't delete the VM and raise the exception.

We can implement a verification check at the end of API whether the volume is detached successfully or not, but it will have some performance penalty.

Any thoughts!

Prateek Arora (parora)
Changed in cinder:
assignee: Gourav Singla (gsingla294) → Prateek Arora (parora)
assignee: Prateek Arora (parora) → nobody
Changed in nova:
assignee: nobody → Prateek Arora (parora)
Revision history for this message
Prateek Arora (parora) wrote :

One thing we can do here is have a periodic task in nova or cinder and find all the volumes that are attached to instances that do not exist and delete those volumes. Opinions are welcome on this solution.

Prateek Arora (parora)
Changed in nova:
assignee: Prateek Arora (parora) → nobody
zhaolihui (zhaolh)
Changed in nova:
assignee: nobody → zhaolihui (zhaolh)
Revision history for this message
Sujitha (sujitha-neti) wrote :

@zhaolihui Please change the status to InProgress if you are currently working on it.

zhaolihui (zhaolh)
Changed in nova:
assignee: zhaolihui (zhaolh) → nobody
Revision history for this message
Markus Zoeller (markus_z) (mzoeller) wrote : Cleanup EOL bug report

This is an automated cleanup. This bug report has been closed because it
is older than 18 months and there is no open code change to fix this.
After this time it is unlikely that the circumstances which lead to
the observed issue can be reproduced.

If you can reproduce the bug, please:
* reopen the bug report (set to status "New")
* AND add the detailed steps to reproduce the issue (if applicable)
* AND leave a comment "CONFIRMED FOR: <RELEASE_NAME>"
  Only still supported release names are valid (LIBERTY, MITAKA, OCATA, NEWTON).
  Valid example: CONFIRMED FOR: LIBERTY

Changed in nova:
importance: High → Undecided
status: Confirmed → Expired
Revision history for this message
Sean McGinnis (sean-mcginnis) wrote :

I think things have changed enough that this particular failure isn't an issue anymore. Can anyone confirm?

Michael Dovgal (mdovgal)
Changed in cinder:
assignee: nobody → Michael Dovgal (mdovgal)
Revision history for this message
Michael Dovgal (mdovgal) wrote :

This bug can be reproduced by cinder tempest tests on which i'm working now.

Also the way to reproduce it described in this bug [0] that i marked as duplicated

https://bugs.launchpad.net/cinder/+bug/1650277

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

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

Changed in cinder:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/413141
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=d0abb60fe7798c6fea4bfb9247db25cf15591340
Submitter: Jenkins
Branch: master

commit d0abb60fe7798c6fea4bfb9247db25cf15591340
Author: Michael Dovgal <email address hidden>
Date: Tue Dec 20 15:16:03 2016 +0200

    Attach/Delete volume for tgt driver race condition fix

    We have race condition when remove target operation still
    isn't completed and we try to run create target operation.
    This patch fixed this race condition by retrying create
    iscsi target operation run.

    Change-Id: I151990cc5148eb3f56e27e01fce71d87da2d9759
    Closes-Bug: #1335889

Changed in cinder:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to cinder (master)

Reviewed: https://review.openstack.org/411277
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=4f5adf435eaf1f6f8b5ff8195a8f371afe3fbf21
Submitter: Jenkins
Branch: master

commit 4f5adf435eaf1f6f8b5ff8195a8f371afe3fbf21
Author: Michael Dovgal <email address hidden>
Date: Thu Dec 8 17:05:29 2016 +0200

    Add volume backup tempest tests

    Tempest tests that are added:
    1) test volume snapshot backup
    2) test backup create and restore to an existing volume
    This test can reproduce the bug that is related.
    3) test incremental backup

    Co-Authored-By: Oleksii Butenko <email address hidden>
    Related-Bug: #1335889

    Change-Id: Iff857ee11ed15665315077a0ba22cf31f92efe4b

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder 10.0.0.0b3

This issue was fixed in the openstack/cinder 10.0.0.0b3 development milestone.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Related blueprints

Remote bug watches

Bug watches keep track of this bug in other bug trackers.