Open Stack

On 12/2/2015 11:37 AM, Rosa, Andrea (HP Cloud Services) wrote:
> Hi
>
> thanks Sean for bringing this point, I have been working on the change and on the (abandoned) spec.
> I'll try here to summarize all the discussions we had and what we decided.
>
>> From: Sean Dague [mailto:sean at dague.net]
>> Sent: 02 December 2015 13:31
>> To: OpenStack Development Mailing List (not for usage questions)
>> Subject: [openstack-dev] [nova] what are the key errors with volume detach
>>
>> This patch to add a bunch of logic to nova-manage for forcing volume detach
>> raised a bunch of questions
>> https://review.openstack.org/#/c/184537/24/nova/cmd/manage.py,cm
>
> On this specific review there are some valid concerns that I am happy to address, but first we need to understand if we want this change.
> FWIW I think it is still a valid change, please see below.
>
>> In thinking about this for the last day, I think the real concern is that we have
>> so many safety checks on volume delete, that if we failed with a partially
>> setup volume, we have too many safety latches to tear it down again.
>>
>> Do we have some detailed bugs about how that happens? Is it possible to
>> just fix DELETE to work correctly even when we're in these odd states?
>
> In a simplified view of a detach volume we can say that the nova code does:
> 1 detach the volume from the instance
> 2 Inform cinder about the detach and call the terminate_connection on the cinder API.
> 3 delete the dbm recod in the nova DB

We actually:

1. terminate the connection in cinder:

https://github.com/openstack/nova/blob/c4ca1abb4a49bf0bce765acd3ce906bd117ce9b7/nova/compute/manager.py#L2312

2. detach the volume

https://github.com/openstack/nova/blob/c4ca1abb4a49bf0bce765acd3ce906bd117ce9b7/nova/compute/manager.py#L2315

3. delete the volume (if marked for delete_on_termination):

https://github.com/openstack/nova/blob/c4ca1abb4a49bf0bce765acd3ce906bd117ce9b7/nova/compute/manager.py#L2348

4. delete the bdm in the nova db:

https://github.com/openstack/nova/blob/c4ca1abb4a49bf0bce765acd3ce906bd117ce9b7/nova/compute/manager.py#L908

So if terminate_connection fails, we shouldn't get to detach. And if 
detach fails, we shouldn't get to delete.

>
> If 2 fails the volumes get stuck in a detaching status and any further attempt to delete or detach the volume will fail:
> "Delete for volume <volume_id> failed: Volume <volume_id> is still attached, detach volume first. (HTTP 400)"
>
> And if you try to detach:
> "EROR (BadRequest): Invalid input received: Invalid volume: Unable to detach volume. Volume status must be 'in-use' and attach_status must be 'attached' to detach. Currently: status: 'detaching', attach_status: 'attached.' (HTTP 400)"
>
> at the moment the only way to clean up the situation is to hack the nova DB for deleting the bdm record and do some hack on the cinder side as well.
> We wanted a way to clean up the situation avoiding the manual hack to the nova DB.

Can't cinder rollback state somehow if it's bogus or failed an 
operation? For example, if detach failed, shouldn't we not be in 
'detaching' state? This is like auto-reverting task_state on server 
instances when an operation fails so that we can reset or delete those 
servers if needed.

>
> Solution proposed #1
> Move the deletion of the bdm record so as it happens before calling cinder, I thought that was ok as from nova side we have done, no leaking bdm and the problem was just in the cinder side, but I was wrong.
> We have to call the terminate_connection otherwise the device may show back on the nova host, for example that is true for iSCSI volumes:
>   "if an iSCSI session from the compute host to the storage backend still exists (because other volumes are connected), then the volume you just removed will show back up on the next scsi bus rescan."
> The key point here is that Nova must call the terminate_connection because just Nova has the "connector info" to call the terminate connection method, so Cinder can't fix it.
>
> Solution proposed #2
> Then I thought, ok, so let's expose a new nova API called force delete volume which skips all the check and allow to detach a volume in "detaching" status, I thought it was ok but I was wrong (again).
> The main concern here is that we do not want to have the concept of "force delete", the user already asked for detaching the volume and the call should be idempotent and just work.
> So adding a new API was just adding a technical debt in the RESP API for a buggy/weak interaction between the Cinder API and Nova, or in other words we are adding a Nova API for fixing a bug in Cinder, which is very odd.
>
> Solution proposed #3
> Ok, so the solution is to fix the Cinder API and makes the interaction between Nova volume manager and that API robust.
> This time I was right (YAY) but as you can imagine this fix is not going to be an easy one and after talking with Cinder guys they clearly told me that thatt is going to be a massive change in the Cinder API and it is unlikely to land in the N(utella) or O(melette)  release.
>
> Solution proposed #4: The trade-off
> This solution is the solution I am proposing in the patch you mentioned, the idea is to have a temporary solution which allows us to give a handy tool for the operators to easily fix a problem which can occur quite often.
> The solution should be something with a low impact on the nova code and easy to remove when we will have the proper solution for the root cause.
> "quick", "useful", "dirty", "tool", "trade-off",  "db"....we call it nova-manage!
> The idea is to put a new method in the compute/api.py which skips all the checks on the volume status and go ahead with calling the detach_volume on the compute manager to detach the volume, call the terminate_connection and clean the bdm entry.

But I thought calling detach on the cinder API when the volume is 
already in detaching status would be a failure? Or is that only the 
check in the nova API?

> Nova-manage will have a command to call directly that new method.
>
> That is a recap that I hope can help to understand how we decided to use nova-manage instead of other solutions, it was a bit long but I tried to condensate the comments from 53 spec patches + 24 code change patches (and counting).
>
> PS: I added the cinder tag as they are interested in this change as well.
>
> Thanks
> --
> Andrea Rosa
>
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>

As Sean pointed out in another reply, I feel like what we're really 
missing here is some rollback code in the case that delete fails so we 
don't get in this stuck state and have to rely on deleting the BDMs 
manually in the database just to delete the instance.

We should rollback on delete fail 1 so that delete request 2 can pass 
the 'check attach' checks again.

-- 

Thanks,

Matt Riedemann