rebuild_instance doesn't detach cinder volumes correctly

[Walt Boring]

The compute manager's rebuild_instance doesn't detach a volume correctly prior to trying to stand up the vm again.

https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2873

rebuild_instance() defines detach_block_devices to call cinder's detach_volume() API.

This doesn't actually detach anything. Cinder's detach API just updates the record of the volume in the Cinder DB.

https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L748

The cinder volume manager calls the cinder driver's detach_volume method, which doesn't actually detach anything.

The proper way to detach a volume is to call terminate_connection and then detach_volume.

Nova compute manager detach_volume does this correctly

https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4725

I have included a stack dump of what happens due to this.

[Loganathan Parthipan (parthipan)]

IMHO, we may have got the context wrong here. You only need to remove the volume from libvirt and not do anything beyond it. Calling a full detach preceded by volume_termination will make the volumes available for other instances and this is not what we want.

I think the original error in the rebuild is calling volume_api.detach() instead of just self._detach_volume(). What do you all say?

[Walt Boring (walter-boring)]

If you are going to call libvirt's volume driver to remove the volume, it will eventually call the libvirt volume driver to remove the volume from the host.

Why not just remove the volume from virsh, then re-add it to the vm once it's back up, instead of removing the volume attached to the host?

[Loganathan Parthipan (parthipan)]

That's exactly what I meant. Calling self._detach_volume does just that.

[Loganathan Parthipan (parthipan)]

Of course, if it's a bad practice to use private methods, then we could call remove_volume_connection(), which would achieve the same thing though it would remove it from host.

Thinking more on this, I'd prefer remove_volume_connection() since, evacuate calls rebuild internally (heaven knows why) and we might indeed need to remove the initiators.

[Scott DAngelo (scott-dangelo)]

 I looked at changing detach_block_devices to use self._detach or self._remove_volume_connection, but neither would work
because during the rebuild Nova checks the Cinder state during the attach of the volumes to the rebuilt instance. If Cinder is
not called to fully detach, the state in Cinder will still be 'attached' and 'in-use', so the re-attach will fail.

[Scott DAngelo (scott-dangelo)]

I attempted to change detach_block_devices to use self.detach(...) but that resulted in this Traceback:
https://etherpad.openstack.org/p/novaRebuildUseDetach

I'm still trying to debug that one, since I would think that self.Detach should work in this case.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Change abandoned by dave-mcnally (<email address hidden>) on branch: master
Review: https://review.openstack.org/172951
Reason: This is fixed by a change that has now merged:
https://review.openstack.org/#/c/170907/